[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



> 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>

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.