1. Patchew
  2. Seabios
  3. View series

[SeaBIOS] [Bochs] [PATCH RFC] shadow: Change order of PCI writes

petr.berky@email.cz posted 1 patch 7 years, 1 month ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/seabios tags/patchew/2GM.4larv.5I6lv0KJ{Gh.1Opdwd@seznam.cz
src/fw/shadow.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
[SeaBIOS] [Bochs] [PATCH RFC] shadow: Change order of PCI writes
Posted by petr.berky@email.cz 7 years, 1 month ago 
I am working on an issue with SeaBIOS running in Bochs emulator. I found
I need to revert SeaBIOS patch d449a11, otherwise Bochs freezes. After
the further investigation, I narrowed down the problem to those lines:

File: src/fw/shadow.c
Function: __make_bios_writable_intel
 48  // Write PAM settings back to pci config space
 49  pci_config_writel(bdf, ALIGN_DOWN(pam0, 4), pamdata.data32[0]);
 50  pci_config_writel(bdf, ALIGN_DOWN(pam0, 4) + 4, pamdata.data32[1]);

File: src/fw/shadow.c
Function: make_bios_readonly_intel
109  // Write PAM settings back to pci config space
110  pci_config_writel(bdf, ALIGN_DOWN(pam0, 4), pamdata.data32[0]);
111  pci_config_writel(bdf, ALIGN_DOWN(pam0, 4) + 4, pamdata.data32[1]);

What has changed, comparing to the code before patch d449a11 is the
order of writes to PAM registers.

Before d449a11 the order was: 0x5a 0x5b 0x5c 0x5d 0x5e 0x5f 0x59
After  d449a11 the order is : 0x59 0x5a 0x5b 0x5c 0x5d 0x5e 0x5f

I believe also the 0x58 (DRAMT?) register is written after the patch
d449a11 and was not before.

I checked, and the registers values after write are exactly the same
in both cases (before/after patch). This lead me to think the order
of writes matters.

So I prepared the patch that restores the old order. I works well and
fixes the issue with Bochs. But there is still one think I cannot
explain and I hope I can find some help here. If you look on the patch
below, you will see I had to use different pci write functions in
__make_bios_writable_intel and make_bios_readonly_intel. The only
difference in those functions (I think) is the size of data that
is written at one call. Still It does not work for me when written
some other way. Again, I would appreciate if someone could explain
it to me or point me some documentation.

Just to summarize, I have two questions:

  1.  Why the order of pci writes makes a difference here?
  2.  Why the size of pci writes matters?


From 953c133a6ade49e9148a0734ffefc0fb27c116fb Mon Sep 17 00:00:00 2001
From: Petr Berky <petr.berky@email.cz>
Date: Sun, 19 Mar 2017 10:14:49 +0100
Subject: [PATCH RFC] shadow: Change order of PCI writes

This change solves the problem with Bochs freeze
---
 src/fw/shadow.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/src/fw/shadow.c b/src/fw/shadow.c
index c80b266..508af20 100644
--- a/src/fw/shadow.c
+++ b/src/fw/shadow.c
@@ -23,6 +23,7 @@
 
 union pamdata_u {
     u8 data8[8];
+    u16 data16[4];
     u32 data32[2];
 };
 
@@ -46,8 +47,9 @@ __make_bios_writable_intel(u16 bdf, u32 pam0)
     pam[0] = 0x30;
 
     // Write PAM settings back to pci config space
-    pci_config_writel(bdf, ALIGN_DOWN(pam0, 4), pamdata.data32[0]);
+    pci_config_writew(bdf, ALIGN_DOWN(pam0, 4) + 2, pamdata.data16[1]);
     pci_config_writel(bdf, ALIGN_DOWN(pam0, 4) + 4, pamdata.data32[1]);
+    pci_config_writeb(bdf, ALIGN_DOWN(pam0, 4) + 1, pamdata.data8[1]);
 
     if (!ram_present)
         // Copy bios.
@@ -107,8 +109,11 @@ make_bios_readonly_intel(u16 bdf, u32 pam0)
     pam[0] = 0x10;
 
     // Write PAM settings back to pci config space
-    pci_config_writel(bdf, ALIGN_DOWN(pam0, 4), pamdata.data32[0]);
-    pci_config_writel(bdf, ALIGN_DOWN(pam0, 4) + 4, pamdata.data32[1]);
+    pci_config_writew(bdf, ALIGN_DOWN(pam0, 4) + 2, pamdata.data16[1]);
+    pci_config_writew(bdf, ALIGN_DOWN(pam0, 4) + 4, pamdata.data16[2]);
+    pci_config_writeb(bdf, ALIGN_DOWN(pam0, 4) + 6, pamdata.data8[6]);
+    pci_config_writeb(bdf, ALIGN_DOWN(pam0, 4) + 7, pamdata.data8[7]);
+    pci_config_writeb(bdf, ALIGN_DOWN(pam0, 4) + 1, pamdata.data8[1]);
 }
 
 static int ShadowBDF = -1;
-- 
2.11.0


_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://www.coreboot.org/mailman/listinfo/seabios
Re: [SeaBIOS] [Bochs] [PATCH RFC] shadow: Change order of PCI writes
Posted by Kevin O'Connor 7 years, 1 month ago 
On Sun, Mar 19, 2017 at 01:50:47PM +0100, petr.berky@email.cz wrote:
> I am working on an issue with SeaBIOS running in Bochs emulator. I found
> I need to revert SeaBIOS patch d449a11, otherwise Bochs freezes. After
> the further investigation, I narrowed down the problem to those lines:
> 
> File: src/fw/shadow.c
> Function: __make_bios_writable_intel
>  48  // Write PAM settings back to pci config space
>  49  pci_config_writel(bdf, ALIGN_DOWN(pam0, 4), pamdata.data32[0]);
>  50  pci_config_writel(bdf, ALIGN_DOWN(pam0, 4) + 4, pamdata.data32[1]);
> 
> File: src/fw/shadow.c
> Function: make_bios_readonly_intel
> 109  // Write PAM settings back to pci config space
> 110  pci_config_writel(bdf, ALIGN_DOWN(pam0, 4), pamdata.data32[0]);
> 111  pci_config_writel(bdf, ALIGN_DOWN(pam0, 4) + 4, pamdata.data32[1]);
> 
> What has changed, comparing to the code before patch d449a11 is the
> order of writes to PAM registers.
> 
> Before d449a11 the order was: 0x5a 0x5b 0x5c 0x5d 0x5e 0x5f 0x59
> After  d449a11 the order is : 0x59 0x5a 0x5b 0x5c 0x5d 0x5e 0x5f
> 
> I believe also the 0x58 (DRAMT?) register is written after the patch
> d449a11 and was not before.
> 
> I checked, and the registers values after write are exactly the same
> in both cases (before/after patch). This lead me to think the order
> of writes matters.
> 
> So I prepared the patch that restores the old order. I works well and
> fixes the issue with Bochs. But there is still one think I cannot
> explain and I hope I can find some help here. If you look on the patch
> below, you will see I had to use different pci write functions in
> __make_bios_writable_intel and make_bios_readonly_intel. The only
> difference in those functions (I think) is the size of data that
> is written at one call. Still It does not work for me when written
> some other way. Again, I would appreciate if someone could explain
> it to me or point me some documentation.

As background, the reason for d449a11 is to improve the startup time
on QEMU.  When the updates to the PAM registers were written one byte
at a time it caused significantly more time for QEMU to make the
memory changes.

> Just to summarize, I have two questions:
> 
>   1.  Why the order of pci writes makes a difference here?

To the best of my knowledge, it shouldn't.  It doesn't on QEMU.  It
might be a Bochs bug.  Also, it may not be the ordering that is
causing the problem - Bochs may not like the 4 byte writes.

>   2.  Why the size of pci writes matters?

Again, to the best of my knowledge, it shouldn't.

Can you check if the problem is really due to ordering of writes:

    for (i=0; i<8; i++)
        pci_config_writeb(bdf, ALIGN_DOWN(pam0, 4) + i, pamdata.data8[i]);

or just due to the write to 0x58:

    for (i=0; i<7; i++)
        pci_config_writeb(bdf, pam0 + i, pamdata.data8[(pam0 % 4) + i]);

or because 4 byte pci config writes are done?

    for (i=0; i<4; i++)
        pci_config_writew(bdf, ALIGN_DOWN(pam0, 4) + i*2, pamdata.data16[i]);

I suspect the fix will require detecting that we aren't running on
QEMU and writing out the data in single bytes.  That way QEMU still
gets the startup speed improvement and the code still runs on Bochs.

Thanks,
-Kevin

_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://www.coreboot.org/mailman/listinfo/seabios

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.