Re: [Xen-devel] [PATCH v4 03/28] VIOMMU: Add irq request callback to deal with irq remapping

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Fri, Nov 17, 2017 at 02:22:10PM +0800, Chao Gao wrote:
> From: Lan Tianyu <tianyu.lan@xxxxxxxxx>
> 
> 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>
> Signed-off-by: Chao Gao <chao.gao@xxxxxxxxx>
> ---
>  xen/common/viommu.c          | 15 ++++++++++++
>  xen/include/asm-x86/viommu.h | 54 
> ++++++++++++++++++++++++++++++++++++++++++++
>  xen/include/xen/viommu.h     |  6 +++++
>  3 files changed, 75 insertions(+)
>  create mode 100644 xen/include/asm-x86/viommu.h
> 
> diff --git a/xen/common/viommu.c b/xen/common/viommu.c
> index fd8b7fd..53d4b70 100644
> --- a/xen/common/viommu.c
> +++ b/xen/common/viommu.c
> @@ -114,6 +114,21 @@ int viommu_domctl(struct domain *d, struct 
> xen_domctl_viommu_op *op)
>      return rc;
>  }
>  
> +int viommu_handle_irq_request(const struct domain *d,
> +                              const struct arch_irq_remapping_request 
> *request)
> +{
> +    struct viommu *viommu = d->arch.hvm_domain.viommu;
> +
> +    if ( !viommu )
> +        return -ENODEV;
> +
> +    ASSERT(viommu->ops);
> +    if ( !viommu->ops->handle_irq_request )
> +        return -EINVAL;

EOPNOTSUPP? EINVAL seems like something the handler itself should
return when processing the inputs.

> +
> +    return viommu->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..01ec80e
> --- /dev/null
> +++ b/xen/include/asm-x86/viommu.h
> @@ -0,0 +1,54 @@
> +/*
> + * 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__
> +
> +/* IRQ request type */
> +enum viommu_irq_request_type {
> +    VIOMMU_REQUEST_IRQ_MSI = 0,
> +    VIOMMU_REQUEST_IRQ_APIC = 1
> +};
> +
> +struct arch_irq_remapping_request
> +{
> +    union {
> +        /* MSI */
> +        struct {
> +            uint64_t addr;
> +            uint32_t data;
> +        } msi;
> +        /* Redirection Entry in IOAPIC */
> +        uint64_t rte;
> +    } msg;

Do you really need the msg name here? IIRC we support anonymous
unions for non public structures.

> +    uint16_t source_id;
> +    enum viommu_irq_request_type type;
> +};

This structure looks fine, but it would be more helpful to introduce
the struct together with the device specific handle_irq_request
function, or else the fields inside of this struct are not relevant.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel