| Message ID | 1489750043-17260-3-git-send-email-tianyu.lan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Mar 17, 2017 at 07:27:02PM +0800, Lan Tianyu wrote: > This patch is to introduce create, destroy and query capabilities > command for vIOMMU. vIOMMU layer will deal with requests and call > arch vIOMMU ops. > > Signed-off-by: Lan Tianyu <tianyu.lan@intel.com> > --- > xen/arch/x86/hvm/dm.c | 29 +++++++++++++++++++++++++++++ > xen/include/public/hvm/dm_op.h | 39 +++++++++++++++++++++++++++++++++++++++ > 2 files changed, 68 insertions(+) > > diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c > index 2122c45..2b28f70 100644 > --- a/xen/arch/x86/hvm/dm.c > +++ b/xen/arch/x86/hvm/dm.c > @@ -491,6 +491,35 @@ static int dm_op(domid_t domid, > break; > } > > + case XEN_DMOP_create_viommu: > + { > + struct xen_dm_op_create_viommu *data = > + &op.u.create_viommu; > + > + rc = viommu_create(d, data->base_address, data->length, data->capabilities); > + if (rc >= 0) { The style guide is is to have a space here and { on a newline. > + data->viommu_id = rc; > + rc = 0; > + } > + break; > + } Newline here.. > + case XEN_DMOP_destroy_viommu: > + { > + const struct xen_dm_op_destroy_viommu *data = > + &op.u.destroy_viommu; > + > + rc = viommu_destroy(d, data->viommu_id); > + break; > + } Ahem? > + case XEN_DMOP_query_viommu_caps: > + { > + struct xen_dm_op_query_viommu_caps *data = > + &op.u.query_viommu_caps; > + > + data->caps = viommu_query_caps(d); > + rc = 0; > + break; > + } And here. > default: > rc = -EOPNOTSUPP; > break; > diff --git a/xen/include/public/hvm/dm_op.h b/xen/include/public/hvm/dm_op.h > index f54cece..b8c7359 100644 > --- a/xen/include/public/hvm/dm_op.h > +++ b/xen/include/public/hvm/dm_op.h > @@ -318,6 +318,42 @@ struct xen_dm_op_inject_msi { > uint64_aligned_t addr; > }; > > +/* > + * XEN_DMOP_create_viommu: Create vIOMMU device. > + */ > +#define XEN_DMOP_create_viommu 15 > + > +struct xen_dm_op_create_viommu { > + /* IN - MMIO base address of vIOMMU */ Any limit? Can it be zero? > + uint64_t base_address; > + /* IN - Length of MMIO region */ Any restrictions? Can it be say 2 bytes? Or is this in page-size granularity? > + uint64_t length; > + /* IN - Capabilities with which we want to create */ > + uint64_t capabilities; That sounds like some form of flags? > + /* OUT - vIOMMU identity */ > + uint32_t viommu_id; > +}; > + > +/* > + * XEN_DMOP_destroy_viommu: Destroy vIOMMU device. > + */ > +#define XEN_DMOP_destroy_viommu 16 > + > +struct xen_dm_op_destroy_viommu { > + /* OUT - vIOMMU identity */ Out? Not in? > + uint32_t viommu_id; > +}; > + > +/* > + * XEN_DMOP_q_viommu: Query vIOMMU capabilities. > + */ > +#define XEN_DMOP_query_viommu_caps 17 > + > +struct xen_dm_op_query_viommu_caps { > + /* OUT - vIOMMU Capabilities*/ Don't you need to also mention which vIOMMU? As you could have potentially many of them? > + uint64_t caps; > +}; > + > struct xen_dm_op { > uint32_t op; > uint32_t pad; > @@ -336,6 +372,9 @@ struct xen_dm_op { > struct xen_dm_op_set_mem_type set_mem_type; > struct xen_dm_op_inject_event inject_event; > struct xen_dm_op_inject_msi inject_msi; > + struct xen_dm_op_create_viommu create_viommu; > + struct xen_dm_op_destroy_viommu destroy_viommu; > + struct xen_dm_op_query_viommu_caps query_viommu_caps; > } u; > }; > > -- > 1.8.3.1 > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > https://lists.xen.org/xen-devel
Hi Konrad: Thanks for your review. On 2017年04月17日 22:36, Konrad Rzeszutek Wilk wrote: > On Fri, Mar 17, 2017 at 07:27:02PM +0800, Lan Tianyu wrote: >> This patch is to introduce create, destroy and query capabilities >> command for vIOMMU. vIOMMU layer will deal with requests and call >> arch vIOMMU ops. >> >> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com> >> --- >> xen/arch/x86/hvm/dm.c | 29 +++++++++++++++++++++++++++++ >> xen/include/public/hvm/dm_op.h | 39 +++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 68 insertions(+) >> >> diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c >> index 2122c45..2b28f70 100644 >> --- a/xen/arch/x86/hvm/dm.c >> +++ b/xen/arch/x86/hvm/dm.c >> @@ -491,6 +491,35 @@ static int dm_op(domid_t domid, >> break; >> } >> >> + case XEN_DMOP_create_viommu: >> + { >> + struct xen_dm_op_create_viommu *data = >> + &op.u.create_viommu; >> + >> + rc = viommu_create(d, data->base_address, data->length, data->capabilities); >> + if (rc >= 0) { > > The style guide is is to have a space here and { on a newline. Yes, will fix. > >> + data->viommu_id = rc; >> + rc = 0; >> + } >> + break; >> + } > > Newline here.. > > >> + case XEN_DMOP_destroy_viommu: >> + { >> + const struct xen_dm_op_destroy_viommu *data = >> + &op.u.destroy_viommu; >> + >> + rc = viommu_destroy(d, data->viommu_id); >> + break; >> + } > > Ahem? >> + case XEN_DMOP_query_viommu_caps: >> + { >> + struct xen_dm_op_query_viommu_caps *data = >> + &op.u.query_viommu_caps; >> + >> + data->caps = viommu_query_caps(d); >> + rc = 0; >> + break; >> + } > > And here. >> default: >> rc = -EOPNOTSUPP; >> break; >> diff --git a/xen/include/public/hvm/dm_op.h b/xen/include/public/hvm/dm_op.h >> index f54cece..b8c7359 100644 >> --- a/xen/include/public/hvm/dm_op.h >> +++ b/xen/include/public/hvm/dm_op.h >> @@ -318,6 +318,42 @@ struct xen_dm_op_inject_msi { >> uint64_aligned_t addr; >> }; >> >> +/* >> + * XEN_DMOP_create_viommu: Create vIOMMU device. >> + */ >> +#define XEN_DMOP_create_viommu 15 >> + >> +struct xen_dm_op_create_viommu { >> + /* IN - MMIO base address of vIOMMU */ > > Any limit? Can it be zero? In current patchset, base address is allocated by toolstack and passed to Qemu to create vIOMMU in hyervisor. Toolstack should make sure the range won't be conflicted with other resource. > >> + uint64_t base_address; >> + /* IN - Length of MMIO region */ > > Any restrictions? Can it be say 2 bytes? Or is this in page-size granularity? From the VTD spec, register size must be an integer multiple of 4KB and I think the vIOMMU device model(E,G vvtd) in hypervisor should check the lengh. Different vendor may have different restriction. > >> + uint64_t length; >> + /* IN - Capabilities with which we want to create */ >> + uint64_t capabilities; > > That sounds like some form of flags? Yes, this patchset just introduces interrupt remapping flag and other vendor also can use it to add new features. > >> + /* OUT - vIOMMU identity */ >> + uint32_t viommu_id; >> +}; >> + >> +/* >> + * XEN_DMOP_destroy_viommu: Destroy vIOMMU device. >> + */ >> +#define XEN_DMOP_destroy_viommu 16 >> + >> +struct xen_dm_op_destroy_viommu { >> + /* OUT - vIOMMU identity */ > > Out? Not in? Sorry, it should be OUT parameter. > >> + uint32_t viommu_id; >> +}; >> + >> +/* >> + * XEN_DMOP_q_viommu: Query vIOMMU capabilities. >> + */ >> +#define XEN_DMOP_query_viommu_caps 17 >> + >> +struct xen_dm_op_query_viommu_caps { >> + /* OUT - vIOMMU Capabilities*/ > > Don't you need to also mention which vIOMMU? As you > could have potentially many of them? If we want to support different vendors' vIOMMU, it's necessary to do that and we need to introduce a new field "vIOMMU type" (E,G Intel, AMD and ARM IOMMU). > >> + uint64_t caps; >> +}; >> + >> struct xen_dm_op { >> uint32_t op; >> uint32_t pad; >> @@ -336,6 +372,9 @@ struct xen_dm_op { >> struct xen_dm_op_set_mem_type set_mem_type; >> struct xen_dm_op_inject_event inject_event; >> struct xen_dm_op_inject_msi inject_msi; >> + struct xen_dm_op_create_viommu create_viommu; >> + struct xen_dm_op_destroy_viommu destroy_viommu; >> + struct xen_dm_op_query_viommu_caps query_viommu_caps; >> } u; >> }; >> >> -- >> 1.8.3.1 >> >> >> _______________________________________________ >> Xen-devel mailing list >> Xen-devel@lists.xen.org >> https://lists.xen.org/xen-devel
On Tue, Apr 18, 2017 at 03:24:35PM +0800, Lan Tianyu wrote: > Hi Konrad: > Thanks for your review. > > On 2017年04月17日 22:36, Konrad Rzeszutek Wilk wrote: > > On Fri, Mar 17, 2017 at 07:27:02PM +0800, Lan Tianyu wrote: > >> This patch is to introduce create, destroy and query capabilities > >> command for vIOMMU. vIOMMU layer will deal with requests and call > >> arch vIOMMU ops. > >> > >> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com> > >> --- > >> xen/arch/x86/hvm/dm.c | 29 +++++++++++++++++++++++++++++ > >> xen/include/public/hvm/dm_op.h | 39 +++++++++++++++++++++++++++++++++++++++ > >> 2 files changed, 68 insertions(+) > >> > >> diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c > >> index 2122c45..2b28f70 100644 > >> --- a/xen/arch/x86/hvm/dm.c > >> +++ b/xen/arch/x86/hvm/dm.c > >> @@ -491,6 +491,35 @@ static int dm_op(domid_t domid, > >> break; > >> } > >> > >> + case XEN_DMOP_create_viommu: > >> + { > >> + struct xen_dm_op_create_viommu *data = > >> + &op.u.create_viommu; > >> + > >> + rc = viommu_create(d, data->base_address, data->length, data->capabilities); > >> + if (rc >= 0) { > > > > The style guide is is to have a space here and { on a newline. > > Yes, will fix. > > > > >> + data->viommu_id = rc; > >> + rc = 0; > >> + } > >> + break; > >> + } > > > > Newline here.. > > > > > >> + case XEN_DMOP_destroy_viommu: > >> + { > >> + const struct xen_dm_op_destroy_viommu *data = > >> + &op.u.destroy_viommu; > >> + > >> + rc = viommu_destroy(d, data->viommu_id); > >> + break; > >> + } > > > > Ahem? > >> + case XEN_DMOP_query_viommu_caps: > >> + { > >> + struct xen_dm_op_query_viommu_caps *data = > >> + &op.u.query_viommu_caps; > >> + > >> + data->caps = viommu_query_caps(d); > >> + rc = 0; > >> + break; > >> + } > > > > And here. > >> default: > >> rc = -EOPNOTSUPP; > >> break; > >> diff --git a/xen/include/public/hvm/dm_op.h b/xen/include/public/hvm/dm_op.h > >> index f54cece..b8c7359 100644 > >> --- a/xen/include/public/hvm/dm_op.h > >> +++ b/xen/include/public/hvm/dm_op.h > >> @@ -318,6 +318,42 @@ struct xen_dm_op_inject_msi { > >> uint64_aligned_t addr; > >> }; > >> > >> +/* > >> + * XEN_DMOP_create_viommu: Create vIOMMU device. > >> + */ > >> +#define XEN_DMOP_create_viommu 15 > >> + > >> +struct xen_dm_op_create_viommu { > >> + /* IN - MMIO base address of vIOMMU */ > > > > Any limit? Can it be zero? > > In current patchset, base address is allocated by toolstack and passed > to Qemu to create vIOMMU in hyervisor. Toolstack should make sure the > range won't be conflicted with other resource. Sure, but the hypervisor should also do some sanity checking. Having some idea of limits/sizes would be quite helpfull. Either in the code or in this comment. > > > > >> + uint64_t base_address; > >> + /* IN - Length of MMIO region */ > > > > Any restrictions? Can it be say 2 bytes? Or is this in page-size granularity? > > >From the VTD spec, register size must be an integer multiple of 4KB and > I think the vIOMMU device model(E,G vvtd) in hypervisor should check the > lengh. Different vendor may have different restriction. Right, but I thinking you should document this, or at least make it clear what the expectations are. > > > > >> + uint64_t length; > >> + /* IN - Capabilities with which we want to create */ > >> + uint64_t capabilities; > > > > That sounds like some form of flags? > > Yes, this patchset just introduces interrupt remapping flag and other > vendor also can use it to add new features. > > > > >> + /* OUT - vIOMMU identity */ > >> + uint32_t viommu_id; > >> +}; > >> + > >> +/* > >> + * XEN_DMOP_destroy_viommu: Destroy vIOMMU device. > >> + */ > >> +#define XEN_DMOP_destroy_viommu 16 > >> + > >> +struct xen_dm_op_destroy_viommu { > >> + /* OUT - vIOMMU identity */ > > > > Out? Not in? > > Sorry, it should be OUT parameter. > > > > >> + uint32_t viommu_id; > >> +}; > >> + > >> +/* > >> + * XEN_DMOP_q_viommu: Query vIOMMU capabilities. > >> + */ > >> +#define XEN_DMOP_query_viommu_caps 17 > >> + > >> +struct xen_dm_op_query_viommu_caps { > >> + /* OUT - vIOMMU Capabilities*/ > > > > Don't you need to also mention which vIOMMU? As you > > could have potentially many of them? > > If we want to support different vendors' vIOMMU, it's necessary to do > that and we need to introduce a new field "vIOMMU type" (E,G Intel, AMD > and ARM IOMMU). Right, but the 'xen_dm_op_create_viommu' has the capabilities and retuirns the vIOMMU identity. It looks like it could be called multiple times which means you could have multiple vIOMMU and each could have a different capability. As such this call should also have as IN the vIOMMU identity to at least be symmetrical with the other hypercalls.
diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c index 2122c45..2b28f70 100644 --- a/xen/arch/x86/hvm/dm.c +++ b/xen/arch/x86/hvm/dm.c @@ -491,6 +491,35 @@ static int dm_op(domid_t domid, break; } + case XEN_DMOP_create_viommu: + { + struct xen_dm_op_create_viommu *data = + &op.u.create_viommu; + + rc = viommu_create(d, data->base_address, data->length, data->capabilities); + if (rc >= 0) { + data->viommu_id = rc; + rc = 0; + } + break; + } + case XEN_DMOP_destroy_viommu: + { + const struct xen_dm_op_destroy_viommu *data = + &op.u.destroy_viommu; + + rc = viommu_destroy(d, data->viommu_id); + break; + } + case XEN_DMOP_query_viommu_caps: + { + struct xen_dm_op_query_viommu_caps *data = + &op.u.query_viommu_caps; + + data->caps = viommu_query_caps(d); + rc = 0; + break; + } default: rc = -EOPNOTSUPP; break; diff --git a/xen/include/public/hvm/dm_op.h b/xen/include/public/hvm/dm_op.h index f54cece..b8c7359 100644 --- a/xen/include/public/hvm/dm_op.h +++ b/xen/include/public/hvm/dm_op.h @@ -318,6 +318,42 @@ struct xen_dm_op_inject_msi { uint64_aligned_t addr; }; +/* + * XEN_DMOP_create_viommu: Create vIOMMU device. + */ +#define XEN_DMOP_create_viommu 15 + +struct xen_dm_op_create_viommu { + /* IN - MMIO base address of vIOMMU */ + uint64_t base_address; + /* IN - Length of MMIO region */ + uint64_t length; + /* IN - Capabilities with which we want to create */ + uint64_t capabilities; + /* OUT - vIOMMU identity */ + uint32_t viommu_id; +}; + +/* + * XEN_DMOP_destroy_viommu: Destroy vIOMMU device. + */ +#define XEN_DMOP_destroy_viommu 16 + +struct xen_dm_op_destroy_viommu { + /* OUT - vIOMMU identity */ + uint32_t viommu_id; +}; + +/* + * XEN_DMOP_q_viommu: Query vIOMMU capabilities. + */ +#define XEN_DMOP_query_viommu_caps 17 + +struct xen_dm_op_query_viommu_caps { + /* OUT - vIOMMU Capabilities*/ + uint64_t caps; +}; + struct xen_dm_op { uint32_t op; uint32_t pad; @@ -336,6 +372,9 @@ struct xen_dm_op { struct xen_dm_op_set_mem_type set_mem_type; struct xen_dm_op_inject_event inject_event; struct xen_dm_op_inject_msi inject_msi; + struct xen_dm_op_create_viommu create_viommu; + struct xen_dm_op_destroy_viommu destroy_viommu; + struct xen_dm_op_query_viommu_caps query_viommu_caps; } u; };
This patch is to introduce create, destroy and query capabilities command for vIOMMU. vIOMMU layer will deal with requests and call arch vIOMMU ops. Signed-off-by: Lan Tianyu <tianyu.lan@intel.com> --- xen/arch/x86/hvm/dm.c | 29 +++++++++++++++++++++++++++++ xen/include/public/hvm/dm_op.h | 39 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 68 insertions(+)