| Message ID | 20240514224508.212318-2-romank@linux.microsoft.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | arm64/hyperv: Support Virtual Trust Level Boot | expand |
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... Where is the binding for this? Best regards, Krzysztof
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
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/ > > 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
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. >> >> 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
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
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 >
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 --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 */
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(-)