> -----Original Message----- > From: Jan Beulich <[email protected]> > Sent: 31 July 2020 14:41 > To: [email protected] > Cc: [email protected]; Durrant, Paul <[email protected]>; > 'Andrew Cooper' > <[email protected]>; 'Wei Liu' <[email protected]>; 'Roger Pau Monné' > <[email protected]> > Subject: RE: [EXTERNAL] [PATCH v2 2/2] x86/hvm: simplify 'mmio_direct' check > in epte_get_entry_emt() > > CAUTION: This email originated from outside of the organization. Do not click > links or open > attachments unless you can confirm the sender and know the content is safe. > > > > On 31.07.2020 15:07, Paul Durrant wrote: > >> -----Original Message----- > >> From: Jan Beulich <[email protected]> > >> Sent: 31 July 2020 13:58 > >> To: Paul Durrant <[email protected]> > >> Cc: [email protected]; Paul Durrant <[email protected]>; > >> Andrew Cooper > >> <[email protected]>; Wei Liu <[email protected]>; Roger Pau Monné > >> <[email protected]> > >> Subject: Re: [PATCH v2 2/2] x86/hvm: simplify 'mmio_direct' check in > >> epte_get_entry_emt() > >> > >> On 31.07.2020 14:39, Paul Durrant wrote: > >>> From: Paul Durrant <[email protected]> > >>> > >>> Re-factor the code to take advantage of the fact that the APIC access > >>> page is > >>> a 'special' page. > >> > >> Hmm, that's going quite as far as I was thinking to go: In > >> particular, you leave in place the set_mmio_p2m_entry() use > >> in vmx_alloc_vlapic_mapping(). With that replaced, the > >> re-ordering in epte_get_entry_emt() that you do shouldn't > >> be necessary; you'd simple drop the checking of the > >> specific MFN. > > > > Ok, it still needs to go in the p2m though so are you suggesting > > just calling p2m_set_entry() directly? > > Yes, if this works. The main question really is whether there are > any hidden assumptions elsewhere that this page gets mapped as an > MMIO one. > > >>> --- a/xen/arch/x86/hvm/mtrr.c > >>> +++ b/xen/arch/x86/hvm/mtrr.c > >>> @@ -814,29 +814,22 @@ int epte_get_entry_emt(struct domain *d, unsigned > >>> long gfn, mfn_t mfn, > >>> return -1; > >>> } > >>> > >>> - if ( direct_mmio ) > >>> - { > >>> - if ( (mfn_x(mfn) ^ mfn_x(d->arch.hvm.vmx.apic_access_mfn)) >> > >>> order ) > >>> - return MTRR_TYPE_UNCACHABLE; > >>> - if ( order ) > >>> - return -1; > >> > >> ... this part of the logic wants retaining, I think, i.e. > >> reporting back to the guest that the mapping needs splitting. > >> I'm afraid I have to withdraw my R-b on patch 1 for this > >> reason, as the check needs to be added there already. > > > > To be clear... You mean I need the: > > > > if ( order ) > > return -1; > > > > there? > > Not just - first of all you need to check whether the requested > range overlaps a special page.
Ok. I'll add that. Paul > > Jan
