| Message ID | da353fee-b7f7-73ab-9ebe-632b4ea4152d@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/p2m: drop p2m_access_t parameter from set_mmio_p2m_entry() | expand |
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.
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
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.
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
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
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
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
> 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>
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>
--- 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);
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>