[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH 1/1 V3] x86/AMD-Vi: Add additional check for invalid special->handle



On 9/13/2013 4:24 AM, Jan Beulich wrote:
On 12.09.13 at 19:00, <suravee.suthikulpanit@xxxxxxx> wrote:
--- a/xen/drivers/passthrough/amd/iommu_acpi.c
+++ b/xen/drivers/passthrough/amd/iommu_acpi.c
@@ -664,19 +664,46 @@ static void __init parse_ivrs_hpet(char *str)
ASSERT(*s == '[');
      id = simple_strtoul(s + 1, &s, 0);
-    if ( id != (typeof(hpet_sbdf.id))id || *s != ']' || *++s != '=' )
+    if ( (*s != ']') || (*++s != '=') )
No, unless you have a very good reason.
Oh, sorry. This is my mistake.

          return;
s = parse_pci(s + 1, &seg, &bus, &dev, &func);
      if ( !s || *s )
          return;
+ hpet_sbdf.id = id;
In essence this is the only change not contained in the patch I sent.
So I'd be inclined to commit this (and perhaps the one debug
message adjustment below) under your name, and my proposed
change as a separate one.
Ok.

      hpet_sbdf.bdf = PCI_BDF(bus, dev, func);
      hpet_sbdf.seg = seg;
      hpet_sbdf.cmdline = 1;
  }
  custom_param("ivrs_hpet[", parse_ivrs_hpet);
+static bool_t is_ioapic_overidden(u16 seg, u16 bdf, u8 handle)
Missing __init annotation. And anyway, I can't really see why
putting this in a separate function is a significant benefit. It's
only being used in one place afaics.
Sorry, I forgot the __init. I normally just like to keep to code that does a specific thing inside it's own function for the ease of reading. But this is purely coding style.
I can put it in the caller function if you prefer.

+{
+    bool_t ret = 0;
+    int 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, handle, seg, PCI_BUS(bdf),
+                        PCI_SLOT(bdf), PCI_FUNC(bdf));
+        ret = 1;
+    }
+
+    return ret;
+}
+
  static u16 __init parse_ivhd_device_special(
      const struct acpi_ivrs_device8c *special, u16 seg,
      u16 header_length, u16 block_length, struct amd_iommu *iommu)
@@ -698,16 +725,18 @@ 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:
-        if ( !iommu_intremap )
+        if ( !iommu_intremap ||
+             is_ioapic_overidden(seg, bdf, special->handle) )
              break;
+
          /*
           * Some BIOSes have IOAPIC broken entries so we check for IVRS
           * consistency here --- whether entry's IOAPIC ID is valid and
@@ -725,10 +754,7 @@ static u16 __init parse_ivhd_device_special(
                  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 ( ioapic_sbdf[special->handle].pin_2_idx )
Again - no, unless you have a very good reason.
I think with the check for command line stuff already handle prior to this point, I don't see why we still need to keep this here. It should not need to go through
the logic here.

              {
                  if ( ioapic_sbdf[special->handle].bdf == bdf &&
                       ioapic_sbdf[special->handle].seg == seg )
@@ -770,6 +796,16 @@ static u16 __init parse_ivhd_device_special(
          }
          break;
      case ACPI_IVHD_HPET:
+        if ( hpet_sbdf.cmdline )
+        {
+            AMD_IOMMU_DEBUG("IVHD: Command line override present for HPET %#x "
+                            "(IVRS: %#x devID %04x:%02x:%02x.%u)\n",
+                            hpet_sbdf.id, special->handle, seg, PCI_BUS(bdf),
+                            PCI_SLOT(bdf), PCI_FUNC(bdf));
+            hpet_sbdf.iommu = iommu;
+            break;
+        }
+
          /* set device id of hpet */
          if ( hpet_sbdf.iommu ||
               (hpet_sbdf.cmdline && hpet_sbdf.id != special->handle) )
@@ -777,12 +813,10 @@ static u16 __init parse_ivhd_device_special(
              printk(XENLOG_WARNING "Only one IVHD HPET entry is supported\n");
              break;
          }
+
          hpet_sbdf.id = special->handle;
-        if ( !hpet_sbdf.cmdline )
-        {
-            hpet_sbdf.bdf = bdf;
-            hpet_sbdf.seg = seg;
-        }
+        hpet_sbdf.bdf = bdf;
+        hpet_sbdf.seg = seg;
I don't see what benefit these HPET related changes provide. Or
if there is any that I overlook, then the previous uses of
hpet_sbdf.cmdline should all be eliminated.

Jan

I found an issue when I was testing. if the "handle" for the IVRS HPET is not the same where:
    - IVRS Table HPET handle = 0x0
    - HPET Table = 0x2

This throws the following error message:
(XEN) AMD-Vi: Failed to setup HPET MSI remapping: Wrong HPET

In this case, the handle is wrong and I am trying to change it.
When I am trying to specify the command line option "ivrs_hpet[2]=00:14.0"
this does not get used because the in the parse_ivrs_hpet() did not store the override id. So, I modify the logic in the parse_ivrs_hpet() to override the hpet_sbdf.bdf with the new ID specified from command line. Then it doesn't need to go though this logic here. Also, the check "hpet_sbdf.cmdline && hpet_sbdf.id != special->handle" should also be eliminated.

I will send out V4 with updates, and thank you for your patience.

Suravee


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


 


Rackspace

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