[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v5 05/10] domain: introduce XEN_DOMCTL_CDF_iommu flag

Hi Paul,

On 14/08/2019 15:26, Paul Durrant wrote:
-----Original Message-----
From: Julien Grall <julien.grall@xxxxxxx>
Sent: 14 August 2019 15:00
To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
Cc: Ian Jackson <Ian.Jackson@xxxxxxxxxx>; Wei Liu <wl@xxxxxxx>; Anthony Perard
<anthony.perard@xxxxxxxxxx>; Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; George 
<George.Dunlap@xxxxxxxxxx>; Jan Beulich <jbeulich@xxxxxxxx>; Konrad Rzeszutek 
<konrad.wilk@xxxxxxxxxx>; Stefano Stabellini <sstabellini@xxxxxxxxxx>; Tim (Xen.org) 
Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>; Roger Pau Monne 
Subject: Re: [PATCH v5 05/10] domain: introduce XEN_DOMCTL_CDF_iommu flag

Hi Paul,

On 14/08/2019 14:38, 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.

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>
Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
Cc: Wei Liu <wl@xxxxxxx>
Cc: Anthony PERARD <anthony.perard@xxxxxxxxxx>
Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
Cc: George Dunlap <George.Dunlap@xxxxxxxxxxxxx>
Cc: Jan Beulich <jbeulich@xxxxxxxx>
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>
Cc: "Roger Pau Monné" <roger.pau@xxxxxxxxxx>

Previously part of series 

   - 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/libxl/libxl_create.c            | 8 ++++++++
   xen/arch/arm/domain.c                 | 1 -
   xen/arch/arm/setup.c                  | 3 +++
   xen/arch/x86/setup.c                  | 3 +++
   xen/common/domain.c                   | 9 ++++++++-
   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 +++++
   9 files changed, 37 insertions(+), 5 deletions(-)

diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 03ce166f4f..050ef042cd 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -555,6 +555,7 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config 
               .max_grant_frames = b_info->max_grant_frames,
               .max_maptrack_frames = b_info->max_maptrack_frames,
+        libxl_physinfo physinfo;

           if (info->type != LIBXL_DOMAIN_TYPE_PV) {
               create.flags |= XEN_DOMCTL_CDF_hvm_guest;
@@ -564,6 +565,13 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config 
                   libxl_defbool_val(info->oos) ? 0 : XEN_DOMCTL_CDF_oos_off;

+        rc = libxl_get_physinfo(ctx, &physinfo);
+        if (rc < 0)
+            goto out;
+        if (physinfo.cap_hvm_directio)
+            create.flags |= XEN_DOMCTL_CDF_iommu;

This is not going to work well on Arm as XEN_SYSCTL_PHYSCAP_directio is never 

Oh, that's true. I need to pull that into the common sysctl handler. It also 
looks like XEN_SYSCTL_PHYSCAP_hvm should always be set for too, but ARMs 
arch_do_physinfo() is completely empty at the moment.

Looking at the header (include/public/sysctl.h) it is pretty clear that the two PHYSCAP are x86 specific.

Even if Arm guests are based on the same concept as HVM/PVH guest. They are just called "Arm guest". We have always taken action so far to say they are HVM/PVH guest when presenting to the user. For instance, we strongly recommend to have the guest type in your configuration.

So I am not entirely sure we want to expose the capability for Arm. Otherwise a user may think it is fine to use "hvm" guest type.


Julien Grall

Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.