On 02/04/2019 04:24, Tian, Kevin wrote: >> From: Jan Beulich [mailto:jbeul...@suse.com] >> Sent: Friday, March 29, 2019 5:13 PM >> >>>>> On 28.03.19 at 18:37, <andrew.coop...@citrix.com> 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.t...@intel.com>
No point posting this series a second time just for the rename. With the suggested adjustments, Reviewed-by: Andrew Cooper <andrew.coop...@citrix.com> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
