[v3,07/14] x86/hyperv: Change vTOM handling to use standard coco mechanisms

  Hyper-V guests on AMD SEV-SNP hardware have the option of using the
"virtual Top Of Memory" (vTOM) feature specified by the SEV-SNP
architecture. With vTOM, shared vs. private memory accesses are
controlled by splitting the guest physical address space into two
halves.  vTOM is the dividing line where the uppermost bit of the
physical address space is set; e.g., with 47 bits of guest physical
address space, vTOM is 0x40000000000 (bit 46 is set).  Guest phyiscal
memory is accessible at two parallel physical addresses -- one below
vTOM and one above vTOM.  Accesses below vTOM are private (encrypted)
while accesses above vTOM are shared (decrypted). In this sense, vTOM
is like the GPA.SHARED bit in Intel TDX.

Support for Hyper-V guests using vTOM was added to the Linux kernel in
two patch sets[1][2]. This support treats the vTOM bit as part of
the physical address. For accessing shared (decrypted) memory, these
patch sets create a second kernel virtual mapping that maps to physical
addresses above vTOM.

A better approach is to treat the vTOM bit as a protection flag, not
as part of the physical address. This new approach is like the approach
for the GPA.SHARED bit in Intel TDX. Rather than creating a second kernel
virtual mapping, the existing mapping is updated using recently added
coco mechanisms.  When memory is changed between private and shared using
set_memory_decrypted() and set_memory_encrypted(), the PTEs for the
existing kernel mapping are changed to add or remove the vTOM bit
in the guest physical address, just as with TDX. The hypercalls to
change the memory status on the host side are made using the existing
callback mechanism. Everything just works, with a minor tweak to map
the I/O APIC to use private accesses.

To accomplish the switch in approach, the following must be done in
in this single patch:

* Update Hyper-V initialization to set the cc_mask based on vTOM
  and do other coco initialization.

* Update physical_mask so the vTOM bit is no longer treated as part
  of the physical address

* Update cc_mkenc() and cc_mkdec() to be active for Hyper-V guests.
  This makes the vTOM bit part of the protection flags.

* Code already exists to make hypercalls to inform Hyper-V about pages
  changing between shared and private.  Update this code to run as a
  callback from __set_memory_enc_pgtable().

* Remove the Hyper-V special case from __set_memory_enc_dec()

* Remove the Hyper-V specific call to swiotlb_update_mem_attributes()
  since mem_encrypt_init() will now do it.

[1] https://lore.kernel.org/all/20211025122116.264793-1-ltykernel@gmail.com/
[2] https://lore.kernel.org/all/20211213071407.314309-1-ltykernel@gmail.com/

Signed-off-by: Michael Kelley <mikelley@microsoft.com>
---
 arch/x86/coco/core.c            | 11 +++++++++-
 arch/x86/hyperv/hv_init.c       | 11 ----------
 arch/x86/hyperv/ivm.c           | 45 +++++++++++++++++++++++++++++++----------
 arch/x86/include/asm/mshyperv.h |  8 ++------
 arch/x86/kernel/cpu/mshyperv.c  | 15 +++++++-------
 arch/x86/mm/pat/set_memory.c    |  3 ---
 6 files changed, 53 insertions(+), 40 deletions(-)
  

On 11/17/2022 2:41 AM, Michael Kelley wrote:
> Hyper-V guests on AMD SEV-SNP hardware have the option of using the
> "virtual Top Of Memory" (vTOM) feature specified by the SEV-SNP
> architecture. With vTOM, shared vs. private memory accesses are
> controlled by splitting the guest physical address space into two
> halves.  vTOM is the dividing line where the uppermost bit of the
> physical address space is set; e.g., with 47 bits of guest physical
> address space, vTOM is 0x40000000000 (bit 46 is set).  Guest phyiscal
> memory is accessible at two parallel physical addresses -- one below
> vTOM and one above vTOM.  Accesses below vTOM are private (encrypted)
> while accesses above vTOM are shared (decrypted). In this sense, vTOM
> is like the GPA.SHARED bit in Intel TDX.
> 
> Support for Hyper-V guests using vTOM was added to the Linux kernel in
> two patch sets[1][2]. This support treats the vTOM bit as part of
> the physical address. For accessing shared (decrypted) memory, these
> patch sets create a second kernel virtual mapping that maps to physical
> addresses above vTOM.
> 
> A better approach is to treat the vTOM bit as a protection flag, not
> as part of the physical address. This new approach is like the approach
> for the GPA.SHARED bit in Intel TDX. Rather than creating a second kernel
> virtual mapping, the existing mapping is updated using recently added
> coco mechanisms.  When memory is changed between private and shared using
> set_memory_decrypted() and set_memory_encrypted(), the PTEs for the
> existing kernel mapping are changed to add or remove the vTOM bit
> in the guest physical address, just as with TDX. The hypercalls to
> change the memory status on the host side are made using the existing
> callback mechanism. Everything just works, with a minor tweak to map
> the I/O APIC to use private accesses.
> 
> To accomplish the switch in approach, the following must be done in
> in this single patch:
> 
> * Update Hyper-V initialization to set the cc_mask based on vTOM
>    and do other coco initialization.
> 
> * Update physical_mask so the vTOM bit is no longer treated as part
>    of the physical address
> 
> * Update cc_mkenc() and cc_mkdec() to be active for Hyper-V guests.
>    This makes the vTOM bit part of the protection flags.
> 
> * Code already exists to make hypercalls to inform Hyper-V about pages
>    changing between shared and private.  Update this code to run as a
>    callback from __set_memory_enc_pgtable().
> 
> * Remove the Hyper-V special case from __set_memory_enc_dec()
> 
> * Remove the Hyper-V specific call to swiotlb_update_mem_attributes()
>    since mem_encrypt_init() will now do it.
> 
> [1]https://lore.kernel.org/all/20211025122116.264793-1-ltykernel@gmail.com/
> [2]https://lore.kernel.org/all/20211213071407.314309-1-ltykernel@gmail.com/
> 
> Signed-off-by: Michael Kelley<mikelley@microsoft.com>
> ---
>   arch/x86/coco/core.c            | 11 +++++++++-
>   arch/x86/hyperv/hv_init.c       | 11 ----------
>   arch/x86/hyperv/ivm.c           | 45 +++++++++++++++++++++++++++++++----------
>   arch/x86/include/asm/mshyperv.h |  8 ++------
>   arch/x86/kernel/cpu/mshyperv.c  | 15 +++++++-------
>   arch/x86/mm/pat/set_memory.c    |  3 ---
>   6 files changed, 53 insertions(+), 40 deletions(-)

Reviewed-by: Tianyu Lan <Tianyu.Lan@microsoft.com>
  

On Wed, Nov 16, 2022 at 10:41:30AM -0800, Michael Kelley wrote:
> Hyper-V guests on AMD SEV-SNP hardware have the option of using the
> "virtual Top Of Memory" (vTOM) feature specified by the SEV-SNP
> architecture. With vTOM, shared vs. private memory accesses are
> controlled by splitting the guest physical address space into two
> halves.  vTOM is the dividing line where the uppermost bit of the
> physical address space is set; e.g., with 47 bits of guest physical
> address space, vTOM is 0x40000000000 (bit 46 is set).  Guest phyiscal

Unknown word [phyiscal] in commit message.
Suggestions: ['physical', 'physically', ... ]

Please introduce a spellchecker into your patch creation workflow.

...

> @@ -108,6 +115,7 @@ u64 cc_mkenc(u64 val)
>  	switch (vendor) {
>  	case CC_VENDOR_AMD:
>  		return val | cc_mask;
> +	case CC_VENDOR_HYPERV:
>  	case CC_VENDOR_INTEL:
>  		return val & ~cc_mask;
>  	default:
> @@ -121,6 +129,7 @@ u64 cc_mkdec(u64 val)
>  	switch (vendor) {
>  	case CC_VENDOR_AMD:
>  		return val & ~cc_mask;
> +	case CC_VENDOR_HYPERV:
>  	case CC_VENDOR_INTEL:
>  		return val | cc_mask;
>  	default:

Uuuh, this needs a BIG FAT COMMENT.

You're running on SNP and yet the enc/dec meaning is flipped. And that's
because of vTOM.

What happens if you have other types of SNP-based VMs on HyperV? The
isolation VMs thing? Or is that the same?

What happens when you do TDX guests with HyperV?

This becomes wrong then.

I think you need a more finer-grained check here in the sense of "is it
a HyperV guest using a paravisor and vTOM is enabled" or so.

Otherwise, I like the removal of the HyperV-specific checks ofc.

Thx.
  

From: Borislav Petkov <bp@alien8.de> Sent: Monday, November 21, 2022 7:03 AM
> 
> On Wed, Nov 16, 2022 at 10:41:30AM -0800, Michael Kelley wrote:
> ...
> 
> > @@ -108,6 +115,7 @@ u64 cc_mkenc(u64 val)
> >  	switch (vendor) {
> >  	case CC_VENDOR_AMD:
> >  		return val | cc_mask;
> > +	case CC_VENDOR_HYPERV:
> >  	case CC_VENDOR_INTEL:
> >  		return val & ~cc_mask;
> >  	default:
> > @@ -121,6 +129,7 @@ u64 cc_mkdec(u64 val)
> >  	switch (vendor) {
> >  	case CC_VENDOR_AMD:
> >  		return val & ~cc_mask;
> > +	case CC_VENDOR_HYPERV:
> >  	case CC_VENDOR_INTEL:
> >  		return val | cc_mask;
> >  	default:
> 
> Uuuh, this needs a BIG FAT COMMENT.
> 
> You're running on SNP and yet the enc/dec meaning is flipped. And that's
> because of vTOM.
> 
> What happens if you have other types of SNP-based VMs on HyperV? The
> isolation VMs thing? Or is that the same?
> 
> What happens when you do TDX guests with HyperV?
> 
> This becomes wrong then.
> 
> I think you need a more finer-grained check here in the sense of "is it
> a HyperV guest using a paravisor and vTOM is enabled" or so.
> 
> Otherwise, I like the removal of the HyperV-specific checks ofc.
> 

I think the core problem here is the naming and meaning of
CC_VENDOR_HYPERV. The name was created originally when the
Hyper-V vTOM handling code was a lot of special cases.   With the
changes in this patch series that make the vTOM functionality more
mainstream, the name would be better as CC_VENDOR_AMD_VTOM.
vTOM is part of the AMD SEV-SNP spec, and it's a different way of
doing the encryption from the "C-bit" based approach.  As much as
possible, I'm trying to not make it be Hyper-V specific, though currently
we have N=1 for hypervisors that offer the vTOM option, so it's a little
hard to generalize.

With the thinking oriented that way, a Linux guest on Hyper-V using
TDX will run with CC_VENDOR_INTEL.  A Linux guest on Hyper-V that
is fully enlightened to use the "C-bit" will run with CC_VENDOR_AMD.

Dexuan Cui just posted a patch set for initial TDX support on Hyper-V,
and I think that runs with CC_VENDOR_INTEL (Dexuan -- correct me if
I'm wrong about that -- I haven't reviewed your patches yet).  Tianyu Lan
has a patch set out for Hyper-V guests using the "C-bit".  That patch set
still uses CC_VENDOR_HYPERV.  Tianyu and I need to work through
whether his patch set can run with CC_VENDOR_AMD like everyone
else using the "C-bit" approach.

Yes, the polarity of the AMD vTOM bit matches the polarity of the
TDX GPA.SHARED bit, and is the opposite polarity of the AMD "C-bit".
I'll add a comment to that effect.

Anyway, that's where I think this should go. Does it make sense?
Other thoughts?

Michael
  

On 11/22/22 10:22, Michael Kelley (LINUX) wrote:
> Anyway, that's where I think this should go. Does it make sense?
> Other thoughts?

I think hard-coding the C-bit behavior and/or position to a vendor was
probably a bad idea.  Even the comment:

u64 cc_mkenc(u64 val)
{
        /*
         * Both AMD and Intel use a bit in the page table to indicate
         * encryption status of the page.
         *
         * - for AMD, bit *set* means the page is encrypted
         * - for Intel *clear* means encrypted.
         */

doesn't make a lot of sense now.  Maybe we should just have a:

	CC_ATTR_ENC_SET

which gets set for the "AMD" behavior and is clear for the "Intel"
behavior.  Hyper-V code running on AMD can set the attribute to get teh
"Intel" behavior.

That sure beats having a Hyper-V vendor.
  

From: Dave Hansen <dave.hansen@intel.com> Sent: Tuesday, November 22, 2022 10:30 AM
> 
> On 11/22/22 10:22, Michael Kelley (LINUX) wrote:
> > Anyway, that's where I think this should go. Does it make sense?
> > Other thoughts?
> 
> I think hard-coding the C-bit behavior and/or position to a vendor was
> probably a bad idea.  Even the comment:
> 
> u64 cc_mkenc(u64 val)
> {
>         /*
>          * Both AMD and Intel use a bit in the page table to indicate
>          * encryption status of the page.
>          *
>          * - for AMD, bit *set* means the page is encrypted
>          * - for Intel *clear* means encrypted.
>          */
> 
> doesn't make a lot of sense now.  Maybe we should just have a:
> 
> 	CC_ATTR_ENC_SET
> 
> which gets set for the "AMD" behavior and is clear for the "Intel"
> behavior.  Hyper-V code running on AMD can set the attribute to get teh
> "Intel" behavior.
> 
> That sure beats having a Hyper-V vendor.

To me, figuring out the encryption bit polarity and position isn't
the hard part to set up.  We've got three technologies: TDX,
AMD "C-bit", and arguably, AMD vTOM.  Future silicon and architectural
enhancements will most likely be variations and improvements on
these rather than something completely new (not that I'm necessarily
aware of what the processor vendors may have planned).   The AMD
"C-bit" technology already has sub-cases (SME, SEV, SEV-ES, SEV-SNP)
because of the architectural history.  Any of the technologies may
get additional subcases over time.  Whether those subcases are
represented as new CC_VENDOR_* options, or CC_ATTR_* options
on an existing CC_VENDOR_* should be driven by the level of
commonality with what already exists.  There's no hard-and-fast
rule.  Just do whatever makes the most sense.

I'm proposing AMD vTOM as a separate CC_VENDOR_AMD_VTOM
because it is rather different from CC_VENDOR_AMD, and not just
in the polarity and position of the encryption bit.  But if we really
wanted to just make it a sub-case of CC_VENDOR_AMD, I could
probably be convinced.  The key is cleanly handling all the other
attributes like CC_ATTR_GUEST_STATE_ENCRYPT,
CC_ATTR_ACCESS_IOAPIC_ENCRYPTED (that I add in this patch set),
CC_ATTR_GUEST_UNROLL_STRING_IO, etc. where we want to avoid
too many hacky special cases.  The current code structure where the
CC_VENDOR_* selection determines the CC_ATTR_* values seems
to work OK.

Anyway, that's my thinking.  CC_VENDOR_HYPERV goes away, but
the behavior is still essentially the same when replaced by
CC_VENDOR_AMD_VTOM.

Michael
  

On Tue, Nov 22, 2022 at 06:22:46PM +0000, Michael Kelley (LINUX) wrote:
> I think the core problem here is the naming and meaning of
> CC_VENDOR_HYPERV. The name was created originally when the
> Hyper-V vTOM handling code was a lot of special cases.   With the
> changes in this patch series that make the vTOM functionality more
> mainstream, the name would be better as CC_VENDOR_AMD_VTOM.

No, if CC_VENDOR_HYPERV means different things depending on what kind of
guests you're doing, then you should not use a CC_VENDOR at all.

> vTOM is part of the AMD SEV-SNP spec, and it's a different way of
> doing the encryption from the "C-bit" based approach. As much as
> possible, I'm trying to not make it be Hyper-V specific, though
> currently we have N=1 for hypervisors that offer the vTOM option, so
> it's a little hard to generalize.

Actually, it is very simple to generalize: vTOM and the paravisor and
VMPL are all part of the effort to support unenlightened, unmodified
guests with SNP.

So, if KVM wants to run Windows NT 4.0 guests as SNP guests, then it
probably would need the same contraptions.

> With the thinking oriented that way, a Linux guest on Hyper-V using
> TDX will run with CC_VENDOR_INTEL.  A Linux guest on Hyper-V that
> is fully enlightened to use the "C-bit" will run with CC_VENDOR_AMD.

Right.

> Dexuan Cui just posted a patch set for initial TDX support on Hyper-V,
> and I think that runs with CC_VENDOR_INTEL (Dexuan -- correct me if
> I'm wrong about that -- I haven't reviewed your patches yet).  Tianyu Lan
> has a patch set out for Hyper-V guests using the "C-bit".  That patch set
> still uses CC_VENDOR_HYPERV.  Tianyu and I need to work through
> whether his patch set can run with CC_VENDOR_AMD like everyone
> else using the "C-bit" approach.

So I'm not sure the vendor is the right approach here. I guess we need
to specify the *type* of guest being supported.

> Yes, the polarity of the AMD vTOM bit matches the polarity of the
> TDX GPA.SHARED bit, and is the opposite polarity of the AMD "C-bit".
> I'll add a comment to that effect.
> 
> Anyway, that's where I think this should go. Does it make sense?
> Other thoughts?

I think all that polarity doesn't matter as long as we abstract it away
with, "mark encrypted" and "mark decrypted".

But it is late now and I could be wrong - I'll look at the rest
tomorrow-ish.

Thx.
  

From: Borislav Petkov <bp@alien8.de> Sent: Tuesday, November 22, 2022 2:18 PM
> 
> On Tue, Nov 22, 2022 at 06:22:46PM +0000, Michael Kelley (LINUX) wrote:
> > I think the core problem here is the naming and meaning of
> > CC_VENDOR_HYPERV. The name was created originally when the
> > Hyper-V vTOM handling code was a lot of special cases.   With the
> > changes in this patch series that make the vTOM functionality more
> > mainstream, the name would be better as CC_VENDOR_AMD_VTOM.
> 
> No, if CC_VENDOR_HYPERV means different things depending on what kind of
> guests you're doing, then you should not use a CC_VENDOR at all.

Agreed.   My proposal is to drop CC_VENDOR_HYPERV entirely.
Replace it with CC_VENDOR_AMD_VTOM (or something like that) that
is set *only* by Linux guests that are running on AMD SEV-SNP processors
and using the vTOM scheme instead of the AMD C-bit scheme.

> 
> > vTOM is part of the AMD SEV-SNP spec, and it's a different way of
> > doing the encryption from the "C-bit" based approach. As much as
> > possible, I'm trying to not make it be Hyper-V specific, though
> > currently we have N=1 for hypervisors that offer the vTOM option, so
> > it's a little hard to generalize.
> 
> Actually, it is very simple to generalize: vTOM and the paravisor and
> VMPL are all part of the effort to support unenlightened, unmodified
> guests with SNP.
> 
> So, if KVM wants to run Windows NT 4.0 guests as SNP guests, then it
> probably would need the same contraptions.

Yes, agreed.  My point about generalization is that Hyper-V is the only
actual implementation today.  Edge cases, like whether the IO-APIC is
accessed as encrypted or as decrypted don't have a pattern yet.  But
that's not a blocker.  Such cases can be resolved or special-cased later
when/if N > 1.

> 
> > With the thinking oriented that way, a Linux guest on Hyper-V using
> > TDX will run with CC_VENDOR_INTEL.  A Linux guest on Hyper-V that
> > is fully enlightened to use the "C-bit" will run with CC_VENDOR_AMD.
> 
> Right.

Good. We're in agreement.  :-)

> 
> > Dexuan Cui just posted a patch set for initial TDX support on Hyper-V,
> > and I think that runs with CC_VENDOR_INTEL (Dexuan -- correct me if
> > I'm wrong about that -- I haven't reviewed your patches yet).

I confirmed with Dexuan that his new patch set for TDX guests on Hyper-V
has the guest running with CC_VENDOR_INTEL, which is what we want.

> > Tianyu Lan
> > has a patch set out for Hyper-V guests using the "C-bit".  That patch set
> > still uses CC_VENDOR_HYPERV.  Tianyu and I need to work through
> > whether his patch set can run with CC_VENDOR_AMD like everyone
> > else using the "C-bit" approach.

I haven't followed up with Tianyu yet.

> 
> So I'm not sure the vendor is the right approach here. I guess we need
> to specify the *type* of guest being supported.

Yes, calling it the "vendor" turns out to not quite be right because in
the AMD case, the technology/architecture/scheme/"type" (or
whatever you want to call it) is not 1:1 with the vendor.   Intel has just
one (TDX) while AMD has two (C-bit and vTOM).   "vendor" is just a label,
but we should get the label right to avoid future confusion.  The key point
is that we'll have three top-level types:

* TDX
* AMD with C-bit  (and this has some sub-types)
* AMD with vTOM

The CC_ATTR_* values are then derived from the "type".

> 
> > Yes, the polarity of the AMD vTOM bit matches the polarity of the
> > TDX GPA.SHARED bit, and is the opposite polarity of the AMD "C-bit".
> > I'll add a comment to that effect.
> >
> > Anyway, that's where I think this should go. Does it make sense?
> > Other thoughts?
> 
> I think all that polarity doesn't matter as long as we abstract it away
> with, "mark encrypted" and "mark decrypted".

Agreed.

Michael
  

From: Michael Kelley (LINUX) <mikelley@microsoft.com> Sent: Tuesday, November 22, 2022 4:59 PM
> 
> From: Borislav Petkov <bp@alien8.de> Sent: Tuesday, November 22, 2022 2:18 PM
> >
> > On Tue, Nov 22, 2022 at 06:22:46PM +0000, Michael Kelley (LINUX) wrote:
> > > I think the core problem here is the naming and meaning of
> > > CC_VENDOR_HYPERV. The name was created originally when the
> > > Hyper-V vTOM handling code was a lot of special cases.   With the
> > > changes in this patch series that make the vTOM functionality more
> > > mainstream, the name would be better as CC_VENDOR_AMD_VTOM.
> >
> > No, if CC_VENDOR_HYPERV means different things depending on what kind of
> > guests you're doing, then you should not use a CC_VENDOR at all.
> 
> Agreed.   My proposal is to drop CC_VENDOR_HYPERV entirely.
> Replace it with CC_VENDOR_AMD_VTOM (or something like that) that
> is set *only* by Linux guests that are running on AMD SEV-SNP processors
> and using the vTOM scheme instead of the AMD C-bit scheme.
> 
> >
> > > vTOM is part of the AMD SEV-SNP spec, and it's a different way of
> > > doing the encryption from the "C-bit" based approach. As much as
> > > possible, I'm trying to not make it be Hyper-V specific, though
> > > currently we have N=1 for hypervisors that offer the vTOM option, so
> > > it's a little hard to generalize.
> >
> > Actually, it is very simple to generalize: vTOM and the paravisor and
> > VMPL are all part of the effort to support unenlightened, unmodified
> > guests with SNP.
> >
> > So, if KVM wants to run Windows NT 4.0 guests as SNP guests, then it
> > probably would need the same contraptions.
> 
> Yes, agreed.  My point about generalization is that Hyper-V is the only
> actual implementation today.  Edge cases, like whether the IO-APIC is
> accessed as encrypted or as decrypted don't have a pattern yet.  But
> that's not a blocker.  Such cases can be resolved or special-cased later
> when/if N > 1.
> 
> >
> > > With the thinking oriented that way, a Linux guest on Hyper-V using
> > > TDX will run with CC_VENDOR_INTEL.  A Linux guest on Hyper-V that
> > > is fully enlightened to use the "C-bit" will run with CC_VENDOR_AMD.
> >
> > Right.
> 
> Good. We're in agreement.  :-)
> 
> >
> > > Dexuan Cui just posted a patch set for initial TDX support on Hyper-V,
> > > and I think that runs with CC_VENDOR_INTEL (Dexuan -- correct me if
> > > I'm wrong about that -- I haven't reviewed your patches yet).
> 
> I confirmed with Dexuan that his new patch set for TDX guests on Hyper-V
> has the guest running with CC_VENDOR_INTEL, which is what we want.
> 
> > > Tianyu Lan
> > > has a patch set out for Hyper-V guests using the "C-bit".  That patch set
> > > still uses CC_VENDOR_HYPERV.  Tianyu and I need to work through
> > > whether his patch set can run with CC_VENDOR_AMD like everyone
> > > else using the "C-bit" approach.
> 
> I haven't followed up with Tianyu yet.
> 
> >
> > So I'm not sure the vendor is the right approach here. I guess we need
> > to specify the *type* of guest being supported.
> 
> Yes, calling it the "vendor" turns out to not quite be right because in
> the AMD case, the technology/architecture/scheme/"type" (or
> whatever you want to call it) is not 1:1 with the vendor.   Intel has just
> one (TDX) while AMD has two (C-bit and vTOM).   "vendor" is just a label,
> but we should get the label right to avoid future confusion.  The key point
> is that we'll have three top-level types:
> 
> * TDX
> * AMD with C-bit  (and this has some sub-types)
> * AMD with vTOM
> 
> The CC_ATTR_* values are then derived from the "type".
> 
> >
> > > Yes, the polarity of the AMD vTOM bit matches the polarity of the
> > > TDX GPA.SHARED bit, and is the opposite polarity of the AMD "C-bit".
> > > I'll add a comment to that effect.
> > >
> > > Anyway, that's where I think this should go. Does it make sense?
> > > Other thoughts?
> >
> > I think all that polarity doesn't matter as long as we abstract it away
> > with, "mark encrypted" and "mark decrypted".
> 
> Agreed.
>

Boris --

Any further comment on this patch?  I think we're agreement.  For
this patch series I propose to change the symbol "CC_VENDOR_HYPERV"
to "CC_VENDOR_AMD_VTOM" and the function name
hyperv_cc_platform_has() to amd_vtom_cc_platform_has().  There's
no functionality change vs. the current version of this patch.  The naming
changes just make clear that the scope is only for the AMD vTOM encryption
scheme.

As discussed, the concept "vendor" isn't quite right either, but let's not
add that change to this patch series.  I can follow with a separate patch
to change symbols with "vendor" or "VENDOR" to something else, which
will probably require some discussion as to what that something else
should be.

Michael
  

On Mon, Nov 28, 2022 at 02:38:11PM +0000, Michael Kelley (LINUX) wrote:
> Any further comment on this patch?  I think we're agreement.  For
> this patch series I propose to change the symbol "CC_VENDOR_HYPERV"
> to "CC_VENDOR_AMD_VTOM" and the function name
> hyperv_cc_platform_has() to amd_vtom_cc_platform_has().

That doesn't sound optimal to me.

So, let's clarify things first: those Isolation VMs - are they going to
be the paravisors?

I don't see any other option because the unmodified guest must be some
old windoze....

So, if they're going to be that, then I guess this should be called

	CC_ATTR_PARAVISOR

to denote that it is a thin layer of virt gunk between an unmodified
guest and a hypervisor.

And if TDX wants to do that too later, then they can use that flag too.

Yes, no?
  

From: Borislav Petkov <bp@alien8.de> Sent: Monday, November 28, 2022 8:34 AM
> 
> On Mon, Nov 28, 2022 at 02:38:11PM +0000, Michael Kelley (LINUX) wrote:
> > Any further comment on this patch?  I think we're agreement.  For
> > this patch series I propose to change the symbol "CC_VENDOR_HYPERV"
> > to "CC_VENDOR_AMD_VTOM" and the function name
> > hyperv_cc_platform_has() to amd_vtom_cc_platform_has().
> 
> That doesn't sound optimal to me.
> 
> So, let's clarify things first: those Isolation VMs - are they going to
> be the paravisors?

No.  The paravisor exists in another layer (VMPL 0 in the AMD vTOM
architecture) that isn't visible to the Linux guest.  The paravisor is
a separate code base that isn't currently open source.  All code
in this patch series is code that runs in the Linux guest.

From an encryption standpoint, the Linux guest sees the following:

1) Guest memory is encrypted by default
2) The Linux guest must set the vTOM flag in a PTE to access a page
as unencrypted.
3) The Linux guest must explicitly notify the hypervisor to change a
page from private (encrypted) to shared (decrypted), and vice versa.

> 
> I don't see any other option because the unmodified guest must be some
> old windoze....

What Windows guests do isn't really relevant.  Again, the code in this patch
series all runs directly in the Linux guest, not the paravisor.  And the Linux
guest isn't unmodified.  We've added changes to understand vTOM and
the need to communicate with the hypervisor about page changes
between private and shared.  But there are other changes for a fully
enlightened guest that we don't have to make when using AMD vTOM,
because the paravisor transparently (to the guest -- Linux or Windows)
handles those issues.

> 
> So, if they're going to be that, then I guess this should be called
> 
> 	CC_ATTR_PARAVISOR
> 
> to denote that it is a thin layer of virt gunk between an unmodified
> guest and a hypervisor.

No, the code is this patch series is not that at all.  It's not code that is
part of the paravisor.  It's Linux guest code that understands it is running
in an environment where AMD vTOM is the encryption scheme, which is
different from the AMD C-bit encryption scheme.

> 
> And if TDX wants to do that too later, then they can use that flag too.
> 

Again, no.  What I have proposed as CC_VENDOR_AMD_VTOM is
specific to AMD's virtual-Top-of-Memory architecture.  The TDX
architecture doesn't really have a way to use a paravisor.

To summarize, the code in this patch series is about a 3rd encryption
scheme that is used by the guest.  It is completely parallel to the AMD
C-bit encryption scheme and the Intel TDX encryption scheme.   With
the AMD vTOM scheme, there is a paravisor that transparently emulates
some things for the guest so there are fewer code changes needed in the
guest, but this patch series is not about that paravisor code.

Michael
  

On Mon, Nov 28, 2022 at 04:59:27PM +0000, Michael Kelley (LINUX) wrote:
> 2) The Linux guest must set the vTOM flag in a PTE to access a page
> as unencrypted.

What exactly do you call the "vTOM flag in the PTE"?

I see this here:

"When bit 1 (vTOM) of SEV_FEATURES is set in the VMSA of an SNP-active
VM, the VIRTUAL_TOM field is used to determine the C-bit for data
accesses instead of the guest page table contents. All data accesses
below VIRTUAL_TOM are accessed with an effective C-bit of 1 and all
addresses at or above VIRTUAL_TOM are accessed with an effective C-bit
of 0."

Now you say

"vTOM is the dividing line where the uppermost bit of the physical
address space is set; e.g., with 47 bits of guest physical address
space, vTOM is 0x40000000000 (bit 46 is set)."

So on your guests, is VIRTUAL_TOM == 0x400000000000?

Btw, that 0x4000.. does not have bit 46 set but bit 42. Bit 46 set is

	0x400000000000

which means one more '0' at the end.

So before we discuss this further, let's agree on the basics first.

> What Windows guests do isn't really relevant.  Again, the code in this patch
> series all runs directly in the Linux guest, not the paravisor.  And the Linux
> guest isn't unmodified.  We've added changes to understand vTOM and
> the need to communicate with the hypervisor about page changes
> between private and shared.  But there are other changes for a fully
> enlightened guest that we don't have to make when using AMD vTOM,
> because the paravisor transparently (to the guest -- Linux or Windows)
> handles those issues.

So this is some other type of guest you wanna run.

Where is the documentation of that thing?

I'd like to know what exactly it is going to use in the kernel.

> Again, no.  What I have proposed as CC_VENDOR_AMD_VTOM is

There's no vendor AMD_VTOM!

We did the vendor thing to denote Intel or AMD wrt to confidential
computing.

Now you're coming up with something special. It can't be HYPERV because
Hyper-V does other types of confidential solutions too, apparently.

Now before this goes any further I'd like for "something special" to be
defined properly and not just be a one-off Hyper-V thing.

> specific to AMD's virtual-Top-of-Memory architecture.  The TDX
> architecture doesn't really have a way to use a paravisor.
> 
> To summarize, the code in this patch series is about a 3rd encryption
> scheme that is used by the guest.

Yes, can that third thing be used by other hypervisors or is this
Hyper-V special?

> It is completely parallel to the AMD C-bit encryption scheme and
> the Intel TDX encryption scheme. With the AMD vTOM scheme, there is
> a paravisor that transparently emulates some things for the guest
> so there are fewer code changes needed in the guest, but this patch
> series is not about that paravisor code.

Would other hypervisors want to support such a scheme?

Is this architecture documented somewhere? If so, where?

What would it mean for the kernel to support it.

And so on and so on.

Thanks.
  

From: Borislav Petkov <bp@alien8.de> Sent: Monday, November 28, 2022 9:25 AM
> 
> On Mon, Nov 28, 2022 at 04:59:27PM +0000, Michael Kelley (LINUX) wrote:
> > 2) The Linux guest must set the vTOM flag in a PTE to access a page
> > as unencrypted.
> 
> What exactly do you call the "vTOM flag in the PTE"?
> 
> I see this here:
> 
> "When bit 1 (vTOM) of SEV_FEATURES is set in the VMSA of an SNP-active
> VM, the VIRTUAL_TOM field is used to determine the C-bit for data
> accesses instead of the guest page table contents. All data accesses
> below VIRTUAL_TOM are accessed with an effective C-bit of 1 and all
> addresses at or above VIRTUAL_TOM are accessed with an effective C-bit
> of 0."
> 
> Now you say
> 
> "vTOM is the dividing line where the uppermost bit of the physical
> address space is set; e.g., with 47 bits of guest physical address
> space, vTOM is 0x40000000000 (bit 46 is set)."
> 
> So on your guests, is VIRTUAL_TOM == 0x400000000000?
> 
> Btw, that 0x4000.. does not have bit 46 set but bit 42. Bit 46 set is
> 
> 	0x400000000000
> 
> which means one more '0' at the end.

Yeah, looks like I dropped a '0' in my comment text.  Will fix.

> 
> So before we discuss this further, let's agree on the basics first.
> 
> > What Windows guests do isn't really relevant.  Again, the code in this patch
> > series all runs directly in the Linux guest, not the paravisor.  And the Linux
> > guest isn't unmodified.  We've added changes to understand vTOM and
> > the need to communicate with the hypervisor about page changes
> > between private and shared.  But there are other changes for a fully
> > enlightened guest that we don't have to make when using AMD vTOM,
> > because the paravisor transparently (to the guest -- Linux or Windows)
> > handles those issues.
> 
> So this is some other type of guest you wanna run.
> 
> Where is the documentation of that thing?
> 
> I'd like to know what exactly it is going to use in the kernel.

Standard Linux is the guest.  It's fully functional for running general
Purpose workloads that want "confidential computing" where guest
memory is encrypted and the data in the guest is not visible to the
host hypervisor.  It's a standard Linux kernel.  I'm not sure what you
mean by "some other type of guest".

> 
> > Again, no.  What I have proposed as CC_VENDOR_AMD_VTOM is
> 
> There's no vendor AMD_VTOM!
> 
> We did the vendor thing to denote Intel or AMD wrt to confidential
> computing.

But vendor AMD effectively offers two different encryption schemes that
could be seen by the guest VM.  The hypervisor chooses which scheme a
particular guest will see.  Hyper-V has chosen to present the vTOM scheme
to guest VMs, including normal Linux and Windows guests, that have been
modestly updated to understand vTOM.

> 
> Now you're coming up with something special. It can't be HYPERV because
> Hyper-V does other types of confidential solutions too, apparently.

In the future, Hyper-V may also choose to present original AMD C-bit scheme
in some guest VMs, depending on the use case.  And it will present the Intel
TDX scheme when running on that hardware.

> 
> Now before this goes any further I'd like for "something special" to be
> defined properly and not just be a one-off Hyper-V thing.
> 
> > specific to AMD's virtual-Top-of-Memory architecture.  The TDX
> > architecture doesn't really have a way to use a paravisor.
> >
> > To summarize, the code in this patch series is about a 3rd encryption
> > scheme that is used by the guest.
> 
> Yes, can that third thing be used by other hypervisors or is this
> Hyper-V special?

This third thing is part of the AMD SEV-SNP architecture and could be used
by any hypervisor.

> 
> > It is completely parallel to the AMD C-bit encryption scheme and
> > the Intel TDX encryption scheme. With the AMD vTOM scheme, there is
> > a paravisor that transparently emulates some things for the guest
> > so there are fewer code changes needed in the guest, but this patch
> > series is not about that paravisor code.
> 
> Would other hypervisors want to support such a scheme?
> 
> Is this architecture documented somewhere? If so, where?

The AMD vTOM scheme is documented in the AMD SEV-SNP
architecture specs.

> 
> What would it mean for the kernel to support it.

The Linux kernel running as a guest already supports it, at least when
running on Hyper-V.   The code went into the 5.15 kernel [1][2] about
a year ago.  But that code is more Hyper-V specific than is desirable. 
This patch set makes the AMD vTOM support more parallel to the Intel
TDX and AMD C-bit support.

To my knowledge, KVM does not support the AMD vTOM scheme.
Someone from AMD may have a better sense whether adding that
support is likely in the future.

[1] https://lore.kernel.org/all/20211025122116.264793-1-ltykernel@gmail.com/
[2] https://lore.kernel.org/all/20211213071407.314309-1-ltykernel@gmail.com/

Michael
  

On Mon, Nov 28, 2022 at 05:55:11PM +0000, Michael Kelley (LINUX) wrote:
> But vendor AMD effectively offers two different encryption schemes that
> could be seen by the guest VM.  The hypervisor chooses which scheme a
> particular guest will see.  Hyper-V has chosen to present the vTOM scheme
> to guest VMs, including normal Linux and Windows guests, that have been
> modestly updated to understand vTOM.

If this is a standard SNP guest then you can detect vTOM support using
SEV_FEATURES. See this thread here:

https://lore.kernel.org/r/20221117044433.244656-1-nikunj@amd.com

Which then means, you don't need any special gunk except extending this
patch above to check SNP has vTOM support.

> In the future, Hyper-V may also choose to present original AMD C-bit scheme
> in some guest VMs, depending on the use case.  And it will present the Intel
> TDX scheme when running on that hardware.

And all those should JustWork(tm) because we already support such guests.

> To my knowledge, KVM does not support the AMD vTOM scheme.
> Someone from AMD may have a better sense whether adding that
> support is likely in the future.

Yah, see above.

Thx.
  

From: Borislav Petkov <bp@alien8.de> Sent: Monday, November 28, 2022 11:57 AM
> 
> On Mon, Nov 28, 2022 at 05:55:11PM +0000, Michael Kelley (LINUX) wrote:
> > But vendor AMD effectively offers two different encryption schemes that
> > could be seen by the guest VM.  The hypervisor chooses which scheme a
> > particular guest will see.  Hyper-V has chosen to present the vTOM scheme
> > to guest VMs, including normal Linux and Windows guests, that have been
> > modestly updated to understand vTOM.
> 
> If this is a standard SNP guest then you can detect vTOM support using
> SEV_FEATURES. See this thread here:
> 
> https://lore.kernel.org/all/20221117044433.244656-1-nikunj@amd.com/
> 
> Which then means, you don't need any special gunk except extending this
> patch above to check SNP has vTOM support.

Yes, we could do that.  But this patch set is still needed to get the preliminaries
in place.  The initial code for supporting AMD vTOM that went upstream a
year ago is too Hyper-V specific.  This patch set removes the Hyper-V specific
stuff, and integrates vTOM support into the overall confidential computing
framework in arch/x86/coco/core.c.  The Hyper-V code was already somewhat
there via CC_VENDOR_HYPERV, but that really should be named something
else now.  So that's why I'm suggesting CC_VENDOR_AMD_VTOM.
 
> 
> > In the future, Hyper-V may also choose to present original AMD C-bit scheme
> > in some guest VMs, depending on the use case.  And it will present the Intel
> > TDX scheme when running on that hardware.
> 
> And all those should JustWork(tm) because we already support such guests.

Agreed.  That's where we want to be.

Of course, when you go from N=1 hypervisors (i.e., KVM) to N=2 (KVM
and Hyper-V, you find some places where incorrect assumptions were made
or some generalizations are needed.  Dexuan Cui's patch set for TDX support
is fixing those places that he has encountered.  But with those fixes, the TDX
support will JustWork(tm) for Linux guests on Hyper-V.

I haven't gone deeply into the situation with AMD C-bit support on Hyper-V.
Tianyu Lan's set of patches for that support is a bit bigger, and I'm
planning to look closely to understand whether it's also just fixing incorrect
assumptions and such, or whether they really are some differences with
Hyper-V.  If there are differences, I want to understand why.

Michael
  

On Tue, Nov 29, 2022 at 01:15:39AM +0000, Michael Kelley (LINUX) wrote:
> So that's why I'm suggesting CC_VENDOR_AMD_VTOM.

There's no CC_VENDOR_AMD_VTOM. How many times do I need to explain this?!
CC_VENDOR is well the CC vendor - not some special case.

IOW, the goal here is for generic SNP functionality to be the same for
*all* SNP guests. We won't do the AMD's version of vTOM enablement,
Hyper-V's version of vTOM enablement and so on. It must be a single
vTOM feature which works on *all* hypervisors as vTOM is a generic SNP
feature - not Hyper-V feature.

> Of course, when you go from N=1 hypervisors (i.e., KVM) to N=2 (KVM
> and Hyper-V, you find some places where incorrect assumptions were
> made or some generalizations are needed. Dexuan Cui's patch set for
> TDX support is fixing those places that he has encountered. But with
> those fixes, the TDX support will JustWork(tm) for Linux guests on
> Hyper-V.

That sounds like the right direction to take.

> I haven't gone deeply into the situation with AMD C-bit support on
> Hyper-V. Tianyu Lan's set of patches for that support is a bit bigger,
> and I'm planning to look closely to understand whether it's also just
> fixing incorrect assumptions and such, or whether they really are
> some differences with Hyper-V. If there are differences, I want to
> understand why.

You and me too. So I guess we should look at Tianyu's set first.

Thx.
  

From: Borislav Petkov <bp@alien8.de> Sent: Tuesday, November 29, 2022 12:41 AM
> 
> On Tue, Nov 29, 2022 at 01:15:39AM +0000, Michael Kelley (LINUX) wrote:
> > So that's why I'm suggesting CC_VENDOR_AMD_VTOM.
> 
> There's no CC_VENDOR_AMD_VTOM. How many times do I need to explain this?!
> CC_VENDOR is well the CC vendor - not some special case.

Let's back up half a step.  We have CC_VENDOR_INTEL and
CC_VENDOR_AMD because that's a convenient way to identify
two fairly different schemes for doing guest encryption.  The
schemes have some things in common, but the details are pretty
different.  Tagging the schemes based on the vendor makes sense
because the schemes were independently developed by each
processor vendor.

But it turns out that AMD really has two fairly different schemes:
the C-bit scheme and the vTOM scheme.  The details of these
two AMD schemes are pretty different.  vTOM is *not* just a minor
option on the C-bit scheme.  It's an either/or -- a guest VM is either
doing the C-bit scheme or the vTOM scheme, not some combination.
Linux code in coco/core.c could choose to treat C-bit and vTOM as
two sub-schemes under CC_VENDOR_AMD, but that makes the
code a bit messy because we end up with "if" statements to figure
out whether to do things the C-bit way or the vTOM way.  The
difference in the C-bit way and the vTOM way is not Hyper-V
specific.  Any hypervisor running on an AMD processor can make
the same choice to offer C-bit VMs or vTOM VMs.

Or we could model the two AMD schemes as two different vendors,
which is what I'm suggesting.  Doing so recognizes that the two schemes
are fairly disjoint, and it makes the code cleaner.

Tom Lendacky -- any thoughts on this question from AMD's
standpoint?   

> 
> IOW, the goal here is for generic SNP functionality to be the same for
> *all* SNP guests. We won't do the AMD's version of vTOM enablement,
> Hyper-V's version of vTOM enablement and so on. It must be a single
> vTOM feature which works on *all* hypervisors as vTOM is a generic SNP
> feature - not Hyper-V feature.

Yes, there's only one version of vTOM enablement -- the AMD version.

But the broader AMD SNP functionality is bifurcated:  there's the
C-bit scheme and the vTOM scheme.  The question is how Linux code
should model those two different schemes.  I'm suggesting that things
are cleaner conceptually and in the code if we model the two different
AMD schemes as two different vendors.

Michael

> 
> > Of course, when you go from N=1 hypervisors (i.e., KVM) to N=2 (KVM
> > and Hyper-V, you find some places where incorrect assumptions were
> > made or some generalizations are needed. Dexuan Cui's patch set for
> > TDX support is fixing those places that he has encountered. But with
> > those fixes, the TDX support will JustWork(tm) for Linux guests on
> > Hyper-V.
> 
> That sounds like the right direction to take.
> 
> > I haven't gone deeply into the situation with AMD C-bit support on
> > Hyper-V. Tianyu Lan's set of patches for that support is a bit bigger,
> > and I'm planning to look closely to understand whether it's also just
> > fixing incorrect assumptions and such, or whether they really are
> > some differences with Hyper-V. If there are differences, I want to
> > understand why.
> 
> You and me too. So I guess we should look at Tianyu's set first.
>
  

On Tue, Nov 29, 2022 at 03:49:06PM +0000, Michael Kelley (LINUX) wrote:
> But it turns out that AMD really has two fairly different schemes:
> the C-bit scheme and the vTOM scheme.

Except it doesn't:

"In the VMSA of an SNP-active guest, the VIRTUAL_TOM field designates
a 2MB aligned guest physical address called the virtual top of memory.
When bit 1 (vTOM) of SEV_FEATURES is set in the VMSA of an SNP-active
VM, the VIRTUAL_TOM..."

So SEV_FEATURES[1] is vTOM and it is part of SNP.

Why do you keep harping on this being something else is beyond me...

I already pointed you to the patch which adds this along with the other
SEV_FEATURES.

> The details of these two AMD schemes are pretty different. vTOM is
> *not* just a minor option on the C-bit scheme. It's an either/or -- a
> guest VM is either doing the C-bit scheme or the vTOM scheme, not some
> combination. Linux code in coco/core.c could choose to treat C-bit and
> vTOM as two sub-schemes under CC_VENDOR_AMD, but that makes the code a
> bit messy because we end up with "if" statements to figure out whether
> to do things the C-bit way or the vTOM way.

Are you saying that that:

	if (cc_vendor == CC_VENDOR_AMD &&
	    sev_features & MSR_AMD64_SNP_VTOM_ENABLED)

is messy? Why?

We will have to support vTOM sooner or later.

> Or we could model the two AMD schemes as two different vendors,
> which is what I'm suggesting.  Doing so recognizes that the two schemes
> are fairly disjoint, and it makes the code cleaner.

How is that any different from the above check?

You *need* some sort of a check to differentiate between the two anyway.
  

From: Borislav Petkov <bp@alien8.de> Sent: Tuesday, November 29, 2022 9:47 AM
> 
> On Tue, Nov 29, 2022 at 03:49:06PM +0000, Michael Kelley (LINUX) wrote:
> > But it turns out that AMD really has two fairly different schemes:
> > the C-bit scheme and the vTOM scheme.
> 
> Except it doesn't:
> 
> "In the VMSA of an SNP-active guest, the VIRTUAL_TOM field designates
> a 2MB aligned guest physical address called the virtual top of memory.
> When bit 1 (vTOM) of SEV_FEATURES is set in the VMSA of an SNP-active
> VM, the VIRTUAL_TOM..."
> 
> So SEV_FEATURES[1] is vTOM and it is part of SNP.
> 
> Why do you keep harping on this being something else is beyond me...
> 
> I already pointed you to the patch which adds this along with the other
> SEV_FEATURES.
> 
> > The details of these two AMD schemes are pretty different. vTOM is
> > *not* just a minor option on the C-bit scheme. It's an either/or -- a
> > guest VM is either doing the C-bit scheme or the vTOM scheme, not some
> > combination. Linux code in coco/core.c could choose to treat C-bit and
> > vTOM as two sub-schemes under CC_VENDOR_AMD, but that makes the code a
> > bit messy because we end up with "if" statements to figure out whether
> > to do things the C-bit way or the vTOM way.
> 
> Are you saying that that:
> 
> 	if (cc_vendor == CC_VENDOR_AMD &&
> 	    sev_features & MSR_AMD64_SNP_VTOM_ENABLED)
> 
> is messy? Why?
> 
> We will have to support vTOM sooner or later.
> 
> > Or we could model the two AMD schemes as two different vendors,
> > which is what I'm suggesting.  Doing so recognizes that the two schemes
> > are fairly disjoint, and it makes the code cleaner.
> 
> How is that any different from the above check?
> 
> You *need* some sort of a check to differentiate between the two anyway.
> 

Alright.  Enough conceptual debate.  I'll do a v4 of the patch series with
the AMD C-bit and vTOM schemes folder under CC_VENDOR_AMD and
we can see if there's any further feedback.  I should have that v4 out later
today or tomorrow.

Michael