Re: [Qemu-devel] [PATCH 3/3] prep: improve Raven PCI host emulation

[Hervé Poussineau]

Peter Maydell a écrit :
> On 23 August 2013 19:52, Hervé Poussineau <address@hidden> wrote:
>
> > - let it load a firmware (raw or elf image)
> > - add a GPIO to let it handle the non-contiguous I/O address space
> > - provide a bus master address space
>
> > Also move isa_mem_base from PCI host to machine board.
>
> > Simplify prep board code by relying on Raven PCI host to handle
> > non-contiguous I/O, and to load BIOS (with a small hack required
> > for Open Hack'Ware).
>
> That seems like quite a lot to be doing in a single patch. Does
> any of it split out for easier code review?

Yes, v2 is in preparation. This patch will be split in multiple patches.

> > +static uint64_t prep_io_read(void *opaque, hwaddr addr,
> > +                             unsigned int size)
> > +{
> > +    PREPPCIState *s = opaque;
> > +    uint8_t buf[4];
> > +    uint64_t val;
> > +
> > +    if (s->contiguous_map == 0) {
> > +        /* 64 KB contiguous space for IOs */
> > +        addr &= 0xFFFF;
> > +    } else {
> > +        /* 8 MB non-contiguous space for IOs */
> > +        addr = (addr & 0x1F) | ((addr & 0x007FFF000) >> 7);
> > +    }
> > +
> > +    address_space_read(&s->pci_io_as, addr + 0x80000000, buf, size);
> > +    memcpy(&val, buf, size);
> > +    return val;
> > +}
>
> Since this is just forwarding accesses to another address
> space, I'm fairly sure you could do it with a suitable collection
> of alias and container memory regions.

Yes, aliases should probably work, but it won't be handy to create lots 
of them. Moreover, this function needs to be expanded later to handle an 
additional endianness switch, which will change both addresses and values...

> > +
> > +static void prep_io_write(void *opaque, hwaddr addr,
> > +                          uint64_t val, unsigned int size)
> > +{
> > +    PREPPCIState *s = opaque;
> > +    uint8_t buf[4];
> > +
> > +    if (s->contiguous_map == 0) {
> > +        /* 64 KB contiguous space for IOs */
> > +        addr &= 0xFFFF;
> > +    } else {
> > +        /* 8 MB non-contiguous space for IOs */
> > +        addr = (addr & 0x1F) | ((addr & 0x007FFF000) >> 7);
> > +    }
> > +
> > +    memcpy(buf, &val, size);
> > +    address_space_write(&s->pci_io_as, addr + 0x80000000, buf, size);
> > +}
>
> ...if you don't do it that way, please at least factor out the
> address conversion code from the read and write routines.

... So, yes, I'll put conversion code in a common routine.

Hervé