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

[Richard Henderson]

On 1/9/22 1:25 AM, WANG Xuerui wrote:
> > +#define TARGET_PAGE_BITS 14
> Aren't we capable of page sizes up to 64KiB? Minimal feasible page size is indeed 16KiB 
> though (due to cache aliasing, although 4KiB pages are supported in hardware, they don't 
> work in practice).

You must use the minimal page size here.  64k pages would be handled solely within 
tlb_fill, as multiples of the minimal page size.

> > +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.)

Prefer positive tests over negative tests, so

#ifdef CONFIG_USER_ONLY
    return true;
#else
#error
#endif

> > +    data = FIELD_DP32(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.

> > +    uint64_t lladdr; /* LL virtual address compared against SC */
> Similarly, do we explain every field with comments *here*? I think if fields are named 
> according to the manuals, people will naturally look up names there so there's no worry 
> for misunderstanding.
> > +    uint64_t llval;

These two fields are not architectural, so they do require explanation.  Not that there 
aren't other targets that lack this documentation...


r~