Re: [Qemu-devel] [PATCH v4 23/39] lsi53c895a: convert to memory API

[Gerhard Wiesinger <lists@wiesinger.com>]

Hello Avi,

qemu/qemu-kvm doesn't boot any more with LSI option rom (-option-rom 
8xx_64.rom). Guess it comes from changes of the memory API. Also latest 
git version from seabios.

Sorry, no further details currently available.

Ciao,
Gerhard

--
http://www.wiesinger.com/


On Mon, 8 Aug 2011, Avi Kivity wrote:

> An optimization that fast-pathed DMA reads from the SCRIPTS memory
> was removed int the process.  Likely it breaks with iommus anyway.
>
> Reviewed-by: Richard Henderson <rth@twiddle.net>
> Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>
> Signed-off-by: Avi Kivity <avi@redhat.com>
> ---
> hw/lsi53c895a.c |  258 ++++++++++++-------------------------------------------
> 1 files changed, 56 insertions(+), 202 deletions(-)
>
> diff --git a/hw/lsi53c895a.c b/hw/lsi53c895a.c
> index e9904c4..0ab8c78 100644
> --- a/hw/lsi53c895a.c
> +++ b/hw/lsi53c895a.c
> @@ -185,9 +185,9 @@ typedef struct lsi_request {
>
> typedef struct {
>     PCIDevice dev;
> -    int mmio_io_addr;
> -    int ram_io_addr;
> -    uint32_t script_ram_base;
> +    MemoryRegion mmio_io;
> +    MemoryRegion ram_io;
> +    MemoryRegion io_io;
>
>     int carry; /* ??? Should this be an a visible register somewhere?  */
>     int status;
> @@ -391,10 +391,9 @@ static inline uint32_t read_dword(LSIState *s, uint32_t addr)
> {
>     uint32_t buf;
>
> -    /* Optimize reading from SCRIPTS RAM.  */
> -    if ((addr & 0xffffe000) == s->script_ram_base) {
> -        return s->script_ram[(addr & 0x1fff) >> 2];
> -    }
> +    /* XXX: an optimization here used to fast-path the read from scripts
> +     * memory.  But that bypasses any iommu.
> +     */
>     cpu_physical_memory_read(addr, (uint8_t *)&buf, 4);
>     return cpu_to_le32(buf);
> }
> @@ -1899,232 +1898,90 @@ static void lsi_reg_writeb(LSIState *s, int offset, uint8_t val)
> #undef CASE_SET_REG32
> }
>
> -static void lsi_mmio_writeb(void *opaque, target_phys_addr_t addr, uint32_t val)
> +static void lsi_mmio_write(void *opaque, target_phys_addr_t addr,
> +                           uint64_t val, unsigned size)
> {
>     LSIState *s = opaque;
>
>     lsi_reg_writeb(s, addr & 0xff, val);
> }
>
> -static void lsi_mmio_writew(void *opaque, target_phys_addr_t addr, uint32_t val)
> -{
> -    LSIState *s = opaque;
> -
> -    addr &= 0xff;
> -    lsi_reg_writeb(s, addr, val & 0xff);
> -    lsi_reg_writeb(s, addr + 1, (val >> 8) & 0xff);
> -}
> -
> -static void lsi_mmio_writel(void *opaque, target_phys_addr_t addr, uint32_t val)
> -{
> -    LSIState *s = opaque;
> -
> -    addr &= 0xff;
> -    lsi_reg_writeb(s, addr, val & 0xff);
> -    lsi_reg_writeb(s, addr + 1, (val >> 8) & 0xff);
> -    lsi_reg_writeb(s, addr + 2, (val >> 16) & 0xff);
> -    lsi_reg_writeb(s, addr + 3, (val >> 24) & 0xff);
> -}
> -
> -static uint32_t lsi_mmio_readb(void *opaque, target_phys_addr_t addr)
> +static uint64_t lsi_mmio_read(void *opaque, target_phys_addr_t addr,
> +                              unsigned size)
> {
>     LSIState *s = opaque;
>
>     return lsi_reg_readb(s, addr & 0xff);
> }
>
> -static uint32_t lsi_mmio_readw(void *opaque, target_phys_addr_t addr)
> -{
> -    LSIState *s = opaque;
> -    uint32_t val;
> -
> -    addr &= 0xff;
> -    val = lsi_reg_readb(s, addr);
> -    val |= lsi_reg_readb(s, addr + 1) << 8;
> -    return val;
> -}
> -
> -static uint32_t lsi_mmio_readl(void *opaque, target_phys_addr_t addr)
> -{
> -    LSIState *s = opaque;
> -    uint32_t val;
> -    addr &= 0xff;
> -    val = lsi_reg_readb(s, addr);
> -    val |= lsi_reg_readb(s, addr + 1) << 8;
> -    val |= lsi_reg_readb(s, addr + 2) << 16;
> -    val |= lsi_reg_readb(s, addr + 3) << 24;
> -    return val;
> -}
> -
> -static CPUReadMemoryFunc * const lsi_mmio_readfn[3] = {
> -    lsi_mmio_readb,
> -    lsi_mmio_readw,
> -    lsi_mmio_readl,
> -};
> -
> -static CPUWriteMemoryFunc * const lsi_mmio_writefn[3] = {
> -    lsi_mmio_writeb,
> -    lsi_mmio_writew,
> -    lsi_mmio_writel,
> +static const MemoryRegionOps lsi_mmio_ops = {
> +    .read = lsi_mmio_read,
> +    .write = lsi_mmio_write,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +    .impl = {
> +        .min_access_size = 1,
> +        .max_access_size = 1,
> +    },
> };
>
> -static void lsi_ram_writeb(void *opaque, target_phys_addr_t addr, uint32_t val)
> +static void lsi_ram_write(void *opaque, target_phys_addr_t addr,
> +                          uint64_t val, unsigned size)
> {
>     LSIState *s = opaque;
>     uint32_t newval;
> +    uint32_t mask;
>     int shift;
>
> -    addr &= 0x1fff;
>     newval = s->script_ram[addr >> 2];
>     shift = (addr & 3) * 8;
> -    newval &= ~(0xff << shift);
> +    mask = ((uint64_t)1 << (size * 8)) - 1;
> +    newval &= ~(mask << shift);
>     newval |= val << shift;
>     s->script_ram[addr >> 2] = newval;
> }
>
> -static void lsi_ram_writew(void *opaque, target_phys_addr_t addr, uint32_t val)
> -{
> -    LSIState *s = opaque;
> -    uint32_t newval;
> -
> -    addr &= 0x1fff;
> -    newval = s->script_ram[addr >> 2];
> -    if (addr & 2) {
> -        newval = (newval & 0xffff) | (val << 16);
> -    } else {
> -        newval = (newval & 0xffff0000) | val;
> -    }
> -    s->script_ram[addr >> 2] = newval;
> -}
> -
> -
> -static void lsi_ram_writel(void *opaque, target_phys_addr_t addr, uint32_t val)
> -{
> -    LSIState *s = opaque;
> -
> -    addr &= 0x1fff;
> -    s->script_ram[addr >> 2] = val;
> -}
> -
> -static uint32_t lsi_ram_readb(void *opaque, target_phys_addr_t addr)
> +static uint64_t lsi_ram_read(void *opaque, target_phys_addr_t addr,
> +                             unsigned size)
> {
>     LSIState *s = opaque;
>     uint32_t val;
> +    uint32_t mask;
>
> -    addr &= 0x1fff;
>     val = s->script_ram[addr >> 2];
> +    mask = ((uint64_t)1 << (size * 8)) - 1;
>     val >>= (addr & 3) * 8;
> -    return val & 0xff;
> -}
> -
> -static uint32_t lsi_ram_readw(void *opaque, target_phys_addr_t addr)
> -{
> -    LSIState *s = opaque;
> -    uint32_t val;
> -
> -    addr &= 0x1fff;
> -    val = s->script_ram[addr >> 2];
> -    if (addr & 2)
> -        val >>= 16;
> -    return val;
> -}
> -
> -static uint32_t lsi_ram_readl(void *opaque, target_phys_addr_t addr)
> -{
> -    LSIState *s = opaque;
> -
> -    addr &= 0x1fff;
> -    return s->script_ram[addr >> 2];
> +    return val & mask;
> }
>
> -static CPUReadMemoryFunc * const lsi_ram_readfn[3] = {
> -    lsi_ram_readb,
> -    lsi_ram_readw,
> -    lsi_ram_readl,
> +static const MemoryRegionOps lsi_ram_ops = {
> +    .read = lsi_ram_read,
> +    .write = lsi_ram_write,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> };
>
> -static CPUWriteMemoryFunc * const lsi_ram_writefn[3] = {
> -    lsi_ram_writeb,
> -    lsi_ram_writew,
> -    lsi_ram_writel,
> -};
> -
> -static uint32_t lsi_io_readb(void *opaque, uint32_t addr)
> +static uint64_t lsi_io_read(void *opaque, target_phys_addr_t addr,
> +                            unsigned size)
> {
>     LSIState *s = opaque;
>     return lsi_reg_readb(s, addr & 0xff);
> }
>
> -static uint32_t lsi_io_readw(void *opaque, uint32_t addr)
> -{
> -    LSIState *s = opaque;
> -    uint32_t val;
> -    addr &= 0xff;
> -    val = lsi_reg_readb(s, addr);
> -    val |= lsi_reg_readb(s, addr + 1) << 8;
> -    return val;
> -}
> -
> -static uint32_t lsi_io_readl(void *opaque, uint32_t addr)
> -{
> -    LSIState *s = opaque;
> -    uint32_t val;
> -    addr &= 0xff;
> -    val = lsi_reg_readb(s, addr);
> -    val |= lsi_reg_readb(s, addr + 1) << 8;
> -    val |= lsi_reg_readb(s, addr + 2) << 16;
> -    val |= lsi_reg_readb(s, addr + 3) << 24;
> -    return val;
> -}
> -
> -static void lsi_io_writeb(void *opaque, uint32_t addr, uint32_t val)
> +static void lsi_io_write(void *opaque, target_phys_addr_t addr,
> +                         uint64_t val, unsigned size)
> {
>     LSIState *s = opaque;
>     lsi_reg_writeb(s, addr & 0xff, val);
> }
>
> -static void lsi_io_writew(void *opaque, uint32_t addr, uint32_t val)
> -{
> -    LSIState *s = opaque;
> -    addr &= 0xff;
> -    lsi_reg_writeb(s, addr, val & 0xff);
> -    lsi_reg_writeb(s, addr + 1, (val >> 8) & 0xff);
> -}
> -
> -static void lsi_io_writel(void *opaque, uint32_t addr, uint32_t val)
> -{
> -    LSIState *s = opaque;
> -    addr &= 0xff;
> -    lsi_reg_writeb(s, addr, val & 0xff);
> -    lsi_reg_writeb(s, addr + 1, (val >> 8) & 0xff);
> -    lsi_reg_writeb(s, addr + 2, (val >> 16) & 0xff);
> -    lsi_reg_writeb(s, addr + 3, (val >> 24) & 0xff);
> -}
> -
> -static void lsi_io_mapfunc(PCIDevice *pci_dev, int region_num,
> -                           pcibus_t addr, pcibus_t size, int type)
> -{
> -    LSIState *s = DO_UPCAST(LSIState, dev, pci_dev);
> -
> -    DPRINTF("Mapping IO at %08"FMT_PCIBUS"\n", addr);
> -
> -    register_ioport_write(addr, 256, 1, lsi_io_writeb, s);
> -    register_ioport_read(addr, 256, 1, lsi_io_readb, s);
> -    register_ioport_write(addr, 256, 2, lsi_io_writew, s);
> -    register_ioport_read(addr, 256, 2, lsi_io_readw, s);
> -    register_ioport_write(addr, 256, 4, lsi_io_writel, s);
> -    register_ioport_read(addr, 256, 4, lsi_io_readl, s);
> -}
> -
> -static void lsi_ram_mapfunc(PCIDevice *pci_dev, int region_num,
> -                            pcibus_t addr, pcibus_t size, int type)
> -{
> -    LSIState *s = DO_UPCAST(LSIState, dev, pci_dev);
> -
> -    DPRINTF("Mapping ram at %08"FMT_PCIBUS"\n", addr);
> -    s->script_ram_base = addr;
> -    cpu_register_physical_memory(addr + 0, 0x2000, s->ram_io_addr);
> -}
> +static const MemoryRegionOps lsi_io_ops = {
> +    .read = lsi_io_read,
> +    .write = lsi_io_write,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +    .impl = {
> +        .min_access_size = 1,
> +        .max_access_size = 1,
> +    },
> +};
>
> static void lsi_scsi_reset(DeviceState *dev)
> {
> @@ -2231,8 +2088,9 @@ static int lsi_scsi_uninit(PCIDevice *d)
> {
>     LSIState *s = DO_UPCAST(LSIState, dev, d);
>
> -    cpu_unregister_io_memory(s->mmio_io_addr);
> -    cpu_unregister_io_memory(s->ram_io_addr);
> +    memory_region_destroy(&s->mmio_io);
> +    memory_region_destroy(&s->ram_io);
> +    memory_region_destroy(&s->io_io);
>
>     return 0;
> }
> @@ -2256,18 +2114,14 @@ static int lsi_scsi_init(PCIDevice *dev)
>     /* Interrupt pin 1 */
>     pci_conf[PCI_INTERRUPT_PIN] = 0x01;
>
> -    s->mmio_io_addr = cpu_register_io_memory(lsi_mmio_readfn,
> -                                             lsi_mmio_writefn, s,
> -                                             DEVICE_NATIVE_ENDIAN);
> -    s->ram_io_addr = cpu_register_io_memory(lsi_ram_readfn,
> -                                            lsi_ram_writefn, s,
> -                                            DEVICE_NATIVE_ENDIAN);
> -
> -    pci_register_bar(&s->dev, 0, 256,
> -                           PCI_BASE_ADDRESS_SPACE_IO, lsi_io_mapfunc);
> -    pci_register_bar_simple(&s->dev, 1, 0x400, 0, s->mmio_io_addr);
> -    pci_register_bar(&s->dev, 2, 0x2000,
> -                           PCI_BASE_ADDRESS_SPACE_MEMORY, lsi_ram_mapfunc);
> +    memory_region_init_io(&s->mmio_io, &lsi_mmio_ops, s, "lsi-mmio", 0x400);
> +    memory_region_init_io(&s->ram_io, &lsi_ram_ops, s, "lsi-ram", 0x2000);
> +    memory_region_init_io(&s->io_io, &lsi_io_ops, s, "lsi-io", 256);
> +
> +    pci_register_bar_region(&s->dev, 0, PCI_BASE_ADDRESS_SPACE_IO, &s->io_io);
> +    pci_register_bar_region(&s->dev, 1, 0, &s->mmio_io);
> +    pci_register_bar_region(&s->dev, 2, PCI_BASE_ADDRESS_SPACE_MEMORY,
> +                            &s->ram_io);
>     QTAILQ_INIT(&s->queue);
>
>     scsi_bus_new(&s->bus, &dev->qdev, 1, LSI_MAX_DEVS, &lsi_scsi_ops);
> -- 
> 1.7.5.3
>
>
>