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, [email protected] wrote: >>>>> From: Roman Kisel <[email protected]> >>>>> >>>>> 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 <[email protected]> >>>>> --- >>>>> 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
