[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 2/4] xen/arm: acpi: The fixmap area should always be cleared during failure/unmap



Hi Stefano,

On 01/10/2020 01:30, Stefano Stabellini wrote:
On Sat, 26 Sep 2020, Julien Grall wrote:
From: Julien Grall <jgrall@xxxxxxxxxx>

Commit 022387ee1ad3 "xen/arm: mm: Don't open-code Xen PT update in
{set, clear}_fixmap()" enforced that each set_fixmap() should be
paired with a clear_fixmap(). Any failure to follow the model would
result to a platform crash.

Unfortunately, the use of fixmap in the ACPI code was overlooked as it
is calling set_fixmap() but not clear_fixmap().

The function __acpi_os_map_table() is reworked so:
     - We know before the mapping whether the fixmap region is big
     enough for the mapping.
     - It will fail if the fixmap is always inuse.

I take you mean "it will fail if the fixmap is *already* in use"?

Yes.


If so, can it be a problem? Or the expectation is that in practice
__acpi_os_map_table() will only get called once before SYS_STATE_boot?

Looking at the code it would seem that even before this patch
__acpi_os_map_table() wasn't able to handle multiple calls before
SYS_STATE_boot.

Correct, I am not changing any expectation here. It is only making clearer because before commit 022387ee1ad3 we would just overwrite the existing mapping with no warning.

After commit 022387ee1ad3, we would just hit the BUG_ON() in set_fixmap().

I will clarify it in the commit message.

[...]

  bool __acpi_unmap_table(void *ptr, unsigned long size)
  {
-    return ( vaddr >= FIXMAP_ADDR(FIXMAP_ACPI_BEGIN) &&
-             vaddr < (FIXMAP_ADDR(FIXMAP_ACPI_END) + PAGE_SIZE) );
+    vaddr_t vaddr = (vaddr_t)ptr;
+    unsigned int idx;
+
+    /* We are only handling fixmap address in the arch code */
+    if ( vaddr < FIXMAP_ADDR(FIXMAP_ACPI_BEGIN) ||
+         vaddr >= FIXMAP_ADDR(FIXMAP_ACPI_END) )

The "+ PAGE_SIZE" got lost

Hmmm yes.



+        return false;
+
+    /*
+     * __acpi_map_table() will always return a pointer in the first page
+     * for the ACPI fixmap region. The caller is expected to free with
+     * the same address.
+     */
+    ASSERT((vaddr & PAGE_MASK) == FIXMAP_ADDR(FIXMAP_ACPI_BEGIN));
+
+    /* The region allocated fit in the ACPI fixmap region. */
+    ASSERT(size < (FIXMAP_ADDR(FIXMAP_ACPI_END) + PAGE_SIZE - vaddr));
+    ASSERT(fixmap_inuse);
+
+    fixmap_inuse = false;
+
+    size += FIXMAP_ADDR(FIXMAP_ACPI_BEGIN) - vaddr;

Sorry I got confused.. Shouldn't this be:

   size += vaddr - FIXMAP_ADDR(FIXMAP_ACPI_BEGIN);

?

It should be. :) I guess this was unoticed because vaddr == FIXMAP_ADDR(FIXMAP_ACPI_BEGIN) in my testing.

I will fix it.



+    idx = FIXMAP_ACPI_BEGIN;
+
+    do
+    {
+        clear_fixmap(idx);
+        size -= min(size, (unsigned long)PAGE_SIZE);
+        idx++;
+    } while ( size > 0 );
+
+    return true;
  }
/* True to indicate PSCI 0.2+ is implemented */
--
2.17.1


Cheers,

--
Julien Grall



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.