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

[Anthony Liguori <anthony@codemonkey.ws>]

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