|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v3 1/3] introduce GFN notification for translated domains
> -----Original Message-----
> From: Xen-devel <xen-devel-bounces@xxxxxxxxxxxxxxxxxxxx> On Behalf Of Jan
> Beulich
> Sent: 25 November 2019 09:58
> To: xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Juergen Gross <jgross@xxxxxxxx>; Stefano Stabellini
> <sstabellini@xxxxxxxxxx>; Julien Grall <julien@xxxxxxx>; Wei Liu
> <wl@xxxxxxx>; Konrad Wilk <konrad.wilk@xxxxxxxxxx>; George Dunlap
> <George.Dunlap@xxxxxxxxxxxxx>; Andrew Cooper <andrew.cooper3@xxxxxxxxxx>;
> Sander Eikelenboom <linux@xxxxxxxxxxxxxx>; Ian Jackson
> <ian.jackson@xxxxxxxxxx>; Roger Pau Monné <roger.pau@xxxxxxxxxx>
> Subject: [Xen-devel] [PATCH v3 1/3] introduce GFN notification for
> translated domains
>
> In order for individual IOMMU drivers (and from an abstract pov also
> architectures) to be able to adjust, ahead of actual mapping requests,
> their data structures when they might cover only a sub-range of all
> possible GFNs, introduce a notification call used by various code paths
> potentially installing a fresh mapping of a never used GFN (for a
> particular domain).
>
> Note that before this patch, in gnttab_transfer(), once past
> assign_pages(), further errors modifying the physmap are ignored
> (presumably because it would be too complicated to try to roll back at
> that point). This patch follows suit by ignoring failed notify_gfn()s or
> races due to the need to intermediately drop locks, simply printing out
> a warning that the gfn may not be accessible due to the failure.
>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> v3: Conditionalize upon CONFIG_IOMMU_FORCE_PT_SHARE, also covering the
> share_p2m_table() functionality as appropriate. Un-comment the
> GNTMAP_host_map check.
> v2: Introduce arch_notify_gfn(), to invoke gfn_valid() on x86 (this
> unfortunately means it and notify_gfn() now need to be macros, or
> else include file dependencies get in the way, as gfn_valid() lives
> in paging.h, which we shouldn't include from xen/sched.h). Improve
> description.
>
> TBD: Does Arm actually have anything to check against in its
> arch_notify_gfn()?
>
> --- a/xen/arch/x86/hvm/dom0_build.c
> +++ b/xen/arch/x86/hvm/dom0_build.c
> @@ -173,7 +173,8 @@ static int __init pvh_populate_memory_ra
> continue;
> }
>
> - rc = guest_physmap_add_page(d, _gfn(start), page_to_mfn(page),
> + rc = notify_gfn(d, _gfn(start + (1UL << order) - 1)) ?:
> + guest_physmap_add_page(d, _gfn(start), page_to_mfn(page),
> order);
> if ( rc != 0 )
> {
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -4304,9 +4304,17 @@ static int hvmop_set_param(
> if ( a.value > SHUTDOWN_MAX )
> rc = -EINVAL;
> break;
> +
> case HVM_PARAM_IOREQ_SERVER_PFN:
> - d->arch.hvm.ioreq_gfn.base = a.value;
> + if ( d->arch.hvm.params[HVM_PARAM_NR_IOREQ_SERVER_PAGES] )
> + rc = notify_gfn(
> + d,
> + _gfn(a.value + d->arch.hvm.params
> + [HVM_PARAM_NR_IOREQ_SERVER_PAGES] -
> 1));
IIRC the PFN is typically set by the toolstack before the number of pages, so
the notify will be for a.value - 1, i.e. the previous gfn. Is that a problem?
Paul
> + if ( !rc )
> + d->arch.hvm.ioreq_gfn.base = a.value;
> break;
> +
> case HVM_PARAM_NR_IOREQ_SERVER_PAGES:
> {
> unsigned int i;
> @@ -4317,6 +4325,9 @@ static int hvmop_set_param(
> rc = -EINVAL;
> break;
> }
> + rc = notify_gfn(d, _gfn(d->arch.hvm.ioreq_gfn.base + a.value -
> 1));
> + if ( rc )
> + break;
> for ( i = 0; i < a.value; i++ )
> set_bit(i, &d->arch.hvm.ioreq_gfn.mask);
>
> @@ -4330,7 +4341,11 @@ static int hvmop_set_param(
> BUILD_BUG_ON(HVM_PARAM_BUFIOREQ_PFN >
> sizeof(d->arch.hvm.ioreq_gfn.legacy_mask) * 8);
> if ( a.value )
> - set_bit(a.index, &d->arch.hvm.ioreq_gfn.legacy_mask);
> + {
> + rc = notify_gfn(d, _gfn(a.value));
> + if ( !rc )
> + set_bit(a.index, &d->arch.hvm.ioreq_gfn.legacy_mask);
> + }
> break;
>
> case HVM_PARAM_X87_FIP_WIDTH:
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -946,6 +946,16 @@ map_grant_ref(
> return;
> }
>
> + if ( paging_mode_translate(ld) && (op->flags & GNTMAP_host_map) &&
> + (rc = notify_gfn(ld, gaddr_to_gfn(op->host_addr))) )
> + {
> + gdprintk(XENLOG_INFO, "notify(%"PRI_gfn") -> %d\n",
> + gfn_x(gaddr_to_gfn(op->host_addr)), rc);
> + op->status = GNTST_general_error;
> + return;
> + BUILD_BUG_ON(GNTST_okay);
> + }
> +
> if ( unlikely((rd = rcu_lock_domain_by_id(op->dom)) == NULL) )
> {
> gdprintk(XENLOG_INFO, "Could not find domain %d\n", op->dom);
> @@ -2123,6 +2133,7 @@ gnttab_transfer(
> {
> bool_t okay;
> int rc;
> + gfn_t gfn;
>
> if ( i && hypercall_preempt_check() )
> return i;
> @@ -2300,21 +2311,52 @@ gnttab_transfer(
> act = active_entry_acquire(e->grant_table, gop.ref);
>
> if ( evaluate_nospec(e->grant_table->gt_version == 1) )
> + gfn = _gfn(shared_entry_v1(e->grant_table, gop.ref).frame);
> + else
> + gfn = _gfn(shared_entry_v2(e->grant_table,
> gop.ref).full_page.frame);
> +
> + if ( paging_mode_translate(e) )
> {
> - grant_entry_v1_t *sha = &shared_entry_v1(e->grant_table,
> gop.ref);
> + gfn_t gfn2;
> +
> + active_entry_release(act);
> + grant_read_unlock(e->grant_table);
> +
> + rc = notify_gfn(e, gfn);
> + if ( rc )
> + printk(XENLOG_G_WARNING
> + "%pd: gref %u: xfer GFN %"PRI_gfn" may be
> inaccessible (%d)\n",
> + e, gop.ref, gfn_x(gfn), rc);
> +
> + grant_read_lock(e->grant_table);
> + act = active_entry_acquire(e->grant_table, gop.ref);
>
> - guest_physmap_add_page(e, _gfn(sha->frame), mfn, 0);
> - if ( !paging_mode_translate(e) )
> - sha->frame = mfn_x(mfn);
> + if ( evaluate_nospec(e->grant_table->gt_version == 1) )
> + gfn2 = _gfn(shared_entry_v1(e->grant_table,
> gop.ref).frame);
> + else
> + gfn2 = _gfn(shared_entry_v2(e->grant_table, gop.ref).
> + full_page.frame);
> +
> + if ( !gfn_eq(gfn, gfn2) )
> + {
> + printk(XENLOG_G_WARNING
> + "%pd: gref %u: xfer GFN went %"PRI_gfn" ->
> %"PRI_gfn"\n",
> + e, gop.ref, gfn_x(gfn), gfn_x(gfn2));
> + gfn = gfn2;
> + }
> }
> - else
> - {
> - grant_entry_v2_t *sha = &shared_entry_v2(e->grant_table,
> gop.ref);
>
> - guest_physmap_add_page(e, _gfn(sha->full_page.frame), mfn,
> 0);
> - if ( !paging_mode_translate(e) )
> - sha->full_page.frame = mfn_x(mfn);
> + guest_physmap_add_page(e, gfn, mfn, 0);
> +
> + if ( !paging_mode_translate(e) )
> + {
> + if ( evaluate_nospec(e->grant_table->gt_version == 1) )
> + shared_entry_v1(e->grant_table, gop.ref).frame =
> mfn_x(mfn);
> + else
> + shared_entry_v2(e->grant_table, gop.ref).full_page.frame
> =
> + mfn_x(mfn);
> }
> +
> smp_wmb();
> shared_entry_header(e->grant_table, gop.ref)->flags |=
> GTF_transfer_completed;
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -203,6 +203,10 @@ static void populate_physmap(struct memo
> if ( unlikely(__copy_from_guest_offset(&gpfn, a->extent_list, i,
> 1)) )
> goto out;
>
> + if ( paging_mode_translate(d) &&
> + notify_gfn(d, _gfn(gpfn + (1U << a->extent_order) - 1)) )
> + goto out;
> +
> if ( a->memflags & MEMF_populate_on_demand )
> {
> /* Disallow populating PoD pages on oneself. */
> @@ -745,6 +749,10 @@ static long memory_exchange(XEN_GUEST_HA
> continue;
> }
>
> + if ( paging_mode_translate(d) )
> + rc = notify_gfn(d,
> + _gfn(gpfn + (1U << exch.out.extent_order)
> - 1));
> +
> mfn = page_to_mfn(page);
> guest_physmap_add_page(d, _gfn(gpfn), mfn,
> exch.out.extent_order);
> @@ -813,12 +821,20 @@ int xenmem_add_to_physmap(struct domain
> extra.foreign_domid = DOMID_INVALID;
>
> if ( xatp->space != XENMAPSPACE_gmfn_range )
> - return xenmem_add_to_physmap_one(d, xatp->space, extra,
> + return notify_gfn(d, _gfn(xatp->gpfn)) ?:
> + xenmem_add_to_physmap_one(d, xatp->space, extra,
> xatp->idx, _gfn(xatp->gpfn));
>
> if ( xatp->size < start )
> return -EILSEQ;
>
> + if ( !start && xatp->size )
> + {
> + rc = notify_gfn(d, _gfn(xatp->gpfn + xatp->size - 1));
> + if ( rc )
> + return rc;
> + }
> +
> xatp->idx += start;
> xatp->gpfn += start;
> xatp->size -= start;
> @@ -891,7 +907,8 @@ static int xenmem_add_to_physmap_batch(s
> extent, 1)) )
> return -EFAULT;
>
> - rc = xenmem_add_to_physmap_one(d, xatpb->space,
> + rc = notify_gfn(d, _gfn(gpfn)) ?:
> + xenmem_add_to_physmap_one(d, xatpb->space,
> xatpb->u,
> idx, _gfn(gpfn));
>
> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -522,6 +522,7 @@ int iommu_do_domctl(
> return ret;
> }
>
> +#ifndef CONFIG_IOMMU_FORCE_PT_SHARE
> void iommu_share_p2m_table(struct domain* d)
> {
> ASSERT(hap_enabled(d));
> @@ -530,6 +531,15 @@ void iommu_share_p2m_table(struct domain
> iommu_get_ops()->share_p2m(d);
> }
>
> +int iommu_notify_gfn(struct domain *d, gfn_t gfn)
> +{
> + const struct iommu_ops *ops = dom_iommu(d)->platform_ops;
> +
> + return need_iommu_pt_sync(d) && ops->notify_dfn
> + ? iommu_call(ops, notify_dfn, d, _dfn(gfn_x(gfn))) : 0;
> +}
> +#endif
> +
> void iommu_crash_shutdown(void)
> {
> if ( !iommu_crash_disable )
> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -272,6 +272,8 @@ static inline void free_vcpu_guest_conte
>
> static inline void arch_vcpu_block(struct vcpu *v) {}
>
> +#define arch_notify_gfn(d, gfn) ((void)(d), (void)(gfn), 0)
> +
> #endif /* __ASM_DOMAIN_H__ */
>
> /*
> --- a/xen/include/asm-x86/domain.h
> +++ b/xen/include/asm-x86/domain.h
> @@ -647,6 +647,8 @@ static inline void free_vcpu_guest_conte
>
> void arch_vcpu_regs_init(struct vcpu *v);
>
> +#define arch_notify_gfn(d, gfn) (gfn_valid(d, gfn) ? 0 : -EADDRNOTAVAIL)
> +
> struct vcpu_hvm_context;
> int arch_set_info_hvm_guest(struct vcpu *v, const struct vcpu_hvm_context
> *ctx);
>
> --- a/xen/include/xen/iommu.h
> +++ b/xen/include/xen/iommu.h
> @@ -237,6 +237,11 @@ struct iommu_ops {
> int __must_check (*lookup_page)(struct domain *d, dfn_t dfn, mfn_t
> *mfn,
> unsigned int *flags);
>
> +#ifndef CONFIG_IOMMU_FORCE_PT_SHARE
> + void (*share_p2m)(struct domain *d);
> + int __must_check (*notify_dfn)(struct domain *d, dfn_t dfn);
> +#endif
> +
> void (*free_page_table)(struct page_info *);
>
> #ifdef CONFIG_X86
> @@ -253,7 +258,6 @@ struct iommu_ops {
>
> int __must_check (*suspend)(void);
> void (*resume)(void);
> - void (*share_p2m)(struct domain *d);
> void (*crash_shutdown)(void);
> int __must_check (*iotlb_flush)(struct domain *d, dfn_t dfn,
> unsigned int page_count,
> @@ -330,7 +334,15 @@ void iommu_resume(void);
> void iommu_crash_shutdown(void);
> int iommu_get_reserved_device_memory(iommu_grdm_t *, void *);
>
> +#ifndef CONFIG_IOMMU_FORCE_PT_SHARE
> void iommu_share_p2m_table(struct domain *d);
> +int __must_check iommu_notify_gfn(struct domain *d, gfn_t gfn);
> +#else
> +static inline int __must_check iommu_notify_gfn(struct domain *d, gfn_t
> gfn)
> +{
> + return 0;
> +}
> +#endif
>
> #ifdef CONFIG_HAS_PCI
> int iommu_do_pci_domctl(struct xen_domctl *, struct domain *d,
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -1039,6 +1039,8 @@ static always_inline bool is_iommu_enabl
> return evaluate_nospec(d->options & XEN_DOMCTL_CDF_iommu);
> }
>
> +#define notify_gfn(d, gfn) (arch_notify_gfn(d, gfn) ?:
> iommu_notify_gfn(d, gfn))
> +
> extern bool sched_smt_power_savings;
> extern bool sched_disable_smt_switching;
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxxxxxxxxx
> https://lists.xenproject.org/mailman/listinfo/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |