Message-ID: <Y9FC7Dpzr5Uge/Mi@zn.tnic>
Date:   Wed, 25 Jan 2023 15:55:40 +0100
From:   Borislav Petkov <bp@...en8.de>
To:     "Michael Kelley (LINUX)" <mikelley@...rosoft.com>
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

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

Powered by blists - more mailing lists