Re: [Xen-devel] [PATCH V2 2/25] VIOMMU: Add irq request callback to deal with irq remapping

[Lan Tianyu <tianyu.lan@xxxxxxxxx>]

On 2017年08月17日 19:18, Wei Liu wrote:
> On Wed, Aug 09, 2017 at 04:34:03PM -0400, Lan Tianyu wrote:
>> 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>
>> ---
>>  xen/common/viommu.c          | 15 +++++++++
>>  xen/include/asm-x86/viommu.h | 73 
>> ++++++++++++++++++++++++++++++++++++++++++++
>>  xen/include/xen/viommu.h     |  9 ++++++
>>  3 files changed, 97 insertions(+)
>>  create mode 100644 xen/include/asm-x86/viommu.h
>>
>> diff --git a/xen/common/viommu.c b/xen/common/viommu.c
>> index a4d004d..f4d34e6 100644
>> --- a/xen/common/viommu.c
>> +++ b/xen/common/viommu.c
>> @@ -198,6 +198,21 @@ int __init viommu_setup(void)
>>      return 0;
>>  }
>>  
>> +int viommu_handle_irq_request(struct domain *d, u32 viommu_id,
>> +                              struct irq_remapping_request *request)
>> +{
>> +    struct viommu_info *info = &d->viommu;
> 
> Does this compile? This patch and the previous one don't have viommu
> added to struct domain.

Oh. Sorry. Missed patch "VIOMMU: Add vIOMMU helper functions to create,
 destroy and query capabilities" in this series. I just sent out and
followed this mail. Please have a look.

> 
>> +
>> +    if ( viommu_id >= info->nr_viommu
>> +         || !info->viommu[viommu_id] )
> 
> Join this to previous line?
>

OK.

>> +        return -EINVAL;
> 
> ASSERT(info->viommu[viommu_id]->ops);
> 
> For extra safety.

Or check ops in the previous if?

> 
>> +
>> +    if ( !info->viommu[viommu_id]->ops->handle_irq_request )
>> +        return -EINVAL;
>> +
>> +    return info->viommu[viommu_id]->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..51bda72
>> --- /dev/null
>> +++ b/xen/include/asm-x86/viommu.h
>> @@ -0,0 +1,73 @@
>> +/*
>> + * 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__
>> +
> 
> Is a corresponding ARM header needed? Given viommu is common code.

I added such ARM header file in previous version but Julien hope vIOMMU
should be disabled for ARM because ARM doesn't support vIOMMU so far.

> 
>> +#include <xen/viommu.h>
> 
> I think you're probably doing it wrong.
> 
> It should be that the common header header includes arch header, then
> the code only uses the common header (I haven't read the rest of your
> series at this point).

OK. Will fix it.

> 
>> +#include <asm/types.h>
>> +
>> +/* IRQ request type */
>> +#define VIOMMU_REQUEST_IRQ_MSI          0
>> +#define VIOMMU_REQUEST_IRQ_APIC         1
>> +
>> +struct irq_remapping_request
>> +{
>> +    union {
>> +        /* MSI */
>> +        struct {
>> +            u64 addr;
>> +            u32 data;
>> +        } msi;
>> +        /* Redirection Entry in IOAPIC */
>> +        u64 rte;
>> +    } msg;
>> +    u16 source_id;
>> +    u8 type;
> 
> uintXX_t please.

Sure.

> 
>> +};
>> +
>> +static inline void irq_request_ioapic_fill(struct irq_remapping_request 
>> *req,
>> +                             uint32_t ioapic_id, uint64_t rte)
> 
> Indentation.
> 
>> +{
>> +    ASSERT(req);
>> +    req->type = VIOMMU_REQUEST_IRQ_APIC;
>> +    req->source_id = ioapic_id;
>> +    req->msg.rte = rte;
>> +}
>> +
>> +static inline void irq_request_msi_fill(struct irq_remapping_request *req,
>> +                          uint32_t source_id, uint64_t addr, uint32_t data)
> 
> Indentation.
> 
>> +{
>> +    ASSERT(req);
>> +    req->type = VIOMMU_REQUEST_IRQ_MSI;
>> +    req->source_id = source_id;
>> +    req->msg.msi.addr = addr;
>> +    req->msg.msi.data = data;
>> +}
>> +
>> +#endif /* __ARCH_X86_VIOMMU_H__ */
>> +
>> +/*
>> + * Local variables:
>> + * mode: C
>> + * c-file-style: "BSD"
>> + * c-basic-offset: 4
>> + * tab-width: 4
>> + * End:
>> + */
>> diff --git a/xen/include/xen/viommu.h b/xen/include/xen/viommu.h
>> index 527afb1..0be1b3a 100644
>> --- a/xen/include/xen/viommu.h
>> +++ b/xen/include/xen/viommu.h
>> @@ -20,6 +20,8 @@
>>  #ifndef __XEN_VIOMMU_H__
>>  #define __XEN_VIOMMU_H__
>>  
>> +#include <asm/viommu.h>
>> +
> 
> Circular inclusion? Note the #include <xen/viommu.h> some lines above.
> 
>>  #define NR_VIOMMU_PER_DOMAIN 1
>>  
>>  struct viommu;
>> @@ -28,6 +30,8 @@ struct viommu_ops {
>>      u64 (*query_caps)(struct domain *d);
>>      int (*create)(struct domain *d, struct viommu *viommu);
>>      int (*destroy)(struct viommu *viommu);
>> +    int (*handle_irq_request)(struct domain *d,
>> +                              struct irq_remapping_request *request);
>>  };
>>  
>>  struct viommu {
>> @@ -52,6 +56,8 @@ int viommu_register_type(u64 type, struct viommu_ops * 
>> ops);
>>  int viommu_domctl(struct domain *d, struct xen_domctl_viommu_op *op,
>>                    bool_t *need_copy);
>>  int viommu_setup(void);
>> +int viommu_handle_irq_request(struct domain *d, u32 viommu_id,
>> +                              struct irq_remapping_request *request);
>>  #else
>>  static inline int viommu_init_domain(struct domain *d) { return 0; }
>>  static inline int viommu_register_type(u64 type, struct viommu_ops * ops)
>> @@ -62,6 +68,9 @@ static inline int viommu_domctl(struct domain *d,
>>                                  struct xen_domctl_viommu_op *op,
>>                                  bool *need_copy)
>>  { return -ENODEV };
>> +static inline int viommu_handle_irq_request(struct domain *d, u32 viommu_id,
>> +                              struct irq_remapping_request *request)
>> +{ return 0 };
> 
> This should fail.
> 
>>  #endif
>>  
>>  #endif /* __XEN_VIOMMU_H__ */
>> -- 
>> 1.8.3.1
>>


-- 
Best regards
Tianyu Lan

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