Re: [Xen-devel] [PATCH v2 2/2] AMD/IOMMU: without XT, x2APIC needs to be forced into physical mode

[Jan Beulich <jbeulich@xxxxxxxx>]

On 28.02.2020 15:48, Andrew Cooper wrote:
> On 28/02/2020 12:12, Jan Beulich wrote:
>> The wider cluster mode APIC IDs aren't generally representable. Convert
>> the iommu_intremap variable into a tristate, allowing the AMD IOMMU
>> driver to signal this special restriction to the apic_x2apic_probe().
>> (Note: assignments to the variable get adjusted, while existing
>> consumers - all assuming a boolean property - are left alone.)
> 
> I think it would be helpful to state that while we are not aware of any
> hardware with this as a restriction, it is a situation which can be
> created on fully x2apic-capable systems via firmware settings.

Added.

>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>> ---
>> v2: New.
>>
>> --- a/xen/arch/x86/genapic/x2apic.c
>> +++ b/xen/arch/x86/genapic/x2apic.c
>> @@ -236,12 +236,21 @@ const struct genapic *__init apic_x2apic
>>          x2apic_phys = !iommu_intremap ||
>>                        (acpi_gbl_FADT.flags & ACPI_FADT_APIC_PHYSICAL);
>>      }
>> -    else if ( !x2apic_phys && !iommu_intremap )
>> -    {
>> -        printk("WARNING: x2APIC cluster mode is not supported without 
>> interrupt remapping\n"
>> -               "x2APIC: forcing phys mode\n");
>> -        x2apic_phys = true;
>> -    }
>> +    else if ( !x2apic_phys )
>> +        switch ( iommu_intremap )
>> +        {
>> +        case iommu_intremap_off:
>> +        case iommu_intremap_restricted:
>> +            printk("WARNING: x2APIC cluster mode is not supported %s 
>> interrupt remapping\n"
>> +                   "x2APIC: forcing phys mode\n",
> 
> Any chance to fold this into a single line with "- forcing phys mode\n"
> as a suffix?

I did consider doing so myself, but didn't do it then for being
an unrelated change. Now that you ask for it - done.

>> +                   iommu_intremap == iommu_intremap_off ? "without"
>> +                                                        : "with 
>> restricted");
>> +            x2apic_phys = true;
>> +            break;
>> +
>> +        case iommu_intremap_full:
>> +            break;
>> +        }
>>  
>>      if ( x2apic_phys )
>>          return &apic_x2apic_phys;
>> --- a/xen/include/xen/iommu.h
>> +++ b/xen/include/xen/iommu.h
>> @@ -54,7 +54,18 @@ static inline bool_t dfn_eq(dfn_t x, dfn
>>  
>>  extern bool_t iommu_enable, iommu_enabled;
>>  extern bool force_iommu, iommu_quarantine, iommu_verbose, iommu_igfx;
>> -extern bool_t iommu_snoop, iommu_qinval, iommu_intremap, iommu_intpost;
>> +extern bool_t iommu_snoop, iommu_qinval, iommu_intpost;
>> +extern enum __packed iommu_intremap {
>> +   /*
>> +    * In order to allow traditional boolean uses of the iommu_intremap
>> +    * variable, the "off" value has to come first (yielding a value of 
>> zero).
>> +    */
>> +   iommu_intremap_off,
>> +#ifdef CONFIG_X86
>> +   iommu_intremap_restricted,
> 
> This needs a note about its meaning.  How about this?
> 
> /* Interrupt remapping enabled, but only able to generate interrupts
> with an 8-bit APIC ID. */

Added.

> Otherwise, Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

Thanks.

> Not an issue for now, but "restricted" might become ambiguous with
> future extensions.

Yes, fair point.

Jan

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