qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Anthony Liguori
Subject: Re: [Qemu-devel] [PATCH 07/15] ac97: convert to new PCI API
Date: Tue, 09 Feb 2010 17:52:37 -0600
User-agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1.5) Gecko/20091209 Fedora/3.0-4.fc12 Lightning/1.0pre Thunderbird/3.0

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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]