All of lore.kernel.org
 help / color / mirror / Atom feed
* RE: [PATCH v8 1/6] arm64: hyperv: Add Hyper-V hypercall and register access utilities
       [not found] ` <1614649360-5087-2-git-send-email-mikelley@microsoft.com>
@ 2021-03-03  0:34     ` Sunil Muthuswamy
  0 siblings, 0 replies; 26+ messages in thread
From: Sunil Muthuswamy @ 2021-03-03  0:34 UTC (permalink / raw)
  To: Michael Kelley
  Cc: will, catalin.marinas, Mark Rutland, linux-arm-kernel,
	linux-kernel, linux-hyperv, linux-efi, arnd, wei.liu, ardb,
	daniel.lezcano, KY Srinivasan

> +}
> +EXPORT_SYMBOL_GPL(hv_do_fast_hypercall8);
> +
> +
nit: Extra line, here and few other places

> +u64 hv_get_vpreg(u32 msr)
> +{
> +	struct hv_get_vp_registers_input	*input;
> +	struct hv_get_vp_registers_output	*output;
> +	u64					result;
It seems like the code is using both spaces and tabs to align variable names. Can
we stick to one or the other, preferably spaces.

> +
> +	/*
> +	 * Allocate a power of 2 size so alignment to that size is
> +	 * guaranteed, since the hypercall input and output areas
> +	 * must not cross a page boundary.
> +	 */
> +	input = kzalloc(roundup_pow_of_two(sizeof(input->header) +
> +				sizeof(input->element[0])), GFP_ATOMIC);
> +	output = kmalloc(roundup_pow_of_two(sizeof(*output)), GFP_ATOMIC);
> +
Check for null from these malloc routines? Here and in other places.

> +	__hv_get_vpreg_128(msr, input, output);

+ * Linux-specific definitions for managing interactions with Microsoft's
+ * Hyper-V hypervisor. The definitions in this file are specific to
+ * the ARM64 architecture.  See include/asm-generic/mshyperv.h for
nit: Two space before 'See'. Here and in couple of other places.


^ permalink raw reply	[flat|nested] 26+ messages in thread

* RE: [PATCH v8 1/6] arm64: hyperv: Add Hyper-V hypercall and register access utilities
@ 2021-03-03  0:34     ` Sunil Muthuswamy
  0 siblings, 0 replies; 26+ messages in thread
From: Sunil Muthuswamy @ 2021-03-03  0:34 UTC (permalink / raw)
  To: Michael Kelley
  Cc: will, catalin.marinas, Mark Rutland, linux-arm-kernel,
	linux-kernel, linux-hyperv, linux-efi, arnd, wei.liu, ardb,
	daniel.lezcano, KY Srinivasan

> +}
> +EXPORT_SYMBOL_GPL(hv_do_fast_hypercall8);
> +
> +
nit: Extra line, here and few other places

> +u64 hv_get_vpreg(u32 msr)
> +{
> +	struct hv_get_vp_registers_input	*input;
> +	struct hv_get_vp_registers_output	*output;
> +	u64					result;
It seems like the code is using both spaces and tabs to align variable names. Can
we stick to one or the other, preferably spaces.

> +
> +	/*
> +	 * Allocate a power of 2 size so alignment to that size is
> +	 * guaranteed, since the hypercall input and output areas
> +	 * must not cross a page boundary.
> +	 */
> +	input = kzalloc(roundup_pow_of_two(sizeof(input->header) +
> +				sizeof(input->element[0])), GFP_ATOMIC);
> +	output = kmalloc(roundup_pow_of_two(sizeof(*output)), GFP_ATOMIC);
> +
Check for null from these malloc routines? Here and in other places.

> +	__hv_get_vpreg_128(msr, input, output);

+ * Linux-specific definitions for managing interactions with Microsoft's
+ * Hyper-V hypervisor. The definitions in this file are specific to
+ * the ARM64 architecture.  See include/asm-generic/mshyperv.h for
nit: Two space before 'See'. Here and in couple of other places.


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 26+ messages in thread

* RE: [PATCH v8 1/6] arm64: hyperv: Add Hyper-V hypercall and register access utilities
  2021-03-03  0:34     ` Sunil Muthuswamy
@ 2021-03-03 18:28       ` Sunil Muthuswamy
  -1 siblings, 0 replies; 26+ messages in thread
From: Sunil Muthuswamy @ 2021-03-03 18:28 UTC (permalink / raw)
  To: Sunil Muthuswamy, Michael Kelley
  Cc: will, catalin.marinas, Mark Rutland, linux-arm-kernel,
	linux-kernel, linux-hyperv, linux-efi, arnd, wei.liu, ardb,
	daniel.lezcano, KY Srinivasan

> > +
> > +	/*
> > +	 * Allocate a power of 2 size so alignment to that size is
> > +	 * guaranteed, since the hypercall input and output areas
> > +	 * must not cross a page boundary.
> > +	 */
> > +	input = kzalloc(roundup_pow_of_two(sizeof(input->header) +
> > +				sizeof(input->element[0])), GFP_ATOMIC);
> > +	output = kmalloc(roundup_pow_of_two(sizeof(*output)), GFP_ATOMIC);
> > +
> Check for null from these malloc routines? Here and in other places.

Between, is there a plan to setup a percpu input/output page for hypercall
input/output on ARM (like we do for x64)? I didn't check the specific size requirement
for this particular call, but, that generally will remove the need for these
allocations.

^ permalink raw reply	[flat|nested] 26+ messages in thread

* RE: [PATCH v8 1/6] arm64: hyperv: Add Hyper-V hypercall and register access utilities
@ 2021-03-03 18:28       ` Sunil Muthuswamy
  0 siblings, 0 replies; 26+ messages in thread
From: Sunil Muthuswamy @ 2021-03-03 18:28 UTC (permalink / raw)
  To: Sunil Muthuswamy, Michael Kelley
  Cc: will, catalin.marinas, Mark Rutland, linux-arm-kernel,
	linux-kernel, linux-hyperv, linux-efi, arnd, wei.liu, ardb,
	daniel.lezcano, KY Srinivasan

> > +
> > +	/*
> > +	 * Allocate a power of 2 size so alignment to that size is
> > +	 * guaranteed, since the hypercall input and output areas
> > +	 * must not cross a page boundary.
> > +	 */
> > +	input = kzalloc(roundup_pow_of_two(sizeof(input->header) +
> > +				sizeof(input->element[0])), GFP_ATOMIC);
> > +	output = kmalloc(roundup_pow_of_two(sizeof(*output)), GFP_ATOMIC);
> > +
> Check for null from these malloc routines? Here and in other places.

Between, is there a plan to setup a percpu input/output page for hypercall
input/output on ARM (like we do for x64)? I didn't check the specific size requirement
for this particular call, but, that generally will remove the need for these
allocations.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 26+ messages in thread

* RE: [PATCH v8 6/6] Drivers: hv: Enable Hyper-V code to be built on ARM64
       [not found] ` <1614649360-5087-7-git-send-email-mikelley@microsoft.com>
@ 2021-03-03 19:22     ` Sunil Muthuswamy
  0 siblings, 0 replies; 26+ messages in thread
From: Sunil Muthuswamy @ 2021-03-03 19:22 UTC (permalink / raw)
  To: Michael Kelley
  Cc: will, catalin.marinas, Mark Rutland, linux-arm-kernel,
	linux-kernel, linux-hyperv, linux-efi, arnd, wei.liu, ardb,
	daniel.lezcano, KY Srinivasan

> Update drivers/hv/Kconfig so CONFIG_HYPERV can be selected on
> ARM64, causing the Hyper-V specific code to be built.
> 
> Signed-off-by: Michael Kelley <mikelley@microsoft.com>
> ---
>  drivers/hv/Kconfig | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hv/Kconfig b/drivers/hv/Kconfig
> index 79e5356..d492682 100644
> --- a/drivers/hv/Kconfig
> +++ b/drivers/hv/Kconfig
> @@ -4,7 +4,8 @@ menu "Microsoft Hyper-V guest support"
> 
>  config HYPERV
>  	tristate "Microsoft Hyper-V client drivers"
> -	depends on X86 && ACPI && X86_LOCAL_APIC && HYPERVISOR_GUEST
> +	depends on ACPI && ((X86 && X86_LOCAL_APIC && HYPERVISOR_GUEST) \
> +		|| (ARM64 && !CPU_BIG_ENDIAN))
>  	select PARAVIRT
>  	select X86_HV_CALLBACK_VECTOR
>  	help
> --
> 1.8.3.1

Reviewed-by: Sunil Muthuswamy <sunilmut@microsoft.com>


^ permalink raw reply	[flat|nested] 26+ messages in thread

* RE: [PATCH v8 6/6] Drivers: hv: Enable Hyper-V code to be built on ARM64
@ 2021-03-03 19:22     ` Sunil Muthuswamy
  0 siblings, 0 replies; 26+ messages in thread
From: Sunil Muthuswamy @ 2021-03-03 19:22 UTC (permalink / raw)
  To: Michael Kelley
  Cc: will, catalin.marinas, Mark Rutland, linux-arm-kernel,
	linux-kernel, linux-hyperv, linux-efi, arnd, wei.liu, ardb,
	daniel.lezcano, KY Srinivasan

> Update drivers/hv/Kconfig so CONFIG_HYPERV can be selected on
> ARM64, causing the Hyper-V specific code to be built.
> 
> Signed-off-by: Michael Kelley <mikelley@microsoft.com>
> ---
>  drivers/hv/Kconfig | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hv/Kconfig b/drivers/hv/Kconfig
> index 79e5356..d492682 100644
> --- a/drivers/hv/Kconfig
> +++ b/drivers/hv/Kconfig
> @@ -4,7 +4,8 @@ menu "Microsoft Hyper-V guest support"
> 
>  config HYPERV
>  	tristate "Microsoft Hyper-V client drivers"
> -	depends on X86 && ACPI && X86_LOCAL_APIC && HYPERVISOR_GUEST
> +	depends on ACPI && ((X86 && X86_LOCAL_APIC && HYPERVISOR_GUEST) \
> +		|| (ARM64 && !CPU_BIG_ENDIAN))
>  	select PARAVIRT
>  	select X86_HV_CALLBACK_VECTOR
>  	help
> --
> 1.8.3.1

Reviewed-by: Sunil Muthuswamy <sunilmut@microsoft.com>


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 26+ messages in thread

* RE: [PATCH v8 2/6] arm64: hyperv: Add Hyper-V clocksource/clockevent support
       [not found] ` <1614649360-5087-3-git-send-email-mikelley@microsoft.com>
@ 2021-03-03 19:29     ` Sunil Muthuswamy
  0 siblings, 0 replies; 26+ messages in thread
From: Sunil Muthuswamy @ 2021-03-03 19:29 UTC (permalink / raw)
  To: Michael Kelley
  Cc: will, catalin.marinas, Mark Rutland, linux-arm-kernel,
	linux-kernel, linux-hyperv, linux-efi, arnd, wei.liu, ardb,
	daniel.lezcano, KY Srinivasan

> +/* Define the interrupt ID used by STIMER0 Direct Mode interrupts. This
> + * value can't come from ACPI tables because it is needed before the
> + * Linux ACPI subsystem is initialized.
> + */
> +#define HYPERV_STIMER0_VECTOR	31
nit: On x64, this is defined in HEX

> +
> +	return 0;
> +}
> +TIMER_ACPI_DECLARE(hyperv, ACPI_SIG_GTDT, hyperv_timer_init);
> --
> 1.8.3.1

Reviewed-by: Sunil Muthuswamy <sunilmut@microsoft.com>


^ permalink raw reply	[flat|nested] 26+ messages in thread

* RE: [PATCH v8 2/6] arm64: hyperv: Add Hyper-V clocksource/clockevent support
@ 2021-03-03 19:29     ` Sunil Muthuswamy
  0 siblings, 0 replies; 26+ messages in thread
From: Sunil Muthuswamy @ 2021-03-03 19:29 UTC (permalink / raw)
  To: Michael Kelley
  Cc: will, catalin.marinas, Mark Rutland, linux-arm-kernel,
	linux-kernel, linux-hyperv, linux-efi, arnd, wei.liu, ardb,
	daniel.lezcano, KY Srinivasan

> +/* Define the interrupt ID used by STIMER0 Direct Mode interrupts. This
> + * value can't come from ACPI tables because it is needed before the
> + * Linux ACPI subsystem is initialized.
> + */
> +#define HYPERV_STIMER0_VECTOR	31
nit: On x64, this is defined in HEX

> +
> +	return 0;
> +}
> +TIMER_ACPI_DECLARE(hyperv, ACPI_SIG_GTDT, hyperv_timer_init);
> --
> 1.8.3.1

Reviewed-by: Sunil Muthuswamy <sunilmut@microsoft.com>


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 26+ messages in thread

* RE: [PATCH v8 4/6] arm64: hyperv: Initialize hypervisor on boot
       [not found] ` <1614649360-5087-5-git-send-email-mikelley@microsoft.com>
@ 2021-03-03 20:21     ` Sunil Muthuswamy
  0 siblings, 0 replies; 26+ messages in thread
From: Sunil Muthuswamy @ 2021-03-03 20:21 UTC (permalink / raw)
  To: Michael Kelley
  Cc: will, catalin.marinas, Mark Rutland, linux-arm-kernel,
	linux-kernel, linux-hyperv, linux-efi, arnd, wei.liu, ardb,
	daniel.lezcano, KY Srinivasan

> +static u64 hypercall_output __initdata;
> +
> +static int __init hyperv_init(void)
> +{
> +	struct hv_get_vpindex_from_apicid_input *input;
> +	u64	status;
> +	int	i;
nit: both, tabs & spaces are being used to indent variable names. Can we stick
to one?

> +
> +	/*
> +	 * Hypercall inputs must not cross a page boundary, so allocate
> +	 * power of 2 size, which will be aligned to that size.
> +	 */
> +	input = kzalloc(roundup_pow_of_two(sizeof(input->header) +
> +				sizeof(input->element[0])), GFP_KERNEL);
> +	if (!input)
> +		return -ENOMEM;
Similar to the comment on the other patch, can this be done through a
per-cpu hypercall input page?

> +		if ((status & HV_HYPERCALL_RESULT_MASK) == HV_STATUS_SUCCESS)
> +			hv_vp_index[i] = hypercall_output;
> +		else {
CNF suggests using braces even for single line 'if' statements if the 'else' is a multi-line
statement

> +			pr_warn("Hyper-V: No VP index for CPU %d MPIDR %llx status %llx\n",
> +				i, cpu_logical_map(i), status);
> +			hv_vp_index[i] = VP_INVAL;
> +		}

> +bool hv_is_hyperv_initialized(void)
> +{
> +	return hyperv_initialized;
> +}
> +EXPORT_SYMBOL_GPL(hv_is_hyperv_initialized);

This to me seems like more of an indication that whether the hypervisor is Hyper-V or
something else, rather than if necessary Hyper-V pieces have been initialized. If that is
the case, suggest renaming. Maybe just call it 'hv_is_hyperv'?

^ permalink raw reply	[flat|nested] 26+ messages in thread

* RE: [PATCH v8 4/6] arm64: hyperv: Initialize hypervisor on boot
@ 2021-03-03 20:21     ` Sunil Muthuswamy
  0 siblings, 0 replies; 26+ messages in thread
From: Sunil Muthuswamy @ 2021-03-03 20:21 UTC (permalink / raw)
  To: Michael Kelley
  Cc: will, catalin.marinas, Mark Rutland, linux-arm-kernel,
	linux-kernel, linux-hyperv, linux-efi, arnd, wei.liu, ardb,
	daniel.lezcano, KY Srinivasan

> +static u64 hypercall_output __initdata;
> +
> +static int __init hyperv_init(void)
> +{
> +	struct hv_get_vpindex_from_apicid_input *input;
> +	u64	status;
> +	int	i;
nit: both, tabs & spaces are being used to indent variable names. Can we stick
to one?

> +
> +	/*
> +	 * Hypercall inputs must not cross a page boundary, so allocate
> +	 * power of 2 size, which will be aligned to that size.
> +	 */
> +	input = kzalloc(roundup_pow_of_two(sizeof(input->header) +
> +				sizeof(input->element[0])), GFP_KERNEL);
> +	if (!input)
> +		return -ENOMEM;
Similar to the comment on the other patch, can this be done through a
per-cpu hypercall input page?

> +		if ((status & HV_HYPERCALL_RESULT_MASK) == HV_STATUS_SUCCESS)
> +			hv_vp_index[i] = hypercall_output;
> +		else {
CNF suggests using braces even for single line 'if' statements if the 'else' is a multi-line
statement

> +			pr_warn("Hyper-V: No VP index for CPU %d MPIDR %llx status %llx\n",
> +				i, cpu_logical_map(i), status);
> +			hv_vp_index[i] = VP_INVAL;
> +		}

> +bool hv_is_hyperv_initialized(void)
> +{
> +	return hyperv_initialized;
> +}
> +EXPORT_SYMBOL_GPL(hv_is_hyperv_initialized);

This to me seems like more of an indication that whether the hypervisor is Hyper-V or
something else, rather than if necessary Hyper-V pieces have been initialized. If that is
the case, suggest renaming. Maybe just call it 'hv_is_hyperv'?

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 26+ messages in thread

* RE: [PATCH v8 3/6] arm64: hyperv: Add kexec and panic handlers
       [not found] ` <1614649360-5087-4-git-send-email-mikelley@microsoft.com>
@ 2021-03-03 21:29     ` Sunil Muthuswamy
  0 siblings, 0 replies; 26+ messages in thread
From: Sunil Muthuswamy @ 2021-03-03 21:29 UTC (permalink / raw)
  To: Michael Kelley
  Cc: will, catalin.marinas, Mark Rutland, linux-arm-kernel,
	linux-kernel, linux-hyperv, linux-efi, arnd, wei.liu, ardb,
	daniel.lezcano, KY Srinivasan

> +EXPORT_SYMBOL_GPL(hv_setup_crash_handler);
> +
> +void hv_remove_crash_handler(void)
> +{
> +}
> +EXPORT_SYMBOL_GPL(hv_remove_crash_handler);
> --
> 1.8.3.1

Reviewed-by: Sunil Muthuswamy <sunilmut@microsoft.com>

^ permalink raw reply	[flat|nested] 26+ messages in thread

* RE: [PATCH v8 3/6] arm64: hyperv: Add kexec and panic handlers
@ 2021-03-03 21:29     ` Sunil Muthuswamy
  0 siblings, 0 replies; 26+ messages in thread
From: Sunil Muthuswamy @ 2021-03-03 21:29 UTC (permalink / raw)
  To: Michael Kelley
  Cc: will, catalin.marinas, Mark Rutland, linux-arm-kernel,
	linux-kernel, linux-hyperv, linux-efi, arnd, wei.liu, ardb,
	daniel.lezcano, KY Srinivasan

> +EXPORT_SYMBOL_GPL(hv_setup_crash_handler);
> +
> +void hv_remove_crash_handler(void)
> +{
> +}
> +EXPORT_SYMBOL_GPL(hv_remove_crash_handler);
> --
> 1.8.3.1

Reviewed-by: Sunil Muthuswamy <sunilmut@microsoft.com>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 26+ messages in thread

* RE: [PATCH v8 1/6] arm64: hyperv: Add Hyper-V hypercall and register access utilities
  2021-03-03  0:34     ` Sunil Muthuswamy
@ 2021-03-05 20:56       ` Michael Kelley
  -1 siblings, 0 replies; 26+ messages in thread
From: Michael Kelley @ 2021-03-05 20:56 UTC (permalink / raw)
  To: Sunil Muthuswamy
  Cc: will, catalin.marinas, Mark Rutland, linux-arm-kernel,
	linux-kernel, linux-hyperv, linux-efi, arnd, wei.liu, ardb,
	daniel.lezcano, KY Srinivasan

From: Sunil Muthuswamy <sunilmut@microsoft.com> Sent: Tuesday, March 2, 2021 4:35 PM
> 
> > +}
> > +EXPORT_SYMBOL_GPL(hv_do_fast_hypercall8);
> > +
> > +
> nit: Extra line, here and few other places

I'll try to clean these up.

> 
> > +u64 hv_get_vpreg(u32 msr)
> > +{
> > +	struct hv_get_vp_registers_input	*input;
> > +	struct hv_get_vp_registers_output	*output;
> > +	u64					result;
> It seems like the code is using both spaces and tabs to align variable names. Can
> we stick to one or the other, preferably spaces.

I think the general preference is to use tabs for alignment, not
spaces, so I'll try to clean these up in the next version.

> 
> > +
> > +	/*
> > +	 * Allocate a power of 2 size so alignment to that size is
> > +	 * guaranteed, since the hypercall input and output areas
> > +	 * must not cross a page boundary.
> > +	 */
> > +	input = kzalloc(roundup_pow_of_two(sizeof(input->header) +
> > +				sizeof(input->element[0])), GFP_ATOMIC);
> > +	output = kmalloc(roundup_pow_of_two(sizeof(*output)), GFP_ATOMIC);
> > +
> Check for null from these malloc routines? Here and in other places.

See my comment back to Boqun Feng on the same point.

> 
> > +	__hv_get_vpreg_128(msr, input, output);
> 
> + * Linux-specific definitions for managing interactions with Microsoft's
> + * Hyper-V hypervisor. The definitions in this file are specific to
> + * the ARM64 architecture.  See include/asm-generic/mshyperv.h for
> nit: Two space before 'See'. Here and in couple of other places.

The debate rages in the broader world of writing about using one
space vs. two spaces. :-)   I'm still old school on this point, and use two
spaces out of decades of habit, particularly with monospaced fonts,
though I'm probably not 100% consistent.

Michael


^ permalink raw reply	[flat|nested] 26+ messages in thread

* RE: [PATCH v8 1/6] arm64: hyperv: Add Hyper-V hypercall and register access utilities
@ 2021-03-05 20:56       ` Michael Kelley
  0 siblings, 0 replies; 26+ messages in thread
From: Michael Kelley @ 2021-03-05 20:56 UTC (permalink / raw)
  To: Sunil Muthuswamy
  Cc: will, catalin.marinas, Mark Rutland, linux-arm-kernel,
	linux-kernel, linux-hyperv, linux-efi, arnd, wei.liu, ardb,
	daniel.lezcano, KY Srinivasan

From: Sunil Muthuswamy <sunilmut@microsoft.com> Sent: Tuesday, March 2, 2021 4:35 PM
> 
> > +}
> > +EXPORT_SYMBOL_GPL(hv_do_fast_hypercall8);
> > +
> > +
> nit: Extra line, here and few other places

I'll try to clean these up.

> 
> > +u64 hv_get_vpreg(u32 msr)
> > +{
> > +	struct hv_get_vp_registers_input	*input;
> > +	struct hv_get_vp_registers_output	*output;
> > +	u64					result;
> It seems like the code is using both spaces and tabs to align variable names. Can
> we stick to one or the other, preferably spaces.

I think the general preference is to use tabs for alignment, not
spaces, so I'll try to clean these up in the next version.

> 
> > +
> > +	/*
> > +	 * Allocate a power of 2 size so alignment to that size is
> > +	 * guaranteed, since the hypercall input and output areas
> > +	 * must not cross a page boundary.
> > +	 */
> > +	input = kzalloc(roundup_pow_of_two(sizeof(input->header) +
> > +				sizeof(input->element[0])), GFP_ATOMIC);
> > +	output = kmalloc(roundup_pow_of_two(sizeof(*output)), GFP_ATOMIC);
> > +
> Check for null from these malloc routines? Here and in other places.

See my comment back to Boqun Feng on the same point.

> 
> > +	__hv_get_vpreg_128(msr, input, output);
> 
> + * Linux-specific definitions for managing interactions with Microsoft's
> + * Hyper-V hypervisor. The definitions in this file are specific to
> + * the ARM64 architecture.  See include/asm-generic/mshyperv.h for
> nit: Two space before 'See'. Here and in couple of other places.

The debate rages in the broader world of writing about using one
space vs. two spaces. :-)   I'm still old school on this point, and use two
spaces out of decades of habit, particularly with monospaced fonts,
though I'm probably not 100% consistent.

Michael


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 26+ messages in thread

* RE: [PATCH v8 1/6] arm64: hyperv: Add Hyper-V hypercall and register access utilities
  2021-03-03 18:28       ` Sunil Muthuswamy
@ 2021-03-05 20:58         ` Michael Kelley
  -1 siblings, 0 replies; 26+ messages in thread
From: Michael Kelley @ 2021-03-05 20:58 UTC (permalink / raw)
  To: Sunil Muthuswamy
  Cc: will, catalin.marinas, Mark Rutland, linux-arm-kernel,
	linux-kernel, linux-hyperv, linux-efi, arnd, wei.liu, ardb,
	daniel.lezcano, KY Srinivasan

From: Sunil Muthuswamy <sunilmut@microsoft.com> Sent: Wednesday, March 3, 2021 10:28 AM
> 
> > > +
> > > +	/*
> > > +	 * Allocate a power of 2 size so alignment to that size is
> > > +	 * guaranteed, since the hypercall input and output areas
> > > +	 * must not cross a page boundary.
> > > +	 */
> > > +	input = kzalloc(roundup_pow_of_two(sizeof(input->header) +
> > > +				sizeof(input->element[0])), GFP_ATOMIC);
> > > +	output = kmalloc(roundup_pow_of_two(sizeof(*output)), GFP_ATOMIC);
> > > +
> > Check for null from these malloc routines? Here and in other places.
> 
> Between, is there a plan to setup a percpu input/output page for hypercall
> input/output on ARM (like we do for x64)? I didn't check the specific size requirement
> for this particular call, but, that generally will remove the need for these
> allocations.

I'm not planning to add percpu input/output pages in this patch set.  As noted
in another reply, we can eliminate these memory allocations entirely by using
"fast" hypercalls.  At that point, there would be no usage of percpu input/output
pages in this patch set.  If the need arises in the context of future work, we can
add them at that time.

Michael

^ permalink raw reply	[flat|nested] 26+ messages in thread

* RE: [PATCH v8 1/6] arm64: hyperv: Add Hyper-V hypercall and register access utilities
@ 2021-03-05 20:58         ` Michael Kelley
  0 siblings, 0 replies; 26+ messages in thread
From: Michael Kelley @ 2021-03-05 20:58 UTC (permalink / raw)
  To: Sunil Muthuswamy
  Cc: will, catalin.marinas, Mark Rutland, linux-arm-kernel,
	linux-kernel, linux-hyperv, linux-efi, arnd, wei.liu, ardb,
	daniel.lezcano, KY Srinivasan

From: Sunil Muthuswamy <sunilmut@microsoft.com> Sent: Wednesday, March 3, 2021 10:28 AM
> 
> > > +
> > > +	/*
> > > +	 * Allocate a power of 2 size so alignment to that size is
> > > +	 * guaranteed, since the hypercall input and output areas
> > > +	 * must not cross a page boundary.
> > > +	 */
> > > +	input = kzalloc(roundup_pow_of_two(sizeof(input->header) +
> > > +				sizeof(input->element[0])), GFP_ATOMIC);
> > > +	output = kmalloc(roundup_pow_of_two(sizeof(*output)), GFP_ATOMIC);
> > > +
> > Check for null from these malloc routines? Here and in other places.
> 
> Between, is there a plan to setup a percpu input/output page for hypercall
> input/output on ARM (like we do for x64)? I didn't check the specific size requirement
> for this particular call, but, that generally will remove the need for these
> allocations.

I'm not planning to add percpu input/output pages in this patch set.  As noted
in another reply, we can eliminate these memory allocations entirely by using
"fast" hypercalls.  At that point, there would be no usage of percpu input/output
pages in this patch set.  If the need arises in the context of future work, we can
add them at that time.

Michael

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 26+ messages in thread

* RE: [PATCH v8 4/6] arm64: hyperv: Initialize hypervisor on boot
  2021-03-03 20:21     ` Sunil Muthuswamy
@ 2021-03-05 21:03       ` Michael Kelley
  -1 siblings, 0 replies; 26+ messages in thread
From: Michael Kelley @ 2021-03-05 21:03 UTC (permalink / raw)
  To: Sunil Muthuswamy
  Cc: will, catalin.marinas, Mark Rutland, linux-arm-kernel,
	linux-kernel, linux-hyperv, linux-efi, arnd, wei.liu, ardb,
	daniel.lezcano, KY Srinivasan

From: Sunil Muthuswamy <sunilmut@microsoft.com> Sent: Wednesday, March 3, 2021 12:21 PM
> 
> > +static u64 hypercall_output __initdata;
> > +
> > +static int __init hyperv_init(void)
> > +{
> > +	struct hv_get_vpindex_from_apicid_input *input;
> > +	u64	status;
> > +	int	i;
> nit: both, tabs & spaces are being used to indent variable names. Can we stick
> to one?
> 

Agreed.  Will make this consistent in the next version.

> > +
> > +	/*
> > +	 * Hypercall inputs must not cross a page boundary, so allocate
> > +	 * power of 2 size, which will be aligned to that size.
> > +	 */
> > +	input = kzalloc(roundup_pow_of_two(sizeof(input->header) +
> > +				sizeof(input->element[0])), GFP_KERNEL);
> > +	if (!input)
> > +		return -ENOMEM;
> Similar to the comment on the other patch, can this be done through a
> per-cpu hypercall input page?

Per comments on other previous patch, these allocations can be eliminated
and the problem avoided entirely.

> 
> > +		if ((status & HV_HYPERCALL_RESULT_MASK) == HV_STATUS_SUCCESS)
> > +			hv_vp_index[i] = hypercall_output;
> > +		else {
> CNF suggests using braces even for single line 'if' statements if the 'else' is a multi-line
> statement

Will do.

> 
> > +			pr_warn("Hyper-V: No VP index for CPU %d MPIDR %llx status
> %llx\n",
> > +				i, cpu_logical_map(i), status);
> > +			hv_vp_index[i] = VP_INVAL;
> > +		}
> 
> > +bool hv_is_hyperv_initialized(void)
> > +{
> > +	return hyperv_initialized;
> > +}
> > +EXPORT_SYMBOL_GPL(hv_is_hyperv_initialized);
> 
> This to me seems like more of an indication that whether the hypervisor is Hyper-V or
> something else, rather than if necessary Hyper-V pieces have been initialized. If that is
> the case, suggest renaming. Maybe just call it 'hv_is_hyperv'?

This function also exists on the x86/x64 side and is used in architecture independent
code.  It's meaning is somewhere between "is Hyper-V" and "is Hyper-V initialized".
I'm not sure there's much value in changing the name everywhere.

Michael

^ permalink raw reply	[flat|nested] 26+ messages in thread

* RE: [PATCH v8 4/6] arm64: hyperv: Initialize hypervisor on boot
@ 2021-03-05 21:03       ` Michael Kelley
  0 siblings, 0 replies; 26+ messages in thread
From: Michael Kelley @ 2021-03-05 21:03 UTC (permalink / raw)
  To: Sunil Muthuswamy
  Cc: will, catalin.marinas, Mark Rutland, linux-arm-kernel,
	linux-kernel, linux-hyperv, linux-efi, arnd, wei.liu, ardb,
	daniel.lezcano, KY Srinivasan

From: Sunil Muthuswamy <sunilmut@microsoft.com> Sent: Wednesday, March 3, 2021 12:21 PM
> 
> > +static u64 hypercall_output __initdata;
> > +
> > +static int __init hyperv_init(void)
> > +{
> > +	struct hv_get_vpindex_from_apicid_input *input;
> > +	u64	status;
> > +	int	i;
> nit: both, tabs & spaces are being used to indent variable names. Can we stick
> to one?
> 

Agreed.  Will make this consistent in the next version.

> > +
> > +	/*
> > +	 * Hypercall inputs must not cross a page boundary, so allocate
> > +	 * power of 2 size, which will be aligned to that size.
> > +	 */
> > +	input = kzalloc(roundup_pow_of_two(sizeof(input->header) +
> > +				sizeof(input->element[0])), GFP_KERNEL);
> > +	if (!input)
> > +		return -ENOMEM;
> Similar to the comment on the other patch, can this be done through a
> per-cpu hypercall input page?

Per comments on other previous patch, these allocations can be eliminated
and the problem avoided entirely.

> 
> > +		if ((status & HV_HYPERCALL_RESULT_MASK) == HV_STATUS_SUCCESS)
> > +			hv_vp_index[i] = hypercall_output;
> > +		else {
> CNF suggests using braces even for single line 'if' statements if the 'else' is a multi-line
> statement

Will do.

> 
> > +			pr_warn("Hyper-V: No VP index for CPU %d MPIDR %llx status
> %llx\n",
> > +				i, cpu_logical_map(i), status);
> > +			hv_vp_index[i] = VP_INVAL;
> > +		}
> 
> > +bool hv_is_hyperv_initialized(void)
> > +{
> > +	return hyperv_initialized;
> > +}
> > +EXPORT_SYMBOL_GPL(hv_is_hyperv_initialized);
> 
> This to me seems like more of an indication that whether the hypervisor is Hyper-V or
> something else, rather than if necessary Hyper-V pieces have been initialized. If that is
> the case, suggest renaming. Maybe just call it 'hv_is_hyperv'?

This function also exists on the x86/x64 side and is used in architecture independent
code.  It's meaning is somewhere between "is Hyper-V" and "is Hyper-V initialized".
I'm not sure there's much value in changing the name everywhere.

Michael

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 26+ messages in thread

* RE: [PATCH v8 1/6] arm64: hyperv: Add Hyper-V hypercall and register access utilities
  2021-02-24  2:37     ` Boqun Feng
@ 2021-03-05 20:50       ` Michael Kelley
  -1 siblings, 0 replies; 26+ messages in thread
From: Michael Kelley @ 2021-03-05 20:50 UTC (permalink / raw)
  To: Boqun Feng
  Cc: will, catalin.marinas, Mark Rutland, linux-arm-kernel,
	linux-kernel, linux-hyperv, linux-efi, arnd, wei.liu, ardb,
	daniel.lezcano, KY Srinivasan

From: Boqun Feng <boqun.feng@gmail.com> Sent: Tuesday, February 23, 2021 6:37 PM
> 
> On Thu, Feb 18, 2021 at 03:16:29PM -0800, Michael Kelley wrote:
> [...]
> > +
> > +/*
> > + * Get the value of a single VP register.  One version
> > + * returns just 64 bits and another returns the full 128 bits.
> > + * The two versions are separate to avoid complicating the
> > + * calling sequence for the more frequently used 64 bit version.
> > + */
> > +
> > +void __hv_get_vpreg_128(u32 msr,
> > +			struct hv_get_vp_registers_input  *input,
> > +			struct hv_get_vp_registers_output *res)
> > +{
> > +	u64	status;
> > +
> > +	input->header.partitionid = HV_PARTITION_ID_SELF;
> > +	input->header.vpindex = HV_VP_INDEX_SELF;
> > +	input->header.inputvtl = 0;
> > +	input->element[0].name0 = msr;
> > +	input->element[0].name1 = 0;
> > +
> > +
> > +	status = hv_do_hypercall(
> > +		HVCALL_GET_VP_REGISTERS | HV_HYPERCALL_REP_COMP_1,
> > +		input, res);
> > +
> > +	/*
> > +	 * Something is fundamentally broken in the hypervisor if
> > +	 * getting a VP register fails. There's really no way to
> > +	 * continue as a guest VM, so panic.
> > +	 */
> > +	BUG_ON((status & HV_HYPERCALL_RESULT_MASK) != HV_STATUS_SUCCESS);
> > +}
> > +
> > +u64 hv_get_vpreg(u32 msr)
> > +{
> > +	struct hv_get_vp_registers_input	*input;
> > +	struct hv_get_vp_registers_output	*output;
> > +	u64					result;
> > +
> > +	/*
> > +	 * Allocate a power of 2 size so alignment to that size is
> > +	 * guaranteed, since the hypercall input and output areas
> > +	 * must not cross a page boundary.
> > +	 */
> > +	input = kzalloc(roundup_pow_of_two(sizeof(input->header) +
> > +				sizeof(input->element[0])), GFP_ATOMIC);
> > +	output = kmalloc(roundup_pow_of_two(sizeof(*output)), GFP_ATOMIC);
> > +
> 
> Do we need to BUG_ON(!input || !output)? Or we expect the page fault
> (for input being NULL) or the failure of hypercall (for output being
> NULL) to tell us the allocation failed?
> 
> Hmm.. think a bit more on this, maybe we'd better retry the allocation
> if it failed. Because say we are under memory pressusre, and only have
> memory enough for doing one hvcall, and one thread allocates that memory
> but gets preempted by another thread trying to do another hvcall:
> 
> 	<thread 1>
> 	hv_get_vpreg():
> 	  input = kzalloc(...);
> 	  output = kmalloc(...);
> 	<preempted and switch to thread 2>
> 	hv_get_vpreg():
> 	  intput = kzalloc(...); // allocation fails, but actually if
> 	                         // we wait for thread 1 to finish its
> 				 // hvcall, we can get enough memory.
> 
> , in this case, if thread 2 retried, it might get the enough memory,
> therefore there is no need to BUG_ON() on allocation failure. That said,
> I don't think this is likely to happen, and there may be better
> solutions for this, so maybe we can keep it as it is (assuming that
> memory allocation for hvcall never fails) and improve later.
> 
> Regards,
> Boqun

Having to do these memory allocations in order to make a
hypercall results in a lot of messiness.  I've just gone back to
try again at doing hv_get_vpreg() and hv_get_vpreg_128()
as "fast" hypercalls that pass inputs (and outputs) in registers
like hv_set_vpreg().  I have it working now, with some tweaks
to arm_smccc_1_1_hvc() to allow outputs in a wider range
of registers than just X0 thru X3.  This wider range of registers
is allowed by the SMCCC version 1.2 and 1.3 specs, so hopefully
is acceptable.  I'll send out a new version using this "fast"
hypercall approach that completely avoids all these memory
allocation problems.

Michael

> 
> > +	__hv_get_vpreg_128(msr, input, output);
> > +
> > +	result = output->as64.low;
> > +	kfree(input);
> > +	kfree(output);
> > +	return result;
> > +}
> > +EXPORT_SYMBOL_GPL(hv_get_vpreg);
> > +
> > +void hv_get_vpreg_128(u32 msr, struct hv_get_vp_registers_output *res)
> > +{
> > +	struct hv_get_vp_registers_input	*input;
> > +	struct hv_get_vp_registers_output	*output;
> > +
> > +	/*
> > +	 * Allocate a power of 2 size so alignment to that size is
> > +	 * guaranteed, since the hypercall input and output areas
> > +	 * must not cross a page boundary.
> > +	 */
> > +	input = kzalloc(roundup_pow_of_two(sizeof(input->header) +
> > +				sizeof(input->element[0])), GFP_ATOMIC);
> > +	output = kmalloc(roundup_pow_of_two(sizeof(*output)), GFP_ATOMIC);
> > +
> > +	__hv_get_vpreg_128(msr, input, output);
> > +
> > +	res->as64.low = output->as64.low;
> > +	res->as64.high = output->as64.high;
> > +	kfree(input);
> > +	kfree(output);
> > +}
> [...]

^ permalink raw reply	[flat|nested] 26+ messages in thread

* RE: [PATCH v8 1/6] arm64: hyperv: Add Hyper-V hypercall and register access utilities
@ 2021-03-05 20:50       ` Michael Kelley
  0 siblings, 0 replies; 26+ messages in thread
From: Michael Kelley @ 2021-03-05 20:50 UTC (permalink / raw)
  To: Boqun Feng
  Cc: will, catalin.marinas, Mark Rutland, linux-arm-kernel,
	linux-kernel, linux-hyperv, linux-efi, arnd, wei.liu, ardb,
	daniel.lezcano, KY Srinivasan

From: Boqun Feng <boqun.feng@gmail.com> Sent: Tuesday, February 23, 2021 6:37 PM
> 
> On Thu, Feb 18, 2021 at 03:16:29PM -0800, Michael Kelley wrote:
> [...]
> > +
> > +/*
> > + * Get the value of a single VP register.  One version
> > + * returns just 64 bits and another returns the full 128 bits.
> > + * The two versions are separate to avoid complicating the
> > + * calling sequence for the more frequently used 64 bit version.
> > + */
> > +
> > +void __hv_get_vpreg_128(u32 msr,
> > +			struct hv_get_vp_registers_input  *input,
> > +			struct hv_get_vp_registers_output *res)
> > +{
> > +	u64	status;
> > +
> > +	input->header.partitionid = HV_PARTITION_ID_SELF;
> > +	input->header.vpindex = HV_VP_INDEX_SELF;
> > +	input->header.inputvtl = 0;
> > +	input->element[0].name0 = msr;
> > +	input->element[0].name1 = 0;
> > +
> > +
> > +	status = hv_do_hypercall(
> > +		HVCALL_GET_VP_REGISTERS | HV_HYPERCALL_REP_COMP_1,
> > +		input, res);
> > +
> > +	/*
> > +	 * Something is fundamentally broken in the hypervisor if
> > +	 * getting a VP register fails. There's really no way to
> > +	 * continue as a guest VM, so panic.
> > +	 */
> > +	BUG_ON((status & HV_HYPERCALL_RESULT_MASK) != HV_STATUS_SUCCESS);
> > +}
> > +
> > +u64 hv_get_vpreg(u32 msr)
> > +{
> > +	struct hv_get_vp_registers_input	*input;
> > +	struct hv_get_vp_registers_output	*output;
> > +	u64					result;
> > +
> > +	/*
> > +	 * Allocate a power of 2 size so alignment to that size is
> > +	 * guaranteed, since the hypercall input and output areas
> > +	 * must not cross a page boundary.
> > +	 */
> > +	input = kzalloc(roundup_pow_of_two(sizeof(input->header) +
> > +				sizeof(input->element[0])), GFP_ATOMIC);
> > +	output = kmalloc(roundup_pow_of_two(sizeof(*output)), GFP_ATOMIC);
> > +
> 
> Do we need to BUG_ON(!input || !output)? Or we expect the page fault
> (for input being NULL) or the failure of hypercall (for output being
> NULL) to tell us the allocation failed?
> 
> Hmm.. think a bit more on this, maybe we'd better retry the allocation
> if it failed. Because say we are under memory pressusre, and only have
> memory enough for doing one hvcall, and one thread allocates that memory
> but gets preempted by another thread trying to do another hvcall:
> 
> 	<thread 1>
> 	hv_get_vpreg():
> 	  input = kzalloc(...);
> 	  output = kmalloc(...);
> 	<preempted and switch to thread 2>
> 	hv_get_vpreg():
> 	  intput = kzalloc(...); // allocation fails, but actually if
> 	                         // we wait for thread 1 to finish its
> 				 // hvcall, we can get enough memory.
> 
> , in this case, if thread 2 retried, it might get the enough memory,
> therefore there is no need to BUG_ON() on allocation failure. That said,
> I don't think this is likely to happen, and there may be better
> solutions for this, so maybe we can keep it as it is (assuming that
> memory allocation for hvcall never fails) and improve later.
> 
> Regards,
> Boqun

Having to do these memory allocations in order to make a
hypercall results in a lot of messiness.  I've just gone back to
try again at doing hv_get_vpreg() and hv_get_vpreg_128()
as "fast" hypercalls that pass inputs (and outputs) in registers
like hv_set_vpreg().  I have it working now, with some tweaks
to arm_smccc_1_1_hvc() to allow outputs in a wider range
of registers than just X0 thru X3.  This wider range of registers
is allowed by the SMCCC version 1.2 and 1.3 specs, so hopefully
is acceptable.  I'll send out a new version using this "fast"
hypercall approach that completely avoids all these memory
allocation problems.

Michael

> 
> > +	__hv_get_vpreg_128(msr, input, output);
> > +
> > +	result = output->as64.low;
> > +	kfree(input);
> > +	kfree(output);
> > +	return result;
> > +}
> > +EXPORT_SYMBOL_GPL(hv_get_vpreg);
> > +
> > +void hv_get_vpreg_128(u32 msr, struct hv_get_vp_registers_output *res)
> > +{
> > +	struct hv_get_vp_registers_input	*input;
> > +	struct hv_get_vp_registers_output	*output;
> > +
> > +	/*
> > +	 * Allocate a power of 2 size so alignment to that size is
> > +	 * guaranteed, since the hypercall input and output areas
> > +	 * must not cross a page boundary.
> > +	 */
> > +	input = kzalloc(roundup_pow_of_two(sizeof(input->header) +
> > +				sizeof(input->element[0])), GFP_ATOMIC);
> > +	output = kmalloc(roundup_pow_of_two(sizeof(*output)), GFP_ATOMIC);
> > +
> > +	__hv_get_vpreg_128(msr, input, output);
> > +
> > +	res->as64.low = output->as64.low;
> > +	res->as64.high = output->as64.high;
> > +	kfree(input);
> > +	kfree(output);
> > +}
> [...]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v8 1/6] arm64: hyperv: Add Hyper-V hypercall and register access utilities
  2021-02-18 23:16   ` Michael Kelley
@ 2021-02-24  2:37     ` Boqun Feng
  -1 siblings, 0 replies; 26+ messages in thread
From: Boqun Feng @ 2021-02-24  2:37 UTC (permalink / raw)
  To: Michael Kelley
  Cc: will, catalin.marinas, mark.rutland, linux-arm-kernel,
	linux-kernel, linux-hyperv, linux-efi, arnd, wei.liu, ardb,
	daniel.lezcano, kys

On Thu, Feb 18, 2021 at 03:16:29PM -0800, Michael Kelley wrote:
[...]
> +
> +/*
> + * Get the value of a single VP register.  One version
> + * returns just 64 bits and another returns the full 128 bits.
> + * The two versions are separate to avoid complicating the
> + * calling sequence for the more frequently used 64 bit version.
> + */
> +
> +void __hv_get_vpreg_128(u32 msr,
> +			struct hv_get_vp_registers_input  *input,
> +			struct hv_get_vp_registers_output *res)
> +{
> +	u64	status;
> +
> +	input->header.partitionid = HV_PARTITION_ID_SELF;
> +	input->header.vpindex = HV_VP_INDEX_SELF;
> +	input->header.inputvtl = 0;
> +	input->element[0].name0 = msr;
> +	input->element[0].name1 = 0;
> +
> +
> +	status = hv_do_hypercall(
> +		HVCALL_GET_VP_REGISTERS | HV_HYPERCALL_REP_COMP_1,
> +		input, res);
> +
> +	/*
> +	 * Something is fundamentally broken in the hypervisor if
> +	 * getting a VP register fails. There's really no way to
> +	 * continue as a guest VM, so panic.
> +	 */
> +	BUG_ON((status & HV_HYPERCALL_RESULT_MASK) != HV_STATUS_SUCCESS);
> +}
> +
> +u64 hv_get_vpreg(u32 msr)
> +{
> +	struct hv_get_vp_registers_input	*input;
> +	struct hv_get_vp_registers_output	*output;
> +	u64					result;
> +
> +	/*
> +	 * Allocate a power of 2 size so alignment to that size is
> +	 * guaranteed, since the hypercall input and output areas
> +	 * must not cross a page boundary.
> +	 */
> +	input = kzalloc(roundup_pow_of_two(sizeof(input->header) +
> +				sizeof(input->element[0])), GFP_ATOMIC);
> +	output = kmalloc(roundup_pow_of_two(sizeof(*output)), GFP_ATOMIC);
> +

Do we need to BUG_ON(!input || !output)? Or we expect the page fault
(for input being NULL) or the failure of hypercall (for output being
NULL) to tell us the allocation failed?

Hmm.. think a bit more on this, maybe we'd better retry the allocation
if it failed. Because say we are under memory pressusre, and only have
memory enough for doing one hvcall, and one thread allocates that memory
but gets preempted by another thread trying to do another hvcall:

	<thread 1>
	hv_get_vpreg():
	  input = kzalloc(...);
	  output = kmalloc(...);
	<preempted and switch to thread 2>
	hv_get_vpreg():
	  intput = kzalloc(...); // allocation fails, but actually if
	                         // we wait for thread 1 to finish its
				 // hvcall, we can get enough memory.

, in this case, if thread 2 retried, it might get the enough memory,
therefore there is no need to BUG_ON() on allocation failure. That said,
I don't think this is likely to happen, and there may be better
solutions for this, so maybe we can keep it as it is (assuming that
memory allocation for hvcall never fails) and improve later.

Regards,
Boqun

> +	__hv_get_vpreg_128(msr, input, output);
> +
> +	result = output->as64.low;
> +	kfree(input);
> +	kfree(output);
> +	return result;
> +}
> +EXPORT_SYMBOL_GPL(hv_get_vpreg);
> +
> +void hv_get_vpreg_128(u32 msr, struct hv_get_vp_registers_output *res)
> +{
> +	struct hv_get_vp_registers_input	*input;
> +	struct hv_get_vp_registers_output	*output;
> +
> +	/*
> +	 * Allocate a power of 2 size so alignment to that size is
> +	 * guaranteed, since the hypercall input and output areas
> +	 * must not cross a page boundary.
> +	 */
> +	input = kzalloc(roundup_pow_of_two(sizeof(input->header) +
> +				sizeof(input->element[0])), GFP_ATOMIC);
> +	output = kmalloc(roundup_pow_of_two(sizeof(*output)), GFP_ATOMIC);
> +
> +	__hv_get_vpreg_128(msr, input, output);
> +
> +	res->as64.low = output->as64.low;
> +	res->as64.high = output->as64.high;
> +	kfree(input);
> +	kfree(output);
> +}
[...]

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v8 1/6] arm64: hyperv: Add Hyper-V hypercall and register access utilities
@ 2021-02-24  2:37     ` Boqun Feng
  0 siblings, 0 replies; 26+ messages in thread
From: Boqun Feng @ 2021-02-24  2:37 UTC (permalink / raw)
  To: Michael Kelley
  Cc: mark.rutland, linux-hyperv, linux-efi, arnd, catalin.marinas,
	daniel.lezcano, linux-kernel, wei.liu, kys, will, ardb,
	linux-arm-kernel

On Thu, Feb 18, 2021 at 03:16:29PM -0800, Michael Kelley wrote:
[...]
> +
> +/*
> + * Get the value of a single VP register.  One version
> + * returns just 64 bits and another returns the full 128 bits.
> + * The two versions are separate to avoid complicating the
> + * calling sequence for the more frequently used 64 bit version.
> + */
> +
> +void __hv_get_vpreg_128(u32 msr,
> +			struct hv_get_vp_registers_input  *input,
> +			struct hv_get_vp_registers_output *res)
> +{
> +	u64	status;
> +
> +	input->header.partitionid = HV_PARTITION_ID_SELF;
> +	input->header.vpindex = HV_VP_INDEX_SELF;
> +	input->header.inputvtl = 0;
> +	input->element[0].name0 = msr;
> +	input->element[0].name1 = 0;
> +
> +
> +	status = hv_do_hypercall(
> +		HVCALL_GET_VP_REGISTERS | HV_HYPERCALL_REP_COMP_1,
> +		input, res);
> +
> +	/*
> +	 * Something is fundamentally broken in the hypervisor if
> +	 * getting a VP register fails. There's really no way to
> +	 * continue as a guest VM, so panic.
> +	 */
> +	BUG_ON((status & HV_HYPERCALL_RESULT_MASK) != HV_STATUS_SUCCESS);
> +}
> +
> +u64 hv_get_vpreg(u32 msr)
> +{
> +	struct hv_get_vp_registers_input	*input;
> +	struct hv_get_vp_registers_output	*output;
> +	u64					result;
> +
> +	/*
> +	 * Allocate a power of 2 size so alignment to that size is
> +	 * guaranteed, since the hypercall input and output areas
> +	 * must not cross a page boundary.
> +	 */
> +	input = kzalloc(roundup_pow_of_two(sizeof(input->header) +
> +				sizeof(input->element[0])), GFP_ATOMIC);
> +	output = kmalloc(roundup_pow_of_two(sizeof(*output)), GFP_ATOMIC);
> +

Do we need to BUG_ON(!input || !output)? Or we expect the page fault
(for input being NULL) or the failure of hypercall (for output being
NULL) to tell us the allocation failed?

Hmm.. think a bit more on this, maybe we'd better retry the allocation
if it failed. Because say we are under memory pressusre, and only have
memory enough for doing one hvcall, and one thread allocates that memory
but gets preempted by another thread trying to do another hvcall:

	<thread 1>
	hv_get_vpreg():
	  input = kzalloc(...);
	  output = kmalloc(...);
	<preempted and switch to thread 2>
	hv_get_vpreg():
	  intput = kzalloc(...); // allocation fails, but actually if
	                         // we wait for thread 1 to finish its
				 // hvcall, we can get enough memory.

, in this case, if thread 2 retried, it might get the enough memory,
therefore there is no need to BUG_ON() on allocation failure. That said,
I don't think this is likely to happen, and there may be better
solutions for this, so maybe we can keep it as it is (assuming that
memory allocation for hvcall never fails) and improve later.

Regards,
Boqun

> +	__hv_get_vpreg_128(msr, input, output);
> +
> +	result = output->as64.low;
> +	kfree(input);
> +	kfree(output);
> +	return result;
> +}
> +EXPORT_SYMBOL_GPL(hv_get_vpreg);
> +
> +void hv_get_vpreg_128(u32 msr, struct hv_get_vp_registers_output *res)
> +{
> +	struct hv_get_vp_registers_input	*input;
> +	struct hv_get_vp_registers_output	*output;
> +
> +	/*
> +	 * Allocate a power of 2 size so alignment to that size is
> +	 * guaranteed, since the hypercall input and output areas
> +	 * must not cross a page boundary.
> +	 */
> +	input = kzalloc(roundup_pow_of_two(sizeof(input->header) +
> +				sizeof(input->element[0])), GFP_ATOMIC);
> +	output = kmalloc(roundup_pow_of_two(sizeof(*output)), GFP_ATOMIC);
> +
> +	__hv_get_vpreg_128(msr, input, output);
> +
> +	res->as64.low = output->as64.low;
> +	res->as64.high = output->as64.high;
> +	kfree(input);
> +	kfree(output);
> +}
[...]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v8 1/6] arm64: hyperv: Add Hyper-V hypercall and register access utilities
  2021-02-18 23:16   ` Michael Kelley
@ 2021-02-22 10:20     ` Wei Liu
  -1 siblings, 0 replies; 26+ messages in thread
From: Wei Liu @ 2021-02-22 10:20 UTC (permalink / raw)
  To: Michael Kelley
  Cc: will, catalin.marinas, mark.rutland, linux-arm-kernel,
	linux-kernel, linux-hyperv, linux-efi, arnd, wei.liu, ardb,
	daniel.lezcano, kys

On Thu, Feb 18, 2021 at 03:16:29PM -0800, Michael Kelley wrote:
> hyperv-tlfs.h defines Hyper-V interfaces from the Hyper-V Top Level
> Functional Spec (TLFS), and #includes the architecture-independent
> part of hyperv-tlfs.h in include/asm-generic.  The published TLFS
> is distinctly oriented to x86/x64, so the ARM64-specific
> hyperv-tlfs.h includes information for ARM64 that is not yet formally
> published. The TLFS is available here:
> 
>   docs.microsoft.com/en-us/virtualization/hyper-v-on-windows/reference/tlfs
> 
> mshyperv.h defines Linux-specific structures and routines for
> interacting with Hyper-V on ARM64, and #includes the architecture-
> independent part of mshyperv.h in include/asm-generic.
> 
> Use these definitions to provide utility functions to make
> Hyper-V hypercalls and to get and set Hyper-V provided
> registers associated with a virtual processor.
> 
> Signed-off-by: Michael Kelley <mikelley@microsoft.com>

Reviewed-by: Wei Liu <wei.liu@kernel.org>

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v8 1/6] arm64: hyperv: Add Hyper-V hypercall and register access utilities
@ 2021-02-22 10:20     ` Wei Liu
  0 siblings, 0 replies; 26+ messages in thread
From: Wei Liu @ 2021-02-22 10:20 UTC (permalink / raw)
  To: Michael Kelley
  Cc: mark.rutland, linux-hyperv, linux-efi, arnd, catalin.marinas,
	daniel.lezcano, linux-kernel, wei.liu, kys, will, ardb,
	linux-arm-kernel

On Thu, Feb 18, 2021 at 03:16:29PM -0800, Michael Kelley wrote:
> hyperv-tlfs.h defines Hyper-V interfaces from the Hyper-V Top Level
> Functional Spec (TLFS), and #includes the architecture-independent
> part of hyperv-tlfs.h in include/asm-generic.  The published TLFS
> is distinctly oriented to x86/x64, so the ARM64-specific
> hyperv-tlfs.h includes information for ARM64 that is not yet formally
> published. The TLFS is available here:
> 
>   docs.microsoft.com/en-us/virtualization/hyper-v-on-windows/reference/tlfs
> 
> mshyperv.h defines Linux-specific structures and routines for
> interacting with Hyper-V on ARM64, and #includes the architecture-
> independent part of mshyperv.h in include/asm-generic.
> 
> Use these definitions to provide utility functions to make
> Hyper-V hypercalls and to get and set Hyper-V provided
> registers associated with a virtual processor.
> 
> Signed-off-by: Michael Kelley <mikelley@microsoft.com>

Reviewed-by: Wei Liu <wei.liu@kernel.org>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH v8 1/6] arm64: hyperv: Add Hyper-V hypercall and register access utilities
  2021-02-18 23:16 [PATCH v8 0/6] Enable Linux guests on Hyper-V on ARM64 Michael Kelley
@ 2021-02-18 23:16   ` Michael Kelley
  0 siblings, 0 replies; 26+ messages in thread
From: Michael Kelley @ 2021-02-18 23:16 UTC (permalink / raw)
  To: will, catalin.marinas, mark.rutland, linux-arm-kernel,
	linux-kernel, linux-hyperv, linux-efi, arnd, wei.liu, ardb,
	daniel.lezcano, kys
  Cc: mikelley

hyperv-tlfs.h defines Hyper-V interfaces from the Hyper-V Top Level
Functional Spec (TLFS), and #includes the architecture-independent
part of hyperv-tlfs.h in include/asm-generic.  The published TLFS
is distinctly oriented to x86/x64, so the ARM64-specific
hyperv-tlfs.h includes information for ARM64 that is not yet formally
published. The TLFS is available here:

  docs.microsoft.com/en-us/virtualization/hyper-v-on-windows/reference/tlfs

mshyperv.h defines Linux-specific structures and routines for
interacting with Hyper-V on ARM64, and #includes the architecture-
independent part of mshyperv.h in include/asm-generic.

Use these definitions to provide utility functions to make
Hyper-V hypercalls and to get and set Hyper-V provided
registers associated with a virtual processor.

Signed-off-by: Michael Kelley <mikelley@microsoft.com>
---
 MAINTAINERS                          |   3 +
 arch/arm64/Kbuild                    |   1 +
 arch/arm64/hyperv/Makefile           |   2 +
 arch/arm64/hyperv/hv_core.c          | 167 +++++++++++++++++++++++++++++++++++
 arch/arm64/include/asm/hyperv-tlfs.h |  69 +++++++++++++++
 arch/arm64/include/asm/mshyperv.h    |  55 ++++++++++++
 6 files changed, 297 insertions(+)
 create mode 100644 arch/arm64/hyperv/Makefile
 create mode 100644 arch/arm64/hyperv/hv_core.c
 create mode 100644 arch/arm64/include/asm/hyperv-tlfs.h
 create mode 100644 arch/arm64/include/asm/mshyperv.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 546aa66..0a22f3a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8235,6 +8235,9 @@ T:	git git://git.kernel.org/pub/scm/linux/kernel/git/hyperv/linux.git
 F:	Documentation/ABI/stable/sysfs-bus-vmbus
 F:	Documentation/ABI/testing/debugfs-hyperv
 F:	Documentation/networking/device_drivers/ethernet/microsoft/netvsc.rst
+F:	arch/arm64/hyperv
+F:	arch/arm64/include/asm/hyperv-tlfs.h
+F:	arch/arm64/include/asm/mshyperv.h
 F:	arch/x86/hyperv
 F:	arch/x86/include/asm/hyperv-tlfs.h
 F:	arch/x86/include/asm/mshyperv.h
diff --git a/arch/arm64/Kbuild b/arch/arm64/Kbuild
index d646582..7a37608 100644
--- a/arch/arm64/Kbuild
+++ b/arch/arm64/Kbuild
@@ -3,4 +3,5 @@ obj-y			+= kernel/ mm/
 obj-$(CONFIG_NET)	+= net/
 obj-$(CONFIG_KVM)	+= kvm/
 obj-$(CONFIG_XEN)	+= xen/
+obj-$(subst m,y,$(CONFIG_HYPERV))	+= hyperv/
 obj-$(CONFIG_CRYPTO)	+= crypto/
diff --git a/arch/arm64/hyperv/Makefile b/arch/arm64/hyperv/Makefile
new file mode 100644
index 0000000..1697d30
--- /dev/null
+++ b/arch/arm64/hyperv/Makefile
@@ -0,0 +1,2 @@
+# SPDX-License-Identifier: GPL-2.0
+obj-y		:= hv_core.o
diff --git a/arch/arm64/hyperv/hv_core.c b/arch/arm64/hyperv/hv_core.c
new file mode 100644
index 0000000..9a37124
--- /dev/null
+++ b/arch/arm64/hyperv/hv_core.c
@@ -0,0 +1,167 @@
+// SPDX-License-Identifier: GPL-2.0
+
+/*
+ * Low level utility routines for interacting with Hyper-V.
+ *
+ * Copyright (C) 2021, Microsoft, Inc.
+ *
+ * Author : Michael Kelley <mikelley@microsoft.com>
+ */
+
+
+#include <linux/types.h>
+#include <linux/log2.h>
+#include <linux/export.h>
+#include <linux/mm.h>
+#include <linux/slab.h>
+#include <linux/hyperv.h>
+#include <linux/arm-smccc.h>
+#include <linux/vmalloc.h>
+#include <linux/module.h>
+#include <asm-generic/bug.h>
+#include <asm/hyperv-tlfs.h>
+#include <asm/mshyperv.h>
+
+/*
+ * hv_do_hypercall- Invoke the specified hypercall
+ */
+u64 hv_do_hypercall(u64 control, void *input, void *output)
+{
+	u64 input_address;
+	u64 output_address;
+	struct arm_smccc_res res;
+
+	input_address = input ? virt_to_phys(input) : 0;
+	output_address = output ? virt_to_phys(output) : 0;
+
+	arm_smccc_1_1_hvc(HV_FUNC_ID, control,
+			  input_address, output_address, &res);
+	return res.a0;
+}
+EXPORT_SYMBOL_GPL(hv_do_hypercall);
+
+/*
+ * hv_do_fast_hypercall8 -- Invoke the specified hypercall
+ * with arguments in registers instead of physical memory.
+ * Avoids the overhead of virt_to_phys for simple hypercalls.
+ */
+
+u64 hv_do_fast_hypercall8(u16 code, u64 input)
+{
+	u64 control;
+	struct arm_smccc_res res;
+
+	control = (u64)code | HV_HYPERCALL_FAST_BIT;
+
+	arm_smccc_1_1_hvc(HV_FUNC_ID, control, input, &res);
+	return res.a0;
+}
+EXPORT_SYMBOL_GPL(hv_do_fast_hypercall8);
+
+
+/*
+ * Set a single VP register to a 64-bit value.
+ */
+void hv_set_vpreg(u32 msr, u64 value)
+{
+	struct arm_smccc_res res;
+
+	arm_smccc_1_1_hvc(
+		HV_FUNC_ID,
+		HVCALL_SET_VP_REGISTERS | HV_HYPERCALL_FAST_BIT |
+			HV_HYPERCALL_REP_COMP_1,
+		HV_PARTITION_ID_SELF,
+		HV_VP_INDEX_SELF,
+		msr,
+		0,
+		value,
+		0,
+		&res);
+
+	/*
+	 * Something is fundamentally broken in the hypervisor if
+	 * setting a VP register fails. There's really no way to
+	 * continue as a guest VM, so panic.
+	 */
+	BUG_ON((res.a0 & HV_HYPERCALL_RESULT_MASK) != HV_STATUS_SUCCESS);
+}
+EXPORT_SYMBOL_GPL(hv_set_vpreg);
+
+/*
+ * Get the value of a single VP register.  One version
+ * returns just 64 bits and another returns the full 128 bits.
+ * The two versions are separate to avoid complicating the
+ * calling sequence for the more frequently used 64 bit version.
+ */
+
+void __hv_get_vpreg_128(u32 msr,
+			struct hv_get_vp_registers_input  *input,
+			struct hv_get_vp_registers_output *res)
+{
+	u64	status;
+
+	input->header.partitionid = HV_PARTITION_ID_SELF;
+	input->header.vpindex = HV_VP_INDEX_SELF;
+	input->header.inputvtl = 0;
+	input->element[0].name0 = msr;
+	input->element[0].name1 = 0;
+
+
+	status = hv_do_hypercall(
+		HVCALL_GET_VP_REGISTERS | HV_HYPERCALL_REP_COMP_1,
+		input, res);
+
+	/*
+	 * Something is fundamentally broken in the hypervisor if
+	 * getting a VP register fails. There's really no way to
+	 * continue as a guest VM, so panic.
+	 */
+	BUG_ON((status & HV_HYPERCALL_RESULT_MASK) != HV_STATUS_SUCCESS);
+}
+
+u64 hv_get_vpreg(u32 msr)
+{
+	struct hv_get_vp_registers_input	*input;
+	struct hv_get_vp_registers_output	*output;
+	u64					result;
+
+	/*
+	 * Allocate a power of 2 size so alignment to that size is
+	 * guaranteed, since the hypercall input and output areas
+	 * must not cross a page boundary.
+	 */
+	input = kzalloc(roundup_pow_of_two(sizeof(input->header) +
+				sizeof(input->element[0])), GFP_ATOMIC);
+	output = kmalloc(roundup_pow_of_two(sizeof(*output)), GFP_ATOMIC);
+
+	__hv_get_vpreg_128(msr, input, output);
+
+	result = output->as64.low;
+	kfree(input);
+	kfree(output);
+	return result;
+}
+EXPORT_SYMBOL_GPL(hv_get_vpreg);
+
+void hv_get_vpreg_128(u32 msr, struct hv_get_vp_registers_output *res)
+{
+	struct hv_get_vp_registers_input	*input;
+	struct hv_get_vp_registers_output	*output;
+
+	/*
+	 * Allocate a power of 2 size so alignment to that size is
+	 * guaranteed, since the hypercall input and output areas
+	 * must not cross a page boundary.
+	 */
+	input = kzalloc(roundup_pow_of_two(sizeof(input->header) +
+				sizeof(input->element[0])), GFP_ATOMIC);
+	output = kmalloc(roundup_pow_of_two(sizeof(*output)), GFP_ATOMIC);
+
+	__hv_get_vpreg_128(msr, input, output);
+
+	res->as64.low = output->as64.low;
+	res->as64.high = output->as64.high;
+	kfree(input);
+	kfree(output);
+}
+EXPORT_SYMBOL_GPL(hv_get_vpreg_128);
diff --git a/arch/arm64/include/asm/hyperv-tlfs.h b/arch/arm64/include/asm/hyperv-tlfs.h
new file mode 100644
index 0000000..4d964a7
--- /dev/null
+++ b/arch/arm64/include/asm/hyperv-tlfs.h
@@ -0,0 +1,69 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+/*
+ * This file contains definitions from the Hyper-V Hypervisor Top-Level
+ * Functional Specification (TLFS):
+ * https://docs.microsoft.com/en-us/virtualization/hyper-v-on-windows/reference/tlfs
+ *
+ * Copyright (C) 2021, Microsoft, Inc.
+ *
+ * Author : Michael Kelley <mikelley@microsoft.com>
+ */
+
+#ifndef _ASM_HYPERV_TLFS_H
+#define _ASM_HYPERV_TLFS_H
+
+#include <linux/types.h>
+
+/*
+ * All data structures defined in the TLFS that are shared between Hyper-V
+ * and a guest VM use Little Endian byte ordering.  This matches the default
+ * byte ordering of Linux running on ARM64, so no special handling is required.
+ */
+
+/*
+ * These Hyper-V registers provide information equivalent to the CPUID
+ * instruction on x86/x64.
+ */
+#define HV_REGISTER_HYPERVISOR_VERSION		0x00000100 /*CPUID 0x40000002 */
+#define HV_REGISTER_FEATURES			0x00000200 /*CPUID 0x40000003 */
+#define HV_REGISTER_ENLIGHTENMENTS		0x00000201 /*CPUID 0x40000004 */
+
+/*
+ * Group C Features. See the asm-generic version of hyperv-tlfs.h
+ * for a description of Feature Groups.
+ */
+
+/* Crash MSRs available */
+#define HV_FEATURE_GUEST_CRASH_MSR_AVAILABLE	BIT(8)
+
+/* STIMER direct mode is available */
+#define HV_STIMER_DIRECT_MODE_AVAILABLE		BIT(13)
+
+/*
+ * Synthetic register definitions equivalent to MSRs on x86/x64
+ */
+#define HV_REGISTER_CRASH_P0		0x00000210
+#define HV_REGISTER_CRASH_P1		0x00000211
+#define HV_REGISTER_CRASH_P2		0x00000212
+#define HV_REGISTER_CRASH_P3		0x00000213
+#define HV_REGISTER_CRASH_P4		0x00000214
+#define HV_REGISTER_CRASH_CTL		0x00000215
+
+#define HV_REGISTER_GUEST_OSID		0x00090002
+#define HV_REGISTER_VP_INDEX		0x00090003
+#define HV_REGISTER_TIME_REF_COUNT	0x00090004
+#define HV_REGISTER_REFERENCE_TSC	0x00090017
+
+#define HV_REGISTER_SINT0		0x000A0000
+#define HV_REGISTER_SCONTROL		0x000A0010
+#define HV_REGISTER_SIEFP		0x000A0012
+#define HV_REGISTER_SIMP		0x000A0013
+#define HV_REGISTER_EOM			0x000A0014
+
+#define HV_REGISTER_STIMER0_CONFIG	0x000B0000
+#define HV_REGISTER_STIMER0_COUNT	0x000B0001
+
+#include <asm-generic/hyperv-tlfs.h>
+
+#endif
diff --git a/arch/arm64/include/asm/mshyperv.h b/arch/arm64/include/asm/mshyperv.h
new file mode 100644
index 0000000..44ee012
--- /dev/null
+++ b/arch/arm64/include/asm/mshyperv.h
@@ -0,0 +1,55 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+/*
+ * Linux-specific definitions for managing interactions with Microsoft's
+ * Hyper-V hypervisor. The definitions in this file are specific to
+ * the ARM64 architecture.  See include/asm-generic/mshyperv.h for
+ * definitions are that architecture independent.
+ *
+ * Definitions that are specified in the Hyper-V Top Level Functional
+ * Spec (TLFS) should not go in this file, but should instead go in
+ * hyperv-tlfs.h.
+ *
+ * Copyright (C) 2021, Microsoft, Inc.
+ *
+ * Author : Michael Kelley <mikelley@microsoft.com>
+ */
+
+#ifndef _ASM_MSHYPERV_H
+#define _ASM_MSHYPERV_H
+
+#include <linux/types.h>
+#include <linux/arm-smccc.h>
+#include <asm/hyperv-tlfs.h>
+
+/*
+ * Declare calls to get and set Hyper-V VP register values on ARM64, which
+ * requires a hypercall.
+ */
+
+extern void hv_set_vpreg(u32 reg, u64 value);
+extern u64 hv_get_vpreg(u32 reg);
+extern void __hv_get_vpreg_128(u32 reg, struct hv_get_vp_registers_input *input,
+					struct hv_get_vp_registers_output *result);
+
+static inline void hv_set_register(unsigned int reg, u64 value)
+{
+	hv_set_vpreg(reg, value);
+}
+
+static inline u64 hv_get_register(unsigned int reg)
+{
+	return hv_get_vpreg(reg);
+}
+
+/* SMCCC hypercall parameters */
+#define HV_SMCCC_FUNC_NUMBER	1
+#define HV_FUNC_ID	ARM_SMCCC_CALL_VAL(			\
+				ARM_SMCCC_STD_CALL,		\
+				ARM_SMCCC_SMC_64,		\
+				ARM_SMCCC_OWNER_VENDOR_HYP,	\
+				HV_SMCCC_FUNC_NUMBER)
+
+#include <asm-generic/mshyperv.h>
+
+#endif
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH v8 1/6] arm64: hyperv: Add Hyper-V hypercall and register access utilities
@ 2021-02-18 23:16   ` Michael Kelley
  0 siblings, 0 replies; 26+ messages in thread
From: Michael Kelley @ 2021-02-18 23:16 UTC (permalink / raw)
  To: will, catalin.marinas, mark.rutland, linux-arm-kernel,
	linux-kernel, linux-hyperv, linux-efi, arnd, wei.liu, ardb,
	daniel.lezcano, kys
  Cc: mikelley

hyperv-tlfs.h defines Hyper-V interfaces from the Hyper-V Top Level
Functional Spec (TLFS), and #includes the architecture-independent
part of hyperv-tlfs.h in include/asm-generic.  The published TLFS
is distinctly oriented to x86/x64, so the ARM64-specific
hyperv-tlfs.h includes information for ARM64 that is not yet formally
published. The TLFS is available here:

  docs.microsoft.com/en-us/virtualization/hyper-v-on-windows/reference/tlfs

mshyperv.h defines Linux-specific structures and routines for
interacting with Hyper-V on ARM64, and #includes the architecture-
independent part of mshyperv.h in include/asm-generic.

Use these definitions to provide utility functions to make
Hyper-V hypercalls and to get and set Hyper-V provided
registers associated with a virtual processor.

Signed-off-by: Michael Kelley <mikelley@microsoft.com>
---
 MAINTAINERS                          |   3 +
 arch/arm64/Kbuild                    |   1 +
 arch/arm64/hyperv/Makefile           |   2 +
 arch/arm64/hyperv/hv_core.c          | 167 +++++++++++++++++++++++++++++++++++
 arch/arm64/include/asm/hyperv-tlfs.h |  69 +++++++++++++++
 arch/arm64/include/asm/mshyperv.h    |  55 ++++++++++++
 6 files changed, 297 insertions(+)
 create mode 100644 arch/arm64/hyperv/Makefile
 create mode 100644 arch/arm64/hyperv/hv_core.c
 create mode 100644 arch/arm64/include/asm/hyperv-tlfs.h
 create mode 100644 arch/arm64/include/asm/mshyperv.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 546aa66..0a22f3a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8235,6 +8235,9 @@ T:	git git://git.kernel.org/pub/scm/linux/kernel/git/hyperv/linux.git
 F:	Documentation/ABI/stable/sysfs-bus-vmbus
 F:	Documentation/ABI/testing/debugfs-hyperv
 F:	Documentation/networking/device_drivers/ethernet/microsoft/netvsc.rst
+F:	arch/arm64/hyperv
+F:	arch/arm64/include/asm/hyperv-tlfs.h
+F:	arch/arm64/include/asm/mshyperv.h
 F:	arch/x86/hyperv
 F:	arch/x86/include/asm/hyperv-tlfs.h
 F:	arch/x86/include/asm/mshyperv.h
diff --git a/arch/arm64/Kbuild b/arch/arm64/Kbuild
index d646582..7a37608 100644
--- a/arch/arm64/Kbuild
+++ b/arch/arm64/Kbuild
@@ -3,4 +3,5 @@ obj-y			+= kernel/ mm/
 obj-$(CONFIG_NET)	+= net/
 obj-$(CONFIG_KVM)	+= kvm/
 obj-$(CONFIG_XEN)	+= xen/
+obj-$(subst m,y,$(CONFIG_HYPERV))	+= hyperv/
 obj-$(CONFIG_CRYPTO)	+= crypto/
diff --git a/arch/arm64/hyperv/Makefile b/arch/arm64/hyperv/Makefile
new file mode 100644
index 0000000..1697d30
--- /dev/null
+++ b/arch/arm64/hyperv/Makefile
@@ -0,0 +1,2 @@
+# SPDX-License-Identifier: GPL-2.0
+obj-y		:= hv_core.o
diff --git a/arch/arm64/hyperv/hv_core.c b/arch/arm64/hyperv/hv_core.c
new file mode 100644
index 0000000..9a37124
--- /dev/null
+++ b/arch/arm64/hyperv/hv_core.c
@@ -0,0 +1,167 @@
+// SPDX-License-Identifier: GPL-2.0
+
+/*
+ * Low level utility routines for interacting with Hyper-V.
+ *
+ * Copyright (C) 2021, Microsoft, Inc.
+ *
+ * Author : Michael Kelley <mikelley@microsoft.com>
+ */
+
+
+#include <linux/types.h>
+#include <linux/log2.h>
+#include <linux/export.h>
+#include <linux/mm.h>
+#include <linux/slab.h>
+#include <linux/hyperv.h>
+#include <linux/arm-smccc.h>
+#include <linux/vmalloc.h>
+#include <linux/module.h>
+#include <asm-generic/bug.h>
+#include <asm/hyperv-tlfs.h>
+#include <asm/mshyperv.h>
+
+/*
+ * hv_do_hypercall- Invoke the specified hypercall
+ */
+u64 hv_do_hypercall(u64 control, void *input, void *output)
+{
+	u64 input_address;
+	u64 output_address;
+	struct arm_smccc_res res;
+
+	input_address = input ? virt_to_phys(input) : 0;
+	output_address = output ? virt_to_phys(output) : 0;
+
+	arm_smccc_1_1_hvc(HV_FUNC_ID, control,
+			  input_address, output_address, &res);
+	return res.a0;
+}
+EXPORT_SYMBOL_GPL(hv_do_hypercall);
+
+/*
+ * hv_do_fast_hypercall8 -- Invoke the specified hypercall
+ * with arguments in registers instead of physical memory.
+ * Avoids the overhead of virt_to_phys for simple hypercalls.
+ */
+
+u64 hv_do_fast_hypercall8(u16 code, u64 input)
+{
+	u64 control;
+	struct arm_smccc_res res;
+
+	control = (u64)code | HV_HYPERCALL_FAST_BIT;
+
+	arm_smccc_1_1_hvc(HV_FUNC_ID, control, input, &res);
+	return res.a0;
+}
+EXPORT_SYMBOL_GPL(hv_do_fast_hypercall8);
+
+
+/*
+ * Set a single VP register to a 64-bit value.
+ */
+void hv_set_vpreg(u32 msr, u64 value)
+{
+	struct arm_smccc_res res;
+
+	arm_smccc_1_1_hvc(
+		HV_FUNC_ID,
+		HVCALL_SET_VP_REGISTERS | HV_HYPERCALL_FAST_BIT |
+			HV_HYPERCALL_REP_COMP_1,
+		HV_PARTITION_ID_SELF,
+		HV_VP_INDEX_SELF,
+		msr,
+		0,
+		value,
+		0,
+		&res);
+
+	/*
+	 * Something is fundamentally broken in the hypervisor if
+	 * setting a VP register fails. There's really no way to
+	 * continue as a guest VM, so panic.
+	 */
+	BUG_ON((res.a0 & HV_HYPERCALL_RESULT_MASK) != HV_STATUS_SUCCESS);
+}
+EXPORT_SYMBOL_GPL(hv_set_vpreg);
+
+/*
+ * Get the value of a single VP register.  One version
+ * returns just 64 bits and another returns the full 128 bits.
+ * The two versions are separate to avoid complicating the
+ * calling sequence for the more frequently used 64 bit version.
+ */
+
+void __hv_get_vpreg_128(u32 msr,
+			struct hv_get_vp_registers_input  *input,
+			struct hv_get_vp_registers_output *res)
+{
+	u64	status;
+
+	input->header.partitionid = HV_PARTITION_ID_SELF;
+	input->header.vpindex = HV_VP_INDEX_SELF;
+	input->header.inputvtl = 0;
+	input->element[0].name0 = msr;
+	input->element[0].name1 = 0;
+
+
+	status = hv_do_hypercall(
+		HVCALL_GET_VP_REGISTERS | HV_HYPERCALL_REP_COMP_1,
+		input, res);
+
+	/*
+	 * Something is fundamentally broken in the hypervisor if
+	 * getting a VP register fails. There's really no way to
+	 * continue as a guest VM, so panic.
+	 */
+	BUG_ON((status & HV_HYPERCALL_RESULT_MASK) != HV_STATUS_SUCCESS);
+}
+
+u64 hv_get_vpreg(u32 msr)
+{
+	struct hv_get_vp_registers_input	*input;
+	struct hv_get_vp_registers_output	*output;
+	u64					result;
+
+	/*
+	 * Allocate a power of 2 size so alignment to that size is
+	 * guaranteed, since the hypercall input and output areas
+	 * must not cross a page boundary.
+	 */
+	input = kzalloc(roundup_pow_of_two(sizeof(input->header) +
+				sizeof(input->element[0])), GFP_ATOMIC);
+	output = kmalloc(roundup_pow_of_two(sizeof(*output)), GFP_ATOMIC);
+
+	__hv_get_vpreg_128(msr, input, output);
+
+	result = output->as64.low;
+	kfree(input);
+	kfree(output);
+	return result;
+}
+EXPORT_SYMBOL_GPL(hv_get_vpreg);
+
+void hv_get_vpreg_128(u32 msr, struct hv_get_vp_registers_output *res)
+{
+	struct hv_get_vp_registers_input	*input;
+	struct hv_get_vp_registers_output	*output;
+
+	/*
+	 * Allocate a power of 2 size so alignment to that size is
+	 * guaranteed, since the hypercall input and output areas
+	 * must not cross a page boundary.
+	 */
+	input = kzalloc(roundup_pow_of_two(sizeof(input->header) +
+				sizeof(input->element[0])), GFP_ATOMIC);
+	output = kmalloc(roundup_pow_of_two(sizeof(*output)), GFP_ATOMIC);
+
+	__hv_get_vpreg_128(msr, input, output);
+
+	res->as64.low = output->as64.low;
+	res->as64.high = output->as64.high;
+	kfree(input);
+	kfree(output);
+}
+EXPORT_SYMBOL_GPL(hv_get_vpreg_128);
diff --git a/arch/arm64/include/asm/hyperv-tlfs.h b/arch/arm64/include/asm/hyperv-tlfs.h
new file mode 100644
index 0000000..4d964a7
--- /dev/null
+++ b/arch/arm64/include/asm/hyperv-tlfs.h
@@ -0,0 +1,69 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+/*
+ * This file contains definitions from the Hyper-V Hypervisor Top-Level
+ * Functional Specification (TLFS):
+ * https://docs.microsoft.com/en-us/virtualization/hyper-v-on-windows/reference/tlfs
+ *
+ * Copyright (C) 2021, Microsoft, Inc.
+ *
+ * Author : Michael Kelley <mikelley@microsoft.com>
+ */
+
+#ifndef _ASM_HYPERV_TLFS_H
+#define _ASM_HYPERV_TLFS_H
+
+#include <linux/types.h>
+
+/*
+ * All data structures defined in the TLFS that are shared between Hyper-V
+ * and a guest VM use Little Endian byte ordering.  This matches the default
+ * byte ordering of Linux running on ARM64, so no special handling is required.
+ */
+
+/*
+ * These Hyper-V registers provide information equivalent to the CPUID
+ * instruction on x86/x64.
+ */
+#define HV_REGISTER_HYPERVISOR_VERSION		0x00000100 /*CPUID 0x40000002 */
+#define HV_REGISTER_FEATURES			0x00000200 /*CPUID 0x40000003 */
+#define HV_REGISTER_ENLIGHTENMENTS		0x00000201 /*CPUID 0x40000004 */
+
+/*
+ * Group C Features. See the asm-generic version of hyperv-tlfs.h
+ * for a description of Feature Groups.
+ */
+
+/* Crash MSRs available */
+#define HV_FEATURE_GUEST_CRASH_MSR_AVAILABLE	BIT(8)
+
+/* STIMER direct mode is available */
+#define HV_STIMER_DIRECT_MODE_AVAILABLE		BIT(13)
+
+/*
+ * Synthetic register definitions equivalent to MSRs on x86/x64
+ */
+#define HV_REGISTER_CRASH_P0		0x00000210
+#define HV_REGISTER_CRASH_P1		0x00000211
+#define HV_REGISTER_CRASH_P2		0x00000212
+#define HV_REGISTER_CRASH_P3		0x00000213
+#define HV_REGISTER_CRASH_P4		0x00000214
+#define HV_REGISTER_CRASH_CTL		0x00000215
+
+#define HV_REGISTER_GUEST_OSID		0x00090002
+#define HV_REGISTER_VP_INDEX		0x00090003
+#define HV_REGISTER_TIME_REF_COUNT	0x00090004
+#define HV_REGISTER_REFERENCE_TSC	0x00090017
+
+#define HV_REGISTER_SINT0		0x000A0000
+#define HV_REGISTER_SCONTROL		0x000A0010
+#define HV_REGISTER_SIEFP		0x000A0012
+#define HV_REGISTER_SIMP		0x000A0013
+#define HV_REGISTER_EOM			0x000A0014
+
+#define HV_REGISTER_STIMER0_CONFIG	0x000B0000
+#define HV_REGISTER_STIMER0_COUNT	0x000B0001
+
+#include <asm-generic/hyperv-tlfs.h>
+
+#endif
diff --git a/arch/arm64/include/asm/mshyperv.h b/arch/arm64/include/asm/mshyperv.h
new file mode 100644
index 0000000..44ee012
--- /dev/null
+++ b/arch/arm64/include/asm/mshyperv.h
@@ -0,0 +1,55 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+/*
+ * Linux-specific definitions for managing interactions with Microsoft's
+ * Hyper-V hypervisor. The definitions in this file are specific to
+ * the ARM64 architecture.  See include/asm-generic/mshyperv.h for
+ * definitions are that architecture independent.
+ *
+ * Definitions that are specified in the Hyper-V Top Level Functional
+ * Spec (TLFS) should not go in this file, but should instead go in
+ * hyperv-tlfs.h.
+ *
+ * Copyright (C) 2021, Microsoft, Inc.
+ *
+ * Author : Michael Kelley <mikelley@microsoft.com>
+ */
+
+#ifndef _ASM_MSHYPERV_H
+#define _ASM_MSHYPERV_H
+
+#include <linux/types.h>
+#include <linux/arm-smccc.h>
+#include <asm/hyperv-tlfs.h>
+
+/*
+ * Declare calls to get and set Hyper-V VP register values on ARM64, which
+ * requires a hypercall.
+ */
+
+extern void hv_set_vpreg(u32 reg, u64 value);
+extern u64 hv_get_vpreg(u32 reg);
+extern void __hv_get_vpreg_128(u32 reg, struct hv_get_vp_registers_input *input,
+					struct hv_get_vp_registers_output *result);
+
+static inline void hv_set_register(unsigned int reg, u64 value)
+{
+	hv_set_vpreg(reg, value);
+}
+
+static inline u64 hv_get_register(unsigned int reg)
+{
+	return hv_get_vpreg(reg);
+}
+
+/* SMCCC hypercall parameters */
+#define HV_SMCCC_FUNC_NUMBER	1
+#define HV_FUNC_ID	ARM_SMCCC_CALL_VAL(			\
+				ARM_SMCCC_STD_CALL,		\
+				ARM_SMCCC_SMC_64,		\
+				ARM_SMCCC_OWNER_VENDOR_HYP,	\
+				HV_SMCCC_FUNC_NUMBER)
+
+#include <asm-generic/mshyperv.h>
+
+#endif
-- 
1.8.3.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 26+ messages in thread

end of thread, other threads:[~2021-03-05 21:05 UTC | newest]

Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <1614649360-5087-1-git-send-email-mikelley@microsoft.com>
     [not found] ` <1614649360-5087-2-git-send-email-mikelley@microsoft.com>
2021-03-03  0:34   ` [PATCH v8 1/6] arm64: hyperv: Add Hyper-V hypercall and register access utilities Sunil Muthuswamy
2021-03-03  0:34     ` Sunil Muthuswamy
2021-03-03 18:28     ` Sunil Muthuswamy
2021-03-03 18:28       ` Sunil Muthuswamy
2021-03-05 20:58       ` Michael Kelley
2021-03-05 20:58         ` Michael Kelley
2021-03-05 20:56     ` Michael Kelley
2021-03-05 20:56       ` Michael Kelley
     [not found] ` <1614649360-5087-7-git-send-email-mikelley@microsoft.com>
2021-03-03 19:22   ` [PATCH v8 6/6] Drivers: hv: Enable Hyper-V code to be built on ARM64 Sunil Muthuswamy
2021-03-03 19:22     ` Sunil Muthuswamy
     [not found] ` <1614649360-5087-3-git-send-email-mikelley@microsoft.com>
2021-03-03 19:29   ` [PATCH v8 2/6] arm64: hyperv: Add Hyper-V clocksource/clockevent support Sunil Muthuswamy
2021-03-03 19:29     ` Sunil Muthuswamy
     [not found] ` <1614649360-5087-5-git-send-email-mikelley@microsoft.com>
2021-03-03 20:21   ` [PATCH v8 4/6] arm64: hyperv: Initialize hypervisor on boot Sunil Muthuswamy
2021-03-03 20:21     ` Sunil Muthuswamy
2021-03-05 21:03     ` Michael Kelley
2021-03-05 21:03       ` Michael Kelley
     [not found] ` <1614649360-5087-4-git-send-email-mikelley@microsoft.com>
2021-03-03 21:29   ` [PATCH v8 3/6] arm64: hyperv: Add kexec and panic handlers Sunil Muthuswamy
2021-03-03 21:29     ` Sunil Muthuswamy
2021-02-18 23:16 [PATCH v8 0/6] Enable Linux guests on Hyper-V on ARM64 Michael Kelley
2021-02-18 23:16 ` [PATCH v8 1/6] arm64: hyperv: Add Hyper-V hypercall and register access utilities Michael Kelley
2021-02-18 23:16   ` Michael Kelley
2021-02-22 10:20   ` Wei Liu
2021-02-22 10:20     ` Wei Liu
2021-02-24  2:37   ` Boqun Feng
2021-02-24  2:37     ` Boqun Feng
2021-03-05 20:50     ` Michael Kelley
2021-03-05 20:50       ` Michael Kelley

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.