q35 series take #1

From: Isaku Yamahata <[email protected]>

Pass opaque argument to pci_map_irq_fn like pci_set_irq_fn. ICH9 irq routing is not static, but configurable by chipset configuration registers, so the corresponding irq mapping function of pci_map_irq_fn needs to know the pointer to ich9.

[[email protected]: minor tweaks]

Signed-off-by Isaku Yamahata <[email protected]>

Signed-off-by Jason Baron <[email protected]>

---
 hw/apb_pci.c        |    4 ++--
 hw/bonito.c         |    2 +-
 hw/dec_pci.c        |    2 +-
 hw/grackle_pci.c    |    2 +-
 hw/gt64xxx.c        |    2 +-
 hw/pci.c            |    4 ++--
 hw/pci.h            |    2 +-
 hw/pci_bridge_dev.c |    2 +-
 hw/piix_pci.c       |    2 +-
 hw/ppc4xx_pci.c     |    2 +-
 hw/ppce500_pci.c    |    2 +-

 hw/prep_pci.c       |    2 +-
 hw/sh_pci.c         |    2 +-
 hw/unin_pci.c       |    2 +-
 hw/versatile_pci.c  |    2 +-
 hw/xen.h            |    2 +-
 xen-all.c           |    2 +-
 xen-stub.c          |    2 +-
 18 files changed, 20 insertions(+), 20 deletions(-)

From: Isaku Yamahata <[email protected]>

Introduce a helper function which initializes the ahci port with ide devices. It will be used by q35 support.

Cc Alexander Graf <[email protected]>

Signed-off-by Isaku Yamahata <[email protected]>

Signed-off-by Jason Baron <[email protected]>

---
 hw/ide.h      |    3 +++
 hw/ide/ahci.c |   16 ++++++++++++++++
 2 files changed, 19 insertions(+), 0 deletions(-)

hw/ide.h +3 -0

index 2db4079..8df872e 100644
@@ -36,4 +36,7 @@ int ide_get_bios_chs_trans(BusState *bus, int unit);
 /* ide/core.c */
 void ide_drive_get(DriveInfo **hd, int max_bus);
 
+/* ide/ahci.c */
+void pci_ahci_ide_create_devs(PCIDevice *pci_dev, DriveInfo **hd_table);
+
 #endif /* HW_IDE_H */

hw/ide/ahci.c +6 -0

index 5ea3cad..9561210 100644
@@ -1260,3 +1260,19 @@ static void sysbus_ahci_register_types(void)
 }
 
 type_init(sysbus_ahci_register_types)
+
+void pci_ahci_ide_create_devs(PCIDevice *pci_dev, DriveInfo **hd_table)
+{
+    struct AHCIPCIState *dev = DO_UPCAST(struct AHCIPCIState, card, pci_dev);
+    int i;
+

Quoted 39 lines · Jason Baron

From: Isaku Yamahata <[email protected]>

Introduce a helper function which initializes the ahci port with ide devices. It will be used by q35 support.

Cc Alexander Graf <[email protected]>

Signed-off-by Isaku Yamahata <[email protected]>

Signed-off-by Jason Baron <[email protected]>

---
 hw/ide.h      |    3 +++
 hw/ide/ahci.c |   16 ++++++++++++++++
 2 files changed, 19 insertions(+), 0 deletions(-)

hw/ide.h +3 -0

index 2db4079..8df872e 100644
@@ -36,4 +36,7 @@ int ide_get_bios_chs_trans(BusState *bus, int unit);
 /* ide/core.c */
 void ide_drive_get(DriveInfo **hd, int max_bus);
 
+/* ide/ahci.c */
+void pci_ahci_ide_create_devs(PCIDevice *pci_dev, DriveInfo **hd_table);
+
 #endif /* HW_IDE_H */

hw/ide/ahci.c +6 -0

index 5ea3cad..9561210 100644
@@ -1260,3 +1260,19 @@ static void sysbus_ahci_register_types(void)
 }
 
 type_init(sysbus_ahci_register_types)
+
+void pci_ahci_ide_create_devs(PCIDevice *pci_dev, DriveInfo **hd_table)
+{
+    struct AHCIPCIState *dev = DO_UPCAST(struct AHCIPCIState, card, pci_dev);
+    int i;
+

hw/ide/ahci.c static void sysbus_ahci_register_types(void) · line 1278

Quoted 10 lines

hw/ide/ahci.c +10 -0

+    for (i = 0; i < dev->ahci.ports; i++) {
+        /* master device only, ignore slaves */
+        if (hd_table[i * MAX_IDE_DEVS] == NULL) {
+            continue;
+        }
+        ide_create_drive(&dev->ahci.dev[i].port, 0,
+                         hd_table[i * MAX_IDE_DEVS]);
+    }
+}
+

Ignores odd entries in hd_table[] (MAX_IDE_DEVS is 2). Here's my attempt at explaining why.

-drive has parameters bus, unit, and index. index and (bus, unit) are related in a well-known way that depends on parameter if. For if=ide, index = bus * 2 + unit. This relationship is ABI, i.e. it cannot be changed.

"bus * 2 + unit" makes sense for IDE, because each IDE bus can connect two IDE devices, "master" and "slave".

Boards implementing IDE reject drives with (bus, unit) that make no sense for the board's IDE controller(s). A typical board has a single controller with two buses, which means bus > 1 get rejected.

q35 implements AHCI instead of IDE. It connects if=ide drives to AHCI, because that's felt to be convenient.

An AHCI port can connect a single AHCI device, unlike an IDE bus. This patch identifies maps -drive's bus to AHCI port number.

PATCH 11/25 sets up argument hd_table[] as follows:

    ide_drive_get(hd, MAX_SATA_PORTS);

This rejects bus > MAX_SATA_PORTS. It doesn't reject unit == 1. I believe these get silently ignored. Bug or feature?

Should we reject unit == 1 instead?

Should we map -drive's index to AHCI port number instead?

Quoted 84 lines · Markus Armbruster

Quoted 41 lines · Jason Baron

From: Isaku Yamahata <[email protected]>

Introduce a helper function which initializes the ahci port with ide devices.
It will be used by q35 support.

Cc Alexander Graf <[email protected]>

Signed-off-by Isaku Yamahata <[email protected]>

Signed-off-by Jason Baron <[email protected]>

---
 hw/ide.h      |    3 +++
 hw/ide/ahci.c |   16 ++++++++++++++++
 2 files changed, 19 insertions(+), 0 deletions(-)

hw/ide.h +3 -0

index 2db4079..8df872e 100644
@@ -36,4 +36,7 @@ int ide_get_bios_chs_trans(BusState *bus, int unit);
 /* ide/core.c */
 void ide_drive_get(DriveInfo **hd, int max_bus);
 
+/* ide/ahci.c */
+void pci_ahci_ide_create_devs(PCIDevice *pci_dev, DriveInfo **hd_table);
+
 #endif /* HW_IDE_H */

hw/ide/ahci.c +4 -0

index 5ea3cad..9561210 100644
@@ -1260,3 +1260,19 @@ static void sysbus_ahci_register_types(void)
 }
 
 type_init(sysbus_ahci_register_types)
+
+void pci_ahci_ide_create_devs(PCIDevice *pci_dev, DriveInfo **hd_table)
+{
+ struct AHCIPCIState *dev = DO_UPCAST(struct AHCIPCIState, card,

pci_dev);

hw/ide/ahci.c +2 -0

+    int i;
+

hw/ide/ahci.c static void sysbus_ahci_register_types(void) · line 1278

Quoted 10 lines

hw/ide/ahci.c +10 -0

+    for (i = 0; i < dev->ahci.ports; i++) {
+        /* master device only, ignore slaves */
+        if (hd_table[i * MAX_IDE_DEVS] == NULL) {
+            continue;
+        }
+        ide_create_drive(&dev->ahci.dev[i].port, 0,
+                         hd_table[i * MAX_IDE_DEVS]);
+    }
+}
+

Ignores odd entries in hd_table[] (MAX_IDE_DEVS is 2). Here's my attempt at explaining why.

-drive has parameters bus, unit, and index. index and (bus, unit) are related in a well-known way that depends on parameter if. For if=ide, index = bus * 2 + unit. This relationship is ABI, i.e. it cannot be changed.

"bus * 2 + unit" makes sense for IDE, because each IDE bus can connect two IDE devices, "master" and "slave".

Boards implementing IDE reject drives with (bus, unit) that make no sense for the board's IDE controller(s). A typical board has a single controller with two buses, which means bus > 1 get rejected.

q35 implements AHCI instead of IDE. It connects if=ide drives to AHCI, because that's felt to be convenient.

An AHCI port can connect a single AHCI device, unlike an IDE bus. This patch identifies maps -drive's bus to AHCI port number.

PATCH 11/25 sets up argument hd_table[] as follows:

    ide_drive_get(hd, MAX_SATA_PORTS);

This rejects bus > MAX_SATA_PORTS. It doesn't reject unit == 1. I believe these get silently ignored. Bug or feature?

Should we reject unit == 1 instead?

Quoted 1 line

Should we map -drive's index to AHCI port number instead?

Right, so now that we have ide disks that can be attached to either the legacy ide controller or to ahci, I think we need to differentiate which controller we mean. That is, as proposed q35 is treating -drive if=ide as an ide attached to the ahci controller. I think that is broken behavior b/c we need a way to differentiate between the controllers.

As Alexander Graf has proposed before, I think we need a -drive if=ahci introduced. In that case, I think we reject unit > 0, as you've suggested.

In terms of the current q35 patch series, I think the first step would be to introduce the ahci interface type, and have hda-hdd be added with the default type for q35 of ahci. Then, we can simply fetch ahci drives of index 0-3, and populate the controller, without any of this skipping odd numbers stuff.

The next step would then to add if=ahci interface to -drive.

Quoted 87 lines · Jason Baron

Quoted 85 lines · Markus Armbruster

Quoted 41 lines · Jason Baron

From: Isaku Yamahata <[email protected]>

Introduce a helper function which initializes the ahci port with ide devices.
It will be used by q35 support.

Cc Alexander Graf <[email protected]>

Signed-off-by Isaku Yamahata <[email protected]>

Signed-off-by Jason Baron <[email protected]>

---
 hw/ide.h      |    3 +++
 hw/ide/ahci.c |   16 ++++++++++++++++
 2 files changed, 19 insertions(+), 0 deletions(-)

hw/ide.h +3 -0

index 2db4079..8df872e 100644
@@ -36,4 +36,7 @@ int ide_get_bios_chs_trans(BusState *bus, int unit);
 /* ide/core.c */
 void ide_drive_get(DriveInfo **hd, int max_bus);
 
+/* ide/ahci.c */
+void pci_ahci_ide_create_devs(PCIDevice *pci_dev, DriveInfo **hd_table);
+
 #endif /* HW_IDE_H */

hw/ide/ahci.c +4 -0

index 5ea3cad..9561210 100644
@@ -1260,3 +1260,19 @@ static void sysbus_ahci_register_types(void)
 }
 
 type_init(sysbus_ahci_register_types)
+
+void pci_ahci_ide_create_devs(PCIDevice *pci_dev, DriveInfo **hd_table)
+{
+ struct AHCIPCIState *dev = DO_UPCAST(struct AHCIPCIState, card,

pci_dev);

hw/ide/ahci.c +2 -0

+    int i;
+

hw/ide/ahci.c static void sysbus_ahci_register_types(void) · line 1278

Quoted 10 lines

hw/ide/ahci.c +10 -0

+    for (i = 0; i < dev->ahci.ports; i++) {
+        /* master device only, ignore slaves */
+        if (hd_table[i * MAX_IDE_DEVS] == NULL) {
+            continue;
+        }
+        ide_create_drive(&dev->ahci.dev[i].port, 0,
+                         hd_table[i * MAX_IDE_DEVS]);
+    }
+}
+

Ignores odd entries in hd_table[] (MAX_IDE_DEVS is 2). Here's my attempt at explaining why.

-drive has parameters bus, unit, and index. index and (bus, unit) are related in a well-known way that depends on parameter if. For if=ide, index = bus * 2 + unit. This relationship is ABI, i.e. it cannot be changed.

"bus * 2 + unit" makes sense for IDE, because each IDE bus can connect two IDE devices, "master" and "slave".

Boards implementing IDE reject drives with (bus, unit) that make no sense for the board's IDE controller(s). A typical board has a single controller with two buses, which means bus > 1 get rejected.

q35 implements AHCI instead of IDE. It connects if=ide drives to AHCI, because that's felt to be convenient.

An AHCI port can connect a single AHCI device, unlike an IDE bus. This patch identifies maps -drive's bus to AHCI port number.

PATCH 11/25 sets up argument hd_table[] as follows:

    ide_drive_get(hd, MAX_SATA_PORTS);

This rejects bus > MAX_SATA_PORTS. It doesn't reject unit == 1. I believe these get silently ignored. Bug or feature?

Should we reject unit == 1 instead?

Should we map -drive's index to AHCI port number instead?

Quoted 5 lines

Right, so now that we have ide disks that can be attached to either the legacy ide controller or to ahci, I think we need to differentiate which controller we mean. That is, as proposed q35 is treating -drive if=ide as an ide attached to the ahci controller. I think that is broken behavior b/c we need a way to differentiate between the controllers.

What exactly is broken?

Quoted 3 lines

As Alexander Graf has proposed before, I think we need a -drive if=ahci introduced. In that case, I think we reject unit > 0, as you've suggested.

Achieved by setting if_max_devs[IF_AHCI] to one. bus becomes an alias for index, and unit must be zero.

Alternatively, keep if_max_devs[IF_AHCI] zero. Swaps role of bus and unit.

Alex had if_max_devs[IF_AHCI] = 6.

Quoted 7 lines

In terms of the current q35 patch series, I think the first step would be to introduce the ahci interface type, and have hda-hdd be added with the default type for q35 of ahci. Then, we can simply fetch ahci drives of index 0-3, and populate the controller, without any of this skipping odd numbers stuff.

The next step would then to add if=ahci interface to -drive.

We discussed if=ahci at length before, without reaching consensus. I'd rather not rehash the old arguments again. Instead, let's examine how the command line should behave, and only then figure out how to get that.

1. Drives created with -hd[a-d], -cdrom, or the non-option image argument should connect to well-known connectors of the board's preferred controller.

For current pc, the preferred controller is piix3-ide. -hda connects to its primary bus as master, -hdb as slave. -hdc connects to its secondary bus as master, -hdd as slave.

For pseries, the preferred controller is spapr-vscsi. -hda connects as SCSI ID 0, -hdb as 1, and so forth.

For s390-virtio, the preferred controller is virtio-blk-s390. -hda and -hdb connect to their own virtio-blk-s390 controller, -hdc and -hdd get silently ignored. Yes, that's wacky. Your current q35 patch is similarly wacky, as far as I can tell: -hdb and -hdd get silently ignored.

For q35, the preferred controller is ich9-ahci. I'd expect -hda to connect to port 0, -hdb to port 1, and so forth.

Below the hood, -hda is currently like -drive index=0,media=disk, -hd[b-d] same with index=1..3, and -cdrom is like -drive index=2,media=cdrom, independent of the board.

It follows that -cdrom connects to the same connector as -hdc for all boards. Fine for pc, but may not be as fine for some other boards. You can't use both -hdc and -cdrom at the same time.

The non-option image argument is equivalent to -hda. You can't use both at the same time.

2. Drives created with -drive without if, index, bus, and unit connect to the next unused connector of the board's preferred controller.

If all connectors are in use, behavior currently depends on the board, I think.

3. -drive parameters (if, index) provide more control over the connector to use.

Which controller you get for which if depends on the board. So does the meaning of index. The details can get messy.

For instance, drives with (if, index) the board doesn't support sometimes get ignored silently, and sometimes get flagged as error.

Currently, -drive without parameter if is equivalent to either if=ide or if=scsi. Could be changed for new machine types.

For q35, -drive index=0..5 should connect to ports 0..5 of the board's ich9-ahci.

4. -drive parameters (bus, unit) are an alternate way to specify parameter index. The mapping between index and (bus, unit) depends on the board.

Actually, it depends only on parameter if right now. For if=ide, index = bus * 2 + unit, unit<2. For if=scsi, index = bus * 7 + unit, unit < 7. For everything else, index = unit, bus = 0. We might want to make it depend on the board, see commit 27d6bf40.

For q35, we want index = bus * 6 + unit, unit<5.

An easy way to get that is new if=ahci. Backfires when an AHCI controller with a different number of ports shows up.

We really should make the mapping between index and (bus, unit) depend on the board. And then we can just as well use if=ide to refer to q35's one and only IDE-like controller ich9-ahci.

Quoted 89 lines · Markus Armbruster

Quoted 87 lines · Jason Baron

Quoted 85 lines · Markus Armbruster

Quoted 41 lines · Jason Baron

From: Isaku Yamahata <[email protected]>

Introduce a helper function which initializes the ahci port with ide devices.
It will be used by q35 support.

Cc Alexander Graf <[email protected]>

Signed-off-by Isaku Yamahata <[email protected]>

Signed-off-by Jason Baron <[email protected]>

---
 hw/ide.h      |    3 +++
 hw/ide/ahci.c |   16 ++++++++++++++++
 2 files changed, 19 insertions(+), 0 deletions(-)

hw/ide.h +3 -0

index 2db4079..8df872e 100644
@@ -36,4 +36,7 @@ int ide_get_bios_chs_trans(BusState *bus, int unit);
 /* ide/core.c */
 void ide_drive_get(DriveInfo **hd, int max_bus);
 
+/* ide/ahci.c */
+void pci_ahci_ide_create_devs(PCIDevice *pci_dev, DriveInfo **hd_table);
+
 #endif /* HW_IDE_H */

hw/ide/ahci.c +4 -0

index 5ea3cad..9561210 100644
@@ -1260,3 +1260,19 @@ static void sysbus_ahci_register_types(void)
 }
 
 type_init(sysbus_ahci_register_types)
+
+void pci_ahci_ide_create_devs(PCIDevice *pci_dev, DriveInfo **hd_table)
+{
+ struct AHCIPCIState *dev = DO_UPCAST(struct AHCIPCIState, card,

pci_dev);

hw/ide/ahci.c +2 -0

+    int i;
+

hw/ide/ahci.c static void sysbus_ahci_register_types(void) · line 1278

Quoted 10 lines

hw/ide/ahci.c +10 -0

+    for (i = 0; i < dev->ahci.ports; i++) {
+        /* master device only, ignore slaves */
+        if (hd_table[i * MAX_IDE_DEVS] == NULL) {
+            continue;
+        }
+        ide_create_drive(&dev->ahci.dev[i].port, 0,
+                         hd_table[i * MAX_IDE_DEVS]);
+    }
+}
+

Ignores odd entries in hd_table[] (MAX_IDE_DEVS is 2). Here's my attempt at explaining why.

-drive has parameters bus, unit, and index. index and (bus, unit) are related in a well-known way that depends on parameter if. For if=ide, index = bus * 2 + unit. This relationship is ABI, i.e. it cannot be changed.

"bus * 2 + unit" makes sense for IDE, because each IDE bus can connect two IDE devices, "master" and "slave".

Boards implementing IDE reject drives with (bus, unit) that make no sense for the board's IDE controller(s). A typical board has a single controller with two buses, which means bus > 1 get rejected.

q35 implements AHCI instead of IDE. It connects if=ide drives to AHCI, because that's felt to be convenient.

An AHCI port can connect a single AHCI device, unlike an IDE bus. This patch identifies maps -drive's bus to AHCI port number.

PATCH 11/25 sets up argument hd_table[] as follows:

    ide_drive_get(hd, MAX_SATA_PORTS);

This rejects bus > MAX_SATA_PORTS. It doesn't reject unit == 1. I believe these get silently ignored. Bug or feature?

Should we reject unit == 1 instead?

Should we map -drive's index to AHCI port number instead?

Quoted 8 lines

Quoted 5 lines

Right, so now that we have ide disks that can be attached to either the legacy ide controller or to ahci, I think we need to differentiate which controller we mean. That is, as proposed q35 is treating -drive if=ide as an ide attached to the ahci controller. I think that is broken behavior b/c we need a way to differentiate between the controllers.

What exactly is broken?

I think that -drive if=ide should result in a disk attached piix3-ide. Not in an ide disk attached to the ahci controller (which is current q35 bahavior, and is 'broken' b/c we don't want that to change after q35 is introdued). The reason being is that I think there should be an easy way to create an ide drive on piix3-ide, and an ide drive on the ahci controller. But it sounds like you don't agree with this point.

Quoted 86 lines

Quoted 3 lines

As Alexander Graf has proposed before, I think we need a -drive if=ahci introduced. In that case, I think we reject unit > 0, as you've suggested.

Achieved by setting if_max_devs[IF_AHCI] to one. bus becomes an alias for index, and unit must be zero.

Alternatively, keep if_max_devs[IF_AHCI] zero. Swaps role of bus and unit.

Alex had if_max_devs[IF_AHCI] = 6.

Quoted 7 lines

In terms of the current q35 patch series, I think the first step would be to introduce the ahci interface type, and have hda-hdd be added with the default type for q35 of ahci. Then, we can simply fetch ahci drives of index 0-3, and populate the controller, without any of this skipping odd numbers stuff.

The next step would then to add if=ahci interface to -drive.

We discussed if=ahci at length before, without reaching consensus. I'd rather not rehash the old arguments again. Instead, let's examine how the command line should behave, and only then figure out how to get that.

1. Drives created with -hd[a-d], -cdrom, or the non-option image argument should connect to well-known connectors of the board's preferred controller.

For current pc, the preferred controller is piix3-ide. -hda connects to its primary bus as master, -hdb as slave. -hdc connects to its secondary bus as master, -hdd as slave.

For pseries, the preferred controller is spapr-vscsi. -hda connects as SCSI ID 0, -hdb as 1, and so forth.

For s390-virtio, the preferred controller is virtio-blk-s390. -hda and -hdb connect to their own virtio-blk-s390 controller, -hdc and -hdd get silently ignored. Yes, that's wacky. Your current q35 patch is similarly wacky, as far as I can tell: -hdb and -hdd get silently ignored.

For q35, the preferred controller is ich9-ahci. I'd expect -hda to connect to port 0, -hdb to port 1, and so forth.

Below the hood, -hda is currently like -drive index=0,media=disk, -hd[b-d] same with index=1..3, and -cdrom is like -drive index=2,media=cdrom, independent of the board.

It follows that -cdrom connects to the same connector as -hdc for all boards. Fine for pc, but may not be as fine for some other boards. You can't use both -hdc and -cdrom at the same time.

The non-option image argument is equivalent to -hda. You can't use both at the same time.

2. Drives created with -drive without if, index, bus, and unit connect to the next unused connector of the board's preferred controller.

If all connectors are in use, behavior currently depends on the board, I think.

3. -drive parameters (if, index) provide more control over the connector to use.

Which controller you get for which if depends on the board. So does the meaning of index. The details can get messy.

For instance, drives with (if, index) the board doesn't support sometimes get ignored silently, and sometimes get flagged as error.

Currently, -drive without parameter if is equivalent to either if=ide or if=scsi. Could be changed for new machine types.

For q35, -drive index=0..5 should connect to ports 0..5 of the board's ich9-ahci.

4. -drive parameters (bus, unit) are an alternate way to specify parameter index. The mapping between index and (bus, unit) depends on the board.

Actually, it depends only on parameter if right now. For if=ide, index = bus * 2 + unit, unit<2. For if=scsi, index = bus * 7 + unit, unit < 7. For everything else, index = unit, bus = 0. We might want to make it depend on the board, see commit 27d6bf40.

Quoted 5 lines

For q35, we want index = bus * 6 + unit, unit<5.

An easy way to get that is new if=ahci. Backfires when an AHCI controller with a different number of ports shows up.

I agree with points 1-4.

Quoted 3 lines

We really should make the mapping between index and (bus, unit) depend on the board. And then we can just as well use if=ide to refer to q35's one and only IDE-like controller ich9-ahci.

I agree mapping should depend on the board.

But I'm not sure about using if=ide to use ich9-ahci. I'm suggesting that if=ide should continue to refer to piix3-ide.

Quoted 6 lines · Jason Baron

I think that -drive if=ide should result in a disk attached piix3-ide. Not in an ide disk attached to the ahci controller (which is current q35 bahavior, and is 'broken' b/c we don't want that to change after q35 is introdued). The reason being is that I think there should be an easy way to create an ide drive on piix3-ide, and an ide drive on the ahci controller. But it sounds like you don't agree with this point.

Two issues with that:

1. Why should q35 have a piix3-ide? The ICH9 southbridge provides only SATA, so the board needs additional circuitry to provide PATA. As far as I can tell, intel's Q35 doesn't. ICH9-based boards that do certainly won't use a piix3-ide, because that's a *function* of the PIIX southbridge device. It doesn't exist separately.

2. Why should we connect -drive if=ide to a slow PATA controller instead of a perfectly servicable SATA controller?

Quoted 2 lines

But I'm not sure about using if=ide to use ich9-ahci. I'm suggesting that if=ide should continue to refer to piix3-ide.

Perhaps it should mean "the board's preferred ATA controller", perhaps it should mean "the board's preferred PATA controller". Certainly debatable.

If the latter, not necessarily piix3-ide.

Quoted 99 lines · Jason Baron

Quoted 89 lines · Markus Armbruster

Quoted 87 lines · Jason Baron

Quoted 85 lines · Markus Armbruster

Quoted 41 lines · Jason Baron

From: Isaku Yamahata <[email protected]>

Introduce a helper function which initializes the ahci port with ide devices.
It will be used by q35 support.

Cc Alexander Graf <[email protected]>

Signed-off-by Isaku Yamahata <[email protected]>

Signed-off-by Jason Baron <[email protected]>

---
 hw/ide.h      |    3 +++
 hw/ide/ahci.c |   16 ++++++++++++++++
 2 files changed, 19 insertions(+), 0 deletions(-)

hw/ide.h +3 -0

index 2db4079..8df872e 100644
@@ -36,4 +36,7 @@ int ide_get_bios_chs_trans(BusState *bus, int unit);
 /* ide/core.c */
 void ide_drive_get(DriveInfo **hd, int max_bus);
 
+/* ide/ahci.c */
+void pci_ahci_ide_create_devs(PCIDevice *pci_dev, DriveInfo **hd_table);
+
 #endif /* HW_IDE_H */

hw/ide/ahci.c +4 -0

index 5ea3cad..9561210 100644
@@ -1260,3 +1260,19 @@ static void sysbus_ahci_register_types(void)
 }
 
 type_init(sysbus_ahci_register_types)
+
+void pci_ahci_ide_create_devs(PCIDevice *pci_dev, DriveInfo **hd_table)
+{
+ struct AHCIPCIState *dev = DO_UPCAST(struct AHCIPCIState, card,

pci_dev);

hw/ide/ahci.c +2 -0

+    int i;
+

hw/ide/ahci.c static void sysbus_ahci_register_types(void) · line 1278

Quoted 10 lines

hw/ide/ahci.c +10 -0

+    for (i = 0; i < dev->ahci.ports; i++) {
+        /* master device only, ignore slaves */
+        if (hd_table[i * MAX_IDE_DEVS] == NULL) {
+            continue;
+        }
+        ide_create_drive(&dev->ahci.dev[i].port, 0,
+                         hd_table[i * MAX_IDE_DEVS]);
+    }
+}
+

Ignores odd entries in hd_table[] (MAX_IDE_DEVS is 2). Here's my attempt at explaining why.

-drive has parameters bus, unit, and index. index and (bus, unit) are related in a well-known way that depends on parameter if. For if=ide, index = bus * 2 + unit. This relationship is ABI, i.e. it cannot be changed.

"bus * 2 + unit" makes sense for IDE, because each IDE bus can connect two IDE devices, "master" and "slave".

Boards implementing IDE reject drives with (bus, unit) that make no sense for the board's IDE controller(s). A typical board has a single controller with two buses, which means bus > 1 get rejected.

q35 implements AHCI instead of IDE. It connects if=ide drives to AHCI, because that's felt to be convenient.

An AHCI port can connect a single AHCI device, unlike an IDE bus. This patch identifies maps -drive's bus to AHCI port number.

PATCH 11/25 sets up argument hd_table[] as follows:

    ide_drive_get(hd, MAX_SATA_PORTS);

This rejects bus > MAX_SATA_PORTS. It doesn't reject unit == 1. I believe these get silently ignored. Bug or feature?

Should we reject unit == 1 instead?

Should we map -drive's index to AHCI port number instead?

Quoted 8 lines

Quoted 5 lines

Right, so now that we have ide disks that can be attached to either the legacy ide controller or to ahci, I think we need to differentiate which controller we mean. That is, as proposed q35 is treating -drive if=ide as an ide attached to the ahci controller. I think that is broken behavior b/c we need a way to differentiate between the controllers.

What exactly is broken?

Quoted 6 lines

I think that -drive if=ide should result in a disk attached piix3-ide. Not in an ide disk attached to the ahci controller (which is current q35 bahavior, and is 'broken' b/c we don't want that to change after q35 is introdued). The reason being is that I think there should be an easy way to create an ide drive on piix3-ide, and an ide drive on the ahci controller. But it sounds like you don't agree with this point.

Two issues with that:

1. Why should q35 have a piix3-ide? The ICH9 southbridge provides only SATA, so the board needs additional circuitry to provide PATA. As far as I can tell, intel's Q35 doesn't. ICH9-based boards that do certainly won't use a piix3-ide, because that's a *function* of the PIIX southbridge device. It doesn't exist separately.

2. Why should we connect -drive if=ide to a slow PATA controller instead of a perfectly servicable SATA controller?

Quoted 92 lines

Quoted 86 lines

Quoted 3 lines

As Alexander Graf has proposed before, I think we need a -drive if=ahci introduced. In that case, I think we reject unit > 0, as you've suggested.

Achieved by setting if_max_devs[IF_AHCI] to one. bus becomes an alias for index, and unit must be zero.

Alternatively, keep if_max_devs[IF_AHCI] zero. Swaps role of bus and unit.

Alex had if_max_devs[IF_AHCI] = 6.

Quoted 7 lines

In terms of the current q35 patch series, I think the first step would be to introduce the ahci interface type, and have hda-hdd be added with the default type for q35 of ahci. Then, we can simply fetch ahci drives of index 0-3, and populate the controller, without any of this skipping odd numbers stuff.

The next step would then to add if=ahci interface to -drive.

We discussed if=ahci at length before, without reaching consensus. I'd rather not rehash the old arguments again. Instead, let's examine how the command line should behave, and only then figure out how to get that.

1. Drives created with -hd[a-d], -cdrom, or the non-option image argument should connect to well-known connectors of the board's preferred controller.

For current pc, the preferred controller is piix3-ide. -hda connects to its primary bus as master, -hdb as slave. -hdc connects to its secondary bus as master, -hdd as slave.

For pseries, the preferred controller is spapr-vscsi. -hda connects as SCSI ID 0, -hdb as 1, and so forth.

For s390-virtio, the preferred controller is virtio-blk-s390. -hda and -hdb connect to their own virtio-blk-s390 controller, -hdc and -hdd get silently ignored. Yes, that's wacky. Your current q35 patch is similarly wacky, as far as I can tell: -hdb and -hdd get silently ignored.

For q35, the preferred controller is ich9-ahci. I'd expect -hda to connect to port 0, -hdb to port 1, and so forth.

Below the hood, -hda is currently like -drive index=0,media=disk, -hd[b-d] same with index=1..3, and -cdrom is like -drive index=2,media=cdrom, independent of the board.

It follows that -cdrom connects to the same connector as -hdc for all boards. Fine for pc, but may not be as fine for some other boards. You can't use both -hdc and -cdrom at the same time.

The non-option image argument is equivalent to -hda. You can't use both at the same time.

2. Drives created with -drive without if, index, bus, and unit connect to the next unused connector of the board's preferred controller.

If all connectors are in use, behavior currently depends on the board, I think.

3. -drive parameters (if, index) provide more control over the connector to use.

Which controller you get for which if depends on the board. So does the meaning of index. The details can get messy.

For instance, drives with (if, index) the board doesn't support sometimes get ignored silently, and sometimes get flagged as error.

Currently, -drive without parameter if is equivalent to either if=ide or if=scsi. Could be changed for new machine types.

For q35, -drive index=0..5 should connect to ports 0..5 of the board's ich9-ahci.

4. -drive parameters (bus, unit) are an alternate way to specify parameter index. The mapping between index and (bus, unit) depends on the board.

Actually, it depends only on parameter if right now. For if=ide, index = bus * 2 + unit, unit<2. For if=scsi, index = bus * 7 + unit, unit < 7. For everything else, index = unit, bus = 0. We might want to make it depend on the board, see commit 27d6bf40.

Quoted 5 lines

For q35, we want index = bus * 6 + unit, unit<5.

An easy way to get that is new if=ahci. Backfires when an AHCI controller with a different number of ports shows up.

Quoted 7 lines

I agree with points 1-4.

Quoted 3 lines

We really should make the mapping between index and (bus, unit) depend on the board. And then we can just as well use if=ide to refer to q35's one and only IDE-like controller ich9-ahci.

I agree mapping should depend on the board.

We agree on the important things then.

Quoted 2 lines

But I'm not sure about using if=ide to use ich9-ahci. I'm suggesting that if=ide should continue to refer to piix3-ide.

Perhaps it should mean "the board's preferred ATA controller", perhaps it should mean "the board's preferred PATA controller". Certainly debatable.

If the latter, not necessarily piix3-ide.

From: Isaku Yamahata <[email protected]>

Factor out smram/pam logic for later use.
Which will be used by q35 too.

[[email protected]: changes for updated memory API]

Signed-off-by Isaku Yamahata <[email protected]>

Signed-off-by Jason Baron <[email protected]>

---
 hw/i386/Makefile.objs |    1 +
 hw/pam.c              |  121 +++++++++++++++++++++++++++++++++++++++++++++++++
 hw/pam.h              |   98 +++++++++++++++++++++++++++++++++++++++
 hw/piix_pci.c         |   65 ++++----------------------
 4 files changed, 230 insertions(+), 55 deletions(-)
 create mode 100644 hw/pam.c
 create mode 100644 hw/pam.h

hw/i386/Makefile.objs +1 -0

index 8c764bb..2f0c172 100644
@@ -6,6 +6,7 @@ obj-y += pci-hotplug.o smbios.o wdt_ib700.o
 obj-y += debugcon.o multiboot.o
 obj-y += pc_piix.o
 obj-y += pc_sysfw.o
+obj-y += pam.o
 obj-$(CONFIG_XEN) += xen_platform.o xen_apic.o
 obj-$(CONFIG_XEN_PCI_PASSTHROUGH) += xen-host-pci-device.o
 obj-$(CONFIG_XEN_PCI_PASSTHROUGH) += xen_pt.o xen_pt_config_init.o xen_pt_msi.o

hw/pam.c new file +54 -0

new file mode 100644
index 0000000..11afeb3
@@ -0,0 +1,121 @@
+/*
+ * QEMU i440FX/PIIX3 PCI Bridge Emulation
+ *
+ * Copyright (c) 2006 Fabrice Bellard
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ *
+ * Split out from piix_pci.c
+ * Copyright (c) 2011 Isaku Yamahata <yamahata at valinux co jp>
+ *                    VA Linux Systems Japan K.K.
+ * Copyright (c) 2012 Jason Baron <[email protected]>
+ *
+ */
+
+#include "sysemu.h"
+#include "pam.h"
+
+void smram_update(MemoryRegion *smram_region, uint8_t smram,
+                  uint8_t smm_enabled)
+{
+    bool smram_enabled;
+
+    smram_enabled = ((smm_enabled && (smram & SMRAM_G_SMRAME)) ||
+                        (smram & SMRAM_D_OPEN));
+    memory_region_set_enabled(smram_region, !smram_enabled);
+}
+
+void smram_set_smm(uint8_t *host_smm_enabled, int smm, uint8_t smram,
+                   MemoryRegion *smram_region)
+{
+    uint8_t smm_enabled = (smm != 0);
+    if (*host_smm_enabled != smm_enabled) {
+        *host_smm_enabled = smm_enabled;
+        smram_update(smram_region, smram, *host_smm_enabled);
+    }
+}
+
+static void pam_update_seg(PAMMemoryRegion *mem, uint32_t start, uint32_t size,

hw/pam.c new file +57 -0

+    switch (attr) {
+    case PAM_ATTR_WE | PAM_ATTR_RE:
+        /* RAM */
+        memory_region_init_alias(&mem->mem, "pam-ram", ram_memory,
+                                 start, size);
+        break;
+    case PAM_ATTR_RE:
+        /* ROM (XXX: not quite correct) */
+        memory_region_init_alias(&mem->mem, "pam-rom", ram_memory,
+                                 start, size);
+        memory_region_set_readonly(&mem->mem, true);
+        break;
+    case PAM_ATTR_WE:
+    case 0:
+        /* XXX: should distinguish read/write cases */
+        memory_region_init_alias(&mem->mem, "pam-pci", pci_address_space,
+                                 start, size);
+        break;
+    default:
+        abort();
+        break;
+    }
+    memory_region_add_subregion_overlap(system_memory, start, &mem->mem, 1);
+    mem->initialized = true;
+
+}
+
+static uint8_t pam_attr(uint8_t val, int hi)
+{
+    return (val >> ((!!hi) * 4)) & PAM_ATTR_MASK;
+}
+
+void pam_update(PAMMemoryRegion *mem, int idx, uint8_t val,
+                MemoryRegion *ram_memory, MemoryRegion *pci_address_space,
+                MemoryRegion *system_memory)
+{
+    uint32_t phys_addr;
+    int map_idx;
+
+    assert(0 <= idx && idx <= PAM_IDX_MAX);
+
+    if (idx == 0) {
+        pam_update_seg(&mem[0], PAM_BIOS_BASE, PAM_BIOS_SIZE, ram_memory,
+                       pci_address_space, system_memory, pam_attr(val, 1));
+        return;
+    }
+
+    map_idx = (idx - 1) * 2;
+
+    phys_addr = PAM_EXPAN_BASE + PAM_EXPAN_SIZE * map_idx;
+    pam_update_seg(&mem[map_idx + 1], phys_addr, PAM_EXPAN_SIZE, ram_memory,
+                   pci_address_space, system_memory, pam_attr(val, 0));
+
+    phys_addr += PAM_EXPAN_SIZE;
+    pam_update_seg(&mem[map_idx + 2], phys_addr, PAM_EXPAN_SIZE, ram_memory,
+                   pci_address_space, system_memory, pam_attr(val, 1));
+}

hw/pam.h new file +98 -0

new file mode 100644
index 0000000..ce89a2a
@@ -0,0 +1,98 @@
+#ifndef QEMU_PAM_H
+#define QEMU_PAM_H
+
+/*
+ * Copyright (c) 2006 Fabrice Bellard
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+/*
+ * Split out from piix_pci.c
+ * Copyright (c) 2011 Isaku Yamahata <yamahata at valinux co jp>
+ *                    VA Linux Systems Japan K.K.
+ * Copyright (c) 2012 Jason Baron <[email protected]>
+ *
+ * SMRAM memory area and PAM memory area in Legacy address range for PC.
+ * PAM: Programmable Attribute Map registers
+ *
+ * 0xa0000 - 0xbffff compatible SMRAM
+ *
+ * 0xc0000 - 0xc3fff Expansion area memory segments
+ * 0xc4000 - 0xc7fff
+ * 0xc8000 - 0xcbfff
+ * 0xcc000 - 0xcffff
+ * 0xd0000 - 0xd7fff
+ * 0xd8000 - 0xdbfff
+ * 0xdc000 - 0xdffff
+ * 0xe0000 - 0xe3fff Extended System BIOS Area Memory Segments
+ * 0xe4000 - 0xe7fff
+ * 0xe8000 - 0xebfff
+ * 0xec000 - 0xeffff
+ *
+ * 0xf0000 - 0xfffff System BIOS Area Memory Segments
+ */
+
+#include "qemu-common.h"
+#include "memory.h"
+
+#define SMRAM_C_BASE    0xa0000
+#define SMRAM_C_END     0xc0000
+#define SMRAM_C_SIZE    0x20000
+
+
+#define PAM_EXPAN_BASE  0xc0000
+#define PAM_EXPAN_SIZE  0x04000
+
+#define PAM_EXBIOS_BASE 0xe0000
+#define PAM_EXBIOS_SIZE 0x04000
+
+#define PAM_BIOS_BASE   0xf0000
+#define PAM_BIOS_END    0xfffff
+/* 64KB: Intel 3 series express chipset family p. 58*/
+#define PAM_BIOS_SIZE   0x10000
+
+/* PAM registers: log nibble and high nibble*/
+#define PAM_ATTR_WE     ((uint8_t)2)
+#define PAM_ATTR_RE     ((uint8_t)1)
+#define PAM_ATTR_MASK   ((uint8_t)3)
+
+#define PAM_IDX_MAX     6       /* pam0 - pam6 */
+
+/* SMRAM register */
+#define SMRAM_D_OPEN           ((uint8_t)(1 << 6))
+#define SMRAM_D_CLS            ((uint8_t)(1 << 5))
+#define SMRAM_D_LCK            ((uint8_t)(1 << 4))
+#define SMRAM_G_SMRAME         ((uint8_t)(1 << 3))
+#define SMRAM_C_BASE_SEG_MASK  ((uint8_t)0x7)
+#define SMRAM_C_BASE_SEG       ((uint8_t)0x2)  /* hardwired to b010 */
+
+typedef struct PAMMemoryRegion {
+    MemoryRegion mem;
+    bool initialized;
+} PAMMemoryRegion;
+
+void smram_update(MemoryRegion *smram_region, uint8_t smram,
+                  uint8_t smm_enabled);
+void smram_set_smm(uint8_t *host_smm_enabled, int smm, uint8_t smram,
+                   MemoryRegion *smram_region);
+void pam_update(PAMMemoryRegion *mem, int idx, uint8_t val,
+                MemoryRegion *ram_memory, MemoryRegion *pci_address_space,
+                MemoryRegion *system_memory);
+
+#endif /* QEMU_PAM_H */

hw/piix_pci.c +10 -56

index 376a287..4861cf6 100644
@@ -30,6 +30,7 @@
 #include "sysbus.h"
 #include "range.h"
 #include "xen.h"
+#include "pam.h"
 
 /*
  * I440FX chipset data sheet.

@@ -68,11 +69,6 @@ typedef struct PIIX3State {
     int32_t pci_irq_levels_vmstate[PIIX_NUM_PIRQS];
 } PIIX3State;
 
-typedef struct PAMMemoryRegion {
-    MemoryRegion mem;
-    bool initialized;
-} PAMMemoryRegion;
-
 struct PCII440FXState {
     PCIDevice dev;
     MemoryRegion *system_memory;

@@ -105,56 +101,16 @@ static int pci_slot_get_pirq(void *opaque, PCIDevice *pci_dev, int pci_intx)
     return (pci_intx + slot_addend) & 3;
 }
 
-static void update_pam(PCII440FXState *d, uint32_t start, uint32_t end, int r,
-                       PAMMemoryRegion *mem)
-{
-    if (mem->initialized) {
-        memory_region_del_subregion(d->system_memory, &mem->mem);
-        memory_region_destroy(&mem->mem);
-    }
-
-    //    printf("ISA mapping %08x-0x%08x: %d\n", start, end, r);
-    switch(r) {
-    case 3:
-        /* RAM */
-        memory_region_init_alias(&mem->mem, "pam-ram", d->ram_memory,
-                                 start, end - start);
-        break;
-    case 1:
-        /* ROM (XXX: not quite correct) */
-        memory_region_init_alias(&mem->mem, "pam-rom", d->ram_memory,
-                                 start, end - start);
-        memory_region_set_readonly(&mem->mem, true);
-        break;
-    case 2:
-    case 0:
-        /* XXX: should distinguish read/write cases */
-        memory_region_init_alias(&mem->mem, "pam-pci", d->pci_address_space,
-                                 start, end - start);
-        break;
-    }
-    memory_region_add_subregion_overlap(d->system_memory,
-                                        start, &mem->mem, 1);
-    mem->initialized = true;
-}
-
 static void i440fx_update_memory_mappings(PCII440FXState *d)
 {
-    int i, r;
-    uint32_t smram;
-    bool smram_enabled;
+    int i;
 
     memory_region_transaction_begin();
-    update_pam(d, 0xf0000, 0x100000, (d->dev.config[I440FX_PAM] >> 4) & 3,
-               &d->pam_regions[0]);
-    for(i = 0; i < 12; i++) {
-        r = (d->dev.config[(i >> 1) + (I440FX_PAM + 1)] >> ((i & 1) * 4)) & 3;
-        update_pam(d, 0xc0000 + 0x4000 * i, 0xc0000 + 0x4000 * (i + 1), r,
-                   &d->pam_regions[i+1]);
+    for (i = 0; i <= PAM_IDX_MAX; i++) {
+        pam_update(&d->pam_regions[0], i, d->dev.config[I440FX_PAM + i],
+                    d->ram_memory, d->pci_address_space, d->system_memory);
     }
-    smram = d->dev.config[I440FX_SMRAM];
-    smram_enabled = (d->smm_enabled && (smram & 0x08)) || (smram & 0x40);
-    memory_region_set_enabled(&d->smram_region, !smram_enabled);
+    smram_update(&d->smram_region, d->dev.config[I440FX_SMRAM], d->smm_enabled);
     memory_region_transaction_commit();
 }
 

@@ -162,11 +118,10 @@ static void i440fx_set_smm(int val, void *arg)
 {
     PCII440FXState *d = arg;
 
-    val = (val != 0);
-    if (d->smm_enabled != val) {
-        d->smm_enabled = val;
-        i440fx_update_memory_mappings(d);
-    }
+    memory_region_transaction_begin();
+    smram_set_smm(&d->smm_enabled, val, d->dev.config[I440FX_SMRAM],
+                  &d->smram_region);
+    memory_region_transaction_commit();
 }
 
 
--

1.7.1

Let's use PCIExpressHost with QOM.

Signed-off-by Jason Baron <[email protected]>

hw/pc_q35.c line 326

Quoted 3 lines · Jason Baron

hw/pc_q35.c new file +3 -0

+    if (ram_size >= 0xe0000000) {
+        above_4g_mem_size = ram_size - 0xe0000000;
+        below_4g_mem_size = 0xe0000000;

We should make the hole larger to get more pci i/o address space below 4G, especially as q35 needs a good chunk of it for mmconfig.

I think at least 0xc0000000, maybe even 0xb0000000. The later would allow to place mmconfig @ 0xb0000000, one big 512MB pci bar @ 0xc0000000 (think gfx card pass-through) and all the remaining little stuff above 0xe0000000.

cheers,
Gerd

hw/pc_q35.c line 326

Quoted 3 lines · Jason Baron

hw/pc_q35.c new file +3 -0

+    if (ram_size >= 0xe0000000) {
+        above_4g_mem_size = ram_size - 0xe0000000;
+        below_4g_mem_size = 0xe0000000;

We should make the hole larger to get more pci i/o address space below 4G, especially as q35 needs a good chunk of it for mmconfig.

I think at least 0xc0000000, maybe even 0xb0000000. The later would allow to place mmconfig @ 0xb0000000, one big 512MB pci bar @ 0xc0000000 (think gfx card pass-through) and all the remaining little stuff above 0xe0000000.

From: Isaku Yamahata <[email protected]>

pc q35 based chipset emulator to support pci express natively.

Signed-off-by Isaku Yamahata <[email protected]>

Signed-off-by Jason Baron <[email protected]>

---
 hw/acpi_ich9.c        |  315 ++++++++++++++++++
 hw/acpi_ich9.h        |   53 +++
 hw/i386/Makefile.objs |    1 +
 hw/pc_q35.c           |  378 +++++++++++++++++++++
 hw/pci_ids.h          |   14 +
 hw/q35.c              |  877 +++++++++++++++++++++++++++++++++++++++++++++++++
 hw/q35.h              |  272 +++++++++++++++
 hw/q35_smbus.c        |  154 +++++++++
 8 files changed, 2064 insertions(+), 0 deletions(-)
 create mode 100644 hw/acpi_ich9.c
 create mode 100644 hw/acpi_ich9.h
 create mode 100644 hw/pc_q35.c
 create mode 100644 hw/q35.c
 create mode 100644 hw/q35.h
 create mode 100644 hw/q35_smbus.c

hw/acpi_ich9.c new file +11 -0

new file mode 100644
index 0000000..59c0807
@@ -0,0 +1,315 @@
+/*
+ * ACPI implementation
+ *
+ * Copyright (c) 2006 Fabrice Bellard
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License version 2 as published by the Free Software Foundation.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of

hw/acpi_ich9.c new file +1 -0

+ */

hw/acpi_ich9.h new file +16 -0

new file mode 100644
index 0000000..f55c0e9
@@ -0,0 +1,53 @@
+/*
+ * QEMU GMCH/ICH9 LPC PM Emulation
+ *
+ *  Copyright (c) 2009 Isaku Yamahata <yamahata at valinux co jp>
+ *                     VA Linux Systems Japan K.K.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *

I think it's best to smash the following patch into this one otherwise review becomes painful. Some short notes:

Quoted 34 lines

---
 hw/acpi_ich9.c        |  315 ++++++++++++++++++
 hw/acpi_ich9.h        |   53 +++
 hw/i386/Makefile.objs |    1 +
 hw/pc_q35.c           |  378 +++++++++++++++++++++
 hw/pci_ids.h          |   14 +
 hw/q35.c              |  877 +++++++++++++++++++++++++++++++++++++++++++++++++
 hw/q35.h              |  272 +++++++++++++++
 hw/q35_smbus.c        |  154 +++++++++
 8 files changed, 2064 insertions(+), 0 deletions(-)
 create mode 100644 hw/acpi_ich9.c
 create mode 100644 hw/acpi_ich9.h
 create mode 100644 hw/pc_q35.c
 create mode 100644 hw/q35.c
 create mode 100644 hw/q35.h
 create mode 100644 hw/q35_smbus.c

hw/acpi_ich9.c new file +11 -0

new file mode 100644
index 0000000..59c0807
@@ -0,0 +1,315 @@
+/*
+ * ACPI implementation
+ *
+ * Copyright (c) 2006 Fabrice Bellard
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License version 2 as published by the Free Software Foundation.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of

hw/acpi_ich9.c line 21

Quoted 10 lines

hw/acpi_ich9.c new file +10 -0

+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>
+ */
+/*
+ *  Copyright (c) 2009 Isaku Yamahata <yamahata at valinux co jp>
+ *                     VA Linux Systems Japan K.K.
+ *

Probably two headers should be combined.

hw/acpi_ich9.c line 22

Quoted 1 line

hw/acpi_ich9.c new file +1 -0

+ *  This is based on acpi.c.

This should probably go into commit log instead.

Quoted 1 line

hw/acpi_ich9.c new file +1 -0

+ */

..

Quoted 28 lines

hw/acpi_ich9.h new file +22 -0

new file mode 100644
index 0000000..f55c0e9
@@ -0,0 +1,53 @@
+/*
+ * QEMU GMCH/ICH9 LPC PM Emulation
+ *
+ *  Copyright (c) 2009 Isaku Yamahata <yamahata at valinux co jp>
+ *                     VA Linux Systems Japan K.K.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>
+ */
+
+#ifndef HW_ACPI_ICH9_H
+#define HW_ACPI_ICH9_H

hw/acpi_ich9.h line 26

Quoted 4 lines

hw/acpi_ich9.h new file +4 -0

+
+#include "acpi.h"
+
+typedef struct ICH9_LPCPmRegs {

This naming scheme conflicts with qemu coding style. Pls use Ich9LpcPmRegs or something.
Same comment applies everywhere.

hw/pc_q35.c new file +368 -0

new file mode 100644
index 0000000..4f75d97
@@ -0,0 +1,378 @@
+/*
+ * QEMU PC System Emulator
+ *
+ * Copyright (c) 2003-2004 Fabrice Bellard
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+/*
+ *  Q35 chipset based pc system emulator
+ *
+ *  Copyright (c) 2009, 2010
+ *                     Isaku Yamahata <yamahata at valinux co jp>
+ *                     VA Linux Systems Japan K.K.
+ *
+ *  This is based on pc.c, but heavily modified.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>
+ */
+#include "hw.h"
+#include "arch_init.h"
+#include "pc.h"
+#include "fdc.h"
+#include "pci.h"
+#include "pci_bridge.h"
+#include "pci_p2pbr.h"
+#include "ioh3420.h"
+#include "xio3130_upstream.h"
+#include "xio3130_downstream.h"
+#include "block.h"
+#include "blockdev.h"
+#include "sysemu.h"
+#include "audio/audio.h"
+#include "net.h"
+#include "smbus.h"
+#include "boards.h"
+#include "monitor.h"
+#include "fw_cfg.h"
+#include "hpet_emul.h"
+#include "watchdog.h"
+#include "smbios.h"
+#include "ide.h"
+#include "usb-uhci.h"
+
+#include "q35.h"
+
+/* ICH9 AHCI has 6 ports */
+#define MAX_SATA_PORTS     6
+
+#define I21154_REV            0x05
+#define I21154_PI             0x00
+
+static PCIBridge *i21154_init(PCIBus *bus, int devfn, const char *bus_name,
+                              bool multifunction)
+{
+    const PCIP2PBridgeInit init = {
+        .bus = bus,
+        .devfn = devfn,
+        .multifunction = multifunction,
+
+        .bus_name = bus_name,
+        .map_irq = pci_swizzle_map_irq_fn,
+    };
+    const PCIP2PBridgeProp prop = {
+        .vendor_id = PCI_VENDOR_ID_DEC,
+        .device_id = PCI_DEVICE_ID_DEC_21154,
+        .revision_id = I21154_REV,
+        .prog_interface = I21154_PI,
+    };
+    return pci_p2pbr_create_simple(&init, &prop);
+}
+
+static void pc_q35_bridge_init(PCIBus *host_bus, PCIBus *pci_bus)
+{
+    uint8_t dev;
+    uint8_t sec_bus;
+    uint8_t port = 0;
+    uint8_t chassis = 0;
+    uint16_t slot = 0;
+    uint8_t upstream_port;
+    PCIESlot *s;
+    uint8_t fn;
+    PCIESlot *root_port;
+    PCIBus *root_port_bus;
+    char buf[16];
+
+    /* PCI to PCI bridge b6:d[29 - 31]:f0, 6:[1c - 1f].0 with subordinate bus
+       of 7 - 9 on b0:d30:f0, 0.1e.0 = bus */
+#define Q35_P2P_BRDIGE_DEV_BASE         28
+#define Q35_P2P_BRDIGE_DEV_MAX          32
+#define Q35_P2P_BRDIGE_SUBBUS_BASE      (ICH9_D2P_SECONDARY_DEFAULT + 1)
+    for (dev = Q35_P2P_BRDIGE_DEV_BASE; dev < Q35_P2P_BRDIGE_DEV_MAX; dev++) {
+        PCIBridge *br;
+        sec_bus = Q35_P2P_BRDIGE_SUBBUS_BASE + dev - Q35_P2P_BRDIGE_DEV_BASE;
+
+        snprintf(buf, sizeof(buf), "pci.%d", sec_bus);
+        br = i21154_init(pci_bus, PCI_DEVFN(dev, 0), buf, true);
+    }
+
+    /* PCIe root port b0:d1:f0 in GMCH.
+     * Actually it's vid/did = 0x8086:0x29c1, but we substitute ioh for it.
+     */
+    sec_bus = 32;
+    snprintf(buf, sizeof(buf), "pcie.%d", sec_bus);
+    s = ioh3420_init(host_bus, PCI_DEVFN(GMCH_PCIE_DEV, GMCH_PCIE_FUNC), true,
+                     buf, pci_swizzle_map_irq_fn, port, chassis, slot);
+
+
+    /* more slots. ICH9 doesn't have those, but many slots are wanted. */
+//#define Q35_MANY_SLOTS
+#undef Q35_MANY_SLOTS
+
+#ifdef Q35_MANY_SLOTS
+#define Q35_NR_ROOTPORT         6
+#define Q35_NR_UPSTREAM         8
+#define Q35_NR_DOWNSTREAM       16
+#else
+#define Q35_NR_ROOTPORT         1
+#define Q35_NR_UPSTREAM         1
+#define Q35_NR_DOWNSTREAM       1
+#endif
+
+    /* PCIe root port b0:d23:f[0-5], 0.17.[0-5] */
+    for (fn = 0; fn < Q35_NR_ROOTPORT; fn++) {
+        sec_bus++;
+        port++;
+        slot++;
+
+        snprintf(buf, sizeof(buf), "pcie.%d", sec_bus);
+        s = ioh3420_init(host_bus, PCI_DEVFN(23, fn), true,
+                         buf, pci_swizzle_map_irq_fn, port, chassis, slot);
+    }
+
+    /* PCIe root port b0:d24:f0 */
+    sec_bus++;
+    port++;
+    slot++;
+    snprintf(buf, sizeof(buf), "pcie.%d", sec_bus);
+    root_port = ioh3420_init(host_bus, PCI_DEVFN(24, 0), true,
+                             buf, pci_swizzle_map_irq_fn, port, chassis, slot);
+    root_port_bus = pci_bridge_get_sec_bus(&root_port->port.br);
+
+    /* 8 * 16 = 128 slots */
+    upstream_port = 0;
+    for (fn = 0; fn < Q35_NR_UPSTREAM; fn++) {
+        PCIEPort *upstream;
+        PCIBus *upstream_bus;
+        uint16_t downstream_port;
+
+        uint8_t ds_dev_max;
+        uint8_t ds_dev;
+        uint8_t ds_fn_max;
+        uint8_t ds_fn;
+
+        /* PCIe upstream port d0:f[0-7] */
+        sec_bus++;
+        snprintf(buf, sizeof(buf), "pcie.%d", sec_bus);
+        upstream = xio3130_upstream_init(root_port_bus, PCI_DEVFN(0, fn),
+                                         true, buf, pci_swizzle_map_irq_fn,
+                                         upstream_port);
+
+        upstream_bus = pci_bridge_get_sec_bus(&upstream->br);
+        upstream_port++;
+
+        /* PCIe downstream port */
+        downstream_port = 0;
+        ds_fn_max = MIN(Q35_NR_DOWNSTREAM / PCI_SLOT_MAX, PCI_FUNC_MAX);
+        ds_dev_max = MIN(Q35_NR_DOWNSTREAM / (ds_fn_max + 1), PCI_SLOT_MAX);
+
+        for (ds_dev = 0; ds_dev <= ds_dev_max &&
+                 downstream_port < Q35_NR_DOWNSTREAM; ds_dev++) {
+            for (ds_fn = 0; ds_fn <= ds_fn_max &&
+                     downstream_port < Q35_NR_DOWNSTREAM; ds_fn++) {
+                sec_bus++;
+                slot++;
+                snprintf(buf, sizeof(buf), "pcie.%d", sec_bus);
+
+                xio3130_downstream_init(upstream_bus, PCI_DEVFN(ds_dev, ds_fn),
+                                        true, buf, pci_swizzle_map_irq_fn,
+                                        downstream_port, chassis, slot);
+                downstream_port++;
+            }
+        }
+    }
+
+    /* PCIe root port b0:d28:f[0-6] in ICH9.
+     * Actually it's vid/did = 0x8086:0x294[02468A], but we substitute ioh
+     * for them.
+     */
+    for (fn = 0; fn < ICH9_PCIE_FUNC_MAX; fn++) {
+        sec_bus++;
+        port++;
+        slot++;
+
+        snprintf(buf, sizeof(buf), "pcie.%d", sec_bus);
+        s = ioh3420_init(host_bus, PCI_DEVFN(ICH9_PCIE_DEV, fn), true,
+                         buf, pci_swizzle_map_irq_fn,
+                         port, chassis, slot);
+    }
+}
+
+static void pc_q35_init_early(qemu_irq *isa_irq, IsaIrqState *isa_irq_state,
+                              DeviceState **gmch_host_p,
+                              PCIBus **host_bus_p, PCIBus **pci_bus_p,
+                              PCIDevice **lpc_p)
+{
+    DeviceState *gmch_host;
+    PCIBus *host_bus;
+    PCIBus *pci_bus;
+
+    PCIDevice *gmch_state;
+    PCIDevice *lpc;
+
+    /* create pci host bus */
+    host_bus = gmch_host_init(&gmch_host, isa_irq, isa_irq_state->ioapic);
+    gmch_state = gmch_init(gmch_host, host_bus);
+
+    /* create conventional pci bus: pcie2pci bridge */
+    pci_bus = ich9_d2pbr_init(host_bus, PCI_DEVFN(ICH9_D2P_BRIDGE_DEV,
+                                                  ICH9_D2P_BRIDGE_FUNC),
+                              ICH9_D2P_SECONDARY_DEFAULT);
+
+    /* create child pci/pcie buses */
+    pc_q35_bridge_init(host_bus, pci_bus);
+
+    /* create ISA bus */
+    lpc = gmch_lpc_init(gmch_host, host_bus);
+
+    *gmch_host_p = gmch_host;
+    *host_bus_p = host_bus;
+    *pci_bus_p = pci_bus;
+    *lpc_p = lpc;
+}
+
+static void pc_q35_init_late(BusState **idebus, ISADevice *rtc_state,
+                             DeviceState *gmch_host,
+                             PCIBus *host_bus, PCIBus *pci_bus,
+                             PCIDevice *lpc)
+{
+    qemu_irq *cmos_s3;
+    PCIDevice *ahci;
+    DriveInfo *hd[MAX_SATA_PORTS * MAX_IDE_DEVS];
+
+    /* connect pm stuff to lpc */
+    cmos_s3 = qemu_allocate_irqs(pc_cmos_set_s3_resume, rtc_state, 1);
+    ich9_lpc_pm_init(gmch_host, lpc, *cmos_s3);
+
+    /* ahci and SATA device */
+    ide_drive_get(hd, MAX_SATA_PORTS);
+    ahci = pci_create_simple_multifunction(host_bus,
+                                           PCI_DEVFN(ICH9_SATA1_DEV,
+                                                     ICH9_SATA1_FUNC),
+                                           true, "ich9-ahci");
+    pci_ahci_ide_create_devs(ahci, hd);
+    idebus[0] = qdev_get_child_bus(&ahci->qdev, "ide.0");
+    idebus[1] = qdev_get_child_bus(&ahci->qdev, "ide.1");
+
+    if (usb_enabled) {
+        /* Should we create 6 UHCI according to ich9 spec? */
+        pci_create_simple_multifunction(
+            host_bus, PCI_DEVFN(ICH9_USB_UHCI1_DEV, ICH9_USB_UHCI1_FUNC),
+            true, "ich9-usb-uhci1");
+        /* XXX: EHCI */
+    }
+
+    /* TODO: Populate SPD eeprom data.  */
+    smbus_eeprom_init(ich9_smb_init(host_bus,
+                                    PCI_DEVFN(ICH9_SMB_DEV, ICH9_SMB_FUNC),
+                                    0xb100),
+                      8, NULL, 0);
+}
+
+/* PC hardware initialisation */
+static void pc_q35_init(ram_addr_t ram_size,
+                        const char *boot_device,
+                        const char *kernel_filename,
+                        const char *kernel_cmdline,
+                        const char *initrd_filename,
+                        const char *cpu_model)
+{
+    ram_addr_t below_4g_mem_size, above_4g_mem_size;
+    DeviceState *gmch_host;
+    PCIBus *host_bus;
+    PCIBus *pci_bus;
+    PCIDevice *lpc;
+    qemu_irq *isa_irq;
+    IsaIrqState *isa_irq_state;
+    BusState *idebus[MAX_SATA_PORTS];
+    ISADevice *rtc_state;
+    MemoryRegion *pci_memory;
+    MemoryRegion *rom_memory;
+    MemoryRegion *ram_memory;
+
+    pc_cpus_init(cpu_model);
+
+    /* FIXME: add kvm clock ? */
+
+    if (ram_size >= 0xe0000000) {
+        above_4g_mem_size = ram_size - 0xe0000000;
+        below_4g_mem_size = 0xe0000000;
+    } else {
+        above_4g_mem_size = 0;
+        below_4g_mem_size = ram_size;
+    }
+
+    /* pci enabled */
+    pci_memory = g_new(MemoryRegion, 1);
+    memory_region_init(pci_memory, "pci", INT64_MAX);
+    rom_memory = pci_memory;
+
+    /* allocate ram and load rom/bios */
+    pc_memory_init(get_system_memory(), kernel_filename, kernel_cmdline,
+                   initrd_filename, below_4g_mem_size, above_4g_mem_size,
+                   rom_memory, &ram_memory);
+
+    /* irq lines */
+    isa_irq = pc_isa_irq(&isa_irq_state);
+    ioapic_init(isa_irq_state);
+
+    pc_q35_init_early(isa_irq, isa_irq_state,
+                      &gmch_host, &host_bus, &pci_bus, &lpc);
+    isa_bus_irqs(isa_irq);
+    pc_register_ferr_irq(isa_get_irq(13));
+
+    /* init basic PC hardware */
+    pc_basic_device_init(isa_irq, &rtc_state, false);
+
+    pc_q35_init_late(idebus, rtc_state, gmch_host, host_bus, pci_bus, lpc);
+
+    pc_cmos_init(below_4g_mem_size, above_4g_mem_size, boot_device,
+                 idebus[0], idebus[1], rtc_state);
+
+    /* the rest devices to which pci devfn is automatically assigned */
+    pc_vga_init(host_bus);
+    audio_init(isa_irq, pci_bus);
+    pc_nic_init(pci_bus);
+    pc_pci_device_init(pci_bus);
+}
+
+static QEMUMachine pc_q35_machine = {
+    .name = "pc_q35",
+    .desc = "Q35 chipset PC",

Quoted 374 lines · Jason Baron

hw/pc_q35.c new file +368 -0

new file mode 100644
index 0000000..4f75d97
@@ -0,0 +1,378 @@
+/*
+ * QEMU PC System Emulator
+ *
+ * Copyright (c) 2003-2004 Fabrice Bellard
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+/*
+ *  Q35 chipset based pc system emulator
+ *
+ *  Copyright (c) 2009, 2010
+ *                     Isaku Yamahata <yamahata at valinux co jp>
+ *                     VA Linux Systems Japan K.K.
+ *
+ *  This is based on pc.c, but heavily modified.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>
+ */
+#include "hw.h"
+#include "arch_init.h"
+#include "pc.h"
+#include "fdc.h"
+#include "pci.h"
+#include "pci_bridge.h"
+#include "pci_p2pbr.h"
+#include "ioh3420.h"
+#include "xio3130_upstream.h"
+#include "xio3130_downstream.h"
+#include "block.h"
+#include "blockdev.h"
+#include "sysemu.h"
+#include "audio/audio.h"
+#include "net.h"
+#include "smbus.h"
+#include "boards.h"
+#include "monitor.h"
+#include "fw_cfg.h"
+#include "hpet_emul.h"
+#include "watchdog.h"
+#include "smbios.h"
+#include "ide.h"
+#include "usb-uhci.h"
+
+#include "q35.h"
+
+/* ICH9 AHCI has 6 ports */
+#define MAX_SATA_PORTS     6
+
+#define I21154_REV            0x05
+#define I21154_PI             0x00
+
+static PCIBridge *i21154_init(PCIBus *bus, int devfn, const char *bus_name,
+                              bool multifunction)
+{
+    const PCIP2PBridgeInit init = {
+        .bus = bus,
+        .devfn = devfn,
+        .multifunction = multifunction,
+
+        .bus_name = bus_name,
+        .map_irq = pci_swizzle_map_irq_fn,
+    };
+    const PCIP2PBridgeProp prop = {
+        .vendor_id = PCI_VENDOR_ID_DEC,
+        .device_id = PCI_DEVICE_ID_DEC_21154,
+        .revision_id = I21154_REV,
+        .prog_interface = I21154_PI,
+    };
+    return pci_p2pbr_create_simple(&init, &prop);
+}
+
+static void pc_q35_bridge_init(PCIBus *host_bus, PCIBus *pci_bus)
+{
+    uint8_t dev;
+    uint8_t sec_bus;
+    uint8_t port = 0;
+    uint8_t chassis = 0;
+    uint16_t slot = 0;
+    uint8_t upstream_port;
+    PCIESlot *s;
+    uint8_t fn;
+    PCIESlot *root_port;
+    PCIBus *root_port_bus;
+    char buf[16];
+
+    /* PCI to PCI bridge b6:d[29 - 31]:f0, 6:[1c - 1f].0 with subordinate bus
+       of 7 - 9 on b0:d30:f0, 0.1e.0 = bus */
+#define Q35_P2P_BRDIGE_DEV_BASE         28
+#define Q35_P2P_BRDIGE_DEV_MAX          32
+#define Q35_P2P_BRDIGE_SUBBUS_BASE      (ICH9_D2P_SECONDARY_DEFAULT + 1)
+    for (dev = Q35_P2P_BRDIGE_DEV_BASE; dev < Q35_P2P_BRDIGE_DEV_MAX; dev++) {
+        PCIBridge *br;
+        sec_bus = Q35_P2P_BRDIGE_SUBBUS_BASE + dev - Q35_P2P_BRDIGE_DEV_BASE;
+
+        snprintf(buf, sizeof(buf), "pci.%d", sec_bus);
+        br = i21154_init(pci_bus, PCI_DEVFN(dev, 0), buf, true);
+    }
+
+    /* PCIe root port b0:d1:f0 in GMCH.
+     * Actually it's vid/did = 0x8086:0x29c1, but we substitute ioh for it.
+     */
+    sec_bus = 32;
+    snprintf(buf, sizeof(buf), "pcie.%d", sec_bus);
+    s = ioh3420_init(host_bus, PCI_DEVFN(GMCH_PCIE_DEV, GMCH_PCIE_FUNC), true,
+                     buf, pci_swizzle_map_irq_fn, port, chassis, slot);
+
+
+    /* more slots. ICH9 doesn't have those, but many slots are wanted. */
+//#define Q35_MANY_SLOTS
+#undef Q35_MANY_SLOTS
+
+#ifdef Q35_MANY_SLOTS
+#define Q35_NR_ROOTPORT         6
+#define Q35_NR_UPSTREAM         8
+#define Q35_NR_DOWNSTREAM       16
+#else
+#define Q35_NR_ROOTPORT         1
+#define Q35_NR_UPSTREAM         1
+#define Q35_NR_DOWNSTREAM       1
+#endif
+
+    /* PCIe root port b0:d23:f[0-5], 0.17.[0-5] */
+    for (fn = 0; fn < Q35_NR_ROOTPORT; fn++) {
+        sec_bus++;
+        port++;
+        slot++;
+
+        snprintf(buf, sizeof(buf), "pcie.%d", sec_bus);
+        s = ioh3420_init(host_bus, PCI_DEVFN(23, fn), true,
+                         buf, pci_swizzle_map_irq_fn, port, chassis, slot);
+    }
+
+    /* PCIe root port b0:d24:f0 */
+    sec_bus++;
+    port++;
+    slot++;
+    snprintf(buf, sizeof(buf), "pcie.%d", sec_bus);
+    root_port = ioh3420_init(host_bus, PCI_DEVFN(24, 0), true,
+                             buf, pci_swizzle_map_irq_fn, port, chassis, slot);
+    root_port_bus = pci_bridge_get_sec_bus(&root_port->port.br);
+
+    /* 8 * 16 = 128 slots */
+    upstream_port = 0;
+    for (fn = 0; fn < Q35_NR_UPSTREAM; fn++) {
+        PCIEPort *upstream;
+        PCIBus *upstream_bus;
+        uint16_t downstream_port;
+
+        uint8_t ds_dev_max;
+        uint8_t ds_dev;
+        uint8_t ds_fn_max;
+        uint8_t ds_fn;
+
+        /* PCIe upstream port d0:f[0-7] */
+        sec_bus++;
+        snprintf(buf, sizeof(buf), "pcie.%d", sec_bus);
+        upstream = xio3130_upstream_init(root_port_bus, PCI_DEVFN(0, fn),
+                                         true, buf, pci_swizzle_map_irq_fn,
+                                         upstream_port);
+
+        upstream_bus = pci_bridge_get_sec_bus(&upstream->br);
+        upstream_port++;
+
+        /* PCIe downstream port */
+        downstream_port = 0;
+        ds_fn_max = MIN(Q35_NR_DOWNSTREAM / PCI_SLOT_MAX, PCI_FUNC_MAX);
+        ds_dev_max = MIN(Q35_NR_DOWNSTREAM / (ds_fn_max + 1), PCI_SLOT_MAX);
+
+        for (ds_dev = 0; ds_dev <= ds_dev_max &&
+                 downstream_port < Q35_NR_DOWNSTREAM; ds_dev++) {
+            for (ds_fn = 0; ds_fn <= ds_fn_max &&
+                     downstream_port < Q35_NR_DOWNSTREAM; ds_fn++) {
+                sec_bus++;
+                slot++;
+                snprintf(buf, sizeof(buf), "pcie.%d", sec_bus);
+
+                xio3130_downstream_init(upstream_bus, PCI_DEVFN(ds_dev, ds_fn),
+                                        true, buf, pci_swizzle_map_irq_fn,
+                                        downstream_port, chassis, slot);
+                downstream_port++;
+            }
+        }
+    }
+
+    /* PCIe root port b0:d28:f[0-6] in ICH9.
+     * Actually it's vid/did = 0x8086:0x294[02468A], but we substitute ioh
+     * for them.
+     */
+    for (fn = 0; fn < ICH9_PCIE_FUNC_MAX; fn++) {
+        sec_bus++;
+        port++;
+        slot++;
+
+        snprintf(buf, sizeof(buf), "pcie.%d", sec_bus);
+        s = ioh3420_init(host_bus, PCI_DEVFN(ICH9_PCIE_DEV, fn), true,
+                         buf, pci_swizzle_map_irq_fn,
+                         port, chassis, slot);
+    }
+}
+
+static void pc_q35_init_early(qemu_irq *isa_irq, IsaIrqState *isa_irq_state,
+                              DeviceState **gmch_host_p,
+                              PCIBus **host_bus_p, PCIBus **pci_bus_p,
+                              PCIDevice **lpc_p)
+{
+    DeviceState *gmch_host;
+    PCIBus *host_bus;
+    PCIBus *pci_bus;
+
+    PCIDevice *gmch_state;
+    PCIDevice *lpc;
+
+    /* create pci host bus */
+    host_bus = gmch_host_init(&gmch_host, isa_irq, isa_irq_state->ioapic);
+    gmch_state = gmch_init(gmch_host, host_bus);
+
+    /* create conventional pci bus: pcie2pci bridge */
+    pci_bus = ich9_d2pbr_init(host_bus, PCI_DEVFN(ICH9_D2P_BRIDGE_DEV,
+                                                  ICH9_D2P_BRIDGE_FUNC),
+                              ICH9_D2P_SECONDARY_DEFAULT);
+
+    /* create child pci/pcie buses */
+    pc_q35_bridge_init(host_bus, pci_bus);
+
+    /* create ISA bus */
+    lpc = gmch_lpc_init(gmch_host, host_bus);
+
+    *gmch_host_p = gmch_host;
+    *host_bus_p = host_bus;
+    *pci_bus_p = pci_bus;
+    *lpc_p = lpc;
+}
+
+static void pc_q35_init_late(BusState **idebus, ISADevice *rtc_state,
+                             DeviceState *gmch_host,
+                             PCIBus *host_bus, PCIBus *pci_bus,
+                             PCIDevice *lpc)
+{
+    qemu_irq *cmos_s3;
+    PCIDevice *ahci;
+    DriveInfo *hd[MAX_SATA_PORTS * MAX_IDE_DEVS];
+
+    /* connect pm stuff to lpc */
+    cmos_s3 = qemu_allocate_irqs(pc_cmos_set_s3_resume, rtc_state, 1);
+    ich9_lpc_pm_init(gmch_host, lpc, *cmos_s3);
+
+    /* ahci and SATA device */
+    ide_drive_get(hd, MAX_SATA_PORTS);
+    ahci = pci_create_simple_multifunction(host_bus,
+                                           PCI_DEVFN(ICH9_SATA1_DEV,
+                                                     ICH9_SATA1_FUNC),
+                                           true, "ich9-ahci");
+    pci_ahci_ide_create_devs(ahci, hd);
+    idebus[0] = qdev_get_child_bus(&ahci->qdev, "ide.0");
+    idebus[1] = qdev_get_child_bus(&ahci->qdev, "ide.1");
+
+    if (usb_enabled) {
+        /* Should we create 6 UHCI according to ich9 spec? */
+        pci_create_simple_multifunction(
+            host_bus, PCI_DEVFN(ICH9_USB_UHCI1_DEV, ICH9_USB_UHCI1_FUNC),
+            true, "ich9-usb-uhci1");
+        /* XXX: EHCI */
+    }
+
+    /* TODO: Populate SPD eeprom data.  */
+    smbus_eeprom_init(ich9_smb_init(host_bus,
+                                    PCI_DEVFN(ICH9_SMB_DEV, ICH9_SMB_FUNC),
+                                    0xb100),
+                      8, NULL, 0);
+}
+
+/* PC hardware initialisation */
+static void pc_q35_init(ram_addr_t ram_size,
+                        const char *boot_device,
+                        const char *kernel_filename,
+                        const char *kernel_cmdline,
+                        const char *initrd_filename,
+                        const char *cpu_model)
+{
+    ram_addr_t below_4g_mem_size, above_4g_mem_size;
+    DeviceState *gmch_host;
+    PCIBus *host_bus;
+    PCIBus *pci_bus;
+    PCIDevice *lpc;
+    qemu_irq *isa_irq;
+    IsaIrqState *isa_irq_state;
+    BusState *idebus[MAX_SATA_PORTS];
+    ISADevice *rtc_state;
+    MemoryRegion *pci_memory;
+    MemoryRegion *rom_memory;
+    MemoryRegion *ram_memory;
+
+    pc_cpus_init(cpu_model);
+
+    /* FIXME: add kvm clock ? */
+
+    if (ram_size >= 0xe0000000) {
+        above_4g_mem_size = ram_size - 0xe0000000;
+        below_4g_mem_size = 0xe0000000;
+    } else {
+        above_4g_mem_size = 0;
+        below_4g_mem_size = ram_size;
+    }
+
+    /* pci enabled */
+    pci_memory = g_new(MemoryRegion, 1);
+    memory_region_init(pci_memory, "pci", INT64_MAX);
+    rom_memory = pci_memory;
+
+    /* allocate ram and load rom/bios */
+    pc_memory_init(get_system_memory(), kernel_filename, kernel_cmdline,
+                   initrd_filename, below_4g_mem_size, above_4g_mem_size,
+                   rom_memory, &ram_memory);
+
+    /* irq lines */
+    isa_irq = pc_isa_irq(&isa_irq_state);
+    ioapic_init(isa_irq_state);
+
+    pc_q35_init_early(isa_irq, isa_irq_state,
+                      &gmch_host, &host_bus, &pci_bus, &lpc);
+    isa_bus_irqs(isa_irq);
+    pc_register_ferr_irq(isa_get_irq(13));
+
+    /* init basic PC hardware */
+    pc_basic_device_init(isa_irq, &rtc_state, false);
+
+    pc_q35_init_late(idebus, rtc_state, gmch_host, host_bus, pci_bus, lpc);
+
+    pc_cmos_init(below_4g_mem_size, above_4g_mem_size, boot_device,
+                 idebus[0], idebus[1], rtc_state);
+
+    /* the rest devices to which pci devfn is automatically assigned */
+    pc_vga_init(host_bus);
+    audio_init(isa_irq, pci_bus);
+    pc_nic_init(pci_bus);
+    pc_pci_device_init(pci_bus);
+}
+
+static QEMUMachine pc_q35_machine = {
+    .name = "pc_q35",
+    .desc = "Q35 chipset PC",

hw/pc_q35.c line 378

Quoted 10 lines

hw/pc_q35.c new file +10 -0

+    .init = pc_q35_init,
+    .max_cpus = 255,
+};
+
+static void pc_q35_machine_init(void)
+{
+    qemu_register_machine(&pc_q35_machine);
+}
+
+machine_init(pc_q35_machine_init);

I wonder if it's possible for q35 to have no devices at all in the default machine and have it fully qualified by -device switches?

I envision -M qemu-1.3 switch that does exactly that, so that machine type only handles things like cross version compatibility.

Signature · 2 lines

-- 
MST

Quoted 7 lines

I wonder if it's possible for q35 to have no devices at all in the default machine and have it fully qualified by -device switches?

I envision -M qemu-1.3 switch that does exactly that, so that machine type only handles things like cross version compatibility.

It's more than just "no devices". It's a matter of proper modeling of the chipset, the super I/O chip, and peripherals.

Rebase q35 to 1.2 - memory api updates, acpi updates, qom...

Signed-off-by Jason Baron <[email protected]>

---
 hw/acpi_ich9.c |   65 ++++----
 hw/acpi_ich9.h |    9 +-
 hw/pc.h        |    2 +
 hw/pc_piix.c   |    4 +-
 hw/pc_q35.c    |  189 +++++++++++++++-------
 hw/q35.c       |  477 +++++++++++++++++++++++++++++++-------------------------
 hw/q35.h       |   97 +++++++++++-
 hw/q35_smbus.c |   78 +++++----
 8 files changed, 568 insertions(+), 353 deletions(-)

hw/acpi_ich9.c +32 -33

index 59c0807..0d66109 100644
@@ -18,6 +18,7 @@
 /*
  *  Copyright (c) 2009 Isaku Yamahata <yamahata at valinux co jp>
  *                     VA Linux Systems Japan K.K.
+ *  Copyright (C) 2012 Jason Baron <[email protected]>
  *
  *  This is based on acpi.c.
  */

@@ -47,9 +48,9 @@ static void pm_update_sci(ICH9_LPCPmRegs *pm)
 {
     int sci_level, pm1a_sts;
 
-    pm1a_sts = acpi_pm1_evt_get_sts(&pm->pm1a, pm->tmr.overflow_time);
+    pm1a_sts = acpi_pm1_evt_get_sts(&pm->acpi_regs);
 
-    sci_level = (((pm1a_sts & pm->pm1a.en) &
+    sci_level = (((pm1a_sts & pm->acpi_regs.pm1.evt.en) &
                   (ACPI_BITMASK_RT_CLOCK_ENABLE |
                    ACPI_BITMASK_POWER_BUTTON_ENABLE |
                    ACPI_BITMASK_GLOBAL_LOCK_ENABLE |

@@ -57,14 +58,14 @@ static void pm_update_sci(ICH9_LPCPmRegs *pm)
     qemu_set_irq(pm->irq, sci_level);
 
     /* schedule a timer interruption if needed */
-    acpi_pm_tmr_update(&pm->tmr,
-                       (pm->pm1a.en & ACPI_BITMASK_TIMER_ENABLE) &&
+    acpi_pm_tmr_update(&pm->acpi_regs,
+                       (pm->acpi_regs.pm1.evt.en & ACPI_BITMASK_TIMER_ENABLE) &&
                        !(pm1a_sts & ACPI_BITMASK_TIMER_STATUS));
 }
 
-static void ich9_pm_update_sci_fn(ACPIPMTimer *tmr)
+static void ich9_pm_update_sci_fn(ACPIREGS *regs)
 {
-    ICH9_LPCPmRegs *pm = container_of(tmr, ICH9_LPCPmRegs, tmr);
+    ICH9_LPCPmRegs *pm = container_of(regs, ICH9_LPCPmRegs, acpi_regs);
     pm_update_sci(pm);
 }
 

@@ -74,7 +75,7 @@ static void pm_ioport_writeb(void *opaque, uint32_t addr, uint32_t val)
 
     switch (addr & ICH9_PMIO_MASK) {
     case ICH9_PMIO_GPE0_STS ... (ICH9_PMIO_GPE0_STS + ICH9_PMIO_GPE0_LEN - 1):
-        acpi_gpe_ioport_writeb(&pm->gpe0, addr, val);
+        acpi_gpe_ioport_writeb(&pm->acpi_regs, addr, val);
         break;
     default:
         break;

@@ -90,7 +91,7 @@ static uint32_t pm_ioport_readb(void *opaque, uint32_t addr)
 
     switch (addr & ICH9_PMIO_MASK) {
     case ICH9_PMIO_GPE0_STS ... (ICH9_PMIO_GPE0_STS + ICH9_PMIO_GPE0_LEN - 1):
-        val = acpi_gpe_ioport_readb(&pm->gpe0, addr);
+        val = acpi_gpe_ioport_readb(&pm->acpi_regs, addr);
         break;
     default:
         val = 0;

@@ -106,15 +107,15 @@ static void pm_ioport_writew(void *opaque, uint32_t addr, uint32_t val)
 
     switch (addr & ICH9_PMIO_MASK) {
     case ICH9_PMIO_PM1_STS:
-        acpi_pm1_evt_write_sts(&pm->pm1a, &pm->tmr, val);
+        acpi_pm1_evt_write_sts(&pm->acpi_regs, val);
         pm_update_sci(pm);
         break;
     case ICH9_PMIO_PM1_EN:
-        pm->pm1a.en = val;
+        pm->acpi_regs.pm1.evt.en = val;
         pm_update_sci(pm);
         break;
     case ICH9_PMIO_PM1_CNT:
-        acpi_pm1_cnt_write(&pm->pm1a, &pm->pm1_cnt, val);
+        acpi_pm1_cnt_write(&pm->acpi_regs, val, 0);
         break;
     default:
         pm_ioport_write_fallback(opaque, addr, 2, val);

@@ -130,13 +131,13 @@ static uint32_t pm_ioport_readw(void *opaque, uint32_t addr)
 
     switch (addr & ICH9_PMIO_MASK) {
     case ICH9_PMIO_PM1_STS:
-        val = acpi_pm1_evt_get_sts(&pm->pm1a, pm->tmr.overflow_time);
+        val = acpi_pm1_evt_get_sts(&pm->acpi_regs);
         break;
     case ICH9_PMIO_PM1_EN:
-        val = pm->pm1a.en;
+        val = pm->acpi_regs.pm1.evt.en;
         break;
     case ICH9_PMIO_PM1_CNT:
-        val = pm->pm1_cnt.cnt;
+        val = pm->acpi_regs.pm1.cnt.cnt;
         break;
     default:
         val = pm_ioport_read_fallback(opaque, addr, 2);

@@ -168,7 +169,7 @@ static uint32_t pm_ioport_readl(void *opaque, uint32_t addr)
 
     switch (addr & ICH9_PMIO_MASK) {
     case ICH9_PMIO_PM1_TMR:
-        val = acpi_pm_tmr_get(&pm->tmr);
+        val = acpi_pm_tmr_get(&pm->acpi_regs);
         break;
     case ICH9_PMIO_SMI_EN:
         val = pm->smi_en;

@@ -238,7 +239,7 @@ void ich9_pm_iospace_update(ICH9_LPCPmRegs *pm, uint32_t pm_io_base)
     register_ioport_read(pm_io_base, ICH9_PMIO_SIZE, 4, pm_ioport_readl, pm);
 
     pm->pm_io_base = pm_io_base;
-    acpi_gpe_blk(&pm->gpe0, pm_io_base + ICH9_PMIO_GPE0_STS);
+    acpi_gpe_blk(&pm->acpi_regs, pm_io_base + ICH9_PMIO_GPE0_STS);
 }
 
 static int ich9_pm_post_load(void *opaque, int version_id)

@@ -268,13 +269,13 @@ const VMStateDescription vmstate_ich9_pm = {
     .minimum_version_id_old = 1,
     .post_load = ich9_pm_post_load,
     .fields = (VMStateField[]) {
-        VMSTATE_UINT16(pm1a.sts, ICH9_LPCPmRegs),
-        VMSTATE_UINT16(pm1a.en, ICH9_LPCPmRegs),
-        VMSTATE_UINT16(pm1_cnt.cnt, ICH9_LPCPmRegs),
-        VMSTATE_TIMER(tmr.timer, ICH9_LPCPmRegs),
-        VMSTATE_INT64(tmr.overflow_time, ICH9_LPCPmRegs),
-        VMSTATE_GPE_ARRAY(gpe0.sts, ICH9_LPCPmRegs),
-        VMSTATE_GPE_ARRAY(gpe0.en, ICH9_LPCPmRegs),
+        VMSTATE_UINT16(acpi_regs.pm1.evt.sts, ICH9_LPCPmRegs),
+        VMSTATE_UINT16(acpi_regs.pm1.evt.en, ICH9_LPCPmRegs),
+        VMSTATE_UINT16(acpi_regs.pm1.cnt.cnt, ICH9_LPCPmRegs),
+        VMSTATE_TIMER(acpi_regs.tmr.timer, ICH9_LPCPmRegs),
+        VMSTATE_INT64(acpi_regs.tmr.overflow_time, ICH9_LPCPmRegs),
+        VMSTATE_GPE_ARRAY(acpi_regs.gpe.sts, ICH9_LPCPmRegs),
+        VMSTATE_GPE_ARRAY(acpi_regs.gpe.en, ICH9_LPCPmRegs),
         VMSTATE_UINT32(smi_en, ICH9_LPCPmRegs),
         VMSTATE_UINT32(smi_sts, ICH9_LPCPmRegs),
         VMSTATE_END_OF_LIST()

@@ -286,10 +287,10 @@ static void pm_reset(void *opaque)
     ICH9_LPCPmRegs *pm = opaque;
     ich9_pm_iospace_update(pm, 0);
 
-    acpi_pm1_evt_reset(&pm->pm1a);
-    acpi_pm1_cnt_reset(&pm->pm1_cnt);
-    acpi_pm_tmr_reset(&pm->tmr);
-    acpi_gpe_reset(&pm->gpe0);
+    acpi_pm1_evt_reset(&pm->acpi_regs);
+    acpi_pm1_cnt_reset(&pm->acpi_regs);
+    acpi_pm_tmr_reset(&pm->acpi_regs);
+    acpi_gpe_reset(&pm->acpi_regs);
 
     pm_update_sci(pm);
 }

@@ -297,17 +298,15 @@ static void pm_reset(void *opaque)
 static void pm_powerdown(void *opaque, int irq, int power_failing)
 {
     ICH9_LPCPmRegs *pm = opaque;
-    ACPIPM1EVT *pm1a = pm ? &pm->pm1a : NULL;
-    ACPIPMTimer *tmr = pm ? &pm->tmr : NULL;
 
-    acpi_pm1_evt_power_down(pm1a, tmr);
+    acpi_pm1_evt_power_down(&pm->acpi_regs);
 }
 
 void ich9_pm_init(ICH9_LPCPmRegs *pm, qemu_irq sci_irq, qemu_irq cmos_s3)
 {
-    acpi_pm_tmr_init(&pm->tmr, ich9_pm_update_sci_fn);
-    acpi_pm1_cnt_init(&pm->pm1_cnt, cmos_s3);
-    acpi_gpe_init(&pm->gpe0, ICH9_PMIO_GPE0_LEN);
+    acpi_pm_tmr_init(&pm->acpi_regs, ich9_pm_update_sci_fn);
+    acpi_pm1_cnt_init(&pm->acpi_regs);
+    acpi_gpe_init(&pm->acpi_regs, ICH9_PMIO_GPE0_LEN);
 
     pm->irq = sci_irq;
     qemu_register_reset(pm_reset, pm);

hw/acpi_ich9.h +1 -8

index f55c0e9..9ff4c42 100644
@@ -24,19 +24,12 @@
 #include "acpi.h"
 
 typedef struct ICH9_LPCPmRegs {
-    ACPIPM1EVT pm1a;
-
     /*
      * In ich9 spec says that pm1_cnt register is 32bit width and
      * that the upper 16bits are reserved and unused.
      * PM1a_CNT_BLK = 2 in FADT so it is defined as uint16_t.
      */
-    ACPIPM1CNT pm1_cnt;
-
-    ACPIPMTimer tmr;
-
-    ACPIGPE gpe0;
-
+    ACPIREGS acpi_regs;
     uint32_t smi_en;
     uint32_t smi_sts;
 

hw/pc.h +2 -0

index c78923c..125c1fd 100644
@@ -69,6 +69,8 @@ int pic_read_irq(DeviceState *d);
 int pic_get_output(DeviceState *d);
 void pic_info(Monitor *mon);
 void irq_info(Monitor *mon);
+void kvm_piix3_gsi_handler(void *opaque, int n, int level);
+void kvm_piix3_setup_irq_routing(bool pci_enabled);
 
 /* Global System Interrupts */
 

hw/pc_piix.c +2 -2

index 60c7166..57830ec 100644
@@ -53,7 +53,7 @@ static const int ide_iobase[MAX_IDE_BUS] = { 0x1f0, 0x170 };
 static const int ide_iobase2[MAX_IDE_BUS] = { 0x3f6, 0x376 };
 static const int ide_irq[MAX_IDE_BUS] = { 14, 15 };
 
-static void kvm_piix3_setup_irq_routing(bool pci_enabled)
+void kvm_piix3_setup_irq_routing(bool pci_enabled)
 {
 #ifdef CONFIG_KVM
     KVMState *s = kvm_state;

@@ -82,7 +82,7 @@ static void kvm_piix3_setup_irq_routing(bool pci_enabled)
 #endif /* CONFIG_KVM */
 }
 
-static void kvm_piix3_gsi_handler(void *opaque, int n, int level)
+void kvm_piix3_gsi_handler(void *opaque, int n, int level)
 {
     GSIState *s = opaque;
 

hw/pc_q35.c +124 -56

index 4f75d97..9d58519 100644
@@ -27,6 +27,7 @@
  *  Copyright (c) 2009, 2010
  *                     Isaku Yamahata <yamahata at valinux co jp>
  *                     VA Linux Systems Japan K.K.
+ *  Copyright (C) 2012 Jason Baron <[email protected]>
  *
  *  This is based on pc.c, but heavily modified.
  *

@@ -49,7 +50,6 @@
 #include "fdc.h"
 #include "pci.h"
 #include "pci_bridge.h"
-#include "pci_p2pbr.h"
 #include "ioh3420.h"
 #include "xio3130_upstream.h"
 #include "xio3130_downstream.h"

@@ -66,36 +66,16 @@
 #include "watchdog.h"
 #include "smbios.h"
 #include "ide.h"
-#include "usb-uhci.h"
+#include "mc146818rtc.h"
+#include "xen.h"
+#include "kvm.h"
 
 #include "q35.h"
+#include "exec-memory.h"
 
 /* ICH9 AHCI has 6 ports */
 #define MAX_SATA_PORTS     6
 
-#define I21154_REV            0x05
-#define I21154_PI             0x00
-
-static PCIBridge *i21154_init(PCIBus *bus, int devfn, const char *bus_name,
-                              bool multifunction)
-{
-    const PCIP2PBridgeInit init = {
-        .bus = bus,
-        .devfn = devfn,
-        .multifunction = multifunction,
-
-        .bus_name = bus_name,
-        .map_irq = pci_swizzle_map_irq_fn,
-    };
-    const PCIP2PBridgeProp prop = {
-        .vendor_id = PCI_VENDOR_ID_DEC,
-        .device_id = PCI_DEVICE_ID_DEC_21154,
-        .revision_id = I21154_REV,
-        .prog_interface = I21154_PI,
-    };
-    return pci_p2pbr_create_simple(&init, &prop);
-}
-
 static void pc_q35_bridge_init(PCIBus *host_bus, PCIBus *pci_bus)
 {
     uint8_t dev;

@@ -104,7 +84,6 @@ static void pc_q35_bridge_init(PCIBus *host_bus, PCIBus *pci_bus)
     uint8_t chassis = 0;
     uint16_t slot = 0;
     uint8_t upstream_port;
-    PCIESlot *s;
     uint8_t fn;
     PCIESlot *root_port;
     PCIBus *root_port_bus;

@@ -116,11 +95,10 @@ static void pc_q35_bridge_init(PCIBus *host_bus, PCIBus *pci_bus)
 #define Q35_P2P_BRDIGE_DEV_MAX          32
 #define Q35_P2P_BRDIGE_SUBBUS_BASE      (ICH9_D2P_SECONDARY_DEFAULT + 1)
     for (dev = Q35_P2P_BRDIGE_DEV_BASE; dev < Q35_P2P_BRDIGE_DEV_MAX; dev++) {
-        PCIBridge *br;
         sec_bus = Q35_P2P_BRDIGE_SUBBUS_BASE + dev - Q35_P2P_BRDIGE_DEV_BASE;
 
         snprintf(buf, sizeof(buf), "pci.%d", sec_bus);
-        br = i21154_init(pci_bus, PCI_DEVFN(dev, 0), buf, true);
+        i21154_init(pci_bus, PCI_DEVFN(dev, 0), buf, true);
     }
 
     /* PCIe root port b0:d1:f0 in GMCH.

@@ -128,8 +106,8 @@ static void pc_q35_bridge_init(PCIBus *host_bus, PCIBus *pci_bus)
      */
     sec_bus = 32;
     snprintf(buf, sizeof(buf), "pcie.%d", sec_bus);
-    s = ioh3420_init(host_bus, PCI_DEVFN(GMCH_PCIE_DEV, GMCH_PCIE_FUNC), true,
-                     buf, pci_swizzle_map_irq_fn, port, chassis, slot);
+    ioh3420_init(host_bus, PCI_DEVFN(GMCH_PCIE_DEV, GMCH_PCIE_FUNC), true,
+                 buf, pci_swizzle_map_irq_fn, port, chassis, slot);
 
 
     /* more slots. ICH9 doesn't have those, but many slots are wanted. */

@@ -153,8 +131,8 @@ static void pc_q35_bridge_init(PCIBus *host_bus, PCIBus *pci_bus)
         slot++;
 
         snprintf(buf, sizeof(buf), "pcie.%d", sec_bus);
-        s = ioh3420_init(host_bus, PCI_DEVFN(23, fn), true,
-                         buf, pci_swizzle_map_irq_fn, port, chassis, slot);
+        ioh3420_init(host_bus, PCI_DEVFN(23, fn), true,
+                     buf, pci_swizzle_map_irq_fn, port, chassis, slot);
     }
 
     /* PCIe root port b0:d24:f0 */

@@ -219,27 +197,38 @@ static void pc_q35_bridge_init(PCIBus *host_bus, PCIBus *pci_bus)
         slot++;
 
         snprintf(buf, sizeof(buf), "pcie.%d", sec_bus);
-        s = ioh3420_init(host_bus, PCI_DEVFN(ICH9_PCIE_DEV, fn), true,
-                         buf, pci_swizzle_map_irq_fn,
-                         port, chassis, slot);
+        ioh3420_init(host_bus, PCI_DEVFN(ICH9_PCIE_DEV, fn), true,
+                     buf, pci_swizzle_map_irq_fn,
+                     port, chassis, slot);
     }
 }
 
-static void pc_q35_init_early(qemu_irq *isa_irq, IsaIrqState *isa_irq_state,
+static void pc_q35_init_early(qemu_irq *gsi, GSIState *gsi_state,
                               DeviceState **gmch_host_p,
                               PCIBus **host_bus_p, PCIBus **pci_bus_p,
-                              PCIDevice **lpc_p)
+                              PCIDevice **lpc_p, ISABus **isa_bus,
+                              MemoryRegion *system_memory,
+                              MemoryRegion *pci_address_space,
+                              MemoryRegion *address_space_io,
+                              MemoryRegion *ram_memory,
+                              ram_addr_t below_4g_mem_size,
+                              ram_addr_t above_4g_mem_size)
 {
+    target_phys_addr_t pci_hole64_size;
     DeviceState *gmch_host;
     PCIBus *host_bus;
     PCIBus *pci_bus;
 
     PCIDevice *gmch_state;
     PCIDevice *lpc;
+    GMCH_PCIState *gmps;
+    ICH9_LPCState *ich9_lpc;
 
     /* create pci host bus */
-    host_bus = gmch_host_init(&gmch_host, isa_irq, isa_irq_state->ioapic);
+    host_bus = gmch_host_init(&gmch_host, gsi, gsi_state->ioapic_irq,
+                              pci_address_space, address_space_io);
     gmch_state = gmch_init(gmch_host, host_bus);
+    gmps = GMCH_PCI_DEVICE(gmch_state);
 
     /* create conventional pci bus: pcie2pci bridge */
     pci_bus = ich9_d2pbr_init(host_bus, PCI_DEVFN(ICH9_D2P_BRIDGE_DEV,

@@ -252,10 +241,53 @@ static void pc_q35_init_early(qemu_irq *isa_irq, IsaIrqState *isa_irq_state,
     /* create ISA bus */
     lpc = gmch_lpc_init(gmch_host, host_bus);
 
+    gmps->ram_memory = ram_memory;
+    gmps->pci_address_space = pci_address_space;
+    gmps->system_memory = system_memory;
+    /* pci */
+    memory_region_init_alias(&gmps->pci_hole, "pci-hole",
+                             gmps->pci_address_space,
+                             below_4g_mem_size,
+                             0x100000000ULL - below_4g_mem_size);
+    memory_region_add_subregion(gmps->system_memory, below_4g_mem_size,
+                                &gmps->pci_hole);
+    pci_hole64_size = (sizeof(target_phys_addr_t) == 4 ? 0 :
+                                            ((uint64_t)1 << 62));
+    memory_region_init_alias(&gmps->pci_hole_64bit, "pci-hole64",
+                             gmps->pci_address_space,
+                             0x100000000ULL + above_4g_mem_size,
+                             pci_hole64_size);
+    if (pci_hole64_size) {
+        memory_region_add_subregion(gmps->system_memory,
+                                    0x100000000ULL + above_4g_mem_size,
+                                    &gmps->pci_hole_64bit);
+    }
+
+    /* smram */
+    memory_region_init_alias(&gmps->smram_region, "smram-region",
+                             pci_address_space, 0xa0000, 0x20000);
+    memory_region_add_subregion_overlap(system_memory, 0xa0000,
+                                        &gmps->smram_region, 1);
+    memory_region_set_enabled(&gmps->smram_region, false);
+
     *gmch_host_p = gmch_host;
     *host_bus_p = host_bus;
     *pci_bus_p = pci_bus;
     *lpc_p = lpc;
+    ich9_lpc = ICH9_LPC_DEVICE(lpc);
+    *isa_bus = ich9_lpc->isa_bus;
+}
+
+
+/* set CMOS shutdown status register (index 0xF) as S3_resume(0xFE)
+ *    BIOS will read it and start S3 resume at POST Entry */
+static void pc_cmos_set_s3_resume(void *opaque, int irq, int level)
+{
+    ISADevice *s = opaque;
+
+    if (level) {
+        rtc_set_memory(s, 0xF, 0xFE);
+    }
 }
 
 static void pc_q35_init_late(BusState **idebus, ISADevice *rtc_state,

@@ -309,18 +341,22 @@ static void pc_q35_init(ram_addr_t ram_size,
     PCIBus *host_bus;
     PCIBus *pci_bus;
     PCIDevice *lpc;
-    qemu_irq *isa_irq;
-    IsaIrqState *isa_irq_state;
     BusState *idebus[MAX_SATA_PORTS];
     ISADevice *rtc_state;
+    ISADevice *floppy;
     MemoryRegion *pci_memory;
     MemoryRegion *rom_memory;
     MemoryRegion *ram_memory;
+    GSIState *gsi_state;
+    ISABus *isa_bus;
+    int pci_enabled = 1;
+    qemu_irq *cpu_irq;
+    qemu_irq *gsi;
+    qemu_irq *i8259;
+    int i;
 
     pc_cpus_init(cpu_model);
 
-    /* FIXME: add kvm clock ? */
-
     if (ram_size >= 0xe0000000) {
         above_4g_mem_size = ram_size - 0xe0000000;
         below_4g_mem_size = 0xe0000000;

@@ -330,37 +366,70 @@ static void pc_q35_init(ram_addr_t ram_size,
     }
 
     /* pci enabled */
-    pci_memory = g_new(MemoryRegion, 1);
-    memory_region_init(pci_memory, "pci", INT64_MAX);
-    rom_memory = pci_memory;
+    if (pci_enabled) {
+        pci_memory = g_new(MemoryRegion, 1);
+        memory_region_init(pci_memory, "pci", INT64_MAX);
+        rom_memory = pci_memory;
+    } else {
+        pci_memory = NULL;
+        rom_memory = get_system_memory();
+    }
 
     /* allocate ram and load rom/bios */
-    pc_memory_init(get_system_memory(), kernel_filename, kernel_cmdline,
-                   initrd_filename, below_4g_mem_size, above_4g_mem_size,
-                   rom_memory, &ram_memory);
+    if (!xen_enabled()) {
+        pc_memory_init(get_system_memory(), kernel_filename, kernel_cmdline,
+                       initrd_filename, below_4g_mem_size, above_4g_mem_size,
+                       rom_memory, &ram_memory);
+    }
 
     /* irq lines */
-    isa_irq = pc_isa_irq(&isa_irq_state);
-    ioapic_init(isa_irq_state);
+    gsi_state = g_malloc0(sizeof(*gsi_state));
+    if (kvm_irqchip_in_kernel()) {
+        kvm_piix3_setup_irq_routing(pci_enabled);
+        gsi = qemu_allocate_irqs(kvm_piix3_gsi_handler, gsi_state,
+                                 GSI_NUM_PINS);
+    } else {
+        gsi = qemu_allocate_irqs(gsi_handler, gsi_state, GSI_NUM_PINS);
+    }
+
+    pc_q35_init_early(gsi, gsi_state,
+                      &gmch_host, &host_bus, &pci_bus, &lpc, &isa_bus,
+                      get_system_memory(), pci_memory, get_system_io(),
+                      ram_memory, below_4g_mem_size, above_4g_mem_size);
+    isa_bus_irqs(isa_bus, gsi);
+
+    if (kvm_irqchip_in_kernel()) {
+        i8259 = kvm_i8259_init(isa_bus);
+    } else if (xen_enabled()) {
+        i8259 = xen_interrupt_controller_init();
+    } else {
+        cpu_irq = pc_allocate_cpu_irq();
+        i8259 = i8259_init(isa_bus, cpu_irq[0]);
+    }
+
+    for (i = 0; i < ISA_NUM_IRQS; i++) {
+        gsi_state->i8259_irq[i] = i8259[i];
+    }
+    if (pci_enabled) {
+        ioapic_init_gsi(gsi_state, NULL);
+    }
 
-    pc_q35_init_early(isa_irq, isa_irq_state,
-                      &gmch_host, &host_bus, &pci_bus, &lpc);
-    isa_bus_irqs(isa_irq);
-    pc_register_ferr_irq(isa_get_irq(13));
+    pc_register_ferr_irq(gsi[13]);
 
     /* init basic PC hardware */
-    pc_basic_device_init(isa_irq, &rtc_state, false);
+    pc_basic_device_init(isa_bus, gsi, &rtc_state, &floppy, false);
 
     pc_q35_init_late(idebus, rtc_state, gmch_host, host_bus, pci_bus, lpc);
 
     pc_cmos_init(below_4g_mem_size, above_4g_mem_size, boot_device,
-                 idebus[0], idebus[1], rtc_state);
+                 floppy, idebus[0], idebus[1], rtc_state);