[Xen-devel,3/8] xen/arm: acpi: Don't fallback on DT when user request ACPI

Message ID 1486149538-20432-6-git-send-email-julien.grall@arm.com
State New
Headers show
Series
  • Untitled series #332
Related show

Commit Message

Julien Grall Feb. 3, 2017, 7:18 p.m.
On ARM, when the user put 'acpi=force' Xen will use ACPI over DT.
Currently, if Xen fails to initialize ACPI it will fallback on DT.

This behavior makes difficult for a user to notice Xen didn't used ACPI
has requested on platform where the firmware is providing both ACPI and DT.

Rather than fallback on DT during a failure, panic when 'acpi=force'.

Signed-off-by: Julien Grall <julien.grall@arm.com>

---
    I am wondering if we should do the same with acpi=on. So a user
    would notice directly if something went wrong with ACPI.
    Otherwise you would boot up to the prompt and barely notice that DT
    was used.
---
 xen/arch/arm/acpi/boot.c | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Stefano Stabellini Feb. 16, 2017, 1:41 a.m. | #1
On Fri, 3 Feb 2017, Julien Grall wrote:
> On ARM, when the user put 'acpi=force' Xen will use ACPI over DT.
> Currently, if Xen fails to initialize ACPI it will fallback on DT.
> 
> This behavior makes difficult for a user to notice Xen didn't used ACPI
> has requested on platform where the firmware is providing both ACPI and DT.
> 
> Rather than fallback on DT during a failure, panic when 'acpi=force'.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> 
> ---
>     I am wondering if we should do the same with acpi=on. So a user
>     would notice directly if something went wrong with ACPI.
>     Otherwise you would boot up to the prompt and barely notice that DT
>     was used.

I would keep the current behavior as is and add new parameter that means
"acpi and only acpi". Today acpi=force means "acpi is preferred to
device tree". It doesn't mean that if acpi fail, Xen should panic.

Maybe acpi=strict or acpi=mandatory?


> ---
>  xen/arch/arm/acpi/boot.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/xen/arch/arm/acpi/boot.c b/xen/arch/arm/acpi/boot.c
> index 889208a..65ef632 100644
> --- a/xen/arch/arm/acpi/boot.c
> +++ b/xen/arch/arm/acpi/boot.c
> @@ -272,6 +272,11 @@ int __init acpi_boot_table_init(void)
>      return 0;
>  
>  disable:
> +
> +    /* Panic if the user has requested ACPI but Xen is able to initialize. */
> +    if ( param_acpi_force )
> +        panic("Unable to use ACPI");
> +
>      disable_acpi();
>  
>      return error;
> -- 
> 1.9.1
>
Julien Grall Feb. 20, 2017, 8:47 a.m. | #2
Hi Stefano,

On 02/16/2017 01:41 AM, Stefano Stabellini wrote:
> On Fri, 3 Feb 2017, Julien Grall wrote:
>> On ARM, when the user put 'acpi=force' Xen will use ACPI over DT.
>> Currently, if Xen fails to initialize ACPI it will fallback on DT.
>>
>> This behavior makes difficult for a user to notice Xen didn't used ACPI
>> has requested on platform where the firmware is providing both ACPI and DT.
>>
>> Rather than fallback on DT during a failure, panic when 'acpi=force'.
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>
>> ---
>>     I am wondering if we should do the same with acpi=on. So a user
>>     would notice directly if something went wrong with ACPI.
>>     Otherwise you would boot up to the prompt and barely notice that DT
>>     was used.
>
> I would keep the current behavior as is and add new parameter that means
> "acpi and only acpi". Today acpi=force means "acpi is preferred to
> device tree". It doesn't mean that if acpi fail, Xen should panic.
>
> Maybe acpi=strict or acpi=mandatory?

We have a bunch of option that makes little sense today:
    - off: Turned off ACPI
    - on: Turned on ACPI only if DT is empty
    - force: Turned on ACPI

If ACPI fails you fallback on DT. This is a pain to detect whether DT or 
ACPI is been used. So I think adding another option will only confuse 
the user.

Looking at Linux the option are:
     - off: Turned off ACPI
     - on: Prefer ACPI over DT. Fallback on DT is failed
     - force: Use ACPI and panic if not available

I would much prefer to use the behavior of the acpi param on Linux 
because it will avoid the user to be confused on the usage.

Cheers,
Stefano Stabellini Feb. 20, 2017, 6:12 p.m. | #3
On Mon, 20 Feb 2017, Julien Grall wrote:
> Hi Stefano,
> 
> On 02/16/2017 01:41 AM, Stefano Stabellini wrote:
> > On Fri, 3 Feb 2017, Julien Grall wrote:
> > > On ARM, when the user put 'acpi=force' Xen will use ACPI over DT.
> > > Currently, if Xen fails to initialize ACPI it will fallback on DT.
> > > 
> > > This behavior makes difficult for a user to notice Xen didn't used ACPI
> > > has requested on platform where the firmware is providing both ACPI and
> > > DT.
> > > 
> > > Rather than fallback on DT during a failure, panic when 'acpi=force'.
> > > 
> > > Signed-off-by: Julien Grall <julien.grall@arm.com>
> > > 
> > > ---
> > >     I am wondering if we should do the same with acpi=on. So a user
> > >     would notice directly if something went wrong with ACPI.
> > >     Otherwise you would boot up to the prompt and barely notice that DT
> > >     was used.
> > 
> > I would keep the current behavior as is and add new parameter that means
> > "acpi and only acpi". Today acpi=force means "acpi is preferred to
> > device tree". It doesn't mean that if acpi fail, Xen should panic.
> > 
> > Maybe acpi=strict or acpi=mandatory?
> 
> We have a bunch of option that makes little sense today:
>    - off: Turned off ACPI
>    - on: Turned on ACPI only if DT is empty
>    - force: Turned on ACPI
> 
> If ACPI fails you fallback on DT. This is a pain to detect whether DT or ACPI
> is been used. So I think adding another option will only confuse the user.
> 
> Looking at Linux the option are:
>     - off: Turned off ACPI
>     - on: Prefer ACPI over DT. Fallback on DT is failed
>     - force: Use ACPI and panic if not available
> 
> I would much prefer to use the behavior of the acpi param on Linux because it
> will avoid the user to be confused on the usage.

All right, but in this case you need to update
docs/misc/xen-command-line.markdown.
Julien Grall Feb. 20, 2017, 6:13 p.m. | #4
Hi Stefano,

On 20/02/17 18:12, Stefano Stabellini wrote:
> On Mon, 20 Feb 2017, Julien Grall wrote:
>> Hi Stefano,
>>
>> On 02/16/2017 01:41 AM, Stefano Stabellini wrote:
>>> On Fri, 3 Feb 2017, Julien Grall wrote:
>>>> On ARM, when the user put 'acpi=force' Xen will use ACPI over DT.
>>>> Currently, if Xen fails to initialize ACPI it will fallback on DT.
>>>>
>>>> This behavior makes difficult for a user to notice Xen didn't used ACPI
>>>> has requested on platform where the firmware is providing both ACPI and
>>>> DT.
>>>>
>>>> Rather than fallback on DT during a failure, panic when 'acpi=force'.
>>>>
>>>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>>>
>>>> ---
>>>>     I am wondering if we should do the same with acpi=on. So a user
>>>>     would notice directly if something went wrong with ACPI.
>>>>     Otherwise you would boot up to the prompt and barely notice that DT
>>>>     was used.
>>>
>>> I would keep the current behavior as is and add new parameter that means
>>> "acpi and only acpi". Today acpi=force means "acpi is preferred to
>>> device tree". It doesn't mean that if acpi fail, Xen should panic.
>>>
>>> Maybe acpi=strict or acpi=mandatory?
>>
>> We have a bunch of option that makes little sense today:
>>    - off: Turned off ACPI
>>    - on: Turned on ACPI only if DT is empty
>>    - force: Turned on ACPI
>>
>> If ACPI fails you fallback on DT. This is a pain to detect whether DT or ACPI
>> is been used. So I think adding another option will only confuse the user.
>>
>> Looking at Linux the option are:
>>     - off: Turned off ACPI
>>     - on: Prefer ACPI over DT. Fallback on DT is failed
>>     - force: Use ACPI and panic if not available
>>
>> I would much prefer to use the behavior of the acpi param on Linux because it
>> will avoid the user to be confused on the usage.
>
> All right, but in this case you need to update
> docs/misc/xen-command-line.markdown.

Sure, I will do it in the next version.

Cheers,
Julien Grall March 3, 2017, 6:35 p.m. | #5
Hi Stefano,

On 20/02/17 18:13, Julien Grall wrote:
> On 20/02/17 18:12, Stefano Stabellini wrote:
>> On Mon, 20 Feb 2017, Julien Grall wrote:
>>> Hi Stefano,
>>>
>>> On 02/16/2017 01:41 AM, Stefano Stabellini wrote:
>>>> On Fri, 3 Feb 2017, Julien Grall wrote:
>>>>> On ARM, when the user put 'acpi=force' Xen will use ACPI over DT.
>>>>> Currently, if Xen fails to initialize ACPI it will fallback on DT.
>>>>>
>>>>> This behavior makes difficult for a user to notice Xen didn't used
>>>>> ACPI
>>>>> has requested on platform where the firmware is providing both ACPI
>>>>> and
>>>>> DT.
>>>>>
>>>>> Rather than fallback on DT during a failure, panic when 'acpi=force'.
>>>>>
>>>>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>>>>
>>>>> ---
>>>>>     I am wondering if we should do the same with acpi=on. So a user
>>>>>     would notice directly if something went wrong with ACPI.
>>>>>     Otherwise you would boot up to the prompt and barely notice
>>>>> that DT
>>>>>     was used.
>>>>
>>>> I would keep the current behavior as is and add new parameter that
>>>> means
>>>> "acpi and only acpi". Today acpi=force means "acpi is preferred to
>>>> device tree". It doesn't mean that if acpi fail, Xen should panic.
>>>>
>>>> Maybe acpi=strict or acpi=mandatory?
>>>
>>> We have a bunch of option that makes little sense today:
>>>    - off: Turned off ACPI
>>>    - on: Turned on ACPI only if DT is empty
>>>    - force: Turned on ACPI
>>>
>>> If ACPI fails you fallback on DT. This is a pain to detect whether DT
>>> or ACPI
>>> is been used. So I think adding another option will only confuse the
>>> user.
>>>
>>> Looking at Linux the option are:
>>>     - off: Turned off ACPI
>>>     - on: Prefer ACPI over DT. Fallback on DT is failed
>>>     - force: Use ACPI and panic if not available
>>>
>>> I would much prefer to use the behavior of the acpi param on Linux
>>> because it
>>> will avoid the user to be confused on the usage.
>>
>> All right, but in this case you need to update
>> docs/misc/xen-command-line.markdown.
>
> Sure, I will do it in the next version.

Actually, I will defer this patch a bit and resend the rest of the series.

Cheers,

Patch hide | download patch | download mbox

diff --git a/xen/arch/arm/acpi/boot.c b/xen/arch/arm/acpi/boot.c
index 889208a..65ef632 100644
--- a/xen/arch/arm/acpi/boot.c
+++ b/xen/arch/arm/acpi/boot.c
@@ -272,6 +272,11 @@  int __init acpi_boot_table_init(void)
     return 0;
 
 disable:
+
+    /* Panic if the user has requested ACPI but Xen is able to initialize. */
+    if ( param_acpi_force )
+        panic("Unable to use ACPI");
+
     disable_acpi();
 
     return error;