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

[Jan Beulich <jbeulich@xxxxxxxx>]

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?

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.

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

> @@ -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.

> --- 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.

> @@ -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?

Jan

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