Hi All, I have brought myself up to speed with this PCI stuff. First of all I am assuming you are talking about the PCI Config space only? This (hw/pci.h):
struct PCIDevice { DeviceState qdev; /* PCI config space */ uint8_t *config; [snip] /* Used to implement RW1C(Write 1 to Clear) bytes */ uint8_t *w1cmask; /* Used to allocate config space for capabilities. */ uint8_t *used; On Wed, Mar 6, 2013 at 12:32 AM, Michael S. Tsirkin <m...@redhat.com> wrote: > On Tue, Mar 05, 2013 at 03:17:13PM +0100, Gerd Hoffmann wrote: >> Hi, >> >> >>>> We also need uint8_t, uint16_t and uint64_t versions for some devices. >> >>>> Perhaps it would be better to implement a uint64_t device which can be >> >>>> used with shorter widths or even stronger connection with memory API. >> >>> >> >>> Why not uint8_t for everyone? >> >> >> >> That would be simple, but then modeling for example 32 bit registers >> >> gets clumsy. >> > >> > The way we do this in pci is support wrappers for word/long accesses. >> > This is a nice way to do endian-ness conversion anyway, I guess. >> > If people are interested, it shouldn't be hard to generalize the pci >> > code... So the issue I have with the uint8_t based defintion + accessor is you need to use code driven to set all the properties (w1cmask, used and friends). eg (hw/pci/shpc.c): pci_set_byte(shpc->wmask + SHPC_CMD_CODE, 0xff); pci_set_byte(shpc->wmask + SHPC_CMD_TRGT, SHPC_CMD_TRGT_MAX); pci_set_byte(shpc->wmask + SHPC_CMD_TRGT, SHPC_CMD_TRGT_MAX); I'm trying to break that, with table driven instantiation of this information. Check patch 3 of this series for the guinea pig example that does this, short except below: +static const UInt32StateInfo xilinx_devcfg_regs_info[] = { + [R_CTRL] = { .name = "CTRL", + .reset = PCAP_PR | PCAP_MODE | 0x3 << 13, + .ro = 0x107f6000 | USER_MODE, + .ge0 = 0x3 << 13, + }, + [R_LOCK] = { .name = "LOCK", .width = 5, .nw0 = ~0 }, + [R_CFG] = { .name = "CFG", + .width = 12, + .reset = 1 << RFIFO_TH_SHIFT | 1 << WFIFO_TH_SHIFT | 0x8, + .ge1 = 0x7, + .ge0 = 0x8, + .ro = 0x00f, + }, This philosophy also breaks the parallel array approach adopted by PCI, so to share with PCI that will require some major earthworks PCI side. >> >> > At least with PCI, guest can perform a long access and host word access >> > to the same register, so width is not a register property. >> Guest access is the easiest part as you can use wrappers/accessors that translate memory-API to uint8_t. But for hardware side access its nice to be able to bang on the raw uint32_t[]. E.G. (from next patch): + DB_PRINT("dma operation finished\n"); + s->regs[R_INT_STS] |= DMA_DONE_INT | DMA_P_DONE_INT; This code raises the interrupt from the hardware side by setting a register bit directly. It would be tedious (and an enourmous tree-wide change pattern) to have the change this over to use a wrapping accessor. My goal here is that only guest side access are regulated by this API - hardware has free reign. >> Thanks, but I'm not interested. >> >> Memory API handles this just fine for me, and there is zero reason to >> care about how the guests accesses the registers (unless the hardware >> you are emulating behaves strange enough that you have to care to get it >> right). >> So in V2 I want to use something close to the memory API in interface, so there is only trivial glue on the device level between the memory API callback and this API. >> cheers, >> Gerd > > If the intended audience uses the memory API, then it's not needed. Not 100% accurate. My goal here it to control (or wrap) only guest accesses, in the first instance via the Memory API, but other forms of guest access are perfectly valid as well, and PCI config space, would be a good example. If we are going to share code however, we will need to make changes PCI side, as that uint8_t with compulsory accessors is too verbose to be used in devices. I have a solution that may work for everyone however. Leave the storage format (uint32_t, uint8_t etc) up to the client. In PCI config this is the uint8_t *config as it stands today. In my device its uint32_t *regs. The client then casts the accessed area to uint8_t and is responsible for defining the endianness policy via the accompanying Uint32StateInfo struct. For PCI, this is PCIs endianness. For my device this is just the endianness of the host machine, as thats how the hardware side accesses (eg s->regs[R_INT_STS] |= DMA_DONE_INT;) will operate. Might be easier to describe as patches rather than talk though. Regards, Peter > pci config is not going through a memory API ATM though > I think it might be possible to make it do this by creating a separate > config address space per device. > > -- > MST >