From: Borislav Petkov <bp@xxxxxxxxx> Sent: Friday, January 20, 2023 12:15 PM > > On Thu, Jan 12, 2023 at 01:42:25PM -0800, Michael Kelley wrote: > > In a AMD SEV-SNP VM using vTOM, devices in MMIO space may be provided by > > the paravisor and need to be mapped as encrypted. Provide a function > > for the hypervisor to specify the address range for such devices. > > In __ioremap_caller(), map addresses in this range as encrypted. > > > > Only a single range is supported. If multiple devices need to be > > mapped encrypted, the paravisor must place them within the single > > contiguous range. > > This already is starting to sound insufficient and hacky. And it also makes > CC_ATTR_ACCESS_IOAPIC_ENCRYPTED insufficient either. > > So, the situation we have is, we're a SEV-SNP VM using vTOM. Which means, > MSR_AMD64_SEV[3] = 1. Or SEV_FEATURES[1], alternatively - same thing. > > That MSR cannot be intercepted by the HV and we use it extensively in Linux when > it runs as a SEV-* guest. And I had asked this before, during review, but why > aren't you checking this bit above when you wanna do vTOM-specific work? Per the commit message for 009767dbf42a, it's not safe to read MSR_AMD64_SEV on all implementations of AMD processors. 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. Or am I being overly paranoid about old processor models or hypervisor versions potentially doing something weird? 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. > > Because then you can do that check and > > 1. map the IO-APIC encrypted > 2. map MMIO space of devices from the driver encrypted too > 3. ... 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. 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. It's probably premature to build that robust mechanism now, but when it comes, my work-around would be replaced. 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. 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. Finally, I would have liked to handle the IO-APIC and the vTPM the same way. But the IO-APIC doesn't use the normal ioremap_* functions because it's done early in boot via the fixmap. I didn't see a reasonable way to unify them. Anyway, that's my thoughts. I'm certainly open if someone has a better way to do it. Michael > > and so on. > > And you won't need those other, not as nice things... > > Hmmm.
