[PATCH 0/2] hyperv: Move some features to common code

[PATCH 0/2] hyperv: Move some features to common code

[Nuno Das Neves]

There are several bits of Hyper-V-related code that today live in
arch/x86 but are not really specific to x86_64 and will work on arm64
too.

Some of these will be needed in the upcoming mshv driver code (for
Linux as root partition on Hyper-V). So this is a good time to move
them to hv_common.c.

Signed-off-by: Nuno Das Neves <nudasnev@microsoft.com>

Nuno Das Neves (2):
  hyperv: Move hv_current_partition_id to arch-generic code
  hyperv: Move create_vp and deposit_pages hvcalls to hv_common.c

 arch/arm64/hyperv/mshyperv.c    |   3 +
 arch/x86/hyperv/hv_init.c       |  25 +----
 arch/x86/hyperv/hv_proc.c       | 144 ---------------------------
 arch/x86/include/asm/mshyperv.h |   4 -
 drivers/hv/hv_common.c          | 168 ++++++++++++++++++++++++++++++++
 include/asm-generic/mshyperv.h  |   4 +
 6 files changed, 176 insertions(+), 172 deletions(-)

-- 
2.34.1

[PATCH 1/2] hyperv: Move hv_current_partition_id to arch-generic code

[Nuno Das Neves]

From: Nuno Das Neves <nudasnev@microsoft.com>

Make hv_current_partition_id available in both x86_64 and arm64.
This feature isn't specific to x86_64 and will be needed by common
code.

While at it, replace the BUG()s with WARN()s. Failing to get the id
need not crash the machine (although it is a very bad sign).

Signed-off-by: Nuno Das Neves <nudasnev@microsoft.com>
---
 arch/arm64/hyperv/mshyperv.c    |  3 +++
 arch/x86/hyperv/hv_init.c       | 25 +------------------------
 arch/x86/include/asm/mshyperv.h |  2 --
 drivers/hv/hv_common.c          | 23 +++++++++++++++++++++++
 include/asm-generic/mshyperv.h  |  2 ++
 5 files changed, 29 insertions(+), 26 deletions(-)

diff --git a/arch/arm64/hyperv/mshyperv.c b/arch/arm64/hyperv/mshyperv.c
index b1a4de4eee29..5050e748d266 100644
--- a/arch/arm64/hyperv/mshyperv.c
+++ b/arch/arm64/hyperv/mshyperv.c
@@ -19,6 +19,9 @@
 
 static bool hyperv_initialized;
 
+u64 hv_current_partition_id = HV_PARTITION_ID_SELF;
+EXPORT_SYMBOL_GPL(hv_current_partition_id);
+
 int hv_get_hypervisor_version(union hv_hypervisor_version_info *info)
 {
 	hv_get_vpreg_128(HV_REGISTER_HYPERVISOR_VERSION,
diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
index 95eada2994e1..950f5ccdb9d9 100644
--- a/arch/x86/hyperv/hv_init.c
+++ b/arch/x86/hyperv/hv_init.c
@@ -35,7 +35,7 @@
 #include <clocksource/hyperv_timer.h>
 #include <linux/highmem.h>
 
-u64 hv_current_partition_id = ~0ull;
+u64 hv_current_partition_id = HV_PARTITION_ID_SELF;
 EXPORT_SYMBOL_GPL(hv_current_partition_id);
 
 void *hv_hypercall_pg;
@@ -394,24 +394,6 @@ static void __init hv_stimer_setup_percpu_clockev(void)
 		old_setup_percpu_clockev();
 }
 
-static void __init hv_get_partition_id(void)
-{
-	struct hv_get_partition_id *output_page;
-	u64 status;
-	unsigned long flags;
-
-	local_irq_save(flags);
-	output_page = *this_cpu_ptr(hyperv_pcpu_output_arg);
-	status = hv_do_hypercall(HVCALL_GET_PARTITION_ID, NULL, output_page);
-	if (!hv_result_success(status)) {
-		/* No point in proceeding if this failed */
-		pr_err("Failed to get partition ID: %lld\n", status);
-		BUG();
-	}
-	hv_current_partition_id = output_page->partition_id;
-	local_irq_restore(flags);
-}
-
 #if IS_ENABLED(CONFIG_HYPERV_VTL_MODE)
 static u8 __init get_vtl(void)
 {
@@ -606,11 +588,6 @@ void __init hyperv_init(void)
 
 	register_syscore_ops(&hv_syscore_ops);
 
-	if (cpuid_ebx(HYPERV_CPUID_FEATURES) & HV_ACCESS_PARTITION_ID)
-		hv_get_partition_id();
-
-	BUG_ON(hv_root_partition && hv_current_partition_id == ~0ull);
-
 #ifdef CONFIG_PCI_MSI
 	/*
 	 * If we're running as root, we want to create our own PCI MSI domain.
diff --git a/arch/x86/include/asm/mshyperv.h b/arch/x86/include/asm/mshyperv.h
index 5f0bc6a6d025..9eeca2a6d047 100644
--- a/arch/x86/include/asm/mshyperv.h
+++ b/arch/x86/include/asm/mshyperv.h
@@ -44,8 +44,6 @@ extern bool hyperv_paravisor_present;
 
 extern void *hv_hypercall_pg;
 
-extern u64 hv_current_partition_id;
-
 extern union hv_ghcb * __percpu *hv_ghcb_pg;
 
 bool hv_isolation_type_snp(void);
diff --git a/drivers/hv/hv_common.c b/drivers/hv/hv_common.c
index 7a35c82976e0..819bcfd2b149 100644
--- a/drivers/hv/hv_common.c
+++ b/drivers/hv/hv_common.c
@@ -278,11 +278,34 @@ static void hv_kmsg_dump_register(void)
 	}
 }
 
+static void __init hv_get_partition_id(void)
+{
+	struct hv_get_partition_id *output_page;
+	u64 status;
+	unsigned long flags;
+
+	local_irq_save(flags);
+	output_page = *this_cpu_ptr(hyperv_pcpu_output_arg);
+	status = hv_do_hypercall(HVCALL_GET_PARTITION_ID, NULL, output_page);
+	if (!hv_result_success(status)) {
+		local_irq_restore(flags);
+		WARN(true, "Failed to get partition ID: %lld\n", status);
+		return;
+	}
+	hv_current_partition_id = output_page->partition_id;
+	local_irq_restore(flags);
+}
+
 int __init hv_common_init(void)
 {
 	int i;
 	union hv_hypervisor_version_info version;
 
+	if (ms_hyperv.priv_high & HV_ACCESS_PARTITION_ID)
+		hv_get_partition_id();
+
+	WARN_ON(hv_root_partition && hv_current_partition_id == HV_PARTITION_ID_SELF);
+
 	/* Get information about the Hyper-V host version */
 	if (!hv_get_hypervisor_version(&version))
 		pr_info("Hyper-V: Host Build %d.%d.%d.%d-%d-%d\n",
diff --git a/include/asm-generic/mshyperv.h b/include/asm-generic/mshyperv.h
index 8fe7aaab2599..8c4ff6e9aae7 100644
--- a/include/asm-generic/mshyperv.h
+++ b/include/asm-generic/mshyperv.h
@@ -60,6 +60,8 @@ struct ms_hyperv_info {
 extern struct ms_hyperv_info ms_hyperv;
 extern bool hv_nested;
 
+extern u64 hv_current_partition_id;
+
 extern void * __percpu *hyperv_pcpu_input_arg;
 extern void * __percpu *hyperv_pcpu_output_arg;
 
-- 
2.34.1

Re: [PATCH 1/2] hyperv: Move hv_current_partition_id to arch-generic code

[Wei Liu]

On Fri, Dec 06, 2024 at 02:21:46PM -0800, Nuno Das Neves wrote:
> From: Nuno Das Neves <nudasnev@microsoft.com>
> 
> Make hv_current_partition_id available in both x86_64 and arm64.
> This feature isn't specific to x86_64 and will be needed by common
> code.
> 
> While at it, replace the BUG()s with WARN()s. Failing to get the id
> need not crash the machine (although it is a very bad sign).
> 

A lot of things have changed since it was introduced. I don't remember
why I decided to use BUG() instead of WARN() in the first place.

If the system can still function without knowing its partition id, then
can this be removed completely? We can always use the SELF id.

Thanks,
Wei.

> Signed-off-by: Nuno Das Neves <nudasnev@microsoft.com>
> ---
>  arch/arm64/hyperv/mshyperv.c    |  3 +++
>  arch/x86/hyperv/hv_init.c       | 25 +------------------------
>  arch/x86/include/asm/mshyperv.h |  2 --
>  drivers/hv/hv_common.c          | 23 +++++++++++++++++++++++
>  include/asm-generic/mshyperv.h  |  2 ++
>  5 files changed, 29 insertions(+), 26 deletions(-)
> 
> diff --git a/arch/arm64/hyperv/mshyperv.c b/arch/arm64/hyperv/mshyperv.c
> index b1a4de4eee29..5050e748d266 100644
> --- a/arch/arm64/hyperv/mshyperv.c
> +++ b/arch/arm64/hyperv/mshyperv.c
> @@ -19,6 +19,9 @@
>  
>  static bool hyperv_initialized;
>  
> +u64 hv_current_partition_id = HV_PARTITION_ID_SELF;
> +EXPORT_SYMBOL_GPL(hv_current_partition_id);
> +
>  int hv_get_hypervisor_version(union hv_hypervisor_version_info *info)
>  {
>  	hv_get_vpreg_128(HV_REGISTER_HYPERVISOR_VERSION,
> diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
> index 95eada2994e1..950f5ccdb9d9 100644
> --- a/arch/x86/hyperv/hv_init.c
> +++ b/arch/x86/hyperv/hv_init.c
> @@ -35,7 +35,7 @@
>  #include <clocksource/hyperv_timer.h>
>  #include <linux/highmem.h>
>  
> -u64 hv_current_partition_id = ~0ull;
> +u64 hv_current_partition_id = HV_PARTITION_ID_SELF;
>  EXPORT_SYMBOL_GPL(hv_current_partition_id);
>  
>  void *hv_hypercall_pg;
> @@ -394,24 +394,6 @@ static void __init hv_stimer_setup_percpu_clockev(void)
>  		old_setup_percpu_clockev();
>  }
>  
> -static void __init hv_get_partition_id(void)
> -{
> -	struct hv_get_partition_id *output_page;
> -	u64 status;
> -	unsigned long flags;
> -
> -	local_irq_save(flags);
> -	output_page = *this_cpu_ptr(hyperv_pcpu_output_arg);
> -	status = hv_do_hypercall(HVCALL_GET_PARTITION_ID, NULL, output_page);
> -	if (!hv_result_success(status)) {
> -		/* No point in proceeding if this failed */
> -		pr_err("Failed to get partition ID: %lld\n", status);
> -		BUG();
> -	}
> -	hv_current_partition_id = output_page->partition_id;
> -	local_irq_restore(flags);
> -}
> -
>  #if IS_ENABLED(CONFIG_HYPERV_VTL_MODE)
>  static u8 __init get_vtl(void)
>  {
> @@ -606,11 +588,6 @@ void __init hyperv_init(void)
>  
>  	register_syscore_ops(&hv_syscore_ops);
>  
> -	if (cpuid_ebx(HYPERV_CPUID_FEATURES) & HV_ACCESS_PARTITION_ID)
> -		hv_get_partition_id();
> -
> -	BUG_ON(hv_root_partition && hv_current_partition_id == ~0ull);
> -
>  #ifdef CONFIG_PCI_MSI
>  	/*
>  	 * If we're running as root, we want to create our own PCI MSI domain.
> diff --git a/arch/x86/include/asm/mshyperv.h b/arch/x86/include/asm/mshyperv.h
> index 5f0bc6a6d025..9eeca2a6d047 100644
> --- a/arch/x86/include/asm/mshyperv.h
> +++ b/arch/x86/include/asm/mshyperv.h
> @@ -44,8 +44,6 @@ extern bool hyperv_paravisor_present;
>  
>  extern void *hv_hypercall_pg;
>  
> -extern u64 hv_current_partition_id;
> -
>  extern union hv_ghcb * __percpu *hv_ghcb_pg;
>  
>  bool hv_isolation_type_snp(void);
> diff --git a/drivers/hv/hv_common.c b/drivers/hv/hv_common.c
> index 7a35c82976e0..819bcfd2b149 100644
> --- a/drivers/hv/hv_common.c
> +++ b/drivers/hv/hv_common.c
> @@ -278,11 +278,34 @@ static void hv_kmsg_dump_register(void)
>  	}
>  }
>  
> +static void __init hv_get_partition_id(void)
> +{
> +	struct hv_get_partition_id *output_page;
> +	u64 status;
> +	unsigned long flags;
> +
> +	local_irq_save(flags);
> +	output_page = *this_cpu_ptr(hyperv_pcpu_output_arg);
> +	status = hv_do_hypercall(HVCALL_GET_PARTITION_ID, NULL, output_page);
> +	if (!hv_result_success(status)) {
> +		local_irq_restore(flags);
> +		WARN(true, "Failed to get partition ID: %lld\n", status);
> +		return;
> +	}
> +	hv_current_partition_id = output_page->partition_id;
> +	local_irq_restore(flags);
> +}
> +
>  int __init hv_common_init(void)
>  {
>  	int i;
>  	union hv_hypervisor_version_info version;
>  
> +	if (ms_hyperv.priv_high & HV_ACCESS_PARTITION_ID)
> +		hv_get_partition_id();
> +
> +	WARN_ON(hv_root_partition && hv_current_partition_id == HV_PARTITION_ID_SELF);
> +
>  	/* Get information about the Hyper-V host version */
>  	if (!hv_get_hypervisor_version(&version))
>  		pr_info("Hyper-V: Host Build %d.%d.%d.%d-%d-%d\n",
> diff --git a/include/asm-generic/mshyperv.h b/include/asm-generic/mshyperv.h
> index 8fe7aaab2599..8c4ff6e9aae7 100644
> --- a/include/asm-generic/mshyperv.h
> +++ b/include/asm-generic/mshyperv.h
> @@ -60,6 +60,8 @@ struct ms_hyperv_info {
>  extern struct ms_hyperv_info ms_hyperv;
>  extern bool hv_nested;
>  
> +extern u64 hv_current_partition_id;
> +
>  extern void * __percpu *hyperv_pcpu_input_arg;
>  extern void * __percpu *hyperv_pcpu_output_arg;
>  
> -- 
> 2.34.1
> 

Re: [PATCH 1/2] hyperv: Move hv_current_partition_id to arch-generic code

[Nuno Das Neves]

On 12/6/2024 11:36 PM, Wei Liu wrote:
> On Fri, Dec 06, 2024 at 02:21:46PM -0800, Nuno Das Neves wrote:
>> From: Nuno Das Neves <nudasnev@microsoft.com>
>>
>> Make hv_current_partition_id available in both x86_64 and arm64.
>> This feature isn't specific to x86_64 and will be needed by common
>> code.
>>
>> While at it, replace the BUG()s with WARN()s. Failing to get the id
>> need not crash the machine (although it is a very bad sign).
>>
> 
> A lot of things have changed since it was introduced. I don't remember
> why I decided to use BUG() instead of WARN() in the first place.
> 
> If the system can still function without knowing its partition id, then
> can this be removed completely? We can always use the SELF id.
> 

Missed this earlier, sorry.

I checked the code and it seems that HV_PARTITION_ID_SELF may be sufficient.
However, I'm not sure if there is some case where it is actually needed,
so I'm hesitant to remove it at the moment.

Nuno

> Thanks,
> Wei.
> 
>> Signed-off-by: Nuno Das Neves <nudasnev@microsoft.com>
>> ---
>>  arch/arm64/hyperv/mshyperv.c    |  3 +++
>>  arch/x86/hyperv/hv_init.c       | 25 +------------------------
>>  arch/x86/include/asm/mshyperv.h |  2 --
>>  drivers/hv/hv_common.c          | 23 +++++++++++++++++++++++
>>  include/asm-generic/mshyperv.h  |  2 ++
>>  5 files changed, 29 insertions(+), 26 deletions(-)
>>
>> diff --git a/arch/arm64/hyperv/mshyperv.c b/arch/arm64/hyperv/mshyperv.c
>> index b1a4de4eee29..5050e748d266 100644
>> --- a/arch/arm64/hyperv/mshyperv.c
>> +++ b/arch/arm64/hyperv/mshyperv.c
>> @@ -19,6 +19,9 @@
>>  
>>  static bool hyperv_initialized;
>>  
>> +u64 hv_current_partition_id = HV_PARTITION_ID_SELF;
>> +EXPORT_SYMBOL_GPL(hv_current_partition_id);
>> +
>>  int hv_get_hypervisor_version(union hv_hypervisor_version_info *info)
>>  {
>>  	hv_get_vpreg_128(HV_REGISTER_HYPERVISOR_VERSION,
>> diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
>> index 95eada2994e1..950f5ccdb9d9 100644
>> --- a/arch/x86/hyperv/hv_init.c
>> +++ b/arch/x86/hyperv/hv_init.c
>> @@ -35,7 +35,7 @@
>>  #include <clocksource/hyperv_timer.h>
>>  #include <linux/highmem.h>
>>  
>> -u64 hv_current_partition_id = ~0ull;
>> +u64 hv_current_partition_id = HV_PARTITION_ID_SELF;
>>  EXPORT_SYMBOL_GPL(hv_current_partition_id);
>>  
>>  void *hv_hypercall_pg;
>> @@ -394,24 +394,6 @@ static void __init hv_stimer_setup_percpu_clockev(void)
>>  		old_setup_percpu_clockev();
>>  }
>>  
>> -static void __init hv_get_partition_id(void)
>> -{
>> -	struct hv_get_partition_id *output_page;
>> -	u64 status;
>> -	unsigned long flags;
>> -
>> -	local_irq_save(flags);
>> -	output_page = *this_cpu_ptr(hyperv_pcpu_output_arg);
>> -	status = hv_do_hypercall(HVCALL_GET_PARTITION_ID, NULL, output_page);
>> -	if (!hv_result_success(status)) {
>> -		/* No point in proceeding if this failed */
>> -		pr_err("Failed to get partition ID: %lld\n", status);
>> -		BUG();
>> -	}
>> -	hv_current_partition_id = output_page->partition_id;
>> -	local_irq_restore(flags);
>> -}
>> -
>>  #if IS_ENABLED(CONFIG_HYPERV_VTL_MODE)
>>  static u8 __init get_vtl(void)
>>  {
>> @@ -606,11 +588,6 @@ void __init hyperv_init(void)
>>  
>>  	register_syscore_ops(&hv_syscore_ops);
>>  
>> -	if (cpuid_ebx(HYPERV_CPUID_FEATURES) & HV_ACCESS_PARTITION_ID)
>> -		hv_get_partition_id();
>> -
>> -	BUG_ON(hv_root_partition && hv_current_partition_id == ~0ull);
>> -
>>  #ifdef CONFIG_PCI_MSI
>>  	/*
>>  	 * If we're running as root, we want to create our own PCI MSI domain.
>> diff --git a/arch/x86/include/asm/mshyperv.h b/arch/x86/include/asm/mshyperv.h
>> index 5f0bc6a6d025..9eeca2a6d047 100644
>> --- a/arch/x86/include/asm/mshyperv.h
>> +++ b/arch/x86/include/asm/mshyperv.h
>> @@ -44,8 +44,6 @@ extern bool hyperv_paravisor_present;
>>  
>>  extern void *hv_hypercall_pg;
>>  
>> -extern u64 hv_current_partition_id;
>> -
>>  extern union hv_ghcb * __percpu *hv_ghcb_pg;
>>  
>>  bool hv_isolation_type_snp(void);
>> diff --git a/drivers/hv/hv_common.c b/drivers/hv/hv_common.c
>> index 7a35c82976e0..819bcfd2b149 100644
>> --- a/drivers/hv/hv_common.c
>> +++ b/drivers/hv/hv_common.c
>> @@ -278,11 +278,34 @@ static void hv_kmsg_dump_register(void)
>>  	}
>>  }
>>  
>> +static void __init hv_get_partition_id(void)
>> +{
>> +	struct hv_get_partition_id *output_page;
>> +	u64 status;
>> +	unsigned long flags;
>> +
>> +	local_irq_save(flags);
>> +	output_page = *this_cpu_ptr(hyperv_pcpu_output_arg);
>> +	status = hv_do_hypercall(HVCALL_GET_PARTITION_ID, NULL, output_page);
>> +	if (!hv_result_success(status)) {
>> +		local_irq_restore(flags);
>> +		WARN(true, "Failed to get partition ID: %lld\n", status);
>> +		return;
>> +	}
>> +	hv_current_partition_id = output_page->partition_id;
>> +	local_irq_restore(flags);
>> +}
>> +
>>  int __init hv_common_init(void)
>>  {
>>  	int i;
>>  	union hv_hypervisor_version_info version;
>>  
>> +	if (ms_hyperv.priv_high & HV_ACCESS_PARTITION_ID)
>> +		hv_get_partition_id();
>> +
>> +	WARN_ON(hv_root_partition && hv_current_partition_id == HV_PARTITION_ID_SELF);
>> +
>>  	/* Get information about the Hyper-V host version */
>>  	if (!hv_get_hypervisor_version(&version))
>>  		pr_info("Hyper-V: Host Build %d.%d.%d.%d-%d-%d\n",
>> diff --git a/include/asm-generic/mshyperv.h b/include/asm-generic/mshyperv.h
>> index 8fe7aaab2599..8c4ff6e9aae7 100644
>> --- a/include/asm-generic/mshyperv.h
>> +++ b/include/asm-generic/mshyperv.h
>> @@ -60,6 +60,8 @@ struct ms_hyperv_info {
>>  extern struct ms_hyperv_info ms_hyperv;
>>  extern bool hv_nested;
>>  
>> +extern u64 hv_current_partition_id;
>> +
>>  extern void * __percpu *hyperv_pcpu_input_arg;
>>  extern void * __percpu *hyperv_pcpu_output_arg;
>>  
>> -- 
>> 2.34.1
>>

RE: [PATCH 1/2] hyperv: Move hv_current_partition_id to arch-generic code

[Michael Kelley]

From: Nuno Das Neves <nunodasneves@linux.microsoft.com> Sent: Friday, December 6, 2024 2:22 PM
> 
> Make hv_current_partition_id available in both x86_64 and arm64.
> This feature isn't specific to x86_64 and will be needed by common
> code.
> 
> While at it, replace the BUG()s with WARN()s. Failing to get the id
> need not crash the machine (although it is a very bad sign).
> 
> Signed-off-by: Nuno Das Neves <nudasnev@microsoft.com>
> ---
>  arch/arm64/hyperv/mshyperv.c    |  3 +++
>  arch/x86/hyperv/hv_init.c       | 25 +------------------------
>  arch/x86/include/asm/mshyperv.h |  2 --
>  drivers/hv/hv_common.c          | 23 +++++++++++++++++++++++
>  include/asm-generic/mshyperv.h  |  2 ++
>  5 files changed, 29 insertions(+), 26 deletions(-)
> 
> diff --git a/arch/arm64/hyperv/mshyperv.c b/arch/arm64/hyperv/mshyperv.c
> index b1a4de4eee29..5050e748d266 100644
> --- a/arch/arm64/hyperv/mshyperv.c
> +++ b/arch/arm64/hyperv/mshyperv.c
> @@ -19,6 +19,9 @@
> 
>  static bool hyperv_initialized;
> 
> +u64 hv_current_partition_id = HV_PARTITION_ID_SELF;
> +EXPORT_SYMBOL_GPL(hv_current_partition_id);
> +

Instead of adding a definition of hv_current_partition_id on
the arm64 side, couldn't the definition on the x86 side in
hv_init.c be moved to hv_common.c (or maybe somewhere
else that is specific to running in the root partition, per my
comments in the cover letter), so there is only one definition
shared by both architectures?

>  int hv_get_hypervisor_version(union hv_hypervisor_version_info *info)
>  {
>  	hv_get_vpreg_128(HV_REGISTER_HYPERVISOR_VERSION,
> diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
> index 95eada2994e1..950f5ccdb9d9 100644
> --- a/arch/x86/hyperv/hv_init.c
> +++ b/arch/x86/hyperv/hv_init.c
> @@ -35,7 +35,7 @@
>  #include <clocksource/hyperv_timer.h>
>  #include <linux/highmem.h>
> 
> -u64 hv_current_partition_id = ~0ull;
> +u64 hv_current_partition_id = HV_PARTITION_ID_SELF;
>  EXPORT_SYMBOL_GPL(hv_current_partition_id);
> 
>  void *hv_hypercall_pg;
> @@ -394,24 +394,6 @@ static void __init hv_stimer_setup_percpu_clockev(void)
>  		old_setup_percpu_clockev();
>  }
> 
> -static void __init hv_get_partition_id(void)
> -{
> -	struct hv_get_partition_id *output_page;
> -	u64 status;
> -	unsigned long flags;
> -
> -	local_irq_save(flags);
> -	output_page = *this_cpu_ptr(hyperv_pcpu_output_arg);
> -	status = hv_do_hypercall(HVCALL_GET_PARTITION_ID, NULL, output_page);
> -	if (!hv_result_success(status)) {
> -		/* No point in proceeding if this failed */
> -		pr_err("Failed to get partition ID: %lld\n", status);
> -		BUG();
> -	}
> -	hv_current_partition_id = output_page->partition_id;
> -	local_irq_restore(flags);
> -}
> -
>  #if IS_ENABLED(CONFIG_HYPERV_VTL_MODE)
>  static u8 __init get_vtl(void)
>  {
> @@ -606,11 +588,6 @@ void __init hyperv_init(void)
> 
>  	register_syscore_ops(&hv_syscore_ops);
> 
> -	if (cpuid_ebx(HYPERV_CPUID_FEATURES) & HV_ACCESS_PARTITION_ID)
> -		hv_get_partition_id();
> -
> -	BUG_ON(hv_root_partition && hv_current_partition_id == ~0ull);
> -
>  #ifdef CONFIG_PCI_MSI
>  	/*
>  	 * If we're running as root, we want to create our own PCI MSI domain.
> diff --git a/arch/x86/include/asm/mshyperv.h b/arch/x86/include/asm/mshyperv.h
> index 5f0bc6a6d025..9eeca2a6d047 100644
> --- a/arch/x86/include/asm/mshyperv.h
> +++ b/arch/x86/include/asm/mshyperv.h
> @@ -44,8 +44,6 @@ extern bool hyperv_paravisor_present;
> 
>  extern void *hv_hypercall_pg;
> 
> -extern u64 hv_current_partition_id;
> -
>  extern union hv_ghcb * __percpu *hv_ghcb_pg;
> 
>  bool hv_isolation_type_snp(void);
> diff --git a/drivers/hv/hv_common.c b/drivers/hv/hv_common.c
> index 7a35c82976e0..819bcfd2b149 100644
> --- a/drivers/hv/hv_common.c
> +++ b/drivers/hv/hv_common.c
> @@ -278,11 +278,34 @@ static void hv_kmsg_dump_register(void)
>  	}
>  }
> 
> +static void __init hv_get_partition_id(void)
> +{
> +	struct hv_get_partition_id *output_page;
> +	u64 status;
> +	unsigned long flags;
> +
> +	local_irq_save(flags);
> +	output_page = *this_cpu_ptr(hyperv_pcpu_output_arg);
> +	status = hv_do_hypercall(HVCALL_GET_PARTITION_ID, NULL, output_page);
> +	if (!hv_result_success(status)) {
> +		local_irq_restore(flags);
> +		WARN(true, "Failed to get partition ID: %lld\n", status);
> +		return;
> +	}
> +	hv_current_partition_id = output_page->partition_id;
> +	local_irq_restore(flags);
> +}
> +
>  int __init hv_common_init(void)
>  {
>  	int i;
>  	union hv_hypervisor_version_info version;
> 
> +	if (ms_hyperv.priv_high & HV_ACCESS_PARTITION_ID)
> +		hv_get_partition_id();

hv_get_partition_id() uses the hyperv_pcpu_output_arg, and at
this point, hyperv_pcpu_output_arg isn't set. That setup
is done later in hv_common_init().

> +
> +	WARN_ON(hv_root_partition && hv_current_partition_id == HV_PARTITION_ID_SELF);
> +

Since the hypercall will fail cleanly if the calling VM doesn't
have the HV_ACCESS_PARTITION_ID privilege, could the
above be simplified to just this?

	if (hv_root_partition)
		hv_get_partition_id():

A non-root partition VM doesn't need to get the partition ID, while a
root partition should have the privilege. If the hypercall fails, there's
already a WARN, so there's no value in doing another WARN. Also if
the hypercall succeeds, it presumably returns a specific partitionID, not
HV_PARTITION_ID_SELF, so we know we have what we want.

There's already an "if (hv_root_partition)" statement for setting up
the hyperv_pcpu_output_arg. The call to hv_get_partition_id() could
go under that existing "if" *after* the hyperv_pcpu_output_arg is
set. :-)

Michael

>  	/* Get information about the Hyper-V host version */
>  	if (!hv_get_hypervisor_version(&version))
>  		pr_info("Hyper-V: Host Build %d.%d.%d.%d-%d-%d\n",
> diff --git a/include/asm-generic/mshyperv.h b/include/asm-generic/mshyperv.h
> index 8fe7aaab2599..8c4ff6e9aae7 100644
> --- a/include/asm-generic/mshyperv.h
> +++ b/include/asm-generic/mshyperv.h
> @@ -60,6 +60,8 @@ struct ms_hyperv_info {
>  extern struct ms_hyperv_info ms_hyperv;
>  extern bool hv_nested;
> 
> +extern u64 hv_current_partition_id;
> +
>  extern void * __percpu *hyperv_pcpu_input_arg;
>  extern void * __percpu *hyperv_pcpu_output_arg;
> 
> --
> 2.34.1

Re: [PATCH 1/2] hyperv: Move hv_current_partition_id to arch-generic code

[Nuno Das Neves]

On 12/7/2024 7:01 PM, Michael Kelley wrote:
> From: Nuno Das Neves <nunodasneves@linux.microsoft.com> Sent: Friday, December 6, 2024 2:22 PM
>>
>> Make hv_current_partition_id available in both x86_64 and arm64.
>> This feature isn't specific to x86_64 and will be needed by common
>> code.
>>
>> While at it, replace the BUG()s with WARN()s. Failing to get the id
>> need not crash the machine (although it is a very bad sign).
>>
>> Signed-off-by: Nuno Das Neves <nudasnev@microsoft.com>
>> ---
>>  arch/arm64/hyperv/mshyperv.c    |  3 +++
>>  arch/x86/hyperv/hv_init.c       | 25 +------------------------
>>  arch/x86/include/asm/mshyperv.h |  2 --
>>  drivers/hv/hv_common.c          | 23 +++++++++++++++++++++++
>>  include/asm-generic/mshyperv.h  |  2 ++
>>  5 files changed, 29 insertions(+), 26 deletions(-)
>>
>> diff --git a/arch/arm64/hyperv/mshyperv.c b/arch/arm64/hyperv/mshyperv.c
>> index b1a4de4eee29..5050e748d266 100644
>> --- a/arch/arm64/hyperv/mshyperv.c
>> +++ b/arch/arm64/hyperv/mshyperv.c
>> @@ -19,6 +19,9 @@
>>
>>  static bool hyperv_initialized;
>>
>> +u64 hv_current_partition_id = HV_PARTITION_ID_SELF;
>> +EXPORT_SYMBOL_GPL(hv_current_partition_id);
>> +
> 
> Instead of adding a definition of hv_current_partition_id on
> the arm64 side, couldn't the definition on the x86 side in
> hv_init.c be moved to hv_common.c (or maybe somewhere
> else that is specific to running in the root partition, per my
> comments in the cover letter), so there is only one definition
> shared by both architectures?
> 
Yes, that's a better idea.

>>  int hv_get_hypervisor_version(union hv_hypervisor_version_info *info)
>>  {
>>  	hv_get_vpreg_128(HV_REGISTER_HYPERVISOR_VERSION,
>> diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
>> index 95eada2994e1..950f5ccdb9d9 100644
>> --- a/arch/x86/hyperv/hv_init.c
>> +++ b/arch/x86/hyperv/hv_init.c
>> @@ -35,7 +35,7 @@
>>  #include <clocksource/hyperv_timer.h>
>>  #include <linux/highmem.h>
>>
>> -u64 hv_current_partition_id = ~0ull;
>> +u64 hv_current_partition_id = HV_PARTITION_ID_SELF;
>>  EXPORT_SYMBOL_GPL(hv_current_partition_id);
>>
>>  void *hv_hypercall_pg;
>> @@ -394,24 +394,6 @@ static void __init hv_stimer_setup_percpu_clockev(void)
>>  		old_setup_percpu_clockev();
>>  }
>>
>> -static void __init hv_get_partition_id(void)
>> -{
>> -	struct hv_get_partition_id *output_page;
>> -	u64 status;
>> -	unsigned long flags;
>> -
>> -	local_irq_save(flags);
>> -	output_page = *this_cpu_ptr(hyperv_pcpu_output_arg);
>> -	status = hv_do_hypercall(HVCALL_GET_PARTITION_ID, NULL, output_page);
>> -	if (!hv_result_success(status)) {
>> -		/* No point in proceeding if this failed */
>> -		pr_err("Failed to get partition ID: %lld\n", status);
>> -		BUG();
>> -	}
>> -	hv_current_partition_id = output_page->partition_id;
>> -	local_irq_restore(flags);
>> -}
>> -
>>  #if IS_ENABLED(CONFIG_HYPERV_VTL_MODE)
>>  static u8 __init get_vtl(void)
>>  {
>> @@ -606,11 +588,6 @@ void __init hyperv_init(void)
>>
>>  	register_syscore_ops(&hv_syscore_ops);
>>
>> -	if (cpuid_ebx(HYPERV_CPUID_FEATURES) & HV_ACCESS_PARTITION_ID)
>> -		hv_get_partition_id();
>> -
>> -	BUG_ON(hv_root_partition && hv_current_partition_id == ~0ull);
>> -
>>  #ifdef CONFIG_PCI_MSI
>>  	/*
>>  	 * If we're running as root, we want to create our own PCI MSI domain.
>> diff --git a/arch/x86/include/asm/mshyperv.h b/arch/x86/include/asm/mshyperv.h
>> index 5f0bc6a6d025..9eeca2a6d047 100644
>> --- a/arch/x86/include/asm/mshyperv.h
>> +++ b/arch/x86/include/asm/mshyperv.h
>> @@ -44,8 +44,6 @@ extern bool hyperv_paravisor_present;
>>
>>  extern void *hv_hypercall_pg;
>>
>> -extern u64 hv_current_partition_id;
>> -
>>  extern union hv_ghcb * __percpu *hv_ghcb_pg;
>>
>>  bool hv_isolation_type_snp(void);
>> diff --git a/drivers/hv/hv_common.c b/drivers/hv/hv_common.c
>> index 7a35c82976e0..819bcfd2b149 100644
>> --- a/drivers/hv/hv_common.c
>> +++ b/drivers/hv/hv_common.c
>> @@ -278,11 +278,34 @@ static void hv_kmsg_dump_register(void)
>>  	}
>>  }
>>
>> +static void __init hv_get_partition_id(void)
>> +{
>> +	struct hv_get_partition_id *output_page;
>> +	u64 status;
>> +	unsigned long flags;
>> +
>> +	local_irq_save(flags);
>> +	output_page = *this_cpu_ptr(hyperv_pcpu_output_arg);
>> +	status = hv_do_hypercall(HVCALL_GET_PARTITION_ID, NULL, output_page);
>> +	if (!hv_result_success(status)) {
>> +		local_irq_restore(flags);
>> +		WARN(true, "Failed to get partition ID: %lld\n", status);
>> +		return;
>> +	}
>> +	hv_current_partition_id = output_page->partition_id;
>> +	local_irq_restore(flags);
>> +}
>> +
>>  int __init hv_common_init(void)
>>  {
>>  	int i;
>>  	union hv_hypervisor_version_info version;
>>
>> +	if (ms_hyperv.priv_high & HV_ACCESS_PARTITION_ID)
>> +		hv_get_partition_id();
> 
> hv_get_partition_id() uses the hyperv_pcpu_output_arg, and at
> this point, hyperv_pcpu_output_arg isn't set. That setup
> is done later in hv_common_init().
> 
>> +
>> +	WARN_ON(hv_root_partition && hv_current_partition_id == HV_PARTITION_ID_SELF);
>> +
> 
> Since the hypercall will fail cleanly if the calling VM doesn't
> have the HV_ACCESS_PARTITION_ID privilege, could the
> above be simplified to just this?
> 
> 	if (hv_root_partition)
> 		hv_get_partition_id():
> 
> A non-root partition VM doesn't need to get the partition ID, while a
> root partition should have the privilege. If the hypercall fails, there's
> already a WARN, so there's no value in doing another WARN. Also if
> the hypercall succeeds, it presumably returns a specific partitionID, not
> HV_PARTITION_ID_SELF, so we know we have what we want.
> 
> There's already an "if (hv_root_partition)" statement for setting up
> the hyperv_pcpu_output_arg. The call to hv_get_partition_id() could
> go under that existing "if" *after* the hyperv_pcpu_output_arg is
> set. :-)
> 
Thank you, that makes sense. I'll make the changes you suggested for v2.

Nuno

> Michael
> 
>>  	/* Get information about the Hyper-V host version */
>>  	if (!hv_get_hypervisor_version(&version))
>>  		pr_info("Hyper-V: Host Build %d.%d.%d.%d-%d-%d\n",
>> diff --git a/include/asm-generic/mshyperv.h b/include/asm-generic/mshyperv.h
>> index 8fe7aaab2599..8c4ff6e9aae7 100644
>> --- a/include/asm-generic/mshyperv.h
>> +++ b/include/asm-generic/mshyperv.h
>> @@ -60,6 +60,8 @@ struct ms_hyperv_info {
>>  extern struct ms_hyperv_info ms_hyperv;
>>  extern bool hv_nested;
>>
>> +extern u64 hv_current_partition_id;
>> +
>>  extern void * __percpu *hyperv_pcpu_input_arg;
>>  extern void * __percpu *hyperv_pcpu_output_arg;
>>
>> --
>> 2.34.1

[PATCH 2/2] hyperv: Move create_vp and deposit_pages hvcalls to hv_common.c

[Nuno Das Neves]

From: Nuno Das Neves <nudasnev@microsoft.com>

These are not specific to x86_64 and will be needed by common code.

Signed-off-by: Nuno Das Neves <nudasnev@microsoft.com>
---
 arch/x86/hyperv/hv_proc.c       | 144 -------------------------------
 arch/x86/include/asm/mshyperv.h |   2 -
 drivers/hv/hv_common.c          | 145 ++++++++++++++++++++++++++++++++
 include/asm-generic/mshyperv.h  |   2 +
 4 files changed, 147 insertions(+), 146 deletions(-)

diff --git a/arch/x86/hyperv/hv_proc.c b/arch/x86/hyperv/hv_proc.c
index 3fa1f2ee7b0d..119354d00637 100644
--- a/arch/x86/hyperv/hv_proc.c
+++ b/arch/x86/hyperv/hv_proc.c
@@ -10,109 +10,8 @@
 #include <asm/hypervisor.h>
 #include <asm/mshyperv.h>
 #include <asm/apic.h>
-
 #include <asm/trace/hyperv.h>
 
-/*
- * See struct hv_deposit_memory. The first u64 is partition ID, the rest
- * are GPAs.
- */
-#define HV_DEPOSIT_MAX (HV_HYP_PAGE_SIZE / sizeof(u64) - 1)
-
-/* Deposits exact number of pages. Must be called with interrupts enabled.  */
-int hv_call_deposit_pages(int node, u64 partition_id, u32 num_pages)
-{
-	struct page **pages, *page;
-	int *counts;
-	int num_allocations;
-	int i, j, page_count;
-	int order;
-	u64 status;
-	int ret;
-	u64 base_pfn;
-	struct hv_deposit_memory *input_page;
-	unsigned long flags;
-
-	if (num_pages > HV_DEPOSIT_MAX)
-		return -E2BIG;
-	if (!num_pages)
-		return 0;
-
-	/* One buffer for page pointers and counts */
-	page = alloc_page(GFP_KERNEL);
-	if (!page)
-		return -ENOMEM;
-	pages = page_address(page);
-
-	counts = kcalloc(HV_DEPOSIT_MAX, sizeof(int), GFP_KERNEL);
-	if (!counts) {
-		free_page((unsigned long)pages);
-		return -ENOMEM;
-	}
-
-	/* Allocate all the pages before disabling interrupts */
-	i = 0;
-
-	while (num_pages) {
-		/* Find highest order we can actually allocate */
-		order = 31 - __builtin_clz(num_pages);
-
-		while (1) {
-			pages[i] = alloc_pages_node(node, GFP_KERNEL, order);
-			if (pages[i])
-				break;
-			if (!order) {
-				ret = -ENOMEM;
-				num_allocations = i;
-				goto err_free_allocations;
-			}
-			--order;
-		}
-
-		split_page(pages[i], order);
-		counts[i] = 1 << order;
-		num_pages -= counts[i];
-		i++;
-	}
-	num_allocations = i;
-
-	local_irq_save(flags);
-
-	input_page = *this_cpu_ptr(hyperv_pcpu_input_arg);
-
-	input_page->partition_id = partition_id;
-
-	/* Populate gpa_page_list - these will fit on the input page */
-	for (i = 0, page_count = 0; i < num_allocations; ++i) {
-		base_pfn = page_to_pfn(pages[i]);
-		for (j = 0; j < counts[i]; ++j, ++page_count)
-			input_page->gpa_page_list[page_count] = base_pfn + j;
-	}
-	status = hv_do_rep_hypercall(HVCALL_DEPOSIT_MEMORY,
-				     page_count, 0, input_page, NULL);
-	local_irq_restore(flags);
-	if (!hv_result_success(status)) {
-		pr_err("Failed to deposit pages: %lld\n", status);
-		ret = hv_result(status);
-		goto err_free_allocations;
-	}
-
-	ret = 0;
-	goto free_buf;
-
-err_free_allocations:
-	for (i = 0; i < num_allocations; ++i) {
-		base_pfn = page_to_pfn(pages[i]);
-		for (j = 0; j < counts[i]; ++j)
-			__free_page(pfn_to_page(base_pfn + j));
-	}
-
-free_buf:
-	free_page((unsigned long)pages);
-	kfree(counts);
-	return ret;
-}
-
 int hv_call_add_logical_proc(int node, u32 lp_index, u32 apic_id)
 {
 	struct hv_input_add_logical_processor *input;
@@ -154,46 +53,3 @@ int hv_call_add_logical_proc(int node, u32 lp_index, u32 apic_id)
 	return ret;
 }
 
-int hv_call_create_vp(int node, u64 partition_id, u32 vp_index, u32 flags)
-{
-	struct hv_create_vp *input;
-	u64 status;
-	unsigned long irq_flags;
-	int ret = HV_STATUS_SUCCESS;
-
-	/* Root VPs don't seem to need pages deposited */
-	if (partition_id != hv_current_partition_id) {
-		/* The value 90 is empirically determined. It may change. */
-		ret = hv_call_deposit_pages(node, partition_id, 90);
-		if (ret)
-			return ret;
-	}
-
-	do {
-		local_irq_save(irq_flags);
-
-		input = *this_cpu_ptr(hyperv_pcpu_input_arg);
-
-		input->partition_id = partition_id;
-		input->vp_index = vp_index;
-		input->flags = flags;
-		input->subnode_type = HvSubnodeAny;
-		input->proximity_domain_info = hv_numa_node_to_pxm_info(node);
-		status = hv_do_hypercall(HVCALL_CREATE_VP, input, NULL);
-		local_irq_restore(irq_flags);
-
-		if (hv_result(status) != HV_STATUS_INSUFFICIENT_MEMORY) {
-			if (!hv_result_success(status)) {
-				pr_err("%s: vcpu %u, lp %u, %lld\n", __func__,
-				       vp_index, flags, status);
-				ret = hv_result(status);
-			}
-			break;
-		}
-		ret = hv_call_deposit_pages(node, partition_id, 1);
-
-	} while (!ret);
-
-	return ret;
-}
-
diff --git a/arch/x86/include/asm/mshyperv.h b/arch/x86/include/asm/mshyperv.h
index 9eeca2a6d047..5bad88cfccba 100644
--- a/arch/x86/include/asm/mshyperv.h
+++ b/arch/x86/include/asm/mshyperv.h
@@ -57,9 +57,7 @@ u64 hv_tdx_hypercall(u64 control, u64 param1, u64 param2);
 #define HV_AP_INIT_GPAT_DEFAULT		0x0007040600070406ULL
 #define HV_AP_SEGMENT_LIMIT		0xffffffff
 
-int hv_call_deposit_pages(int node, u64 partition_id, u32 num_pages);
 int hv_call_add_logical_proc(int node, u32 lp_index, u32 acpi_id);
-int hv_call_create_vp(int node, u64 partition_id, u32 vp_index, u32 flags);
 
 /*
  * If the hypercall involves no input or output parameters, the hypervisor
diff --git a/drivers/hv/hv_common.c b/drivers/hv/hv_common.c
index 819bcfd2b149..591cc51e8817 100644
--- a/drivers/hv/hv_common.c
+++ b/drivers/hv/hv_common.c
@@ -679,3 +679,148 @@ u64 __weak hv_tdx_hypercall(u64 control, u64 param1, u64 param2)
 	return HV_STATUS_INVALID_PARAMETER;
 }
 EXPORT_SYMBOL_GPL(hv_tdx_hypercall);
+
+int hv_call_create_vp(int node, u64 partition_id, u32 vp_index, u32 flags)
+{
+	struct hv_create_vp *input;
+	u64 status;
+	unsigned long irq_flags;
+	int ret = HV_STATUS_SUCCESS;
+
+	/* Root VPs don't seem to need pages deposited */
+	if (partition_id != hv_current_partition_id) {
+		/* The value 90 is empirically determined. It may change. */
+		ret = hv_call_deposit_pages(node, partition_id, 90);
+		if (ret)
+			return ret;
+	}
+
+	do {
+		local_irq_save(irq_flags);
+
+		input = *this_cpu_ptr(hyperv_pcpu_input_arg);
+
+		input->partition_id = partition_id;
+		input->vp_index = vp_index;
+		input->flags = flags;
+		input->subnode_type = HvSubnodeAny;
+		input->proximity_domain_info = hv_numa_node_to_pxm_info(node);
+		status = hv_do_hypercall(HVCALL_CREATE_VP, input, NULL);
+		local_irq_restore(irq_flags);
+
+		if (hv_result(status) != HV_STATUS_INSUFFICIENT_MEMORY) {
+			if (!hv_result_success(status)) {
+				pr_err("%s: vcpu %u, lp %u, %lld\n", __func__,
+				       vp_index, flags, status);
+				ret = hv_result(status);
+			}
+			break;
+		}
+		ret = hv_call_deposit_pages(node, partition_id, 1);
+
+	} while (!ret);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(hv_call_create_vp);
+
+/*
+ * See struct hv_deposit_memory. The first u64 is partition ID, the rest
+ * are GPAs.
+ */
+#define HV_DEPOSIT_MAX (HV_HYP_PAGE_SIZE / sizeof(u64) - 1)
+
+/* Deposits exact number of pages. Must be called with interrupts enabled.  */
+int hv_call_deposit_pages(int node, u64 partition_id, u32 num_pages)
+{
+	struct page **pages, *page;
+	int *counts;
+	int num_allocations;
+	int i, j, page_count;
+	int order;
+	u64 status;
+	int ret;
+	u64 base_pfn;
+	struct hv_deposit_memory *input_page;
+	unsigned long flags;
+
+	if (num_pages > HV_DEPOSIT_MAX)
+		return -E2BIG;
+	if (!num_pages)
+		return 0;
+
+	/* One buffer for page pointers and counts */
+	page = alloc_page(GFP_KERNEL);
+	if (!page)
+		return -ENOMEM;
+	pages = page_address(page);
+
+	counts = kcalloc(HV_DEPOSIT_MAX, sizeof(int), GFP_KERNEL);
+	if (!counts) {
+		free_page((unsigned long)pages);
+		return -ENOMEM;
+	}
+
+	/* Allocate all the pages before disabling interrupts */
+	i = 0;
+
+	while (num_pages) {
+		/* Find highest order we can actually allocate */
+		order = 31 - __builtin_clz(num_pages);
+
+		while (1) {
+			pages[i] = alloc_pages_node(node, GFP_KERNEL, order);
+			if (pages[i])
+				break;
+			if (!order) {
+				ret = -ENOMEM;
+				num_allocations = i;
+				goto err_free_allocations;
+			}
+			--order;
+		}
+
+		split_page(pages[i], order);
+		counts[i] = 1 << order;
+		num_pages -= counts[i];
+		i++;
+	}
+	num_allocations = i;
+
+	local_irq_save(flags);
+
+	input_page = *this_cpu_ptr(hyperv_pcpu_input_arg);
+
+	input_page->partition_id = partition_id;
+
+	/* Populate gpa_page_list - these will fit on the input page */
+	for (i = 0, page_count = 0; i < num_allocations; ++i) {
+		base_pfn = page_to_pfn(pages[i]);
+		for (j = 0; j < counts[i]; ++j, ++page_count)
+			input_page->gpa_page_list[page_count] = base_pfn + j;
+	}
+	status = hv_do_rep_hypercall(HVCALL_DEPOSIT_MEMORY,
+				     page_count, 0, input_page, NULL);
+	local_irq_restore(flags);
+	if (!hv_result_success(status)) {
+		pr_err("Failed to deposit pages: %lld\n", status);
+		ret = hv_result(status);
+		goto err_free_allocations;
+	}
+
+	ret = 0;
+	goto free_buf;
+
+err_free_allocations:
+	for (i = 0; i < num_allocations; ++i) {
+		base_pfn = page_to_pfn(pages[i]);
+		for (j = 0; j < counts[i]; ++j)
+			__free_page(pfn_to_page(base_pfn + j));
+	}
+
+free_buf:
+	free_page((unsigned long)pages);
+	kfree(counts);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(hv_call_deposit_pages);
diff --git a/include/asm-generic/mshyperv.h b/include/asm-generic/mshyperv.h
index 8c4ff6e9aae7..46c0a6cae4e6 100644
--- a/include/asm-generic/mshyperv.h
+++ b/include/asm-generic/mshyperv.h
@@ -305,6 +305,8 @@ u64 hv_tdx_hypercall(u64 control, u64 param1, u64 param2);
 void hyperv_cleanup(void);
 bool hv_query_ext_cap(u64 cap_query);
 void hv_setup_dma_ops(struct device *dev, bool coherent);
+int hv_call_deposit_pages(int node, u64 partition_id, u32 num_pages);
+int hv_call_create_vp(int node, u64 partition_id, u32 vp_index, u32 flags);
 #else /* CONFIG_HYPERV */
 static inline bool hv_is_hyperv_initialized(void) { return false; }
 static inline bool hv_is_hibernation_supported(void) { return false; }
-- 
2.34.1

Re: [PATCH 2/2] hyperv: Move create_vp and deposit_pages hvcalls to hv_common.c

[Wei Liu]

On Fri, Dec 06, 2024 at 02:21:47PM -0800, Nuno Das Neves wrote:
> From: Nuno Das Neves <nudasnev@microsoft.com>
> 
> These are not specific to x86_64 and will be needed by common code.
> 
> Signed-off-by: Nuno Das Neves <nudasnev@microsoft.com>

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

RE: [PATCH 0/2] hyperv: Move some features to common code

[Michael Kelley]

From: Nuno Das Neves <nunodasneves@linux.microsoft.com> Sent: Friday, December 6, 2024 2:22 PM
> 
> There are several bits of Hyper-V-related code that today live in
> arch/x86 but are not really specific to x86_64 and will work on arm64
> too.
> 
> Some of these will be needed in the upcoming mshv driver code (for
> Linux as root partition on Hyper-V).

Previously, Linux as the root partition on Hyper-V was x86 only, which is
why the code is currently under arch/x86. So evidently the mshv driver
is being expanded to support both x86 and arm64, correct? Assuming
that's the case, I have some thoughts about how the source code should
be organized and built. It's probably best to get this right to start with so
it doesn't need to be changed again.

* Patch 2 of this series moves hv_call_deposit_pages() and
   hv_call_create_vp() to common code, but does not move
   hv_call_add_logical_proc(). All three are used together, so
   I'm wondering why hv_call_add_logical_proc() isn't moved.

* These three functions were originally put in a separate source
   code file because of being specific to running in the root partition,
   and not needed for generic Linux guest support. I think there's
   value in keeping them in a separate file, rather than merging them
   into hv_common.c. Maybe just move the entire hv_proc.c file?
   And then later, perhaps move the entire irqdomain.c file as well?
   There's also an interesting question of whether to move them into
   drivers/hv, or create a new directory virt/hyperv. Hyper-V support
   started out 15 years ago structured as a driver, hence "drivers/hv".
   But over the time, the support has become significantly more than
   just a driver, so "virt/hyperv" might be a better location for
   non-driver code that had previously been under arch/x86 but is
   now common to all architectures.

* Today, the code for running in the root partition is built along
   with the rest of the Hyper-V support, and so is present in kernels
   built for normal Linux guests on Hyper-V. I haven't thought about
   all the implications, but perhaps there's value in having a CONFIG
   option to build for the root partition, so that code can be dropped
   from normal kernels. There's a significant amount of new code still
   to come for mshv that could be excluded from normal guests in this
   way. Also, the tests of the hv_root_partition variable could be
   changed to a function the compiler detects is always "false" in a
   kernel built without the CONFIG option, in which case it can drop
   the code for where hv_root_partition is "true".

* The code currently in hv_proc.c is built for x86 only, and validly
   assumes the page size is 4K. But when the code moves to be
   common across architectures, that assumption is no longer
   valid in the general case. Perhaps the intent is that kernels for
   the root partition should always be built with page size 4K on
   arm64, but nothing enforces that intent. Personally, I think the code
   should be made to work with page sizes other than 4K so as to not
   leave technical debt. But I realize you may have other priorities. If
   there were a CONFIG option for building for the root partition,
   that option could be setup to enforce the 4K page size on arm64.

Anyway, thinking through these decisions up front could avoid
the need for additional moves later on.

Michael

> So this is a good time to move
> them to hv_common.c.
> 
> Signed-off-by: Nuno Das Neves <nudasnev@microsoft.com>
> 
> Nuno Das Neves (2):
>   hyperv: Move hv_current_partition_id to arch-generic code
>   hyperv: Move create_vp and deposit_pages hvcalls to hv_common.c
> 
>  arch/arm64/hyperv/mshyperv.c    |   3 +
>  arch/x86/hyperv/hv_init.c       |  25 +----
>  arch/x86/hyperv/hv_proc.c       | 144 ---------------------------
>  arch/x86/include/asm/mshyperv.h |   4 -
>  drivers/hv/hv_common.c          | 168 ++++++++++++++++++++++++++++++++
>  include/asm-generic/mshyperv.h  |   4 +
>  6 files changed, 176 insertions(+), 172 deletions(-)
> 
> --
> 2.34.1

Re: [PATCH 0/2] hyperv: Move some features to common code

[Nuno Das Neves]

On 12/7/2024 6:59 PM, Michael Kelley wrote:
> From: Nuno Das Neves <nunodasneves@linux.microsoft.com> Sent: Friday, December 6, 2024 2:22 PM
>>
>> There are several bits of Hyper-V-related code that today live in
>> arch/x86 but are not really specific to x86_64 and will work on arm64
>> too.
>>
>> Some of these will be needed in the upcoming mshv driver code (for
>> Linux as root partition on Hyper-V).
> 
> Previously, Linux as the root partition on Hyper-V was x86 only, which is
> why the code is currently under arch/x86. So evidently the mshv driver
> is being expanded to support both x86 and arm64, correct? Assuming
> that's the case, I have some thoughts about how the source code should
> be organized and built. It's probably best to get this right to start with so
> it doesn't need to be changed again.

Yes, we plan on supporting both architectures (eventually). I completely agree
that it's better to sort out these issues now rather than later.

> 
> * Patch 2 of this series moves hv_call_deposit_pages() and
>    hv_call_create_vp() to common code, but does not move
>    hv_call_add_logical_proc(). All three are used together, so
>    I'm wondering why hv_call_add_logical_proc() isn't moved.
> 

The only reason is that in our internal tree there's no common or arm64 code
yet that uses it - there is no reason it can't also become common code!

> * These three functions were originally put in a separate source
>    code file because of being specific to running in the root partition,
>    and not needed for generic Linux guest support. I think there's
>    value in keeping them in a separate file, rather than merging them
>    into hv_common.c. Maybe just move the entire hv_proc.c file?

Agreed. I think it should be renamed too - this file will eventually
contain some additional hypercall helper functions, some of which may also be
shared by the driver code. Something like "hv_call_common.c"?

>    And then later, perhaps move the entire irqdomain.c file as well?
Yes, may as well move it too.

>    There's also an interesting question of whether to move them into
>    drivers/hv, or create a new directory virt/hyperv. Hyper-V support
>    started out 15 years ago structured as a driver, hence "drivers/hv".
>    But over the time, the support has become significantly more than
>    just a driver, so "virt/hyperv" might be a better location for
>    non-driver code that had previously been under arch/x86 but is
>    now common to all architectures.
> 
I'd be fine with using "virt/hyperv", but I thought "virt" was only for
KVM.

Another option would be to create subdirectories in "drivers/hv" to
organize the different modules more cleanly (i.e. when the /dev/mshv
driver code is introduced).

> * Today, the code for running in the root partition is built along
>    with the rest of the Hyper-V support, and so is present in kernels
>    built for normal Linux guests on Hyper-V. I haven't thought about
>    all the implications, but perhaps there's value in having a CONFIG
>    option to build for the root partition, so that code can be dropped
>    from normal kernels. There's a significant amount of new code still
>    to come for mshv that could be excluded from normal guests in this
>    way. Also, the tests of the hv_root_partition variable could be
>    changed to a function the compiler detects is always "false" in a
>    kernel built without the CONFIG option, in which case it can drop
>    the code for where hv_root_partition is "true".
> 
Using hv_root_partition is a good way to do it, since it won't require
many #ifdefs or moving the existing code around too much.

I can certainly give it a try, and create a separate patch series
introducing the option. I suppose "CONFIG_HYPERV_ROOT" makes sense as a
name?

> * The code currently in hv_proc.c is built for x86 only, and validly
>    assumes the page size is 4K. But when the code moves to be
>    common across architectures, that assumption is no longer
>    valid in the general case. Perhaps the intent is that kernels for
>    the root partition should always be built with page size 4K on
>    arm64, but nothing enforces that intent. Personally, I think the code
>    should be made to work with page sizes other than 4K so as to not
>    leave technical debt. But I realize you may have other priorities. If
>    there were a CONFIG option for building for the root partition,
>    that option could be setup to enforce the 4K page size on arm64.
> 
That makes sense. I suppose this can be done by selecting PAGE_SIZE_4KB
under HYPERV in drivers/hv/Kconfig?

I'm not how easy it will be to make the code work with different page
sizes, since we use alloc_page() and similar in a few places, assuming 4k.

Thanks
Nuno

> Anyway, thinking through these decisions up front could avoid
> the need for additional moves later on.
> 
> Michael
> 
>> So this is a good time to move
>> them to hv_common.c.
>>
>> Signed-off-by: Nuno Das Neves <nudasnev@microsoft.com>
>>
>> Nuno Das Neves (2):
>>   hyperv: Move hv_current_partition_id to arch-generic code
>>   hyperv: Move create_vp and deposit_pages hvcalls to hv_common.c
>>
>>  arch/arm64/hyperv/mshyperv.c    |   3 +
>>  arch/x86/hyperv/hv_init.c       |  25 +----
>>  arch/x86/hyperv/hv_proc.c       | 144 ---------------------------
>>  arch/x86/include/asm/mshyperv.h |   4 -
>>  drivers/hv/hv_common.c          | 168 ++++++++++++++++++++++++++++++++
>>  include/asm-generic/mshyperv.h  |   4 +
>>  6 files changed, 176 insertions(+), 172 deletions(-)
>>
>> --
>> 2.34.1

RE: [PATCH 0/2] hyperv: Move some features to common code

[Michael Kelley]

From: Nuno Das Neves <nunodasneves@linux.microsoft.com> Sent: Monday, December 9, 2024 12:20 PM
> 
> On 12/7/2024 6:59 PM, Michael Kelley wrote:
> > From: Nuno Das Neves <nunodasneves@linux.microsoft.com> Sent: Friday, December 6, 2024 2:22 PM
> >>
> >> There are several bits of Hyper-V-related code that today live in
> >> arch/x86 but are not really specific to x86_64 and will work on arm64
> >> too.
> >>
> >> Some of these will be needed in the upcoming mshv driver code (for
> >> Linux as root partition on Hyper-V).
> >
> > Previously, Linux as the root partition on Hyper-V was x86 only, which is
> > why the code is currently under arch/x86. So evidently the mshv driver
> > is being expanded to support both x86 and arm64, correct? Assuming
> > that's the case, I have some thoughts about how the source code should
> > be organized and built. It's probably best to get this right to start with so
> > it doesn't need to be changed again.
> 
> Yes, we plan on supporting both architectures (eventually). I completely agree
> that it's better to sort out these issues now rather than later.
> 
> >
> > * Patch 2 of this series moves hv_call_deposit_pages() and
> >    hv_call_create_vp() to common code, but does not move
> >    hv_call_add_logical_proc(). All three are used together, so
> >    I'm wondering why hv_call_add_logical_proc() isn't moved.
> >
> 
> The only reason is that in our internal tree there's no common or arm64 code
> yet that uses it - there is no reason it can't also become common code!

Maybe I'm missing your point, but hv_call_add_logical_proc() and
hv_call_create_vp() are called in succession in hv_smp_prepare_cpus(),
so it seems like they very much go together. Presumably a similar
sequence will be needed on the arm64 side when running as the
root partition?

> 
> > * These three functions were originally put in a separate source
> >    code file because of being specific to running in the root partition,
> >    and not needed for generic Linux guest support. I think there's
> >    value in keeping them in a separate file, rather than merging them
> >    into hv_common.c. Maybe just move the entire hv_proc.c file?
> 
> Agreed. I think it should be renamed too - this file will eventually
> contain some additional hypercall helper functions, some of which may also be
> shared by the driver code. Something like "hv_call_common.c"?

I went back and looked at your patch series from a year ago [1], and
got a better understanding of where this work is headed. I wanted
to comment on that series back then, but got subsumed in wrapping
things up for my retirement. :-(  I see significant portions of that
series have been posted independently and accepted, so my further
comments here assume the rest of the series is still the macro-level
approach you are taking.

From that series, you are planning three modules, controlled by
CONFIG_MSHV, CONFIG_MSHV_ROOT, and CONFIG_MSHV_VTL.
That makes sense, and addresses one of my top-level concerns,
which is that normal guest kernels shouldn't include all that code.
And apparently that code works as a module as well as built-in.

The code currently in hv_proc.c is similar to the code in hv_call.c
and mshv_root_hv_call.c from that series -- it's a wrapper around
Hyper-V hypercalls. But a difference is that the code in hv_proc.c
can't be a module because it is called from hv_smp_prepare_cpus(),
which must be built-in. So it can't be added to the proposed
hv_call.c without making all of hv_call.c built-in. Ideally, there
would be a separate source file just for the code that must be
built-in. I'm not familiar enough with root partition requirements
to understand what hv_smp_prepare_cpus() and its calls to
hv_call_add_logical_proc() and hv_call_create_vp() are doing.
Evidently it is related to bringing up CPUs in the root partition,
and not related to guest VMs. But those two hv_call_* functions
would also be used for managing guest VMs from /dev/mshv.

As for the name, I don't really like "common", even though I'm the
one who created "hv_common.c" back when doing the initial arm64
support on Hyper-V. :-( My thinking is that anything that isn't under
arch/x86 or arch/arm64 is by definition shared across architectures,
so having "common" in the name is superfluous. Maybe just
staying with hv_proc.c is OK.

> 
> >    And then later, perhaps move the entire irqdomain.c file as well?
> Yes, may as well move it too.

irqdomain.c is also in that category of needing to be built-in. But
looking more closely, it is x86 specific and should stay where it is. I
can't immediately tell whether it's feasible to make the Hyper-V
IOMMU driver (and irqdomain.c) architecture neutral, or whether
a separate arm64 implementation will be needed. My guess is the
latter.

> 
> >    There's also an interesting question of whether to move them into
> >    drivers/hv, or create a new directory virt/hyperv. Hyper-V support
> >    started out 15 years ago structured as a driver, hence "drivers/hv".
> >    But over the time, the support has become significantly more than
> >    just a driver, so "virt/hyperv" might be a better location for
> >    non-driver code that had previously been under arch/x86 but is
> >    now common to all architectures.
> >
> I'd be fine with using "virt/hyperv", but I thought "virt" was only for
> KVM.

Now that I see the bigger picture from your previous patch series,
keeping the files in drivers/hv seems OK to me. The 'mshv' code *is*
a driver that implements /dev/mshv. :-)

> 
> Another option would be to create subdirectories in "drivers/hv" to
> organize the different modules more cleanly (i.e. when the /dev/mshv
> driver code is introduced).

Putting all the mshv and "running as root partition" files in a single
sub-directory might make sense. Multiple sub-directories might be
overkill. But I don't have a strong opinion either way. Putting them
all directly in drivers/hv seems OK, as does one or more sub-directories.

One thing I encountered back when first doing the arm64 support is
that everything in the drivers/hv directory could be built as a module.
The Hyper-V support under arch/x86 was always built-in, and the
arch/arm64 code I added was as well. But there was no obvious place
to put common code that must always be built-in. At the time, I thought
about introducing virt/hyperv, but had only a single relatively small source
code file, and introducing that new pathname with its own Makefile, etc.
seemed like overkill. So drivers/hv/hv_common.c came into existence. I
tweaked the existing drivers/hv/Makefile so hv_common.c is always
built-in even when CONFIG_HYPERV=m. It's a little messy, but not too
bad.

The difference in what can be built as a module vs. must be built-in
might be a factor in the directory structure, along with the
CONFIG options that control whether the root partition code
gets built at all (see below). The details will govern what works
well and what ends up being a bit of a mess.

> 
> > * Today, the code for running in the root partition is built along
> >    with the rest of the Hyper-V support, and so is present in kernels
> >    built for normal Linux guests on Hyper-V. I haven't thought about
> >    all the implications, but perhaps there's value in having a CONFIG
> >    option to build for the root partition, so that code can be dropped
> >    from normal kernels. There's a significant amount of new code still
> >    to come for mshv that could be excluded from normal guests in this
> >    way. Also, the tests of the hv_root_partition variable could be
> >    changed to a function the compiler detects is always "false" in a
> >    kernel built without the CONFIG option, in which case it can drop
> >    the code for where hv_root_partition is "true".
> >
> Using hv_root_partition is a good way to do it, since it won't require
> many #ifdefs or moving the existing code around too much.
> 
> I can certainly give it a try, and create a separate patch series
> introducing the option. I suppose "CONFIG_HYPERV_ROOT" makes sense as a
> name?

Now that I see you have CONFIG_MSHV, CONFIG_MSHV_ROOT, and
CONFIG_MSHV_VTL planned, could building the hv_root_partition code
just be under control of CONFIG_MSHV_ROOT without introducing
another CONFIG variable? Is any of the root partition code like
hv_proc.c and irqdomain.c needed if CONFIG_MSHV_ROOT=n?
Stubs will be needed for functions called from the generic kernel code
when CONFIG_MSHV_ROOT=n, but those are easily supplied in
asm/mshyperv.h. If hv_root_partition becomes a function whose
return value is gated by CONFIG_MSHV_ROOT, then the compiler
can know when the value is always "false" and drop even the code
that calls the stubs. But I'm pretty sure the stubs are still needed to
avoid compile errors, even when the compiler drops the code.

With this approach, you can avoid #ifdefs except in asm/mshyperv.h
for the stubs, and in the hv_root_partition() function.

One more code structure topic:  In the previous patch series,
some of the mshv code is x86 specific. There is x86 assembler
code, and references to x86 specific registers (all the HV_X64_*
registers). Do you plan to put architecture specific code under
drivers/hv, or under arch/[x86/arm64]/hyperv? We've made
drivers/hv be architecture neutral -- currently there's only one
"cheat" with an #ifdef CONFIG_ARM64 in the hv_balloon.c driver
that turns off some functionality.

As we discussed previously, the new Hyper-V #include files
provide the union of x86 and arm64 definitions, with just an
occasional #ifdef where needed. That was justified because
that's how the definitions come from the Windows/Hyper-V
world. But it seems like the mshv code should be structured
with arch specific code is under arch, not in drivers/hv with
#ifdefs. The Makefiles in arch/[x86,arm64]/hyperv will need
to use CONFIG_MSHV, CONFIG_MSHV_ROOT, and
CONFIG_MSHV_VTL to decide what to build, and whether to
build as a module vs. built-in.

Maybe putting the mshv code in a "mshv" subdirectory under
both drivers/hv and arch/[x86,arm64]/hyperv would conceptually
help tie together the arch neutral and arch specific portions.
Or something like that ....

> 
> > * The code currently in hv_proc.c is built for x86 only, and validly
> >    assumes the page size is 4K. But when the code moves to be
> >    common across architectures, that assumption is no longer
> >    valid in the general case. Perhaps the intent is that kernels for
> >    the root partition should always be built with page size 4K on
> >    arm64, but nothing enforces that intent. Personally, I think the code
> >    should be made to work with page sizes other than 4K so as to not
> >    leave technical debt. But I realize you may have other priorities. If
> >    there were a CONFIG option for building for the root partition,
> >    that option could be setup to enforce the 4K page size on arm64.
> >
> That makes sense. I suppose this can be done by selecting PAGE_SIZE_4KB
> under HYPERV in drivers/hv/Kconfig?

Since the PAGE_SIZE value is independently selectable in the .config
file, I'm not sure if you can override it when CONFIG_MSHV_ROOT is
set. But you can allow CONFIG_MSHV_ROOT to be set only if
PAGE_SIZE_4KB is selected on arm64. I'd have to look in more detail
to figure out the best way to create an appropriate dependency.

> 
> I'm not how easy it will be to make the code work with different page
> sizes, since we use alloc_page() and similar in a few places, assuming 4k.

alloc_page() is not the problem as it is relatively easy to break up a
16K or 64K page into multiple 4K pages when depositing memory.
And you can round up the amount of deposited memory to the
larger boundary without doing any harm. But in your previous patch
series, I see hv_call_withdraw_memory(), wherein Hyper-V gives
back individual 4K pages in no particular order. That's the killer, as
it is not feasible to re-assemble a random set of 4K pages into larger
16K or 64K pages for free_page().

So scratch that idea. :-( The root partition must run with a page
size that matches the size Hyper-V uses to do memory deposits to
and from a partition, and that's 4K.

Michael

[1] https://lore.kernel.org/lkml/1696010501-24584-1-git-send-email-nunodasneves@linux.microsoft.com/

Re: [PATCH 0/2] hyperv: Move some features to common code

[Nuno Das Neves]

On 12/17/2024 9:48 AM, Michael Kelley wrote:
> From: Nuno Das Neves <nunodasneves@linux.microsoft.com> Sent: Monday, December 9, 2024 12:20 PM
>>
>> On 12/7/2024 6:59 PM, Michael Kelley wrote:
>>> From: Nuno Das Neves <nunodasneves@linux.microsoft.com> Sent: Friday, December 6, 2024 2:22 PM
>>>>
>>>> There are several bits of Hyper-V-related code that today live in
>>>> arch/x86 but are not really specific to x86_64 and will work on arm64
>>>> too.
>>>>
>>>> Some of these will be needed in the upcoming mshv driver code (for
>>>> Linux as root partition on Hyper-V).
>>>
>>> Previously, Linux as the root partition on Hyper-V was x86 only, which is
>>> why the code is currently under arch/x86. So evidently the mshv driver
>>> is being expanded to support both x86 and arm64, correct? Assuming
>>> that's the case, I have some thoughts about how the source code should
>>> be organized and built. It's probably best to get this right to start with so
>>> it doesn't need to be changed again.
>>
>> Yes, we plan on supporting both architectures (eventually). I completely agree
>> that it's better to sort out these issues now rather than later.
>>
>>>
>>> * Patch 2 of this series moves hv_call_deposit_pages() and
>>>    hv_call_create_vp() to common code, but does not move
>>>    hv_call_add_logical_proc(). All three are used together, so
>>>    I'm wondering why hv_call_add_logical_proc() isn't moved.
>>>
>>
>> The only reason is that in our internal tree there's no common or arm64 code
>> yet that uses it - there is no reason it can't also become common code!
> 
> Maybe I'm missing your point, but hv_call_add_logical_proc() and
> hv_call_create_vp() are called in succession in hv_smp_prepare_cpus(),
> so it seems like they very much go together. Presumably a similar
> sequence will be needed on the arm64 side when running as the
> root partition?
> 

Yes that's right, sorry I wasn't being too clear.

>>
>>> * These three functions were originally put in a separate source
>>>    code file because of being specific to running in the root partition,
>>>    and not needed for generic Linux guest support. I think there's
>>>    value in keeping them in a separate file, rather than merging them
>>>    into hv_common.c. Maybe just move the entire hv_proc.c file?
>>
>> Agreed. I think it should be renamed too - this file will eventually
>> contain some additional hypercall helper functions, some of which may also be
>> shared by the driver code. Something like "hv_call_common.c"?
> 
> I went back and looked at your patch series from a year ago [1], and
> got a better understanding of where this work is headed. I wanted
> to comment on that series back then, but got subsumed in wrapping
> things up for my retirement. :-(  I see significant portions of that
> series have been posted independently and accepted, so my further
> comments here assume the rest of the series is still the macro-level
> approach you are taking.
> 
>>From that series, you are planning three modules, controlled by
> CONFIG_MSHV, CONFIG_MSHV_ROOT, and CONFIG_MSHV_VTL.
> That makes sense, and addresses one of my top-level concerns,
> which is that normal guest kernels shouldn't include all that code.
> And apparently that code works as a module as well as built-in.
> 
> The code currently in hv_proc.c is similar to the code in hv_call.c
> and mshv_root_hv_call.c from that series -- it's a wrapper around
> Hyper-V hypercalls. But a difference is that the code in hv_proc.c
> can't be a module because it is called from hv_smp_prepare_cpus(),
> which must be built-in. So it can't be added to the proposed
> hv_call.c without making all of hv_call.c built-in. Ideally, there
> would be a separate source file just for the code that must be
> built-in. I'm not familiar enough with root partition requirements
> to understand what hv_smp_prepare_cpus() and its calls to
> hv_call_add_logical_proc() and hv_call_create_vp() are doing.
> Evidently it is related to bringing up CPUs in the root partition,
> and not related to guest VMs. But those two hv_call_* functions
> would also be used for managing guest VMs from /dev/mshv.
> 

It makes sense to keep the different kinds of functionality
separated based on where they are needed. I do wonder if it will be
cumbersome to keep all the cases in their own files - builtin/module,
root/guest, and separated into arch directories vs drivers/hv...

> As for the name, I don't really like "common", even though I'm the
> one who created "hv_common.c" back when doing the initial arm64
> support on Hyper-V. :-( My thinking is that anything that isn't under
> arch/x86 or arch/arm64 is by definition shared across architectures,
> so having "common" in the name is superfluous. Maybe just
> staying with hv_proc.c is OK.
> 

I'll stick with hv_proc.c.

>>
>>>    And then later, perhaps move the entire irqdomain.c file as well?
>> Yes, may as well move it too.
> 
> irqdomain.c is also in that category of needing to be built-in. But
> looking more closely, it is x86 specific and should stay where it is. I
> can't immediately tell whether it's feasible to make the Hyper-V
> IOMMU driver (and irqdomain.c) architecture neutral, or whether
> a separate arm64 implementation will be needed. My guess is the
> latter.
> 
>>
>>>    There's also an interesting question of whether to move them into
>>>    drivers/hv, or create a new directory virt/hyperv. Hyper-V support
>>>    started out 15 years ago structured as a driver, hence "drivers/hv".
>>>    But over the time, the support has become significantly more than
>>>    just a driver, so "virt/hyperv" might be a better location for
>>>    non-driver code that had previously been under arch/x86 but is
>>>    now common to all architectures.
>>>
>> I'd be fine with using "virt/hyperv", but I thought "virt" was only for
>> KVM.
> 
> Now that I see the bigger picture from your previous patch series,
> keeping the files in drivers/hv seems OK to me. The 'mshv' code *is*
> a driver that implements /dev/mshv. :-)
> 
>>
>> Another option would be to create subdirectories in "drivers/hv" to
>> organize the different modules more cleanly (i.e. when the /dev/mshv
>> driver code is introduced).
> 
> Putting all the mshv and "running as root partition" files in a single
> sub-directory might make sense. Multiple sub-directories might be
> overkill. But I don't have a strong opinion either way. Putting them
> all directly in drivers/hv seems OK, as does one or more sub-directories.
> 
> One thing I encountered back when first doing the arm64 support is
> that everything in the drivers/hv directory could be built as a module.
> The Hyper-V support under arch/x86 was always built-in, and the
> arch/arm64 code I added was as well. But there was no obvious place
> to put common code that must always be built-in. At the time, I thought
> about introducing virt/hyperv, but had only a single relatively small source
> code file, and introducing that new pathname with its own Makefile, etc.
> seemed like overkill. So drivers/hv/hv_common.c came into existence. I
> tweaked the existing drivers/hv/Makefile so hv_common.c is always
> built-in even when CONFIG_HYPERV=m. It's a little messy, but not too
> bad.
> 
> The difference in what can be built as a module vs. must be built-in
> might be a factor in the directory structure, along with the
> CONFIG options that control whether the root partition code
> gets built at all (see below). The details will govern what works
> well and what ends up being a bit of a mess.
> 
>>
>>> * Today, the code for running in the root partition is built along
>>>    with the rest of the Hyper-V support, and so is present in kernels
>>>    built for normal Linux guests on Hyper-V. I haven't thought about
>>>    all the implications, but perhaps there's value in having a CONFIG
>>>    option to build for the root partition, so that code can be dropped
>>>    from normal kernels. There's a significant amount of new code still
>>>    to come for mshv that could be excluded from normal guests in this
>>>    way. Also, the tests of the hv_root_partition variable could be
>>>    changed to a function the compiler detects is always "false" in a
>>>    kernel built without the CONFIG option, in which case it can drop
>>>    the code for where hv_root_partition is "true".
>>>
>> Using hv_root_partition is a good way to do it, since it won't require
>> many #ifdefs or moving the existing code around too much.
>>
>> I can certainly give it a try, and create a separate patch series
>> introducing the option. I suppose "CONFIG_HYPERV_ROOT" makes sense as a
>> name?
> 
> Now that I see you have CONFIG_MSHV, CONFIG_MSHV_ROOT, and
> CONFIG_MSHV_VTL planned, could building the hv_root_partition code
> just be under control of CONFIG_MSHV_ROOT without introducing
> another CONFIG variable? Is any of the root partition code like
> hv_proc.c and irqdomain.c needed if CONFIG_MSHV_ROOT=n?

I will experiment with it - I think we can probably do it that way, yes.

> Stubs will be needed for functions called from the generic kernel code
> when CONFIG_MSHV_ROOT=n, but those are easily supplied in
> asm/mshyperv.h. If hv_root_partition becomes a function whose
> return value is gated by CONFIG_MSHV_ROOT, then the compiler
> can know when the value is always "false" and drop even the code
> that calls the stubs. But I'm pretty sure the stubs are still needed to
> avoid compile errors, even when the compiler drops the code.
> 
> With this approach, you can avoid #ifdefs except in asm/mshyperv.h
> for the stubs, and in the hv_root_partition() function.
> 
> One more code structure topic:  In the previous patch series,
> some of the mshv code is x86 specific. There is x86 assembler
> code, and references to x86 specific registers (all the HV_X64_*
> registers). Do you plan to put architecture specific code under
> drivers/hv, or under arch/[x86/arm64]/hyperv? We've made
> drivers/hv be architecture neutral -- currently there's only one
> "cheat" with an #ifdef CONFIG_ARM64 in the hv_balloon.c driver
> that turns off some functionality.

Today in our internal tree, all the module code lives in drivers/hv.
Some of the code is arch-neutral "in-theory", but uses hypervisor
features that are not yet supported on arm64, (but may be in future.)

For such features, we use #ifdefs in drivers/hv to conditionally compile
it. Note that today we only compile test arm64, we don't actually have
a running build for it yet, so the driver will be x86_64 only at first.

There is very little driver code that actually deals with architectural
details. If necessary, I imagine we would create arch-specific files
inside drivers/hv at first, and maybe move them to arch/ if appropriate.

> 
> As we discussed previously, the new Hyper-V #include files
> provide the union of x86 and arm64 definitions, with just an
> occasional #ifdef where needed. That was justified because
> that's how the definitions come from the Windows/Hyper-V
> world. But it seems like the mshv code should be structured
> with arch specific code is under arch, not in drivers/hv with
> #ifdefs. The Makefiles in arch/[x86,arm64]/hyperv will need
> to use CONFIG_MSHV, CONFIG_MSHV_ROOT, and
> CONFIG_MSHV_VTL to decide what to build, and whether to
> build as a module vs. built-in.
> 
> Maybe putting the mshv code in a "mshv" subdirectory under
> both drivers/hv and arch/[x86,arm64]/hyperv would conceptually
> help tie together the arch neutral and arch specific portions.
> Or something like that ....
> 

I think it could evolve to that point, but it's hard to say until we
get closer.

>>
>>> * The code currently in hv_proc.c is built for x86 only, and validly
>>>    assumes the page size is 4K. But when the code moves to be
>>>    common across architectures, that assumption is no longer
>>>    valid in the general case. Perhaps the intent is that kernels for
>>>    the root partition should always be built with page size 4K on
>>>    arm64, but nothing enforces that intent. Personally, I think the code
>>>    should be made to work with page sizes other than 4K so as to not
>>>    leave technical debt. But I realize you may have other priorities. If
>>>    there were a CONFIG option for building for the root partition,
>>>    that option could be setup to enforce the 4K page size on arm64.
>>>
>> That makes sense. I suppose this can be done by selecting PAGE_SIZE_4KB
>> under HYPERV in drivers/hv/Kconfig?
> 
> Since the PAGE_SIZE value is independently selectable in the .config
> file, I'm not sure if you can override it when CONFIG_MSHV_ROOT is
> set. But you can allow CONFIG_MSHV_ROOT to be set only if
> PAGE_SIZE_4KB is selected on arm64. I'd have to look in more detail
> to figure out the best way to create an appropriate dependency.
> 
>>
>> I'm not how easy it will be to make the code work with different page
>> sizes, since we use alloc_page() and similar in a few places, assuming 4k.
> 
> alloc_page() is not the problem as it is relatively easy to break up a
> 16K or 64K page into multiple 4K pages when depositing memory.
> And you can round up the amount of deposited memory to the
> larger boundary without doing any harm. But in your previous patch
> series, I see hv_call_withdraw_memory(), wherein Hyper-V gives
> back individual 4K pages in no particular order. That's the killer, as
> it is not feasible to re-assemble a random set of 4K pages into larger
> 16K or 64K pages for free_page().
> 
> So scratch that idea. :-( The root partition must run with a page
> size that matches the size Hyper-V uses to do memory deposits to
> and from a partition, and that's 4K.

Yes that does seem to clinch it. There will have to be a dependency.

Thanks for the detailed responses, sorry it took me some time to get to.

Nuno

> 
> Michael
> 
> [1] https://lore.kernel.org/lkml/1696010501-24584-1-git-send-email-nunodasneves@linux.microsoft.com/