Xen devel - Aug 2013 - [PATCH V2] x86/AMD-Vi: Add additional check for invalid special->handle

From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

This patch add an additional logic to check for the often case when the
special->handle is not initialized due to firmware bugs.
but the special->usedid is correct. If users overide this using the
command line option ivrs_ioapic, then it should use the value instead.
---
This patch is supposed to follow the patches:
[Xen-devel] [PATCH RFC 2/2] AMD IOMMU: allow command line overrides for broken
IVRS tables

Changes in V2:
	- Fix logic to match the special->used_id with the value provided by users.

 xen/drivers/passthrough/amd/iommu_acpi.c |   55 ++++++++++++++++++++----------
 1 file changed, 37 insertions(+), 18 deletions(-)

diff --git a/xen/drivers/passthrough/amd/iommu_acpi.c
b/xen/drivers/passthrough/amd/iommu_acpi.c
index 89b359c..8b14112 100644
--- a/xen/drivers/passthrough/amd/iommu_acpi.c
+++ b/xen/drivers/passthrough/amd/iommu_acpi.c
@@ -698,14 +698,16 @@ static u16 __init parse_ivhd_device_special(
         return 0;
     }
 
-    AMD_IOMMU_DEBUG("IVHD Special: %04x:%02x:%02x.%u variety %#x handle
%#x\n",
+    AMD_IOMMU_DEBUG("IVHD Special: %04x:%02x:%02x.%u variety %#x handle
%#x used_id %#x\n",
                     seg, PCI_BUS(bdf), PCI_SLOT(bdf), PCI_FUNC(bdf),
-                    special->variety, special->handle);
+                    special->variety, special->handle,
special->used_id);
     add_ivrs_mapping_entry(bdf, bdf, special->header.data_setting, iommu);
 
     switch ( special->variety )
     {
     case ACPI_IVHD_IOAPIC:
+    {
+        unsigned int apic_id = 0;
         if ( !iommu_intremap )
             break;
         /*
@@ -715,29 +717,45 @@ static u16 __init parse_ivhd_device_special(
          */
         for ( apic = 0; apic < nr_ioapics; apic++ )
         {
-            if ( IO_APIC_ID(apic) != special->handle )
-                continue;
+            apic_id = special->handle;
 
-            if ( special->handle >= ARRAY_SIZE(ioapic_sbdf) )
+            if ( IO_APIC_ID(apic) != apic_id )
+            {
+                /* Some BIOSes have invalid value in the special->handle, 
+                 * but the special->used_id is correct. Here we check to see
if
+                 * the user is overiding the special->handle from
commandline
+                 */
+                if ( (ioapic_sbdf[IO_APIC_ID(apic)].bdf == special->used_id)
&&
+                     (test_bit(IO_APIC_ID(apic), ioapic_cmdline)) )
+                {
+                    apic_id = IO_APIC_ID(apic);
+                    AMD_IOMMU_DEBUG ("IVHD Special: Usinging command
override "
+                        "value for IO-APIC %#x, bdf=%#x\n", 
+                        apic_id, ioapic_sbdf[apic_id].bdf);
+                }
+                else
+                    continue;
+            }
+		
+            if ( apic_id >= ARRAY_SIZE(ioapic_sbdf) )
             {
                 printk(XENLOG_ERR "IVHD Error: IO-APIC %#x entry beyond
bounds\n",
-                       special->handle);
+                       apic_id);
                 return 0;
             }
 
-            if ( test_bit(special->handle, ioapic_cmdline) )
-                AMD_IOMMU_DEBUG("IVHD: Command line override present for
IO-APIC %#x\n",
-                                special->handle);
-            else if ( ioapic_sbdf[special->handle].pin_2_idx )
+            if ( test_bit(apic_id, ioapic_cmdline) )
+                break;
+            else if ( ioapic_sbdf[apic_id].pin_2_idx )
             {
-                if ( ioapic_sbdf[special->handle].bdf == bdf &&
-                     ioapic_sbdf[special->handle].seg == seg )
+                if ( ioapic_sbdf[apic_id].bdf == bdf &&
+                     ioapic_sbdf[apic_id].seg == seg )
                     AMD_IOMMU_DEBUG("IVHD Warning: Duplicate IO-APIC %#x
entries\n",
-                                    special->handle);
+                                    apic_id);
                 else
                 {
                     printk(XENLOG_ERR "IVHD Error: Conflicting IO-APIC %#x
entries\n",
-                           special->handle);
+                           apic_id);
                     if ( amd_iommu_perdev_intremap )
                         return 0;
                 }
@@ -745,10 +763,10 @@ static u16 __init parse_ivhd_device_special(
             else
             {
                 /* set device id of ioapic */
-                ioapic_sbdf[special->handle].bdf = bdf;
-                ioapic_sbdf[special->handle].seg = seg;
+                ioapic_sbdf[apic_id].bdf = bdf;
+                ioapic_sbdf[apic_id].seg = seg;
 
-                ioapic_sbdf[special->handle].pin_2_idx = xmalloc_array(
+                ioapic_sbdf[apic_id].pin_2_idx = xmalloc_array(
                     u16, nr_ioapic_entries[apic]);
                 if ( nr_ioapic_entries[apic] &&
                      !ioapic_sbdf[IO_APIC_ID(apic)].pin_2_idx )
@@ -765,10 +783,11 @@ static u16 __init parse_ivhd_device_special(
         if ( apic == nr_ioapics )
         {
             printk(XENLOG_ERR "IVHD Error: Invalid IO-APIC %#x\n",
-                   special->handle);
+                   apic_id);
             return 0;
         }
         break;
+    }
     case ACPI_IVHD_HPET:
         /* set device id of hpet */
         if ( hpet_sbdf.iommu ||
-- 
1.7.10.4

>>> On 28.08.13 at 19:34, <suravee.suthikulpanit@amd.com> wrote:
> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> 
> This patch add an additional logic to check for the often case when the
> special->handle is not initialized due to firmware bugs.
> but the special->usedid is correct. If users overide this using the
> command line option ivrs_ioapic, then it should use the value instead.
> ---
> This patch is supposed to follow the patches:
> [Xen-devel] [PATCH RFC 2/2] AMD IOMMU: allow command line overrides for 
> broken IVRS tables
> 
> Changes in V2:
> 	- Fix logic to match the special->used_id with the value provided by
users.
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.

Jan

On 8/29/2013 2:17 AM, Jan Beulich wrote:
>>>> On 28.08.13 at 19:34, <suravee.suthikulpanit@amd.com>
wrote:
>> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>>
>> This patch add an additional logic to check for the often case when the
>> special->handle is not initialized due to firmware bugs.
>> but the special->usedid is correct. If users overide this using the
>> command line option ivrs_ioapic, then it should use the value instead.
>> ---
>> This patch is supposed to follow the patches:
>> [Xen-devel] [PATCH RFC 2/2] AMD IOMMU: allow command line overrides for
>> broken IVRS tables
>>
>> Changes in V2:
>> 	- Fix logic to match the special->used_id with the value provided
by users.
> 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.
>
> Jan
>
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.  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.

Suravee

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

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







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

>>> On 30.08.13 at 22:35, Suravee Suthikulpanit
<suravee.suthikulpanit@amd.com>
wrote:
> 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.
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

On 9/4/2013 4:57 AM, Jan Beulich wrote:
>>>> On 30.08.13 at 22:35, Suravee Suthikulpanit
<suravee.suthikulpanit@amd.com>
> wrote:
>> 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.
> 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.
Yes, I agree that the current code here is handling the CASE1 mentioned 
below, but not the CASE3,
  which I am trying to handle.
>
>> 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.
Just to make sure.  For example, in the case below:

IOAPIC 0: handle = 0x0: bdf=00:14.0   //good
IOAPIC 1: handle = 0x0: bdf=00:00.1   //bad handle due to duplication

Here, user should specify "ivrs_ioapic[00:00.1]=0x1" which will
override
IOAPIC 1 and keep the IOAPIC 0.
Although, I have never seen the case where bdf is duplicated.
>> 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).
We should be able to identify the invalid one by checking if the handle 
is not in the APIC table, in which case, we will not be using the value 
in the IVRS.
If users gives option "ivrs_ioapic[bb:dd.f]=<handle>", then just
use
that one.
> 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).
I still don''t think we should have two version of the same command.
This
would only make it more confusing.  For example,
IOAPIC 0: handle = 0x00: bdf=00:14.0   //good
IOAPIC 1: handle = 0xff: bdf=00:00.1    //bad since the value 0xff is 
not in the APIC table.
Here, this would be sufficient to just specify option 
"ivrs_ioapic[00:00.1]=0x1".
> 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.
See above.
> 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.
I think we can just ignore this case.
> 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>
After looking through the examples above, I still think we only need the 
"ivrs_ioapic[<sbdf>]=<id>" version.

Suravee

>>> On 05.09.13 at 00:48, Suravee Suthikulpanit
<suravee.suthikulpanit@amd.com> wrote:
> On 9/4/2013 4:57 AM, Jan Beulich wrote:
>> 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>
> After looking through the examples above, I still think we only need the 
> "ivrs_ioapic[<sbdf>]=<id>" version.
Perhaps, but then could you talk to your Linux side colleagues to
find out why they picked (only) the other alternative? I''d really
like to keep such workarounds largely in sync (a superset is fine,
but a subset, not to speak of a disjoint set, are not) with Linux...

Jan

On 9/5/2013 2:14 AM, Jan Beulich wrote:
>>>> On 05.09.13 at 00:48, Suravee Suthikulpanit
<suravee.suthikulpanit@amd.com> wrote:
>> On 9/4/2013 4:57 AM, Jan Beulich wrote:
>>> 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>
>> After looking through the examples above, I still think we only need
the
>> "ivrs_ioapic[<sbdf>]=<id>" version.
> Perhaps, but then could you talk to your Linux side colleagues to
> find out why they picked (only) the other alternative? I''d really
> like to keep such workarounds largely in sync (a superset is fine,
> but a subset, not to speak of a disjoint set, are not) with Linux...
>
> Jan
At this point, I think it''s mainly syntax.  Since the implementation is
different.  If you want to keep the syntax the same (i.e. 
ivrs_ioapic[id]=<sdbf>), we can do that.  I don''t think Linux
tries to
interpret as "pin id" or "pin sdbf".  All it does is just
ties both
values together.

Suravee

>>> On 12.09.13 at 00:31, Suravee Suthikulpanit
<suravee.suthikulpanit@amd.com> wrote:
> On 9/5/2013 2:14 AM, Jan Beulich wrote:
>>>>> On 05.09.13 at 00:48, Suravee Suthikulpanit
<suravee.suthikulpanit@amd.com> wrote:
>>> After looking through the examples above, I still think we only
need the
>>> "ivrs_ioapic[<sbdf>]=<id>" version.
>> Perhaps, but then could you talk to your Linux side colleagues to
>> find out why they picked (only) the other alternative? I''d
really
>> like to keep such workarounds largely in sync (a superset is fine,
>> but a subset, not to speak of a disjoint set, are not) with Linux...
>>
> At this point, I think it''s mainly syntax.  Since the
implementation is
> different.  If you want to keep the syntax the same (i.e. 
> ivrs_ioapic[id]=<sdbf>), we can do that.  I don''t think
Linux tries to
> interpret as "pin id" or "pin sdbf".  All it does is
just ties both
> values together.
I continue to disagree: add_special_device() matches on only ->id,
i.e. there''s no replacing of IVRS entries with a bad ID. The
difference is that they simply add a device with the provided
attributes, whereas we truly use the command line setting as an
override for an existing entry. By adding the inverse syntax, we''d
provide more fine grained control to the use than Linux does.

I guess I should try to get to send you a prototype for this.

Jan

>>> On 12.09.13 at 00:31, Suravee Suthikulpanit
<suravee.suthikulpanit@amd.com>
wrote:
> On 9/5/2013 2:14 AM, Jan Beulich wrote:
>>>>> On 05.09.13 at 00:48, Suravee Suthikulpanit
<suravee.suthikulpanit@amd.com>
> wrote:
>>> On 9/4/2013 4:57 AM, Jan Beulich wrote:
>>>> 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>
>>> After looking through the examples above, I still think we only
need the
>>> "ivrs_ioapic[<sbdf>]=<id>" version.
>> Perhaps, but then could you talk to your Linux side colleagues to
>> find out why they picked (only) the other alternative? I''d
really
>> like to keep such workarounds largely in sync (a superset is fine,
>> but a subset, not to speak of a disjoint set, are not) with Linux...
>>
> At this point, I think it''s mainly syntax.  Since the
implementation is
> different.  If you want to keep the syntax the same (i.e. 
> ivrs_ioapic[id]=<sdbf>), we can do that.  I don''t think
Linux tries to
> interpret as "pin id" or "pin sdbf".  All it does is
just ties both
> values together.
So how about the below then?

Jan

AMD IOMMU: also match IVRS overrides on device ID

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/drivers/passthrough/amd/iommu_acpi.c
+++ b/xen/drivers/passthrough/amd/iommu_acpi.c
@@ -713,6 +713,24 @@ static u16 __init parse_ivhd_device_spec
          * consistency here --- whether entry''s IOAPIC ID is valid and
          * whether there are conflicting/duplicated entries.
          */
+        apic = find_first_bit(ioapic_cmdline, ARRAY_SIZE(ioapic_sbdf));
+        while ( apic < ARRAY_SIZE(ioapic_sbdf) )
+        {
+            if ( ioapic_sbdf[apic].bdf == bdf &&
+                 ioapic_sbdf[apic].seg == seg )
+                break;
+            apic = find_next_bit(ioapic_cmdline, ARRAY_SIZE(ioapic_sbdf),
+                                 apic + 1);
+        }
+        if ( apic < ARRAY_SIZE(ioapic_sbdf) )
+        {
+            AMD_IOMMU_DEBUG("IVHD: Command line override present for
IO-APIC %#x"
+                            "(IVRS: %#x devID %04x:%02x:%02x.%u)\n",
+                            apic, special->handle, seg, PCI_BUS(bdf),
+                            PCI_SLOT(bdf), PCI_FUNC(bdf));
+            break;
+        }
+
         for ( apic = 0; apic < nr_ioapics; apic++ )
         {
             if ( IO_APIC_ID(apic) != special->handle )

On 9/12/2013 3:50 AM, Jan Beulich wrote:
>>
>> Jan
>>
>> AMD IOMMU: also match IVRS overrides on device ID
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> --- a/xen/drivers/passthrough/amd/iommu_acpi.c
>> +++ b/xen/drivers/passthrough/amd/iommu_acpi.c
>> @@ -713,6 +713,24 @@ static u16 __init parse_ivhd_device_spec
>>            * consistency here --- whether entry''s IOAPIC ID is
valid and
>>            * whether there are conflicting/duplicated entries.
>>            */
>> +        apic = find_first_bit(ioapic_cmdline,
ARRAY_SIZE(ioapic_sbdf));
>> +        while ( apic < ARRAY_SIZE(ioapic_sbdf) )
>> +        {
>> +            if ( ioapic_sbdf[apic].bdf == bdf &&
>> +                 ioapic_sbdf[apic].seg == seg )
>> +                break;
>> +            apic = find_next_bit(ioapic_cmdline,
ARRAY_SIZE(ioapic_sbdf),
>> +                                 apic + 1);
>> +        }
>> +        if ( apic < ARRAY_SIZE(ioapic_sbdf) )
>> +        {
>> +            AMD_IOMMU_DEBUG("IVHD: Command line override present
for IO-APIC %#x"
>> +                            "(IVRS: %#x devID
%04x:%02x:%02x.%u)\n",
>> +                            apic, special->handle, seg,
PCI_BUS(bdf),
>> +                            PCI_SLOT(bdf), PCI_FUNC(bdf));
>> +            break;
>> +        }
>> +
>>           for ( apic = 0; apic < nr_ioapics; apic++ )
>>           {
>>               if ( IO_APIC_ID(apic) != special->handle )
>>
>>
>>
>>
Jan,

This looks good. I''ll used it in V3 that I already sent out.

Suravee


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

On 9/12/2013 3:50 AM, Jan Beulich wrote:
> So how about the below then?
>
> Jan
>
> AMD IOMMU: also match IVRS overrides on device ID
>
> Signed-off-by: Jan Beulich<jbeulich@suse.com>
>
> --- a/xen/drivers/passthrough/amd/iommu_acpi.c
> +++ b/xen/drivers/passthrough/amd/iommu_acpi.c
> @@ -713,6 +713,24 @@ static u16 __init parse_ivhd_device_spec
>            * consistency here --- whether entry''s IOAPIC ID is
valid and
>            * whether there are conflicting/duplicated entries.
>            */
> +        apic = find_first_bit(ioapic_cmdline, ARRAY_SIZE(ioapic_sbdf));
> +        while ( apic < ARRAY_SIZE(ioapic_sbdf) )
> +        {
> +            if ( ioapic_sbdf[apic].bdf == bdf &&
> +                 ioapic_sbdf[apic].seg == seg )
> +                break;
> +            apic = find_next_bit(ioapic_cmdline, ARRAY_SIZE(ioapic_sbdf),
> +                                 apic + 1);
> +        }
> +        if ( apic < ARRAY_SIZE(ioapic_sbdf) )
> +        {
> +            AMD_IOMMU_DEBUG("IVHD: Command line override present for
IO-APIC %#x"
> +                            "(IVRS: %#x devID
%04x:%02x:%02x.%u)\n",
> +                            apic, special->handle, seg, PCI_BUS(bdf),
> +                            PCI_SLOT(bdf), PCI_FUNC(bdf));
> +            break;
> +        }
> +
>           for ( apic = 0; apic < nr_ioapics; apic++ )
>           {
>               if ( IO_APIC_ID(apic) != special->handle )
>
>
>
Jan,

This looks good. I''m using it in the V3 that I am sending out.

Suravee


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