[PATCH v4 2/8] powerpc: Introduce FW_FEATURE_ULTRAVISOR

[Michael Ellerman]

Claudio Carvalho <cclaudio at linux.ibm.com> writes:
> diff --git a/arch/powerpc/include/asm/ultravisor.h b/arch/powerpc/include/asm/ultravisor.h
> new file mode 100644
> index 000000000000..e5009b0d84ea
> --- /dev/null
> +++ b/arch/powerpc/include/asm/ultravisor.h
> @@ -0,0 +1,15 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Ultravisor definitions
> + *
> + * Copyright 2019, IBM Corporation.
> + *
> + */
> +#ifndef _ASM_POWERPC_ULTRAVISOR_H
> +#define _ASM_POWERPC_ULTRAVISOR_H
> +
> +/* Internal functions */
> +extern int early_init_dt_scan_ultravisor(unsigned long node, const char *uname,
> +					 int depth, void *data);

Please don't use extern in new headers.

> diff --git a/arch/powerpc/kernel/ultravisor.c b/arch/powerpc/kernel/ultravisor.c
> new file mode 100644
> index 000000000000..dc6021f63c97
> --- /dev/null
> +++ b/arch/powerpc/kernel/ultravisor.c

Is there a reason this (and other later files) aren't in platforms/powernv ?

> @@ -0,0 +1,24 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Ultravisor high level interfaces
> + *
> + * Copyright 2019, IBM Corporation.
> + *
> + */
> +#include <linux/init.h>
> +#include <linux/printk.h>
> +#include <linux/string.h>
> +
> +#include <asm/ultravisor.h>
> +#include <asm/firmware.h>
> +
> +int __init early_init_dt_scan_ultravisor(unsigned long node, const char *uname,
> +					 int depth, void *data)
> +{
> +	if (depth != 1 || strcmp(uname, "ibm,ultravisor") != 0)
> +		return 0;

I know you're following the example of OPAL, but this is not the best
way to search for the ultravisor node.

It makes the location and name of the node part of the ABI, when there's
no need for it to be.

If instead you just scan the tree for a node that is *compatible* with
"ibm,ultravisor" (or whatever compatible string) then the node can be
placed any where in the tree and have any name, which gives us the most
flexibility in future to change the location of the device tree node.

cheers