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

["Michael S. Tsirkin" <mst@redhat.com>]

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