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

Commit Message

Paul Durrant July 31, 2020, 12:39 p.m. UTC

From: Paul Durrant <[e-mail]>

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

Suggested-by: Jan Beulich <[e-mail]>
Signed-off-by: Paul Durrant <[e-mail]>
---
Cc: Jan Beulich <[e-mail]>
Cc: Andrew Cooper <[e-mail]>
Cc: Wei Liu <[e-mail]>
Cc: "Roger Pau Monné" <[e-mail]>
---
 xen/arch/x86/hvm/mtrr.c | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

Comments

Jan Beulich July 31, 2020, 12:58 p.m. UTC | #1

On 31.07.2020 14:39, Paul Durrant wrote:
> From: Paul Durrant <[e-mail]>
> 
> 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

Paul Durrant July 31, 2020, 1:07 p.m. UTC | #2

> -----Original Message-----
> From: Jan Beulich <[e-mail]>
> Sent: 31 July 2020 13:58
> To: Paul Durrant <[e-mail]>
> Cc: [e-mail]; Paul Durrant <[e-mail]>; Andrew Cooper
> <[e-mail]>; Wei Liu <[e-mail]>; Roger Pau Monné <[e-mail]>
> 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 <[e-mail]>
> >
> > 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

Jan Beulich July 31, 2020, 1:40 p.m. UTC | #3

On 31.07.2020 15:07, Paul Durrant wrote:
>> -----Original Message-----
>> From: Jan Beulich <[e-mail]>
>> Sent: 31 July 2020 13:58
>> To: Paul Durrant <[e-mail]>
>> Cc: [e-mail]; Paul Durrant <[e-mail]>; Andrew Cooper
>> <[e-mail]>; Wei Liu <[e-mail]>; Roger Pau Monné <[e-mail]>
>> 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 <[e-mail]>
>>>
>>> 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

Paul Durrant July 31, 2020, 1:43 p.m. UTC | #4

> -----Original Message-----
> From: Jan Beulich <[e-mail]>
> Sent: 31 July 2020 14:41
> To: [e-mail]
> Cc: [e-mail]; 'Paul Durrant' <[e-mail]>; 'Andrew Cooper'
> <[e-mail]>; 'Wei Liu' <[e-mail]>; 'Roger Pau Monné' <[e-mail]>
> 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 <[e-mail]>
> >> Sent: 31 July 2020 13:58
> >> To: Paul Durrant <[e-mail]>
> >> Cc: [e-mail]; Paul Durrant <[e-mail]>; Andrew Cooper
> >> <[e-mail]>; Wei Liu <[e-mail]>; Roger Pau Monné <[e-mail]>
> >> 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 <[e-mail]>
> >>>
> >>> 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

Durrant, Paul July 31, 2020, 1:45 p.m. UTC | #5

> -----Original Message-----
> From: Jan Beulich <[e-mail]>
> Sent: 31 July 2020 14:41
> To: [e-mail]
> Cc: [e-mail]; Durrant, Paul <[e-mail]>; 'Andrew Cooper'
> <[e-mail]>; 'Wei Liu' <[e-mail]>; 'Roger Pau Monné' <[e-mail]>
> 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 <[e-mail]>
> >> Sent: 31 July 2020 13:58
> >> To: Paul Durrant <[e-mail]>
> >> Cc: [e-mail]; Paul Durrant <[e-mail]>; Andrew Cooper
> >> <[e-mail]>; Wei Liu <[e-mail]>; Roger Pau Monné <[e-mail]>
> >> 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 <[e-mail]>
> >>>
> >>> 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

Paul Durrant July 31, 2020, 1:54 p.m. UTC | #6

> -----Original Message-----
> From: Paul Durrant <[e-mail]>
> Sent: 31 July 2020 14:44
> To: 'Jan Beulich' <[e-mail]>
> Cc: [e-mail]; 'Paul Durrant' <[e-mail]>; 'Andrew Cooper'
> <[e-mail]>; 'Wei Liu' <[e-mail]>; 'Roger Pau Monné' <[e-mail]>
> Subject: RE: [PATCH v2 2/2] x86/hvm: simplify 'mmio_direct' check in epte_get_entry_emt()
> 
> > -----Original Message-----
> > From: Jan Beulich <[e-mail]>
> > Sent: 31 July 2020 14:41
> > To: [e-mail]
> > Cc: [e-mail]; 'Paul Durrant' <[e-mail]>; 'Andrew Cooper'
> > <[e-mail]>; 'Wei Liu' <[e-mail]>; 'Roger Pau Monné' <[e-mail]>
> > 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 <[e-mail]>
> > >> Sent: 31 July 2020 13:58
> > >> To: Paul Durrant <[e-mail]>
> > >> Cc: [e-mail]; Paul Durrant <[e-mail]>; Andrew Cooper
> > >> <[e-mail]>; Wei Liu <[e-mail]>; Roger Pau Monné <[e-mail]>
> > >> 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 <[e-mail]>
> > >>>
> > >>> 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

Jan Beulich July 31, 2020, 2:14 p.m. UTC | #7

On 31.07.2020 15:43, Paul Durrant wrote:
>> -----Original Message-----
>> From: Jan Beulich <[e-mail]>
>> Sent: 31 July 2020 14:41
>> To: [e-mail]
>> Cc: [e-mail]; 'Paul Durrant' <[e-mail]>; 'Andrew Cooper'
>> <[e-mail]>; 'Wei Liu' <[e-mail]>; 'Roger Pau Monné' <[e-mail]>
>> 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 <[e-mail]>
>>>> Sent: 31 July 2020 13:58
>>>> To: Paul Durrant <[e-mail]>
>>>> Cc: [e-mail]; Paul Durrant <[e-mail]>; Andrew Cooper
>>>> <[e-mail]>; Wei Liu <[e-mail]>; Roger Pau Monné <[e-mail]>
>>>> 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 <[e-mail]>
>>>>>
>>>>> 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

Patch

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 )
     {