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

Re: [Xen-devel] [PATCH v2 05/10] xen/arm: Setup virtual paging for non-boot CPUs on hotplug/resume

On 04/24/2018 03:50 PM, Mirela Simonovic wrote:
Hi Julien,


Sorry I forgot to answer to some part of the e-mail.

On Mon, Apr 23, 2018 at 1:28 PM, Julien Grall <julien.grall@xxxxxxx> wrote:
Hi Mirela,
On 20/04/18 13:25, Mirela Simonovic wrote:
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index d43c3aa896..9bb62c13cd 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1451,13 +1451,21 @@ err:
       return page;
   -static void __init setup_virt_paging_one(void *data)
+static void setup_virt_paging_one(void *data)
       unsigned long val = (unsigned long)data;
       WRITE_SYSREG32(val, VTCR_EL2);
   +/* VTCR value to be configured by all CPUs. Set only once by the boot
CPU */
+static unsigned long __read_mostly vtcr_value;

VTCR is a register, so the type should be represented in term of bits (i.e

I followed the type used in setup_virt_paging() and it's unsigned
long. However, the spec says the VTCR_EL2 is 32-bit register, meaning
that in the current implementation the type is not correct.
If I want the type to be correct in this patch Xen will not compile.
Are you suggesting to fix the type in existing implementation?

The unsigned long is just a workaround to avoid an extra variable for the cast. As you introduce a static variable, then the cast become unnecessary.

+void setup_virt_paging_secondary(void)
+    setup_virt_paging_one((void *)vtcr_value);

That's fairly ugly. Is there any way to rework the interface? For instance,
because you have a static variable which contain the VTCR, you could just
use the variable in setup_virt_paging one.

I If the argument provided to setup_virt_paging_one() is NULL within the
setup_virt_paging_one() I configure saved static vtcr_value? If that
is what you meant it was submitted in previous version of this patch
Are you suggesting to revert the change to v1?

I would suggest a mix between v1 and v2. Something like:

static uint64_t vtcr;

static setup_init_paging_one(void *data)

void __init setup_virt_paging(...)
    vtcr = val;

Potentially, you could drop val and use vtcr everywhere in setup_virt_paging().


Julien Grall

Xen-devel mailing list



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