[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH V2] x86/AMD-Vi: Add additional check for invalid special->handle
On 9/4/2013 4:57 AM, Jan Beulich wrote: Yes, I agree that the current code here is handling the CASE1 mentioned below, but not the CASE3,On 30.08.13 at 22:35, Suravee Suthikulpanit <suravee.suthikulpanit@xxxxxxx>wrote:On 8/30/2013 3:06 AM, Jan Beulich wrote:On 29.08.13 at 22:26, Suravee Suthikulpanit <suravee.suthikulpanit@xxxxxxx>wrote:On 8/29/2013 2:17 AM, Jan Beulich wrote:As said in a previous reply - I'm convinced we can't fix things both ways with just a single command line option: We need to know which value to use as anchor (I'd generally think that ought to be the value inside the square backets) and which value to use as overrides. And since my earlier patch was inspired by the Linux one - I don't think Linux allows fixing up things the way you try to here either.Actually, on Linux, the it would try matching the handle (i.e. the value in the square bracket). If it is specified via command line, it will override the value in the IVRS.Right - that's matching the behavior of my patch. Or am I missing something here?I believe in your patch is slightly different. In your version, it has the following check in the driver/passthrough/amd/iommu_acpi.c:parse_ivhd_device_special(). */ for ( apic = 0; apic < nr_ioapics; apic++ ) { if ( IO_APIC_ID(apic) != special->handle ) continue; ..... First, the code tries to match the IO_APIC_ID(apic) with the special->handle. If none is matched, it will go directly to the exiting code (see below) without trying to check the command line. if ( apic == nr_ioapics ) { printk(XENLOG_ERR "IVHD Error: Invalid IO-APIC %#x\n", special->handle); return 0; } break; This is different on Linux where it check if the value it is trying to matched is coming from the command line.But again - there being an ID on the command line that doesn't have any match in IVHD is being taken care of by the adjustment to parse_ivrs_table(). At least afaict. which I am trying to handle. Let's clarify the cases we are trying to handle here: CASE1: IOAPIC handle ismissing in IVRS, but exist in APIC table Users should specify the missing IOAPIC using ioapic_ivrs[<handle>]=bdf CASE2: IOAPIC handleare duplicated in IVRS entries This case,the code already check for duplications inIVRS. Here,we cannot trust any existing entries in the IVRS,One of them might still be right, and hence not need overriding. Just to make sure. For example, in the case below: IOAPIC 0: handle = 0x0: bdf=00:14.0 //good IOAPIC 1: handle = 0x0: bdf=00:00.1 //bad handle due to duplicationHere, user should specify "ivrs_ioapic[00:00.1]=0x1" which will override IOAPIC 1 and keep the IOAPIC 0. Although, I have never seen the case where bdf is duplicated. We should be able to identify the invalid one by checking if the handle is not in the APIC table, in which case, we will not be using the value in the IVRS. If users gives option "ivrs_ioapic[bb:dd.f]=<handle>", then just use that one.and we shouldonly rely ontheioapic_ivrs[<handle>]=bdf options for all IOAPICs. CASE3: IOAPIChandle are Bogus (i.e. 0xff ornot existing in APIC table) We cannot trust the entry with bogus handle, and users would need to specify the ioapic_ivrs option. Which casedo you thinkwould require the second command line option which we wouldanchor the BDF?Case 3 in any event would need not only specification of valid entries on the command line, but also a way to identify the invalid ones (to avoid parse_ivhd_device_special() returning 0, which is its failure indicator). I still don't think we should have two version of the same command. This would only make it more confusing. For example,As long as the handle here is invalid, but devid is correct, that would be a case where we'd need a devid-anchored command line option (or some other second command line option identifying the IVRS entries needing to be ignored). IOAPIC 0: handle = 0x00: bdf=00:14.0 //goodIOAPIC 1: handle = 0xff: bdf=00:00.1 //bad since the value 0xff is not in the APIC table. Here, this would be sufficient to just specify option "ivrs_ioapic[00:00.1]=0x1". Since for case 2 the duplicate handles may again be associated with valid devid-s, the same would apply there, except we'd not need invalidation, but just overriding of the handles. Which would again most easily be done by a devid-anchored command line option. See above. Case 4 really is where things don't matter: If some entries have neither a valid handle nor a valid devid, then overriding them can be done in either way (but some mechanism to identify which ones they are in order to ignore them would still be needed). I wonder, however, whether that's a case we indeed need to worry about: If everything the firmware tells us is a lie, we'd better not do anything fancy on such a system. I think we can just ignore this case. After looking through the examples above, I still think we only need the "ivrs_ioapic[<sbdf>]=<id>" version.It would, btw, be possible to get along with a single command line option, just having two alternating forms: ivrs_ioapic[id]=<sbdf> ivrs_ioapic[<sbdf>]=<id> Suravee _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |