On 7/11/19 9:57 AM, Michael Ellerman wrote:
> Claudio Carvalho <cclaudio@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 ?

Yes, there is.
https://www.spinics.net/lists/kvm-ppc/msg14998.html

We also need to do ucalls from a secure guest and its kernel may not have CONFIG_PPC_POWERNV=y. I can make it clear in the commit message.


>
>> @@ -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.

I will do that in the next version.

Thanks,
Claudio


>
> cheers
>