[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH V2] xen/arm: Initialize acpi_disabled to true during declaration


  • To: Julien Grall <julien@xxxxxxx>, Oleksandr Tyshchenko <Oleksandr_Tyshchenko@xxxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • From: "Orzel, Michal" <michal.orzel@xxxxxxx>
  • Date: Tue, 25 Mar 2025 16:35:39 +0100
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=amd.com; dmarc=pass action=none header.from=amd.com; dkim=pass header.d=amd.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector10001; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=nC1Vn21WNWdvDhr0BOmuRDdfFSgfJjNuavugtAXje1o=; b=pDko8wVXIz7ikKaqBXUVX9/icx4+h3UFh4JxVN9Nh/8p61hmsXWGBIa79vLOF/d9GddXnqVmy1aRKB34vLI0G5/kESIv15M/jS+8lnHhgXqf6Y2AwPmxSYC0tjpt4s+rTQEo9ODJ/2gW9l8e3Zy1nRKN7zauOwTfufuBmt5Z1MGPPkujOYLjz7cl87C4glHmKWUUWPW8oX9dAift6HWF81ZQvmh0gHXWG4JNgaZyBMZJOg2ShXZL5u9pJxvmqAxoA4RgfAqZncjTQNXx2Sfb4DxETMAChEfI9dTpVFLRdnnqhyPnDruEb4gvDPIXKPn16uysUpaU+kQiJE+Cmufxgg==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector10001; d=microsoft.com; cv=none; b=vw0PCHe/K+BzVl+w4pw3MwAgWHaV69SHhbatpQewAZfcUetpPxwjqR1mjmpW8crC9u7BLK0c9nurK4xaRhKCvYGHHQ76AkWGOe9axg3QWv94YZsg/kU63m1RxzVJpMowqL4q/kP8LLXKBfsHfBzNI9IwMGgjqeK1onrQSQpn4dNUqbxTM5DNGKsuBJRVrt4i6pyy8piOfaJa1Dgl7gitJvCRCfsQYCq+YAYrJwJe+o8Ksk4oGZ0BGSqbYRpoFbKv8/QEnVCWmTYEYYH1LObSDv5ZvQPY1PrbpNbNMRXD7pfTrZP1a6FqeUpVR5pnbphHG480ksQ7aW2vNojnMz3AnQ==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=amd.com;
  • Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>, Bertrand Marquis <bertrand.marquis@xxxxxxx>, Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
  • Delivery-date: Tue, 25 Mar 2025 15:35:58 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>


On 25/03/2025 16:23, Julien Grall wrote:
> 
> 
> Hi Oleksandr, Michal,
> 
> On 25/03/2025 15:08, Oleksandr Tyshchenko wrote:
>> On the device-tree-based Arm64 system, if Xen is built with
>> CONFIG_ACPI=y, CONFIG_STATIC_MEMORY=y, and the static memory range
>> is provided in the host device tree, the BUG is triggered in
>> common/page_alloc.c during Xen's early boot. The BUG occurs when
>> the first page from the static range is fed to the domain
>> sub-allocator and finally ends up in mark_page_free().
>> The pg->count_info & PGC_state is not in the state that
>> the code expects to see there.
>>
>> (XEN) Checking for initrd in /chosen
>> (XEN) Checking for "xen,static-mem" in domain node
>> (XEN) RAM: 0000000040000000 - 00000000bfffffff
>> (XEN)
>> (XEN) MODULE[0]: 0000000043200000 - 0000000043343fff Xen
>> (XEN) MODULE[1]: 0000000043400000 - 0000000043402fff Device Tree
>> (XEN) MODULE[2]: 0000000042e00000 - 0000000043111f82 Ramdisk
>> (XEN) MODULE[3]: 0000000040400000 - 0000000042cfffff Kernel
>> (XEN)  RESVD[0]: 0000000050000000 - 000000005fffffff
>> (XEN)
>> (XEN) CMDLINE[0000000040400000]:domU0 console=ttyAMA0
>> (XEN)
>> (XEN) Command line: console=dtuart conswitch=ax
>> (XEN) pg MFN 50000 c=0x2180000000000000 o=0 v=0 t=0
>> (XEN) Xen BUG at common/page_alloc.c:1474
>> [snip]
>>
>> The problem is that the static range gets mistakenly unreserved
>> in populate_boot_allocator() and reaches init_boot_pages().
>> This happens since by the time the populate_boot_allocator()
>> is executed, the evaluated in fw_unreserved_regions()
>> an acpi_disabled variable is still false and as the result
>> the dt_unreserved_regions() which should simply skip that static range
>> does not get called. The acpi_disabled will be set to the actual value
>> (in our case it is true) later on in acpi_boot_table_init().
>  > > The important question is why acpi_disabled is false by the time
Simply because it's a static bool variable with no assigned value i.e. gets
defaulted to false. And it stays at default value until acpi_boot_table_init()
call that cannot really be moved before setup_mm().

>> setup_mm() is executed. With CONFIG_ACPI=n it is a macro that is always
>> true, but with CONFIG_ACPI=y it is a boolean that is false from the very
>> beggining, even though the entire acpi_boot_table_init() (which is called
>> after setup_mm()) is written with the assumption that ACPI is off by default
>> at the start. So, initialize acpi_disabled to true during declaration
>> if CONFIG_ACPI=y to avoid an issue and match to acpi_boot_table_init().
> 
> While I agree that acpi_disabled should be false. It feels like a bit of
You meant true (?) i.e. ACPI default off not to make any assumptions whether
it's really on/off which can only be determined in acpi_boot_table_init(). I
think we still need this patch to match the code expectation.

> a workaround for the issue you are trying to solve here. If
> fw_unreserved_regions() doesn't work with ACPI enabled, then it is still
> a problem after your patch.
I don't understand. It does work with ACPI enabled provided that it's indeed
enabled. When booting with ACPI, reserved memory regions are not used - we even
have a comment in struct bootinfo. The issue here is that acpi_disabled is set
to false i.e. incorrectly there is assumption that ACPI is enabled by default
and calling fw_unreserved_regions() prior to acpi_boot_table_init() works as
long as we respect the expected default value.

> 
> Furthermore, what happen if we decide to use ACPI afterwards? Wouldn't
> this mean that the static regions would be reserved even if ACPI doesn't
> use static memory (all the memory is expected to be given to the allocator)?
I don't think such hybrid configuration is valid (booting with ACPI yet
declaring reserved regions in DT). See commit:
9c2bc0f24b2ba7082df408b3c33ec9a86bf20cf0

~Michal




 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.