[PATCH v2 0/2] epte_get_entry_emt() modifications

[PATCH v2 0/2] epte_get_entry_emt() modifications

[Paul Durrant]

From: Paul Durrant <pdurrant@amazon.com>

This series was originally a singleton (of patch #1)

Paul Durrant (2):
  x86/hvm: set 'ipat' in EPT for special pages
  x86/hvm: simplify 'mmio_direct' check in epte_get_entry_emt()

 xen/arch/x86/hvm/mtrr.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)
---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>
Cc: Wei Liu <wl@xen.org>
-- 
2.20.1

[PATCH v2 1/2] x86/hvm: set 'ipat' in EPT for special pages

[Paul Durrant]

From: Paul Durrant <pdurrant@amazon.com>

All non-MMIO ranges (i.e those not mapping real device MMIO regions) that
map valid MFNs are normally marked MTRR_TYPE_WRBACK and 'ipat' is set. Hence
when PV drivers running in a guest populate the BAR space of the Xen Platform
PCI Device with pages such as the Shared Info page or Grant Table pages,
accesses to these pages will be cachable.

However, should IOMMU mappings be enabled be enabled for the guest then these
accesses become uncachable. This has a substantial negative effect on I/O
throughput of PV devices. Arguably PV drivers should bot be using BAR space to
host the Shared Info and Grant Table pages but it is currently commonplace for
them to do this and so this problem needs mitigation. Hence this patch makes
sure the 'ipat' bit is set for any special page regardless of where in GFN
space it is mapped.

NOTE: Clearly this mitigation only applies to Intel EPT. It is not obvious
      that there is any similar mitigation possible for AMD NPT. Downstreams
      such as Citrix XenServer have been carrying a patch similar to this for
      several releases though.

Signed-off-by: Paul Durrant <pdurrant@amazon.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Wei Liu <wl@xen.org>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>
---
 xen/arch/x86/hvm/mtrr.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/hvm/mtrr.c b/xen/arch/x86/hvm/mtrr.c
index 511c3be1c8..3ad813ed15 100644
--- a/xen/arch/x86/hvm/mtrr.c
+++ b/xen/arch/x86/hvm/mtrr.c
@@ -830,7 +830,8 @@ int epte_get_entry_emt(struct domain *d, unsigned long gfn, mfn_t mfn,
         return MTRR_TYPE_UNCACHABLE;
     }
 
-    if ( !is_iommu_enabled(d) && !cache_flush_permitted(d) )
+    if ( (!is_iommu_enabled(d) && !cache_flush_permitted(d)) ||
+         is_special_page(mfn_to_page(mfn)) )
     {
         *ipat = 1;
         return MTRR_TYPE_WRBACK;
-- 
2.20.1

[PATCH v2 2/2] x86/hvm: simplify 'mmio_direct' check in epte_get_entry_emt()

[Paul Durrant]

From: Paul Durrant <pdurrant@amazon.com>

Re-factor the code to take advantage of the fact that the APIC access page is
a 'special' page.

Suggested-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Wei Liu <wl@xen.org>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>
---
 xen/arch/x86/hvm/mtrr.c | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/xen/arch/x86/hvm/mtrr.c b/xen/arch/x86/hvm/mtrr.c
index 3ad813ed15..0992f05e8f 100644
--- 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;
-        *ipat = 1;
-        return MTRR_TYPE_WRBACK;
-    }
-
     if ( !mfn_valid(mfn) )
     {
         *ipat = 1;
         return MTRR_TYPE_UNCACHABLE;
     }
 
-    if ( (!is_iommu_enabled(d) && !cache_flush_permitted(d)) ||
+    if ( (!direct_mmio && !is_iommu_enabled(d) && !cache_flush_permitted(d)) ||
          is_special_page(mfn_to_page(mfn)) )
     {
         *ipat = 1;
         return MTRR_TYPE_WRBACK;
     }
 
+    if ( direct_mmio )
+        return MTRR_TYPE_UNCACHABLE;
+
     gmtrr_mtype = hvm_get_mem_pinned_cacheattr(d, _gfn(gfn), order);
     if ( gmtrr_mtype >= 0 )
     {
-- 
2.20.1

Re: [PATCH v2 2/2] x86/hvm: simplify 'mmio_direct' check in epte_get_entry_emt()

[Jan Beulich]

On 31.07.2020 14:39, Paul Durrant wrote:
> From: Paul Durrant <pdurrant@amazon.com>
> 
> 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. However, ...

> --- 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.

Jan

RE: [PATCH v2 2/2] x86/hvm: simplify 'mmio_direct' check in epte_get_entry_emt()

[Paul Durrant]

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: 31 July 2020 13:58
> To: Paul Durrant <paul@xen.org>
> Cc: xen-devel@lists.xenproject.org; Paul Durrant <pdurrant@amazon.com>; Andrew Cooper
> <andrew.cooper3@citrix.com>; Wei Liu <wl@xen.org>; Roger Pau Monné <roger.pau@citrix.com>
> 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 <pdurrant@amazon.com>
> >
> > 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?

> However, ...
> 
> > --- 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?

  Paul

> 
> Jan

Re: [PATCH v2 2/2] x86/hvm: simplify 'mmio_direct' check in epte_get_entry_emt()

[Jan Beulich]

On 31.07.2020 15:07, Paul Durrant wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: 31 July 2020 13:58
>> To: Paul Durrant <paul@xen.org>
>> Cc: xen-devel@lists.xenproject.org; Paul Durrant <pdurrant@amazon.com>; Andrew Cooper
>> <andrew.cooper3@citrix.com>; Wei Liu <wl@xen.org>; Roger Pau Monné <roger.pau@citrix.com>
>> 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 <pdurrant@amazon.com>
>>>
>>> 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.

Jan

RE: [PATCH v2 2/2] x86/hvm: simplify 'mmio_direct' check in epte_get_entry_emt()

[Paul Durrant]

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: 31 July 2020 14:41
> To: paul@xen.org
> Cc: xen-devel@lists.xenproject.org; 'Paul Durrant' <pdurrant@amazon.com>; 'Andrew Cooper'
> <andrew.cooper3@citrix.com>; 'Wei Liu' <wl@xen.org>; 'Roger Pau Monné' <roger.pau@citrix.com>
> Subject: Re: [PATCH v2 2/2] x86/hvm: simplify 'mmio_direct' check in epte_get_entry_emt()
> 
> On 31.07.2020 15:07, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: Jan Beulich <jbeulich@suse.com>
> >> Sent: 31 July 2020 13:58
> >> To: Paul Durrant <paul@xen.org>
> >> Cc: xen-devel@lists.xenproject.org; Paul Durrant <pdurrant@amazon.com>; Andrew Cooper
> >> <andrew.cooper3@citrix.com>; Wei Liu <wl@xen.org>; Roger Pau Monné <roger.pau@citrix.com>
> >> 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 <pdurrant@amazon.com>
> >>>
> >>> 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.
>

Actually, it occurs to me that logdirty is going to be an issue if I use p2m_ram_rw. If I'm not going to use p2m_mmio_direct then do you have another suggestion?

  Paul

RE: [PATCH v2 2/2] x86/hvm: simplify 'mmio_direct' check in epte_get_entry_emt()

[Durrant, Paul]

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: 31 July 2020 14:41
> To: paul@xen.org
> Cc: xen-devel@lists.xenproject.org; Durrant, Paul <pdurrant@amazon.co.uk>; 'Andrew Cooper'
> <andrew.cooper3@citrix.com>; 'Wei Liu' <wl@xen.org>; 'Roger Pau Monné' <roger.pau@citrix.com>
> 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 <jbeulich@suse.com>
> >> Sent: 31 July 2020 13:58
> >> To: Paul Durrant <paul@xen.org>
> >> Cc: xen-devel@lists.xenproject.org; Paul Durrant <pdurrant@amazon.com>; Andrew Cooper
> >> <andrew.cooper3@citrix.com>; Wei Liu <wl@xen.org>; Roger Pau Monné <roger.pau@citrix.com>
> >> 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 <pdurrant@amazon.com>
> >>>
> >>> 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

RE: [PATCH v2 2/2] x86/hvm: simplify 'mmio_direct' check in epte_get_entry_emt()

[Paul Durrant]

> -----Original Message-----
> From: Paul Durrant <xadimgnik@gmail.com>
> Sent: 31 July 2020 14:44
> To: 'Jan Beulich' <jbeulich@suse.com>
> Cc: xen-devel@lists.xenproject.org; 'Paul Durrant' <pdurrant@amazon.com>; 'Andrew Cooper'
> <andrew.cooper3@citrix.com>; 'Wei Liu' <wl@xen.org>; 'Roger Pau Monné' <roger.pau@citrix.com>
> Subject: RE: [PATCH v2 2/2] x86/hvm: simplify 'mmio_direct' check in epte_get_entry_emt()
> 
> > -----Original Message-----
> > From: Jan Beulich <jbeulich@suse.com>
> > Sent: 31 July 2020 14:41
> > To: paul@xen.org
> > Cc: xen-devel@lists.xenproject.org; 'Paul Durrant' <pdurrant@amazon.com>; 'Andrew Cooper'
> > <andrew.cooper3@citrix.com>; 'Wei Liu' <wl@xen.org>; 'Roger Pau Monné' <roger.pau@citrix.com>
> > Subject: Re: [PATCH v2 2/2] x86/hvm: simplify 'mmio_direct' check in epte_get_entry_emt()
> >
> > On 31.07.2020 15:07, Paul Durrant wrote:
> > >> -----Original Message-----
> > >> From: Jan Beulich <jbeulich@suse.com>
> > >> Sent: 31 July 2020 13:58
> > >> To: Paul Durrant <paul@xen.org>
> > >> Cc: xen-devel@lists.xenproject.org; Paul Durrant <pdurrant@amazon.com>; Andrew Cooper
> > >> <andrew.cooper3@citrix.com>; Wei Liu <wl@xen.org>; Roger Pau Monné <roger.pau@citrix.com>
> > >> 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 <pdurrant@amazon.com>
> > >>>
> > >>> 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.
> >
> 
> Actually, it occurs to me that logdirty is going to be an issue if I use p2m_ram_rw. If I'm not going
> to use p2m_mmio_direct then do you have another suggestion?
> 

Looking further I'm uneasy to move away from setting that APIC access page to anything other than mmio_direct so I'd rather leave the VMX code alone and re-order things here.

  Paul

Re: [PATCH v2 2/2] x86/hvm: simplify 'mmio_direct' check in epte_get_entry_emt()

[Jan Beulich]

On 31.07.2020 15:43, Paul Durrant wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: 31 July 2020 14:41
>> To: paul@xen.org
>> Cc: xen-devel@lists.xenproject.org; 'Paul Durrant' <pdurrant@amazon.com>; 'Andrew Cooper'
>> <andrew.cooper3@citrix.com>; 'Wei Liu' <wl@xen.org>; 'Roger Pau Monné' <roger.pau@citrix.com>
>> Subject: Re: [PATCH v2 2/2] x86/hvm: simplify 'mmio_direct' check in epte_get_entry_emt()
>>
>> On 31.07.2020 15:07, Paul Durrant wrote:
>>>> -----Original Message-----
>>>> From: Jan Beulich <jbeulich@suse.com>
>>>> Sent: 31 July 2020 13:58
>>>> To: Paul Durrant <paul@xen.org>
>>>> Cc: xen-devel@lists.xenproject.org; Paul Durrant <pdurrant@amazon.com>; Andrew Cooper
>>>> <andrew.cooper3@citrix.com>; Wei Liu <wl@xen.org>; Roger Pau Monné <roger.pau@citrix.com>
>>>> 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 <pdurrant@amazon.com>
>>>>>
>>>>> 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.
>>
> 
> Actually, it occurs to me that logdirty is going to be an issue if I
> use p2m_ram_rw. If I'm not going to use p2m_mmio_direct then do you
> have another suggestion?

p2m_ram_rw is also not good because of allowing execution. If we don't
want to create a new type, how about (ab)using p2m_grant_map_rw (in
the sense of Xen granting the domain access to this page)? Possible
problems with this are
- replace_grant_p2m_mapping()
- hvm_translate_get_page()
All other uses of p2m_grant_map_rw and p2m_is_grant() look to be
compatible with this approach.

For replace_grant_p2m_mapping() I think there's no issue because the
grant table code won't find a suitable active entry.

For hvm_translate_get_page() the question is why it checks for the
two grant types in the first place; iow I wonder whether that check
couldn't be dropped.

Independent of this, btw, you may need to call set_gpfn_from_mfn()
alongside p2m_set_entry().

Jan