| Message ID | 20210225012243.28530-1-sstabellini@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen: introduce XENFEAT_direct_mapped and XENFEAT_not_direct_mapped | expand |
On 25.02.2021 02:22, Stefano Stabellini wrote: > --- a/xen/include/public/features.h > +++ b/xen/include/public/features.h > @@ -114,6 +114,13 @@ > */ > #define XENFEAT_linux_rsdp_unrestricted 15 > > +/* > + * A direct-mapped (or 1:1 mapped) domain is a domain for which its > + * local pages have gfn == mfn. > + */ > +#define XENFEAT_not_direct_mapped 16 > +#define XENFEAT_direct_mapped 17 Why two new values? Absence of XENFEAT_direct_mapped requires implying not-direct-mapped by the consumer anyway, doesn't it? Further, quoting xen/mm.h: "For a non-translated guest which is aware of Xen, gfn == mfn." This to me implies that PV would need to get XENFEAT_direct_mapped set; not sure whether this simply means x86'es is_domain_direct_mapped() is wrong, but if it is, uses elsewhere in the code would likely need changing. Also, nit: Please keep the right sides aligned with #define-s higher up in the file. Jan
On Thu, 25 Feb 2021, Jan Beulich wrote: > On 25.02.2021 02:22, Stefano Stabellini wrote: > > --- a/xen/include/public/features.h > > +++ b/xen/include/public/features.h > > @@ -114,6 +114,13 @@ > > */ > > #define XENFEAT_linux_rsdp_unrestricted 15 > > > > +/* > > + * A direct-mapped (or 1:1 mapped) domain is a domain for which its > > + * local pages have gfn == mfn. > > + */ > > +#define XENFEAT_not_direct_mapped 16 > > +#define XENFEAT_direct_mapped 17 > > Why two new values? Absence of XENFEAT_direct_mapped requires > implying not-direct-mapped by the consumer anyway, doesn't it? That's because if we add both flags we can avoid all unpleasant guessing games in the guest kernel. If one flag or the other flag is set, we can make an informed decision. But if neither flag is set, it means we are running on an older Xen, and we fall back on the current checks. > Further, quoting xen/mm.h: "For a non-translated guest which > is aware of Xen, gfn == mfn." This to me implies that PV would > need to get XENFEAT_direct_mapped set; not sure whether this > simply means x86'es is_domain_direct_mapped() is wrong, but if > it is, uses elsewhere in the code would likely need changing. That's a good point, I didn't think about x86 PV. I think the two flags are needed for autotranslated guests. I don't know for sure what is best for non-autotranslated guests. Maybe we could say that XENFEAT_not_direct_mapped and XENFEAT_direct_mapped only apply to XENFEAT_auto_translated_physmap guests. And it would match the implementation of is_domain_direct_mapped(). For non XENFEAT_auto_translated_physmap guests we could either do: - neither flag is set - set XENFEAT_direct_mapped (without changing the implementation of is_domain_direct_mapped) What do you think? I am happy either way. > Also, nit: Please keep the right sides aligned with #define-s > higher up in the file. OK
On 25.02.2021 21:51, Stefano Stabellini wrote: > On Thu, 25 Feb 2021, Jan Beulich wrote: >> On 25.02.2021 02:22, Stefano Stabellini wrote: >>> --- a/xen/include/public/features.h >>> +++ b/xen/include/public/features.h >>> @@ -114,6 +114,13 @@ >>> */ >>> #define XENFEAT_linux_rsdp_unrestricted 15 >>> >>> +/* >>> + * A direct-mapped (or 1:1 mapped) domain is a domain for which its >>> + * local pages have gfn == mfn. >>> + */ >>> +#define XENFEAT_not_direct_mapped 16 >>> +#define XENFEAT_direct_mapped 17 >> >> Why two new values? Absence of XENFEAT_direct_mapped requires >> implying not-direct-mapped by the consumer anyway, doesn't it? > > That's because if we add both flags we can avoid all unpleasant guessing > games in the guest kernel. > > If one flag or the other flag is set, we can make an informed decision. > > But if neither flag is set, it means we are running on an older Xen, > and we fall back on the current checks. Oh, okay - if there's guesswork to avoid, then I see the point. Maybe mention in the description? >> Further, quoting xen/mm.h: "For a non-translated guest which >> is aware of Xen, gfn == mfn." This to me implies that PV would >> need to get XENFEAT_direct_mapped set; not sure whether this >> simply means x86'es is_domain_direct_mapped() is wrong, but if >> it is, uses elsewhere in the code would likely need changing. > > That's a good point, I didn't think about x86 PV. I think the two flags > are needed for autotranslated guests. I don't know for sure what is best > for non-autotranslated guests. > > Maybe we could say that XENFEAT_not_direct_mapped and > XENFEAT_direct_mapped only apply to XENFEAT_auto_translated_physmap > guests. And it would match the implementation of > is_domain_direct_mapped(). I'm having trouble understanding this last sentence, and hence I'm not sure I understand the rest in the way you may mean it. Neither x86'es nor Arm's is_domain_direct_mapped() has any check towards a guest being translated (obviously such a check would be redundant on Arm). > For non XENFEAT_auto_translated_physmap guests we could either do: > > - neither flag is set > - set XENFEAT_direct_mapped (without changing the implementation of > is_domain_direct_mapped) > > What do you think? I am happy either way. I'm happy either way as well; suitably described perhaps setting XENFEAT_direct_mapped when !paging_mode_translate() would be slightly more "natural". But a spelled out and enforced dependency upon XENFEAT_auto_translated_physmap would too be fine with me. Jan
On Fri, 26 Feb 2021, Jan Beulich wrote: > On 25.02.2021 21:51, Stefano Stabellini wrote: > > On Thu, 25 Feb 2021, Jan Beulich wrote: > >> On 25.02.2021 02:22, Stefano Stabellini wrote: > >>> --- a/xen/include/public/features.h > >>> +++ b/xen/include/public/features.h > >>> @@ -114,6 +114,13 @@ > >>> */ > >>> #define XENFEAT_linux_rsdp_unrestricted 15 > >>> > >>> +/* > >>> + * A direct-mapped (or 1:1 mapped) domain is a domain for which its > >>> + * local pages have gfn == mfn. > >>> + */ > >>> +#define XENFEAT_not_direct_mapped 16 > >>> +#define XENFEAT_direct_mapped 17 > >> > >> Why two new values? Absence of XENFEAT_direct_mapped requires > >> implying not-direct-mapped by the consumer anyway, doesn't it? > > > > That's because if we add both flags we can avoid all unpleasant guessing > > games in the guest kernel. > > > > If one flag or the other flag is set, we can make an informed decision. > > > > But if neither flag is set, it means we are running on an older Xen, > > and we fall back on the current checks. > > Oh, okay - if there's guesswork to avoid, then I see the point. > Maybe mention in the description? Yes, I can do that. > >> Further, quoting xen/mm.h: "For a non-translated guest which > >> is aware of Xen, gfn == mfn." This to me implies that PV would > >> need to get XENFEAT_direct_mapped set; not sure whether this > >> simply means x86'es is_domain_direct_mapped() is wrong, but if > >> it is, uses elsewhere in the code would likely need changing. > > > > That's a good point, I didn't think about x86 PV. I think the two flags > > are needed for autotranslated guests. I don't know for sure what is best > > for non-autotranslated guests. > > > > Maybe we could say that XENFEAT_not_direct_mapped and > > XENFEAT_direct_mapped only apply to XENFEAT_auto_translated_physmap > > guests. And it would match the implementation of > > is_domain_direct_mapped(). > > I'm having trouble understanding this last sentence, and hence I'm > not sure I understand the rest in the way you may mean it. Neither > x86'es nor Arm's is_domain_direct_mapped() has any check towards a > guest being translated (obviously such a check would be redundant > on Arm). I meant that we are not explicitely checking for auto_translated in neither version of is_domain_direct_mapped(), but it is sort of implied. > > For non XENFEAT_auto_translated_physmap guests we could either do: > > > > - neither flag is set > > - set XENFEAT_direct_mapped (without changing the implementation of > > is_domain_direct_mapped) > > > > What do you think? I am happy either way. > > I'm happy either way as well; suitably described perhaps setting > XENFEAT_direct_mapped when !paging_mode_translate() would be > slightly more "natural". But a spelled out and enforced > dependency upon XENFEAT_auto_translated_physmap would too be fine > with me. OK. I'll go with: if ( is_domain_direct_mapped(d) || !paging_mode_translate(d) ) fi.submap |= (1U << XENFEAT_direct_mapped); else fi.submap |= (1U << XENFEAT_not_direct_mapped); With an appropriate explanation.
diff --git a/xen/common/kernel.c b/xen/common/kernel.c index 7a345ae45e..6ca1377dec 100644 --- a/xen/common/kernel.c +++ b/xen/common/kernel.c @@ -560,6 +560,10 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) (1U << XENFEAT_hvm_callback_vector) | (has_pirq(d) ? (1U << XENFEAT_hvm_pirqs) : 0); #endif + if ( is_domain_direct_mapped(d) ) + fi.submap |= (1U << XENFEAT_direct_mapped); + else + fi.submap |= (1U << XENFEAT_not_direct_mapped); break; default: return -EINVAL; diff --git a/xen/include/public/features.h b/xen/include/public/features.h index 1613b2aab8..18e3e61df0 100644 --- a/xen/include/public/features.h +++ b/xen/include/public/features.h @@ -114,6 +114,13 @@ */ #define XENFEAT_linux_rsdp_unrestricted 15 +/* + * A direct-mapped (or 1:1 mapped) domain is a domain for which its + * local pages have gfn == mfn. + */ +#define XENFEAT_not_direct_mapped 16 +#define XENFEAT_direct_mapped 17 + #define XENFEAT_NR_SUBMAPS 1 #endif /* __XEN_PUBLIC_FEATURES_H__ */
Introduce two feature flags to tell the domain whether it is direct-mapped or not. It allows the guest kernel to make informed decisions on things such as swiotlb-xen enablement. Currently, only Dom0 on ARM is direct-mapped, see: xen/include/asm-arm/domain.h:is_domain_direct_mapped xen/include/asm-x86/domain.h:is_domain_direct_mapped However, given that it is theoretically possible to have direct-mapped domains on x86 too, the two new feature flags are arch-neutral. Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com> CC: jbeulich@suse.com CC: andrew.cooper3@citrix.com CC: julien@xen.org --- xen/common/kernel.c | 4 ++++ xen/include/public/features.h | 7 +++++++ 2 files changed, 11 insertions(+)