Re: [PATCH v14 02/26] target/loongarch: Add core definition

[gaosong]

Hi,

On 2022/1/10 上午2:49, Richard Henderson wrote:

> > > +static bool
> > >           loongarch_cpu_has_work(CPUState *cs)
> > >           
> > >           +{
> > >           
> > >           +    return true;
> > >           
> > >         
> >         
> >         Note: this is only applicable to CONFIG_USER_ONLY, and needs to
> >         be changed in the following commits adding system emulation. To
> >         better convey your intention it may be better to use an #ifdef
> >         guard, something like this:
> >         
> >         
> >         #ifndef CONFIG_USER_ONLY
> >         
> >         #error System emulation TODO
> >         
> >         #else
> >         
> >              return true;
> >         
> >         #endif
> >         
> >         
> >         (I'm not sure if this is okay in QEMU coding style, so please
> >         correct me if this isn't the case.)
> >         
> >       
>       
>     
In my opinion, we don't need to do this. As you pointed out below, SPW shouldn't appear in this series. All CONFIG_USER_ONLY  macors should appear in the system emulation series.
Liks this:
20220108091419.2027710-1-yangxiaojuan@loongson.cn/20220108091419.2027710-12-yangxiaojuan@loongson.cn/">https://patchew.org/QEMU/20220108091419.2027710-1-yangxiaojuan@loongson.cn/20220108091419.2027710-12-yangxiaojuan@loongson.cn/
 static bool loongarch_cpu_has_work(CPUState *cs)
 {
+#ifdef CONFIG_USER_ONLY
     return true;
+#else
+    LoongArchCPU *cpu = LOONGARCH_CPU(cs);
+    CPULoongArchState *env = &cpu->env;
+    bool has_work = false;
+
+    if ((cs->interrupt_request & CPU_INTERRUPT_HARD) &&
+        cpu_loongarch_hw_interrupts_pending(env)) {
+        has_work = true;
+    }
+
+    return has_work;
+#endif
 }
> Prefer
>       positive tests over negative tests, so
>       
>       
>       #ifdef CONFIG_USER_ONLY
>       
>           return true;
>       
>       #else
>       
>       #error
>       
>       #endif
>       
>       
>     
> > > +    data =
> > >           "" CPUCFG2, LSPW, 1);
> > >           
> > >         
> >         Do you support the SPW extension in this series? If not you
> >         probably don't want to set this bit.
> >         
> >       
>       
>       Correct, you can't expose features that you don't implement. 
>     
Accept this suggesstions.
Thanks
Song