Message-ID: <BYAPR21MB168897DBA98E91B72B4087E1D7CA9@BYAPR21MB1688.namprd21.prod.outlook.com>
Date:   Sat, 21 Jan 2023 04:10:23 +0000
From:   "Michael Kelley (LINUX)" <mikelley@...rosoft.com>
To:     Borislav Petkov <bp@...en8.de>
CC:     "hpa@...or.com" <hpa@...or.com>, KY Srinivasan <kys@...rosoft.com>,
        Haiyang Zhang <haiyangz@...rosoft.com>,
        "wei.liu@...nel.org" <wei.liu@...nel.org>,
        Dexuan Cui <decui@...rosoft.com>,
        "luto@...nel.org" <luto@...nel.org>,
        "peterz@...radead.org" <peterz@...radead.org>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "edumazet@...gle.com" <edumazet@...gle.com>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "pabeni@...hat.com" <pabeni@...hat.com>,
        "lpieralisi@...nel.org" <lpieralisi@...nel.org>,
        "robh@...nel.org" <robh@...nel.org>, "kw@...ux.com" <kw@...ux.com>,
        "bhelgaas@...gle.com" <bhelgaas@...gle.com>,
        "arnd@...db.de" <arnd@...db.de>, "hch@....de" <hch@....de>,
        "m.szyprowski@...sung.com" <m.szyprowski@...sung.com>,
        "robin.murphy@....com" <robin.murphy@....com>,
        "thomas.lendacky@....com" <thomas.lendacky@....com>,
        "brijesh.singh@....com" <brijesh.singh@....com>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        "mingo@...hat.com" <mingo@...hat.com>,
        "dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
        Tianyu Lan <Tianyu.Lan@...rosoft.com>,
        "kirill.shutemov@...ux.intel.com" <kirill.shutemov@...ux.intel.com>,
        "sathyanarayanan.kuppuswamy@...ux.intel.com" 
        <sathyanarayanan.kuppuswamy@...ux.intel.com>,
        "ak@...ux.intel.com" <ak@...ux.intel.com>,
        "isaku.yamahata@...el.com" <isaku.yamahata@...el.com>,
        "dan.j.williams@...el.com" <dan.j.williams@...el.com>,
        "jane.chu@...cle.com" <jane.chu@...cle.com>,
        "seanjc@...gle.com" <seanjc@...gle.com>,
        "tony.luck@...el.com" <tony.luck@...el.com>,
        "x86@...nel.org" <x86@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-hyperv@...r.kernel.org" <linux-hyperv@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
        "linux-arch@...r.kernel.org" <linux-arch@...r.kernel.org>,
        "iommu@...ts.linux.dev" <iommu@...ts.linux.dev>
Subject: RE: [PATCH v5 06/14] x86/ioremap: Support hypervisor specified range
 to map as encrypted

From: Borislav Petkov <bp@...en8.de> 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.

Powered by blists - more mailing lists