[Qemu-devel] Re: [PATCH 09/11] piix_pci: define symbolic value for PAM0, PAM6 and SMRAM.

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

On Mon, Dec 14, 2009 at 09:48:24PM +0900, Isaku Yamahata wrote:
> Define symbolic value in i440fx configuration space
> for 0x59, 0x5f and 0x7f and use them.
> 
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>

Good overall.
Can you pls verify that applying this patch generates
same binary as before? Small mistakes are hard to catch.

Also I am not familiar with the hardware.
Anyone on list can review this patch?
When can one get the spec for this hardware?

> ---
>  hw/piix_pci.c |   13 +++++++++----
>  1 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/piix_pci.c b/hw/piix_pci.c
> index 1b67475..7bbaf50 100644
> --- a/hw/piix_pci.c
> +++ b/hw/piix_pci.c
> @@ -44,6 +44,10 @@ struct PCII440FXState {
>      PIIX3State *piix3;
>  };
>  
> +#define I440FX_PAM0     0x59
> +#define I440FX_PAM6     0x5f

Hmm, will we end up with 7 of these?
Maybe
#define I440FX_PAM (0x59) and use
I440FX_PAM + 6 instead of I440FX_PAM6?

> +#define I440FX_SMRAM    0x72
> +
>  static void piix3_set_irq(void *opaque, int irq_num, int level);
>  
>  /* return the global irq number corresponding to a given device irq
> @@ -88,12 +92,12 @@ static void i440fx_update_memory_mappings(PCII440FXState *d)
>      int i, r;
>      uint32_t smram, addr;
>  
> -    update_pam(d, 0xf0000, 0x100000, (d->dev.config[0x59] >> 4) & 3);
> +    update_pam(d, 0xf0000, 0x100000, (d->dev.config[I440FX_PAM0] >> 4) & 3);
>      for(i = 0; i < 12; i++) {
>          r = (d->dev.config[(i >> 1) + 0x5a] >> ((i & 1) * 4)) & 3;

is 0x5a a PAM register as well?

>          update_pam(d, 0xc0000 + 0x4000 * i, 0xc0000 + 0x4000 * (i + 1), r);
>      }
> -    smram = d->dev.config[0x72];
> +    smram = d->dev.config[I440FX_SMRAM];
>      if ((d->smm_enabled && (smram & 0x08)) || (smram & 0x40)) {
>          cpu_register_physical_memory(0xa0000, 0x20000, 0xa0000);
>      } else {
> @@ -132,7 +136,8 @@ static void i440fx_write_config(PCIDevice *dev,
>  
>      /* XXX: implement SMRAM.D_LOCK */
>      pci_default_write_config(dev, address, val, len);
> -    if ((address >= 0x59 && address <= 0x5f) || address == 0x72)
> +    if ((address >= I440FX_PAM0 && address <= I440FX_PAM6) ||
> +        address == I440FX_SMRAM)
>          i440fx_update_memory_mappings(d);
>  }
>  
> @@ -196,7 +201,7 @@ static int i440fx_initfn(PCIDevice *dev)
>      pci_config_set_class(d->dev.config, PCI_CLASS_BRIDGE_HOST);
>      d->dev.config[PCI_HEADER_TYPE] = PCI_HEADER_TYPE_NORMAL; // header_type
>  
> -    d->dev.config[0x72] = 0x02; /* SMRAM */
> +    d->dev.config[I440FX_SMRAM] = 0x02; /* SMRAM */

I think you can remove the comment.
>  
>      return 0;
>  }
> -- 
> 1.6.5.4