Re: [Qemu-devel] [SeaBIOS] [PATCH] acpi: hide 64-bit PCI hole for Windows XP

[Gerd Hoffmann]

On 08/08/13 16:13, Michael S. Tsirkin wrote:
> On Thu, Aug 08, 2013 at 12:21:32PM +0200, Gerd Hoffmann wrote:
>> On 08/08/13 11:52, Michael S. Tsirkin wrote:
>>> On Thu, Aug 08, 2013 at 10:57:44AM +0200, Gerd Hoffmann wrote:
>>>> On 08/08/13 10:37, Michael S. Tsirkin wrote:
>>>>> On Thu, Aug 08, 2013 at 09:57:39AM +0200, Gerd Hoffmann wrote:
>>>>>> Also coreboot and seabios use different values for pmbase.  coreboot on
>>>>>> q35 maps the pmbase below 0x1000.  Which surely makes sense.  When we
>>>>>> don't place chipset stuff at 0xb000 we can assign the 0xb000->0xbfff
>>>>>> window to a pci bridge instead.
> 
> Re-reading this - if this has value, can't we generalize it
> and make all firmware behave the same, getting values from QEMU?

pmbase is a compile-time constant (aka #define) in both seabios and
coreboot, and making this runtime-configurable is non-trivial.  See
src/smm.c in seabios for one reason why.

Moving it to another fixed address below 0x1000 doesn't work, breaks
with old qemu+new seabios and other way around.

I think easiest is to allow firmware pick it and fixup the tables
provided by qemu accordingly.

Picking pmbase works in firmware today btw as both coreboot+seabios
generate a fadt with the correct values.  Only nit is that this depends
on qemu 1.4+ as piix4 pmbase register was read-only in older qemu
versions.  So everybody uses 0xb000 by default for bug compatibility
with older qemu versions.

>> Yes, the address ranges used for pci devices (aka 32bit + 64bit pci
>> window) need to be there.  Well, placing in SSDT, then referencing from
>> DSDT works too, and this is what seabios does today to dynamically
>> adjust stuff.  Fixing up the SSDT using the linker is probably easier as
>> we generate it anyway.
>>
>> cheers,
>>   Gerd
> 
> Yes but as I said, this makes things messy, since AML encoding for
> numbers isn't fixed width.

In seabios we have fixed 32bit / 64bit width today, from acpi.c:

    // store pci io windows
    *(u32*)&ssdt_ptr[acpi_pci32_start[0]] = cpu_to_le32(pcimem_start);
    *(u32*)&ssdt_ptr[acpi_pci32_end[0]] = cpu_to_le32(pcimem_end - 1);
    if (pcimem64_start) {
        ssdt_ptr[acpi_pci64_valid[0]] = 1;
        *(u64*)&ssdt_ptr[acpi_pci64_start[0]] = cpu_to_le64(pcimem64_start);
        *(u64*)&ssdt_ptr[acpi_pci64_end[0]] = cpu_to_le64(pcimem64_end - 1);
        *(u64*)&ssdt_ptr[acpi_pci64_length[0]] = cpu_to_le64(
            pcimem64_end - pcimem64_start);
    } else {
        ssdt_ptr[acpi_pci64_valid[0]] = 0;
    }

Storing fixup instructions for these fields in the linker script
shouldn't be hard I think.

cheers,
  Gerd