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

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

On 02/07/2014 10:50 AM, Jan Beulich wrote:
>>>> On 07.02.14 at 16:38, Boris Ostrovsky <boris.ostrovsky@oracle.com> wrote:
>> On 02/07/2014 10:23 AM, Jan Beulich wrote:
>>>>>> On 07.02.14 at 16:12, Boris Ostrovsky <boris.ostrovsky@oracle.com> 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@amd.com>.
>>>>>
>>>>> Reported-by: Eric Houby <ehouby@yahoo.com>
>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>> Tested-by: Eric Houby <ehouby@yahoo.com>
>>>>>
>>>>> --- 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.

This is exactly what I am asking: Suppose we have IOAPIC in the NB (I 
think it's something like 0:02.0) *and* IVRS is broken. Currently we can 
say 'ivrs_ioapic[0]=0:02.0' and we are good to go (right?). Will we 
still be able to do this?

-boris


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