| Message ID | 20240430165845.81696-2-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/x86: support foreign mappings for HVM | expand |
On 30.04.2024 18:58, Roger Pau Monne wrote: > Keep track of the maximum gfn that has ever been populated into the p2m, and > also account for the number of foreign mappings. Such information will be > needed in order to remove foreign mappings during teardown for HVM guests. Is "needed" the right term? We could e.g. traverse the P2M tree (didn't look at patch 2 yet as to how exactly you use these two new fields there), at which point we might get away without either or both of these extra statistics, while at the same time also not needing to iterate over a gigantic range of GFNs. Going from populated page tables would roughly match "max_gfn", with the benefit of certain removals of P2M entries then also shrinking the upper bound. > @@ -1049,6 +1057,8 @@ static inline int p2m_entry_modify(struct p2m_domain *p2m, p2m_type_t nt, > if ( !page_get_owner_and_reference(mfn_to_page(nfn)) ) > return -EBUSY; > > + p2m->nr_foreign++; > + > break; > > default: > @@ -1069,6 +1079,7 @@ static inline int p2m_entry_modify(struct p2m_domain *p2m, p2m_type_t nt, > return -EINVAL; > } > put_page(mfn_to_page(ofn)); > + p2m->nr_foreign--; > break; Like for the ioreq accounting I'm a little worried of putting this here, especially with the decrement thus coming ahead of the actual page table update, but probably I'm overly concerned here. The put_page() living here would clearly be doing bigger damage if not unconditionally followed by a page table write. IOW - just a remark, no request for any kind of change. > --- a/xen/arch/x86/mm/p2m.c > +++ b/xen/arch/x86/mm/p2m.c > @@ -413,6 +413,8 @@ int p2m_set_entry(struct p2m_domain *p2m, gfn_t gfn, mfn_t mfn, > set_rc = p2m->set_entry(p2m, gfn, mfn, order, p2mt, p2ma, -1); > if ( set_rc ) > rc = set_rc; > + else > + p2m->max_gfn = gfn_max(gfn_add(gfn, 1u << order), p2m->max_gfn); For one a (new) field named "max_..." wants to record the maximum value, not one above. And then you want to use 1UL, to match ... > gfn = gfn_add(gfn, 1UL << order); > if ( !mfn_eq(mfn, INVALID_MFN) ) ... surrounding code (more just out of context). Further I can't really convince myself that doing the update just here is enough, or whether alternatively the update wouldn't want to be further constrained to happen just on newly set foreign entries. In that latter case it would be far easier to reason whether doing the update just here is sufficient. Plus iirc foreign entries are also necessarily order-0 (else p2m_entry_modify() wouldn't be correct as is), which would allow to store just the gfn we have in hands, thus resulting in the field then being properly named (as to its prefix; it would likely want to become "max_foreign_gfn" then). Jan
On Mon, May 06, 2024 at 12:07:33PM +0200, Jan Beulich wrote: > On 30.04.2024 18:58, Roger Pau Monne wrote: > > Keep track of the maximum gfn that has ever been populated into the p2m, and > > also account for the number of foreign mappings. Such information will be > > needed in order to remove foreign mappings during teardown for HVM guests. > > Is "needed" the right term? We could e.g. traverse the P2M tree (didn't look > at patch 2 yet as to how exactly you use these two new fields there), at which > point we might get away without either or both of these extra statistics, > while at the same time also not needing to iterate over a gigantic range of > GFNs. Going from populated page tables would roughly match "max_gfn", with the > benefit of certain removals of P2M entries then also shrinking the upper bound. The nr_foreign field is also used as a way to signal whether iteration over the p2m is needed in the first place. If there are no foreign entries the iteration can be avoided (which is likely the case for a lot of domains). Note that in 2/2 max_gfn is also used as the cursor for the teardown iteration, and points to the last processed p2m entry. So even if the maximum gfn is obtained from the p2m page-tables directly, we would still need some kind of cursor to signal the position during teardown. Or alternatively remove all entries from the p2m, regardless of their type, so that the p2m shrinks. > > --- a/xen/arch/x86/mm/p2m.c > > +++ b/xen/arch/x86/mm/p2m.c > > @@ -413,6 +413,8 @@ int p2m_set_entry(struct p2m_domain *p2m, gfn_t gfn, mfn_t mfn, > > set_rc = p2m->set_entry(p2m, gfn, mfn, order, p2mt, p2ma, -1); > > if ( set_rc ) > > rc = set_rc; > > + else > > + p2m->max_gfn = gfn_max(gfn_add(gfn, 1u << order), p2m->max_gfn); > > For one a (new) field named "max_..." wants to record the maximum value, not > one above. And then you want to use 1UL, to match ... So gfn + (1UL << order) - 1. > > gfn = gfn_add(gfn, 1UL << order); > > if ( !mfn_eq(mfn, INVALID_MFN) ) > > ... surrounding code (more just out of context). Oh, indeed. > Further I can't really convince myself that doing the update just here is > enough, or whether alternatively the update wouldn't want to be further > constrained to happen just on newly set foreign entries. In that latter > case it would be far easier to reason whether doing the update just here is > sufficient. Plus iirc foreign entries are also necessarily order-0 (else > p2m_entry_modify() wouldn't be correct as is), which would allow to store > just the gfn we have in hands, thus resulting in the field then being > properly named (as to its prefix; it would likely want to become > "max_foreign_gfn" then). I didn't want to limit this to foreign entries exclusively, as it could be useful for other purposes. My initial intention was to do it in p2m_entry_modify() so that nr_foreign and max_gfn where set in the same function, but that requires passing yet another parameter to the function. Thanks, Roger.
On 06.05.2024 16:32, Roger Pau Monné wrote: > On Mon, May 06, 2024 at 12:07:33PM +0200, Jan Beulich wrote: >> On 30.04.2024 18:58, Roger Pau Monne wrote: >>> Keep track of the maximum gfn that has ever been populated into the p2m, and >>> also account for the number of foreign mappings. Such information will be >>> needed in order to remove foreign mappings during teardown for HVM guests. >> >> Is "needed" the right term? We could e.g. traverse the P2M tree (didn't look >> at patch 2 yet as to how exactly you use these two new fields there), at which >> point we might get away without either or both of these extra statistics, >> while at the same time also not needing to iterate over a gigantic range of >> GFNs. Going from populated page tables would roughly match "max_gfn", with the >> benefit of certain removals of P2M entries then also shrinking the upper bound. > > The nr_foreign field is also used as a way to signal whether iteration > over the p2m is needed in the first place. If there are no foreign > entries the iteration can be avoided (which is likely the case for a > lot of domains). > > Note that in 2/2 max_gfn is also used as the cursor for the teardown > iteration, and points to the last processed p2m entry. So even if the > maximum gfn is obtained from the p2m page-tables directly, we would > still need some kind of cursor to signal the position during teardown. > Or alternatively remove all entries from the p2m, regardless of their > type, so that the p2m shrinks. Having such a cursor just for teardown wouldn't be a big deal, I think. >>> --- a/xen/arch/x86/mm/p2m.c >>> +++ b/xen/arch/x86/mm/p2m.c >>> @@ -413,6 +413,8 @@ int p2m_set_entry(struct p2m_domain *p2m, gfn_t gfn, mfn_t mfn, >>> set_rc = p2m->set_entry(p2m, gfn, mfn, order, p2mt, p2ma, -1); >>> if ( set_rc ) >>> rc = set_rc; >>> + else >>> + p2m->max_gfn = gfn_max(gfn_add(gfn, 1u << order), p2m->max_gfn); >> >> For one a (new) field named "max_..." wants to record the maximum value, not >> one above. And then you want to use 1UL, to match ... > > So gfn + (1UL << order) - 1. Right, or give the field a different name. >>> gfn = gfn_add(gfn, 1UL << order); >>> if ( !mfn_eq(mfn, INVALID_MFN) ) >> >> ... surrounding code (more just out of context). > > Oh, indeed. > >> Further I can't really convince myself that doing the update just here is >> enough, or whether alternatively the update wouldn't want to be further >> constrained to happen just on newly set foreign entries. In that latter >> case it would be far easier to reason whether doing the update just here is >> sufficient. Plus iirc foreign entries are also necessarily order-0 (else >> p2m_entry_modify() wouldn't be correct as is), which would allow to store >> just the gfn we have in hands, thus resulting in the field then being >> properly named (as to its prefix; it would likely want to become >> "max_foreign_gfn" then). > > I didn't want to limit this to foreign entries exclusively, as it > could be useful for other purposes. I see. > My initial intention was to do it > in p2m_entry_modify() so that nr_foreign and max_gfn where set in the > same function, but that requires passing yet another parameter to the > function. I was indeed implying that would have been the reason for you to not have put it there. What you don't answer though is the question of how you determined that none of the other ->set_entry() invocations would need to have similar code added. There are quite a few of them, after all. Jan
On Mon, May 06, 2024 at 04:55:45PM +0200, Jan Beulich wrote: > On 06.05.2024 16:32, Roger Pau Monné wrote: > > On Mon, May 06, 2024 at 12:07:33PM +0200, Jan Beulich wrote: > >> On 30.04.2024 18:58, Roger Pau Monne wrote: > > My initial intention was to do it > > in p2m_entry_modify() so that nr_foreign and max_gfn where set in the > > same function, but that requires passing yet another parameter to the > > function. > > I was indeed implying that would have been the reason for you to not have > put it there. > > What you don't answer though is the question of how you determined that > none of the other ->set_entry() invocations would need to have similar > code added. There are quite a few of them, after all. Aside from the mem_sharing copying/forking usages, the rest of the uses of ->set_entry() looked like changes over existing entries, and strictly not adding new entries. I might be wrong however, I see that some of the altp2m usages could also end up populating altp2m entries (not that the teardown will work with altp2m-s anyway). Thanks, Roger.
On Mon, May 06, 2024 at 12:07:33PM +0200, Jan Beulich wrote: > On 30.04.2024 18:58, Roger Pau Monne wrote: > > Keep track of the maximum gfn that has ever been populated into the p2m, and > > also account for the number of foreign mappings. Such information will be > > needed in order to remove foreign mappings during teardown for HVM guests. > > Is "needed" the right term? We could e.g. traverse the P2M tree (didn't look > at patch 2 yet as to how exactly you use these two new fields there), at which > point we might get away without either or both of these extra statistics, > while at the same time also not needing to iterate over a gigantic range of > GFNs. Going from populated page tables would roughly match "max_gfn", with the > benefit of certain removals of P2M entries then also shrinking the upper bound. One note about traversing the p2m tree that I forgot to add earlier: AFAICT we would need one implementation for EPT and one for NPT, as I expect the different page-table format won't allow us to use the same code against both EPT and NPT page-tables (I really need to check). Thanks, Roger.
On 06.05.2024 17:33, Roger Pau Monné wrote: > On Mon, May 06, 2024 at 12:07:33PM +0200, Jan Beulich wrote: >> On 30.04.2024 18:58, Roger Pau Monne wrote: >>> Keep track of the maximum gfn that has ever been populated into the p2m, and >>> also account for the number of foreign mappings. Such information will be >>> needed in order to remove foreign mappings during teardown for HVM guests. >> >> Is "needed" the right term? We could e.g. traverse the P2M tree (didn't look >> at patch 2 yet as to how exactly you use these two new fields there), at which >> point we might get away without either or both of these extra statistics, >> while at the same time also not needing to iterate over a gigantic range of >> GFNs. Going from populated page tables would roughly match "max_gfn", with the >> benefit of certain removals of P2M entries then also shrinking the upper bound. > > One note about traversing the p2m tree that I forgot to add earlier: > AFAICT we would need one implementation for EPT and one for NPT, as I > expect the different page-table format won't allow us to use the same > code against both EPT and NPT page-tables (I really need to check). Yes, that would be pretty much unavoidable, I agree. Jan
diff --git a/xen/arch/x86/include/asm/p2m.h b/xen/arch/x86/include/asm/p2m.h index 111badf89a6e..d95341ef4242 100644 --- a/xen/arch/x86/include/asm/p2m.h +++ b/xen/arch/x86/include/asm/p2m.h @@ -380,6 +380,14 @@ struct p2m_domain { unsigned int flags; unsigned long entry_count; } ioreq; + + /* + * Max gfn possibly mapped into the guest p2m. Note max_gfn is not + * adjusted to account for removals from the p2m. + */ + gfn_t max_gfn; + /* Number of foreign mappings. */ + unsigned long nr_foreign; #endif /* CONFIG_HVM */ }; @@ -1049,6 +1057,8 @@ static inline int p2m_entry_modify(struct p2m_domain *p2m, p2m_type_t nt, if ( !page_get_owner_and_reference(mfn_to_page(nfn)) ) return -EBUSY; + p2m->nr_foreign++; + break; default: @@ -1069,6 +1079,7 @@ static inline int p2m_entry_modify(struct p2m_domain *p2m, p2m_type_t nt, return -EINVAL; } put_page(mfn_to_page(ofn)); + p2m->nr_foreign--; break; default: diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c index ce742c12e0de..05d8536adcd7 100644 --- a/xen/arch/x86/mm/p2m.c +++ b/xen/arch/x86/mm/p2m.c @@ -413,6 +413,8 @@ int p2m_set_entry(struct p2m_domain *p2m, gfn_t gfn, mfn_t mfn, set_rc = p2m->set_entry(p2m, gfn, mfn, order, p2mt, p2ma, -1); if ( set_rc ) rc = set_rc; + else + p2m->max_gfn = gfn_max(gfn_add(gfn, 1u << order), p2m->max_gfn); gfn = gfn_add(gfn, 1UL << order); if ( !mfn_eq(mfn, INVALID_MFN) )
Keep track of the maximum gfn that has ever been populated into the p2m, and also account for the number of foreign mappings. Such information will be needed in order to remove foreign mappings during teardown for HVM guests. Right now the introduced counters are not consumed. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- xen/arch/x86/include/asm/p2m.h | 11 +++++++++++ xen/arch/x86/mm/p2m.c | 2 ++ 2 files changed, 13 insertions(+)