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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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@xxxxxxxxx>
> Signed-off-by: Lan Tianyu <tianyu.lan@xxxxxxxxx>
> ---
> 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.

Thanks, Roger.

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