RE: [PATCH v5 06/14] x86/ioremap: Support hypervisor specified range to map as encrypted

[Michael Kelley (LINUX)]

From: Borislav Petkov <bp@xxxxxxxxx> Sent: Wednesday, January 25, 2023 6:56 AM
> 
> On Sat, Jan 21, 2023 at 04:10:23AM +0000, Michael Kelley (LINUX) wrote:

[snip]

> 
> > But in any case, the whole point of cc_platform_has() is to provide a level of
> > abstraction from the hardware registers, and it's fully safe to use on every x86
> > bare-metal system or VM.  And while I don't anticipate it now, maybe there's
> > some future scheme where a paravisor-like entity could be used with Intel
> > TDX.  It seems like using a cc_platform_has() abstraction is better than directly
> > accessing the MSR.
> 
> That's fine but we're talking about this particular implementation and that is
> vTOM-like with the address space split. If TDX does address space split later,
> we can accomodate it too. (Although I think they are not interested in this).
> 
> And if you really want to use cc_platform_has(), we could do
> 
> 	cc_platform_has(CC_ADDRESS_SPACE_SPLIT_ON_A_PARAVISOR)
> 
> or something with a better name.

I do think it makes sense to use the cc_platform_has() abstraction.  It's
then a question of agreeing on how to name the attribute.  We've
discussed various approaches in different versions of this patch series:

v1 & v2:  CC_ATTR_HAS_PARAVISOR
v3:  CC_ATTR_EMULATED_IOAPIC
v4 & v5:  CC_ATTR_ACCESS_IOAPIC_ENCRYPTED

I could do:
1.  CC_ATTR_PARAVISOR_SPLIT_ADDRESS_SPACE, which is similar to
    what I had for v1 & v2.   At the time, somebody commented that
    this might be a bit too general.
2.  Keep CC_ATTR_ACCESS_IOAPIC_ENCRYPTED and add
    CC_ATTR_ACCESS_TPM_ENCRYPTED, which would decouple them
3.  CC_ATTR_ACCESS_IOAPIC_AND_TPM_ENCRYPTED, which is very
    narrow and specific.

I have weak preference for #1 above, but I could go with any of them.
What's your preference?

> > My resolution of the TPM driver issue is admittedly a work-around.   I think
> > of it as temporary in anticipation of future implementations of PCIe TDISP
> > hardware, which allows PCI devices to DMA directly into guest encrypted
> > memory.
> 
> Yap, that sounds real nice.
> 
> > TDISP also places the device's BAR values in an encrypted portion
> > of the GPA space. Assuming TDISP hardware comes along in the next couple
> > of years, Linux will need a robust way to deal with a mix of PCI devices
> > being in unencrypted and encrypted GPA space.  I don't know how a
> > specific device will be mapped correctly, but I hope it can happen in the
> > generic PCI code, and not by modifying each device driver.
> 
> I guess those devices would advertize that capability somehow so that code can
> query it and act accordingly.
> 
> > It's probably premature to build that robust mechanism now, but when it comes,
> > my work-around would be replaced.
> 
> It would be replaced if it doesn't have any users. By the looks of it, it'll
> soon grow others and then good luck removing it.
> 
> > With all that in mind, I don't want to modify the TPM driver to special-case
> > its MMIO space being encrypted.  FWIW, the TPM driver today uses
> > devm_ioremap_resource() to do the mapping, which defaults to mapping
> > decrypted except for the exceptions implemented in __ioremap_caller().
> > There's no devm_* option for specifying encrypted.
> 
> You mean, it is hard to add a DEVM_IOREMAP_ENCRYPTED type which will have
> __devm_ioremap() call ioremap_encrypted()?
> 
> Or define a IORESOURCE_ENCRYPTED and pass it through the ioresource flags?
> 
> Why is that TPM driver so precious that it can be touched and the arch code
> would have to accept hacks?
> 
> > Handling decrypted vs. encrypted in the driver would require extending the
> > driver API to provide an "encrypted" option, and that seems like going in the
> > wrong long-term direction.
> 
> Sorry, I can't follow here.
> 

For v6 of the patch series, I've coded devm_ioremap_resource_enc() to call
__devm_ioremap(), which then calls ioremap_encrypted().  I've updated the
TPM driver to use cc_platform_has() with whatever attribute name we agree
on to decide between devm_ioremap_resource_enc() and
devm_ioremap_resource().

If this approach is OK with the TPM driver maintainers, I'm good with it.
More robust handling of a mix of encrypted and decrypted devices can get
sorted out later.

Michael