Re: [RFC PATCH 3/23] VIOMMU: Add irq request callback to deal with irq remapping

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Tue, Apr 18, 2017 at 04:18:52PM +0800, Lan Tianyu wrote:
> On 2017年04月17日 22:39, Konrad Rzeszutek Wilk wrote:
> > On Fri, Mar 17, 2017 at 07:27:03PM +0800, Lan Tianyu wrote:
> >> This patch is to add irq request callback for platform implementation
> >> to deal with irq remapping request.
> >>
> >> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
> >> ---
> >>  xen/common/viommu.c          | 11 +++++++++++
> >>  xen/include/asm-arm/viommu.h |  4 ++++
> >>  xen/include/asm-x86/viommu.h | 15 +++++++++++++++
> >>  xen/include/xen/viommu.h     |  8 ++++++++
> >>  4 files changed, 38 insertions(+)
> >>
> >> diff --git a/xen/common/viommu.c b/xen/common/viommu.c
> >> index 4c1c788..62c66db 100644
> >> --- a/xen/common/viommu.c
> >> +++ b/xen/common/viommu.c
> >> @@ -87,6 +87,17 @@ u64 viommu_query_caps(struct domain *d)
> >>      return info->ops->query_caps(d);
> >>  }
> >>  
> >> +int viommu_handle_irq_request(struct domain *d,
> >> +        struct irq_remapping_request *request)
> >> +{
> >> +    struct viommu_info *info = &d->viommu;
> >> +
> >> +    if ( !info || !info->ops || !info->ops->handle_irq_request)
> > 
> > You are missing an space at the end.
> 
> Yes, will fix.
> 
> >> +        return -EINVAL;
> >> +
> >> +    return info->ops->handle_irq_request(d, request);
> >> +}
> >> +
> >>  /*
> >>   * Local variables:
> >>   * mode: C
> >> diff --git a/xen/include/asm-arm/viommu.h b/xen/include/asm-arm/viommu.h
> >> index ef6a60b..6a81ecb 100644
> >> --- a/xen/include/asm-arm/viommu.h
> >> +++ b/xen/include/asm-arm/viommu.h
> >> @@ -22,6 +22,10 @@
> >>  
> >>  #include <xen/viommu.h>
> >>  
> >> +struct irq_remapping_request
> >> +{
> >> +};
> >> +
> >>  static inline const struct viommu_ops *viommu_get_ops(void)
> >>  {
> >>      return NULL;
> >> diff --git a/xen/include/asm-x86/viommu.h b/xen/include/asm-x86/viommu.h
> >> index efb435f..b6e01a5 100644
> >> --- a/xen/include/asm-x86/viommu.h
> >> +++ b/xen/include/asm-x86/viommu.h
> >> @@ -23,6 +23,21 @@
> >>  #include <xen/viommu.h>
> >>  #include <asm/types.h>
> >>  
> >> +struct irq_remapping_request
> >> +{
> >> +    u8 type;
> >> +    u16 source_id;
> >> +    union {
> >> +        /* MSI */
> >> +        struct {
> >> +            u64 addr;
> >> +            u32 data;
> >> +        } msi;
> >> +        /* Redirection Entry in IOAPIC */
> >> +        u64 rte;
> >> +    } msg;
> >> +};
> > 
> > Will this work right? As in with the default padding and such?
> 
> Sorry. Could you elaborate this?

If you run 'pahole' on the xen-syms and then look at this
structure - do the offsets match with what the hardware expects?

As in, I see this:

struct irq_remapping_request {
        u8                         type;                 /*     0     1 */

        /* XXX 1 byte hole, try to pack */

        u16                        source_id;            /*     2     2 */

        /* XXX 4 bytes hole, try to pack */

        union {
                struct {
                        u64        addr;                 /*     8     8 */
                        u32        data;                 /*    16     4 */
                } msi;                                   /*          16 */
                u64                rte;                  /*           8 */
        } msg;                                           /*     8    16 */

        /* size: 24, cachelines: 1, members: 3 */
        /* sum members: 19, holes: 2, sum holes: 5 */
        /* last cacheline: 24 bytes */
};

Is this OK? Or should the offsets be closer together?

> 
> >> +
> >>  static inline const struct viommu_ops *viommu_get_ops(void)
> >>  {
> >>      return NULL;
> >> diff --git a/xen/include/xen/viommu.h b/xen/include/xen/viommu.h
> >> index a0abbdf..246b29d 100644
> >> --- a/xen/include/xen/viommu.h
> >> +++ b/xen/include/xen/viommu.h
> >> @@ -24,6 +24,10 @@
> >>  
> >>  #define NR_VIOMMU_PER_DOMAIN 1
> >>  
> >> +/* IRQ request type */
> >> +#define VIOMMU_REQUEST_IRQ_MSI          0
> >> +#define VIOMMU_REQUEST_IRQ_APIC         1
> > 
> > What is this used for?
> 
> This is to designate interrupt type of irq remapping request which
> contains in the structure irq_remapping_request. The vIOMMU device model
> uses it to parse request data.

Does it make sense to have it as part of this patch? Even thought
it is not used here?

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel