From mboxrd@z Thu Jan 1 00:00:00 1970 From: Suravee Suthikulpanit Subject: Re: [RESEND] Re: [PATCH RFC 2/2] AMD IOMMU: allow command line overrides for broken IVRS tables Date: Wed, 28 Aug 2013 12:33:40 -0500 Message-ID: <521E3474.9040308@amd.com> References: <521E36AF02000078000EF2FD@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <521E36AF02000078000EF2FD@nat28.tlf.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich , Sander Eikelenboom Cc: Andrew Cooper , Keir Fraser , "xen-devel@lists.xen.org" List-Id: xen-devel@lists.xenproject.org On 8/28/2013 10:43 AM, Jan Beulich wrote: >>>> On 28.08.13 at 16:59, Suravee Suthikulpanit 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 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