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

[Anthony Liguori]

On 02/09/2010 06:24 PM, malc wrote:
> On Tue, 9 Feb 2010, Anthony Liguori wrote:
>
>    
> > 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.
> >      
> It's completely right, until some future C standard implements proper
> algebraic data types.
>    

I've been thinking about what to do here and I think what I've come up 
with is to change the callbacks to take be a separate type.  So instead 
of just:

void (PCIOWriteFunc)(PCIDevice *, pcibus_t addr, int size, uint32_t value);
uint32_t (PCIOReadFunc)(PCIDevice *, pcibus_t addr, int size);

We would have:

typedef struct PCIIOHandler
{
   void (*write)(PCIIOHandler *, pcibus_t addr, int size, uint32_t);
   uint32_t (*read)(PCIIOHandler *, pcibus_t addr, int size);
} PCIIOHandler;

And then we would have:

pci_register_io_region(PCIDevice *, ... PCIIOHandler *handler);

We can then introduce a mechanism to dispatch to individual functions 
based on size.  That way, instead of pushing an if (size == 1) else if 
(size == 2) check to each device, we let the device decide how it wants 
to be invoked.

> > 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.
> >      
> That's up to device to register an accessor and return what's appropriate.
>    

I think the point though is to make it possible for common mechanisms to 
be formalized into common code.  For instance, a common set of rules 
will be, all registers can be accessed in byte, word, or dword size but 
most be accessed at natural size boundaries.  We should make it simple 
for a device to essentially choose that pattern and not have to worry 
about explicitly coding it themselves.
> > 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.
> >
> >      
> Lovely, we are heading full speed into fanatsy land, where PCI BUS itself
> accesses device specific stuff.
>    

I never said the PCI BUS is the one that actually got this info.  It 
could just be common code that implements this logic.

But there are good reasons to normalize device emulation at this level.  
For instance, it allows us to implement consistent tracing for device 
models using symbolic names for registers as opposed to only being able 
to trace IO operations at addresses.

Regards,

Anthony Liguori