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

From: Andrew Cooper <andrew.cooper3@citrix.com>
To: "Tian, Kevin" <kevin.tian@intel.com>, Jan Beulich <JBeulich@suse.com>
Cc: Wei Liu <wei.liu2@citrix.com>, Brian Woods <brian.woods@amd.com>,
	xen-devel <xen-devel@lists.xenproject.org>,
	Roger Pau Monne <roger.pau@citrix.com>
Subject: Re: [PATCH 6/7] x86/IOMMU: abstract Intel-specific iommu_{en, dis}able_x2apic_IR()
Date: Tue, 2 Apr 2019 11:18:15 +0100	[thread overview]
Message-ID: <ae9e547f-fc55-04eb-7ba0-48c445704a78@citrix.com> (raw)
In-Reply-To: <AADFC41AFE54684AB9EE6CBC0274A5D19C9C807F@SHSMSX104.ccr.corp.intel.com>

On 02/04/2019 04:24, Tian, Kevin wrote:
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: Friday, March 29, 2019 5:13 PM
>>
>>>>> On 28.03.19 at 18:37, <andrew.cooper3@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.tian@intel.com>

No point posting this series a second time just for the rename.  With
the suggested adjustments, Reviewed-by: Andrew Cooper
<andrew.cooper3@citrix.com>

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