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

[Julien Grall]

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.

--
Julien Grall