| Message ID | 1402394278-9850-5-git-send-email-ian.campbell@citrix.com |
|---|---|
| State | New |
| Headers | show |
On 06/10/2014 10:57 AM, Ian Campbell wrote: > 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 +161,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) ) Regardless the current check, I think this should be p2m_entry(pte) to help code comprehension. Indeed, the can only get the address if the pte is pointed to a memory block. Regards,
On Tue, 2014-06-10 at 12:37 +0100, Julien Grall wrote: > On 06/10/2014 10:57 AM, Ian Campbell wrote: > > 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 +161,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) ) > > Regardless the current check, I think this should be p2m_entry(pte) to > help code comprehension. > > Indeed, the can only get the address if the pte is pointed to a memory > block. Yes, but an L3 PTE has the table bit set, which would make p2m_entry(pte) false...
On 06/10/2014 12:46 PM, Ian Campbell wrote: > On Tue, 2014-06-10 at 12:37 +0100, Julien Grall wrote: >> On 06/10/2014 10:57 AM, Ian Campbell wrote: >>> 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 +161,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) ) >> >> Regardless the current check, I think this should be p2m_entry(pte) to >> help code comprehension. >> >> Indeed, the can only get the address if the pte is pointed to a memory >> block. > > Yes, but an L3 PTE has the table bit set, which would make > p2m_entry(pte) false... Hmmm... right. But this bit (ie table bit) doesn't have the same meaning on L3. Your comment on p2m_table is confusing. Anyway: Acked-by: Julien Grall <julien.grall@linaro.org>
On Tue, 2014-06-10 at 12:54 +0100, Julien Grall wrote: > On 06/10/2014 12:46 PM, Ian Campbell wrote: > > On Tue, 2014-06-10 at 12:37 +0100, Julien Grall wrote: > >> On 06/10/2014 10:57 AM, Ian Campbell wrote: > >>> 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 +161,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) ) > >> > >> Regardless the current check, I think this should be p2m_entry(pte) to > >> help code comprehension. > >> > >> Indeed, the can only get the address if the pte is pointed to a memory > >> block. > > > > Yes, but an L3 PTE has the table bit set, which would make > > p2m_entry(pte) false... > > Hmmm... right. But this bit (ie table bit) doesn't have the same meaning > on L3. Your comment on p2m_table is confusing. You mean: /* Remember: L3 entries set the table bit! */ ? That was intended to be a comment on the two helpers which follow. Do you have an idea how I could make it clearer? Perhaps appending "So these two functions will return the opposite to what you expect for L3 ptes"? > > Anyway: > > Acked-by: Julien Grall <julien.grall@linaro.org> > Thanks.
On 06/10/2014 01:29 PM, Ian Campbell wrote: > On Tue, 2014-06-10 at 12:54 +0100, Julien Grall wrote: >> On 06/10/2014 12:46 PM, Ian Campbell wrote: >>> On Tue, 2014-06-10 at 12:37 +0100, Julien Grall wrote: >>>> On 06/10/2014 10:57 AM, Ian Campbell wrote: >>>>> 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 +161,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) ) >>>> >>>> Regardless the current check, I think this should be p2m_entry(pte) to >>>> help code comprehension. >>>> >>>> Indeed, the can only get the address if the pte is pointed to a memory >>>> block. >>> >>> Yes, but an L3 PTE has the table bit set, which would make >>> p2m_entry(pte) false... >> >> Hmmm... right. But this bit (ie table bit) doesn't have the same meaning >> on L3. Your comment on p2m_table is confusing. > > You mean: /* Remember: L3 entries set the table bit! */ ? > > That was intended to be a comment on the two helpers which follow. Do > you have an idea how I could make it clearer? Perhaps appending "So > these two functions will return the opposite to what you expect for L3 > ptes"? I was thinking something like "These two functions are only valid for L1 and L2 ptes".
On Tue, 2014-06-10 at 13:52 +0100, Julien Grall wrote: > >>>> Indeed, the can only get the address if the pte is pointed to a memory > >>>> block. > >>> > >>> Yes, but an L3 PTE has the table bit set, which would make > >>> p2m_entry(pte) false... > >> > >> Hmmm... right. But this bit (ie table bit) doesn't have the same meaning > >> on L3. Your comment on p2m_table is confusing. > > > > You mean: /* Remember: L3 entries set the table bit! */ ? > > > > That was intended to be a comment on the two helpers which follow. Do > > you have an idea how I could make it clearer? Perhaps appending "So > > these two functions will return the opposite to what you expect for L3 > > ptes"? > > I was thinking something like "These two functions are only valid for L1 > and L2 ptes". I think I'll do some combination of both. Ian.
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c index 5d654ce..233df72 100644 --- a/xen/arch/arm/p2m.c +++ b/xen/arch/arm/p2m.c @@ -14,6 +14,11 @@ #define P2M_FIRST_ORDER 1 #define P2M_FIRST_ENTRIES (LPAE_ENTRIES<<P2M_FIRST_ORDER) +#define p2m_valid(pte) ((pte).p2m.valid) +/* Remember: L3 entries set the table bit! */ +#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 +144,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 +161,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 +372,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 +389,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 +399,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> --- xen/arch/arm/p2m.c | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-)