Re: [Qemu-devel] [PATCH 07/15] ac97: convert to new PCI API

[Anthony Liguori <anthony@codemonkey.ws>]

On 02/09/2010 05:36 PM, malc wrote:
> Let's see:
>
> Currently we have this
>
>
> readb(...):
>    dostuff
>    return stuff
>
> readw(...):
>    dostuff
>    return stuff
>    

And this is completely wrong.

For the most part, device models don't consistently handle access to 
registers via their non native sizes.  Some devices return the lower 
part of a dword register when accessed with a word accessor.  Some 
devices only return the register when there's a dword access.

What's worse, is that if a device only registers a byte accessor, 
because of the default accessors, a word access returns two registers 
but a dword access returns ~0U.  Some device models actually rely on 
this behaviour.

> You are replacing it with
>
> read(size...):
>    if (size == 1): do1
>    elif (size == 2): do2
>    else: # and here your code assumes that everything is handy dandy
>          # and size is 4
>      do4
>    

This is ugly because it's a programmatic conversion.  Once we have this 
API, we can switch to:

read(addr, size...):
   switch(addr):
   case REG0:
     return s->reg0;
   case REG1:
     return s->reg1;

Along with having a table like:

pci_regs = { {REG0, DWORD}, {REG1, BYTE}, {REG2, BYTE}, {REG3, WORD} }

That allows the PCI layer to invoke the callback and do the right 
operations to transparently handle the access size without the device 
model having to deal with it.  The reason I've left size in the callback 
at all is that I suspect there will always be a device that behaves 
weirdly and needs to know about the accessor size.  But for the most 
part, I think this model will make things much more consistent and 
eliminate a huge class of emulation bugs.

We can even take it further, and do something like:

pci_regs = {
   PCI_REG_DWORD(REG0, DeviceState, reg0),
   PCI_REG_BYTE(REG1, DeviceState, reg1),
   ...
}

In which case, we only need to handle the case in switch() if we need to 
implement a side effect of the register operation.

But none of this is possible if we completely rely on every device to 
implement non-dword access in it's own broken way.

Regards,

Anthony Liguori