[Qemu-devel] [PATCH V5 0/8] memory: unifiy ioport registration

[Qemu-devel] [PATCH V5 0/8] memory: unifiy ioport registration

[Julien Grall]

This is the fifth version of patch series about ioport registration.
The fourth version was sent few months ago
(https://lists.gnu.org/archive/html/qemu-devel/2012-04/msg01396.html).

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 V4:
   - Rebase on qemu upstream.
   - Forget some ioport_register_* in acpi_piix4.c.
   - Register 0x3b0 - 0x3df range for cirrus instead of ioport by ioport.

Julien Grall (8):
  isa: add isa_address_space_io
  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/apm.c: replace register_ioport*
  smb: replace_register_ioport*

 hw/acpi_piix4.c   |  160 +++++++++++++++++++++++++++++++++++++++++------------
 hw/apm.c          |   24 ++++++--
 hw/apm.h          |    5 +-
 hw/cirrus_vga.c   |   42 ++++++++------
 hw/dma.c          |  108 ++++++++++++++++++++++++------------
 hw/isa-bus.c      |    5 ++
 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, 325 insertions(+), 124 deletions(-)

-- 
Julien Grall

[Qemu-devel] [PATCH V5 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 |    5 +++++
 hw/isa.h     |    1 +
 2 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/hw/isa-bus.c b/hw/isa-bus.c
index f9b2373..662c86b 100644
--- a/hw/isa-bus.c
+++ b/hw/isa-bus.c
@@ -244,4 +244,9 @@ MemoryRegion *isa_address_space(ISADevice *dev)
     return get_system_memory();
 }
 
+MemoryRegion *isa_address_space_io(ISADevice *dev)
+{
+    return get_system_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

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

[Andreas Färber]

Am 22.08.2012 14:27, schrieb 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 |    5 +++++
>  hw/isa.h     |    1 +
>  2 files changed, 6 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/isa-bus.c b/hw/isa-bus.c
> index f9b2373..662c86b 100644
> --- a/hw/isa-bus.c
> +++ b/hw/isa-bus.c
> @@ -244,4 +244,9 @@ MemoryRegion *isa_address_space(ISADevice *dev)
>      return get_system_memory();
>  }
>  
> +MemoryRegion *isa_address_space_io(ISADevice *dev)
> +{
> +    return get_system_io();
> +}

Unlike the address_space above, there's an address_space_io field in
ISABus, so I guess the implementation of this function should rather
obtain the device's BusState via isa_bus_from_device(dev) and return its
field rather than hardcoding get_system_io() here.

For x86 it shouldn't make a difference but I think on PReP there's two
different runtime-switchable I/O space configurations or so...

Regards,
Andreas

> +
>  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);

-- 
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 V5 1/8] isa: add isa_address_space_io

[Julien Grall]

On 08/24/2012 05:10 PM, Andreas Färber wrote:
> Am 22.08.2012 14:27, schrieb 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 |    5 +++++
>>   hw/isa.h     |    1 +
>>   2 files changed, 6 insertions(+), 0 deletions(-)
>>
>> diff --git a/hw/isa-bus.c b/hw/isa-bus.c
>> index f9b2373..662c86b 100644
>> --- a/hw/isa-bus.c
>> +++ b/hw/isa-bus.c
>> @@ -244,4 +244,9 @@ MemoryRegion *isa_address_space(ISADevice *dev)
>>       return get_system_memory();
>>   }
>>
>> +MemoryRegion *isa_address_space_io(ISADevice *dev)
>> +{
>> +    return get_system_io();
>> +}
>>      
> Unlike the address_space above, there's an address_space_io field in
> ISABus, so I guess the implementation of this function should rather
> obtain the device's BusState via isa_bus_from_device(dev) and return its
> field rather than hardcoding get_system_io() here.
>    
I use this function in hw/dma.c. For the moment, the code doesn't
use ISA device, so I pass NULL to isa_address_space_io
(See patch 6).

Instead of using isa_bus_from_device, can I just return
"isabus->address_space_io" as in isa_register_portio_list ?

-- 
Julien Grall

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

[Jan Kiszka]

On 2012-08-28 17:42, Julien Grall wrote:
> On 08/24/2012 05:10 PM, Andreas Färber wrote:
>> Am 22.08.2012 14:27, schrieb 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 |    5 +++++
>>>   hw/isa.h     |    1 +
>>>   2 files changed, 6 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/hw/isa-bus.c b/hw/isa-bus.c
>>> index f9b2373..662c86b 100644
>>> --- a/hw/isa-bus.c
>>> +++ b/hw/isa-bus.c
>>> @@ -244,4 +244,9 @@ MemoryRegion *isa_address_space(ISADevice *dev)
>>>       return get_system_memory();
>>>   }
>>>
>>> +MemoryRegion *isa_address_space_io(ISADevice *dev)
>>> +{
>>> +    return get_system_io();
>>> +}
>>>      
>> Unlike the address_space above, there's an address_space_io field in
>> ISABus, so I guess the implementation of this function should rather
>> obtain the device's BusState via isa_bus_from_device(dev) and return its
>> field rather than hardcoding get_system_io() here.
>>    
> I use this function in hw/dma.c. For the moment, the code doesn't
> use ISA device, so I pass NULL to isa_address_space_io
> (See patch 6).

Yes, there are some old drivers in QEMU that doesn't follow QOM or even
qdev patterns. They may not work on systems Andreas is thinking of anyway.

But this is a generic service, so it should try harder: If a device is
given, use the io space of its bus. If not, fall back to isabus.

Jan

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

[Qemu-devel] [PATCH V5 2/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 |  160 ++++++++++++++++++++++++++++++++++++++++++------------
 1 files changed, 124 insertions(+), 36 deletions(-)

diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
index 0aace60..26d5559 100644
--- a/hw/acpi_piix4.c
+++ b/hw/acpi_piix4.c
@@ -28,6 +28,7 @@
 #include "range.h"
 #include "ioport.h"
 #include "fw_cfg.h"
+#include "exec-memory.h"
 
 //#define DEBUG
 
@@ -41,8 +42,7 @@
 
 #define GPE_BASE 0xafe0
 #define GPE_LEN 4
-#define PCI_UP_BASE 0xae00
-#define PCI_DOWN_BASE 0xae04
+#define PCI_BASE 0xaa00
 #define PCI_EJ_BASE 0xae08
 #define PCI_RMV_BASE 0xae0c
 
@@ -55,7 +55,7 @@ struct pci_status {
 
 typedef struct PIIX4PMState {
     PCIDevice dev;
-    IORange ioport;
+    MemoryRegion pm_io;
     ACPIREGS ar;
 
     APMState apm;
@@ -63,6 +63,13 @@ typedef struct PIIX4PMState {
     PMSMBus smb;
     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;
     int kvm_enabled;
@@ -108,12 +115,12 @@ 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 size)
 {
-    PIIX4PMState *s = container_of(ioport, PIIX4PMState, ioport);
+    PIIX4PMState *s = opaque;
 
-    if (width != 2) {
+    if (size != 2) {
         PIIX4_DPRINTF("PM write port=0x%04x width=%d val=0x%08x\n",
                       (unsigned)addr, width, (unsigned)val);
     }
@@ -137,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 size)
 {
-    PIIX4PMState *s = container_of(ioport, PIIX4PMState, ioport);
-    uint32_t val;
+    PIIX4PMState *s = opaque;
+    uint64_t val;
 
     switch(addr) {
     case 0x00:
@@ -161,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_NATIVE_ENDIAN,
+    .impl = {
+        .min_access_size = 2,
+        .max_access_size = 2,
+    },
 };
 
 static void apm_ctrl_changed(uint32_t val, void *arg)
@@ -183,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);
 }
@@ -198,8 +212,10 @@ 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_init_io(&s->pm_io, &pm_io_ops, s, "piix4-pm", 64);
+        memory_region_add_subregion(pci_address_space_io(&s->dev),
+                                    pm_io_base, &s->pm_io);
     }
 }
 
@@ -381,6 +397,25 @@ 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_NATIVE_ENDIAN,
+    .impl = {
+        .min_access_size = 1,
+        .max_access_size = 1,
+    },
+};
+
+static const MemoryRegionOps acpi_io_ops = {
+    .write = acpi_dbg_writel,
+    .endianness = DEVICE_NATIVE_ENDIAN,
+    .impl = {
+        .min_access_size = 4,
+        .max_access_size = 4,
+    },
+};
+
 static int piix4_pm_initfn(PCIDevice *dev)
 {
     PIIX4PMState *s = DO_UPCAST(PIIX4PMState, dev, dev);
@@ -395,9 +430,11 @@ static int piix4_pm_initfn(PCIDevice *dev)
     pci_conf[0x40] = 0x01; /* PM io base read only bit */
 
     /* 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);
+    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
@@ -410,8 +447,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);
@@ -496,16 +535,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;
 
@@ -537,21 +577,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;
 
@@ -561,20 +604,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_NATIVE_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_NATIVE_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

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

[Jan Kiszka]

On 2012-08-22 14: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 |  160 ++++++++++++++++++++++++++++++++++++++++++------------
>  1 files changed, 124 insertions(+), 36 deletions(-)
> 

...

> @@ -198,8 +212,10 @@ 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_init_io(&s->pm_io, &pm_io_ops, s, "piix4-pm", 64);
> +        memory_region_add_subregion(pci_address_space_io(&s->dev),
> +                                    pm_io_base, &s->pm_io);

This was broken before, but now I'm worried a guest can even crash qemu
by updating an existing mapping. So you will have to
 - initialize pm_io only once
 - track the active address
 - remove a registered region before registering a new one

Didn't look at the spec, but I bet that (config[0x80] & 1) == 0 means
disable this mapping. Should be fixed as well if that is true.

Jan

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

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

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 11266 bytes --]

On 2012-08-22 14: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 |  160 ++++++++++++++++++++++++++++++++++++++++++------------
>  1 files changed, 124 insertions(+), 36 deletions(-)
> 
> diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
> index 0aace60..26d5559 100644
> --- a/hw/acpi_piix4.c
> +++ b/hw/acpi_piix4.c
> @@ -28,6 +28,7 @@
>  #include "range.h"
>  #include "ioport.h"
>  #include "fw_cfg.h"
> +#include "exec-memory.h"
>  
>  //#define DEBUG
>  
> @@ -41,8 +42,7 @@
>  
>  #define GPE_BASE 0xafe0
>  #define GPE_LEN 4
> -#define PCI_UP_BASE 0xae00
> -#define PCI_DOWN_BASE 0xae04
> +#define PCI_BASE 0xaa00
>  #define PCI_EJ_BASE 0xae08
>  #define PCI_RMV_BASE 0xae0c
>  
> @@ -55,7 +55,7 @@ struct pci_status {
>  
>  typedef struct PIIX4PMState {
>      PCIDevice dev;
> -    IORange ioport;
> +    MemoryRegion pm_io;
>      ACPIREGS ar;
>  
>      APMState apm;
> @@ -63,6 +63,13 @@ typedef struct PIIX4PMState {
>      PMSMBus smb;
>      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;
>      int kvm_enabled;
> @@ -108,12 +115,12 @@ 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 size)
>  {
> -    PIIX4PMState *s = container_of(ioport, PIIX4PMState, ioport);
> +    PIIX4PMState *s = opaque;
>  
> -    if (width != 2) {
> +    if (size != 2) {
>          PIIX4_DPRINTF("PM write port=0x%04x width=%d val=0x%08x\n",
>                        (unsigned)addr, width, (unsigned)val);
>      }
> @@ -137,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 size)
>  {
> -    PIIX4PMState *s = container_of(ioport, PIIX4PMState, ioport);
> -    uint32_t val;
> +    PIIX4PMState *s = opaque;
> +    uint64_t val;
>  
>      switch(addr) {
>      case 0x00:
> @@ -161,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_NATIVE_ENDIAN,
> +    .impl = {
> +        .min_access_size = 2,
> +        .max_access_size = 2,
> +    },
>  };
>  
>  static void apm_ctrl_changed(uint32_t val, void *arg)
> @@ -183,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);
>  }
> @@ -198,8 +212,10 @@ 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_init_io(&s->pm_io, &pm_io_ops, s, "piix4-pm", 64);
> +        memory_region_add_subregion(pci_address_space_io(&s->dev),
> +                                    pm_io_base, &s->pm_io);
>      }
>  }
>  
> @@ -381,6 +397,25 @@ 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_NATIVE_ENDIAN,
> +    .impl = {
> +        .min_access_size = 1,
> +        .max_access_size = 1,
> +    },
> +};
> +
> +static const MemoryRegionOps acpi_io_ops = {
> +    .write = acpi_dbg_writel,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +    .impl = {
> +        .min_access_size = 4,
> +        .max_access_size = 4,
> +    },
> +};
> +
>  static int piix4_pm_initfn(PCIDevice *dev)
>  {
>      PIIX4PMState *s = DO_UPCAST(PIIX4PMState, dev, dev);
> @@ -395,9 +430,11 @@ static int piix4_pm_initfn(PCIDevice *dev)
>      pci_conf[0x40] = 0x01; /* PM io base read only bit */
>  
>      /* 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);
> +    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
> @@ -410,8 +447,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);
> @@ -496,16 +535,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;
>  
> @@ -537,21 +577,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;
>  
> @@ -561,20 +604,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_NATIVE_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_NATIVE_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);
>  }
> 

This patch doesn't build without patch 8 and then still generates warnings.

Jan



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

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

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 11746 bytes --]

On 2012-08-26 11:10, Jan Kiszka wrote:
> On 2012-08-22 14: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 |  160 ++++++++++++++++++++++++++++++++++++++++++------------
>>  1 files changed, 124 insertions(+), 36 deletions(-)
>>
>> diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
>> index 0aace60..26d5559 100644
>> --- a/hw/acpi_piix4.c
>> +++ b/hw/acpi_piix4.c
>> @@ -28,6 +28,7 @@
>>  #include "range.h"
>>  #include "ioport.h"
>>  #include "fw_cfg.h"
>> +#include "exec-memory.h"
>>  
>>  //#define DEBUG
>>  
>> @@ -41,8 +42,7 @@
>>  
>>  #define GPE_BASE 0xafe0
>>  #define GPE_LEN 4
>> -#define PCI_UP_BASE 0xae00
>> -#define PCI_DOWN_BASE 0xae04
>> +#define PCI_BASE 0xaa00
>>  #define PCI_EJ_BASE 0xae08
>>  #define PCI_RMV_BASE 0xae0c
>>  
>> @@ -55,7 +55,7 @@ struct pci_status {
>>  
>>  typedef struct PIIX4PMState {
>>      PCIDevice dev;
>> -    IORange ioport;
>> +    MemoryRegion pm_io;
>>      ACPIREGS ar;
>>  
>>      APMState apm;
>> @@ -63,6 +63,13 @@ typedef struct PIIX4PMState {
>>      PMSMBus smb;
>>      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;
>>      int kvm_enabled;
>> @@ -108,12 +115,12 @@ 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 size)
>>  {
>> -    PIIX4PMState *s = container_of(ioport, PIIX4PMState, ioport);
>> +    PIIX4PMState *s = opaque;
>>  
>> -    if (width != 2) {
>> +    if (size != 2) {
>>          PIIX4_DPRINTF("PM write port=0x%04x width=%d val=0x%08x\n",
>>                        (unsigned)addr, width, (unsigned)val);
>>      }
>> @@ -137,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 size)
>>  {
>> -    PIIX4PMState *s = container_of(ioport, PIIX4PMState, ioport);
>> -    uint32_t val;
>> +    PIIX4PMState *s = opaque;
>> +    uint64_t val;
>>  
>>      switch(addr) {
>>      case 0x00:
>> @@ -161,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_NATIVE_ENDIAN,
>> +    .impl = {
>> +        .min_access_size = 2,
>> +        .max_access_size = 2,
>> +    },
>>  };
>>  
>>  static void apm_ctrl_changed(uint32_t val, void *arg)
>> @@ -183,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);
>>  }
>> @@ -198,8 +212,10 @@ 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_init_io(&s->pm_io, &pm_io_ops, s, "piix4-pm", 64);
>> +        memory_region_add_subregion(pci_address_space_io(&s->dev),
>> +                                    pm_io_base, &s->pm_io);
>>      }
>>  }
>>  
>> @@ -381,6 +397,25 @@ 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_NATIVE_ENDIAN,
>> +    .impl = {
>> +        .min_access_size = 1,
>> +        .max_access_size = 1,
>> +    },
>> +};
>> +
>> +static const MemoryRegionOps acpi_io_ops = {
>> +    .write = acpi_dbg_writel,
>> +    .endianness = DEVICE_NATIVE_ENDIAN,
>> +    .impl = {
>> +        .min_access_size = 4,
>> +        .max_access_size = 4,
>> +    },
>> +};
>> +
>>  static int piix4_pm_initfn(PCIDevice *dev)
>>  {
>>      PIIX4PMState *s = DO_UPCAST(PIIX4PMState, dev, dev);
>> @@ -395,9 +430,11 @@ static int piix4_pm_initfn(PCIDevice *dev)
>>      pci_conf[0x40] = 0x01; /* PM io base read only bit */
>>  
>>      /* 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);
>> +    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
>> @@ -410,8 +447,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);
>> @@ -496,16 +535,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;
>>  
>> @@ -537,21 +577,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;
>>  
>> @@ -561,20 +604,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_NATIVE_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_NATIVE_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);
>>  }
>>
> 
> This patch doesn't build without patch 8 and then still generates warnings.

Sorry, it depends on patches 7 & 8. Then the warnings are gone as well.
Anyway, every patch has to build and work on its own.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

[Qemu-devel] [PATCH V5 3/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 |   42 ++++++++++++++++++++++++------------------
 1 files changed, 24 insertions(+), 18 deletions(-)

diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
index e8dcc6b..adfc855 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;
@@ -2528,12 +2529,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;
@@ -2657,7 +2661,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 +2787,18 @@ static const MemoryRegionOps cirrus_linear_io_ops = {
     },
 };
 
+static const MemoryRegionOps cirrus_vga_io_ops = {
+    .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 +2830,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 +2901,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 +2943,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

Re: [Qemu-devel] [PATCH V5 3/8] hw/cirrus_vga.c: replace register_ioport*

[Jan Kiszka]

On 2012-08-22 14:27, Julien Grall wrote:
> 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 |   42 ++++++++++++++++++++++++------------------
>  1 files changed, 24 insertions(+), 18 deletions(-)
> 
> diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
> index e8dcc6b..adfc855 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;
> @@ -2528,12 +2529,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;
> @@ -2657,7 +2661,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 +2787,18 @@ static const MemoryRegionOps cirrus_linear_io_ops = {
>      },
>  };
>  
> +static const MemoryRegionOps cirrus_vga_io_ops = {
> +    .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 +2830,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);

Can't imagine that this reflects the original ranges and constraints
correctly. Or were they all wrong?

Jan

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

Re: [Qemu-devel] [PATCH V5 3/8] hw/cirrus_vga.c: replace register_ioport*

[Julien Grall]

On 08/24/2012 02:44 PM, Jan Kiszka wrote:
> On 2012-08-22 14:27, Julien Grall wrote:
>    
>> 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 |   42 ++++++++++++++++++++++++------------------
>>   1 files changed, 24 insertions(+), 18 deletions(-)
>>
>> diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
>> index e8dcc6b..adfc855 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;
>> @@ -2528,12 +2529,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;
>> @@ -2657,7 +2661,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 +2787,18 @@ static const MemoryRegionOps cirrus_linear_io_ops = {
>>       },
>>   };
>>
>> +static const MemoryRegionOps cirrus_vga_io_ops = {
>> +    .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 +2830,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);
>>      
> Can't imagine that this reflects the original ranges and constraints
> correctly. Or were they all wrong?
>
>    

I made a version (V4) with the same mapping, but Anthony has
proposed to register 0x3b0 - 0x3df
(https://lists.gnu.org/archive/html/qemu-devel/2012-04/msg03329.html)

I don't see a problem, and it works on my computer.

By the way, I will resend this patch because I forget read access in
MemoryRegionOps. Sorry.

> Jan
>
>    

Re: [Qemu-devel] [PATCH V5 3/8] hw/cirrus_vga.c: replace register_ioport*

[Jan Kiszka]

On 2012-08-24 16:49, Julien Grall wrote:
> On 08/24/2012 02:44 PM, Jan Kiszka wrote:
>> On 2012-08-22 14:27, Julien Grall wrote:
>>    
>>> 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 |   42 ++++++++++++++++++++++++------------------
>>>   1 files changed, 24 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
>>> index e8dcc6b..adfc855 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;
>>> @@ -2528,12 +2529,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;
>>> @@ -2657,7 +2661,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 +2787,18 @@ static const MemoryRegionOps cirrus_linear_io_ops = {
>>>       },
>>>   };
>>>
>>> +static const MemoryRegionOps cirrus_vga_io_ops = {
>>> +    .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 +2830,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);
>>>      
>> Can't imagine that this reflects the original ranges and constraints
>> correctly. Or were they all wrong?
>>
>>    
> 
> I made a version (V4) with the same mapping, but Anthony has
> proposed to register 0x3b0 - 0x3df
> (https://lists.gnu.org/archive/html/qemu-devel/2012-04/msg03329.html)

Yes, likely no problem, the handlers seem to catch invalid accesses.

> 
> I don't see a problem, and it works on my computer.
> 
> By the way, I will resend this patch because I forget read access in
> MemoryRegionOps. Sorry.

Well, the fix for patch 2 is also still required. ;)

I'm currently working on removing the remaining register_ioport users as
I'd like to tweak the portio interface. I will follow up on your series.

Jan

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

Re: [Qemu-devel] [PATCH V5 3/8] hw/cirrus_vga.c: replace register_ioport*

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2012 bytes --]

On 2012-08-22 14:27, Julien Grall wrote:
> 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 |   42 ++++++++++++++++++++++++------------------
>  1 files changed, 24 insertions(+), 18 deletions(-)
> 
> diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
> index e8dcc6b..adfc855 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;
> @@ -2528,12 +2529,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;
> @@ -2657,7 +2661,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 +2787,18 @@ static const MemoryRegionOps cirrus_linear_io_ops = {
>      },
>  };
>  
> +static const MemoryRegionOps cirrus_vga_io_ops = {
> +    .write = cirrus_vga_ioport_write,

Missing .read. Crashes immediately when you test it.

Jan



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

[Qemu-devel] [PATCH V5 4/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 V5 5/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..f65abd8 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_address_space_io(NULL), 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_address_space_io(NULL), 0xf0, ioportF0_io);
 
     /*
      * Check if an HPET shall be created.
-- 
Julien Grall

[Qemu-devel] [PATCH V5 6/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

[Qemu-devel] [PATCH V5 7/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/apm.c      |   24 +++++++++++++++++++-----
 hw/apm.h      |    5 ++++-
 hw/vt82c686.c |    2 +-
 3 files changed, 24 insertions(+), 7 deletions(-)

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 V5 8/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/pm_smbus.c |    7 ++++---
 hw/pm_smbus.h |    6 ++++--
 hw/vt82c686.c |   18 ++++++++++++++++--
 3 files changed, 24 insertions(+), 7 deletions(-)

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..6b35155 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,
+    .endianess = DEVICE_NATIVE_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

Re: [Qemu-devel] [PATCH V5 8/8] smb: replace_register_ioport*

[Jan Kiszka]

On 2012-08-22 14:27, Julien Grall wrote:
> 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/pm_smbus.c |    7 ++++---
>  hw/pm_smbus.h |    6 ++++--
>  hw/vt82c686.c |   18 ++++++++++++++++--
>  3 files changed, 24 insertions(+), 7 deletions(-)
> 
> 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..6b35155 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,
> +    .endianess = DEVICE_NATIVE_ENDIAN,
        ^^^^^^^^^
Typo. Please ensure that you build-test everything (full QEMU build).

> +    .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);
>  
> 

This file is only half-converted. Working on the other bits...

Jan

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

Re: [Qemu-devel] [PATCH V5 0/8] memory: unifiy ioport registration

[Jan Kiszka]

On 2012-08-22 14:27, Julien Grall wrote:
> This is the fifth version of patch series about ioport registration.
> The fourth version was sent few months ago
> (https://lists.gnu.org/archive/html/qemu-devel/2012-04/msg01396.html).
> 
> 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 V4:
>    - Rebase on qemu upstream.
>    - Forget some ioport_register_* in acpi_piix4.c.
>    - Register 0x3b0 - 0x3df range for cirrus instead of ioport by ioport.
> 
> Julien Grall (8):
>   isa: add isa_address_space_io
>   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/apm.c: replace register_ioport*
>   smb: replace_register_ioport*
> 
>  hw/acpi_piix4.c   |  160 +++++++++++++++++++++++++++++++++++++++++------------
>  hw/apm.c          |   24 ++++++--
>  hw/apm.h          |    5 +-
>  hw/cirrus_vga.c   |   42 ++++++++------
>  hw/dma.c          |  108 ++++++++++++++++++++++++------------
>  hw/isa-bus.c      |    5 ++
>  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, 325 insertions(+), 124 deletions(-)
> 

Nice, only few users of register_ioport_read/write remaining after this.
Hope we can drop those functions soon.

Jan

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