Re: [Xen-devel] [PATCH V3 20/29] VIOMMU: Add get irq info callback to convert irq remapping request

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

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@xxxxxxxxx>
> ---
>  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.

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