[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 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. > 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. > 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). 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). 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. 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. 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> Since parse_pci() tells us whether the input was a valid PCI device specification, we could try that on the input string first, and only upon failure use the current code (expecting the ID as index). But of course the resulting tracking structure would still need to identify the piece of information to trust and the one to override. Jan _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |