xen/arm: static event channel

[PATCH 2/2] xen/arm: fix booting ACPI based system after static evtchn series

Posted by Rahul Singh 3 years, 4 months ago

When ACPI is enabled and the system booted with ACPI, BUG() is observed
after merging the static event channel series. As there is not DT when
booted with ACPI there will be no chosen node because of that
"BUG_ON(chosen == NULL)" will be hit.

(XEN) Xen BUG at arch/arm/domain_build.c:3578

Move call to alloc_static_evtchn() under acpi_disabled check to fix the
issue.

Fixes: 1fe16b3ed78a (xen/arm: introduce xen-evtchn dom0less property)
Signed-off-by: Rahul Singh <rahul.singh@arm.com>
---
 xen/arch/arm/setup.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 61b4f258a0..4395640019 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -1166,9 +1166,10 @@ void __init start_xen(unsigned long boot_phys_offset,
         printk(XENLOG_INFO "Xen dom0less mode detected\n");
 
     if ( acpi_disabled )
+    {
         create_domUs();
-
-    alloc_static_evtchn();
+        alloc_static_evtchn();
+    }
 
     /*
      * This needs to be called **before** heap_init_late() so modules
-- 
2.25.1

Re: [PATCH 2/2] xen/arm: fix booting ACPI based system after static evtchn series

Posted by Bertrand Marquis 3 years, 4 months ago

Hi,

> On 23 Sep 2022, at 13:02, Rahul Singh <Rahul.Singh@arm.com> wrote:
> 
> When ACPI is enabled and the system booted with ACPI, BUG() is observed
> after merging the static event channel series. As there is not DT when
> booted with ACPI there will be no chosen node because of that
> "BUG_ON(chosen == NULL)" will be hit.
> 
> (XEN) Xen BUG at arch/arm/domain_build.c:3578
> 
> Move call to alloc_static_evtchn() under acpi_disabled check to fix the
> issue.
> 
> Fixes: 1fe16b3ed78a (xen/arm: introduce xen-evtchn dom0less property)
> Signed-off-by: Rahul Singh <rahul.singh@arm.com>

Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

This is needed to fix ACPI build issues and was release-acked already.

Cheers
Bertrand

> ---
> xen/arch/arm/setup.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 61b4f258a0..4395640019 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -1166,9 +1166,10 @@ void __init start_xen(unsigned long boot_phys_offset,
>         printk(XENLOG_INFO "Xen dom0less mode detected\n");
> 
>     if ( acpi_disabled )
> +    {
>         create_domUs();
> -
> -    alloc_static_evtchn();
> +        alloc_static_evtchn();
> +    }
> 
>     /*
>      * This needs to be called **before** heap_init_late() so modules
> -- 
> 2.25.1
>

Re: [PATCH 2/2] xen/arm: fix booting ACPI based system after static evtchn series

Posted by Ayan Kumar Halder 3 years, 4 months ago

Hi Rahul,

On 23/09/2022 12:02, Rahul Singh wrote:
> CAUTION: This message has originated from an External Source. Please use proper judgment and caution when opening attachments, clicking links, or responding to this email.
>
>
> When ACPI is enabled and the system booted with ACPI, BUG() is observed
> after merging the static event channel series. As there is not DT when
[NIT] : s/not/no
> booted with ACPI there will be no chosen node because of that
> "BUG_ON(chosen == NULL)" will be hit.
>
> (XEN) Xen BUG at arch/arm/domain_build.c:3578
Is the bug seen on the gitlab ci ?
>
> Move call to alloc_static_evtchn() under acpi_disabled check to fix the
> issue.
>
> Fixes: 1fe16b3ed78a (xen/arm: introduce xen-evtchn dom0less property)
> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
> ---
>   xen/arch/arm/setup.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 61b4f258a0..4395640019 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -1166,9 +1166,10 @@ void __init start_xen(unsigned long boot_phys_offset,
>           printk(XENLOG_INFO "Xen dom0less mode detected\n");
>
>       if ( acpi_disabled )
> +    {
>           create_domUs();
> -
> -    alloc_static_evtchn();
> +        alloc_static_evtchn();

Can the code in alloc_static_evtchn() be guarded with "#ifndef 
CONFIG_ACPI ... endif" ?

- Ayan

> +    }
>
>       /*
>        * This needs to be called **before** heap_init_late() so modules
> --
> 2.25.1
>
>

Re: [PATCH 2/2] xen/arm: fix booting ACPI based system after static evtchn series

Posted by Jan Beulich 3 years, 4 months ago

On 23.09.2022 14:10, Ayan Kumar Halder wrote:
> On 23/09/2022 12:02, Rahul Singh wrote:
>> --- a/xen/arch/arm/setup.c
>> +++ b/xen/arch/arm/setup.c
>> @@ -1166,9 +1166,10 @@ void __init start_xen(unsigned long boot_phys_offset,
>>           printk(XENLOG_INFO "Xen dom0less mode detected\n");
>>
>>       if ( acpi_disabled )
>> +    {
>>           create_domUs();
>> -
>> -    alloc_static_evtchn();
>> +        alloc_static_evtchn();
> 
> Can the code in alloc_static_evtchn() be guarded with "#ifndef 
> CONFIG_ACPI ... endif" ?

This wouldn't help the issue, but at best code size when !CONFIG_ACPI. When
CONFIG_ACPI=y, acpi_disabled might still be true, and hence the function
may still need skipping. Apart from this I'd also consider it odd to have
a non-ACPI function have such an #ifdef ...

Jan

Re: [PATCH 2/2] xen/arm: fix booting ACPI based system after static evtchn series

Posted by Ayan Kumar Halder 3 years, 4 months ago

On 26/09/2022 08:38, Jan Beulich wrote:
> On 23.09.2022 14:10, Ayan Kumar Halder wrote:
>> On 23/09/2022 12:02, Rahul Singh wrote:
>>> --- a/xen/arch/arm/setup.c
>>> +++ b/xen/arch/arm/setup.c
>>> @@ -1166,9 +1166,10 @@ void __init start_xen(unsigned long boot_phys_offset,
>>>            printk(XENLOG_INFO "Xen dom0less mode detected\n");
>>>
>>>        if ( acpi_disabled )
>>> +    {
>>>            create_domUs();
>>> -
>>> -    alloc_static_evtchn();
>>> +        alloc_static_evtchn();
>> Can the code in alloc_static_evtchn() be guarded with "#ifndef
>> CONFIG_ACPI ... endif" ?
> This wouldn't help the issue, but at best code size when !CONFIG_ACPI. When
> CONFIG_ACPI=y, acpi_disabled might still be true, and hence the function
> may still need skipping. Apart from this I'd also consider it odd to have
> a non-ACPI function have such an #ifdef ...

I think this makes sense. Also Rahul's previous comments looks 
reasonable to me.

Reviewed-by: Ayan Kumar Halder <ayankuma@amd.com>

>
> Jan

Re: [PATCH 2/2] xen/arm: fix booting ACPI based system after static evtchn series

Posted by Rahul Singh 3 years, 4 months ago

Hi Ayan,
 
> On 23 Sep 2022, at 1:10 pm, Ayan Kumar Halder <ayankuma@amd.com> wrote:
> 
> Hi Rahul,
> 
> On 23/09/2022 12:02, Rahul Singh wrote:
>> CAUTION: This message has originated from an External Source. Please use proper judgment and caution when opening attachments, clicking links, or responding to this email.
>> 
>> 
>> When ACPI is enabled and the system booted with ACPI, BUG() is observed
>> after merging the static event channel series. As there is not DT when
> [NIT] : s/not/no

Ack. 
>> booted with ACPI there will be no chosen node because of that
>> "BUG_ON(chosen == NULL)" will be hit.
>> 
>> (XEN) Xen BUG at arch/arm/domain_build.c:3578
> Is the bug seen on the gitlab ci ?

No, I found the issue while testing the ACPI boot. But going forward we will add this in our internal ci.
>> 
>> Move call to alloc_static_evtchn() under acpi_disabled check to fix the
>> issue.
>> 
>> Fixes: 1fe16b3ed78a (xen/arm: introduce xen-evtchn dom0less property)
>> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
>> ---
>>  xen/arch/arm/setup.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>> 
>> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
>> index 61b4f258a0..4395640019 100644
>> --- a/xen/arch/arm/setup.c
>> +++ b/xen/arch/arm/setup.c
>> @@ -1166,9 +1166,10 @@ void __init start_xen(unsigned long boot_phys_offset,
>>          printk(XENLOG_INFO "Xen dom0less mode detected\n");
>> 
>>      if ( acpi_disabled )
>> +    {
>>          create_domUs();
>> -
>> -    alloc_static_evtchn();
>> +        alloc_static_evtchn();
> 
> Can the code in alloc_static_evtchn() be guarded with "#ifndef CONFIG_ACPI ... endif" ?

Not required as acpi_disabled will take care of that. acpi_disabled variable is used to avoid the CONFIG_ACPI.

Regards,
Rahul