Re: [Xen-devel] [PATCH v2 16/24] xen/arm: page: Use ARMv8 naming to improve readability

[Julien Grall <julien.grall@xxxxxxx>]

Hi,

On 20/09/17 00:45, Stefano Stabellini wrote:
> > diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
> > index 30fcfa0778..899fd1801a 100644
> > --- a/xen/include/asm-arm/page.h
> > +++ b/xen/include/asm-arm/page.h
> > @@ -26,14 +26,14 @@
> >    * LPAE entry; the 8-bit fields are packed little-endian into MAIR0 and MAIR1.
> >    *
> >    *                    ai    encoding
> > - *   MT_UNCACHED      000   0000 0000  -- Strongly Ordered
> > - *   MT_BUFFERABLE    001   0100 0100  -- Non-Cacheable
> > - *   MT_WRITETHROUGH  010   1010 1010  -- Write-through
> > - *   MT_WRITEBACK     011   1110 1110  -- Write-back
> > - *   MT_DEV_SHARED    100   0000 0100  -- Device
> > + *   MT_DEVICE_nGnRE  000   0000 0000  -- Strongly Ordered/Device nGnRnE
> I admit I always hated the "nGnRE" acronym. However, it is on the ARM
> ARM too, so if you'd like to introduce it here, I'll accept it. But
> please at least expand the acronym in the comment to make it
> understandable (same with nGnRnE).
"nGnRE" acronym are not great but convey the meaning of what would be 
the resulting attribute. For instance MT_UNCACHED does not really say if 
it is for device or memory. Lets not even mention MT_BUFFERABLE which is 
in fact non-cacheable memory :).
> Also, the comment say "nGnRnE" while the definition is MT_DEVICE_nGnRE.
Actually, the comment is correct but not the naming. It should 
MT_DEVICE_nGnRnE. I will rename it.
Aside that, I think the comment is understandable. nGnRnE is equivalent 
to Strongly ordered. I could expand nGnRnE (non-Gatherable, 
non-Reordering, No Early write acknowledgment) but I feel at this stage 
you can just search the name in the ARM ARM...
> > + *   MT_NORMAL_NC     001   0100 0100  -- Non-Cacheable
> > + *   MT_NORMAL_WT     010   1010 1010  -- Write-through
> > + *   MT_NORMAL_WB     011   1110 1110  -- Write-back
> > + *   MT_DEVICE_nGnRE  100   0000 0100  -- Device nGnRE
> >    *   ??               101
> >    *   reserved         110
> > - *   MT_WRITEALLOC    111   1111 1111  -- Write-back write-allocate
> > + *   MT_NORMAL        111   1111 1111  -- Write-back write-allocate
> >    */
> >   #define MAIR0VAL 0xeeaa4400
> >   #define MAIR1VAL 0xff000004
> > @@ -47,16 +47,16 @@
> >    * registers, as defined above.
> >    *
> >    */
> > -#define MT_UNCACHED      0x0
> > -#define MT_BUFFERABLE    0x1
> > -#define MT_WRITETHROUGH  0x2
> > -#define MT_WRITEBACK     0x3
> > -#define MT_DEV_SHARED    0x4
> > -#define MT_WRITEALLOC    0x7
> > -
> > -#define PAGE_HYPERVISOR         (MT_WRITEALLOC)
> > -#define PAGE_HYPERVISOR_NOCACHE (MT_DEV_SHARED)
> > -#define PAGE_HYPERVISOR_WC      (MT_BUFFERABLE)
> > +#define MT_DEVICE_nGnRnE 0x0
> > +#define MT_NORMAL_NC     0x1
> > +#define MT_NORMAL_WT     0x2
> > +#define MT_NORMAL_WB     0x3
> > +#define MT_DEVICE_nGnRE  0x4
> > +#define MT_NORMAL        0x7
> > +
> > +#define PAGE_HYPERVISOR         (MT_NORMAL)
> > +#define PAGE_HYPERVISOR_NOCACHE (MT_DEVICE_nGnRE)
> > +#define PAGE_HYPERVISOR_WC      (MT_NORMAL_NC)
> >   
> >   /*
> >    * Defines for changing the hypervisor PTE .ro and .nx bits. This is only to 
> > be
Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel