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.
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.
Jan
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?
Suravee
