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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
[snip]
> >
> > 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?
> 
> 
> Guest also may enable DMA protection mechanism in linux kernel which
> limits address space of emulated device and this depends on the vIOMMU's
> DMA translation function. In vIOMMU's MMIO emulation part is in the Xen
> hypersior and the guest shadow IO page table will be only in the
> hypervisor. To translate emulated device's DMA request. It's necessary
> to pass the DMA request to hypervisor.
> 

What do you mean by DMA request though? Are you intending to make some form of 
hypercall to read or write guest memory? If so then why not introduce a call to 
map the guest memory (via bus address) and read or write directly.

> So far we don't support DMA translation and so doesn't pass DMA request.
> 

Indeed. We map guest memory using guest physical address because, without an 
emulated IOMMU, guest physical address === bus address. This is why I suggest a 
new mapping operation rather than 'passing a DMA request' to the hypervisor.

> Map/umap guest memory already support in Qemu and just like emulated
> device model access guest memory. Qemu also provides vIOMMU hook to
> receive DMA request and return target guest address. vIOMMU framework
> will read/write target address.

That's the part I don't get... why have the vIOMMU code do the reads and 
writes? Why not have it provide a mapping function and then have the device 
model in QEMU read and write directly as it does now?

> What we need to do is to translate DMA
> request to target address according shadow IO page table in the hypervisor.
> 

Yes, so the mapping has to be done by the hypervisor (as is the case for priv 
mapping or grant mapping) but the memory accesses themselves can be done 
directly by the device model in QEMU.

> 
> 
> >
> >> 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.
> >
> 
> So you agree to divide vIOMMU's hypercalls into two parts(DMOP and
> Domctl), right?
> 

Yes, I agree with the logic of the split.

  Cheers,

   Paul

> 
> >   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
> 
> 
> --
> Best regards
> Tianyu Lan
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel