From mboxrd@z Thu Jan 1 00:00:00 1970 From: Suravee Suthikulpanit Subject: Re: [PATCH V2] x86/AMD-Vi: Add additional check for invalid special->handle Date: Fri, 30 Aug 2013 15:35:20 -0500 Message-ID: <52210208.9000809@amd.com> References: <1377711297-2705-1-git-send-email-suravee.suthikulpanit@amd.com> <521F11B602000078000EF4ED@nat28.tlf.novell.com> <521FAE67.7080309@amd.com> <52206EA002000078000EFA11@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============9101648164591688376==" Return-path: In-Reply-To: <52206EA002000078000EFA11@nat28.tlf.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: andrew.cooper3@citrix.com, xen-devel@lists.xen.org, keir@xen.org, linux@eikelenboom.it List-Id: xen-devel@lists.xenproject.org --===============9101648164591688376== Content-Type: multipart/alternative; boundary="------------020305040707000100030507" --------------020305040707000100030507 Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 7bit On 8/30/2013 3:06 AM, Jan Beulich wrote: >>>> On 29.08.13 at 22:26, Suravee Suthikulpanit 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[]=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[]=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 --------------020305040707000100030507 Content-Type: text/html; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit
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 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




 --------------020305040707000100030507-- --===============9101648164591688376== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel --===============9101648164591688376==--