qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: Jiahui Cen <cenjiahui@huawei.com>
To: "Michael S. Tsirkin" <mst@redhat.com>
Cc: xieyingtai@huawei.com, Igor Mammedov <imammedo@redhat.com>,
	qemu-devel@nongnu.org, Gerd Hoffmann <kraxel@redhat.com>
Subject: Re: [PATCH] acpi/gpex: Inform os to keep firmware resource map
Date: Fri, 18 Dec 2020 13:56:29 +0800	[thread overview]
Message-ID: <0c7bcfe9-437b-f750-29d5-983d09a47b3c@huawei.com> (raw)
In-Reply-To: <20201217144933-mutt-send-email-mst@kernel.org>

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


  reply	other threads:[~2020-12-18  5:59 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-12-17 13:29 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 [this message]
2020-12-19 19:06     ` Michael S. Tsirkin
2020-12-21  1:12       ` Jiahui Cen
2021-07-22  5:22 ` Guenter Roeck

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=0c7bcfe9-437b-f750-29d5-983d09a47b3c@huawei.com \
    --to=cenjiahui@huawei.com \
    --cc=imammedo@redhat.com \
    --cc=kraxel@redhat.com \
    --cc=mst@redhat.com \
    --cc=qemu-devel@nongnu.org \
    --cc=xieyingtai@huawei.com \
    --subject='Re: [PATCH] acpi/gpex: Inform os to keep firmware resource map' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

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).