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

[Chao Gao <chao.gao@xxxxxxxxx>]

On Fri, Feb 09, 2018 at 03:11:25PM +0000, Roger Pau Monné wrote:
>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?

Maybe it is caused by our poor function name and no comments to point
out what the function does.  As you know, interrupts has two formats:
legacy format and remappable format.  The format is indicated by one bit
of MSI msg or IOAPIC RTE. Roughly, only remappable format should be
translated by IOMMU. So every time we want to handle an interrupt, we
should know its format and we think the remappable format varies
from different vendors. This is why we introduce a new field here in
order to abstract the check of remapping format.

If you could come up with a better name, that will be very helpful.

Thanks
Chao
>
>> +
>>  /*
>>   * 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