RE: [RFC PATCH V4 2/4] drivers/hv: Allow vmbus message synic interrupt injected from Hyper-V

[Michael Kelley]

From: Tianyu Lan <ltykernel@xxxxxxxxx> Sent: Saturday, July 26, 2025 6:43 AM
> 

Nit: The patch "Subject:" prefix here should be "Drivers: hv:" with no slash, as
it was in v3 of the patch. That's admittedly not consistent with "x86/hyperv:"
that is used for the other patches in this series, but it is consistent with historical
practice for the files in the drivers/hv folder. You have to look at past commits
for a particular file to see what the typical prefix is.

Michael

> When Secure AVIC is enabled, VMBus driver should
> call x2apic Secure AVIC interface to allow Hyper-V
> to inject VMBus message interrupt.
> 
> Signed-off-by: Tianyu Lan <tiala@xxxxxxxxxxxxx>
> ---
> Change since RFC V3:
>        - Disable VMBus Message interrupt via hv_enable_
>        	 coco_interrupt() in the hv_synic_disable_regs().
> ---
>  arch/x86/hyperv/hv_apic.c      | 5 +++++
>  drivers/hv/hv.c                | 2 ++
>  drivers/hv/hv_common.c         | 5 +++++
>  include/asm-generic/mshyperv.h | 1 +
>  4 files changed, 13 insertions(+)
> 
> diff --git a/arch/x86/hyperv/hv_apic.c b/arch/x86/hyperv/hv_apic.c
> index e669053b637d..a8de503def37 100644
> --- a/arch/x86/hyperv/hv_apic.c
> +++ b/arch/x86/hyperv/hv_apic.c
> @@ -53,6 +53,11 @@ static void hv_apic_icr_write(u32 low, u32 id)
>  	wrmsrq(HV_X64_MSR_ICR, reg_val);
>  }
> 
> +void hv_enable_coco_interrupt(unsigned int cpu, unsigned int vector, bool set)
> +{
> +	apic_update_vector(cpu, vector, set);
> +}
> +
>  static u32 hv_apic_read(u32 reg)
>  {
>  	u32 reg_val, hi;
> diff --git a/drivers/hv/hv.c b/drivers/hv/hv.c
> index 308c8f279df8..aa384dbf38ac 100644
> --- a/drivers/hv/hv.c
> +++ b/drivers/hv/hv.c
> @@ -310,6 +310,7 @@ void hv_synic_enable_regs(unsigned int cpu)
>  	if (vmbus_irq != -1)
>  		enable_percpu_irq(vmbus_irq, 0);
>  	shared_sint.as_uint64 = hv_get_msr(HV_MSR_SINT0 + VMBUS_MESSAGE_SINT);
> +	hv_enable_coco_interrupt(cpu, vmbus_interrupt, true);
> 
>  	shared_sint.vector = vmbus_interrupt;
>  	shared_sint.masked = false;

Something I just noticed. The existing code in hv_synic_enable_regs()
is reading the SINT MSR, updating some values, and then writing back
the SINT MSR. Those steps act as a unit to update the MSR. You've added
the call to hv_enable_coco_interrupts() in the middle of that unit, which
implies there might be a reason for it. If there's not a reason, I would
expect the call to hv_enable_coco_interrupt() to be before the unit,
not in the middle of it.

> @@ -342,6 +343,7 @@ void hv_synic_disable_regs(unsigned int cpu)
>  	union hv_synic_scontrol sctrl;
> 
>  	shared_sint.as_uint64 = hv_get_msr(HV_MSR_SINT0 + VMBUS_MESSAGE_SINT);
> +	hv_enable_coco_interrupt(cpu, vmbus_interrupt, false);

Same here with the hv_enable_coco_interrupt() call in the middle
of the unit that is updating the SINT MSR. In the disable path, I would
have expected hv_enable_coco_interrupt() to be *after* the unit so
that disable operations are in reverse order of the corresponding enable
operation.

> 
>  	shared_sint.masked = 1;
> 
> diff --git a/drivers/hv/hv_common.c b/drivers/hv/hv_common.c
> index 49898d10faff..0f024ab3d360 100644
> --- a/drivers/hv/hv_common.c
> +++ b/drivers/hv/hv_common.c
> @@ -716,6 +716,11 @@ u64 __weak hv_tdx_hypercall(u64 control, u64 param1, u64 param2)
>  }
>  EXPORT_SYMBOL_GPL(hv_tdx_hypercall);
> 
> +void __weak hv_enable_coco_interrupt(unsigned int cpu, unsigned int vector, bool set)
> +{
> +}
> +EXPORT_SYMBOL_GPL(hv_enable_coco_interrupt);
> +
>  void hv_identify_partition_type(void)
>  {
>  	/* Assume guest role */
> diff --git a/include/asm-generic/mshyperv.h b/include/asm-generic/mshyperv.h
> index a729b77983fa..7907c9878369 100644
> --- a/include/asm-generic/mshyperv.h
> +++ b/include/asm-generic/mshyperv.h
> @@ -333,6 +333,7 @@ bool hv_is_isolation_supported(void);
>  bool hv_isolation_type_snp(void);
>  u64 hv_ghcb_hypercall(u64 control, void *input, void *output, u32 input_size);
>  u64 hv_tdx_hypercall(u64 control, u64 param1, u64 param2);
> +void hv_enable_coco_interrupt(unsigned int cpu, unsigned int vector, bool set);
>  void hyperv_cleanup(void);
>  bool hv_query_ext_cap(u64 cap_query);
>  void hv_setup_dma_ops(struct device *dev, bool coherent);
> --
> 2.25.1
>