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

[Chao Gao <chao.gao@xxxxxxxxx>]

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.

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