| Message ID | 1402504804-29173-6-git-send-email-ian.campbell@citrix.com |
|---|---|
| State | New |
| Headers | show |
Hi Ian, On 11/06/14 17:40, Ian Campbell wrote: > Not terribly helpful right now, since they aren't widely used, but makes future > patches easier to read. > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com> > --- > v2: clarify common on p2m_{table,entry} > --- > xen/arch/arm/p2m.c | 21 ++++++++++++++------- > 1 file changed, 14 insertions(+), 7 deletions(-) > > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c > index 8a6d295..2a93ff9 100644 > --- a/xen/arch/arm/p2m.c > +++ b/xen/arch/arm/p2m.c > @@ -14,6 +14,13 @@ > #define P2M_FIRST_ORDER 1 > #define P2M_FIRST_ENTRIES (LPAE_ENTRIES<<P2M_FIRST_ORDER) > > +#define p2m_valid(pte) ((pte).p2m.valid) > +/* These two can only be used on L0..L2 ptes because L3 mappings set > + * the table bit and therefore these would return the opposite to what > + * you would expect. */ > +#define p2m_table(pte) (p2m_valid(pte) && (pte).p2m.table) > +#define p2m_entry(pte) (p2m_valid(pte) && !(pte).p2m.table) Sorry, I didn't spot it on the previous version. You are using twice pte here. Depending on how complex pte we may duplicate the operation (masking the address + dereference the table). I'm wondering if we need a temporary variable in both p2m_table and p2m_entry. It seems that in your patch #7, you always use these 2 macros with non-complex variable. So I'm fine with one or the other way: Acked-by: Julien Grall <julien.grall@linaro.org> Regards,
On Wed, 2014-06-11 at 22:39 +0100, Julien Grall wrote: > Hi Ian, > > On 11/06/14 17:40, Ian Campbell wrote: > > Not terribly helpful right now, since they aren't widely used, but makes future > > patches easier to read. > > > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com> > > --- > > v2: clarify common on p2m_{table,entry} > > --- > > xen/arch/arm/p2m.c | 21 ++++++++++++++------- > > 1 file changed, 14 insertions(+), 7 deletions(-) > > > > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c > > index 8a6d295..2a93ff9 100644 > > --- a/xen/arch/arm/p2m.c > > +++ b/xen/arch/arm/p2m.c > > @@ -14,6 +14,13 @@ > > #define P2M_FIRST_ORDER 1 > > #define P2M_FIRST_ENTRIES (LPAE_ENTRIES<<P2M_FIRST_ORDER) > > > > +#define p2m_valid(pte) ((pte).p2m.valid) > > +/* These two can only be used on L0..L2 ptes because L3 mappings set > > + * the table bit and therefore these would return the opposite to what > > + * you would expect. */ > > +#define p2m_table(pte) (p2m_valid(pte) && (pte).p2m.table) > > +#define p2m_entry(pte) (p2m_valid(pte) && !(pte).p2m.table) > > Sorry, I didn't spot it on the previous version. You are using twice pte > here. Depending on how complex pte we may duplicate the operation > (masking the address + dereference the table). I'm wondering if we need > a temporary variable in both p2m_table and p2m_entry. A static function would be preferable to that I think. > It seems that in your patch #7, you always use these 2 macros with > non-complex variable. Yes, this was deliberate. > So I'm fine with one or the other way: > > Acked-by: Julien Grall <julien.grall@linaro.org> > > Regards, >
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c index 8a6d295..2a93ff9 100644 --- a/xen/arch/arm/p2m.c +++ b/xen/arch/arm/p2m.c @@ -14,6 +14,13 @@ #define P2M_FIRST_ORDER 1 #define P2M_FIRST_ENTRIES (LPAE_ENTRIES<<P2M_FIRST_ORDER) +#define p2m_valid(pte) ((pte).p2m.valid) +/* These two can only be used on L0..L2 ptes because L3 mappings set + * the table bit and therefore these would return the opposite to what + * you would expect. */ +#define p2m_table(pte) (p2m_valid(pte) && (pte).p2m.table) +#define p2m_entry(pte) (p2m_valid(pte) && !(pte).p2m.table) + void dump_p2m_lookup(struct domain *d, paddr_t addr) { struct p2m_domain *p2m = &d->arch.p2m; @@ -139,13 +146,13 @@ paddr_t p2m_lookup(struct domain *d, paddr_t paddr, p2m_type_t *t) mask = FIRST_MASK; pte = first[first_table_offset(paddr)]; - if ( !pte.p2m.valid || !pte.p2m.table ) + if ( !p2m_table(pte) ) goto done; mask = SECOND_MASK; second = map_domain_page(pte.p2m.base); pte = second[second_table_offset(paddr)]; - if ( !pte.p2m.valid || !pte.p2m.table ) + if ( !p2m_table(pte) ) goto done; mask = THIRD_MASK; @@ -156,11 +163,11 @@ paddr_t p2m_lookup(struct domain *d, paddr_t paddr, p2m_type_t *t) pte = third[third_table_offset(paddr)]; /* This bit must be one in the level 3 entry */ - if ( !pte.p2m.table ) + if ( !p2m_table(pte) ) pte.bits = 0; done: - if ( pte.p2m.valid ) + if ( p2m_valid(pte) ) { ASSERT(pte.p2m.type != p2m_invalid); maddr = (pte.bits & PADDR_MASK & mask) | (paddr & ~mask); @@ -367,7 +374,7 @@ static int apply_p2m_changes(struct domain *d, cur_first_page = p2m_first_level_index(addr); } - if ( !first[first_table_offset(addr)].p2m.valid ) + if ( !p2m_valid(first[first_table_offset(addr)]) ) { if ( !populate ) { @@ -384,7 +391,7 @@ static int apply_p2m_changes(struct domain *d, } } - BUG_ON(!first[first_table_offset(addr)].p2m.valid); + BUG_ON(!p2m_valid(first[first_table_offset(addr)])); if ( cur_first_offset != first_table_offset(addr) ) { @@ -394,7 +401,7 @@ static int apply_p2m_changes(struct domain *d, } /* else: second already valid */ - if ( !second[second_table_offset(addr)].p2m.valid ) + if ( !p2m_valid(second[second_table_offset(addr)]) ) { if ( !populate ) {
Not terribly helpful right now, since they aren't widely used, but makes future patches easier to read. Signed-off-by: Ian Campbell <ian.campbell@citrix.com> --- v2: clarify common on p2m_{table,entry} --- xen/arch/arm/p2m.c | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-)