[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>, "Orzel, Michal" <michal.orzel@xxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • From: Oleksandr Tyshchenko <Oleksandr_Tyshchenko@xxxxxxxx>
  • Date: Tue, 25 Mar 2025 16:05:12 +0000
  • Accept-language: en-US, ru-RU
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=epam.com; dmarc=pass action=none header.from=epam.com; dkim=pass header.d=epam.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=n+xLwXX1S1PCgI2QGprnuobsNb2l5t9uLiOux5CQpg8=; b=xbWEw2Ucl76lgijTK8Ex2UGf1esZGoyynx5AmSaBg22jzzptqAwYCPfQTyXsNmnYpbqdqD8G87da0AXVR9fNRZHbboUebGmK/9Om++4E0WyGW7ZxtZ6SSnqjADQWWk7HRii6lV36ugZqaPfh97bvzc0WBWkJAsVMCiRz9CMsl5MaGvEqrPJafVJJAyEsCqkWLXqiYo5M/GWFYbv9RqpF9Abt6ewz2XbNfKXaK3VbAZtq3lKT+QL9Ve5nbgNIdtzPQED+nR6HkyVaz72DMs70JgUC64iI1BV3ip/TM3ivLVQGOox5FGzTbDGd7nK3yPFT2pgTgkIzy74OgCMm3t/uwA==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector10001; d=microsoft.com; cv=none; b=ity8eBcic7W8R/af0fJ62N+Wp49or8BDBg+NeUUHf2O6bZVxNKHitdEs3Bz/K4GfF8VcIhpyWWU9CYaliYzxJs8s7E3aEK9kHzY372TuU4CS+HGwkHSf+CLtWV2jE10J83AJqNy+55rhCYoe3K2uMtsVdb/YyK9VyGWzsSX4E+i+nGznNyv5J1NE8hRetpSKtGvm8NAJwRA47vXSan5AeiaJNYWHH5rHDNhgWmWsYvp4uaCSFNiiPNqYrhqRlxfrnc/7OnWy8YVWqUk5mKWb/WWO/dWf6bQT1VZYvicv/4kGaq3DpvQv/BiCp3bte+UQxuIduxTExxNn4QgSthqJkw==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=epam.com;
  • Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>, Bertrand Marquis <bertrand.marquis@xxxxxxx>, Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
  • Delivery-date: Tue, 25 Mar 2025 16:05:24 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Thread-index: AQHbnZfMrs+h0o/uEkuTNWO41qxIcLOD+IUAgAADaoCAAAM8AIAABQEA
  • Thread-topic: [PATCH V2] xen/arm: Initialize acpi_disabled to true during declaration


On 25.03.25 17:47, Julien Grall wrote:

Hello Julien, Michal.


> Hi Michal,
> 
> On 25/03/2025 15:35, Orzel, Michal wrote:
>>
>>
>> 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.
> 
> I agree with that.
> 
>>
>>> 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.
> 
> My concern is that some region may have been reserved and used somewhere 
> else. But then a second call to fw_unreserved_regions() would free those 
> regions and possibly trigger another BUG...
> 
>> 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.
> 
> This is arguable whether it is a bug or not. The value of acpi_disabled 
> is the same as on x86. The assumption is that when CONFIG_ACPI=y, then 
> ACPI is used unless it is explicitely turned off afterwards.
> 
>>
>>>
>>> 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
> 
> I don't think the commit is preventing hybrid configuration. It is just 
> saying that the region (which could be a static region because this is 
> not supported) will be unreserved.
> 
> IOW, when booting with Device-Tree you may be able to use static memory. 
> But if you were booting with ACPI, static memory is not supported and 
> therefore the regions should be free for other purpose.


Julien, I see your points, but the current patch does not attempt to 
make static (reserved) memory properly work on ACPI-based system (if it 
is available there), current patch tries to solve the real issue on 
device-tree-based system with Xen compiled with CONFIG_ACPI=y (at least 
unintentionally). However, I wonder, why it has not been noticed so far.

It took some time to understand why just enabling CONFIG_STATIC_MEMORY=y
triggered a BUG in common code. And it turned out that it was 
CONFIG_ACPI=y in my Xen's .config that caused that consequence (I 
specially wrote so long description to provide full context).



> 
> Cheers,
> 

 


Rackspace

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