From: Michael Kelley (LINUX) <redacted>
Sent: Monday, August 21, 2023 12:33 PM
 [...]
From: Dexuan Cui <decui@microsoft.com> Sent: Sunday, August 20, 2023

quoted

The new variable hyperv_paravisor_present is set only when the VM
is a SNP/TDX with the paravisor running: see ms_hyperv_init_platform().

In many places, hyperv_paravisor_present can replace

You said "In many places".  Are there places where it can't replace
ms_hyperv.paravisor_present?  It looks like all the uses are gone
after this patch.

Sorry for the inaccuracy. I meant to say "everywhere" rather than
"In many places".  I think hyperv_paravisor_present and
ms_hyperv.paravisor_present can be used interchangeably except that
I need to use hyperv_paravisor_present in arch/x86/include/asm/mshyperv.h
to avoid the header file dependency issue.

As we discussed offline, we'll add some hypercall function structure in future
for different VM types, and then hyperv_paravisor_present and
ms_hyperv.paravisor_present would go away when we make hypercalls.

So let me use hyperv_paravisor_present only in
arch/x86/include/asm/mshyperv.h to make the patch smaller.

quoted

ms_hyperv.paravisor_present, and it's also used to replace
hv_isolation_type_snp() in drivers/hv/hv.c.

Call hv_vtom_init() when it's a VBS VM or when hyperv_paravisor_present
is true (i.e. the VM is a SNP/TDX VM with the paravisor).

Enhance hv_vtom_init() for a TDX VM with the paravisor.

The biggest motive to introduce hyperv_paravisor_present is that we
can not use ms_hyperv.paravisor_present in

arch/x86/include/asm/mshyperv.h:

quoted

that would introduce a complicated header file dependency issue.

The discussion in this commit messages about hyperv_paravisor_present
is a bit scattered and confusing.  I think you are introducing the global
variable
to solve the header file dependency issue.  Otherwise, the ms_hyperv field
would be equivalent.  Then you are using hyperv_paravisor_present for
several purposes, including to decide whether to call hv_vtom_init() and
to simplify the logic in drivers/hv/hv.c.  Maybe you could reorganize
the commit message a bit to be more direct regarding the purpose.

The new changelog will be:

    x86/hyperv: Introduce a global variable hyperv_paravisor_present

    The new variable hyperv_paravisor_present is set only when the VM
    is a SNP/TDX VM with the paravisor running: see ms_hyperv_init_platform().

    We introduce hyperv_paravisor_present because we can not use
    ms_hyperv.paravisor_present in arch/x86/include/asm/mshyperv.h:

    struct ms_hyperv_info is defined in include/asm-generic/mshyperv.h, which
    is included at the end of arch/x86/include/asm/mshyperv.h, but at the
    beginning of arch/x86/include/asm/mshyperv.h, we would already need to use
    struct ms_hyperv_info in hv_do_hypercall().

    We use hyperv_paravisor_present only in include/asm-generic/mshyperv.h,
    and use ms_hyperv.paravisor_present elsewhere. In the future, we'll
    introduce a hypercall function structure for different VM types, and
    at boot time, the right function pointers would be written into the
    structure so that runtime testing of TDX vs. SNP vs. normal will be
    avoided and hyperv_paravisor_present will no longer be needed.

    Call hv_vtom_init() when it's a VBS VM or when ms_hyperv.paravisor_present
    is true, i.e. the VM is a SNP VM or TDX VM with the paravisor.

    Enhance hv_vtom_init() for a TDX VM with the paravisor.

    In hv_common_cpu_init(), don't decrypt the hyperv_pcpu_input_arg
    for a TDX VM with the paravisor, just like we don't decrypt the page
    for a SNP VM with the paravisor.

BTW, please refer to the link for the v3 version of this patch (WIP):

quoted

In arch/x86/include/asm/mshyperv.h, _hv_do_fast_hypercall8()
is changed to specially handle HVCALL_SIGNAL_EVENT for a TDX VM with

the

quoted

paravisor, because such a VM must use TDX GHCI (see hv_tdx_hypercall())
for this hypercall. See vmbus_set_event() -> hv_do_fast_hypercall8() ->
_hv_do_fast_hypercall8() -> hv_tdx_hypercall().

Embedding the special case for HVCALL_SIGNAL_EVENT within
hv_do_fast_hypercall8() is not consistent with how this special case
is handled for SNP.  For SNP, the special case is coded directly into
vmbus_set_event().  Any reason not to do the same for TDX + paravisor?

Ok, will handle it directly in vmbus_set_event().

quoted

@@ -497,13 +497,29 @@ int hv_snp_boot_ap(int cpu, unsigned long

start_ip)

quoted

 void __init hv_vtom_init(void)
 {
+	enum hv_isolation_type type = hv_get_isolation_type();
 	/*
 	 * By design, a VM using vTOM doesn't see the SEV setting,
 	 * so SEV initialization is bypassed and sev_status isn't set.
 	 * Set it here to indicate a vTOM VM.
 	 */

The above comment applies just to the case HV_ISOLATION_TYPE_SNP,
not to the entire switch statement, so it should be moved under the
case.

Will fix.

quoted

[...]

--- a/arch/x86/include/asm/mshyperv.h
+++ b/arch/x86/include/asm/mshyperv.h

[...]

quoted

@@ -134,7 +135,9 @@ static inline u64 _hv_do_fast_hypercall8(u64

control, u64 input1)

quoted

 	u64 hv_status;

 #ifdef CONFIG_X86_64
-	if (hv_isolation_type_tdx())
+	if (hv_isolation_type_tdx() &&
+		(!hyperv_paravisor_present ||
+		 control == (HVCALL_SIGNAL_EVENT |

HV_HYPERCALL_FAST_BIT)))

See comment above.  This would be more consistent with SNP if it were
handled directly in vmbus_set_event().

Will fix.

quoted

--- a/drivers/hv/connection.c
+++ b/drivers/hv/connection.c

@@ -484,7 +484,7 @@ void vmbus_set_event(struct vmbus_channel

*channel)

quoted

 	++channel->sig_events;

-	if (hv_isolation_type_snp())
+	if (hv_isolation_type_snp() && hyperv_paravisor_present)

This code change seems to be more properly part of Patch 9 in the
series when hv_isolation_type_en_snp() goes away.

The change here is part of the efforts to correctly support hypercalls for
a TDX VM with the paravisor. Patch 9 is just a clean-up patch, which is
not really required for a TDX VM (with or with the paravisor) to run on
Hyper-V, so IMO it's better to keep the change here in this patch.

BTW, please refer to the link for the v3 version of this patch (WIP):