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

[Kevin O'Connor]

On Sun, Mar 19, 2017 at 01:50:47PM +0100, [e-mail] 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