Re: [PATCH V2] x86/AMD-Vi: Add additional check for invalid special->handle

[Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3678 bytes --]

On 8/30/2013 3:06 AM, Jan Beulich wrote:
>>>> On 29.08.13 at 22:26, Suravee Suthikulpanit <suravee.suthikulpanit@amd.com> 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 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, 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?
Or, am  I missing some other cases here?

Suravee






[-- Attachment #1.2: Type: text/html, Size: 10908 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel