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

[Lan Tianyu <tianyu.lan@intel.com>]

On 2017年10月19日 23:42, Roger Pau Monné wrote:
> 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@intel.com>
>> ---
>>  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)

Sure. I will add some comments.

> 
>> +
>> +    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.

Yes, this makes sense.

> 
>> +};
>> +
>>  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.

Will update.

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

Did you mean to keep d and request in the same line? This will exceed 80
chars.


-- 
Best regards
Tianyu Lan

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