Re: [Qemu-devel] [PATCH v3 07/26] pc/piix_pci: factor out smram/pam logic

["Michael S. Tsirkin" <mst@redhat.com>]

On Fri, Oct 19, 2012 at 04:43:30PM -0400, Jason Baron wrote:
> From: Isaku Yamahata <yamahata@valinux.co.jp>
> 
> Factor out smram/pam logic for later use.
> Which will be used by q35 too.
> 
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> [jbaron@redhat.com: changes for updated memory API]
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> Signed-off-by: Jason Baron <jbaron@redhat.com>

I dropped this patch from pci branch for now as it needs to be rebased.
Can you do this pls?

> ---
>  hw/i386/Makefile.objs |    1 +
>  hw/pam.c              |  120 +++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/pam.h              |   98 ++++++++++++++++++++++++++++++++++++++++
>  hw/piix_pci.c         |   65 ++++----------------------
>  4 files changed, 229 insertions(+), 55 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 8c764bb..2f0c172 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-$(CONFIG_XEN) += xen_platform.o xen_apic.o
>  obj-$(CONFIG_XEN_PCI_PASSTHROUGH) += xen-host-pci-device.o
>  obj-$(CONFIG_XEN_PCI_PASSTHROUGH) += xen_pt.o xen_pt_config_init.o xen_pt_msi.o
> diff --git a/hw/pam.c b/hw/pam.c
> new file mode 100644
> index 0000000..9ec5861
> --- /dev/null
> +++ b/hw/pam.c
> @@ -0,0 +1,120 @@
> +/*
> + * QEMU i440FX/PIIX3 PCI Bridge Emulation
> + *
> + * Copyright (c) 2006 Fabrice Bellard
> + *
> + * 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.
> + *
> + * Split out from piix_pci.c
> + * Copyright (c) 2011 Isaku Yamahata <yamahata at valinux co jp>
> + *                    VA Linux Systems Japan K.K.
> + * Copyright (c) 2012 Jason Baron <jbaron@redhat.com>
> + *
> + */
> +
> +#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);
> +    }
> +}
> +
> +static void pam_update_seg(PAMMemoryRegion *mem, uint32_t start, uint32_t size,
> +                           MemoryRegion *ram_memory,
> +                           MemoryRegion *pci_address_space,
> +                           MemoryRegion *system_memory, uint8_t attr)
> +{
> +    if (mem->initialized) {
> +        memory_region_del_subregion(system_memory, &mem->mem);
> +        memory_region_destroy(&mem->mem);
> +    }
> +
> +    switch (attr) {
> +    case PAM_ATTR_WE | PAM_ATTR_RE:
> +        /* RAM */
> +        memory_region_init_alias(&mem->mem, "pam-ram", ram_memory,
> +                                 start, size);
> +        break;
> +    case PAM_ATTR_RE:
> +        /* ROM (XXX: not quite correct) */
> +        memory_region_init_alias(&mem->mem, "pam-rom", ram_memory,
> +                                 start, size);
> +        memory_region_set_readonly(&mem->mem, true);
> +        break;
> +    case PAM_ATTR_WE:
> +    case 0:
> +        /* XXX: should distinguish read/write cases */
> +        memory_region_init_alias(&mem->mem, "pam-pci", pci_address_space,
> +                                 start, size);
> +        break;
> +    default:
> +        abort();
> +        break;
> +    }
> +    memory_region_add_subregion_overlap(system_memory, start, &mem->mem, 1);
> +    mem->initialized = true;
> +
> +}
> +
> +static uint8_t pam_attr(uint8_t val, int hi)
> +{
> +    return (val >> ((!!hi) * 4)) & PAM_ATTR_MASK;
> +}
> +
> +void pam_update(PAMMemoryRegion *mem, int idx, uint8_t val,
> +                MemoryRegion *ram_memory, MemoryRegion *pci_address_space,
> +                MemoryRegion *system_memory)
> +{
> +    uint32_t phys_addr;
> +    int map_idx;
> +
> +    assert(0 <= idx && idx <= PAM_IDX_MAX);
> +
> +    if (idx == 0) {
> +        pam_update_seg(&mem[0], PAM_BIOS_BASE, PAM_BIOS_SIZE, ram_memory,
> +                       pci_address_space, system_memory, pam_attr(val, 1));
> +        return;
> +    }
> +
> +    map_idx = (idx - 1) * 2;
> +
> +    phys_addr = PAM_EXPAN_BASE + PAM_EXPAN_SIZE * map_idx;
> +    pam_update_seg(&mem[map_idx + 1], phys_addr, PAM_EXPAN_SIZE, ram_memory,
> +                   pci_address_space, system_memory, pam_attr(val, 0));
> +
> +    phys_addr += PAM_EXPAN_SIZE;
> +    pam_update_seg(&mem[map_idx + 2], phys_addr, PAM_EXPAN_SIZE, ram_memory,
> +                   pci_address_space, system_memory, pam_attr(val, 1));
> +}
> diff --git a/hw/pam.h b/hw/pam.h
> new file mode 100644
> index 0000000..ce89a2a
> --- /dev/null
> +++ b/hw/pam.h
> @@ -0,0 +1,98 @@
> +#ifndef QEMU_PAM_H
> +#define QEMU_PAM_H
> +
> +/*
> + * Copyright (c) 2006 Fabrice Bellard
> + *
> + * 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.
> + */
> +/*
> + * Split out from piix_pci.c
> + * Copyright (c) 2011 Isaku Yamahata <yamahata at valinux co jp>
> + *                    VA Linux Systems Japan K.K.
> + * Copyright (c) 2012 Jason Baron <jbaron@redhat.com>
> + *
> + * 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 - 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)
> +
> +#define PAM_IDX_MAX     6       /* pam0 - pam6 */
> +
> +/* 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 mem;
> +    bool initialized;
> +} 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 pam_update(PAMMemoryRegion *mem, int idx, uint8_t val,
> +                MemoryRegion *ram_memory, MemoryRegion *pci_address_space,
> +                MemoryRegion *system_memory);
> +
> +#endif /* QEMU_PAM_H */
> diff --git a/hw/piix_pci.c b/hw/piix_pci.c
> index 537fc19..02b161d 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 mem;
> -    bool initialized;
> -} PAMMemoryRegion;
> -
>  struct PCII440FXState {
>      PCIDevice dev;
>      MemoryRegion *system_memory;
> @@ -105,56 +101,16 @@ static int pci_slot_get_pirq(PCIDevice *pci_dev, int pci_intx)
>      return (pci_intx + slot_addend) & 3;
>  }
>  
> -static void update_pam(PCII440FXState *d, uint32_t start, uint32_t end, int r,
> -                       PAMMemoryRegion *mem)
> -{
> -    if (mem->initialized) {
> -        memory_region_del_subregion(d->system_memory, &mem->mem);
> -        memory_region_destroy(&mem->mem);
> -    }
> -
> -    //    printf("ISA mapping %08x-0x%08x: %d\n", start, end, r);
> -    switch(r) {
> -    case 3:
> -        /* RAM */
> -        memory_region_init_alias(&mem->mem, "pam-ram", d->ram_memory,
> -                                 start, end - start);
> -        break;
> -    case 1:
> -        /* ROM (XXX: not quite correct) */
> -        memory_region_init_alias(&mem->mem, "pam-rom", d->ram_memory,
> -                                 start, end - start);
> -        memory_region_set_readonly(&mem->mem, true);
> -        break;
> -    case 2:
> -    case 0:
> -        /* XXX: should distinguish read/write cases */
> -        memory_region_init_alias(&mem->mem, "pam-pci", d->pci_address_space,
> -                                 start, end - start);
> -        break;
> -    }
> -    memory_region_add_subregion_overlap(d->system_memory,
> -                                        start, &mem->mem, 1);
> -    mem->initialized = 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, 0xf0000, 0x100000, (d->dev.config[I440FX_PAM] >> 4) & 3,
> -               &d->pam_regions[0]);
> -    for(i = 0; i < 12; i++) {
> -        r = (d->dev.config[(i >> 1) + (I440FX_PAM + 1)] >> ((i & 1) * 4)) & 3;
> -        update_pam(d, 0xc0000 + 0x4000 * i, 0xc0000 + 0x4000 * (i + 1), r,
> -                   &d->pam_regions[i+1]);
> +    for (i = 0; i <= PAM_IDX_MAX; i++) {
> +        pam_update(&d->pam_regions[0], i, d->dev.config[I440FX_PAM + i],
> +                    d->ram_memory, d->pci_address_space, d->system_memory);
>      }
> -    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();
>  }
>  
> @@ -162,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();
>  }
>  
>  
> -- 
> 1.7.1