Re: [PATCH 1/6] arm64/hyperv: Support DeviceTree

[Easwar Hariharan <eahariha@linux.microsoft.com>]

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