Re: [Xen-devel] [PATCH v4 05/28] VIOMMU: Introduce callback of checking irq remapping mode

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

On Fri, Nov 17, 2017 at 02:22:12PM +0800, Chao Gao wrote:
> From: Lan Tianyu <tianyu.lan@xxxxxxxxx>
> 
> This patch is to add callback for vIOAPIC and vMSI to check whether interrupt
> remapping is enabled.

Same as with the previous patches, not adding the actual code in
check_irq_remapping makes reviewing this impossible.

> 
> Signed-off-by: Lan Tianyu <tianyu.lan@xxxxxxxxx>
> Signed-off-by: Chao Gao <chao.gao@xxxxxxxxx>
> ---
>  xen/common/viommu.c      | 15 +++++++++++++++
>  xen/include/xen/viommu.h |  4 ++++
>  2 files changed, 19 insertions(+)
> 
> diff --git a/xen/common/viommu.c b/xen/common/viommu.c
> index 9eafdef..72173c3 100644
> --- a/xen/common/viommu.c
> +++ b/xen/common/viommu.c
> @@ -145,6 +145,21 @@ int viommu_get_irq_info(const struct domain *d,
>      return viommu->ops->get_irq_info(d, request, irq_info);
>  }
>  
> +bool viommu_check_irq_remapping(const struct domain *d,
> +                                const struct arch_irq_remapping_request 
> *request)
> +{
> +    const struct viommu *viommu = d->arch.hvm_domain.viommu;
> +
> +    if ( !viommu )
> +        return false;
> +
> +    ASSERT(viommu->ops);
> +    if ( !viommu->ops->check_irq_remapping )
> +        return false;
> +
> +    return viommu->ops->check_irq_remapping(d, request);
> +}

Having a helper for each functionality you want to support seems
extremely cumbersome, I would image this to grow so that you will also
have viommu_check_mem_mapping and others.

Isn't it better to just have something like viommu_check_feature, or
even just expose a features field in the viommu struct itself?

> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/xen/include/xen/viommu.h b/xen/include/xen/viommu.h
> index 73b853f..c1dfaec 100644
> --- a/xen/include/xen/viommu.h
> +++ b/xen/include/xen/viommu.h
> @@ -29,6 +29,8 @@ struct viommu;
>  struct viommu_ops {
>      uint8_t type;
>      int (*create)(struct domain *d, struct viommu *viommu);
> +    bool (*check_irq_remapping)(const struct domain *d,
> +                                const struct arch_irq_remapping_request 
> *request);

Why add it here, instead of at the end of the struct?

>      int (*destroy)(struct viommu *viommu);
>      int (*handle_irq_request)(const struct domain *d,
>                                const struct arch_irq_remapping_request 
> *request);
> @@ -56,6 +58,8 @@ int viommu_handle_irq_request(const struct domain *d,
>  int viommu_get_irq_info(const struct domain *d,
>                          const struct arch_irq_remapping_request *request,
>                          struct arch_irq_remapping_info *irq_info);
> +bool viommu_check_irq_remapping(const struct domain *d,
> +                                const struct arch_irq_remapping_request 
> *request);
>  #else
>  static inline int viommu_destroy_domain(struct domain *d)
>  {
> -- 
> 1.8.3.1
> 

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