linux-kernel - Re: [PATCH v7 12/17] ARM64: ACPI: Check if it runs on Xen to enable or disable ACPI

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 31 Mar 2016 12:04:42 +0100
From:	Stefano Stabellini <stefano.stabellini@...citrix.com>
To:	Shannon Zhao <zhaoshenglong@...wei.com>
CC:	Mark Rutland <mark.rutland@....com>,
	Will Deacon <will.deacon@....com>,
	Shannon Zhao <shannon.zhao@...aro.org>,
	<linux-arm-kernel@...ts.infradead.org>,
	<stefano.stabellini@...rix.com>, <david.vrabel@...rix.com>,
	<devicetree@...r.kernel.org>, <linux-efi@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, <catalin.marinas@....com>,
	<julien.grall@....com>, <peter.huangpeng@...wei.com>,
	<xen-devel@...ts.xen.org>
Subject: Re: [PATCH v7 12/17] ARM64: ACPI: Check if it runs on Xen to enable
 or disable ACPI

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@...aro.org>
> >>> > > Reviewed-by: Stefano Stabellini <stefano.stabellini@...citrix.com>
> >>> > > Acked-by: Hanjun Guo <hanjun.guo@...aro.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.