qemu-devel
[Top][All Lists]
Advanced

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

Re: [Qemu-devel] [PULL 1/8] acpi-build: append description for non-hotpl


From: Michael S. Tsirkin
Subject: Re: [Qemu-devel] [PULL 1/8] acpi-build: append description for non-hotplug
Date: Thu, 27 Feb 2014 17:17:06 +0200

On Thu, Feb 27, 2014 at 09:57:10AM -0500, Gabriel L. Somlo wrote:
> 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

I think that's good enough, thanks a lot!



> 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 <address@hidden>
> > Signed-off-by: Michael S. Tsirkin <address@hidden>
> > ---
> >  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
> > 



reply via email to

[Prev in Thread] Current Thread [Next in Thread]