All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH for-2.5] piix: Document coreboot-specific RAM size config register
@ 2015-08-07 19:15 Eduardo Habkost
  2015-08-10  1:48 ` Ed Swierk
  2015-08-25  9:06 ` Marcel Apfelbaum
  0 siblings, 2 replies; 6+ messages in thread
From: Eduardo Habkost @ 2015-08-07 19:15 UTC (permalink / raw)
  To: Michael S. Tsirkin; +Cc: Ed Swierk, qemu-devel, Richard Smith

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

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [Qemu-devel] [PATCH for-2.5] piix: Document coreboot-specific RAM size config register
  2015-08-07 19:15 [Qemu-devel] [PATCH for-2.5] piix: Document coreboot-specific RAM size config register Eduardo Habkost
@ 2015-08-10  1:48 ` Ed Swierk
  2015-08-13 15:30   ` Richard Smith
  2015-08-25  9:06 ` Marcel Apfelbaum
  1 sibling, 1 reply; 6+ messages in thread
From: Ed Swierk @ 2015-08-10  1:48 UTC (permalink / raw)
  To: Eduardo Habkost; +Cc: Richard Smith, qemu-devel, Michael S. Tsirkin

[-- Attachment #1: Type: text/plain, Size: 2717 bytes --]

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
>
>

[-- Attachment #2: Type: text/html, Size: 3492 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Qemu-devel] [PATCH for-2.5] piix: Document coreboot-specific RAM size config register
  2015-08-10  1:48 ` Ed Swierk
@ 2015-08-13 15:30   ` Richard Smith
  2015-08-17 18:58     ` Eduardo Habkost
  0 siblings, 1 reply; 6+ messages in thread
From: Richard Smith @ 2015-08-13 15:30 UTC (permalink / raw)
  To: Ed Swierk, Eduardo Habkost; +Cc: qemu-devel, Michael S. Tsirkin

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.

-- 
Richard A. Smith

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Qemu-devel] [PATCH for-2.5] piix: Document coreboot-specific RAM size config register
  2015-08-13 15:30   ` Richard Smith
@ 2015-08-17 18:58     ` Eduardo Habkost
  2015-08-25  9:52       ` Thomas Lamprecht
  0 siblings, 1 reply; 6+ messages in thread
From: Eduardo Habkost @ 2015-08-17 18:58 UTC (permalink / raw)
  To: Richard Smith; +Cc: Ed Swierk, qemu-devel, Michael S. Tsirkin

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)

-- 
Eduardo

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Qemu-devel] [PATCH for-2.5] piix: Document coreboot-specific RAM size config register
  2015-08-07 19:15 [Qemu-devel] [PATCH for-2.5] piix: Document coreboot-specific RAM size config register Eduardo Habkost
  2015-08-10  1:48 ` Ed Swierk
@ 2015-08-25  9:06 ` Marcel Apfelbaum
  1 sibling, 0 replies; 6+ messages in thread
From: Marcel Apfelbaum @ 2015-08-25  9:06 UTC (permalink / raw)
  To: Eduardo Habkost, Michael S. Tsirkin; +Cc: Ed Swierk, qemu-devel, Richard Smith

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);
>
>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Qemu-devel] [PATCH for-2.5] piix: Document coreboot-specific RAM size config register
  2015-08-17 18:58     ` Eduardo Habkost
@ 2015-08-25  9:52       ` Thomas Lamprecht
  0 siblings, 0 replies; 6+ messages in thread
From: Thomas Lamprecht @ 2015-08-25  9:52 UTC (permalink / raw)
  To: qemu-devel



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

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2015-08-25 10:00 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-08-07 19:15 [Qemu-devel] [PATCH for-2.5] piix: Document coreboot-specific RAM size config register Eduardo Habkost
2015-08-10  1:48 ` Ed Swierk
2015-08-13 15:30   ` Richard Smith
2015-08-17 18:58     ` Eduardo Habkost
2015-08-25  9:52       ` Thomas Lamprecht
2015-08-25  9:06 ` Marcel Apfelbaum

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.