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;
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:
- 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.
- 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
"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:
- 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.
- 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. " Kevin, thanks a lot for explaining all of these to me - it was a big help. At the end this was a problem in Bochs that was recently fixed. " Thanks, -Kevin "
-
Kevin O'Connor -
petr.berky＠email.cz