diff mbox series

Drivers: hv: Use nested hypercall for post message and signal event

Message ID 1680564178-31023-1-git-send-email-nunodasneves@linux.microsoft.com
State New
Headers
Series Drivers: hv: Use nested hypercall for post message and signal event |

Commit Message

Nuno Das Neves April 3, 2023, 11:22 p.m. UTC 
  When running nested, these hypercalls must be sent to the L0 hypervisor
or vmbus will fail.
Only relevant for x86; nested functionality is not available in ARM64.

Signed-off-by: Nuno Das Neves <nunodasneves@linux.microsoft.com>
---
 drivers/hv/connection.c | 4 ++++
 drivers/hv/hv.c         | 5 +++++
 2 files changed, 9 insertions(+)

Comments

Olaf Hering April 4, 2023, 6:45 a.m. UTC | #1 
Mon,  3 Apr 2023 16:22:58 -0700 Nuno Das Neves <nunodasneves@linux.microsoft.com>:

> Only relevant for x86; nested functionality is not available in ARM64.

> +#if defined(CONFIG_X86_64)
> +	else if (hv_nested)

Should there be a hv_nested in the ARM64 code path?
Looks like c4bdf94f97c86 provided such thing, so the Kconfig conditional could be removed.

Olaf
Nuno Das Neves April 4, 2023, 4:56 p.m. UTC | #2 
On 4/3/2023 11:45 PM, Olaf Hering wrote:
> Mon,  3 Apr 2023 16:22:58 -0700 Nuno Das Neves <nunodasneves@linux.microsoft.com>:
> 
>> Only relevant for x86; nested functionality is not available in ARM64.
> 
>> +#if defined(CONFIG_X86_64)
>> +	else if (hv_nested)
> 
> Should there be a hv_nested in the ARM64 code path?
> Looks like c4bdf94f97c86 provided such thing, so the Kconfig conditional could be removed.
> 
> Olaf

This will not compile on ARM64 without the guard, because hv_do_nested_hypercall and
hv_do_fast_nested_hypercall8 are not defined.
These are inline functions only defined in the x86 mshyperv.h header.

The alternative to these guards would be defining dummy inline functions for the nested
versions of hv_do_hypercall in the ARM64 mshyperv.h.
I could take that approach if it is preferable.
Michael Kelley (LINUX) April 7, 2023, 10:28 p.m. UTC | #3 
From: Nuno Das Neves <nunodasneves@linux.microsoft.com> Sent: Tuesday, April 4, 2023 9:57 AM
> 
> On 4/3/2023 11:45 PM, Olaf Hering wrote:
> > Mon,  3 Apr 2023 16:22:58 -0700 Nuno Das Neves
> <nunodasneves@linux.microsoft.com>:
> >
> >> Only relevant for x86; nested functionality is not available in ARM64.
> >
> >> +#if defined(CONFIG_X86_64)
> >> +	else if (hv_nested)
> >
> > Should there be a hv_nested in the ARM64 code path?
> > Looks like c4bdf94f97c86 provided such thing, so the Kconfig conditional could be
> removed.
> >
> > Olaf
> 
> This will not compile on ARM64 without the guard, because hv_do_nested_hypercall
> and hv_do_fast_nested_hypercall8 are not defined.
> These are inline functions only defined in the x86 mshyperv.h header.
> 
> The alternative to these guards would be defining dummy inline functions for the
> nested versions of hv_do_hypercall in the ARM64 mshyperv.h.
> I could take that approach if it is preferable.

Having to do "if (hv_nested)" and "#ifdef CONFIG_X86_64" at multiple call
sites is indeed rather messy.  I wonder if it is feasible to fold all this logic into
the x86 version of hv_do_hypercall() and friends, so that the call sites are not
affected?  That's what was done with hv_get/set_register().  And it would
allow the ARM64 side to be unchanged.

Here's an approach:
1)  Create a global bitmap with one bit for each hypercall code that Hyper-V
accepts.  The max is something less than 512,  so this bitmap is less than 64
bytes.  Initialize the bitmap to all zeros.
2)  During early initialization, if hv_nested is set to "true", set the bit in
the bitmap corresponding to hypercalls that need the HV_HYPERCALL_NESTED
flag added.
3) In hv_do_hypercall(), use the hypercall code to index into the bitmap and
retrieve the bit.  Use that bit to decide whether to set HV_HYPERCALL_NESTED.
Note that hv_nested doesn't even need to be tested because the bitmap will
be all zeros when hv_nested is "false".

The one snag is extended hypercalls, for which the hypercall code is a much
bigger value, and we might not want to make the bitmap that big.  Maybe
they have to be special-cased to be non-nested.

This approach assumes that a hypercall is always either nested or non-nested
from all call sites.

If the bitmap approach is too obscure, testing hv_nested and doing a switch
statement to handle the cases that need HV_HYPERCALL_NESTED would
also work.  The performance probably wouldn't be as good, but it probably
doesn't matter.  Again, this would be like hv_get/set_register().

With this approach, hv_do_nested_hypercall() and friends are not needed,
but I don't know what other usage patterns for hv_do_nested_hypercall()
might be planned.  These other usage patterns might make this idea not so
workable.

Michael
diff mbox series

Patch

diff --git a/drivers/hv/connection.c b/drivers/hv/connection.c
index 9dc27e5d367a..04bf7f168976 100644
--- a/drivers/hv/connection.c
+++ b/drivers/hv/connection.c
@@ -539,6 +539,10 @@  void vmbus_set_event(struct vmbus_channel *channel)
 	if (hv_isolation_type_snp())
 		hv_ghcb_hypercall(HVCALL_SIGNAL_EVENT, &channel->sig_event,
 				NULL, sizeof(channel->sig_event));
+#if defined(CONFIG_X86_64)
+	else if (hv_nested)
+		hv_do_fast_nested_hypercall8(HVCALL_SIGNAL_EVENT, channel->sig_event);
+#endif
 	else
 		hv_do_fast_hypercall8(HVCALL_SIGNAL_EVENT, channel->sig_event);
 }
diff --git a/drivers/hv/hv.c b/drivers/hv/hv.c
index 8b0dd8e5244d..c7f7652932ca 100644
--- a/drivers/hv/hv.c
+++ b/drivers/hv/hv.c
@@ -102,6 +102,11 @@  int hv_post_message(union hv_connection_id connection_id,
 		status = hv_ghcb_hypercall(HVCALL_POST_MESSAGE,
 				(void *)aligned_msg, NULL,
 				sizeof(*aligned_msg));
+#if defined(CONFIG_X86_64)
+	else if (hv_nested)
+		status = hv_do_nested_hypercall(HVCALL_POST_MESSAGE,
+				aligned_msg, NULL);
+#endif
 	else
 		status = hv_do_hypercall(HVCALL_POST_MESSAGE,
 				aligned_msg, NULL);