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

[Jan Kiszka]

On 2012-09-04 18:33, Julien Grall wrote:
> On 09/04/2012 04:15 PM, Jan Kiszka wrote:
>> On 2012-09-04 09:28, 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<address@hidden>
>>> ---
>>>   hw/acpi_piix4.c |  145 
>>> +++++++++++++++++++++++++++++++++++++++++++------------
>>>   1 files changed, 113 insertions(+), 32 deletions(-)
>>>
>>> diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
>>> index 0b4ad24..cd70610 100644
>>> --- a/hw/acpi_piix4.c
>>> +++ b/hw/acpi_piix4.c
>>> @@ -41,10 +41,10 @@
>>>
>>>   #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
>>> +#define PM_BASE 0x00
>>>
>>>   #define PIIX4_PCI_HOTPLUG_STATUS 2
>>>
>>> @@ -55,7 +55,7 @@ struct pci_status {
>>>
>>>   typedef struct PIIX4PMState {
>>>       PCIDevice dev;
>>> -    IORange ioport;
>>> +    MemoryRegion pm_io;
>>>       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,
>>>      
>> Where do these constraints come from?
>>    
> I don't pay enough attention about the size.
> 
>> OK, this one breaks my Win7 guest. Following my suspect above and the
>> endless loop over
>>
>>      kvm_pio:              pio_read at 0xb008 size 4 count 1
>>
>> I played with max_access_size = 4 for pm_io_ops, and Windows boots
>> again. Looking at the details, the PIO range was apparently not properly
>> specified so far. It implements 2-bytes accesses for the offsets 0x00,
>> 0x02, 0x04 and 4-bytes access for 0x08. But the specification was that
>> accesses of all sizes are provided.
>>
>> Given this experience, we will have to review at least the hacky ACPI
>> stuff very carefully.
>>    
> 
> Could we change max_access_size to 4 and check on each PIO if
> the size is correct ? ie 2-bytes access for 0x00, 0x02, 0x04 and 4-bytes
> access for 0x08.
> 

TBH, I have no clue what access constraints exist for this PIO region.
Unless someone can point them out, it is probably best to not apply any
additional checks, like in the original code, just extend to 4 as
maximum size.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux