| Message ID | 175df1a2-a95f-462b-ad49-3a0fef727658@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/EPT: avoid undue forcing of MMIO accesses to UC | expand |
On Wed, Jun 12, 2024 at 03:16:37PM +0200, Jan Beulich wrote: > mfn_valid() granularity is (currently) 256Mb. Therefore the start of a > 1Gb page passing the test doesn't necessarily mean all parts of such a > range would also pass. How would such a superpage end up in the EPT? I would assume this can only happen when adding a superpage MMIO that has part of it return success from mfn_valid()? > Yet using the result of mfn_to_page() on an MFN > which doesn't pass mfn_valid() checking is liable to result in a crash > (the invocation of mfn_to_page() alone is presumably "just" UB in such a > case). > > Fixes: ca24b2ffdbd9 ("x86/hvm: set 'ipat' in EPT for special pages") > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> > --- > Of course we could leverage mfn_valid() granularity here to do an > increment by more than 1 if mfn_valid() returned false. Yet doing so > likely would want a suitable helper to be introduced first, rather than > open-coding such logic here. We would still need to call is_special_page() on each 4K chunk, at which point taking advantage of the mfn_valid() granularity is likely to make the code more complicated to follow IMO. Thanks, Roger.
On 12.06.2024 16:11, Roger Pau Monné wrote: > On Wed, Jun 12, 2024 at 03:16:37PM +0200, Jan Beulich wrote: >> mfn_valid() granularity is (currently) 256Mb. Therefore the start of a >> 1Gb page passing the test doesn't necessarily mean all parts of such a >> range would also pass. > > How would such a superpage end up in the EPT? > > I would assume this can only happen when adding a superpage MMIO that > has part of it return success from mfn_valid()? Yes, that's the only way I can think of. >> Yet using the result of mfn_to_page() on an MFN >> which doesn't pass mfn_valid() checking is liable to result in a crash >> (the invocation of mfn_to_page() alone is presumably "just" UB in such a >> case). >> >> Fixes: ca24b2ffdbd9 ("x86/hvm: set 'ipat' in EPT for special pages") >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> Thanks. >> --- >> Of course we could leverage mfn_valid() granularity here to do an >> increment by more than 1 if mfn_valid() returned false. Yet doing so >> likely would want a suitable helper to be introduced first, rather than >> open-coding such logic here. > > We would still need to call is_special_page() on each 4K chunk, Why? Within any block for which mfn_valid() returns false, there can be no RAM pages and hence also no special ones. It's only blocks where mfn_valid() returns true that we'd need to iterate through page-by-page. > at > which point taking advantage of the mfn_valid() granularity is likely > to make the code more complicated to follow IMO. Right, this making it more complicated is the main counter argument. Hence why I think that if to go such a route at all, it would need some new helper(s) such that at the use sites things still would remain reasonably clear. Jan
On Wed, Jun 12, 2024 at 04:47:12PM +0200, Jan Beulich wrote: > On 12.06.2024 16:11, Roger Pau Monné wrote: > > On Wed, Jun 12, 2024 at 03:16:37PM +0200, Jan Beulich wrote: > >> mfn_valid() granularity is (currently) 256Mb. Therefore the start of a > >> 1Gb page passing the test doesn't necessarily mean all parts of such a > >> range would also pass. > > > > How would such a superpage end up in the EPT? > > > > I would assume this can only happen when adding a superpage MMIO that > > has part of it return success from mfn_valid()? > > Yes, that's the only way I can think of. > > >> Yet using the result of mfn_to_page() on an MFN > >> which doesn't pass mfn_valid() checking is liable to result in a crash > >> (the invocation of mfn_to_page() alone is presumably "just" UB in such a > >> case). > >> > >> Fixes: ca24b2ffdbd9 ("x86/hvm: set 'ipat' in EPT for special pages") > >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > > > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> > > Thanks. > > >> --- > >> Of course we could leverage mfn_valid() granularity here to do an > >> increment by more than 1 if mfn_valid() returned false. Yet doing so > >> likely would want a suitable helper to be introduced first, rather than > >> open-coding such logic here. > > > > We would still need to call is_special_page() on each 4K chunk, > > Why? Within any block for which mfn_valid() returns false, there can be > no RAM pages and hence also no special ones. It's only blocks where > mfn_valid() returns true that we'd need to iterate through page-by-page. Oh right, I was thinking the other way around (mfn_valid() returning true), never mind. > > at > > which point taking advantage of the mfn_valid() granularity is likely > > to make the code more complicated to follow IMO. > > Right, this making it more complicated is the main counter argument. Hence > why I think that if to go such a route at all, it would need some new > helper(s) such that at the use sites things still would remain reasonably > clear. We could also add an extra check to exit the loop early if special pages have been found but don't match the current loop index, as it's all special pages or none. Thanks, Roger.
On 12.06.2024 17:02, Roger Pau Monné wrote: > We could also add an extra check to exit the loop early if special > pages have been found but don't match the current loop index, as it's > all special pages or none. I was actually considering to make such a change, but then concluded that in the common case there'll be no special pages anyway, and hence we need to run the loop to completion anyway. Jan
On Wed, 2024-06-12 at 15:16 +0200, Jan Beulich wrote: > mfn_valid() granularity is (currently) 256Mb. Therefore the start of > a > 1Gb page passing the test doesn't necessarily mean all parts of such > a > range would also pass. Yet using the result of mfn_to_page() on an > MFN > which doesn't pass mfn_valid() checking is liable to result in a > crash > (the invocation of mfn_to_page() alone is presumably "just" UB in > such a > case). > > Fixes: ca24b2ffdbd9 ("x86/hvm: set 'ipat' in EPT for special pages") > Signed-off-by: Jan Beulich <jbeulich@suse.com> Release-Acked-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> ~ Oleksii > --- > Of course we could leverage mfn_valid() granularity here to do an > increment by more than 1 if mfn_valid() returned false. Yet doing so > likely would want a suitable helper to be introduced first, rather > than > open-coding such logic here. > --- > v2: New. > > --- a/xen/arch/x86/mm/p2m-ept.c > +++ b/xen/arch/x86/mm/p2m-ept.c > @@ -519,8 +519,12 @@ int epte_get_entry_emt(struct domain *d, > } > > for ( special_pgs = i = 0; i < (1ul << order); i++ ) > - if ( is_special_page(mfn_to_page(mfn_add(mfn, i))) ) > + { > + mfn_t cur = mfn_add(mfn, i); > + > + if ( mfn_valid(cur) && is_special_page(mfn_to_page(cur)) ) > special_pgs++; > + } > > if ( special_pgs ) > { >
--- a/xen/arch/x86/mm/p2m-ept.c +++ b/xen/arch/x86/mm/p2m-ept.c @@ -519,8 +519,12 @@ int epte_get_entry_emt(struct domain *d, } for ( special_pgs = i = 0; i < (1ul << order); i++ ) - if ( is_special_page(mfn_to_page(mfn_add(mfn, i))) ) + { + mfn_t cur = mfn_add(mfn, i); + + if ( mfn_valid(cur) && is_special_page(mfn_to_page(cur)) ) special_pgs++; + } if ( special_pgs ) {
mfn_valid() granularity is (currently) 256Mb. Therefore the start of a 1Gb page passing the test doesn't necessarily mean all parts of such a range would also pass. Yet using the result of mfn_to_page() on an MFN which doesn't pass mfn_valid() checking is liable to result in a crash (the invocation of mfn_to_page() alone is presumably "just" UB in such a case). Fixes: ca24b2ffdbd9 ("x86/hvm: set 'ipat' in EPT for special pages") Signed-off-by: Jan Beulich <jbeulich@suse.com> --- Of course we could leverage mfn_valid() granularity here to do an increment by more than 1 if mfn_valid() returned false. Yet doing so likely would want a suitable helper to be introduced first, rather than open-coding such logic here. --- v2: New.