Re: [PATCH v4 12/28] x86/vvtd: decode interrupt attribute from IRTE

[Chao Gao <chao.gao@intel.com>]

On Mon, Feb 12, 2018 at 11:55:42AM +0000, Roger Pau Monné wrote:
>On Fri, Nov 17, 2017 at 02:22:19PM +0800, Chao Gao wrote:
>> Without interrupt remapping, interrupt attributes can be extracted from
>> msi message or IOAPIC RTE. However, with interrupt remapping enabled,
>> the attributes are enclosed in the associated IRTE. This callback is
>> for cases in which the caller wants to acquire interrupt attributes, for
>> example:
>> 1. vioapic_get_vector(). With vIOMMU, the RTE may don't contain vector.
>                                                ^ doesn't contain the vector.
>> 2. perform EOI which is always based on the interrupt vector.
>> 
>> Signed-off-by: Chao Gao <chao.gao@intel.com>
>> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
>> ---
>> v3:
>>  - add example cases in which we will use this function.
>
>I'm still missing the actual usage of vvtd_get_irq_info. This handler
>is introduced without any user.
>
>> ---
>>  xen/drivers/passthrough/vtd/vvtd.c | 25 +++++++++++++++++++++++++
>>  1 file changed, 25 insertions(+)
>> 
>> diff --git a/xen/drivers/passthrough/vtd/vvtd.c b/xen/drivers/passthrough/vtd/vvtd.c
>> index 927e715..9890cc2 100644
>> --- a/xen/drivers/passthrough/vtd/vvtd.c
>> +++ b/xen/drivers/passthrough/vtd/vvtd.c
>> @@ -541,6 +541,30 @@ static int vvtd_handle_irq_request(const struct domain *d,
>>      return ret;
>>  }
>>  
>> +static int vvtd_get_irq_info(const struct domain *d,
>
>IMO for internal (static) functions you can drop the vvtd_ prefix.
>
>> +                             const struct arch_irq_remapping_request *irq,
>> +                             struct arch_irq_remapping_info *info)
>> +{
>> +    int ret;
>> +    struct iremap_entry irte;
>> +    struct vvtd *vvtd = domain_vvtd(d);
>> +
>> +    if ( !vvtd )
>> +        return -ENODEV;
>> +
>> +    ret = vvtd_get_entry(vvtd, irq, &irte);
>> +    /* not in an interrupt delivery, don't report faults to guest */
>> +    if ( ret )
>> +        return ret;
>> +
>> +    info->vector = irte.remap.vector;
>> +    info->dest = irte_dest(vvtd, irte.remap.dst);
>> +    info->dest_mode = irte.remap.dm;
>> +    info->delivery_mode = irte.remap.dlm;
>> +
>> +    return 0;
>> +}
>> +
>>  static void vvtd_reset(struct vvtd *vvtd)
>>  {
>>      uint64_t cap = cap_set_num_fault_regs(VVTD_FRCD_NUM)
>> @@ -603,6 +627,7 @@ static const struct viommu_ops vvtd_hvm_vmx_ops = {
>>      .create = vvtd_create,
>>      .destroy = vvtd_destroy,
>>      .handle_irq_request = vvtd_handle_irq_request,
>> +    .get_irq_info = vvtd_get_irq_info,
>
>So the public helper to this arch specific hook is added in 4/28, yet
>the arch specific code is added here, and I still have to figure out
>where this will actually be hooked into the vIOAPIC or vMSI code.
>
>Would it be possible to have a single patch, which contains 4/28, the
>code in this patch and the glue that hooks this into the vIOAPIC and
>vMSI code?
>
>The above likely applies to quite a lot of patches in this series.
>It's fine to try to reduce the size of patches as much as possible,
>but at least in this series this is actually harming (at least my)
>capability to review them.

Yes. I will put related changes in one patch.

Thanks
Chao

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