Date:   Thu, 2 Feb 2023 05:49:44 +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: 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

Powered by blists - more mailing lists