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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Julien Grall <julien.grall@xxxxxxx>
> Sent: 05 September 2019 21:28
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Jan Beulich <jbeulich@xxxxxxxx>; Ian Jackson <Ian.Jackson@xxxxxxxxxx>; 
> Wei Liu <wl@xxxxxxx>;
> Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; George Dunlap 
> <George.Dunlap@xxxxxxxxxx>; Konrad Rzeszutek
> Wilk <konrad.wilk@xxxxxxxxxx>; Stefano Stabellini <sstabellini@xxxxxxxxxx>; 
> Tim (Xen.org)
> <tim@xxxxxxx>; Anthony Perard <anthony.perard@xxxxxxxxxx>; Christian Lindig
> <christian.lindig@xxxxxxxxxx>; David Scott <dave@xxxxxxxxxx>; Volodymyr 
> Babchuk
> <Volodymyr_Babchuk@xxxxxxxx>; Roger Pau Monne <roger.pau@xxxxxxxxxx>
> Subject: Re: [PATCH v8 6/6] introduce a 'passthrough' configuration option to 
> xl.cfg...
> 
> Hi,
> 
> On 9/2/19 3:50 PM, Paul Durrant wrote:
> > ...and hence the ability to disable IOMMU mappings, and control EPT
> > sharing.
> >
> > This patch introduces a new 'libxl_passthrough' enumeration into
> > libxl_domain_create_info. The value will be set by xl either when it parses
> > a new 'passthrough' option in xl.cfg, or implicitly if there is passthrough
> > hardware specified for the domain.
> >
> > If the value of the passthrough configuration option is 'disabled' then
> > the XEN_DOMCTL_CDF_iommu flag will be clear in the xen_domctl_createdomain
> > flags, thus allowing the toolstack to control whether the domain gets
> > IOMMU mappings or not (where previously they were globally set).
> >
> > If the value of the passthrough configuration option is 'sync_pt' then
> > a new 'iommu_opts' field in xen_domctl_createdomain will be set with the
> > value XEN_DOMCTL_IOMMU_no_sharept. This will override the global default
> > set in iommu_hap_pt_share, thus allowing the toolstack to control whether
> > EPT sharing is used for the domain.
> >
> > NOTE: The 'iommu_memkb' overhead in libxl_domain_build_info will only be
> >        set to zero if passthrough is 'disabled'. It is not safe to set the
> >        overhead to zero in the 'share_pt' case because the toolstack has no
> >        means of knowing whether the hardware actually supports IOMMU page
> >        table sharing.
> >
> > Signed-off-by: Paul Durrant <paul.durrant@xxxxxxxxxx>
> > Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> > ---
> > 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: Anthony PERARD <anthony.perard@xxxxxxxxxx>
> > Cc: Christian Lindig <christian.lindig@xxxxxxxxxx>
> > Cc: David Scott <dave@xxxxxxxxxx>
> > Cc: Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
> > Cc: "Roger Pau Monné" <roger.pau@xxxxxxxxxx>
> >
> > Previously part of series 
> > https://lists.xenproject.org/archives/html/xen-devel/2019-07/msg02267.html
> >
> > v7:
> >   - Added missing breaks
> >   - Added missing ocaml binding changes
> >
> > v6:
> >   - Remove the libxl_physinfo() call since it's usefulness is limited to x86
> >
> > v5:
> >   - Expand xen_domctl_createdomain flags field and hence bump interface
> >     version
> >   - Fix spelling mistakes in context line
> > ---
> >   docs/man/xl.cfg.5.pod.in            |  52 +++++++++++
> >   tools/libxl/libxl.h                 |   5 +
> >   tools/libxl/libxl_create.c          |   9 ++
> >   tools/libxl/libxl_types.idl         |   7 ++
> >   tools/ocaml/libs/xc/xenctrl.ml      |   4 +
> >   tools/ocaml/libs/xc/xenctrl.mli     |   4 +
> >   tools/ocaml/libs/xc/xenctrl_stubs.c |  15 ++-
> >   tools/xl/xl_parse.c                 | 140 ++++++++++++++++++----------
> >   xen/arch/arm/domain.c               |  10 +-
> >   xen/arch/x86/domain.c               |   2 +-
> >   xen/common/domain.c                 |   7 ++
> >   xen/common/domctl.c                 |  13 ---
> >   xen/drivers/passthrough/iommu.c     |  13 ++-
> >   xen/include/public/domctl.h         |   6 +-
> >   xen/include/xen/iommu.h             |  19 ++--
> >   15 files changed, 229 insertions(+), 77 deletions(-)
> >
> > diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
> > index c99d40307e..fd35685e9e 100644
> > --- 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.
> 
> I don't particularly like the idea to allow the user (or any external
> toolstack) to rely on passthrough=share_pt for Arm. This may put us in a
> corner if we ever support IOMMU that can't use the CPU PT (I know a few
> of them).

I could just say 'not currently available' and, if ARM gains a non-shared 
implementation then the manpage could be changed. I don't think xl.cfg files 
need to be considered a stable interface, do they?

> 
> It feels to me we want a "default" that can let the toolstack (or the
> hypervisor) to select what ever is the most suitable for the preferred way.
> 
> For now default, could be aliased to share_pt for Arm in the toolstack.
> The point here is any toolstack built on top of libxl will not rely on
> passthrough=share_pt.

I don't really like that. The 'default' option is chosen by not putting any 
option in the cfg, I don't see why it needs to be explicit for this option.

  Paul

> 
> Cheers,
> 
> --
> Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel