| Message ID | 1312808972-1718-27-git-send-email-avi@redhat.com |
|---|---|
| State | New |
| Headers | show |
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.
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
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.
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
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
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).
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;