Re: [Xen-devel] [PATCH v8 2/6] domain: introduce XEN_DOMCTL_CDF_iommu flag

[Julien Grall <julien.grall@xxxxxxx>]

Hi Paul,

Apologies for the late answer. A couple of comments below.

On 9/2/19 3:50 PM, Paul Durrant wrote:
> This patch introduces a common domain creation flag to determine whether
> the domain is permitted to make use of the IOMMU. Currently the flag is
> always set (for both dom0 and domU) if the IOMMU is globally enabled
> (i.e. iommu_enabled == 1). sanitise_domain_config() is modified to reject
> the flag if !iommu_enabled.
This is not entirely correct for Arm, the flag will not be set for domU 
created by Xen directly (i.e dom0less). Device passthrough is not yet 
supported for dom0less so this is not much a concern.
However, there are a series to add support for platform device 
passthrough (see [1]). So there are two possibilities:
   1) If your series goes first, then we should at least document it in 
the commit message that IOMMU will be disabled for domain (other than 
dom0) created by Xen.
   2) If your series goes second, then you (or Stefano) would have to 
ensure this does not break [1].
I haven't yet had the chance to review the latest version of Stefano and 
this patch looks in good shape. So 1) seems more suitable. Stefano would 
have to ensure the flags is set when doing device assignment.
> A new helper function, is_iommu_enabled(), is added to test the flag and
> iommu_domain_init() will return immediately if !is_iommu_enabled(). This is
> slightly different to the previous behaviour based on !iommu_enabled where
> the call to arch_iommu_domain_init() was made regardless, however it appears
> that this call was only necessary to initialize the dt_devices list for ARM
> such that iommu_release_dt_devices() can be called unconditionally by
> domain_relinquish_resources(). Adding a simple check of is_iommu_enabled()
> into iommu_release_dt_devices() keeps this unconditional call working.
>
> No functional change should be observed with this patch applied.
>
> Subsequent patches will allow the toolstack to control whether use of the
> IOMMU is enabled for a domain.
>
> NOTE: The introduction of the is_iommu_enabled() helper function might
>        seem excessive but its use is expected to increase with subsequent
>        patches. Also, having iommu_domain_init() bail before calling
>        arch_iommu_domain_init() is not strictly necessary, but I think the
>        consequent addition of the call to is_iommu_enabled() in
>        iommu_release_dt_devices() makes the code clearer.
>
> Signed-off-by: Paul Durrant <paul.durrant@xxxxxxxxxx>
> Reviewed-by: "Roger Pau Monné" <roger.pau@xxxxxxxxxx>
> Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> Cc: Christian Lindig <christian.lindig@xxxxxxxxxx>
> Cc: David Scott <dave@xxxxxxxxxx>
> Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> Cc: Wei Liu <wl@xxxxxxx>
> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Cc: George Dunlap <George.Dunlap@xxxxxxxxxxxxx>
> Cc: Julien Grall <julien.grall@xxxxxxx>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> Cc: Tim Deegan <tim@xxxxxxx>
> Cc: Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
>
> Previously part of series 
> https://lists.xenproject.org/archives/html/xen-devel/2019-07/msg02267.html
>
> v7:
>   - Add a check to verify that the toolstack has not set XEN_DOMCTL_CDF_iommu
>   - Add missing ocaml binding changes
>
> v6:
>   - Remove the toolstack parts as there's no nice method of testing whether
>     the IOMMU is enabled in an architecture-neutral way
>
> v5:
>   - Move is_iommu_enabled() check into iommu_domain_init()
>   - Reject XEN_DOMCTL_CDF_iommu in sanitise_domain_config() if !iommu_enabled
>   - Use evaluate_nospec() in defintion of is_iommu_enabled()
> ---
>   tools/ocaml/libs/xc/xenctrl.ml        |  8 +++++++-
>   tools/ocaml/libs/xc/xenctrl.mli       |  8 +++++++-
>   xen/arch/arm/setup.c                  |  3 +++
>   xen/arch/x86/setup.c                  |  3 +++
>   xen/common/domain.c                   |  9 ++++++++-
>   xen/common/domctl.c                   | 13 +++++++++++++
>   xen/drivers/passthrough/device_tree.c |  3 +++
>   xen/drivers/passthrough/iommu.c       |  6 +++---
>   xen/include/public/domctl.h           |  4 ++++
>   xen/include/xen/sched.h               |  5 +++++
>   10 files changed, 56 insertions(+), 6 deletions(-)
>
> diff --git a/tools/ocaml/libs/xc/xenctrl.ml b/tools/ocaml/libs/xc/xenctrl.ml
> index 35958b94d5..bdf3f2e395 100644
> --- a/tools/ocaml/libs/xc/xenctrl.ml
> +++ b/tools/ocaml/libs/xc/xenctrl.ml
> @@ -56,7 +56,13 @@ type arch_domainconfig =
>         | ARM of xen_arm_arch_domainconfig
>         | X86 of xen_x86_arch_domainconfig
>   
> -type domain_create_flag = CDF_HVM | CDF_HAP
> +type domain_create_flag =
> +       | CDF_HVM
> +       | CDF_HAP
> +       | CDF_S3_INTEGRITY
> +       | CDF_OOS_OFF
> +       | CDF_XS_DOMAIN
> +       | CDF_IOMMU
This patch is only adding the last flag, but you are adding more here. I 
understand that you are just re-syncing the type with Xen. IHMO, this 
should belong in a separate patch as we may want to backport it.
If backporting is not necessary, then the change should at least be 
mentioned in the commit message as this seems a bit off-topic.
Cheers,

[1] https://lists.xen.org/archives/html/xen-devel/2019-08/msg01974.html

--
Julien Grall

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