diff mbox series

[1/6] arm64/hyperv: Support DeviceTree

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

Commit Message

Roman Kisel May 10, 2024, 4:05 p.m. UTC
From: Roman Kisel <romank@linux.microsoft.com>

Update the driver to support DeviceTree boot as well along with ACPI.
This enables the Virtual Trust Level platforms boot up on ARM64.

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

Comments

Easwar Hariharan May 14, 2024, 10:46 p.m. UTC | #1
On 5/10/2024 10:42 AM, Roman Kisel wrote:
> 
> 
> On 5/10/2024 10:04 AM, Easwar Hariharan wrote:
>> On 5/10/2024 9:05 AM, romank@linux.microsoft.com wrote:
>>> From: Roman Kisel <romank@linux.microsoft.com>
>>>
>>> Update the driver to support DeviceTree boot as well along with ACPI.
>>> This enables the Virtual Trust Level platforms boot up on ARM64.
>>>
>>> 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
>>> +}
>>> +
>>
>> Could using IS_ENABLED() allow collapsing these two functions into one hyperv_detect_fw()?
>> I am wondering if #ifdef was explicitly chosen to allow for the code to be compiled in if CONFIG* is defined
>> v/s IS_ENABLED() only being true if the CONFIG value is true.
>>
> In the hyperv_detect_fdt function, the #ifdef has been chosen due to the functions being declared only when the macro is defined, hence I could not rely on `if IS_ENABLED()` and the run-time detection. For the compile-time option, `#if IS_ENABLED()` would convey the intent better, could update with that.

In patch 2/6, just under the diff you have, we already `select OF_EARLY_FLATTREE if OF`, so the declarations (and definitions)
of the functions being present is already handled, AFAIK. Are we thinking there may be some weird config where neither OF nor
ACPI is selected? If so, we should set a `depends on ACPI || OF` for config HYPERV to prevent that.

I don't know if I'm missing something obvious here, so please correct me if I'm wrong.

> 
> In the hyperv_detect_acpi function, using of the #ifdef appears to be not needed, will remove that in the next version of the patch set. Appreciate your help!
> 
> As for combining the functions into one, the result looks less readable due to more if statements and is mixing the concerns to an extent. That said, unless you feel strongly about having just one function here, perhaps could keep the both functions.

Agreed on the readability, let's keep both functions.

Thanks,
Easwar
Roman Kisel May 14, 2024, 11:17 p.m. UTC | #2
On 5/14/2024 3:46 PM, Easwar Hariharan wrote:
> On 5/10/2024 10:42 AM, Roman Kisel wrote:
>>
>>
>> On 5/10/2024 10:04 AM, Easwar Hariharan wrote:
>>> On 5/10/2024 9:05 AM, romank@linux.microsoft.com wrote:
>>>> From: Roman Kisel <romank@linux.microsoft.com>
>>>>
>>>> Update the driver to support DeviceTree boot as well along with ACPI.
>>>> This enables the Virtual Trust Level platforms boot up on ARM64.
>>>>
>>>> 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
>>>> +}
>>>> +
>>>
>>> Could using IS_ENABLED() allow collapsing these two functions into one hyperv_detect_fw()?
>>> I am wondering if #ifdef was explicitly chosen to allow for the code to be compiled in if CONFIG* is defined
>>> v/s IS_ENABLED() only being true if the CONFIG value is true.
>>>
>> In the hyperv_detect_fdt function, the #ifdef has been chosen due to the functions being declared only when the macro is defined, hence I could not rely on `if IS_ENABLED()` and the run-time detection. For the compile-time option, `#if IS_ENABLED()` would convey the intent better, could update with that.
> 
> In patch 2/6, just under the diff you have, we already `select OF_EARLY_FLATTREE if OF`, so the declarations (and definitions)
> of the functions being present is already handled, AFAIK. Are we thinking there may be some weird config where neither OF nor
> ACPI is selected? If so, we should set a `depends on ACPI || OF` for config HYPERV to prevent that.
> 
> I don't know if I'm missing something obvious here, so please correct me if I'm wrong.
> 
I just sent out the v2 of the patch set, and (un?)fortunately missed the 
change I had for the #ifdef's in this chunk (to use #if IS_ENABLED() and 
remove pre-processor directives from the ACPI-related function).

I am making the point that the change you are suggesting (as I 
understand) is this conditional statement

if IS_ENABLED(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");
}

and for it to link successfully, one needs 
of_get_flat_dt_subnode_by_name defined. From the source code, that needs 
CONFIG_OF_EARLY_FLATTREE as there is no stub available when 
CONFIG_OF_EARLY_FLATTREE is not defined. I'll check on successful 
linking with the config without CONFIG_OF and the above snippet. Do feel 
free to provide the code you have in mind.

>>
>> In the hyperv_detect_acpi function, using of the #ifdef appears to be not needed, will remove that in the next version of the patch set. Appreciate your help!
>>
>> As for combining the functions into one, the result looks less readable due to more if statements and is mixing the concerns to an extent. That said, unless you feel strongly about having just one function here, perhaps could keep the both functions.
> 
> Agreed on the readability, let's keep both functions.
> 
> Thanks,
> Easwar
Easwar Hariharan May 15, 2024, midnight UTC | #3
On 5/14/2024 4:17 PM, Roman Kisel wrote:
> 
> 
> On 5/14/2024 3:46 PM, Easwar Hariharan wrote:
>> On 5/10/2024 10:42 AM, Roman Kisel wrote:
>>>
>>>
>>> On 5/10/2024 10:04 AM, Easwar Hariharan wrote:
>>>> On 5/10/2024 9:05 AM, romank@linux.microsoft.com wrote:
>>>>> From: Roman Kisel <romank@linux.microsoft.com>
>>>>>
>>>>> Update the driver to support DeviceTree boot as well along with ACPI.
>>>>> This enables the Virtual Trust Level platforms boot up on ARM64.
>>>>>
>>>>> 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
>>>>> +}
>>>>> +
>>>>
>>>> Could using IS_ENABLED() allow collapsing these two functions into one hyperv_detect_fw()?
>>>> I am wondering if #ifdef was explicitly chosen to allow for the code to be compiled in if CONFIG* is defined
>>>> v/s IS_ENABLED() only being true if the CONFIG value is true.
>>>>
>>> In the hyperv_detect_fdt function, the #ifdef has been chosen due to the functions being declared only when the macro is defined, hence I could not rely on `if IS_ENABLED()` and the run-time detection. For the compile-time option, `#if IS_ENABLED()` would convey the intent better, could update with that.
>>
>> In patch 2/6, just under the diff you have, we already `select OF_EARLY_FLATTREE if OF`, so the declarations (and definitions)
>> of the functions being present is already handled, AFAIK. Are we thinking there may be some weird config where neither OF nor
>> ACPI is selected? If so, we should set a `depends on ACPI || OF` for config HYPERV to prevent that.
>>
>> I don't know if I'm missing something obvious here, so please correct me if I'm wrong.
>>
> I just sent out the v2 of the patch set, and (un?)fortunately missed the change I had for the #ifdef's in this chunk (to use #if IS_ENABLED() and remove pre-processor directives from the ACPI-related function).
> 
> I am making the point that the change you are suggesting (as I understand) is this conditional statement
> 
> if IS_ENABLED(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");
> }
> 

That's right, that's the suggestion I'm making.

> and for it to link successfully, one needs of_get_flat_dt_subnode_by_name defined. From the source code, that needs CONFIG_OF_EARLY_FLATTREE as there is no stub available when CONFIG_OF_EARLY_FLATTREE is not defined.

We agree that you need of_get_flat_dt_subnode_by_name declared and defined, and it's not available, stub or otherwise, if CONFIG_OF_EARLY_FLATTREE is not defined.

In my mind, I believed that either ACPI or OF had to be selected for some sort of firmware handoff to occur, but I did some experimentation and ended up with an
x86_64 kernel config that has neither enabled, but nonetheless has CONFIG_HYPERV enabled. The kernel builds with such a config, whether it's useful for anything
is another question. ARM64 requires CONFIG_OF so that side of the equation is clear.

That's where my question above came in: Are we thinking there may be some weird config where neither OF nor ACPI is selected? I thought that was not a reasonable
config, thus my prescription to set `depends on ACPI || OF` for config HYPERV. If there is a use case for an x86_64 kernel that has neither ACPI nor OpenFirmware/FDT,
but nevertheless sets CONFIG_HYPERV, feel free to ignore this comment thread entirely.


Thanks,
Easwar
Roman Kisel May 20, 2024, 5:08 p.m. UTC | #4
On 5/14/2024 5:00 PM, Easwar Hariharan wrote:
> On 5/14/2024 4:17 PM, Roman Kisel wrote:
>>
>>
>> On 5/14/2024 3:46 PM, Easwar Hariharan wrote:
>>> On 5/10/2024 10:42 AM, Roman Kisel wrote:
>>>>
>>>>
>>>> On 5/10/2024 10:04 AM, Easwar Hariharan wrote:
>>>>> On 5/10/2024 9:05 AM, romank@linux.microsoft.com wrote:
>>>>>> From: Roman Kisel <romank@linux.microsoft.com>
>>>>>>
>>>>>> Update the driver to support DeviceTree boot as well along with ACPI.
>>>>>> This enables the Virtual Trust Level platforms boot up on ARM64.
>>>>>>
>>>>>> 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
>>>>>> +}
>>>>>> +
>>>>>
>>>>> Could using IS_ENABLED() allow collapsing these two functions into one hyperv_detect_fw()?
>>>>> I am wondering if #ifdef was explicitly chosen to allow for the code to be compiled in if CONFIG* is defined
>>>>> v/s IS_ENABLED() only being true if the CONFIG value is true.
>>>>>
>>>> In the hyperv_detect_fdt function, the #ifdef has been chosen due to the functions being declared only when the macro is defined, hence I could not rely on `if IS_ENABLED()` and the run-time detection. For the compile-time option, `#if IS_ENABLED()` would convey the intent better, could update with that.
>>>
>>> In patch 2/6, just under the diff you have, we already `select OF_EARLY_FLATTREE if OF`, so the declarations (and definitions)
>>> of the functions being present is already handled, AFAIK. Are we thinking there may be some weird config where neither OF nor
>>> ACPI is selected? If so, we should set a `depends on ACPI || OF` for config HYPERV to prevent that.
>>>
>>> I don't know if I'm missing something obvious here, so please correct me if I'm wrong.
>>>
>> I just sent out the v2 of the patch set, and (un?)fortunately missed the change I had for the #ifdef's in this chunk (to use #if IS_ENABLED() and remove pre-processor directives from the ACPI-related function).
>>
>> I am making the point that the change you are suggesting (as I understand) is this conditional statement
>>
>> if IS_ENABLED(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");
>> }
>>
> 
> That's right, that's the suggestion I'm making.
> 
>> and for it to link successfully, one needs of_get_flat_dt_subnode_by_name defined. From the source code, that needs CONFIG_OF_EARLY_FLATTREE as there is no stub available when CONFIG_OF_EARLY_FLATTREE is not defined.
> 
> We agree that you need of_get_flat_dt_subnode_by_name declared and defined, and it's not available, stub or otherwise, if CONFIG_OF_EARLY_FLATTREE is not defined.
> 
> In my mind, I believed that either ACPI or OF had to be selected for some sort of firmware handoff to occur, but I did some experimentation and ended up with an
> x86_64 kernel config that has neither enabled, but nonetheless has CONFIG_HYPERV enabled. The kernel builds with such a config, whether it's useful for anything
> is another question. ARM64 requires CONFIG_OF so that side of the equation is clear.
> 
> That's where my question above came in: Are we thinking there may be some weird config where neither OF nor ACPI is selected? I thought that was not a reasonable
> config, thus my prescription to set `depends on ACPI || OF` for config HYPERV. If there is a use case for an x86_64 kernel that has neither ACPI nor OpenFirmware/FDT,
> but nevertheless sets CONFIG_HYPERV, feel free to ignore this comment thread entirely.
> 
Thanks for laying out the details so patiently for me! I believe I 
understand your concerns better now. In the V2 of the patch series, 
Michael Kelly suggested rethinking how the Kconfig change is carried 
out. Appears natural to try out what your suggesting alongside with the 
Michael's suggestions.

As for your other point, I wouldn't think, too, the system could boot or 
be useful without some coordinates of the environment passed via ACPI || DT.

> 
> Thanks,
> Easwar
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 */