| Message ID | 20170912100330.2168-18-julien.grall@arm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm: Memory subsystem clean-up | expand |
On Tue, 12 Sep 2017, Julien Grall wrote: > Currently MAIRVAL is defined in term of MAIR0VAL and MAIR1VAL which are > both hardcoded value. This makes quite difficult to understand the value > written in both registers. > > Rework the definition by using value of each attribute shifted by their > associated index. > > Signed-off-by: Julien Grall <julien.grall@arm.com> Ah! That's why you haven't properly updated MAIR0VAL and MAIR1VAL in the previous patches. In that case, please say explicitly in the commit messages of those patches that MAIR0VAL and MAIR1VAL will be properly update in a follow-up patch. > --- > Changes in v2: > - Move this patch after "xen/arm: page: Use ARMv8 naming to > improve readability" > --- > xen/include/asm-arm/page.h | 42 +++++++++++++++++++++++++----------------- > 1 file changed, 25 insertions(+), 17 deletions(-) > > diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h > index 899fd1801a..088746828d 100644 > --- a/xen/include/asm-arm/page.h > +++ b/xen/include/asm-arm/page.h > @@ -22,6 +22,21 @@ > #define LPAE_SH_INNER 0x3 > > /* > + * Attribute Indexes. > + * > + * These are valid in the AttrIndx[2:0] field of an LPAE stage 1 page > + * table entry. They are indexes into the bytes of the MAIR* > + * registers, as defined above. "as defined above" should be "has defined below" now. Aside from this: Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > + * > + */ > +#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 > + > +/* > * LPAE Memory region attributes. Indexed by the AttrIndex bits of a > * LPAE entry; the 8-bit fields are packed little-endian into MAIR0 and MAIR1. > * > @@ -35,24 +50,17 @@ > * reserved 110 > * MT_NORMAL 111 1111 1111 -- Write-back write-allocate > */ > -#define MAIR0VAL 0xeeaa4400 > -#define MAIR1VAL 0xff000004 > -#define MAIRVAL (MAIR0VAL|MAIR1VAL<<32) > +#define MAIR(attr, mt) (_AC(attr, ULL) << ((mt) * 8)) > > -/* > - * Attribute Indexes. > - * > - * These are valid in the AttrIndx[2:0] field of an LPAE stage 1 page > - * table entry. They are indexes into the bytes of the MAIR* > - * registers, as defined above. > - * > - */ > -#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 MAIRVAL (MAIR(0x00, MT_DEVICE_nGnRnE)| \ > + MAIR(0x44, MT_NORMAL_NC) | \ > + MAIR(0xaa, MT_NORMAL_WT) | \ > + MAIR(0xee, MT_NORMAL_WB) | \ > + MAIR(0x04, MT_DEVICE_nGnRE) | \ > + MAIR(0xff, MT_NORMAL)) > + > +#define MAIR0VAL (MAIRVAL & 0xffffffff) > +#define MAIR1VAL (MAIRVAL >> 32) > > #define PAGE_HYPERVISOR (MT_NORMAL) > #define PAGE_HYPERVISOR_NOCACHE (MT_DEVICE_nGnRE) > -- > 2.11.0 >
Hi, On 20/09/17 00:51, Stefano Stabellini wrote: > On Tue, 12 Sep 2017, Julien Grall wrote: >> Currently MAIRVAL is defined in term of MAIR0VAL and MAIR1VAL which are >> both hardcoded value. This makes quite difficult to understand the value >> written in both registers. >> >> Rework the definition by using value of each attribute shifted by their >> associated index. >> >> Signed-off-by: Julien Grall <julien.grall@arm.com> > > Ah! That's why you haven't properly updated MAIR0VAL and MAIR1VAL in the > previous patches. In that case, please say explicitly in the commit > messages of those patches that MAIR0VAL and MAIR1VAL will be properly > update in a follow-up patch. I didn't need to update MAIR0VAL and MAIR1VAL because the remove attributes were just alias. Not because I was modifying the 2 defines here. Cheers,
On Wed, 20 Sep 2017, Julien Grall wrote: > Hi, > > On 20/09/17 00:51, Stefano Stabellini wrote: > > On Tue, 12 Sep 2017, Julien Grall wrote: > > > Currently MAIRVAL is defined in term of MAIR0VAL and MAIR1VAL which are > > > both hardcoded value. This makes quite difficult to understand the value > > > written in both registers. > > > > > > Rework the definition by using value of each attribute shifted by their > > > associated index. > > > > > > Signed-off-by: Julien Grall <julien.grall@arm.com> > > > > Ah! That's why you haven't properly updated MAIR0VAL and MAIR1VAL in the > > previous patches. In that case, please say explicitly in the commit > > messages of those patches that MAIR0VAL and MAIR1VAL will be properly > > update in a follow-up patch. > > I didn't need to update MAIR0VAL and MAIR1VAL because the remove attributes > were just alias. Not because I was modifying the 2 defines here. I thought their AIs were unique, but no, even the AIs are aliases. You are right. In that case, I'll add my reviewed-by to them.
Hi Stefano, On 20/09/2017 22:03, Stefano Stabellini wrote: > On Wed, 20 Sep 2017, Julien Grall wrote: >> Hi, >> >> On 20/09/17 00:51, Stefano Stabellini wrote: >>> On Tue, 12 Sep 2017, Julien Grall wrote: >>>> Currently MAIRVAL is defined in term of MAIR0VAL and MAIR1VAL which are >>>> both hardcoded value. This makes quite difficult to understand the value >>>> written in both registers. >>>> >>>> Rework the definition by using value of each attribute shifted by their >>>> associated index. >>>> >>>> Signed-off-by: Julien Grall <julien.grall@arm.com> >>> >>> Ah! That's why you haven't properly updated MAIR0VAL and MAIR1VAL in the >>> previous patches. In that case, please say explicitly in the commit >>> messages of those patches that MAIR0VAL and MAIR1VAL will be properly >>> update in a follow-up patch. >> >> I didn't need to update MAIR0VAL and MAIR1VAL because the remove attributes >> were just alias. Not because I was modifying the 2 defines here. > > I thought their AIs were unique, but no, even the AIs are aliases. You > are right. In that case, I'll add my reviewed-by to them. I guess it because it is copied from ARMv7 Linux which also use aliases. But I think they are more confusing than really helpful. Thank you for the reviewed-by. Cheers,
diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h index 899fd1801a..088746828d 100644 --- a/xen/include/asm-arm/page.h +++ b/xen/include/asm-arm/page.h @@ -22,6 +22,21 @@ #define LPAE_SH_INNER 0x3 /* + * Attribute Indexes. + * + * These are valid in the AttrIndx[2:0] field of an LPAE stage 1 page + * table entry. They are indexes into the bytes of the MAIR* + * registers, as defined above. + * + */ +#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 + +/* * LPAE Memory region attributes. Indexed by the AttrIndex bits of a * LPAE entry; the 8-bit fields are packed little-endian into MAIR0 and MAIR1. * @@ -35,24 +50,17 @@ * reserved 110 * MT_NORMAL 111 1111 1111 -- Write-back write-allocate */ -#define MAIR0VAL 0xeeaa4400 -#define MAIR1VAL 0xff000004 -#define MAIRVAL (MAIR0VAL|MAIR1VAL<<32) +#define MAIR(attr, mt) (_AC(attr, ULL) << ((mt) * 8)) -/* - * Attribute Indexes. - * - * These are valid in the AttrIndx[2:0] field of an LPAE stage 1 page - * table entry. They are indexes into the bytes of the MAIR* - * registers, as defined above. - * - */ -#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 MAIRVAL (MAIR(0x00, MT_DEVICE_nGnRnE)| \ + MAIR(0x44, MT_NORMAL_NC) | \ + MAIR(0xaa, MT_NORMAL_WT) | \ + MAIR(0xee, MT_NORMAL_WB) | \ + MAIR(0x04, MT_DEVICE_nGnRE) | \ + MAIR(0xff, MT_NORMAL)) + +#define MAIR0VAL (MAIRVAL & 0xffffffff) +#define MAIR1VAL (MAIRVAL >> 32) #define PAGE_HYPERVISOR (MT_NORMAL) #define PAGE_HYPERVISOR_NOCACHE (MT_DEVICE_nGnRE)
Currently MAIRVAL is defined in term of MAIR0VAL and MAIR1VAL which are both hardcoded value. This makes quite difficult to understand the value written in both registers. Rework the definition by using value of each attribute shifted by their associated index. Signed-off-by: Julien Grall <julien.grall@arm.com> --- Changes in v2: - Move this patch after "xen/arm: page: Use ARMv8 naming to improve readability" --- xen/include/asm-arm/page.h | 42 +++++++++++++++++++++++++----------------- 1 file changed, 25 insertions(+), 17 deletions(-)