Messages in this thread | | | Subject | Re: [PATCH 19/26] x86/tdx: Make pages shared in ioremap() | From | Dave Hansen <> | Date | Thu, 23 Dec 2021 11:45:19 -0800 |
| |
On 12/23/21 9:15 AM, Kirill A. Shutemov wrote: >>> +pgprot_t pgprot_cc_encrypted(pgprot_t prot) >>> +{ >>> + if (cc_platform_has(CC_ATTR_HOST_MEM_ENCRYPT)) >>> + return __pgprot(__sme_set(pgprot_val(prot))); >>> + else if (cc_platform_has(CC_ATTR_GUEST_TDX)) >>> + return __pgprot(pgprot_val(prot) & ~tdx_shared_mask()); >>> + >> Hmmm... I believe this breaks SEV guests. __sme_set() uses sme_me_mask which >> is used for both SME and SEV. With the current checks, an SEV guest will end >> up never setting an encrypted address through this path. Ditto below on the >> decrypted path. > Hm, okay. What if I rewrite code like this: > > pgprot_t pgprot_cc_encrypted(pgprot_t prot) > { > if (cc_platform_has(CC_ATTR_GUEST_TDX)) > return __pgprot(pgprot_val(prot) & ~tdx_shared_mask()); > else > return __pgprot(__sme_set(pgprot_val(prot))); > } > > I believe it should cover all cases, right?
I _think_ that should be fine for now. But, it does expose that __sme_set() is weird because it get used on non-SME systems while tdx_shared_mask() is only used on TDX systems.
Ideally, we'd eventually get to something close to what you had originally:
pgprot_t pgprot_cc_encrypted(pgprot_t prot) { if (cc_platform_has(CC_ATTR_GUEST_TDX)) return __pgprot(pgprot_val(prot) & ~tdx_shared_mask()); if (cc_platform_has(CC_ATTR_SME_SOMETHING??)) return __pgprot(pgprot_val(prot) | sme_me_mask));
return prot; }
CC_ATTR_SME_SOMETHING would get set when sme_me_mask is initialized to something non-zero. That will keep folks from falling into the same trap that you did in the long term.
The SEV code wasn't crazy for doing what it did when it was the only game in town. But, now that TDX is joining the party, we need to make sure that SEV isn't special.
| |