qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] acpi/gpex: Inform os to keep firmware resource map
@ 2020-12-17 13:29 Jiahui Cen
  2020-12-17 13:52 ` Jiahui Cen
                   ` (3 more replies)
  0 siblings, 4 replies; 11+ messages in thread
From: Jiahui Cen @ 2020-12-17 13:29 UTC (permalink / raw)
  To: qemu-devel
  Cc: xieyingtai, Igor Mammedov, Jiahui Cen, Gerd Hoffmann, Michael S. Tsirkin

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;
-- 
2.28.0



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] acpi/gpex: Inform os to keep firmware resource map
  2020-12-17 13:29 [PATCH] acpi/gpex: Inform os to keep firmware resource map Jiahui Cen
@ 2020-12-17 13:52 ` Jiahui Cen
  2020-12-17 17:23   ` Laszlo Ersek
  2020-12-17 18:29 ` Michael S. Tsirkin
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 11+ messages in thread
From: Jiahui Cen @ 2020-12-17 13:52 UTC (permalink / raw)
  To: qemu-devel
  Cc: xieyingtai, Igor Mammedov, Laszlo Ersek, Gerd Hoffmann,
	Michael S. Tsirkin

+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;
> 


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] acpi/gpex: Inform os to keep firmware resource map
  2020-12-17 13:52 ` Jiahui Cen
@ 2020-12-17 17:23   ` Laszlo Ersek
  2020-12-17 19:24     ` Ard Biesheuvel
  0 siblings, 1 reply; 11+ messages in thread
From: Laszlo Ersek @ 2020-12-17 17:23 UTC (permalink / raw)
  To: Jiahui Cen, qemu-devel
  Cc: xieyingtai, Michael S. Tsirkin, Gerd Hoffmann,
	Ard Biesheuvel (ARM address),
	Igor Mammedov

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



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] acpi/gpex: Inform os to keep firmware resource map
  2020-12-17 13:29 [PATCH] acpi/gpex: Inform os to keep firmware resource map Jiahui Cen
  2020-12-17 13:52 ` Jiahui Cen
@ 2020-12-17 18:29 ` Michael S. Tsirkin
  2020-12-18  5:56   ` Jiahui Cen
  2020-12-17 20:04 ` Michael S. Tsirkin
  2021-07-22  5:22 ` Guenter Roeck
  3 siblings, 1 reply; 11+ messages in thread
From: Michael S. Tsirkin @ 2020-12-17 18:29 UTC (permalink / raw)
  To: Jiahui Cen; +Cc: xieyingtai, Igor Mammedov, qemu-devel, Gerd Hoffmann

On Thu, Dec 17, 2020 at 09:29:26PM +0800, 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.

A bit more data here please. When does this result in a 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;
> -- 
> 2.28.0



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] acpi/gpex: Inform os to keep firmware resource map
  2020-12-17 17:23   ` Laszlo Ersek
@ 2020-12-17 19:24     ` Ard Biesheuvel
  0 siblings, 0 replies; 11+ messages in thread
From: Ard Biesheuvel @ 2020-12-17 19:24 UTC (permalink / raw)
  To: Laszlo Ersek, Jiahui Cen, qemu-devel
  Cc: xieyingtai, Igor Mammedov, Gerd Hoffmann, Michael S. Tsirkin

On 12/17/20 6:23 PM, Laszlo Ersek wrote:
> 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?
> 

Hmm, I had no recollection of those changes going in, but I was clearly
aware at the time, given my acks.

So we clearly do support DSM #5 to prevent resource reallocation, and it
makes sense to use it in the proposed way.



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] acpi/gpex: Inform os to keep firmware resource map
  2020-12-17 13:29 [PATCH] acpi/gpex: Inform os to keep firmware resource map Jiahui Cen
  2020-12-17 13:52 ` Jiahui Cen
  2020-12-17 18:29 ` Michael S. Tsirkin
@ 2020-12-17 20:04 ` Michael S. Tsirkin
  2020-12-18  5:56   ` Jiahui Cen
  2021-07-22  5:22 ` Guenter Roeck
  3 siblings, 1 reply; 11+ messages in thread
From: Michael S. Tsirkin @ 2020-12-17 20:04 UTC (permalink / raw)
  To: Jiahui Cen; +Cc: xieyingtai, Igor Mammedov, qemu-devel, Gerd Hoffmann

On Thu, Dec 17, 2020 at 09:29:26PM +0800, 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);


Hmm what is this change for?

I also noticed something weird.
Spec seems to say for _DSM for PCI Express Slot Information:


Arguments:
Arg0: UUID: E5C937D0-3553-4d7a-9117-EA4D19C3434D
Arg1: Revision ID: 2
Arg2: Function Index: 1
Arg3: Empty Package


how come we are comparing function index to 0 here?



Also, as long as we are changing this probably shouldn't hard-code
1 as array size ...


>      aml_append(ifctx1, aml_return(buf));
>      aml_append(ifctx, ifctx1);
> +
> +    /* PCI Firmware Specification 3.2
> +     * 4.6.5. _DSM for Ignoring PCI Boot Configurations

Note you must always quote the most recent spec that
your change refers to. This is so people can figure out
legacy guest compatibility.

In this case I think this first appeard in 3.1 not 3.2

> +     * The UUID in _DSM in this context is
> +     * {E5C937D0-3553-4D7A-9117-EA4D19C3434D}

This is just five lines earier, I don't think we need it here.

> +     */
> +    ifctx1 = aml_if(aml_equal(aml_arg(2), aml_int(5)));

add comment:
	/* Arg2: Function Index: 5 */

> +    aml_append(ifctx1, aml_return(aml_int(0)));


add comment: /* 0 - do not ignore ... (quote spec I don't have it to hand) */




> +    aml_append(ifctx, ifctx1);
>      aml_append(method, ifctx);
>  
>      byte_list[0] = 0;
> -- 
> 2.28.0



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] acpi/gpex: Inform os to keep firmware resource map
  2020-12-17 18:29 ` Michael S. Tsirkin
@ 2020-12-18  5:56   ` Jiahui Cen
  0 siblings, 0 replies; 11+ messages in thread
From: Jiahui Cen @ 2020-12-18  5:56 UTC (permalink / raw)
  To: Michael S. Tsirkin; +Cc: xieyingtai, Igor Mammedov, qemu-devel, Gerd Hoffmann

Hi Michael,

On 2020/12/18 2:29, Michael S. Tsirkin wrote:
> On Thu, Dec 17, 2020 at 09:29:26PM +0800, 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.

Sorry, I made a mistake. Kernel will use 0x1FFFFFF (the half of 0x3FFFFFF)
not 0x2FFFFFF as alignment, so the size 0x4100000 would be aligned
to 0x6000000.

>>
>> The diffences could result in resource assignment failure.
> 
> A bit more data here please. When does this result in a failure?
> 

EDK2 would use 0x4100000 to calculate the root bus's PMem64 resource
window, while Kernel would try to allocate 0x6000000 from the PMem64
resource window. Since the resource window is not large enough,
the allocation would fail.

Is this clear enough? I would add this into my next patch.

Thanks,
Jiahui

>> 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;
>> -- 
>> 2.28.0
> 
> .
> 


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] acpi/gpex: Inform os to keep firmware resource map
  2020-12-17 20:04 ` Michael S. Tsirkin
@ 2020-12-18  5:56   ` Jiahui Cen
  2020-12-19 19:06     ` Michael S. Tsirkin
  0 siblings, 1 reply; 11+ messages in thread
From: Jiahui Cen @ 2020-12-18  5:56 UTC (permalink / raw)
  To: Michael S. Tsirkin; +Cc: xieyingtai, Igor Mammedov, qemu-devel, Gerd Hoffmann

Hi Michael,

On 2020/12/18 4:04, Michael S. Tsirkin wrote:
> On Thu, Dec 17, 2020 at 09:29:26PM +0800, 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);
> 
> 
> Hmm what is this change for?
> 
> I also noticed something weird.
> Spec seems to say for _DSM for PCI Express Slot Information:
> 
> 
> Arguments:
> Arg0: UUID: E5C937D0-3553-4d7a-9117-EA4D19C3434D
> Arg1: Revision ID: 2
> Arg2: Function Index: 1
> Arg3: Empty Package
> 
> 
> how come we are comparing function index to 0 here?
> 

PCI Firmware Spec says in 4.6.1. _DSM for PCI Express Slot Information

Note: Function 0 is a generic Query function that is supported by _DSMs with any UUID and
Revision ID. The definition of function 0 is generic to _DSM and specified in the ACPI Specification,
Version 3.0 (or later).


And ACPI Spec says in 9.1.1 _DSM (Device Specific Method)

Return Value Information:
If Function Index is zero, the return is a buffer containing one bit for each function index, starting with zero. Bit 0
indicates whether there is support for any functions other than function 0 for the specified UUID and Revision ID.
If set to zero, no functions are supported (other than function zero) for the specified UUID and Revision ID. If set
to one, at least one additional function is supported. For all other bits in the buffer, a bit is set to zero to indicate if
that function index is not supported for the specific UUID and Revision ID. (For example, bit 1 set to 0 indicates that
function index 1 is not supported for the specific UUID and Revision ID.)


I have no idea whether the original code does aim to use _DSM #0
by setting function index 0 (The return value seems not to be suitable
with _DSM #1). But if it does, I think it is necessary to set bit 5
in return value to indicate _DSM #5 function is supported.

> 
> Also, as long as we are changing this probably shouldn't hard-code
> 1 as array size ...
> 

Is a macro enough? Like #define RET_BUF_SIZE 2

> 
>>      aml_append(ifctx1, aml_return(buf));
>>      aml_append(ifctx, ifctx1);
>> +
>> +    /* PCI Firmware Specification 3.2
>> +     * 4.6.5. _DSM for Ignoring PCI Boot Configurations
> 
> Note you must always quote the most recent spec that
> your change refers to. This is so people can figure out
> legacy guest compatibility.
> 
> In this case I think this first appeard in 3.1 not 3.2
> 

OK, I'll fix this.

>> +     * The UUID in _DSM in this context is
>> +     * {E5C937D0-3553-4D7A-9117-EA4D19C3434D}
> 
> This is just five lines earier, I don't think we need it here.
> 

Will remove.

>> +     */
>> +    ifctx1 = aml_if(aml_equal(aml_arg(2), aml_int(5)));
> 
> add comment:
> 	/* Arg2: Function Index: 5 */

Will add.

> 
>> +    aml_append(ifctx1, aml_return(aml_int(0)));
> 
> 
> add comment: /* 0 - do not ignore ... (quote spec I don't have it to hand) */
> 

Will add.

Thanks,
Jiahui

> 
> 
> 
>> +    aml_append(ifctx, ifctx1);
>>      aml_append(method, ifctx);
>>  
>>      byte_list[0] = 0;
>> -- 
>> 2.28.0
> 
> .
> 


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] acpi/gpex: Inform os to keep firmware resource map
  2020-12-18  5:56   ` Jiahui Cen
@ 2020-12-19 19:06     ` Michael S. Tsirkin
  2020-12-21  1:12       ` Jiahui Cen
  0 siblings, 1 reply; 11+ messages in thread
From: Michael S. Tsirkin @ 2020-12-19 19:06 UTC (permalink / raw)
  To: Jiahui Cen; +Cc: xieyingtai, Igor Mammedov, qemu-devel, Gerd Hoffmann

On Fri, Dec 18, 2020 at 01:56:29PM +0800, Jiahui Cen wrote:
> Hi Michael,
> 
> On 2020/12/18 4:04, Michael S. Tsirkin wrote:
> > On Thu, Dec 17, 2020 at 09:29:26PM +0800, 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);
> > 
> > 
> > Hmm what is this change for?
> > 
> > I also noticed something weird.
> > Spec seems to say for _DSM for PCI Express Slot Information:
> > 
> > 
> > Arguments:
> > Arg0: UUID: E5C937D0-3553-4d7a-9117-EA4D19C3434D
> > Arg1: Revision ID: 2
> > Arg2: Function Index: 1
> > Arg3: Empty Package
> > 
> > 
> > how come we are comparing function index to 0 here?
> > 
> 
> PCI Firmware Spec says in 4.6.1. _DSM for PCI Express Slot Information
> 
> Note: Function 0 is a generic Query function that is supported by _DSMs with any UUID and
> Revision ID. The definition of function 0 is generic to _DSM and specified in the ACPI Specification,
> Version 3.0 (or later).
> 
> 
> And ACPI Spec says in 9.1.1 _DSM (Device Specific Method)
> 
> Return Value Information:
> If Function Index is zero, the return is a buffer containing one bit for each function index, starting with zero. Bit 0
> indicates whether there is support for any functions other than function 0 for the specified UUID and Revision ID.
> If set to zero, no functions are supported (other than function zero) for the specified UUID and Revision ID. If set
> to one, at least one additional function is supported. For all other bits in the buffer, a bit is set to zero to indicate if
> that function index is not supported for the specific UUID and Revision ID. (For example, bit 1 set to 0 indicates that
> function index 1 is not supported for the specific UUID and Revision ID.)
> 
> 
> I have no idea whether the original code does aim to use _DSM #0
> by setting function index 0 (The return value seems not to be suitable
> with _DSM #1). But if it does, I think it is necessary to set bit 5
> in return value to indicate _DSM #5 function is supported.



So let's make it self documenting:

{
	0x1 << 0 /* support for support for any functions other than function 0 */ |
	0x1 << 5 /* support for function 5 */
}



> > 
> > Also, as long as we are changing this probably shouldn't hard-code
> > 1 as array size ...
> > 
> 
> Is a macro enough? Like #define RET_BUF_SIZE 2

Better to use 

uint8_t byte_list[] = { ... };

And then ARRAY_SIZE(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
> > 
> > Note you must always quote the most recent spec that
> > your change refers to. This is so people can figure out
> > legacy guest compatibility.
> > 
> > In this case I think this first appeard in 3.1 not 3.2
> > 
> 
> OK, I'll fix this.
> 
> >> +     * The UUID in _DSM in this context is
> >> +     * {E5C937D0-3553-4D7A-9117-EA4D19C3434D}
> > 
> > This is just five lines earier, I don't think we need it here.
> > 
> 
> Will remove.
> 
> >> +     */
> >> +    ifctx1 = aml_if(aml_equal(aml_arg(2), aml_int(5)));
> > 
> > add comment:
> > 	/* Arg2: Function Index: 5 */
> 
> Will add.
> 
> > 
> >> +    aml_append(ifctx1, aml_return(aml_int(0)));
> > 
> > 
> > add comment: /* 0 - do not ignore ... (quote spec I don't have it to hand) */
> > 
> 
> Will add.
> 
> Thanks,
> Jiahui
> 
> > 
> > 
> > 
> >> +    aml_append(ifctx, ifctx1);
> >>      aml_append(method, ifctx);
> >>  
> >>      byte_list[0] = 0;
> >> -- 
> >> 2.28.0
> > 
> > .
> > 



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] acpi/gpex: Inform os to keep firmware resource map
  2020-12-19 19:06     ` Michael S. Tsirkin
@ 2020-12-21  1:12       ` Jiahui Cen
  0 siblings, 0 replies; 11+ messages in thread
From: Jiahui Cen @ 2020-12-21  1:12 UTC (permalink / raw)
  To: Michael S. Tsirkin; +Cc: xieyingtai, Igor Mammedov, qemu-devel, Gerd Hoffmann

Hi Michael,

On 2020/12/20 3:06, Michael S. Tsirkin wrote:
> On Fri, Dec 18, 2020 at 01:56:29PM +0800, Jiahui Cen wrote:
>> Hi Michael,
>>
>> On 2020/12/18 4:04, Michael S. Tsirkin wrote:
>>> On Thu, Dec 17, 2020 at 09:29:26PM +0800, 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);
>>>
>>>
>>> Hmm what is this change for?
>>>
>>> I also noticed something weird.
>>> Spec seems to say for _DSM for PCI Express Slot Information:
>>>
>>>
>>> Arguments:
>>> Arg0: UUID: E5C937D0-3553-4d7a-9117-EA4D19C3434D
>>> Arg1: Revision ID: 2
>>> Arg2: Function Index: 1
>>> Arg3: Empty Package
>>>
>>>
>>> how come we are comparing function index to 0 here?
>>>
>>
>> PCI Firmware Spec says in 4.6.1. _DSM for PCI Express Slot Information
>>
>> Note: Function 0 is a generic Query function that is supported by _DSMs with any UUID and
>> Revision ID. The definition of function 0 is generic to _DSM and specified in the ACPI Specification,
>> Version 3.0 (or later).
>>
>>
>> And ACPI Spec says in 9.1.1 _DSM (Device Specific Method)
>>
>> Return Value Information:
>> If Function Index is zero, the return is a buffer containing one bit for each function index, starting with zero. Bit 0
>> indicates whether there is support for any functions other than function 0 for the specified UUID and Revision ID.
>> If set to zero, no functions are supported (other than function zero) for the specified UUID and Revision ID. If set
>> to one, at least one additional function is supported. For all other bits in the buffer, a bit is set to zero to indicate if
>> that function index is not supported for the specific UUID and Revision ID. (For example, bit 1 set to 0 indicates that
>> function index 1 is not supported for the specific UUID and Revision ID.)
>>
>>
>> I have no idea whether the original code does aim to use _DSM #0
>> by setting function index 0 (The return value seems not to be suitable
>> with _DSM #1). But if it does, I think it is necessary to set bit 5
>> in return value to indicate _DSM #5 function is supported.
> 
> 
> 
> So let's make it self documenting:
> 
> {
> 	0x1 << 0 /* support for support for any functions other than function 0 */ |
> 	0x1 << 5 /* support for function 5 */
> }
> 
> 
> 
>>>
>>> Also, as long as we are changing this probably shouldn't hard-code
>>> 1 as array size ...
>>>
>>
>> Is a macro enough? Like #define RET_BUF_SIZE 2
> 
> Better to use 
> 
> uint8_t byte_list[] = { ... };
> 
> And then ARRAY_SIZE(byte_list)
> 

Got it. I'll fix in the next patch.

Thanks,
Jiahui

> 
>>>
>>>>      aml_append(ifctx1, aml_return(buf));
>>>>      aml_append(ifctx, ifctx1);
>>>> +
>>>> +    /* PCI Firmware Specification 3.2
>>>> +     * 4.6.5. _DSM for Ignoring PCI Boot Configurations
>>>
>>> Note you must always quote the most recent spec that
>>> your change refers to. This is so people can figure out
>>> legacy guest compatibility.
>>>
>>> In this case I think this first appeard in 3.1 not 3.2
>>>
>>
>> OK, I'll fix this.
>>
>>>> +     * The UUID in _DSM in this context is
>>>> +     * {E5C937D0-3553-4D7A-9117-EA4D19C3434D}
>>>
>>> This is just five lines earier, I don't think we need it here.
>>>
>>
>> Will remove.
>>
>>>> +     */
>>>> +    ifctx1 = aml_if(aml_equal(aml_arg(2), aml_int(5)));
>>>
>>> add comment:
>>> 	/* Arg2: Function Index: 5 */
>>
>> Will add.
>>
>>>
>>>> +    aml_append(ifctx1, aml_return(aml_int(0)));
>>>
>>>
>>> add comment: /* 0 - do not ignore ... (quote spec I don't have it to hand) */
>>>
>>
>> Will add.
>>
>> Thanks,
>> Jiahui
>>
>>>
>>>
>>>
>>>> +    aml_append(ifctx, ifctx1);
>>>>      aml_append(method, ifctx);
>>>>  
>>>>      byte_list[0] = 0;
>>>> -- 
>>>> 2.28.0
>>>
>>> .
>>>
> 
> .
> 


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] acpi/gpex: Inform os to keep firmware resource map
  2020-12-17 13:29 [PATCH] acpi/gpex: Inform os to keep firmware resource map Jiahui Cen
                   ` (2 preceding siblings ...)
  2020-12-17 20:04 ` Michael S. Tsirkin
@ 2021-07-22  5:22 ` Guenter Roeck
  3 siblings, 0 replies; 11+ messages in thread
From: Guenter Roeck @ 2021-07-22  5:22 UTC (permalink / raw)
  To: Jiahui Cen
  Cc: xieyingtai, Igor Mammedov, Michael S. Tsirkin, qemu-devel, Gerd Hoffmann

Hi,

On Thu, Dec 17, 2020 at 09:29:26PM +0800, 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>

Since this patch is in qemu (ie starting with qemu v6.0), some of my qemu
tests booting aarch64 systems with efi bios no longer work. For example,
the following command fails to instantiate the Ethernet interface.

CMDLINE="root=/dev/vda console=ttyAMA0"
ROOTFS="rootfs.ext2"

qemu-system-aarch64 -M virt -kernel arch/arm64/boot/Image -no-reboot \
	-m 512 -cpu cortex-a57 -no-reboot \
	-device tulip,netdev=net0 -netdev user,id=net0 \
	-bios QEMU_EFI-aarch64.fd \
	-snapshot \
	-device virtio-blk-device,drive=d0 \
	-drive file=${ROOTFS},if=none,id=d0,format=raw \
	-nographic -serial stdio -monitor none \
	--append "${CMDLINE}"

QEMU_EFI-aarch64.fd is from https://retrage.github.io/edk2-nightly/.

Key difference is PCI BAR assignment.

good (qemu v5.2):

[    3.921801] pci 0000:00:01.0: [1011:0019] type 00 class 0x020000
[    3.922207] pci 0000:00:01.0: reg 0x10: [io  0x0000-0x007f]
[    3.922505] pci 0000:00:01.0: reg 0x14: [mem 0x10000000-0x1000007f]
[    3.927111] pci 0000:00:01.0: BAR 0: assigned [io  0x1000-0x107f]
[    3.927455] pci 0000:00:01.0: BAR 1: assigned [mem 0x10000000-0x1000007f]

bad (qemu v6.0 and later):

[    3.922887] pci 0000:00:01.0: [1011:0019] type 00 class 0x020000
[    3.923278] pci 0000:00:01.0: reg 0x10: [io  0x0000-0x007f]
[    3.923451] pci 0000:00:01.0: reg 0x14: [mem 0x10000000-0x1000007f]

Reverting this patch fixes the problem.

Does this mean that I can no longer run aarch64 efi boot tests
with qemu v6.0 and later if the test involves PCI devices ?
Or is there some alternative command line which would still work ?

Thanks,
Guenter


^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2021-07-22  5:24 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-17 13:29 [PATCH] acpi/gpex: Inform os to keep firmware resource map Jiahui Cen
2020-12-17 13:52 ` Jiahui Cen
2020-12-17 17:23   ` Laszlo Ersek
2020-12-17 19:24     ` Ard Biesheuvel
2020-12-17 18:29 ` Michael S. Tsirkin
2020-12-18  5:56   ` Jiahui Cen
2020-12-17 20:04 ` Michael S. Tsirkin
2020-12-18  5:56   ` Jiahui Cen
2020-12-19 19:06     ` Michael S. Tsirkin
2020-12-21  1:12       ` Jiahui Cen
2021-07-22  5:22 ` Guenter Roeck

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).