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

[malc]

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

-- 
mailto:address@hidden