RE: [PATCH 6/6] Drivers: hv: vmbus: Support TDX guests

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> From: Zhi Wang <zhi.wang.linux@xxxxxxxxx>
> Sent: Friday, January 6, 2023 3:00 AM
> > diff --git a/arch/x86/mm/pat/set_memory.c
> > @@ -2120,7 +2120,7 @@ static int __set_memory_enc_pgtable(unsigned
> long
> > addr, int numpages, bool enc)
> >  static int __set_memory_enc_dec(unsigned long addr, int numpages, bool
> > enc) {
> > -	if (hv_is_isolation_supported())
> > +	if (hv_is_isolation_supported() && !hv_isolation_type_tdx())
> >  		return hv_set_mem_host_visibility(addr, numpages, !enc);
> >
> >  	if (cc_platform_has(CC_ATTR_MEM_ENCRYPT))

The change here is kind of a hack to not call hv_set_mem_host_visibility()
for TDX guests on Hyper-V. The original change was also a hack to me to
call hv_set_mem_host_visibility() for SNP guests with pavavisor on Hyper-V.

> Let's say there will be four cases:
> ----
> case a. SEV-SNP guest with paravisor
> 
> In the code, this case is represented by:
> 
> hv_is_isolation_supported() && hv_isolation_type_snp()
> hv_is_isolation_supported() && !hv_isolation_type_tdx()
These look bad to me...
 
> case b. TDX guest with paravisor
> ?
As of now, this is not supported yet. I'll need to figure out how exactly
this scenario will look like.

> case c. SEV-SNP guest *without* paravisor
> ?
Tianyu Lan is working on this:
https://lwn.net/ml/linux-kernel/20221119034633.1728632-1-ltykernel@xxxxxxxxx/
set_memory_decrypted() calls __set_memory_enc_dec() directly. This
is the same as a SNP guest running on KVM.
 
> case d. TDX guest *without* paravisor
> 
> In the code, this case is represented by:
> 
> hv_is_isolation_supported() && hv_isolation_type_tdx()
This looks bad to me...

> ----
> 
> 1. It would be better to use "hv_is_isolation_supported() &&
> hv_isolation_type_snp()" to represent case a to avoid confusion in the
> above patch.
> 
> 2. For now, hv_is_isolation_supported() only shows if the guest is a CC
> guest or not. hv_isolation_type_*() only represent SNP or TDX but
> not "w/ or w/o paravisor".
> 
> How are you going to represent case b and c in __set_memory_enc_dec()?
> 
> I think you are looking for something to show if the guest is running
> with a paravisor or not here:
> 
> if (hv_is_isolation_supported() && hv_is_isolation_with_paravisor())
> ...
> 
> Thanks,
> Zhi.

Michael's patchset removes the special path for SNP with pavavisor on Hyper-V:
https://lwn.net/ml/linux-kernel/1669951831-4180-7-git-send-email-mikelley%40microsoft.com/

With Michael's patchset, I don't need the change to __set_memory_enc_dec()
at all. The plan was that Michael's patchset would be merged into the upstream
first and I would rebase my TDX patchset accordingly, but Michael's patchset
has been pending for almost 2 months... 

so I probably need to post v3 with the below version, which looks a little
better to me because it hides the Hyper-V specific logic in a Hyper-V specific
file arch/x86/hyperv/ivm.c, and if necessary we can change the implementation
of hv_set_memory_enc_dec_needed() in future, e.g. Tianyu can change
hv_set_memory_enc_dec_needed() to distinguish between SNP with pavavisor
and SNP without pavavisor. Of course, I still hope Michael's patchset would
be merged soon so I can avoid this kind of mess...

diff --git a/arch/x86/hyperv/ivm.c b/arch/x86/hyperv/ivm.c
index 07e4253b5809..4398042f10d5 100644
--- a/arch/x86/hyperv/ivm.c
+++ b/arch/x86/hyperv/ivm.c
@@ -258,6 +258,11 @@ bool hv_is_isolation_supported(void)
        return hv_get_isolation_type() != HV_ISOLATION_TYPE_NONE;
 }

+bool hv_set_memory_enc_dec_needed(void)
+{
+       return hv_is_isolation_supported() && !hv_isolation_type_tdx();
+}
+
 DEFINE_STATIC_KEY_FALSE(isolation_type_snp);

 /*

diff --git a/arch/x86/mm/pat/set_memory.c b/arch/x86/mm/pat/set_memory.c
index 2e5a045731de..5892196f8ade 100644
--- a/arch/x86/mm/pat/set_memory.c
+++ b/arch/x86/mm/pat/set_memory.c
@@ -2120,7 +2120,7 @@ static int __set_memory_enc_pgtable(unsigned long addr, int numpages, bool enc)

 static int __set_memory_enc_dec(unsigned long addr, int numpages, bool enc)
 {
-       if (hv_is_isolation_supported())
+       if (hv_set_memory_enc_dec_needed())
                return hv_set_mem_host_visibility(addr, numpages, !enc);

        if (cc_platform_has(CC_ATTR_MEM_ENCRYPT))
diff --git a/drivers/hv/hv_common.c b/drivers/hv/hv_common.c
index a9a03ab04b97..192dcf295dfc 100644
--- a/drivers/hv/hv_common.c
+++ b/drivers/hv/hv_common.c
@@ -262,6 +262,12 @@ bool __weak hv_is_isolation_supported(void)
 }
 EXPORT_SYMBOL_GPL(hv_is_isolation_supported);

+bool __weak hv_set_memory_enc_dec_needed(void)
+{
+       return false;
+}
+EXPORT_SYMBOL_GPL(hv_set_memory_enc_dec_needed);
+
 bool __weak hv_isolation_type_snp(void)
 {
        return false;
diff --git a/include/asm-generic/mshyperv.h b/include/asm-generic/mshyperv.h
index bfb9eb9d7215..b7b1b18c9854 100644
--- a/include/asm-generic/mshyperv.h
+++ b/include/asm-generic/mshyperv.h
@@ -262,6 +262,7 @@ bool hv_is_hyperv_initialized(void);
 bool hv_is_hibernation_supported(void);
 enum hv_isolation_type hv_get_isolation_type(void);
 bool hv_is_isolation_supported(void);
+bool hv_set_memory_enc_dec_needed(void);
 bool hv_isolation_type_snp(void);
 u64 hv_ghcb_hypercall(u64 control, void *input, void *output, u32 input_size);
 void hyperv_cleanup(void);
@@ -274,6 +275,7 @@ static inline bool hv_is_hyperv_initialized(void) { return false; }
 static inline bool hv_is_hibernation_supported(void) { return false; }
 static inline void hyperv_cleanup(void) {}
 static inline bool hv_is_isolation_supported(void) { return false; }
+static inline bool hv_set_memory_enc_dec_needed(void) { return false; }
 static inline enum hv_isolation_type hv_get_isolation_type(void)
 {
        return HV_ISOLATION_TYPE_NONE;





[Index of Archives]     [Linux Kernel]     [Kernel Newbies]     [x86 Platform Driver]     [Netdev]     [Linux Wireless]     [Netfilter]     [Bugtraq]     [Linux Filesystems]     [Yosemite Discussion]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Device Mapper]

  Powered by Linux