Commit bc905fa8b633 ("hyperv: Switch from hyperv-tlfs.h to hyperv/hvhdk.h")
changed the type of the output pointer to `struct hv_register_assoc` from
`struct hv_get_vp_registers_output`. That leads to an incorrect computation,
and leaves the system broken.

Use the correct pointer type for the output of the GetVpRegisters hypercall.

Fixes: bc905fa8b633 ("hyperv: Switch from hyperv-tlfs.h to hyperv/hvhdk.h")
Signed-off-by: Roman Kisel <romank@linux.microsoft.com>
---
 arch/x86/hyperv/hv_init.c   | 6 +++---
 include/hyperv/hvgdk_mini.h | 3 ---
 2 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
index 3cf2a227d666..c7185c6a290b 100644
--- a/arch/x86/hyperv/hv_init.c
+++ b/arch/x86/hyperv/hv_init.c
@@ -416,13 +416,13 @@ static u8 __init get_vtl(void)
 {
 	u64 control = HV_HYPERCALL_REP_COMP_1 | HVCALL_GET_VP_REGISTERS;
 	struct hv_input_get_vp_registers *input;
-	struct hv_register_assoc *output;
+	struct hv_get_vp_registers_output *output;
 	unsigned long flags;
 	u64 ret;
 
 	local_irq_save(flags);
 	input = *this_cpu_ptr(hyperv_pcpu_input_arg);
-	output = (struct hv_register_assoc *)input;
+	output = (struct hv_get_vp_registers_output *)input;
 
 	memset(input, 0, struct_size(input, names, 1));
 	input->partition_id = HV_PARTITION_ID_SELF;
@@ -432,7 +432,7 @@ static u8 __init get_vtl(void)
 
 	ret = hv_do_hypercall(control, input, output);
 	if (hv_result_success(ret)) {
-		ret = output->value.reg8 & HV_X64_VTL_MASK;
+		ret = output->as64.low & HV_X64_VTL_MASK;
 	} else {
 		pr_err("Failed to get VTL(error: %lld) exiting...\n", ret);
 		BUG();
diff --git a/include/hyperv/hvgdk_mini.h b/include/hyperv/hvgdk_mini.h
index db3d1aaf7330..0b1a10828f33 100644
--- a/include/hyperv/hvgdk_mini.h
+++ b/include/hyperv/hvgdk_mini.h
@@ -1107,7 +1107,6 @@ union hv_register_value {
 	union hv_x64_pending_interruption_register pending_interruption;
 };
 
-#if defined(CONFIG_ARM64)
 /* HvGetVpRegisters returns an array of these output elements */
 struct hv_get_vp_registers_output {
 	union {
@@ -1124,8 +1123,6 @@ struct hv_get_vp_registers_output {
 	};
 };
 
-#endif /* CONFIG_ARM64 */
-
 struct hv_register_assoc {
 	u32 name;			/* enum hv_register_name */
 	u32 reserved1;
-- 
2.34.1

On 12/18/2024 12:54 PM, Roman Kisel wrote:
> Commit bc905fa8b633 ("hyperv: Switch from hyperv-tlfs.h to hyperv/hvhdk.h")
> changed the type of the output pointer to `struct hv_register_assoc` from
> `struct hv_get_vp_registers_output`. That leads to an incorrect computation,
> and leaves the system broken.
> 
My bad! But, lets not use `struct hv_get_registers_output`. Instead, use
`struct hv_register_value`, since that is the more complete definition of a
register value. The output of the get_vp_registers hypercall is just an array
of these values.

Ideally we remove `struct hv_get_vp_registers_output` at some point, since
it serves the same role as `struct hv_register_value` but in a more limited
capacity.

Thanks
Nuno

> Use the correct pointer type for the output of the GetVpRegisters hypercall.
> 
> Fixes: bc905fa8b633 ("hyperv: Switch from hyperv-tlfs.h to hyperv/hvhdk.h")
> Signed-off-by: Roman Kisel <romank@linux.microsoft.com>
> ---
>  arch/x86/hyperv/hv_init.c   | 6 +++---
>  include/hyperv/hvgdk_mini.h | 3 ---
>  2 files changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
> index 3cf2a227d666..c7185c6a290b 100644
> --- a/arch/x86/hyperv/hv_init.c
> +++ b/arch/x86/hyperv/hv_init.c
> @@ -416,13 +416,13 @@ static u8 __init get_vtl(void)
>  {
>  	u64 control = HV_HYPERCALL_REP_COMP_1 | HVCALL_GET_VP_REGISTERS;
>  	struct hv_input_get_vp_registers *input;
> -	struct hv_register_assoc *output;
> +	struct hv_get_vp_registers_output *output;
>  	unsigned long flags;
>  	u64 ret;
>  
>  	local_irq_save(flags);
>  	input = *this_cpu_ptr(hyperv_pcpu_input_arg);
> -	output = (struct hv_register_assoc *)input;
> +	output = (struct hv_get_vp_registers_output *)input;
>  
>  	memset(input, 0, struct_size(input, names, 1));
>  	input->partition_id = HV_PARTITION_ID_SELF;
> @@ -432,7 +432,7 @@ static u8 __init get_vtl(void)
>  
>  	ret = hv_do_hypercall(control, input, output);
>  	if (hv_result_success(ret)) {
> -		ret = output->value.reg8 & HV_X64_VTL_MASK;
> +		ret = output->as64.low & HV_X64_VTL_MASK;
>  	} else {
>  		pr_err("Failed to get VTL(error: %lld) exiting...\n", ret);
>  		BUG();
> diff --git a/include/hyperv/hvgdk_mini.h b/include/hyperv/hvgdk_mini.h
> index db3d1aaf7330..0b1a10828f33 100644
> --- a/include/hyperv/hvgdk_mini.h
> +++ b/include/hyperv/hvgdk_mini.h
> @@ -1107,7 +1107,6 @@ union hv_register_value {
>  	union hv_x64_pending_interruption_register pending_interruption;
>  };
>  
> -#if defined(CONFIG_ARM64)
>  /* HvGetVpRegisters returns an array of these output elements */
>  struct hv_get_vp_registers_output {
>  	union {
> @@ -1124,8 +1123,6 @@ struct hv_get_vp_registers_output {
>  	};
>  };
>  
> -#endif /* CONFIG_ARM64 */
> -
>  struct hv_register_assoc {
>  	u32 name;			/* enum hv_register_name */
>  	u32 reserved1;

On 12/19/2024 10:40 AM, Nuno Das Neves wrote:
> On 12/18/2024 12:54 PM, Roman Kisel wrote:
>> Commit bc905fa8b633 ("hyperv: Switch from hyperv-tlfs.h to hyperv/hvhdk.h")
>> changed the type of the output pointer to `struct hv_register_assoc` from
>> `struct hv_get_vp_registers_output`. That leads to an incorrect computation,
>> and leaves the system broken.
>>
> My bad! But, lets not use `struct hv_get_registers_output`. Instead, use
> `struct hv_register_value`, since that is the more complete definition of a
> register value. The output of the get_vp_registers hypercall is just an array
> of these values.
> 
> Ideally we remove `struct hv_get_vp_registers_output` at some point, since
> it serves the same role as `struct hv_register_value` but in a more limited
> capacity.
Nuno, thank you very much for your help! Agreed, I will use `struct
  hv_register_value` in v2.

> 
> Thanks
> Nuno
Thank you,
Roman

> 
>> Use the correct pointer type for the output of the GetVpRegisters hypercall.
>>
>> Fixes: bc905fa8b633 ("hyperv: Switch from hyperv-tlfs.h to hyperv/hvhdk.h")
>> Signed-off-by: Roman Kisel <romank@linux.microsoft.com>
>> ---
>>   arch/x86/hyperv/hv_init.c   | 6 +++---
>>   include/hyperv/hvgdk_mini.h | 3 ---
>>   2 files changed, 3 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
>> index 3cf2a227d666..c7185c6a290b 100644
>> --- a/arch/x86/hyperv/hv_init.c
>> +++ b/arch/x86/hyperv/hv_init.c
>> @@ -416,13 +416,13 @@ static u8 __init get_vtl(void)
>>   {
>>   	u64 control = HV_HYPERCALL_REP_COMP_1 | HVCALL_GET_VP_REGISTERS;
>>   	struct hv_input_get_vp_registers *input;
>> -	struct hv_register_assoc *output;
>> +	struct hv_get_vp_registers_output *output;
>>   	unsigned long flags;
>>   	u64 ret;
>>   
>>   	local_irq_save(flags);
>>   	input = *this_cpu_ptr(hyperv_pcpu_input_arg);
>> -	output = (struct hv_register_assoc *)input;
>> +	output = (struct hv_get_vp_registers_output *)input;
>>   
>>   	memset(input, 0, struct_size(input, names, 1));
>>   	input->partition_id = HV_PARTITION_ID_SELF;
>> @@ -432,7 +432,7 @@ static u8 __init get_vtl(void)
>>   
>>   	ret = hv_do_hypercall(control, input, output);
>>   	if (hv_result_success(ret)) {
>> -		ret = output->value.reg8 & HV_X64_VTL_MASK;
>> +		ret = output->as64.low & HV_X64_VTL_MASK;
>>   	} else {
>>   		pr_err("Failed to get VTL(error: %lld) exiting...\n", ret);
>>   		BUG();
>> diff --git a/include/hyperv/hvgdk_mini.h b/include/hyperv/hvgdk_mini.h
>> index db3d1aaf7330..0b1a10828f33 100644
>> --- a/include/hyperv/hvgdk_mini.h
>> +++ b/include/hyperv/hvgdk_mini.h
>> @@ -1107,7 +1107,6 @@ union hv_register_value {
>>   	union hv_x64_pending_interruption_register pending_interruption;
>>   };
>>   
>> -#if defined(CONFIG_ARM64)
>>   /* HvGetVpRegisters returns an array of these output elements */
>>   struct hv_get_vp_registers_output {
>>   	union {
>> @@ -1124,8 +1123,6 @@ struct hv_get_vp_registers_output {
>>   	};
>>   };
>>   
>> -#endif /* CONFIG_ARM64 */
>> -
>>   struct hv_register_assoc {
>>   	u32 name;			/* enum hv_register_name */
>>   	u32 reserved1;

-- 
Thank you,
Roman

On 12/19/2024 10:40 AM, Nuno Das Neves wrote:
> On 12/18/2024 12:54 PM, Roman Kisel wrote:
>> Commit bc905fa8b633 ("hyperv: Switch from hyperv-tlfs.h to hyperv/hvhdk.h")
>> changed the type of the output pointer to `struct hv_register_assoc` from
>> `struct hv_get_vp_registers_output`. That leads to an incorrect computation,
>> and leaves the system broken.
>>
> My bad! But, lets not use `struct hv_get_registers_output`. Instead, use
> `struct hv_register_value`, since that is the more complete definition of a
> register value. The output of the get_vp_registers hypercall is just an array
> of these values.
> 
> Ideally we remove `struct hv_get_vp_registers_output` at some point, since
> it serves the same role as `struct hv_register_value` but in a more limited
> capacity.
> 
> Thanks
> Nuno
> 

I had much the same conversation with Roman off-list yesterday.

The choice is between using hv_get_registers_output which is clearly the
output of the GetVpRegisters hypercall by name, albeit limited as you
said, and hv_register_value which is the more complete definition and
what the hypervisor actually returns, but does not currently include the
arm64 definitions in our copy of hvgdk_mini.h. hv_get_registers_output
and hv_register_value overlap in layout for Roman's purposes.

FWIW, I'm in favor of adding the arm64 definitions to hv_register_value
and using it for this get_vtl() patch.

This could be accompanied with migration of hv_get_vpreg128 in arm64/
and removal of struct hv_get_registers_output, or that could be deferred
to a later patch.

- Easwar

On 12/19/2024 11:13 AM, Easwar Hariharan wrote:
> On 12/19/2024 10:40 AM, Nuno Das Neves wrote:
>> On 12/18/2024 12:54 PM, Roman Kisel wrote:
>>> Commit bc905fa8b633 ("hyperv: Switch from hyperv-tlfs.h to hyperv/hvhdk.h")
>>> changed the type of the output pointer to `struct hv_register_assoc` from
>>> `struct hv_get_vp_registers_output`. That leads to an incorrect computation,
>>> and leaves the system broken.
>>>
>> My bad! But, lets not use `struct hv_get_registers_output`. Instead, use
>> `struct hv_register_value`, since that is the more complete definition of a
>> register value. The output of the get_vp_registers hypercall is just an array
>> of these values.
>>
>> Ideally we remove `struct hv_get_vp_registers_output` at some point, since
>> it serves the same role as `struct hv_register_value` but in a more limited
>> capacity.
>>
>> Thanks
>> Nuno
>>
> 
> I had much the same conversation with Roman off-list yesterday.
Appreciate your help, Easwar!

> 
> The choice is between using hv_get_registers_output which is clearly the
> output of the GetVpRegisters hypercall by name, albeit limited as you
> said, and hv_register_value which is the more complete definition and
> what the hypervisor actually returns, but does not currently include the
> arm64 definitions in our copy of hvgdk_mini.h. hv_get_registers_output
> and hv_register_value overlap in layout for Roman's purposes.
> 
> FWIW, I'm in favor of adding the arm64 definitions to hv_register_value
> and using it for this get_vtl() patch.
I could do that, will wait to see if no objections.

> 
> This could be accompanied with migration of hv_get_vpreg128 in arm64/
> and removal of struct hv_get_registers_output, or that could be deferred
> to a later patch.
Having an equivalent of `hv_get_vpreg128` would be awesome on x64!
I'd bet something like that should exist in dom0/mshv already if you'd
like to have a dedicated function. So perhaps we could let mshv take
the lead with that?

If the desire is also to use the fast hypercall technique as
`hv_get_vpreg128` does, then we'd need fast hypercalls on x64 that
can use XMM registers (aka the fast extended hypercalls) that aren't
implemented in the hyperv drivers from my read of the code (although
it seems KVM knows how to do that when it emulates Hyper-V,
arch/x86/kvm/hyperv.c, struct kvm_hv_hcall *hc).

These considerations make me lean towards deferring hv_get_vpreg128-like
implementation to a later time.

> 
> - Easwar
> 

-- 
Thank you,
Roman

On 12/19/2024 12:00 PM, Roman Kisel wrote:
> 
> 
> On 12/19/2024 11:13 AM, Easwar Hariharan wrote:
>> On 12/19/2024 10:40 AM, Nuno Das Neves wrote:
>>> On 12/18/2024 12:54 PM, Roman Kisel wrote:
>>>> Commit bc905fa8b633 ("hyperv: Switch from hyperv-tlfs.h to hyperv/
>>>> hvhdk.h")
>>>> changed the type of the output pointer to `struct hv_register_assoc`
>>>> from
>>>> `struct hv_get_vp_registers_output`. That leads to an incorrect
>>>> computation,
>>>> and leaves the system broken.
>>>>
>>> My bad! But, lets not use `struct hv_get_registers_output`. Instead, use
>>> `struct hv_register_value`, since that is the more complete
>>> definition of a
>>> register value. The output of the get_vp_registers hypercall is just
>>> an array
>>> of these values.
>>>
>>> Ideally we remove `struct hv_get_vp_registers_output` at some point,
>>> since
>>> it serves the same role as `struct hv_register_value` but in a more
>>> limited
>>> capacity.
>>>
>>> Thanks
>>> Nuno
>>>
>>
>> I had much the same conversation with Roman off-list yesterday.
> Appreciate your help, Easwar!
> 
>>
>> The choice is between using hv_get_registers_output which is clearly the
>> output of the GetVpRegisters hypercall by name, albeit limited as you
>> said, and hv_register_value which is the more complete definition and
>> what the hypervisor actually returns, but does not currently include the
>> arm64 definitions in our copy of hvgdk_mini.h. hv_get_registers_output
>> and hv_register_value overlap in layout for Roman's purposes.
>>
>> FWIW, I'm in favor of adding the arm64 definitions to hv_register_value
>> and using it for this get_vtl() patch.
> I could do that, will wait to see if no objections.
> 
>>
>> This could be accompanied with migration of hv_get_vpreg128 in arm64/
>> and removal of struct hv_get_registers_output, or that could be deferred
>> to a later patch.
> Having an equivalent of `hv_get_vpreg128` would be awesome on x64!
> I'd bet something like that should exist in dom0/mshv already if you'd
> like to have a dedicated function. So perhaps we could let mshv take
> the lead with that?
> 
> If the desire is also to use the fast hypercall technique as
> `hv_get_vpreg128` does, then we'd need fast hypercalls on x64 that
> can use XMM registers (aka the fast extended hypercalls) that aren't
> implemented in the hyperv drivers from my read of the code (although
> it seems KVM knows how to do that when it emulates Hyper-V,
> arch/x86/kvm/hyperv.c, struct kvm_hv_hcall *hc).
> 
> These considerations make me lean towards deferring hv_get_vpreg128-like
> implementation to a later time.
>

To clarify, I didn't mean to include implementing extended fast
hypercalls with the XMM registers, or an implementation of
hv_get_vpreg128() for x64 in my suggestion.

Just to fix up the existing hv_get_vpreg128 in arch/arm64 to use
hv_register_value (or its Linux-specific helper as Nuno suggested).

Thanks,
Easwar

On 12/19/2024 12:04 PM, Easwar Hariharan wrote:

[snip]

>>
>>>
>>> This could be accompanied with migration of hv_get_vpreg128 in arm64/
>>> and removal of struct hv_get_registers_output, or that could be deferred
>>> to a later patch.

[snip]

> 
> To clarify, I didn't mean to include implementing extended fast
> hypercalls with the XMM registers, or an implementation of
> hv_get_vpreg128() for x64 in my suggestion.
> 
> Just to fix up the existing hv_get_vpreg128 in arch/arm64 to use
> hv_register_value (or its Linux-specific helper as Nuno suggested).
Oooh, I see, thanks! I'll try that, looks like that should be pretty
straightforward, and hopefully, it will :)

> 
> Thanks,
> Easwar

-- 
Thank you,
Roman

On 12/19/2024 11:13 AM, Easwar Hariharan wrote:
> On 12/19/2024 10:40 AM, Nuno Das Neves wrote:
>> On 12/18/2024 12:54 PM, Roman Kisel wrote:
>>> Commit bc905fa8b633 ("hyperv: Switch from hyperv-tlfs.h to hyperv/hvhdk.h")
>>> changed the type of the output pointer to `struct hv_register_assoc` from
>>> `struct hv_get_vp_registers_output`. That leads to an incorrect computation,
>>> and leaves the system broken.
>>>
>> My bad! But, lets not use `struct hv_get_registers_output`. Instead, use
>> `struct hv_register_value`, since that is the more complete definition of a
>> register value. The output of the get_vp_registers hypercall is just an array
>> of these values.
>>
>> Ideally we remove `struct hv_get_vp_registers_output` at some point, since
>> it serves the same role as `struct hv_register_value` but in a more limited
>> capacity.
>>
>> Thanks
>> Nuno
>>
> 
> I had much the same conversation with Roman off-list yesterday.
> 
> The choice is between using hv_get_registers_output which is clearly the
> output of the GetVpRegisters hypercall by name, albeit limited as you

If it's desirable to have a more 'friendly' naming here, then I'd be okay with:
```
/* NOTE: Linux helper struct - NOT from Hyper-V code */
struct hv_output_get_vp_registers {
	DECLARE_FLEX_ARRAY(struct hv_register_value, values);
}
```
Note also the name is prefixed with "hv_output_" to match other hypercall outputs.

> said, and hv_register_value which is the more complete definition and
> what the hypervisor actually returns, but does not currently include the
> arm64 definitions in our copy of hvgdk_mini.h. hv_get_registers_output
> and hv_register_value overlap in layout for Roman's purposes.
> 
> FWIW, I'm in favor of adding the arm64 definitions to hv_register_value
> and using it for this get_vtl() patch.
> 

> This could be accompanied with migration of hv_get_vpreg128 in arm64/
> and removal of struct hv_get_registers_output, or that could be deferred
> to a later patch.

I'd be happy to submit a followup patch to update the arm64 code to use
hv_register_value, or a new struct as outlined above.

It is a pretty small change though, it might be easier to just include it in
this series.

Nuno

> 
> - Easwar

On 12/19/2024 11:23 AM, Nuno Das Neves wrote:
> On 12/19/2024 11:13 AM, Easwar Hariharan wrote:
>> On 12/19/2024 10:40 AM, Nuno Das Neves wrote:
>>> On 12/18/2024 12:54 PM, Roman Kisel wrote:
>>>> Commit bc905fa8b633 ("hyperv: Switch from hyperv-tlfs.h to hyperv/hvhdk.h")
>>>> changed the type of the output pointer to `struct hv_register_assoc` from
>>>> `struct hv_get_vp_registers_output`. That leads to an incorrect computation,
>>>> and leaves the system broken.
>>>>
>>> My bad! But, lets not use `struct hv_get_registers_output`. Instead, use
>>> `struct hv_register_value`, since that is the more complete definition of a
>>> register value. The output of the get_vp_registers hypercall is just an array
>>> of these values.
>>>
>>> Ideally we remove `struct hv_get_vp_registers_output` at some point, since
>>> it serves the same role as `struct hv_register_value` but in a more limited
>>> capacity.
>>>
>>> Thanks
>>> Nuno
>>>
>>
>> I had much the same conversation with Roman off-list yesterday.
>>
>> The choice is between using hv_get_registers_output which is clearly the
>> output of the GetVpRegisters hypercall by name, albeit limited as you
> 
> If it's desirable to have a more 'friendly' naming here, then I'd be okay with:
> ```
> /* NOTE: Linux helper struct - NOT from Hyper-V code */
> struct hv_output_get_vp_registers {
> 	DECLARE_FLEX_ARRAY(struct hv_register_value, values);
> }
> ```
> Note also the name is prefixed with "hv_output_" to match other hypercall outputs.

I like the idea of improving our code readability and consistency in
interface naming independently of the hypervisor. That comment should
allow for clarity when new definitions are imported.

> 
>> said, and hv_register_value which is the more complete definition and
>> what the hypervisor actually returns, but does not currently include the
>> arm64 definitions in our copy of hvgdk_mini.h. hv_get_registers_output
>> and hv_register_value overlap in layout for Roman's purposes.
>>
>> FWIW, I'm in favor of adding the arm64 definitions to hv_register_value
>> and using it for this get_vtl() patch.
>>
>> This could be accompanied with migration of hv_get_vpreg128 in arm64/
>> and removal of struct hv_get_registers_output, or that could be deferred
>> to a later patch.
> 
> I'd be happy to submit a followup patch to update the arm64 code to use
> hv_register_value, or a new struct as outlined above.
> 
> It is a pretty small change though, it might be easier to just include it in
> this series.
> 

Thank you! I'll leave it you and Roman to decide how you go about that.

- Easwar

On 12/19/2024 11:32 AM, Easwar Hariharan wrote:
> On 12/19/2024 11:23 AM, Nuno Das Neves wrote:
>> On 12/19/2024 11:13 AM, Easwar Hariharan wrote:
>>> On 12/19/2024 10:40 AM, Nuno Das Neves wrote:
>>>> On 12/18/2024 12:54 PM, Roman Kisel wrote:
>>>>> Commit bc905fa8b633 ("hyperv: Switch from hyperv-tlfs.h to hyperv/hvhdk.h")
>>>>> changed the type of the output pointer to `struct hv_register_assoc` from
>>>>> `struct hv_get_vp_registers_output`. That leads to an incorrect computation,
>>>>> and leaves the system broken.
>>>>>
>>>> My bad! But, lets not use `struct hv_get_registers_output`. Instead, use
>>>> `struct hv_register_value`, since that is the more complete definition of a
>>>> register value. The output of the get_vp_registers hypercall is just an array
>>>> of these values.
>>>>
>>>> Ideally we remove `struct hv_get_vp_registers_output` at some point, since
>>>> it serves the same role as `struct hv_register_value` but in a more limited
>>>> capacity.
>>>>
>>>> Thanks
>>>> Nuno
>>>>
>>>
>>> I had much the same conversation with Roman off-list yesterday.
>>>
>>> The choice is between using hv_get_registers_output which is clearly the
>>> output of the GetVpRegisters hypercall by name, albeit limited as you
>>
>> If it's desirable to have a more 'friendly' naming here, then I'd be okay with:
>> ```
>> /* NOTE: Linux helper struct - NOT from Hyper-V code */
>> struct hv_output_get_vp_registers {
>> 	DECLARE_FLEX_ARRAY(struct hv_register_value, values);
>> }
>> ```
>> Note also the name is prefixed with "hv_output_" to match other hypercall outputs.
> 
> I like the idea of improving our code readability and consistency in
> interface naming independently of the hypervisor. That comment should
> allow for clarity when new definitions are imported.
> 
Thank you, Easwar and Nuno, I'll use that in v2!

>>
>>> said, and hv_register_value which is the more complete definition and
>>> what the hypervisor actually returns, but does not currently include the
>>> arm64 definitions in our copy of hvgdk_mini.h. hv_get_registers_output
>>> and hv_register_value overlap in layout for Roman's purposes.
>>>
>>> FWIW, I'm in favor of adding the arm64 definitions to hv_register_value
>>> and using it for this get_vtl() patch.
>>>
>>> This could be accompanied with migration of hv_get_vpreg128 in arm64/
>>> and removal of struct hv_get_registers_output, or that could be deferred
>>> to a later patch.
>>
>> I'd be happy to submit a followup patch to update the arm64 code to use
>> hv_register_value, or a new struct as outlined above.
>>
>> It is a pretty small change though, it might be easier to just include it in
>> this series.
>>
> 
> Thank you! I'll leave it you and Roman to decide how you go about that.
> 
> - Easwar

-- 
Thank you,
Roman

On Wed, Dec 18, 2024 at 12:54:20PM -0800, Roman Kisel wrote:
> Commit bc905fa8b633 ("hyperv: Switch from hyperv-tlfs.h to hyperv/hvhdk.h")
> changed the type of the output pointer to `struct hv_register_assoc` from
> `struct hv_get_vp_registers_output`. That leads to an incorrect computation,
> and leaves the system broken.
> 
> Use the correct pointer type for the output of the GetVpRegisters hypercall.
> 
> Fixes: bc905fa8b633 ("hyperv: Switch from hyperv-tlfs.h to hyperv/hvhdk.h")

This commit is not in the mainline kernel yet, so this tag is not
needed.

It will most likely to be wrong since I will need to rebase the
hyperv-next branch.

I can fold this patch into the original patch and leave your
Signed-off-by there.

Thanks,
Wei.

> Signed-off-by: Roman Kisel <romank@linux.microsoft.com>
> ---
>  arch/x86/hyperv/hv_init.c   | 6 +++---
>  include/hyperv/hvgdk_mini.h | 3 ---
>  2 files changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
> index 3cf2a227d666..c7185c6a290b 100644
> --- a/arch/x86/hyperv/hv_init.c
> +++ b/arch/x86/hyperv/hv_init.c
> @@ -416,13 +416,13 @@ static u8 __init get_vtl(void)
>  {
>  	u64 control = HV_HYPERCALL_REP_COMP_1 | HVCALL_GET_VP_REGISTERS;
>  	struct hv_input_get_vp_registers *input;
> -	struct hv_register_assoc *output;
> +	struct hv_get_vp_registers_output *output;
>  	unsigned long flags;
>  	u64 ret;
>  
>  	local_irq_save(flags);
>  	input = *this_cpu_ptr(hyperv_pcpu_input_arg);
> -	output = (struct hv_register_assoc *)input;
> +	output = (struct hv_get_vp_registers_output *)input;
>  
>  	memset(input, 0, struct_size(input, names, 1));
>  	input->partition_id = HV_PARTITION_ID_SELF;
> @@ -432,7 +432,7 @@ static u8 __init get_vtl(void)
>  
>  	ret = hv_do_hypercall(control, input, output);
>  	if (hv_result_success(ret)) {
> -		ret = output->value.reg8 & HV_X64_VTL_MASK;
> +		ret = output->as64.low & HV_X64_VTL_MASK;
>  	} else {
>  		pr_err("Failed to get VTL(error: %lld) exiting...\n", ret);
>  		BUG();
> diff --git a/include/hyperv/hvgdk_mini.h b/include/hyperv/hvgdk_mini.h
> index db3d1aaf7330..0b1a10828f33 100644
> --- a/include/hyperv/hvgdk_mini.h
> +++ b/include/hyperv/hvgdk_mini.h
> @@ -1107,7 +1107,6 @@ union hv_register_value {
>  	union hv_x64_pending_interruption_register pending_interruption;
>  };
>  
> -#if defined(CONFIG_ARM64)
>  /* HvGetVpRegisters returns an array of these output elements */
>  struct hv_get_vp_registers_output {
>  	union {
> @@ -1124,8 +1123,6 @@ struct hv_get_vp_registers_output {
>  	};
>  };
>  
> -#endif /* CONFIG_ARM64 */
> -
>  struct hv_register_assoc {
>  	u32 name;			/* enum hv_register_name */
>  	u32 reserved1;
> -- 
> 2.34.1
>

On 12/18/2024 6:45 PM, Wei Liu wrote:
> On Wed, Dec 18, 2024 at 12:54:20PM -0800, Roman Kisel wrote:
>> Commit bc905fa8b633 ("hyperv: Switch from hyperv-tlfs.h to hyperv/hvhdk.h")
>> changed the type of the output pointer to `struct hv_register_assoc` from
>> `struct hv_get_vp_registers_output`. That leads to an incorrect computation,
>> and leaves the system broken.
>>
>> Use the correct pointer type for the output of the GetVpRegisters hypercall.
>>
>> Fixes: bc905fa8b633 ("hyperv: Switch from hyperv-tlfs.h to hyperv/hvhdk.h")
> 
> This commit is not in the mainline kernel yet, so this tag is not
> needed.
Got it, thanks for the explanation!

> 
> It will most likely to be wrong since I will need to rebase the
> hyperv-next branch.
> 
> I can fold this patch into the original patch and leave your
> Signed-off-by there.
That would be great and appreciated very much, thank you!

> 
> Thanks,
> Wei.
Thank you,
Roman

> 
>> Signed-off-by: Roman Kisel <romank@linux.microsoft.com>
>> ---
>>   arch/x86/hyperv/hv_init.c   | 6 +++---
>>   include/hyperv/hvgdk_mini.h | 3 ---
>>   2 files changed, 3 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
>> index 3cf2a227d666..c7185c6a290b 100644
>> --- a/arch/x86/hyperv/hv_init.c
>> +++ b/arch/x86/hyperv/hv_init.c
>> @@ -416,13 +416,13 @@ static u8 __init get_vtl(void)
>>   {
>>   	u64 control = HV_HYPERCALL_REP_COMP_1 | HVCALL_GET_VP_REGISTERS;
>>   	struct hv_input_get_vp_registers *input;
>> -	struct hv_register_assoc *output;
>> +	struct hv_get_vp_registers_output *output;
>>   	unsigned long flags;
>>   	u64 ret;
>>   
>>   	local_irq_save(flags);
>>   	input = *this_cpu_ptr(hyperv_pcpu_input_arg);
>> -	output = (struct hv_register_assoc *)input;
>> +	output = (struct hv_get_vp_registers_output *)input;
>>   
>>   	memset(input, 0, struct_size(input, names, 1));
>>   	input->partition_id = HV_PARTITION_ID_SELF;
>> @@ -432,7 +432,7 @@ static u8 __init get_vtl(void)
>>   
>>   	ret = hv_do_hypercall(control, input, output);
>>   	if (hv_result_success(ret)) {
>> -		ret = output->value.reg8 & HV_X64_VTL_MASK;
>> +		ret = output->as64.low & HV_X64_VTL_MASK;
>>   	} else {
>>   		pr_err("Failed to get VTL(error: %lld) exiting...\n", ret);
>>   		BUG();
>> diff --git a/include/hyperv/hvgdk_mini.h b/include/hyperv/hvgdk_mini.h
>> index db3d1aaf7330..0b1a10828f33 100644
>> --- a/include/hyperv/hvgdk_mini.h
>> +++ b/include/hyperv/hvgdk_mini.h
>> @@ -1107,7 +1107,6 @@ union hv_register_value {
>>   	union hv_x64_pending_interruption_register pending_interruption;
>>   };
>>   
>> -#if defined(CONFIG_ARM64)
>>   /* HvGetVpRegisters returns an array of these output elements */
>>   struct hv_get_vp_registers_output {
>>   	union {
>> @@ -1124,8 +1123,6 @@ struct hv_get_vp_registers_output {
>>   	};
>>   };
>>   
>> -#endif /* CONFIG_ARM64 */
>> -
>>   struct hv_register_assoc {
>>   	u32 name;			/* enum hv_register_name */
>>   	u32 reserved1;
>> -- 
>> 2.34.1
>>

-- 
Thank you,
Roman