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

[Roger Pau Monné <roger.pau@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;
> +
> +    if ( viommu_id >= info->nr_viommu
> +         || !info->viommu[viommu_id] )

This fits on the same line, no need to split it.

> +        return -EINVAL;
> +
> +    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__
> +
> +#include <xen/viommu.h>
> +#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;
> +};
> +
> +static inline void irq_request_ioapic_fill(struct irq_remapping_request *req,
> +                             uint32_t ioapic_id, uint64_t rte)
> +{
> +    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)
> +{
> +    ASSERT(req);
> +    req->type = VIOMMU_REQUEST_IRQ_MSI;
> +    req->source_id = source_id;
> +    req->msg.msi.addr = addr;
> +    req->msg.msi.data = data;
> +}

What's the usage of those two functions? AFAICT they don't have any
callers in this patch.

> +
> +#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>
> +
>  #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);

I'm slightly lost, you add the function pointer here and some inline
functions in asm-x86/viommu.h, yet I don't see them being hooked into
the struct viommu_ops in any way.

Roger.

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