Re: [Qemu-devel] [PATCH V7 4/8] hw/acpi_piix4.c: replace register_ioport*

[Julien Grall <julien.grall@citrix.com>]

On 09/04/2012 08:24 AM, Jan Kiszka wrote:
> On 2012-09-03 12:03, Julien Grall wrote:
>    
>> This patch replaces all register_ioport* with the new memory API. It permits
>> to use the new Memory stuff like listener.
>>
>> Signed-off-by: Julien Grall<julien.grall@citrix.com>
>> ---
>>   hw/acpi_piix4.c |  151 +++++++++++++++++++++++++++++++++++++++++++------------
>>   1 files changed, 119 insertions(+), 32 deletions(-)
>>
>> diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
>> index 1ad45ce..527dfc1 100644
>> --- a/hw/acpi_piix4.c
>> +++ b/hw/acpi_piix4.c
>> @@ -41,8 +41,7 @@
>>
>>   #define GPE_BASE 0xafe0
>>   #define GPE_LEN 4
>> -#define PCI_UP_BASE 0xae00
>> -#define PCI_DOWN_BASE 0xae04
>> +#define PCI_BASE 0xae00
>>   #define PCI_EJ_BASE 0xae08
>>   #define PCI_RMV_BASE 0xae0c
>>
>> @@ -55,7 +54,8 @@ struct pci_status {
>>
>>   typedef struct PIIX4PMState {
>>       PCIDevice dev;
>> -    IORange ioport;
>> +    MemoryRegion pm_io;
>> +    uint32_t pm_io_base;
>>       ACPIREGS ar;
>>
>>       APMState apm;
>> @@ -64,6 +64,11 @@ typedef struct PIIX4PMState {
>>       uint32_t smb_io_base;
>>
>>       MemoryRegion smb_io;
>> +    MemoryRegion acpi_io;
>> +    MemoryRegion acpi_hot_io;
>> +    PortioList pci_hot_port_list;
>> +    MemoryRegion pciej_hot_io;
>> +    MemoryRegion pcirmv_hot_io;
>>
>>       qemu_irq irq;
>>       qemu_irq smi_irq;
>> @@ -110,10 +115,10 @@ static void pm_tmr_timer(ACPIREGS *ar)
>>       pm_update_sci(s);
>>   }
>>
>> -static void pm_ioport_write(IORange *ioport, uint64_t addr, unsigned width,
>> -                            uint64_t val)
>> +static void pm_ioport_write(void *opaque, target_phys_addr_t addr,
>> +                            uint64_t val, unsigned width)
>>   {
>> -    PIIX4PMState *s = container_of(ioport, PIIX4PMState, ioport);
>> +    PIIX4PMState *s = opaque;
>>
>>       if (width != 2) {
>>           PIIX4_DPRINTF("PM write port=0x%04x width=%d val=0x%08x\n",
>> @@ -139,11 +144,11 @@ static void pm_ioport_write(IORange *ioport, uint64_t addr, unsigned width,
>>                     (unsigned int)val);
>>   }
>>
>> -static void pm_ioport_read(IORange *ioport, uint64_t addr, unsigned width,
>> -                            uint64_t *data)
>> +static uint64_t pm_ioport_read(void *opaque, target_phys_addr_t addr,
>> +                               unsigned width)
>>   {
>> -    PIIX4PMState *s = container_of(ioport, PIIX4PMState, ioport);
>> -    uint32_t val;
>> +    PIIX4PMState *s = opaque;
>> +    uint64_t val;
>>
>>       switch(addr) {
>>       case 0x00:
>> @@ -163,12 +168,18 @@ static void pm_ioport_read(IORange *ioport, uint64_t addr, unsigned width,
>>           break;
>>       }
>>       PIIX4_DPRINTF("PM readw port=0x%04x val=0x%04x\n", (unsigned int)addr, val);
>> -    *data = val;
>> +
>> +    return val;
>>   }
>>
>> -static const IORangeOps pm_iorange_ops = {
>> +static const MemoryRegionOps pm_io_ops = {
>>       .read = pm_ioport_read,
>>       .write = pm_ioport_write,
>> +    .endianness = DEVICE_LITTLE_ENDIAN,
>> +    .impl = {
>> +        .min_access_size = 2,
>> +        .max_access_size = 2,
>> +    },
>>   };
>>
>>   static void apm_ctrl_changed(uint32_t val, void *arg)
>> @@ -185,7 +196,8 @@ static void apm_ctrl_changed(uint32_t val, void *arg)
>>       }
>>   }
>>
>> -static void acpi_dbg_writel(void *opaque, uint32_t addr, uint32_t val)
>> +static void acpi_dbg_writel(void *opaque, target_phys_addr_t addr,
>> +                            uint64_t val, unsigned size)
>>   {
>>       PIIX4_DPRINTF("ACPI: DBG: 0x%08x\n", val);
>>   }
>> @@ -200,8 +212,18 @@ static void pm_io_space_update(PIIX4PMState *s)
>>
>>           /* XXX: need to improve memory and ioport allocation */
>>           PIIX4_DPRINTF("PM: mapping to 0x%x\n", pm_io_base);
>> -        iorange_init(&s->ioport,&pm_iorange_ops, pm_io_base, 64);
>> -        ioport_register(&s->ioport);
>> +
>> +        if (!s->pm_io_base) {
>> +            memory_region_add_subregion(pci_address_space_io(&s->dev),
>> +                                        pm_io_base,&s->pm_io);
>>      
> This doesn't work reliably. Add the region in a disabled state (ie. the
> default) during init, only toggle enable and update the base here.
>    
What is the default base address ? The datasheet
doesn't give default value. But the base address seems to
always be 0xb000. Is there any drawbacks to used it ?

-- 
Julien Grall