[Qemu-devel] [PATCH 01/17] e1000: switch to symbolic names for pci registers

[Qemu-devel] [PATCH 01/17] e1000: switch to symbolic names for pci registers

[Michael S. Tsirkin]

No functional changes. I verified that the generated
object binary does not change.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/e1000.c |   11 +++++++----
 1 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/hw/e1000.c b/hw/e1000.c
index ad7a267..300b11d 100644
--- a/hw/e1000.c
+++ b/hw/e1000.c
@@ -1089,12 +1089,15 @@ static int pci_e1000_init(PCIDevice *pci_dev)
 
     pci_config_set_vendor_id(pci_conf, PCI_VENDOR_ID_INTEL);
     pci_config_set_device_id(pci_conf, E1000_DEVID);
-    *(uint16_t *)(pci_conf+0x06) = cpu_to_le16(0x0010);
-    pci_conf[0x08] = 0x03;
+    /* TODO: we have no capabilities, so why is this bit set? */
+    pci_set_word(pci_conf + PCI_STATUS, PCI_STATUS_CAP_LIST);
+    pci_conf[PCI_REVISION_ID] = 0x03;
     pci_config_set_class(pci_conf, PCI_CLASS_NETWORK_ETHERNET);
-    pci_conf[0x0c] = 0x10;
+    /* TODO: RST# value should be 0, PCI spec 6.2.4 */
+    pci_conf[PCI_CACHE_LINE_SIZE] = 0x10;
 
-    pci_conf[0x3d] = 1; // interrupt pin 0
+    /* TODO: RST# value should be 0 if programmable, PCI spec 6.2.4 */
+    pci_conf[PCI_INTERRUPT_PIN] = 1; // interrupt pin 0
 
     d->mmio_index = cpu_register_io_memory(e1000_mmio_read,
             e1000_mmio_write, d);
-- 
1.6.6.rc1.43.gf55cc

[Qemu-devel] [PATCH 02/17] ne2000: switch to symbolic names for pci registers

[Michael S. Tsirkin]

No functional changes. I verified that the generated binary
does not change.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/ne2000.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/hw/ne2000.c b/hw/ne2000.c
index d1416cf..78fe14f 100644
--- a/hw/ne2000.c
+++ b/hw/ne2000.c
@@ -724,7 +724,8 @@ static int pci_ne2000_init(PCIDevice *pci_dev)
     pci_config_set_device_id(pci_conf, PCI_DEVICE_ID_REALTEK_8029);
     pci_config_set_class(pci_conf, PCI_CLASS_NETWORK_ETHERNET);
     pci_conf[PCI_HEADER_TYPE] = PCI_HEADER_TYPE_NORMAL; // header_type
-    pci_conf[0x3d] = 1; // interrupt pin 0
+    /* TODO: RST# value should be 0. PCI spec 6.2.4 */
+    pci_conf[PCI_INTERRUPT_PIN] = 1; // interrupt pin 0
 
     pci_register_bar(&d->dev, 0, 0x100,
                            PCI_BASE_ADDRESS_SPACE_IO, ne2000_map);
-- 
1.6.6.rc1.43.gf55cc

[Qemu-devel] [PATCH 03/17] rtl: switch to symbolic names for pci registers

[Michael S. Tsirkin]

No functional changes. I verified that the generated binary
does not change.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/rtl8139.c |   16 ++++++++++------
 1 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/hw/rtl8139.c b/hw/rtl8139.c
index 9fd05a8..2b2910d 100644
--- a/hw/rtl8139.c
+++ b/hw/rtl8139.c
@@ -3320,16 +3320,20 @@ static int pci_rtl8139_init(PCIDevice *dev)
     pci_conf = s->dev.config;
     pci_config_set_vendor_id(pci_conf, PCI_VENDOR_ID_REALTEK);
     pci_config_set_device_id(pci_conf, PCI_DEVICE_ID_REALTEK_8139);
-    pci_conf[0x04] = 0x05; /* command = I/O space, Bus Master */
-    pci_conf[0x08] = RTL8139_PCI_REVID; /* PCI revision ID; >=0x20 is for 8139C+ */
+    /* TODO: value should be 0 at RST#. */
+    pci_conf[PCI_COMMAND] = PCI_COMMAND_IO | PCI_COMMAND_MASTER;
+    pci_conf[PCI_REVISION_ID] = RTL8139_PCI_REVID; /* >=0x20 is for 8139C+ */
     pci_config_set_class(pci_conf, PCI_CLASS_NETWORK_ETHERNET);
-    pci_conf[PCI_HEADER_TYPE] = PCI_HEADER_TYPE_NORMAL; /* header_type */
-    pci_conf[0x3d] = 1;    /* interrupt pin 0 */
-    pci_conf[0x34] = 0xdc;
+    pci_conf[PCI_HEADER_TYPE] = PCI_HEADER_TYPE_NORMAL;
+    /* TODO: value should be 0 at RST# */
+    pci_conf[PCI_INTERRUPT_PIN] = 1;    /* interrupt pin 0 */
+    /* TODO: start of capability list, but no capability
+     * list bit in status register, and offset 0xdc seems unused. */
+    pci_conf[PCI_CAPABILITY_LIST] = 0xdc;
 
     /* I/O handler for memory-mapped I/O */
     s->rtl8139_mmio_io_addr =
-    cpu_register_io_memory(rtl8139_mmio_read, rtl8139_mmio_write, s);
+        cpu_register_io_memory(rtl8139_mmio_read, rtl8139_mmio_write, s);
 
     pci_register_bar(&s->dev, 0, 0x100,
                            PCI_BASE_ADDRESS_SPACE_IO,  rtl8139_ioport_map);
-- 
1.6.6.rc1.43.gf55cc

[Qemu-devel] [PATCH 04/17] pcnet: switch to symbolic names for pci registers

[Michael S. Tsirkin]

No functional changes. I verified that the generated binary
does not change.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/pci.h   |    1 +
 hw/pcnet.c |   26 +++++++++++++++++---------
 2 files changed, 18 insertions(+), 9 deletions(-)

diff --git a/hw/pci.h b/hw/pci.h
index d279e3f..dd61fa1 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -165,6 +165,7 @@ typedef struct PCIIORegion {
 #define PCI_STATUS_66MHZ	0x020
 #define PCI_STATUS_RESERVED2	0x040
 #define PCI_STATUS_FAST_BACK	0x080
+#define PCI_STATUS_DEVSEL_MEDIUM 0x200
 #define PCI_STATUS_DEVSEL	0x600
 
 #define PCI_STATUS_RESERVED_MASK_LO (PCI_STATUS_RESERVED1 | \
diff --git a/hw/pcnet.c b/hw/pcnet.c
index 138fbc6..91d106d 100644
--- a/hw/pcnet.c
+++ b/hw/pcnet.c
@@ -1981,24 +1981,32 @@ static int pci_pcnet_init(PCIDevice *pci_dev)
 
     pci_config_set_vendor_id(pci_conf, PCI_VENDOR_ID_AMD);
     pci_config_set_device_id(pci_conf, PCI_DEVICE_ID_AMD_LANCE);
-    *(uint16_t *)&pci_conf[0x04] = cpu_to_le16(0x0007);
-    *(uint16_t *)&pci_conf[0x06] = cpu_to_le16(0x0280);
-    pci_conf[0x08] = 0x10;
-    pci_conf[0x09] = 0x00;
+    /* TODO: value should be 0 at RST# */
+    pci_set_word(pci_conf + PCI_COMMAND,
+                 PCI_COMMAND_IO | PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER);
+    pci_set_word(pci_conf + PCI_STATUS,
+                 PCI_STATUS_FAST_BACK | PCI_STATUS_DEVSEL_MEDIUM);
+    pci_conf[PCI_REVISION_ID] = 0x10;
+    /* TODO: 0 is the default anyway, no need to set it. */
+    pci_conf[PCI_CLASS_PROG] = 0x00;
     pci_config_set_class(pci_conf, PCI_CLASS_NETWORK_ETHERNET);
     pci_conf[PCI_HEADER_TYPE] = PCI_HEADER_TYPE_NORMAL; // header_type
 
-    *(uint32_t *)&pci_conf[0x10] = cpu_to_le32(0x00000001);
-    *(uint32_t *)&pci_conf[0x14] = cpu_to_le32(0x00000000);
+    /* TODO: not necessary, is set when BAR is registered. */
+    pci_set_long(pci_conf + PCI_BASE_ADDRESS_0, PCI_BASE_ADDRESS_SPACE_IO);
+    pci_set_long(pci_conf + PCI_BASE_ADDRESS_0 + 4,
+                 PCI_BASE_ADDRESS_SPACE_MEMORY);
 
-    pci_conf[0x3d] = 1; // interrupt pin 0
-    pci_conf[0x3e] = 0x06;
-    pci_conf[0x3f] = 0xff;
+    /* TODO: value must be 0 at RST# */
+    pci_conf[PCI_INTERRUPT_PIN] = 1; // interrupt pin 0
+    pci_conf[PCI_MIN_GNT] = 0x06;
+    pci_conf[PCI_MAX_LAT] = 0xff;
 
     /* Handler for memory-mapped I/O */
     s->mmio_index =
       cpu_register_io_memory(pcnet_mmio_read, pcnet_mmio_write, &d->state);
 
+    /* TODO: use pci_dev, avoid cast below. */
     pci_register_bar((PCIDevice *)d, 0, PCNET_IOPORT_SIZE,
                            PCI_BASE_ADDRESS_SPACE_IO, pcnet_ioport_map);
 
-- 
1.6.6.rc1.43.gf55cc

[Qemu-devel] [PATCH 05/17] pci: add more status bits

[Michael S. Tsirkin]

will be used by eepro100.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/pci.h |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/hw/pci.h b/hw/pci.h
index dd61fa1..738506c 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -167,6 +167,8 @@ typedef struct PCIIORegion {
 #define PCI_STATUS_FAST_BACK	0x080
 #define PCI_STATUS_DEVSEL_MEDIUM 0x200
 #define PCI_STATUS_DEVSEL	0x600
+#define  PCI_STATUS_SIG_TARGET_ABORT	0x800 /* Set on target abort */
+#define  PCI_STATUS_REC_MASTER_ABORT	0x2000 /* Set on master abort */
 
 #define PCI_STATUS_RESERVED_MASK_LO (PCI_STATUS_RESERVED1 | \
                 PCI_STATUS_INT_STATUS | PCI_STATUS_CAPABILITIES | \
-- 
1.6.6.rc1.43.gf55cc

[Qemu-devel] [PATCH 06/17] eepro100: symbolic names for pci registers

[Michael S. Tsirkin]

No functional changes. I verified that the generated binary
does not change in meaningful ways. Survived light usage
with linux guest.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/eepro100.c |   49 ++++++++++++++++++++++++++++++++-----------------
 1 files changed, 32 insertions(+), 17 deletions(-)

diff --git a/hw/eepro100.c b/hw/eepro100.c
index 2a9e3b5..82e3766 100644
--- a/hw/eepro100.c
+++ b/hw/eepro100.c
@@ -412,19 +412,24 @@ static void pci_reset(EEPRO100State * s)
     pci_config_set_vendor_id(pci_conf, PCI_VENDOR_ID_INTEL);
     /* PCI Device ID depends on device and is set below. */
     /* PCI Command */
+    /* TODO: this is the default, do not override. */
     PCI_CONFIG_16(PCI_COMMAND, 0x0000);
     /* PCI Status */
-    PCI_CONFIG_16(PCI_STATUS, 0x2800);
+    /* TODO: this seems to make no sense. */
+    /* TODO: Value at RST# should be 0. */
+    PCI_CONFIG_16(PCI_STATUS,
+                  PCI_STATUS_REC_MASTER_ABORT | PCI_STATUS_SIG_TARGET_ABORT);
     /* PCI Revision ID */
     PCI_CONFIG_8(PCI_REVISION_ID, 0x08);
+    /* TODO: this is the default, do not override. */
     /* PCI Class Code */
-    PCI_CONFIG_8(0x09, 0x00);
+    PCI_CONFIG_8(PCI_CLASS_PROG, 0x00);
     pci_config_set_class(pci_conf, PCI_CLASS_NETWORK_ETHERNET);
     /* PCI Cache Line Size */
     /* check cache line size!!! */
     //~ PCI_CONFIG_8(0x0c, 0x00);
     /* PCI Latency Timer */
-    PCI_CONFIG_8(0x0d, 0x20);   // latency timer = 32 clocks
+    PCI_CONFIG_8(PCI_LATENCY_TIMER, 0x20);   // latency timer = 32 clocks
     /* PCI Header Type */
     /* BIST (built-in self test) */
 #if defined(TARGET_I386)
@@ -446,16 +451,20 @@ static void pci_reset(EEPRO100State * s)
 #endif
 #endif
     /* Expansion ROM Base Address (depends on boot disable!!!) */
-    PCI_CONFIG_32(0x30, 0x00000000);
+    /* TODO: not needed, set when BAR is registered */
+    PCI_CONFIG_32(PCI_ROM_ADDRESS, PCI_BASE_ADDRESS_SPACE_MEMORY);
     /* Capability Pointer */
-    PCI_CONFIG_8(0x34, 0xdc);
+    /* TODO: revisions with power_management 1 use this but
+     * do not set new capability list bit in status register. */
+    PCI_CONFIG_8(PCI_CAPABILITY_LIST, 0xdc);
     /* Interrupt Line */
     /* Interrupt Pin */
-    PCI_CONFIG_8(0x3d, 1);      // interrupt pin 0
+    /* TODO: RST# value should be 0 */
+    PCI_CONFIG_8(PCI_INTERRUPT_PIN, 1);      // interrupt pin 0
     /* Minimum Grant */
-    PCI_CONFIG_8(0x3e, 0x08);
+    PCI_CONFIG_8(PCI_MIN_GNT, 0x08);
     /* Maximum Latency */
-    PCI_CONFIG_8(0x3f, 0x18);
+    PCI_CONFIG_8(PCI_MAX_LAT, 0x18);
 
     switch (device) {
     case i82550:
@@ -479,52 +488,57 @@ static void pci_reset(EEPRO100State * s)
     case i82557A:
         pci_config_set_device_id(pci_conf, PCI_DEVICE_ID_INTEL_82557);
         PCI_CONFIG_8(PCI_REVISION_ID, 0x01);
-        PCI_CONFIG_8(0x34, 0x00);
+        PCI_CONFIG_8(PCI_CAPABILITY_LIST, 0x00);
         power_management = 0;
         break;
     case i82557B:
         pci_config_set_device_id(pci_conf, PCI_DEVICE_ID_INTEL_82557);
         PCI_CONFIG_8(PCI_REVISION_ID, 0x02);
-        PCI_CONFIG_8(0x34, 0x00);
+        PCI_CONFIG_8(PCI_CAPABILITY_LIST, 0x00);
         power_management = 0;
         break;
     case i82557C:
         pci_config_set_device_id(pci_conf, PCI_DEVICE_ID_INTEL_82557);
         PCI_CONFIG_8(PCI_REVISION_ID, 0x03);
-        PCI_CONFIG_8(0x34, 0x00);
+        PCI_CONFIG_8(PCI_CAPABILITY_LIST, 0x00);
         power_management = 0;
         break;
     case i82558A:
         pci_config_set_device_id(pci_conf, PCI_DEVICE_ID_INTEL_82557);
-        PCI_CONFIG_16(PCI_STATUS, 0x0290);
+        PCI_CONFIG_16(PCI_STATUS, PCI_STATUS_DEVSEL_MEDIUM |
+                                  PCI_STATUS_FAST_BACK | PCI_STATUS_CAP_LIST);
         PCI_CONFIG_8(PCI_REVISION_ID, 0x04);
         s->stats_size = 76;
         s->has_extended_tcb_support = 1;
         break;
     case i82558B:
         pci_config_set_device_id(pci_conf, PCI_DEVICE_ID_INTEL_82557);
-        PCI_CONFIG_16(PCI_STATUS, 0x0290);
+        PCI_CONFIG_16(PCI_STATUS, PCI_STATUS_DEVSEL_MEDIUM |
+                                  PCI_STATUS_FAST_BACK | PCI_STATUS_CAP_LIST);
         PCI_CONFIG_8(PCI_REVISION_ID, 0x05);
         s->stats_size = 76;
         s->has_extended_tcb_support = 1;
         break;
     case i82559A:
         pci_config_set_device_id(pci_conf, PCI_DEVICE_ID_INTEL_82557);
-        PCI_CONFIG_16(PCI_STATUS, 0x0290);
+        PCI_CONFIG_16(PCI_STATUS, PCI_STATUS_DEVSEL_MEDIUM |
+                                  PCI_STATUS_FAST_BACK | PCI_STATUS_CAP_LIST);
         PCI_CONFIG_8(PCI_REVISION_ID, 0x06);
         s->stats_size = 80;
         s->has_extended_tcb_support = 1;
         break;
     case i82559B:
         pci_config_set_device_id(pci_conf, PCI_DEVICE_ID_INTEL_82557);
-        PCI_CONFIG_16(PCI_STATUS, 0x0290);
+        PCI_CONFIG_16(PCI_STATUS, PCI_STATUS_DEVSEL_MEDIUM |
+                                  PCI_STATUS_FAST_BACK | PCI_STATUS_CAP_LIST);
         PCI_CONFIG_8(PCI_REVISION_ID, 0x07);
         s->stats_size = 80;
         s->has_extended_tcb_support = 1;
         break;
     case i82559C:
         pci_config_set_device_id(pci_conf, PCI_DEVICE_ID_INTEL_82557);
-        PCI_CONFIG_16(PCI_STATUS, 0x0290);
+        PCI_CONFIG_16(PCI_STATUS, PCI_STATUS_DEVSEL_MEDIUM |
+                                  PCI_STATUS_FAST_BACK | PCI_STATUS_CAP_LIST);
         PCI_CONFIG_8(PCI_REVISION_ID, 0x08);
         // TODO: Windows wants revision id 0x0c.
         PCI_CONFIG_8(PCI_REVISION_ID, 0x0c);
@@ -537,7 +551,8 @@ static void pci_reset(EEPRO100State * s)
         break;
     case i82559ER:
         pci_config_set_device_id(pci_conf, PCI_DEVICE_ID_INTEL_82551IT);
-        PCI_CONFIG_16(PCI_STATUS, 0x0290);
+        PCI_CONFIG_16(PCI_STATUS, PCI_STATUS_DEVSEL_MEDIUM |
+                                  PCI_STATUS_FAST_BACK | PCI_STATUS_CAP_LIST);
         PCI_CONFIG_8(PCI_REVISION_ID, 0x09);
         s->stats_size = 80;
         s->has_extended_tcb_support = 1;
-- 
1.6.6.rc1.43.gf55cc

[Qemu-devel] [PATCH 07/17] piix: symbolic constants

[Michael S. Tsirkin]

No functional changes. I verified that the generated binary
does not change.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/ide/piix.c |   13 ++++++++-----
 1 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index ec93f29..b1f66a5 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -106,10 +106,13 @@ static void piix3_reset(void *opaque)
         ide_dma_reset(&d->bmdma[i]);
     }
 
-    pci_conf[0x04] = 0x00;
-    pci_conf[0x05] = 0x00;
-    pci_conf[0x06] = 0x80; /* FBC */
-    pci_conf[0x07] = 0x02; // PCI_status_devsel_medium
+    /* TODO: this is the default. do not override. */
+    pci_conf[PCI_COMMAND] = 0x00;
+    /* TODO: this is the default. do not override. */
+    pci_conf[PCI_COMMAND + 1] = 0x00;
+    /* TODO: use pci_set_word */
+    pci_conf[PCI_STATUS] = PCI_STATUS_FAST_BACK;
+    pci_conf[PCI_STATUS + 1] = PCI_STATUS_DEVSEL_MEDIUM >> 8;
     pci_conf[0x20] = 0x01; /* BMIBA: 20-23h */
 }
 
@@ -117,7 +120,7 @@ static int pci_piix_ide_initfn(PCIIDEState *d)
 {
     uint8_t *pci_conf = d->dev.config;
 
-    pci_conf[0x09] = 0x80; // legacy ATA mode
+    pci_conf[PCI_CLASS_PROG] = 0x80; // legacy ATA mode
     pci_config_set_class(pci_conf, PCI_CLASS_STORAGE_IDE);
     pci_conf[PCI_HEADER_TYPE] = PCI_HEADER_TYPE_NORMAL; // header_type
 
-- 
1.6.6.rc1.43.gf55cc

[Qemu-devel] [PATCH 08/17] cmd646: symbolic names for pci registers

[Michael S. Tsirkin]

No functional changes. I verified that the generated binary
does not change.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/ide/cmd646.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
index 3b7c606..086ff62 100644
--- a/hw/ide/cmd646.c
+++ b/hw/ide/cmd646.c
@@ -209,8 +209,8 @@ static int pci_cmd646_ide_initfn(PCIDevice *dev)
     pci_config_set_vendor_id(pci_conf, PCI_VENDOR_ID_CMD);
     pci_config_set_device_id(pci_conf, PCI_DEVICE_ID_CMD_646);
 
-    pci_conf[0x08] = 0x07; // IDE controller revision
-    pci_conf[0x09] = 0x8f;
+    pci_conf[PCI_REVISION_ID] = 0x07; // IDE controller revision
+    pci_conf[PCI_CLASS_PROG] = 0x8f;
 
     pci_config_set_class(pci_conf, PCI_CLASS_STORAGE_IDE);
     pci_conf[PCI_HEADER_TYPE] = PCI_HEADER_TYPE_NORMAL; // header_type
@@ -227,7 +227,8 @@ static int pci_cmd646_ide_initfn(PCIDevice *dev)
     pci_register_bar(dev, 3, 0x4, PCI_BASE_ADDRESS_SPACE_IO, ide_map);
     pci_register_bar(dev, 4, 0x10, PCI_BASE_ADDRESS_SPACE_IO, bmdma_map);
 
-    pci_conf[0x3d] = 0x01; // interrupt on pin 1
+    /* TODO: RST# value should be 0 */
+    pci_conf[PCI_INTERRUPT_PIN] = 0x01; // interrupt on pin 1
 
     irq = qemu_allocate_irqs(cmd646_set_irq, d, 2);
     ide_bus_new(&d->bus[0], &d->dev.qdev);
-- 
1.6.6.rc1.43.gf55cc

[Qemu-devel] [PATCH 09/17] vmware_vga: symbolic names for pci registers

[Michael S. Tsirkin]

No functional changes. I verified that the generated binary
does not change.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/vmware_vga.c |   20 +++++++++++---------
 1 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/hw/vmware_vga.c b/hw/vmware_vga.c
index 240731a..d63306c 100644
--- a/hw/vmware_vga.c
+++ b/hw/vmware_vga.c
@@ -1171,16 +1171,18 @@ static int pci_vmsvga_initfn(PCIDevice *dev)
 
     pci_config_set_vendor_id(s->card.config, PCI_VENDOR_ID_VMWARE);
     pci_config_set_device_id(s->card.config, SVGA_PCI_DEVICE_ID);
-    s->card.config[PCI_COMMAND]		= 0x07;		/* I/O + Memory */
+    s->card.config[PCI_COMMAND]	= PCI_COMMAND_IO |
+                                  PCI_COMMAND_MEMORY |
+                                  PCI_COMMAND_MASTER; /* I/O + Memory */
     pci_config_set_class(s->card.config, PCI_CLASS_DISPLAY_VGA);
-    s->card.config[0x0c]		= 0x08;		/* Cache line size */
-    s->card.config[0x0d]		= 0x40;		/* Latency timer */
-    s->card.config[PCI_HEADER_TYPE]	= PCI_HEADER_TYPE_NORMAL;
-    s->card.config[0x2c]		= PCI_VENDOR_ID_VMWARE & 0xff;
-    s->card.config[0x2d]		= PCI_VENDOR_ID_VMWARE >> 8;
-    s->card.config[0x2e]		= SVGA_PCI_DEVICE_ID & 0xff;
-    s->card.config[0x2f]		= SVGA_PCI_DEVICE_ID >> 8;
-    s->card.config[0x3c]		= 0xff;		/* End */
+    s->card.config[PCI_CACHE_LINE_SIZE]	= 0x08;		/* Cache line size */
+    s->card.config[PCI_LATENCY_TIMER] = 0x40;		/* Latency timer */
+    s->card.config[PCI_HEADER_TYPE] = PCI_HEADER_TYPE_NORMAL;
+    s->card.config[PCI_SUBSYSTEM_VENDOR_ID] = PCI_VENDOR_ID_VMWARE & 0xff;
+    s->card.config[PCI_SUBSYSTEM_VENDOR_ID + 1]	= PCI_VENDOR_ID_VMWARE >> 8;
+    s->card.config[PCI_SUBSYSTEM_ID] = SVGA_PCI_DEVICE_ID & 0xff;
+    s->card.config[PCI_SUBSYSTEM_ID + 1] = SVGA_PCI_DEVICE_ID >> 8;
+    s->card.config[PCI_INTERRUPT_LINE] = 0xff;		/* End */
 
     pci_register_bar(&s->card, 0, 0x10,
                     PCI_BASE_ADDRESS_SPACE_IO, pci_vmsvga_map_ioport);
-- 
1.6.6.rc1.43.gf55cc

[Qemu-devel] [PATCH 10/17] lsi: symbolic names for pci registers

[Michael S. Tsirkin]

No functional changes. I verified that the generated binary
does not change.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/lsi53c895a.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/hw/lsi53c895a.c b/hw/lsi53c895a.c
index 014c85d..463a889 100644
--- a/hw/lsi53c895a.c
+++ b/hw/lsi53c895a.c
@@ -2120,18 +2120,20 @@ static int lsi_scsi_init(PCIDevice *dev)
     /* PCI base class code */
     pci_config_set_class(pci_conf, PCI_CLASS_STORAGE_SCSI);
     /* PCI subsystem ID */
-    pci_conf[0x2e] = 0x00;
-    pci_conf[0x2f] = 0x10;
+    pci_conf[PCI_SUBSYSTEM_ID] = 0x00;
+    pci_conf[PCI_SUBSYSTEM_ID + 1] = 0x10;
     /* PCI latency timer = 255 */
-    pci_conf[0x0d] = 0xff;
+    pci_conf[PCI_LATENCY_TIMER] = 0xff;
+    /* TODO: RST# value should be 0 */
     /* Interrupt pin 1 */
-    pci_conf[0x3d] = 0x01;
+    pci_conf[PCI_INTERRUPT_PIN] = 0x01;
 
     s->mmio_io_addr = cpu_register_io_memory(lsi_mmio_readfn,
                                              lsi_mmio_writefn, s);
     s->ram_io_addr = cpu_register_io_memory(lsi_ram_readfn,
                                             lsi_ram_writefn, s);
 
+    /* TODO: use dev and get rid of cast below */
     pci_register_bar((struct PCIDevice *)s, 0, 256,
                            PCI_BASE_ADDRESS_SPACE_IO, lsi_io_mapfunc);
     pci_register_bar((struct PCIDevice *)s, 1, 0x400,
-- 
1.6.6.rc1.43.gf55cc

[Qemu-devel] [PATCH 11/17] pci: add another devsel macro

[Michael S. Tsirkin]

will be used by ensoniq emulation

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/pci.h |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/hw/pci.h b/hw/pci.h
index 738506c..4015d45 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -166,6 +166,7 @@ typedef struct PCIIORegion {
 #define PCI_STATUS_RESERVED2	0x040
 #define PCI_STATUS_FAST_BACK	0x080
 #define PCI_STATUS_DEVSEL_MEDIUM 0x200
+#define PCI_STATUS_DEVSEL_SLOW	0x400
 #define PCI_STATUS_DEVSEL	0x600
 #define  PCI_STATUS_SIG_TARGET_ABORT	0x800 /* Set on target abort */
 #define  PCI_STATUS_REC_MASTER_ABORT	0x2000 /* Set on master abort */
-- 
1.6.6.rc1.43.gf55cc

[Qemu-devel] [PATCH 12/17] es1370: symbolic names for pci registers

[Michael S. Tsirkin]

No functional changes. I verified that the generated binary
does not change.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/es1370.c |   29 +++++++++++++++--------------
 1 files changed, 15 insertions(+), 14 deletions(-)

diff --git a/hw/es1370.c b/hw/es1370.c
index c358253..40cb48c 100644
--- a/hw/es1370.c
+++ b/hw/es1370.c
@@ -1000,27 +1000,28 @@ static int es1370_initfn (PCIDevice *dev)
 
     pci_config_set_vendor_id (c, PCI_VENDOR_ID_ENSONIQ);
     pci_config_set_device_id (c, PCI_DEVICE_ID_ENSONIQ_ES1370);
-    c[0x07] = 2 << 1;
+    c[PCI_STATUS + 1] = PCI_STATUS_DEVSEL_SLOW >> 8;
     pci_config_set_class (c, PCI_CLASS_MULTIMEDIA_AUDIO);
 
 #if 1
-    c[0x2c] = 0x42;
-    c[0x2d] = 0x49;
-    c[0x2e] = 0x4c;
-    c[0x2f] = 0x4c;
+    c[PCI_SUBSYSTEM_VENDOR_ID] = 0x42;
+    c[PCI_SUBSYSTEM_VENDOR_ID + 1] = 0x49;
+    c[PCI_SUBSYSTEM_ID] = 0x4c;
+    c[PCI_SUBSYSTEM_ID + 1] = 0x4c;
 #else
-    c[0x2c] = 0x74;
-    c[0x2d] = 0x12;
-    c[0x2e] = 0x71;
-    c[0x2f] = 0x13;
-    c[0x34] = 0xdc;
-    c[0x3c] = 10;
+    c[PCI_SUBSYSTEM_VENDOR_ID] = 0x74;
+    c[PCI_SUBSYSTEM_VENDOR_ID + 1] = 0x12;
+    c[PCI_SUBSYSTEM_ID] = 0x71;
+    c[PCI_SUBSYSTEM_ID + 1] = 0x13;
+    c[PCI_CAPABILITY_LIST] = 0xdc;
+    c[PCI_INTERRUPT_LINE] = 10;
     c[0xdc] = 0x00;
 #endif
 
-    c[0x3d] = 1;
-    c[0x3e] = 0x0c;
-    c[0x3f] = 0x80;
+    /* TODO: RST# value should be 0. */
+    c[PCI_INTERRUPT_PIN] = 1;
+    c[PCI_MIN_GNT] = 0x0c;
+    c[PCI_MAX_LAT] = 0x80;
 
     pci_register_bar (&s->dev, 0, 256, PCI_BASE_ADDRESS_SPACE_IO, es1370_map);
     qemu_register_reset (es1370_on_reset, s);
-- 
1.6.6.rc1.43.gf55cc

[Qemu-devel] [PATCH 13/17] wdt_i6300esb: symbolic names for pci registers

[Michael S. Tsirkin]

No functional changes. I verified that the generated binary
does not change.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/wdt_i6300esb.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/hw/wdt_i6300esb.c b/hw/wdt_i6300esb.c
index 805b302..be0e89e 100644
--- a/hw/wdt_i6300esb.c
+++ b/hw/wdt_i6300esb.c
@@ -411,7 +411,7 @@ static int i6300esb_init(PCIDevice *dev)
     pci_config_set_vendor_id(pci_conf, PCI_VENDOR_ID_INTEL);
     pci_config_set_device_id(pci_conf, PCI_DEVICE_ID_INTEL_ESB_9);
     pci_config_set_class(pci_conf, PCI_CLASS_SYSTEM_OTHER);
-    pci_conf[0x0e] = 0x00;
+    pci_conf[PCI_HEADER_TYPE] = 0x00;
 
     pci_register_bar(&d->dev, 0, 0x10,
                             PCI_BASE_ADDRESS_SPACE_MEMORY, i6300esb_map);
-- 
1.6.6.rc1.43.gf55cc

[Qemu-devel] [PATCH 14/17] ac97: symbolic names for pci registers

[Michael S. Tsirkin]

No functional changes. I verified that the generated binary
does not change.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/ac97.c |   57 +++++++++++++++++++++++++++++++--------------------------
 1 files changed, 31 insertions(+), 26 deletions(-)

diff --git a/hw/ac97.c b/hw/ac97.c
index 62e349a..4319bc8 100644
--- a/hw/ac97.c
+++ b/hw/ac97.c
@@ -1284,37 +1284,42 @@ static int ac97_initfn (PCIDevice *dev)
     pci_config_set_vendor_id (c, PCI_VENDOR_ID_INTEL); /* ro */
     pci_config_set_device_id (c, PCI_DEVICE_ID_INTEL_82801AA_5); /* ro */
 
-    c[0x04] = 0x00;      /* pcicmd pci command rw, ro */
-    c[0x05] = 0x00;
+    /* TODO: no need to override */
+    c[PCI_COMMAND] = 0x00;      /* pcicmd pci command rw, ro */
+    c[PCI_COMMAND + 1] = 0x00;
 
-    c[0x06] = 0x80;      /* pcists pci status rwc, ro */
-    c[0x07] = 0x02;
+    /* TODO: */
+    c[PCI_STATUS] = PCI_STATUS_FAST_BACK;      /* pcists pci status rwc, ro */
+    c[PCI_STATUS + 1] = PCI_STATUS_DEVSEL_MEDIUM >> 8;
 
-    c[0x08] = 0x01;      /* rid revision ro */
-    c[0x09] = 0x00;      /* pi programming interface ro */
+    c[PCI_REVISION_ID] = 0x01;      /* rid revision ro */
+    c[PCI_CLASS_PROG] = 0x00;      /* pi programming interface ro */
     pci_config_set_class (c, PCI_CLASS_MULTIMEDIA_AUDIO); /* ro */
     c[PCI_HEADER_TYPE] = PCI_HEADER_TYPE_NORMAL; /* headtyp header type ro */
 
-    c[0x10] = 0x01;      /* nabmar native audio mixer base
-                            address rw */
-    c[0x11] = 0x00;
-    c[0x12] = 0x00;
-    c[0x13] = 0x00;
-
-    c[0x14] = 0x01;      /* nabmbar native audio bus mastering
-                            base address rw */
-    c[0x15] = 0x00;
-    c[0x16] = 0x00;
-    c[0x17] = 0x00;
-
-    c[0x2c] = 0x86;      /* svid subsystem vendor id rwo */
-    c[0x2d] = 0x80;
-
-    c[0x2e] = 0x00;      /* sid subsystem id rwo */
-    c[0x2f] = 0x00;
-
-    c[0x3c] = 0x00;      /* intr_ln interrupt line rw */
-    c[0x3d] = 0x01;      /* intr_pn interrupt pin ro */
+    /* TODO set when bar is registered. no need to override. */
+    /* nabmar native audio mixer base address rw */
+    c[PCI_BASE_ADDRESS_0] = PCI_BASE_ADDRESS_SPACE_IO;
+    c[PCI_BASE_ADDRESS_0 + 1] = 0x00;
+    c[PCI_BASE_ADDRESS_0 + 2] = 0x00;
+    c[PCI_BASE_ADDRESS_0 + 3] = 0x00;
+
+    /* TODO set when bar is registered. no need to override. */
+      /* nabmbar native audio bus mastering base address rw */
+    c[PCI_BASE_ADDRESS_0 + 4] = PCI_BASE_ADDRESS_SPACE_IO;
+    c[PCI_BASE_ADDRESS_0 + 5] = 0x00;
+    c[PCI_BASE_ADDRESS_0 + 6] = 0x00;
+    c[PCI_BASE_ADDRESS_0 + 7] = 0x00;
+
+    c[PCI_SUBSYSTEM_VENDOR_ID] = 0x86;      /* svid subsystem vendor id rwo */
+    c[PCI_SUBSYSTEM_VENDOR_ID + 1] = 0x80;
+
+    c[PCI_SUBSYSTEM_ID] = 0x00;      /* sid subsystem id rwo */
+    c[PCI_SUBSYSTEM_ID + 1] = 0x00;
+
+    c[PCI_INTERRUPT_LINE] = 0x00;      /* intr_ln interrupt line rw */
+    /* TODO: RST# value should be 0. */
+    c[PCI_INTERRUPT_PIN] = 0x01;      /* intr_pn interrupt pin ro */
 
     pci_register_bar (&s->dev, 0, 256 * 4, PCI_BASE_ADDRESS_SPACE_IO,
                       ac97_map);
-- 
1.6.6.rc1.43.gf55cc

[Qemu-devel] [PATCH 15/17] usb-uhci: symbolic names for pci registers

[Michael S. Tsirkin]

No functional changes. I verified that the generated binary
does not change.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/usb-uhci.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/hw/usb-uhci.c b/hw/usb-uhci.c
index ba26a4e..70a98c6 100644
--- a/hw/usb-uhci.c
+++ b/hw/usb-uhci.c
@@ -1065,11 +1065,12 @@ static int usb_uhci_common_initfn(UHCIState *s)
     uint8_t *pci_conf = s->dev.config;
     int i;
 
-    pci_conf[0x08] = 0x01; // revision number
-    pci_conf[0x09] = 0x00;
+    pci_conf[PCI_REVISION_ID] = 0x01; // revision number
+    pci_conf[PCI_CLASS_PROG] = 0x00;
     pci_config_set_class(pci_conf, PCI_CLASS_SERIAL_USB);
     pci_conf[PCI_HEADER_TYPE] = PCI_HEADER_TYPE_NORMAL; // header_type
-    pci_conf[0x3d] = 4; // interrupt pin 3
+    /* TODO: reset value should be 0. */
+    pci_conf[PCI_INTERRUPT_PIN] = 4; // interrupt pin 3
     pci_conf[0x60] = 0x10; // release number
 
     usb_bus_new(&s->bus, &s->dev.qdev);
-- 
1.6.6.rc1.43.gf55cc

[Qemu-devel] [PATCH 16/17] usb-uhci: symbolic names for pci registers

[Michael S. Tsirkin]

No functional changes. I verified that the generated binary
does not change.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/usb-ohci.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/usb-ohci.c b/hw/usb-ohci.c
index 7ab3a98..deab7f3 100644
--- a/hw/usb-ohci.c
+++ b/hw/usb-ohci.c
@@ -1721,14 +1721,16 @@ static int usb_ohci_initfn_pci(struct PCIDevice *dev)
     pci_config_set_vendor_id(ohci->pci_dev.config, PCI_VENDOR_ID_APPLE);
     pci_config_set_device_id(ohci->pci_dev.config,
                              PCI_DEVICE_ID_APPLE_IPID_USB);
-    ohci->pci_dev.config[0x09] = 0x10; /* OHCI */
+    ohci->pci_dev.config[PCI_CLASS_PROG] = 0x10; /* OHCI */
     pci_config_set_class(ohci->pci_dev.config, PCI_CLASS_SERIAL_USB);
-    ohci->pci_dev.config[0x3d] = 0x01; /* interrupt pin 1 */
+    /* TODO: RST# value should be 0. */
+    ohci->pci_dev.config[PCI_INTERRUPT_PIN] = 0x01; /* interrupt pin 1 */
 
     usb_ohci_init(&ohci->state, &dev->qdev, num_ports,
                   ohci->pci_dev.devfn, ohci->pci_dev.irq[0],
                   OHCI_TYPE_PCI, ohci->pci_dev.name, 0);
 
+    /* TODO: avoid cast below by using dev */
     pci_register_bar((struct PCIDevice *)ohci, 0, 256,
                            PCI_BASE_ADDRESS_SPACE_MEMORY, ohci_mapfunc);
     return 0;
-- 
1.6.6.rc1.43.gf55cc

[Qemu-devel] [PATCH 17/17] pci: remove unused macro

[Michael S. Tsirkin]

PCI_STATUS_DEVSEL is unused, and it also
has a different name in pci_regs.h
Remove.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/pci.h |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/hw/pci.h b/hw/pci.h
index 4015d45..475b044 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -167,7 +167,6 @@ typedef struct PCIIORegion {
 #define PCI_STATUS_FAST_BACK	0x080
 #define PCI_STATUS_DEVSEL_MEDIUM 0x200
 #define PCI_STATUS_DEVSEL_SLOW	0x400
-#define PCI_STATUS_DEVSEL	0x600
 #define  PCI_STATUS_SIG_TARGET_ABORT	0x800 /* Set on target abort */
 #define  PCI_STATUS_REC_MASTER_ABORT	0x2000 /* Set on master abort */
 
-- 
1.6.6.rc1.43.gf55cc

[Qemu-devel] Re: [PATCH 16/17] usb-uhci: symbolic names for pci registers

[Michael S. Tsirkin]

On Sat, Dec 12, 2009 at 04:41:07PM +0100, Juan Quintela wrote:
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > No functional changes. I verified that the generated binary
> > does not change.
> 
> s/uhci/ohci/ in subject :)

Ugh. Right. Good catch.

> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  hw/usb-ohci.c |    6 ++++--
> >  1 files changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/usb-ohci.c b/hw/usb-ohci.c
> > index 7ab3a98..deab7f3 100644
> > --- a/hw/usb-ohci.c
> > +++ b/hw/usb-ohci.c
> > @@ -1721,14 +1721,16 @@ static int usb_ohci_initfn_pci(struct PCIDevice *dev)
> >      pci_config_set_vendor_id(ohci->pci_dev.config, PCI_VENDOR_ID_APPLE);
> >      pci_config_set_device_id(ohci->pci_dev.config,
> >                               PCI_DEVICE_ID_APPLE_IPID_USB);
> > -    ohci->pci_dev.config[0x09] = 0x10; /* OHCI */
> > +    ohci->pci_dev.config[PCI_CLASS_PROG] = 0x10; /* OHCI */
> >      pci_config_set_class(ohci->pci_dev.config, PCI_CLASS_SERIAL_USB);
> > -    ohci->pci_dev.config[0x3d] = 0x01; /* interrupt pin 1 */
> > +    /* TODO: RST# value should be 0. */
> > +    ohci->pci_dev.config[PCI_INTERRUPT_PIN] = 0x01; /* interrupt pin 1 */
> >  
> >      usb_ohci_init(&ohci->state, &dev->qdev, num_ports,
> >                    ohci->pci_dev.devfn, ohci->pci_dev.irq[0],
> >                    OHCI_TYPE_PCI, ohci->pci_dev.name, 0);
> >  
> > +    /* TODO: avoid cast below by using dev */
> >      pci_register_bar((struct PCIDevice *)ohci, 0, 256,
> >                             PCI_BASE_ADDRESS_SPACE_MEMORY, ohci_mapfunc);
> >      return 0;

Re: [Qemu-devel] [PATCH 06/17] eepro100: symbolic names for pci registers

[Stefan Weil]

Michael S. Tsirkin schrieb:
> No functional changes. I verified that the generated binary
> does not change in meaningful ways. Survived light usage
> with linux guest.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> hw/eepro100.c | 49 ++++++++++++++++++++++++++++++++-----------------
> 1 files changed, 32 insertions(+), 17 deletions(-)
>
> diff --git a/hw/eepro100.c b/hw/eepro100.c
> index 2a9e3b5..82e3766 100644
> --- a/hw/eepro100.c
> +++ b/hw/eepro100.c
> @@ -412,19 +412,24 @@ static void pci_reset(EEPRO100State * s)
> pci_config_set_vendor_id(pci_conf, PCI_VENDOR_ID_INTEL);
> /* PCI Device ID depends on device and is set below. */
> /* PCI Command */
> + /* TODO: this is the default, do not override. */
> PCI_CONFIG_16(PCI_COMMAND, 0x0000);
> /* PCI Status */
> - PCI_CONFIG_16(PCI_STATUS, 0x2800);
> + /* TODO: this seems to make no sense. */
> + /* TODO: Value at RST# should be 0. */
> + PCI_CONFIG_16(PCI_STATUS,
> + PCI_STATUS_REC_MASTER_ABORT | PCI_STATUS_SIG_TARGET_ABORT);
> /* PCI Revision ID */
Hi,

this PCI status value is wrong. The correct value for PCI_STATUS is 0x0280
and was fixed in the maintainer version in 2007:
http://repo.or.cz/w/qemu/ar7.git/commitdiff/9da3830d81948cc1f666fcf562699f165b029a79

It was also fixed in a patch sent to qemu-devel (which was never applied):
http://patchwork.ozlabs.org/patch/33962/

I'll send a new patch which fixes the wrong status value.

Antony, how can we speed up the synchronisation process for
eepro100.c? Today, patches get lost without feedback.
This is no wonder because you have to work on hundreds of patches.
It was suggested that I should send patch series.
My last patch serie with 3 patches is ready for integration
since 2009-12-20.

Paul, if I had commit rights, I could integrate the missing
parts myself and maintain eepro100.c in the future. Of course
I'd send the single changes to the list before comitting them.
Don't you think that would be the best solution for all of us?

Regards,
Stefan

[Qemu-devel] [PATCH] eepro100: Fix initial value for PCI_STATUS

[Stefan Weil]

The numerical value was wrong (0x2800 instead of 0x0280)
which indeed did not make sense.

Signed-off-by: Stefan Weil <weil@mail.berlios.de>
---
 hw/eepro100.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/hw/eepro100.c b/hw/eepro100.c
index 336ca49..a21c984 100644
--- a/hw/eepro100.c
+++ b/hw/eepro100.c
@@ -420,10 +420,8 @@ static void pci_reset(EEPRO100State * s)
     /* TODO: this is the default, do not override. */
     PCI_CONFIG_16(PCI_COMMAND, 0x0000);
     /* PCI Status */
-    /* TODO: this seems to make no sense. */
     /* TODO: Value at RST# should be 0. */
-    PCI_CONFIG_16(PCI_STATUS,
-                  PCI_STATUS_REC_MASTER_ABORT | PCI_STATUS_SIG_TARGET_ABORT);
+    PCI_CONFIG_16(PCI_STATUS, PCI_STATUS_DEVSEL_MEDIUM | PCI_STATUS_FAST_BACK);
     /* PCI Revision ID */
     PCI_CONFIG_8(PCI_REVISION_ID, 0x08);
     /* TODO: this is the default, do not override. */
-- 
1.6.5

[Qemu-devel] Re: [PATCH] eepro100: Fix initial value for PCI_STATUS

[Michael S. Tsirkin]

On Thu, Jan 07, 2010 at 12:15:25PM +0100, Stefan Weil wrote:
> The numerical value was wrong (0x2800 instead of 0x0280)
> which indeed did not make sense.

Aha! conversion to symbolic names paying off.

> Signed-off-by: Stefan Weil <weil@mail.berlios.de>

Applied, thanks.

> ---
>  hw/eepro100.c |    4 +---
>  1 files changed, 1 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/eepro100.c b/hw/eepro100.c
> index 336ca49..a21c984 100644
> --- a/hw/eepro100.c
> +++ b/hw/eepro100.c
> @@ -420,10 +420,8 @@ static void pci_reset(EEPRO100State * s)
>      /* TODO: this is the default, do not override. */
>      PCI_CONFIG_16(PCI_COMMAND, 0x0000);
>      /* PCI Status */
> -    /* TODO: this seems to make no sense. */
>      /* TODO: Value at RST# should be 0. */

So this second todo can go too. I've removed it in my tree.

> -    PCI_CONFIG_16(PCI_STATUS,
> -                  PCI_STATUS_REC_MASTER_ABORT | PCI_STATUS_SIG_TARGET_ABORT);
> +    PCI_CONFIG_16(PCI_STATUS, PCI_STATUS_DEVSEL_MEDIUM | PCI_STATUS_FAST_BACK);
>      /* PCI Revision ID */
>      PCI_CONFIG_8(PCI_REVISION_ID, 0x08);

BTW if you are not afraid of churn, there's no reason
for PCI_CONFIG_8 and friends anymore, because pci.h
has much nicer pci_set_byte etc.

>      /* TODO: this is the default, do not override. */
> -- 
> 1.6.5

Re: [Qemu-devel] [PATCH 06/17] eepro100: symbolic names for pci registers

[Michael S. Tsirkin]

On Thu, Jan 07, 2010 at 12:14:17PM +0100, Stefan Weil wrote:
> Michael S. Tsirkin schrieb:
> > No functional changes. I verified that the generated binary
> > does not change in meaningful ways. Survived light usage
> > with linux guest.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> > hw/eepro100.c | 49 ++++++++++++++++++++++++++++++++-----------------
> > 1 files changed, 32 insertions(+), 17 deletions(-)
> >
> > diff --git a/hw/eepro100.c b/hw/eepro100.c
> > index 2a9e3b5..82e3766 100644
> > --- a/hw/eepro100.c
> > +++ b/hw/eepro100.c
> > @@ -412,19 +412,24 @@ static void pci_reset(EEPRO100State * s)
> > pci_config_set_vendor_id(pci_conf, PCI_VENDOR_ID_INTEL);
> > /* PCI Device ID depends on device and is set below. */
> > /* PCI Command */
> > + /* TODO: this is the default, do not override. */
> > PCI_CONFIG_16(PCI_COMMAND, 0x0000);
> > /* PCI Status */
> > - PCI_CONFIG_16(PCI_STATUS, 0x2800);
> > + /* TODO: this seems to make no sense. */
> > + /* TODO: Value at RST# should be 0. */
> > + PCI_CONFIG_16(PCI_STATUS,
> > + PCI_STATUS_REC_MASTER_ABORT | PCI_STATUS_SIG_TARGET_ABORT);
> > /* PCI Revision ID */
> Hi,
> 
> this PCI status value is wrong. The correct value for PCI_STATUS is 0x0280
> and was fixed in the maintainer version in 2007:
> http://repo.or.cz/w/qemu/ar7.git/commitdiff/9da3830d81948cc1f666fcf562699f165b029a79
> 
> It was also fixed in a patch sent to qemu-devel (which was never applied):
> http://patchwork.ozlabs.org/patch/33962/
> 
> I'll send a new patch which fixes the wrong status value.
> 
> Antony, how can we speed up the synchronisation process for
> eepro100.c? Today, patches get lost without feedback.
> This is no wonder because you have to work on hundreds of patches.
> It was suggested that I should send patch series.
> My last patch serie with 3 patches is ready for integration
> since 2009-12-20.

A simple solution that worked for me is to publish a git tree and send
pull requests (in addition to individual patches).  This guarantees that
nothing is lost and makes it easy for Anthony to apply a set of changes
in one go. git merge also has smarter heuristics than git am.

I put the patch you just posted on my tree as it's PCI related and I
understand it, but if you create your own tree and want me to drop this
patch from my tree, pls let me know and I will.

> Paul, if I had commit rights, I could integrate the missing
> parts myself and maintain eepro100.c in the future. Of course
> I'd send the single changes to the list before comitting them.
> Don't you think that would be the best solution for all of us?
> 
> Regards,
> Stefan

Personally, I find it comforting that someone else pulls my tree instead
of commit rights: this ensures at least build testing is done before
my changes are inflicted on others :)

-- 
MST

Re: [Qemu-devel] [PATCH 06/17] eepro100: symbolic names for pci registers

[Anthony Liguori]

On 01/07/2010 05:14 AM, Stefan Weil wrote:
> Michael S. Tsirkin schrieb:
>    
>> No functional changes. I verified that the generated binary
>> does not change in meaningful ways. Survived light usage
>> with linux guest.
>>
>> Signed-off-by: Michael S. Tsirkin<mst@redhat.com>
>> ---
>> hw/eepro100.c | 49 ++++++++++++++++++++++++++++++++-----------------
>> 1 files changed, 32 insertions(+), 17 deletions(-)
>>
>> diff --git a/hw/eepro100.c b/hw/eepro100.c
>> index 2a9e3b5..82e3766 100644
>> --- a/hw/eepro100.c
>> +++ b/hw/eepro100.c
>> @@ -412,19 +412,24 @@ static void pci_reset(EEPRO100State * s)
>> pci_config_set_vendor_id(pci_conf, PCI_VENDOR_ID_INTEL);
>> /* PCI Device ID depends on device and is set below. */
>> /* PCI Command */
>> + /* TODO: this is the default, do not override. */
>> PCI_CONFIG_16(PCI_COMMAND, 0x0000);
>> /* PCI Status */
>> - PCI_CONFIG_16(PCI_STATUS, 0x2800);
>> + /* TODO: this seems to make no sense. */
>> + /* TODO: Value at RST# should be 0. */
>> + PCI_CONFIG_16(PCI_STATUS,
>> + PCI_STATUS_REC_MASTER_ABORT | PCI_STATUS_SIG_TARGET_ABORT);
>> /* PCI Revision ID */
>>      
> Hi,
>
> this PCI status value is wrong. The correct value for PCI_STATUS is 0x0280
> and was fixed in the maintainer version in 2007:
> http://repo.or.cz/w/qemu/ar7.git/commitdiff/9da3830d81948cc1f666fcf562699f165b029a79
>
> It was also fixed in a patch sent to qemu-devel (which was never applied):
> http://patchwork.ozlabs.org/patch/33962/
>
> I'll send a new patch which fixes the wrong status value.
>
> Antony, how can we speed up the synchronisation process for
> eepro100.c? Today, patches get lost without feedback.
> This is no wonder because you have to work on hundreds of patches.
> It was suggested that I should send patch series.
> My last patch serie with 3 patches is ready for integration
> since 2009-12-20.
>    

The big problem with eepro100 is that you're doing a bunch of work in an 
external tree, and then trickling things slowly onto the list.  Take 
some time and send out one big series getting your internal tree in 
sync.  Of course, the end target must conform to CodingStyle which is a 
problem right now in your tree.

> Paul, if I had commit rights, I could integrate the missing
> parts myself and maintain eepro100.c in the future. Of course
> I'd send the single changes to the list before comitting them.
> Don't you think that would be the best solution for all of us?
>    

Pull requests work just as well as commit rights.  However, pull 
requests only work when the patches are ready to go and don't need 
iteration.  A bare minimum for that is going to be conforming to 
CodingStyle which has been a problem with eepro100.

But look, you send out patches during a holiday, and we're still 
catching up.  If it had been during a normal time period, that would be 
one thing, but please exercise a bit of patience.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 06/17] eepro100: symbolic names for pci registers

[Anthony Liguori]

On 01/07/2010 06:51 AM, Michael S. Tsirkin wrote:
> A simple solution that worked for me is to publish a git tree and send
> pull requests (in addition to individual patches).  This guarantees that
> nothing is lost and makes it easy for Anthony to apply a set of changes
> in one go. git merge also has smarter heuristics than git am.
>
> I put the patch you just posted on my tree as it's PCI related and I
> understand it, but if you create your own tree and want me to drop this
> patch from my tree, pls let me know and I will.
>    

I would actually be happy to pull eepro100 changes through Michael's tree.

Michael has been very good about reviewing patches quickly and so far, 
it's been working pretty well.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 06/17] eepro100: symbolic names for pci registers

[Stefan Weil]

Anthony Liguori schrieb:
> On 01/07/2010 05:14 AM, Stefan Weil wrote:
>> Michael S. Tsirkin schrieb:
>>   
>>> No functional changes. I verified that the generated binary
>>> does not change in meaningful ways. Survived light usage
>>> with linux guest.
>>>
>>> Signed-off-by: Michael S. Tsirkin<mst@redhat.com>
>>> ---
>>> hw/eepro100.c | 49 ++++++++++++++++++++++++++++++++-----------------
>>> 1 files changed, 32 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/hw/eepro100.c b/hw/eepro100.c
>>> index 2a9e3b5..82e3766 100644
>>> --- a/hw/eepro100.c
>>> +++ b/hw/eepro100.c
>>> @@ -412,19 +412,24 @@ static void pci_reset(EEPRO100State * s)
>>> pci_config_set_vendor_id(pci_conf, PCI_VENDOR_ID_INTEL);
>>> /* PCI Device ID depends on device and is set below. */
>>> /* PCI Command */
>>> + /* TODO: this is the default, do not override. */
>>> PCI_CONFIG_16(PCI_COMMAND, 0x0000);
>>> /* PCI Status */
>>> - PCI_CONFIG_16(PCI_STATUS, 0x2800);
>>> + /* TODO: this seems to make no sense. */
>>> + /* TODO: Value at RST# should be 0. */
>>> + PCI_CONFIG_16(PCI_STATUS,
>>> + PCI_STATUS_REC_MASTER_ABORT | PCI_STATUS_SIG_TARGET_ABORT);
>>> /* PCI Revision ID */
>>>      
>> Hi,
>>
>> this PCI status value is wrong. The correct value for PCI_STATUS is
>> 0x0280
>> and was fixed in the maintainer version in 2007:
>> http://repo.or.cz/w/qemu/ar7.git/commitdiff/9da3830d81948cc1f666fcf562699f165b029a79
>>
>>
>> It was also fixed in a patch sent to qemu-devel (which was never
>> applied):
>> http://patchwork.ozlabs.org/patch/33962/
>>
>> I'll send a new patch which fixes the wrong status value.
>>
>> Antony, how can we speed up the synchronisation process for
>> eepro100.c? Today, patches get lost without feedback.
>> This is no wonder because you have to work on hundreds of patches.
>> It was suggested that I should send patch series.
>> My last patch serie with 3 patches is ready for integration
>> since 2009-12-20.
>>    
>
> The big problem with eepro100 is that you're doing a bunch of work in
> an external tree, and then trickling things slowly onto the list. 
> Take some time and send out one big series getting your internal tree
> in sync.  Of course, the end target must conform to CodingStyle which
> is a problem right now in your tree.
>
>> Paul, if I had commit rights, I could integrate the missing
>> parts myself and maintain eepro100.c in the future. Of course
>> I'd send the single changes to the list before comitting them.
>> Don't you think that would be the best solution for all of us?
>>    
>
> Pull requests work just as well as commit rights.  However, pull
> requests only work when the patches are ready to go and don't need
> iteration.  A bare minimum for that is going to be conforming to
> CodingStyle which has been a problem with eepro100.
>
> But look, you send out patches during a holiday, and we're still
> catching up.  If it had been during a normal time period, that would
> be one thing, but please exercise a bit of patience.
>
> Regards,
>
> Anthony Liguori


Hello Antony,

thank you for your fast feedback. I also think that coding style
is important, so don't hesitate to tell me when there are problems.

The only violations of coding style in eepro100.c I am aware of
are C++ comments and data types using _t. They are relicts from
the time when QEMU did not have a documented coding style,
were addressed in previous patches and are also fixed in new patches.

Are there other coding style issues which I did not see?

Regards,

Stefan Weil

[Qemu-devel] [RFC] API change for pci_set_word and related functions (was Re: [PATCH] eepro100: Fix initial value for PCI_STATUS)

[Stefan Weil]

Michael S. Tsirkin schrieb:
> On Thu, Jan 07, 2010 at 12:15:25PM +0100, Stefan Weil wrote:
> ...
>> ---
>> hw/eepro100.c | 4 +---
>> 1 files changed, 1 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/eepro100.c b/hw/eepro100.c
>> index 336ca49..a21c984 100644
>> --- a/hw/eepro100.c
>> +++ b/hw/eepro100.c
>> @@ -420,10 +420,8 @@ static void pci_reset(EEPRO100State * s)
>> /* TODO: this is the default, do not override. */
>> PCI_CONFIG_16(PCI_COMMAND, 0x0000);
>> /* PCI Status */
>> - /* TODO: this seems to make no sense. */
>> /* TODO: Value at RST# should be 0. */
>
> So this second todo can go too. I've removed it in my tree.
>
>> - PCI_CONFIG_16(PCI_STATUS,
>> - PCI_STATUS_REC_MASTER_ABORT | PCI_STATUS_SIG_TARGET_ABORT);
>> + PCI_CONFIG_16(PCI_STATUS, PCI_STATUS_DEVSEL_MEDIUM |
>> PCI_STATUS_FAST_BACK);
>> /* PCI Revision ID */
>> PCI_CONFIG_8(PCI_REVISION_ID, 0x08);
>
> BTW if you are not afraid of churn, there's no reason
> for PCI_CONFIG_8 and friends anymore, because pci.h
> has much nicer pci_set_byte etc.

Hello Michael,

I already noticed pci_set_byte, pci_set_word, pci_set_long and
the corresponding pci_get_xxx functions and thought about using them.

I did not start it because I want to suggest a different API
for use in PCI device emulations:

instead of

pci_set_word(pci_conf + PCI_STATUS, PCI_STATUS_CAP_LIST);

or

pci_set_word(&pci_conf[PCI_STATUS], PCI_STATUS_CAP_LIST);

it would be better to call

pci_set_word(pci_conf, PCI_STATUS, PCI_STATUS_CAP_LIST);


The prototypes would look like this:

/* Set PCI config value. */
void pci_set_word(PCIDevice *s, uint8_t offset, uint16_t val);

/* Set PCI cmask value. */
void pci_set_cmask_word(PCIDevice *s, uint8_t offset, uint16_t val);

/* Set PCI wmask value. */
void pci_set_wmask_word(PCIDevice *s, uint8_t offset, uint16_t val);

What are the advantages?

* strict type checking (the old API takes any uint8_t *)
* many other pci_* functions also have a first parameter of type PCIDevice
* calls look nicer (at least in my opinion)
* strict range checking (offset is limited to 0...255, additional
  assertions possible - the old API is unsafe because it just takes
  a pointer)

The functions are inline, so the resulting code won't differ.

Instead of _byte, _word and _long I personally prefer something
like _8, _16, _32 because _word and _long need interpretation.
But this is only a matter of taste - the API change is more important.


Regards,

Stefan Weil

[Qemu-devel] Re: [RFC] API change for pci_set_word and related functions (was Re: [PATCH] eepro100: Fix initial value for PCI_STATUS)

[Michael S. Tsirkin]

On Thu, Jan 07, 2010 at 04:07:26PM +0100, Stefan Weil wrote:
> Michael S. Tsirkin schrieb:
> > On Thu, Jan 07, 2010 at 12:15:25PM +0100, Stefan Weil wrote:
> > ...
> >> ---
> >> hw/eepro100.c | 4 +---
> >> 1 files changed, 1 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/hw/eepro100.c b/hw/eepro100.c
> >> index 336ca49..a21c984 100644
> >> --- a/hw/eepro100.c
> >> +++ b/hw/eepro100.c
> >> @@ -420,10 +420,8 @@ static void pci_reset(EEPRO100State * s)
> >> /* TODO: this is the default, do not override. */
> >> PCI_CONFIG_16(PCI_COMMAND, 0x0000);
> >> /* PCI Status */
> >> - /* TODO: this seems to make no sense. */
> >> /* TODO: Value at RST# should be 0. */
> >
> > So this second todo can go too. I've removed it in my tree.
> >
> >> - PCI_CONFIG_16(PCI_STATUS,
> >> - PCI_STATUS_REC_MASTER_ABORT | PCI_STATUS_SIG_TARGET_ABORT);
> >> + PCI_CONFIG_16(PCI_STATUS, PCI_STATUS_DEVSEL_MEDIUM |
> >> PCI_STATUS_FAST_BACK);
> >> /* PCI Revision ID */
> >> PCI_CONFIG_8(PCI_REVISION_ID, 0x08);
> >
> > BTW if you are not afraid of churn, there's no reason
> > for PCI_CONFIG_8 and friends anymore, because pci.h
> > has much nicer pci_set_byte etc.
> 
> Hello Michael,
> 
> I already noticed pci_set_byte, pci_set_word, pci_set_long and
> the corresponding pci_get_xxx functions and thought about using them.
> 
> I did not start it because I want to suggest a different API
> for use in PCI device emulations:
> 
> instead of
> 
> pci_set_word(pci_conf + PCI_STATUS, PCI_STATUS_CAP_LIST);
> 
> or
> 
> pci_set_word(&pci_conf[PCI_STATUS], PCI_STATUS_CAP_LIST);
> 
> it would be better to call
> 
> pci_set_word(pci_conf, PCI_STATUS, PCI_STATUS_CAP_LIST);
> 
> 
> The prototypes would look like this:
> 
> /* Set PCI config value. */
> void pci_set_word(PCIDevice *s, uint8_t offset, uint16_t val);
> 
> /* Set PCI cmask value. */
> void pci_set_cmask_word(PCIDevice *s, uint8_t offset, uint16_t val);
> 
> /* Set PCI wmask value. */
> void pci_set_wmask_word(PCIDevice *s, uint8_t offset, uint16_t val);
> 
> What are the advantages?
> * strict type checking (the old API takes any uint8_t *)

So IMO it's easier to make mistakes with your proposed API because if
you confuse offset and value compiler does not complain.  More
importantly, if you want to pass some other uint8_t pointer to e.g.
pci_set_word, you can *and nothing unexpected will happen*
it will set the word to the given value. So the current
API is more type safe than what you propose.

> * many other pci_* functions also have a first parameter of type PCIDevice

So what would make sense IMO is higer level abstraction,
for example similar to what we have with capabilities
and msix, I think we could have something like this
for e.g. power management.

For low-level bit tweaking, the advantages of current API is that same
thing can be used to set wmask, cmask, config itself, and whatever else
we will come up with.

> * calls look nicer (at least in my opinion)

What I value is the fact that it's obvious which
data is changed.

> * strict range checking (offset is limited to 0...255, additional
>   assertions possible - the old API is unsafe because it just takes
>   a pointer)

I don't think we want to add return status, so there wouldn't
be a benefit to range checking as we can't act on it.
Anyway, it's very unusual to use anything but a constant
as an offset, so range errors are very uncommon.

> The functions are inline, so the resulting code won't differ.
> 
> Instead of _byte, _word and _long I personally prefer something
> like _8, _16, _32 because _word and _long need interpretation.
> But this is only a matter of taste - the API change is more important.
> 
> 
> Regards,
> 
> Stefan Weil



-- 
MST

[Qemu-devel] Re: [RFC] API change for pci_set_word and related functions

[Stefan Weil]

Michael S. Tsirkin schrieb:
> On Thu, Jan 07, 2010 at 04:07:26PM +0100, Stefan Weil wrote:
>> Michael S. Tsirkin schrieb:
>>> On Thu, Jan 07, 2010 at 12:15:25PM +0100, Stefan Weil wrote:
>>> ...
>>>> ---
>>>> hw/eepro100.c | 4 +---
>>>> 1 files changed, 1 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/hw/eepro100.c b/hw/eepro100.c
>>>> index 336ca49..a21c984 100644
>>>> --- a/hw/eepro100.c
>>>> +++ b/hw/eepro100.c
>>>> @@ -420,10 +420,8 @@ static void pci_reset(EEPRO100State * s)
>>>> /* TODO: this is the default, do not override. */
>>>> PCI_CONFIG_16(PCI_COMMAND, 0x0000);
>>>> /* PCI Status */
>>>> - /* TODO: this seems to make no sense. */
>>>> /* TODO: Value at RST# should be 0. */
>>> So this second todo can go too. I've removed it in my tree.
>>>
>>>> - PCI_CONFIG_16(PCI_STATUS,
>>>> - PCI_STATUS_REC_MASTER_ABORT | PCI_STATUS_SIG_TARGET_ABORT);
>>>> + PCI_CONFIG_16(PCI_STATUS, PCI_STATUS_DEVSEL_MEDIUM |
>>>> PCI_STATUS_FAST_BACK);
>>>> /* PCI Revision ID */
>>>> PCI_CONFIG_8(PCI_REVISION_ID, 0x08);
>>> BTW if you are not afraid of churn, there's no reason
>>> for PCI_CONFIG_8 and friends anymore, because pci.h
>>> has much nicer pci_set_byte etc.
>> Hello Michael,
>>
>> I already noticed pci_set_byte, pci_set_word, pci_set_long and
>> the corresponding pci_get_xxx functions and thought about using them.
>>
>> I did not start it because I want to suggest a different API
>> for use in PCI device emulations:
>>
>> instead of
>>
>> pci_set_word(pci_conf + PCI_STATUS, PCI_STATUS_CAP_LIST);
>>
>> or
>>
>> pci_set_word(&pci_conf[PCI_STATUS], PCI_STATUS_CAP_LIST);
>>
>> it would be better to call
>>
>> pci_set_word(pci_conf, PCI_STATUS, PCI_STATUS_CAP_LIST);
>>
>>
>> The prototypes would look like this:
>>
>> /* Set PCI config value. */
>> void pci_set_word(PCIDevice *s, uint8_t offset, uint16_t val);
>>
>> /* Set PCI cmask value. */
>> void pci_set_cmask_word(PCIDevice *s, uint8_t offset, uint16_t val);
>>
>> /* Set PCI wmask value. */
>> void pci_set_wmask_word(PCIDevice *s, uint8_t offset, uint16_t val);
>>
>> What are the advantages?
>> * strict type checking (the old API takes any uint8_t *)
>
> So IMO it's easier to make mistakes with your proposed API because if
> you confuse offset and value compiler does not complain. More
> importantly, if you want to pass some other uint8_t pointer to e.g.
> pci_set_word, you can *and nothing unexpected will happen*
> it will set the word to the given value. So the current
> API is more type safe than what you propose.
>

No. The current API takes any uint8_t pointer to read or write
a value. This is not safe.

The proposed API only takes a PCIDevice pointer
and reads or writes only configuration (or cmask or
wmask) values. Yes, you can take offset for value.
If you are lucky and value is an uint16_t or uint32_t,
your compiler will complain. And even if your compiler
does not complain, it is wrong but still safe, because
the code will only access the PCI configuration data.


>> * many other pci_* functions also have a first parameter of type PCIDevice
>
> So what would make sense IMO is higer level abstraction,
> for example similar to what we have with capabilities
> and msix, I think we could have something like this
> for e.g. power management.
>
> For low-level bit tweaking, the advantages of current API is that same
> thing can be used to set wmask, cmask, config itself, and whatever else
> we will come up with.

The low level API can be used where low level is
adequate: in pci.c for example.

To implement emulated PCI devices, a more robust API
would be better. Think of the number of devices which
are still missing, think of people who want to write
a new PCI device emulation for QEMU without being
a QEMU expert.

>
>> * calls look nicer (at least in my opinion)
>
> What I value is the fact that it's obvious which
> data is changed.

Here there is no difference between current and
proposed API:

old: pci_set_word(&pci_conf[PCI_STATUS], PCI_STATUS_CAP_LIST);
new: pci_set_word(pci_conf, PCI_STATUS, PCI_STATUS_CAP_LIST);

Every function call hides what happens. If you really wanted
to see which data is changed, you would have to write

*(uint16_t *)&pci_conf[PCI_STATUS] = cpu_to_le16(PCI_STATUS_CAP_LIST);

>
>> * strict range checking (offset is limited to 0...255, additional
>>   assertions possible - the old API is unsafe because it just takes
>>   a pointer)
>
> I don't think we want to add return status, so there wouldn't
> be a benefit to range checking as we can't act on it.
> Anyway, it's very unusual to use anything but a constant
> as an offset, so range errors are very uncommon.

There is an implicit range checking in the proposed
API because the offset is uint8_t, so it cannot
exceed the range which is valid for configuration
offsets.

A more elaborated check could require that
configuration byte values are only addressed
using pci_set_byte (not pci_set_long)
and raise a fatal runtime error otherwise.

Runtime checks without return values
are well established in QEMU's code,
and they are very useful for code writers.

>
>> The functions are inline, so the resulting code won't differ.
>>
>> Instead of _byte, _word and _long I personally prefer something
>> like _8, _16, _32 because _word and _long need interpretation.
>> But this is only a matter of taste - the API change is more important.
>>
>>
>> Regards,
>>
>> Stefan Weil

[Qemu-devel] Re: [RFC] API change for pci_set_word and related functions

[Michael S. Tsirkin]

On Mon, Jan 11, 2010 at 08:38:53PM +0100, Stefan Weil wrote:
> Michael S. Tsirkin schrieb:
> > On Thu, Jan 07, 2010 at 04:07:26PM +0100, Stefan Weil wrote:
> >> Michael S. Tsirkin schrieb:
> >>> On Thu, Jan 07, 2010 at 12:15:25PM +0100, Stefan Weil wrote:
> >>> ...
> >>>> ---
> >>>> hw/eepro100.c | 4 +---
> >>>> 1 files changed, 1 insertions(+), 3 deletions(-)
> >>>>
> >>>> diff --git a/hw/eepro100.c b/hw/eepro100.c
> >>>> index 336ca49..a21c984 100644
> >>>> --- a/hw/eepro100.c
> >>>> +++ b/hw/eepro100.c
> >>>> @@ -420,10 +420,8 @@ static void pci_reset(EEPRO100State * s)
> >>>> /* TODO: this is the default, do not override. */
> >>>> PCI_CONFIG_16(PCI_COMMAND, 0x0000);
> >>>> /* PCI Status */
> >>>> - /* TODO: this seems to make no sense. */
> >>>> /* TODO: Value at RST# should be 0. */
> >>> So this second todo can go too. I've removed it in my tree.
> >>>
> >>>> - PCI_CONFIG_16(PCI_STATUS,
> >>>> - PCI_STATUS_REC_MASTER_ABORT | PCI_STATUS_SIG_TARGET_ABORT);
> >>>> + PCI_CONFIG_16(PCI_STATUS, PCI_STATUS_DEVSEL_MEDIUM |
> >>>> PCI_STATUS_FAST_BACK);
> >>>> /* PCI Revision ID */
> >>>> PCI_CONFIG_8(PCI_REVISION_ID, 0x08);
> >>> BTW if you are not afraid of churn, there's no reason
> >>> for PCI_CONFIG_8 and friends anymore, because pci.h
> >>> has much nicer pci_set_byte etc.
> >> Hello Michael,
> >>
> >> I already noticed pci_set_byte, pci_set_word, pci_set_long and
> >> the corresponding pci_get_xxx functions and thought about using them.
> >>
> >> I did not start it because I want to suggest a different API
> >> for use in PCI device emulations:
> >>
> >> instead of
> >>
> >> pci_set_word(pci_conf + PCI_STATUS, PCI_STATUS_CAP_LIST);
> >>
> >> or
> >>
> >> pci_set_word(&pci_conf[PCI_STATUS], PCI_STATUS_CAP_LIST);
> >>
> >> it would be better to call
> >>
> >> pci_set_word(pci_conf, PCI_STATUS, PCI_STATUS_CAP_LIST);
> >>
> >>
> >> The prototypes would look like this:
> >>
> >> /* Set PCI config value. */
> >> void pci_set_word(PCIDevice *s, uint8_t offset, uint16_t val);
> >>
> >> /* Set PCI cmask value. */
> >> void pci_set_cmask_word(PCIDevice *s, uint8_t offset, uint16_t val);
> >>
> >> /* Set PCI wmask value. */
> >> void pci_set_wmask_word(PCIDevice *s, uint8_t offset, uint16_t val);
> >>
> >> What are the advantages?
> >> * strict type checking (the old API takes any uint8_t *)
> >
> > So IMO it's easier to make mistakes with your proposed API because if
> > you confuse offset and value compiler does not complain. More
> > importantly, if you want to pass some other uint8_t pointer to e.g.
> > pci_set_word, you can *and nothing unexpected will happen*
> > it will set the word to the given value. So the current
> > API is more type safe than what you propose.
> >
> 
> No. The current API takes any uint8_t pointer to read or write
> a value. This is not safe.

Why isn't it?

> The proposed API only takes a PCIDevice pointer
> and reads or writes only configuration (or cmask or
> wmask) values. Yes, you can take offset for value.
> If you are lucky and value is an uint16_t or uint32_t,
> your compiler will complain.

Such a compiler will also complain over most of qemu code.

> And even if your compiler
> does not complain, it is wrong but still safe, because
> the code will only access the PCI configuration data.
> 

Correct and safe beats wrong and safe every time.

> >> * many other pci_* functions also have a first parameter of type PCIDevice
> >
> > So what would make sense IMO is higer level abstraction,
> > for example similar to what we have with capabilities
> > and msix, I think we could have something like this
> > for e.g. power management.
> >
> > For low-level bit tweaking, the advantages of current API is that same
> > thing can be used to set wmask, cmask, config itself, and whatever else
> > we will come up with.
> 
> The low level API can be used where low level is
> adequate: in pci.c for example.
> 
> To implement emulated PCI devices, a more robust API
> would be better. Think of the number of devices which
> are still missing, think of people who want to write
> a new PCI device emulation for QEMU without being
> a QEMU expert.
> 
> >
> >> * calls look nicer (at least in my opinion)
> >
> > What I value is the fact that it's obvious which
> > data is changed.
> 
> Here there is no difference between current and
> proposed API:
> 
> old: pci_set_word(&pci_conf[PCI_STATUS], PCI_STATUS_CAP_LIST);
> new: pci_set_word(pci_conf, PCI_STATUS, PCI_STATUS_CAP_LIST);
> 
> Every function call hides what happens. If you really wanted
> to see which data is changed, you would have to write
> 
> *(uint16_t *)&pci_conf[PCI_STATUS] = cpu_to_le16(PCI_STATUS_CAP_LIST);

That's what we used to have, and it's not all bad, but very verbose and
ugly.

> >
> >> * strict range checking (offset is limited to 0...255, additional
> >>   assertions possible - the old API is unsafe because it just takes
> >>   a pointer)
> >
> > I don't think we want to add return status, so there wouldn't
> > be a benefit to range checking as we can't act on it.
> > Anyway, it's very unusual to use anything but a constant
> > as an offset, so range errors are very uncommon.
> 
> There is an implicit range checking in the proposed
> API because the offset is uint8_t, so it cannot
> exceed the range which is valid for configuration
> offsets.

Oh, btw, this is wrong on pci express.

> A more elaborated check could require that
> configuration byte values are only addressed
> using pci_set_byte (not pci_set_long)
> and raise a fatal runtime error otherwise.
> 
> Runtime checks without return values
> are well established in QEMU's code,
> and they are very useful for code writers.
> 
> >
> >> The functions are inline, so the resulting code won't differ.
> >>
> >> Instead of _byte, _word and _long I personally prefer something
> >> like _8, _16, _32 because _word and _long need interpretation.
> >> But this is only a matter of taste - the API change is more important.
> >>
> >>
> >> Regards,
> >>
> >> Stefan Weil
> 
> 

[Qemu-devel] Re: [RFC] API change for pci_set_word and related functions

[Stefan Weil]

Michael S. Tsirkin schrieb:
> On Mon, Jan 11, 2010 at 08:38:53PM +0100, Stefan Weil wrote:
>> Michael S. Tsirkin schrieb:
>>> On Thu, Jan 07, 2010 at 04:07:26PM +0100, Stefan Weil wrote:
>>>> Michael S. Tsirkin schrieb:
>>>>> On Thu, Jan 07, 2010 at 12:15:25PM +0100, Stefan Weil wrote:
>>>>> ...
>>>>>> - PCI_CONFIG_16(PCI_STATUS,
>>>>>> - PCI_STATUS_REC_MASTER_ABORT | PCI_STATUS_SIG_TARGET_ABORT);
>>>>>> + PCI_CONFIG_16(PCI_STATUS, PCI_STATUS_DEVSEL_MEDIUM |
>>>>>> PCI_STATUS_FAST_BACK);
>>>>>> /* PCI Revision ID */
>>>>>> PCI_CONFIG_8(PCI_REVISION_ID, 0x08);
>>>>> BTW if you are not afraid of churn, there's no reason
>>>>> for PCI_CONFIG_8 and friends anymore, because pci.h
>>>>> has much nicer pci_set_byte etc.
>>>> Hello Michael,
>>>>
>>>> I already noticed pci_set_byte, pci_set_word, pci_set_long and
>>>> the corresponding pci_get_xxx functions and thought about using them.
>>>>
>>>> I did not start it because I want to suggest a different API
>>>> for use in PCI device emulations:
>>>>
>>>> instead of
>>>>
>>>> pci_set_word(pci_conf + PCI_STATUS, PCI_STATUS_CAP_LIST);
>>>>
>>>> or
>>>>
>>>> pci_set_word(&pci_conf[PCI_STATUS], PCI_STATUS_CAP_LIST);
>>>>
>>>> it would be better to call
>>>>
>>>> pci_set_word(pci_conf, PCI_STATUS, PCI_STATUS_CAP_LIST);
>>>>
>>>>
>>>> The prototypes would look like this:
>>>>
>>>> /* Set PCI config value. */
>>>> void pci_set_word(PCIDevice *s, uint8_t offset, uint16_t val);
>>>>
>>>> /* Set PCI cmask value. */
>>>> void pci_set_cmask_word(PCIDevice *s, uint8_t offset, uint16_t val);
>>>>
>>>> /* Set PCI wmask value. */
>>>> void pci_set_wmask_word(PCIDevice *s, uint8_t offset, uint16_t val);
>>>>
>>>> What are the advantages?
>>>> * strict type checking (the old API takes any uint8_t *)
>>> So IMO it's easier to make mistakes with your proposed API because if
>>> you confuse offset and value compiler does not complain. More
>>> importantly, if you want to pass some other uint8_t pointer to e.g.
>>> pci_set_word, you can *and nothing unexpected will happen*
>>> it will set the word to the given value. So the current
>>> API is more type safe than what you propose.
>>>
>> No. The current API takes any uint8_t pointer to read or write
>> a value. This is not safe.
>
> Why isn't it?

It is not safe, because it allows programmers to write silly code
like these examples:

pci_set_word(&gen_opc_cc_op[PCI_STATUS], PCI_STATUS_CAP_LIST);
pci_set_word(&pci_conf[UINT32_MAX], PCI_STATUS_CAP_LIST);

for (i = 0; i < sizeof(pci_conf); i++) {
    pci_set_long(&pci_conf[i], 0);
}

All three will result in runtime failures which can be very
difficult to detect.

>
>> The proposed API only takes a PCIDevice pointer
>> and reads or writes only configuration (or cmask or
>> wmask) values. Yes, you can take offset for value.
>> If you are lucky and value is an uint16_t or uint32_t,
>> your compiler will complain.
>
> Such a compiler will also complain over most of qemu code.

Yes, but it's good to see that QEMU's code is
improving.

By the way, such a compiler is gcc when called with
-Wconversion, so it is easy to see how much code
is affected.

>
>> And even if your compiler
>> does not complain, it is wrong but still safe, because
>> the code will only access the PCI configuration data.
>>
>
> Correct and safe beats wrong and safe every time.

See example above.

>
>>>> * many other pci_* functions also have a first parameter of type
>>>> PCIDevice
>>> So what would make sense IMO is higer level abstraction,
>>> for example similar to what we have with capabilities
>>> and msix, I think we could have something like this
>>> for e.g. power management.
>>>
>>> For low-level bit tweaking, the advantages of current API is that same
>>> thing can be used to set wmask, cmask, config itself, and whatever else
>>> we will come up with.
>> The low level API can be used where low level is
>> adequate: in pci.c for example.
>>
>> To implement emulated PCI devices, a more robust API
>> would be better. Think of the number of devices which
>> are still missing, think of people who want to write
>> a new PCI device emulation for QEMU without being
>> a QEMU expert.
>>
>>>> * calls look nicer (at least in my opinion)
>>> What I value is the fact that it's obvious which
>>> data is changed.
>> Here there is no difference between current and
>> proposed API:
>>
>> old: pci_set_word(&pci_conf[PCI_STATUS], PCI_STATUS_CAP_LIST);
>> new: pci_set_word(pci_conf, PCI_STATUS, PCI_STATUS_CAP_LIST);
>>
>> Every function call hides what happens. If you really wanted
>> to see which data is changed, you would have to write
>>
>> *(uint16_t *)&pci_conf[PCI_STATUS] = cpu_to_le16(PCI_STATUS_CAP_LIST);
>
> That's what we used to have, and it's not all bad, but very verbose and
> ugly.

Yes. I also prefer a function API.

>
>>>> * strict range checking (offset is limited to 0...255, additional
>>>> assertions possible - the old API is unsafe because it just takes
>>>> a pointer)
>>> I don't think we want to add return status, so there wouldn't
>>> be a benefit to range checking as we can't act on it.
>>> Anyway, it's very unusual to use anything but a constant
>>> as an offset, so range errors are very uncommon.
>> There is an implicit range checking in the proposed
>> API because the offset is uint8_t, so it cannot
>> exceed the range which is valid for configuration
>> offsets.
>
> Oh, btw, this is wrong on pci express.

Great, so PCI express devices should have their own
set of functions with the correct runtime checks:

pci_e_set_config, ...

>
>> A more elaborated check could require that
>> configuration byte values are only addressed
>> using pci_set_byte (not pci_set_long)
>> and raise a fatal runtime error otherwise.
>>
>> Runtime checks without return values
>> are well established in QEMU's code,
>> and they are very useful for code writers.
>>
>>>> The functions are inline, so the resulting code won't differ.
>>>>
>>>> Instead of _byte, _word and _long I personally prefer something
>>>> like _8, _16, _32 because _word and _long need interpretation.
>>>> But this is only a matter of taste - the API change is more important.
>>>>
>>>>
>>>> Regards,
>>>>
>>>> Stefan Weil
>>

[Qemu-devel] Re: [RFC] API change for pci_set_word and related functions

[Michael S. Tsirkin]

On Mon, Jan 11, 2010 at 09:18:51PM +0100, Stefan Weil wrote:
> Michael S. Tsirkin schrieb:
> > On Mon, Jan 11, 2010 at 08:38:53PM +0100, Stefan Weil wrote:
> >> Michael S. Tsirkin schrieb:
> >>> On Thu, Jan 07, 2010 at 04:07:26PM +0100, Stefan Weil wrote:
> >>>> Michael S. Tsirkin schrieb:
> >>>>> On Thu, Jan 07, 2010 at 12:15:25PM +0100, Stefan Weil wrote:
> >>>>> ...
> >>>>>> - PCI_CONFIG_16(PCI_STATUS,
> >>>>>> - PCI_STATUS_REC_MASTER_ABORT | PCI_STATUS_SIG_TARGET_ABORT);
> >>>>>> + PCI_CONFIG_16(PCI_STATUS, PCI_STATUS_DEVSEL_MEDIUM |
> >>>>>> PCI_STATUS_FAST_BACK);
> >>>>>> /* PCI Revision ID */
> >>>>>> PCI_CONFIG_8(PCI_REVISION_ID, 0x08);
> >>>>> BTW if you are not afraid of churn, there's no reason
> >>>>> for PCI_CONFIG_8 and friends anymore, because pci.h
> >>>>> has much nicer pci_set_byte etc.
> >>>> Hello Michael,
> >>>>
> >>>> I already noticed pci_set_byte, pci_set_word, pci_set_long and
> >>>> the corresponding pci_get_xxx functions and thought about using them.
> >>>>
> >>>> I did not start it because I want to suggest a different API
> >>>> for use in PCI device emulations:
> >>>>
> >>>> instead of
> >>>>
> >>>> pci_set_word(pci_conf + PCI_STATUS, PCI_STATUS_CAP_LIST);
> >>>>
> >>>> or
> >>>>
> >>>> pci_set_word(&pci_conf[PCI_STATUS], PCI_STATUS_CAP_LIST);
> >>>>
> >>>> it would be better to call
> >>>>
> >>>> pci_set_word(pci_conf, PCI_STATUS, PCI_STATUS_CAP_LIST);
> >>>>
> >>>>
> >>>> The prototypes would look like this:
> >>>>
> >>>> /* Set PCI config value. */
> >>>> void pci_set_word(PCIDevice *s, uint8_t offset, uint16_t val);
> >>>>
> >>>> /* Set PCI cmask value. */
> >>>> void pci_set_cmask_word(PCIDevice *s, uint8_t offset, uint16_t val);
> >>>>
> >>>> /* Set PCI wmask value. */
> >>>> void pci_set_wmask_word(PCIDevice *s, uint8_t offset, uint16_t val);
> >>>>
> >>>> What are the advantages?
> >>>> * strict type checking (the old API takes any uint8_t *)
> >>> So IMO it's easier to make mistakes with your proposed API because if
> >>> you confuse offset and value compiler does not complain. More
> >>> importantly, if you want to pass some other uint8_t pointer to e.g.
> >>> pci_set_word, you can *and nothing unexpected will happen*
> >>> it will set the word to the given value. So the current
> >>> API is more type safe than what you propose.
> >>>
> >> No. The current API takes any uint8_t pointer to read or write
> >> a value. This is not safe.
> >
> > Why isn't it?
> 
> It is not safe, because it allows programmers to write silly code
> like these examples:
> 
> pci_set_word(&gen_opc_cc_op[PCI_STATUS], PCI_STATUS_CAP_LIST);

This might be valid and useful for all I know.

> pci_set_word(&pci_conf[UINT32_MAX], PCI_STATUS_CAP_LIST);
> 
> for (i = 0; i < sizeof(pci_conf); i++) {
>     pci_set_long(&pci_conf[i], 0);
> }
> 
> All three will result in runtime failures which can be very
> difficult to detect.

In theory. In practice I expect pci_set_word(pci_dev, 0x0, PCI_STATUS)
to be much more common with your proposed API. Just look how
common swapping memset parameters is.

> >
> >> The proposed API only takes a PCIDevice pointer
> >> and reads or writes only configuration (or cmask or
> >> wmask) values. Yes, you can take offset for value.
> >> If you are lucky and value is an uint16_t or uint32_t,
> >> your compiler will complain.
> >
> > Such a compiler will also complain over most of qemu code.
> 
> Yes, but it's good to see that QEMU's code is
> improving.
> 
> By the way, such a compiler is gcc when called with
> -Wconversion, so it is easy to see how much code
> is affected.

Didn't try it. So it will warn on
pci_set_word(pci_dev, 0x0, PCI_STATUS)
but not
pci_set_word(pci_dev, PCI_STATUS, 0x0)
?

> >
> >> And even if your compiler
> >> does not complain, it is wrong but still safe, because
> >> the code will only access the PCI configuration data.
> >>
> >
> > Correct and safe beats wrong and safe every time.
> 
> See example above.

Example above tries to show that usng pointers in C is bad, switching to
indexes all over is proposed as a solution.  Oh well .. but I am
surpised you bring up type safety as an argument, is is exactly the
reason to use pointers.

> >
> >>>> * many other pci_* functions also have a first parameter of type
> >>>> PCIDevice
> >>> So what would make sense IMO is higer level abstraction,
> >>> for example similar to what we have with capabilities
> >>> and msix, I think we could have something like this
> >>> for e.g. power management.
> >>>
> >>> For low-level bit tweaking, the advantages of current API is that same
> >>> thing can be used to set wmask, cmask, config itself, and whatever else
> >>> we will come up with.
> >> The low level API can be used where low level is
> >> adequate: in pci.c for example.
> >>
> >> To implement emulated PCI devices, a more robust API
> >> would be better. Think of the number of devices which
> >> are still missing, think of people who want to write
> >> a new PCI device emulation for QEMU without being
> >> a QEMU expert.
> >>
> >>>> * calls look nicer (at least in my opinion)
> >>> What I value is the fact that it's obvious which
> >>> data is changed.
> >> Here there is no difference between current and
> >> proposed API:
> >>
> >> old: pci_set_word(&pci_conf[PCI_STATUS], PCI_STATUS_CAP_LIST);
> >> new: pci_set_word(pci_conf, PCI_STATUS, PCI_STATUS_CAP_LIST);
> >>
> >> Every function call hides what happens. If you really wanted
> >> to see which data is changed, you would have to write
> >>
> >> *(uint16_t *)&pci_conf[PCI_STATUS] = cpu_to_le16(PCI_STATUS_CAP_LIST);
> >
> > That's what we used to have, and it's not all bad, but very verbose and
> > ugly.
> 
> Yes. I also prefer a function API.
> 
> >
> >>>> * strict range checking (offset is limited to 0...255, additional
> >>>> assertions possible - the old API is unsafe because it just takes
> >>>> a pointer)
> >>> I don't think we want to add return status, so there wouldn't
> >>> be a benefit to range checking as we can't act on it.
> >>> Anyway, it's very unusual to use anything but a constant
> >>> as an offset, so range errors are very uncommon.
> >> There is an implicit range checking in the proposed
> >> API because the offset is uint8_t, so it cannot
> >> exceed the range which is valid for configuration
> >> offsets.
> >
> > Oh, btw, this is wrong on pci express.
> 
> Great, so PCI express devices should have their own
> set of functions with the correct runtime checks:
> 
> pci_e_set_config, ...

Oh no.

> >
> >> A more elaborated check could require that
> >> configuration byte values are only addressed
> >> using pci_set_byte (not pci_set_long)
> >> and raise a fatal runtime error otherwise.
> >>
> >> Runtime checks without return values
> >> are well established in QEMU's code,
> >> and they are very useful for code writers.
> >>
> >>>> The functions are inline, so the resulting code won't differ.
> >>>>
> >>>> Instead of _byte, _word and _long I personally prefer something
> >>>> like _8, _16, _32 because _word and _long need interpretation.
> >>>> But this is only a matter of taste - the API change is more important.
> >>>>
> >>>>
> >>>> Regards,
> >>>>
> >>>> Stefan Weil

Sorry, I dislike the API you propose and I do not think it will help
avoid bugs.

-- 
MST

[Qemu-devel] Re: [RFC] API change for pci_set_word and related functions

[Anthony Liguori]

On 01/11/2010 02:30 PM, Michael S. Tsirkin wrote:
> On Mon, Jan 11, 2010 at 09:18:51PM +0100, Stefan Weil wrote:
>    
>> Michael S. Tsirkin schrieb:
>>      
>>> On Mon, Jan 11, 2010 at 08:38:53PM +0100, Stefan Weil wrote:
>>>        
>>>> Michael S. Tsirkin schrieb:
>>>>          
>>>>> On Thu, Jan 07, 2010 at 04:07:26PM +0100, Stefan Weil wrote:
>>>>>            
>>>>>> Michael S. Tsirkin schrieb:
>>>>>>              
>>>>>>> On Thu, Jan 07, 2010 at 12:15:25PM +0100, Stefan Weil wrote:
>>>>>>> ...
>>>>>>>                
>>>>>>>> - PCI_CONFIG_16(PCI_STATUS,
>>>>>>>> - PCI_STATUS_REC_MASTER_ABORT | PCI_STATUS_SIG_TARGET_ABORT);
>>>>>>>> + PCI_CONFIG_16(PCI_STATUS, PCI_STATUS_DEVSEL_MEDIUM |
>>>>>>>> PCI_STATUS_FAST_BACK);
>>>>>>>> /* PCI Revision ID */
>>>>>>>> PCI_CONFIG_8(PCI_REVISION_ID, 0x08);
>>>>>>>>                  
>>>>>>> BTW if you are not afraid of churn, there's no reason
>>>>>>> for PCI_CONFIG_8 and friends anymore, because pci.h
>>>>>>> has much nicer pci_set_byte etc.
>>>>>>>                
>>>>>> Hello Michael,
>>>>>>
>>>>>> I already noticed pci_set_byte, pci_set_word, pci_set_long and
>>>>>> the corresponding pci_get_xxx functions and thought about using them.
>>>>>>
>>>>>> I did not start it because I want to suggest a different API
>>>>>> for use in PCI device emulations:
>>>>>>
>>>>>> instead of
>>>>>>
>>>>>> pci_set_word(pci_conf + PCI_STATUS, PCI_STATUS_CAP_LIST);
>>>>>>
>>>>>> or
>>>>>>
>>>>>> pci_set_word(&pci_conf[PCI_STATUS], PCI_STATUS_CAP_LIST);
>>>>>>
>>>>>> it would be better to call
>>>>>>
>>>>>> pci_set_word(pci_conf, PCI_STATUS, PCI_STATUS_CAP_LIST);
>>>>>>
>>>>>>
>>>>>> The prototypes would look like this:
>>>>>>
>>>>>> /* Set PCI config value. */
>>>>>> void pci_set_word(PCIDevice *s, uint8_t offset, uint16_t val);
>>>>>>
>>>>>> /* Set PCI cmask value. */
>>>>>> void pci_set_cmask_word(PCIDevice *s, uint8_t offset, uint16_t val);
>>>>>>
>>>>>> /* Set PCI wmask value. */
>>>>>> void pci_set_wmask_word(PCIDevice *s, uint8_t offset, uint16_t val);
>>>>>>
>>>>>> What are the advantages?
>>>>>> * strict type checking (the old API takes any uint8_t *)
>>>>>>              
>>>>> So IMO it's easier to make mistakes with your proposed API because if
>>>>> you confuse offset and value compiler does not complain. More
>>>>> importantly, if you want to pass some other uint8_t pointer to e.g.
>>>>> pci_set_word, you can *and nothing unexpected will happen*
>>>>> it will set the word to the given value. So the current
>>>>> API is more type safe than what you propose.
>>>>>
>>>>>            
>>>> No. The current API takes any uint8_t pointer to read or write
>>>> a value. This is not safe.
>>>>          
>>> Why isn't it?
>>>        
>> It is not safe, because it allows programmers to write silly code
>> like these examples:
>>
>> pci_set_word(&gen_opc_cc_op[PCI_STATUS], PCI_STATUS_CAP_LIST);
>>      
> This might be valid and useful for all I know.
>
>    
>> pci_set_word(&pci_conf[UINT32_MAX], PCI_STATUS_CAP_LIST);
>>
>> for (i = 0; i<  sizeof(pci_conf); i++) {
>>      pci_set_long(&pci_conf[i], 0);
>> }
>>
>> All three will result in runtime failures which can be very
>> difficult to detect.
>>      
> In theory. In practice I expect pci_set_word(pci_dev, 0x0, PCI_STATUS)
> to be much more common with your proposed API. Just look how
> common swapping memset parameters is.
>
>    
>>>        
>>>> The proposed API only takes a PCIDevice pointer
>>>> and reads or writes only configuration (or cmask or
>>>> wmask) values. Yes, you can take offset for value.
>>>> If you are lucky and value is an uint16_t or uint32_t,
>>>> your compiler will complain.
>>>>          
>>> Such a compiler will also complain over most of qemu code.
>>>        
>> Yes, but it's good to see that QEMU's code is
>> improving.
>>
>> By the way, such a compiler is gcc when called with
>> -Wconversion, so it is easy to see how much code
>> is affected.
>>      
> Didn't try it. So it will warn on
> pci_set_word(pci_dev, 0x0, PCI_STATUS)
> but not
> pci_set_word(pci_dev, PCI_STATUS, 0x0)
> ?
>    

I haven't read this whole thread, but I really prefer things like

pci_set_vendor_id(pci_dev, XXXX);

A close alternative, would be some refactoring to allow PCI config space 
to be represented as a C structure.  Gerd had some patches at one point 
for this.

Regards,

Anthony Liguori

[Qemu-devel] Re: [RFC] API change for pci_set_word and related functions

[Stefan Weil]

Anthony Liguori schrieb:
> On 01/11/2010 02:30 PM, Michael S. Tsirkin wrote:
>> On Mon, Jan 11, 2010 at 09:18:51PM +0100, Stefan Weil wrote:
>>   
>>> Michael S. Tsirkin schrieb:
>>>     
>>>> On Mon, Jan 11, 2010 at 08:38:53PM +0100, Stefan Weil wrote:
>>>>       
>>>>> Michael S. Tsirkin schrieb:
>>>>>         
>>>>>> On Thu, Jan 07, 2010 at 04:07:26PM +0100, Stefan Weil wrote:
>>>>>>           
>>>>>>> Michael S. Tsirkin schrieb:
>>>>>>>             
>>>>>>>> On Thu, Jan 07, 2010 at 12:15:25PM +0100, Stefan Weil wrote:
>>>>>>>> ...
>>>>>>>>               
>>>>>>>>> - PCI_CONFIG_16(PCI_STATUS,
>>>>>>>>> - PCI_STATUS_REC_MASTER_ABORT | PCI_STATUS_SIG_TARGET_ABORT);
>>>>>>>>> + PCI_CONFIG_16(PCI_STATUS, PCI_STATUS_DEVSEL_MEDIUM |
>>>>>>>>> PCI_STATUS_FAST_BACK);
>>>>>>>>> /* PCI Revision ID */
>>>>>>>>> PCI_CONFIG_8(PCI_REVISION_ID, 0x08);
>>>>>>>>>                  
>>>>>>>> BTW if you are not afraid of churn, there's no reason
>>>>>>>> for PCI_CONFIG_8 and friends anymore, because pci.h
>>>>>>>> has much nicer pci_set_byte etc.
>>>>>>>>                
>>>>>>> Hello Michael,
>>>>>>>
>>>>>>> I already noticed pci_set_byte, pci_set_word, pci_set_long and
>>>>>>> the corresponding pci_get_xxx functions and thought about using
>>>>>>> them.
>>>>>>>
>>>>>>> I did not start it because I want to suggest a different API
>>>>>>> for use in PCI device emulations:
>>>>>>>
>>>>>>> instead of
>>>>>>>
>>>>>>> pci_set_word(pci_conf + PCI_STATUS, PCI_STATUS_CAP_LIST);
>>>>>>>
>>>>>>> or
>>>>>>>
>>>>>>> pci_set_word(&pci_conf[PCI_STATUS], PCI_STATUS_CAP_LIST);
>>>>>>>
>>>>>>> it would be better to call
>>>>>>>
>>>>>>> pci_set_word(pci_conf, PCI_STATUS, PCI_STATUS_CAP_LIST);
>>>>>>>
>>>>>>>
>>>>>>> The prototypes would look like this:
>>>>>>>
>>>>>>> /* Set PCI config value. */
>>>>>>> void pci_set_word(PCIDevice *s, uint8_t offset, uint16_t val);
>>>>>>>
>>>>>>> /* Set PCI cmask value. */
>>>>>>> void pci_set_cmask_word(PCIDevice *s, uint8_t offset, uint16_t
>>>>>>> val);
>>>>>>>
>>>>>>> /* Set PCI wmask value. */
>>>>>>> void pci_set_wmask_word(PCIDevice *s, uint8_t offset, uint16_t
>>>>>>> val);
>>>>>>>
>>>>>>> What are the advantages?
>>>>>>> * strict type checking (the old API takes any uint8_t *)
>>>>>>>              
>>>>>> So IMO it's easier to make mistakes with your proposed API
>>>>>> because if
>>>>>> you confuse offset and value compiler does not complain. More
>>>>>> importantly, if you want to pass some other uint8_t pointer to e.g.
>>>>>> pci_set_word, you can *and nothing unexpected will happen*
>>>>>> it will set the word to the given value. So the current
>>>>>> API is more type safe than what you propose.
>>>>>>
>>>>>>            
>>>>> No. The current API takes any uint8_t pointer to read or write
>>>>> a value. This is not safe.
>>>>>          
>>>> Why isn't it?
>>>>        
>>> It is not safe, because it allows programmers to write silly code
>>> like these examples:
>>>
>>> pci_set_word(&gen_opc_cc_op[PCI_STATUS], PCI_STATUS_CAP_LIST);
>>>      
>> This might be valid and useful for all I know.
>>
>>   
>>> pci_set_word(&pci_conf[UINT32_MAX], PCI_STATUS_CAP_LIST);
>>>
>>> for (i = 0; i<  sizeof(pci_conf); i++) {
>>>      pci_set_long(&pci_conf[i], 0);
>>> }
>>>
>>> All three will result in runtime failures which can be very
>>> difficult to detect.
>>>      
>> In theory. In practice I expect pci_set_word(pci_dev, 0x0, PCI_STATUS)
>> to be much more common with your proposed API. Just look how
>> common swapping memset parameters is.
>>
>>   
>>>>       
>>>>> The proposed API only takes a PCIDevice pointer
>>>>> and reads or writes only configuration (or cmask or
>>>>> wmask) values. Yes, you can take offset for value.
>>>>> If you are lucky and value is an uint16_t or uint32_t,
>>>>> your compiler will complain.
>>>>>          
>>>> Such a compiler will also complain over most of qemu code.
>>>>        
>>> Yes, but it's good to see that QEMU's code is
>>> improving.
>>>
>>> By the way, such a compiler is gcc when called with
>>> -Wconversion, so it is easy to see how much code
>>> is affected.
>>>      
>> Didn't try it. So it will warn on
>> pci_set_word(pci_dev, 0x0, PCI_STATUS)
>> but not
>> pci_set_word(pci_dev, PCI_STATUS, 0x0)
>> ?

Only code which reduces precision would result in a warning:
pci_set_word(pci_dev, 0x4711, PCI_STATUS)

>>    
>
> I haven't read this whole thread, but I really prefer things like
>
> pci_set_vendor_id(pci_dev, XXXX);
>
> A close alternative, would be some refactoring to allow PCI config
> space to be represented as a C structure.  Gerd had some patches at
> one point for this.
>
> Regards,
>
> Anthony Liguori

This is a good solution for the standard configuration entries,
so most code could use such calls if they were complete.

For entries above offset 0x40, I'm afraid that it won't work
(neither with individual functions nor with a C structure).

Regards,

Stefan Weil

[Qemu-devel] Re: [RFC] API change for pci_set_word and related functions

[Anthony Liguori]

On 01/11/2010 04:10 PM, Stefan Weil wrote:
>> I haven't read this whole thread, but I really prefer things like
>>
>> pci_set_vendor_id(pci_dev, XXXX);
>>
>> A close alternative, would be some refactoring to allow PCI config
>> space to be represented as a C structure.  Gerd had some patches at
>> one point for this.
>>
>> Regards,
>>
>> Anthony Liguori
>>      
> This is a good solution for the standard configuration entries,
> so most code could use such calls if they were complete.
>
> For entries above offset 0x40, I'm afraid that it won't work
> (neither with individual functions nor with a C structure).
>    

Which is fine. Device specific entries in the config space are uncommon 
compared to the standard entries.  The same model could apply (devices 
write their own wrapper functions).

The problem with the set_word interface is that quite a lot of important 
config space fields are stored in a byte or less.

Regards,

Anthony Liguori

> Regards,
>
> Stefan Weil
>
>