| Message ID | 64d81a49e5e00527ae01b707080f6f0e14ee667c.1671497984.git.demi@invisiblethingslab.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Make PAT handling less brittle | expand |
On 20.12.2022 02:07, Demi Marie Obenour wrote: > --- a/xen/arch/x86/mm.c > +++ b/xen/arch/x86/mm.c > @@ -145,6 +145,8 @@ > > #ifdef CONFIG_PV > #include "pv/mm.h" > +bool allow_invalid_cacheability; static and __ro_after_init please (the former not the least with Misra in mind). > +boolean_param("allow_invalid_cacheability", allow_invalid_cacheability); > #endif Any new command line option will need to come with an entry in docs/misc/xen-command-line.pandoc. Furthermore we're trying to avoid underscores in command line option names, when dashes serve the purpose quite fine. Also please add a blank line at least between #include and your addition. Personally I would find it more natural if such a single-use control was defined next to the place it is used, i.e. > @@ -1343,7 +1345,33 @@ static int promote_l1_table(struct page_info *page) > } > else > { ... here. > - switch ( ret = get_page_from_l1e(pl1e[i], d, d) ) > + l1_pgentry_t l1e = pl1e[i]; > + > + if ( !allow_invalid_cacheability ) > + { > + switch ( l1e.l1 & PAGE_CACHE_ATTRS ) > + { > + case _PAGE_WB: > + case _PAGE_UC: > + case _PAGE_UCM: > + case _PAGE_WC: > + case _PAGE_WT: > + case _PAGE_WP: > + break; > + default: Nit (style): Blank line please between non-fall-through case blocks. > + /* > + * If we get here, a PV guest tried to use one of the > + * reserved values in Xen's PAT. This indicates a bug in > + * the guest, so inject #GP to cause the guest to log a > + * stack trace. > + */ And likely make it die. Which, yes, ... > + pv_inject_hw_exception(TRAP_gp_fault, 0); > + ret = -EINVAL; ... also may or may not be the result of returning failure here (if the guest "checks" for errors by using a BUG()-like construct), but that's then more of its own fault than not coping with a non-architectural #GP. Also wasn't there talk of having this behavior in debug hypervisors only? Without that restriction I'm yet less happy with the change ... Jan
On Thu, Dec 22, 2022 at 10:48:02AM +0100, Jan Beulich wrote: > On 20.12.2022 02:07, Demi Marie Obenour wrote: > > --- a/xen/arch/x86/mm.c > > +++ b/xen/arch/x86/mm.c > > @@ -145,6 +145,8 @@ > > > > #ifdef CONFIG_PV > > #include "pv/mm.h" > > +bool allow_invalid_cacheability; > > static and __ro_after_init please (the former not the least with Misra > in mind). Will fix in v6. > > +boolean_param("allow_invalid_cacheability", allow_invalid_cacheability); > > #endif > > Any new command line option will need to come with an entry in > docs/misc/xen-command-line.pandoc. Furthermore we're trying to avoid > underscores in command line option names, when dashes serve the > purpose quite fine. Will fix in v6. > Also please add a blank line at least between #include and your > addition. Personally I would find it more natural if such a single-use > control was defined next to the place it is used, i.e. Will fix in v6. > > @@ -1343,7 +1345,33 @@ static int promote_l1_table(struct page_info *page) > > } > > else > > { > > ... here. Will move in v6. > > - switch ( ret = get_page_from_l1e(pl1e[i], d, d) ) > > + l1_pgentry_t l1e = pl1e[i]; > > + > > + if ( !allow_invalid_cacheability ) > > + { > > + switch ( l1e.l1 & PAGE_CACHE_ATTRS ) > > + { > > + case _PAGE_WB: > > + case _PAGE_UC: > > + case _PAGE_UCM: > > + case _PAGE_WC: > > + case _PAGE_WT: > > + case _PAGE_WP: > > + break; > > + default: > > Nit (style): Blank line please between non-fall-through case blocks. Will fix in v6. > > + /* > > + * If we get here, a PV guest tried to use one of the > > + * reserved values in Xen's PAT. This indicates a bug in > > + * the guest, so inject #GP to cause the guest to log a > > + * stack trace. > > + */ > > And likely make it die. Which, yes, ... > > > + pv_inject_hw_exception(TRAP_gp_fault, 0); > > + ret = -EINVAL; > > ... also may or may not be the result of returning failure here (if the > guest "checks" for errors by using a BUG()-like construct), but that's > then more of its own fault than not coping with a non-architectural #GP. Is there really any architectural behavior here? > Also wasn't there talk of having this behavior in debug hypervisors > only? Without that restriction I'm yet less happy with the change ... My understanding was that Andrew preferred the behavior to be turned on in release hypervisors too. I am inclined to agree with Andrew, if for no other reason than that those working on guest OSs do not generally use debug hypervisors if they are not also working on Xen itself.
On 22.12.2022 10:55, Demi Marie Obenour wrote: > On Thu, Dec 22, 2022 at 10:48:02AM +0100, Jan Beulich wrote: >> Also wasn't there talk of having this behavior in debug hypervisors >> only? Without that restriction I'm yet less happy with the change ... > > My understanding was that Andrew preferred the behavior to be turned on > in release hypervisors too. I am inclined to agree with Andrew, if for > no other reason than that those working on guest OSs do not generally > use debug hypervisors if they are not also working on Xen itself. That's likely a mistake then, at least for work on PV guest OSes. In particular PV memory management code is sprinkled with gdprintk(), in an attempt to help those people understand why some operation has failed. Jan
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c index a72556668633ee57b77c9a57d3a13dd5a12d9bbf..69ce597c7cd5283ae4b5f3bc0a6dfa0bb3228d3d 100644 --- a/xen/arch/x86/mm.c +++ b/xen/arch/x86/mm.c @@ -145,6 +145,8 @@ #ifdef CONFIG_PV #include "pv/mm.h" +bool allow_invalid_cacheability; +boolean_param("allow_invalid_cacheability", allow_invalid_cacheability); #endif /* Override macros from asm/page.h to make them work with mfn_t */ @@ -1343,7 +1345,33 @@ static int promote_l1_table(struct page_info *page) } else { - switch ( ret = get_page_from_l1e(pl1e[i], d, d) ) + l1_pgentry_t l1e = pl1e[i]; + + if ( !allow_invalid_cacheability ) + { + switch ( l1e.l1 & PAGE_CACHE_ATTRS ) + { + case _PAGE_WB: + case _PAGE_UC: + case _PAGE_UCM: + case _PAGE_WC: + case _PAGE_WT: + case _PAGE_WP: + break; + default: + /* + * If we get here, a PV guest tried to use one of the + * reserved values in Xen's PAT. This indicates a bug in + * the guest, so inject #GP to cause the guest to log a + * stack trace. + */ + pv_inject_hw_exception(TRAP_gp_fault, 0); + ret = -EINVAL; + goto fail; + } + } + + switch ( ret = get_page_from_l1e(l1e, d, d) ) { default: goto fail;
Setting cacheability flags that are not ones specified by Xen is a bug in the guest. By default, inject #GP into any guest that does this. allow_invalid_cacheability can be used on the Xen command line to disable this check. Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com> Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com> --- Changes since v4: - Remove pointless BUILD_BUG_ON(). - Add comment explaining why an exception is being injected. Changes since v3: - Add Andrew Cooper’s Suggested-by --- xen/arch/x86/mm.c | 30 +++++++++++++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-)