Michael, This seems to work great, on top of current master (git://git.qemu-project.org/qemu.git).
Did you want me to test how this interacts with any other stuff (i.e. pull from your own git tree), or is this confirmation good enough ? Thanks again, --Gabriel On Thu, Feb 27, 2014 at 03:52:35PM +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: > > Hardware -> PCI Cards: > > Card Type Driver Installed Slot > *ethernet Ethernet Controller Yes PCI Slot 2 > pci8086,2934 USB UHC Yes PCI Slot 29 > > ethernet: > Type: Ethernet Controller > Driver Installed: Yes > MSI: No > Bus: PCI > Slot PCI Slot 2 > Vendor ID: 0x8086 > Device ID: 0x100e > Subsystem Vendor ID: 0x1af4 > Subsystem ID: 0x1100 > Revision ID: 0x0003 > > Hardware -> Ethernet Cards > > ethernet: > Type: Ethernet Controller > Bus: PCI > Slot PCI Slot 2 > Vendor ID: 0x8086 > Device ID: 0x100e > Subsystem Vendor ID: 0x1af4 > Subsystem ID: 0x1100 > Revision ID: 0x0003 > BSD name: en0 > Kext name: AppleIntel8254XEthernet.kext > Location: /System/Library/Extensions/... > Version: 3.1.1b1 > > After commit 99fd437dee468609de8218f0eb3b16621fb6a9c9, users get: > > Hardware -> PCI Cards: > > This computer doesn't contain any PCI cards. If you installed PCI > cards, make sure they're properly installed. > > Hardware -> Ethernet Cards > > ethernet: > Type: Ethernet Controller > Bus: PCI > Vendor ID: 0x8086 > Device ID: 0x100e > Subsystem Vendor ID: 0x1af4 > Subsystem ID: 0x1100 > Revision ID: 0x0003 > BSD name: en0 > Kext name: AppleIntel8254XEthernet.kext > Location: /System/Library/Extensions/... > Version: 3.1.1b1 > > 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 devices > are now described in SSDT, so we need to drop description from DSDT. > And ISA devices are used in DSDT so drop them from SSDT. > > Reported-by: Gabriel L. Somlo <gso...@gmail.com> > Signed-off-by: Michael S. Tsirkin <m...@redhat.com> > --- > hw/i386/acpi-build.c | 143 > ++++++++++++++++++++++++++++++++++++++-------- > tests/acpi-test.c | 2 +- > hw/i386/acpi-dsdt.dsl | 33 ++--------- > hw/i386/q35-acpi-dsdt.dsl | 25 +------- > hw/i386/ssdt-pcihp.dsl | 50 ++++++++++++++++ > 5 files changed, 178 insertions(+), 75 deletions(-) > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > index b1a7ebb..b667d31 100644 > --- a/hw/i386/acpi-build.c > +++ b/hw/i386/acpi-build.c > @@ -643,6 +643,21 @@ 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_ADR (*ssdt_pcinohp_adr - *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_PCIVGA_OFFSET_HEX (*ssdt_pcivga_name - *ssdt_pcivga_start + 1) > +#define ACPI_PCIVGA_OFFSET_ADR (*ssdt_pcivga_adr - *ssdt_pcivga_start) > +#define ACPI_PCIVGA_SIZEOF (*ssdt_pcivga_end - *ssdt_pcivga_start) > +#define ACPI_PCIVGA_AML (ssdp_pcihp_aml + *ssdt_pcivga_start) > + > +#define ACPI_PCIQXL_OFFSET_HEX (*ssdt_pciqxl_name - *ssdt_pciqxl_start + 1) > +#define ACPI_PCIQXL_OFFSET_ADR (*ssdt_pciqxl_adr - *ssdt_pciqxl_start) > +#define ACPI_PCIQXL_SIZEOF (*ssdt_pciqxl_end - *ssdt_pciqxl_start) > +#define ACPI_PCIQXL_AML (ssdp_pcihp_aml + *ssdt_pciqxl_start) > + > #define ACPI_SSDT_SIGNATURE 0x54445353 /* SSDT */ > #define ACPI_SSDT_HEADER_LENGTH 36 > > @@ -677,6 +692,33 @@ 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_ADR + 2] = slot; > +} > + > +static void patch_pcivga(int slot, uint8_t *ssdt_ptr) > +{ > + unsigned devfn = PCI_DEVFN(slot, 0); > + > + ssdt_ptr[ACPI_PCIVGA_OFFSET_HEX] = acpi_get_hex(devfn >> 4); > + ssdt_ptr[ACPI_PCIVGA_OFFSET_HEX + 1] = acpi_get_hex(devfn); > + ssdt_ptr[ACPI_PCIVGA_OFFSET_ADR + 2] = slot; > +} > + > +static void patch_pciqxl(int slot, uint8_t *ssdt_ptr) > +{ > + unsigned devfn = PCI_DEVFN(slot, 0); > + > + ssdt_ptr[ACPI_PCIQXL_OFFSET_HEX] = acpi_get_hex(devfn >> 4); > + ssdt_ptr[ACPI_PCIQXL_OFFSET_HEX + 1] = acpi_get_hex(devfn); > + ssdt_ptr[ACPI_PCIQXL_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 +779,10 @@ 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); > + DECLARE_BITMAP(slot_device_system, PCI_SLOT_MAX); > + DECLARE_BITMAP(slot_device_vga, PCI_SLOT_MAX); > + DECLARE_BITMAP(slot_device_qxl, PCI_SLOT_MAX); > uint8_t op; > int i; > QObject *bsel; > @@ -764,40 +810,82 @@ 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); > + } else { > + /* No bsel - no slots are hot-pluggable */ > + memset(slot_hotplug_enable, 0x00, sizeof slot_hotplug_enable); > + } > > - for (i = 0; i < ARRAY_SIZE(bus->devices); ++i) { > - DeviceClass *dc; > - PCIDeviceClass *pc; > - PCIDevice *pdev = bus->devices[i]; > + memset(slot_device_present, 0x00, sizeof slot_device_present); > + memset(slot_device_system, 0x00, sizeof slot_device_present); > + memset(slot_device_vga, 0x00, sizeof slot_device_vga); > + memset(slot_device_qxl, 0x00, sizeof slot_device_qxl); > > - if (!pdev) { > - continue; > - } > + for (i = 0; i < ARRAY_SIZE(bus->devices); i += PCI_FUNC_MAX) { > + DeviceClass *dc; > + PCIDeviceClass *pc; > + PCIDevice *pdev = bus->devices[i]; > + int slot = PCI_SLOT(i); > > - pc = PCI_DEVICE_GET_CLASS(pdev); > - dc = DEVICE_GET_CLASS(pdev); > + if (!pdev) { > + continue; > + } > > - if (!dc->hotpluggable || pc->is_bridge) { > - int slot = PCI_SLOT(i); > + set_bit(slot, slot_device_present); > + pc = PCI_DEVICE_GET_CLASS(pdev); > + dc = DEVICE_GET_CLASS(pdev); > > - clear_bit(slot, slot_hotplug_enable); > - } > + if (pc->class_id == PCI_CLASS_BRIDGE_ISA) { > + set_bit(slot, slot_device_system); > } > > - /* 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; > + if (pc->class_id == PCI_CLASS_DISPLAY_VGA) { > + set_bit(slot, slot_device_vga); > + > + if (object_dynamic_cast(OBJECT(pdev), "qxl-vga")) { > + set_bit(slot, slot_device_qxl); > } > } > > + if (!dc->hotpluggable || pc->is_bridge) { > + clear_bit(slot, slot_hotplug_enable); > + } > + } > + > + /* Append Device object for each slot */ > + 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); > + bool vga = test_bit(i, slot_device_vga); > + bool qxl = test_bit(i, slot_device_qxl); > + bool system = test_bit(i, slot_device_system); > + 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 (qxl) { > + void *pcihp = acpi_data_push(bus_table, > + ACPI_PCIQXL_SIZEOF); > + memcpy(pcihp, ACPI_PCIQXL_AML, ACPI_PCIQXL_SIZEOF); > + patch_pciqxl(i, pcihp); > + } else if (vga) { > + void *pcihp = acpi_data_push(bus_table, > + ACPI_PCIVGA_SIZEOF); > + memcpy(pcihp, ACPI_PCIVGA_AML, ACPI_PCIVGA_SIZEOF); > + patch_pcivga(i, pcihp); > + } else if (system) { > + /* Nothing to do: system devices are in DSDT. */ > + } else if (present) { > + void *pcihp = acpi_data_push(bus_table, > + ACPI_PCINOHP_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 +1064,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/tests/acpi-test.c b/tests/acpi-test.c > index 31f5359..613dda8 100644 > --- a/tests/acpi-test.c > +++ b/tests/acpi-test.c > @@ -153,7 +153,7 @@ static void free_test_data(test_data *data) > g_free(temp->aml); > } > if (temp->aml_file) { > - if (g_strstr_len(temp->aml_file, -1, "aml-")) { > + if (!temp->asl_file_retain && g_strstr_len(temp->aml_file, -1, > "aml-")) { > unlink(temp->aml_file); > } > g_free(temp->aml_file); > diff --git a/hw/i386/acpi-dsdt.dsl b/hw/i386/acpi-dsdt.dsl > index b23d5e0..0a1e252 100644 > --- a/hw/i386/acpi-dsdt.dsl > +++ b/hw/i386/acpi-dsdt.dsl > @@ -80,6 +80,8 @@ DefinitionBlock ( > Name(_HID, EisaId("PNP0A03")) > Name(_ADR, 0x00) > Name(_UID, 1) > +//#define PX13 S0B_ > +// External(PX13, DeviceObj) > } > } > > @@ -88,34 +90,6 @@ DefinitionBlock ( > > > /**************************************************************** > - * VGA > - ****************************************************************/ > - > - Scope(\_SB.PCI0) { > - Device(VGA) { > - Name(_ADR, 0x00020000) > - OperationRegion(PCIC, PCI_Config, Zero, 0x4) > - Field(PCIC, DWordAcc, NoLock, Preserve) { > - VEND, 32 > - } > - Method(_S1D, 0, NotSerialized) { > - Return (0x00) > - } > - Method(_S2D, 0, NotSerialized) { > - Return (0x00) > - } > - Method(_S3D, 0, NotSerialized) { > - If (LEqual(VEND, 0x1001b36)) { > - Return (0x03) // QXL > - } Else { > - Return (0x00) > - } > - } > - } > - } > - > - > -/**************************************************************** > * PIIX4 PM > ****************************************************************/ > > @@ -132,6 +106,9 @@ DefinitionBlock ( > ****************************************************************/ > > Scope(\_SB.PCI0) { > + > + External(ISA, DeviceObj) > + > Device(ISA) { > Name(_ADR, 0x00010000) > > diff --git a/hw/i386/q35-acpi-dsdt.dsl b/hw/i386/q35-acpi-dsdt.dsl > index d618e9e..f4d2a2d 100644 > --- a/hw/i386/q35-acpi-dsdt.dsl > +++ b/hw/i386/q35-acpi-dsdt.dsl > @@ -72,6 +72,8 @@ DefinitionBlock ( > Name(_ADR, 0x00) > Name(_UID, 1) > > + External(ISA, DeviceObj) > + > // _OSC: based on sample of ACPI3.0b spec > Name(SUPP, 0) // PCI _OSC Support Field value > Name(CTRL, 0) // PCI _OSC Control Field value > @@ -134,34 +136,13 @@ DefinitionBlock ( > > > /**************************************************************** > - * VGA > - ****************************************************************/ > - > - Scope(\_SB.PCI0) { > - Device(VGA) { > - Name(_ADR, 0x00010000) > - Method(_S1D, 0, NotSerialized) { > - Return (0x00) > - } > - Method(_S2D, 0, NotSerialized) { > - Return (0x00) > - } > - Method(_S3D, 0, NotSerialized) { > - Return (0x00) > - } > - } > - } > - > - > -/**************************************************************** > * LPC ISA bridge > ****************************************************************/ > > Scope(\_SB.PCI0) { > /* PCI D31:f0 LPC ISA bridge */ > Device(ISA) { > - /* PCI D31:f0 */ > - Name(_ADR, 0x001f0000) > + Name (_ADR, 0x001F0000) // _ADR: Address > > /* ICH9 PCI to ISA irq remapping */ > OperationRegion(PIRQ, PCI_Config, 0x60, 0x0C) > diff --git a/hw/i386/ssdt-pcihp.dsl b/hw/i386/ssdt-pcihp.dsl > index cc245c3..ac91c05 100644 > --- a/hw/i386/ssdt-pcihp.dsl > +++ b/hw/i386/ssdt-pcihp.dsl > @@ -46,5 +46,55 @@ 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_DWORD_CONST ssdt_pcinohp_adr > + Name(_ADR, 0xAA0000) > + } > + > + ACPI_EXTRACT_DEVICE_START ssdt_pcivga_start > + ACPI_EXTRACT_DEVICE_END ssdt_pcivga_end > + ACPI_EXTRACT_DEVICE_STRING ssdt_pcivga_name > + > + // Extract the offsets of the device name, address dword and the slot > + // name byte - we fill them in for each device. > + Device(SCC) { > + ACPI_EXTRACT_NAME_DWORD_CONST ssdt_pcivga_adr > + Name(_ADR, 0xAA0000) > + Method(_S1D, 0, NotSerialized) { > + Return (0x00) > + } > + Method(_S2D, 0, NotSerialized) { > + Return (0x00) > + } > + Method(_S3D, 0, NotSerialized) { > + Return (0x00) > + } > + } > + > + ACPI_EXTRACT_DEVICE_START ssdt_pciqxl_start > + ACPI_EXTRACT_DEVICE_END ssdt_pciqxl_end > + ACPI_EXTRACT_DEVICE_STRING ssdt_pciqxl_name > + > + // Extract the offsets of the device name, address dword and the slot > + // name byte - we fill them in for each device. > + Device(SDD) { > + ACPI_EXTRACT_NAME_DWORD_CONST ssdt_pciqxl_adr > + Name(_ADR, 0xAA0000) > + Method(_S1D, 0, NotSerialized) { > + Return (0x00) > + } > + Method(_S2D, 0, NotSerialized) { > + Return (0x00) > + } > + Method(_S3D, 0, NotSerialized) { > + Return (0x03) // QXL > + } > + } > } > } > -- > MST >