diff mbox

[PULL,1/5] acpi-build: append description for non-hotplug

Message ID 20140217145138.GJ29329@ERROL.INI.CMU.EDU
State New
Headers show

Commit Message

Gabriel L. Somlo Feb. 17, 2014, 2:51 p.m. UTC 
Michael,

On Mon, Feb 17, 2014 at 04:25:26PM +0200, Michael S. Tsirkin wrote:
> As reported in
> http://article.gmane.org/gmane.comp.emulators.qemu/253987
> Mac OSX actually requires describing all occupied slots
> in ACPI - even if hotplug isn't enabled.
> 
> I didn't expect this so I dropped description of all
> non hotpluggable slots from ACPI.
> As a result: before
> commit 99fd437dee468609de8218f0eb3b16621fb6a9c9 (enable
> hotplug for pci bridges), PCI cards show up in the "device tree" of OS X
> (System Information). E.g., on MountainLion users have:
> 
> ...
> 
> Ethernet still works, but it's not showing up on the PCI bus, and it
> no longer thinks it's plugged in to slot #2, as it used to before the
> change.
> 
> To fix, append description for all occupied non hotpluggable PCI slots.
> 
> One need to be careful when doing this: VGA and ISA device were already
> described, so we need to drop description from DSDT.
> 
> Reported-by: Gabriel L. Somlo <gsomlo@gmail.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> ...

With this latest version of your patch, I crash during OS X boot with
"unable to find driver for this platform:\"ACPI\".\n"@/SourceCache/xnu/xnu-2050.48.12/iokit/Kernel/IOPlatformExpert.cpp:1514"

Your original patch (slightly doctored since it no longer applies cleanly
to the current qemu git master) is included below, and still works for me.

Thanks,
--Gabriel

Comments

Michael S. Tsirkin Feb. 17, 2014, 4:44 p.m. UTC | #1 
On Mon, Feb 17, 2014 at 09:51:39AM -0500, Gabriel L. Somlo wrote:
> Michael,
> 
> On Mon, Feb 17, 2014 at 04:25:26PM +0200, Michael S. Tsirkin wrote:
> > As reported in
> > http://article.gmane.org/gmane.comp.emulators.qemu/253987
> > Mac OSX actually requires describing all occupied slots
> > in ACPI - even if hotplug isn't enabled.
> > 
> > I didn't expect this so I dropped description of all
> > non hotpluggable slots from ACPI.
> > As a result: before
> > commit 99fd437dee468609de8218f0eb3b16621fb6a9c9 (enable
> > hotplug for pci bridges), PCI cards show up in the "device tree" of OS X
> > (System Information). E.g., on MountainLion users have:
> > 
> > ...
> > 
> > Ethernet still works, but it's not showing up on the PCI bus, and it
> > no longer thinks it's plugged in to slot #2, as it used to before the
> > change.
> > 
> > To fix, append description for all occupied non hotpluggable PCI slots.
> > 
> > One need to be careful when doing this: VGA and ISA device were already
> > described, so we need to drop description from DSDT.
> > 
> > Reported-by: Gabriel L. Somlo <gsomlo@gmail.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> > ...
> 
> With this latest version of your patch, I crash during OS X boot with
> "unable to find driver for this platform:\"ACPI\".\n"@/SourceCache/xnu/xnu-2050.48.12/iokit/Kernel/IOPlatformExpert.cpp:1514"
> 
> Your original patch (slightly doctored since it no longer applies cleanly
> to the current qemu git master) is included below, and still works for me.
> 
> Thanks,
> --Gabriel


Thanks for the report!

Peter, if not too late, pls don't pull until we figure it out.

> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index b1a7ebb..4cc8a92 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -643,6 +643,13 @@ static inline char acpi_get_hex(uint32_t val)
>  #define ACPI_PCIHP_SIZEOF (*ssdt_pcihp_end - *ssdt_pcihp_start)
>  #define ACPI_PCIHP_AML (ssdp_pcihp_aml + *ssdt_pcihp_start)
>  
> +#define ACPI_PCINOHP_OFFSET_HEX (*ssdt_pcinohp_name - *ssdt_pcinohp_start + 1)
> +#define ACPI_PCINOHP_OFFSET_ID (*ssdt_pcinohp_id - *ssdt_pcinohp_start)
> +#define ACPI_PCINOHP_OFFSET_ADR (*ssdt_pcinohp_adr - *ssdt_pcinohp_start)
> +#define ACPI_PCINOHP_OFFSET_EJ0 (*ssdt_pcinohp_ej0 - *ssdt_pcinohp_start)
> +#define ACPI_PCINOHP_SIZEOF (*ssdt_pcinohp_end - *ssdt_pcinohp_start)
> +#define ACPI_PCINOHP_AML (ssdp_pcihp_aml + *ssdt_pcinohp_start)
> +
>  #define ACPI_SSDT_SIGNATURE 0x54445353 /* SSDT */
>  #define ACPI_SSDT_HEADER_LENGTH 36
>  
> @@ -677,6 +684,16 @@ static void patch_pcihp(int slot, uint8_t *ssdt_ptr)
>      ssdt_ptr[ACPI_PCIHP_OFFSET_ADR + 2] = slot;
>  }
>  
> +static void patch_pcinohp(int slot, uint8_t *ssdt_ptr)
> +{
> +    unsigned devfn = PCI_DEVFN(slot, 0);
> +
> +    ssdt_ptr[ACPI_PCINOHP_OFFSET_HEX] = acpi_get_hex(devfn >> 4);
> +    ssdt_ptr[ACPI_PCINOHP_OFFSET_HEX + 1] = acpi_get_hex(devfn);
> +    ssdt_ptr[ACPI_PCINOHP_OFFSET_ID] = slot;
> +    ssdt_ptr[ACPI_PCINOHP_OFFSET_ADR + 2] = slot;
> +}
> +
>  /* Assign BSEL property to all buses.  In the future, this can be changed
>   * to only assign to buses that support hotplug.
>   */
> @@ -737,6 +754,7 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
>      AcpiBuildPciBusHotplugState *parent = child->parent;
>      GArray *bus_table = build_alloc_array();
>      DECLARE_BITMAP(slot_hotplug_enable, PCI_SLOT_MAX);
> +    DECLARE_BITMAP(slot_device_present, PCI_SLOT_MAX);
>      uint8_t op;
>      int i;
>      QObject *bsel;
> @@ -764,40 +782,51 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
>          build_append_byte(bus_table, 0x08); /* NameOp */
>          build_append_nameseg(bus_table, "BSEL");
>          build_append_int(bus_table, qint_get_int(qobject_to_qint(bsel)));
> +    }
>  
> -        memset(slot_hotplug_enable, 0xff, sizeof slot_hotplug_enable);
> +    memset(slot_hotplug_enable, 0xff, sizeof slot_hotplug_enable);
> +    memset(slot_device_present, 0x00, sizeof slot_device_present);
>  
> -        for (i = 0; i < ARRAY_SIZE(bus->devices); ++i) {
> -            DeviceClass *dc;
> -            PCIDeviceClass *pc;
> -            PCIDevice *pdev = bus->devices[i];
> +    for (i = 0; i < ARRAY_SIZE(bus->devices); ++i) {
> +        DeviceClass *dc;
> +        PCIDeviceClass *pc;
> +        PCIDevice *pdev = bus->devices[i];
> +        int slot = PCI_SLOT(i);
>  
> -            if (!pdev) {
> -                continue;
> -            }
> +        if (!pdev) {
> +            continue;
> +        }
>  
> -            pc = PCI_DEVICE_GET_CLASS(pdev);
> -            dc = DEVICE_GET_CLASS(pdev);
> +        set_bit(slot, slot_device_present);
> +        pc = PCI_DEVICE_GET_CLASS(pdev);
> +        dc = DEVICE_GET_CLASS(pdev);
>  
> -            if (!dc->hotpluggable || pc->is_bridge) {
> -                int slot = PCI_SLOT(i);
> +        if (!dc->hotpluggable || pc->is_bridge) {
> +            int slot = PCI_SLOT(i);
>  
> -                clear_bit(slot, slot_hotplug_enable);
> -            }
> +            clear_bit(slot, slot_hotplug_enable);
>          }
> +    }
>  
> -        /* Append Device object for each slot which supports eject */
> -        for (i = 0; i < PCI_SLOT_MAX; i++) {
> -            bool can_eject = test_bit(i, slot_hotplug_enable);
> -            if (can_eject) {
> -                void *pcihp = acpi_data_push(bus_table,
> -                                             ACPI_PCIHP_SIZEOF);
> -                memcpy(pcihp, ACPI_PCIHP_AML, ACPI_PCIHP_SIZEOF);
> -                patch_pcihp(i, pcihp);
> -                bus_hotplug_support = true;
> -            }
> +    /* Append Device object for each slot which supports eject */
> +    for (i = 0; i < PCI_SLOT_MAX; i++) {
> +        bool can_eject = test_bit(i, slot_hotplug_enable);
> +        bool present = test_bit(i, slot_device_present);
> +        if (can_eject) {
> +            void *pcihp = acpi_data_push(bus_table,
> +                                         ACPI_PCIHP_SIZEOF);
> +            memcpy(pcihp, ACPI_PCIHP_AML, ACPI_PCIHP_SIZEOF);
> +            patch_pcihp(i, pcihp);
> +            bus_hotplug_support = true;
> +        } else if (present) {
> +            void *pcihp = acpi_data_push(bus_table,
> +                                         ACPI_PCIHP_SIZEOF);
> +            memcpy(pcihp, ACPI_PCINOHP_AML, ACPI_PCINOHP_SIZEOF);
> +            patch_pcinohp(i, pcihp);
>          }
> +    }
>  
> +    if (bsel) {
>          method = build_alloc_method("DVNT", 2);
>  
>          for (i = 0; i < PCI_SLOT_MAX; i++) {
> @@ -976,7 +1005,14 @@ build_ssdt(GArray *table_data, GArray *linker,
>  
>          {
>              AcpiBuildPciBusHotplugState hotplug_state;
> -            PCIBus *bus = find_i440fx(); /* TODO: Q35 support */
> +            Object *pci_host;
> +            PCIBus *bus = NULL;
> +            bool ambiguous;
> +
> +            pci_host = object_resolve_path_type("", TYPE_PCI_HOST_BRIDGE, &ambiguous);
> +            if (!ambiguous && pci_host) {
> +                bus = PCI_HOST_BRIDGE(pci_host)->bus;
> +            }
>  
>              build_pci_bus_state_init(&hotplug_state, NULL);
>  
> diff --git a/hw/i386/ssdt-pcihp.dsl b/hw/i386/ssdt-pcihp.dsl
> index cc245c3..ea4b9e1 100644
> --- a/hw/i386/ssdt-pcihp.dsl
> +++ b/hw/i386/ssdt-pcihp.dsl
> @@ -46,5 +46,17 @@ DefinitionBlock ("ssdt-pcihp.aml", "SSDT", 0x01, "BXPC", "BXSSDTPCIHP", 0x1)
>              }
>          }
>  
> +        ACPI_EXTRACT_DEVICE_START ssdt_pcinohp_start
> +        ACPI_EXTRACT_DEVICE_END ssdt_pcinohp_end
> +        ACPI_EXTRACT_DEVICE_STRING ssdt_pcinohp_name
> +
> +        // Extract the offsets of the device name, address dword and the slot
> +        // name byte - we fill them in for each device.
> +        Device(SBB) {
> +            ACPI_EXTRACT_NAME_BYTE_CONST ssdt_pcinohp_id
> +            Name(_SUN, 0xAA)
> +            ACPI_EXTRACT_NAME_DWORD_CONST ssdt_pcinohp_adr
> +            Name(_ADR, 0xAA0000)
> +        }
>      }
>  }
Peter Maydell Feb. 19, 2014, 1:52 p.m. UTC | #2 
On 17 February 2014 16:44, Michael S. Tsirkin <mst@redhat.com> wrote:
> Peter, if not too late, pls don't pull until we figure it out.

If you want a pull request to not be applied you need to follow
up to the 00/nn cover letter for the pull request to say so.
Otherwise I am likely to either miss the request or not to
remember it when I'm going through the folder of pull
requests to be applied.

thanks
-- PMM
Michael S. Tsirkin Feb. 19, 2014, 2:36 p.m. UTC | #3 
On Wed, Feb 19, 2014 at 01:52:20PM +0000, Peter Maydell wrote:
> On 17 February 2014 16:44, Michael S. Tsirkin <mst@redhat.com> wrote:
> > Peter, if not too late, pls don't pull until we figure it out.
> 
> If you want a pull request to not be applied you need to follow
> up to the 00/nn cover letter for the pull request to say so.
> Otherwise I am likely to either miss the request or not to
> remember it when I'm going through the folder of pull
> requests to be applied.
> 
> thanks
> -- PMM

Good thing you noticed this time :)
Good to know, will do so in the future.

Thanks,
Alex Williamson Feb. 19, 2014, 4:09 p.m. UTC | #4 
On Mon, 2014-02-17 at 09:51 -0500, Gabriel L. Somlo wrote:
> Michael,
> 
> On Mon, Feb 17, 2014 at 04:25:26PM +0200, Michael S. Tsirkin wrote:
> > As reported in
> > http://article.gmane.org/gmane.comp.emulators.qemu/253987
> > Mac OSX actually requires describing all occupied slots
> > in ACPI - even if hotplug isn't enabled.
> > 
> > I didn't expect this so I dropped description of all
> > non hotpluggable slots from ACPI.
> > As a result: before
> > commit 99fd437dee468609de8218f0eb3b16621fb6a9c9 (enable
> > hotplug for pci bridges), PCI cards show up in the "device tree" of OS X
> > (System Information). E.g., on MountainLion users have:
> > 
> > ...
> > 
> > Ethernet still works, but it's not showing up on the PCI bus, and it
> > no longer thinks it's plugged in to slot #2, as it used to before the
> > change.
> > 
> > To fix, append description for all occupied non hotpluggable PCI slots.
> > 
> > One need to be careful when doing this: VGA and ISA device were already
> > described, so we need to drop description from DSDT.
> > 
> > Reported-by: Gabriel L. Somlo <gsomlo@gmail.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> > ...
> 
> With this latest version of your patch, I crash during OS X boot with
> "unable to find driver for this platform:\"ACPI\".\n"@/SourceCache/xnu/xnu-2050.48.12/iokit/Kernel/IOPlatformExpert.cpp:1514"

Also getting a STOP 0xA5 BSOD for win7x64 guest with this.  Thanks,

Alex
diff mbox

Patch

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index b1a7ebb..4cc8a92 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -643,6 +643,13 @@  static inline char acpi_get_hex(uint32_t val)
 #define ACPI_PCIHP_SIZEOF (*ssdt_pcihp_end - *ssdt_pcihp_start)
 #define ACPI_PCIHP_AML (ssdp_pcihp_aml + *ssdt_pcihp_start)
 
+#define ACPI_PCINOHP_OFFSET_HEX (*ssdt_pcinohp_name - *ssdt_pcinohp_start + 1)
+#define ACPI_PCINOHP_OFFSET_ID (*ssdt_pcinohp_id - *ssdt_pcinohp_start)
+#define ACPI_PCINOHP_OFFSET_ADR (*ssdt_pcinohp_adr - *ssdt_pcinohp_start)
+#define ACPI_PCINOHP_OFFSET_EJ0 (*ssdt_pcinohp_ej0 - *ssdt_pcinohp_start)
+#define ACPI_PCINOHP_SIZEOF (*ssdt_pcinohp_end - *ssdt_pcinohp_start)
+#define ACPI_PCINOHP_AML (ssdp_pcihp_aml + *ssdt_pcinohp_start)
+
 #define ACPI_SSDT_SIGNATURE 0x54445353 /* SSDT */
 #define ACPI_SSDT_HEADER_LENGTH 36
 
@@ -677,6 +684,16 @@  static void patch_pcihp(int slot, uint8_t *ssdt_ptr)
     ssdt_ptr[ACPI_PCIHP_OFFSET_ADR + 2] = slot;
 }
 
+static void patch_pcinohp(int slot, uint8_t *ssdt_ptr)
+{
+    unsigned devfn = PCI_DEVFN(slot, 0);
+
+    ssdt_ptr[ACPI_PCINOHP_OFFSET_HEX] = acpi_get_hex(devfn >> 4);
+    ssdt_ptr[ACPI_PCINOHP_OFFSET_HEX + 1] = acpi_get_hex(devfn);
+    ssdt_ptr[ACPI_PCINOHP_OFFSET_ID] = slot;
+    ssdt_ptr[ACPI_PCINOHP_OFFSET_ADR + 2] = slot;
+}
+
 /* Assign BSEL property to all buses.  In the future, this can be changed
  * to only assign to buses that support hotplug.
  */
@@ -737,6 +754,7 @@  static void build_pci_bus_end(PCIBus *bus, void *bus_state)
     AcpiBuildPciBusHotplugState *parent = child->parent;
     GArray *bus_table = build_alloc_array();
     DECLARE_BITMAP(slot_hotplug_enable, PCI_SLOT_MAX);
+    DECLARE_BITMAP(slot_device_present, PCI_SLOT_MAX);
     uint8_t op;
     int i;
     QObject *bsel;
@@ -764,40 +782,51 @@  static void build_pci_bus_end(PCIBus *bus, void *bus_state)
         build_append_byte(bus_table, 0x08); /* NameOp */
         build_append_nameseg(bus_table, "BSEL");
         build_append_int(bus_table, qint_get_int(qobject_to_qint(bsel)));
+    }
 
-        memset(slot_hotplug_enable, 0xff, sizeof slot_hotplug_enable);
+    memset(slot_hotplug_enable, 0xff, sizeof slot_hotplug_enable);
+    memset(slot_device_present, 0x00, sizeof slot_device_present);
 
-        for (i = 0; i < ARRAY_SIZE(bus->devices); ++i) {
-            DeviceClass *dc;
-            PCIDeviceClass *pc;
-            PCIDevice *pdev = bus->devices[i];
+    for (i = 0; i < ARRAY_SIZE(bus->devices); ++i) {
+        DeviceClass *dc;
+        PCIDeviceClass *pc;
+        PCIDevice *pdev = bus->devices[i];
+        int slot = PCI_SLOT(i);
 
-            if (!pdev) {
-                continue;
-            }
+        if (!pdev) {
+            continue;
+        }
 
-            pc = PCI_DEVICE_GET_CLASS(pdev);
-            dc = DEVICE_GET_CLASS(pdev);
+        set_bit(slot, slot_device_present);
+        pc = PCI_DEVICE_GET_CLASS(pdev);
+        dc = DEVICE_GET_CLASS(pdev);
 
-            if (!dc->hotpluggable || pc->is_bridge) {
-                int slot = PCI_SLOT(i);
+        if (!dc->hotpluggable || pc->is_bridge) {
+            int slot = PCI_SLOT(i);
 
-                clear_bit(slot, slot_hotplug_enable);
-            }
+            clear_bit(slot, slot_hotplug_enable);
         }
+    }
 
-        /* Append Device object for each slot which supports eject */
-        for (i = 0; i < PCI_SLOT_MAX; i++) {
-            bool can_eject = test_bit(i, slot_hotplug_enable);
-            if (can_eject) {
-                void *pcihp = acpi_data_push(bus_table,
-                                             ACPI_PCIHP_SIZEOF);
-                memcpy(pcihp, ACPI_PCIHP_AML, ACPI_PCIHP_SIZEOF);
-                patch_pcihp(i, pcihp);
-                bus_hotplug_support = true;
-            }
+    /* Append Device object for each slot which supports eject */
+    for (i = 0; i < PCI_SLOT_MAX; i++) {
+        bool can_eject = test_bit(i, slot_hotplug_enable);
+        bool present = test_bit(i, slot_device_present);
+        if (can_eject) {
+            void *pcihp = acpi_data_push(bus_table,
+                                         ACPI_PCIHP_SIZEOF);
+            memcpy(pcihp, ACPI_PCIHP_AML, ACPI_PCIHP_SIZEOF);
+            patch_pcihp(i, pcihp);
+            bus_hotplug_support = true;
+        } else if (present) {
+            void *pcihp = acpi_data_push(bus_table,
+                                         ACPI_PCIHP_SIZEOF);
+            memcpy(pcihp, ACPI_PCINOHP_AML, ACPI_PCINOHP_SIZEOF);
+            patch_pcinohp(i, pcihp);
         }
+    }
 
+    if (bsel) {
         method = build_alloc_method("DVNT", 2);
 
         for (i = 0; i < PCI_SLOT_MAX; i++) {
@@ -976,7 +1005,14 @@  build_ssdt(GArray *table_data, GArray *linker,
 
         {
             AcpiBuildPciBusHotplugState hotplug_state;
-            PCIBus *bus = find_i440fx(); /* TODO: Q35 support */
+            Object *pci_host;
+            PCIBus *bus = NULL;
+            bool ambiguous;
+
+            pci_host = object_resolve_path_type("", TYPE_PCI_HOST_BRIDGE, &ambiguous);
+            if (!ambiguous && pci_host) {
+                bus = PCI_HOST_BRIDGE(pci_host)->bus;
+            }
 
             build_pci_bus_state_init(&hotplug_state, NULL);
 
diff --git a/hw/i386/ssdt-pcihp.dsl b/hw/i386/ssdt-pcihp.dsl
index cc245c3..ea4b9e1 100644
--- a/hw/i386/ssdt-pcihp.dsl
+++ b/hw/i386/ssdt-pcihp.dsl
@@ -46,5 +46,17 @@  DefinitionBlock ("ssdt-pcihp.aml", "SSDT", 0x01, "BXPC", "BXSSDTPCIHP", 0x1)
             }
         }
 
+        ACPI_EXTRACT_DEVICE_START ssdt_pcinohp_start
+        ACPI_EXTRACT_DEVICE_END ssdt_pcinohp_end
+        ACPI_EXTRACT_DEVICE_STRING ssdt_pcinohp_name
+
+        // Extract the offsets of the device name, address dword and the slot
+        // name byte - we fill them in for each device.
+        Device(SBB) {
+            ACPI_EXTRACT_NAME_BYTE_CONST ssdt_pcinohp_id
+            Name(_SUN, 0xAA)
+            ACPI_EXTRACT_NAME_DWORD_CONST ssdt_pcinohp_adr
+            Name(_ADR, 0xAA0000)
+        }
     }
 }