diff mbox series

[PATCHv5,08/12] x86/mm: Provide helpers for unaccepted memory

Message ID 20220425033934.68551-9-kirill.shutemov@linux.intel.com (mailing list archive)
State New
Headers show
Series mm, x86/cc: Implement support for unaccepted memory | expand

Commit Message

Kirill A. Shutemov April 25, 2022, 3:39 a.m. UTC 
Core-mm requires few helpers to support unaccepted memory:

 - accept_memory() checks the range of addresses against the bitmap and
   accept memory if needed.

 - memory_is_unaccepted() check if anything within the range requires
   acceptance.

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
---
 arch/x86/include/asm/page.h              |  3 ++
 arch/x86/include/asm/unaccepted_memory.h |  4 ++
 arch/x86/mm/Makefile                     |  2 +
 arch/x86/mm/unaccepted_memory.c          | 56 ++++++++++++++++++++++++
 4 files changed, 65 insertions(+)
 create mode 100644 arch/x86/mm/unaccepted_memory.c

Comments

Borislav Petkov May 4, 2022, 11:12 a.m. UTC | #1 
On Mon, Apr 25, 2022 at 06:39:30AM +0300, Kirill A. Shutemov wrote:
> +/* Protects unaccepted memory bitmap */
> +static DEFINE_SPINLOCK(unaccepted_memory_lock);
> +
> +void accept_memory(phys_addr_t start, phys_addr_t end)
> +{
> +	unsigned long *unaccepted_memory;

shorten that name.

> +	unsigned long flags;
> +	unsigned long range_start, range_end;

The tip-tree preferred ordering of variable declarations at the
beginning of a function is reverse fir tree order::

	struct long_struct_name *descriptive_name;
	unsigned long foo, bar;
	unsigned int tmp;
	int ret;

The above is faster to parse than the reverse ordering::

	int ret;
	unsigned int tmp;
	unsigned long foo, bar;
	struct long_struct_name *descriptive_name;

And even more so than random ordering::

	unsigned long foo, bar;
	int ret;
	struct long_struct_name *descriptive_name;
	unsigned int tmp;

> +
> +	if (!boot_params.unaccepted_memory)
> +		return;
> +
> +	unaccepted_memory = __va(boot_params.unaccepted_memory);
> +	range_start = start / PMD_SIZE;
> +
> +	spin_lock_irqsave(&unaccepted_memory_lock, flags);
> +	for_each_set_bitrange_from(range_start, range_end, unaccepted_memory,
> +				   DIV_ROUND_UP(end, PMD_SIZE)) {
> +		unsigned long len = range_end - range_start;
> +
> +		/* Platform-specific memory-acceptance call goes here */
> +		panic("Cannot accept memory");

Yeah, no, WARN_ON_ONCE() pls.

> +		bitmap_clear(unaccepted_memory, range_start, len);
> +	}
> +	spin_unlock_irqrestore(&unaccepted_memory_lock, flags);
> +}
> +
> +bool memory_is_unaccepted(phys_addr_t start, phys_addr_t end)
> +{
> +	unsigned long *unaccepted_memory = __va(boot_params.unaccepted_memory);

As above, shorten that one.

> +	unsigned long flags;
> +	bool ret = false;
> +
> +	spin_lock_irqsave(&unaccepted_memory_lock, flags);
> +	while (start < end) {
> +		if (test_bit(start / PMD_SIZE, unaccepted_memory)) {
> +			ret = true;

Wait, what?

That thing is lying: it'll return true for *some* PMD which is accepted
but not the whole range of [start, end].
Kirill A . Shutemov May 6, 2022, 4:13 p.m. UTC | #2 
On Wed, May 04, 2022 at 01:12:06PM +0200, Borislav Petkov wrote:
> On Mon, Apr 25, 2022 at 06:39:30AM +0300, Kirill A. Shutemov wrote:
> > +	unaccepted_memory = __va(boot_params.unaccepted_memory);
> > +	range_start = start / PMD_SIZE;
> > +
> > +	spin_lock_irqsave(&unaccepted_memory_lock, flags);
> > +	for_each_set_bitrange_from(range_start, range_end, unaccepted_memory,
> > +				   DIV_ROUND_UP(end, PMD_SIZE)) {
> > +		unsigned long len = range_end - range_start;
> > +
> > +		/* Platform-specific memory-acceptance call goes here */
> > +		panic("Cannot accept memory");
> 
> Yeah, no, WARN_ON_ONCE() pls.

Failure to accept the memory is fatal. Why pretend it is not?

For TDX it will result in a crash on the first access. Prolonging the
suffering just make it harder to understand what happened.

> > +	unsigned long flags;
> > +	bool ret = false;
> > +
> > +	spin_lock_irqsave(&unaccepted_memory_lock, flags);
> > +	while (start < end) {
> > +		if (test_bit(start / PMD_SIZE, unaccepted_memory)) {
> > +			ret = true;
> 
> Wait, what?
> 
> That thing is lying: it'll return true for *some* PMD which is accepted
> but not the whole range of [start, end].

That's true. Note also that the check is inherently racy. Other CPU can
get the range or subrange accepted just after spin_unlock().

The check indicates that accept_memory() has to be called on the range
before first access.

Do you have problem with a name? Maybe has_unaccepted_memory()?
Borislav Petkov May 10, 2022, 6:32 p.m. UTC | #3 
On Fri, May 06, 2022 at 07:13:59PM +0300, Kirill A. Shutemov wrote:
> Failure to accept the memory is fatal. Why pretend it is not?
> 
> For TDX it will result in a crash on the first access. Prolonging the
> suffering just make it harder to understand what happened.

Ok then. Does that panic message contain enough info so that the
acceptance failure can be debugged?

Just "Cannot accept memory" doesn't seem very helpful to me...

> That's true. Note also that the check is inherently racy. Other CPU can
> get the range or subrange accepted just after spin_unlock().
> 
> The check indicates that accept_memory() has to be called on the range
> before first access.
> 
> Do you have problem with a name? Maybe has_unaccepted_memory()?

I have a problem with the definition of this function, what it is
supposed to do and how it is supposed to be used.

Right now, it looks weird and strange: is it supposed to check for *all*
in-between (start, end)? It doesn't, atm, so what's the meaning of
@start and @end then at all?
Kirill A . Shutemov May 11, 2022, 1:15 a.m. UTC | #4 
On Tue, May 10, 2022 at 08:32:23PM +0200, Borislav Petkov wrote:
> On Fri, May 06, 2022 at 07:13:59PM +0300, Kirill A. Shutemov wrote:
> > Failure to accept the memory is fatal. Why pretend it is not?
> > 
> > For TDX it will result in a crash on the first access. Prolonging the
> > suffering just make it harder to understand what happened.
> 
> Ok then. Does that panic message contain enough info so that the
> acceptance failure can be debugged?
> 
> Just "Cannot accept memory" doesn't seem very helpful to me...

Okay. Fair enough. I will change it to

			panic("Cannot accept memory: unknown platform.");

> 
> > That's true. Note also that the check is inherently racy. Other CPU can
> > get the range or subrange accepted just after spin_unlock().
> > 
> > The check indicates that accept_memory() has to be called on the range
> > before first access.
> > 
> > Do you have problem with a name? Maybe has_unaccepted_memory()?
> 
> I have a problem with the definition of this function, what it is
> supposed to do and how it is supposed to be used.
> 
> Right now, it looks weird and strange: is it supposed to check for *all*
> in-between (start, end)? It doesn't, atm, so what's the meaning of
> @start and @end then at all?

It checks if the range of memory requires accept_memory() call before it
can be accessed.

If any part of the range is not accepted, the call is required.
accept_memory() knows what exactly has to be done. Note that
accept_memory() call is harmless for any valid memory range.
It can be called on already accepted memory.
Borislav Petkov May 11, 2022, 9:07 a.m. UTC | #5 
On Wed, May 11, 2022 at 04:15:35AM +0300, Kirill A. Shutemov wrote:
> Okay. Fair enough. I will change it to
> 
> 			panic("Cannot accept memory: unknown platform.");

So I haven't went all the way in the patchset but what I see is:

                /* Platform-specific memory-acceptance call goes here */
                if (cpu_feature_enabled(X86_FEATURE_TDX_GUEST)) {
                        tdx_accept_memory(range_start * PMD_SIZE,
                                          range_end * PMD_SIZE);
                } else {
                        panic("Cannot accept memory");
                }

so how would you decide for some other platform that it should panic?

TDX should panic, that I get. But you can just as well WARN_ONCE() here
so that it gets fixed. Panicking is counterproductive.

> It checks if the range of memory requires accept_memory() call before it
> can be accessed.
> 
> If any part of the range is not accepted, the call is required.
> accept_memory() knows what exactly has to be done. Note that
> accept_memory() call is harmless for any valid memory range.
> It can be called on already accepted memory.

Aaah, so that's what I was missing. So this function definitely needs a
comment ontop of it. And a name change to something like

	range_contains_unaccepted_memory()

or so to actually state what it does.
diff mbox series

Patch

diff --git a/arch/x86/include/asm/page.h b/arch/x86/include/asm/page.h
index 9cc82f305f4b..df4ec3a988dc 100644
--- a/arch/x86/include/asm/page.h
+++ b/arch/x86/include/asm/page.h
@@ -19,6 +19,9 @@ 
 struct page;
 
 #include <linux/range.h>
+
+#include <asm/unaccepted_memory.h>
+
 extern struct range pfn_mapped[];
 extern int nr_pfn_mapped;
 
diff --git a/arch/x86/include/asm/unaccepted_memory.h b/arch/x86/include/asm/unaccepted_memory.h
index 41fbfc798100..a59264ee0ab3 100644
--- a/arch/x86/include/asm/unaccepted_memory.h
+++ b/arch/x86/include/asm/unaccepted_memory.h
@@ -7,6 +7,10 @@  struct boot_params;
 
 void process_unaccepted_memory(struct boot_params *params, u64 start, u64 num);
 
+#ifdef CONFIG_UNACCEPTED_MEMORY
+
 void accept_memory(phys_addr_t start, phys_addr_t end);
+bool memory_is_unaccepted(phys_addr_t start, phys_addr_t end);
 
 #endif
+#endif
diff --git a/arch/x86/mm/Makefile b/arch/x86/mm/Makefile
index fe3d3061fc11..e327f83e6bbf 100644
--- a/arch/x86/mm/Makefile
+++ b/arch/x86/mm/Makefile
@@ -60,3 +60,5 @@  obj-$(CONFIG_AMD_MEM_ENCRYPT)	+= mem_encrypt_amd.o
 
 obj-$(CONFIG_AMD_MEM_ENCRYPT)	+= mem_encrypt_identity.o
 obj-$(CONFIG_AMD_MEM_ENCRYPT)	+= mem_encrypt_boot.o
+
+obj-$(CONFIG_UNACCEPTED_MEMORY)	+= unaccepted_memory.o
diff --git a/arch/x86/mm/unaccepted_memory.c b/arch/x86/mm/unaccepted_memory.c
new file mode 100644
index 000000000000..1327f64d5205
--- /dev/null
+++ b/arch/x86/mm/unaccepted_memory.c
@@ -0,0 +1,56 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+#include <linux/memblock.h>
+#include <linux/mm.h>
+#include <linux/pfn.h>
+#include <linux/spinlock.h>
+
+#include <asm/io.h>
+#include <asm/setup.h>
+#include <asm/unaccepted_memory.h>
+
+/* Protects unaccepted memory bitmap */
+static DEFINE_SPINLOCK(unaccepted_memory_lock);
+
+void accept_memory(phys_addr_t start, phys_addr_t end)
+{
+	unsigned long *unaccepted_memory;
+	unsigned long flags;
+	unsigned long range_start, range_end;
+
+	if (!boot_params.unaccepted_memory)
+		return;
+
+	unaccepted_memory = __va(boot_params.unaccepted_memory);
+	range_start = start / PMD_SIZE;
+
+	spin_lock_irqsave(&unaccepted_memory_lock, flags);
+	for_each_set_bitrange_from(range_start, range_end, unaccepted_memory,
+				   DIV_ROUND_UP(end, PMD_SIZE)) {
+		unsigned long len = range_end - range_start;
+
+		/* Platform-specific memory-acceptance call goes here */
+		panic("Cannot accept memory");
+		bitmap_clear(unaccepted_memory, range_start, len);
+	}
+	spin_unlock_irqrestore(&unaccepted_memory_lock, flags);
+}
+
+bool memory_is_unaccepted(phys_addr_t start, phys_addr_t end)
+{
+	unsigned long *unaccepted_memory = __va(boot_params.unaccepted_memory);
+	unsigned long flags;
+	bool ret = false;
+
+	spin_lock_irqsave(&unaccepted_memory_lock, flags);
+	while (start < end) {
+		if (test_bit(start / PMD_SIZE, unaccepted_memory)) {
+			ret = true;
+			break;
+		}
+
+		start += PMD_SIZE;
+	}
+	spin_unlock_irqrestore(&unaccepted_memory_lock, flags);
+
+	return ret;
+}