Re: [Qemu-devel] [PATCH v1 01/13] pc/piix_pci: factor out smram/pam logic

[Anthony Liguori]

Jason Baron <address@hidden> writes:

> From: Isaku Yamahata <address@hidden>
>
> Factor out smram/pam logic for later use.
> Which will be used by q35 too.
>
> address@hidden: changes for updated memory API]
> Signed-off-by: Isaku Yamahata <address@hidden>
> Signed-off-by: Jason Baron <address@hidden>

This is really not the right approach to solving this problem.

"pam" is not a device.

Instead, the common bits here are essentially the northbridge logic.
That's what we should model here.

It's pretty simple actually.  Just throw this all into a device where
the memory regions are owned by the device.  But also make the device
own creation of all RAM (not just the areas controlled by PAM).

Then embed the northbridge into the i440fx and q35.

Regards,

Anthony Liguori

> ---
>  hw/i386/Makefile.objs |    1 +
>  hw/pam.c              |   87 ++++++++++++++++++++++++++++++++++++++++++++
>  hw/pam.h              |   97 
> +++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/piix_pci.c         |   68 ++++++++---------------------------
>  4 files changed, 200 insertions(+), 53 deletions(-)
>  create mode 100644 hw/pam.c
>  create mode 100644 hw/pam.h
>
> diff --git a/hw/i386/Makefile.objs b/hw/i386/Makefile.objs
> index d400d1a..693bd18 100644
> --- a/hw/i386/Makefile.objs
> +++ b/hw/i386/Makefile.objs
> @@ -6,6 +6,7 @@ obj-y += pci-hotplug.o smbios.o wdt_ib700.o
>  obj-y += debugcon.o multiboot.o
>  obj-y += pc_piix.o
>  obj-y += pc_sysfw.o
> +obj-y += pam.o
>  obj-y += acpi_ich9.o lpc_ich9.o smbus_ich9.o
>  obj-$(CONFIG_XEN) += xen_platform.o xen_apic.o
>  obj-$(CONFIG_XEN_PCI_PASSTHROUGH) += xen-host-pci-device.o
> diff --git a/hw/pam.c b/hw/pam.c
> new file mode 100644
> index 0000000..a95e2cf
> --- /dev/null
> +++ b/hw/pam.c
> @@ -0,0 +1,87 @@
> +/*
> + * QEMU i440FX/PIIX3 PCI Bridge Emulation
> + *
> + * Copyright (c) 2006 Fabrice Bellard
> + * Copyright (c) 2011 Isaku Yamahata <yamahata at valinux co jp>
> + *                    VA Linux Systems Japan K.K.
> + * Copyright (c) 2012 Jason Baron <address@hidden>
> + *
> + * Split out from piix_pci.c
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a 
> copy
> + * of this software and associated documentation files (the "Software"), to 
> deal
> + * in the Software without restriction, including without limitation the 
> rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
> FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +#include "sysemu.h"
> +#include "pam.h"
> +
> +void smram_update(MemoryRegion *smram_region, uint8_t smram,
> +                  uint8_t smm_enabled)
> +{
> +    bool smram_enabled;
> +
> +    smram_enabled = ((smm_enabled && (smram & SMRAM_G_SMRAME)) ||
> +                        (smram & SMRAM_D_OPEN));
> +    memory_region_set_enabled(smram_region, !smram_enabled);
> +}
> +
> +void smram_set_smm(uint8_t *host_smm_enabled, int smm, uint8_t smram,
> +                   MemoryRegion *smram_region)
> +{
> +    uint8_t smm_enabled = (smm != 0);
> +    if (*host_smm_enabled != smm_enabled) {
> +        *host_smm_enabled = smm_enabled;
> +        smram_update(smram_region, smram, *host_smm_enabled);
> +    }
> +}
> +
> +void init_pam(MemoryRegion *ram_memory, MemoryRegion *system_memory,
> +              MemoryRegion *pci_address_space, PAMMemoryRegion *mem,
> +              uint32_t start, uint32_t size)
> +{
> +    int i;
> +
> +    /* RAM */
> +    memory_region_init_alias(&mem->alias[3], "pam-ram", ram_memory,
> +                             start, size);
> +    /* ROM (XXX: not quite correct) */
> +    memory_region_init_alias(&mem->alias[1], "pam-rom", ram_memory,
> +                             start, size);
> +    memory_region_set_readonly(&mem->alias[1], true);
> +
> +    /* XXX: should distinguish read/write cases */
> +    memory_region_init_alias(&mem->alias[0], "pam-pci", pci_address_space,
> +                             start, size);
> +    memory_region_init_alias(&mem->alias[2], "pam-pci", pci_address_space,
> +                             start, size);
> +
> +    for (i = 0; i < 4; ++i) {
> +        memory_region_set_enabled(&mem->alias[i], false);
> +        memory_region_add_subregion_overlap(system_memory, start,
> +                                            &mem->alias[i], 1);
> +    }
> +    mem->current = 0;
> +}
> +
> +void pam_update(PAMMemoryRegion *pam, int idx, uint8_t val)
> +{
> +    assert(0 <= idx && idx <= 12);
> +
> +    memory_region_set_enabled(&pam->alias[pam->current], false);
> +    pam->current = (val >> ((!(idx & 1)) * 4)) & PAM_ATTR_MASK;
> +    memory_region_set_enabled(&pam->alias[pam->current], true);
> +}
> diff --git a/hw/pam.h b/hw/pam.h
> new file mode 100644
> index 0000000..2d77ebe
> --- /dev/null
> +++ b/hw/pam.h
> @@ -0,0 +1,97 @@
> +#ifndef QEMU_PAM_H
> +#define QEMU_PAM_H
> +
> +/*
> + * Copyright (c) 2006 Fabrice Bellard
> + * Copyright (c) 2011 Isaku Yamahata <yamahata at valinux co jp>
> + *               VA Linux Systems Japan K.K.
> + * Copyright (c) 2012 Jason Baron <address@hidden>
> + *
> + * Split out from piix_pci.c
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a 
> copy
> + * of this software and associated documentation files (the "Software"), to 
> deal
> + * in the Software without restriction, including without limitation the 
> rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
> FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +/*
> + * SMRAM memory area and PAM memory area in Legacy address range for PC.
> + * PAM: Programmable Attribute Map registers
> + *
> + * 0xa0000 - 0xbffff compatible SMRAM
> + *
> + * 0xc0000 - 0xc3fff Expansion area memory segments
> + * 0xc4000 - 0xc7fff
> + * 0xc8000 - 0xcbfff
> + * 0xcc000 - 0xcffff
> + * 0xd0000 - 0xd3fff
> + * 0xd4000 - 0xd7fff
> + * 0xd8000 - 0xdbfff
> + * 0xdc000 - 0xdffff
> + * 0xe0000 - 0xe3fff Extended System BIOS Area Memory Segments
> + * 0xe4000 - 0xe7fff
> + * 0xe8000 - 0xebfff
> + * 0xec000 - 0xeffff
> + *
> + * 0xf0000 - 0xfffff System BIOS Area Memory Segments
> + */
> +
> +#include "qemu-common.h"
> +#include "memory.h"
> +
> +#define SMRAM_C_BASE    0xa0000
> +#define SMRAM_C_END     0xc0000
> +#define SMRAM_C_SIZE    0x20000
> +
> +#define PAM_EXPAN_BASE  0xc0000
> +#define PAM_EXPAN_SIZE  0x04000
> +
> +#define PAM_EXBIOS_BASE 0xe0000
> +#define PAM_EXBIOS_SIZE 0x04000
> +
> +#define PAM_BIOS_BASE   0xf0000
> +#define PAM_BIOS_END    0xfffff
> +/* 64KB: Intel 3 series express chipset family p. 58*/
> +#define PAM_BIOS_SIZE   0x10000
> +
> +/* PAM registers: log nibble and high nibble*/
> +#define PAM_ATTR_WE     ((uint8_t)2)
> +#define PAM_ATTR_RE     ((uint8_t)1)
> +#define PAM_ATTR_MASK   ((uint8_t)3)
> +
> +/* SMRAM register */
> +#define SMRAM_D_OPEN           ((uint8_t)(1 << 6))
> +#define SMRAM_D_CLS            ((uint8_t)(1 << 5))
> +#define SMRAM_D_LCK            ((uint8_t)(1 << 4))
> +#define SMRAM_G_SMRAME         ((uint8_t)(1 << 3))
> +#define SMRAM_C_BASE_SEG_MASK  ((uint8_t)0x7)
> +#define SMRAM_C_BASE_SEG       ((uint8_t)0x2)  /* hardwired to b010 */
> +
> +typedef struct PAMMemoryRegion {
> +    MemoryRegion alias[4];  /* index = PAM value */
> +    unsigned current;
> +} PAMMemoryRegion;
> +
> +void smram_update(MemoryRegion *smram_region, uint8_t smram,
> +                  uint8_t smm_enabled);
> +void smram_set_smm(uint8_t *host_smm_enabled, int smm, uint8_t smram,
> +                   MemoryRegion *smram_region);
> +void init_pam(MemoryRegion *ram, MemoryRegion *system, MemoryRegion *pci,
> +              PAMMemoryRegion *mem, uint32_t start, uint32_t size);
> +void pam_update(PAMMemoryRegion *mem, int idx, uint8_t val);
> +
> +#endif /* QEMU_PAM_H */
> diff --git a/hw/piix_pci.c b/hw/piix_pci.c
> index 9af5847..ba1b3de 100644
> --- a/hw/piix_pci.c
> +++ b/hw/piix_pci.c
> @@ -30,6 +30,7 @@
>  #include "sysbus.h"
>  #include "range.h"
>  #include "xen.h"
> +#include "pam.h"
>  
>  /*
>   * I440FX chipset data sheet.
> @@ -68,11 +69,6 @@ typedef struct PIIX3State {
>      int32_t pci_irq_levels_vmstate[PIIX_NUM_PIRQS];
>  } PIIX3State;
>  
> -typedef struct PAMMemoryRegion {
> -    MemoryRegion alias[4];  /* index = PAM value */
> -    unsigned current;
> -} PAMMemoryRegion;
> -
>  struct PCII440FXState {
>      PCIDevice dev;
>      MemoryRegion *system_memory;
> @@ -105,52 +101,16 @@ static int pci_slot_get_pirq(PCIDevice *pci_dev, int 
> pci_intx)
>      return (pci_intx + slot_addend) & 3;
>  }
>  
> -static void init_pam(PCII440FXState *d, PAMMemoryRegion *mem,
> -                     uint32_t start, uint32_t size)
> -{
> -    int i;
> -
> -    /* RAM */
> -    memory_region_init_alias(&mem->alias[3], "pam-ram", d->ram_memory, 
> start, size);
> -    /* ROM (XXX: not quite correct) */
> -    memory_region_init_alias(&mem->alias[1], "pam-rom", d->ram_memory, 
> start, size);
> -    memory_region_set_readonly(&mem->alias[1], true);
> -
> -    /* XXX: should distinguish read/write cases */
> -    memory_region_init_alias(&mem->alias[0], "pam-pci", d->pci_address_space,
> -                             start, size);
> -    memory_region_init_alias(&mem->alias[2], "pam-pci", d->pci_address_space,
> -                             start, size);
> -
> -    for (i = 0; i < 4; ++i) {
> -        memory_region_set_enabled(&mem->alias[i], false);
> -        memory_region_add_subregion_overlap(d->system_memory, start, 
> &mem->alias[i], 1);
> -    }
> -    mem->current = 0;
> -}
> -
> -static void update_pam(PAMMemoryRegion *pam, unsigned r)
> -{
> -    memory_region_set_enabled(&pam->alias[pam->current], false);
> -    pam->current = r;
> -    memory_region_set_enabled(&pam->alias[pam->current], true);
> -}
> -
>  static void i440fx_update_memory_mappings(PCII440FXState *d)
>  {
> -    int i, r;
> -    uint32_t smram;
> -    bool smram_enabled;
> +    int i;
>  
>      memory_region_transaction_begin();
> -    update_pam(&d->pam_regions[0], (d->dev.config[I440FX_PAM] >> 4) & 3);
> -    for(i = 0; i < 12; i++) {
> -        r = (d->dev.config[(i >> 1) + (I440FX_PAM + 1)] >> ((i & 1) * 4)) & 
> 3;
> -        update_pam(&d->pam_regions[i+1], r);
> +    for (i = 0; i < 13; i++) {
> +        pam_update(&d->pam_regions[i], i,
> +                   d->dev.config[I440FX_PAM + ((i + 1) / 2)]);
>      }
> -    smram = d->dev.config[I440FX_SMRAM];
> -    smram_enabled = (d->smm_enabled && (smram & 0x08)) || (smram & 0x40);
> -    memory_region_set_enabled(&d->smram_region, !smram_enabled);
> +    smram_update(&d->smram_region, d->dev.config[I440FX_SMRAM], 
> d->smm_enabled);
>      memory_region_transaction_commit();
>  }
>  
> @@ -158,11 +118,10 @@ static void i440fx_set_smm(int val, void *arg)
>  {
>      PCII440FXState *d = arg;
>  
> -    val = (val != 0);
> -    if (d->smm_enabled != val) {
> -        d->smm_enabled = val;
> -        i440fx_update_memory_mappings(d);
> -    }
> +    memory_region_transaction_begin();
> +    smram_set_smm(&d->smm_enabled, val, d->dev.config[I440FX_SMRAM],
> +                  &d->smram_region);
> +    memory_region_transaction_commit();
>  }
>  
>  
> @@ -300,9 +259,12 @@ static PCIBus *i440fx_common_init(const char 
> *device_name,
>      memory_region_add_subregion_overlap(f->system_memory, 0xa0000,
>                                          &f->smram_region, 1);
>      memory_region_set_enabled(&f->smram_region, false);
> -    init_pam(f, &f->pam_regions[0], 0xf0000, 0x10000);
> +    init_pam(f->ram_memory, f->system_memory, f->pci_address_space,
> +             &f->pam_regions[0], PAM_BIOS_BASE, PAM_BIOS_SIZE);
>      for (i = 0; i < 12; ++i) {
> -        init_pam(f, &f->pam_regions[i+1], 0xc0000 + i * 0x4000, 0x4000);
> +        init_pam(f->ram_memory, f->system_memory, f->pci_address_space,
> +                 &f->pam_regions[i+1], PAM_EXPAN_BASE + i * PAM_EXPAN_SIZE,
> +                 PAM_EXPAN_SIZE);
>      }
>  
>      /* Xen supports additional interrupt routes from the PCI devices to
> -- 
> 1.7.1