Re: [PATCH] x86/mm: Print the encryption features correctly when a paravisor is present
On Fri, Oct 20, 2023 at 08:00:13PM +, Dexuan Cui wrote:
> Currently arch/x86/mm/mem_encrypt.c: print_mem_encrypt_feature_info()
> prints an incorrect and confusing message
> "Memory Encryption Features active: AMD SEV".
> when an Intel TDX VM with a paravisor runs on Hyper-V.
>
> So I think a kernel patch is needed.
So I'm trying to parse this:
"Hyper-V provides two modes for running a TDX/SNP VM:
1) In TD Partitioning mode (TDX) or vTOM mode (SNP) with a paravisor;
2) In "fully enlightened" mode with the normal TDX shared bit or SNP C-bit
control over page encryption, and no paravisor."
and it all sounds like word salad to me.
The fact that you've managed to advertize a salad of CPUID bits to the
guest to lead to such confusing statement, sounds like a major insanity.
> the native TDX/SNP CPUID capability is hidden from the VM
Why do you wonder then that it detects wrong?! You're hiding it!
> but cc_platform_has(CC_ATTR_MEM_ENCRYPT) and
> cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT) are true;
I guess you need to go to talk to Michael:
812b0597fb40 ("x86/hyperv: Change vTOM handling to use standard coco
mechanisms")
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
RE: [PATCH] x86/mm: Print the encryption features correctly when a paravisor is present
> From: Dave Hansen > Sent: Friday, October 20, 2023 1:14 PM > To: Dexuan Cui ; KY Srinivasan > [...] > On 10/20/23 13:00, Dexuan Cui wrote: > >> OK, then what good is this patch in the first place? If you are right, > >> then this would give equivalent information: > >> > >> cat /proc/cpuinfo | grep -q Intel && echo 'TDX' > >> cat /proc/cpuinfo | grep -q AMD && echo 'SEV' > >> > >> No kernel patching needed, right? > > Currently arch/x86/mm/mem_encrypt.c: > print_mem_encrypt_feature_info() > > prints an incorrect and confusing message > > "Memory Encryption Features active: AMD SEV". > > when an Intel TDX VM with a paravisor runs on Hyper-V. > > > > So I think a kernel patch is needed. > > How about either removing the message entirely or removing the ": AMD > SEV" part? It looks good to me if we can update/remove the message in print_mem_encrypt_feature_info(), but I guess AMD folks might want to keep the ": AMD SEV" part?
Re: [PATCH] x86/mm: Print the encryption features correctly when a paravisor is present
On 10/20/23 13:00, Dexuan Cui wrote: >> OK, then what good is this patch in the first place? If you are right, >> then this would give equivalent information: >> >> cat /proc/cpuinfo | grep -q Intel && echo 'TDX' >> cat /proc/cpuinfo | grep -q AMD && echo 'SEV' >> >> No kernel patching needed, right? > Currently arch/x86/mm/mem_encrypt.c: print_mem_encrypt_feature_info() > prints an incorrect and confusing message > "Memory Encryption Features active: AMD SEV". > when an Intel TDX VM with a paravisor runs on Hyper-V. > > So I think a kernel patch is needed. How about either removing the message entirely or removing the ": AMD SEV" part?
RE: [PATCH] x86/mm: Print the encryption features correctly when a paravisor is present
> From: Dave Hansen > Sent: Friday, October 20, 2023 11:40 AM > To: Dexuan Cui ; KY Srinivasan > [...] > On 10/19/23 23:01, Dexuan Cui wrote: > > This patch only modifies x86 related files. I think it's unlikely to see > > a third hardware Coco implementation for x86 in the foreseeable feature > (?) > > OK, then what good is this patch in the first place? If you are right, > then this would give equivalent information: > > cat /proc/cpuinfo | grep -q Intel && echo 'TDX' > cat /proc/cpuinfo | grep -q AMD && echo 'SEV' > > No kernel patching needed, right? Currently arch/x86/mm/mem_encrypt.c: print_mem_encrypt_feature_info() prints an incorrect and confusing message "Memory Encryption Features active: AMD SEV". when an Intel TDX VM with a paravisor runs on Hyper-V. So I think a kernel patch is needed.
Re: [PATCH] x86/mm: Print the encryption features correctly when a paravisor is present
On 10/19/23 23:01, Dexuan Cui wrote: > This patch only modifies x86 related files. I think it's unlikely to see > a third hardware Coco implementation for x86 in the foreseeable feature (?) OK, then what good is this patch in the first place? If you are right, then this would give equivalent information: cat /proc/cpuinfo | grep -q Intel && echo 'TDX' cat /proc/cpuinfo | grep -q AMD && echo 'SEV' No kernel patching needed, right?
RE: [PATCH] x86/mm: Print the encryption features correctly when a paravisor is present
> From: Dave Hansen
> Sent: Thursday, October 19, 2023 8:54 AM
> To: Dexuan Cui ; KY Srinivasan
> [...]
> > --- a/arch/x86/hyperv/ivm.c
> > +++ b/arch/x86/hyperv/ivm.c
> > @@ -450,6 +450,16 @@ static bool hv_is_private_mmio(u64 addr)
> > return false;
> > }
> >
> > +static void hv_print_mem_enc_feature_info(void)
> > +{
> > + enum hv_isolation_type type = hv_get_isolation_type();
> > +
> > + if (type == HV_ISOLATION_TYPE_SNP)
> > + pr_info("Memory Encryption Features active: AMD
> SEV\n");
> > + else if (type == HV_ISOLATION_TYPE_TDX)
> > + pr_info("Memory Encryption Features active: Intel
> > TDX\n");
> > +}
>
> If we draw this to its logical conclusion, every paravisor will need a
> pr_info() for every hardware CoCo implementation. That M*N pr_info()s.
> That seems nuts.
This patch only modifies x86 related files. I think it's unlikely to see
a third hardware Coco implementation for x86 in the foreseeable feature (?)
When we have a third implementation, I suppose more code, e.g., the existing
print_mem_encrypt_feature_info(), will have to be changed as well.
Currently it looks like there is only 1 paravisor implementation.
I think we'll know if some code can be shared only when a second paravisor
implementation appears.
I can use the below version if you think it's better:
static const char *hv_mem_enc_features[] = {
[ HV_ISOLATION_TYPE_SNP ] = "AMD SEV",
[ HV_ISOLATION_TYPE_TDX ] = "Intel TDX",
};
static void hv_print_mem_enc_feature_info(void)
{
enum hv_isolation_type type = hv_get_isolation_type();
if (type < HV_ISOLATION_TYPE_SNP || type > HV_ISOLATION_TYPE_TDX)
return;
pr_info("Memory Encryption Features active:: %s\n",
hv_mem_enc_features[type]);
}
Thanks,
Dexuan
Re: [PATCH] x86/mm: Print the encryption features correctly when a paravisor is present
> --- a/arch/x86/hyperv/ivm.c
> +++ b/arch/x86/hyperv/ivm.c
> @@ -450,6 +450,16 @@ static bool hv_is_private_mmio(u64 addr)
> return false;
> }
>
> +static void hv_print_mem_enc_feature_info(void)
> +{
> + enum hv_isolation_type type = hv_get_isolation_type();
> +
> + if (type == HV_ISOLATION_TYPE_SNP)
> + pr_info("Memory Encryption Features active: AMD SEV\n");
> + else if (type == HV_ISOLATION_TYPE_TDX)
> + pr_info("Memory Encryption Features active: Intel TDX\n");
> +}
If we draw this to its logical conclusion, every paravisor will need a
pr_info() for every hardware CoCo implementation. That M*N pr_info()s.
That seems nuts.
