diff mbox series

[v2,1/6] arm64/hyperv: Support DeviceTree

Message ID 20240514224508.212318-2-romank@linux.microsoft.com
State New
Headers show
Series arm64/hyperv: Support Virtual Trust Level Boot | expand

Commit Message

Roman Kisel May 14, 2024, 10:43 p.m. UTC
The Virtual Trust Level platforms rely on DeviceTree, and the
arm64/hyperv code supports ACPI only. Update the logic to
support DeviceTree on boot as well as ACPI.

Signed-off-by: Roman Kisel <romank@linux.microsoft.com>
---
 arch/arm64/hyperv/mshyperv.c | 34 +++++++++++++++++++++++++++++-----
 1 file changed, 29 insertions(+), 5 deletions(-)

Comments

Roman Kisel May 15, 2024, 5:33 p.m. UTC | #1
On 5/15/2024 12:45 AM, Krzysztof Kozlowski wrote:
> On 15/05/2024 00:43, Roman Kisel wrote:
>> The Virtual Trust Level platforms rely on DeviceTree, and the
>> arm64/hyperv code supports ACPI only. Update the logic to
>> support DeviceTree on boot as well as ACPI.
>>
>> Signed-off-by: Roman Kisel <romank@linux.microsoft.com>
>> ---
>>   arch/arm64/hyperv/mshyperv.c | 34 +++++++++++++++++++++++++++++-----
>>   1 file changed, 29 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/arm64/hyperv/mshyperv.c b/arch/arm64/hyperv/mshyperv.c
>> index b1a4de4eee29..208a3bcb9686 100644
>> --- a/arch/arm64/hyperv/mshyperv.c
>> +++ b/arch/arm64/hyperv/mshyperv.c
>> @@ -15,6 +15,9 @@
>>   #include <linux/errno.h>
>>   #include <linux/version.h>
>>   #include <linux/cpuhotplug.h>
>> +#include <linux/libfdt.h>
>> +#include <linux/of.h>
>> +#include <linux/of_fdt.h>
>>   #include <asm/mshyperv.h>
>>   
>>   static bool hyperv_initialized;
>> @@ -27,6 +30,29 @@ int hv_get_hypervisor_version(union hv_hypervisor_version_info *info)
>>   	return 0;
>>   }
>>   
>> +static bool hyperv_detect_fdt(void)
>> +{
>> +#ifdef CONFIG_OF
>> +	const unsigned long hyp_node = of_get_flat_dt_subnode_by_name(
>> +			of_get_flat_dt_root(), "hypervisor");
> 
> Why do you add an ABI for node name? Although name looks OK, but is it
> really described in the spec that you depend on it? I really do not like
> name dependencies...

Followed the existing DeviceTree's of naming and approaches in the 
kernel to surprise less and "invent" even less. As for the spec, the 
Hyper-V TLFS'es part discussing Hypervisor Discovery talks about x86 
(https://learn.microsoft.com/en-us/virtualization/hyper-v-on-windows/tlfs/feature-discovery) 
only and via the ISA-provided means only. For arm64, Hyper-V code 
discovers the hypervisor presence via ACPI. Felt only natural to do the 
same for DeviceTree and arm64.

> 
> Where is the binding for this?
> 
Have not added, my mistake. Will place under 
Documentation/devicetree/bindings/bus/microsoft,hyperv.yaml

> Best regards,
> Krzysztof
Krzysztof Kozlowski May 20, 2024, 6:45 a.m. UTC | #2
On 15/05/2024 19:33, Roman Kisel wrote:
>>>   static bool hyperv_initialized;
>>> @@ -27,6 +30,29 @@ int hv_get_hypervisor_version(union hv_hypervisor_version_info *info)
>>>   	return 0;
>>>   }
>>>   
>>> +static bool hyperv_detect_fdt(void)
>>> +{
>>> +#ifdef CONFIG_OF
>>> +	const unsigned long hyp_node = of_get_flat_dt_subnode_by_name(
>>> +			of_get_flat_dt_root(), "hypervisor");
>>
>> Why do you add an ABI for node name? Although name looks OK, but is it
>> really described in the spec that you depend on it? I really do not like
>> name dependencies...
> 
> Followed the existing DeviceTree's of naming and approaches in the 
> kernel to surprise less and "invent" even less. As for the spec, the 

I am sorry, but there is no approved existing approach of adding ABI for
node names. There are exceptions or specific cases, but that's not
"invent less" approach. ABI is defined by compatible.



Best regards,
Krzysztof
Roman Kisel May 20, 2024, 8:36 p.m. UTC | #3
On 5/19/2024 11:45 PM, Krzysztof Kozlowski wrote:
> On 15/05/2024 19:33, Roman Kisel wrote:
>>>>    static bool hyperv_initialized;
>>>> @@ -27,6 +30,29 @@ int hv_get_hypervisor_version(union hv_hypervisor_version_info *info)
>>>>    	return 0;
>>>>    }
>>>>    
>>>> +static bool hyperv_detect_fdt(void)
>>>> +{
>>>> +#ifdef CONFIG_OF
>>>> +	const unsigned long hyp_node = of_get_flat_dt_subnode_by_name(
>>>> +			of_get_flat_dt_root(), "hypervisor");
>>>
>>> Why do you add an ABI for node name? Although name looks OK, but is it
>>> really described in the spec that you depend on it? I really do not like
>>> name dependencies...
>>
>> Followed the existing DeviceTree's of naming and approaches in the
>> kernel to surprise less and "invent" even less. As for the spec, the
> 
> I am sorry, but there is no approved existing approach of adding ABI for
> node names. There are exceptions or specific cases, but that's not
> "invent less" approach. ABI is defined by compatible.
I should check on the compatible instead of adding a node that is not 
mentioned in the DeviceTree spec as I understand. Appreciate your help!

> 
> Best regards,
> Krzysztof
>
Roman Kisel June 11, 2024, 2:55 p.m. UTC | #4
On 5/16/2024 8:27 AM, Roman Kisel wrote:
> 
> 
> On 5/15/2024 3:02 PM, Elliot Berman wrote:
>> On Tue, May 14, 2024 at 03:43:48PM -0700, Roman Kisel wrote:
>>> The Virtual Trust Level platforms rely on DeviceTree, and the
>>> arm64/hyperv code supports ACPI only. Update the logic to
>>> support DeviceTree on boot as well as ACPI.
>>
>> Could you use Call UID query from SMCCC? KVM [1] and Gunyah [2] have
>> been using this to identify if guest is running under those respective
>> hypervisors. This works in both DT and ACPI cases.
>>
>> [1]: https://lore.kernel.org/all/20210330145430.996981-2-maz@kernel.org/
>> [2]: 
>> https://lore.kernel.org/all/20240222-gunyah-v17-4-1e9da6763d38@quicinc.com/
> 
> That would be very neat indeed, thanks! Talking to the hypervisor folks.
> 
We have that now. Will send out the revised patches sometime during the 
next week most likely.

>>>
>>> Signed-off-by: Roman Kisel <romank@linux.microsoft.com>
>>> ---
>>>   arch/arm64/hyperv/mshyperv.c | 34 +++++++++++++++++++++++++++++-----
>>>   1 file changed, 29 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/arch/arm64/hyperv/mshyperv.c b/arch/arm64/hyperv/mshyperv.c
>>> index b1a4de4eee29..208a3bcb9686 100644
>>> --- a/arch/arm64/hyperv/mshyperv.c
>>> +++ b/arch/arm64/hyperv/mshyperv.c
>>> @@ -15,6 +15,9 @@
>>>   #include <linux/errno.h>
>>>   #include <linux/version.h>
>>>   #include <linux/cpuhotplug.h>
>>> +#include <linux/libfdt.h>
>>> +#include <linux/of.h>
>>> +#include <linux/of_fdt.h>
>>>   #include <asm/mshyperv.h>
>>>   static bool hyperv_initialized;
>>> @@ -27,6 +30,29 @@ int hv_get_hypervisor_version(union 
>>> hv_hypervisor_version_info *info)
>>>       return 0;
>>>   }
>>> +static bool hyperv_detect_fdt(void)
>>> +{
>>> +#ifdef CONFIG_OF
>>> +    const unsigned long hyp_node = of_get_flat_dt_subnode_by_name(
>>> +            of_get_flat_dt_root(), "hypervisor");
>>> +
>>> +    return (hyp_node != -FDT_ERR_NOTFOUND) &&
>>> +            of_flat_dt_is_compatible(hyp_node, "microsoft,hyperv");
>>> +#else
>>> +    return false;
>>> +#endif
>>> +}
>>> +
>>> +static bool hyperv_detect_acpi(void)
>>> +{
>>> +#ifdef CONFIG_ACPI
>>> +    return !acpi_disabled &&
>>> +            !strncmp((char *)&acpi_gbl_FADT.hypervisor_id, 
>>> "MsHyperV", 8);
>>> +#else
>>> +    return false;
>>> +#endif
>>> +}
>>> +
>>>   static int __init hyperv_init(void)
>>>   {
>>>       struct hv_get_vp_registers_output    result;
>>> @@ -35,13 +61,11 @@ static int __init hyperv_init(void)
>>>       /*
>>>        * Allow for a kernel built with CONFIG_HYPERV to be running in
>>> -     * a non-Hyper-V environment, including on DT instead of ACPI.
>>> +     * a non-Hyper-V environment.
>>> +     *
>>>        * In such cases, do nothing and return success.
>>>        */
>>> -    if (acpi_disabled)
>>> -        return 0;
>>> -
>>> -    if (strncmp((char *)&acpi_gbl_FADT.hypervisor_id, "MsHyperV", 8))
>>> +    if (!hyperv_detect_fdt() && !hyperv_detect_acpi())
>>>           return 0;
>>>       /* Setup the guest ID */
>>> -- 
>>> 2.45.0
>>>
>>>
>>> _______________________________________________
>>> linux-arm-kernel mailing list
>>> linux-arm-kernel@lists.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
diff mbox series

Patch

diff --git a/arch/arm64/hyperv/mshyperv.c b/arch/arm64/hyperv/mshyperv.c
index b1a4de4eee29..208a3bcb9686 100644
--- a/arch/arm64/hyperv/mshyperv.c
+++ b/arch/arm64/hyperv/mshyperv.c
@@ -15,6 +15,9 @@ 
 #include <linux/errno.h>
 #include <linux/version.h>
 #include <linux/cpuhotplug.h>
+#include <linux/libfdt.h>
+#include <linux/of.h>
+#include <linux/of_fdt.h>
 #include <asm/mshyperv.h>
 
 static bool hyperv_initialized;
@@ -27,6 +30,29 @@  int hv_get_hypervisor_version(union hv_hypervisor_version_info *info)
 	return 0;
 }
 
+static bool hyperv_detect_fdt(void)
+{
+#ifdef CONFIG_OF
+	const unsigned long hyp_node = of_get_flat_dt_subnode_by_name(
+			of_get_flat_dt_root(), "hypervisor");
+
+	return (hyp_node != -FDT_ERR_NOTFOUND) &&
+			of_flat_dt_is_compatible(hyp_node, "microsoft,hyperv");
+#else
+	return false;
+#endif
+}
+
+static bool hyperv_detect_acpi(void)
+{
+#ifdef CONFIG_ACPI
+	return !acpi_disabled &&
+			!strncmp((char *)&acpi_gbl_FADT.hypervisor_id, "MsHyperV", 8);
+#else
+	return false;
+#endif
+}
+
 static int __init hyperv_init(void)
 {
 	struct hv_get_vp_registers_output	result;
@@ -35,13 +61,11 @@  static int __init hyperv_init(void)
 
 	/*
 	 * Allow for a kernel built with CONFIG_HYPERV to be running in
-	 * a non-Hyper-V environment, including on DT instead of ACPI.
+	 * a non-Hyper-V environment.
+	 *
 	 * In such cases, do nothing and return success.
 	 */
-	if (acpi_disabled)
-		return 0;
-
-	if (strncmp((char *)&acpi_gbl_FADT.hypervisor_id, "MsHyperV", 8))
+	if (!hyperv_detect_fdt() && !hyperv_detect_acpi())
 		return 0;
 
 	/* Setup the guest ID */