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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 07.02.14 at 17:03, Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx> wrote:
> On 02/07/2014 10:50 AM, Jan Beulich wrote:
>>>>> 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.
> 
> 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?).

No - there _has_ to be an IO-APIC in the SB.

> Will we still be able to do this?

It shouldn't have worked before either, unless I'm not really
understanding the purpose of the check in Linux.

Jan

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