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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

Jan



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