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 <gso...@gmail.com> > > Signed-off-by: Michael S. Tsirkin <m...@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) > + } > } > }