Re: [PATCH v5 3/5] hyperv: Enable the hypercall output page for the VTL mode

[Stanislav Kinsburskii <skinsburskii@linux.microsoft.com>]

On Tue, Jan 07, 2025 at 03:11:15PM -0800, Roman Kisel wrote:
> 
> 
> On 1/7/2025 11:18 AM, Stanislav Kinsburskii wrote:
> > On Mon, Jan 06, 2025 at 01:07:25PM -0800, Roman Kisel wrote:
> > 
> 
> [...]
> 
> > My point is that the proposed fix looks more like an Underhill-tailored
> > bandage and doesn't take the needs of other stake holders into
> > consideration.
> The patch takes as much into consideration as present in the hyperv-next
> tree. Working on the open-source project seems to be harder otherwise.
> A bandage, or not, that's a matter of opinion. There's a been a break,
> here's the bandage.
> 
> > 
> > What is the urgency in merging of this particular change?
> 
> The get_vtl function is broken thus blocking any further work on
> upstreaming VTL mode patches, ARM64 and more. That's not an urgent
> urgency where customers are in pain, more like the urgency of needing
> to take the trash out, and until that happens, continuing inhaling the
> fumes.
> 
> The urgency of unblocking is to continue work on proposing VTL mode
> patches not to carry lots of out-of-tree code in the fork.
> 
> There might be a future where the Hyper-V code offers an API surface
> covering needs of consumers like dom0 and VTLs whereby they maybe can
> be built as an out-of-tree modules so the opinions wouldn't clash as
> much.
> 
> Avoiding using the output hypercall page leads to something like[1]
> and it looks quite complicated although that's the bare bones, lots
> of notes.
> 

How is this related to the original discussion?
My concern was about the piece allocating of the output page guarded by
the VTL config option.

Thanks,
Stas

> [1]
> 
> /*
>  * Fast extended hypercall with 20 bytes of input and 16 bytes of
>  * output for getting a VP register.
>  *
>  * NOTES:
>  *  1. The function is __init only atm, so the XMM context isn't
>  *     used by the user mode.
>  *  2. X86_64 only.
>  *  3. Fast extended hypercalls may use XMM0..XMM6, and XMM is
>  *     architerctural on X86_64 yet the support should be enabled
>  *     in the CR's. Here, need RDX, R8 and XMM0 for input and RDX,
>  *     R8 for output
>  *  4. No provisions for TDX and SEV-SNP for the sake of simplicity
>  *     (the hypervisor cannot see the guest registers in the
>  *     confidential VM), would need to fallback.
>  *  5. The robust implementation would need to check if fast extended
>  *     hypercalls are available by checking the synthehtic CPUID leaves.
>  *     A separate leaf indicates fast output support.
>  *     It _almost_ certainly has to be, unless somehow disabled, hard
>  *     to see why that would be needed.
>  */
> struct hv_u128 {
> 	u64 low_part;
> 	u64 high_part;
> } __packed;
> 
> static __init u64 hv_vp_get_register_xfast(u32 reg_name,
> 		struct hv_u128 *value)
> {
> 	u64 control = HV_HYPERCALL_REP_COMP_1 | HVCALL_GET_VP_REGISTERS |
> 					HV_HYPERCALL_FAST_BIT;
> 	unsigned long flags;
> 	u64 hv_status;
> 
> 	union {
> 		struct hv_get_vp_registers_input input;
> 		struct {
> 			u64 lo;
> 			u64 hi;
> 		} __packed as_u128;
> 	} hv_input;
> 	register u64 rdx asm("rdx");
> 	register u64 r8 asm("r8");
> 	register u64 r12 asm("r12");
> 
> 	local_irq_save(flags);
> 
> 	hv_input.as_u128.lo = hv_input.as_u128.hi = 0;
> 	hv_input.input.header.partitionid = HV_PARTITION_ID_SELF;
> 	hv_input.input.header.vpindex = HV_VP_INDEX_SELF;
> 	hv_input.input.header.inputvtl = 0;
> 
> 	rdx = hv_input.as_u128.lo;
> 	r8 = hv_input.as_u128.hi;
> 	r12 = reg_name;
> 
> 	__asm__ __volatile__(
> 			"subq		$16, %%rsp\n"
> 			"movups		%%xmm0, 16(%%rsp)\n"
> 			"movd		%%r12, %%xmm0\n"
> 			CALL_NOSPEC
> 			"movups		16(%%rsp), %%xmm0\n"
> 			"addq		$16, %%rsp\n"
> 			: "=a" (hv_status), ASM_CALL_CONSTRAINT,
> 			"+c" (control), "+r" (rdx), "+r" (r8)
> 			: THUNK_TARGET(hv_hypercall_pg), "r"(r12)
> 			: "cc", "memory", "r9", "r10", "r11");
> 
> 	if (hv_result_success(hv_status)) {
> 		value->low_part = rdx;
> 		value->high_part = r8;
> 	}
> 
> 	local_irq_restore(flags);
> 	return hv_status;
> }
> 
> #if IS_ENABLED(CONFIG_HYPERV_VTL_MODE)
> u8 __init get_vtl(void)
> {
> 	struct hv_u128 reg_value;
> 	u64 ret = hv_vp_get_register_xfast(HV_REGISTER_VSM_VP_STATUS, &reg_value);
> 
> 	if (hv_result_success(ret)) {
> 		ret = reg_value.low_part & HV_VTL_MASK;
> 	} else {
> 		pr_err("Failed to get VTL(error: %lld) exiting...\n", ret);
> 		BUG();
> 	}
> 
> 	return ret;
> }
> #endif
> 
> > 
> > Thanks,
> > Stas
> 
> -- 
> Thank you,
> Roman
>