Re: [PATCH] acpi/gpex: Inform os to keep firmware resource map

[Laszlo Ersek]

On 12/17/20 14:52, Jiahui Cen wrote:
> +Laszlo
> 
> On 2020/12/17 21:29, Jiahui Cen wrote:
>> There may be some differences in pci resource assignment between guest os
>> and firmware.
>>
>> Eg. A Bridge with Bus [d2]
>>     -+-[0000:d2]---01.0-[d3]----01.0
>>
>>     where [d2:01.00] is a pcie-pci-bridge with BAR0 (mem, 64-bit, non-pref) 
>> [size=256]
>>           [d3:01.00] is a PCI Device with BAR0 (mem, 64-bit, pref) 
>> [size=128K]
>>                                           BAR4 (mem, 64-bit, pref) [size=64M]
>>
>>     In EDK2, the Resource Map would be:
>>         PciBus: Resource Map for Bridge [D2|01|00]
>>         Type = PMem64; Base = 0x8004000000;     Length = 0x4100000;     
>> Alignment = 0x3FFFFFF
>>            Base = 0x8004000000; Length = 0x4000000;     Alignment = 
>> 0x3FFFFFF;  Owner = PCI [D3|01|00:20]
>>            Base = 0x8008000000; Length = 0x20000;       Alignment = 0x1FFFF; 
>>    Owner = PCI [D3|01|00:10]
>>         Type =  Mem64; Base = 0x8008100000;     Length = 0x100; Alignment = 
>> 0xFFF
>>
>>     While in Linux, kernel will use 0x2FFFFFF as the alignment to calculate
>>     the PMem64 size, which would be 0x6000000.
>>
>> The diffences could result in resource assignment failure.
>>
>> Using _DSM #5 method to inform guest os not to ignore the PCI configuration
>> that firmware has done at boot time could handle the differences.
>>
>> Signed-off-by: Jiahui Cen <cenjiahui@huawei.com>
>> ---
>>  hw/pci-host/gpex-acpi.c | 11 ++++++++++-
>>  1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/pci-host/gpex-acpi.c b/hw/pci-host/gpex-acpi.c
>> index 071aa11b5c..2b490f3379 100644
>> --- a/hw/pci-host/gpex-acpi.c
>> +++ b/hw/pci-host/gpex-acpi.c
>> @@ -112,10 +112,19 @@ static void acpi_dsdt_add_pci_osc(Aml *dev)
>>      UUID = aml_touuid("E5C937D0-3553-4D7A-9117-EA4D19C3434D");
>>      ifctx = aml_if(aml_equal(aml_arg(0), UUID));
>>      ifctx1 = aml_if(aml_equal(aml_arg(2), aml_int(0)));
>> -    uint8_t byte_list[1] = {1};
>> +    uint8_t byte_list[1] = {0x21};
>>      buf = aml_buffer(1, byte_list);
>>      aml_append(ifctx1, aml_return(buf));
>>      aml_append(ifctx, ifctx1);
>> +
>> +    /* PCI Firmware Specification 3.2
>> +     * 4.6.5. _DSM for Ignoring PCI Boot Configurations
>> +     * The UUID in _DSM in this context is
>> +     * {E5C937D0-3553-4D7A-9117-EA4D19C3434D}
>> +     */
>> +    ifctx1 = aml_if(aml_equal(aml_arg(2), aml_int(5)));
>> +    aml_append(ifctx1, aml_return(aml_int(0)));
>> +    aml_append(ifctx, ifctx1);
>>      aml_append(method, ifctx);
>>  
>>      byte_list[0] = 0;
>>
> 

Seems to make sense to me (I didn't realize we already had the _DSM
method with this GUID!), but now I'm not sure what to expect of the
guest kernel, in light of what Ard said. So if it works now, is that by
accident, or is it an intentional, fresh commit in the kernel? Like
a78cf9657ba5 ("PCI/ACPI: Evaluate PCI Boot Configuration _DSM", 2019-06-21)?

Benjamin: can you please tell us something about this Linux commit? What
was the motivation for it?

Hmm.... this commit seems to be a part of the following series:

a78cf9657ba5 PCI/ACPI: Evaluate PCI Boot Configuration _DSM
7ac0d094fbe9 PCI: Don't auto-realloc if we're preserving firmware config
3e8ba9686600 arm64: PCI: Allow resource reallocation if necessary
85dc04136e86 arm64: PCI: Preserve firmware configuration when desired

OK, after reading through the commit messages in those commits (esp.
7ac0d094fbe9), I think the Linux change was made exactly for the purpose
that we want it for -- stick with the firmware assignments.

Ard, does that seem right, or am I misunderstanding the kernel series?

Thanks
Laszlo