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: 06 September 2019 10:06
> 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 Paul,
> 
> On 9/6/19 9:08 AM, Paul Durrant wrote:
> >> -----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?
> 
> I am not too worried about the xl.cfg files. My worry is regarding the
> libxl_types.idl which is definitely stable.
> 
> If you say the field is currently not available for Arm, the field will
> still be fill up by a given value. That given value will have to be
> handled differently when ARM gains a non-shared implementation.
> 

The idl is not architecture specific so I don't see any issue there. If ARM 
gains a non-shared implementation then the toolstack would not need to change, 
just the documentation. Setting sync_pt results in the 'no shared PT' IOMMU 
option being passed to domain create. That would currently be rejected by ARM's 
sanitise_domain_config() but that, of course, can be easily changed.

> >>
> >> 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.r
> 
> Well, here you impose the user to know how to configure the IOMMU (i.e.
> shared vs non-shared). He/She may not know about it and therefore he/she
> will have to try different value until Xen accepts it.
> 
> For the benefits of the user (and make it easy to extend in the future)
> having an option to say "let Xen chose" allow an external toolstack to
> not replicate the exact code you wrote in xl_parse.c.

Ok. I was discussing the Roger and Andy on IRC and I think I'm persuaded.

  Paul

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