Suravee Suthikulpanit
2013-Aug-28 17:33 UTC
Re: [RESEND] Re: [PATCH RFC 2/2] AMD IOMMU: allow command line overrides for broken IVRS tables
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
Jan Beulich
2013-Aug-29 06:22 UTC
Re: [PATCH RFC 2/2] AMD IOMMU: allow command line overrides for broken IVRS tables
>>> 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