Message-ID:
 <SN6PR02MB4157B7036994536E2F89436DD472A@SN6PR02MB4157.namprd02.prod.outlook.com>
Date: Wed, 18 Jun 2025 16:19:16 +0000
From: Michael Kelley <mhklinux@...look.com>
To: Roman Kisel <romank@...ux.microsoft.com>, "alok.a.tiwari@...cle.com"
	<alok.a.tiwari@...cle.com>, "arnd@...db.de" <arnd@...db.de>, "bp@...en8.de"
	<bp@...en8.de>, "corbet@....net" <corbet@....net>,
	"dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
	"decui@...rosoft.com" <decui@...rosoft.com>, "haiyangz@...rosoft.com"
	<haiyangz@...rosoft.com>, "hpa@...or.com" <hpa@...or.com>,
	"kys@...rosoft.com" <kys@...rosoft.com>, "mingo@...hat.com"
	<mingo@...hat.com>, "tglx@...utronix.de" <tglx@...utronix.de>,
	"wei.liu@...nel.org" <wei.liu@...nel.org>, "linux-arch@...r.kernel.org"
	<linux-arch@...r.kernel.org>, "linux-doc@...r.kernel.org"
	<linux-doc@...r.kernel.org>, "linux-hyperv@...r.kernel.org"
	<linux-hyperv@...r.kernel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "x86@...nel.org" <x86@...nel.org>
CC: "apais@...rosoft.com" <apais@...rosoft.com>, "benhill@...rosoft.com"
	<benhill@...rosoft.com>, "bperkins@...rosoft.com" <bperkins@...rosoft.com>,
	"sunilmut@...rosoft.com" <sunilmut@...rosoft.com>
Subject: RE: [PATCH hyperv-next v3 11/15] Drivers: hv: Functions for setting
 up and tearing down the paravisor SynIC

From: Roman Kisel <romank@...ux.microsoft.com> Sent: Tuesday, June 3, 2025 5:44 PM
> 
> The confidential VMBus runs with the paravisor SynIC and requires
> configuring it with the paravisor.
> 
> Add the functions for configuring the paravisor SynIC

Suggest:

Add the functions for configuring the paravisor SynIC. Update
overall SynIC initialization logic to initialize the SynIC if it is present.
Finally, break out SynIC interrupt enable/disable code into separate
functions so that SynIC interrupts can be enabled/disable via the
paravisor instead of the hypervisor if the paravisor SynIC is present.

> 
> Signed-off-by: Roman Kisel <romank@...ux.microsoft.com>
> ---
>  drivers/hv/hv.c | 180 +++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 169 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/hv/hv.c b/drivers/hv/hv.c
> index 2b561825089a..c9649ab3439e 100644
> --- a/drivers/hv/hv.c
> +++ b/drivers/hv/hv.c
> @@ -203,7 +203,7 @@ int hv_synic_alloc(void)
>  				decrypt, "hypervisor SynIC event");
>  			if (ret)
>  				goto err;
> -			}
> +		}

This looks like a code cleanup that should be part of Patch 6 of this series.

> 
>  		if (vmbus_is_confidential()) {
>  			ret = hv_alloc_page(cpu, &hv_cpu->para_synic_message_page,
> @@ -267,7 +267,6 @@ void hv_hyp_synic_enable_regs(unsigned int cpu)
>  	union hv_synic_simp simp;
>  	union hv_synic_siefp siefp;
>  	union hv_synic_sint shared_sint;
> -	union hv_synic_scontrol sctrl;
> 
>  	/* Setup the Synic's message page */
>  	simp.as_uint64 = hv_get_msr(HV_MSR_SIMP);
> @@ -288,7 +287,7 @@ void hv_hyp_synic_enable_regs(unsigned int cpu)
> 
>  	hv_set_msr(HV_MSR_SIMP, simp.as_uint64);
> 
> -	/* Setup the Synic's event page */
> +	/* Setup the Synic's event page with the hypervisor. */

You added "with the hypervisor" to this comment, but not the similar
comment above for the message page. Consistency .... :-)

>  	siefp.as_uint64 = hv_get_msr(HV_MSR_SIEFP);
>  	siefp.siefp_enabled = 1;
> 
> @@ -316,6 +315,11 @@ void hv_hyp_synic_enable_regs(unsigned int cpu)
>  	shared_sint.masked = false;
>  	shared_sint.auto_eoi = hv_recommend_using_aeoi();
>  	hv_set_msr(HV_MSR_SINT0 + VMBUS_MESSAGE_SINT, shared_sint.as_uint64);
> +}
> +
> +static void hv_hyp_synic_enable_interrupts(void)
> +{
> +	union hv_synic_scontrol sctrl;
> 
>  	/* Enable the global synic bit */
>  	sctrl.as_uint64 = hv_get_msr(HV_MSR_SCONTROL);
> @@ -324,13 +328,90 @@ void hv_hyp_synic_enable_regs(unsigned int cpu)
>  	hv_set_msr(HV_MSR_SCONTROL, sctrl.as_uint64);
>  }
> 
> +/*
> + * The paravisor might not support proxying SynIC, and this
> + * function may fail.
> + */
> +static int hv_para_synic_enable_regs(unsigned int cpu)
> +{
> +	int err;
> +	union hv_synic_simp simp;
> +	union hv_synic_siefp siefp;
> +	struct hv_per_cpu_context *hv_cpu
> +		= per_cpu_ptr(hv_context.cpu_context, cpu);
> +
> +	/* Setup the Synic's message page with the paravisor. */
> +	err = hv_para_get_synic_register(HV_MSR_SIMP, &simp.as_uint64);
> +	if (err)
> +		return err;
> +	simp.simp_enabled = 1;
> +	simp.base_simp_gpa = virt_to_phys(hv_cpu->para_synic_message_page)
> +			>> HV_HYP_PAGE_SHIFT;
> +	err = hv_para_set_synic_register(HV_MSR_SIMP, simp.as_uint64);
> +	if (err)
> +		return err;
> +
> +	/* Setup the Synic's event page with the paravisor. */
> +	err = hv_para_get_synic_register(HV_MSR_SIEFP, &siefp.as_uint64);
> +	if (err)
> +		return err;
> +	siefp.siefp_enabled = 1;
> +	siefp.base_siefp_gpa = virt_to_phys(hv_cpu->para_synic_event_page)
> +			>> HV_HYP_PAGE_SHIFT;
> +	return hv_para_set_synic_register(HV_MSR_SIEFP, siefp.as_uint64);
> +}
> +
> +static int hv_para_synic_enable_interrupts(void)
> +{
> +	union hv_synic_scontrol sctrl;
> +	int err;
> +
> +	/* Enable the global synic bit */
> +	err = hv_para_get_synic_register(HV_MSR_SCONTROL, &sctrl.as_uint64);
> +	if (err)
> +		return err;
> +	sctrl.enable = 1;
> +
> +	return hv_para_set_synic_register(HV_MSR_SCONTROL, sctrl.as_uint64);
> +}
> +
>  int hv_synic_init(unsigned int cpu)
>  {
> +	int err;
> +
> +	/*
> +	 * The paravisor may not support the confidential VMBus,
> +	 * check on that first.
> +	 */
> +	if (vmbus_is_confidential()) {
> +		err = hv_para_synic_enable_regs(cpu);
> +		if (err)
> +			goto fail;
> +	}
> +
>  	hv_hyp_synic_enable_regs(cpu);
> +	if (vmbus_is_confidential()) {
> +		err = hv_para_synic_enable_interrupts();
> +		if (err)
> +			goto fail;
> +	} else
> +		hv_hyp_synic_enable_interrupts();

It would be nice to have some detailed code comments somewhere
describing how VMBus interrupts work with Confidential VMBus. I
see that the SINT is set in hv_hyperv_synic_enable_regs() by calling
hv_set_msr().  hv_set_msr() in turn has special case code for the
SINT MSRs that write to the hypervisor version of the MSR *and*
the paravisor version of the MSR (but *without* the proxy bit when
VMBus is confidential). Then the code above enables interrupts
via the paravisor if VMBus is confidential, and otherwise via the
hypervisor.

Assuming my description is correct, maybe just writing that down
in a comment somewhere will confirm to later developers of this
code about what is supposed to be happening.

> 
>  	hv_stimer_legacy_init(cpu, VMBUS_MESSAGE_SINT);
> 
>  	return 0;
> +
> +fail:
> +	/*
> +	 * The failure may only come from enabling the paravisor SynIC.
> +	 * That in turn means that the confidential VMBus cannot be used
> +	 * which is not an error: the setup will be re-tried with the
> +	 * non-confidential VMBus.
> +	 *
> +	 * We also don't bother attempting to reset the paravisor registers
> +	 * as something isn't working there anyway.
> +	 */
> +	return err;
>  }
> 
>  void hv_hyp_synic_disable_regs(unsigned int cpu)
> @@ -340,7 +421,6 @@ void hv_hyp_synic_disable_regs(unsigned int cpu)
>  	union hv_synic_sint shared_sint;
>  	union hv_synic_simp simp;
>  	union hv_synic_siefp siefp;
> -	union hv_synic_scontrol sctrl;
> 
>  	shared_sint.as_uint64 = hv_get_msr(HV_MSR_SINT0 + VMBUS_MESSAGE_SINT);
> 
> @@ -378,14 +458,71 @@ void hv_hyp_synic_disable_regs(unsigned int cpu)
>  	}
> 
>  	hv_set_msr(HV_MSR_SIEFP, siefp.as_uint64);
> +}
> +
> +static void hv_hyp_synic_disable_interrupts(void)
> +{
> +	union hv_synic_scontrol sctrl;
> 
>  	/* Disable the global synic bit */
>  	sctrl.as_uint64 = hv_get_msr(HV_MSR_SCONTROL);
>  	sctrl.enable = 0;
>  	hv_set_msr(HV_MSR_SCONTROL, sctrl.as_uint64);
> +}
> 
> -	if (vmbus_irq != -1)
> -		disable_percpu_irq(vmbus_irq);
> +static void hv_para_synic_disable_regs(unsigned int cpu)
> +{
> +	/*
> +	 * When a get/set register error is encountered, the function
> +	 * returns as the paravisor may not support these registers.
> +	 */
> +	int err;
> +	union hv_synic_simp simp;
> +	union hv_synic_siefp siefp;
> +
> +	struct hv_per_cpu_context *hv_cpu
> +		= per_cpu_ptr(hv_context.cpu_context, cpu);
> +
> +	/* Disable SynIC's message page in the paravisor. */
> +	err = hv_para_get_synic_register(HV_MSR_SIMP, &simp.as_uint64);
> +	if (err)
> +		return;
> +	simp.simp_enabled = 0;
> +
> +	if (hv_cpu->para_synic_message_page) {
> +		free_page((u64)(hv_cpu->para_synic_message_page));
> +		hv_cpu->para_synic_message_page = NULL;
> +	}

Freeing the para_synic_message_page memory here causes problems if a
CPU is taken offline, then back online. This function gets called when the CPU
goes offline, so the para_synic_message_page and para_synic_event_page
memory is freed. Then if the CPU comes back online,
hv_para_synic_enable_regs() runs without the memory being allocated
again.

Be sure to test the CPU offlining/onlining scenario to make sure it
works throughout the stack, including the paravisor.

> +
> +	err = hv_para_set_synic_register(HV_MSR_SIMP, simp.as_uint64);
> +	if (err)
> +		return;
> +
> +	/* Disable SynIC's event page in the paravisor. */
> +	err = hv_para_get_synic_register(HV_MSR_SIEFP, &siefp.as_uint64);
> +	if (err)
> +		return;
> +	siefp.siefp_enabled = 0;
> +
> +	if (hv_cpu->para_synic_event_page) {
> +		free_page((u64)(hv_cpu->para_synic_event_page));
> +		hv_cpu->para_synic_event_page = NULL;

As above, don't free the memory here.

> +	}
> +
> +	hv_para_set_synic_register(HV_MSR_SIEFP, siefp.as_uint64);
> +}
> +
> +static void hv_para_synic_disable_interrupts(void)
> +{
> +	union hv_synic_scontrol sctrl;
> +	int err;
> +
> +	/* Disable the global synic bit */
> +	err = hv_para_get_synic_register(HV_MSR_SCONTROL, &sctrl.as_uint64);
> +	if (err)
> +		return;
> +	sctrl.enable = 0;
> +	hv_para_set_synic_register(HV_MSR_SCONTROL, sctrl.as_uint64);
>  }
> 
>  #define HV_MAX_TRIES 3
> @@ -398,16 +535,18 @@ void hv_hyp_synic_disable_regs(unsigned int cpu)
>   * that the normal interrupt handling mechanism will find and process the channel interrupt
>   * "very soon", and in the process clear the bit.
>   */
> -static bool hv_synic_event_pending(void)
> +static bool __hv_synic_event_pending(union hv_synic_event_flags *event, int sint)
>  {
> -	struct hv_per_cpu_context *hv_cpu = this_cpu_ptr(hv_context.cpu_context);
> -	union hv_synic_event_flags *event =
> -		(union hv_synic_event_flags *)hv_cpu->hyp_synic_event_page + VMBUS_MESSAGE_SINT;
> -	unsigned long *recv_int_page = event->flags; /* assumes VMBus version >= VERSION_WIN8 */
> +	unsigned long *recv_int_page;
>  	bool pending;
>  	u32 relid;
>  	int tries = 0;
> 
> +	if (!event)
> +		return false;
> +
> +	event += sint;
> +	recv_int_page = event->flags; /* assumes VMBus version >= VERSION_WIN8 */
>  retry:
>  	pending = false;
>  	for_each_set_bit(relid, recv_int_page, HV_EVENT_FLAGS_COUNT) {
> @@ -424,6 +563,17 @@ static bool hv_synic_event_pending(void)
>  	return pending;
>  }
> 
> +static bool hv_synic_event_pending(void)
> +{
> +	struct hv_per_cpu_context *hv_cpu = this_cpu_ptr(hv_context.cpu_context);
> +	union hv_synic_event_flags *hyp_synic_event_page = hv_cpu->hyp_synic_event_page;
> +	union hv_synic_event_flags *para_synic_event_page = hv_cpu->para_synic_event_page;
> +
> +	return
> +		__hv_synic_event_pending(hyp_synic_event_page, VMBUS_MESSAGE_SINT) ||
> +		__hv_synic_event_pending(para_synic_event_page, VMBUS_MESSAGE_SINT);
> +}
> +
>  static int hv_pick_new_cpu(struct vmbus_channel *channel)
>  {
>  	int ret = -EBUSY;
> @@ -517,6 +667,14 @@ int hv_synic_cleanup(unsigned int cpu)
>  	hv_stimer_legacy_cleanup(cpu);
> 
>  	hv_hyp_synic_disable_regs(cpu);
> +	if (vmbus_is_confidential()) {
> +		hv_para_synic_disable_regs(cpu);
> +		hv_para_synic_disable_interrupts();

The ordering of the interrupt handling here is a bit scary to me, but maybe
it's right. hv_hyp_synic_disable_regs() first masks the SINT, and because it's
a SINT MSR, it does so first in the hypervisor and then in the paravisor.
Then hv_para_synic_disable_regs() resets the SIMP and SIEFP. Finally
hv_para_synic_disable_interrupts() does a global interrupt disable via
the paravisor. I worry about there being gaps between steps where bad
things can happen. Might want to confirm with the hypervisor and
paravisor folks.

When VMBus is not confidential, the sequencing is the same as it is
now, which is good. There's presumably no reason to change that.

This is another aspect of my earlier comment about adding comments
that clearly describe how interrupts are to be setup/disabled when both
SynICs are present.

> +	} else {
> +		hv_hyp_synic_disable_interrupts();
> +	}
> +	if (vmbus_irq != -1)
> +		disable_percpu_irq(vmbus_irq);
> 
>  	return ret;
>  }
> --
> 2.43.0

Powered by blists - more mailing lists