Patchwork [07/15] ac97: convert to new PCI API

login
register
mail settings
Submitter Anthony Liguori
Date Feb. 9, 2010, 10:01 p.m.
Message ID <1265752899-26980-8-git-send-email-aliguori@us.ibm.com>
Download mbox | patch
Permalink /patch/44967/
State New
Headers show

Comments

Anthony Liguori - Feb. 9, 2010, 10:01 p.m.
- fixed bug with size of registered ioport regions

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 hw/ac97.c |  146 +++++++++++++++++++++++++------------------------------------
 1 files changed, 60 insertions(+), 86 deletions(-)
malc - Feb. 9, 2010, 10:45 p.m.
On Tue, 9 Feb 2010, Anthony Liguori wrote:

>  - fixed bug with size of registered ioport regions
> 
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> ---
>  hw/ac97.c |  146 +++++++++++++++++++++++++------------------------------------
>  1 files changed, 60 insertions(+), 86 deletions(-)
> 
> diff --git a/hw/ac97.c b/hw/ac97.c
> index 4319bc8..9fdf591 100644
> --- a/hw/ac97.c
> +++ b/hw/ac97.c
> @@ -223,7 +223,7 @@ static void fetch_bd (AC97LinkState *s, AC97BusMasterRegs *r)
>  {
>      uint8_t b[8];
>  
> -    cpu_physical_memory_read (r->bdbar + r->civ * 8, b, 8);
> +    pci_memory_read (&s->dev, r->bdbar + r->civ * 8, b, 8);
>      r->bd_valid = 1;
>      r->bd.addr = le32_to_cpu (*(uint32_t *) &b[0]) & ~3;
>      r->bd.ctl_len = le32_to_cpu (*(uint32_t *) &b[4]);
> @@ -569,50 +569,35 @@ static void mixer_reset (AC97LinkState *s)
>  
>  /**
>   * Native audio mixer
> - * I/O Reads
>   */
> -static uint32_t nam_readb (void *opaque, uint32_t addr)
> -{
> -    AC97LinkState *s = opaque;
> -    dolog ("U nam readb %#x\n", addr);
> -    s->cas = 0;
> -    return ~0U;
> -}
>  
> -static uint32_t nam_readw (void *opaque, uint32_t addr)
> +static uint32_t nam_read (PCIDevice *dev, pcibus_t addr, int size)
>  {
> -    AC97LinkState *s = opaque;
> -    uint32_t val = ~0U;
> -    uint32_t index = addr - s->base[0];
> -    s->cas = 0;
> -    val = mixer_load (s, index);
> -    return val;
> -}
> +    AC97LinkState *s = DO_UPCAST (AC97LinkState, dev, dev);
> +    uint32_t value;
> +
> +    if (size == 2) {
> +        value = mixer_load (s, addr);
> +    } else {
> +        dolog ("U nam read[%d] %#" FMT_PCIBUS "\n", size, addr);
> +        s->cas = 0;
> +        value = ~0U;
> +    }
>  

Yeah right, and then PCI SIG adds qword accessors and all hell breaks
loose, this interface sucks..

[..snip..]
Anthony Liguori - Feb. 9, 2010, 10:52 p.m.
On 02/09/2010 04:45 PM, malc wrote:
> On Tue, 9 Feb 2010, Anthony Liguori wrote:
>
>    
>>   - fixed bug with size of registered ioport regions
>>
>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>> ---
>>   hw/ac97.c |  146 +++++++++++++++++++++++++------------------------------------
>>   1 files changed, 60 insertions(+), 86 deletions(-)
>>
>> diff --git a/hw/ac97.c b/hw/ac97.c
>> index 4319bc8..9fdf591 100644
>> --- a/hw/ac97.c
>> +++ b/hw/ac97.c
>> @@ -223,7 +223,7 @@ static void fetch_bd (AC97LinkState *s, AC97BusMasterRegs *r)
>>   {
>>       uint8_t b[8];
>>
>> -    cpu_physical_memory_read (r->bdbar + r->civ * 8, b, 8);
>> +    pci_memory_read (&s->dev, r->bdbar + r->civ * 8, b, 8);
>>       r->bd_valid = 1;
>>       r->bd.addr = le32_to_cpu (*(uint32_t *)&b[0])&  ~3;
>>       r->bd.ctl_len = le32_to_cpu (*(uint32_t *)&b[4]);
>> @@ -569,50 +569,35 @@ static void mixer_reset (AC97LinkState *s)
>>
>>   /**
>>    * Native audio mixer
>> - * I/O Reads
>>    */
>> -static uint32_t nam_readb (void *opaque, uint32_t addr)
>> -{
>> -    AC97LinkState *s = opaque;
>> -    dolog ("U nam readb %#x\n", addr);
>> -    s->cas = 0;
>> -    return ~0U;
>> -}
>>
>> -static uint32_t nam_readw (void *opaque, uint32_t addr)
>> +static uint32_t nam_read (PCIDevice *dev, pcibus_t addr, int size)
>>   {
>> -    AC97LinkState *s = opaque;
>> -    uint32_t val = ~0U;
>> -    uint32_t index = addr - s->base[0];
>> -    s->cas = 0;
>> -    val = mixer_load (s, index);
>> -    return val;
>> -}
>> +    AC97LinkState *s = DO_UPCAST (AC97LinkState, dev, dev);
>> +    uint32_t value;
>> +
>> +    if (size == 2) {
>> +        value = mixer_load (s, addr);
>> +    } else {
>> +        dolog ("U nam read[%d] %#" FMT_PCIBUS "\n", size, addr);
>> +        s->cas = 0;
>> +        value = ~0U;
>> +    }
>>
>>      
> Yeah right, and then PCI SIG adds qword accessors and all hell breaks
> loose, this interface sucks..
>    

We already have this problem with the current interface.

The options to address would be to always return/accept a uint64_t or to 
deal with a void *.  The later interface has non-obvious byte order 
semantics.

I avoided the former to avoid complaints about possibly introducing a 
performance regression with passing larger data types.  I don't believe 
that it would be a problem though.

Regards,

Anthony Liguori

> [..snip..]
>
>
malc - Feb. 9, 2010, 11:06 p.m.
On Tue, 9 Feb 2010, Anthony Liguori wrote:

> On 02/09/2010 04:45 PM, malc wrote:
> > On Tue, 9 Feb 2010, Anthony Liguori wrote:
> > 
> >    
> > >   - fixed bug with size of registered ioport regions
> > > 
> > > Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
> > > ---
> > >   hw/ac97.c |  146
> > > +++++++++++++++++++++++++------------------------------------
> > >   1 files changed, 60 insertions(+), 86 deletions(-)
> > > 
> > > diff --git a/hw/ac97.c b/hw/ac97.c
> > > index 4319bc8..9fdf591 100644
> > > --- a/hw/ac97.c
> > > +++ b/hw/ac97.c
> > > @@ -223,7 +223,7 @@ static void fetch_bd (AC97LinkState *s,
> > > AC97BusMasterRegs *r)
> > >   {
> > >       uint8_t b[8];
> > > 
> > > -    cpu_physical_memory_read (r->bdbar + r->civ * 8, b, 8);
> > > +    pci_memory_read (&s->dev, r->bdbar + r->civ * 8, b, 8);
> > >       r->bd_valid = 1;
> > >       r->bd.addr = le32_to_cpu (*(uint32_t *)&b[0])&  ~3;
> > >       r->bd.ctl_len = le32_to_cpu (*(uint32_t *)&b[4]);
> > > @@ -569,50 +569,35 @@ static void mixer_reset (AC97LinkState *s)
> > > 
> > >   /**
> > >    * Native audio mixer
> > > - * I/O Reads
> > >    */
> > > -static uint32_t nam_readb (void *opaque, uint32_t addr)
> > > -{
> > > -    AC97LinkState *s = opaque;
> > > -    dolog ("U nam readb %#x\n", addr);
> > > -    s->cas = 0;
> > > -    return ~0U;
> > > -}
> > > 
> > > -static uint32_t nam_readw (void *opaque, uint32_t addr)
> > > +static uint32_t nam_read (PCIDevice *dev, pcibus_t addr, int size)
> > >   {
> > > -    AC97LinkState *s = opaque;
> > > -    uint32_t val = ~0U;
> > > -    uint32_t index = addr - s->base[0];
> > > -    s->cas = 0;
> > > -    val = mixer_load (s, index);
> > > -    return val;
> > > -}
> > > +    AC97LinkState *s = DO_UPCAST (AC97LinkState, dev, dev);
> > > +    uint32_t value;
> > > +
> > > +    if (size == 2) {
> > > +        value = mixer_load (s, addr);
> > > +    } else {
> > > +        dolog ("U nam read[%d] %#" FMT_PCIBUS "\n", size, addr);
> > > +        s->cas = 0;
> > > +        value = ~0U;
> > > +    }
> > > 
> > >      
> > Yeah right, and then PCI SIG adds qword accessors and all hell breaks
> > loose, this interface sucks..
> >    
> 
> We already have this problem with the current interface.

Uh, i've meant the registration of one function to rule them all, instead
of how it's done currently - separate accessors for b/w/l/whatever.

> 
> The options to address would be to always return/accept a uint64_t or to deal
> with a void *.  The later interface has non-obvious byte order semantics.
> 
> I avoided the former to avoid complaints about possibly introducing a
> performance regression with passing larger data types.  I don't believe that
> it would be a problem though.
> 
> Regards,
> 
> Anthony Liguori
> 
> > [..snip..]
> > 
> >
Anthony Liguori - Feb. 9, 2010, 11:10 p.m.
On 02/09/2010 05:06 PM, malc wrote:
>
>> We already have this problem with the current interface.
>>      
> Uh, i've meant the registration of one function to rule them all, instead
> of how it's done currently - separate accessors for b/w/l/whatever.
>    

How does that make any difference?  Both the ioport and memory 
registrations interface take the same function pointer regardless of 
access size.

If you wanted to introduce a quad-word specific accessor, you would need 
to introduce a different registration mechanism or you would have to 
change the signature for all of the functions.

Regards,

Anthony Liguori
malc - Feb. 9, 2010, 11:36 p.m.
On Tue, 9 Feb 2010, Anthony Liguori wrote:

> On 02/09/2010 05:06 PM, malc wrote:
> > 
> > > We already have this problem with the current interface.
> > >      
> > Uh, i've meant the registration of one function to rule them all, instead
> > of how it's done currently - separate accessors for b/w/l/whatever.
> >    
> 
> How does that make any difference?  Both the ioport and memory registrations
> interface take the same function pointer regardless of access size.
> 
> If you wanted to introduce a quad-word specific accessor, you would need to
> introduce a different registration mechanism or you would have to change the
> signature for all of the functions.

Let's see:

Currently we have this


readb(...):
  dostuff
  return stuff

readw(...):
  dostuff
  return stuff

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

The interface being implicit rather than explicit about the sizes
makes this possible, so i'm against it. The code was written the
way it was written for a purpose.
Anthony Liguori - Feb. 9, 2010, 11:52 p.m.
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
malc - Feb. 10, 2010, 12:24 a.m.
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.

> 
> 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.

> 
> 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;

This is exactly equivalent to your original if, due to the C language
being what it is.

> 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.

Eh? Care to name one emulation bug?

> 
> 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.
Anthony Liguori - Feb. 11, 2010, 2:20 p.m.
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

Patch

diff --git a/hw/ac97.c b/hw/ac97.c
index 4319bc8..9fdf591 100644
--- a/hw/ac97.c
+++ b/hw/ac97.c
@@ -223,7 +223,7 @@  static void fetch_bd (AC97LinkState *s, AC97BusMasterRegs *r)
 {
     uint8_t b[8];
 
-    cpu_physical_memory_read (r->bdbar + r->civ * 8, b, 8);
+    pci_memory_read (&s->dev, r->bdbar + r->civ * 8, b, 8);
     r->bd_valid = 1;
     r->bd.addr = le32_to_cpu (*(uint32_t *) &b[0]) & ~3;
     r->bd.ctl_len = le32_to_cpu (*(uint32_t *) &b[4]);
@@ -569,50 +569,35 @@  static void mixer_reset (AC97LinkState *s)
 
 /**
  * Native audio mixer
- * I/O Reads
  */
-static uint32_t nam_readb (void *opaque, uint32_t addr)
-{
-    AC97LinkState *s = opaque;
-    dolog ("U nam readb %#x\n", addr);
-    s->cas = 0;
-    return ~0U;
-}
 
-static uint32_t nam_readw (void *opaque, uint32_t addr)
+static uint32_t nam_read (PCIDevice *dev, pcibus_t addr, int size)
 {
-    AC97LinkState *s = opaque;
-    uint32_t val = ~0U;
-    uint32_t index = addr - s->base[0];
-    s->cas = 0;
-    val = mixer_load (s, index);
-    return val;
-}
+    AC97LinkState *s = DO_UPCAST (AC97LinkState, dev, dev);
+    uint32_t value;
+
+    if (size == 2) {
+        value = mixer_load (s, addr);
+    } else {
+        dolog ("U nam read[%d] %#" FMT_PCIBUS "\n", size, addr);
+        s->cas = 0;
+        value = ~0U;
+    }
 
-static uint32_t nam_readl (void *opaque, uint32_t addr)
-{
-    AC97LinkState *s = opaque;
-    dolog ("U nam readl %#x\n", addr);
-    s->cas = 0;
-    return ~0U;
+    return value;
 }
 
-/**
- * Native audio mixer
- * I/O Writes
- */
-static void nam_writeb (void *opaque, uint32_t addr, uint32_t val)
+static void nam_write (PCIDevice *dev, pcibus_t addr, int size, uint32_t val)
 {
-    AC97LinkState *s = opaque;
-    dolog ("U nam writeb %#x <- %#x\n", addr, val);
-    s->cas = 0;
-}
+    AC97LinkState *s = DO_UPCAST (AC97LinkState, dev, dev);
+    uint32_t index = addr;
 
-static void nam_writew (void *opaque, uint32_t addr, uint32_t val)
-{
-    AC97LinkState *s = opaque;
-    uint32_t index = addr - s->base[0];
     s->cas = 0;
+    if (size != 2) {
+        dolog ("U nam write[%d] %#" FMT_PCIBUS " <- %#x\n", addr, val);
+        return;
+    }
+
     switch (index) {
     case AC97_Reset:
         mixer_reset (s);
@@ -699,22 +684,13 @@  static void nam_writew (void *opaque, uint32_t addr, uint32_t val)
     }
 }
 
-static void nam_writel (void *opaque, uint32_t addr, uint32_t val)
-{
-    AC97LinkState *s = opaque;
-    dolog ("U nam writel %#x <- %#x\n", addr, val);
-    s->cas = 0;
-}
-
 /**
  * Native audio bus master
  * I/O Reads
  */
-static uint32_t nabm_readb (void *opaque, uint32_t addr)
+static uint32_t nabm_readb (AC97LinkState *s, uint32_t index)
 {
-    AC97LinkState *s = opaque;
     AC97BusMasterRegs *r = NULL;
-    uint32_t index = addr - s->base[1];
     uint32_t val = ~0U;
 
     switch (index) {
@@ -765,11 +741,9 @@  static uint32_t nabm_readb (void *opaque, uint32_t addr)
     return val;
 }
 
-static uint32_t nabm_readw (void *opaque, uint32_t addr)
+static uint32_t nabm_readw (AC97LinkState *s, uint32_t index)
 {
-    AC97LinkState *s = opaque;
     AC97BusMasterRegs *r = NULL;
-    uint32_t index = addr - s->base[1];
     uint32_t val = ~0U;
 
     switch (index) {
@@ -794,11 +768,9 @@  static uint32_t nabm_readw (void *opaque, uint32_t addr)
     return val;
 }
 
-static uint32_t nabm_readl (void *opaque, uint32_t addr)
+static uint32_t nabm_readl (AC97LinkState *s, uint32_t index)
 {
-    AC97LinkState *s = opaque;
     AC97BusMasterRegs *r = NULL;
-    uint32_t index = addr - s->base[1];
     uint32_t val = ~0U;
 
     switch (index) {
@@ -840,15 +812,29 @@  static uint32_t nabm_readl (void *opaque, uint32_t addr)
     return val;
 }
 
+static uint32_t nabm_read (PCIDevice *dev, pcibus_t addr, int size)
+{
+    AC97LinkState *s = DO_UPCAST (AC97LinkState, dev, dev);
+    uint32_t value;
+
+    if (size == 1) {
+        value = nabm_readb (s, addr);
+    } else if (size == 2) {
+        value = nabm_readw (s, addr);
+    } else {
+        value = nabm_readl (s, addr);
+    }
+
+    return value;
+}
+
 /**
  * Native audio bus master
  * I/O Writes
  */
-static void nabm_writeb (void *opaque, uint32_t addr, uint32_t val)
+static void nabm_writeb (AC97LinkState *s, uint32_t index, uint32_t val)
 {
-    AC97LinkState *s = opaque;
     AC97BusMasterRegs *r = NULL;
-    uint32_t index = addr - s->base[1];
     switch (index) {
     case PI_LVI:
     case PO_LVI:
@@ -954,6 +940,19 @@  static void nabm_writel (void *opaque, uint32_t addr, uint32_t val)
     }
 }
 
+static void nabm_write (PCIDevice *dev, pcibus_t addr, int size, uint32_t value)
+{
+    AC97LinkState *s = DO_UPCAST (AC97LinkState, dev, dev);
+
+    if (size == 1) {
+        nabm_writeb (s, addr, value);
+    } else if (size == 2) {
+        nabm_writew (s, addr, value);
+    } else {
+        nabm_writel (s, addr, value);
+    }
+}
+
 static int write_audio (AC97LinkState *s, AC97BusMasterRegs *r,
                         int max, int *stop)
 {
@@ -972,7 +971,7 @@  static int write_audio (AC97LinkState *s, AC97BusMasterRegs *r,
     while (temp) {
         int copied;
         to_copy = audio_MIN (temp, sizeof (tmpbuf));
-        cpu_physical_memory_read (addr, tmpbuf, to_copy);
+        pci_memory_read (&s->dev, addr, tmpbuf, to_copy);
         copied = AUD_write (s->voice_po, tmpbuf, to_copy);
         dolog ("write_audio max=%x to_copy=%x copied=%x\n",
                max, to_copy, copied);
@@ -1056,7 +1055,7 @@  static int read_audio (AC97LinkState *s, AC97BusMasterRegs *r,
             *stop = 1;
             break;
         }
-        cpu_physical_memory_write (addr, tmpbuf, acquired);
+        pci_memory_write (&s->dev, addr, tmpbuf, acquired);
         temp -= acquired;
         addr += acquired;
         nread += acquired;
@@ -1234,32 +1233,6 @@  static const VMStateDescription vmstate_ac97 = {
     }
 };
 
-static void ac97_map (PCIDevice *pci_dev, int region_num,
-                      pcibus_t addr, pcibus_t size, int type)
-{
-    AC97LinkState *s = DO_UPCAST (AC97LinkState, dev, pci_dev);
-    PCIDevice *d = &s->dev;
-
-    if (!region_num) {
-        s->base[0] = addr;
-        register_ioport_read (addr, 256 * 1, 1, nam_readb, d);
-        register_ioport_read (addr, 256 * 2, 2, nam_readw, d);
-        register_ioport_read (addr, 256 * 4, 4, nam_readl, d);
-        register_ioport_write (addr, 256 * 1, 1, nam_writeb, d);
-        register_ioport_write (addr, 256 * 2, 2, nam_writew, d);
-        register_ioport_write (addr, 256 * 4, 4, nam_writel, d);
-    }
-    else {
-        s->base[1] = addr;
-        register_ioport_read (addr, 64 * 1, 1, nabm_readb, d);
-        register_ioport_read (addr, 64 * 2, 2, nabm_readw, d);
-        register_ioport_read (addr, 64 * 4, 4, nabm_readl, d);
-        register_ioport_write (addr, 64 * 1, 1, nabm_writeb, d);
-        register_ioport_write (addr, 64 * 2, 2, nabm_writew, d);
-        register_ioport_write (addr, 64 * 4, 4, nabm_writel, d);
-    }
-}
-
 static void ac97_on_reset (void *opaque)
 {
     AC97LinkState *s = opaque;
@@ -1321,9 +1294,10 @@  static int ac97_initfn (PCIDevice *dev)
     /* TODO: RST# value should be 0. */
     c[PCI_INTERRUPT_PIN] = 0x01;      /* intr_pn interrupt pin ro */
 
-    pci_register_bar (&s->dev, 0, 256 * 4, PCI_BASE_ADDRESS_SPACE_IO,
-                      ac97_map);
-    pci_register_bar (&s->dev, 1, 64 * 4, PCI_BASE_ADDRESS_SPACE_IO, ac97_map);
+    pci_register_io_region (&s->dev, 0, 256 * 4, PCI_BASE_ADDRESS_SPACE_IO,
+                            nam_read, nam_write);
+    pci_register_io_region (&s->dev, 1, 64 * 4, PCI_BASE_ADDRESS_SPACE_IO,
+                            nabm_read, nabm_write);
     qemu_register_reset (ac97_on_reset, s);
     AUD_register_card ("ac97", &s->card);
     ac97_on_reset (s);