Patchwork [v4,26/39] pcnet: convert to memory API

login
register
mail settings
Submitter Avi Kivity
Date Aug. 8, 2011, 1:09 p.m.
Message ID <1312808972-1718-27-git-send-email-avi@redhat.com>
Download mbox | patch
Permalink /patch/108936/
State New
Headers show

Comments

Avi Kivity - Aug. 8, 2011, 1:09 p.m.
Also related chips.

Reviewed-by: Richard Henderson <rth@twiddle.net>
Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>
Signed-off-by: Avi Kivity <avi@redhat.com>
---
 hw/lance.c     |   31 ++++++++++-------------
 hw/pcnet-pci.c |   74 +++++++++++++++++++++++++++++++++----------------------
 hw/pcnet.h     |    4 ++-
 3 files changed, 61 insertions(+), 48 deletions(-)
Avi Kivity - Aug. 9, 2011, 6:52 a.m.
On 08/09/2011 09:55 AM, Bob Breuer wrote:
> >   static void lance_cleanup(VLANClientState *nc)
> >  @@ -117,13 +116,11 @@ static int lance_init(SysBusDevice *dev)
> >       SysBusPCNetState *d = FROM_SYSBUS(SysBusPCNetState, dev);
> >       PCNetState *s =&d->state;
> >
> >  -    s->mmio_index =
> >  -        cpu_register_io_memory(lance_mem_read, lance_mem_write, d,
> >  -                               DEVICE_NATIVE_ENDIAN);
> >  +    memory_region_init_io(&s->mmio,&lance_mem_ops, s, "lance-mmio", 4);
>
> You've switched up d and s here, so anything that tries to talk to the
> ethernet, such as a sparc32 guest, will now cause Qemu to segfault.
>
>

Good catch; will post a fix.

Maybe keeping the opaque wasn't such a good idea.
Bob Breuer - Aug. 9, 2011, 6:55 a.m.
Avi Kivity wrote:
> Also related chips.
> 
> Reviewed-by: Richard Henderson <rth@twiddle.net>
> Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>
> Signed-off-by: Avi Kivity <avi@redhat.com>
> ---
>  hw/lance.c     |   31 ++++++++++-------------
>  hw/pcnet-pci.c |   74 +++++++++++++++++++++++++++++++++----------------------
>  hw/pcnet.h     |    4 ++-
>  3 files changed, 61 insertions(+), 48 deletions(-)
> 
> diff --git a/hw/lance.c b/hw/lance.c
> index ddb1cbb..8e20360 100644
> --- a/hw/lance.c
> +++ b/hw/lance.c
> @@ -55,8 +55,8 @@ static void parent_lance_reset(void *opaque, int irq, int level)
>          pcnet_h_reset(&d->state);
>  }
>  
> -static void lance_mem_writew(void *opaque, target_phys_addr_t addr,
> -                             uint32_t val)
> +static void lance_mem_write(void *opaque, target_phys_addr_t addr,
> +                            uint64_t val, unsigned size)
>  {
>      SysBusPCNetState *d = opaque;
>  
> @@ -64,7 +64,8 @@ static void lance_mem_writew(void *opaque, target_phys_addr_t addr,
>      pcnet_ioport_writew(&d->state, addr, val & 0xffff);
>  }
>  
> -static uint32_t lance_mem_readw(void *opaque, target_phys_addr_t addr)
> +static uint64_t lance_mem_read(void *opaque, target_phys_addr_t addr,
> +                               unsigned size)
>  {
>      SysBusPCNetState *d = opaque;
>      uint32_t val;
> @@ -74,16 +75,14 @@ static uint32_t lance_mem_readw(void *opaque, target_phys_addr_t addr)
>      return val & 0xffff;
>  }
>  
> -static CPUReadMemoryFunc * const lance_mem_read[3] = {
> -    NULL,
> -    lance_mem_readw,
> -    NULL,
> -};
> -
> -static CPUWriteMemoryFunc * const lance_mem_write[3] = {
> -    NULL,
> -    lance_mem_writew,
> -    NULL,
> +static const MemoryRegionOps lance_mem_ops = {
> +    .read = lance_mem_read,
> +    .write = lance_mem_write,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +    .valid = {
> +        .min_access_size = 2,
> +        .max_access_size = 2,
> +    },
>  };
>  
>  static void lance_cleanup(VLANClientState *nc)
> @@ -117,13 +116,11 @@ static int lance_init(SysBusDevice *dev)
>      SysBusPCNetState *d = FROM_SYSBUS(SysBusPCNetState, dev);
>      PCNetState *s = &d->state;
>  
> -    s->mmio_index =
> -        cpu_register_io_memory(lance_mem_read, lance_mem_write, d,
> -                               DEVICE_NATIVE_ENDIAN);
> +    memory_region_init_io(&s->mmio, &lance_mem_ops, s, "lance-mmio", 4);

You've switched up d and s here, so anything that tries to talk to the
ethernet, such as a sparc32 guest, will now cause Qemu to segfault.

Bob
Michael S. Tsirkin - Aug. 9, 2011, 12:42 p.m.
On Tue, Aug 09, 2011 at 09:52:17AM +0300, Avi Kivity wrote:
> On 08/09/2011 09:55 AM, Bob Breuer wrote:
> >>   static void lance_cleanup(VLANClientState *nc)
> >>  @@ -117,13 +116,11 @@ static int lance_init(SysBusDevice *dev)
> >>       SysBusPCNetState *d = FROM_SYSBUS(SysBusPCNetState, dev);
> >>       PCNetState *s =&d->state;
> >>
> >>  -    s->mmio_index =
> >>  -        cpu_register_io_memory(lance_mem_read, lance_mem_write, d,
> >>  -                               DEVICE_NATIVE_ENDIAN);
> >>  +    memory_region_init_io(&s->mmio,&lance_mem_ops, s, "lance-mmio", 4);
> >
> >You've switched up d and s here, so anything that tries to talk to the
> >ethernet, such as a sparc32 guest, will now cause Qemu to segfault.
> >
> >
> 
> Good catch; will post a fix.
> 
> Maybe keeping the opaque wasn't such a good idea.

Yes, we typically can get from the mmio to the device state
using container_of.

> -- 
> I have a truly marvellous patch that fixes the bug which this
> signature is too narrow to contain.
Avi Kivity - Aug. 9, 2011, 12:44 p.m.
On 08/09/2011 03:42 PM, Michael S. Tsirkin wrote:
> On Tue, Aug 09, 2011 at 09:52:17AM +0300, Avi Kivity wrote:
> >  On 08/09/2011 09:55 AM, Bob Breuer wrote:
> >  >>    static void lance_cleanup(VLANClientState *nc)
> >  >>   @@ -117,13 +116,11 @@ static int lance_init(SysBusDevice *dev)
> >  >>        SysBusPCNetState *d = FROM_SYSBUS(SysBusPCNetState, dev);
> >  >>        PCNetState *s =&d->state;
> >  >>
> >  >>   -    s->mmio_index =
> >  >>   -        cpu_register_io_memory(lance_mem_read, lance_mem_write, d,
> >  >>   -                               DEVICE_NATIVE_ENDIAN);
> >  >>   +    memory_region_init_io(&s->mmio,&lance_mem_ops, s, "lance-mmio", 4);
> >  >
> >  >You've switched up d and s here, so anything that tries to talk to the
> >  >ethernet, such as a sparc32 guest, will now cause Qemu to segfault.
> >  >
> >  >
> >
> >  Good catch; will post a fix.
> >
> >  Maybe keeping the opaque wasn't such a good idea.
>
> Yes, we typically can get from the mmio to the device state
> using container_of.
>
>

But in some cases, we can't, and the it's a pain having to wrap 
MemoryRegion in another structure containing an opaque.

Maybe a good compromise is to:

   - keep MemoryRegion::opaque
   - pass a MemoryRegion *mr to callbacks instead of opaque
   - use container_of() when possible
   - use mr->opaque otherwise
Michael S. Tsirkin - Aug. 9, 2011, 12:48 p.m.
On Tue, Aug 09, 2011 at 03:44:35PM +0300, Avi Kivity wrote:
> On 08/09/2011 03:42 PM, Michael S. Tsirkin wrote:
> >On Tue, Aug 09, 2011 at 09:52:17AM +0300, Avi Kivity wrote:
> >>  On 08/09/2011 09:55 AM, Bob Breuer wrote:
> >>  >>    static void lance_cleanup(VLANClientState *nc)
> >>  >>   @@ -117,13 +116,11 @@ static int lance_init(SysBusDevice *dev)
> >>  >>        SysBusPCNetState *d = FROM_SYSBUS(SysBusPCNetState, dev);
> >>  >>        PCNetState *s =&d->state;
> >>  >>
> >>  >>   -    s->mmio_index =
> >>  >>   -        cpu_register_io_memory(lance_mem_read, lance_mem_write, d,
> >>  >>   -                               DEVICE_NATIVE_ENDIAN);
> >>  >>   +    memory_region_init_io(&s->mmio,&lance_mem_ops, s, "lance-mmio", 4);
> >>  >
> >>  >You've switched up d and s here, so anything that tries to talk to the
> >>  >ethernet, such as a sparc32 guest, will now cause Qemu to segfault.
> >>  >
> >>  >
> >>
> >>  Good catch; will post a fix.
> >>
> >>  Maybe keeping the opaque wasn't such a good idea.
> >
> >Yes, we typically can get from the mmio to the device state
> >using container_of.
> >
> >
> 
> But in some cases, we can't, and the it's a pain having to wrap
> MemoryRegion in another structure containing an opaque.

I guess, even though that wrapping structure would
use a proper type, not an opaque.

> Maybe a good compromise is to:
> 
>   - keep MemoryRegion::opaque
>   - pass a MemoryRegion *mr to callbacks instead of opaque
>   - use container_of() when possible
>   - use mr->opaque otherwise

Right. This even saves a memory dereference when opaque is
unused.

> -- 
> error compiling committee.c: too many arguments to function
Avi Kivity - Aug. 9, 2011, 12:52 p.m.
On 08/09/2011 03:48 PM, Michael S. Tsirkin wrote:
> >
> >  But in some cases, we can't, and the it's a pain having to wrap
> >  MemoryRegion in another structure containing an opaque.
>
> I guess, even though that wrapping structure would
> use a proper type, not an opaque.

Yes, of course - that's what the first version did.

> >  Maybe a good compromise is to:
> >
> >    - keep MemoryRegion::opaque
> >    - pass a MemoryRegion *mr to callbacks instead of opaque
> >    - use container_of() when possible
> >    - use mr->opaque otherwise
>
> Right. This even saves a memory dereference when opaque is
> unused.
>

I'll put this on the TODO (as well as writing the TODO).

Patch

diff --git a/hw/lance.c b/hw/lance.c
index ddb1cbb..8e20360 100644
--- a/hw/lance.c
+++ b/hw/lance.c
@@ -55,8 +55,8 @@  static void parent_lance_reset(void *opaque, int irq, int level)
         pcnet_h_reset(&d->state);
 }
 
-static void lance_mem_writew(void *opaque, target_phys_addr_t addr,
-                             uint32_t val)
+static void lance_mem_write(void *opaque, target_phys_addr_t addr,
+                            uint64_t val, unsigned size)
 {
     SysBusPCNetState *d = opaque;
 
@@ -64,7 +64,8 @@  static void lance_mem_writew(void *opaque, target_phys_addr_t addr,
     pcnet_ioport_writew(&d->state, addr, val & 0xffff);
 }
 
-static uint32_t lance_mem_readw(void *opaque, target_phys_addr_t addr)
+static uint64_t lance_mem_read(void *opaque, target_phys_addr_t addr,
+                               unsigned size)
 {
     SysBusPCNetState *d = opaque;
     uint32_t val;
@@ -74,16 +75,14 @@  static uint32_t lance_mem_readw(void *opaque, target_phys_addr_t addr)
     return val & 0xffff;
 }
 
-static CPUReadMemoryFunc * const lance_mem_read[3] = {
-    NULL,
-    lance_mem_readw,
-    NULL,
-};
-
-static CPUWriteMemoryFunc * const lance_mem_write[3] = {
-    NULL,
-    lance_mem_writew,
-    NULL,
+static const MemoryRegionOps lance_mem_ops = {
+    .read = lance_mem_read,
+    .write = lance_mem_write,
+    .endianness = DEVICE_NATIVE_ENDIAN,
+    .valid = {
+        .min_access_size = 2,
+        .max_access_size = 2,
+    },
 };
 
 static void lance_cleanup(VLANClientState *nc)
@@ -117,13 +116,11 @@  static int lance_init(SysBusDevice *dev)
     SysBusPCNetState *d = FROM_SYSBUS(SysBusPCNetState, dev);
     PCNetState *s = &d->state;
 
-    s->mmio_index =
-        cpu_register_io_memory(lance_mem_read, lance_mem_write, d,
-                               DEVICE_NATIVE_ENDIAN);
+    memory_region_init_io(&s->mmio, &lance_mem_ops, s, "lance-mmio", 4);
 
     qdev_init_gpio_in(&dev->qdev, parent_lance_reset, 1);
 
-    sysbus_init_mmio(dev, 4, s->mmio_index);
+    sysbus_init_mmio_region(dev, &s->mmio);
 
     sysbus_init_irq(dev, &s->irq);
 
diff --git a/hw/pcnet-pci.c b/hw/pcnet-pci.c
index 216cf81..a25f565 100644
--- a/hw/pcnet-pci.c
+++ b/hw/pcnet-pci.c
@@ -46,6 +46,7 @@ 
 typedef struct {
     PCIDevice pci_dev;
     PCNetState state;
+    MemoryRegion io_bar;
 } PCIPCNetState;
 
 static void pcnet_aprom_writeb(void *opaque, uint32_t addr, uint32_t val)
@@ -69,25 +70,41 @@  static uint32_t pcnet_aprom_readb(void *opaque, uint32_t addr)
     return val;
 }
 
-static void pcnet_ioport_map(PCIDevice *pci_dev, int region_num,
-                             pcibus_t addr, pcibus_t size, int type)
+static uint64_t pcnet_ioport_read(void *opaque, target_phys_addr_t addr,
+                                  unsigned size)
 {
-    PCNetState *d = &DO_UPCAST(PCIPCNetState, pci_dev, pci_dev)->state;
+    PCNetState *d = opaque;
 
-#ifdef PCNET_DEBUG_IO
-    printf("pcnet_ioport_map addr=0x%04"FMT_PCIBUS" size=0x%04"FMT_PCIBUS"\n",
-           addr, size);
-#endif
+    if (addr < 16 && size == 1) {
+        return pcnet_aprom_readb(d, addr);
+    } else if (addr >= 0x10 && addr < 0x20 && size == 2) {
+        return pcnet_ioport_readw(d, addr);
+    } else if (addr >= 0x10 && addr < 0x20 && size == 4) {
+        return pcnet_ioport_readl(d, addr);
+    }
+    return ((uint64_t)1 << (size * 8)) - 1;
+}
 
-    register_ioport_write(addr, 16, 1, pcnet_aprom_writeb, d);
-    register_ioport_read(addr, 16, 1, pcnet_aprom_readb, d);
+static void pcnet_ioport_write(void *opaque, target_phys_addr_t addr,
+                               uint64_t data, unsigned size)
+{
+    PCNetState *d = opaque;
 
-    register_ioport_write(addr + 0x10, 0x10, 2, pcnet_ioport_writew, d);
-    register_ioport_read(addr + 0x10, 0x10, 2, pcnet_ioport_readw, d);
-    register_ioport_write(addr + 0x10, 0x10, 4, pcnet_ioport_writel, d);
-    register_ioport_read(addr + 0x10, 0x10, 4, pcnet_ioport_readl, d);
+    if (addr < 16 && size == 1) {
+        return pcnet_aprom_writeb(d, addr, data);
+    } else if (addr >= 0x10 && addr < 0x20 && size == 2) {
+        return pcnet_ioport_writew(d, addr, data);
+    } else if (addr >= 0x10 && addr < 0x20 && size == 4) {
+        return pcnet_ioport_writel(d, addr, data);
+    }
 }
 
+static const MemoryRegionOps pcnet_io_ops = {
+    .read = pcnet_ioport_read,
+    .write = pcnet_ioport_write,
+    .endianness = DEVICE_NATIVE_ENDIAN,
+};
+
 static void pcnet_mmio_writeb(void *opaque, target_phys_addr_t addr, uint32_t val)
 {
     PCNetState *d = opaque;
@@ -202,16 +219,12 @@  static const VMStateDescription vmstate_pci_pcnet = {
 
 /* PCI interface */
 
-static CPUWriteMemoryFunc * const pcnet_mmio_write[] = {
-    &pcnet_mmio_writeb,
-    &pcnet_mmio_writew,
-    &pcnet_mmio_writel
-};
-
-static CPUReadMemoryFunc * const pcnet_mmio_read[] = {
-    &pcnet_mmio_readb,
-    &pcnet_mmio_readw,
-    &pcnet_mmio_readl
+static const MemoryRegionOps pcnet_mmio_ops = {
+    .old_mmio = {
+        .read = { pcnet_mmio_readb, pcnet_mmio_readw, pcnet_mmio_readl },
+        .write = { pcnet_mmio_writeb, pcnet_mmio_writew, pcnet_mmio_writel },
+    },
+    .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
 static void pci_physical_memory_write(void *dma_opaque, target_phys_addr_t addr,
@@ -237,7 +250,8 @@  static int pci_pcnet_uninit(PCIDevice *dev)
 {
     PCIPCNetState *d = DO_UPCAST(PCIPCNetState, pci_dev, dev);
 
-    cpu_unregister_io_memory(d->state.mmio_index);
+    memory_region_destroy(&d->state.mmio);
+    memory_region_destroy(&d->io_bar);
     qemu_del_timer(d->state.poll_timer);
     qemu_free_timer(d->state.poll_timer);
     qemu_del_vlan_client(&d->state.nic->nc);
@@ -276,14 +290,14 @@  static int pci_pcnet_init(PCIDevice *pci_dev)
     pci_conf[PCI_MAX_LAT] = 0xff;
 
     /* Handler for memory-mapped I/O */
-    s->mmio_index =
-      cpu_register_io_memory(pcnet_mmio_read, pcnet_mmio_write, &d->state,
-                             DEVICE_NATIVE_ENDIAN);
+    memory_region_init_io(&d->state.mmio, &pcnet_mmio_ops, d, "pcnet-mmio",
+                          PCNET_PNPMMIO_SIZE);
 
-    pci_register_bar(pci_dev, 0, PCNET_IOPORT_SIZE,
-                           PCI_BASE_ADDRESS_SPACE_IO, pcnet_ioport_map);
+    memory_region_init_io(&d->io_bar, &pcnet_io_ops, d, "pcnet-io",
+                          PCNET_IOPORT_SIZE);
+    pci_register_bar_region(pci_dev, 0, PCI_BASE_ADDRESS_SPACE_IO, &d->io_bar);
 
-    pci_register_bar_simple(pci_dev, 1, PCNET_PNPMMIO_SIZE, 0, s->mmio_index);
+    pci_register_bar_region(pci_dev, 1, 0, &s->mmio);
 
     s->irq = pci_dev->irq[0];
     s->phys_mem_read = pci_physical_memory_read;
diff --git a/hw/pcnet.h b/hw/pcnet.h
index 534bdf9..7e1c685 100644
--- a/hw/pcnet.h
+++ b/hw/pcnet.h
@@ -4,6 +4,7 @@ 
 #define PCNET_LOOPTEST_CRC	1
 #define PCNET_LOOPTEST_NOCRC	2
 
+#include "memory.h"
 
 typedef struct PCNetState_st PCNetState;
 
@@ -17,7 +18,8 @@  struct PCNetState_st {
     uint16_t csr[128];
     uint16_t bcr[32];
     uint64_t timer;
-    int mmio_index, xmit_pos;
+    MemoryRegion mmio;
+    int xmit_pos;
     uint8_t buffer[4096];
     int tx_busy;
     qemu_irq irq;