Re: [Qemu-devel] [PATCH 4/4] i386/acpi-build: build_crs(): fetch BAR from PCI config space directly

[Laszlo Ersek <lersek@redhat.com>]

On 06/07/15 11:23, Michael S. Tsirkin wrote:
> On Sat, Jun 06, 2015 at 01:46:29AM +0200, Laszlo Ersek wrote:
>> OVMF downloads the ACPI linker/loader script from QEMU when the edk2 PCI
>> Bus driver globally signals the firmware that PCI enumeration and resource
>> allocation have completed. At this point QEMU regenerates the ACPI payload
>> in an fw_cfg read callback, and this is when the PXB's _CRS gets
>> populated.
>>
>> Unfortunately, when this happens, the PCI_COMMAND_MEMORY bit is clear in
>> the root bus's command register, *unlike* under SeaBIOS. The consequences
>> unfold as follows:
>>
>> - When build_crs() fetches dev->io_regions[i].addr, it is all-bits-one,
>>   because pci_update_mappings() --> pci_bar_address() calculated it as
>>   PCI_BAR_UNMAPPED, due to the PCI_COMMAND_MEMORY bit being clear.
>>
>> - Consequently, the SHPC MMIO BAR (bar 0) of the bridge is not added to
>>   the _CRS, *despite* having been programmed in PCI config space.
>>
>> - Similarly, the SHPC MMIO BAR of the PXB is not removed from the main
>>   root bus's DWordMemory descriptor.
>>
>> - Guest OSes (Linux and Windows alike) notice the pre-programmed SHPC BAR
>>   within the PXB's config space, and notice that it conflicts with the
>>   main root bus's memory resource descriptors. Linux reports
>>
>>   pci 0000:04:00.0: BAR 0: can't assign mem (size 0x100)
>>   pci 0000:04:00.0: BAR 0: trying firmware assignment [mem
>>                            0x88200000-0x882000ff 64bit]
>>   pci 0000:04:00.0: BAR 0: [mem 0x88200000-0x882000ff 64bit] conflicts
>>                            with PCI Bus 0000:00 [mem
>>                            0x88200000-0xfebfffff]
>>
>>   While Windows Server 2012 R2 reports
>>
>>     https://technet.microsoft.com/en-us/library/cc732199%28v=ws.10%29.aspx
>>
>>     This device cannot find enough free resources that it can use. If you
>>     want to use this device, you will need to disable one of the other
>>     devices on this system. (Code 12)
>>
>> (This issue was apparently encountered earlier, see:
>>
>>   https://lists.nongnu.org/archive/html/qemu-devel/2015-01/msg02983.html
>>
>> and the current hole-punching logic in build_crs() and build_ssdt() is
>> probably supposed to remedy exactly that problem -- however, for OVMF they
>> don't work, because at the end of the PCI enumeration and resource
>> allocation, which cues the ACPI linker/loader client, the command register
>> is clear.)
>>
>> The solution is to fetch the BAR addresses from PCI config space directly,
>> for the purposes of build_crs(), regardless of the PCI command register
>> and any MemoryRegion state.
>>
>> Example MMIO maps for a 2GB guest:
>>
>> SeaBIOS:
>>
>>   main: 0x80000000..0xFC000000 / 0x7C000000
>>   pxb:  0xFC000000..0xFC200000 / 0x00200000
>>   main: 0xFC200000..0xFC213000 / 0x00013000
>>   pxb:  0xFC213000..0xFC213100 / 0x00000100 <- SHPC BAR
>>   main: 0xFC213100..0xFEA00000 / 0x027ECF00
>>   pxb:  0xFEA00000..0xFEC00000 / 0x00200000
>>
>> OVMF, without the fix:
>>
>>   main: 0x80000000..0x88100000 / 0x08100000
>>   pxb:  0x88100000..0x88200000 / 0x00100000
>>   main: 0x88200000..0xFEC00000 / 0x76A00000
>>
>> OVMF, with the fix:
>>
>>   main: 0x80000000..0x88100000 / 0x08100000
>>   pxb:  0x88100000..0x88200000 / 0x00100000
>>   pxb:  0x88200000..0x88200100 / 0x00000100 <- SHPC BAR
>>   main: 0x88200100..0xFEC00000 / 0x769FFF00
>>
>> (Note that under OVMF the PCI enumerator includes requests for
>> prefetchable memory in the nonprefetchable memory pool -- separate windows
>> for nonprefetchable and prefetchable memory are not supported (yet) --
>> that's why there's one fewer pxb range in the corrected OVMF case than in
>> the SeaBIOS case.)
>>
>> Cc: Marcel Apfelbaum <marcel@redhat.com>
>> Cc: Michael S. Tsirkin <mst@redhat.com>
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> 
> This is problematic - disabled BAR values have no meaning according to
> the PCI spec.
> 
> It might be best to add a property to just disable shpc in the bridge so
> no devices reside directly behind the pxb?

If that solves this problem (and I do think it should), then I don't
have anything against the idea. I guess I could look at other property
code for examples. I think I'd have the main problem with implementing
the "policy" part: when exactly do you flip the new property to false,
and where. (Also, I'm not really sure what the SHPC bar is good for, and
if it won't be missed by someone who wanted to use PXB otherwise.)

Thanks
Laszlo

>> ---
>>  hw/i386/acpi-build.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>> index b71e942..60d4f75 100644
>> --- a/hw/i386/acpi-build.c
>> +++ b/hw/i386/acpi-build.c
>> @@ -784,8 +784,8 @@ static Aml *build_crs(PCIHostState *host,
>>          for (i = 0; i < PCI_NUM_REGIONS; i++) {
>>              PCIIORegion *r = &dev->io_regions[i];
>>  
>> -            range_base = r->addr;
>> -            range_limit = r->addr + r->size - 1;
>> +            range_base = pci_bar_address(dev, i, r->type, r->size, false);
>> +            range_limit = range_base + r->size - 1;
>>  
>>              /*
>>               * Work-around for old bioses
>> -- 
>> 1.8.3.1