| Message ID | 13a02c99e22d6180bc4a5d58d7b38b249773fa73.1346741532.git.julien.grall@citrix.com |
|---|---|
| State | New |
| Headers | show |
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 <julien.grall@citrix.com> > --- > 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? > + }, > }; > > 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,11 @@ 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); > + > + memory_region_set_address(&s->pm_io, pm_io_base); > + memory_region_set_enabled(&s->pm_io, true); > + } else { > + memory_region_set_enabled(&s->pm_io, false); > } > } > > @@ -395,6 +410,15 @@ static const MemoryRegionOps smb_io_ops = { > }, > }; > > +static const MemoryRegionOps acpi_io_ops = { > + .write = acpi_dbg_writel, > + .endianness = DEVICE_LITTLE_ENDIAN, > + .impl = { > + .min_access_size = 4, > + .max_access_size = 4, > + }, > +}; > + > static int piix4_pm_initfn(PCIDevice *dev) > { > PIIX4PMState *s = DO_UPCAST(PIIX4PMState, dev, dev); > @@ -409,7 +433,9 @@ static int piix4_pm_initfn(PCIDevice *dev) > /* APM */ > apm_init(dev, &s->apm, apm_ctrl_changed, s); > > - register_ioport_write(ACPI_DBG_IO_ADDR, 4, 4, acpi_dbg_writel, s); > + memory_region_init_io(&s->acpi_io, &acpi_io_ops, s, "piix4-acpi", 4); > + memory_region_add_subregion(pci_address_space_io(dev), ACPI_DBG_IO_ADDR, > + &s->acpi_io); > > if (s->kvm_enabled) { > /* Mark SMM as already inited to prevent SMM from running. KVM does not > @@ -427,6 +453,12 @@ static int piix4_pm_initfn(PCIDevice *dev) > memory_region_add_subregion(pci_address_space_io(dev), s->smb_io_base, > &s->smb_io); > > + /* PM */ > + memory_region_init_io(&s->pm_io, &pm_io_ops, s, "piix4-pm", 64); > + memory_region_set_enabled(&s->pm_io, false); > + memory_region_add_subregion(pci_address_space_io(&s->dev), > + PM_BASE, &s->pm_io); > + > acpi_pm_tmr_init(&s->ar, pm_tmr_timer); > acpi_gpe_init(&s->ar, GPE_LEN); > > @@ -510,16 +542,17 @@ static void piix4_pm_register_types(void) > > type_init(piix4_pm_register_types) > > -static uint32_t gpe_readb(void *opaque, uint32_t addr) > +static uint64_t gpe_readb(void *opaque, target_phys_addr_t addr, unsigned size) > { > PIIX4PMState *s = opaque; > - uint32_t val = acpi_gpe_ioport_readb(&s->ar, addr); > + uint64_t val = acpi_gpe_ioport_readb(&s->ar, addr); > > PIIX4_DPRINTF("gpe read %x == %x\n", addr, val); > return val; > } > > -static void gpe_writeb(void *opaque, uint32_t addr, uint32_t val) > +static void gpe_writeb(void *opaque, target_phys_addr_t addr, uint64_t val, > + unsigned size) > { > PIIX4PMState *s = opaque; > > @@ -551,21 +584,24 @@ static uint32_t pci_down_read(void *opaque, uint32_t addr) > return val; > } > > -static uint32_t pci_features_read(void *opaque, uint32_t addr) > +static uint64_t pci_features_read(void *opaque, target_phys_addr_t addr, > + unsigned size) > { > /* No feature defined yet */ > PIIX4_DPRINTF("pci_features_read %x\n", 0); > return 0; > } > > -static void pciej_write(void *opaque, uint32_t addr, uint32_t val) > +static void pciej_write(void *opaque, target_phys_addr_t addr, uint64_t val, > + unsigned size) > { > acpi_piix_eject_slot(opaque, val); > > PIIX4_DPRINTF("pciej write %x <== %d\n", addr, val); > } > > -static uint32_t pcirmv_read(void *opaque, uint32_t addr) > +static uint64_t pcirmv_read(void *opaque, target_phys_addr_t addr, > + unsigned size) > { > PIIX4PMState *s = opaque; > > @@ -575,20 +611,65 @@ static uint32_t pcirmv_read(void *opaque, uint32_t addr) > static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev, > PCIHotplugState state); > > -static void piix4_acpi_system_hot_add_init(PCIBus *bus, PIIX4PMState *s) > -{ > +static const MemoryRegionOps acpi_hot_io_ops = { > + .read = gpe_readb, > + .write = gpe_writeb, > + .endianness = DEVICE_LITTLE_ENDIAN, > + .impl = { > + .min_access_size = 1, > + .max_access_size = 1, > + }, > +}; > > - register_ioport_write(GPE_BASE, GPE_LEN, 1, gpe_writeb, s); > - register_ioport_read(GPE_BASE, GPE_LEN, 1, gpe_readb, s); > - acpi_gpe_blk(&s->ar, GPE_BASE); > +/* PCI hot plug registers */ > +static const MemoryRegionPortio pci_hot_portio_list[] = { > + { 0x00, 4, 4, .read = pci_up_read, }, /* 0xae00 */ > + { 0x04, 4, 4, .read = pci_down_read, }, /* 0xae04 */ > + PORTIO_END_OF_LIST(), > +}; > > - register_ioport_read(PCI_UP_BASE, 4, 4, pci_up_read, s); > - register_ioport_read(PCI_DOWN_BASE, 4, 4, pci_down_read, s); > +static const MemoryRegionOps pciej_hot_io_ops = { > + .read = pci_features_read, > + .write = pciej_write, > + .endianness = DEVICE_LITTLE_ENDIAN, > + .impl = { > + .min_access_size = 4, > + .max_access_size = 4, > + }, > +}; > + > +static const MemoryRegionOps pcirmv_hot_io_ops = { > + .read = pcirmv_read, > + .endianness = DEVICE_LITTLE_ENDIAN, > + .impl = { > + .min_access_size = 4, > + .max_access_size = 4, > + }, > +}; > > - register_ioport_write(PCI_EJ_BASE, 4, 4, pciej_write, s); > - register_ioport_read(PCI_EJ_BASE, 4, 4, pci_features_read, s); > +static void piix4_acpi_system_hot_add_init(PCIBus *bus, PIIX4PMState *s) > +{ > > - register_ioport_read(PCI_RMV_BASE, 4, 4, pcirmv_read, s); > + memory_region_init_io(&s->acpi_hot_io, &acpi_hot_io_ops, s, > + "piix4-acpi-hot", GPE_LEN); > + memory_region_add_subregion(pci_address_space_io(&s->dev), GPE_BASE, > + &s->acpi_hot_io); > + acpi_gpe_blk(&s->ar, 0); > + > + portio_list_init(&s->pci_hot_port_list, pci_hot_portio_list, s, > + "piix4-pci-hot"); > + portio_list_add(&s->pci_hot_port_list, pci_address_space_io(&s->dev), > + PCI_BASE); > + > + memory_region_init_io(&s->pciej_hot_io, &pciej_hot_io_ops, s, > + "piix4-pciej-hot", 4); > + memory_region_add_subregion(pci_address_space_io(&s->dev), PCI_EJ_BASE, > + &s->pciej_hot_io); > + > + memory_region_init_io(&s->pcirmv_hot_io, &pcirmv_hot_io_ops, s, > + "piix4-pcirmv-hot", 4); > + memory_region_add_subregion(pci_address_space_io(&s->dev), PCI_RMV_BASE, > + &s->pcirmv_hot_io); > > pci_bus_hotplug(bus, piix4_device_hotplug, &s->dev.qdev); > } > 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. Jan
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<julien.grall@citrix.com> >> --- >> 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.
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<julien.grall@citrix.com> >>> --- >>> 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
On 2012-09-04 18:51, Jan Kiszka wrote: > 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<julien.grall@citrix.com> >>>> --- >>>> 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. ...and better also allow byte access. Then we should not be able to regress. Jan
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, + }, }; 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,11 @@ 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); + + memory_region_set_address(&s->pm_io, pm_io_base); + memory_region_set_enabled(&s->pm_io, true); + } else { + memory_region_set_enabled(&s->pm_io, false); } } @@ -395,6 +410,15 @@ static const MemoryRegionOps smb_io_ops = { }, }; +static const MemoryRegionOps acpi_io_ops = { + .write = acpi_dbg_writel, + .endianness = DEVICE_LITTLE_ENDIAN, + .impl = { + .min_access_size = 4, + .max_access_size = 4, + }, +}; + static int piix4_pm_initfn(PCIDevice *dev) { PIIX4PMState *s = DO_UPCAST(PIIX4PMState, dev, dev); @@ -409,7 +433,9 @@ static int piix4_pm_initfn(PCIDevice *dev) /* APM */ apm_init(dev, &s->apm, apm_ctrl_changed, s); - register_ioport_write(ACPI_DBG_IO_ADDR, 4, 4, acpi_dbg_writel, s); + memory_region_init_io(&s->acpi_io, &acpi_io_ops, s, "piix4-acpi", 4); + memory_region_add_subregion(pci_address_space_io(dev), ACPI_DBG_IO_ADDR, + &s->acpi_io); if (s->kvm_enabled) { /* Mark SMM as already inited to prevent SMM from running. KVM does not @@ -427,6 +453,12 @@ static int piix4_pm_initfn(PCIDevice *dev) memory_region_add_subregion(pci_address_space_io(dev), s->smb_io_base, &s->smb_io); + /* PM */ + memory_region_init_io(&s->pm_io, &pm_io_ops, s, "piix4-pm", 64); + memory_region_set_enabled(&s->pm_io, false); + memory_region_add_subregion(pci_address_space_io(&s->dev), + PM_BASE, &s->pm_io); + acpi_pm_tmr_init(&s->ar, pm_tmr_timer); acpi_gpe_init(&s->ar, GPE_LEN); @@ -510,16 +542,17 @@ static void piix4_pm_register_types(void) type_init(piix4_pm_register_types) -static uint32_t gpe_readb(void *opaque, uint32_t addr) +static uint64_t gpe_readb(void *opaque, target_phys_addr_t addr, unsigned size) { PIIX4PMState *s = opaque; - uint32_t val = acpi_gpe_ioport_readb(&s->ar, addr); + uint64_t val = acpi_gpe_ioport_readb(&s->ar, addr); PIIX4_DPRINTF("gpe read %x == %x\n", addr, val); return val; } -static void gpe_writeb(void *opaque, uint32_t addr, uint32_t val) +static void gpe_writeb(void *opaque, target_phys_addr_t addr, uint64_t val, + unsigned size) { PIIX4PMState *s = opaque; @@ -551,21 +584,24 @@ static uint32_t pci_down_read(void *opaque, uint32_t addr) return val; } -static uint32_t pci_features_read(void *opaque, uint32_t addr) +static uint64_t pci_features_read(void *opaque, target_phys_addr_t addr, + unsigned size) { /* No feature defined yet */ PIIX4_DPRINTF("pci_features_read %x\n", 0); return 0; } -static void pciej_write(void *opaque, uint32_t addr, uint32_t val) +static void pciej_write(void *opaque, target_phys_addr_t addr, uint64_t val, + unsigned size) { acpi_piix_eject_slot(opaque, val); PIIX4_DPRINTF("pciej write %x <== %d\n", addr, val); } -static uint32_t pcirmv_read(void *opaque, uint32_t addr) +static uint64_t pcirmv_read(void *opaque, target_phys_addr_t addr, + unsigned size) { PIIX4PMState *s = opaque; @@ -575,20 +611,65 @@ static uint32_t pcirmv_read(void *opaque, uint32_t addr) static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev, PCIHotplugState state); -static void piix4_acpi_system_hot_add_init(PCIBus *bus, PIIX4PMState *s) -{ +static const MemoryRegionOps acpi_hot_io_ops = { + .read = gpe_readb, + .write = gpe_writeb, + .endianness = DEVICE_LITTLE_ENDIAN, + .impl = { + .min_access_size = 1, + .max_access_size = 1, + }, +}; - register_ioport_write(GPE_BASE, GPE_LEN, 1, gpe_writeb, s); - register_ioport_read(GPE_BASE, GPE_LEN, 1, gpe_readb, s); - acpi_gpe_blk(&s->ar, GPE_BASE); +/* PCI hot plug registers */ +static const MemoryRegionPortio pci_hot_portio_list[] = { + { 0x00, 4, 4, .read = pci_up_read, }, /* 0xae00 */ + { 0x04, 4, 4, .read = pci_down_read, }, /* 0xae04 */ + PORTIO_END_OF_LIST(), +}; - register_ioport_read(PCI_UP_BASE, 4, 4, pci_up_read, s); - register_ioport_read(PCI_DOWN_BASE, 4, 4, pci_down_read, s); +static const MemoryRegionOps pciej_hot_io_ops = { + .read = pci_features_read, + .write = pciej_write, + .endianness = DEVICE_LITTLE_ENDIAN, + .impl = { + .min_access_size = 4, + .max_access_size = 4, + }, +}; + +static const MemoryRegionOps pcirmv_hot_io_ops = { + .read = pcirmv_read, + .endianness = DEVICE_LITTLE_ENDIAN, + .impl = { + .min_access_size = 4, + .max_access_size = 4, + }, +}; - register_ioport_write(PCI_EJ_BASE, 4, 4, pciej_write, s); - register_ioport_read(PCI_EJ_BASE, 4, 4, pci_features_read, s); +static void piix4_acpi_system_hot_add_init(PCIBus *bus, PIIX4PMState *s) +{ - register_ioport_read(PCI_RMV_BASE, 4, 4, pcirmv_read, s); + memory_region_init_io(&s->acpi_hot_io, &acpi_hot_io_ops, s, + "piix4-acpi-hot", GPE_LEN); + memory_region_add_subregion(pci_address_space_io(&s->dev), GPE_BASE, + &s->acpi_hot_io); + acpi_gpe_blk(&s->ar, 0); + + portio_list_init(&s->pci_hot_port_list, pci_hot_portio_list, s, + "piix4-pci-hot"); + portio_list_add(&s->pci_hot_port_list, pci_address_space_io(&s->dev), + PCI_BASE); + + memory_region_init_io(&s->pciej_hot_io, &pciej_hot_io_ops, s, + "piix4-pciej-hot", 4); + memory_region_add_subregion(pci_address_space_io(&s->dev), PCI_EJ_BASE, + &s->pciej_hot_io); + + memory_region_init_io(&s->pcirmv_hot_io, &pcirmv_hot_io_ops, s, + "piix4-pcirmv-hot", 4); + memory_region_add_subregion(pci_address_space_io(&s->dev), PCI_RMV_BASE, + &s->pcirmv_hot_io); pci_bus_hotplug(bus, piix4_device_hotplug, &s->dev.qdev); }
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 | 145 +++++++++++++++++++++++++++++++++++++++++++------------ 1 files changed, 113 insertions(+), 32 deletions(-)