| Message ID | b590b590-764a-4dfe-9e13-975122790329@suse.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] x86/P2M: synchronize fast and slow paths of p2m_get_page_from_gfn() | expand |
On Tue, Mar 25, 2025 at 08:17:04AM +0100, Jan Beulich wrote: > Handling of both grants and foreign pages was different between the two > paths. > > While permitting access to grants would be desirable, doing so would > require more involved handling; undo that for the time being. In > particular the page reference obtained would prevent the owning domain > from changing e.g. the page's type (after the grantee has released the > last reference of the grant). Instead perhaps another reference on the > grant would need obtaining. Which in turn would require determining > which grant that was. > > Foreign pages in any event need permitting on both paths. I've been wondering about this, and AFAICT the PV equivalent of this function (the tail of get_page_from_gfn()) is much more limited in that it just allows access to domain owned RAM or MMIO that has a backing page (I expect mostly for the low 1MB?). However for HVM domains we allow to take references to foreign mapped pages and taking references to MMIO pages is not permitted. Should the tail of get_page_from_gfn() also be adjusted to make use of the newly introduced get_page_from_mfn_and_type(), thus unifying the logic for both PV and HVM? Could possibly be done in a separate change, I'm just trying to understand why we have this seemingly different handling for PV vs HVM. Thanks, Roger.
On 27.03.2025 10:00, Roger Pau Monné wrote: > On Tue, Mar 25, 2025 at 08:17:04AM +0100, Jan Beulich wrote: >> Handling of both grants and foreign pages was different between the two >> paths. >> >> While permitting access to grants would be desirable, doing so would >> require more involved handling; undo that for the time being. In >> particular the page reference obtained would prevent the owning domain >> from changing e.g. the page's type (after the grantee has released the >> last reference of the grant). Instead perhaps another reference on the >> grant would need obtaining. Which in turn would require determining >> which grant that was. >> >> Foreign pages in any event need permitting on both paths. > > I've been wondering about this, and AFAICT the PV equivalent of this > function (the tail of get_page_from_gfn()) is much more limited in > that it just allows access to domain owned RAM or MMIO that has a > backing page (I expect mostly for the low 1MB?). > > However for HVM domains we allow to take references to foreign mapped > pages and taking references to MMIO pages is not permitted. > > Should the tail of get_page_from_gfn() also be adjusted to make use of > the newly introduced get_page_from_mfn_and_type(), thus unifying the > logic for both PV and HVM? There's no (proper) use of P2M types in PV, so I don't think using this function is viable for PV. In particular we'd never observe p2m_foreign in PV, if I'm not mistaken. > Could possibly be done in a separate change, I'm just trying to > understand why we have this seemingly different handling for PV vs > HVM. The difference is because access to foreign pages was hacked in to work for PVH in a much different (and imo yet more hacky) way than it was made work far longer ago for PV. The crucial part of that is in get_page_from_l1e(), so get_page_from_gfn() isn't that relevant there (I think). Jan
On Thu, Mar 27, 2025 at 10:24:02AM +0100, Jan Beulich wrote: > On 27.03.2025 10:00, Roger Pau Monné wrote: > > On Tue, Mar 25, 2025 at 08:17:04AM +0100, Jan Beulich wrote: > >> Handling of both grants and foreign pages was different between the two > >> paths. > >> > >> While permitting access to grants would be desirable, doing so would > >> require more involved handling; undo that for the time being. In > >> particular the page reference obtained would prevent the owning domain > >> from changing e.g. the page's type (after the grantee has released the > >> last reference of the grant). Instead perhaps another reference on the > >> grant would need obtaining. Which in turn would require determining > >> which grant that was. > >> > >> Foreign pages in any event need permitting on both paths. > > > > I've been wondering about this, and AFAICT the PV equivalent of this > > function (the tail of get_page_from_gfn()) is much more limited in > > that it just allows access to domain owned RAM or MMIO that has a > > backing page (I expect mostly for the low 1MB?). > > > > However for HVM domains we allow to take references to foreign mapped > > pages and taking references to MMIO pages is not permitted. > > > > Should the tail of get_page_from_gfn() also be adjusted to make use of > > the newly introduced get_page_from_mfn_and_type(), thus unifying the > > logic for both PV and HVM? > > There's no (proper) use of P2M types in PV, so I don't think using this > function is viable for PV. In particular we'd never observe p2m_foreign > in PV, if I'm not mistaken. Indeed, p2m types are just occasionally faked for PV, like in get_page_from_gfn(). > > Could possibly be done in a separate change, I'm just trying to > > understand why we have this seemingly different handling for PV vs > > HVM. > > The difference is because access to foreign pages was hacked in to work > for PVH in a much different (and imo yet more hacky) way than it was > made work far longer ago for PV. The crucial part of that is in > get_page_from_l1e(), so get_page_from_gfn() isn't that relevant there > (I think). OK. I'm kind of surprised we need foreign handling for HVM and not for PV in get_page_from_gfn(), as even with specific handling of foreign pages in get_page_from_l1e(), the former is used in a lot of places that don't seem obviously tied or gated to a call to get_page_from_l1e(). For example the XEN_DOMCTL_getpageframeinfo3 usage of get_page_from_gfn() will have different results when used against a foreign page depending on whether the caller is a PV or an HVM domain. Thanks, Roger.
On 27.03.2025 11:38, Roger Pau Monné wrote: > On Thu, Mar 27, 2025 at 10:24:02AM +0100, Jan Beulich wrote: >> On 27.03.2025 10:00, Roger Pau Monné wrote: >>> On Tue, Mar 25, 2025 at 08:17:04AM +0100, Jan Beulich wrote: >>>> Handling of both grants and foreign pages was different between the two >>>> paths. >>>> >>>> While permitting access to grants would be desirable, doing so would >>>> require more involved handling; undo that for the time being. In >>>> particular the page reference obtained would prevent the owning domain >>>> from changing e.g. the page's type (after the grantee has released the >>>> last reference of the grant). Instead perhaps another reference on the >>>> grant would need obtaining. Which in turn would require determining >>>> which grant that was. >>>> >>>> Foreign pages in any event need permitting on both paths. >>> >>> I've been wondering about this, and AFAICT the PV equivalent of this >>> function (the tail of get_page_from_gfn()) is much more limited in >>> that it just allows access to domain owned RAM or MMIO that has a >>> backing page (I expect mostly for the low 1MB?). >>> >>> However for HVM domains we allow to take references to foreign mapped >>> pages and taking references to MMIO pages is not permitted. >>> >>> Should the tail of get_page_from_gfn() also be adjusted to make use of >>> the newly introduced get_page_from_mfn_and_type(), thus unifying the >>> logic for both PV and HVM? >> >> There's no (proper) use of P2M types in PV, so I don't think using this >> function is viable for PV. In particular we'd never observe p2m_foreign >> in PV, if I'm not mistaken. > > Indeed, p2m types are just occasionally faked for PV, like in > get_page_from_gfn(). > >>> Could possibly be done in a separate change, I'm just trying to >>> understand why we have this seemingly different handling for PV vs >>> HVM. >> >> The difference is because access to foreign pages was hacked in to work >> for PVH in a much different (and imo yet more hacky) way than it was >> made work far longer ago for PV. The crucial part of that is in >> get_page_from_l1e(), so get_page_from_gfn() isn't that relevant there >> (I think). > > OK. I'm kind of surprised we need foreign handling for HVM and not > for PV in get_page_from_gfn(), as even with specific handling of > foreign pages in get_page_from_l1e(), the former is used in a lot of > places that don't seem obviously tied or gated to a call to > get_page_from_l1e(). > > For example the XEN_DOMCTL_getpageframeinfo3 usage of > get_page_from_gfn() will have different results when used against a > foreign page depending on whether the caller is a PV or an HVM > domain. Which imo is an unintended side effect of how foreign page support was added to p2m_get_page_from_gfn(). Considering what XEN_DOMCTL_getpageframeinfo3 is used for, I don't think foreign pages were intended to ever be observed there. A guest controlling another guest isn't assumed to be migrated, aiui. I expect more uses of get_page_from_gfn() can be found where foreign pages better wouldn't have "success" returned. Yet any of this is mere fallout from some callers apparently needing that case to work, and hence why logic was hacked into there. Maybe get_page_from_gfn() should take another P2M_FOREIGN flag, which only callers caring about foreign pages would set? Imo none of this is directly relevant for the patch at hand here - fast and slow paths differing is a problem no matter what. In fact I'm surprised no issues were ever reported that could be attributed back to that anomaly. Jan
On Thu, Mar 27, 2025 at 11:54:09AM +0100, Jan Beulich wrote: > On 27.03.2025 11:38, Roger Pau Monné wrote: > > On Thu, Mar 27, 2025 at 10:24:02AM +0100, Jan Beulich wrote: > >> On 27.03.2025 10:00, Roger Pau Monné wrote: > >>> On Tue, Mar 25, 2025 at 08:17:04AM +0100, Jan Beulich wrote: > >>>> Handling of both grants and foreign pages was different between the two > >>>> paths. > >>>> > >>>> While permitting access to grants would be desirable, doing so would > >>>> require more involved handling; undo that for the time being. In > >>>> particular the page reference obtained would prevent the owning domain > >>>> from changing e.g. the page's type (after the grantee has released the > >>>> last reference of the grant). Instead perhaps another reference on the > >>>> grant would need obtaining. Which in turn would require determining > >>>> which grant that was. > >>>> > >>>> Foreign pages in any event need permitting on both paths. > >>> > >>> I've been wondering about this, and AFAICT the PV equivalent of this > >>> function (the tail of get_page_from_gfn()) is much more limited in > >>> that it just allows access to domain owned RAM or MMIO that has a > >>> backing page (I expect mostly for the low 1MB?). > >>> > >>> However for HVM domains we allow to take references to foreign mapped > >>> pages and taking references to MMIO pages is not permitted. > >>> > >>> Should the tail of get_page_from_gfn() also be adjusted to make use of > >>> the newly introduced get_page_from_mfn_and_type(), thus unifying the > >>> logic for both PV and HVM? > >> > >> There's no (proper) use of P2M types in PV, so I don't think using this > >> function is viable for PV. In particular we'd never observe p2m_foreign > >> in PV, if I'm not mistaken. > > > > Indeed, p2m types are just occasionally faked for PV, like in > > get_page_from_gfn(). > > > >>> Could possibly be done in a separate change, I'm just trying to > >>> understand why we have this seemingly different handling for PV vs > >>> HVM. > >> > >> The difference is because access to foreign pages was hacked in to work > >> for PVH in a much different (and imo yet more hacky) way than it was > >> made work far longer ago for PV. The crucial part of that is in > >> get_page_from_l1e(), so get_page_from_gfn() isn't that relevant there > >> (I think). > > > > OK. I'm kind of surprised we need foreign handling for HVM and not > > for PV in get_page_from_gfn(), as even with specific handling of > > foreign pages in get_page_from_l1e(), the former is used in a lot of > > places that don't seem obviously tied or gated to a call to > > get_page_from_l1e(). > > > > For example the XEN_DOMCTL_getpageframeinfo3 usage of > > get_page_from_gfn() will have different results when used against a > > foreign page depending on whether the caller is a PV or an HVM > > domain. > > Which imo is an unintended side effect of how foreign page support was added > to p2m_get_page_from_gfn(). Considering what XEN_DOMCTL_getpageframeinfo3 is > used for, I don't think foreign pages were intended to ever be observed there. > A guest controlling another guest isn't assumed to be migrated, aiui. > > I expect more uses of get_page_from_gfn() can be found where foreign pages > better wouldn't have "success" returned. Yet any of this is mere fallout from > some callers apparently needing that case to work, and hence why logic was > hacked into there. Maybe get_page_from_gfn() should take another P2M_FOREIGN > flag, which only callers caring about foreign pages would set? > > Imo none of this is directly relevant for the patch at hand here - fast and > slow paths differing is a problem no matter what. In fact I'm surprised no > issues were ever reported that could be attributed back to that anomaly. Sorry, indeed, I didn't meant to imply that your patch caused any of this. It's just weirdness that I've observed while looking into this, and couldn't explain myself. I think the change is fine, but I find this slight behavior differences between PV and HVM confusing to say the least. IOW: I'm unsure why foreign pages need to be handled here for HVM and not for PV. As you say this is probably due to the different implementation of foreign pages in PV vs HVM. In any case, such diverging behavior should be fixed in a separate patch (if it does need fixing in hte first place). Thanks, Roger.
On Tue, Mar 25, 2025 at 08:17:04AM +0100, Jan Beulich wrote: > Handling of both grants and foreign pages was different between the two > paths. > > While permitting access to grants would be desirable, doing so would > require more involved handling; undo that for the time being. In > particular the page reference obtained would prevent the owning domain > from changing e.g. the page's type (after the grantee has released the > last reference of the grant). Instead perhaps another reference on the > grant would need obtaining. Which in turn would require determining > which grant that was. > > Foreign pages in any event need permitting on both paths. > > Introduce a helper function to be used on both paths, such that > respective checking differs in just the extra "to be unshared" condition > on the fast path. > > While there adjust the sanity check for foreign pages: Don't leak the > reference on release builds when on a debug build the assertion would > have triggered. (Thanks to Roger for the suggestion.) > > Fixes: 80ea7af17269 ("x86/mm: Introduce get_page_from_gfn()") > Fixes: 50fe6e737059 ("pvh dom0: add and remove foreign pages") > Fixes: cbbca7be4aaa ("x86/p2m: make p2m_get_page_from_gfn() handle grant case correctly") > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> Thanks, Roger.
--- a/xen/arch/x86/mm/p2m.c +++ b/xen/arch/x86/mm/p2m.c @@ -328,12 +328,46 @@ void p2m_put_gfn(struct p2m_domain *p2m, gfn_unlock(p2m, gfn_x(gfn), 0); } +static struct page_info *get_page_from_mfn_and_type( + const struct domain *d, mfn_t mfn, p2m_type_t t) +{ + struct page_info *page; + + if ( !mfn_valid(mfn) ) + return NULL; + + page = mfn_to_page(mfn); + + if ( p2m_is_ram(t) ) + { + if ( p2m_is_shared(t) ) + d = dom_cow; + + if ( get_page(page, d) ) + return page; + } + else if ( unlikely(p2m_is_foreign(t)) ) + { + const struct domain *fdom = page_get_owner_and_reference(page); + + if ( fdom ) + { + if ( likely(fdom != d) ) + return page; + ASSERT_UNREACHABLE(); + put_page(page); + } + } + + return NULL; +} + /* Atomically look up a GFN and take a reference count on the backing page. */ struct page_info *p2m_get_page_from_gfn( struct p2m_domain *p2m, gfn_t gfn, p2m_type_t *t, p2m_access_t *a, p2m_query_t q) { - struct page_info *page = NULL; + struct page_info *page; p2m_access_t _a; p2m_type_t _t; mfn_t mfn; @@ -347,26 +381,9 @@ struct page_info *p2m_get_page_from_gfn( /* Fast path: look up and get out */ p2m_read_lock(p2m); mfn = p2m_get_gfn_type_access(p2m, gfn, t, a, 0, NULL, 0); - if ( p2m_is_any_ram(*t) && mfn_valid(mfn) - && !((q & P2M_UNSHARE) && p2m_is_shared(*t)) ) - { - page = mfn_to_page(mfn); - if ( unlikely(p2m_is_foreign(*t)) || unlikely(p2m_is_grant(*t)) ) - { - struct domain *fdom = page_get_owner_and_reference(page); - - ASSERT(!p2m_is_foreign(*t) || fdom != p2m->domain); - if ( fdom == NULL ) - page = NULL; - } - else - { - struct domain *d = !p2m_is_shared(*t) ? p2m->domain : dom_cow; - - if ( !get_page(page, d) ) - page = NULL; - } - } + page = !(q & P2M_UNSHARE) || !p2m_is_shared(*t) + ? get_page_from_mfn_and_type(p2m->domain, mfn, *t) + : NULL; p2m_read_unlock(p2m); if ( page ) @@ -380,14 +397,7 @@ struct page_info *p2m_get_page_from_gfn( /* Slow path: take the write lock and do fixups */ mfn = get_gfn_type_access(p2m, gfn_x(gfn), t, a, q, NULL); - if ( p2m_is_ram(*t) && mfn_valid(mfn) ) - { - struct domain *d = !p2m_is_shared(*t) ? p2m->domain : dom_cow; - - page = mfn_to_page(mfn); - if ( !get_page(page, d) ) - page = NULL; - } + page = get_page_from_mfn_and_type(p2m->domain, mfn, *t); put_gfn(p2m->domain, gfn_x(gfn)); return page;
Handling of both grants and foreign pages was different between the two paths. While permitting access to grants would be desirable, doing so would require more involved handling; undo that for the time being. In particular the page reference obtained would prevent the owning domain from changing e.g. the page's type (after the grantee has released the last reference of the grant). Instead perhaps another reference on the grant would need obtaining. Which in turn would require determining which grant that was. Foreign pages in any event need permitting on both paths. Introduce a helper function to be used on both paths, such that respective checking differs in just the extra "to be unshared" condition on the fast path. While there adjust the sanity check for foreign pages: Don't leak the reference on release builds when on a debug build the assertion would have triggered. (Thanks to Roger for the suggestion.) Fixes: 80ea7af17269 ("x86/mm: Introduce get_page_from_gfn()") Fixes: 50fe6e737059 ("pvh dom0: add and remove foreign pages") Fixes: cbbca7be4aaa ("x86/p2m: make p2m_get_page_from_gfn() handle grant case correctly") Signed-off-by: Jan Beulich <jbeulich@suse.com> --- v2: Have helper take const struct domain *.