Re: [Xen-devel] [PATCH V2 2/25] VIOMMU: Add irq request callback to deal with irq remapping

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Wed, Aug 09, 2017 at 04:34:03PM -0400, 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@xxxxxxxxx>
> ---
>  xen/common/viommu.c          | 15 +++++++++
>  xen/include/asm-x86/viommu.h | 73 
> ++++++++++++++++++++++++++++++++++++++++++++
>  xen/include/xen/viommu.h     |  9 ++++++
>  3 files changed, 97 insertions(+)
>  create mode 100644 xen/include/asm-x86/viommu.h
> 
> diff --git a/xen/common/viommu.c b/xen/common/viommu.c
> index a4d004d..f4d34e6 100644
> --- a/xen/common/viommu.c
> +++ b/xen/common/viommu.c
> @@ -198,6 +198,21 @@ int __init viommu_setup(void)
>      return 0;
>  }
>  
> +int viommu_handle_irq_request(struct domain *d, u32 viommu_id,
> +                              struct irq_remapping_request *request)
> +{
> +    struct viommu_info *info = &d->viommu;

Does this compile? This patch and the previous one don't have viommu
added to struct domain.

> +
> +    if ( viommu_id >= info->nr_viommu
> +         || !info->viommu[viommu_id] )

Join this to previous line?

> +        return -EINVAL;

ASSERT(info->viommu[viommu_id]->ops);

For extra safety.

> +
> +    if ( !info->viommu[viommu_id]->ops->handle_irq_request )
> +        return -EINVAL;
> +
> +    return info->viommu[viommu_id]->ops->handle_irq_request(d, request);
> +}
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/xen/include/asm-x86/viommu.h b/xen/include/asm-x86/viommu.h
> new file mode 100644
> index 0000000..51bda72
> --- /dev/null
> +++ b/xen/include/asm-x86/viommu.h
> @@ -0,0 +1,73 @@
> +/*
> + * include/asm-x86/viommu.h
> + *
> + * Copyright (c) 2017 Intel Corporation.
> + * Author: Lan Tianyu <tianyu.lan@xxxxxxxxx> 
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along 
> with
> + * this program; If not, see <http://www.gnu.org/licenses/>.
> + *
> + */
> +#ifndef __ARCH_X86_VIOMMU_H__
> +#define __ARCH_X86_VIOMMU_H__
> +

Is a corresponding ARM header needed? Given viommu is common code.

> +#include <xen/viommu.h>

I think you're probably doing it wrong.

It should be that the common header header includes arch header, then
the code only uses the common header (I haven't read the rest of your
series at this point).

> +#include <asm/types.h>
> +
> +/* IRQ request type */
> +#define VIOMMU_REQUEST_IRQ_MSI          0
> +#define VIOMMU_REQUEST_IRQ_APIC         1
> +
> +struct irq_remapping_request
> +{
> +    union {
> +        /* MSI */
> +        struct {
> +            u64 addr;
> +            u32 data;
> +        } msi;
> +        /* Redirection Entry in IOAPIC */
> +        u64 rte;
> +    } msg;
> +    u16 source_id;
> +    u8 type;

uintXX_t please.

> +};
> +
> +static inline void irq_request_ioapic_fill(struct irq_remapping_request *req,
> +                             uint32_t ioapic_id, uint64_t rte)

Indentation.

> +{
> +    ASSERT(req);
> +    req->type = VIOMMU_REQUEST_IRQ_APIC;
> +    req->source_id = ioapic_id;
> +    req->msg.rte = rte;
> +}
> +
> +static inline void irq_request_msi_fill(struct irq_remapping_request *req,
> +                          uint32_t source_id, uint64_t addr, uint32_t data)

Indentation.

> +{
> +    ASSERT(req);
> +    req->type = VIOMMU_REQUEST_IRQ_MSI;
> +    req->source_id = source_id;
> +    req->msg.msi.addr = addr;
> +    req->msg.msi.data = data;
> +}
> +
> +#endif /* __ARCH_X86_VIOMMU_H__ */
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 4
> + * End:
> + */
> diff --git a/xen/include/xen/viommu.h b/xen/include/xen/viommu.h
> index 527afb1..0be1b3a 100644
> --- a/xen/include/xen/viommu.h
> +++ b/xen/include/xen/viommu.h
> @@ -20,6 +20,8 @@
>  #ifndef __XEN_VIOMMU_H__
>  #define __XEN_VIOMMU_H__
>  
> +#include <asm/viommu.h>
> +

Circular inclusion? Note the #include <xen/viommu.h> some lines above.

>  #define NR_VIOMMU_PER_DOMAIN 1
>  
>  struct viommu;
> @@ -28,6 +30,8 @@ struct viommu_ops {
>      u64 (*query_caps)(struct domain *d);
>      int (*create)(struct domain *d, struct viommu *viommu);
>      int (*destroy)(struct viommu *viommu);
> +    int (*handle_irq_request)(struct domain *d,
> +                              struct irq_remapping_request *request);
>  };
>  
>  struct viommu {
> @@ -52,6 +56,8 @@ 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);
> +int viommu_handle_irq_request(struct domain *d, u32 viommu_id,
> +                              struct irq_remapping_request *request);
>  #else
>  static inline int viommu_init_domain(struct domain *d) { return 0; }
>  static inline int viommu_register_type(u64 type, struct viommu_ops * ops)
> @@ -62,6 +68,9 @@ static inline int viommu_domctl(struct domain *d,
>                                  struct xen_domctl_viommu_op *op,
>                                  bool *need_copy)
>  { return -ENODEV };
> +static inline int viommu_handle_irq_request(struct domain *d, u32 viommu_id,
> +                              struct irq_remapping_request *request)
> +{ return 0 };

This should fail.

>  #endif
>  
>  #endif /* __XEN_VIOMMU_H__ */
> -- 
> 1.8.3.1
> 

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