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

[Laszlo Ersek]

On 08/09/13 17:49, Michael S. Tsirkin wrote:
> On Fri, Aug 09, 2013 at 12:13:06AM -0400, Kevin O'Connor wrote:
>> On Thu, Aug 08, 2013 at 04:56:55PM +0200, Gerd Hoffmann wrote:
>>> 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.
>>
>> I don't think SeaBIOS should modify tables provided by QEMU.  That
>> leads to confusion on the source of the data and mixed
>> responsibilities which results in greater complexity in both QEMU and
>> SeaBIOS.
>>
>> SeaBIOS doesn't have any info that QEMU doesn't have.  So, I think
>> it's safe for QEMU to be the sole authority for the table content.
>>
>> Converting src/smm.c to use a runtime value isn't hard - just change
>> the assembler from: "mov $" __stringify(PORT_ACPI_PM_BASE) " + 0x04,
>> %dx\n" to: "mov 4(my_acpi_base), %dx\n" and make sure to define the
>> global variable my_acpi_base as VARFSEG.
>>
>>>>> 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.
>>>>>
>>>> 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.
>>
>> I don't think SeaBIOS should continue to do the above once the tables
>> are moved to QEMU.  QEMU has all the info SeaBIOS has, so it can
>> generate the tables correctly on its own.
>>
>> In all practical situations, the PCI window should be at least an
>> order of magnatude greater than the sum of the PCI bars in the system.
>> If the bars are actually bigger than the window, then things are going
>> to fail - the best the firmware can do is try to fail gracefully.  I
>> don't think it's worth the complexity to design mixed ownership and
>> advanced interfaces just so we can fail slightly better.
>>
>> If this is a real worry, QEMU can sum all the PCI bars and warn the
>> user if they don't fit.
>>
>> -Kevin
> 
> If we make it a rule that PCI is`setup before ACPI tables
> are read, then QEMU can do the patching itself when
> it detects BIOS reading the tables.
> 
> Gerd, Laszlo,others,  does this rule work for alternative firmwares?

OVMF currently determines the boundaries of the PCI window (32-bit) that
it is going to advertise in the _CRS method by scanning the "map of the
memory resources in the global coherency domain of the processor".

It finds the top of the system memory (under 4GB) and the smallest
interval that covers all of the memory mapped IO ranges. The subset of
the latter interval that is above the top of the system memory is then
advertised.

See PopulateFwData() in "OvmfPkg/AcpiPlatformDxe/Qemu.c", and
GetMemorySpaceMap() in "VOLUME 2: Platform Initialization Specification
/ Driver Execution Environment Core Interface".

... With this I'm trying to say that for now OVMF keys the 32-bit window
it advertises off regions that are "currently being decoded by a
component as memory-mapped I/O that can be used to access I/O devices in
the platform".

This seems to imply that PCI setup precedes ACPI table installation.

Laszlo