Re: [PATCH 2/4] Drivers: hv: Use nested hypercall for post message and signal event

[Nuno Das Neves <nunodasneves@linux.microsoft.com>]

On 6/11/2025 4:06 PM, Michael Kelley wrote:
> From: Nuno Das Neves <nunodasneves@linux.microsoft.com> Sent: Tuesday, June 10, 2025 4:52 PM
>>
>> When running nested, these hypercalls must be sent to the L0 hypervisor
>> or vmbus will fail.
> 
> s/vmbus/VMBus/
> 

Ack

>>
>> Add ARM64 stubs for the nested hypercall helpers to not break
>> compilation (nested is still only supported in x86).
>>
>> Signed-off-by: Nuno Das Neves <nunodasneves@linux.microsoft.com>
>> ---
>>  arch/arm64/include/asm/mshyperv.h | 10 ++++++++++
>>  drivers/hv/connection.c           |  3 +++
>>  drivers/hv/hv.c                   |  3 +++
>>  3 files changed, 16 insertions(+)
>>
>> diff --git a/arch/arm64/include/asm/mshyperv.h
>> b/arch/arm64/include/asm/mshyperv.h
>> index b721d3134ab6..893d6a2e8dab 100644
>> --- a/arch/arm64/include/asm/mshyperv.h
>> +++ b/arch/arm64/include/asm/mshyperv.h
>> @@ -53,6 +53,16 @@ static inline u64 hv_get_non_nested_msr(unsigned int reg)
>>  	return hv_get_msr(reg);
>>  }
>>
>> +static inline u64 hv_do_nested_hypercall(u64 control, void *input, void *output)
>> +{
>> +	return U64_MAX;
>> +}
>> +
>> +static inline u64 hv_do_fast_nested_hypercall8(u64 control, u64 input1)
>> +{
>> +	return U64_MAX;
>> +}
> 
> I think the definitions of hv_do_nested_hypercall() and
> hv_do_fast_nested_hypercall8() are architecture independent. All
> they do is add the HV_HYPERCALL_NESTED flag, which when
> implemented for ARM64, will presumably be the same flag as
> currently defined for x86.  As such, couldn't the definitions of
> hv_do_nested_hypercall() and hv_do_fast_nested_hypercall8()
> be moved to asm-generic/mshyperv.h? Then stubs would not
> be needed for ARM64. These two functions would never be
> called on ARM64 because hv_nested is never true on ARM64
> (at least for now), but the code would compile. And if either
> function was erroneously called on ARM64, presumably
> Hyper-V would return an error because HV_HYPERCALL_NESTED
> is set.
> 

Good point - letting the hypervisor return the error is fine.

>> +
>>  /* SMCCC hypercall parameters */
>>  #define HV_SMCCC_FUNC_NUMBER	1
>>  #define HV_FUNC_ID	ARM_SMCCC_CALL_VAL(			\
>> diff --git a/drivers/hv/connection.c b/drivers/hv/connection.c
>> index be490c598785..992022bc770c 100644
>> --- a/drivers/hv/connection.c
>> +++ b/drivers/hv/connection.c
>> @@ -518,6 +518,9 @@ void vmbus_set_event(struct vmbus_channel *channel)
>>  					 channel->sig_event, 0);
>>  		else
>>  			WARN_ON_ONCE(1);
>> +	} else if (hv_nested) {
>> +		hv_do_fast_nested_hypercall8(HVCALL_SIGNAL_EVENT,
>> +					     channel->sig_event);
>>  	} else {
>>  		hv_do_fast_hypercall8(HVCALL_SIGNAL_EVENT, channel->sig_event);
>>  	}
>> diff --git a/drivers/hv/hv.c b/drivers/hv/hv.c
>> index 308c8f279df8..99b73e779bf0 100644
>> --- a/drivers/hv/hv.c
>> +++ b/drivers/hv/hv.c
>> @@ -84,6 +84,9 @@ int hv_post_message(union hv_connection_id connection_id,
>>  						   sizeof(*aligned_msg));
>>  		else
>>  			status = HV_STATUS_INVALID_PARAMETER;
>> +	} else if (hv_nested) {
>> +		status = hv_do_nested_hypercall(HVCALL_POST_MESSAGE,
>> +						aligned_msg, NULL);
>>  	} else {
>>  		status = hv_do_hypercall(HVCALL_POST_MESSAGE,
>>  					 aligned_msg, NULL);
> 
> Are HVCALL_SIGNAL_EVENT and HVCALL_POST_MESSAGE the only two
> hypercalls that are ever expected to need a "nested" version? I'm
> wondering if the function hv_do_nested_hypercall() and
> hv_do_fast_nested_hypercall8() could be dropped entirely, and just
> pass the first argument to hv_do_hypercall() or hv_do_fast_hypercall8()
> as <hypercall_name> | HV_HYPERCALL_NESTED. For only two cases, a
> little bit of open coding might be preferable to the overhead of defining
> functions just to wrap the or'ing of HV_HYPERCALL_NESTED. 
> 
> The code above could then look like:
> 
> 	} else {
> 		u64 control = HVCALL_POST_MESSAGE;
> 
> 		control |= hv_nested ? HV_HYPERCALL_NESTED : 0;
> 		status = hv_do_hypercall(control, aligned_msg, NULL);
> 	}
> 
> Again, ARM64 is implicitly handled because hv_nested is never set.
> 
> This is just a suggestion. It's motivated by the fact that we already have
> three flavors of hypercall for HVCALL_SIGNAL_EVENT and
> HVCALL_POST_MESSAGE, and I was looking for a way to avoid adding
> a fourth flavor. But it's a marginal win, and if you prefer to keep the
> inline functions, I'm OK with that.
> 

I like the suggestion to open-code these cases.

There are several places we need to get/set nested MSRs, but as for hypercalls
it is just these 2, so far. When I consider it from that angle, it feels cleaner
to just open-code them, and remove the existing '_nested' versions of the
hypercall helpers for now.

Thanks
Nuno

> Michael