| Message ID | 1502310866-10450-2-git-send-email-tianyu.lan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Aug 09, 2017 at 04:34:02PM -0400, Lan Tianyu wrote: [...] > > +int viommu_domctl(struct domain *d, struct xen_domctl_viommu_op *op, > + bool *need_copy) > +{ > + int rc = -EINVAL, ret; > + > + if ( !viommu_enabled() ) > + return rc; > + > + switch ( op->cmd ) > + { > + case XEN_DOMCTL_create_viommu: > + ret = viommu_create(d, op->u.create_viommu.viommu_type, > + op->u.create_viommu.base_address, > + op->u.create_viommu.length, > + op->u.create_viommu.capabilities); Please align these with "d" in previous line. > + if ( ret >= 0 ) { Coding style is wrong. > + op->u.create_viommu.viommu_id = ret; > + *need_copy = true; > + rc = 0; /* return 0 if success */ No need to have that comment. > + } > + break; > + > + case XEN_DOMCTL_destroy_viommu: > + rc = viommu_destroy(d, op->u.destroy_viommu.viommu_id); > + break; > + > + case XEN_DOMCTL_query_viommu_caps: > + ret = viommu_query_caps(d, op->u.query_caps.viommu_type); > + if ( ret >= 0 ) > + { > + op->u.query_caps.capabilities = ret; > + rc = 0; > + } > + *need_copy = true; > + break; > + > + default: > + break; > + } > + > + return rc; > +} > + > int __init viommu_setup(void) > { > INIT_LIST_HEAD(&type_list); > diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h > index ff39762..4b10f26 100644 > --- a/xen/include/public/domctl.h > +++ b/xen/include/public/domctl.h > @@ -1149,6 +1149,56 @@ struct xen_domctl_psr_cat_op { > typedef struct xen_domctl_psr_cat_op xen_domctl_psr_cat_op_t; > DEFINE_XEN_GUEST_HANDLE(xen_domctl_psr_cat_op_t); > > +/* vIOMMU helper > + * > + * vIOMMU interface can be used to create/destroy vIOMMU and > + * query vIOMMU capabilities. > + */ > + > +/* vIOMMU type - specify vendor vIOMMU device model */ > +#define VIOMMU_TYPE_INTEL_VTD (1u << 0) Why use a bit when the types are mutually exclusive? Using a number should be fine? > + > +/* vIOMMU capabilities */ > +#define VIOMMU_CAP_IRQ_REMAPPING (1u << 0) > + > +struct xen_domctl_viommu_op { > + uint32_t cmd; > +#define XEN_DOMCTL_create_viommu 0 > +#define XEN_DOMCTL_destroy_viommu 1 > +#define XEN_DOMCTL_query_viommu_caps 2 > + union { > + struct { > + /* IN - vIOMMU type */ > + uint64_t viommu_type; > + /* > + * IN - MMIO base address of vIOMMU. vIOMMU device models > + * are in charge of to check base_address and length. > + */ > + 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; > + } create_viommu; create should be fine. > + > + struct { > + /* IN - vIOMMU identity */ > + uint32_t viommu_id; > + } destroy_viommu; destroy should be fine.
Hi Wei: Thanks for your review. On 2017年08月17日 19:18, Wei Liu wrote: > On Wed, Aug 09, 2017 at 04:34:02PM -0400, Lan Tianyu wrote: >> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h >> index ff39762..4b10f26 100644 >> --- a/xen/include/public/domctl.h >> +++ b/xen/include/public/domctl.h >> @@ -1149,6 +1149,56 @@ struct xen_domctl_psr_cat_op { >> typedef struct xen_domctl_psr_cat_op xen_domctl_psr_cat_op_t; >> DEFINE_XEN_GUEST_HANDLE(xen_domctl_psr_cat_op_t); >> >> +/* vIOMMU helper >> + * >> + * vIOMMU interface can be used to create/destroy vIOMMU and >> + * query vIOMMU capabilities. >> + */ >> + >> +/* vIOMMU type - specify vendor vIOMMU device model */ >> +#define VIOMMU_TYPE_INTEL_VTD (1u << 0) > > Why use a bit when the types are mutually exclusive? Using a number > should be fine? Yes, will update. > >> + >> +/* vIOMMU capabilities */ >> +#define VIOMMU_CAP_IRQ_REMAPPING (1u << 0) >> + >> +struct xen_domctl_viommu_op { >> + uint32_t cmd; >> +#define XEN_DOMCTL_create_viommu 0 >> +#define XEN_DOMCTL_destroy_viommu 1 >> +#define XEN_DOMCTL_query_viommu_caps 2 >> + union { >> + struct { >> + /* IN - vIOMMU type */ >> + uint64_t viommu_type; >> + /* >> + * IN - MMIO base address of vIOMMU. vIOMMU device models >> + * are in charge of to check base_address and length. >> + */ >> + 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; >> + } create_viommu; > > create should be fine. > OK. >> + >> + struct { >> + /* IN - vIOMMU identity */ >> + uint32_t viommu_id; >> + } destroy_viommu; > > destroy should be fine. > OK.
On Wed, Aug 09, 2017 at 04:34:02PM -0400, 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/common/domctl.c | 3 +++ > xen/common/viommu.c | 43 +++++++++++++++++++++++++++++++++++++ I'm confused, I don't see this file in the repo, and the cover letter doesn't mention this being based on top of any other series, where does this viommu.c file come from? > xen/include/public/domctl.h | 52 +++++++++++++++++++++++++++++++++++++++++++++ > xen/include/xen/viommu.h | 6 ++++++ > 4 files changed, 104 insertions(+) > > diff --git a/xen/common/domctl.c b/xen/common/domctl.c > index d80488b..01c3024 100644 > --- a/xen/common/domctl.c > +++ b/xen/common/domctl.c > @@ -1144,6 +1144,9 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl) > if ( !ret ) > copyback = 1; > break; > + case XEN_DOMCTL_viommu_op: > + ret = viommu_domctl(d, &op->u.viommu_op, ©back); > + break; Hm, shouldn't this be protected with #ifdef CONFIG_VIOMMU? > default: > ret = arch_do_domctl(op, d, u_domctl); > diff --git a/xen/common/viommu.c b/xen/common/viommu.c > index 6874d9f..a4d004d 100644 > --- a/xen/common/viommu.c > +++ b/xen/common/viommu.c > @@ -148,6 +148,49 @@ static u64 viommu_query_caps(struct domain *d, u64 type) > return viommu_type->ops->query_caps(d); > } > > +int viommu_domctl(struct domain *d, struct xen_domctl_viommu_op *op, > + bool *need_copy) > +{ > + int rc = -EINVAL, ret; Do you really need both ret and rc? > + if ( !viommu_enabled() ) > + return rc; EINVAL? Maybe ENODEV? > + > + switch ( op->cmd ) > + { > + case XEN_DOMCTL_create_viommu: > + ret = viommu_create(d, op->u.create_viommu.viommu_type, > + op->u.create_viommu.base_address, > + op->u.create_viommu.length, > + op->u.create_viommu.capabilities); I would rather prefer for viommu_create to simply return an error or 0, and store the viommu_id by passing a pointer parameter to viommu_create, ie: rc = viommu_create(d, op->u.create_viommu.viommu_type, op->u.create_viommu.base_address, op->u.create_viommu.length, op->u.create_viommu.capabilities, &op->u.create_viommu.viommu_id); > + if ( ret >= 0 ) { ^ coding style > + op->u.create_viommu.viommu_id = ret; > + *need_copy = true; > + rc = 0; /* return 0 if success */ > + } > + break; > + > + case XEN_DOMCTL_destroy_viommu: > + rc = viommu_destroy(d, op->u.destroy_viommu.viommu_id); > + break; > + > + case XEN_DOMCTL_query_viommu_caps: > + ret = viommu_query_caps(d, op->u.query_caps.viommu_type); Same here, I would rather pass another parameter and use the return for error only. > + if ( ret >= 0 ) > + { > + op->u.query_caps.capabilities = ret; > + rc = 0; > + } > + *need_copy = true; > + break; > + > + default: > + break; Here you should return ENOSYS. > + } > + > + return rc; > +} > + > int __init viommu_setup(void) > { > INIT_LIST_HEAD(&type_list); > diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h > index ff39762..4b10f26 100644 > --- a/xen/include/public/domctl.h > +++ b/xen/include/public/domctl.h > @@ -1149,6 +1149,56 @@ struct xen_domctl_psr_cat_op { > typedef struct xen_domctl_psr_cat_op xen_domctl_psr_cat_op_t; > DEFINE_XEN_GUEST_HANDLE(xen_domctl_psr_cat_op_t); > > +/* vIOMMU helper > + * > + * vIOMMU interface can be used to create/destroy vIOMMU and > + * query vIOMMU capabilities. > + */ > + > +/* vIOMMU type - specify vendor vIOMMU device model */ > +#define VIOMMU_TYPE_INTEL_VTD (1u << 0) If this going to be used to specify the vendor only, it doesn't need to be a bitfield, because it doesn't make sense to specify for example VIOMMU_TYPE_INTEL_VTD | VIOMMU_TYPE_AMD, it's either Intel or AMD. Do you have plans to expand this with other uses? In which case the comment should be fixed. > + > +/* vIOMMU capabilities */ > +#define VIOMMU_CAP_IRQ_REMAPPING (1u << 0) > + > +struct xen_domctl_viommu_op { > + uint32_t cmd; > +#define XEN_DOMCTL_create_viommu 0 > +#define XEN_DOMCTL_destroy_viommu 1 > +#define XEN_DOMCTL_query_viommu_caps 2 > + union { > + struct { > + /* IN - vIOMMU type */ > + uint64_t viommu_type; > + /* > + * IN - MMIO base address of vIOMMU. vIOMMU device models > + * are in charge of to check base_address and length. > + */ > + uint64_t base_address; > + /* IN - Length of MMIO region */ > + uint64_t length; It seems weird that you can specify the length, is that something that a user would like to set? Isn't the length of the IOMMU MMIO region fixed by the hardware spec? > + /* IN - Capabilities with which we want to create */ > + uint64_t capabilities; > + /* OUT - vIOMMU identity */ > + uint32_t viommu_id; > + } create_viommu; > + > + struct { > + /* IN - vIOMMU identity */ > + uint32_t viommu_id; > + } destroy_viommu; > + > + struct { > + /* IN - vIOMMU type */ > + uint64_t viommu_type; > + /* OUT - vIOMMU Capabilities */ > + uint64_t capabilities; > + } query_caps; This also seems weird, shouldn't you query the capabilities of an already created vIOMMU, rather than a vIOMMU type? Shouldn't the first field be viommu_id? Roger.
Hi Roger: Thanks for your review. On 2017年08月22日 22:32, Roger Pau Monné wrote: > On Wed, Aug 09, 2017 at 04:34:02PM -0400, 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/common/domctl.c | 3 +++ >> xen/common/viommu.c | 43 +++++++++++++++++++++++++++++++++++++ > > I'm confused, I don't see this file in the repo, and the cover letter > doesn't mention this being based on top of any other series, where > does this viommu.c file come from? > >> xen/include/public/domctl.h | 52 +++++++++++++++++++++++++++++++++++++++++++++ >> xen/include/xen/viommu.h | 6 ++++++ >> 4 files changed, 104 insertions(+) >> >> diff --git a/xen/common/domctl.c b/xen/common/domctl.c >> index d80488b..01c3024 100644 >> --- a/xen/common/domctl.c >> +++ b/xen/common/domctl.c >> @@ -1144,6 +1144,9 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl) >> if ( !ret ) >> copyback = 1; >> break; >> + case XEN_DOMCTL_viommu_op: >> + ret = viommu_domctl(d, &op->u.viommu_op, ©back); >> + break; > > Hm, shouldn't this be protected with #ifdef CONFIG_VIOMMU? > Added viommu_domctl() always returns -ENODEV when CONFIG_VIOMMU is unset. >> default: >> ret = arch_do_domctl(op, d, u_domctl); >> diff --git a/xen/common/viommu.c b/xen/common/viommu.c >> index 6874d9f..a4d004d 100644 >> --- a/xen/common/viommu.c >> +++ b/xen/common/viommu.c >> @@ -148,6 +148,49 @@ static u64 viommu_query_caps(struct domain *d, u64 type) >> return viommu_type->ops->query_caps(d); >> } >> >> +int viommu_domctl(struct domain *d, struct xen_domctl_viommu_op *op, >> + bool *need_copy) >> +{ >> + int rc = -EINVAL, ret; > > Do you really need both ret and rc? > >> + if ( !viommu_enabled() ) >> + return rc; > > EINVAL? Maybe ENODEV? OK. > >> + >> + switch ( op->cmd ) >> + { >> + case XEN_DOMCTL_create_viommu: >> + ret = viommu_create(d, op->u.create_viommu.viommu_type, >> + op->u.create_viommu.base_address, >> + op->u.create_viommu.length, >> + op->u.create_viommu.capabilities); > > I would rather prefer for viommu_create to simply return an error or > 0, and store the viommu_id by passing a pointer parameter to viommu_create, ie: > > rc = viommu_create(d, op->u.create_viommu.viommu_type, > op->u.create_viommu.base_address, > op->u.create_viommu.length, > op->u.create_viommu.capabilities, > &op->u.create_viommu.viommu_id); > Got it. Will update in the next version. >> + if ( ret >= 0 ) { > ^ coding style >> + op->u.create_viommu.viommu_id = ret; >> + *need_copy = true; >> + rc = 0; /* return 0 if success */ >> + } >> + break; >> + >> + case XEN_DOMCTL_destroy_viommu: >> + rc = viommu_destroy(d, op->u.destroy_viommu.viommu_id); >> + break; >> + >> + case XEN_DOMCTL_query_viommu_caps: >> + ret = viommu_query_caps(d, op->u.query_caps.viommu_type); > > Same here, I would rather pass another parameter and use the return > for error only. > >> + if ( ret >= 0 ) >> + { >> + op->u.query_caps.capabilities = ret; >> + rc = 0; >> + } >> + *need_copy = true; >> + break; >> + >> + default: >> + break; > > Here you should return ENOSYS. OK. > >> + } >> + >> + return rc; >> +} >> + >> int __init viommu_setup(void) >> { >> INIT_LIST_HEAD(&type_list); >> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h >> index ff39762..4b10f26 100644 >> --- a/xen/include/public/domctl.h >> +++ b/xen/include/public/domctl.h >> @@ -1149,6 +1149,56 @@ struct xen_domctl_psr_cat_op { >> typedef struct xen_domctl_psr_cat_op xen_domctl_psr_cat_op_t; >> DEFINE_XEN_GUEST_HANDLE(xen_domctl_psr_cat_op_t); >> >> +/* vIOMMU helper >> + * >> + * vIOMMU interface can be used to create/destroy vIOMMU and >> + * query vIOMMU capabilities. >> + */ >> + >> +/* vIOMMU type - specify vendor vIOMMU device model */ >> +#define VIOMMU_TYPE_INTEL_VTD (1u << 0) > > If this going to be used to specify the vendor only, it doesn't need > to be a bitfield, because it doesn't make sense to specify for > example VIOMMU_TYPE_INTEL_VTD | VIOMMU_TYPE_AMD, it's either Intel or > AMD. Do you have plans to expand this with other uses? In which case > the comment should be fixed. Wei suggested to replace this bitfield with a number directly. Will update. > >> + >> +/* vIOMMU capabilities */ >> +#define VIOMMU_CAP_IRQ_REMAPPING (1u << 0) >> + >> +struct xen_domctl_viommu_op { >> + uint32_t cmd; >> +#define XEN_DOMCTL_create_viommu 0 >> +#define XEN_DOMCTL_destroy_viommu 1 >> +#define XEN_DOMCTL_query_viommu_caps 2 >> + union { >> + struct { >> + /* IN - vIOMMU type */ >> + uint64_t viommu_type; >> + /* >> + * IN - MMIO base address of vIOMMU. vIOMMU device models >> + * are in charge of to check base_address and length. >> + */ >> + uint64_t base_address; >> + /* IN - Length of MMIO region */ >> + uint64_t length; > > It seems weird that you can specify the length, is that something > that a user would like to set? Isn't the length of the IOMMU MMIO > region fixed by the hardware spec? Different vendor may have different IOMMU register region sizes. (e.g, VTD has one page size for register region). The length field is to make vIOMMU device model not to abuse address space. Some registers' offsets are reported by other register and these offsets are emulated by vIOMMU device model. If it's not necessary, we can remove it and add it when there is real such requirement. > >> + /* IN - Capabilities with which we want to create */ >> + uint64_t capabilities; >> + /* OUT - vIOMMU identity */ >> + uint32_t viommu_id; >> + } create_viommu; >> + >> + struct { >> + /* IN - vIOMMU identity */ >> + uint32_t viommu_id; >> + } destroy_viommu; >> + >> + struct { >> + /* IN - vIOMMU type */ >> + uint64_t viommu_type; >> + /* OUT - vIOMMU Capabilities */ >> + uint64_t capabilities; >> + } query_caps; > > This also seems weird, shouldn't you query the capabilities of an > already created vIOMMU, rather than a vIOMMU type? Shouldn't the first > field be viommu_id? > Query interface here is to check what capabilities the vIOMMU device model specified by viommu_type can support before create vIOMMU (suppose user may select different capabilities). If capabilities returned by query interface doesn't meet user configuration, tool stack should return error. So it just accepts viommu_type.
On Wed, Aug 23, 2017 at 02:06:17PM +0800, Lan Tianyu wrote: > Hi Roger: > Thanks for your review. > > On 2017年08月22日 22:32, Roger Pau Monné wrote: > > On Wed, Aug 09, 2017 at 04:34:02PM -0400, Lan Tianyu wrote: > >> + > >> +/* vIOMMU capabilities */ > >> +#define VIOMMU_CAP_IRQ_REMAPPING (1u << 0) > >> + > >> +struct xen_domctl_viommu_op { > >> + uint32_t cmd; > >> +#define XEN_DOMCTL_create_viommu 0 > >> +#define XEN_DOMCTL_destroy_viommu 1 > >> +#define XEN_DOMCTL_query_viommu_caps 2 > >> + union { > >> + struct { > >> + /* IN - vIOMMU type */ > >> + uint64_t viommu_type; > >> + /* > >> + * IN - MMIO base address of vIOMMU. vIOMMU device models > >> + * are in charge of to check base_address and length. > >> + */ > >> + uint64_t base_address; > >> + /* IN - Length of MMIO region */ > >> + uint64_t length; > > > > It seems weird that you can specify the length, is that something > > that a user would like to set? Isn't the length of the IOMMU MMIO > > region fixed by the hardware spec? > > Different vendor may have different IOMMU register region sizes. (e.g, > VTD has one page size for register region). The length field is to make > vIOMMU device model not to abuse address space. Some registers' offsets > are reported by other register and these offsets are emulated by vIOMMU > device model. If it's not necessary, we can remove it and add it when > there is real such requirement. So from my understanding the size of the IOMMU MMIO region is implicit in the IOMMU type that the user chooses. I don't think this field is needed. > > > >> + /* IN - Capabilities with which we want to create */ > >> + uint64_t capabilities; > >> + /* OUT - vIOMMU identity */ > >> + uint32_t viommu_id; > >> + } create_viommu; > >> + > >> + struct { > >> + /* IN - vIOMMU identity */ > >> + uint32_t viommu_id; > >> + } destroy_viommu; > >> + > >> + struct { > >> + /* IN - vIOMMU type */ > >> + uint64_t viommu_type; > >> + /* OUT - vIOMMU Capabilities */ > >> + uint64_t capabilities; > >> + } query_caps; > > > > This also seems weird, shouldn't you query the capabilities of an > > already created vIOMMU, rather than a vIOMMU type? Shouldn't the first > > field be viommu_id? > > > > Query interface here is to check what capabilities the vIOMMU device > model specified by viommu_type can support before create vIOMMU (suppose > user may select different capabilities). If capabilities returned by > query interface doesn't meet user configuration, tool stack should > return error. So it just accepts viommu_type. I don't think that's needed, if the chosen capabilities are not supported by the selected IOMMU type simply return error in XEN_DOMCTL_create_viommu. The capabilities of each IOMMU type should be listed in the man page, and the user should select a supported set or else XEN_DOMCTL_create_viommu will fail. Doing the checks both in the toolstack and in XEN_DOMCTL_create_viommu seems pointless and prone to errors. Roger.
On 2017年08月23日 15:22, Roger Pau Monné wrote: > On Wed, Aug 23, 2017 at 02:06:17PM +0800, Lan Tianyu wrote: >> Hi Roger: >> Thanks for your review. >> >> On 2017年08月22日 22:32, Roger Pau Monné wrote: >>> On Wed, Aug 09, 2017 at 04:34:02PM -0400, Lan Tianyu wrote: >>>> + >>>> +/* vIOMMU capabilities */ >>>> +#define VIOMMU_CAP_IRQ_REMAPPING (1u << 0) >>>> + >>>> +struct xen_domctl_viommu_op { >>>> + uint32_t cmd; >>>> +#define XEN_DOMCTL_create_viommu 0 >>>> +#define XEN_DOMCTL_destroy_viommu 1 >>>> +#define XEN_DOMCTL_query_viommu_caps 2 >>>> + union { >>>> + struct { >>>> + /* IN - vIOMMU type */ >>>> + uint64_t viommu_type; >>>> + /* >>>> + * IN - MMIO base address of vIOMMU. vIOMMU device models >>>> + * are in charge of to check base_address and length. >>>> + */ >>>> + uint64_t base_address; >>>> + /* IN - Length of MMIO region */ >>>> + uint64_t length; >>> >>> It seems weird that you can specify the length, is that something >>> that a user would like to set? Isn't the length of the IOMMU MMIO >>> region fixed by the hardware spec? >> >> Different vendor may have different IOMMU register region sizes. (e.g, >> VTD has one page size for register region). The length field is to make >> vIOMMU device model not to abuse address space. Some registers' offsets >> are reported by other register and these offsets are emulated by vIOMMU >> device model. If it's not necessary, we can remove it and add it when >> there is real such requirement. > > So from my understanding the size of the IOMMU MMIO region is implicit > in the IOMMU type that the user chooses. I don't think this field is > needed. OK. Will remove it.
Hi Roger, On 23/08/17 08:22, Roger Pau Monné wrote: > On Wed, Aug 23, 2017 at 02:06:17PM +0800, Lan Tianyu wrote: >> Hi Roger: >> Thanks for your review. >> >> On 2017年08月22日 22:32, Roger Pau Monné wrote: >>> On Wed, Aug 09, 2017 at 04:34:02PM -0400, Lan Tianyu wrote: >>>> + >>>> +/* vIOMMU capabilities */ >>>> +#define VIOMMU_CAP_IRQ_REMAPPING (1u << 0) >>>> + >>>> +struct xen_domctl_viommu_op { >>>> + uint32_t cmd; >>>> +#define XEN_DOMCTL_create_viommu 0 >>>> +#define XEN_DOMCTL_destroy_viommu 1 >>>> +#define XEN_DOMCTL_query_viommu_caps 2 >>>> + union { >>>> + struct { >>>> + /* IN - vIOMMU type */ >>>> + uint64_t viommu_type; >>>> + /* >>>> + * IN - MMIO base address of vIOMMU. vIOMMU device models >>>> + * are in charge of to check base_address and length. >>>> + */ >>>> + uint64_t base_address; >>>> + /* IN - Length of MMIO region */ >>>> + uint64_t length; >>> >>> It seems weird that you can specify the length, is that something >>> that a user would like to set? Isn't the length of the IOMMU MMIO >>> region fixed by the hardware spec? >> >> Different vendor may have different IOMMU register region sizes. (e.g, >> VTD has one page size for register region). The length field is to make >> vIOMMU device model not to abuse address space. Some registers' offsets >> are reported by other register and these offsets are emulated by vIOMMU >> device model. If it's not necessary, we can remove it and add it when >> there is real such requirement. > > So from my understanding the size of the IOMMU MMIO region is implicit > in the IOMMU type that the user chooses. I don't think this field is > needed. To me, it makes more sense to care both the base and the size rather than only the former. The toolstack is in charge of the address space and should be aware of the size of everything. This address space may not be static and it makes sense to give this information to Xen and verify we had the same assumption. > >>> >>>> + /* IN - Capabilities with which we want to create */ >>>> + uint64_t capabilities; >>>> + /* OUT - vIOMMU identity */ >>>> + uint32_t viommu_id; >>>> + } create_viommu; >>>> + >>>> + struct { >>>> + /* IN - vIOMMU identity */ >>>> + uint32_t viommu_id; >>>> + } destroy_viommu; >>>> + >>>> + struct { >>>> + /* IN - vIOMMU type */ >>>> + uint64_t viommu_type; >>>> + /* OUT - vIOMMU Capabilities */ >>>> + uint64_t capabilities; >>>> + } query_caps; >>> >>> This also seems weird, shouldn't you query the capabilities of an >>> already created vIOMMU, rather than a vIOMMU type? Shouldn't the first >>> field be viommu_id? >>> >> >> Query interface here is to check what capabilities the vIOMMU device >> model specified by viommu_type can support before create vIOMMU (suppose >> user may select different capabilities). If capabilities returned by >> query interface doesn't meet user configuration, tool stack should >> return error. So it just accepts viommu_type. > > I don't think that's needed, if the chosen capabilities are not > supported by the selected IOMMU type simply return error in > XEN_DOMCTL_create_viommu. > > The capabilities of each IOMMU type should be listed in the man page, > and the user should select a supported set or else > XEN_DOMCTL_create_viommu will fail. Doing the checks both in the > toolstack and in XEN_DOMCTL_create_viommu seems pointless and prone to > errors. What if the some capabilities depends on host IOMMU? How are you going to report that to the user? Cheers,
On Wed, Aug 23, 2017 at 11:19:01AM +0100, Julien Grall wrote: > Hi Roger, > > On 23/08/17 08:22, Roger Pau Monné wrote: > > On Wed, Aug 23, 2017 at 02:06:17PM +0800, Lan Tianyu wrote: > > > Hi Roger: > > > Thanks for your review. > > > > > > On 2017年08月22日 22:32, Roger Pau Monné wrote: > > > > On Wed, Aug 09, 2017 at 04:34:02PM -0400, Lan Tianyu wrote: > > > > > + > > > > > +/* vIOMMU capabilities */ > > > > > +#define VIOMMU_CAP_IRQ_REMAPPING (1u << 0) > > > > > + > > > > > +struct xen_domctl_viommu_op { > > > > > + uint32_t cmd; > > > > > +#define XEN_DOMCTL_create_viommu 0 > > > > > +#define XEN_DOMCTL_destroy_viommu 1 > > > > > +#define XEN_DOMCTL_query_viommu_caps 2 > > > > > + union { > > > > > + struct { > > > > > + /* IN - vIOMMU type */ > > > > > + uint64_t viommu_type; > > > > > + /* > > > > > + * IN - MMIO base address of vIOMMU. vIOMMU device models > > > > > + * are in charge of to check base_address and length. > > > > > + */ > > > > > + uint64_t base_address; > > > > > + /* IN - Length of MMIO region */ > > > > > + uint64_t length; > > > > > > > > It seems weird that you can specify the length, is that something > > > > that a user would like to set? Isn't the length of the IOMMU MMIO > > > > region fixed by the hardware spec? > > > > > > Different vendor may have different IOMMU register region sizes. (e.g, > > > VTD has one page size for register region). The length field is to make > > > vIOMMU device model not to abuse address space. Some registers' offsets > > > are reported by other register and these offsets are emulated by vIOMMU > > > device model. If it's not necessary, we can remove it and add it when > > > there is real such requirement. > > > > So from my understanding the size of the IOMMU MMIO region is implicit > > in the IOMMU type that the user chooses. I don't think this field is > > needed. > > To me, it makes more sense to care both the base and the size rather than > only the former. > > The toolstack is in charge of the address space and should be aware of the > size of everything. This address space may not be static and it makes sense > to give this information to Xen and verify we had the same assumption. Does this imply that we will have variable size vIOMMU MMIO regions? If not the toolstack should know the size of the MMIO region at all times, unless you are running a toolstack version != Xen version, which is not supported. > > > > > > > > > > > + /* IN - Capabilities with which we want to create */ > > > > > + uint64_t capabilities; > > > > > + /* OUT - vIOMMU identity */ > > > > > + uint32_t viommu_id; > > > > > + } create_viommu; > > > > > + > > > > > + struct { > > > > > + /* IN - vIOMMU identity */ > > > > > + uint32_t viommu_id; > > > > > + } destroy_viommu; > > > > > + > > > > > + struct { > > > > > + /* IN - vIOMMU type */ > > > > > + uint64_t viommu_type; > > > > > + /* OUT - vIOMMU Capabilities */ > > > > > + uint64_t capabilities; > > > > > + } query_caps; > > > > > > > > This also seems weird, shouldn't you query the capabilities of an > > > > already created vIOMMU, rather than a vIOMMU type? Shouldn't the first > > > > field be viommu_id? > > > > > > > > > > Query interface here is to check what capabilities the vIOMMU device > > > model specified by viommu_type can support before create vIOMMU (suppose > > > user may select different capabilities). If capabilities returned by > > > query interface doesn't meet user configuration, tool stack should > > > return error. So it just accepts viommu_type. > > > > I don't think that's needed, if the chosen capabilities are not > > supported by the selected IOMMU type simply return error in > > XEN_DOMCTL_create_viommu. > > > > The capabilities of each IOMMU type should be listed in the man page, > > and the user should select a supported set or else > > XEN_DOMCTL_create_viommu will fail. Doing the checks both in the > > toolstack and in XEN_DOMCTL_create_viommu seems pointless and prone to > > errors. > > What if the some capabilities depends on host IOMMU? How are you going to > report that to the user? I would print a message on the hypervisor console, I don't see the value of doing the same check in the toolstack that Xen will also need to do in XEN_DOMCTL_create_viommu. I would see value on having such a query hypercall once we have an implementation that indeed has different capabilities depending on the hardware, and once a xl command to fetch and print such capabilities is introduced. As said, the above query is only used to perform the checks done in XEN_DOMCTL_create_viommu on the toolstack. Roger.
Hi, On 23/08/17 15:05, Roger Pau Monné wrote: > On Wed, Aug 23, 2017 at 11:19:01AM +0100, Julien Grall wrote: >> Hi Roger, >> >> On 23/08/17 08:22, Roger Pau Monné wrote: >>> On Wed, Aug 23, 2017 at 02:06:17PM +0800, Lan Tianyu wrote: >>>> Hi Roger: >>>> Thanks for your review. >>>> >>>> On 2017年08月22日 22:32, Roger Pau Monné wrote: >>>>> On Wed, Aug 09, 2017 at 04:34:02PM -0400, Lan Tianyu wrote: >>>>>> + >>>>>> +/* vIOMMU capabilities */ >>>>>> +#define VIOMMU_CAP_IRQ_REMAPPING (1u << 0) >>>>>> + >>>>>> +struct xen_domctl_viommu_op { >>>>>> + uint32_t cmd; >>>>>> +#define XEN_DOMCTL_create_viommu 0 >>>>>> +#define XEN_DOMCTL_destroy_viommu 1 >>>>>> +#define XEN_DOMCTL_query_viommu_caps 2 >>>>>> + union { >>>>>> + struct { >>>>>> + /* IN - vIOMMU type */ >>>>>> + uint64_t viommu_type; >>>>>> + /* >>>>>> + * IN - MMIO base address of vIOMMU. vIOMMU device models >>>>>> + * are in charge of to check base_address and length. >>>>>> + */ >>>>>> + uint64_t base_address; >>>>>> + /* IN - Length of MMIO region */ >>>>>> + uint64_t length; >>>>> >>>>> It seems weird that you can specify the length, is that something >>>>> that a user would like to set? Isn't the length of the IOMMU MMIO >>>>> region fixed by the hardware spec? >>>> >>>> Different vendor may have different IOMMU register region sizes. (e.g, >>>> VTD has one page size for register region). The length field is to make >>>> vIOMMU device model not to abuse address space. Some registers' offsets >>>> are reported by other register and these offsets are emulated by vIOMMU >>>> device model. If it's not necessary, we can remove it and add it when >>>> there is real such requirement. >>> >>> So from my understanding the size of the IOMMU MMIO region is implicit >>> in the IOMMU type that the user chooses. I don't think this field is >>> needed. >> >> To me, it makes more sense to care both the base and the size rather than >> only the former. >> >> The toolstack is in charge of the address space and should be aware of the >> size of everything. This address space may not be static and it makes sense >> to give this information to Xen and verify we had the same assumption. > > Does this imply that we will have variable size vIOMMU MMIO regions? There are existing IOMMU with multiple MMIO regions. This is the case of the Nvidia SMMU. Whether we will emulate then is another question. But for completeness, I would use address/size. Note that we haven't decided on ARM whether we will emulate the IOMMU or use a PV based solution. > > If not the toolstack should know the size of the MMIO region at all > times, unless you are running a toolstack version != Xen version, > which is not supported. > >>> >>>>> >>>>>> + /* IN - Capabilities with which we want to create */ >>>>>> + uint64_t capabilities; >>>>>> + /* OUT - vIOMMU identity */ >>>>>> + uint32_t viommu_id; >>>>>> + } create_viommu; >>>>>> + >>>>>> + struct { >>>>>> + /* IN - vIOMMU identity */ >>>>>> + uint32_t viommu_id; >>>>>> + } destroy_viommu; >>>>>> + >>>>>> + struct { >>>>>> + /* IN - vIOMMU type */ >>>>>> + uint64_t viommu_type; >>>>>> + /* OUT - vIOMMU Capabilities */ >>>>>> + uint64_t capabilities; >>>>>> + } query_caps; >>>>> >>>>> This also seems weird, shouldn't you query the capabilities of an >>>>> already created vIOMMU, rather than a vIOMMU type? Shouldn't the first >>>>> field be viommu_id? >>>>> >>>> >>>> Query interface here is to check what capabilities the vIOMMU device >>>> model specified by viommu_type can support before create vIOMMU (suppose >>>> user may select different capabilities). If capabilities returned by >>>> query interface doesn't meet user configuration, tool stack should >>>> return error. So it just accepts viommu_type. >>> >>> I don't think that's needed, if the chosen capabilities are not >>> supported by the selected IOMMU type simply return error in >>> XEN_DOMCTL_create_viommu. >>> >>> The capabilities of each IOMMU type should be listed in the man page, >>> and the user should select a supported set or else >>> XEN_DOMCTL_create_viommu will fail. Doing the checks both in the >>> toolstack and in XEN_DOMCTL_create_viommu seems pointless and prone to >>> errors. >> >> What if the some capabilities depends on host IOMMU? How are you going to >> report that to the user? > > I would print a message on the hypervisor console, I don't see the > value of doing the same check in the toolstack that Xen will also need > to do in XEN_DOMCTL_create_viommu. > > I would see value on having such a query hypercall once we have an > implementation that indeed has different capabilities depending on the > hardware, and once a xl command to fetch and print such capabilities > is introduced. Fair enough. > As said, the above query is only used to perform the checks done in > XEN_DOMCTL_create_viommu on the toolstack. Cheers,
On Thu, Aug 24, 2017 at 03:03:49PM +0100, Julien Grall wrote: > Hi, > > On 23/08/17 15:05, Roger Pau Monné wrote: > > On Wed, Aug 23, 2017 at 11:19:01AM +0100, Julien Grall wrote: > > > Hi Roger, > > > > > > On 23/08/17 08:22, Roger Pau Monné wrote: > > > > On Wed, Aug 23, 2017 at 02:06:17PM +0800, Lan Tianyu wrote: > > > > > Hi Roger: > > > > > Thanks for your review. > > > > > > > > > > On 2017年08月22日 22:32, Roger Pau Monné wrote: > > > > > > On Wed, Aug 09, 2017 at 04:34:02PM -0400, Lan Tianyu wrote: > > > > > > > + > > > > > > > +/* vIOMMU capabilities */ > > > > > > > +#define VIOMMU_CAP_IRQ_REMAPPING (1u << 0) > > > > > > > + > > > > > > > +struct xen_domctl_viommu_op { > > > > > > > + uint32_t cmd; > > > > > > > +#define XEN_DOMCTL_create_viommu 0 > > > > > > > +#define XEN_DOMCTL_destroy_viommu 1 > > > > > > > +#define XEN_DOMCTL_query_viommu_caps 2 > > > > > > > + union { > > > > > > > + struct { > > > > > > > + /* IN - vIOMMU type */ > > > > > > > + uint64_t viommu_type; > > > > > > > + /* > > > > > > > + * IN - MMIO base address of vIOMMU. vIOMMU device models > > > > > > > + * are in charge of to check base_address and length. > > > > > > > + */ > > > > > > > + uint64_t base_address; > > > > > > > + /* IN - Length of MMIO region */ > > > > > > > + uint64_t length; > > > > > > > > > > > > It seems weird that you can specify the length, is that something > > > > > > that a user would like to set? Isn't the length of the IOMMU MMIO > > > > > > region fixed by the hardware spec? > > > > > > > > > > Different vendor may have different IOMMU register region sizes. (e.g, > > > > > VTD has one page size for register region). The length field is to make > > > > > vIOMMU device model not to abuse address space. Some registers' offsets > > > > > are reported by other register and these offsets are emulated by vIOMMU > > > > > device model. If it's not necessary, we can remove it and add it when > > > > > there is real such requirement. > > > > > > > > So from my understanding the size of the IOMMU MMIO region is implicit > > > > in the IOMMU type that the user chooses. I don't think this field is > > > > needed. > > > > > > To me, it makes more sense to care both the base and the size rather than > > > only the former. > > > > > > The toolstack is in charge of the address space and should be aware of the > > > size of everything. This address space may not be static and it makes sense > > > to give this information to Xen and verify we had the same assumption. > > > > Does this imply that we will have variable size vIOMMU MMIO regions? > > There are existing IOMMU with multiple MMIO regions. This is the case of the > Nvidia SMMU. Whether we will emulate then is another question. But for > completeness, I would use address/size. The proposed implementation does not support multiple MMIO regions anyway. I'm not going to oppose to this anymore, but I don't see much value on implementing things just for completeness without having a real use case, specially when this is a domctl interface that is not stable, ie: we can always modify it later on without any issues. Roger.
diff --git a/xen/common/domctl.c b/xen/common/domctl.c index d80488b..01c3024 100644 --- a/xen/common/domctl.c +++ b/xen/common/domctl.c @@ -1144,6 +1144,9 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl) if ( !ret ) copyback = 1; break; + case XEN_DOMCTL_viommu_op: + ret = viommu_domctl(d, &op->u.viommu_op, ©back); + break; default: ret = arch_do_domctl(op, d, u_domctl); diff --git a/xen/common/viommu.c b/xen/common/viommu.c index 6874d9f..a4d004d 100644 --- a/xen/common/viommu.c +++ b/xen/common/viommu.c @@ -148,6 +148,49 @@ static u64 viommu_query_caps(struct domain *d, u64 type) return viommu_type->ops->query_caps(d); } +int viommu_domctl(struct domain *d, struct xen_domctl_viommu_op *op, + bool *need_copy) +{ + int rc = -EINVAL, ret; + + if ( !viommu_enabled() ) + return rc; + + switch ( op->cmd ) + { + case XEN_DOMCTL_create_viommu: + ret = viommu_create(d, op->u.create_viommu.viommu_type, + op->u.create_viommu.base_address, + op->u.create_viommu.length, + op->u.create_viommu.capabilities); + if ( ret >= 0 ) { + op->u.create_viommu.viommu_id = ret; + *need_copy = true; + rc = 0; /* return 0 if success */ + } + break; + + case XEN_DOMCTL_destroy_viommu: + rc = viommu_destroy(d, op->u.destroy_viommu.viommu_id); + break; + + case XEN_DOMCTL_query_viommu_caps: + ret = viommu_query_caps(d, op->u.query_caps.viommu_type); + if ( ret >= 0 ) + { + op->u.query_caps.capabilities = ret; + rc = 0; + } + *need_copy = true; + break; + + default: + break; + } + + return rc; +} + int __init viommu_setup(void) { INIT_LIST_HEAD(&type_list); diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h index ff39762..4b10f26 100644 --- a/xen/include/public/domctl.h +++ b/xen/include/public/domctl.h @@ -1149,6 +1149,56 @@ struct xen_domctl_psr_cat_op { typedef struct xen_domctl_psr_cat_op xen_domctl_psr_cat_op_t; DEFINE_XEN_GUEST_HANDLE(xen_domctl_psr_cat_op_t); +/* vIOMMU helper + * + * vIOMMU interface can be used to create/destroy vIOMMU and + * query vIOMMU capabilities. + */ + +/* vIOMMU type - specify vendor vIOMMU device model */ +#define VIOMMU_TYPE_INTEL_VTD (1u << 0) + +/* vIOMMU capabilities */ +#define VIOMMU_CAP_IRQ_REMAPPING (1u << 0) + +struct xen_domctl_viommu_op { + uint32_t cmd; +#define XEN_DOMCTL_create_viommu 0 +#define XEN_DOMCTL_destroy_viommu 1 +#define XEN_DOMCTL_query_viommu_caps 2 + union { + struct { + /* IN - vIOMMU type */ + uint64_t viommu_type; + /* + * IN - MMIO base address of vIOMMU. vIOMMU device models + * are in charge of to check base_address and length. + */ + 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; + } create_viommu; + + struct { + /* IN - vIOMMU identity */ + uint32_t viommu_id; + } destroy_viommu; + + struct { + /* IN - vIOMMU type */ + uint64_t viommu_type; + /* OUT - vIOMMU Capabilities */ + uint64_t capabilities; + } query_caps; + } u; +}; +typedef struct xen_domctl_viommu_op xen_domctl_viommu_op; +DEFINE_XEN_GUEST_HANDLE(xen_domctl_viommu_op); + struct xen_domctl { uint32_t cmd; #define XEN_DOMCTL_createdomain 1 @@ -1226,6 +1276,7 @@ struct xen_domctl { #define XEN_DOMCTL_monitor_op 77 #define XEN_DOMCTL_psr_cat_op 78 #define XEN_DOMCTL_soft_reset 79 +#define XEN_DOMCTL_viommu_op 80 #define XEN_DOMCTL_gdbsx_guestmemio 1000 #define XEN_DOMCTL_gdbsx_pausevcpu 1001 #define XEN_DOMCTL_gdbsx_unpausevcpu 1002 @@ -1288,6 +1339,7 @@ struct xen_domctl { struct xen_domctl_psr_cmt_op psr_cmt_op; struct xen_domctl_monitor_op monitor_op; struct xen_domctl_psr_cat_op psr_cat_op; + struct xen_domctl_viommu_op viommu_op; uint8_t pad[128]; } u; }; diff --git a/xen/include/xen/viommu.h b/xen/include/xen/viommu.h index 506ea54..527afb1 100644 --- a/xen/include/xen/viommu.h +++ b/xen/include/xen/viommu.h @@ -49,6 +49,8 @@ extern bool_t opt_viommu; static inline bool viommu_enabled(void) { return opt_viommu; } int viommu_init_domain(struct domain *d); int viommu_register_type(u64 type, struct viommu_ops * ops); +int viommu_domctl(struct domain *d, struct xen_domctl_viommu_op *op, + bool_t *need_copy); int viommu_setup(void); #else static inline int viommu_init_domain(struct domain *d) { return 0; } @@ -56,6 +58,10 @@ static inline int viommu_register_type(u64 type, struct viommu_ops * ops) { return 0; } static inline int __init viommu_setup(void) { return 0; } static inline bool viommu_enabled(void) { return false; } +static inline int viommu_domctl(struct domain *d, + struct xen_domctl_viommu_op *op, + bool *need_copy) +{ return -ENODEV }; #endif #endif /* __XEN_VIOMMU_H__ */
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/common/domctl.c | 3 +++ xen/common/viommu.c | 43 +++++++++++++++++++++++++++++++++++++ xen/include/public/domctl.h | 52 +++++++++++++++++++++++++++++++++++++++++++++ xen/include/xen/viommu.h | 6 ++++++ 4 files changed, 104 insertions(+)