Hi Boris,


On 9/29/21 9:34 AM, Borislav Petkov wrote:

...

quoted

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@google.com/#t (local)

quoted

Signed-off-by: Brijesh Singh <redacted>
---
 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()

quoted

+{
+	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