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

2018-04-25 Thread Julien Grall 



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

Hi Julien,


Hi,

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


On Mon, Apr 23, 2018 at 1:28 PM, Julien Grall  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);
   isb();
   }
   +/* 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
uint*_t).


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)
{
   WRITE_SYSREG(vtcr, VTRC_EL2);
   [...]
}
r

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

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


Cheers,

--
Julien Grall

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

2018-04-24 Thread Julien Grall 



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

Hi Julien,


Hi Mirela,


Thanks for the feedback.

On Mon, Apr 23, 2018 at 1:28 PM, Julien Grall  wrote:

Hi Mirela,


On 20/04/18 13:25, Mirela Simonovic wrote:


In existing code the paging for non-boot CPUs is setup only on boot. The
setup is triggered from start_xen() after all CPUs are brought online.
In other words, the initialization of VTCR_EL2 register is done out of the
cpu_up/start_secondary() control flow. However, the cpu_up flow is also
used
to hotplug non-boot CPUs on resume from suspend to RAM state, in which
case
the virtual paging will not be configured.
With this patch the setting of paging is triggered from start_secondary()
function if the current system state is not boot. This way, the paging
for secondary CPUs will be setup in non-boot scenarios as well, while the
setup in boot scenario remains unchanged.
It is assumed here that after the system completed the boot, CPUs that
execute start_secondary() were booted as well when the Xen itself was
booted. According to this assumption non-boot CPUs will always be
compliant
with the VTCR_EL2 value that was selected by Xen on boot.
Currently, these in no mechanism to trigger hotplugging of a CPU. This
will be added with the suspend to RAM support for ARM, where the hotplug
of non-boot CPUs will be triggered via enable_nonboot_cpus() call.

Signed-off-by: Mirela Simonovic 

---
CC: Stefano Stabellini 
CC: Julien Grall 
---
Changes in v2:
-Fix commit message
-Save configured VTCR_EL2 value into static variable that will be used
   by non-boot CPUs on hotplug
-Add setup_virt_paging_secondary() and invoke it from start_secondary()
   if that CPU has to setup virtual paging (if the system state is not
boot)
---
   xen/arch/arm/p2m.c| 13 -
   xen/arch/arm/smpboot.c|  3 +++
   xen/include/asm-arm/p2m.h |  3 +++
   3 files changed, 18 insertions(+), 1 deletion(-)

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);
   isb();
   }
   +/* 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
uint*_t).


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?




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



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?


+}
+
   void __init setup_virt_paging(void)
   {
   /* Setup Stage 2 address translation */
@@ -1540,6 +1548,9 @@ void __init setup_virt_paging(void)
   BUG_ON( P2M_ROOT_LEVEL == 0 && P2M_ROOT_ORDER > 0 );
   setup_virt_paging_one((void *)val);
   smp_call_function(setup_virt_paging_one, (void *)val, 1);
+
+/* Save configured value (to be used later for secondary CPUs
hotplug) */
+vtcr_value = val;
   }
 /*
diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
index 38b665a6d2..abc642804f 100644
--- a/xen/arch/arm/smpboot.c
+++ b/xen/arch/arm/smpboot.c
@@ -358,6 +358,9 @@ void start_secondary(unsigned long boot_phys_offset,
   local_irq_enable();
   local_abort_enable();
   +if ( system_state != SYS_STATE_boot )
+setup_virt_paging_secondary();


I think this code needs some documentation. So people understand why you
only call setup_virt_paging_secondary() after boot. But is there any reason
to not use a notifier (see notify_cpu_starting)? This would avoid yet
another export.


It works using notifiers, but I wouldn't say it's worth the overhead.
Implementation using notifiers requires 1 additional data structure
(notifier_block) and 2 functions to be implemented (an __init function
to register a notifier and another one for callback).
Moreover, such a callback would be called for each CPU event, which is
3 times when the CPU is hot-unplugged and nothing needs to be done.
I've

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

2018-04-24 Thread Mirela Simonovic 
Hi Julien,

Thanks for the feedback.

On Mon, Apr 23, 2018 at 1:28 PM, Julien Grall  wrote:
> Hi Mirela,
>
>
> On 20/04/18 13:25, Mirela Simonovic wrote:
>>
>> In existing code the paging for non-boot CPUs is setup only on boot. The
>> setup is triggered from start_xen() after all CPUs are brought online.
>> In other words, the initialization of VTCR_EL2 register is done out of the
>> cpu_up/start_secondary() control flow. However, the cpu_up flow is also
>> used
>> to hotplug non-boot CPUs on resume from suspend to RAM state, in which
>> case
>> the virtual paging will not be configured.
>> With this patch the setting of paging is triggered from start_secondary()
>> function if the current system state is not boot. This way, the paging
>> for secondary CPUs will be setup in non-boot scenarios as well, while the
>> setup in boot scenario remains unchanged.
>> It is assumed here that after the system completed the boot, CPUs that
>> execute start_secondary() were booted as well when the Xen itself was
>> booted. According to this assumption non-boot CPUs will always be
>> compliant
>> with the VTCR_EL2 value that was selected by Xen on boot.
>> Currently, these in no mechanism to trigger hotplugging of a CPU. This
>> will be added with the suspend to RAM support for ARM, where the hotplug
>> of non-boot CPUs will be triggered via enable_nonboot_cpus() call.
>>
>> Signed-off-by: Mirela Simonovic 
>>
>> ---
>> CC: Stefano Stabellini 
>> CC: Julien Grall 
>> ---
>> Changes in v2:
>> -Fix commit message
>> -Save configured VTCR_EL2 value into static variable that will be used
>>   by non-boot CPUs on hotplug
>> -Add setup_virt_paging_secondary() and invoke it from start_secondary()
>>   if that CPU has to setup virtual paging (if the system state is not
>> boot)
>> ---
>>   xen/arch/arm/p2m.c| 13 -
>>   xen/arch/arm/smpboot.c|  3 +++
>>   xen/include/asm-arm/p2m.h |  3 +++
>>   3 files changed, 18 insertions(+), 1 deletion(-)
>>
>> 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);
>>   isb();
>>   }
>>   +/* 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
> uint*_t).

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?

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

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?

>> +}
>> +
>>   void __init setup_virt_paging(void)
>>   {
>>   /* Setup Stage 2 address translation */
>> @@ -1540,6 +1548,9 @@ void __init setup_virt_paging(void)
>>   BUG_ON( P2M_ROOT_LEVEL == 0 && P2M_ROOT_ORDER > 0 );
>>   setup_virt_paging_one((void *)val);
>>   smp_call_function(setup_virt_paging_one, (void *)val, 1);
>> +
>> +/* Save configured value (to be used later for secondary CPUs
>> hotplug) */
>> +vtcr_value = val;
>>   }
>> /*
>> diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
>> index 38b665a6d2..abc642804f 100644
>> --- a/xen/arch/arm/smpboot.c
>> +++ b/xen/arch/arm/smpboot.c
>> @@ -358,6 +358,9 @@ void start_secondary(unsigned long boot_phys_offset,
>>   local_irq_enable();
>>   local_abort_enable();
>>   +if ( system_state != SYS_STATE_boot )
>> +setup_virt_paging_secondary();
>
> I think this code needs some documentation. So people understand why you
> only call setup_virt_paging_secondary() after boot. But is there any reason
> to not use a notifier (see notify_cpu_starting)? This would avoid yet
> another export.

It works using notifiers, but I wouldn't say it's worth the overhead.
Implementation using notifiers requires 1 additional data structure
(notifier_block) and 2 functions to be implemented (an __init function
to register a

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

2018-04-23 Thread Julien Grall 

Hi Mirela,

On 20/04/18 13:25, Mirela Simonovic wrote:

In existing code the paging for non-boot CPUs is setup only on boot. The
setup is triggered from start_xen() after all CPUs are brought online.
In other words, the initialization of VTCR_EL2 register is done out of the
cpu_up/start_secondary() control flow. However, the cpu_up flow is also used
to hotplug non-boot CPUs on resume from suspend to RAM state, in which case
the virtual paging will not be configured.
With this patch the setting of paging is triggered from start_secondary()
function if the current system state is not boot. This way, the paging
for secondary CPUs will be setup in non-boot scenarios as well, while the
setup in boot scenario remains unchanged.
It is assumed here that after the system completed the boot, CPUs that
execute start_secondary() were booted as well when the Xen itself was
booted. According to this assumption non-boot CPUs will always be compliant
with the VTCR_EL2 value that was selected by Xen on boot.
Currently, these in no mechanism to trigger hotplugging of a CPU. This
will be added with the suspend to RAM support for ARM, where the hotplug
of non-boot CPUs will be triggered via enable_nonboot_cpus() call.

Signed-off-by: Mirela Simonovic 

---
CC: Stefano Stabellini 
CC: Julien Grall 
---
Changes in v2:
-Fix commit message
-Save configured VTCR_EL2 value into static variable that will be used
  by non-boot CPUs on hotplug
-Add setup_virt_paging_secondary() and invoke it from start_secondary()
  if that CPU has to setup virtual paging (if the system state is not boot)
---
  xen/arch/arm/p2m.c| 13 -
  xen/arch/arm/smpboot.c|  3 +++
  xen/include/asm-arm/p2m.h |  3 +++
  3 files changed, 18 insertions(+), 1 deletion(-)

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);
  isb();
  }
  
+/* 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 uint*_t).



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



+}
+
  void __init setup_virt_paging(void)
  {
  /* Setup Stage 2 address translation */
@@ -1540,6 +1548,9 @@ void __init setup_virt_paging(void)
  BUG_ON( P2M_ROOT_LEVEL == 0 && P2M_ROOT_ORDER > 0 );
  setup_virt_paging_one((void *)val);
  smp_call_function(setup_virt_paging_one, (void *)val, 1);
+
+/* Save configured value (to be used later for secondary CPUs hotplug) */
+vtcr_value = val;
  }
  
  /*

diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
index 38b665a6d2..abc642804f 100644
--- a/xen/arch/arm/smpboot.c
+++ b/xen/arch/arm/smpboot.c
@@ -358,6 +358,9 @@ void start_secondary(unsigned long boot_phys_offset,
  local_irq_enable();
  local_abort_enable();
  
+if ( system_state != SYS_STATE_boot )

+setup_virt_paging_secondary();
I think this code needs some documentation. So people understand why you 
only call setup_virt_paging_secondary() after boot. But is there any 
reason to not use a notifier (see notify_cpu_starting)? This would avoid 
yet another export.



+
  check_local_cpu_errata();
  
  printk(XENLOG_DEBUG "CPU %u booted.\n", smp_processor_id());

diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index 8823707c17..85b66a1196 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -153,6 +153,9 @@ void p2m_altp2m_check(struct vcpu *v, uint16_t idx)
  /* Second stage paging setup, to be called on all CPUs */
  void setup_virt_paging(void);
  
+/* To be called by secondary CPU on hotplug */

+void setup_virt_paging_secondary(void);
+
  /* Init the datastructures for later use by the p2m code */
  int p2m_init(struct domain *d);
  



Cheers,

--
Julien Grall

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

2018-04-20 Thread Mirela Simonovic 
In existing code the paging for non-boot CPUs is setup only on boot. The
setup is triggered from start_xen() after all CPUs are brought online.
In other words, the initialization of VTCR_EL2 register is done out of the
cpu_up/start_secondary() control flow. However, the cpu_up flow is also used
to hotplug non-boot CPUs on resume from suspend to RAM state, in which case
the virtual paging will not be configured.
With this patch the setting of paging is triggered from start_secondary()
function if the current system state is not boot. This way, the paging
for secondary CPUs will be setup in non-boot scenarios as well, while the
setup in boot scenario remains unchanged.
It is assumed here that after the system completed the boot, CPUs that
execute start_secondary() were booted as well when the Xen itself was
booted. According to this assumption non-boot CPUs will always be compliant
with the VTCR_EL2 value that was selected by Xen on boot.
Currently, these in no mechanism to trigger hotplugging of a CPU. This
will be added with the suspend to RAM support for ARM, where the hotplug
of non-boot CPUs will be triggered via enable_nonboot_cpus() call.

Signed-off-by: Mirela Simonovic 

---
CC: Stefano Stabellini 
CC: Julien Grall 
---
Changes in v2:
-Fix commit message
-Save configured VTCR_EL2 value into static variable that will be used
 by non-boot CPUs on hotplug
-Add setup_virt_paging_secondary() and invoke it from start_secondary()
 if that CPU has to setup virtual paging (if the system state is not boot)
---
 xen/arch/arm/p2m.c| 13 -
 xen/arch/arm/smpboot.c|  3 +++
 xen/include/asm-arm/p2m.h |  3 +++
 3 files changed, 18 insertions(+), 1 deletion(-)

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);
 isb();
 }
 
+/* VTCR value to be configured by all CPUs. Set only once by the boot CPU */
+static unsigned long __read_mostly vtcr_value;
+
+void setup_virt_paging_secondary(void)
+{
+setup_virt_paging_one((void *)vtcr_value);
+}
+
 void __init setup_virt_paging(void)
 {
 /* Setup Stage 2 address translation */
@@ -1540,6 +1548,9 @@ void __init setup_virt_paging(void)
 BUG_ON( P2M_ROOT_LEVEL == 0 && P2M_ROOT_ORDER > 0 );
 setup_virt_paging_one((void *)val);
 smp_call_function(setup_virt_paging_one, (void *)val, 1);
+
+/* Save configured value (to be used later for secondary CPUs hotplug) */
+vtcr_value = val;
 }
 
 /*
diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
index 38b665a6d2..abc642804f 100644
--- a/xen/arch/arm/smpboot.c
+++ b/xen/arch/arm/smpboot.c
@@ -358,6 +358,9 @@ void start_secondary(unsigned long boot_phys_offset,
 local_irq_enable();
 local_abort_enable();
 
+if ( system_state != SYS_STATE_boot )
+setup_virt_paging_secondary();
+
 check_local_cpu_errata();
 
 printk(XENLOG_DEBUG "CPU %u booted.\n", smp_processor_id());
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index 8823707c17..85b66a1196 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -153,6 +153,9 @@ void p2m_altp2m_check(struct vcpu *v, uint16_t idx)
 /* Second stage paging setup, to be called on all CPUs */
 void setup_virt_paging(void);
 
+/* To be called by secondary CPU on hotplug */
+void setup_virt_paging_secondary(void);
+
 /* Init the datastructures for later use by the p2m code */
 int p2m_init(struct domain *d);
 
-- 
2.13.0


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel