Re: [Xen-devel] [PATCH v4 05/28] VIOMMU: Introduce callback of checking irq remapping mode

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

On Sat, Feb 10, 2018 at 12:47:07AM +0800, Chao Gao wrote:
> On Fri, Feb 09, 2018 at 03:11:25PM +0000, Roger Pau Monné wrote:
> >On Fri, Nov 17, 2017 at 02:22:12PM +0800, Chao Gao wrote:
> >> From: Lan Tianyu <tianyu.lan@xxxxxxxxx>
> >> 
> >> This patch is to add callback for vIOAPIC and vMSI to check whether 
> >> interrupt
> >> remapping is enabled.
> >
> >Same as with the previous patches, not adding the actual code in
> >check_irq_remapping makes reviewing this impossible.
> >
> >> 
> >> Signed-off-by: Lan Tianyu <tianyu.lan@xxxxxxxxx>
> >> Signed-off-by: Chao Gao <chao.gao@xxxxxxxxx>
> >> ---
> >>  xen/common/viommu.c      | 15 +++++++++++++++
> >>  xen/include/xen/viommu.h |  4 ++++
> >>  2 files changed, 19 insertions(+)
> >> 
> >> diff --git a/xen/common/viommu.c b/xen/common/viommu.c
> >> index 9eafdef..72173c3 100644
> >> --- a/xen/common/viommu.c
> >> +++ b/xen/common/viommu.c
> >> @@ -145,6 +145,21 @@ int viommu_get_irq_info(const struct domain *d,
> >>      return viommu->ops->get_irq_info(d, request, irq_info);
> >>  }
> >>  
> >> +bool viommu_check_irq_remapping(const struct domain *d,
> >> +                                const struct arch_irq_remapping_request 
> >> *request)
> >> +{
> >> +    const struct viommu *viommu = d->arch.hvm_domain.viommu;
> >> +
> >> +    if ( !viommu )
> >> +        return false;
> >> +
> >> +    ASSERT(viommu->ops);
> >> +    if ( !viommu->ops->check_irq_remapping )
> >> +        return false;
> >> +
> >> +    return viommu->ops->check_irq_remapping(d, request);
> >> +}
> >
> >Having a helper for each functionality you want to support seems
> >extremely cumbersome, I would image this to grow so that you will also
> >have viommu_check_mem_mapping and others.
> >
> >Isn't it better to just have something like viommu_check_feature, or
> >even just expose a features field in the viommu struct itself?
> 
> Maybe it is caused by our poor function name and no comments to point
> out what the function does.  As you know, interrupts has two formats:
> legacy format and remappable format.  The format is indicated by one bit
> of MSI msg or IOAPIC RTE. Roughly, only remappable format should be
> translated by IOMMU. So every time we want to handle an interrupt, we
> should know its format and we think the remappable format varies
> from different vendors. This is why we introduce a new field here in
> order to abstract the check of remapping format.

Oh, I see. So this is used to check whether each interrupt needs
remapping or not, it's not used to check whether the arch specific
vIOMMU implementation supports interrupt remapping.

I would maybe rename this to 'check_irq_remapped' or
'check_intr_remapped'.

Thanks, Roger.

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