| Message ID | 20200909145058.72066-1-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/hvm: don't treat MMIO pages as special ones regarding cache attributes | expand |
> -----Original Message----- > From: Roger Pau Monne <roger.pau@citrix.com> > Sent: 09 September 2020 15:51 > To: xen-devel@lists.xenproject.org > Cc: Roger Pau Monne <roger.pau@citrix.com>; Jan Beulich <jbeulich@suse.com>; Andrew Cooper > <andrew.cooper3@citrix.com>; Wei Liu <wl@xen.org>; Paul Durrant <paul@xen.org> > Subject: [PATCH] x86/hvm: don't treat MMIO pages as special ones regarding cache attributes > > MMIO regions below the maximum address on the memory map can have a > backing page struct that's shared with dom_io (see x86 > arch_init_memory and it's usage of share_xen_page_with_guest), and > thus also fulfill the is_special_page check because the page has the > Xen heap bit set. > > This is incorrect for MMIO regions when is_special_page is used by > epte_get_entry_emt, as it will force direct MMIO regions mapped into > the guest p2m to have the cache attributes set to write-back. > > Add an extra check in epte_get_entry_emt in order to detect pages > shared with dom_io (ie: MMIO regions) and don't force them to > write-back cache type on that case. > > Fixes: 81fd0d3ca4b2cd ('x86/hvm: simplify 'mmio_direct' check in epte_get_entry_emt()') > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > --- > Cc: Paul Durrant <paul@xen.org> This looks like the right thing to do... Reviewed-by: Paul Durrant <paul@xen.org>
On 09.09.2020 16:50, Roger Pau Monne wrote: > MMIO regions below the maximum address on the memory map can have a > backing page struct that's shared with dom_io (see x86 > arch_init_memory and it's usage of share_xen_page_with_guest), and > thus also fulfill the is_special_page check because the page has the > Xen heap bit set. > > This is incorrect for MMIO regions when is_special_page is used by > epte_get_entry_emt, as it will force direct MMIO regions mapped into > the guest p2m to have the cache attributes set to write-back. > > Add an extra check in epte_get_entry_emt in order to detect pages > shared with dom_io (ie: MMIO regions) and don't force them to > write-back cache type on that case. Did you consider the alternative of not marking those pages as Xen heap ones? In particular when looking at it from this angle I consider it at least odd for non-RAM (or more precisely non-heap) pages to get marked this way. And I can't currently see anything requiring them to be marked as such - them being owned by DomIO is all that's needed as it seems. Jan
On Thu, Sep 10, 2020 at 11:27:49AM +0200, Jan Beulich wrote: > On 09.09.2020 16:50, Roger Pau Monne wrote: > > MMIO regions below the maximum address on the memory map can have a > > backing page struct that's shared with dom_io (see x86 > > arch_init_memory and it's usage of share_xen_page_with_guest), and > > thus also fulfill the is_special_page check because the page has the > > Xen heap bit set. > > > > This is incorrect for MMIO regions when is_special_page is used by > > epte_get_entry_emt, as it will force direct MMIO regions mapped into > > the guest p2m to have the cache attributes set to write-back. > > > > Add an extra check in epte_get_entry_emt in order to detect pages > > shared with dom_io (ie: MMIO regions) and don't force them to > > write-back cache type on that case. > > Did you consider the alternative of not marking those pages as Xen > heap ones? In particular when looking at it from this angle I > consider it at least odd for non-RAM (or more precisely non-heap) > pages to get marked this way. I wasn't sure whether this could cause issues in other places of the code that would rely on this fact and such change seemed more risky IMO. > And I can't currently see anything > requiring them to be marked as such - them being owned by DomIO is > all that's needed as it seems. Should those pages then simply be assigned to dom_io and set the appropriate flags (PGC_allocated | 1), or should share_xen_page_with_guest be modified to not set the PGC_xen_heap flag? I see that such addition was done in a2b4b8c2041, but I'm afraid I don't fully understand why share_xen_page_with_guest needs to mark pages as Xen heap. Thanks, Roger.
> -----Original Message----- > From: Roger Pau Monné <roger.pau@citrix.com> > Sent: 10 September 2020 11:35 > To: Jan Beulich <jbeulich@suse.com> > Cc: xen-devel@lists.xenproject.org; Andrew Cooper <andrew.cooper3@citrix.com>; Wei Liu <wl@xen.org>; > Paul Durrant <paul@xen.org> > Subject: Re: [PATCH] x86/hvm: don't treat MMIO pages as special ones regarding cache attributes > > On Thu, Sep 10, 2020 at 11:27:49AM +0200, Jan Beulich wrote: > > On 09.09.2020 16:50, Roger Pau Monne wrote: > > > MMIO regions below the maximum address on the memory map can have a > > > backing page struct that's shared with dom_io (see x86 > > > arch_init_memory and it's usage of share_xen_page_with_guest), and > > > thus also fulfill the is_special_page check because the page has the > > > Xen heap bit set. > > > > > > This is incorrect for MMIO regions when is_special_page is used by > > > epte_get_entry_emt, as it will force direct MMIO regions mapped into > > > the guest p2m to have the cache attributes set to write-back. > > > > > > Add an extra check in epte_get_entry_emt in order to detect pages > > > shared with dom_io (ie: MMIO regions) and don't force them to > > > write-back cache type on that case. > > > > Did you consider the alternative of not marking those pages as Xen > > heap ones? In particular when looking at it from this angle I > > consider it at least odd for non-RAM (or more precisely non-heap) > > pages to get marked this way. > > I wasn't sure whether this could cause issues in other places of the > code that would rely on this fact and such change seemed more risky > IMO. > > > And I can't currently see anything > > requiring them to be marked as such - them being owned by DomIO is > > all that's needed as it seems. > > Should those pages then simply be assigned to dom_io and set the > appropriate flags (PGC_allocated | 1), or should > share_xen_page_with_guest be modified to not set the PGC_xen_heap > flag? > > I see that such addition was done in a2b4b8c2041, but I'm afraid I > don't fully understand why share_xen_page_with_guest needs to mark > pages as Xen heap. > In general they are not marked Xen heap then they will be considered domheap and will go through the normal free-ing path on domain destruction. Of course this doesn't apply for a system domain that never gets destroyed. Paul
On 10.09.2020 12:34, Roger Pau Monné wrote: > On Thu, Sep 10, 2020 at 11:27:49AM +0200, Jan Beulich wrote: >> On 09.09.2020 16:50, Roger Pau Monne wrote: >>> MMIO regions below the maximum address on the memory map can have a >>> backing page struct that's shared with dom_io (see x86 >>> arch_init_memory and it's usage of share_xen_page_with_guest), and >>> thus also fulfill the is_special_page check because the page has the >>> Xen heap bit set. >>> >>> This is incorrect for MMIO regions when is_special_page is used by >>> epte_get_entry_emt, as it will force direct MMIO regions mapped into >>> the guest p2m to have the cache attributes set to write-back. >>> >>> Add an extra check in epte_get_entry_emt in order to detect pages >>> shared with dom_io (ie: MMIO regions) and don't force them to >>> write-back cache type on that case. >> >> Did you consider the alternative of not marking those pages as Xen >> heap ones? In particular when looking at it from this angle I >> consider it at least odd for non-RAM (or more precisely non-heap) >> pages to get marked this way. > > I wasn't sure whether this could cause issues in other places of the > code that would rely on this fact and such change seemed more risky > IMO. As said - I don't think there is, but I've not done a full audit. >> And I can't currently see anything >> requiring them to be marked as such - them being owned by DomIO is >> all that's needed as it seems. > > Should those pages then simply be assigned to dom_io and set the > appropriate flags (PGC_allocated | 1), or should > share_xen_page_with_guest be modified to not set the PGC_xen_heap > flag? Either approach may be okay, I think. It would really depend on how little of share_xen_page_with_guest() suffices for the dom_io assignment. > I see that such addition was done in a2b4b8c2041, but I'm afraid I > don't fully understand why share_xen_page_with_guest needs to mark > pages as Xen heap. I see Paul has already answer this part. You can't drop the setting of the flag, but you could make it dependent upon d != dom_io. Jan
On Thu, Sep 10, 2020 at 11:53:10AM +0100, Paul Durrant wrote: > > -----Original Message----- > > From: Roger Pau Monné <roger.pau@citrix.com> > > Sent: 10 September 2020 11:35 > > To: Jan Beulich <jbeulich@suse.com> > > Cc: xen-devel@lists.xenproject.org; Andrew Cooper <andrew.cooper3@citrix.com>; Wei Liu <wl@xen.org>; > > Paul Durrant <paul@xen.org> > > Subject: Re: [PATCH] x86/hvm: don't treat MMIO pages as special ones regarding cache attributes > > > > On Thu, Sep 10, 2020 at 11:27:49AM +0200, Jan Beulich wrote: > > > On 09.09.2020 16:50, Roger Pau Monne wrote: > > > > MMIO regions below the maximum address on the memory map can have a > > > > backing page struct that's shared with dom_io (see x86 > > > > arch_init_memory and it's usage of share_xen_page_with_guest), and > > > > thus also fulfill the is_special_page check because the page has the > > > > Xen heap bit set. > > > > > > > > This is incorrect for MMIO regions when is_special_page is used by > > > > epte_get_entry_emt, as it will force direct MMIO regions mapped into > > > > the guest p2m to have the cache attributes set to write-back. > > > > > > > > Add an extra check in epte_get_entry_emt in order to detect pages > > > > shared with dom_io (ie: MMIO regions) and don't force them to > > > > write-back cache type on that case. > > > > > > Did you consider the alternative of not marking those pages as Xen > > > heap ones? In particular when looking at it from this angle I > > > consider it at least odd for non-RAM (or more precisely non-heap) > > > pages to get marked this way. > > > > I wasn't sure whether this could cause issues in other places of the > > code that would rely on this fact and such change seemed more risky > > IMO. > > > > > And I can't currently see anything > > > requiring them to be marked as such - them being owned by DomIO is > > > all that's needed as it seems. > > > > Should those pages then simply be assigned to dom_io and set the > > appropriate flags (PGC_allocated | 1), or should > > share_xen_page_with_guest be modified to not set the PGC_xen_heap > > flag? > > > > I see that such addition was done in a2b4b8c2041, but I'm afraid I > > don't fully understand why share_xen_page_with_guest needs to mark > > pages as Xen heap. > > > > In general they are not marked Xen heap then they will be considered domheap and will go through the normal free-ing path on domain destruction. Of course this doesn't apply for a system domain that never gets destroyed. Hm, OK, the original commit (a2b4b8c2041) mentions that marking them as Xen heap is done to signal that the virtual address for those is not needed (and not available?). For the MMIO regions I'm not sure it matters much, since those are not assigned to the domain, but just mapped into it. The MMIO regions are not shared with the domain accessing them, but rather with dom_io. It's still not clear to me what option would be better: modify share_xen_page_with_guest to not mark pages as Xen heap, or implement something different to assign MMIO pages to dom_io without setting the Xen heap flag. Thanks, Roger.
> -----Original Message----- > From: Roger Pau Monné <roger.pau@citrix.com> > Sent: 10 September 2020 12:06 > To: paul@xen.org > Cc: 'Jan Beulich' <jbeulich@suse.com>; xen-devel@lists.xenproject.org; 'Andrew Cooper' > <andrew.cooper3@citrix.com>; 'Wei Liu' <wl@xen.org> > Subject: Re: [PATCH] x86/hvm: don't treat MMIO pages as special ones regarding cache attributes > > On Thu, Sep 10, 2020 at 11:53:10AM +0100, Paul Durrant wrote: > > > -----Original Message----- > > > From: Roger Pau Monné <roger.pau@citrix.com> > > > Sent: 10 September 2020 11:35 > > > To: Jan Beulich <jbeulich@suse.com> > > > Cc: xen-devel@lists.xenproject.org; Andrew Cooper <andrew.cooper3@citrix.com>; Wei Liu > <wl@xen.org>; > > > Paul Durrant <paul@xen.org> > > > Subject: Re: [PATCH] x86/hvm: don't treat MMIO pages as special ones regarding cache attributes > > > > > > On Thu, Sep 10, 2020 at 11:27:49AM +0200, Jan Beulich wrote: > > > > On 09.09.2020 16:50, Roger Pau Monne wrote: > > > > > MMIO regions below the maximum address on the memory map can have a > > > > > backing page struct that's shared with dom_io (see x86 > > > > > arch_init_memory and it's usage of share_xen_page_with_guest), and > > > > > thus also fulfill the is_special_page check because the page has the > > > > > Xen heap bit set. > > > > > > > > > > This is incorrect for MMIO regions when is_special_page is used by > > > > > epte_get_entry_emt, as it will force direct MMIO regions mapped into > > > > > the guest p2m to have the cache attributes set to write-back. > > > > > > > > > > Add an extra check in epte_get_entry_emt in order to detect pages > > > > > shared with dom_io (ie: MMIO regions) and don't force them to > > > > > write-back cache type on that case. > > > > > > > > Did you consider the alternative of not marking those pages as Xen > > > > heap ones? In particular when looking at it from this angle I > > > > consider it at least odd for non-RAM (or more precisely non-heap) > > > > pages to get marked this way. > > > > > > I wasn't sure whether this could cause issues in other places of the > > > code that would rely on this fact and such change seemed more risky > > > IMO. > > > > > > > And I can't currently see anything > > > > requiring them to be marked as such - them being owned by DomIO is > > > > all that's needed as it seems. > > > > > > Should those pages then simply be assigned to dom_io and set the > > > appropriate flags (PGC_allocated | 1), or should > > > share_xen_page_with_guest be modified to not set the PGC_xen_heap > > > flag? > > > > > > I see that such addition was done in a2b4b8c2041, but I'm afraid I > > > don't fully understand why share_xen_page_with_guest needs to mark > > > pages as Xen heap. > > > > > > > In general they are not marked Xen heap then they will be considered domheap and will go through the > normal free-ing path on domain destruction. Of course this doesn't apply for a system domain that > never gets destroyed. > > Hm, OK, the original commit (a2b4b8c2041) mentions that marking them > as Xen heap is done to signal that the virtual address for those is > not needed (and not available?). > > For the MMIO regions I'm not sure it matters much, since those are not > assigned to the domain, but just mapped into it. The MMIO regions are > not shared with the domain accessing them, but rather with dom_io. > > It's still not clear to me what option would be better: modify > share_xen_page_with_guest to not mark pages as Xen heap, or implement > something different to assign MMIO pages to dom_io without setting > the Xen heap flag. Would using map_mmio_regions() work? Paul > > Thanks, Roger.
On Thu, Sep 10, 2020 at 12:13:55PM +0100, Paul Durrant wrote: > > -----Original Message----- > > From: Roger Pau Monné <roger.pau@citrix.com> > > Sent: 10 September 2020 12:06 > > To: paul@xen.org > > Cc: 'Jan Beulich' <jbeulich@suse.com>; xen-devel@lists.xenproject.org; 'Andrew Cooper' > > <andrew.cooper3@citrix.com>; 'Wei Liu' <wl@xen.org> > > Subject: Re: [PATCH] x86/hvm: don't treat MMIO pages as special ones regarding cache attributes > > > > On Thu, Sep 10, 2020 at 11:53:10AM +0100, Paul Durrant wrote: > > > > -----Original Message----- > > > > From: Roger Pau Monné <roger.pau@citrix.com> > > > > Sent: 10 September 2020 11:35 > > > > To: Jan Beulich <jbeulich@suse.com> > > > > Cc: xen-devel@lists.xenproject.org; Andrew Cooper <andrew.cooper3@citrix.com>; Wei Liu > > <wl@xen.org>; > > > > Paul Durrant <paul@xen.org> > > > > Subject: Re: [PATCH] x86/hvm: don't treat MMIO pages as special ones regarding cache attributes > > > > > > > > On Thu, Sep 10, 2020 at 11:27:49AM +0200, Jan Beulich wrote: > > > > > On 09.09.2020 16:50, Roger Pau Monne wrote: > > > > > > MMIO regions below the maximum address on the memory map can have a > > > > > > backing page struct that's shared with dom_io (see x86 > > > > > > arch_init_memory and it's usage of share_xen_page_with_guest), and > > > > > > thus also fulfill the is_special_page check because the page has the > > > > > > Xen heap bit set. > > > > > > > > > > > > This is incorrect for MMIO regions when is_special_page is used by > > > > > > epte_get_entry_emt, as it will force direct MMIO regions mapped into > > > > > > the guest p2m to have the cache attributes set to write-back. > > > > > > > > > > > > Add an extra check in epte_get_entry_emt in order to detect pages > > > > > > shared with dom_io (ie: MMIO regions) and don't force them to > > > > > > write-back cache type on that case. > > > > > > > > > > Did you consider the alternative of not marking those pages as Xen > > > > > heap ones? In particular when looking at it from this angle I > > > > > consider it at least odd for non-RAM (or more precisely non-heap) > > > > > pages to get marked this way. > > > > > > > > I wasn't sure whether this could cause issues in other places of the > > > > code that would rely on this fact and such change seemed more risky > > > > IMO. > > > > > > > > > And I can't currently see anything > > > > > requiring them to be marked as such - them being owned by DomIO is > > > > > all that's needed as it seems. > > > > > > > > Should those pages then simply be assigned to dom_io and set the > > > > appropriate flags (PGC_allocated | 1), or should > > > > share_xen_page_with_guest be modified to not set the PGC_xen_heap > > > > flag? > > > > > > > > I see that such addition was done in a2b4b8c2041, but I'm afraid I > > > > don't fully understand why share_xen_page_with_guest needs to mark > > > > pages as Xen heap. > > > > > > > > > > In general they are not marked Xen heap then they will be considered domheap and will go through the > > normal free-ing path on domain destruction. Of course this doesn't apply for a system domain that > > never gets destroyed. > > > > Hm, OK, the original commit (a2b4b8c2041) mentions that marking them > > as Xen heap is done to signal that the virtual address for those is > > not needed (and not available?). > > > > For the MMIO regions I'm not sure it matters much, since those are not > > assigned to the domain, but just mapped into it. The MMIO regions are > > not shared with the domain accessing them, but rather with dom_io. > > > > It's still not clear to me what option would be better: modify > > share_xen_page_with_guest to not mark pages as Xen heap, or implement > > something different to assign MMIO pages to dom_io without setting > > the Xen heap flag. > > Would using map_mmio_regions() work? If you mean using map_mmio_regions against dom_io, then no, that won't work because dom_io is not a translated domain so that would be a noop. Also I don't think map_mmio_regions takes any reference or changes the ownership of MMIO pages, so even if it could be used against a non-translated domain it's likely not what we want. Thanks, Roger.
On 10.09.2020 13:05, Roger Pau Monné wrote: > It's still not clear to me what option would be better: modify > share_xen_page_with_guest to not mark pages as Xen heap, or implement > something different to assign MMIO pages to dom_io without setting > the Xen heap flag. static void __init share_io_page(struct page_info *page) { set_gpfn_from_mfn(mfn_x(page_to_mfn(page)), INVALID_M2P_ENTRY); /* The incremented type count pins as writable. */ page->u.inuse.type_info = PGT_writable_page | PGT_validated | 1; page_set_owner(page, dom_io); page->count_info |= PGC_allocated | 1; } is of course much shorter than share_xen_page_with_guest(), but I'm nevertheless uncertain whether simply making conditional the setting of PGC_xen_heap there isn't the easier route. Of course, not pointlessly acquiring and releasing a lock has its own appeal. Jan
On Thu, Sep 10, 2020 at 01:28:44PM +0200, Jan Beulich wrote: > On 10.09.2020 13:05, Roger Pau Monné wrote: > > It's still not clear to me what option would be better: modify > > share_xen_page_with_guest to not mark pages as Xen heap, or implement > > something different to assign MMIO pages to dom_io without setting > > the Xen heap flag. > > static void __init share_io_page(struct page_info *page) > { > set_gpfn_from_mfn(mfn_x(page_to_mfn(page)), INVALID_M2P_ENTRY); > > /* The incremented type count pins as writable. */ > page->u.inuse.type_info = PGT_writable_page | PGT_validated | 1; > > page_set_owner(page, dom_io); > > page->count_info |= PGC_allocated | 1; > } > > is of course much shorter than share_xen_page_with_guest(), but > I'm nevertheless uncertain whether simply making conditional > the setting of PGC_xen_heap there isn't the easier route. Of > course, not pointlessly acquiring and releasing a lock has its > own appeal. I've went over the existing is_special_page users and I think it's fine for MMIO regions to not be marked as special pages. Will send a new patch. Roger.
diff --git a/xen/arch/x86/hvm/mtrr.c b/xen/arch/x86/hvm/mtrr.c index fb051d59c3..33b1dd9052 100644 --- a/xen/arch/x86/hvm/mtrr.c +++ b/xen/arch/x86/hvm/mtrr.c @@ -829,7 +829,9 @@ int epte_get_entry_emt(struct domain *d, unsigned long gfn, mfn_t mfn, for ( i = 0; i < (1ul << order); i++ ) { - if ( is_special_page(mfn_to_page(mfn_add(mfn, i))) ) + const struct page_info *page = mfn_to_page(mfn_add(mfn, i)); + + if ( is_special_page(page) && page_get_owner(page) != dom_io ) { if ( order ) return -1;
MMIO regions below the maximum address on the memory map can have a backing page struct that's shared with dom_io (see x86 arch_init_memory and it's usage of share_xen_page_with_guest), and thus also fulfill the is_special_page check because the page has the Xen heap bit set. This is incorrect for MMIO regions when is_special_page is used by epte_get_entry_emt, as it will force direct MMIO regions mapped into the guest p2m to have the cache attributes set to write-back. Add an extra check in epte_get_entry_emt in order to detect pages shared with dom_io (ie: MMIO regions) and don't force them to write-back cache type on that case. Fixes: 81fd0d3ca4b2cd ('x86/hvm: simplify 'mmio_direct' check in epte_get_entry_emt()') Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- Cc: Paul Durrant <paul@xen.org> --- xen/arch/x86/hvm/mtrr.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)