Messages in this thread | | | Subject | Re: [PATCH v5 3/3] x86/tdx: Add Quote generation support | From | Kai Huang <> | Date | Fri, 06 May 2022 23:00:29 +1200 |
| |
On Thu, 2022-05-05 at 16:06 -0700, Dave Hansen wrote: > On 5/5/22 15:15, Kai Huang wrote: > > set_memory_xx() is supposedly only for direct-mapping. Please use my > > suggestion above. > > Kai, please take a look at some of the other users, especially > set_memory_x(). See how long the "supposed" requirement holds up. > > That said, I've forgotten by now if this _could_ have used vmalloc() or > vmap() or vmap_pfn(). None of the logic about why or how the allocator > and mapping design decisions were made. Could that be be rectified for > the next post?
Hi Dave,
(Sorry previous reply was too fast..)
I spent some time looking into how __change_page_attr_set_clr() is implemented, which is called by all set_memory_xxx() variants. If my understanding is correct, __change_page_attr_set_clr() will work for vmap() variants, because it internally uses lookup_address(), which walks the page table directly, to find the actual PTE (and PFN). So set_memory_decrypted() can be fixed to support vmap() mapping for TDX.
However, looking at the code, set_memory_decrypted() calls __change_page_attr_set_clr(&cpa, 1). The second argument is 'checkalias', which means even we call set_memory_decrypted() against vmap() address, the aliasing mappings will be changed too. And if I understand correctly, the aliasing mapping includes direct-mapping too:
static int cpa_process_alias(struct cpa_data *cpa) { struct cpa_data alias_cpa; unsigned long laddr = (unsigned long)__va(cpa->pfn << PAGE_SHIFT); unsigned long vaddr; int ret; if (!pfn_range_is_mapped(cpa->pfn, cpa->pfn + 1)) return 0;
/* * No need to redo, when the primary call touched the direct * mapping already: */ vaddr = __cpa_addr(cpa, cpa->curpage); if (!(within(vaddr, PAGE_OFFSET, PAGE_OFFSET + (max_pfn_mapped << PAGE_SHIFT)))) { alias_cpa = *cpa; alias_cpa.vaddr = &laddr; alias_cpa.flags &= ~(CPA_PAGES_ARRAY | CPA_ARRAY); alias_cpa.curpage = 0; cpa->force_flush_all = 1;
ret = __change_page_attr_set_clr(&alias_cpa, 0); if (ret) return ret; }
#ifdef CONFIG_X86_64 /* * If the primary call didn't touch the high mapping already * and the physical address is inside the kernel map, we need * to touch the high mapped kernel as well: */ if (!within(vaddr, (unsigned long)_text, _brk_end) && __cpa_pfn_in_highmap(cpa->pfn)) { unsigned long temp_cpa_vaddr = (cpa->pfn << PAGE_SHIFT) + __START_KERNEL_map - phys_base; alias_cpa = *cpa; alias_cpa.vaddr = &temp_cpa_vaddr; alias_cpa.flags &= ~(CPA_PAGES_ARRAY | CPA_ARRAY); alias_cpa.curpage = 0;
cpa->force_flush_all = 1; /* * The high mapping range is imprecise, so ignore the * return value. */ __change_page_attr_set_clr(&alias_cpa, 0); } #endif
return 0; }
As you can see, the first chunk checks whether the virtual address is direct- mapping, and if it is not, direct mapping is changed too.
The second chunk even changes the high kernel mapping.
So, if we use set_memory_decrypted(), there's no difference whether the address is vmap() or direct mapping address. The direct mapping will be changed anyway.
(However, it seems if we call set_memory_decrypted() against direct mapping address, the vmap() mapping won't be impacted, because it seems cpa_process_alias() doesn't check vmap() area..).
However I may have missed something. Kirill please help to confirm if you see this.
-- Thanks, -Kai
| |