[Xen-devel] [PATCH] x86/p2m: drop p2m_access_t parameter from set_mmio_p2m_entry()

[Xen-devel] [PATCH] x86/p2m: drop p2m_access_t parameter from set_mmio_p2m_entry()

[Jan Beulich]

Both callers request the host P2M's default access, which can as well be
done inside the function. While touching this anyway, make the "gfn"
parameter type-safe as well.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -3037,9 +3037,8 @@ static int vmx_alloc_vlapic_mapping(stru
     share_xen_page_with_guest(pg, d, SHARE_rw);
     d->arch.hvm.vmx.apic_access_mfn = mfn;
 
-    return set_mmio_p2m_entry(d, paddr_to_pfn(APIC_DEFAULT_PHYS_BASE), mfn,
-                              PAGE_ORDER_4K,
-                              p2m_get_hostp2m(d)->default_access);
+    return set_mmio_p2m_entry(d, gaddr_to_gfn(APIC_DEFAULT_PHYS_BASE), mfn,
+                              PAGE_ORDER_4K);
 }
 
 static void vmx_free_vlapic_mapping(struct domain *d)
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1327,15 +1327,16 @@ int set_foreign_p2m_entry(struct domain
                                p2m_get_hostp2m(d)->default_access);
 }
 
-int set_mmio_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn,
-                       unsigned int order, p2m_access_t access)
+int set_mmio_p2m_entry(struct domain *d, gfn_t gfn, mfn_t mfn,
+                       unsigned int order)
 {
     if ( order > PAGE_ORDER_4K &&
          rangeset_overlaps_range(mmio_ro_ranges, mfn_x(mfn),
                                  mfn_x(mfn) + (1UL << order) - 1) )
         return PAGE_ORDER_4K + 1;
 
-    return set_typed_p2m_entry(d, gfn, mfn, order, p2m_mmio_direct, access);
+    return set_typed_p2m_entry(d, gfn_x(gfn), mfn, order, p2m_mmio_direct,
+                               p2m_get_hostp2m(d)->default_access);
 }
 
 int set_identity_p2m_entry(struct domain *d, unsigned long gfn_l,
@@ -2305,9 +2306,8 @@ int map_mmio_regions(struct domain *d,
         for ( order = mmio_order(d, (gfn_x(start_gfn) + i) | (mfn_x(mfn) + i), nr - i); ;
               order = ret - 1 )
         {
-            ret = set_mmio_p2m_entry(d, gfn_x(start_gfn) + i,
-                                     mfn_add(mfn, i), order,
-                                     p2m_get_hostp2m(d)->default_access);
+            ret = set_mmio_p2m_entry(d, gfn_add(start_gfn, i),
+                                     mfn_add(mfn, i), order);
             if ( ret <= 0 )
                 break;
             ASSERT(ret <= order);
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -638,8 +638,8 @@ int p2m_is_logdirty_range(struct p2m_dom
 int set_foreign_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn);
 
 /* Set mmio addresses in the p2m table (for pass-through) */
-int set_mmio_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn,
-                       unsigned int order, p2m_access_t access);
+int set_mmio_p2m_entry(struct domain *d, gfn_t gfn, mfn_t mfn,
+                       unsigned int order);
 int clear_mmio_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn,
                          unsigned int order);
 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] x86/p2m: drop p2m_access_t parameter from set_mmio_p2m_entry()

[Roger Pau Monné]

On Thu, Feb 06, 2020 at 04:20:02PM +0100, Jan Beulich wrote:
> Both callers request the host P2M's default access, which can as well be
> done inside the function. While touching this anyway, make the "gfn"
> parameter type-safe as well.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Roger Pau Monné <roger.pau@ciitrix.com>

Can be added back if there's a need for it.

Thanks.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] x86/p2m: drop p2m_access_t parameter from set_mmio_p2m_entry()

[Jan Beulich]

On 06.02.2020 16:20, Jan Beulich wrote:
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -3037,9 +3037,8 @@ static int vmx_alloc_vlapic_mapping(stru
>      share_xen_page_with_guest(pg, d, SHARE_rw);
>      d->arch.hvm.vmx.apic_access_mfn = mfn;
>  
> -    return set_mmio_p2m_entry(d, paddr_to_pfn(APIC_DEFAULT_PHYS_BASE), mfn,
> -                              PAGE_ORDER_4K,
> -                              p2m_get_hostp2m(d)->default_access);
> +    return set_mmio_p2m_entry(d, gaddr_to_gfn(APIC_DEFAULT_PHYS_BASE), mfn,
> +                              PAGE_ORDER_4K);
>  }

Upon 2nd thought - does this really want to use default access?
Execute permission for this page looks a little suspicious.
Isn't it the case that this page doesn't (normally?) get
accessed at all, and instead its address serves as an indicator
to the CPU? (I even vaguely recall it having been considered to
consolidate this, to e.g. a single page per domain.) In which
case even p2m_access_n might be good enough?

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] x86/p2m: drop p2m_access_t parameter from set_mmio_p2m_entry()

[Roger Pau Monné]

On Fri, Feb 07, 2020 at 09:08:15AM +0100, Jan Beulich wrote:
> On 06.02.2020 16:20, Jan Beulich wrote:
> > --- a/xen/arch/x86/hvm/vmx/vmx.c
> > +++ b/xen/arch/x86/hvm/vmx/vmx.c
> > @@ -3037,9 +3037,8 @@ static int vmx_alloc_vlapic_mapping(stru
> >      share_xen_page_with_guest(pg, d, SHARE_rw);
> >      d->arch.hvm.vmx.apic_access_mfn = mfn;
> >  
> > -    return set_mmio_p2m_entry(d, paddr_to_pfn(APIC_DEFAULT_PHYS_BASE), mfn,
> > -                              PAGE_ORDER_4K,
> > -                              p2m_get_hostp2m(d)->default_access);
> > +    return set_mmio_p2m_entry(d, gaddr_to_gfn(APIC_DEFAULT_PHYS_BASE), mfn,
> > +                              PAGE_ORDER_4K);
> >  }
> 
> Upon 2nd thought - does this really want to use default access?
> Execute permission for this page looks a little suspicious.
> Isn't it the case that this page doesn't (normally?) get
> accessed at all, and instead its address serves as an indicator
> to the CPU? (I even vaguely recall it having been considered to
> consolidate this, to e.g. a single page per domain.) In which
> case even p2m_access_n might be good enough?

Hm, I think p2m_access_n is not enough, as that would trigger an EPT
fault which has preference over the APIC access VM exit (see 29.4.1
Priority of APIC-Access VM Exits).

I think setting it to p2m_access_rw should be enough, and we would get
EPT faults when trying to execute from APIC page.

Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] x86/p2m: drop p2m_access_t parameter from set_mmio_p2m_entry()

[Jan Beulich]

On 07.02.2020 10:52, Roger Pau Monné wrote:
> On Fri, Feb 07, 2020 at 09:08:15AM +0100, Jan Beulich wrote:
>> On 06.02.2020 16:20, Jan Beulich wrote:
>>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>>> @@ -3037,9 +3037,8 @@ static int vmx_alloc_vlapic_mapping(stru
>>>      share_xen_page_with_guest(pg, d, SHARE_rw);
>>>      d->arch.hvm.vmx.apic_access_mfn = mfn;
>>>  
>>> -    return set_mmio_p2m_entry(d, paddr_to_pfn(APIC_DEFAULT_PHYS_BASE), mfn,
>>> -                              PAGE_ORDER_4K,
>>> -                              p2m_get_hostp2m(d)->default_access);
>>> +    return set_mmio_p2m_entry(d, gaddr_to_gfn(APIC_DEFAULT_PHYS_BASE), mfn,
>>> +                              PAGE_ORDER_4K);
>>>  }
>>
>> Upon 2nd thought - does this really want to use default access?
>> Execute permission for this page looks a little suspicious.
>> Isn't it the case that this page doesn't (normally?) get
>> accessed at all, and instead its address serves as an indicator
>> to the CPU? (I even vaguely recall it having been considered to
>> consolidate this, to e.g. a single page per domain.) In which
>> case even p2m_access_n might be good enough?
> 
> Hm, I think p2m_access_n is not enough, as that would trigger an EPT
> fault which has preference over the APIC access VM exit (see 29.4.1
> Priority of APIC-Access VM Exits).

Ah, yes, reading that text I agree. Having just a single such page
per domain would still seem possible, though. Although, if we meant
to support a guest moving the APIC base address, then we couldn't,
again.

> I think setting it to p2m_access_rw should be enough, and we would get
> EPT faults when trying to execute from APIC page.

Then the other question is whether there's any use for introspection
to further limit permissions on this (kind of fake) page. Tamas?

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] x86/p2m: drop p2m_access_t parameter from set_mmio_p2m_entry()

[Tamas K Lengyel]

On Fri, Feb 7, 2020 at 9:54 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 07.02.2020 10:52, Roger Pau Monné wrote:
> > On Fri, Feb 07, 2020 at 09:08:15AM +0100, Jan Beulich wrote:
> >> On 06.02.2020 16:20, Jan Beulich wrote:
> >>> --- a/xen/arch/x86/hvm/vmx/vmx.c
> >>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> >>> @@ -3037,9 +3037,8 @@ static int vmx_alloc_vlapic_mapping(stru
> >>>      share_xen_page_with_guest(pg, d, SHARE_rw);
> >>>      d->arch.hvm.vmx.apic_access_mfn = mfn;
> >>>
> >>> -    return set_mmio_p2m_entry(d, paddr_to_pfn(APIC_DEFAULT_PHYS_BASE), mfn,
> >>> -                              PAGE_ORDER_4K,
> >>> -                              p2m_get_hostp2m(d)->default_access);
> >>> +    return set_mmio_p2m_entry(d, gaddr_to_gfn(APIC_DEFAULT_PHYS_BASE), mfn,
> >>> +                              PAGE_ORDER_4K);
> >>>  }
> >>
> >> Upon 2nd thought - does this really want to use default access?
> >> Execute permission for this page looks a little suspicious.
> >> Isn't it the case that this page doesn't (normally?) get
> >> accessed at all, and instead its address serves as an indicator
> >> to the CPU? (I even vaguely recall it having been considered to
> >> consolidate this, to e.g. a single page per domain.) In which
> >> case even p2m_access_n might be good enough?
> >
> > Hm, I think p2m_access_n is not enough, as that would trigger an EPT
> > fault which has preference over the APIC access VM exit (see 29.4.1
> > Priority of APIC-Access VM Exits).
>
> Ah, yes, reading that text I agree. Having just a single such page
> per domain would still seem possible, though. Although, if we meant
> to support a guest moving the APIC base address, then we couldn't,
> again.
>
> > I think setting it to p2m_access_rw should be enough, and we would get
> > EPT faults when trying to execute from APIC page.
>
> Then the other question is whether there's any use for introspection
> to further limit permissions on this (kind of fake) page. Tamas?

I'm not aware of any use-case that would restrict the EPT permission
for MMIO pages. That said, an application could still request that to
be set later on. Since this function here gets called in
vmx_domain_initialise I suspect a mem_access user didn't even have a
chance to change the default_access to anything custom so I venture it
would be safe to choose whatever permission is sensible. If anyone
wants to mess with the permission later they can do that regardless of
what was set here.

Tamas

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] x86/p2m: drop p2m_access_t parameter from set_mmio_p2m_entry()

[Tamas K Lengyel]

On Fri, Feb 7, 2020 at 10:16 AM Tamas K Lengyel <tamas@tklengyel.com> wrote:
>
> On Fri, Feb 7, 2020 at 9:54 AM Jan Beulich <jbeulich@suse.com> wrote:
> >
> > On 07.02.2020 10:52, Roger Pau Monné wrote:
> > > On Fri, Feb 07, 2020 at 09:08:15AM +0100, Jan Beulich wrote:
> > >> On 06.02.2020 16:20, Jan Beulich wrote:
> > >>> --- a/xen/arch/x86/hvm/vmx/vmx.c
> > >>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> > >>> @@ -3037,9 +3037,8 @@ static int vmx_alloc_vlapic_mapping(stru
> > >>>      share_xen_page_with_guest(pg, d, SHARE_rw);
> > >>>      d->arch.hvm.vmx.apic_access_mfn = mfn;
> > >>>
> > >>> -    return set_mmio_p2m_entry(d, paddr_to_pfn(APIC_DEFAULT_PHYS_BASE), mfn,
> > >>> -                              PAGE_ORDER_4K,
> > >>> -                              p2m_get_hostp2m(d)->default_access);
> > >>> +    return set_mmio_p2m_entry(d, gaddr_to_gfn(APIC_DEFAULT_PHYS_BASE), mfn,
> > >>> +                              PAGE_ORDER_4K);
> > >>>  }
> > >>
> > >> Upon 2nd thought - does this really want to use default access?
> > >> Execute permission for this page looks a little suspicious.
> > >> Isn't it the case that this page doesn't (normally?) get
> > >> accessed at all, and instead its address serves as an indicator
> > >> to the CPU? (I even vaguely recall it having been considered to
> > >> consolidate this, to e.g. a single page per domain.) In which
> > >> case even p2m_access_n might be good enough?
> > >
> > > Hm, I think p2m_access_n is not enough, as that would trigger an EPT
> > > fault which has preference over the APIC access VM exit (see 29.4.1
> > > Priority of APIC-Access VM Exits).
> >
> > Ah, yes, reading that text I agree. Having just a single such page
> > per domain would still seem possible, though. Although, if we meant
> > to support a guest moving the APIC base address, then we couldn't,
> > again.
> >
> > > I think setting it to p2m_access_rw should be enough, and we would get
> > > EPT faults when trying to execute from APIC page.
> >
> > Then the other question is whether there's any use for introspection
> > to further limit permissions on this (kind of fake) page. Tamas?
>
> I'm not aware of any use-case that would restrict the EPT permission
> for MMIO pages. That said, an application could still request that to
> be set later on. Since this function here gets called in
> vmx_domain_initialise I suspect a mem_access user didn't even have a
> chance to change the default_access to anything custom so I venture it
> would be safe to choose whatever permission is sensible. If anyone
> wants to mess with the permission later they can do that regardless of
> what was set here.

One thing to add though regarding using p2m_access_rw here is that in
case something would trigger an X violation it would lead to an event
being sent to a vm_event subscriber, which they may not be able to
make sense of. So I would suggest that if you want to make this
pagetable entry R/W only to use a p2m_type for that instead of a
p2m_access.

Tamas

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] x86/p2m: drop p2m_access_t parameter from set_mmio_p2m_entry()

[Jan Beulich]

On 07.02.2020 18:21, Tamas K Lengyel wrote:
> On Fri, Feb 7, 2020 at 10:16 AM Tamas K Lengyel <tamas@tklengyel.com> wrote:
>>
>> On Fri, Feb 7, 2020 at 9:54 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>
>>> On 07.02.2020 10:52, Roger Pau Monné wrote:
>>>> On Fri, Feb 07, 2020 at 09:08:15AM +0100, Jan Beulich wrote:
>>>>> On 06.02.2020 16:20, Jan Beulich wrote:
>>>>>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>>>>>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>>>>>> @@ -3037,9 +3037,8 @@ static int vmx_alloc_vlapic_mapping(stru
>>>>>>      share_xen_page_with_guest(pg, d, SHARE_rw);
>>>>>>      d->arch.hvm.vmx.apic_access_mfn = mfn;
>>>>>>
>>>>>> -    return set_mmio_p2m_entry(d, paddr_to_pfn(APIC_DEFAULT_PHYS_BASE), mfn,
>>>>>> -                              PAGE_ORDER_4K,
>>>>>> -                              p2m_get_hostp2m(d)->default_access);
>>>>>> +    return set_mmio_p2m_entry(d, gaddr_to_gfn(APIC_DEFAULT_PHYS_BASE), mfn,
>>>>>> +                              PAGE_ORDER_4K);
>>>>>>  }
>>>>>
>>>>> Upon 2nd thought - does this really want to use default access?
>>>>> Execute permission for this page looks a little suspicious.
>>>>> Isn't it the case that this page doesn't (normally?) get
>>>>> accessed at all, and instead its address serves as an indicator
>>>>> to the CPU? (I even vaguely recall it having been considered to
>>>>> consolidate this, to e.g. a single page per domain.) In which
>>>>> case even p2m_access_n might be good enough?
>>>>
>>>> Hm, I think p2m_access_n is not enough, as that would trigger an EPT
>>>> fault which has preference over the APIC access VM exit (see 29.4.1
>>>> Priority of APIC-Access VM Exits).
>>>
>>> Ah, yes, reading that text I agree. Having just a single such page
>>> per domain would still seem possible, though. Although, if we meant
>>> to support a guest moving the APIC base address, then we couldn't,
>>> again.
>>>
>>>> I think setting it to p2m_access_rw should be enough, and we would get
>>>> EPT faults when trying to execute from APIC page.
>>>
>>> Then the other question is whether there's any use for introspection
>>> to further limit permissions on this (kind of fake) page. Tamas?
>>
>> I'm not aware of any use-case that would restrict the EPT permission
>> for MMIO pages. That said, an application could still request that to
>> be set later on. Since this function here gets called in
>> vmx_domain_initialise I suspect a mem_access user didn't even have a
>> chance to change the default_access to anything custom so I venture it
>> would be safe to choose whatever permission is sensible. If anyone
>> wants to mess with the permission later they can do that regardless of
>> what was set here.
> 
> One thing to add though regarding using p2m_access_rw here is that in
> case something would trigger an X violation it would lead to an event
> being sent to a vm_event subscriber, which they may not be able to
> make sense of.

Hmm, good point.

> So I would suggest that if you want to make this
> pagetable entry R/W only to use a p2m_type for that instead of a
> p2m_access.

This would then take a further type derived from p2m_mmio_direct,
with its driving moved to the hypercall interface (as Xen can't
tell which one is which, e.g. ROM vs "ordinary" MMIO, _except_ in
this special case here). So I guess the patch as is looks to be
the better alternative overall.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] x86/p2m: drop p2m_access_t parameter from set_mmio_p2m_entry()

[Tian, Kevin]

> From: Jan Beulich <jbeulich@suse.com>
> Sent: Thursday, February 6, 2020 11:20 PM
> 
> Both callers request the host P2M's default access, which can as well be
> done inside the function. While touching this anyway, make the "gfn"
> parameter type-safe as well.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] x86/p2m: drop p2m_access_t parameter from set_mmio_p2m_entry()

[Andrew Cooper]

On 06/02/2020 15:20, Jan Beulich wrote:
> Both callers request the host P2M's default access, which can as well be
> done inside the function. While touching this anyway, make the "gfn"
> parameter type-safe as well.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel