Re: [RFC PATCH 5/23] Tools/libxc: Add viommu operations in libxc

[Paul Durrant <Paul.Durrant@citrix.com>]

> -----Original Message-----
> From: Xen-devel [mailto:xen-devel-bounces@lists.xen.org] On Behalf Of
> Chao Gao
> Sent: 29 March 2017 01:40
> To: Wei Liu <wei.liu2@citrix.com>
> Cc: Lan Tianyu <tianyu.lan@intel.com>; Kevin Tian <kevin.tian@intel.com>;
> Ian Jackson <Ian.Jackson@citrix.com>; xen-devel@lists.xen.org
> Subject: Re: [Xen-devel] [RFC PATCH 5/23] Tools/libxc: Add viommu
> operations in libxc
> 
> Tianyu is on vacation this two weeks, so I will try to address
> some comments on this series.
> 
> On Tue, Mar 28, 2017 at 05:24:03PM +0100, Wei Liu wrote:
> >On Fri, Mar 17, 2017 at 07:27:05PM +0800, Lan Tianyu wrote:
> >> From: Chao Gao <chao.gao@intel.com>
> >>
> >> In previous patch, we introduce a common vIOMMU layer. In our design,
> >> we create/destroy vIOMMU through DMOP interface instead of creating
> it
> >> according to a config flag of domain. It makes it is possible
> >> to create vIOMMU in device model or in tool stack.
> >>

I've not been following this closely so apologies if this has already been asked...

Why would you need to create a vIOMMU instance in an external device model. Since the toolstack should be in control of the device model configuration why would it not know in advance that one was required?

  Paul

> >> The following toolstack code is to add XEN_DMOP_viommu_XXX syscalls:
> >
> >Hypercalls, not syscalls.
> >
> >>  - query capabilities of vIOMMU emulated by Xen
> >>  - create vIOMMU in Xen hypervisor with base address, capability
> >>  - destroy vIOMMU specified by viommu_id
> >>
> >> Signed-off-by: Chao Gao <chao.gao@intel.com>
> >> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
> >> ---
> >>  tools/libs/devicemodel/core.c                   | 69
> +++++++++++++++++++++++++
> >>  tools/libs/devicemodel/include/xendevicemodel.h | 35 +++++++++++++
> >>  tools/libs/devicemodel/libxendevicemodel.map    |  3 ++
> >>  tools/libxc/include/xenctrl_compat.h            |  5 ++
> >>  tools/libxc/xc_devicemodel_compat.c             | 18 +++++++
> >>  5 files changed, 130 insertions(+)
> >>
> >> diff --git a/tools/libs/devicemodel/core.c b/tools/libs/devicemodel/core.c
> >> index a85cb49..aee1150 100644
> >> --- a/tools/libs/devicemodel/core.c
> >> +++ b/tools/libs/devicemodel/core.c
> >
> >Bear in mind that this library is stable, so whatever ends up here can
> >change in the future.
> >
> >This is not saying the following code is problematic. It is just a
> >general FYI.
> >
> >Obviously the toolstack side is going to follow the hypervisor
> >interface, so I will do a detailed review later.
> 
> Sure. If the hypervisor interface settles down, we can inform you.
> 
> >
> >> +int xendevicemodel_viommu_destroy(
> >> +    xendevicemodel_handle *dmod, domid_t dom, uint32_t viommu_id);
> >>  #endif /* __XEN_TOOLS__ */
> >>
> >>  #endif /* XENDEVICEMODEL_H */
> >> diff --git a/tools/libs/devicemodel/libxendevicemodel.map
> b/tools/libs/devicemodel/libxendevicemodel.map
> >> index 45c773e..c2e0968 100644
> >> --- a/tools/libs/devicemodel/libxendevicemodel.map
> >> +++ b/tools/libs/devicemodel/libxendevicemodel.map
> >> @@ -17,6 +17,9 @@ VERS_1.0 {
> >>  		xendevicemodel_modified_memory;
> >>  		xendevicemodel_set_mem_type;
> >>  		xendevicemodel_inject_event;
> >> +		xendevicemodel_viommu_query_cap;
> >> +		xendevicemodel_viommu_create;
> >> +		xendevicemodel_viommu_destroy;
> >>  		xendevicemodel_restrict;
> >>  		xendevicemodel_close;
> >
> >I suppose this series is going to miss 4.9.
> >
> >Please add these functions to VERS_1.1.
> 
> Yes. We will fix this.
> 
> >
> >>  	local: *; /* Do not expose anything by default */
> >> diff --git a/tools/libxc/include/xenctrl_compat.h
> b/tools/libxc/include/xenctrl_compat.h
> >> index 040e7b2..315c45d 100644
> >> --- a/tools/libxc/include/xenctrl_compat.h
> >> +++ b/tools/libxc/include/xenctrl_compat.h
> >> @@ -164,6 +164,11 @@ int xc_hvm_set_mem_type(
> >>  int xc_hvm_inject_trap(
> >>      xc_interface *xch, domid_t domid, int vcpu, uint8_t vector,
> >>      uint8_t type, uint32_t error_code, uint8_t insn_len, uint64_t cr2);
> >> +int xc_viommu_query_cap(xc_interface *xch, domid_t dom, uint64_t
> *cap);
> >> +int xc_viommu_create(
> >> +    xc_interface *xch, domid_t dom, uint64_t base_addr, uint64_t cap,
> >> +    uint32_t *viommu_id);
> >> +int xc_viommu_destroy(xc_interface *xch, domid_t dom, uint32_t
> viommu_id);
> >>
> >>  #endif /* XC_WANT_COMPAT_DEVICEMODEL_API */
> >>
> >> diff --git a/tools/libxc/xc_devicemodel_compat.c
> b/tools/libxc/xc_devicemodel_compat.c
> >> index e4edeea..62f703a 100644
> >> --- a/tools/libxc/xc_devicemodel_compat.c
> >> +++ b/tools/libxc/xc_devicemodel_compat.c
> >
> >I don't think you need to provide compat wrappers for them. They are new
> >APIs.
> 
> OK. Got it.
> 
> Thanks,
> Chao
> >
> >Wei.
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel