[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 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

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 )

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",
            return 0;

This is different on Linux where it check if the value it is trying to matched is coming from the command line.
In case the entry contains the handle 
value which is not list as IOAPIC in the ACPI APIC table, we should be 
able to invalidate the entry.  This same rule should also apply when 
users specify invalid handle value.  Also, I never see the case where 
the special->used_id is incorrect. The other case I am seeing is when 
the IVRS special entry is missing for IOAPIC, which this should already 
be handled in your original patch.

I have also verified that this works on Linux.  For example, on my 
system, there are 2 IOAPICs with APIC ID 8 and 9 according to the ACPI 
APIC table.  However, in IVRS, the value in both special entries are 
0xff.  When I give the boot options "ivrs_ioapic[8]=00:14.0 
ivrs_ioapic[9]=00:0.1", this allows IOMMU to initialize correctly.
Yes, because you specify the _full_ set. But we've seen many cases
(like Sander's) where one of the IVRS entries is correct and the other
isn't. In that case specifying one command line option should be
sufficient, and fixing it in the direction you propose requires - as said -
a second command line option, anchored at other than the IO-APIC ID.

Let's clarify the cases we are trying to handle here:
CASE1: IOAPIC handle is missing in IVRS, but exist in APIC table
Users should specify the missing IOAPIC using ioapic_ivrs[<handle>]=bdf
CASE2: IOAPIC handle are duplicated in IVRS entries 
This case, the code already check for duplications in IVRS. Here, we cannot trust 
any existing entries in the IVRS, and we should only rely on the ioapic_ivrs[<handle>]=bdf 
options for all IOAPICs.
CASE3: IOAPIC handle are Bogus (i.e. 0xff or not existing in APIC table)
We cannot trust the entry with bogus handle, and users would need to specify the ioapic_ivrs option.
Which case do you think would require the second command line option which we would anchor the BDF?
Or, am I missing some other cases here?

Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.