Re: [Xen-devel] [PATCH 6/7] x86/IOMMU: abstract Intel-specific iommu_{en, dis}able_x2apic_IR()

["Tian, Kevin" <kevin.tian@xxxxxxxxx>]

> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: Tuesday, April 2, 2019 9:17 PM
> 
> >>> On 02.04.19 at 05:24, <kevin.tian@xxxxxxxxx> wrote:
> >>  From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> >> Sent: Friday, March 29, 2019 5:13 PM
> >>
> >> >>> On 28.03.19 at 18:37, <andrew.cooper3@xxxxxxxxxx> wrote:
> >> > On 28/03/2019 14:53, Jan Beulich wrote:
> >> >> @@ -35,6 +35,8 @@ void print_vtd_entries(struct iommu *iom
> >> >>  keyhandler_fn_t vtd_dump_iommu_info;
> >> >>
> >> >>  bool intel_iommu_supports_eim(void);
> >> >> +int intel_iommu_enable_x2apic_IR(void);
> >> >> +void intel_iommu_disable_x2apic_IR(void);
> >> >
> >> > Is there any particular reason why these retain their _IR suffix?
> >>
> >> Well, I've too been thinking about the naming here. I decided to
> >> keep the _IR suffixes because that's what the functions really
> >> do: They enable/disable interrupt remapping for x2APIC mode.
> >> They don't enable x2APIC itself in any way, and iirc x2APIC
> >> mode could be used (without wider APIC IDs and in physical
> >> mode) even without any IR enabled.
> >>
> >> > I'd suggest going with intel_iommu_{en,dis}able_eim() to match the
> >> > supports name here, whereas...
> >> >
> >> >> --- a/xen/drivers/passthrough/vtd/iommu.c
> >> >> +++ b/xen/drivers/passthrough/vtd/iommu.c
> >> >> @@ -2720,6 +2720,8 @@ const struct iommu_ops __initconstrel in
> >> >>      .free_page_table = iommu_free_page_table,
> >> >>      .reassign_device = reassign_device_ownership,
> >> >>      .get_device_group_id = intel_iommu_group_id,
> >> >> +    .enable_x2apic_IR = intel_iommu_enable_x2apic_IR,
> >> >> +    .disable_x2apic_IR = intel_iommu_disable_x2apic_IR,
> >> >>      .update_ire_from_apic = io_apic_write_remap_rte,
> >> >>      .update_ire_from_msi = msi_msg_write_remap_rte,
> >> >>      .read_apic_from_ire = io_apic_read_remap_rte,
> >> >> @@ -2736,6 +2738,7 @@ const struct iommu_ops __initconstrel in
> >> >>  };
> >> >>
> >> >>  const struct iommu_init_ops __initconstrel intel_iommu_init_ops = {
> >> >> +    .ops = &intel_iommu_ops,
> >> >>      .setup = vtd_setup,
> >> >>      .supports_x2apic = intel_iommu_supports_eim,
> >> >>  };
> >> >> --- a/xen/drivers/passthrough/x86/iommu.c
> >> >> +++ b/xen/drivers/passthrough/x86/iommu.c
> >> >> @@ -26,6 +26,24 @@
> >> >>  const struct iommu_init_ops *__initdata iommu_init_ops;
> >> >>  struct iommu_ops __read_mostly iommu_ops;
> >> >>
> >> >> +int iommu_enable_x2apic_IR(void)
> >> >
> >> > ... using iommu_{en,dis}able_x2apic() here to match the
> >> > supports_x2apic() init hook.
> >> >
> >> >
> >> > I don't think these shorter names are any more ambiguous, and loosing
> >> > the _IR suffix does make them more consistent with the rest of Xen's
> >> > function naming conventions.
> >>
> >> The above said, in the end I'm not overly fussed, but before deciding
> >> which route to go I'll wait to see whether in particular Kevin has an
> >> opinion either way.
> >>
> >
> > let's remove _IR. we have intel_iommu prefix which is sufficient
> > to indicate enable_x2apic in iommu context is about IR.
> >
> > since renaming is small thing, here is my:
> >
> >     Reviewed-by: Kevin Tian <kevin.tian@xxxxxxxxx>
> 
> Thanks, but well, I'll then follow Andrew's suggestion and also
> name the VT-d functions intel_iommu_{en,dis}able_eim(). I
> hope that's okay with you.
> 

OK to me.

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