Re: [Xen-devel] [PATCH 6/6] introduce a 'passthrough' configuration option to xl.cfg...

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: 07 August 2019 13:13
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; Julien Grall <julien.grall@xxxxxxx>; 
> Andrew Cooper
> <Andrew.Cooper3@xxxxxxxxxx>; Anthony Perard <anthony.perard@xxxxxxxxxx>; 
> Roger Pau Monne
> <roger.pau@xxxxxxxxxx>; Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>; 
> George Dunlap
> <George.Dunlap@xxxxxxxxxx>; Ian Jackson <Ian.Jackson@xxxxxxxxxx>; Stefano 
> Stabellini
> <sstabellini@xxxxxxxxxx>; Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>; Tim 
> (Xen.org) <tim@xxxxxxx>;
> Wei Liu <wl@xxxxxxx>
> Subject: Re: [PATCH 6/6] introduce a 'passthrough' configuration option to 
> xl.cfg...
> 
> On 30.07.2019 15:44, Paul Durrant wrote:
> > --- a/docs/man/xl.cfg.5.pod.in
> > +++ b/docs/man/xl.cfg.5.pod.in
> > @@ -605,6 +605,58 @@ option should only be used with a trusted device tree.
> >   Note that the partial device tree should avoid using the phandle 65000
> >   which is reserved by the toolstack.
> >
> > +=item B<passthrough="STRING">
> > +
> > +Specify whether IOMMU mappings are enabled for the domain and hence whether
> > +it will be enabled for passthrough hardware. Valid values for this option
> > +are:
> > +
> > +=over 4
> > +
> > +=item B<disabled>
> > +
> > +IOMMU mappings are disabled for the domain and so hardware may not be
> > +passed through.
> > +
> > +This option is the default if no passthrough hardware is specified
> > +in the domain's configuration.
> > +
> > +=item B<sync_pt>
> > +
> > +This option means that IOMMU mappings will be synchronized with the
> > +domain's P2M table as follows:
> > +
> > +For a PV domain, all writable pages assigned to the domain are identity
> > +mapped by MFN in the IOMMU page table. Thus a device driver running in the
> > +domain may program passthrough hardware for DMA using MFN values
> > +(i.e. host/machine frame numbers) looked up in its P2M.
> > +
> > +For an HVM domain, all non-foreign RAM pages present in its P2M will be
> > +mapped by GFN in the IOMMU page table. Thus a device driver running in the
> > +domain may program passthrough hardware using GFN values (i.e. guest
> > +physical frame numbers) without any further translation.
> > +
> > +This option is the default if the domain is PV and passthrough hardware
> > +is specified in the configuration.
> > +
> > +This option is not available on Arm.
> > +
> > +=item B<share_pt>
> > +
> > +This option is unavailable for a PV domain. For an HVM domain, this option
> > +means that the IOMMU will be programmed to directly reference the domain's
> > +P2M table as its page table. From the point of view of a device driver
> > +running in the domain this is functionally equivalent to B<sync_pt> but
> > +places less load on the hypervisor and so should generally be selected in
> > +preference. However, the availability of this option is hardware specific
> > +and thus, if it is specified for a domain running on hardware that does
> > +not allow it, B<sync_pt> will be used instead.
> > +
> > +This option is the default if the domain is HVM and passthrough hardware
> > +is specified in the configuration.
> 
> Perhaps worthwhile to clarify right away that on AMD we'll always
> fall back to sync_pt?

Ok.

> 
> Btw, I've no idea what the convention in xl.cfg is, but in the
> hypervisor I'd ask to use hyphens instead of underscores in the
> option value strings.

Enums in the idl use underscores and so using them in the xl.cfg parameters 
means I can take advantage of the autogenerated string-to-enum helper.

> 
> > --- a/xen/arch/arm/domain.c
> > +++ b/xen/arch/arm/domain.c
> > @@ -614,6 +614,14 @@ int arch_sanitise_domain_config(struct 
> > xen_domctl_createdomain *config)
> >           return -EINVAL;
> >       }
> >
> > +    /* Always share P2M Table between the CPU and the IOMMU */
> > +    if ( config->iommu_opts & XEN_DOMCTL_IOMMU_no_sharept )
> > +    {
> > +        dprintk(XENLOG_INFO,
> > +                "Unsupported iommu option: XEN_DOMCTL_IOMMU_no_sharept\n");
> > +        return -EINVAL;
> 
> Seeing our excessive use of EINVAL, I'm generally suggesting to
> use almost anything else where EINVAL isn't clearly the right
> one to use. EOPNOTSUPP would seem better here, for example, not
> the least because I assume it is at least a hypothetically
> valid setting (if it was implemented).

Julien... Is ARM ever likely to not use shared P2M/IOMMU mappings?

> 
> > @@ -176,6 +176,15 @@ int iommu_domain_init(struct domain *d)
> >       if ( ret )
> >           return ret;
> >
> > +    /*
> > +     * Use shared page tables for HAP and IOMMU if the global option
> > +     * is enabled (from which we can infer the h/w is capable) and
> > +     * the domain options do not disallow it. HAP must, of course, also
> > +     * be enabled.
> > +     */
> > +    hd->hap_pt_share = hap_enabled(d) && iommu_hap_pt_share &&
> > +        !(opts & XEN_DOMCTL_IOMMU_no_sharept);
> 
> The hap_enabled() again raises the question of whether this
> builds for Arm.

It should, as long as the pre-requisite series 
https://lists.xenproject.org/archives/html/xen-devel/2019-07/msg02253.html is 
applied. I'll double-check that though.

> 
> > --- a/xen/include/public/domctl.h
> > +++ b/xen/include/public/domctl.h
> > @@ -68,7 +68,11 @@ struct xen_domctl_createdomain {
> >   #define _XEN_DOMCTL_CDF_iommu         5
> >   #define XEN_DOMCTL_CDF_iommu          (1U<<_XEN_DOMCTL_CDF_iommu)
> >
> > -    uint32_t flags;
> > +    uint16_t flags;
> > +
> > +#define _XEN_DOMCTL_IOMMU_no_sharept  0
> > +#define XEN_DOMCTL_IOMMU_no_sharept   (1U<<_XEN_DOMCTL_IOMMU_no_sharept)
> > +    uint16_t iommu_opts;
> 
> Are we sure of this split into to 16-bit fields? With a
> growing number of settings to be established at domain
> creation I don't think the 11 remaining bits for the
> "general" flags won't last very long. IOW if such a split,
> then I think we'd better have two 32-bit fields.

Ok. With only 6 bits in use for the flags I thought there was enough 
headroom... I'll grow the structure instead.

> 
> > @@ -256,10 +256,18 @@ struct domain_iommu {
> >       /* Features supported by the IOMMU */
> >       DECLARE_BITMAP(features, IOMMU_FEAT_count);
> >
> > +    /*
> > +     * Does the guest share HAP mapping with the IOMMU? This is always
> > +     * true for ARM systems and may be true for x86 systems where the
> > +     * the hardware is capable.
> > +     */
> > +    bool hap_pt_share;
> > +
> >      /*
> >       * Does the guest reqire mappings to be synchonized, to maintain
> > -     * the default dfn == pfn map. (See comment on dfn at the top of
> > -     * include/xen/mm.h).
> > +     * the default dfn == pfn map? (See comment on dfn at the top of
> > +     * include/xen/mm.h). Note that hap_pt_share == false does not
> > +     * necessarily imply this is true.
> 
> Would you mind also correcting the two spelling mistakes in the
> first line of the comment that you don't touch right now?

Happy to do that,

  Paul

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