Re: [RESEND] Re: [PATCH RFC 2/2] AMD IOMMU: allow command line overrides for broken IVRS tables

Re: [RESEND] Re: [PATCH RFC 2/2] AMD IOMMU: allow command line overrides for broken IVRS tables

[Suravee Suthikulpanit]

On 8/28/2013 10:43 AM, Jan Beulich wrote:
>>>> On 28.08.13 at 16:59, Suravee Suthikulpanit <suravee.suthikulpanit@amd.com> wrote:
>> The current patch does not handle the case where the "handle" value is
>> mis-configured in the IVRS.
> Right, but I don't think we can handle all possible cases here.
>
> And in any case (not the least because your patch seems to be
> incremental to my earlier one) - do you still see any issues with that
> one, or does the mail here represent sort of an ack to that earlier
> one (in its split form - as indicated earlier, I broke out the actual
> bug fixes from the workaround, which really is an enhancement)?
With the previous 2 patches, it would work fine for the case when the 
IVHD special is missing for an IOAPIC.
The code looks fine, but I don't have such systems which I can test on.  
If it's working for Sander,
then I think it should be sufficient testing.

Acked-by: Suravee Suthikulpanit <suravee.suthikulapanit@amd.com>

However, it is not handling the case (which I have seen a lot) where,
the special->handle is bad, but the special->used_id is ok. For this case,
we should be able to match the used_id to the BDF values given by users.
That's what my patch is trying to handle (but apparently failed... see 
below).
>
>> I have also included the patch which has additional check.  Below is the
>> output from the patch.
>>
>> (XEN) AMD-Vi: IVHD Device Entry: type 0x48 id 0 flags 0xd7
>> (XEN) AMD-Vi: IVHD Special: 0000:00:14.0 variety 0x1 handle 0xff used_id
>> 0xa0
>> (XEN) AMD-Vi: IVHD Special: Usinging command override value for IO-APIC 0x8, bdf=0xa0
>> (XEN) AMD-Vi: IVHD Device Entry: type 0x48 id 0 flags 0xd7
>> (XEN) AMD-Vi: IVHD Special: 0000:00:14.0 variety 0x2 handle 0 used_id 0xa0
>> (XEN) AMD-Vi: IVHD Device Entry: type 0x48 id 0 flags 0
>> (XEN) AMD-Vi: IVHD Special: 0000:00:00.1 variety 0x1 handle 0xff used_id 0x1
>> (XEN) AMD-Vi: IVHD Special: Usinging command override value for IO-APIC 0x8, bdf=0xa0
>> (XEN) AMD-Vi: IOMMU 0 Enabled.
>> (XEN) I/O virtualisation enabled
> Which already indicates that you apparently only considered the
> single IO-APIC case, because on a multi IO-APIC system ...
Oops, my logic broke when I was cleaning up the code.  I'll fix this.  I 
am planning to handle both IOAPICs.
>
>> @@ -715,29 +717,43 @@ static u16 __init parse_ivhd_device_special(
>>             */
>>            for ( apic = 0; apic < nr_ioapics; apic++ )
>>            {
>> -            if ( IO_APIC_ID(apic) != special->handle )
>> -                continue;
>> +            apic_id = special->handle;
>>
>> -            if ( special->handle >= ARRAY_SIZE(ioapic_sbdf) )
>> +            if ( IO_APIC_ID(apic) != apic_id )
> ... this would otherwise trigger on all IO-APICs but the one we're
> currently supposed to deal with ...
Sorry, logic got messed up during code clean up. I will send out the fix 
shortly.

Suravee

Re: [PATCH RFC 2/2] AMD IOMMU: allow command line overrides for broken IVRS tables

[Jan Beulich]

>>> On 28.08.13 at 19:33, Suravee Suthikulpanit <suravee.suthikulpanit@amd.com> wrote:
> On 8/28/2013 10:43 AM, Jan Beulich wrote:
>>>>> On 28.08.13 at 16:59, Suravee Suthikulpanit <suravee.suthikulpanit@amd.com> wrote:
>>> The current patch does not handle the case where the "handle" value is
>>> mis-configured in the IVRS.
>> Right, but I don't think we can handle all possible cases here.
>>
>> And in any case (not the least because your patch seems to be
>> incremental to my earlier one) - do you still see any issues with that
>> one, or does the mail here represent sort of an ack to that earlier
>> one (in its split form - as indicated earlier, I broke out the actual
>> bug fixes from the workaround, which really is an enhancement)?
> With the previous 2 patches, it would work fine for the case when the 
> IVHD special is missing for an IOAPIC.
> The code looks fine, but I don't have such systems which I can test on.  
> If it's working for Sander,
> then I think it should be sufficient testing.
> 
> Acked-by: Suravee Suthikulpanit <suravee.suthikulapanit@amd.com>
> 
> However, it is not handling the case (which I have seen a lot) where,
> the special->handle is bad, but the special->used_id is ok. For this case,
> we should be able to match the used_id to the BDF values given by users.
> That's what my patch is trying to handle (but apparently failed... see 
> below).

Afaict that's going to require a second command line option,
since the code will need to know which piece of data from the
IVRS structure to consider valid, and which one(s) to replace.

Jan