Re: [Xen-devel] [PATCH v4 03/28] VIOMMU: Add irq request callback to deal with irq remapping

[Chao Gao <chao.gao@xxxxxxxxx>]

On Fri, Feb 09, 2018 at 03:02:03PM +0000, Roger Pau Monné wrote:
>On Fri, Nov 17, 2017 at 02:22:10PM +0800, Chao Gao wrote:
>> From: Lan Tianyu <tianyu.lan@xxxxxxxxx>
>> 
>> 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>
>> Signed-off-by: Chao Gao <chao.gao@xxxxxxxxx>
>> ---
>>  xen/common/viommu.c          | 15 ++++++++++++
>>  xen/include/asm-x86/viommu.h | 54 
>> ++++++++++++++++++++++++++++++++++++++++++++
>>  xen/include/xen/viommu.h     |  6 +++++
>>  3 files changed, 75 insertions(+)
>>  create mode 100644 xen/include/asm-x86/viommu.h
>> 
>> diff --git a/xen/common/viommu.c b/xen/common/viommu.c
>> index fd8b7fd..53d4b70 100644
>> --- a/xen/common/viommu.c
>> +++ b/xen/common/viommu.c
>> @@ -114,6 +114,21 @@ int viommu_domctl(struct domain *d, struct 
>> xen_domctl_viommu_op *op)
>>      return rc;
>>  }
>>  
>> +int viommu_handle_irq_request(const struct domain *d,
>> +                              const struct arch_irq_remapping_request 
>> *request)
>> +{
>> +    struct viommu *viommu = d->arch.hvm_domain.viommu;
>> +
>> +    if ( !viommu )
>> +        return -ENODEV;
>> +
>> +    ASSERT(viommu->ops);
>> +    if ( !viommu->ops->handle_irq_request )
>> +        return -EINVAL;
>
>EOPNOTSUPP? EINVAL seems like something the handler itself should
>return when processing the inputs.

I also prefer EOPNOTSUPP.

>
>> +
>> +    return viommu->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..01ec80e
>> --- /dev/null
>> +++ b/xen/include/asm-x86/viommu.h
>> @@ -0,0 +1,54 @@
>> +/*
>> + * 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__
>> +
>> +/* IRQ request type */
>> +enum viommu_irq_request_type {
>> +    VIOMMU_REQUEST_IRQ_MSI = 0,
>> +    VIOMMU_REQUEST_IRQ_APIC = 1
>> +};
>> +
>> +struct arch_irq_remapping_request
>> +{
>> +    union {
>> +        /* MSI */
>> +        struct {
>> +            uint64_t addr;
>> +            uint32_t data;
>> +        } msi;
>> +        /* Redirection Entry in IOAPIC */
>> +        uint64_t rte;
>> +    } msg;
>
>Do you really need the msg name here? IIRC we support anonymous
>unions for non public structures.

will use anonymous unions.

>
>> +    uint16_t source_id;
>> +    enum viommu_irq_request_type type;
>> +};
>
>This structure looks fine, but it would be more helpful to introduce
>the struct together with the device specific handle_irq_request
>function, or else the fields inside of this struct are not relevant.

Ok, will merge related changes into this one.

Thanks
Chao

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