| Message ID | 20181008183352.16291-8-julien.grall@arm.com |
|---|---|
| State | Accepted |
| Commit | 2a654d312914218782e4e3fc359ff20fe62f7b60 |
| Headers | show |
| Series | xen/arm: Implement Set/Way operations | expand |
On Mon, 8 Oct 2018, Julien Grall wrote: > The LPAE format allows to store information in an entry even with the > valid bit unset. In a follow-up patch, we will take advantage of this > feature to re-purpose the valid bit for generating a translation fault > even if an entry contains valid information. > > So we need a different way to know whether an entry contains valid > information. It is possible to use the information hold in the p2m_type > to know for that purpose. Indeed all entries containing valid > information will have a valid p2m type (i.e p2m_type != p2m_invalid). > > This patch introduces a new helper p2m_is_valid, which implements that > idea, and replace most of lpae_is_valid call with the new helper. The ones > remaining are for TLBs handling and entries accounting. > > With the renaming there are 2 others changes required: > - Generate table entry with a valid p2m type > - Detect new mapping for proper stats accounting > > Signed-off-by: Julien Grall <julien.grall@arm.com> > --- > xen/arch/arm/p2m.c | 34 +++++++++++++++++++++++----------- > 1 file changed, 23 insertions(+), 11 deletions(-) > > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c > index 6c76298ebc..2a1e7e9be2 100644 > --- a/xen/arch/arm/p2m.c > +++ b/xen/arch/arm/p2m.c > @@ -220,17 +220,26 @@ static p2m_access_t p2m_mem_access_radix_get(struct p2m_domain *p2m, gfn_t gfn) > } > > /* > + * In the case of the P2M, the valid bit is used for other purpose. Use > + * the type to check whether an entry is valid. > + */ > +static inline bool p2m_is_valid(lpae_t pte) > +{ > + return pte.p2m.type != p2m_invalid; > +} > + > +/* > * lpae_is_* helpers don't check whether the valid bit is set in the > * PTE. Provide our own overlay to check the valid bit. > */ > static inline bool p2m_is_mapping(lpae_t pte, unsigned int level) > { > - return lpae_is_valid(pte) && lpae_is_mapping(pte, level); > + return p2m_is_valid(pte) && lpae_is_mapping(pte, level); > } > > static inline bool p2m_is_superpage(lpae_t pte, unsigned int level) > { > - return lpae_is_valid(pte) && lpae_is_superpage(pte, level); > + return p2m_is_valid(pte) && lpae_is_superpage(pte, level); > } > > #define GUEST_TABLE_MAP_FAILED 0 > @@ -264,7 +273,7 @@ static int p2m_next_level(struct p2m_domain *p2m, bool read_only, > > entry = *table + offset; > > - if ( !lpae_is_valid(*entry) ) > + if ( !p2m_is_valid(*entry) ) > { > if ( read_only ) > return GUEST_TABLE_MAP_FAILED; > @@ -356,7 +365,7 @@ mfn_t p2m_get_entry(struct p2m_domain *p2m, gfn_t gfn, > > entry = table[offsets[level]]; > > - if ( lpae_is_valid(entry) ) > + if ( p2m_is_valid(entry) ) > { > *t = entry.p2m.type; > > @@ -544,8 +553,11 @@ static lpae_t page_to_p2m_table(struct page_info *page) > /* > * The access value does not matter because the hardware will ignore > * the permission fields for table entry. > + * > + * We use p2m_ram_rw so the entry has a valid type. This is important > + * for p2m_is_valid() to return valid on table entries. > */ > - return mfn_to_p2m_entry(page_to_mfn(page), p2m_invalid, p2m_access_rwx); > + return mfn_to_p2m_entry(page_to_mfn(page), p2m_ram_rw, p2m_access_rwx); > } > > static inline void p2m_write_pte(lpae_t *p, lpae_t pte, bool clean_pte) > @@ -569,7 +581,7 @@ static int p2m_create_table(struct p2m_domain *p2m, lpae_t *entry) > struct page_info *page; > lpae_t *p; > > - ASSERT(!lpae_is_valid(*entry)); > + ASSERT(!p2m_is_valid(*entry)); > > page = alloc_domheap_page(NULL, 0); > if ( page == NULL ) > @@ -626,7 +638,7 @@ static int p2m_mem_access_radix_set(struct p2m_domain *p2m, gfn_t gfn, > */ > static void p2m_put_l3_page(const lpae_t pte) > { > - ASSERT(lpae_is_valid(pte)); > + ASSERT(p2m_is_valid(pte)); > > /* > * TODO: Handle other p2m types > @@ -654,11 +666,11 @@ static void p2m_free_entry(struct p2m_domain *p2m, > struct page_info *pg; > > /* Nothing to do if the entry is invalid. */ > - if ( !lpae_is_valid(entry) ) > + if ( !p2m_is_valid(entry) ) > return; > > /* Nothing to do but updating the stats if the entry is a super-page. */ > - if ( p2m_is_superpage(entry, level) ) > + if ( level == 3 && entry.p2m.table ) Why? > { > p2m->stats.mappings[level]--; > return; > @@ -951,7 +963,7 @@ static int __p2m_set_entry(struct p2m_domain *p2m, > else > p2m->need_flush = true; > } > - else /* new mapping */ > + else if ( !p2m_is_valid(orig_pte) ) /* new mapping */ There are a couple of lpae_is_valid checks just above this line that you missed, why haven't you changed them? If you have a good reason, please explain in a comment and/or commit message. > p2m->stats.mappings[level]++; > > p2m_write_pte(entry, pte, p2m->clean_pte); > @@ -965,7 +977,7 @@ static int __p2m_set_entry(struct p2m_domain *p2m, > * Free the entry only if the original pte was valid and the base > * is different (to avoid freeing when permission is changed). > */ > - if ( lpae_is_valid(orig_pte) && > + if ( p2m_is_valid(orig_pte) && > !mfn_eq(lpae_get_mfn(*entry), lpae_get_mfn(orig_pte)) ) > p2m_free_entry(p2m, orig_pte, level); >
Hi, On 30/10/2018 00:21, Stefano Stabellini wrote: > On Mon, 8 Oct 2018, Julien Grall wrote: >> The LPAE format allows to store information in an entry even with the >> valid bit unset. In a follow-up patch, we will take advantage of this >> feature to re-purpose the valid bit for generating a translation fault >> even if an entry contains valid information. >> >> So we need a different way to know whether an entry contains valid >> information. It is possible to use the information hold in the p2m_type >> to know for that purpose. Indeed all entries containing valid >> information will have a valid p2m type (i.e p2m_type != p2m_invalid). >> >> This patch introduces a new helper p2m_is_valid, which implements that >> idea, and replace most of lpae_is_valid call with the new helper. The ones >> remaining are for TLBs handling and entries accounting. >> >> With the renaming there are 2 others changes required: >> - Generate table entry with a valid p2m type >> - Detect new mapping for proper stats accounting >> >> Signed-off-by: Julien Grall <julien.grall@arm.com> >> --- >> xen/arch/arm/p2m.c | 34 +++++++++++++++++++++++----------- >> 1 file changed, 23 insertions(+), 11 deletions(-) >> >> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c >> index 6c76298ebc..2a1e7e9be2 100644 >> --- a/xen/arch/arm/p2m.c >> +++ b/xen/arch/arm/p2m.c >> @@ -220,17 +220,26 @@ static p2m_access_t p2m_mem_access_radix_get(struct p2m_domain *p2m, gfn_t gfn) >> } >> >> /* >> + * In the case of the P2M, the valid bit is used for other purpose. Use >> + * the type to check whether an entry is valid. >> + */ >> +static inline bool p2m_is_valid(lpae_t pte) >> +{ >> + return pte.p2m.type != p2m_invalid; >> +} >> + >> +/* >> * lpae_is_* helpers don't check whether the valid bit is set in the >> * PTE. Provide our own overlay to check the valid bit. >> */ >> static inline bool p2m_is_mapping(lpae_t pte, unsigned int level) >> { >> - return lpae_is_valid(pte) && lpae_is_mapping(pte, level); >> + return p2m_is_valid(pte) && lpae_is_mapping(pte, level); >> } >> >> static inline bool p2m_is_superpage(lpae_t pte, unsigned int level) >> { >> - return lpae_is_valid(pte) && lpae_is_superpage(pte, level); >> + return p2m_is_valid(pte) && lpae_is_superpage(pte, level); >> } >> >> #define GUEST_TABLE_MAP_FAILED 0 >> @@ -264,7 +273,7 @@ static int p2m_next_level(struct p2m_domain *p2m, bool read_only, >> >> entry = *table + offset; >> >> - if ( !lpae_is_valid(*entry) ) >> + if ( !p2m_is_valid(*entry) ) >> { >> if ( read_only ) >> return GUEST_TABLE_MAP_FAILED; >> @@ -356,7 +365,7 @@ mfn_t p2m_get_entry(struct p2m_domain *p2m, gfn_t gfn, >> >> entry = table[offsets[level]]; >> >> - if ( lpae_is_valid(entry) ) >> + if ( p2m_is_valid(entry) ) >> { >> *t = entry.p2m.type; >> >> @@ -544,8 +553,11 @@ static lpae_t page_to_p2m_table(struct page_info *page) >> /* >> * The access value does not matter because the hardware will ignore >> * the permission fields for table entry. >> + * >> + * We use p2m_ram_rw so the entry has a valid type. This is important >> + * for p2m_is_valid() to return valid on table entries. >> */ >> - return mfn_to_p2m_entry(page_to_mfn(page), p2m_invalid, p2m_access_rwx); >> + return mfn_to_p2m_entry(page_to_mfn(page), p2m_ram_rw, p2m_access_rwx); >> } >> >> static inline void p2m_write_pte(lpae_t *p, lpae_t pte, bool clean_pte) >> @@ -569,7 +581,7 @@ static int p2m_create_table(struct p2m_domain *p2m, lpae_t *entry) >> struct page_info *page; >> lpae_t *p; >> >> - ASSERT(!lpae_is_valid(*entry)); >> + ASSERT(!p2m_is_valid(*entry)); >> >> page = alloc_domheap_page(NULL, 0); >> if ( page == NULL ) >> @@ -626,7 +638,7 @@ static int p2m_mem_access_radix_set(struct p2m_domain *p2m, gfn_t gfn, >> */ >> static void p2m_put_l3_page(const lpae_t pte) >> { >> - ASSERT(lpae_is_valid(pte)); >> + ASSERT(p2m_is_valid(pte)); >> >> /* >> * TODO: Handle other p2m types >> @@ -654,11 +666,11 @@ static void p2m_free_entry(struct p2m_domain *p2m, >> struct page_info *pg; >> >> /* Nothing to do if the entry is invalid. */ >> - if ( !lpae_is_valid(entry) ) >> + if ( !p2m_is_valid(entry) ) >> return; >> >> /* Nothing to do but updating the stats if the entry is a super-page. */ >> - if ( p2m_is_superpage(entry, level) ) >> + if ( level == 3 && entry.p2m.table ) > > Why? Because p2m_is_superpage(...) contains p2m_is_valid(). So we would test twice the validity of the p2m. But I guess this is not a big deal, so I can remove it. > > >> { >> p2m->stats.mappings[level]--; >> return; >> @@ -951,7 +963,7 @@ static int __p2m_set_entry(struct p2m_domain *p2m, >> else >> p2m->need_flush = true; >> } >> - else /* new mapping */ >> + else if ( !p2m_is_valid(orig_pte) ) /* new mapping */ > > There are a couple of lpae_is_valid checks just above this line that you > missed, why haven't you changed them? > > If you have a good reason, please explain in a comment and/or commit > message. This is already explained in the commit message: "This patch introduces a new helper p2m_is_valid, which implements that idea, and replace most of lpae_is_valid call with the new helper. The ones remaining are for TLBs handling and entries accounting." I believe that the code has enough existing comment to understand why lpae_is_valid(...) should be kept. You deal with hardware update and hence you should use the valid bit in the LPAE table. This will tell you whether you need to flush the TLBs. > >> p2m->stats.mappings[level]++; >> >> p2m_write_pte(entry, pte, p2m->clean_pte); >> @@ -965,7 +977,7 @@ static int __p2m_set_entry(struct p2m_domain *p2m, >> * Free the entry only if the original pte was valid and the base >> * is different (to avoid freeing when permission is changed). >> */ >> - if ( lpae_is_valid(orig_pte) && >> + if ( p2m_is_valid(orig_pte) && >> !mfn_eq(lpae_get_mfn(*entry), lpae_get_mfn(orig_pte)) ) >> p2m_free_entry(p2m, orig_pte, level); >> Cheers,
On Tue, 30 Oct 2018, Julien Grall wrote: > Hi, > > On 30/10/2018 00:21, Stefano Stabellini wrote: > > On Mon, 8 Oct 2018, Julien Grall wrote: > > > The LPAE format allows to store information in an entry even with the > > > valid bit unset. In a follow-up patch, we will take advantage of this > > > feature to re-purpose the valid bit for generating a translation fault > > > even if an entry contains valid information. > > > > > > So we need a different way to know whether an entry contains valid > > > information. It is possible to use the information hold in the p2m_type > > > to know for that purpose. Indeed all entries containing valid > > > information will have a valid p2m type (i.e p2m_type != p2m_invalid). > > > > > > This patch introduces a new helper p2m_is_valid, which implements that > > > idea, and replace most of lpae_is_valid call with the new helper. The ones > > > remaining are for TLBs handling and entries accounting. > > > > > > With the renaming there are 2 others changes required: > > > - Generate table entry with a valid p2m type > > > - Detect new mapping for proper stats accounting > > > > > > Signed-off-by: Julien Grall <julien.grall@arm.com> > > > --- > > > xen/arch/arm/p2m.c | 34 +++++++++++++++++++++++----------- > > > 1 file changed, 23 insertions(+), 11 deletions(-) > > > > > > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c > > > index 6c76298ebc..2a1e7e9be2 100644 > > > --- a/xen/arch/arm/p2m.c > > > +++ b/xen/arch/arm/p2m.c > > > @@ -220,17 +220,26 @@ static p2m_access_t p2m_mem_access_radix_get(struct > > > p2m_domain *p2m, gfn_t gfn) > > > } > > > /* > > > + * In the case of the P2M, the valid bit is used for other purpose. Use > > > + * the type to check whether an entry is valid. > > > + */ > > > +static inline bool p2m_is_valid(lpae_t pte) > > > +{ > > > + return pte.p2m.type != p2m_invalid; > > > +} > > > + > > > +/* > > > * lpae_is_* helpers don't check whether the valid bit is set in the > > > * PTE. Provide our own overlay to check the valid bit. > > > */ > > > static inline bool p2m_is_mapping(lpae_t pte, unsigned int level) > > > { > > > - return lpae_is_valid(pte) && lpae_is_mapping(pte, level); > > > + return p2m_is_valid(pte) && lpae_is_mapping(pte, level); > > > } > > > static inline bool p2m_is_superpage(lpae_t pte, unsigned int level) > > > { > > > - return lpae_is_valid(pte) && lpae_is_superpage(pte, level); > > > + return p2m_is_valid(pte) && lpae_is_superpage(pte, level); > > > } > > > #define GUEST_TABLE_MAP_FAILED 0 > > > @@ -264,7 +273,7 @@ static int p2m_next_level(struct p2m_domain *p2m, bool > > > read_only, > > > entry = *table + offset; > > > - if ( !lpae_is_valid(*entry) ) > > > + if ( !p2m_is_valid(*entry) ) > > > { > > > if ( read_only ) > > > return GUEST_TABLE_MAP_FAILED; > > > @@ -356,7 +365,7 @@ mfn_t p2m_get_entry(struct p2m_domain *p2m, gfn_t gfn, > > > entry = table[offsets[level]]; > > > - if ( lpae_is_valid(entry) ) > > > + if ( p2m_is_valid(entry) ) > > > { > > > *t = entry.p2m.type; > > > @@ -544,8 +553,11 @@ static lpae_t page_to_p2m_table(struct page_info > > > *page) > > > /* > > > * The access value does not matter because the hardware will ignore > > > * the permission fields for table entry. > > > + * > > > + * We use p2m_ram_rw so the entry has a valid type. This is important > > > + * for p2m_is_valid() to return valid on table entries. > > > */ > > > - return mfn_to_p2m_entry(page_to_mfn(page), p2m_invalid, > > > p2m_access_rwx); > > > + return mfn_to_p2m_entry(page_to_mfn(page), p2m_ram_rw, > > > p2m_access_rwx); > > > } > > > static inline void p2m_write_pte(lpae_t *p, lpae_t pte, bool > > > clean_pte) > > > @@ -569,7 +581,7 @@ static int p2m_create_table(struct p2m_domain *p2m, > > > lpae_t *entry) > > > struct page_info *page; > > > lpae_t *p; > > > - ASSERT(!lpae_is_valid(*entry)); > > > + ASSERT(!p2m_is_valid(*entry)); > > > page = alloc_domheap_page(NULL, 0); > > > if ( page == NULL ) > > > @@ -626,7 +638,7 @@ static int p2m_mem_access_radix_set(struct p2m_domain > > > *p2m, gfn_t gfn, > > > */ > > > static void p2m_put_l3_page(const lpae_t pte) > > > { > > > - ASSERT(lpae_is_valid(pte)); > > > + ASSERT(p2m_is_valid(pte)); > > > /* > > > * TODO: Handle other p2m types > > > @@ -654,11 +666,11 @@ static void p2m_free_entry(struct p2m_domain *p2m, > > > struct page_info *pg; > > > /* Nothing to do if the entry is invalid. */ > > > - if ( !lpae_is_valid(entry) ) > > > + if ( !p2m_is_valid(entry) ) > > > return; > > > /* Nothing to do but updating the stats if the entry is a > > > super-page. */ > > > - if ( p2m_is_superpage(entry, level) ) > > > + if ( level == 3 && entry.p2m.table ) > > > > Why? > > Because p2m_is_superpage(...) contains p2m_is_valid(). So we would test twice > the validity of the p2m. > > But I guess this is not a big deal, so I can remove it. > > > > > > > > { > > > p2m->stats.mappings[level]--; > > > return; > > > @@ -951,7 +963,7 @@ static int __p2m_set_entry(struct p2m_domain *p2m, > > > else > > > p2m->need_flush = true; > > > } > > > - else /* new mapping */ > > > + else if ( !p2m_is_valid(orig_pte) ) /* new mapping */ > > > > There are a couple of lpae_is_valid checks just above this line that you > > missed, why haven't you changed them? > > > > If you have a good reason, please explain in a comment and/or commit > > message. > > This is already explained in the commit message: > > "This patch introduces a new helper p2m_is_valid, which implements that > idea, and replace most of lpae_is_valid call with the new helper. The ones > remaining are for TLBs handling and entries accounting." > > I believe that the code has enough existing comment to understand why > lpae_is_valid(...) should be kept. You deal with hardware update and hence you > should use the valid bit in the LPAE table. This will tell you whether you > need to flush the TLBs. I checked and it is like you wrote.
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c index 6c76298ebc..2a1e7e9be2 100644 --- a/xen/arch/arm/p2m.c +++ b/xen/arch/arm/p2m.c @@ -220,17 +220,26 @@ static p2m_access_t p2m_mem_access_radix_get(struct p2m_domain *p2m, gfn_t gfn) } /* + * In the case of the P2M, the valid bit is used for other purpose. Use + * the type to check whether an entry is valid. + */ +static inline bool p2m_is_valid(lpae_t pte) +{ + return pte.p2m.type != p2m_invalid; +} + +/* * lpae_is_* helpers don't check whether the valid bit is set in the * PTE. Provide our own overlay to check the valid bit. */ static inline bool p2m_is_mapping(lpae_t pte, unsigned int level) { - return lpae_is_valid(pte) && lpae_is_mapping(pte, level); + return p2m_is_valid(pte) && lpae_is_mapping(pte, level); } static inline bool p2m_is_superpage(lpae_t pte, unsigned int level) { - return lpae_is_valid(pte) && lpae_is_superpage(pte, level); + return p2m_is_valid(pte) && lpae_is_superpage(pte, level); } #define GUEST_TABLE_MAP_FAILED 0 @@ -264,7 +273,7 @@ static int p2m_next_level(struct p2m_domain *p2m, bool read_only, entry = *table + offset; - if ( !lpae_is_valid(*entry) ) + if ( !p2m_is_valid(*entry) ) { if ( read_only ) return GUEST_TABLE_MAP_FAILED; @@ -356,7 +365,7 @@ mfn_t p2m_get_entry(struct p2m_domain *p2m, gfn_t gfn, entry = table[offsets[level]]; - if ( lpae_is_valid(entry) ) + if ( p2m_is_valid(entry) ) { *t = entry.p2m.type; @@ -544,8 +553,11 @@ static lpae_t page_to_p2m_table(struct page_info *page) /* * The access value does not matter because the hardware will ignore * the permission fields for table entry. + * + * We use p2m_ram_rw so the entry has a valid type. This is important + * for p2m_is_valid() to return valid on table entries. */ - return mfn_to_p2m_entry(page_to_mfn(page), p2m_invalid, p2m_access_rwx); + return mfn_to_p2m_entry(page_to_mfn(page), p2m_ram_rw, p2m_access_rwx); } static inline void p2m_write_pte(lpae_t *p, lpae_t pte, bool clean_pte) @@ -569,7 +581,7 @@ static int p2m_create_table(struct p2m_domain *p2m, lpae_t *entry) struct page_info *page; lpae_t *p; - ASSERT(!lpae_is_valid(*entry)); + ASSERT(!p2m_is_valid(*entry)); page = alloc_domheap_page(NULL, 0); if ( page == NULL ) @@ -626,7 +638,7 @@ static int p2m_mem_access_radix_set(struct p2m_domain *p2m, gfn_t gfn, */ static void p2m_put_l3_page(const lpae_t pte) { - ASSERT(lpae_is_valid(pte)); + ASSERT(p2m_is_valid(pte)); /* * TODO: Handle other p2m types @@ -654,11 +666,11 @@ static void p2m_free_entry(struct p2m_domain *p2m, struct page_info *pg; /* Nothing to do if the entry is invalid. */ - if ( !lpae_is_valid(entry) ) + if ( !p2m_is_valid(entry) ) return; /* Nothing to do but updating the stats if the entry is a super-page. */ - if ( p2m_is_superpage(entry, level) ) + if ( level == 3 && entry.p2m.table ) { p2m->stats.mappings[level]--; return; @@ -951,7 +963,7 @@ static int __p2m_set_entry(struct p2m_domain *p2m, else p2m->need_flush = true; } - else /* new mapping */ + else if ( !p2m_is_valid(orig_pte) ) /* new mapping */ p2m->stats.mappings[level]++; p2m_write_pte(entry, pte, p2m->clean_pte); @@ -965,7 +977,7 @@ static int __p2m_set_entry(struct p2m_domain *p2m, * Free the entry only if the original pte was valid and the base * is different (to avoid freeing when permission is changed). */ - if ( lpae_is_valid(orig_pte) && + if ( p2m_is_valid(orig_pte) && !mfn_eq(lpae_get_mfn(*entry), lpae_get_mfn(orig_pte)) ) p2m_free_entry(p2m, orig_pte, level);
The LPAE format allows to store information in an entry even with the valid bit unset. In a follow-up patch, we will take advantage of this feature to re-purpose the valid bit for generating a translation fault even if an entry contains valid information. So we need a different way to know whether an entry contains valid information. It is possible to use the information hold in the p2m_type to know for that purpose. Indeed all entries containing valid information will have a valid p2m type (i.e p2m_type != p2m_invalid). This patch introduces a new helper p2m_is_valid, which implements that idea, and replace most of lpae_is_valid call with the new helper. The ones remaining are for TLBs handling and entries accounting. With the renaming there are 2 others changes required: - Generate table entry with a valid p2m type - Detect new mapping for proper stats accounting Signed-off-by: Julien Grall <julien.grall@arm.com> --- xen/arch/arm/p2m.c | 34 +++++++++++++++++++++++----------- 1 file changed, 23 insertions(+), 11 deletions(-)