On Mon, Jul 28, 2025 at 10:45 PM Michael Kelley <mhkli...@outlook.com> wrote:
>
> From: Tianyu Lan <ltyker...@gmail.com> 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 <ti...@microsoft.com>
> > ---
> > 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.
>
Agree. Have updated in the RFC V5 series. Thanks for your suggestion, Michael!
--
Thanks
Tianyu Lan
