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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Lan, Tianyu [mailto:tianyu.lan@xxxxxxxxx]
> Sent: 14 April 2017 16:38
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>; Wei Liu <wei.liu2@xxxxxxxxxx>;
> Kevin Tian <kevin.tian@xxxxxxxxx>; Ian Jackson <Ian.Jackson@xxxxxxxxxx>;
> xen-devel@xxxxxxxxxxxxx
> Subject: Re: [Xen-devel] [RFC PATCH 5/23] Tools/libxc: Add viommu
> operations in libxc
> 
> Hi Paul:
>       Sorry for later response.
> 
> On 3/31/2017 3:57 AM, Chao Gao wrote:
> > On Wed, Mar 29, 2017 at 09:08:06AM +0000, Paul Durrant wrote:
> >>> -----Original Message-----
> >>> From: Xen-devel [mailto:xen-devel-bounces@xxxxxxxxxxxxx] On Behalf
> Of
> >>> Chao Gao
> >>> Sent: 29 March 2017 01:40
> >>> To: Wei Liu <wei.liu2@xxxxxxxxxx>
> >>> Cc: Lan Tianyu <tianyu.lan@xxxxxxxxx>; Kevin Tian
> <kevin.tian@xxxxxxxxx>;
> >>> Ian Jackson <Ian.Jackson@xxxxxxxxxx>; xen-devel@xxxxxxxxxxxxx
> >>> 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@xxxxxxxxx>
> >>>>>
> >>>>> 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?
> >
> > I assume your question is why we don't create a vIOMMU instance via
> hypercall in toolstack.
> > I think creating in toolstack is also ok and is easier to be reused by pvh.
> >
> > If Tianyu has no concern about this, will move this part to toolstack.
> 
> We can move create/destroy vIOMMU in the tool stack but we still need to
> add such dummy vIOMMU device model in Qemu to pass virtual device's
> DMA
> request into Xen hypervisor.

Not quite sure I understand this. The QEMu device model does not 'pass DMA 
requests' as such, it maps guest RAM and reads or writes to emulate DMA, right? 
So, what's needed is a mechanism to map guest RAM by 'bus address'... i.e. an 
address that will need to be translated through the vIOMMU mappings. This is 
just an evolution of the current 'priv mapping' operations that allow guest RAM 
to be mapped by guest physical address. So you don't need a vIOMMU 'device 
model' as such, do you?

> Qemu is required to use DMOP hypercall and
> tool stack may use domctl hyercall. vIOMMU hypercalls will be divided
> into two part.
> 
> Domctl:
>       create, destroy and query.
> DMOP:
>       vDev's DMA related operations.

Yes, the mapping/unmapping operations should be DMOPs and IMO should be 
designed such that they can be unified with replacements for current 'priv map' 
ops such that QEMU can use the same function call, but with different address 
space identifiers (i.e. bus address, guest physical address, etc.). BTW, I say 
'etc.' because we should also consider mapping the ioreq pages from Xen using 
the same call - with a dedicated address space identifier - as well.

  Cheers,

    Paul

> 
> Is this OK?
> 
> >
> > Thanks,
> > Chao
> >
> >>
> >>  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@xxxxxxxxx>
> >>>>> Signed-off-by: Lan Tianyu <tianyu.lan@xxxxxxxxx>
> >>>>> ---
> >>>>>  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@xxxxxxxxxxxxx
> >>> https://lists.xen.org/xen-devel

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel