On Sat, Jan 21, 2023 at 04:10:23AM +0000, Michael Kelley (LINUX) wrote:

Per the commit message for 009767dbf42a, it's not safe to read MSR_AMD64_SEV
on all implementations of AMD processors.

1. Why does that matter to you? This is Hygon.

2. The MSR access is behind CPUID check.

CC_ATTR_ACCESS_IOAPIC_ENCRYPTED is used in io_apic_set_fixmap(), which is
called on all Intel/AMD systems without any qualifications.   Even if
rdmsrl_safe() works at this point in boot, I'm a little leery of reading a new
MSR in a path that essentially every x86 bare-metal system or VM takes during
boot.

You read the MSR once and cache it. sme_enable() already does that. I don't see
what the problem is.

Or am I being overly paranoid about old processor models or hypervisor
versions potentially doing something weird?

Yes, you are. :)

If they're doing something weird which is out of spec, then we'll deal with them
later.

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.

The point is, you want to do things which are specific for this particular
implementation. If so, then check for it. Do not do hacky things which get the
work done for your case but can very easily be misused by others.

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.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette