Re: [Qemu-devel] [PATCH for-2.5] piix: Document coreboot-specific RAM size config register

[Marcel Apfelbaum <marcel.apfelbaum@gmail.com>]

On 08/07/2015 10:15 PM, Eduardo Habkost wrote:
> The existing i440fx initialization code sets a PCI config register that
> isn't documented anywhere in the Intel 440FX datasheet. Register 0x57 is
> DRAMC (DRAM Control) and has nothing to do with the RAM size.
>
> This was implemented in commit ec5f92ce6ac8ec09056be77e03c941be188648fa
> because old coreboot code tried to read registers 0x5a-0x5f,0x56,0x57 to
> get the RAM size from QEMU, but I couldn't find out why coreboot did
> that. I assume it was a mistake, and the original code was supposed to
> be reading the DRB[0-7] registers (offsets 0x60-0x67).
>
> Document that coreboot-specific register offset in a macro and a
> comment, for future reference.
>
> Cc: Ed Swierk <eswierk@skyportsystems.com>
> Cc: Richard Smith <smithbone@gmail.com>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> References to coreboot commits:
> * Original commit adding code reading register offsets
>    0x5a, 0x5b, 0x5c, 0x5d, 0x5e, 0x5f, 0x56, 0x57 to Intel 440bx code in
>    coreboot:
>    cb8eab482ff09ec256456312ef2d6e7710123551
> * Commit adding the same register offsets to QEMU-specific
>    coreboot code:
>    9cf642bad3fdd2205ffdd83a3222a39855b1ceff
> * coreboot commit replacing the weird register offsets with
>    code that actually reads the DRB7 register, in the I440BX code:
>    1a9c892d58c746aef0cb530481c214e63a6a6871
> * coreboot commit replacing the weird register offets with
>    code reading the CMOS in QEMU-specific code:
>    7339f36961917814ed12d5a4e6f1fe19418b8aca
> ---
>   hw/pci-host/piix.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> index ad55f99..1cb25f3 100644
> --- a/hw/pci-host/piix.c
> +++ b/hw/pci-host/piix.c
> @@ -117,6 +117,11 @@ struct PCII440FXState {
>   #define I440FX_PAM_SIZE 7
>   #define I440FX_SMRAM    0x72
>
> +/* Older coreboot versions (4.0 and older) read a config register that doesn't
> + * exist in real hardware, to get the RAM size from QEMU.
> + */
> +#define I440FX_COREBOOT_RAM_SIZE 0x57
> +
>   static void piix3_set_irq(void *opaque, int pirq, int level);
>   static PCIINTxRoute piix3_route_intx_pin_to_irq(void *opaque, int pci_intx);
>   static void piix3_write_config_xen(PCIDevice *dev,
> @@ -394,7 +399,7 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
>       if (ram_size > 255) {
>           ram_size = 255;
>       }
> -    d->config[0x57] = ram_size;
> +    d->config[I440FX_COREBOOT_RAM_SIZE] = ram_size;
Thanks! Another magic number has now an actual meaning.

Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>


Thanks,
Marcel

>
>       i440fx_update_memory_mappings(f);
>
>