| Message ID | 1438974931-21128-1-git-send-email-ehabkost@redhat.com |
|---|---|
| State | New |
| Headers | show |
That original coreboot code certainly looks like a mistake. Thanks for helping close the decade-long loop. On Fri, Aug 7, 2015 at 12:15 PM, Eduardo Habkost <ehabkost@redhat.com> 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; > > i440fx_update_memory_mappings(f); > > -- > 2.1.0 > >
On 08/09/2015 09:48 PM, Ed Swierk wrote: > 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 I have vague recollection I may have been responsible for this but it was so long ago. I'm having trouble finding the commits in gitweb. When I put those hashes into the commit search at review.coreboot.org I get not found.
On Thu, Aug 13, 2015 at 11:30:57AM -0400, Richard Smith wrote: > On 08/09/2015 09:48 PM, Ed Swierk wrote: > > > >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 > > I have vague recollection I may have been responsible for this but it > was so long ago. I'm having trouble finding the commits in gitweb. > When I put those hashes into the commit search at > review.coreboot.org I get not found. Those are git commits from the repository at http://review.coreboot.org/coreboot.git (I couldn't check if they can be seen in a browser, right now, because the server is returning HTTP 502 errors)
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); > >
On 08/17/2015 08:58 PM, Eduardo Habkost wrote: > On Thu, Aug 13, 2015 at 11:30:57AM -0400, Richard Smith wrote: >> On 08/09/2015 09:48 PM, Ed Swierk wrote: >> >> >>> 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 >> I have vague recollection I may have been responsible for this but it >> was so long ago. I'm having trouble finding the commits in gitweb. >> When I put those hashes into the commit search at >> review.coreboot.org I get not found. > Those are git commits from the repository at > http://review.coreboot.org/coreboot.git > > (I couldn't check if they can be seen in a browser, right now, because > the server is returning HTTP 502 errors) > Server doesn't work for me neither, but here is the commit on the github repo: https://github.com/coreboot/coreboot/commit/cb8eab482ff09ec256456312ef2d6e7710123551
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; i440fx_update_memory_mappings(f);
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(-)