diff mbox

[v7,12/17] ARM64: ACPI: Check if it runs on Xen to enable or disable ACPI

Message ID 1458830676-27075-13-git-send-email-shannon.zhao@linaro.org (mailing list archive)
State New, archived
Headers show

Commit Message

Shannon Zhao March 24, 2016, 2:44 p.m. UTC 
When it's a Xen domain0 booting with ACPI, it will supply a /chosen and
a /hypervisor node in DT. So check if it needs to enable ACPI.

Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Hanjun Guo <hanjun.guo@linaro.org>
---
 arch/arm64/kernel/acpi.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

Comments

Stefano Stabellini March 26, 2016, 12:56 p.m. UTC | #1 
Will, Catalin,

are you OK with this change?
The series is almost ready to go in, I would like to tie up the loose ends.

Thanks,

Stefano

On Thu, 24 Mar 2016, Shannon Zhao wrote:
> When it's a Xen domain0 booting with ACPI, it will supply a /chosen and
> a /hypervisor node in DT. So check if it needs to enable ACPI.
> 
> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Acked-by: Hanjun Guo <hanjun.guo@linaro.org>
> ---
>  arch/arm64/kernel/acpi.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
> index d1ce8e2..4e92be0 100644
> --- a/arch/arm64/kernel/acpi.c
> +++ b/arch/arm64/kernel/acpi.c
> @@ -67,10 +67,13 @@ static int __init dt_scan_depth1_nodes(unsigned long node,
>  {
>  	/*
>  	 * Return 1 as soon as we encounter a node at depth 1 that is
> -	 * not the /chosen node.
> +	 * not the /chosen node, or /hypervisor node when running on Xen.
>  	 */
> -	if (depth == 1 && (strcmp(uname, "chosen") != 0))
> -		return 1;
> +	if (depth == 1 && (strcmp(uname, "chosen") != 0)) {
> +		if (!xen_initial_domain() || (strcmp(uname, "hypervisor") != 0))
> +			return 1;
> +	}
> +
>  	return 0;
>  }
>  
> @@ -184,7 +187,8 @@ void __init acpi_boot_table_init(void)
>  	/*
>  	 * Enable ACPI instead of device tree unless
>  	 * - ACPI has been disabled explicitly (acpi=off), or
> -	 * - the device tree is not empty (it has more than just a /chosen node)
> +	 * - the device tree is not empty (it has more than just a /chosen node,
> +	 *   and a /hypervisor node when running on Xen)
>  	 *   and ACPI has not been force enabled (acpi=force)
>  	 */
>  	if (param_acpi_off ||
> -- 
> 2.1.4
>
Will Deacon March 29, 2016, 4:18 p.m. UTC | #2 
On Thu, Mar 24, 2016 at 10:44:31PM +0800, Shannon Zhao wrote:
> When it's a Xen domain0 booting with ACPI, it will supply a /chosen and
> a /hypervisor node in DT. So check if it needs to enable ACPI.
> 
> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Acked-by: Hanjun Guo <hanjun.guo@linaro.org>
> ---
>  arch/arm64/kernel/acpi.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
> index d1ce8e2..4e92be0 100644
> --- a/arch/arm64/kernel/acpi.c
> +++ b/arch/arm64/kernel/acpi.c
> @@ -67,10 +67,13 @@ static int __init dt_scan_depth1_nodes(unsigned long node,
>  {
>  	/*
>  	 * Return 1 as soon as we encounter a node at depth 1 that is
> -	 * not the /chosen node.
> +	 * not the /chosen node, or /hypervisor node when running on Xen.
>  	 */
> -	if (depth == 1 && (strcmp(uname, "chosen") != 0))
> -		return 1;
> +	if (depth == 1 && (strcmp(uname, "chosen") != 0)) {
> +		if (!xen_initial_domain() || (strcmp(uname, "hypervisor") != 0))
> +			return 1;
> +	}

Hmm, but xen_initial_domain() is false when xen isn't being used at all,
so it feels to me like this is a bit too far-reaching and is basically
claiming the "/hypervisor" namespace for Xen. Couldn't it be renamed to
"xen,hypervisor" or something?

Mark, got any thoughts on this?

Will
Mark Rutland March 29, 2016, 4:31 p.m. UTC | #3 
On Tue, Mar 29, 2016 at 05:18:38PM +0100, Will Deacon wrote:
> On Thu, Mar 24, 2016 at 10:44:31PM +0800, Shannon Zhao wrote:
> > When it's a Xen domain0 booting with ACPI, it will supply a /chosen and
> > a /hypervisor node in DT. So check if it needs to enable ACPI.
> > 
> > Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> > Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > Acked-by: Hanjun Guo <hanjun.guo@linaro.org>
> > ---
> >  arch/arm64/kernel/acpi.c | 12 ++++++++----
> >  1 file changed, 8 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
> > index d1ce8e2..4e92be0 100644
> > --- a/arch/arm64/kernel/acpi.c
> > +++ b/arch/arm64/kernel/acpi.c
> > @@ -67,10 +67,13 @@ static int __init dt_scan_depth1_nodes(unsigned long node,
> >  {
> >  	/*
> >  	 * Return 1 as soon as we encounter a node at depth 1 that is
> > -	 * not the /chosen node.
> > +	 * not the /chosen node, or /hypervisor node when running on Xen.
> >  	 */
> > -	if (depth == 1 && (strcmp(uname, "chosen") != 0))
> > -		return 1;
> > +	if (depth == 1 && (strcmp(uname, "chosen") != 0)) {
> > +		if (!xen_initial_domain() || (strcmp(uname, "hypervisor") != 0))
> > +			return 1;
> > +	}
> 
> Hmm, but xen_initial_domain() is false when xen isn't being used at all,
> so it feels to me like this is a bit too far-reaching and is basically
> claiming the "/hypervisor" namespace for Xen. Couldn't it be renamed to
> "xen,hypervisor" or something?
> 
> Mark, got any thoughts on this?

The node has a compatible string, "xen,xen" per [1], which would tell us
absolutely that xen is present. I'd be happy checking for that
explicitly.

In patch 11 fdt_find_hyper_node checks the compatible string. We could
factor that out into a helper like is_xen_node(node) and use it here
too.

If in future we want to do the same for other hypervisors we can add
explicit checks for their compatible strings and/or wrap that in a more
generic helper to cater for any hypervisor-specific details.

Thanks,
Mark.

[1] Documentation/devicetree/bindings/arm/xen.txt
Shannon Zhao March 30, 2016, 7:19 a.m. UTC | #4 
Hi Will, Mark,

On 2016/3/30 0:31, Mark Rutland wrote:
> On Tue, Mar 29, 2016 at 05:18:38PM +0100, Will Deacon wrote:
>> > On Thu, Mar 24, 2016 at 10:44:31PM +0800, Shannon Zhao wrote:
>>> > > When it's a Xen domain0 booting with ACPI, it will supply a /chosen and
>>> > > a /hypervisor node in DT. So check if it needs to enable ACPI.
>>> > > 
>>> > > Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
>>> > > Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>>> > > Acked-by: Hanjun Guo <hanjun.guo@linaro.org>
>>> > > ---
>>> > >  arch/arm64/kernel/acpi.c | 12 ++++++++----
>>> > >  1 file changed, 8 insertions(+), 4 deletions(-)
>>> > > 
>>> > > diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
>>> > > index d1ce8e2..4e92be0 100644
>>> > > --- a/arch/arm64/kernel/acpi.c
>>> > > +++ b/arch/arm64/kernel/acpi.c
>>> > > @@ -67,10 +67,13 @@ static int __init dt_scan_depth1_nodes(unsigned long node,
>>> > >  {
>>> > >  	/*
>>> > >  	 * Return 1 as soon as we encounter a node at depth 1 that is
>>> > > -	 * not the /chosen node.
>>> > > +	 * not the /chosen node, or /hypervisor node when running on Xen.
>>> > >  	 */
>>> > > -	if (depth == 1 && (strcmp(uname, "chosen") != 0))
>>> > > -		return 1;
>>> > > +	if (depth == 1 && (strcmp(uname, "chosen") != 0)) {
>>> > > +		if (!xen_initial_domain() || (strcmp(uname, "hypervisor") != 0))
>>> > > +			return 1;
>>> > > +	}
>> > 
>> > Hmm, but xen_initial_domain() is false when xen isn't being used at all,
>> > so it feels to me like this is a bit too far-reaching and is basically
>> > claiming the "/hypervisor" namespace for Xen. Couldn't it be renamed to
>> > "xen,hypervisor" or something?
>> > 
>> > Mark, got any thoughts on this?
> The node has a compatible string, "xen,xen" per [1], which would tell us
> absolutely that xen is present. I'd be happy checking for that
> explicitly.
> 
I think actually the xen_initial_domain is the result of the
fdt_find_hyper_node. If the compatible string "xen,xen" doesn't exist,
the xen_initial_domain() will return false and whatever the current node
is the above check will return 1 since the device tree is not empty.

> In patch 11 fdt_find_hyper_node checks the compatible string. We could
> factor that out into a helper like is_xen_node(node) and use it here
> too.
> 
I don't think so because we already check the compatible string before
and we could get the result simply via xen_initial_domain().

Thanks,
Stefano Stabellini March 31, 2016, 11:04 a.m. UTC | #5 
On Wed, 30 Mar 2016, Shannon Zhao wrote:
> Hi Will, Mark,
> 
> On 2016/3/30 0:31, Mark Rutland wrote:
> > On Tue, Mar 29, 2016 at 05:18:38PM +0100, Will Deacon wrote:
> >> > On Thu, Mar 24, 2016 at 10:44:31PM +0800, Shannon Zhao wrote:
> >>> > > When it's a Xen domain0 booting with ACPI, it will supply a /chosen and
> >>> > > a /hypervisor node in DT. So check if it needs to enable ACPI.
> >>> > > 
> >>> > > Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> >>> > > Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> >>> > > Acked-by: Hanjun Guo <hanjun.guo@linaro.org>
> >>> > > ---
> >>> > >  arch/arm64/kernel/acpi.c | 12 ++++++++----
> >>> > >  1 file changed, 8 insertions(+), 4 deletions(-)
> >>> > > 
> >>> > > diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
> >>> > > index d1ce8e2..4e92be0 100644
> >>> > > --- a/arch/arm64/kernel/acpi.c
> >>> > > +++ b/arch/arm64/kernel/acpi.c
> >>> > > @@ -67,10 +67,13 @@ static int __init dt_scan_depth1_nodes(unsigned long node,
> >>> > >  {
> >>> > >  	/*
> >>> > >  	 * Return 1 as soon as we encounter a node at depth 1 that is
> >>> > > -	 * not the /chosen node.
> >>> > > +	 * not the /chosen node, or /hypervisor node when running on Xen.
> >>> > >  	 */
> >>> > > -	if (depth == 1 && (strcmp(uname, "chosen") != 0))
> >>> > > -		return 1;
> >>> > > +	if (depth == 1 && (strcmp(uname, "chosen") != 0)) {
> >>> > > +		if (!xen_initial_domain() || (strcmp(uname, "hypervisor") != 0))
> >>> > > +			return 1;
> >>> > > +	}
> >> > 
> >> > Hmm, but xen_initial_domain() is false when xen isn't being used at all,
> >> > so it feels to me like this is a bit too far-reaching and is basically
> >> > claiming the "/hypervisor" namespace for Xen. Couldn't it be renamed to
> >> > "xen,hypervisor" or something?
> >> > 
> >> > Mark, got any thoughts on this?
> > The node has a compatible string, "xen,xen" per [1], which would tell us
> > absolutely that xen is present. I'd be happy checking for that
> > explicitly.
> > 
> I think actually the xen_initial_domain is the result of the
> fdt_find_hyper_node. If the compatible string "xen,xen" doesn't exist,
> the xen_initial_domain() will return false and whatever the current node
> is the above check will return 1 since the device tree is not empty.

Right.

xen_initial_domain implies both "xen,xen" and XENFEAT_dom0 (which is a
feature retrieved by making a XENVER_get_features hypercall, see
drivers/xen/features.c:xen_setup_features).

So the following check:

 +     if (!xen_initial_domain() || (strcmp(uname, "hypervisor") != 0))
 +         return 1;

means that even if it's xen_initial_domain(), return error unless the
node found is "hypervisor". In other words, even if
xen_initial_domain(), no other nodes are allowed except /chosen and
/hypervisor.

This doesn't look far reaching to me, but yes, we could check explicitly
for the node to be compatible "xen,xen", in addition to be named
"hypervisor", even though the check is already done elsewhere
(arch/arm/xen/enlighten.c).

But I would keep it as it is.

> > In patch 11 fdt_find_hyper_node checks the compatible string. We could
> > factor that out into a helper like is_xen_node(node) and use it here
> > too.
> > 
> I don't think so because we already check the compatible string before
> and we could get the result simply via xen_initial_domain().

We could add a comment saying xen_initial_domain() implies "xen,xen" or
something.
diff mbox

Patch

diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
index d1ce8e2..4e92be0 100644
--- a/arch/arm64/kernel/acpi.c
+++ b/arch/arm64/kernel/acpi.c
@@ -67,10 +67,13 @@  static int __init dt_scan_depth1_nodes(unsigned long node,
 {
 	/*
 	 * Return 1 as soon as we encounter a node at depth 1 that is
-	 * not the /chosen node.
+	 * not the /chosen node, or /hypervisor node when running on Xen.
 	 */
-	if (depth == 1 && (strcmp(uname, "chosen") != 0))
-		return 1;
+	if (depth == 1 && (strcmp(uname, "chosen") != 0)) {
+		if (!xen_initial_domain() || (strcmp(uname, "hypervisor") != 0))
+			return 1;
+	}
+
 	return 0;
 }
 
@@ -184,7 +187,8 @@  void __init acpi_boot_table_init(void)
 	/*
 	 * Enable ACPI instead of device tree unless
 	 * - ACPI has been disabled explicitly (acpi=off), or
-	 * - the device tree is not empty (it has more than just a /chosen node)
+	 * - the device tree is not empty (it has more than just a /chosen node,
+	 *   and a /hypervisor node when running on Xen)
 	 *   and ACPI has not been force enabled (acpi=force)
 	 */
 	if (param_acpi_off ||