Re: [Xen-devel] [PATCH] AMD IOMMU: fail if there is no southbridge IO-APIC

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 07.02.14 at 16:38, Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx> wrote:
> On 02/07/2014 10:23 AM, Jan Beulich wrote:
>>>>> On 07.02.14 at 16:12, Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx> wrote:
>>> On 02/07/2014 04:21 AM, Jan Beulich wrote:
>>>> ... but interrupt remapping is requested (with per-device remapping
>>>> tables). Without it, the timer interrupt is usually not working.
>>>>
>>>> Inspired by Linux'es "iommu/amd: Work around wrong IOAPIC device-id in
>>>> IVRS table" (commit c2ff5cf5294bcbd7fa50f7d860e90a66db7e5059) by Joerg
>>>> Roedel <joerg.roedel@xxxxxxx>.
>>>>
>>>> Reported-by: Eric Houby <ehouby@xxxxxxxxx>
>>>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>>>> Tested-by: Eric Houby <ehouby@xxxxxxxxx>
>>>>
>>>> --- a/xen/drivers/passthrough/amd/iommu_acpi.c
>>>> +++ b/xen/drivers/passthrough/amd/iommu_acpi.c
>>>> @@ -984,6 +984,7 @@ static int __init parse_ivrs_table(struc
>>>>        const struct acpi_ivrs_header *ivrs_block;
>>>>        unsigned long length;
>>>>        unsigned int apic;
>>>> +    bool_t sb_ioapic = !iommu_intremap;
>>>>        int error = 0;
>>>>    
>>>>        BUG_ON(!table);
>>>> @@ -1017,8 +1018,15 @@ static int __init parse_ivrs_table(struc
>>>>        /* Each IO-APIC must have been mentioned in the table. */
>>>>        for ( apic = 0; !error && iommu_intremap && apic < nr_ioapics; 
>>>> ++apic )
>>>>        {
>>>> -        if ( !nr_ioapic_entries[apic] ||
>>>> -             ioapic_sbdf[IO_APIC_ID(apic)].pin_2_idx )
>>>> +        if ( !nr_ioapic_entries[apic] )
>>>> +            continue;
>>>> +
>>>> +        if ( !ioapic_sbdf[IO_APIC_ID(apic)].seg &&
>>>> +             /* SB IO-APIC is always on this device in AMD systems. */
>>>> +             ioapic_sbdf[IO_APIC_ID(apic)].bdf == PCI_BDF(0, 0x14, 0) )
>>>> +            sb_ioapic = 1;
>>>> +
>>>> +        if ( ioapic_sbdf[IO_APIC_ID(apic)].pin_2_idx )
>>>>                continue;
>>>>    
>>>>            if ( !test_bit(IO_APIC_ID(apic), ioapic_cmdline) )
>>> I don't know whether 0:14:0 is set in stone, I don't remember seeing
>>> anywhere that this is architectural.
>>>
>>> In the (unlikely) event that it is moved somewhere else will the user be
>>> able to overwrite where it is? Do you
>>> think that sb_ioapic may need to be set to true if appropriate bit is
>>> set in ioapic_cmdline?
>> These are question you'd need to ask to JÃrg, the author of the
>> original Linux side patch. I took as a precondition here that he
>> knew what he was doing.
> 
> Xen already has a way to override IVRS' view of IOAPICs with 
> ioapic_cmdline, something that Linux doesn't. Presumably if the user 
> sets ivrs_ioapic[] option on boot line then he knows what he is doing 
> (at least one would hope so).

I think the logic we have is sufficiently similar to Linux'es.

> My concern is that this patch would prevent the user from specifying 
> where the IOAPIC is. Will this boot option be useful at all now? When we 
> specify anything but 0:14:0 it will be pretty much ignored, won't it?

But the purpose here isn't to override how the hardware is
structured, but to overcome firmware vendors not getting their
ACPI tables correct.

Furthermore, what is being specified here can very well be
different from 00:14.0 - consider the northbridge IO-APIC and
eventual further ones.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel