qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Jan Kiszka
Subject: Re: [Qemu-devel] [PATCH V8 4/8] hw/acpi_piix4.c: replace register_ioport*
Date: Tue, 04 Sep 2012 17:15:04 +0200
User-agent: Mozilla/5.0 (X11; U; Linux i686 (x86_64); de; rv:1.8.1.12) Gecko/20080226 SUSE/2.0.0.12-1.1 Thunderbird/2.0.0.12 Mnenhy/0.7.5.666

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?

> +    },
>  };
>  
>  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

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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]