| Message ID | 1506049330-11196-21-git-send-email-tianyu.lan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Sep 21, 2017 at 11:02:01PM -0400, Lan Tianyu wrote: > This patch is to add get_irq_info callback for platform implementation > to convert irq remapping request to irq info (E,G vector, dest, dest_mode > and so on). > > Signed-off-by: Lan Tianyu <tianyu.lan@intel.com> > --- > xen/common/viommu.c | 16 ++++++++++++++++ > xen/include/asm-x86/viommu.h | 8 ++++++++ > xen/include/xen/viommu.h | 14 ++++++++++++++ > 3 files changed, 38 insertions(+) > > diff --git a/xen/common/viommu.c b/xen/common/viommu.c > index b517158..0708e43 100644 > --- a/xen/common/viommu.c > +++ b/xen/common/viommu.c > @@ -178,6 +178,22 @@ int viommu_handle_irq_request(struct domain *d, > return viommu->ops->handle_irq_request(d, request); > } > > +int viommu_get_irq_info(struct domain *d, > + struct arch_irq_remapping_request *request, > + struct arch_irq_remapping_info *irq_info) > +{ > + struct viommu *viommu = d->viommu; > + > + if ( !viommu ) > + return -EINVAL; OK, here there's a check for !viommu. Can we please have this written down in the header? (ie: which functions are safe/expected to be called without a viommu) > + > + ASSERT(viommu->ops); > + if ( !viommu->ops->get_irq_info ) > + return -EINVAL; > + > + return viommu->ops->get_irq_info(d, request, irq_info); > +} > + > /* > * Local variables: > * mode: C > diff --git a/xen/include/asm-x86/viommu.h b/xen/include/asm-x86/viommu.h > index 366fbb6..586b6bd 100644 > --- a/xen/include/asm-x86/viommu.h > +++ b/xen/include/asm-x86/viommu.h > @@ -24,6 +24,14 @@ > #define VIOMMU_REQUEST_IRQ_MSI 0 > #define VIOMMU_REQUEST_IRQ_APIC 1 > > +struct arch_irq_remapping_info > +{ > + uint8_t vector; > + uint32_t dest; > + uint32_t dest_mode:1; > + uint32_t delivery_mode:3; Why uint32_t for this two last fields? Also please sort them so that the padding is limited at the end of the structure. > +}; > + > struct arch_irq_remapping_request > { > union { > diff --git a/xen/include/xen/viommu.h b/xen/include/xen/viommu.h > index 230f6b1..beb40cd 100644 > --- a/xen/include/xen/viommu.h > +++ b/xen/include/xen/viommu.h > @@ -21,6 +21,7 @@ > #define __XEN_VIOMMU_H__ > > struct viommu; > +struct arch_irq_remapping_info; > struct arch_irq_remapping_request; If you include asm/viommu.h in viommu.h you don't need to forward declarations. > > struct viommu_ops { > @@ -28,6 +29,9 @@ struct viommu_ops { > int (*destroy)(struct viommu *viommu); > int (*handle_irq_request)(struct domain *d, > struct arch_irq_remapping_request *request); > + int (*get_irq_info)(struct domain *d, > + struct arch_irq_remapping_request *request, AFAICT d and request should be constified. Roger.
On 2017年10月19日 23:42, Roger Pau Monné wrote: > On Thu, Sep 21, 2017 at 11:02:01PM -0400, Lan Tianyu wrote: >> This patch is to add get_irq_info callback for platform implementation >> to convert irq remapping request to irq info (E,G vector, dest, dest_mode >> and so on). >> >> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com> >> --- >> xen/common/viommu.c | 16 ++++++++++++++++ >> xen/include/asm-x86/viommu.h | 8 ++++++++ >> xen/include/xen/viommu.h | 14 ++++++++++++++ >> 3 files changed, 38 insertions(+) >> >> diff --git a/xen/common/viommu.c b/xen/common/viommu.c >> index b517158..0708e43 100644 >> --- a/xen/common/viommu.c >> +++ b/xen/common/viommu.c >> @@ -178,6 +178,22 @@ int viommu_handle_irq_request(struct domain *d, >> return viommu->ops->handle_irq_request(d, request); >> } >> >> +int viommu_get_irq_info(struct domain *d, >> + struct arch_irq_remapping_request *request, >> + struct arch_irq_remapping_info *irq_info) >> +{ >> + struct viommu *viommu = d->viommu; >> + >> + if ( !viommu ) >> + return -EINVAL; > > OK, here there's a check for !viommu. Can we please have this written > down in the header? (ie: which functions are safe/expected to be > called without a viommu) Sure. I will add some comments. > >> + >> + ASSERT(viommu->ops); >> + if ( !viommu->ops->get_irq_info ) >> + return -EINVAL; >> + >> + return viommu->ops->get_irq_info(d, request, irq_info); >> +} >> + >> /* >> * Local variables: >> * mode: C >> diff --git a/xen/include/asm-x86/viommu.h b/xen/include/asm-x86/viommu.h >> index 366fbb6..586b6bd 100644 >> --- a/xen/include/asm-x86/viommu.h >> +++ b/xen/include/asm-x86/viommu.h >> @@ -24,6 +24,14 @@ >> #define VIOMMU_REQUEST_IRQ_MSI 0 >> #define VIOMMU_REQUEST_IRQ_APIC 1 >> >> +struct arch_irq_remapping_info >> +{ >> + uint8_t vector; >> + uint32_t dest; >> + uint32_t dest_mode:1; >> + uint32_t delivery_mode:3; > > Why uint32_t for this two last fields? Also please sort them so that > the padding is limited at the end of the structure. Yes, this makes sense. > >> +}; >> + >> struct arch_irq_remapping_request >> { >> union { >> diff --git a/xen/include/xen/viommu.h b/xen/include/xen/viommu.h >> index 230f6b1..beb40cd 100644 >> --- a/xen/include/xen/viommu.h >> +++ b/xen/include/xen/viommu.h >> @@ -21,6 +21,7 @@ >> #define __XEN_VIOMMU_H__ >> >> struct viommu; >> +struct arch_irq_remapping_info; >> struct arch_irq_remapping_request; > > If you include asm/viommu.h in viommu.h you don't need to forward > declarations. Will update. > >> >> struct viommu_ops { >> @@ -28,6 +29,9 @@ struct viommu_ops { >> int (*destroy)(struct viommu *viommu); >> int (*handle_irq_request)(struct domain *d, >> struct arch_irq_remapping_request *request); >> + int (*get_irq_info)(struct domain *d, >> + struct arch_irq_remapping_request *request, > > AFAICT d and request should be constified. Did you mean to keep d and request in the same line? This will exceed 80 chars.
On 2017年10月25日 15:43, Roger Pau Monné wrote: > On Wed, Oct 25, 2017 at 03:30:39PM +0800, Lan Tianyu wrote: >> On 2017年10月19日 23:42, Roger Pau Monné wrote: >>> On Thu, Sep 21, 2017 at 11:02:01PM -0400, Lan Tianyu wrote: >>> >>>> >>>> struct viommu_ops { >>>> @@ -28,6 +29,9 @@ struct viommu_ops { >>>> int (*destroy)(struct viommu *viommu); >>>> int (*handle_irq_request)(struct domain *d, >>>> struct arch_irq_remapping_request *request); >>>> + int (*get_irq_info)(struct domain *d, >>>> + struct arch_irq_remapping_request *request, >>> >>> AFAICT d and request should be constified. >> >> Did you mean to keep d and request in the same line? This will exceed 80 >> chars. > > No, I meant that the parameters of the function should be "const struct > domain *d" and "const struct arch_irq_remapping_request *request". > AFAICT you should never modify them inside of get_irq_info. > OK. I got it. This makes sense and will update.
On Wed, Oct 25, 2017 at 03:30:39PM +0800, Lan Tianyu wrote: > On 2017年10月19日 23:42, Roger Pau Monné wrote: > > On Thu, Sep 21, 2017 at 11:02:01PM -0400, Lan Tianyu wrote: > > > >> > >> struct viommu_ops { > >> @@ -28,6 +29,9 @@ struct viommu_ops { > >> int (*destroy)(struct viommu *viommu); > >> int (*handle_irq_request)(struct domain *d, > >> struct arch_irq_remapping_request *request); > >> + int (*get_irq_info)(struct domain *d, > >> + struct arch_irq_remapping_request *request, > > > > AFAICT d and request should be constified. > > Did you mean to keep d and request in the same line? This will exceed 80 > chars. No, I meant that the parameters of the function should be "const struct domain *d" and "const struct arch_irq_remapping_request *request". AFAICT you should never modify them inside of get_irq_info. Roger.
diff --git a/xen/common/viommu.c b/xen/common/viommu.c index b517158..0708e43 100644 --- a/xen/common/viommu.c +++ b/xen/common/viommu.c @@ -178,6 +178,22 @@ int viommu_handle_irq_request(struct domain *d, return viommu->ops->handle_irq_request(d, request); } +int viommu_get_irq_info(struct domain *d, + struct arch_irq_remapping_request *request, + struct arch_irq_remapping_info *irq_info) +{ + struct viommu *viommu = d->viommu; + + if ( !viommu ) + return -EINVAL; + + ASSERT(viommu->ops); + if ( !viommu->ops->get_irq_info ) + return -EINVAL; + + return viommu->ops->get_irq_info(d, request, irq_info); +} + /* * Local variables: * mode: C diff --git a/xen/include/asm-x86/viommu.h b/xen/include/asm-x86/viommu.h index 366fbb6..586b6bd 100644 --- a/xen/include/asm-x86/viommu.h +++ b/xen/include/asm-x86/viommu.h @@ -24,6 +24,14 @@ #define VIOMMU_REQUEST_IRQ_MSI 0 #define VIOMMU_REQUEST_IRQ_APIC 1 +struct arch_irq_remapping_info +{ + uint8_t vector; + uint32_t dest; + uint32_t dest_mode:1; + uint32_t delivery_mode:3; +}; + struct arch_irq_remapping_request { union { diff --git a/xen/include/xen/viommu.h b/xen/include/xen/viommu.h index 230f6b1..beb40cd 100644 --- a/xen/include/xen/viommu.h +++ b/xen/include/xen/viommu.h @@ -21,6 +21,7 @@ #define __XEN_VIOMMU_H__ struct viommu; +struct arch_irq_remapping_info; struct arch_irq_remapping_request; struct viommu_ops { @@ -28,6 +29,9 @@ struct viommu_ops { int (*destroy)(struct viommu *viommu); int (*handle_irq_request)(struct domain *d, struct arch_irq_remapping_request *request); + int (*get_irq_info)(struct domain *d, + struct arch_irq_remapping_request *request, + struct arch_irq_remapping_info *info); }; struct viommu { @@ -50,6 +54,9 @@ int viommu_domctl(struct domain *d, struct xen_domctl_viommu_op *op, bool_t *need_copy); int viommu_handle_irq_request(struct domain *d, struct arch_irq_remapping_request *request); +int viommu_get_irq_info(struct domain *d, + struct arch_irq_remapping_request *request, + struct arch_irq_remapping_info *irq_info); #else static inline int viommu_register_type(uint64_t type, struct viommu_ops *ops) { @@ -61,6 +68,13 @@ viommu_handle_irq_request(struct domain *d, { return -EINVAL; } +static inline int +viommu_get_irq_info(struct domain *d, + struct arch_irq_remapping_request *request, + struct arch_irq_remapping_info *irq_info); +{ + return -EINVAL; +} #endif #endif /* __XEN_VIOMMU_H__ */
This patch is to add get_irq_info callback for platform implementation to convert irq remapping request to irq info (E,G vector, dest, dest_mode and so on). Signed-off-by: Lan Tianyu <tianyu.lan@intel.com> --- xen/common/viommu.c | 16 ++++++++++++++++ xen/include/asm-x86/viommu.h | 8 ++++++++ xen/include/xen/viommu.h | 14 ++++++++++++++ 3 files changed, 38 insertions(+)