Re: [Qemu-devel] [RFC PATCH v1 2/4] bitops: Add UInt32StateInfo and helper functions

[Peter Crosthwaite]

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 <address@hidden> 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
>