[Qemu-devel] [PATCH V9 0/8] memory: unify ioport registration

[Qemu-devel] [PATCH V9 0/8] memory: unify ioport registration

[Julien Grall]

This is the nineth version of patch series about ioport registration.

Some part of QEMU still use register_ioport* functions to register ioport.
These functions doesn't allow to use Memory Listener on it.

Modifications between V1 and V2:
   - Remove the use of get_system_io. Instead of use isa and pci IO
     address space;
   - Avoid allocation of PortioList. Use the different device
     structure;
   - Still remove register_ioport* (hw/dma.c, hw/apm.c, hw/acpi_piix4.c);
   - Use MemoryRegion when we have only a range of ioport;
   - For some functions, add IO address space as argument;
   - Add isa_address_space_io function.

Modifications between V2 and V3:
   - Remove some register_ioport_* on hw/vt82c686.c;
   - Split smb ioport part in new patch;
   - Still replace MemoryRegion when we have only a range of ioport;
   - Fix read/write ioports prototype to  be compliant with memory callback.

Modifications between V3 and V4:
   - Fix compilation in hw/dma.c;
   - Fix address conversion (hw/dma.c, hw/acpi_piix4.c) with MemorySection.
     Indeed the new version use offset from MemorySection start instead of 0.

Modifications between V4 and V5:
   - Rebase on qemu upstream;
   - Forget some ioport_register_* in acpi_piix4.c;
   - Register 0x3b0 - 0x3df range for cirrus instead of ioport by ioport.

Modifications between V5 and V6:
   - Add read function on cirrus ioport (forget on the previous patch);
   - Rework PM memory range handling;
   - Fix PCI_BASE in acpi_piix4.c (wrong conversion during port);
   - Rewrite isa_address_space_io to use ISA bus address space;
   - Fix compilation in vt82c686.c.

Modifications between V6 and V7:
   - acpi_piix4: use memory_region_set_enabled instead of a boolean. I'm not
     sure about this modification (adviced by Avi);
   - Fix device endianness in acpi_piix4 (reported by Avi);
   - Avoid dependencies between patches and reorder it (reported by Jan).
     Some code moved from acpi_piix4 patch to smb/apm patches;

Modifications between V7 and V8:
   - Fix device endianness in smb patch, I forgot some on previous version;
   - Register pm io region at initialization with default value instead
     of at first call to pm_io_space_update (reported by Jan).

Modifications between V8 and V9:
   - Fix size constraint in pm_io_ops (reported by Jan).

Julien Grall (8):
  isa: add isa_address_space_io
  hw/apm.c: replace register_ioport*
  smb: replace_register_ioport*
  hw/acpi_piix4.c: replace register_ioport*
  hw/cirrus_vga.c: replace register_ioport*
  hw/serial.c: replace register_ioport*
  hw/pc.c: replace register_ioport*
  hw/dma.c: replace register_ioport*

 hw/acpi_piix4.c   |  165 +++++++++++++++++++++++++++++++++++++++++-----------
 hw/apm.c          |   24 ++++++--
 hw/apm.h          |    5 +-
 hw/cirrus_vga.c   |   50 ++++++++++-------
 hw/dma.c          |  108 +++++++++++++++++++++++------------
 hw/isa-bus.c      |    9 +++
 hw/isa.h          |    1 +
 hw/mips_mipssim.c |    3 +-
 hw/pc.c           |   58 ++++++++++++++-----
 hw/pc.h           |    2 +-
 hw/pm_smbus.c     |    7 +-
 hw/pm_smbus.h     |    6 +-
 hw/serial.c       |    8 ++-
 hw/vt82c686.c     |   20 ++++++-
 14 files changed, 341 insertions(+), 125 deletions(-)

-- 
Julien Grall

[Qemu-devel] [PATCH V9 1/8] isa: add isa_address_space_io

[Julien Grall]

This function permits to retrieve ISA IO address space.
It will be usefull when we need to pass IO address space as argument.

Signed-off-by: Julien Grall <julien.grall@citrix.com>
---
 hw/isa-bus.c |    9 +++++++++
 hw/isa.h     |    1 +
 2 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/hw/isa-bus.c b/hw/isa-bus.c
index f9b2373..c1d8309 100644
--- a/hw/isa-bus.c
+++ b/hw/isa-bus.c
@@ -244,4 +244,13 @@ MemoryRegion *isa_address_space(ISADevice *dev)
     return get_system_memory();
 }
 
+MemoryRegion *isa_address_space_io(ISADevice *dev)
+{
+    if (dev) {
+        return isa_bus_from_device(dev)->address_space_io;
+    }
+
+    return isabus->address_space_io;
+}
+
 type_init(isabus_register_types)
diff --git a/hw/isa.h b/hw/isa.h
index dc97052..3891c1f 100644
--- a/hw/isa.h
+++ b/hw/isa.h
@@ -43,6 +43,7 @@ void isa_bus_irqs(ISABus *bus, qemu_irq *irqs);
 qemu_irq isa_get_irq(ISADevice *dev, int isairq);
 void isa_init_irq(ISADevice *dev, qemu_irq *p, int isairq);
 MemoryRegion *isa_address_space(ISADevice *dev);
+MemoryRegion *isa_address_space_io(ISADevice *dev);
 ISADevice *isa_create(ISABus *bus, const char *name);
 ISADevice *isa_try_create(ISABus *bus, const char *name);
 ISADevice *isa_create_simple(ISABus *bus, const char *name);
-- 
Julien Grall

[Qemu-devel] [PATCH V9 2/8] hw/apm.c: replace register_ioport*

[Julien Grall]

This patch replaces all register_ioport* by a MemorySection.
It permits to use the new Memory stuff like listener.

Moreover, the PCI is added as an argument for apm_init, so we
can register IO inside the pci IO address space.

Signed-off-by: Julien Grall <julien.grall@citrix.com>
---
 hw/acpi_piix4.c |    2 +-
 hw/apm.c        |   24 +++++++++++++++++++-----
 hw/apm.h        |    5 ++++-
 hw/vt82c686.c   |    2 +-
 4 files changed, 25 insertions(+), 8 deletions(-)

diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
index c56220b..fd2fc33 100644
--- a/hw/acpi_piix4.c
+++ b/hw/acpi_piix4.c
@@ -395,7 +395,7 @@ static int piix4_pm_initfn(PCIDevice *dev)
     pci_conf[0x3d] = 0x01; // interrupt pin 1
 
     /* APM */
-    apm_init(&s->apm, apm_ctrl_changed, s);
+    apm_init(dev, &s->apm, apm_ctrl_changed, s);
 
     register_ioport_write(ACPI_DBG_IO_ADDR, 4, 4, acpi_dbg_writel, s);
 
diff --git a/hw/apm.c b/hw/apm.c
index 2aead52..fe7bc21 100644
--- a/hw/apm.c
+++ b/hw/apm.c
@@ -22,6 +22,7 @@
 
 #include "apm.h"
 #include "hw.h"
+#include "pci.h"
 
 //#define DEBUG
 
@@ -35,7 +36,8 @@
 #define APM_CNT_IOPORT  0xb2
 #define APM_STS_IOPORT  0xb3
 
-static void apm_ioport_writeb(void *opaque, uint32_t addr, uint32_t val)
+static void apm_ioport_writeb(void *opaque, target_phys_addr_t addr,
+                              uint64_t val, unsigned size)
 {
     APMState *apm = opaque;
     addr &= 1;
@@ -51,7 +53,8 @@ static void apm_ioport_writeb(void *opaque, uint32_t addr, uint32_t val)
     }
 }
 
-static uint32_t apm_ioport_readb(void *opaque, uint32_t addr)
+static uint64_t apm_ioport_readb(void *opaque, target_phys_addr_t addr,
+                                 unsigned size)
 {
     APMState *apm = opaque;
     uint32_t val;
@@ -78,12 +81,23 @@ const VMStateDescription vmstate_apm = {
     }
 };
 
-void apm_init(APMState *apm, apm_ctrl_changed_t callback, void *arg)
+static const MemoryRegionOps apm_ops = {
+    .read = apm_ioport_readb,
+    .write = apm_ioport_writeb,
+    .impl = {
+        .min_access_size = 1,
+        .max_access_size = 1,
+    },
+};
+
+void apm_init(PCIDevice *dev, APMState *apm, apm_ctrl_changed_t callback,
+              void *arg)
 {
     apm->callback = callback;
     apm->arg = arg;
 
     /* ioport 0xb2, 0xb3 */
-    register_ioport_write(APM_CNT_IOPORT, 2, 1, apm_ioport_writeb, apm);
-    register_ioport_read(APM_CNT_IOPORT, 2, 1, apm_ioport_readb, apm);
+    memory_region_init_io(&apm->io, &apm_ops, apm, "apm-io", 2);
+    memory_region_add_subregion(pci_address_space_io(dev), APM_CNT_IOPORT,
+                                &apm->io);
 }
diff --git a/hw/apm.h b/hw/apm.h
index f7c741e..5431b6d 100644
--- a/hw/apm.h
+++ b/hw/apm.h
@@ -4,6 +4,7 @@
 #include <stdint.h>
 #include "qemu-common.h"
 #include "hw.h"
+#include "memory.h"
 
 typedef void (*apm_ctrl_changed_t)(uint32_t val, void *arg);
 
@@ -13,9 +14,11 @@ typedef struct APMState {
 
     apm_ctrl_changed_t callback;
     void *arg;
+    MemoryRegion io;
 } APMState;
 
-void apm_init(APMState *s, apm_ctrl_changed_t callback, void *arg);
+void apm_init(PCIDevice *dev, APMState *s, apm_ctrl_changed_t callback,
+              void *arg);
 
 extern const VMStateDescription vmstate_apm;
 
diff --git a/hw/vt82c686.c b/hw/vt82c686.c
index 5d7c00c..7f11dbe 100644
--- a/hw/vt82c686.c
+++ b/hw/vt82c686.c
@@ -427,7 +427,7 @@ static int vt82c686b_pm_initfn(PCIDevice *dev)
     register_ioport_write(s->smb_io_base, 0xf, 1, smb_ioport_writeb, &s->smb);
     register_ioport_read(s->smb_io_base, 0xf, 1, smb_ioport_readb, &s->smb);
 
-    apm_init(&s->apm, NULL, s);
+    apm_init(dev, &s->apm, NULL, s);
 
     acpi_pm_tmr_init(&s->ar, pm_tmr_timer);
     acpi_pm1_cnt_init(&s->ar);
-- 
Julien Grall

[Qemu-devel] [PATCH V9 3/8] smb: replace_register_ioport*

[Julien Grall]

This patch fix smb_ioport_* to be compliant with read/write memory callback.
Moreover it replaces all register_ioport* which use theses functions by
the new Memory API.

Signed-off-by: Julien Grall <julien.grall@citrix.com>
---
 hw/acpi_piix4.c |   18 ++++++++++++++++--
 hw/pm_smbus.c   |    7 ++++---
 hw/pm_smbus.h   |    6 ++++--
 hw/vt82c686.c   |   18 ++++++++++++++++--
 4 files changed, 40 insertions(+), 9 deletions(-)

diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
index fd2fc33..0b4ad24 100644
--- a/hw/acpi_piix4.c
+++ b/hw/acpi_piix4.c
@@ -63,6 +63,8 @@ typedef struct PIIX4PMState {
     PMSMBus smb;
     uint32_t smb_io_base;
 
+    MemoryRegion smb_io;
+
     qemu_irq irq;
     qemu_irq smi_irq;
     int kvm_enabled;
@@ -383,6 +385,16 @@ static void piix4_pm_machine_ready(Notifier *n, void *opaque)
 
 }
 
+static const MemoryRegionOps smb_io_ops = {
+    .read = smb_ioport_readb,
+    .write = smb_ioport_writeb,
+    .endianness = DEVICE_LITTLE_ENDIAN,
+    .impl = {
+        .min_access_size = 1,
+        .max_access_size = 1,
+    },
+};
+
 static int piix4_pm_initfn(PCIDevice *dev)
 {
     PIIX4PMState *s = DO_UPCAST(PIIX4PMState, dev, dev);
@@ -410,8 +422,10 @@ static int piix4_pm_initfn(PCIDevice *dev)
     pci_conf[0x90] = s->smb_io_base | 1;
     pci_conf[0x91] = s->smb_io_base >> 8;
     pci_conf[0xd2] = 0x09;
-    register_ioport_write(s->smb_io_base, 64, 1, smb_ioport_writeb, &s->smb);
-    register_ioport_read(s->smb_io_base, 64, 1, smb_ioport_readb, &s->smb);
+
+    memory_region_init_io(&s->smb_io, &smb_io_ops, &s->smb, "piix4-smb", 64);
+    memory_region_add_subregion(pci_address_space_io(dev), s->smb_io_base,
+                                &s->smb_io);
 
     acpi_pm_tmr_init(&s->ar, pm_tmr_timer);
     acpi_gpe_init(&s->ar, GPE_LEN);
diff --git a/hw/pm_smbus.c b/hw/pm_smbus.c
index 5d6046d..fe59ca6 100644
--- a/hw/pm_smbus.c
+++ b/hw/pm_smbus.c
@@ -94,7 +94,8 @@ static void smb_transaction(PMSMBus *s)
     s->smb_stat |= 0x04;
 }
 
-void smb_ioport_writeb(void *opaque, uint32_t addr, uint32_t val)
+void smb_ioport_writeb(void *opaque, target_phys_addr_t addr, uint64_t val,
+                       unsigned size)
 {
     PMSMBus *s = opaque;
     addr &= 0x3f;
@@ -131,10 +132,10 @@ void smb_ioport_writeb(void *opaque, uint32_t addr, uint32_t val)
     }
 }
 
-uint32_t smb_ioport_readb(void *opaque, uint32_t addr)
+uint64_t smb_ioport_readb(void *opaque, target_phys_addr_t addr, unsigned size)
 {
     PMSMBus *s = opaque;
-    uint32_t val;
+    uint64_t val;
 
     addr &= 0x3f;
     switch(addr) {
diff --git a/hw/pm_smbus.h b/hw/pm_smbus.h
index 4750a40..45b4330 100644
--- a/hw/pm_smbus.h
+++ b/hw/pm_smbus.h
@@ -15,7 +15,9 @@ typedef struct PMSMBus {
 } PMSMBus;
 
 void pm_smbus_init(DeviceState *parent, PMSMBus *smb);
-void smb_ioport_writeb(void *opaque, uint32_t addr, uint32_t val);
-uint32_t smb_ioport_readb(void *opaque, uint32_t addr);
+void smb_ioport_writeb(void *opaque, target_phys_addr_t addr, uint64_t val,
+                       unsigned size);
+uint64_t smb_ioport_readb(void *opaque, target_phys_addr_t addr,
+                          unsigned size);
 
 #endif /* !PM_SMBUS_H */
diff --git a/hw/vt82c686.c b/hw/vt82c686.c
index 7f11dbe..f8d79d5 100644
--- a/hw/vt82c686.c
+++ b/hw/vt82c686.c
@@ -163,6 +163,7 @@ typedef struct VT686PMState {
     APMState apm;
     PMSMBus smb;
     uint32_t smb_io_base;
+    MemoryRegion smb_io;
 } VT686PMState;
 
 typedef struct VT686AC97State {
@@ -405,6 +406,16 @@ static TypeInfo via_mc97_info = {
     .class_init    = via_mc97_class_init,
 };
 
+static const MemoryRegionOps smb_io_ops = {
+    .read = smb_ioport_readb,
+    .write = smb_ioport_writeb,
+    .endianness = DEVICE_LITTLE_ENDIAN,
+    .impl = {
+        .min_access_size = 1,
+        .max_access_size = 1,
+    },
+};
+
 /* vt82c686 pm init */
 static int vt82c686b_pm_initfn(PCIDevice *dev)
 {
@@ -424,8 +435,11 @@ static int vt82c686b_pm_initfn(PCIDevice *dev)
     pci_conf[0x90] = s->smb_io_base | 1;
     pci_conf[0x91] = s->smb_io_base >> 8;
     pci_conf[0xd2] = 0x90;
-    register_ioport_write(s->smb_io_base, 0xf, 1, smb_ioport_writeb, &s->smb);
-    register_ioport_read(s->smb_io_base, 0xf, 1, smb_ioport_readb, &s->smb);
+
+    memory_region_init_io(&s->smb_io, &smb_io_ops, &s->smb, "vt82c686b-smb",
+                          0xf);
+    memory_region_add_subregion(pci_address_space_io(dev), s->smb_io_base,
+                                &s->smb_io);
 
     apm_init(dev, &s->apm, NULL, s);
 
-- 
Julien Grall

[Qemu-devel] [PATCH V9 4/8] hw/acpi_piix4.c: replace register_ioport*

[Julien Grall]

This patch replaces all register_ioport* with the new memory API. It permits
to use the new Memory stuff like listener.

Signed-off-by: Julien Grall <julien.grall@citrix.com>
---
 hw/acpi_piix4.c |  145 +++++++++++++++++++++++++++++++++++++++++++------------
 1 files changed, 113 insertions(+), 32 deletions(-)

diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
index 0b4ad24..2400a35 100644
--- a/hw/acpi_piix4.c
+++ b/hw/acpi_piix4.c
@@ -41,10 +41,10 @@
 
 #define GPE_BASE 0xafe0
 #define GPE_LEN 4
-#define PCI_UP_BASE 0xae00
-#define PCI_DOWN_BASE 0xae04
+#define PCI_BASE 0xae00
 #define PCI_EJ_BASE 0xae08
 #define PCI_RMV_BASE 0xae0c
+#define PM_BASE 0x00
 
 #define PIIX4_PCI_HOTPLUG_STATUS 2
 
@@ -55,7 +55,7 @@ struct pci_status {
 
 typedef struct PIIX4PMState {
     PCIDevice dev;
-    IORange ioport;
+    MemoryRegion pm_io;
     ACPIREGS ar;
 
     APMState apm;
@@ -64,6 +64,11 @@ typedef struct PIIX4PMState {
     uint32_t smb_io_base;
 
     MemoryRegion smb_io;
+    MemoryRegion acpi_io;
+    MemoryRegion acpi_hot_io;
+    PortioList pci_hot_port_list;
+    MemoryRegion pciej_hot_io;
+    MemoryRegion pcirmv_hot_io;
 
     qemu_irq irq;
     qemu_irq smi_irq;
@@ -110,10 +115,10 @@ static void pm_tmr_timer(ACPIREGS *ar)
     pm_update_sci(s);
 }
 
-static void pm_ioport_write(IORange *ioport, uint64_t addr, unsigned width,
-                            uint64_t val)
+static void pm_ioport_write(void *opaque, target_phys_addr_t addr,
+                            uint64_t val, unsigned width)
 {
-    PIIX4PMState *s = container_of(ioport, PIIX4PMState, ioport);
+    PIIX4PMState *s = opaque;
 
     if (width != 2) {
         PIIX4_DPRINTF("PM write port=0x%04x width=%d val=0x%08x\n",
@@ -139,11 +144,11 @@ static void pm_ioport_write(IORange *ioport, uint64_t addr, unsigned width,
                   (unsigned int)val);
 }
 
-static void pm_ioport_read(IORange *ioport, uint64_t addr, unsigned width,
-                            uint64_t *data)
+static uint64_t pm_ioport_read(void *opaque, target_phys_addr_t addr,
+                               unsigned width)
 {
-    PIIX4PMState *s = container_of(ioport, PIIX4PMState, ioport);
-    uint32_t val;
+    PIIX4PMState *s = opaque;
+    uint64_t val;
 
     switch(addr) {
     case 0x00:
@@ -163,12 +168,18 @@ static void pm_ioport_read(IORange *ioport, uint64_t addr, unsigned width,
         break;
     }
     PIIX4_DPRINTF("PM readw port=0x%04x val=0x%04x\n", (unsigned int)addr, val);
-    *data = val;
+
+    return val;
 }
 
-static const IORangeOps pm_iorange_ops = {
+static const MemoryRegionOps pm_io_ops = {
     .read = pm_ioport_read,
     .write = pm_ioport_write,
+    .endianness = DEVICE_LITTLE_ENDIAN,
+    .impl = {
+        .min_access_size = 1,
+        .max_access_size = 4,
+    },
 };
 
 static void apm_ctrl_changed(uint32_t val, void *arg)
@@ -185,7 +196,8 @@ static void apm_ctrl_changed(uint32_t val, void *arg)
     }
 }
 
-static void acpi_dbg_writel(void *opaque, uint32_t addr, uint32_t val)
+static void acpi_dbg_writel(void *opaque, target_phys_addr_t addr,
+                            uint64_t val, unsigned size)
 {
     PIIX4_DPRINTF("ACPI: DBG: 0x%08x\n", val);
 }
@@ -200,8 +212,11 @@ static void pm_io_space_update(PIIX4PMState *s)
 
         /* XXX: need to improve memory and ioport allocation */
         PIIX4_DPRINTF("PM: mapping to 0x%x\n", pm_io_base);
-        iorange_init(&s->ioport, &pm_iorange_ops, pm_io_base, 64);
-        ioport_register(&s->ioport);
+
+        memory_region_set_address(&s->pm_io, pm_io_base);
+        memory_region_set_enabled(&s->pm_io, true);
+    } else {
+        memory_region_set_enabled(&s->pm_io, false);
     }
 }
 
@@ -395,6 +410,15 @@ static const MemoryRegionOps smb_io_ops = {
     },
 };
 
+static const MemoryRegionOps acpi_io_ops = {
+    .write = acpi_dbg_writel,
+    .endianness = DEVICE_LITTLE_ENDIAN,
+    .impl = {
+        .min_access_size = 4,
+        .max_access_size = 4,
+    },
+};
+
 static int piix4_pm_initfn(PCIDevice *dev)
 {
     PIIX4PMState *s = DO_UPCAST(PIIX4PMState, dev, dev);
@@ -409,7 +433,9 @@ static int piix4_pm_initfn(PCIDevice *dev)
     /* APM */
     apm_init(dev, &s->apm, apm_ctrl_changed, s);
 
-    register_ioport_write(ACPI_DBG_IO_ADDR, 4, 4, acpi_dbg_writel, s);
+    memory_region_init_io(&s->acpi_io, &acpi_io_ops, s, "piix4-acpi", 4);
+    memory_region_add_subregion(pci_address_space_io(dev), ACPI_DBG_IO_ADDR,
+                                &s->acpi_io);
 
     if (s->kvm_enabled) {
         /* Mark SMM as already inited to prevent SMM from running.  KVM does not
@@ -427,6 +453,12 @@ static int piix4_pm_initfn(PCIDevice *dev)
     memory_region_add_subregion(pci_address_space_io(dev), s->smb_io_base,
                                 &s->smb_io);
 
+    /* PM  */
+    memory_region_init_io(&s->pm_io, &pm_io_ops, s, "piix4-pm", 64);
+    memory_region_set_enabled(&s->pm_io, false);
+    memory_region_add_subregion(pci_address_space_io(&s->dev),
+                                PM_BASE, &s->pm_io);
+
     acpi_pm_tmr_init(&s->ar, pm_tmr_timer);
     acpi_gpe_init(&s->ar, GPE_LEN);
 
@@ -510,16 +542,17 @@ static void piix4_pm_register_types(void)
 
 type_init(piix4_pm_register_types)
 
-static uint32_t gpe_readb(void *opaque, uint32_t addr)
+static uint64_t gpe_readb(void *opaque, target_phys_addr_t addr, unsigned size)
 {
     PIIX4PMState *s = opaque;
-    uint32_t val = acpi_gpe_ioport_readb(&s->ar, addr);
+    uint64_t val = acpi_gpe_ioport_readb(&s->ar, addr);
 
     PIIX4_DPRINTF("gpe read %x == %x\n", addr, val);
     return val;
 }
 
-static void gpe_writeb(void *opaque, uint32_t addr, uint32_t val)
+static void gpe_writeb(void *opaque, target_phys_addr_t addr, uint64_t val,
+                       unsigned size)
 {
     PIIX4PMState *s = opaque;
 
@@ -551,21 +584,24 @@ static uint32_t pci_down_read(void *opaque, uint32_t addr)
     return val;
 }
 
-static uint32_t pci_features_read(void *opaque, uint32_t addr)
+static uint64_t pci_features_read(void *opaque, target_phys_addr_t addr,
+                                  unsigned size)
 {
     /* No feature defined yet */
     PIIX4_DPRINTF("pci_features_read %x\n", 0);
     return 0;
 }
 
-static void pciej_write(void *opaque, uint32_t addr, uint32_t val)
+static void pciej_write(void *opaque, target_phys_addr_t addr, uint64_t val,
+                        unsigned size)
 {
     acpi_piix_eject_slot(opaque, val);
 
     PIIX4_DPRINTF("pciej write %x <== %d\n", addr, val);
 }
 
-static uint32_t pcirmv_read(void *opaque, uint32_t addr)
+static uint64_t pcirmv_read(void *opaque, target_phys_addr_t addr,
+                            unsigned size)
 {
     PIIX4PMState *s = opaque;
 
@@ -575,20 +611,65 @@ static uint32_t pcirmv_read(void *opaque, uint32_t addr)
 static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev,
                                 PCIHotplugState state);
 
-static void piix4_acpi_system_hot_add_init(PCIBus *bus, PIIX4PMState *s)
-{
+static const MemoryRegionOps acpi_hot_io_ops = {
+    .read = gpe_readb,
+    .write = gpe_writeb,
+    .endianness = DEVICE_LITTLE_ENDIAN,
+    .impl = {
+        .min_access_size = 1,
+        .max_access_size = 1,
+    },
+};
+
+/* PCI hot plug registers */
+static const MemoryRegionPortio pci_hot_portio_list[] = {
+    { 0x00, 4, 4, .read = pci_up_read, }, /* 0xae00 */
+    { 0x04, 4, 4, .read = pci_down_read, }, /* 0xae04 */
+    PORTIO_END_OF_LIST(),
+};
 
-    register_ioport_write(GPE_BASE, GPE_LEN, 1, gpe_writeb, s);
-    register_ioport_read(GPE_BASE, GPE_LEN, 1,  gpe_readb, s);
-    acpi_gpe_blk(&s->ar, GPE_BASE);
+static const MemoryRegionOps pciej_hot_io_ops = {
+    .read = pci_features_read,
+    .write = pciej_write,
+    .endianness = DEVICE_LITTLE_ENDIAN,
+    .impl = {
+        .min_access_size = 4,
+        .max_access_size = 4,
+    },
+};
 
-    register_ioport_read(PCI_UP_BASE, 4, 4, pci_up_read, s);
-    register_ioport_read(PCI_DOWN_BASE, 4, 4, pci_down_read, s);
+static const MemoryRegionOps pcirmv_hot_io_ops = {
+    .read = pcirmv_read,
+    .endianness = DEVICE_NATIVE_ENDIAN,
+    .impl = {
+        .min_access_size = 4,
+        .max_access_size = 4,
+    },
+};
 
-    register_ioport_write(PCI_EJ_BASE, 4, 4, pciej_write, s);
-    register_ioport_read(PCI_EJ_BASE, 4, 4,  pci_features_read, s);
+static void piix4_acpi_system_hot_add_init(PCIBus *bus, PIIX4PMState *s)
+{
 
-    register_ioport_read(PCI_RMV_BASE, 4, 4,  pcirmv_read, s);
+    memory_region_init_io(&s->acpi_hot_io, &acpi_hot_io_ops, s,
+                          "piix4-acpi-hot", GPE_LEN);
+    memory_region_add_subregion(pci_address_space_io(&s->dev), GPE_BASE,
+                                &s->acpi_hot_io);
+    acpi_gpe_blk(&s->ar, 0);
+
+    portio_list_init(&s->pci_hot_port_list, pci_hot_portio_list, s,
+                     "piix4-pci-hot");
+    portio_list_add(&s->pci_hot_port_list, pci_address_space_io(&s->dev),
+                    PCI_BASE);
+
+    memory_region_init_io(&s->pciej_hot_io, &pciej_hot_io_ops, s,
+                          "piix4-pciej-hot", 4);
+    memory_region_add_subregion(pci_address_space_io(&s->dev), PCI_EJ_BASE,
+                                &s->pciej_hot_io);
+
+    memory_region_init_io(&s->pcirmv_hot_io, &pcirmv_hot_io_ops, s,
+                          "piix4-pcirmv-hot", 4);
+    memory_region_add_subregion(pci_address_space_io(&s->dev), PCI_RMV_BASE,
+                                &s->pcirmv_hot_io);
 
     pci_bus_hotplug(bus, piix4_device_hotplug, &s->dev.qdev);
 }
-- 
Julien Grall

[Qemu-devel] [PATCH V9 5/8] hw/cirrus_vga.c: replace register_ioport*

[Julien Grall]

This patch replaces all register_ioport* with portio_*. It permits to
use the new Memory stuff like listener.

Signed-off-by: Julien Grall <julien.grall@citrix.com>
---
 hw/cirrus_vga.c |   50 ++++++++++++++++++++++++++++++--------------------
 1 files changed, 30 insertions(+), 20 deletions(-)

diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
index e8dcc6b..aa81f0b 100644
--- a/hw/cirrus_vga.c
+++ b/hw/cirrus_vga.c
@@ -200,6 +200,7 @@ typedef void (*cirrus_fill_t)(struct CirrusVGAState *s,
 typedef struct CirrusVGAState {
     VGACommonState vga;
 
+    MemoryRegion cirrus_vga_io;
     MemoryRegion cirrus_linear_io;
     MemoryRegion cirrus_linear_bitblt_io;
     MemoryRegion cirrus_mmio_io;
@@ -2435,12 +2436,15 @@ static void cirrus_update_memory_access(CirrusVGAState *s)
 
 /* I/O ports */
 
-static uint32_t cirrus_vga_ioport_read(void *opaque, uint32_t addr)
+static uint64_t cirrus_vga_ioport_read(void *opaque, target_phys_addr_t addr,
+                                       unsigned size)
 {
     CirrusVGAState *c = opaque;
     VGACommonState *s = &c->vga;
     int val, index;
 
+    addr += 0x3b0;
+
     if (vga_ioport_invalid(s, addr)) {
 	val = 0xff;
     } else {
@@ -2528,12 +2532,15 @@ static uint32_t cirrus_vga_ioport_read(void *opaque, uint32_t addr)
     return val;
 }
 
-static void cirrus_vga_ioport_write(void *opaque, uint32_t addr, uint32_t val)
+static void cirrus_vga_ioport_write(void *opaque, target_phys_addr_t addr,
+                                    uint64_t val, unsigned size)
 {
     CirrusVGAState *c = opaque;
     VGACommonState *s = &c->vga;
     int index;
 
+    addr += 0x3b0;
+
     /* check port range access depending on color/monochrome mode */
     if (vga_ioport_invalid(s, addr)) {
 	return;
@@ -2645,7 +2652,7 @@ static uint64_t cirrus_mmio_read(void *opaque, target_phys_addr_t addr,
     if (addr >= 0x100) {
         return cirrus_mmio_blt_read(s, addr - 0x100);
     } else {
-        return cirrus_vga_ioport_read(s, addr + 0x3c0);
+        return cirrus_vga_ioport_read(s, addr + 0x10, size);
     }
 }
 
@@ -2657,7 +2664,7 @@ static void cirrus_mmio_write(void *opaque, target_phys_addr_t addr,
     if (addr >= 0x100) {
 	cirrus_mmio_blt_write(s, addr - 0x100, val);
     } else {
-        cirrus_vga_ioport_write(s, addr + 0x3c0, val);
+        cirrus_vga_ioport_write(s, addr + 0x10, val, size);
     }
 }
 
@@ -2783,8 +2790,19 @@ static const MemoryRegionOps cirrus_linear_io_ops = {
     },
 };
 
+static const MemoryRegionOps cirrus_vga_io_ops = {
+    .read = cirrus_vga_ioport_read,
+    .write = cirrus_vga_ioport_write,
+    .endianness = DEVICE_LITTLE_ENDIAN,
+    .impl = {
+        .min_access_size = 1,
+        .max_access_size = 1,
+    },
+};
+
 static void cirrus_init_common(CirrusVGAState * s, int device_id, int is_pci,
-                               MemoryRegion *system_memory)
+                               MemoryRegion *system_memory,
+                               MemoryRegion *system_io)
 {
     int i;
     static int inited;
@@ -2816,19 +2834,10 @@ static void cirrus_init_common(CirrusVGAState * s, int device_id, int is_pci,
             s->bustype = CIRRUS_BUSTYPE_ISA;
     }
 
-    register_ioport_write(0x3c0, 16, 1, cirrus_vga_ioport_write, s);
-
-    register_ioport_write(0x3b4, 2, 1, cirrus_vga_ioport_write, s);
-    register_ioport_write(0x3d4, 2, 1, cirrus_vga_ioport_write, s);
-    register_ioport_write(0x3ba, 1, 1, cirrus_vga_ioport_write, s);
-    register_ioport_write(0x3da, 1, 1, cirrus_vga_ioport_write, s);
-
-    register_ioport_read(0x3c0, 16, 1, cirrus_vga_ioport_read, s);
-
-    register_ioport_read(0x3b4, 2, 1, cirrus_vga_ioport_read, s);
-    register_ioport_read(0x3d4, 2, 1, cirrus_vga_ioport_read, s);
-    register_ioport_read(0x3ba, 1, 1, cirrus_vga_ioport_read, s);
-    register_ioport_read(0x3da, 1, 1, cirrus_vga_ioport_read, s);
+    /* Register ioport 0x3b0 - 0x3df */
+    memory_region_init_io(&s->cirrus_vga_io, &cirrus_vga_io_ops, s,
+                          "cirrus-io", 0x30);
+    memory_region_add_subregion(system_io, 0x3b0, &s->cirrus_vga_io);
 
     memory_region_init(&s->low_mem_container,
                        "cirrus-lowmem-container",
@@ -2896,7 +2905,7 @@ static int vga_initfn(ISADevice *dev)
     s->vram_size_mb = VGA_RAM_SIZE >> 20;
     vga_common_init(s);
     cirrus_init_common(&d->cirrus_vga, CIRRUS_ID_CLGD5430, 0,
-                       isa_address_space(dev));
+                       isa_address_space(dev), isa_address_space_io(dev));
     s->ds = graphic_console_init(s->update, s->invalidate,
                                  s->screen_dump, s->text_update,
                                  s);
@@ -2938,7 +2947,8 @@ static int pci_cirrus_vga_initfn(PCIDevice *dev)
      /* setup VGA */
      s->vga.vram_size_mb = VGA_RAM_SIZE >> 20;
      vga_common_init(&s->vga);
-     cirrus_init_common(s, device_id, 1, pci_address_space(dev));
+     cirrus_init_common(s, device_id, 1, pci_address_space(dev),
+                        pci_address_space_io(dev));
      s->vga.ds = graphic_console_init(s->vga.update, s->vga.invalidate,
                                       s->vga.screen_dump, s->vga.text_update,
                                       &s->vga);
-- 
Julien Grall

[Qemu-devel] [PATCH V9 6/8] hw/serial.c: replace register_ioport*

[Julien Grall]

This patch replaces all register_ioport* with a MemoryRegion. It permits to
use the new Memory stuff like listener.

For more flexibility, the IO address space is passed as an argument.

Signed-off-by: Julien Grall <julien.grall@citrix.com>
---
 hw/mips_mipssim.c |    3 ++-
 hw/pc.h           |    2 +-
 hw/serial.c       |    8 +++++---
 3 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/hw/mips_mipssim.c b/hw/mips_mipssim.c
index 830f635..a204ab1 100644
--- a/hw/mips_mipssim.c
+++ b/hw/mips_mipssim.c
@@ -215,7 +215,8 @@ mips_mipssim_init (ram_addr_t ram_size,
     /* A single 16450 sits at offset 0x3f8. It is attached to
        MIPS CPU INT2, which is interrupt 4. */
     if (serial_hds[0])
-        serial_init(0x3f8, env->irq[4], 115200, serial_hds[0]);
+        serial_init(0x3f8, env->irq[4], 115200, serial_hds[0],
+                    get_system_io());
 
     if (nd_table[0].used)
         /* MIPSnet uses the MIPS CPU INT0, which is interrupt 2. */
diff --git a/hw/pc.h b/hw/pc.h
index e4db071..f2b7af5 100644
--- a/hw/pc.h
+++ b/hw/pc.h
@@ -15,7 +15,7 @@
 /* serial.c */
 
 SerialState *serial_init(int base, qemu_irq irq, int baudbase,
-                         CharDriverState *chr);
+                         CharDriverState *chr, MemoryRegion *system_io);
 SerialState *serial_mm_init(MemoryRegion *address_space,
                             target_phys_addr_t base, int it_shift,
                             qemu_irq irq, int baudbase,
diff --git a/hw/serial.c b/hw/serial.c
index a421d1e..4ed20c0 100644
--- a/hw/serial.c
+++ b/hw/serial.c
@@ -28,6 +28,7 @@
 #include "pc.h"
 #include "qemu-timer.h"
 #include "sysemu.h"
+#include "exec-memory.h"
 
 //#define DEBUG_SERIAL
 
@@ -810,7 +811,7 @@ static const VMStateDescription vmstate_isa_serial = {
 };
 
 SerialState *serial_init(int base, qemu_irq irq, int baudbase,
-                         CharDriverState *chr)
+                         CharDriverState *chr, MemoryRegion *system_io)
 {
     SerialState *s;
 
@@ -823,8 +824,9 @@ SerialState *serial_init(int base, qemu_irq irq, int baudbase,
 
     vmstate_register(NULL, base, &vmstate_serial, s);
 
-    register_ioport_write(base, 8, 1, serial_ioport_write, s);
-    register_ioport_read(base, 8, 1, serial_ioport_read, s);
+    memory_region_init_io(&s->io, &serial_io_ops, s, "serial", 8);
+    memory_region_add_subregion(system_io, base, &s->io);
+
     return s;
 }
 
-- 
Julien Grall

[Qemu-devel] [PATCH V9 7/8] hw/pc.c: replace register_ioport*

[Julien Grall]

This patch replaces all register_ioport* with portio_* or
isa_register_portio_list. It permits to use the new Memory
stuff like listener.

Signed-off-by: Julien Grall <julien.grall@citrix.com>
---
 hw/pc.c |   58 +++++++++++++++++++++++++++++++++++++++++++---------------
 1 files changed, 43 insertions(+), 15 deletions(-)

diff --git a/hw/pc.c b/hw/pc.c
index 112739a..4abd4e2 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -104,7 +104,8 @@ void gsi_handler(void *opaque, int n, int level)
     qemu_set_irq(s->ioapic_irq[n], level);
 }
 
-static void ioport80_write(void *opaque, uint32_t addr, uint32_t data)
+static void ioport80_write(void *opaque, target_phys_addr_t addr,
+                           uint64_t data, unsigned size)
 {
 }
 
@@ -122,7 +123,8 @@ void cpu_set_ferr(CPUX86State *s)
     qemu_irq_raise(ferr_irq);
 }
 
-static void ioportF0_write(void *opaque, uint32_t addr, uint32_t data)
+static void ioportF0_write(void *opaque, target_phys_addr_t addr,
+                           uint64_t data, unsigned size)
 {
     qemu_irq_lower(ferr_irq);
 }
@@ -588,6 +590,17 @@ int e820_add_entry(uint64_t address, uint64_t length, uint32_t type)
     return index;
 }
 
+static const MemoryRegionPortio bochs_bios_portio_list[] = {
+    { 0x400, 2, 2, .write = bochs_bios_write, }, /* 0x400 */
+    { 0x402, 2, 1, .write = bochs_bios_write, }, /* 0x402 */
+    { 0x500, 1, 1, .write = bochs_bios_write, }, /* 0x500 */
+    { 0x501, 1, 1, .write = bochs_bios_write, }, /* 0x501 */
+    { 0x501, 2, 2, .write = bochs_bios_write, }, /* 0x501 */
+    { 0x503, 1, 1, .write = bochs_bios_write, }, /* 0x503  */
+    { 0x8900, 1, 1, .write = bochs_bios_write, }, /* 0x8900 */
+    PORTIO_END_OF_LIST(),
+};
+
 static void *bochs_bios_init(void)
 {
     void *fw_cfg;
@@ -595,18 +608,11 @@ static void *bochs_bios_init(void)
     size_t smbios_len;
     uint64_t *numa_fw_cfg;
     int i, j;
+    PortioList *bochs_bios_port_list = g_new(PortioList, 1);
 
-    register_ioport_write(0x400, 1, 2, bochs_bios_write, NULL);
-    register_ioport_write(0x401, 1, 2, bochs_bios_write, NULL);
-    register_ioport_write(0x402, 1, 1, bochs_bios_write, NULL);
-    register_ioport_write(0x403, 1, 1, bochs_bios_write, NULL);
-    register_ioport_write(0x8900, 1, 1, bochs_bios_write, NULL);
-
-    register_ioport_write(0x501, 1, 1, bochs_bios_write, NULL);
-    register_ioport_write(0x501, 1, 2, bochs_bios_write, NULL);
-    register_ioport_write(0x502, 1, 2, bochs_bios_write, NULL);
-    register_ioport_write(0x500, 1, 1, bochs_bios_write, NULL);
-    register_ioport_write(0x503, 1, 1, bochs_bios_write, NULL);
+    portio_list_init(bochs_bios_port_list, bochs_bios_portio_list,
+                     NULL, "bosch-bios");
+    portio_list_add(bochs_bios_port_list, get_system_io(), 0x0);
 
     fw_cfg = fw_cfg_init(BIOS_CFG_IOPORT, BIOS_CFG_IOPORT + 1, 0, 0);
 
@@ -1059,6 +1065,24 @@ static void cpu_request_exit(void *opaque, int irq, int level)
     }
 }
 
+static const MemoryRegionOps ioport80_io_ops = {
+    .write = ioport80_write,
+    .endianness = DEVICE_NATIVE_ENDIAN,
+    .impl = {
+        .min_access_size = 1,
+        .max_access_size = 1,
+    },
+};
+
+static const MemoryRegionOps ioportF0_io_ops = {
+    .write = ioportF0_write,
+    .endianness = DEVICE_NATIVE_ENDIAN,
+    .impl = {
+        .min_access_size = 1,
+        .max_access_size = 1,
+    },
+};
+
 void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
                           ISADevice **rtc_state,
                           ISADevice **floppy,
@@ -1073,10 +1097,14 @@ void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
     qemu_irq *a20_line;
     ISADevice *i8042, *port92, *vmmouse, *pit = NULL;
     qemu_irq *cpu_exit_irq;
+    MemoryRegion *ioport80_io = g_new(MemoryRegion, 1);
+    MemoryRegion *ioportF0_io = g_new(MemoryRegion, 1);
 
-    register_ioport_write(0x80, 1, 1, ioport80_write, NULL);
+    memory_region_init_io(ioport80_io, &ioport80_io_ops, NULL, "ioport80", 1);
+    memory_region_add_subregion(isa_bus->address_space_io, 0x80, ioport80_io);
 
-    register_ioport_write(0xf0, 1, 1, ioportF0_write, NULL);
+    memory_region_init_io(ioportF0_io, &ioportF0_io_ops, NULL, "ioportF0", 1);
+    memory_region_add_subregion(isa_bus->address_space_io, 0xf0, ioportF0_io);
 
     /*
      * Check if an HPET shall be created.
-- 
Julien Grall

[Qemu-devel] [PATCH V9 8/8] hw/dma.c: replace register_ioport*

[Julien Grall]

This patch replaces all register_ioport* be the new memory API functions.
It permits to use the new Memory stuff like listener.

Signed-off-by: Julien Grall <julien.grall@citrix.com>
---
 hw/dma.c |  108 +++++++++++++++++++++++++++++++++++++++++--------------------
 1 files changed, 72 insertions(+), 36 deletions(-)

diff --git a/hw/dma.c b/hw/dma.c
index 0a9322d..59c0dac 100644
--- a/hw/dma.c
+++ b/hw/dma.c
@@ -58,6 +58,8 @@ static struct dma_cont {
     int dshift;
     struct dma_regs regs[4];
     qemu_irq *cpu_request_exit;
+    MemoryRegion channel_io;
+    MemoryRegion cont_io;
 } dma_controllers[2];
 
 enum {
@@ -149,7 +151,8 @@ static inline int getff (struct dma_cont *d)
     return ff;
 }
 
-static uint32_t read_chan (void *opaque, uint32_t nport)
+static uint64_t read_chan(void *opaque, target_phys_addr_t nport,
+                          unsigned size)
 {
     struct dma_cont *d = opaque;
     int ichan, nreg, iport, ff, val, dir;
@@ -171,7 +174,8 @@ static uint32_t read_chan (void *opaque, uint32_t nport)
     return (val >> (d->dshift + (ff << 3))) & 0xff;
 }
 
-static void write_chan (void *opaque, uint32_t nport, uint32_t data)
+static void write_chan(void *opaque, target_phys_addr_t nport, uint64_t data,
+                       unsigned size)
 {
     struct dma_cont *d = opaque;
     int iport, ichan, nreg;
@@ -189,22 +193,23 @@ static void write_chan (void *opaque, uint32_t nport, uint32_t data)
     }
 }
 
-static void write_cont (void *opaque, uint32_t nport, uint32_t data)
+static void write_cont(void *opaque, target_phys_addr_t nport, uint64_t data,
+                       unsigned size)
 {
     struct dma_cont *d = opaque;
     int iport, ichan = 0;
 
     iport = (nport >> d->dshift) & 0x0f;
     switch (iport) {
-    case 0x08:                  /* command */
+    case 0x01:                  /* command */
         if ((data != 0) && (data & CMD_NOT_SUPPORTED)) {
-            dolog ("command %#x not supported\n", data);
+            dolog("command %#lx not supported\n", data);
             return;
         }
         d->command = data;
         break;
 
-    case 0x09:
+    case 0x02:
         ichan = data & 3;
         if (data & 4) {
             d->status |= 1 << (ichan + 4);
@@ -216,7 +221,7 @@ static void write_cont (void *opaque, uint32_t nport, uint32_t data)
         DMA_run();
         break;
 
-    case 0x0a:                  /* single mask */
+    case 0x03:                  /* single mask */
         if (data & 4)
             d->mask |= 1 << (data & 3);
         else
@@ -224,7 +229,7 @@ static void write_cont (void *opaque, uint32_t nport, uint32_t data)
         DMA_run();
         break;
 
-    case 0x0b:                  /* mode */
+    case 0x04:                  /* mode */
         {
             ichan = data & 3;
 #ifdef DEBUG_DMA
@@ -243,23 +248,23 @@ static void write_cont (void *opaque, uint32_t nport, uint32_t data)
             break;
         }
 
-    case 0x0c:                  /* clear flip flop */
+    case 0x05:                  /* clear flip flop */
         d->flip_flop = 0;
         break;
 
-    case 0x0d:                  /* reset */
+    case 0x06:                  /* reset */
         d->flip_flop = 0;
         d->mask = ~0;
         d->status = 0;
         d->command = 0;
         break;
 
-    case 0x0e:                  /* clear mask for all channels */
+    case 0x07:                  /* clear mask for all channels */
         d->mask = 0;
         DMA_run();
         break;
 
-    case 0x0f:                  /* write mask for all channels */
+    case 0x08:                  /* write mask for all channels */
         d->mask = data;
         DMA_run();
         break;
@@ -277,7 +282,8 @@ static void write_cont (void *opaque, uint32_t nport, uint32_t data)
 #endif
 }
 
-static uint32_t read_cont (void *opaque, uint32_t nport)
+static uint64_t read_cont(void *opaque, target_phys_addr_t nport,
+                          unsigned size)
 {
     struct dma_cont *d = opaque;
     int iport, val;
@@ -463,7 +469,7 @@ void DMA_schedule(int nchan)
 static void dma_reset(void *opaque)
 {
     struct dma_cont *d = opaque;
-    write_cont (d, (0x0d << d->dshift), 0);
+    write_cont(d, (0x06 << d->dshift), 0, 1);
 }
 
 static int dma_phony_handler (void *opaque, int nchan, int dma_pos, int dma_len)
@@ -473,38 +479,68 @@ static int dma_phony_handler (void *opaque, int nchan, int dma_pos, int dma_len)
     return dma_pos;
 }
 
+
+static const MemoryRegionOps channel_io_ops = {
+    .read = read_chan,
+    .write = write_chan,
+    .endianness = DEVICE_NATIVE_ENDIAN,
+    .impl = {
+        .min_access_size = 1,
+        .max_access_size = 1,
+    },
+};
+
+/* IOport from page_base */
+static const MemoryRegionPortio page_portio_list[] = {
+    { 0x01, 3, 1, .write = write_page, .read = read_page, },
+    { 0x07, 1, 1, .write = write_page, .read = read_page, },
+    PORTIO_END_OF_LIST(),
+};
+
+/* IOport from pageh_base */
+static const MemoryRegionPortio pageh_portio_list[] = {
+    { 0x01, 3, 1, .write = write_pageh, .read = read_pageh, },
+    { 0x07, 3, 1, .write = write_pageh, .read = read_pageh, },
+    PORTIO_END_OF_LIST(),
+};
+
+static const MemoryRegionOps cont_io_ops = {
+    .read = read_cont,
+    .write = write_cont,
+    .endianness = DEVICE_NATIVE_ENDIAN,
+    .impl = {
+        .min_access_size = 1,
+        .max_access_size = 1,
+    },
+};
+
 /* dshift = 0: 8 bit DMA, 1 = 16 bit DMA */
 static void dma_init2(struct dma_cont *d, int base, int dshift,
                       int page_base, int pageh_base,
                       qemu_irq *cpu_request_exit)
 {
-    static const int page_port_list[] = { 0x1, 0x2, 0x3, 0x7 };
     int i;
 
     d->dshift = dshift;
     d->cpu_request_exit = cpu_request_exit;
-    for (i = 0; i < 8; i++) {
-        register_ioport_write (base + (i << dshift), 1, 1, write_chan, d);
-        register_ioport_read (base + (i << dshift), 1, 1, read_chan, d);
-    }
-    for (i = 0; i < ARRAY_SIZE (page_port_list); i++) {
-        register_ioport_write (page_base + page_port_list[i], 1, 1,
-                               write_page, d);
-        register_ioport_read (page_base + page_port_list[i], 1, 1,
-                              read_page, d);
-        if (pageh_base >= 0) {
-            register_ioport_write (pageh_base + page_port_list[i], 1, 1,
-                                   write_pageh, d);
-            register_ioport_read (pageh_base + page_port_list[i], 1, 1,
-                                  read_pageh, d);
-        }
-    }
-    for (i = 0; i < 8; i++) {
-        register_ioport_write (base + ((i + 8) << dshift), 1, 1,
-                               write_cont, d);
-        register_ioport_read (base + ((i + 8) << dshift), 1, 1,
-                              read_cont, d);
+
+    memory_region_init_io(&d->channel_io, &channel_io_ops, d,
+                          "dma-chan", 8 << d->dshift);
+    memory_region_add_subregion(isa_address_space_io(NULL),
+                                base, &d->channel_io);
+
+    isa_register_portio_list(NULL, page_base, page_portio_list, d,
+                             "dma-page");
+    if (pageh_base >= 0) {
+        isa_register_portio_list(NULL, pageh_base, pageh_portio_list, d,
+                                 "dma-pageh");
     }
+
+    memory_region_init_io(&d->cont_io, &cont_io_ops, d, "dma-cont",
+                          8 << d->dshift);
+    memory_region_add_subregion(isa_address_space_io(NULL),
+                                base + (8 << d->dshift), &d->cont_io);
+
     qemu_register_reset(dma_reset, d);
     dma_reset(d);
     for (i = 0; i < ARRAY_SIZE (d->regs); ++i) {
-- 
Julien Grall

Re: [Qemu-devel] [PATCH V9 0/8] memory: unify ioport registration

[Julien Grall]

My apologies for this double post,
I made a typing error on Jan's email.

On 09/04/2012 04:13 PM, Julien Grall wrote:
> This is the nineth version of patch series about ioport registration.
>
> Some part of QEMU still use register_ioport* functions to register ioport.
> These functions doesn't allow to use Memory Listener on it.
>
> Modifications between V1 and V2:
>     - Remove the use of get_system_io. Instead of use isa and pci IO
>       address space;
>     - Avoid allocation of PortioList. Use the different device
>       structure;
>     - Still remove register_ioport* (hw/dma.c, hw/apm.c, hw/acpi_piix4.c);
>     - Use MemoryRegion when we have only a range of ioport;
>     - For some functions, add IO address space as argument;
>     - Add isa_address_space_io function.
>
> Modifications between V2 and V3:
>     - Remove some register_ioport_* on hw/vt82c686.c;
>     - Split smb ioport part in new patch;
>     - Still replace MemoryRegion when we have only a range of ioport;
>     - Fix read/write ioports prototype to  be compliant with memory callback.
>
> Modifications between V3 and V4:
>     - Fix compilation in hw/dma.c;
>     - Fix address conversion (hw/dma.c, hw/acpi_piix4.c) with MemorySection.
>       Indeed the new version use offset from MemorySection start instead of 0.
>
> Modifications between V4 and V5:
>     - Rebase on qemu upstream;
>     - Forget some ioport_register_* in acpi_piix4.c;
>     - Register 0x3b0 - 0x3df range for cirrus instead of ioport by ioport.
>
> Modifications between V5 and V6:
>     - Add read function on cirrus ioport (forget on the previous patch);
>     - Rework PM memory range handling;
>     - Fix PCI_BASE in acpi_piix4.c (wrong conversion during port);
>     - Rewrite isa_address_space_io to use ISA bus address space;
>     - Fix compilation in vt82c686.c.
>
> Modifications between V6 and V7:
>     - acpi_piix4: use memory_region_set_enabled instead of a boolean. I'm not
>       sure about this modification (adviced by Avi);
>     - Fix device endianness in acpi_piix4 (reported by Avi);
>     - Avoid dependencies between patches and reorder it (reported by Jan).
>       Some code moved from acpi_piix4 patch to smb/apm patches;
>
> Modifications between V7 and V8:
>     - Fix device endianness in smb patch, I forgot some on previous version;
>     - Register pm io region at initialization with default value instead
>       of at first call to pm_io_space_update (reported by Jan).
>
> Modifications between V8 and V9:
>     - Fix size constraint in pm_io_ops (reported by Jan).
>
> Julien Grall (8):
>    isa: add isa_address_space_io
>    hw/apm.c: replace register_ioport*
>    smb: replace_register_ioport*
>    hw/acpi_piix4.c: replace register_ioport*
>    hw/cirrus_vga.c: replace register_ioport*
>    hw/serial.c: replace register_ioport*
>    hw/pc.c: replace register_ioport*
>    hw/dma.c: replace register_ioport*
>
>   hw/acpi_piix4.c   |  165 +++++++++++++++++++++++++++++++++++++++++-----------
>   hw/apm.c          |   24 ++++++--
>   hw/apm.h          |    5 +-
>   hw/cirrus_vga.c   |   50 ++++++++++-------
>   hw/dma.c          |  108 +++++++++++++++++++++++------------
>   hw/isa-bus.c      |    9 +++
>   hw/isa.h          |    1 +
>   hw/mips_mipssim.c |    3 +-
>   hw/pc.c           |   58 ++++++++++++++-----
>   hw/pc.h           |    2 +-
>   hw/pm_smbus.c     |    7 +-
>   hw/pm_smbus.h     |    6 +-
>   hw/serial.c       |    8 ++-
>   hw/vt82c686.c     |   20 ++++++-
>   14 files changed, 341 insertions(+), 125 deletions(-)
>
>    

Re: [Qemu-devel] [PATCH V9 4/8] hw/acpi_piix4.c: replace register_ioport*

[Avi Kivity]

On 09/04/2012 06:13 PM, Julien Grall wrote:
> This patch replaces all register_ioport* with the new memory API. It permits
> to use the new Memory stuff like listener.
> 

> @@ -200,8 +212,11 @@ static void pm_io_space_update(PIIX4PMState *s)
>  
>          /* XXX: need to improve memory and ioport allocation */
>          PIIX4_DPRINTF("PM: mapping to 0x%x\n", pm_io_base);
> -        iorange_init(&s->ioport, &pm_iorange_ops, pm_io_base, 64);
> -        ioport_register(&s->ioport);
> +
> +        memory_region_set_address(&s->pm_io, pm_io_base);
> +        memory_region_set_enabled(&s->pm_io, true);
> +    } else {
> +        memory_region_set_enabled(&s->pm_io, false);
>      }
>  }
>  

The entire if () can be simplified to

       pm_io_base = le32_to_cpu(*(uint32_t *)(s->dev.config + 0x40));
       pm_io_base &= 0xffc0;
       memory_region_transaction_begin()
       memory_region_set_enabled(&s->pm_io, s->dev.config[0x80] & 1);
       memory_region_set_address(&s->pm_io, pm_io_base);
       memory_region_transaction_commit();


-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH V9 4/8] hw/acpi_piix4.c: replace register_ioport*

[Julien Grall]

On 09/09/2012 03:22 PM, Avi Kivity wrote:
> On 09/04/2012 06:13 PM, Julien Grall wrote:
>    
>> This patch replaces all register_ioport* with the new memory API. It permits
>> to use the new Memory stuff like listener.
>>
>>      
>    
>> @@ -200,8 +212,11 @@ static void pm_io_space_update(PIIX4PMState *s)
>>
>>           /* XXX: need to improve memory and ioport allocation */
>>           PIIX4_DPRINTF("PM: mapping to 0x%x\n", pm_io_base);
>> -        iorange_init(&s->ioport,&pm_iorange_ops, pm_io_base, 64);
>> -        ioport_register(&s->ioport);
>> +
>> +        memory_region_set_address(&s->pm_io, pm_io_base);
>> +        memory_region_set_enabled(&s->pm_io, true);
>> +    } else {
>> +        memory_region_set_enabled(&s->pm_io, false);
>>       }
>>   }
>>
>>      
> The entire if () can be simplified to
>
>         pm_io_base = le32_to_cpu(*(uint32_t *)(s->dev.config + 0x40));
>         pm_io_base&= 0xffc0;
>         memory_region_transaction_begin()
>         memory_region_set_enabled(&s->pm_io, s->dev.config[0x80]&  1);
>         memory_region_set_address(&s->pm_io, pm_io_base);
>         memory_region_transaction_commit();
>    
I will modify, do I need to resend all the patch series or just this patch ?

-- 
Julien Grall

Re: [Qemu-devel] [PATCH V9 4/8] hw/acpi_piix4.c: replace register_ioport*

[Avi Kivity]

On 09/10/2012 01:37 PM, Julien Grall wrote:
> On 09/09/2012 03:22 PM, Avi Kivity wrote:
>> On 09/04/2012 06:13 PM, Julien Grall wrote:
>>   
>>> This patch replaces all register_ioport* with the new memory API. It
>>> permits
>>> to use the new Memory stuff like listener.
>>>
>>>      
>>   
>>> @@ -200,8 +212,11 @@ static void pm_io_space_update(PIIX4PMState *s)
>>>
>>>           /* XXX: need to improve memory and ioport allocation */
>>>           PIIX4_DPRINTF("PM: mapping to 0x%x\n", pm_io_base);
>>> -        iorange_init(&s->ioport,&pm_iorange_ops, pm_io_base, 64);
>>> -        ioport_register(&s->ioport);
>>> +
>>> +        memory_region_set_address(&s->pm_io, pm_io_base);
>>> +        memory_region_set_enabled(&s->pm_io, true);
>>> +    } else {
>>> +        memory_region_set_enabled(&s->pm_io, false);
>>>       }
>>>   }
>>>
>>>      
>> The entire if () can be simplified to
>>
>>         pm_io_base = le32_to_cpu(*(uint32_t *)(s->dev.config + 0x40));
>>         pm_io_base&= 0xffc0;
>>         memory_region_transaction_begin()
>>         memory_region_set_enabled(&s->pm_io, s->dev.config[0x80]&  1);
>>         memory_region_set_address(&s->pm_io, pm_io_base);
>>         memory_region_transaction_commit();
>>    
> I will modify, do I need to resend all the patch series or just this
> patch ?
> 

Just this patch would be fine.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH V9 4/8] hw/acpi_piix4.c: replace register_ioport*

[Julien Grall]

This patch replaces all register_ioport* with the new memory API. It permits
to use the new Memory stuff like listener.

Signed-off-by: Julien Grall <julien.grall@citrix.com>
---
  hw/acpi_piix4.c |  154 
+++++++++++++++++++++++++++++++++++++++++--------------
  1 files changed, 116 insertions(+), 38 deletions(-)

diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
index 0b4ad24..942943c 100644
--- a/hw/acpi_piix4.c
+++ b/hw/acpi_piix4.c
@@ -41,10 +41,10 @@

  #define GPE_BASE 0xafe0
  #define GPE_LEN 4
-#define PCI_UP_BASE 0xae00
-#define PCI_DOWN_BASE 0xae04
+#define PCI_BASE 0xae00
  #define PCI_EJ_BASE 0xae08
  #define PCI_RMV_BASE 0xae0c
+#define PM_BASE 0x00

  #define PIIX4_PCI_HOTPLUG_STATUS 2

@@ -55,7 +55,7 @@ struct pci_status {

  typedef struct PIIX4PMState {
      PCIDevice dev;
-    IORange ioport;
+    MemoryRegion pm_io;
      ACPIREGS ar;

      APMState apm;
@@ -64,6 +64,11 @@ typedef struct PIIX4PMState {
      uint32_t smb_io_base;

      MemoryRegion smb_io;
+    MemoryRegion acpi_io;
+    MemoryRegion acpi_hot_io;
+    PortioList pci_hot_port_list;
+    MemoryRegion pciej_hot_io;
+    MemoryRegion pcirmv_hot_io;

      qemu_irq irq;
      qemu_irq smi_irq;
@@ -110,10 +115,10 @@ static void pm_tmr_timer(ACPIREGS *ar)
      pm_update_sci(s);
  }

-static void pm_ioport_write(IORange *ioport, uint64_t addr, unsigned width,
-                            uint64_t val)
+static void pm_ioport_write(void *opaque, target_phys_addr_t addr,
+                            uint64_t val, unsigned width)
  {
-    PIIX4PMState *s = container_of(ioport, PIIX4PMState, ioport);
+    PIIX4PMState *s = opaque;

      if (width != 2) {
          PIIX4_DPRINTF("PM write port=0x%04x width=%d val=0x%08x\n",
@@ -139,11 +144,11 @@ static void pm_ioport_write(IORange *ioport, 
uint64_t addr, unsigned width,
                    (unsigned int)val);
  }

-static void pm_ioport_read(IORange *ioport, uint64_t addr, unsigned width,
-                            uint64_t *data)
+static uint64_t pm_ioport_read(void *opaque, target_phys_addr_t addr,
+                               unsigned width)
  {
-    PIIX4PMState *s = container_of(ioport, PIIX4PMState, ioport);
-    uint32_t val;
+    PIIX4PMState *s = opaque;
+    uint64_t val;

      switch(addr) {
      case 0x00:
@@ -163,12 +168,18 @@ static void pm_ioport_read(IORange *ioport, 
uint64_t addr, unsigned width,
          break;
      }
      PIIX4_DPRINTF("PM readw port=0x%04x val=0x%04x\n", (unsigned 
int)addr, val);
-    *data = val;
+
+    return val;
  }

-static const IORangeOps pm_iorange_ops = {
+static const MemoryRegionOps pm_io_ops = {
      .read = pm_ioport_read,
      .write = pm_ioport_write,
+    .endianness = DEVICE_LITTLE_ENDIAN,
+    .impl = {
+        .min_access_size = 1,
+        .max_access_size = 4,
+    },
  };

  static void apm_ctrl_changed(uint32_t val, void *arg)
@@ -185,7 +196,8 @@ static void apm_ctrl_changed(uint32_t val, void *arg)
      }
  }

-static void acpi_dbg_writel(void *opaque, uint32_t addr, uint32_t val)
+static void acpi_dbg_writel(void *opaque, target_phys_addr_t addr,
+                            uint64_t val, unsigned size)
  {
      PIIX4_DPRINTF("ACPI: DBG: 0x%08x\n", val);
  }
@@ -194,15 +206,15 @@ static void pm_io_space_update(PIIX4PMState *s)
  {
      uint32_t pm_io_base;

-    if (s->dev.config[0x80] & 1) {
-        pm_io_base = le32_to_cpu(*(uint32_t *)(s->dev.config + 0x40));
-        pm_io_base &= 0xffc0;
+    pm_io_base = le32_to_cpu(*(uint32_t *)(s->dev.config + 0x40));
+    pm_io_base &= 0xffc0;
+    PIIX4_DPRINTF("PM: mapping ot 0x%x enabled = %d\n", pm_io_base,
+                  s->dev.config[0x80] & 1);

-        /* XXX: need to improve memory and ioport allocation */
-        PIIX4_DPRINTF("PM: mapping to 0x%x\n", pm_io_base);
-        iorange_init(&s->ioport, &pm_iorange_ops, pm_io_base, 64);
-        ioport_register(&s->ioport);
-    }
+    memory_region_transaction_begin();
+    memory_region_set_enabled(&s->pm_io, s->dev.config[0x80] & 1);
+    memory_region_set_address(&s->pm_io, pm_io_base);
+    memory_region_transaction_commit();
  }

  static void pm_write_config(PCIDevice *d,
@@ -395,6 +407,15 @@ static const MemoryRegionOps smb_io_ops = {
      },
  };

+static const MemoryRegionOps acpi_io_ops = {
+    .write = acpi_dbg_writel,
+    .endianness = DEVICE_LITTLE_ENDIAN,
+    .impl = {
+        .min_access_size = 4,
+        .max_access_size = 4,
+    },
+};
+
  static int piix4_pm_initfn(PCIDevice *dev)
  {
      PIIX4PMState *s = DO_UPCAST(PIIX4PMState, dev, dev);
@@ -409,7 +430,9 @@ static int piix4_pm_initfn(PCIDevice *dev)
      /* APM */
      apm_init(dev, &s->apm, apm_ctrl_changed, s);

-    register_ioport_write(ACPI_DBG_IO_ADDR, 4, 4, acpi_dbg_writel, s);
+    memory_region_init_io(&s->acpi_io, &acpi_io_ops, s, "piix4-acpi", 4);
+    memory_region_add_subregion(pci_address_space_io(dev), 
ACPI_DBG_IO_ADDR,
+ &s->acpi_io);

      if (s->kvm_enabled) {
          /* Mark SMM as already inited to prevent SMM from running.  
KVM does not
@@ -427,6 +450,12 @@ static int piix4_pm_initfn(PCIDevice *dev)
      memory_region_add_subregion(pci_address_space_io(dev), s->smb_io_base,
&s->smb_io);

+    /* PM  */
+    memory_region_init_io(&s->pm_io, &pm_io_ops, s, "piix4-pm", 64);
+    memory_region_set_enabled(&s->pm_io, false);
+    memory_region_add_subregion(pci_address_space_io(&s->dev),
+                                PM_BASE, &s->pm_io);
+
      acpi_pm_tmr_init(&s->ar, pm_tmr_timer);
      acpi_gpe_init(&s->ar, GPE_LEN);

@@ -510,16 +539,17 @@ static void piix4_pm_register_types(void)

  type_init(piix4_pm_register_types)

-static uint32_t gpe_readb(void *opaque, uint32_t addr)
+static uint64_t gpe_readb(void *opaque, target_phys_addr_t addr, 
unsigned size)
  {
      PIIX4PMState *s = opaque;
-    uint32_t val = acpi_gpe_ioport_readb(&s->ar, addr);
+    uint64_t val = acpi_gpe_ioport_readb(&s->ar, addr);

      PIIX4_DPRINTF("gpe read %x == %x\n", addr, val);
      return val;
  }

-static void gpe_writeb(void *opaque, uint32_t addr, uint32_t val)
+static void gpe_writeb(void *opaque, target_phys_addr_t addr, uint64_t val,
+                       unsigned size)
  {
      PIIX4PMState *s = opaque;

@@ -551,21 +581,24 @@ static uint32_t pci_down_read(void *opaque, 
uint32_t addr)
      return val;
  }

-static uint32_t pci_features_read(void *opaque, uint32_t addr)
+static uint64_t pci_features_read(void *opaque, target_phys_addr_t addr,
+                                  unsigned size)
  {
      /* No feature defined yet */
      PIIX4_DPRINTF("pci_features_read %x\n", 0);
      return 0;
  }

-static void pciej_write(void *opaque, uint32_t addr, uint32_t val)
+static void pciej_write(void *opaque, target_phys_addr_t addr, uint64_t 
val,
+                        unsigned size)
  {
      acpi_piix_eject_slot(opaque, val);

      PIIX4_DPRINTF("pciej write %x <== %d\n", addr, val);
  }

-static uint32_t pcirmv_read(void *opaque, uint32_t addr)
+static uint64_t pcirmv_read(void *opaque, target_phys_addr_t addr,
+                            unsigned size)
  {
      PIIX4PMState *s = opaque;

@@ -575,20 +608,65 @@ static uint32_t pcirmv_read(void *opaque, uint32_t 
addr)
  static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev,
                                  PCIHotplugState state);

-static void piix4_acpi_system_hot_add_init(PCIBus *bus, PIIX4PMState *s)
-{
+static const MemoryRegionOps acpi_hot_io_ops = {
+    .read = gpe_readb,
+    .write = gpe_writeb,
+    .endianness = DEVICE_LITTLE_ENDIAN,
+    .impl = {
+        .min_access_size = 1,
+        .max_access_size = 1,
+    },
+};

-    register_ioport_write(GPE_BASE, GPE_LEN, 1, gpe_writeb, s);
-    register_ioport_read(GPE_BASE, GPE_LEN, 1,  gpe_readb, s);
-    acpi_gpe_blk(&s->ar, GPE_BASE);
+/* PCI hot plug registers */
+static const MemoryRegionPortio pci_hot_portio_list[] = {
+    { 0x00, 4, 4, .read = pci_up_read, }, /* 0xae00 */
+    { 0x04, 4, 4, .read = pci_down_read, }, /* 0xae04 */
+    PORTIO_END_OF_LIST(),
+};

-    register_ioport_read(PCI_UP_BASE, 4, 4, pci_up_read, s);
-    register_ioport_read(PCI_DOWN_BASE, 4, 4, pci_down_read, s);
+static const MemoryRegionOps pciej_hot_io_ops = {
+    .read = pci_features_read,
+    .write = pciej_write,
+    .endianness = DEVICE_LITTLE_ENDIAN,
+    .impl = {
+        .min_access_size = 4,
+        .max_access_size = 4,
+    },
+};
+
+static const MemoryRegionOps pcirmv_hot_io_ops = {
+    .read = pcirmv_read,
+    .endianness = DEVICE_NATIVE_ENDIAN,
+    .impl = {
+        .min_access_size = 4,
+        .max_access_size = 4,
+    },
+};

-    register_ioport_write(PCI_EJ_BASE, 4, 4, pciej_write, s);
-    register_ioport_read(PCI_EJ_BASE, 4, 4,  pci_features_read, s);
+static void piix4_acpi_system_hot_add_init(PCIBus *bus, PIIX4PMState *s)
+{

-    register_ioport_read(PCI_RMV_BASE, 4, 4,  pcirmv_read, s);
+    memory_region_init_io(&s->acpi_hot_io, &acpi_hot_io_ops, s,
+                          "piix4-acpi-hot", GPE_LEN);
+    memory_region_add_subregion(pci_address_space_io(&s->dev), GPE_BASE,
+ &s->acpi_hot_io);
+    acpi_gpe_blk(&s->ar, 0);
+
+    portio_list_init(&s->pci_hot_port_list, pci_hot_portio_list, s,
+                     "piix4-pci-hot");
+    portio_list_add(&s->pci_hot_port_list, pci_address_space_io(&s->dev),
+                    PCI_BASE);
+
+    memory_region_init_io(&s->pciej_hot_io, &pciej_hot_io_ops, s,
+                          "piix4-pciej-hot", 4);
+    memory_region_add_subregion(pci_address_space_io(&s->dev), PCI_EJ_BASE,
+ &s->pciej_hot_io);
+
+    memory_region_init_io(&s->pcirmv_hot_io, &pcirmv_hot_io_ops, s,
+                          "piix4-pcirmv-hot", 4);
+    memory_region_add_subregion(pci_address_space_io(&s->dev), 
PCI_RMV_BASE,
+ &s->pcirmv_hot_io);

      pci_bus_hotplug(bus, piix4_device_hotplug, &s->dev.qdev);
  }
-- 
Julien Grall

Re: [Qemu-devel] [PATCH V9 4/8] hw/acpi_piix4.c: replace register_ioport*

[Jan Kiszka]

On 2012-09-10 19:27, Julien Grall wrote:
> This patch replaces all register_ioport* with the new memory API. It permits
> to use the new Memory stuff like listener.
> 
> Signed-off-by: Julien Grall <julien.grall@citrix.com>
> ---
>   hw/acpi_piix4.c |  154 
> +++++++++++++++++++++++++++++++++++++++++--------------

This is a good indicator that it's line-wrapped.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH V9 4/8] hw/acpi_piix4.c: replace register_ioport*

[Stefano Stabellini]

On Mon, 10 Sep 2012, Jan Kiszka wrote:
> On 2012-09-10 19:27, Julien Grall wrote:
> > This patch replaces all register_ioport* with the new memory API. It permits
> > to use the new Memory stuff like listener.
> > 
> > Signed-off-by: Julien Grall <julien.grall@citrix.com>
> > ---
> >   hw/acpi_piix4.c |  154 
> > +++++++++++++++++++++++++++++++++++++++++--------------
> 
> This is a good indicator that it's line-wrapped.

give a look at Documentation/email-clients.txt in the Linux kernel for a
good explanation on how to send emails avoiding line-wrapping:

http://www.mjmwired.net/kernel/Documentation/email-clients.txt

also using git send-email eliminates most problems

[Qemu-devel] [PATCH V9 4/8] hw/acpi_piix4.c: replace register_ioport*

[Julien Grall]

This patch replaces all register_ioport* with the new memory API. It permits
to use the new Memory stuff like listener.

Signed-off-by: Julien Grall <julien.grall@citrix.com>
---
 hw/acpi_piix4.c |  154 +++++++++++++++++++++++++++++++++++++++++--------------
 1 files changed, 116 insertions(+), 38 deletions(-)

diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
index 0b4ad24..942943c 100644
--- a/hw/acpi_piix4.c
+++ b/hw/acpi_piix4.c
@@ -41,10 +41,10 @@
 
 #define GPE_BASE 0xafe0
 #define GPE_LEN 4
-#define PCI_UP_BASE 0xae00
-#define PCI_DOWN_BASE 0xae04
+#define PCI_BASE 0xae00
 #define PCI_EJ_BASE 0xae08
 #define PCI_RMV_BASE 0xae0c
+#define PM_BASE 0x00
 
 #define PIIX4_PCI_HOTPLUG_STATUS 2
 
@@ -55,7 +55,7 @@ struct pci_status {
 
 typedef struct PIIX4PMState {
     PCIDevice dev;
-    IORange ioport;
+    MemoryRegion pm_io;
     ACPIREGS ar;
 
     APMState apm;
@@ -64,6 +64,11 @@ typedef struct PIIX4PMState {
     uint32_t smb_io_base;
 
     MemoryRegion smb_io;
+    MemoryRegion acpi_io;
+    MemoryRegion acpi_hot_io;
+    PortioList pci_hot_port_list;
+    MemoryRegion pciej_hot_io;
+    MemoryRegion pcirmv_hot_io;
 
     qemu_irq irq;
     qemu_irq smi_irq;
@@ -110,10 +115,10 @@ static void pm_tmr_timer(ACPIREGS *ar)
     pm_update_sci(s);
 }
 
-static void pm_ioport_write(IORange *ioport, uint64_t addr, unsigned width,
-                            uint64_t val)
+static void pm_ioport_write(void *opaque, target_phys_addr_t addr,
+                            uint64_t val, unsigned width)
 {
-    PIIX4PMState *s = container_of(ioport, PIIX4PMState, ioport);
+    PIIX4PMState *s = opaque;
 
     if (width != 2) {
         PIIX4_DPRINTF("PM write port=0x%04x width=%d val=0x%08x\n",
@@ -139,11 +144,11 @@ static void pm_ioport_write(IORange *ioport, uint64_t addr, unsigned width,
                   (unsigned int)val);
 }
 
-static void pm_ioport_read(IORange *ioport, uint64_t addr, unsigned width,
-                            uint64_t *data)
+static uint64_t pm_ioport_read(void *opaque, target_phys_addr_t addr,
+                               unsigned width)
 {
-    PIIX4PMState *s = container_of(ioport, PIIX4PMState, ioport);
-    uint32_t val;
+    PIIX4PMState *s = opaque;
+    uint64_t val;
 
     switch(addr) {
     case 0x00:
@@ -163,12 +168,18 @@ static void pm_ioport_read(IORange *ioport, uint64_t addr, unsigned width,
         break;
     }
     PIIX4_DPRINTF("PM readw port=0x%04x val=0x%04x\n", (unsigned int)addr, val);
-    *data = val;
+
+    return val;
 }
 
-static const IORangeOps pm_iorange_ops = {
+static const MemoryRegionOps pm_io_ops = {
     .read = pm_ioport_read,
     .write = pm_ioport_write,
+    .endianness = DEVICE_LITTLE_ENDIAN,
+    .impl = {
+        .min_access_size = 1,
+        .max_access_size = 4,
+    },
 };
 
 static void apm_ctrl_changed(uint32_t val, void *arg)
@@ -185,7 +196,8 @@ static void apm_ctrl_changed(uint32_t val, void *arg)
     }
 }
 
-static void acpi_dbg_writel(void *opaque, uint32_t addr, uint32_t val)
+static void acpi_dbg_writel(void *opaque, target_phys_addr_t addr,
+                            uint64_t val, unsigned size)
 {
     PIIX4_DPRINTF("ACPI: DBG: 0x%08x\n", val);
 }
@@ -194,15 +206,15 @@ static void pm_io_space_update(PIIX4PMState *s)
 {
     uint32_t pm_io_base;
 
-    if (s->dev.config[0x80] & 1) {
-        pm_io_base = le32_to_cpu(*(uint32_t *)(s->dev.config + 0x40));
-        pm_io_base &= 0xffc0;
+    pm_io_base = le32_to_cpu(*(uint32_t *)(s->dev.config + 0x40));
+    pm_io_base &= 0xffc0;
+    PIIX4_DPRINTF("PM: mapping ot 0x%x enabled = %d\n", pm_io_base,
+                  s->dev.config[0x80] & 1);
 
-        /* XXX: need to improve memory and ioport allocation */
-        PIIX4_DPRINTF("PM: mapping to 0x%x\n", pm_io_base);
-        iorange_init(&s->ioport, &pm_iorange_ops, pm_io_base, 64);
-        ioport_register(&s->ioport);
-    }
+    memory_region_transaction_begin();
+    memory_region_set_enabled(&s->pm_io, s->dev.config[0x80] & 1);
+    memory_region_set_address(&s->pm_io, pm_io_base);
+    memory_region_transaction_commit();
 }
 
 static void pm_write_config(PCIDevice *d,
@@ -395,6 +407,15 @@ static const MemoryRegionOps smb_io_ops = {
     },
 };
 
+static const MemoryRegionOps acpi_io_ops = {
+    .write = acpi_dbg_writel,
+    .endianness = DEVICE_LITTLE_ENDIAN,
+    .impl = {
+        .min_access_size = 4,
+        .max_access_size = 4,
+    },
+};
+
 static int piix4_pm_initfn(PCIDevice *dev)
 {
     PIIX4PMState *s = DO_UPCAST(PIIX4PMState, dev, dev);
@@ -409,7 +430,9 @@ static int piix4_pm_initfn(PCIDevice *dev)
     /* APM */
     apm_init(dev, &s->apm, apm_ctrl_changed, s);
 
-    register_ioport_write(ACPI_DBG_IO_ADDR, 4, 4, acpi_dbg_writel, s);
+    memory_region_init_io(&s->acpi_io, &acpi_io_ops, s, "piix4-acpi", 4);
+    memory_region_add_subregion(pci_address_space_io(dev), ACPI_DBG_IO_ADDR,
+                                &s->acpi_io);
 
     if (s->kvm_enabled) {
         /* Mark SMM as already inited to prevent SMM from running.  KVM does not
@@ -427,6 +450,12 @@ static int piix4_pm_initfn(PCIDevice *dev)
     memory_region_add_subregion(pci_address_space_io(dev), s->smb_io_base,
                                 &s->smb_io);
 
+    /* PM  */
+    memory_region_init_io(&s->pm_io, &pm_io_ops, s, "piix4-pm", 64);
+    memory_region_set_enabled(&s->pm_io, false);
+    memory_region_add_subregion(pci_address_space_io(&s->dev),
+                                PM_BASE, &s->pm_io);
+
     acpi_pm_tmr_init(&s->ar, pm_tmr_timer);
     acpi_gpe_init(&s->ar, GPE_LEN);
 
@@ -510,16 +539,17 @@ static void piix4_pm_register_types(void)
 
 type_init(piix4_pm_register_types)
 
-static uint32_t gpe_readb(void *opaque, uint32_t addr)
+static uint64_t gpe_readb(void *opaque, target_phys_addr_t addr, unsigned size)
 {
     PIIX4PMState *s = opaque;
-    uint32_t val = acpi_gpe_ioport_readb(&s->ar, addr);
+    uint64_t val = acpi_gpe_ioport_readb(&s->ar, addr);
 
     PIIX4_DPRINTF("gpe read %x == %x\n", addr, val);
     return val;
 }
 
-static void gpe_writeb(void *opaque, uint32_t addr, uint32_t val)
+static void gpe_writeb(void *opaque, target_phys_addr_t addr, uint64_t val,
+                       unsigned size)
 {
     PIIX4PMState *s = opaque;
 
@@ -551,21 +581,24 @@ static uint32_t pci_down_read(void *opaque, uint32_t addr)
     return val;
 }
 
-static uint32_t pci_features_read(void *opaque, uint32_t addr)
+static uint64_t pci_features_read(void *opaque, target_phys_addr_t addr,
+                                  unsigned size)
 {
     /* No feature defined yet */
     PIIX4_DPRINTF("pci_features_read %x\n", 0);
     return 0;
 }
 
-static void pciej_write(void *opaque, uint32_t addr, uint32_t val)
+static void pciej_write(void *opaque, target_phys_addr_t addr, uint64_t val,
+                        unsigned size)
 {
     acpi_piix_eject_slot(opaque, val);
 
     PIIX4_DPRINTF("pciej write %x <== %d\n", addr, val);
 }
 
-static uint32_t pcirmv_read(void *opaque, uint32_t addr)
+static uint64_t pcirmv_read(void *opaque, target_phys_addr_t addr,
+                            unsigned size)
 {
     PIIX4PMState *s = opaque;
 
@@ -575,20 +608,65 @@ static uint32_t pcirmv_read(void *opaque, uint32_t addr)
 static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev,
                                 PCIHotplugState state);
 
-static void piix4_acpi_system_hot_add_init(PCIBus *bus, PIIX4PMState *s)
-{
+static const MemoryRegionOps acpi_hot_io_ops = {
+    .read = gpe_readb,
+    .write = gpe_writeb,
+    .endianness = DEVICE_LITTLE_ENDIAN,
+    .impl = {
+        .min_access_size = 1,
+        .max_access_size = 1,
+    },
+};
 
-    register_ioport_write(GPE_BASE, GPE_LEN, 1, gpe_writeb, s);
-    register_ioport_read(GPE_BASE, GPE_LEN, 1,  gpe_readb, s);
-    acpi_gpe_blk(&s->ar, GPE_BASE);
+/* PCI hot plug registers */
+static const MemoryRegionPortio pci_hot_portio_list[] = {
+    { 0x00, 4, 4, .read = pci_up_read, }, /* 0xae00 */
+    { 0x04, 4, 4, .read = pci_down_read, }, /* 0xae04 */
+    PORTIO_END_OF_LIST(),
+};
 
-    register_ioport_read(PCI_UP_BASE, 4, 4, pci_up_read, s);
-    register_ioport_read(PCI_DOWN_BASE, 4, 4, pci_down_read, s);
+static const MemoryRegionOps pciej_hot_io_ops = {
+    .read = pci_features_read,
+    .write = pciej_write,
+    .endianness = DEVICE_LITTLE_ENDIAN,
+    .impl = {
+        .min_access_size = 4,
+        .max_access_size = 4,
+    },
+};
+
+static const MemoryRegionOps pcirmv_hot_io_ops = {
+    .read = pcirmv_read,
+    .endianness = DEVICE_NATIVE_ENDIAN,
+    .impl = {
+        .min_access_size = 4,
+        .max_access_size = 4,
+    },
+};
 
-    register_ioport_write(PCI_EJ_BASE, 4, 4, pciej_write, s);
-    register_ioport_read(PCI_EJ_BASE, 4, 4,  pci_features_read, s);
+static void piix4_acpi_system_hot_add_init(PCIBus *bus, PIIX4PMState *s)
+{
 
-    register_ioport_read(PCI_RMV_BASE, 4, 4,  pcirmv_read, s);
+    memory_region_init_io(&s->acpi_hot_io, &acpi_hot_io_ops, s,
+                          "piix4-acpi-hot", GPE_LEN);
+    memory_region_add_subregion(pci_address_space_io(&s->dev), GPE_BASE,
+                                &s->acpi_hot_io);
+    acpi_gpe_blk(&s->ar, 0);
+
+    portio_list_init(&s->pci_hot_port_list, pci_hot_portio_list, s,
+                     "piix4-pci-hot");
+    portio_list_add(&s->pci_hot_port_list, pci_address_space_io(&s->dev),
+                    PCI_BASE);
+
+    memory_region_init_io(&s->pciej_hot_io, &pciej_hot_io_ops, s,
+                          "piix4-pciej-hot", 4);
+    memory_region_add_subregion(pci_address_space_io(&s->dev), PCI_EJ_BASE,
+                                &s->pciej_hot_io);
+
+    memory_region_init_io(&s->pcirmv_hot_io, &pcirmv_hot_io_ops, s,
+                          "piix4-pcirmv-hot", 4);
+    memory_region_add_subregion(pci_address_space_io(&s->dev), PCI_RMV_BASE,
+                                &s->pcirmv_hot_io);
 
     pci_bus_hotplug(bus, piix4_device_hotplug, &s->dev.qdev);
 }
-- 
Julien Grall

Re: [Qemu-devel] [PATCH V9 0/8] memory: unify ioport registration

[Avi Kivity]

On 09/04/2012 06:13 PM, Julien Grall wrote:
> This is the nineth version of patch series about ioport registration.
> 
> Some part of QEMU still use register_ioport* functions to register ioport.
> These functions doesn't allow to use Memory Listener on it.

Thanks, applied all (w/ updated patch 4), will push shortly.


-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH V9 0/8] memory: unify ioport registration

[Avi Kivity]

On 09/11/2012 12:15 PM, Avi Kivity wrote:
> On 09/04/2012 06:13 PM, Julien Grall wrote:
>> This is the nineth version of patch series about ioport registration.
>> 
>> Some part of QEMU still use register_ioport* functions to register ioport.
>> These functions doesn't allow to use Memory Listener on it.
> 
> Thanks, applied all (w/ updated patch 4), will push shortly.
> 
> 

Aborts with the command line

  qemu-system-x86_64 -device isa-debugcon,iobase=0x402,chardev=stdio -chardev stdio,id=stdio


-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH V9 0/8] memory: unify ioport registration

[Julien Grall]

On 09/11/2012 10:25 AM, Avi Kivity wrote:
> On 09/11/2012 12:15 PM, Avi Kivity wrote:
>    
>> On 09/04/2012 06:13 PM, Julien Grall wrote:
>>      
>>> This is the nineth version of patch series about ioport registration.
>>>
>>> Some part of QEMU still use register_ioport* functions to register ioport.
>>> These functions doesn't allow to use Memory Listener on it.
>>>        
>> Thanks, applied all (w/ updated patch 4), will push shortly.
>>
>>
>>      
> Aborts with the command line
>
>    qemu-system-x86_64 -device isa-debugcon,iobase=0x402,chardev=stdio -chardev stdio,id=stdio
>
>
>    

I have bisected and found the problem with bochs_bios_init in hw/pc.c.
Bosch already register the iport 0x402. I'm not sure that it's a bug.
In fact register_ioport_read/write check if the current ioport is used
with the opaque variable.
Before my patch, bochs_bios_init registered it's ioport with opaque
NULL, so if someone (like debugcon) wants to use the ioport there is
no problem. But now, I used portio_list_init to register bochs ioport,
so the opaque is not NULL.
I don't really know how to resolve this problem. Perhaps we could
just improve the debug message.

-- 
Julien Grall

Re: [Qemu-devel] [PATCH V9 0/8] memory: unify ioport registration

[Jan Kiszka]

On 2012-09-11 13:27, Julien Grall wrote:
> On 09/11/2012 10:25 AM, Avi Kivity wrote:
>> On 09/11/2012 12:15 PM, Avi Kivity wrote:
>>    
>>> On 09/04/2012 06:13 PM, Julien Grall wrote:
>>>      
>>>> This is the nineth version of patch series about ioport registration.
>>>>
>>>> Some part of QEMU still use register_ioport* functions to register ioport.
>>>> These functions doesn't allow to use Memory Listener on it.
>>>>        
>>> Thanks, applied all (w/ updated patch 4), will push shortly.
>>>
>>>
>>>      
>> Aborts with the command line
>>
>>    qemu-system-x86_64 -device isa-debugcon,iobase=0x402,chardev=stdio -chardev stdio,id=stdio
>>
>>
>>    
> 
> I have bisected and found the problem with bochs_bios_init in hw/pc.c.
> Bosch already register the iport 0x402. I'm not sure that it's a bug.
> In fact register_ioport_read/write check if the current ioport is used
> with the opaque variable.
> Before my patch, bochs_bios_init registered it's ioport with opaque
> NULL, so if someone (like debugcon) wants to use the ioport there is
> no problem. But now, I used portio_list_init to register bochs ioport,
> so the opaque is not NULL.
> I don't really know how to resolve this problem. Perhaps we could
> just improve the debug message.

My suggestion:

pc: Don't listen on debug ports by default

Only listen on debug ports when we also handle them. They are better
handled by debugcon these days which is runtime configurable.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>

diff --git a/hw/pc.c b/hw/pc.c
index 112739a..70abf0e 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -539,9 +539,9 @@ static void bochs_bios_write(void *opaque, uint32_t addr, uint32_t val)
     case 0x401:
         /* used to be panic, now unused */
         break;
+#ifdef DEBUG_BIOS
     case 0x402:
     case 0x403:
-#ifdef DEBUG_BIOS
         fprintf(stderr, "%c", val);
 #endif
         break;

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

[Qemu-devel] [PATCH] pc: Don't listen on debug ports by default

[Jan Kiszka]

On 2012-09-11 13:48, Jan Kiszka wrote:
> On 2012-09-11 13:27, Julien Grall wrote:
>> On 09/11/2012 10:25 AM, Avi Kivity wrote:
>>> On 09/11/2012 12:15 PM, Avi Kivity wrote:
>>>    
>>>> On 09/04/2012 06:13 PM, Julien Grall wrote:
>>>>      
>>>>> This is the nineth version of patch series about ioport registration.
>>>>>
>>>>> Some part of QEMU still use register_ioport* functions to register ioport.
>>>>> These functions doesn't allow to use Memory Listener on it.
>>>>>        
>>>> Thanks, applied all (w/ updated patch 4), will push shortly.
>>>>
>>>>
>>>>      
>>> Aborts with the command line
>>>
>>>    qemu-system-x86_64 -device isa-debugcon,iobase=0x402,chardev=stdio -chardev stdio,id=stdio
>>>
>>>
>>>    
>>
>> I have bisected and found the problem with bochs_bios_init in hw/pc.c.
>> Bosch already register the iport 0x402. I'm not sure that it's a bug.
>> In fact register_ioport_read/write check if the current ioport is used
>> with the opaque variable.
>> Before my patch, bochs_bios_init registered it's ioport with opaque
>> NULL, so if someone (like debugcon) wants to use the ioport there is
>> no problem. But now, I used portio_list_init to register bochs ioport,
>> so the opaque is not NULL.
>> I don't really know how to resolve this problem. Perhaps we could
>> just improve the debug message.
> 
> My suggestion:
> 
> pc: Don't listen on debug ports by default
> 
> Only listen on debug ports when we also handle them. They are better
> handled by debugcon these days which is runtime configurable.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> 
> diff --git a/hw/pc.c b/hw/pc.c
> index 112739a..70abf0e 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -539,9 +539,9 @@ static void bochs_bios_write(void *opaque, uint32_t addr, uint32_t val)
>      case 0x401:
>          /* used to be panic, now unused */
>          break;
> +#ifdef DEBUG_BIOS
>      case 0x402:
>      case 0x403:
> -#ifdef DEBUG_BIOS
>          fprintf(stderr, "%c", val);
>  #endif
>          break;
> 

OK, ket's try properly again:

----8<-----

Only listen on debug ports when we also handle them. They are better
handled by debugcon these days which is runtime configurable.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/pc.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/pc.c b/hw/pc.c
index 112739a..134d5f7 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -539,12 +539,12 @@ static void bochs_bios_write(void *opaque, uint32_t addr, uint32_t val)
     case 0x401:
         /* used to be panic, now unused */
         break;
+#ifdef DEBUG_BIOS
     case 0x402:
     case 0x403:
-#ifdef DEBUG_BIOS
         fprintf(stderr, "%c", val);
-#endif
         break;
+#endif
     case 0x8900:
         /* same as Bochs power off */
         if (val == shutdown_str[shutdown_index]) {
@@ -598,8 +598,10 @@ static void *bochs_bios_init(void)
 
     register_ioport_write(0x400, 1, 2, bochs_bios_write, NULL);
     register_ioport_write(0x401, 1, 2, bochs_bios_write, NULL);
+#ifdef DEBUG_BIOS
     register_ioport_write(0x402, 1, 1, bochs_bios_write, NULL);
     register_ioport_write(0x403, 1, 1, bochs_bios_write, NULL);
+#endif
     register_ioport_write(0x8900, 1, 1, bochs_bios_write, NULL);
 
     register_ioport_write(0x501, 1, 1, bochs_bios_write, NULL);
-- 
1.7.3.4

Re: [Qemu-devel] [PATCH] pc: Don't listen on debug ports by default

[Julien Grall]

On 09/11/2012 12:57 PM, Jan Kiszka wrote:

> On 2012-09-11 13:48, Jan Kiszka wrote:
>> On 2012-09-11 13:27, Julien Grall wrote:
>>> On 09/11/2012 10:25 AM, Avi Kivity wrote:
>>>> On 09/11/2012 12:15 PM, Avi Kivity wrote:
>>>>    
>>>>> On 09/04/2012 06:13 PM, Julien Grall wrote:
>>>>>      
>>>>>> This is the nineth version of patch series about ioport registration.
>>>>>>
>>>>>> Some part of QEMU still use register_ioport* functions to register ioport.
>>>>>> These functions doesn't allow to use Memory Listener on it.
>>>>>>        
>>>>> Thanks, applied all (w/ updated patch 4), will push shortly.
>>>>>
>>>>>
>>>>>      
>>>> Aborts with the command line
>>>>
>>>>    qemu-system-x86_64 -device isa-debugcon,iobase=0x402,chardev=stdio -chardev stdio,id=stdio
>>>>
>>>>
>>>>    
>>>
>>> I have bisected and found the problem with bochs_bios_init in hw/pc.c.
>>> Bosch already register the iport 0x402. I'm not sure that it's a bug.
>>> In fact register_ioport_read/write check if the current ioport is used
>>> with the opaque variable.
>>> Before my patch, bochs_bios_init registered it's ioport with opaque
>>> NULL, so if someone (like debugcon) wants to use the ioport there is
>>> no problem. But now, I used portio_list_init to register bochs ioport,
>>> so the opaque is not NULL.
>>> I don't really know how to resolve this problem. Perhaps we could
>>> just improve the debug message.
>>
>> My suggestion:
>>
>> pc: Don't listen on debug ports by default
>>
>> Only listen on debug ports when we also handle them. They are better
>> handled by debugcon these days which is runtime configurable.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> diff --git a/hw/pc.c b/hw/pc.c
>> index 112739a..70abf0e 100644
>> --- a/hw/pc.c
>> +++ b/hw/pc.c
>> @@ -539,9 +539,9 @@ static void bochs_bios_write(void *opaque, uint32_t addr, uint32_t val)
>>      case 0x401:
>>          /* used to be panic, now unused */
>>          break;
>> +#ifdef DEBUG_BIOS
>>      case 0x402:
>>      case 0x403:
>> -#ifdef DEBUG_BIOS
>>          fprintf(stderr, "%c", val);
>>  #endif
>>          break;
>>
> 
> OK, ket's try properly again:
> 
> ----8<-----
> 
> Only listen on debug ports when we also handle them. They are better
> handled by debugcon these days which is runtime configurable.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  hw/pc.c |    6 ++++--
>  1 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/pc.c b/hw/pc.c
> index 112739a..134d5f7 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -539,12 +539,12 @@ static void bochs_bios_write(void *opaque, uint32_t addr, uint32_t val)
>      case 0x401:
>          /* used to be panic, now unused */
>          break;
> +#ifdef DEBUG_BIOS
>      case 0x402:
>      case 0x403:
> -#ifdef DEBUG_BIOS
>          fprintf(stderr, "%c", val);
> -#endif
>          break;
> +#endif
>      case 0x8900:
>          /* same as Bochs power off */
>          if (val == shutdown_str[shutdown_index]) {


Is it possible to modify this part:

> @@ -598,8 +598,10 @@ static void *bochs_bios_init(void)
>  
>      register_ioport_write(0x400, 1, 2, bochs_bios_write, NULL);
>      register_ioport_write(0x401, 1, 2, bochs_bios_write, NULL);
> +#ifdef DEBUG_BIOS
>      register_ioport_write(0x402, 1, 1, bochs_bios_write, NULL);
>      register_ioport_write(0x403, 1, 1, bochs_bios_write, NULL);
> +#endif
>      register_ioport_write(0x8900, 1, 1, bochs_bios_write, NULL);
>  
>      register_ioport_write(0x501, 1, 1, bochs_bios_write, NULL);


by something like that:

--------------------------
@@ -592,7 +592,9 @@ int e820_add_entry(uint64_t address, uint64_t length, uint32_t type)
 
 static const MemoryRegionPortio bochs_bios_portio_list[] = {
     { 0x400, 2, 2, .write = bochs_bios_write, }, /* 0x400 */
+#ifdef DEBUG_BIOS
     { 0x402, 2, 1, .write = bochs_bios_write, }, /* 0x402 */
+#endif
     { 0x500, 1, 1, .write = bochs_bios_write, }, /* 0x500 */
     { 0x501, 1, 1, .write = bochs_bios_write, }, /* 0x501 */
     { 0x501, 2, 2, .write = bochs_bios_write, }, /* 0x501 */

---------------------------
So it can be applied just after: "memory: unify ioport registration"
patch series.Otherwise, I will modify my patch series.

--
Julien Grall

Re: [Qemu-devel] [PATCH] pc: Don't listen on debug ports by default

[Jan Kiszka]

On 2012-09-11 14:14, Julien Grall wrote:
> On 09/11/2012 12:57 PM, Jan Kiszka wrote:
> 
>> On 2012-09-11 13:48, Jan Kiszka wrote:
>>> On 2012-09-11 13:27, Julien Grall wrote:
>>>> On 09/11/2012 10:25 AM, Avi Kivity wrote:
>>>>> On 09/11/2012 12:15 PM, Avi Kivity wrote:
>>>>>    
>>>>>> On 09/04/2012 06:13 PM, Julien Grall wrote:
>>>>>>      
>>>>>>> This is the nineth version of patch series about ioport registration.
>>>>>>>
>>>>>>> Some part of QEMU still use register_ioport* functions to register ioport.
>>>>>>> These functions doesn't allow to use Memory Listener on it.
>>>>>>>        
>>>>>> Thanks, applied all (w/ updated patch 4), will push shortly.
>>>>>>
>>>>>>
>>>>>>      
>>>>> Aborts with the command line
>>>>>
>>>>>    qemu-system-x86_64 -device isa-debugcon,iobase=0x402,chardev=stdio -chardev stdio,id=stdio
>>>>>
>>>>>
>>>>>    
>>>>
>>>> I have bisected and found the problem with bochs_bios_init in hw/pc.c.
>>>> Bosch already register the iport 0x402. I'm not sure that it's a bug.
>>>> In fact register_ioport_read/write check if the current ioport is used
>>>> with the opaque variable.
>>>> Before my patch, bochs_bios_init registered it's ioport with opaque
>>>> NULL, so if someone (like debugcon) wants to use the ioport there is
>>>> no problem. But now, I used portio_list_init to register bochs ioport,
>>>> so the opaque is not NULL.
>>>> I don't really know how to resolve this problem. Perhaps we could
>>>> just improve the debug message.
>>>
>>> My suggestion:
>>>
>>> pc: Don't listen on debug ports by default
>>>
>>> Only listen on debug ports when we also handle them. They are better
>>> handled by debugcon these days which is runtime configurable.
>>>
>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>
>>> diff --git a/hw/pc.c b/hw/pc.c
>>> index 112739a..70abf0e 100644
>>> --- a/hw/pc.c
>>> +++ b/hw/pc.c
>>> @@ -539,9 +539,9 @@ static void bochs_bios_write(void *opaque, uint32_t addr, uint32_t val)
>>>      case 0x401:
>>>          /* used to be panic, now unused */
>>>          break;
>>> +#ifdef DEBUG_BIOS
>>>      case 0x402:
>>>      case 0x403:
>>> -#ifdef DEBUG_BIOS
>>>          fprintf(stderr, "%c", val);
>>>  #endif
>>>          break;
>>>
>>
>> OK, ket's try properly again:
>>
>> ----8<-----
>>
>> Only listen on debug ports when we also handle them. They are better
>> handled by debugcon these days which is runtime configurable.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>  hw/pc.c |    6 ++++--
>>  1 files changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/pc.c b/hw/pc.c
>> index 112739a..134d5f7 100644
>> --- a/hw/pc.c
>> +++ b/hw/pc.c
>> @@ -539,12 +539,12 @@ static void bochs_bios_write(void *opaque, uint32_t addr, uint32_t val)
>>      case 0x401:
>>          /* used to be panic, now unused */
>>          break;
>> +#ifdef DEBUG_BIOS
>>      case 0x402:
>>      case 0x403:
>> -#ifdef DEBUG_BIOS
>>          fprintf(stderr, "%c", val);
>> -#endif
>>          break;
>> +#endif
>>      case 0x8900:
>>          /* same as Bochs power off */
>>          if (val == shutdown_str[shutdown_index]) {
> 
> 
> Is it possible to modify this part:
> 
>> @@ -598,8 +598,10 @@ static void *bochs_bios_init(void)
>>  
>>      register_ioport_write(0x400, 1, 2, bochs_bios_write, NULL);
>>      register_ioport_write(0x401, 1, 2, bochs_bios_write, NULL);
>> +#ifdef DEBUG_BIOS
>>      register_ioport_write(0x402, 1, 1, bochs_bios_write, NULL);
>>      register_ioport_write(0x403, 1, 1, bochs_bios_write, NULL);
>> +#endif
>>      register_ioport_write(0x8900, 1, 1, bochs_bios_write, NULL);
>>  
>>      register_ioport_write(0x501, 1, 1, bochs_bios_write, NULL);
> 
> 
> by something like that:
> 
> --------------------------
> @@ -592,7 +592,9 @@ int e820_add_entry(uint64_t address, uint64_t length, uint32_t type)
>  
>  static const MemoryRegionPortio bochs_bios_portio_list[] = {
>      { 0x400, 2, 2, .write = bochs_bios_write, }, /* 0x400 */
> +#ifdef DEBUG_BIOS
>      { 0x402, 2, 1, .write = bochs_bios_write, }, /* 0x402 */
> +#endif
>      { 0x500, 1, 1, .write = bochs_bios_write, }, /* 0x500 */
>      { 0x501, 1, 1, .write = bochs_bios_write, }, /* 0x501 */
>      { 0x501, 2, 2, .write = bochs_bios_write, }, /* 0x501 */
> 
> ---------------------------
> So it can be applied just after: "memory: unify ioport registration"
> patch series.Otherwise, I will modify my patch series.

No, lets do it again in an order that avoids temporal regressions,
specifically as autotests depend on debugcon.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH] pc: Don't listen on debug ports by default

[Avi Kivity]

On 09/11/2012 02:57 PM, Jan Kiszka wrote:

> Only listen on debug ports when we also handle them. They are better
> handled by debugcon these days which is runtime configurable.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  hw/pc.c |    6 ++++--
>  1 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/pc.c b/hw/pc.c
> index 112739a..134d5f7 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -539,12 +539,12 @@ static void bochs_bios_write(void *opaque, uint32_t addr, uint32_t val)
>      case 0x401:
>          /* used to be panic, now unused */
>          break;
> +#ifdef DEBUG_BIOS
>      case 0x402:
>      case 0x403:
> -#ifdef DEBUG_BIOS
>          fprintf(stderr, "%c", val);
> -#endif
>          break;
> +#endif
>      case 0x8900:
>          /* same as Bochs power off */
>          if (val == shutdown_str[shutdown_index]) {
> @@ -598,8 +598,10 @@ static void *bochs_bios_init(void)
>  
>      register_ioport_write(0x400, 1, 2, bochs_bios_write, NULL);
>      register_ioport_write(0x401, 1, 2, bochs_bios_write, NULL);
> +#ifdef DEBUG_BIOS
>      register_ioport_write(0x402, 1, 1, bochs_bios_write, NULL);
>      register_ioport_write(0x403, 1, 1, bochs_bios_write, NULL);
> +#endif
>      register_ioport_write(0x8900, 1, 1, bochs_bios_write, NULL);
>  
>      register_ioport_write(0x501, 1, 1, bochs_bios_write, NULL);
> 


Why not drop DEBUG_BIOS completely?  If you want to debug the bios, use
debugcon.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] pc: Don't listen on debug ports by default

[Jan Kiszka]

On 2012-09-11 14:53, Avi Kivity wrote:
> On 09/11/2012 02:57 PM, Jan Kiszka wrote:
> 
>> Only listen on debug ports when we also handle them. They are better
>> handled by debugcon these days which is runtime configurable.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>  hw/pc.c |    6 ++++--
>>  1 files changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/pc.c b/hw/pc.c
>> index 112739a..134d5f7 100644
>> --- a/hw/pc.c
>> +++ b/hw/pc.c
>> @@ -539,12 +539,12 @@ static void bochs_bios_write(void *opaque, uint32_t addr, uint32_t val)
>>      case 0x401:
>>          /* used to be panic, now unused */
>>          break;
>> +#ifdef DEBUG_BIOS
>>      case 0x402:
>>      case 0x403:
>> -#ifdef DEBUG_BIOS
>>          fprintf(stderr, "%c", val);
>> -#endif
>>          break;
>> +#endif
>>      case 0x8900:
>>          /* same as Bochs power off */
>>          if (val == shutdown_str[shutdown_index]) {
>> @@ -598,8 +598,10 @@ static void *bochs_bios_init(void)
>>  
>>      register_ioport_write(0x400, 1, 2, bochs_bios_write, NULL);
>>      register_ioport_write(0x401, 1, 2, bochs_bios_write, NULL);
>> +#ifdef DEBUG_BIOS
>>      register_ioport_write(0x402, 1, 1, bochs_bios_write, NULL);
>>      register_ioport_write(0x403, 1, 1, bochs_bios_write, NULL);
>> +#endif
>>      register_ioport_write(0x8900, 1, 1, bochs_bios_write, NULL);
>>  
>>      register_ioport_write(0x501, 1, 1, bochs_bios_write, NULL);
>>
> 
> 
> Why not drop DEBUG_BIOS completely?  If you want to debug the bios, use
> debugcon.

Probably true. There is more practically dead stuff below this that just
prints if DEBUG_BIOS is enabled.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH] pc: Don't listen on debug ports by default

[Avi Kivity]

On 09/11/2012 05:11 PM, Jan Kiszka wrote:
> On 2012-09-11 14:53, Avi Kivity wrote:
>> On 09/11/2012 02:57 PM, Jan Kiszka wrote:
>> 
>>> Only listen on debug ports when we also handle them. They are better
>>> handled by debugcon these days which is runtime configurable.
>>>
>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>> ---
>>>  hw/pc.c |    6 ++++--
>>>  1 files changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/pc.c b/hw/pc.c
>>> index 112739a..134d5f7 100644
>>> --- a/hw/pc.c
>>> +++ b/hw/pc.c
>>> @@ -539,12 +539,12 @@ static void bochs_bios_write(void *opaque, uint32_t addr, uint32_t val)
>>>      case 0x401:
>>>          /* used to be panic, now unused */
>>>          break;
>>> +#ifdef DEBUG_BIOS
>>>      case 0x402:
>>>      case 0x403:
>>> -#ifdef DEBUG_BIOS
>>>          fprintf(stderr, "%c", val);
>>> -#endif
>>>          break;
>>> +#endif
>>>      case 0x8900:
>>>          /* same as Bochs power off */
>>>          if (val == shutdown_str[shutdown_index]) {
>>> @@ -598,8 +598,10 @@ static void *bochs_bios_init(void)
>>>  
>>>      register_ioport_write(0x400, 1, 2, bochs_bios_write, NULL);
>>>      register_ioport_write(0x401, 1, 2, bochs_bios_write, NULL);
>>> +#ifdef DEBUG_BIOS
>>>      register_ioport_write(0x402, 1, 1, bochs_bios_write, NULL);
>>>      register_ioport_write(0x403, 1, 1, bochs_bios_write, NULL);
>>> +#endif
>>>      register_ioport_write(0x8900, 1, 1, bochs_bios_write, NULL);
>>>  
>>>      register_ioport_write(0x501, 1, 1, bochs_bios_write, NULL);
>>>
>> 
>> 
>> Why not drop DEBUG_BIOS completely?  If you want to debug the bios, use
>> debugcon.
> 
> Probably true. There is more practically dead stuff below this that just
> prints if DEBUG_BIOS is enabled.

Actually it is autotest that is at fault here.  It is installing a
debugcon with non-standard iobase atop a builtin device.
Pre-memory-API, we did not detect that.

We can subclass isa-debugcon as bochs-debugcon, change the default
ioport to 0x402, and use that instead of the code in pc.c.  How does
that sound?

Autotest will need to be changed to use the new device type.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] pc: Don't listen on debug ports by default

[Jan Kiszka]

On 2012-09-11 16:19, Avi Kivity wrote:
> On 09/11/2012 05:11 PM, Jan Kiszka wrote:
>> On 2012-09-11 14:53, Avi Kivity wrote:
>>> On 09/11/2012 02:57 PM, Jan Kiszka wrote:
>>>
>>>> Only listen on debug ports when we also handle them. They are better
>>>> handled by debugcon these days which is runtime configurable.
>>>>
>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>> ---
>>>>  hw/pc.c |    6 ++++--
>>>>  1 files changed, 4 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/hw/pc.c b/hw/pc.c
>>>> index 112739a..134d5f7 100644
>>>> --- a/hw/pc.c
>>>> +++ b/hw/pc.c
>>>> @@ -539,12 +539,12 @@ static void bochs_bios_write(void *opaque, uint32_t addr, uint32_t val)
>>>>      case 0x401:
>>>>          /* used to be panic, now unused */
>>>>          break;
>>>> +#ifdef DEBUG_BIOS
>>>>      case 0x402:
>>>>      case 0x403:
>>>> -#ifdef DEBUG_BIOS
>>>>          fprintf(stderr, "%c", val);
>>>> -#endif
>>>>          break;
>>>> +#endif
>>>>      case 0x8900:
>>>>          /* same as Bochs power off */
>>>>          if (val == shutdown_str[shutdown_index]) {
>>>> @@ -598,8 +598,10 @@ static void *bochs_bios_init(void)
>>>>  
>>>>      register_ioport_write(0x400, 1, 2, bochs_bios_write, NULL);
>>>>      register_ioport_write(0x401, 1, 2, bochs_bios_write, NULL);
>>>> +#ifdef DEBUG_BIOS
>>>>      register_ioport_write(0x402, 1, 1, bochs_bios_write, NULL);
>>>>      register_ioport_write(0x403, 1, 1, bochs_bios_write, NULL);
>>>> +#endif
>>>>      register_ioport_write(0x8900, 1, 1, bochs_bios_write, NULL);
>>>>  
>>>>      register_ioport_write(0x501, 1, 1, bochs_bios_write, NULL);
>>>>
>>>
>>>
>>> Why not drop DEBUG_BIOS completely?  If you want to debug the bios, use
>>> debugcon.
>>
>> Probably true. There is more practically dead stuff below this that just
>> prints if DEBUG_BIOS is enabled.
> 
> Actually it is autotest that is at fault here.  It is installing a
> debugcon with non-standard iobase atop a builtin device.
> Pre-memory-API, we did not detect that.

Well, the cruft in pc.c was disabled in practice, just leaving useless
/dev/null-like ioports behind. I still think we should drop all of them,
they have no meaning.

> 
> We can subclass isa-debugcon as bochs-debugcon, change the default
> ioport to 0x402, and use that instead of the code in pc.c.  How does
> that sound?
> 
> Autotest will need to be changed to use the new device type.
> 

That is another story: providing a debugcon that defaults to 0x402, the
port I always forgot when I want to debug the BIOS. We could provide
bochs-debugcon in addition to the existing interface, avoiding autotest
breakage and still making the usage smoother.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH] pc: Don't listen on debug ports by default

[Andreas Färber]

Am 11.09.2012 16:26, schrieb Jan Kiszka:
> On 2012-09-11 16:19, Avi Kivity wrote:
>> On 09/11/2012 05:11 PM, Jan Kiszka wrote:
>>> On 2012-09-11 14:53, Avi Kivity wrote:
>>>> On 09/11/2012 02:57 PM, Jan Kiszka wrote:
>>>>
>>>>> Only listen on debug ports when we also handle them. They are better
>>>>> handled by debugcon these days which is runtime configurable.
>>>>>
>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>>> ---
>>>>>  hw/pc.c |    6 ++++--
>>>>>  1 files changed, 4 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/hw/pc.c b/hw/pc.c
>>>>> index 112739a..134d5f7 100644
>>>>> --- a/hw/pc.c
>>>>> +++ b/hw/pc.c
>>>>> @@ -539,12 +539,12 @@ static void bochs_bios_write(void *opaque, uint32_t addr, uint32_t val)
>>>>>      case 0x401:
>>>>>          /* used to be panic, now unused */
>>>>>          break;
>>>>> +#ifdef DEBUG_BIOS
>>>>>      case 0x402:
>>>>>      case 0x403:
>>>>> -#ifdef DEBUG_BIOS
>>>>>          fprintf(stderr, "%c", val);
>>>>> -#endif
>>>>>          break;
>>>>> +#endif
>>>>>      case 0x8900:
>>>>>          /* same as Bochs power off */
>>>>>          if (val == shutdown_str[shutdown_index]) {
>>>>> @@ -598,8 +598,10 @@ static void *bochs_bios_init(void)
>>>>>  
>>>>>      register_ioport_write(0x400, 1, 2, bochs_bios_write, NULL);
>>>>>      register_ioport_write(0x401, 1, 2, bochs_bios_write, NULL);
>>>>> +#ifdef DEBUG_BIOS
>>>>>      register_ioport_write(0x402, 1, 1, bochs_bios_write, NULL);
>>>>>      register_ioport_write(0x403, 1, 1, bochs_bios_write, NULL);
>>>>> +#endif
>>>>>      register_ioport_write(0x8900, 1, 1, bochs_bios_write, NULL);
>>>>>  
>>>>>      register_ioport_write(0x501, 1, 1, bochs_bios_write, NULL);
>>>>>
>>>>
>>>>
>>>> Why not drop DEBUG_BIOS completely?  If you want to debug the bios, use
>>>> debugcon.
>>>
>>> Probably true. There is more practically dead stuff below this that just
>>> prints if DEBUG_BIOS is enabled.
>>
>> Actually it is autotest that is at fault here.  It is installing a
>> debugcon with non-standard iobase atop a builtin device.
>> Pre-memory-API, we did not detect that.
> 
> Well, the cruft in pc.c was disabled in practice, just leaving useless
> /dev/null-like ioports behind. I still think we should drop all of them,
> they have no meaning.
> 
>>
>> We can subclass isa-debugcon as bochs-debugcon, change the default
>> ioport to 0x402, and use that instead of the code in pc.c.  How does
>> that sound?
>>
>> Autotest will need to be changed to use the new device type.
>>
> 
> That is another story: providing a debugcon that defaults to 0x402, the
> port I always forgot when I want to debug the BIOS. We could provide
> bochs-debugcon in addition to the existing interface, avoiding autotest
> breakage and still making the usage smoother.

Hervé and Anthony had discussed that for qemu-test and I thought their
conclusion was to use debug-con with the iobase parameter? CC'ing.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH V9 0/8] memory: unify ioport registration

[Avi Kivity]

On 09/04/2012 06:13 PM, Julien Grall wrote:
> This is the nineth version of patch series about ioport registration.
> 
> Some part of QEMU still use register_ioport* functions to register ioport.
> These functions doesn't allow to use Memory Listener on it.

Jan't patch that dropped the debug ioports causes conflicts, please
rebase atop upstream.


-- 
error compiling committee.c: too many arguments to function