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

[Julien Grall]

On 08/05/18 15:28, Mirela Simonovic wrote:

Hi Julien,

On Tue, May 8, 2018 at 4:14 PM, Julien Grall  wrote:



On 07/05/18 15:55, Mirela Simonovic wrote:


Hi Julien,



Hi Mirela,


On Mon, Apr 30, 2018 at 4:47 PM, Julien Grall 
wrote:


On 27/04/18 18:12, Mirela Simonovic wrote:


printk("P2M: %d levels with order-%d root, VTCR 0x%lx\n",
-   4 - P2M_ROOT_LEVEL, P2M_ROOT_ORDER, val);
+   4 - P2M_ROOT_LEVEL, P2M_ROOT_ORDER, vtcr);
  p2m_vmid_allocator_init();
  /* It is not allowed to concatenate a level zero root */
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);
+setup_virt_paging_one(NULL);
+smp_call_function(setup_virt_paging_one, NULL, 1);
+}
+
+static int cpu_virt_paging_callback(
+struct notifier_block *nfb, unsigned long action, void *hcpu)




The indentation looks wrong.



Editor indented this for me and it looks the same as in other places
where a notifier is defined. I did
grep -r "struct notifier_block \*nfb,"
to check. It looks weird but seems correct



Indeed, I am not sure why it is done like that for notifiers. I can't see
any reason to split like that given the first parameter can fit on the first
line without hitting the 80 columns.

So I would much prefer if we follow Xen coding style:

static int cpu_virt_paging_callback(struct notifier_block *nfb,
 unsigned long action,
 void *hcpu);



Please just one more clarification: why did you split line after 2nd
argument? 3rd argument could fit in 80 chars.


Both are valid. I feel having the argument separated here is easier to read.


The only coding style I found is in CODING_STYLE file, which doesn't
specify that much details - I'm just trying to understand where to
find more info in order to avoid coding style-related iterations in
future. Is there any other source specifying coding style for Xen?


Sadly the coding style is not very well formalized. At the moment it is 
more a consensus between all the reviewers. There are work going on to 
fix that by introducing a checkpatch.pl.


Cheers,

--
Julien Grall

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

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

[Mirela Simonovic]

Hi Julien,

On Tue, May 8, 2018 at 4:14 PM, Julien Grall  wrote:
>
>
> On 07/05/18 15:55, Mirela Simonovic wrote:
>>
>> Hi Julien,
>
>
> Hi Mirela,
>
>> On Mon, Apr 30, 2018 at 4:47 PM, Julien Grall 
>> wrote:
>>>
>>> On 27/04/18 18:12, Mirela Simonovic wrote:

printk("P2M: %d levels with order-%d root, VTCR 0x%lx\n",
 -   4 - P2M_ROOT_LEVEL, P2M_ROOT_ORDER, val);
 +   4 - P2M_ROOT_LEVEL, P2M_ROOT_ORDER, vtcr);
  p2m_vmid_allocator_init();
  /* It is not allowed to concatenate a level zero root */
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);
 +setup_virt_paging_one(NULL);
 +smp_call_function(setup_virt_paging_one, NULL, 1);
 +}
 +
 +static int cpu_virt_paging_callback(
 +struct notifier_block *nfb, unsigned long action, void *hcpu)
>>>
>>>
>>>
>>> The indentation looks wrong.
>>>
>>
>> Editor indented this for me and it looks the same as in other places
>> where a notifier is defined. I did
>> grep -r "struct notifier_block \*nfb,"
>> to check. It looks weird but seems correct
>
>
> Indeed, I am not sure why it is done like that for notifiers. I can't see
> any reason to split like that given the first parameter can fit on the first
> line without hitting the 80 columns.
>
> So I would much prefer if we follow Xen coding style:
>
> static int cpu_virt_paging_callback(struct notifier_block *nfb,
> unsigned long action,
> void *hcpu);
>

Please just one more clarification: why did you split line after 2nd
argument? 3rd argument could fit in 80 chars.
The only coding style I found is in CODING_STYLE file, which doesn't
specify that much details - I'm just trying to understand where to
find more info in order to avoid coding style-related iterations in
future. Is there any other source specifying coding style for Xen?

Thanks,
Mirela

> Cheers,
>
> --
> Julien Grall

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

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

[Julien Grall]

On 07/05/18 15:55, Mirela Simonovic wrote:

Hi Julien,


Hi Mirela,


On Mon, Apr 30, 2018 at 4:47 PM, Julien Grall  wrote:

On 27/04/18 18:12, Mirela Simonovic wrote:

   printk("P2M: %d levels with order-%d root, VTCR 0x%lx\n",
-   4 - P2M_ROOT_LEVEL, P2M_ROOT_ORDER, val);
+   4 - P2M_ROOT_LEVEL, P2M_ROOT_ORDER, vtcr);
 p2m_vmid_allocator_init();
 /* It is not allowed to concatenate a level zero root */
   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);
+setup_virt_paging_one(NULL);
+smp_call_function(setup_virt_paging_one, NULL, 1);
+}
+
+static int cpu_virt_paging_callback(
+struct notifier_block *nfb, unsigned long action, void *hcpu)



The indentation looks wrong.



Editor indented this for me and it looks the same as in other places
where a notifier is defined. I did
grep -r "struct notifier_block \*nfb,"
to check. It looks weird but seems correct


Indeed, I am not sure why it is done like that for notifiers. I can't 
see any reason to split like that given the first parameter can fit on 
the first line without hitting the 80 columns.


So I would much prefer if we follow Xen coding style:

static int cpu_virt_paging_callback(struct notifier_block *nfb,
unsigned long action,
void *hcpu);

Cheers,

--
Julien Grall

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

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

[Mirela Simonovic]

Hi Julien,


On Mon, Apr 30, 2018 at 4:47 PM, Julien Grall  wrote:
> Hi Mirela,
>
>
> On 27/04/18 18:12, Mirela Simonovic wrote:
>>
>> In existing code the virtual 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 using cpu starting notifier (notify_cpu_starting() call). The
>> notifier is registered in p2m.c using init call. This has to be done with
>> init call rather than presmp_init because the registered callback depends
>> on vtcr configuration value which is setup after the presmp init calls
>> are executed (do_presmp_initcalls() called from start_xen()). Init calls
>> are executed after initial virtual paging is set up for all CPUs on boot.
>> This ensures that no callback can fire until the vtcr value is calculated
>> by Xen and virtual paging is set up initially for all CPUs. Also, this way
>> the virtual paging 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, there is 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)
>>
>> Changes in v3:
>> -Fix commit message
>> -Remove setup_virt_paging_secondary() and use notifier to setup virtual
>>   paging for non-boot CPU on hotplug.
>> -In setup_virt_paging() use vtcr static variable instead of local val
>> -In setup_virt_paging_one() use vtcr static variable instead of provided
>>   argument
>> ---
>>   xen/arch/arm/p2m.c | 82
>> +-
>>   1 file changed, 62 insertions(+), 20 deletions(-)
>>
>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>> index d43c3aa896..98a1fe6de9 100644
>> --- a/xen/arch/arm/p2m.c
>> +++ b/xen/arch/arm/p2m.c
>> @@ -8,6 +8,8 @@
>>   #include 
>>   #include 
>>   #include 
>> +#include 
>> +#include 
>
>
> Please add them alphabetically.
>
>
>>   #include 
>>   #include 
>>   #include 
>> @@ -1451,24 +1453,17 @@ err:
>>   return page;
>>   }
>>   -static void __init setup_virt_paging_one(void *data)
>> +/* VTCR value to be configured by all CPUs. Set only once by the boot CPU
>> */
>> +static uint64_t __read_mostly vtcr;
>> +
>> +static void setup_virt_paging_one(void *data)
>>   {
>> -unsigned long val = (unsigned long)data;
>> -WRITE_SYSREG32(val, VTCR_EL2);
>> +WRITE_SYSREG32(vtcr, VTCR_EL2);
>>   isb();
>>   }
>> void __init setup_virt_paging(void)
>>   {
>> -/* Setup Stage 2 address translation */
>> -unsigned long val =
>> VTCR_RES1|VTCR_SH0_IS|VTCR_ORGN0_WBWA|VTCR_IRGN0_WBWA;
>> -
>> -#ifdef CONFIG_ARM_32
>> -printk("P2M: 40-bit IPA\n");
>> -p2m_ipa_bits = 40;
>> -val |= VTCR_T0SZ(0x18); /* 40 bit IPA */
>> -val |= VTCR_SL0(0x1); /* P2M starts at first level */
>> -#else /* CONFIG_ARM_64 */
>>   const struct {
>>   unsigned int pabits; /* Physical Address Size */
>>   unsigned int t0sz;   /* Desired T0SZ, minimum in comment */
>> @@ -1491,6 +1486,16 @@ void __init setup_virt_paging(void)
>>   unsigned int pa_range = 0x10; /* Larger than any possible value */
>>   bool vmid_8_bit = false;
>
>
> That's not going to build on arm32 because vmid_8_bit & co are not used.
> While I will not ask you to test the changes on 32-bit board, I would at
> least want each change to be build test it on impacted architecture.
>
> In that particular case, you can just move the initialization of vtcr at
> after the endif because there is nothing that required that to be setup very
> early.
>
>
>>   +/* Setup Stage 2 address translation */
>> +vtcr = VTCR_RES1|VTCR_SH0_IS|VTCR_ORGN0_WBWA|VTCR_IRGN0_WBWA;
>> +
>> +#ifdef CONFIG_ARM_32
>> +printk("P2M: 40-bit IPA\n");
>> +p2m_ipa_bits = 40;
>> +vtcr |= 

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

[Julien Grall]

Hi Mirela,

On 27/04/18 18:12, Mirela Simonovic wrote:

In existing code the virtual 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 using cpu starting notifier (notify_cpu_starting() call). The
notifier is registered in p2m.c using init call. This has to be done with
init call rather than presmp_init because the registered callback depends
on vtcr configuration value which is setup after the presmp init calls
are executed (do_presmp_initcalls() called from start_xen()). Init calls
are executed after initial virtual paging is set up for all CPUs on boot.
This ensures that no callback can fire until the vtcr value is calculated
by Xen and virtual paging is set up initially for all CPUs. Also, this way
the virtual paging 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, there is 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)

Changes in v3:
-Fix commit message
-Remove setup_virt_paging_secondary() and use notifier to setup virtual
  paging for non-boot CPU on hotplug.
-In setup_virt_paging() use vtcr static variable instead of local val
-In setup_virt_paging_one() use vtcr static variable instead of provided
  argument
---
  xen/arch/arm/p2m.c | 82 +-
  1 file changed, 62 insertions(+), 20 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index d43c3aa896..98a1fe6de9 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -8,6 +8,8 @@
  #include 
  #include 
  #include 
+#include 
+#include 


Please add them alphabetically.


  #include 
  #include 
  #include 
@@ -1451,24 +1453,17 @@ err:
  return page;
  }
  
-static void __init setup_virt_paging_one(void *data)

+/* VTCR value to be configured by all CPUs. Set only once by the boot CPU */
+static uint64_t __read_mostly vtcr;
+
+static void setup_virt_paging_one(void *data)
  {
-unsigned long val = (unsigned long)data;
-WRITE_SYSREG32(val, VTCR_EL2);
+WRITE_SYSREG32(vtcr, VTCR_EL2);
  isb();
  }
  
  void __init setup_virt_paging(void)

  {
-/* Setup Stage 2 address translation */
-unsigned long val = VTCR_RES1|VTCR_SH0_IS|VTCR_ORGN0_WBWA|VTCR_IRGN0_WBWA;
-
-#ifdef CONFIG_ARM_32
-printk("P2M: 40-bit IPA\n");
-p2m_ipa_bits = 40;
-val |= VTCR_T0SZ(0x18); /* 40 bit IPA */
-val |= VTCR_SL0(0x1); /* P2M starts at first level */
-#else /* CONFIG_ARM_64 */
  const struct {
  unsigned int pabits; /* Physical Address Size */
  unsigned int t0sz;   /* Desired T0SZ, minimum in comment */
@@ -1491,6 +1486,16 @@ void __init setup_virt_paging(void)
  unsigned int pa_range = 0x10; /* Larger than any possible value */
  bool vmid_8_bit = false;


That's not going to build on arm32 because vmid_8_bit & co are not used. 
While I will not ask you to test the changes on 32-bit board, I would at 
least want each change to be build test it on impacted architecture.


In that particular case, you can just move the initialization of vtcr at 
after the endif because there is nothing that required that to be setup 
very early.


  
+/* Setup Stage 2 address translation */

+vtcr = VTCR_RES1|VTCR_SH0_IS|VTCR_ORGN0_WBWA|VTCR_IRGN0_WBWA;
+
+#ifdef CONFIG_ARM_32
+printk("P2M: 40-bit IPA\n");
+p2m_ipa_bits = 40;
+vtcr |= VTCR_T0SZ(0x18); /* 40 bit IPA */
+vtcr |= VTCR_SL0(0x1); /* P2M starts at first level */
+#else /* CONFIG_ARM_64 */
+
  for_each_online_cpu ( cpu )
  {
  const struct cpuinfo_arm *info = _data[cpu];
@@ -1513,14 +1518,14 @@ void __init setup_virt_paging(void)
  if ( pa_range >= ARRAY_SIZE(pa_range_info) || 
!pa_range_info[pa_range].pabits )
  panic("Unknown encoding of ID_AA64MMFR0_EL1.PARange %x\n", 

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

[Mirela Simonovic]

In existing code the virtual 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 using cpu starting notifier (notify_cpu_starting() call). The
notifier is registered in p2m.c using init call. This has to be done with
init call rather than presmp_init because the registered callback depends
on vtcr configuration value which is setup after the presmp init calls
are executed (do_presmp_initcalls() called from start_xen()). Init calls
are executed after initial virtual paging is set up for all CPUs on boot.
This ensures that no callback can fire until the vtcr value is calculated
by Xen and virtual paging is set up initially for all CPUs. Also, this way
the virtual paging 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, there is 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)

Changes in v3:
-Fix commit message
-Remove setup_virt_paging_secondary() and use notifier to setup virtual
 paging for non-boot CPU on hotplug.
-In setup_virt_paging() use vtcr static variable instead of local val
-In setup_virt_paging_one() use vtcr static variable instead of provided
 argument
---
 xen/arch/arm/p2m.c | 82 +-
 1 file changed, 62 insertions(+), 20 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index d43c3aa896..98a1fe6de9 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -8,6 +8,8 @@
 #include 
 #include 
 #include 
+#include 
+#include 
 #include 
 #include 
 #include 
@@ -1451,24 +1453,17 @@ err:
 return page;
 }
 
-static void __init setup_virt_paging_one(void *data)
+/* VTCR value to be configured by all CPUs. Set only once by the boot CPU */
+static uint64_t __read_mostly vtcr;
+
+static void setup_virt_paging_one(void *data)
 {
-unsigned long val = (unsigned long)data;
-WRITE_SYSREG32(val, VTCR_EL2);
+WRITE_SYSREG32(vtcr, VTCR_EL2);
 isb();
 }
 
 void __init setup_virt_paging(void)
 {
-/* Setup Stage 2 address translation */
-unsigned long val = VTCR_RES1|VTCR_SH0_IS|VTCR_ORGN0_WBWA|VTCR_IRGN0_WBWA;
-
-#ifdef CONFIG_ARM_32
-printk("P2M: 40-bit IPA\n");
-p2m_ipa_bits = 40;
-val |= VTCR_T0SZ(0x18); /* 40 bit IPA */
-val |= VTCR_SL0(0x1); /* P2M starts at first level */
-#else /* CONFIG_ARM_64 */
 const struct {
 unsigned int pabits; /* Physical Address Size */
 unsigned int t0sz;   /* Desired T0SZ, minimum in comment */
@@ -1491,6 +1486,16 @@ void __init setup_virt_paging(void)
 unsigned int pa_range = 0x10; /* Larger than any possible value */
 bool vmid_8_bit = false;
 
+/* Setup Stage 2 address translation */
+vtcr = VTCR_RES1|VTCR_SH0_IS|VTCR_ORGN0_WBWA|VTCR_IRGN0_WBWA;
+
+#ifdef CONFIG_ARM_32
+printk("P2M: 40-bit IPA\n");
+p2m_ipa_bits = 40;
+vtcr |= VTCR_T0SZ(0x18); /* 40 bit IPA */
+vtcr |= VTCR_SL0(0x1); /* P2M starts at first level */
+#else /* CONFIG_ARM_64 */
+
 for_each_online_cpu ( cpu )
 {
 const struct cpuinfo_arm *info = _data[cpu];
@@ -1513,14 +1518,14 @@ void __init setup_virt_paging(void)
 if ( pa_range >= ARRAY_SIZE(pa_range_info) || 
!pa_range_info[pa_range].pabits )
 panic("Unknown encoding of ID_AA64MMFR0_EL1.PARange %x\n", pa_range);
 
-val |= VTCR_PS(pa_range);
-val |= VTCR_TG0_4K;
+vtcr |= VTCR_PS(pa_range);
+vtcr |= VTCR_TG0_4K;
 
 /* Set the VS bit only if 16 bit VMID is supported. */
 if ( MAX_VMID == MAX_VMID_16_BIT )
-val |= VTCR_VS;
-val |= VTCR_SL0(pa_range_info[pa_range].sl0);
-val |= VTCR_T0SZ(pa_range_info[pa_range].t0sz);
+vtcr |= VTCR_VS;
+vtcr |= VTCR_SL0(pa_range_info[pa_range].sl0);
+vtcr |= VTCR_T0SZ(pa_range_info[pa_range].t0sz);
 
 p2m_root_order