| Message ID | 20250325150842.2015968-1-oleksandr_tyshchenko@epam.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [V2] xen/arm: Initialize acpi_disabled to true during declaration | expand |
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 > 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 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. 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)? Cheers,
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
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. Cheers,
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, >
Hi Oleksandr, On 25/03/2025 16:05, Oleksandr Tyshchenko wrote: >>>> 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). I am not asking to make ACPI work with static memory. I am asking to not break ACPI if the Device-Tree is specifying static memory region. > However, I wonder, why it has not been noticed so far. ACPI is not a supported feature and gated by UNSUPPORTED. So the implication is you have enabled UNSUPPORTED and anything can happen really ;). > > 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). As I wrote above, the only thing I am asking is that memory for static regions should be unreserved when ACPI is enabled like it is already the case today. Cheers,
On 25.03.25 18:09, Julien Grall wrote: > Hi Oleksandr, Hello Julien > > On 25/03/2025 16:05, Oleksandr Tyshchenko wrote: >>>>> 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). > > I am not asking to make ACPI work with static memory. I am asking to not > break ACPI if the Device-Tree is specifying static memory region. > >> However, I wonder, why it has not been noticed so far. > > ACPI is not a supported feature and gated by UNSUPPORTED. So the > implication is you have enabled UNSUPPORTED and anything can happen > really ;). Indeed, this answers the question. > >> >> 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). > > As I wrote above, the only thing I am asking is that memory for static > regions should be unreserved when ACPI is enabled like it is already the > case today. So the concern is that not reserving provided by the device tree static memory region is going to be a potential waste of the memory on the real ACPI system? Or I missed something? I see two options with unreserving the static memory regions on the real ACPI system. I assume, that we should unreserve only after we definitely know that we are running with ACPI (i.e. after acpi_boot_table_init() completes and acpi_disabled reflects the real state of things), right? 1. either call acpi_boot_table_init() before setup_mm() in Arm64's start_xen(). 2. or go through reserved_mem->nr_banks and unreserve everything marked with MEMBANK_STATIC_DOMAIN after acpi_boot_table_init() completes What do you think? Unfortunately, I do not have such environment in hand (ACPI-based (dom0less?) system but still providing device-tree with static memory) for the real testing. > > Cheers, >
On 25/03/2025 17:54, Oleksandr Tyshchenko wrote: > > > On 25.03.25 18:09, Julien Grall wrote: > > >> Hi Oleksandr, > > Hello Julien > >> >> On 25/03/2025 16:05, Oleksandr Tyshchenko wrote: >>>>>> 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). >> >> I am not asking to make ACPI work with static memory. I am asking to not >> break ACPI if the Device-Tree is specifying static memory region. >> >>> However, I wonder, why it has not been noticed so far. >> >> ACPI is not a supported feature and gated by UNSUPPORTED. So the >> implication is you have enabled UNSUPPORTED and anything can happen >> really ;). > > Indeed, this answers the question. > > >> >>> >>> 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). >> >> As I wrote above, the only thing I am asking is that memory for static >> regions should be unreserved when ACPI is enabled like it is already the >> case today. > > So the concern is that not reserving provided by the device tree static > memory region is going to be a potential waste of the memory on the real > ACPI system? Or I missed something? > > I see two options with unreserving the static memory regions on the real > ACPI system. I assume, that we should unreserve only after we definitely > know that we are running with ACPI (i.e. after acpi_boot_table_init() > completes and acpi_disabled reflects the real state of things), right? > > 1. either call acpi_boot_table_init() before setup_mm() in Arm64's > start_xen(). This cannot be done. acpi_boot_table_init() calls ACPI functions that make use of VMAP and alloc_boot_pages, so the boot allocator is expected to be populated at this point. > 2. or go through reserved_mem->nr_banks and unreserve everything marked > with MEMBANK_STATIC_DOMAIN after acpi_boot_table_init() completes What if we split acpi_boot_table_init() into 2 parts, where first is used to determine the real "acpi_disabled" value, called before setup_mm and second used to parse the tables? There's one issue with this approach. Today, even after we evaluate that we should run with ACPI enabled [*], error in ACPI table parsing is ignored and ACPI is set to disabled. That's not really in the spirit of our current methodology, where we should panic if user requested something that does not work. Example: today, even after specifying "acpi=force", error in table parsing bails out to DT approach which is not what user wanted. ~Michal
On 26.03.25 10:57, Orzel, Michal wrote: Hello Michal, Julien > > > On 25/03/2025 17:54, Oleksandr Tyshchenko wrote: >> >> >> On 25.03.25 18:09, Julien Grall wrote: >> >> >>> Hi Oleksandr, >> >> Hello Julien >> >>> >>> On 25/03/2025 16:05, Oleksandr Tyshchenko wrote: >>>>>>> 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). >>> >>> I am not asking to make ACPI work with static memory. I am asking to not >>> break ACPI if the Device-Tree is specifying static memory region. >>> >>>> However, I wonder, why it has not been noticed so far. >>> >>> ACPI is not a supported feature and gated by UNSUPPORTED. So the >>> implication is you have enabled UNSUPPORTED and anything can happen >>> really ;). >> >> Indeed, this answers the question. >> >> >>> >>>> >>>> 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). >>> >>> As I wrote above, the only thing I am asking is that memory for static >>> regions should be unreserved when ACPI is enabled like it is already the >>> case today. >> >> So the concern is that not reserving provided by the device tree static >> memory region is going to be a potential waste of the memory on the real >> ACPI system? Or I missed something? >> >> I see two options with unreserving the static memory regions on the real >> ACPI system. I assume, that we should unreserve only after we definitely >> know that we are running with ACPI (i.e. after acpi_boot_table_init() >> completes and acpi_disabled reflects the real state of things), right? >> >> 1. either call acpi_boot_table_init() before setup_mm() in Arm64's >> start_xen(). > This cannot be done. acpi_boot_table_init() calls ACPI functions that make use > of VMAP and alloc_boot_pages, so the boot allocator is expected to be populated > at this point. hmm, I got it. > >> 2. or go through reserved_mem->nr_banks and unreserve everything marked >> with MEMBANK_STATIC_DOMAIN after acpi_boot_table_init() completes > What if we split acpi_boot_table_init() into 2 parts, where first is used to > determine the real "acpi_disabled" value, called before setup_mm and second used > to parse the tables? There's one issue with this approach. Today, even after we > evaluate that we should run with ACPI enabled [*], error in ACPI table parsing > is ignored and ACPI is set to disabled. That's not really in the spirit of our > current methodology, where we should panic if user requested something that does > not work. Example: today, even after specifying "acpi=force", error in table > parsing bails out to DT approach which is not what user wanted. Michal, interesting suggestion, thanks. Let's say acpi_preinit() and acpi_boot_table_init(). I see that there is already split for some components (e.g. time, gic). I agree with what you write, yes, looks like it should work if we convert errors in ACPI table parsing into panics (note, this is change to an existing behavior). Also it is not entirely clear what we should do with acpi_disabled=true during declaration (what current patch does), even if we decided to go with splitting should we retain that change? > > ~Michal >
Hi, On 26/03/2025 08:57, Orzel, Michal wrote: > > > On 25/03/2025 17:54, Oleksandr Tyshchenko wrote: >> >> >> On 25.03.25 18:09, Julien Grall wrote: >> >> >>> Hi Oleksandr, >> >> Hello Julien >> >>> >>> On 25/03/2025 16:05, Oleksandr Tyshchenko wrote: >>>>>>> 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). >>> >>> I am not asking to make ACPI work with static memory. I am asking to not >>> break ACPI if the Device-Tree is specifying static memory region. >>> >>>> However, I wonder, why it has not been noticed so far. >>> >>> ACPI is not a supported feature and gated by UNSUPPORTED. So the >>> implication is you have enabled UNSUPPORTED and anything can happen >>> really ;). >> >> Indeed, this answers the question. >> >> >>> >>>> >>>> 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). >>> >>> As I wrote above, the only thing I am asking is that memory for static >>> regions should be unreserved when ACPI is enabled like it is already the >>> case today. >> >> So the concern is that not reserving provided by the device tree static >> memory region is going to be a potential waste of the memory on the real >> ACPI system? Or I missed something? >> >> I see two options with unreserving the static memory regions on the real >> ACPI system. I assume, that we should unreserve only after we definitely >> know that we are running with ACPI (i.e. after acpi_boot_table_init() >> completes and acpi_disabled reflects the real state of things), right? >> >> 1. either call acpi_boot_table_init() before setup_mm() in Arm64's >> start_xen(). > This cannot be done. acpi_boot_table_init() calls ACPI functions that make use > of VMAP and alloc_boot_pages, so the boot allocator is expected to be populated > at this point. > >> 2. or go through reserved_mem->nr_banks and unreserve everything marked >> with MEMBANK_STATIC_DOMAIN after acpi_boot_table_init() completes > What if we split acpi_boot_table_init() into 2 parts, where first is used to > determine the real "acpi_disabled" value, called before setup_mm and second used > to parse the tables? That would be fine with me in principle. > There's one issue with this approach. Today, even after we > evaluate that we should run with ACPI enabled [*], error in ACPI table parsing > is ignored and ACPI is set to disabled. That's not really in the spirit of our > current methodology, where we should panic if user requested something that does > not work. Example: today, even after specifying "acpi=force", error in table > parsing bails out to DT approach which is not what user wanted. If you plan to send such patch, can you have a look at [1]? This was an attempt to panic if we can't enable ACPI and the user requested it. [1] https://lore.kernel.org/all/1486149538-20432-6-git-send-email-julien.grall@arm.com/ > > ~Michal >
Hi Oleksandr, On 26/03/2025 11:45, Oleksandr Tyshchenko wrote: > Also it is not entirely clear what we should do with acpi_disabled=true > during declaration (what current patch does), even if we decided to go > with splitting should we retain that change? It should not be necessary. But I will leave up to you whether you want to upstream this patch. Although, I will need a commit rewording. Cheers,
On 27.03.25 00:33, Julien Grall wrote: Hello Julien > Hi, > > On 26/03/2025 08:57, Orzel, Michal wrote: >> >> >> On 25/03/2025 17:54, Oleksandr Tyshchenko wrote: >>> >>> >>> On 25.03.25 18:09, Julien Grall wrote: >>> >>> >>>> Hi Oleksandr, >>> >>> Hello Julien >>> >>>> >>>> On 25/03/2025 16:05, Oleksandr Tyshchenko wrote: >>>>>>>> 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). >>>> >>>> I am not asking to make ACPI work with static memory. I am asking to >>>> not >>>> break ACPI if the Device-Tree is specifying static memory region. >>>> >>>>> However, I wonder, why it has not been noticed so far. >>>> >>>> ACPI is not a supported feature and gated by UNSUPPORTED. So the >>>> implication is you have enabled UNSUPPORTED and anything can happen >>>> really ;). >>> >>> Indeed, this answers the question. >>> >>> >>>> >>>>> >>>>> 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). >>>> >>>> As I wrote above, the only thing I am asking is that memory for static >>>> regions should be unreserved when ACPI is enabled like it is already >>>> the >>>> case today. >>> >>> So the concern is that not reserving provided by the device tree static >>> memory region is going to be a potential waste of the memory on the real >>> ACPI system? Or I missed something? >>> >>> I see two options with unreserving the static memory regions on the real >>> ACPI system. I assume, that we should unreserve only after we definitely >>> know that we are running with ACPI (i.e. after acpi_boot_table_init() >>> completes and acpi_disabled reflects the real state of things), right? >>> >>> 1. either call acpi_boot_table_init() before setup_mm() in Arm64's >>> start_xen(). >> This cannot be done. acpi_boot_table_init() calls ACPI functions that >> make use >> of VMAP and alloc_boot_pages, so the boot allocator is expected to be >> populated >> at this point. >> >>> 2. or go through reserved_mem->nr_banks and unreserve everything marked >>> with MEMBANK_STATIC_DOMAIN after acpi_boot_table_init() completes >> What if we split acpi_boot_table_init() into 2 parts, where first is >> used to >> determine the real "acpi_disabled" value, called before setup_mm and >> second used >> to parse the tables? > > That would be fine with me in principle. good > >> There's one issue with this approach. Today, even after we >> evaluate that we should run with ACPI enabled [*], error in ACPI table >> parsing >> is ignored and ACPI is set to disabled. That's not really in the >> spirit of our >> current methodology, where we should panic if user requested something >> that does >> not work. Example: today, even after specifying "acpi=force", error in >> table >> parsing bails out to DT approach which is not what user wanted. > > If you plan to send such patch, can you have a look at [1]? This was an > attempt to panic if we can't enable ACPI and the user requested it. I analyzed your patch, thanks for pointing to it. And if I get everything correctly then, I think, that the patch in its current form (as is) will indeed help fix the issue in our particular case (together with splitting acpi_boot_table_init() into acpi_preinit() and acpi_boot_table_init() of course). With the existing patch, we will have the following: acpi=off: Turned off ACPI -> DT is used, static memory is reserved acpi=on: Turned on ACPI only if DT is empty -> DT is not empty, so is still used, static memory is reserved acpi=force: Turned on ACPI -> as there is no ACPI, just panic But IIUC, the conclusion (it that thread) was to change the behavior for "acpi=on", i.e. to prefer ACPI over DT, in which case, I am not sure what the exact Xen behavior for "acpi=on" should be and whether it will fix the issue, please clarify: No matter if the DT is empty or not, we try to initialize the ACPI, and if ACPI is present (initialization success), then it is used, right? And if ACPI is not present (initialization failure), then we fallback to DT or just panic? If the latter, then we are all good, but if the former, then, I am afraid, we will face the same BUG (as we unreserve static memory if acpi_preinit() sets acpi_disabled to false). Or I missed something? > > [1] > https://lore.kernel.org/all/1486149538-20432-6-git-send-email-julien.grall@arm.com/ > >> >> ~Michal >> >
On 27.03.25 00:39, Julien Grall wrote: > Hi Oleksandr, Hello Julien > > On 26/03/2025 11:45, Oleksandr Tyshchenko wrote: >> Also it is not entirely clear what we should do with acpi_disabled=true >> during declaration (what current patch does), even if we decided to go >> with splitting should we retain that change? > > It should not be necessary. Thanks for the clarification. But I will leave up to you whether you want > to upstream this patch. Although, I will need a commit rewording. I would prefer not to spend time on this. > > Cheers, >
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c index ffcae900d7..9e94f1a8c7 100644 --- a/xen/arch/arm/setup.c +++ b/xen/arch/arm/setup.c @@ -57,7 +57,7 @@ struct cpuinfo_arm __read_mostly system_cpuinfo; #ifdef CONFIG_ACPI -bool __read_mostly acpi_disabled; +bool __read_mostly acpi_disabled = true; #endif domid_t __read_mostly max_init_domid;
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 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(). Suggested-by: Michal Orzel <michal.orzel@amd.com> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> --- V2: - drop post-commit remark - use the approach suggested by Michal - update commit subject (WAS xen/device-tree: Switch back to dt_unreserved_regions() in boot allocator) and description --- --- xen/arch/arm/setup.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)