Hi Boris, On 9/29/21 9:34 AM, Borislav Petkov wrote: ... >> Improve the rmp_make_private() to >> invalid state so that pages cannot be used in the direct-map after its >> added in the RMP table, and restore to its default valid permission after >> the pages are removed from the RMP table. > That sentence needs rewriting into proper english. > > The more important thing is, though, this doesn't talk about *why* > you're doing this: you want to remove pages from the direct map when > they're in the RMP table because something might modify the page and > then the RMP check will fail? I'll work to improve the commit description. > Also, set_direct_map_invalid_noflush() simply clears the Present and RW > bit of a pte. > > So what's up? The set_direct_map_invalid_noflush() does two steps 1) Split the host page table (if needed) 2) Clear the present and RW bit from pte In previous patches I was using the set_memory_4k() to split the direct map before adding the pages in the RMP table. Based on Sean's review comment [1], I switched to using set_direct_map_invalid_noflush() so that page is not just split but also removed from the direct map. The thought process is if in the future set_direct_map_default_noflush() is improved to restore the large mapping then it will all work transparently. [1] https://lore.kernel.org/lkml/YO9kP1v0TAFXISHD@xxxxxxxxxx/#t >> Signed-off-by: Brijesh Singh <brijesh.singh@xxxxxxx> >> --- >> arch/x86/kernel/sev.c | 61 ++++++++++++++++++++++++++++++++++++++++++- >> 1 file changed, 60 insertions(+), 1 deletion(-) >> >> diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c >> index 8627c49666c9..bad41deb8335 100644 >> --- a/arch/x86/kernel/sev.c >> +++ b/arch/x86/kernel/sev.c >> @@ -2441,10 +2441,42 @@ int psmash(u64 pfn) >> } >> EXPORT_SYMBOL_GPL(psmash); >> >> +static int restore_direct_map(u64 pfn, int npages) > restore_pages_in_direct_map() > >> +{ >> + int i, ret = 0; >> + >> + for (i = 0; i < npages; i++) { >> + ret = set_direct_map_default_noflush(pfn_to_page(pfn + i)); >> + if (ret) >> + goto cleanup; >> + } > So this is looping over a set of virtually contiguous pages, I presume, > and if so, you should add a function called > > set_memory_p_rw() > > to arch/x86/mm/pat/set_memory.c which does > > return change_page_attr_set(&addr, numpages, __pgprot(_PAGE_PRESENT | _PAGE_RW), 0); > > so that you can do all pages in one go. I will look into it. thanks
