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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

Jan



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