* [PATCH v2 01/10] xen/arm64: Added handling of the trapped access to OSLSR register
2018-04-20 12:25 [PATCH v2 00/10] xen/arm64: Suspend preconditions and CPU hotplug fixes Mirela Simonovic
@ 2018-04-20 12:25 ` Mirela Simonovic
2018-04-23 11:13 ` Julien Grall
2018-04-20 12:25 ` [PATCH v2 02/10] xen/arm: Ignore write to GICD_ISACTIVERn registers (vgic-v2) Mirela Simonovic
` (8 subsequent siblings)
9 siblings, 1 reply; 45+ messages in thread
From: Mirela Simonovic @ 2018-04-20 12:25 UTC (permalink / raw)
To: xen-devel; +Cc: edgar.iglesias, julien.grall, sstabellini, Mirela Simonovic
Linux/dom0 accesses OSLSR register when saving CPU context during the
suspend procedure. Xen traps access to this register, but has no handling
for it. Consequently, Xen injects undef exception to linux, causing it to
crash. This patch adds handling of the trapped access to OSLSR as ro/raz.
Signed-off-by: Mirela Simonovic <mirela.simonovic@aggios.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
---
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>
---
Changes in v2:
- Commit message fix (arm64 related change instead of arm)
- Add Stefano's reviewed-by
---
xen/arch/arm/arm64/vsysreg.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/xen/arch/arm/arm64/vsysreg.c b/xen/arch/arm/arm64/vsysreg.c
index c57ac12503..8f80e1735e 100644
--- a/xen/arch/arm/arm64/vsysreg.c
+++ b/xen/arch/arm/arm64/vsysreg.c
@@ -57,13 +57,14 @@ void do_sysreg(struct cpu_user_regs *regs,
* ARMv8 (DDI 0487A.d): D1-1509 Table D1-58
*
* Unhandled:
- * OSLSR_EL1
* DBGPRCR_EL1
*/
case HSR_SYSREG_OSLAR_EL1:
return handle_wo_wi(regs, regidx, hsr.sysreg.read, hsr, 1);
case HSR_SYSREG_OSDLR_EL1:
return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 1);
+ case HSR_SYSREG_OSLSR_EL1:
+ return handle_ro_raz(regs, regidx, hsr.sysreg.read, hsr, 1);
/*
* MDCR_EL2.TDA
--
2.13.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v2 01/10] xen/arm64: Added handling of the trapped access to OSLSR register
2018-04-20 12:25 ` [PATCH v2 01/10] xen/arm64: Added handling of the trapped access to OSLSR register Mirela Simonovic
@ 2018-04-23 11:13 ` Julien Grall
0 siblings, 0 replies; 45+ messages in thread
From: Julien Grall @ 2018-04-23 11:13 UTC (permalink / raw)
To: Mirela Simonovic, xen-devel; +Cc: edgar.iglesias, sstabellini
Hi Mirela,
On 20/04/18 13:25, Mirela Simonovic wrote:
> Linux/dom0 accesses OSLSR register when saving CPU context during the
> suspend procedure. Xen traps access to this register, but has no handling
> for it. Consequently, Xen injects undef exception to linux, causing it to
> crash. This patch adds handling of the trapped access to OSLSR as ro/raz.
>
> Signed-off-by: Mirela Simonovic <mirela.simonovic@aggios.com>
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Acked-by: Julien Grall <julien.grall@arm.com>
Cheers,
>
> ---
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien.grall@arm.com>
> ---
> Changes in v2:
> - Commit message fix (arm64 related change instead of arm)
> - Add Stefano's reviewed-by
> ---
> xen/arch/arm/arm64/vsysreg.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/arm64/vsysreg.c b/xen/arch/arm/arm64/vsysreg.c
> index c57ac12503..8f80e1735e 100644
> --- a/xen/arch/arm/arm64/vsysreg.c
> +++ b/xen/arch/arm/arm64/vsysreg.c
> @@ -57,13 +57,14 @@ void do_sysreg(struct cpu_user_regs *regs,
> * ARMv8 (DDI 0487A.d): D1-1509 Table D1-58
> *
> * Unhandled:
> - * OSLSR_EL1
> * DBGPRCR_EL1
> */
> case HSR_SYSREG_OSLAR_EL1:
> return handle_wo_wi(regs, regidx, hsr.sysreg.read, hsr, 1);
> case HSR_SYSREG_OSDLR_EL1:
> return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 1);
> + case HSR_SYSREG_OSLSR_EL1:
> + return handle_ro_raz(regs, regidx, hsr.sysreg.read, hsr, 1);
>
> /*
> * MDCR_EL2.TDA
>
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v2 02/10] xen/arm: Ignore write to GICD_ISACTIVERn registers (vgic-v2)
2018-04-20 12:25 [PATCH v2 00/10] xen/arm64: Suspend preconditions and CPU hotplug fixes Mirela Simonovic
2018-04-20 12:25 ` [PATCH v2 01/10] xen/arm64: Added handling of the trapped access to OSLSR register Mirela Simonovic
@ 2018-04-20 12:25 ` Mirela Simonovic
2018-04-23 11:15 ` Julien Grall
2018-04-20 12:25 ` [PATCH v2 03/10] xen/arm: Implement CPU_OFF PSCI call (physical interface) Mirela Simonovic
` (7 subsequent siblings)
9 siblings, 1 reply; 45+ messages in thread
From: Mirela Simonovic @ 2018-04-20 12:25 UTC (permalink / raw)
To: xen-devel; +Cc: edgar.iglesias, julien.grall, sstabellini, Mirela Simonovic
Guests attempt to write into these registers on resume (for example Linux).
Without this patch a data abort exception will be raised to the guest.
This patch handles the write access by ignoring it, but only if the value
to be written is zero. This should be fine because reading these registers
is already handled as 'read as zero'.
Signed-off-by: Mirela Simonovic <mirela.simonovic@aggios.com>
---
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>
---
Changes in v2:
- Write should be ignored only if the value to be written is zero
(in v1 the write was ignored regardless of the value)
---
xen/arch/arm/vgic-v2.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
index 646d1f3d12..afd3e89883 100644
--- a/xen/arch/arm/vgic-v2.c
+++ b/xen/arch/arm/vgic-v2.c
@@ -488,7 +488,9 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info,
printk(XENLOG_G_ERR
"%pv: vGICD: unhandled word write %#"PRIregister" to ISACTIVER%d\n",
v, r, gicd_reg - GICD_ISACTIVER);
- return 0;
+ if ( r != 0 )
+ return 0;
+ goto write_ignore_32;
case VRANGE32(GICD_ICACTIVER, GICD_ICACTIVERN):
printk(XENLOG_G_ERR
--
2.13.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v2 02/10] xen/arm: Ignore write to GICD_ISACTIVERn registers (vgic-v2)
2018-04-20 12:25 ` [PATCH v2 02/10] xen/arm: Ignore write to GICD_ISACTIVERn registers (vgic-v2) Mirela Simonovic
@ 2018-04-23 11:15 ` Julien Grall
2018-04-24 11:02 ` Mirela Simonovic
0 siblings, 1 reply; 45+ messages in thread
From: Julien Grall @ 2018-04-23 11:15 UTC (permalink / raw)
To: Mirela Simonovic, xen-devel; +Cc: edgar.iglesias, sstabellini
Hi Mirela,
On 20/04/18 13:25, Mirela Simonovic wrote:
> Guests attempt to write into these registers on resume (for example Linux).
> Without this patch a data abort exception will be raised to the guest.
> This patch handles the write access by ignoring it, but only if the value
> to be written is zero. This should be fine because reading these registers
> is already handled as 'read as zero'.
>
> Signed-off-by: Mirela Simonovic <mirela.simonovic@aggios.com>
>
> ---
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien.grall@arm.com>
> ---
> Changes in v2:
> - Write should be ignored only if the value to be written is zero
> (in v1 the write was ignored regardless of the value)
> ---
> xen/arch/arm/vgic-v2.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
> index 646d1f3d12..afd3e89883 100644
> --- a/xen/arch/arm/vgic-v2.c
> +++ b/xen/arch/arm/vgic-v2.c
> @@ -488,7 +488,9 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info,
> printk(XENLOG_G_ERR
> "%pv: vGICD: unhandled word write %#"PRIregister" to ISACTIVER%d\n",
> v, r, gicd_reg - GICD_ISACTIVER);
> - return 0;
> + if ( r != 0 )
> + return 0;
It would be better to move the check before the printk. So a warning is
avoided when the guest is writing 0.
Cheers,
> + goto write_ignore_32;
>
> case VRANGE32(GICD_ICACTIVER, GICD_ICACTIVERN):
> printk(XENLOG_G_ERR
>
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v2 02/10] xen/arm: Ignore write to GICD_ISACTIVERn registers (vgic-v2)
2018-04-23 11:15 ` Julien Grall
@ 2018-04-24 11:02 ` Mirela Simonovic
2018-04-24 16:12 ` Julien Grall
0 siblings, 1 reply; 45+ messages in thread
From: Mirela Simonovic @ 2018-04-24 11:02 UTC (permalink / raw)
To: Julien Grall; +Cc: Edgar E. Iglesias, Stefano Stabellini, Xen Devel
Hi Julien,
On Mon, Apr 23, 2018 at 1:15 PM, Julien Grall <julien.grall@arm.com> wrote:
> Hi Mirela,
>
>
> On 20/04/18 13:25, Mirela Simonovic wrote:
>>
>> Guests attempt to write into these registers on resume (for example
>> Linux).
>> Without this patch a data abort exception will be raised to the guest.
>> This patch handles the write access by ignoring it, but only if the value
>> to be written is zero. This should be fine because reading these registers
>> is already handled as 'read as zero'.
>>
>> Signed-off-by: Mirela Simonovic <mirela.simonovic@aggios.com>
>>
>> ---
>> CC: Stefano Stabellini <sstabellini@kernel.org>
>> CC: Julien Grall <julien.grall@arm.com>
>> ---
>> Changes in v2:
>> - Write should be ignored only if the value to be written is zero
>> (in v1 the write was ignored regardless of the value)
>> ---
>> xen/arch/arm/vgic-v2.c | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
>> index 646d1f3d12..afd3e89883 100644
>> --- a/xen/arch/arm/vgic-v2.c
>> +++ b/xen/arch/arm/vgic-v2.c
>> @@ -488,7 +488,9 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v,
>> mmio_info_t *info,
>> printk(XENLOG_G_ERR
>> "%pv: vGICD: unhandled word write %#"PRIregister" to
>> ISACTIVER%d\n",
>> v, r, gicd_reg - GICD_ISACTIVER);
>> - return 0;
>> + if ( r != 0 )
>> + return 0;
>
>
> It would be better to move the check before the printk. So a warning is
> avoided when the guest is writing 0.
>
If we want to avoid printing a warning when the guest is writing 0
then the printk needs to be moved within the check. I guess this is
what you meant:
if ( r != 0 )
{
printk(XENLOG_G_ERR
"%pv: vGICD: unhandled word write %#"PRIregister" to ISACTIVER%d\n",
v, r, gicd_reg - GICD_ISACTIVER);
return 0;
}
goto write_ignore_32;
Please note that in the original patch where I ignored the write
regardless of the value I just followed how it is already done for
GICD_ICACTIVER.
For existing GICD_ICACTIVER case there is no check for the value to be
written and there is a warning printed.
Not checking the value seems fine to me but why is then a warning
printed? Should we suppress that print as well?
> Cheers,
>
>> + goto write_ignore_32;
>> case VRANGE32(GICD_ICACTIVER, GICD_ICACTIVERN):
>> printk(XENLOG_G_ERR
>>
>
> Cheers,
>
> --
> Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v2 02/10] xen/arm: Ignore write to GICD_ISACTIVERn registers (vgic-v2)
2018-04-24 11:02 ` Mirela Simonovic
@ 2018-04-24 16:12 ` Julien Grall
2018-04-24 16:45 ` Mirela Simonovic
0 siblings, 1 reply; 45+ messages in thread
From: Julien Grall @ 2018-04-24 16:12 UTC (permalink / raw)
To: Mirela Simonovic; +Cc: Edgar E. Iglesias, Stefano Stabellini, Xen Devel
Hi Mirela,
On 04/24/2018 12:02 PM, Mirela Simonovic wrote:
> Hi Julien,
>
> On Mon, Apr 23, 2018 at 1:15 PM, Julien Grall <julien.grall@arm.com> wrote:
>> Hi Mirela,
>>
>>
>> On 20/04/18 13:25, Mirela Simonovic wrote:
>>>
>>> Guests attempt to write into these registers on resume (for example
>>> Linux).
>>> Without this patch a data abort exception will be raised to the guest.
>>> This patch handles the write access by ignoring it, but only if the value
>>> to be written is zero. This should be fine because reading these registers
>>> is already handled as 'read as zero'.
>>>
>>> Signed-off-by: Mirela Simonovic <mirela.simonovic@aggios.com>
>>>
>>> ---
>>> CC: Stefano Stabellini <sstabellini@kernel.org>
>>> CC: Julien Grall <julien.grall@arm.com>
>>> ---
>>> Changes in v2:
>>> - Write should be ignored only if the value to be written is zero
>>> (in v1 the write was ignored regardless of the value)
>>> ---
>>> xen/arch/arm/vgic-v2.c | 4 +++-
>>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
>>> index 646d1f3d12..afd3e89883 100644
>>> --- a/xen/arch/arm/vgic-v2.c
>>> +++ b/xen/arch/arm/vgic-v2.c
>>> @@ -488,7 +488,9 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v,
>>> mmio_info_t *info,
>>> printk(XENLOG_G_ERR
>>> "%pv: vGICD: unhandled word write %#"PRIregister" to
>>> ISACTIVER%d\n",
>>> v, r, gicd_reg - GICD_ISACTIVER);
>>> - return 0;
>>> + if ( r != 0 )
>>> + return 0;
>>
>>
>> It would be better to move the check before the printk. So a warning is
>> avoided when the guest is writing 0.
>>
>
> If we want to avoid printing a warning when the guest is writing 0
> then the printk needs to be moved within the check. I guess this is
> what you meant:
> if ( r != 0 )
> {
> printk(XENLOG_G_ERR
> "%pv: vGICD: unhandled word write %#"PRIregister" to ISACTIVER%d\n",
> v, r, gicd_reg - GICD_ISACTIVER);
> return 0;
> }
> goto write_ignore_32;
Either that or:
if ( r == 0 )
goto write_ignore_32;
So you don't need to rework the code too much. Both would probably want
some comment in the code.
>
> Please note that in the original patch where I ignored the write
> regardless of the value I just followed how it is already done for
> GICD_ICACTIVER.
> For existing GICD_ICACTIVER case there is no check for the value to be
> written and there is a warning printed.
>
> Not checking the value seems fine to me but why is then a warning
> printed? Should we suppress that print as well?
The way it is done in ICACTIVER is really fragile. The guest may think
the active bit of the interrupt was cleared but this is not the case.
It is not easy to check if the active bit is set in the current vGIC
(should be better in the new vGIC). So it was decided to just ignore it
to make Linux happy. The warning is here to tell the user that some may
not work as expected.
Regarding the ISACTIVER, you know that if the user write 0 none of the
active state of the interrupts will be changed. So it is fine to avoid
printing the warning. However, if there are one bit set then you really
want to warn the user as the hypervisor will not probably handle it.
So we want to keep the warning in both case.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v2 02/10] xen/arm: Ignore write to GICD_ISACTIVERn registers (vgic-v2)
2018-04-24 16:12 ` Julien Grall
@ 2018-04-24 16:45 ` Mirela Simonovic
0 siblings, 0 replies; 45+ messages in thread
From: Mirela Simonovic @ 2018-04-24 16:45 UTC (permalink / raw)
To: Julien Grall; +Cc: Edgar E. Iglesias, Stefano Stabellini, Xen Devel
Hi Julien,
Thank you for the feedback, we have a v3 agreement.
On Tue, Apr 24, 2018 at 6:12 PM, Julien Grall <julien.grall@arm.com> wrote:
> Hi Mirela,
>
>
> On 04/24/2018 12:02 PM, Mirela Simonovic wrote:
>>
>> Hi Julien,
>>
>> On Mon, Apr 23, 2018 at 1:15 PM, Julien Grall <julien.grall@arm.com>
>> wrote:
>>>
>>> Hi Mirela,
>>>
>>>
>>> On 20/04/18 13:25, Mirela Simonovic wrote:
>>>>
>>>>
>>>> Guests attempt to write into these registers on resume (for example
>>>> Linux).
>>>> Without this patch a data abort exception will be raised to the guest.
>>>> This patch handles the write access by ignoring it, but only if the
>>>> value
>>>> to be written is zero. This should be fine because reading these
>>>> registers
>>>> is already handled as 'read as zero'.
>>>>
>>>> Signed-off-by: Mirela Simonovic <mirela.simonovic@aggios.com>
>>>>
>>>> ---
>>>> CC: Stefano Stabellini <sstabellini@kernel.org>
>>>> CC: Julien Grall <julien.grall@arm.com>
>>>> ---
>>>> Changes in v2:
>>>> - Write should be ignored only if the value to be written is zero
>>>> (in v1 the write was ignored regardless of the value)
>>>> ---
>>>> xen/arch/arm/vgic-v2.c | 4 +++-
>>>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
>>>> index 646d1f3d12..afd3e89883 100644
>>>> --- a/xen/arch/arm/vgic-v2.c
>>>> +++ b/xen/arch/arm/vgic-v2.c
>>>> @@ -488,7 +488,9 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v,
>>>> mmio_info_t *info,
>>>> printk(XENLOG_G_ERR
>>>> "%pv: vGICD: unhandled word write %#"PRIregister" to
>>>> ISACTIVER%d\n",
>>>> v, r, gicd_reg - GICD_ISACTIVER);
>>>> - return 0;
>>>> + if ( r != 0 )
>>>> + return 0;
>>>
>>>
>>>
>>> It would be better to move the check before the printk. So a warning is
>>> avoided when the guest is writing 0.
>>>
>>
>> If we want to avoid printing a warning when the guest is writing 0
>> then the printk needs to be moved within the check. I guess this is
>> what you meant:
>> if ( r != 0 )
>> {
>> printk(XENLOG_G_ERR
>> "%pv: vGICD: unhandled word write %#"PRIregister" to
>> ISACTIVER%d\n",
>> v, r, gicd_reg - GICD_ISACTIVER);
>> return 0;
>> }
>> goto write_ignore_32;
>
>
> Either that or:
>
> if ( r == 0 )
> goto write_ignore_32;
>
> So you don't need to rework the code too much. Both would probably want some
> comment in the code.
>
>>
>> Please note that in the original patch where I ignored the write
>> regardless of the value I just followed how it is already done for
>> GICD_ICACTIVER.
>> For existing GICD_ICACTIVER case there is no check for the value to be
>> written and there is a warning printed.
>>
>> Not checking the value seems fine to me but why is then a warning
>> printed? Should we suppress that print as well?
>
>
> The way it is done in ICACTIVER is really fragile. The guest may think the
> active bit of the interrupt was cleared but this is not the case.
>
> It is not easy to check if the active bit is set in the current vGIC (should
> be better in the new vGIC). So it was decided to just ignore it to make
> Linux happy. The warning is here to tell the user that some may not work as
> expected.
>
> Regarding the ISACTIVER, you know that if the user write 0 none of the
> active state of the interrupts will be changed. So it is fine to avoid
> printing the warning. However, if there are one bit set then you really want
> to warn the user as the hypervisor will not probably handle it.
>
> So we want to keep the warning in both case.
>
> Cheers,
>
> --
> Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v2 03/10] xen/arm: Implement CPU_OFF PSCI call (physical interface)
2018-04-20 12:25 [PATCH v2 00/10] xen/arm64: Suspend preconditions and CPU hotplug fixes Mirela Simonovic
2018-04-20 12:25 ` [PATCH v2 01/10] xen/arm64: Added handling of the trapped access to OSLSR register Mirela Simonovic
2018-04-20 12:25 ` [PATCH v2 02/10] xen/arm: Ignore write to GICD_ISACTIVERn registers (vgic-v2) Mirela Simonovic
@ 2018-04-20 12:25 ` Mirela Simonovic
2018-04-23 11:21 ` Julien Grall
2018-04-20 12:25 ` [PATCH v2 04/10] xen/arm: Remove __initdata and __init to enable CPU hotplug Mirela Simonovic
` (6 subsequent siblings)
9 siblings, 1 reply; 45+ messages in thread
From: Mirela Simonovic @ 2018-04-20 12:25 UTC (permalink / raw)
To: xen-devel; +Cc: edgar.iglesias, julien.grall, sstabellini, Mirela Simonovic
During the system suspend to RAM non-boot CPUs will be hotplugged.
This will be triggered via disable_nonboot_cpus() call. When
hotplugged the CPU will end up in an infinite wfi loop in stop_cpu().
This patch adds PSCI CPU_OFF call to the EL3 with the aim to get powered
down the calling CPU during the suspend.
If PSCI CPU_OFF call to the EL3 succeeds it will not return. Otherwise,
when the PSCI CPU_OFF call returns we'll raise panic, because the calling
CPU could be enabled afterwards.
If a CPU executes stop_cpu() when the system is not suspending the
calling CPU will loop in the infinite while/wfi, as it was looping
before this change.
Note that there is no check for PSCI version in PSCI CPU_OFF
implementation because the version will be checked prior to triggering
the system suspend.
Signed-off-by: Mirela Simonovic <mirela.simonovic@aggios.com>
---
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>
---
Changes in v2:
-Issue PSCI CPU_OFF only if the system is suspending
-If PSCI CPU_OFF call fails (unlikely to ever happen) raise panic
-Fixed commit message
---
xen/arch/arm/psci.c | 11 +++++++++++
xen/arch/arm/smpboot.c | 3 +++
xen/include/asm-arm/psci.h | 1 +
3 files changed, 15 insertions(+)
diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c
index 94b616df9b..7f7b0695a3 100644
--- a/xen/arch/arm/psci.c
+++ b/xen/arch/arm/psci.c
@@ -46,6 +46,17 @@ int call_psci_cpu_on(int cpu)
return call_smc(psci_cpu_on_nr, cpu_logical_map(cpu), __pa(init_secondary), 0);
}
+void call_psci_cpu_off(void)
+{
+ int errno;
+
+ /* If successfull the PSCI cpu_off call doesn't return */
+ errno = call_smc(PSCI_0_2_FN32_CPU_OFF, 0, 0, 0);
+ if ( errno )
+ panic("PSCI cpu off failed for CPU%d err=%d\n", get_processor_id(),
+ errno);
+}
+
void call_psci_system_off(void)
{
if ( psci_ver > PSCI_VERSION(0, 1) )
diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
index b2116f0d2d..1ca3d63261 100644
--- a/xen/arch/arm/smpboot.c
+++ b/xen/arch/arm/smpboot.c
@@ -395,6 +395,9 @@ void stop_cpu(void)
/* Make sure the write happens before we sleep forever */
dsb(sy);
isb();
+ if ( system_state == SYS_STATE_suspend )
+ call_psci_cpu_off();
+
while ( 1 )
wfi();
}
diff --git a/xen/include/asm-arm/psci.h b/xen/include/asm-arm/psci.h
index 9ac820e94a..832f77afff 100644
--- a/xen/include/asm-arm/psci.h
+++ b/xen/include/asm-arm/psci.h
@@ -20,6 +20,7 @@ extern uint32_t psci_ver;
int psci_init(void);
int call_psci_cpu_on(int cpu);
+void call_psci_cpu_off(void);
void call_psci_system_off(void);
void call_psci_system_reset(void);
--
2.13.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v2 03/10] xen/arm: Implement CPU_OFF PSCI call (physical interface)
2018-04-20 12:25 ` [PATCH v2 03/10] xen/arm: Implement CPU_OFF PSCI call (physical interface) Mirela Simonovic
@ 2018-04-23 11:21 ` Julien Grall
2018-04-23 17:12 ` Mirela Simonovic
0 siblings, 1 reply; 45+ messages in thread
From: Julien Grall @ 2018-04-23 11:21 UTC (permalink / raw)
To: Mirela Simonovic, xen-devel; +Cc: edgar.iglesias, sstabellini
Hi Mirela,
On 20/04/18 13:25, Mirela Simonovic wrote:
> During the system suspend to RAM non-boot CPUs will be hotplugged.
> This will be triggered via disable_nonboot_cpus() call. When
> hotplugged the CPU will end up in an infinite wfi loop in stop_cpu().
> This patch adds PSCI CPU_OFF call to the EL3 with the aim to get powered
> down the calling CPU during the suspend.
> If PSCI CPU_OFF call to the EL3 succeeds it will not return. Otherwise,
> when the PSCI CPU_OFF call returns we'll raise panic, because the calling
> CPU could be enabled afterwards.
> If a CPU executes stop_cpu() when the system is not suspending the
> calling CPU will loop in the infinite while/wfi, as it was looping
> before this change.
> Note that there is no check for PSCI version in PSCI CPU_OFF
> implementation because the version will be checked prior to triggering
> the system suspend.
Then the code should contain an ASSERT + comment to make it clear on the
interface. But I don't think this is the right way to go (see below).
>
> Signed-off-by: Mirela Simonovic <mirela.simonovic@aggios.com>
>
> ---
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien.grall@arm.com>
> ---
> Changes in v2:
> -Issue PSCI CPU_OFF only if the system is suspending
> -If PSCI CPU_OFF call fails (unlikely to ever happen) raise panic
> -Fixed commit message
> ---
> xen/arch/arm/psci.c | 11 +++++++++++
> xen/arch/arm/smpboot.c | 3 +++
> xen/include/asm-arm/psci.h | 1 +
> 3 files changed, 15 insertions(+)
>
> diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c
> index 94b616df9b..7f7b0695a3 100644
> --- a/xen/arch/arm/psci.c
> +++ b/xen/arch/arm/psci.c
> @@ -46,6 +46,17 @@ int call_psci_cpu_on(int cpu)
> return call_smc(psci_cpu_on_nr, cpu_logical_map(cpu), __pa(init_secondary), 0);
> }
>
> +void call_psci_cpu_off(void)
> +{
> + int errno;
> +
> + /* If successfull the PSCI cpu_off call doesn't return */
> + errno = call_smc(PSCI_0_2_FN32_CPU_OFF, 0, 0, 0);
CPU_OFF will only return on error. So the if is not necessary below.
> + if ( errno )
> + panic("PSCI cpu off failed for CPU%d err=%d\n", get_processor_id(),
> + errno);
> +}
> +
> void call_psci_system_off(void)
> {
> if ( psci_ver > PSCI_VERSION(0, 1) )
> diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
> index b2116f0d2d..1ca3d63261 100644
> --- a/xen/arch/arm/smpboot.c
> +++ b/xen/arch/arm/smpboot.c
> @@ -395,6 +395,9 @@ void stop_cpu(void)
> /* Make sure the write happens before we sleep forever */
> dsb(sy);
> isb();
> + if ( system_state == SYS_STATE_suspend )
I don't think this should be call only on suspend/resume. System
shutdown could also benefit of PSCI CPU off. This is also paving the way
to more use case of turning off a CPU.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v2 03/10] xen/arm: Implement CPU_OFF PSCI call (physical interface)
2018-04-23 11:21 ` Julien Grall
@ 2018-04-23 17:12 ` Mirela Simonovic
2018-04-23 17:15 ` Julien Grall
0 siblings, 1 reply; 45+ messages in thread
From: Mirela Simonovic @ 2018-04-23 17:12 UTC (permalink / raw)
To: Julien Grall; +Cc: Edgar E. Iglesias, Stefano Stabellini, Xen Devel
Hi Julien,
On Mon, Apr 23, 2018 at 1:21 PM, Julien Grall <julien.grall@arm.com> wrote:
> Hi Mirela,
>
> On 20/04/18 13:25, Mirela Simonovic wrote:
>>
>> During the system suspend to RAM non-boot CPUs will be hotplugged.
>> This will be triggered via disable_nonboot_cpus() call. When
>> hotplugged the CPU will end up in an infinite wfi loop in stop_cpu().
>> This patch adds PSCI CPU_OFF call to the EL3 with the aim to get powered
>> down the calling CPU during the suspend.
>> If PSCI CPU_OFF call to the EL3 succeeds it will not return. Otherwise,
>> when the PSCI CPU_OFF call returns we'll raise panic, because the calling
>> CPU could be enabled afterwards.
>> If a CPU executes stop_cpu() when the system is not suspending the
>> calling CPU will loop in the infinite while/wfi, as it was looping
>> before this change.
>> Note that there is no check for PSCI version in PSCI CPU_OFF
>> implementation because the version will be checked prior to triggering
>> the system suspend.
>
>
> Then the code should contain an ASSERT + comment to make it clear on the
> interface. But I don't think this is the right way to go (see below).
>
>
>>
>> Signed-off-by: Mirela Simonovic <mirela.simonovic@aggios.com>
>>
>> ---
>> CC: Stefano Stabellini <sstabellini@kernel.org>
>> CC: Julien Grall <julien.grall@arm.com>
>> ---
>> Changes in v2:
>> -Issue PSCI CPU_OFF only if the system is suspending
>> -If PSCI CPU_OFF call fails (unlikely to ever happen) raise panic
>> -Fixed commit message
>> ---
>> xen/arch/arm/psci.c | 11 +++++++++++
>> xen/arch/arm/smpboot.c | 3 +++
>> xen/include/asm-arm/psci.h | 1 +
>> 3 files changed, 15 insertions(+)
>>
>> diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c
>> index 94b616df9b..7f7b0695a3 100644
>> --- a/xen/arch/arm/psci.c
>> +++ b/xen/arch/arm/psci.c
>> @@ -46,6 +46,17 @@ int call_psci_cpu_on(int cpu)
>> return call_smc(psci_cpu_on_nr, cpu_logical_map(cpu),
>> __pa(init_secondary), 0);
>> }
>> +void call_psci_cpu_off(void)
>> +{
>> + int errno;
>> +
>> + /* If successfull the PSCI cpu_off call doesn't return */
>> + errno = call_smc(PSCI_0_2_FN32_CPU_OFF, 0, 0, 0);
>
>
> CPU_OFF will only return on error. So the if is not necessary below.
>
Correct
>> + if ( errno )
>> + panic("PSCI cpu off failed for CPU%d err=%d\n",
>> get_processor_id(),
>> + errno);
>> +}
>> +
>> void call_psci_system_off(void)
>> {
>> if ( psci_ver > PSCI_VERSION(0, 1) )
>> diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
>> index b2116f0d2d..1ca3d63261 100644
>> --- a/xen/arch/arm/smpboot.c
>> +++ b/xen/arch/arm/smpboot.c
>> @@ -395,6 +395,9 @@ void stop_cpu(void)
>> /* Make sure the write happens before we sleep forever */
>> dsb(sy);
>> isb();
>> + if ( system_state == SYS_STATE_suspend )
>
>
> I don't think this should be call only on suspend/resume. System shutdown
> could also benefit of PSCI CPU off. This is also paving the way to more use
> case of turning off a CPU.
Ok, but then we do need to check for PSCI version here.
Thanks,
Mirela
>
> Cheers,
>
> --
> Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v2 03/10] xen/arm: Implement CPU_OFF PSCI call (physical interface)
2018-04-23 17:12 ` Mirela Simonovic
@ 2018-04-23 17:15 ` Julien Grall
0 siblings, 0 replies; 45+ messages in thread
From: Julien Grall @ 2018-04-23 17:15 UTC (permalink / raw)
To: Mirela Simonovic; +Cc: Edgar E. Iglesias, Stefano Stabellini, Xen Devel
On 23/04/18 18:12, Mirela Simonovic wrote:
> On Mon, Apr 23, 2018 at 1:21 PM, Julien Grall <julien.grall@arm.com> wrote:
>> On 20/04/18 13:25, Mirela Simonovic wrote:
>>> + if ( errno )
>>> + panic("PSCI cpu off failed for CPU%d err=%d\n",
>>> get_processor_id(),
>>> + errno);
>>> +}
>>> +
>>> void call_psci_system_off(void)
>>> {
>>> if ( psci_ver > PSCI_VERSION(0, 1) )
>>> diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
>>> index b2116f0d2d..1ca3d63261 100644
>>> --- a/xen/arch/arm/smpboot.c
>>> +++ b/xen/arch/arm/smpboot.c
>>> @@ -395,6 +395,9 @@ void stop_cpu(void)
>>> /* Make sure the write happens before we sleep forever */
>>> dsb(sy);
>>> isb();
>>> + if ( system_state == SYS_STATE_suspend )
>>
>>
>> I don't think this should be call only on suspend/resume. System shutdown
>> could also benefit of PSCI CPU off. This is also paving the way to more use
>> case of turning off a CPU.
>
> Ok, but then we do need to check for PSCI version here.
Or inside call_psci_off to match the other psci_* functions. However,
you still need to check the PSCI version in the suspend code to deny any
suspend/resume functionally on platform without PSCI CPU off support.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v2 04/10] xen/arm: Remove __initdata and __init to enable CPU hotplug
2018-04-20 12:25 [PATCH v2 00/10] xen/arm64: Suspend preconditions and CPU hotplug fixes Mirela Simonovic
` (2 preceding siblings ...)
2018-04-20 12:25 ` [PATCH v2 03/10] xen/arm: Implement CPU_OFF PSCI call (physical interface) Mirela Simonovic
@ 2018-04-20 12:25 ` Mirela Simonovic
2018-04-23 11:21 ` Julien Grall
2018-04-20 12:25 ` [PATCH v2 05/10] xen/arm: Setup virtual paging for non-boot CPUs on hotplug/resume Mirela Simonovic
` (5 subsequent siblings)
9 siblings, 1 reply; 45+ messages in thread
From: Mirela Simonovic @ 2018-04-20 12:25 UTC (permalink / raw)
To: xen-devel; +Cc: edgar.iglesias, julien.grall, sstabellini, Mirela Simonovic
CPU up flow is currently used during the initial boot to start secondary
CPUs. However, the same flow should be used for CPU hotplug, e.g. when
hotplugging secondary CPUs within the resume procedure (resume from the
suspend to RAM). Therefore, prefixes __initdata and __init had to be removed
from few data structures and functions that are used within the cpu up flow.
Signed-off-by: Mirela Simonovic <mirela.simonovic@aggios.com>
---
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>
---
xen/arch/arm/arm64/smpboot.c | 2 +-
xen/arch/arm/irq.c | 2 +-
xen/arch/arm/processor.c | 2 +-
xen/arch/arm/smpboot.c | 4 ++--
xen/include/asm-arm/procinfo.h | 4 ++--
5 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/xen/arch/arm/arm64/smpboot.c b/xen/arch/arm/arm64/smpboot.c
index 4fd0ac68b7..694fbf67e6 100644
--- a/xen/arch/arm/arm64/smpboot.c
+++ b/xen/arch/arm/arm64/smpboot.c
@@ -104,7 +104,7 @@ int __init arch_cpu_init(int cpu, struct dt_device_node *dn)
return smp_psci_init(cpu);
}
-int __init arch_cpu_up(int cpu)
+int arch_cpu_up(int cpu)
{
if ( !smp_enable_ops[cpu].prepare_cpu )
return -ENODEV;
diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
index aa4e832cae..098281f8ab 100644
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -65,7 +65,7 @@ irq_desc_t *__irq_to_desc(int irq)
return &irq_desc[irq-NR_LOCAL_IRQS];
}
-int __init arch_init_one_irq_desc(struct irq_desc *desc)
+int arch_init_one_irq_desc(struct irq_desc *desc)
{
desc->arch.type = IRQ_TYPE_INVALID;
return 0;
diff --git a/xen/arch/arm/processor.c b/xen/arch/arm/processor.c
index ce4385064a..acad8b31d6 100644
--- a/xen/arch/arm/processor.c
+++ b/xen/arch/arm/processor.c
@@ -20,7 +20,7 @@
static DEFINE_PER_CPU(struct processor *, processor);
-void __init processor_setup(void)
+void processor_setup(void)
{
const struct proc_info_list *procinfo;
diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
index 1ca3d63261..38b665a6d2 100644
--- a/xen/arch/arm/smpboot.c
+++ b/xen/arch/arm/smpboot.c
@@ -52,8 +52,8 @@ nodemask_t __read_mostly node_online_map = { { [0] = 1UL } };
static unsigned char __initdata cpu0_boot_stack[STACK_SIZE]
__attribute__((__aligned__(STACK_SIZE)));
-/* Initial boot cpu data */
-struct init_info __initdata init_data =
+/* Boot cpu data */
+struct init_info init_data =
{
.stack = cpu0_boot_stack,
};
diff --git a/xen/include/asm-arm/procinfo.h b/xen/include/asm-arm/procinfo.h
index 26306b35f8..02be56e348 100644
--- a/xen/include/asm-arm/procinfo.h
+++ b/xen/include/asm-arm/procinfo.h
@@ -35,9 +35,9 @@ struct proc_info_list {
struct processor *processor;
};
-const __init struct proc_info_list *lookup_processor_type(void);
+const struct proc_info_list *lookup_processor_type(void);
-void __init processor_setup(void);
+void processor_setup(void);
void processor_vcpu_initialise(struct vcpu *v);
#endif
--
2.13.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v2 04/10] xen/arm: Remove __initdata and __init to enable CPU hotplug
2018-04-20 12:25 ` [PATCH v2 04/10] xen/arm: Remove __initdata and __init to enable CPU hotplug Mirela Simonovic
@ 2018-04-23 11:21 ` Julien Grall
0 siblings, 0 replies; 45+ messages in thread
From: Julien Grall @ 2018-04-23 11:21 UTC (permalink / raw)
To: Mirela Simonovic, xen-devel; +Cc: edgar.iglesias, sstabellini
Hi Mirela,
On 20/04/18 13:25, Mirela Simonovic wrote:
> CPU up flow is currently used during the initial boot to start secondary
> CPUs. However, the same flow should be used for CPU hotplug, e.g. when
> hotplugging secondary CPUs within the resume procedure (resume from the
> suspend to RAM). Therefore, prefixes __initdata and __init had to be removed
> from few data structures and functions that are used within the cpu up flow.
>
> Signed-off-by: Mirela Simonovic <mirela.simonovic@aggios.com>
Acked-by: Julien Grall <julien.grall@arm.com>
Cheers,
>
> ---
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien.grall@arm.com>
> ---
> xen/arch/arm/arm64/smpboot.c | 2 +-
> xen/arch/arm/irq.c | 2 +-
> xen/arch/arm/processor.c | 2 +-
> xen/arch/arm/smpboot.c | 4 ++--
> xen/include/asm-arm/procinfo.h | 4 ++--
> 5 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/xen/arch/arm/arm64/smpboot.c b/xen/arch/arm/arm64/smpboot.c
> index 4fd0ac68b7..694fbf67e6 100644
> --- a/xen/arch/arm/arm64/smpboot.c
> +++ b/xen/arch/arm/arm64/smpboot.c
> @@ -104,7 +104,7 @@ int __init arch_cpu_init(int cpu, struct dt_device_node *dn)
> return smp_psci_init(cpu);
> }
>
> -int __init arch_cpu_up(int cpu)
> +int arch_cpu_up(int cpu)
> {
> if ( !smp_enable_ops[cpu].prepare_cpu )
> return -ENODEV;
> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> index aa4e832cae..098281f8ab 100644
> --- a/xen/arch/arm/irq.c
> +++ b/xen/arch/arm/irq.c
> @@ -65,7 +65,7 @@ irq_desc_t *__irq_to_desc(int irq)
> return &irq_desc[irq-NR_LOCAL_IRQS];
> }
>
> -int __init arch_init_one_irq_desc(struct irq_desc *desc)
> +int arch_init_one_irq_desc(struct irq_desc *desc)
> {
> desc->arch.type = IRQ_TYPE_INVALID;
> return 0;
> diff --git a/xen/arch/arm/processor.c b/xen/arch/arm/processor.c
> index ce4385064a..acad8b31d6 100644
> --- a/xen/arch/arm/processor.c
> +++ b/xen/arch/arm/processor.c
> @@ -20,7 +20,7 @@
>
> static DEFINE_PER_CPU(struct processor *, processor);
>
> -void __init processor_setup(void)
> +void processor_setup(void)
> {
> const struct proc_info_list *procinfo;
>
> diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
> index 1ca3d63261..38b665a6d2 100644
> --- a/xen/arch/arm/smpboot.c
> +++ b/xen/arch/arm/smpboot.c
> @@ -52,8 +52,8 @@ nodemask_t __read_mostly node_online_map = { { [0] = 1UL } };
> static unsigned char __initdata cpu0_boot_stack[STACK_SIZE]
> __attribute__((__aligned__(STACK_SIZE)));
>
> -/* Initial boot cpu data */
> -struct init_info __initdata init_data =
> +/* Boot cpu data */
> +struct init_info init_data =
> {
> .stack = cpu0_boot_stack,
> };
> diff --git a/xen/include/asm-arm/procinfo.h b/xen/include/asm-arm/procinfo.h
> index 26306b35f8..02be56e348 100644
> --- a/xen/include/asm-arm/procinfo.h
> +++ b/xen/include/asm-arm/procinfo.h
> @@ -35,9 +35,9 @@ struct proc_info_list {
> struct processor *processor;
> };
>
> -const __init struct proc_info_list *lookup_processor_type(void);
> +const struct proc_info_list *lookup_processor_type(void);
>
> -void __init processor_setup(void);
> +void processor_setup(void);
> void processor_vcpu_initialise(struct vcpu *v);
>
> #endif
>
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v2 05/10] xen/arm: Setup virtual paging for non-boot CPUs on hotplug/resume
2018-04-20 12:25 [PATCH v2 00/10] xen/arm64: Suspend preconditions and CPU hotplug fixes Mirela Simonovic
` (3 preceding siblings ...)
2018-04-20 12:25 ` [PATCH v2 04/10] xen/arm: Remove __initdata and __init to enable CPU hotplug Mirela Simonovic
@ 2018-04-20 12:25 ` Mirela Simonovic
2018-04-23 11:28 ` Julien Grall
2018-04-20 12:25 ` [PATCH v2 06/10] xen/common: Restore IRQ affinity when hotplugging a pCPU Mirela Simonovic
` (4 subsequent siblings)
9 siblings, 1 reply; 45+ messages in thread
From: Mirela Simonovic @ 2018-04-20 12:25 UTC (permalink / raw)
To: xen-devel; +Cc: edgar.iglesias, julien.grall, sstabellini, 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 <mirela.simonovic@aggios.com>
---
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>
---
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
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v2 05/10] xen/arm: Setup virtual paging for non-boot CPUs on hotplug/resume
2018-04-20 12:25 ` [PATCH v2 05/10] xen/arm: Setup virtual paging for non-boot CPUs on hotplug/resume Mirela Simonovic
@ 2018-04-23 11:28 ` Julien Grall
2018-04-24 14:50 ` Mirela Simonovic
0 siblings, 1 reply; 45+ messages in thread
From: Julien Grall @ 2018-04-23 11:28 UTC (permalink / raw)
To: Mirela Simonovic, xen-devel; +Cc: edgar.iglesias, sstabellini
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 <mirela.simonovic@aggios.com>
>
> ---
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien.grall@arm.com>
> ---
> 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
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v2 05/10] xen/arm: Setup virtual paging for non-boot CPUs on hotplug/resume
2018-04-23 11:28 ` Julien Grall
@ 2018-04-24 14:50 ` Mirela Simonovic
2018-04-24 16:33 ` Julien Grall
2018-04-25 8:23 ` Julien Grall
0 siblings, 2 replies; 45+ messages in thread
From: Mirela Simonovic @ 2018-04-24 14:50 UTC (permalink / raw)
To: Julien Grall; +Cc: Edgar E. Iglesias, Stefano Stabellini, Xen Devel
Hi Julien,
Thanks for the feedback.
On Mon, Apr 23, 2018 at 1:28 PM, Julien Grall <julien.grall@arm.com> 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 <mirela.simonovic@aggios.com>
>>
>> ---
>> CC: Stefano Stabellini <sstabellini@kernel.org>
>> CC: Julien Grall <julien.grall@arm.com>
>> ---
>> 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 already implemented notifier-based approach so I have no problem
submitting it. However, I'm not sure what you want to trade. Please
lets clarify.
>
>> +
>> 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
Thanks,
Mirela
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v2 05/10] xen/arm: Setup virtual paging for non-boot CPUs on hotplug/resume
2018-04-24 14:50 ` Mirela Simonovic
@ 2018-04-24 16:33 ` Julien Grall
2018-04-25 8:23 ` Julien Grall
1 sibling, 0 replies; 45+ messages in thread
From: Julien Grall @ 2018-04-24 16:33 UTC (permalink / raw)
To: Mirela Simonovic; +Cc: Edgar E. Iglesias, nd, Stefano Stabellini, Xen Devel
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 <julien.grall@arm.com> 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 <mirela.simonovic@aggios.com>
>>>
>>> ---
>>> CC: Stefano Stabellini <sstabellini@kernel.org>
>>> CC: Julien Grall <julien.grall@arm.com>
>>> ---
>>> 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 already implemented notifier-based approach so I have no problem
> submitting it. However, I'm not sure what you want to trade. Please
> lets clarify.
I want to avoid tens of call in the code to every subsystem. This is
much cleaner to go through the notifiers.
The notifiers also have the advantage that it is much easier to see the
pairing between initialization and deinitialization (when existing) as
we will now have one place to look.
Cheers,
>>
>>> +
>>> 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
>
> Thanks,
> Mirela
>
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v2 05/10] xen/arm: Setup virtual paging for non-boot CPUs on hotplug/resume
2018-04-24 14:50 ` Mirela Simonovic
2018-04-24 16:33 ` Julien Grall
@ 2018-04-25 8:23 ` Julien Grall
1 sibling, 0 replies; 45+ messages in thread
From: Julien Grall @ 2018-04-25 8:23 UTC (permalink / raw)
To: Mirela Simonovic; +Cc: Edgar E. Iglesias, Stefano Stabellini, Xen Devel
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 <julien.grall@arm.com> 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
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v2 06/10] xen/common: Restore IRQ affinity when hotplugging a pCPU
2018-04-20 12:25 [PATCH v2 00/10] xen/arm64: Suspend preconditions and CPU hotplug fixes Mirela Simonovic
` (4 preceding siblings ...)
2018-04-20 12:25 ` [PATCH v2 05/10] xen/arm: Setup virtual paging for non-boot CPUs on hotplug/resume Mirela Simonovic
@ 2018-04-20 12:25 ` Mirela Simonovic
2018-04-20 12:25 ` [PATCH v2 07/10] xen/arm: Release maintenance interrupt when CPU is hot-unplugged Mirela Simonovic
` (3 subsequent siblings)
9 siblings, 0 replies; 45+ messages in thread
From: Mirela Simonovic @ 2018-04-20 12:25 UTC (permalink / raw)
To: xen-devel
Cc: edgar.iglesias, sstabellini, George Dunlap, Dario Faggioli,
julien.grall, Mirela Simonovic
Non-boot pCPUs are being hot-unplugged during the system suspend to
RAM and hotplugged during the resume. When non-boot pCPUs are
hot-unplugged the interrupts that were targeted to them are migrated
to the boot pCPU.
On suspend, each guest could have its own wake-up devices/interrupts
(passthrough) that could trigger the system resume. These interrupts
could be targeted to a non-boot pCPU, e.g. if the guest's vCPU is
pinned to a non-boot pCPU. Due to the hot-unplug of non-boot pCPUs
during the suspend such interrupts will be migrated from non-boot pCPUs
to the boot pCPU (this is fine). However, when non-boot pCPUs are
hotplugged on resume, these interrupts are not migrated back to non-boot
pCPUs, i.e. IRQ affinity is not restored on resume (this is wrong).
This patch adds the restoration of IRQ affinity when a pCPU is hotplugged.
Signed-off-by: Mirela Simonovic <mirela.simonovic@aggios.com>
---
CC: George Dunlap <george.dunlap@eu.citrix.com>
CC: Dario Faggioli <dfaggioli@suse.com>
---
Changes in v2:
-Instead of checking whether the affinity was broken check whether
vcpu's processor has changed in order to trigger restoring of the
IRQ affinity
-Fix commit message
---
xen/common/schedule.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/xen/common/schedule.c b/xen/common/schedule.c
index 343ab6306e..c37ef4846d 100644
--- a/xen/common/schedule.c
+++ b/xen/common/schedule.c
@@ -692,6 +692,7 @@ void restore_vcpu_affinity(struct domain *d)
for_each_vcpu ( d, v )
{
spinlock_t *lock;
+ unsigned int old_cpu = v->processor;
ASSERT(!vcpu_runnable(v));
@@ -724,6 +725,9 @@ void restore_vcpu_affinity(struct domain *d)
lock = vcpu_schedule_lock_irq(v);
v->processor = SCHED_OP(vcpu_scheduler(v), pick_cpu, v);
spin_unlock_irq(lock);
+
+ if ( old_cpu != v->processor )
+ sched_move_irqs(v);
}
domain_update_node_affinity(d);
--
2.13.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 45+ messages in thread* [PATCH v2 07/10] xen/arm: Release maintenance interrupt when CPU is hot-unplugged
2018-04-20 12:25 [PATCH v2 00/10] xen/arm64: Suspend preconditions and CPU hotplug fixes Mirela Simonovic
` (5 preceding siblings ...)
2018-04-20 12:25 ` [PATCH v2 06/10] xen/common: Restore IRQ affinity when hotplugging a pCPU Mirela Simonovic
@ 2018-04-20 12:25 ` Mirela Simonovic
2018-04-23 11:33 ` Julien Grall
2018-04-20 12:25 ` [PATCH v2 08/10] xen/arm: Release timer interrupts " Mirela Simonovic
` (2 subsequent siblings)
9 siblings, 1 reply; 45+ messages in thread
From: Mirela Simonovic @ 2018-04-20 12:25 UTC (permalink / raw)
To: xen-devel; +Cc: edgar.iglesias, julien.grall, sstabellini, Mirela Simonovic
When a CPU is hot-unplugged the maintenance interrupt has to be
released in order to free the memory that was allocated when the CPU
was hotplugged and interrupt requested. The interrupt was requested
using request_irq() which is called from start_secondary->
init_maintenance_interrupt.
Signed-off-by: Mirela Simonovic <mirela.simonovic@aggios.com>
---
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>
---
xen/arch/arm/gic.c | 5 +++++
xen/arch/arm/smpboot.c | 7 +++++++
xen/include/asm-arm/gic.h | 1 +
3 files changed, 13 insertions(+)
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 653a815127..e536b99e84 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -431,6 +431,11 @@ void init_maintenance_interrupt(void)
"irq-maintenance", NULL);
}
+void deinit_maintenance_interrupt(void)
+{
+ release_irq(gic_hw_ops->info->maintenance_irq, NULL);
+}
+
int gic_make_hwdom_dt_node(const struct domain *d,
const struct dt_device_node *gic,
void *fdt)
diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
index abc642804f..449fefc77d 100644
--- a/xen/arch/arm/smpboot.c
+++ b/xen/arch/arm/smpboot.c
@@ -375,11 +375,18 @@ void __cpu_disable(void)
local_irq_disable();
gic_disable_cpu();
+
/* Allow any queued timer interrupts to get serviced */
local_irq_enable();
mdelay(1);
local_irq_disable();
+ /*
+ * Deinitialize interrupts (this will free the memory that was allocated
+ * in respective init interrupt functions called from start_secondary)
+ */
+ deinit_maintenance_interrupt();
+
/* It's now safe to remove this processor from the online map */
cpumask_clear_cpu(cpu, &cpu_online_map);
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index 58b910fe6a..0db42e6cce 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -254,6 +254,7 @@ extern void gic_clear_pending_irqs(struct vcpu *v);
extern int vgic_vcpu_pending_irq(struct vcpu *v);
extern void init_maintenance_interrupt(void);
+extern void deinit_maintenance_interrupt(void);
extern void gic_raise_guest_irq(struct vcpu *v, unsigned int irq,
unsigned int priority);
extern void gic_raise_inflight_irq(struct vcpu *v, unsigned int virtual_irq);
--
2.13.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v2 07/10] xen/arm: Release maintenance interrupt when CPU is hot-unplugged
2018-04-20 12:25 ` [PATCH v2 07/10] xen/arm: Release maintenance interrupt when CPU is hot-unplugged Mirela Simonovic
@ 2018-04-23 11:33 ` Julien Grall
2018-04-25 13:09 ` Mirela Simonovic
0 siblings, 1 reply; 45+ messages in thread
From: Julien Grall @ 2018-04-23 11:33 UTC (permalink / raw)
To: Mirela Simonovic, xen-devel; +Cc: edgar.iglesias, sstabellini
Hi Mirela,
On 20/04/18 13:25, Mirela Simonovic wrote:
> When a CPU is hot-unplugged the maintenance interrupt has to be
> released in order to free the memory that was allocated when the CPU
> was hotplugged and interrupt requested. The interrupt was requested
> using request_irq() which is called from start_secondary->
> init_maintenance_interrupt.
>
> Signed-off-by: Mirela Simonovic <mirela.simonovic@aggios.com>
>
> ---
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien.grall@arm.com>
> ---
> xen/arch/arm/gic.c | 5 +++++
> xen/arch/arm/smpboot.c | 7 +++++++
> xen/include/asm-arm/gic.h | 1 +
> 3 files changed, 13 insertions(+)
>
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 653a815127..e536b99e84 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -431,6 +431,11 @@ void init_maintenance_interrupt(void)
> "irq-maintenance", NULL);
> }
>
> +void deinit_maintenance_interrupt(void)
> +{
> + release_irq(gic_hw_ops->info->maintenance_irq, NULL);
> +}
> +
> int gic_make_hwdom_dt_node(const struct domain *d,
> const struct dt_device_node *gic,
> void *fdt)
> diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
> index abc642804f..449fefc77d 100644
> --- a/xen/arch/arm/smpboot.c
> +++ b/xen/arch/arm/smpboot.c
> @@ -375,11 +375,18 @@ void __cpu_disable(void)
>
> local_irq_disable();
> gic_disable_cpu();
> +
Spurious change.
> /* Allow any queued timer interrupts to get serviced */
> local_irq_enable();
> mdelay(1);
> local_irq_disable();
>
> + /*
> + * Deinitialize interrupts (this will free the memory that was allocated
> + * in respective init interrupt functions called from start_secondary)
> + */
> + deinit_maintenance_interrupt();
Can you have a look at using a notifier (see CPU_DIYING)? This would
avoid exporting too much new function.
> +
> /* It's now safe to remove this processor from the online map */
> cpumask_clear_cpu(cpu, &cpu_online_map);
>
> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index 58b910fe6a..0db42e6cce 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -254,6 +254,7 @@ extern void gic_clear_pending_irqs(struct vcpu *v);
> extern int vgic_vcpu_pending_irq(struct vcpu *v);
>
> extern void init_maintenance_interrupt(void);
> +extern void deinit_maintenance_interrupt(void);
> extern void gic_raise_guest_irq(struct vcpu *v, unsigned int irq,
> unsigned int priority);
> extern void gic_raise_inflight_irq(struct vcpu *v, unsigned int virtual_irq);
>
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v2 07/10] xen/arm: Release maintenance interrupt when CPU is hot-unplugged
2018-04-23 11:33 ` Julien Grall
@ 2018-04-25 13:09 ` Mirela Simonovic
2018-04-25 13:23 ` Julien Grall
0 siblings, 1 reply; 45+ messages in thread
From: Mirela Simonovic @ 2018-04-25 13:09 UTC (permalink / raw)
To: Julien Grall; +Cc: Edgar E. Iglesias, Stefano Stabellini, Xen Devel
Hi Julien,
On Mon, Apr 23, 2018 at 1:33 PM, Julien Grall <julien.grall@arm.com> wrote:
> Hi Mirela,
>
>
> On 20/04/18 13:25, Mirela Simonovic wrote:
>>
>> When a CPU is hot-unplugged the maintenance interrupt has to be
>> released in order to free the memory that was allocated when the CPU
>> was hotplugged and interrupt requested. The interrupt was requested
>> using request_irq() which is called from start_secondary->
>> init_maintenance_interrupt.
>>
>> Signed-off-by: Mirela Simonovic <mirela.simonovic@aggios.com>
>>
>> ---
>> CC: Stefano Stabellini <sstabellini@kernel.org>
>> CC: Julien Grall <julien.grall@arm.com>
>> ---
>> xen/arch/arm/gic.c | 5 +++++
>> xen/arch/arm/smpboot.c | 7 +++++++
>> xen/include/asm-arm/gic.h | 1 +
>> 3 files changed, 13 insertions(+)
>>
>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>> index 653a815127..e536b99e84 100644
>> --- a/xen/arch/arm/gic.c
>> +++ b/xen/arch/arm/gic.c
>> @@ -431,6 +431,11 @@ void init_maintenance_interrupt(void)
>> "irq-maintenance", NULL);
>> }
>> +void deinit_maintenance_interrupt(void)
>> +{
>> + release_irq(gic_hw_ops->info->maintenance_irq, NULL);
>> +}
>> +
>> int gic_make_hwdom_dt_node(const struct domain *d,
>> const struct dt_device_node *gic,
>> void *fdt)
>> diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
>> index abc642804f..449fefc77d 100644
>> --- a/xen/arch/arm/smpboot.c
>> +++ b/xen/arch/arm/smpboot.c
>> @@ -375,11 +375,18 @@ void __cpu_disable(void)
>> local_irq_disable();
>> gic_disable_cpu();
>> +
>
>
> Spurious change.
>
>> /* Allow any queued timer interrupts to get serviced */
>> local_irq_enable();
>> mdelay(1);
>> local_irq_disable();
>> + /*
>> + * Deinitialize interrupts (this will free the memory that was
>> allocated
>> + * in respective init interrupt functions called from
>> start_secondary)
>> + */
>> + deinit_maintenance_interrupt();
>
>
> Can you have a look at using a notifier (see CPU_DIYING)? This would avoid
> exporting too much new function.
I believe releasing of maintenance irq should happen after the dying
CPU's GIC interface is disabled.
To make such ordering using notifiers I would need to move these lines
from __cpu_disable into the notifier callback under the CPU_DYING
case:
local_irq_disable();
gic_disable_cpu();
local_irq_enable();
then below these lines in the callback I would add
release_irq(gic_hw_ops->info->maintenance_irq, NULL);
This would have to be done because CPU_DYING notifiers execute before
__cpu_disable().
How that sounds? If it's ok, should these changes be split into 2
patches (1) notifier based call to gic_disable_cpu + 2) release
maintenance irq, I believe this is better) or should I merge them?
Thanks,
Mirela
>
>> +
>> /* It's now safe to remove this processor from the online map */
>> cpumask_clear_cpu(cpu, &cpu_online_map);
>> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
>> index 58b910fe6a..0db42e6cce 100644
>> --- a/xen/include/asm-arm/gic.h
>> +++ b/xen/include/asm-arm/gic.h
>> @@ -254,6 +254,7 @@ extern void gic_clear_pending_irqs(struct vcpu *v);
>> extern int vgic_vcpu_pending_irq(struct vcpu *v);
>> extern void init_maintenance_interrupt(void);
>> +extern void deinit_maintenance_interrupt(void);
>> extern void gic_raise_guest_irq(struct vcpu *v, unsigned int irq,
>> unsigned int priority);
>> extern void gic_raise_inflight_irq(struct vcpu *v, unsigned int
>> virtual_irq);
>>
>
> Cheers,
>
> --
> Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v2 07/10] xen/arm: Release maintenance interrupt when CPU is hot-unplugged
2018-04-25 13:09 ` Mirela Simonovic
@ 2018-04-25 13:23 ` Julien Grall
2018-04-25 14:28 ` Mirela Simonovic
0 siblings, 1 reply; 45+ messages in thread
From: Julien Grall @ 2018-04-25 13:23 UTC (permalink / raw)
To: Mirela Simonovic; +Cc: Edgar E. Iglesias, Stefano Stabellini, Xen Devel
Hi,
On 25/04/18 14:09, Mirela Simonovic wrote:
> On Mon, Apr 23, 2018 at 1:33 PM, Julien Grall <julien.grall@arm.com> wrote:
>> Hi Mirela,
>>
>>
>> On 20/04/18 13:25, Mirela Simonovic wrote:
>>>
>>> When a CPU is hot-unplugged the maintenance interrupt has to be
>>> released in order to free the memory that was allocated when the CPU
>>> was hotplugged and interrupt requested. The interrupt was requested
>>> using request_irq() which is called from start_secondary->
>>> init_maintenance_interrupt.
>>>
>>> Signed-off-by: Mirela Simonovic <mirela.simonovic@aggios.com>
>>>
>>> ---
>>> CC: Stefano Stabellini <sstabellini@kernel.org>
>>> CC: Julien Grall <julien.grall@arm.com>
>>> ---
>>> xen/arch/arm/gic.c | 5 +++++
>>> xen/arch/arm/smpboot.c | 7 +++++++
>>> xen/include/asm-arm/gic.h | 1 +
>>> 3 files changed, 13 insertions(+)
>>>
>>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>>> index 653a815127..e536b99e84 100644
>>> --- a/xen/arch/arm/gic.c
>>> +++ b/xen/arch/arm/gic.c
>>> @@ -431,6 +431,11 @@ void init_maintenance_interrupt(void)
>>> "irq-maintenance", NULL);
>>> }
>>> +void deinit_maintenance_interrupt(void)
>>> +{
>>> + release_irq(gic_hw_ops->info->maintenance_irq, NULL);
>>> +}
>>> +
>>> int gic_make_hwdom_dt_node(const struct domain *d,
>>> const struct dt_device_node *gic,
>>> void *fdt)
>>> diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
>>> index abc642804f..449fefc77d 100644
>>> --- a/xen/arch/arm/smpboot.c
>>> +++ b/xen/arch/arm/smpboot.c
>>> @@ -375,11 +375,18 @@ void __cpu_disable(void)
>>> local_irq_disable();
>>> gic_disable_cpu();
>>> +
>>
>>
>> Spurious change.
>>
>>> /* Allow any queued timer interrupts to get serviced */
>>> local_irq_enable();
>>> mdelay(1);
>>> local_irq_disable();
>>> + /*
>>> + * Deinitialize interrupts (this will free the memory that was
>>> allocated
>>> + * in respective init interrupt functions called from
>>> start_secondary)
>>> + */
>>> + deinit_maintenance_interrupt();
>>
>>
>> Can you have a look at using a notifier (see CPU_DIYING)? This would avoid
>> exporting too much new function.
>
> I believe releasing of maintenance irq should happen after the dying
> CPU's GIC interface is disabled.
Why? The maintenance interrupt will only be fired when running in guest
context. Furthermore, it is initialized after the GIC has been
initialized, so it makes sense to disable before hand.
> To make such ordering using notifiers I would need to move these lines
> from __cpu_disable into the notifier callback under the CPU_DYING
> case:
> local_irq_disable();
> gic_disable_cpu();
> local_irq_enable();
This looks a bit weird. AFAIU, if you disable the CPU interface, then
you should never receive interrupt after. So why would you re-enable them?
I realize the code in __cpu_disbale do that, but this looks quite wrong
to me. There are no way to receive queued timer interrupt afterwards.
> then below these lines in the callback I would add
> release_irq(gic_hw_ops->info->maintenance_irq, NULL);
>
> This would have to be done because CPU_DYING notifiers execute before
> __cpu_disable().
> How that sounds? If it's ok, should these changes be split into 2
> patches (1) notifier based call to gic_disable_cpu + 2) release
> maintenance irq, I believe this is better) or should I merge them?
I am not sure this is right to do. We want to disable the CPU interface
very late (imagine we need to service interrupt).
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v2 07/10] xen/arm: Release maintenance interrupt when CPU is hot-unplugged
2018-04-25 13:23 ` Julien Grall
@ 2018-04-25 14:28 ` Mirela Simonovic
2018-04-26 10:08 ` Julien Grall
0 siblings, 1 reply; 45+ messages in thread
From: Mirela Simonovic @ 2018-04-25 14:28 UTC (permalink / raw)
To: Julien Grall; +Cc: Edgar E. Iglesias, Stefano Stabellini, Xen Devel
Hi Julien,
On Wed, Apr 25, 2018 at 3:23 PM, Julien Grall <julien.grall@arm.com> wrote:
> Hi,
>
>
> On 25/04/18 14:09, Mirela Simonovic wrote:
>>
>> On Mon, Apr 23, 2018 at 1:33 PM, Julien Grall <julien.grall@arm.com>
>> wrote:
>>>
>>> Hi Mirela,
>>>
>>>
>>> On 20/04/18 13:25, Mirela Simonovic wrote:
>>>>
>>>>
>>>> When a CPU is hot-unplugged the maintenance interrupt has to be
>>>> released in order to free the memory that was allocated when the CPU
>>>> was hotplugged and interrupt requested. The interrupt was requested
>>>> using request_irq() which is called from start_secondary->
>>>> init_maintenance_interrupt.
>>>>
>>>> Signed-off-by: Mirela Simonovic <mirela.simonovic@aggios.com>
>>>>
>>>> ---
>>>> CC: Stefano Stabellini <sstabellini@kernel.org>
>>>> CC: Julien Grall <julien.grall@arm.com>
>>>> ---
>>>> xen/arch/arm/gic.c | 5 +++++
>>>> xen/arch/arm/smpboot.c | 7 +++++++
>>>> xen/include/asm-arm/gic.h | 1 +
>>>> 3 files changed, 13 insertions(+)
>>>>
>>>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>>>> index 653a815127..e536b99e84 100644
>>>> --- a/xen/arch/arm/gic.c
>>>> +++ b/xen/arch/arm/gic.c
>>>> @@ -431,6 +431,11 @@ void init_maintenance_interrupt(void)
>>>> "irq-maintenance", NULL);
>>>> }
>>>> +void deinit_maintenance_interrupt(void)
>>>> +{
>>>> + release_irq(gic_hw_ops->info->maintenance_irq, NULL);
>>>> +}
>>>> +
>>>> int gic_make_hwdom_dt_node(const struct domain *d,
>>>> const struct dt_device_node *gic,
>>>> void *fdt)
>>>> diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
>>>> index abc642804f..449fefc77d 100644
>>>> --- a/xen/arch/arm/smpboot.c
>>>> +++ b/xen/arch/arm/smpboot.c
>>>> @@ -375,11 +375,18 @@ void __cpu_disable(void)
>>>> local_irq_disable();
>>>> gic_disable_cpu();
>>>> +
>>>
>>>
>>>
>>> Spurious change.
>>>
>>>> /* Allow any queued timer interrupts to get serviced */
>>>> local_irq_enable();
>>>> mdelay(1);
>>>> local_irq_disable();
>>>> + /*
>>>> + * Deinitialize interrupts (this will free the memory that was
>>>> allocated
>>>> + * in respective init interrupt functions called from
>>>> start_secondary)
>>>> + */
>>>> + deinit_maintenance_interrupt();
>>>
>>>
>>>
>>> Can you have a look at using a notifier (see CPU_DIYING)? This would
>>> avoid
>>> exporting too much new function.
>>
>>
>> I believe releasing of maintenance irq should happen after the dying
>> CPU's GIC interface is disabled.
>
>
> Why? The maintenance interrupt will only be fired when running in guest
> context. Furthermore, it is initialized after the GIC has been initialized,
> so it makes sense to disable before hand.
>
>> To make such ordering using notifiers I would need to move these lines
>> from __cpu_disable into the notifier callback under the CPU_DYING
>> case:
>> local_irq_disable();
>> gic_disable_cpu();
>> local_irq_enable();
>
>
> This looks a bit weird. AFAIU, if you disable the CPU interface, then you
> should never receive interrupt after. So why would you re-enable them?
>
> I realize the code in __cpu_disbale do that, but this looks quite wrong to
> me. There are no way to receive queued timer interrupt afterwards.
>
That is what I took as a reference, but I asked myself the same.
There is (extremely small, but it exists) time window between
disabling irq locally and disabling CPU interface. An interrupt
received in that time window would propagate to the CPU but I'm not
sure would happen after the GIC CPU interface is disabled and
interrupts are locally enabled. That is the only explanation I can
come up with, although I believe the answer is nothing. Since you're
at ARM you could check this internally.
Anyway, since we're taking the notifier approach the timer interrupt
would be disabled before the GIC CPU interface, so I believe the
mdelay and the surrounding local_irq_enable/disable will not be
needed.
Please lets do such a cleanup out of this series.
>> then below these lines in the callback I would add
>> release_irq(gic_hw_ops->info->maintenance_irq, NULL);
>>
>> This would have to be done because CPU_DYING notifiers execute before
>> __cpu_disable().
>> How that sounds? If it's ok, should these changes be split into 2
>> patches (1) notifier based call to gic_disable_cpu + 2) release
>> maintenance irq, I believe this is better) or should I merge them?
>
> I am not sure this is right to do. We want to disable the CPU interface very
> late (imagine we need to service interrupt).
>
This doesn't mean that the gic_disable_cpu can't be done using
notifiers, we would just first disable maintenance irq and then gic
cpu interface.
However, moving gic_disable_cpu in notifier would mean that interrupts
should be disabled using notifiers (whose priority is higher than gic
notifier's priority) as well.
Please lets finalize the discussion and make an agreement on what
should be done, I would like to get v3 ASAP.
Thanks,
Mirela
> Cheers,
>
> --
> Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v2 07/10] xen/arm: Release maintenance interrupt when CPU is hot-unplugged
2018-04-25 14:28 ` Mirela Simonovic
@ 2018-04-26 10:08 ` Julien Grall
2018-04-26 14:23 ` Tim Deegan
0 siblings, 1 reply; 45+ messages in thread
From: Julien Grall @ 2018-04-26 10:08 UTC (permalink / raw)
To: Mirela Simonovic
Cc: Edgar E. Iglesias, Andre Przywara, Stefano Stabellini,
Tim Deegan, Xen Devel
(+ Andre and Tim)
On 25/04/18 15:28, Mirela Simonovic wrote:
> Hi Julien,
Hi,
>
> On Wed, Apr 25, 2018 at 3:23 PM, Julien Grall <julien.grall@arm.com> wrote:
>> On 25/04/18 14:09, Mirela Simonovic wrote:
>>> On Mon, Apr 23, 2018 at 1:33 PM, Julien Grall <julien.grall@arm.com>
>>> wrote:
>>>> On 20/04/18 13:25, Mirela Simonovic wrote:
>> This looks a bit weird. AFAIU, if you disable the CPU interface, then you
>> should never receive interrupt after. So why would you re-enable them?
>>
>> I realize the code in __cpu_disbale do that, but this looks quite wrong to
>> me. There are no way to receive queued timer interrupt afterwards.
>>
>
> That is what I took as a reference, but I asked myself the same.
> There is (extremely small, but it exists) time window between
> disabling irq locally and disabling CPU interface. An interrupt
> received in that time window would propagate to the CPU but I'm not
> sure would happen after the GIC CPU interface is disabled and
> interrupts are locally enabled. That is the only explanation I can
> come up with, although I believe the answer is nothing. Since you're
> at ARM you could check this internally.
Speaking with Andre (in CC), the GIC CPU interface may have forwarded an
interrupt to the processor before it gets disabled. So when the
interrupt will be re-enabled, the processor will jump to the interrupt
exception entry.
However, looking at the spec (4-78 in ARM IHI 0048B.b), Xen will read a
spurious interrupt ID from GICC_IAR. So I am not sure what the point of
that code. It looks like it has been taken from x86, but some bits are
missing.
AFAIU, x86 will only suspend the timer afterwards (see time_suspend). I
am not fully sure why this code is there on Arm. Whether we expect a
timer interrupt to come up. Stefano, Tim, do you have any insight on
that code?
>
> Anyway, since we're taking the notifier approach the timer interrupt
> would be disabled before the GIC CPU interface, so I believe the
> mdelay and the surrounding local_irq_enable/disable will not be
> needed.
> Please lets do such a cleanup out of this series.
Depending on the outcome of the discussions, do you plan to fix the code?
>
>>> then below these lines in the callback I would add
>>> release_irq(gic_hw_ops->info->maintenance_irq, NULL);
>>>
>>> This would have to be done because CPU_DYING notifiers execute before
>>> __cpu_disable().
>>> How that sounds? If it's ok, should these changes be split into 2
>>> patches (1) notifier based call to gic_disable_cpu + 2) release
>>> maintenance irq, I believe this is better) or should I merge them?
>>
>> I am not sure this is right to do. We want to disable the CPU interface very
>> late (imagine we need to service interrupt).
>>
>
> This doesn't mean that the gic_disable_cpu can't be done using
> notifiers, we would just first disable maintenance irq and then gic
> cpu interface.
> However, moving gic_disable_cpu in notifier would mean that interrupts
> should be disabled using notifiers (whose priority is higher than gic
> notifier's priority) as well.
I would prefer to keep the gic_disable_cpu where it is at the moment.
> Please lets finalize the discussion and make an agreement on what
> should be done, I would like to get v3 ASAP.
>
> Thanks,
> Mirela
>
>> Cheers,
>>
>> --
>> Julien Grall
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v2 07/10] xen/arm: Release maintenance interrupt when CPU is hot-unplugged
2018-04-26 10:08 ` Julien Grall
@ 2018-04-26 14:23 ` Tim Deegan
2018-04-27 9:28 ` Julien Grall
0 siblings, 1 reply; 45+ messages in thread
From: Tim Deegan @ 2018-04-26 14:23 UTC (permalink / raw)
To: Julien Grall
Cc: Edgar E. Iglesias, Andre Przywara, Stefano Stabellini,
Mirela Simonovic, Xen Devel
At 11:08 +0100 on 26 Apr (1524740921), Julien Grall wrote:
> >>>> On 20/04/18 13:25, Mirela Simonovic wrote:
> >> This looks a bit weird. AFAIU, if you disable the CPU interface, then you
> >> should never receive interrupt after. So why would you re-enable them?
> >>
> >> I realize the code in __cpu_disbale do that, but this looks quite wrong to
> >> me. There are no way to receive queued timer interrupt afterwards.
> >>
> >
> > That is what I took as a reference, but I asked myself the same.
> > There is (extremely small, but it exists) time window between
> > disabling irq locally and disabling CPU interface. An interrupt
> > received in that time window would propagate to the CPU but I'm not
> > sure would happen after the GIC CPU interface is disabled and
> > interrupts are locally enabled. That is the only explanation I can
> > come up with, although I believe the answer is nothing. Since you're
> > at ARM you could check this internally.
>
> Speaking with Andre (in CC), the GIC CPU interface may have forwarded an
> interrupt to the processor before it gets disabled. So when the
> interrupt will be re-enabled, the processor will jump to the interrupt
> exception entry.
>
> However, looking at the spec (4-78 in ARM IHI 0048B.b), Xen will read a
> spurious interrupt ID from GICC_IAR. So I am not sure what the point of
> that code. It looks like it has been taken from x86, but some bits are
> missing.
>
> AFAIU, x86 will only suspend the timer afterwards (see time_suspend). I
> am not fully sure why this code is there on Arm. Whether we expect a
> timer interrupt to come up. Stefano, Tim, do you have any insight on
> that code?
Sorry, no. I pretty clearly copied this logic from x86, which copied
it directly from Linux at some point in the past. I don't know why
x86 does it this way, and I haven't dived into linux to find out. :)
But draining the outstanding IRQs seems like a polite thing to do if
you're ever going to re-enable this CPU (at least without resetting
it first).
Cheers,
Tim.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v2 07/10] xen/arm: Release maintenance interrupt when CPU is hot-unplugged
2018-04-26 14:23 ` Tim Deegan
@ 2018-04-27 9:28 ` Julien Grall
2018-04-27 14:15 ` Tim Deegan
0 siblings, 1 reply; 45+ messages in thread
From: Julien Grall @ 2018-04-27 9:28 UTC (permalink / raw)
To: Tim Deegan, Julien Grall
Cc: Edgar E. Iglesias, Andre Przywara, Stefano Stabellini,
Mirela Simonovic, Xen Devel
Hi,
Somehow, I didn't receive the e-mail on my Arm account.
On 26/04/18 15:23, Tim Deegan wrote:
> At 11:08 +0100 on 26 Apr (1524740921), Julien Grall wrote:
>>>>>> On 20/04/18 13:25, Mirela Simonovic wrote:
>>>> This looks a bit weird. AFAIU, if you disable the CPU interface, then you
>>>> should never receive interrupt after. So why would you re-enable them?
>>>>
>>>> I realize the code in __cpu_disbale do that, but this looks quite wrong to
>>>> me. There are no way to receive queued timer interrupt afterwards.
>>>>
>>>
>>> That is what I took as a reference, but I asked myself the same.
>>> There is (extremely small, but it exists) time window between
>>> disabling irq locally and disabling CPU interface. An interrupt
>>> received in that time window would propagate to the CPU but I'm not
>>> sure would happen after the GIC CPU interface is disabled and
>>> interrupts are locally enabled. That is the only explanation I can
>>> come up with, although I believe the answer is nothing. Since you're
>>> at ARM you could check this internally.
>>
>> Speaking with Andre (in CC), the GIC CPU interface may have forwarded an
>> interrupt to the processor before it gets disabled. So when the
>> interrupt will be re-enabled, the processor will jump to the interrupt
>> exception entry.
>>
>> However, looking at the spec (4-78 in ARM IHI 0048B.b), Xen will read a
>> spurious interrupt ID from GICC_IAR. So I am not sure what the point of
>> that code. It looks like it has been taken from x86, but some bits are
>> missing.
>>
>> AFAIU, x86 will only suspend the timer afterwards (see time_suspend). I
>> am not fully sure why this code is there on Arm. Whether we expect a
>> timer interrupt to come up. Stefano, Tim, do you have any insight on
>> that code?
>
> Sorry, no. I pretty clearly copied this logic from x86, which copied
> it directly from Linux at some point in the past. I don't know why
> x86 does it this way, and I haven't dived into linux to find out. :)
> But draining the outstanding IRQs seems like a polite thing to do if
> you're ever going to re-enable this CPU (at least without resetting
> it first).
I am not entirely sure what you mean by draining, do you mean they will
serviced by Xen? If so, what kind of interrupts do you expect to be
serviced (e.g PPI, SPIs) ?
AFAIU, you will not be able to read outstanding interrupts as the
GICC_IAR will always return a spurious interrupt when the GIC CPU
interface is disabled. So you will still take the exception if the
interface signaled the processor for an incoming interrupt but all the
interrupts will stay pending.
Clearly, this code does not seem to be doing what we are expecting.
Speaking the Marc Z. (GIC maintainers in Linux), there are no need to
disable the GIC CPU interface in the hypervisor/OS. You are going to
shutdown the CPU and it will be reset when you are coming back.
So I think we should drop the gic_disable_cpu() call. I am not sure yet
what/if we should with something here. I will wait your answer on my
question above.
Mirela, looking at the PSCI spec, it seems you have to migrate away all
the interrupts from the core before powering down (see 5.5.2 in ARM DEN
0022D). So I think you need to ensure that even SPIs are moved away.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v2 07/10] xen/arm: Release maintenance interrupt when CPU is hot-unplugged
2018-04-27 9:28 ` Julien Grall
@ 2018-04-27 14:15 ` Tim Deegan
2018-04-27 14:38 ` Mirela Simonovic
0 siblings, 1 reply; 45+ messages in thread
From: Tim Deegan @ 2018-04-27 14:15 UTC (permalink / raw)
To: Julien Grall
Cc: Edgar E. Iglesias, Andre Przywara, Stefano Stabellini,
Mirela Simonovic, Xen Devel
Hi,
At 10:28 +0100 on 27 Apr (1524824906), Julien Grall wrote:
> On 26/04/18 15:23, Tim Deegan wrote:
> > At 11:08 +0100 on 26 Apr (1524740921), Julien Grall wrote:
> >>>>>> On 20/04/18 13:25, Mirela Simonovic wrote:
> >>>> This looks a bit weird. AFAIU, if you disable the CPU interface, then you
> >>>> should never receive interrupt after. So why would you re-enable them?
> >>>>
> >>>> I realize the code in __cpu_disbale do that, but this looks quite wrong to
> >>>> me. There are no way to receive queued timer interrupt afterwards.
> >>>>
> >>>
> >>> That is what I took as a reference, but I asked myself the same.
> >>> There is (extremely small, but it exists) time window between
> >>> disabling irq locally and disabling CPU interface. An interrupt
> >>> received in that time window would propagate to the CPU but I'm not
> >>> sure would happen after the GIC CPU interface is disabled and
> >>> interrupts are locally enabled. That is the only explanation I can
> >>> come up with, although I believe the answer is nothing. Since you're
> >>> at ARM you could check this internally.
> >>
> >> Speaking with Andre (in CC), the GIC CPU interface may have forwarded an
> >> interrupt to the processor before it gets disabled. So when the
> >> interrupt will be re-enabled, the processor will jump to the interrupt
> >> exception entry.
> >>
> >> However, looking at the spec (4-78 in ARM IHI 0048B.b), Xen will read a
> >> spurious interrupt ID from GICC_IAR. So I am not sure what the point of
> >> that code. It looks like it has been taken from x86, but some bits are
> >> missing.
> >>
> >> AFAIU, x86 will only suspend the timer afterwards (see time_suspend). I
> >> am not fully sure why this code is there on Arm. Whether we expect a
> >> timer interrupt to come up. Stefano, Tim, do you have any insight on
> >> that code?
> >
> > Sorry, no. I pretty clearly copied this logic from x86, which copied
> > it directly from Linux at some point in the past. I don't know why
> > x86 does it this way, and I haven't dived into linux to find out. :)
> > But draining the outstanding IRQs seems like a polite thing to do if
> > you're ever going to re-enable this CPU (at least without resetting
> > it first).
>
> I am not entirely sure what you mean by draining, do you mean they will
> serviced by Xen? If so, what kind of interrupts do you expect to be
> serviced (e.g PPI, SPIs) ?
All I mean is, when you disable the GICC (or APIC, or whatever), you
know that it won't send any more interrupts to the CPU. But you would
like to also be certain that any interrupts it already sent to the CPU
get processed now. Otherwise, if you bring the CPU up again later
that interrupt could still be there. Better to get it out of the way
now, right?
AIUI that's what x86 is doing by re-enabling interrupts and waiting a
bit, which seems a bit crude but OK. ARM could maybe do the same
thing by disabling GICC, dsb(), then disable interrupts. But I don't
understand the interface between GICD, GICC and CPU well enough to
reason about it properly.
It's also possible that there's some subtlety of the timer interrupt
handling that I don't know about -- I _think_ that the reason timer
interrupts are relevant is that they're generated inside the APIC,
so that even when no interrupts are routed to the core, the APIC could
still generate one as it's being shut down.
> Clearly, this code does not seem to be doing what we are expecting.
> Speaking the Marc Z. (GIC maintainers in Linux), there are no need to
> disable the GIC CPU interface in the hypervisor/OS. You are going to
> shutdown the CPU and it will be reset when you are coming back.
Well that answers my question, then. If you know you're going to
reset the core (and not just put it in a deep sleep and wake it up
again) then I think this is all moot and you can just disable
interrupts once.
Cheers,
Tim.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v2 07/10] xen/arm: Release maintenance interrupt when CPU is hot-unplugged
2018-04-27 14:15 ` Tim Deegan
@ 2018-04-27 14:38 ` Mirela Simonovic
2018-04-27 15:12 ` Julien Grall
0 siblings, 1 reply; 45+ messages in thread
From: Mirela Simonovic @ 2018-04-27 14:38 UTC (permalink / raw)
To: Tim Deegan
Cc: Edgar E. Iglesias, Andre Przywara, Julien Grall,
Stefano Stabellini, Xen Devel
Hi,
On Fri, Apr 27, 2018 at 4:15 PM, Tim Deegan <tim@xen.org> wrote:
> Hi,
>
> At 10:28 +0100 on 27 Apr (1524824906), Julien Grall wrote:
>> On 26/04/18 15:23, Tim Deegan wrote:
>> > At 11:08 +0100 on 26 Apr (1524740921), Julien Grall wrote:
>> >>>>>> On 20/04/18 13:25, Mirela Simonovic wrote:
>> >>>> This looks a bit weird. AFAIU, if you disable the CPU interface, then you
>> >>>> should never receive interrupt after. So why would you re-enable them?
>> >>>>
>> >>>> I realize the code in __cpu_disbale do that, but this looks quite wrong to
>> >>>> me. There are no way to receive queued timer interrupt afterwards.
>> >>>>
>> >>>
>> >>> That is what I took as a reference, but I asked myself the same.
>> >>> There is (extremely small, but it exists) time window between
>> >>> disabling irq locally and disabling CPU interface. An interrupt
>> >>> received in that time window would propagate to the CPU but I'm not
>> >>> sure would happen after the GIC CPU interface is disabled and
>> >>> interrupts are locally enabled. That is the only explanation I can
>> >>> come up with, although I believe the answer is nothing. Since you're
>> >>> at ARM you could check this internally.
>> >>
>> >> Speaking with Andre (in CC), the GIC CPU interface may have forwarded an
>> >> interrupt to the processor before it gets disabled. So when the
>> >> interrupt will be re-enabled, the processor will jump to the interrupt
>> >> exception entry.
>> >>
>> >> However, looking at the spec (4-78 in ARM IHI 0048B.b), Xen will read a
>> >> spurious interrupt ID from GICC_IAR. So I am not sure what the point of
>> >> that code. It looks like it has been taken from x86, but some bits are
>> >> missing.
>> >>
>> >> AFAIU, x86 will only suspend the timer afterwards (see time_suspend). I
>> >> am not fully sure why this code is there on Arm. Whether we expect a
>> >> timer interrupt to come up. Stefano, Tim, do you have any insight on
>> >> that code?
>> >
>> > Sorry, no. I pretty clearly copied this logic from x86, which copied
>> > it directly from Linux at some point in the past. I don't know why
>> > x86 does it this way, and I haven't dived into linux to find out. :)
>> > But draining the outstanding IRQs seems like a polite thing to do if
>> > you're ever going to re-enable this CPU (at least without resetting
>> > it first).
>>
>> I am not entirely sure what you mean by draining, do you mean they will
>> serviced by Xen? If so, what kind of interrupts do you expect to be
>> serviced (e.g PPI, SPIs) ?
>
> All I mean is, when you disable the GICC (or APIC, or whatever), you
> know that it won't send any more interrupts to the CPU. But you would
> like to also be certain that any interrupts it already sent to the CPU
> get processed now. Otherwise, if you bring the CPU up again later
> that interrupt could still be there. Better to get it out of the way
> now, right?
>
> AIUI that's what x86 is doing by re-enabling interrupts and waiting a
> bit, which seems a bit crude but OK. ARM could maybe do the same
> thing by disabling GICC, dsb(), then disable interrupts. But I don't
> understand the interface between GICD, GICC and CPU well enough to
> reason about it properly.
>
> It's also possible that there's some subtlety of the timer interrupt
> handling that I don't know about -- I _think_ that the reason timer
> interrupts are relevant is that they're generated inside the APIC,
> so that even when no interrupts are routed to the core, the APIC could
> still generate one as it's being shut down.
>
>> Clearly, this code does not seem to be doing what we are expecting.
>> Speaking the Marc Z. (GIC maintainers in Linux), there are no need to
>> disable the GIC CPU interface in the hypervisor/OS. You are going to
>> shutdown the CPU and it will be reset when you are coming back.
>
I don't think this assumption is guaranteed to be correct. In current
implementation of ATF and if no additional security software runs the
assumption would likely be correct, but it wouldn't be correct in
general. Linux does disable GICC in such a scenario.
So changing this could cause problems in some scenarios, while keeping
it makes no harm.
Thanks for the feedback, I really appreciate it.
> Well that answers my question, then. If you know you're going to
> reset the core (and not just put it in a deep sleep and wake it up
> again) then I think this is all moot and you can just disable
> interrupts once.
>
> Cheers,
>
> Tim.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v2 07/10] xen/arm: Release maintenance interrupt when CPU is hot-unplugged
2018-04-27 14:38 ` Mirela Simonovic
@ 2018-04-27 15:12 ` Julien Grall
2018-05-09 10:10 ` Mirela Simonovic
0 siblings, 1 reply; 45+ messages in thread
From: Julien Grall @ 2018-04-27 15:12 UTC (permalink / raw)
To: Mirela Simonovic, Tim Deegan
Cc: Edgar E. Iglesias, Andre Przywara, Stefano Stabellini, Xen Devel
On 27/04/18 15:38, Mirela Simonovic wrote:
> Hi,
>
> On Fri, Apr 27, 2018 at 4:15 PM, Tim Deegan <tim@xen.org> wrote:
>> Hi,
>>
>> At 10:28 +0100 on 27 Apr (1524824906), Julien Grall wrote:
>>> On 26/04/18 15:23, Tim Deegan wrote:
>>>> At 11:08 +0100 on 26 Apr (1524740921), Julien Grall wrote:
>>>>>>>>> On 20/04/18 13:25, Mirela Simonovic wrote:
>>>>>>> This looks a bit weird. AFAIU, if you disable the CPU interface, then you
>>>>>>> should never receive interrupt after. So why would you re-enable them?
>>>>>>>
>>>>>>> I realize the code in __cpu_disbale do that, but this looks quite wrong to
>>>>>>> me. There are no way to receive queued timer interrupt afterwards.
>>>>>>>
>>>>>>
>>>>>> That is what I took as a reference, but I asked myself the same.
>>>>>> There is (extremely small, but it exists) time window between
>>>>>> disabling irq locally and disabling CPU interface. An interrupt
>>>>>> received in that time window would propagate to the CPU but I'm not
>>>>>> sure would happen after the GIC CPU interface is disabled and
>>>>>> interrupts are locally enabled. That is the only explanation I can
>>>>>> come up with, although I believe the answer is nothing. Since you're
>>>>>> at ARM you could check this internally.
>>>>>
>>>>> Speaking with Andre (in CC), the GIC CPU interface may have forwarded an
>>>>> interrupt to the processor before it gets disabled. So when the
>>>>> interrupt will be re-enabled, the processor will jump to the interrupt
>>>>> exception entry.
>>>>>
>>>>> However, looking at the spec (4-78 in ARM IHI 0048B.b), Xen will read a
>>>>> spurious interrupt ID from GICC_IAR. So I am not sure what the point of
>>>>> that code. It looks like it has been taken from x86, but some bits are
>>>>> missing.
>>>>>
>>>>> AFAIU, x86 will only suspend the timer afterwards (see time_suspend). I
>>>>> am not fully sure why this code is there on Arm. Whether we expect a
>>>>> timer interrupt to come up. Stefano, Tim, do you have any insight on
>>>>> that code?
>>>>
>>>> Sorry, no. I pretty clearly copied this logic from x86, which copied
>>>> it directly from Linux at some point in the past. I don't know why
>>>> x86 does it this way, and I haven't dived into linux to find out. :)
>>>> But draining the outstanding IRQs seems like a polite thing to do if
>>>> you're ever going to re-enable this CPU (at least without resetting
>>>> it first).
>>>
>>> I am not entirely sure what you mean by draining, do you mean they will
>>> serviced by Xen? If so, what kind of interrupts do you expect to be
>>> serviced (e.g PPI, SPIs) ?
>>
>> All I mean is, when you disable the GICC (or APIC, or whatever), you
>> know that it won't send any more interrupts to the CPU. But you would
>> like to also be certain that any interrupts it already sent to the CPU
>> get processed now. Otherwise, if you bring the CPU up again later
>> that interrupt could still be there. Better to get it out of the way
>> now, right?
>>
>> AIUI that's what x86 is doing by re-enabling interrupts and waiting a
>> bit, which seems a bit crude but OK. ARM could maybe do the same
>> thing by disabling GICC, dsb(), then disable interrupts. But I don't
>> understand the interface between GICD, GICC and CPU well enough to
>> reason about it properly.
>>
>> It's also possible that there's some subtlety of the timer interrupt
>> handling that I don't know about -- I _think_ that the reason timer
>> interrupts are relevant is that they're generated inside the APIC,
>> so that even when no interrupts are routed to the core, the APIC could
>> still generate one as it's being shut down.
>>
>>> Clearly, this code does not seem to be doing what we are expecting.
>>> Speaking the Marc Z. (GIC maintainers in Linux), there are no need to
>>> disable the GIC CPU interface in the hypervisor/OS. You are going to
>>> shutdown the CPU and it will be reset when you are coming back.
>>
>
> I don't think this assumption is guaranteed to be correct. In current
> implementation of ATF and if no additional security software runs the
> assumption would likely be correct, but it wouldn't be correct in
> general. Linux does disable GICC in such a scenario.
Can you please give a pointer to code in Linux? The only place I see the
GICC disabled is when using versatile TC2. It looks like it is just
because they don't have PSCI support so Linux has to do the power
management. That's not a platform we are ever going to fully support in Xen.
> So changing this could cause problems in some scenarios, while keeping
> it makes no harm.
While I guess this code makes no harm, it does not do what is expected
(i.e draining the interrupt). I can't see any reason to keep wrong code,
we should really aim to have code that match the architecture. And
better to fix it when we discover the problem rather than waiting until
we rediscovered it later.
So at least a patch to update the code/comment should be done.
> Thanks for the feedback, I really appreciate it.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v2 07/10] xen/arm: Release maintenance interrupt when CPU is hot-unplugged
2018-04-27 15:12 ` Julien Grall
@ 2018-05-09 10:10 ` Mirela Simonovic
2018-05-09 11:01 ` Julien Grall
0 siblings, 1 reply; 45+ messages in thread
From: Mirela Simonovic @ 2018-05-09 10:10 UTC (permalink / raw)
To: Julien Grall
Cc: Edgar E. Iglesias, Andre Przywara, Stefano Stabellini,
Tim Deegan, Xen Devel
Hi Julien,
On Fri, Apr 27, 2018 at 5:12 PM, Julien Grall <julien.grall@arm.com> wrote:
>
>
> On 27/04/18 15:38, Mirela Simonovic wrote:
>>
>> Hi,
>>
>> On Fri, Apr 27, 2018 at 4:15 PM, Tim Deegan <tim@xen.org> wrote:
>>>
>>> Hi,
>>>
>>> At 10:28 +0100 on 27 Apr (1524824906), Julien Grall wrote:
>>>>
>>>> On 26/04/18 15:23, Tim Deegan wrote:
>>>>>
>>>>> At 11:08 +0100 on 26 Apr (1524740921), Julien Grall wrote:
>>>>>>>>>>
>>>>>>>>>> On 20/04/18 13:25, Mirela Simonovic wrote:
>>>>>>>>
>>>>>>>> This looks a bit weird. AFAIU, if you disable the CPU interface,
>>>>>>>> then you
>>>>>>>> should never receive interrupt after. So why would you re-enable
>>>>>>>> them?
>>>>>>>>
>>>>>>>> I realize the code in __cpu_disbale do that, but this looks quite
>>>>>>>> wrong to
>>>>>>>> me. There are no way to receive queued timer interrupt afterwards.
>>>>>>>>
>>>>>>>
>>>>>>> That is what I took as a reference, but I asked myself the same.
>>>>>>> There is (extremely small, but it exists) time window between
>>>>>>> disabling irq locally and disabling CPU interface. An interrupt
>>>>>>> received in that time window would propagate to the CPU but I'm not
>>>>>>> sure would happen after the GIC CPU interface is disabled and
>>>>>>> interrupts are locally enabled. That is the only explanation I can
>>>>>>> come up with, although I believe the answer is nothing. Since you're
>>>>>>> at ARM you could check this internally.
>>>>>>
>>>>>>
>>>>>> Speaking with Andre (in CC), the GIC CPU interface may have forwarded
>>>>>> an
>>>>>> interrupt to the processor before it gets disabled. So when the
>>>>>> interrupt will be re-enabled, the processor will jump to the interrupt
>>>>>> exception entry.
>>>>>>
>>>>>> However, looking at the spec (4-78 in ARM IHI 0048B.b), Xen will read
>>>>>> a
>>>>>> spurious interrupt ID from GICC_IAR. So I am not sure what the point
>>>>>> of
>>>>>> that code. It looks like it has been taken from x86, but some bits are
>>>>>> missing.
>>>>>>
>>>>>> AFAIU, x86 will only suspend the timer afterwards (see time_suspend).
>>>>>> I
>>>>>> am not fully sure why this code is there on Arm. Whether we expect a
>>>>>> timer interrupt to come up. Stefano, Tim, do you have any insight on
>>>>>> that code?
>>>>>
>>>>>
>>>>> Sorry, no. I pretty clearly copied this logic from x86, which copied
>>>>> it directly from Linux at some point in the past. I don't know why
>>>>> x86 does it this way, and I haven't dived into linux to find out. :)
>>>>> But draining the outstanding IRQs seems like a polite thing to do if
>>>>> you're ever going to re-enable this CPU (at least without resetting
>>>>> it first).
>>>>
>>>>
>>>> I am not entirely sure what you mean by draining, do you mean they will
>>>> serviced by Xen? If so, what kind of interrupts do you expect to be
>>>> serviced (e.g PPI, SPIs) ?
>>>
>>>
>>> All I mean is, when you disable the GICC (or APIC, or whatever), you
>>> know that it won't send any more interrupts to the CPU. But you would
>>> like to also be certain that any interrupts it already sent to the CPU
>>> get processed now. Otherwise, if you bring the CPU up again later
>>> that interrupt could still be there. Better to get it out of the way
>>> now, right?
>>>
>>> AIUI that's what x86 is doing by re-enabling interrupts and waiting a
>>> bit, which seems a bit crude but OK. ARM could maybe do the same
>>> thing by disabling GICC, dsb(), then disable interrupts. But I don't
>>> understand the interface between GICD, GICC and CPU well enough to
>>> reason about it properly.
>>>
>>> It's also possible that there's some subtlety of the timer interrupt
>>> handling that I don't know about -- I _think_ that the reason timer
>>> interrupts are relevant is that they're generated inside the APIC,
>>> so that even when no interrupts are routed to the core, the APIC could
>>> still generate one as it's being shut down.
>>>
>>>> Clearly, this code does not seem to be doing what we are expecting.
>>>> Speaking the Marc Z. (GIC maintainers in Linux), there are no need to
>>>> disable the GIC CPU interface in the hypervisor/OS. You are going to
>>>> shutdown the CPU and it will be reset when you are coming back.
>>>
>>>
>>
>> I don't think this assumption is guaranteed to be correct. In current
>> implementation of ATF and if no additional security software runs the
>> assumption would likely be correct, but it wouldn't be correct in
>> general. Linux does disable GICC in such a scenario.
>
> Can you please give a pointer to code in Linux? The only place I see the
> GICC disabled is when using versatile TC2. It looks like it is just because
> they don't have PSCI support so Linux has to do the power management. That's
> not a platform we are ever going to fully support in Xen.
>
You already found it. Good to clarify, thanks.
>> So changing this could cause problems in some scenarios, while keeping
>> it makes no harm.
>
>
> While I guess this code makes no harm, it does not do what is expected (i.e
> draining the interrupt). I can't see any reason to keep wrong code, we
> should really aim to have code that match the architecture. And better to
> fix it when we discover the problem rather than waiting until we
> rediscovered it later.
>
> So at least a patch to update the code/comment should be done.
>
I don't feel comfortable removing these 3 lines because I have no way
to test and guarantee that the change will not introduce any issues.
However, if despite all you really want me to remove these lines
within this series I don't have a problem doing that in a separate
patch. Please just confirm the plan.
Thanks,
Mirela
>> Thanks for the feedback, I really appreciate it.
>
>
> Cheers,
>
> --
> Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v2 07/10] xen/arm: Release maintenance interrupt when CPU is hot-unplugged
2018-05-09 10:10 ` Mirela Simonovic
@ 2018-05-09 11:01 ` Julien Grall
2018-05-09 11:14 ` Mirela Simonovic
0 siblings, 1 reply; 45+ messages in thread
From: Julien Grall @ 2018-05-09 11:01 UTC (permalink / raw)
To: Mirela Simonovic
Cc: Edgar E. Iglesias, Andre Przywara, Stefano Stabellini,
Tim Deegan, Xen Devel
On 09/05/18 11:10, Mirela Simonovic wrote:
> On Fri, Apr 27, 2018 at 5:12 PM, Julien Grall <julien.grall@arm.com> wrote:
>> On 27/04/18 15:38, Mirela Simonovic wrote:
>>> On Fri, Apr 27, 2018 at 4:15 PM, Tim Deegan <tim@xen.org> wrote:
>>>> At 10:28 +0100 on 27 Apr (1524824906), Julien Grall wrote:
>>>>> On 26/04/18 15:23, Tim Deegan wrote:
>>>>>> At 11:08 +0100 on 26 Apr (1524740921), Julien Grall wrote:
>>>>>>>>>>> On 20/04/18 13:25, Mirela Simonovic wrote:
>> While I guess this code makes no harm, it does not do what is expected (i.e
>> draining the interrupt). I can't see any reason to keep wrong code, we
>> should really aim to have code that match the architecture. And better to
>> fix it when we discover the problem rather than waiting until we
>> rediscovered it later.
>>
>> So at least a patch to update the code/comment should be done.
>>
>
> I don't feel comfortable removing these 3 lines because I have no way
> to test and guarantee that the change will not introduce any issues.
The work you are doing (suspend/resume, hotplug) is not easy to test and
very subtle to get it right. Testing can only uncover obvious bug on
your platform. IMHO, we can only rely on the specifications (ARM ARM,
PSCI...) and extensive review of the series (and the code around).
> However, if despite all you really want me to remove these lines
> within this series I don't have a problem doing that in a separate
> patch. Please just confirm the plan.
I am quite confident that this code should not be there or at least not
in its current form.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v2 07/10] xen/arm: Release maintenance interrupt when CPU is hot-unplugged
2018-05-09 11:01 ` Julien Grall
@ 2018-05-09 11:14 ` Mirela Simonovic
2018-05-09 11:15 ` Julien Grall
0 siblings, 1 reply; 45+ messages in thread
From: Mirela Simonovic @ 2018-05-09 11:14 UTC (permalink / raw)
To: Julien Grall
Cc: Edgar E. Iglesias, Andre Przywara, Stefano Stabellini,
Tim Deegan, Xen Devel
Hi Julien,
On Wed, May 9, 2018 at 1:01 PM, Julien Grall <julien.grall@arm.com> wrote:
>
>
> On 09/05/18 11:10, Mirela Simonovic wrote:
>>
>> On Fri, Apr 27, 2018 at 5:12 PM, Julien Grall <julien.grall@arm.com>
>> wrote:
>>>
>>> On 27/04/18 15:38, Mirela Simonovic wrote:
>>>>
>>>> On Fri, Apr 27, 2018 at 4:15 PM, Tim Deegan <tim@xen.org> wrote:
>>>>>
>>>>> At 10:28 +0100 on 27 Apr (1524824906), Julien Grall wrote:
>>>>>>
>>>>>> On 26/04/18 15:23, Tim Deegan wrote:
>>>>>>>
>>>>>>> At 11:08 +0100 on 26 Apr (1524740921), Julien Grall wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> On 20/04/18 13:25, Mirela Simonovic wrote:
>>>
>>> While I guess this code makes no harm, it does not do what is expected
>>> (i.e
>>> draining the interrupt). I can't see any reason to keep wrong code, we
>>> should really aim to have code that match the architecture. And better to
>>> fix it when we discover the problem rather than waiting until we
>>> rediscovered it later.
>>>
>>> So at least a patch to update the code/comment should be done.
>>>
>>
>> I don't feel comfortable removing these 3 lines because I have no way
>> to test and guarantee that the change will not introduce any issues.
>
>
> The work you are doing (suspend/resume, hotplug) is not easy to test and
> very subtle to get it right. Testing can only uncover obvious bug on your
> platform. IMHO, we can only rely on the specifications (ARM ARM, PSCI...)
> and extensive review of the series (and the code around).
>
>> However, if despite all you really want me to remove these lines
>> within this series I don't have a problem doing that in a separate
>> patch. Please just confirm the plan.
>
>
> I am quite confident that this code should not be there or at least not in
> its current form.
>
Do you want me to include the removal of that code in this series or not?
Thanks,
Mirela
> Cheers,
>
> --
> Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v2 07/10] xen/arm: Release maintenance interrupt when CPU is hot-unplugged
2018-05-09 11:14 ` Mirela Simonovic
@ 2018-05-09 11:15 ` Julien Grall
0 siblings, 0 replies; 45+ messages in thread
From: Julien Grall @ 2018-05-09 11:15 UTC (permalink / raw)
To: Mirela Simonovic
Cc: Edgar E. Iglesias, Andre Przywara, Stefano Stabellini,
Tim Deegan, Xen Devel
On 09/05/18 12:14, Mirela Simonovic wrote:
> Hi Julien,
>
> On Wed, May 9, 2018 at 1:01 PM, Julien Grall <julien.grall@arm.com> wrote:
>>
>>
>> On 09/05/18 11:10, Mirela Simonovic wrote:
>>>
>>> On Fri, Apr 27, 2018 at 5:12 PM, Julien Grall <julien.grall@arm.com>
>>> wrote:
>>>>
>>>> On 27/04/18 15:38, Mirela Simonovic wrote:
>>>>>
>>>>> On Fri, Apr 27, 2018 at 4:15 PM, Tim Deegan <tim@xen.org> wrote:
>>>>>>
>>>>>> At 10:28 +0100 on 27 Apr (1524824906), Julien Grall wrote:
>>>>>>>
>>>>>>> On 26/04/18 15:23, Tim Deegan wrote:
>>>>>>>>
>>>>>>>> At 11:08 +0100 on 26 Apr (1524740921), Julien Grall wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 20/04/18 13:25, Mirela Simonovic wrote:
>>>>
>>>> While I guess this code makes no harm, it does not do what is expected
>>>> (i.e
>>>> draining the interrupt). I can't see any reason to keep wrong code, we
>>>> should really aim to have code that match the architecture. And better to
>>>> fix it when we discover the problem rather than waiting until we
>>>> rediscovered it later.
>>>>
>>>> So at least a patch to update the code/comment should be done.
>>>>
>>>
>>> I don't feel comfortable removing these 3 lines because I have no way
>>> to test and guarantee that the change will not introduce any issues.
>>
>>
>> The work you are doing (suspend/resume, hotplug) is not easy to test and
>> very subtle to get it right. Testing can only uncover obvious bug on your
>> platform. IMHO, we can only rely on the specifications (ARM ARM, PSCI...)
>> and extensive review of the series (and the code around).
>>
>>> However, if despite all you really want me to remove these lines
>>> within this series I don't have a problem doing that in a separate
>>> patch. Please just confirm the plan.
>>
>>
>> I am quite confident that this code should not be there or at least not in
>> its current form.
>>
>
> Do you want me to include the removal of that code in this series or not?
It is not necessary to be in that series.
Cheers,
>
> Thanks,
> Mirela
>
>> Cheers,
>>
>> --
>> Julien Grall
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v2 08/10] xen/arm: Release timer interrupts when CPU is hot-unplugged
2018-04-20 12:25 [PATCH v2 00/10] xen/arm64: Suspend preconditions and CPU hotplug fixes Mirela Simonovic
` (6 preceding siblings ...)
2018-04-20 12:25 ` [PATCH v2 07/10] xen/arm: Release maintenance interrupt when CPU is hot-unplugged Mirela Simonovic
@ 2018-04-20 12:25 ` Mirela Simonovic
2018-04-25 10:34 ` Mirela Simonovic
2018-04-20 12:25 ` [PATCH v2 09/10] xen/arm: Free memory allocated for sibling/core maps on CPU hot-unplug Mirela Simonovic
2018-04-20 12:25 ` [PATCH v2 10/10] xen/arm: Call check_local_cpu_errata for secondary CPU only on boot Mirela Simonovic
9 siblings, 1 reply; 45+ messages in thread
From: Mirela Simonovic @ 2018-04-20 12:25 UTC (permalink / raw)
To: xen-devel; +Cc: edgar.iglesias, julien.grall, sstabellini, Mirela Simonovic
When a CPU is hot-unplugged timer interrupts have to be released
in order to free the memory that was allocated when the interrupts
were requested (using request_irq()). The request_irq is called
for each timer interrupt when the CPU gets hotplugged
(start_secondary->init_timer_interrupt->request_irq).
Signed-off-by: Mirela Simonovic <mirela.simonovic@aggios.com>
---
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>
---
xen/arch/arm/smpboot.c | 1 +
xen/arch/arm/time.c | 7 +++++++
xen/include/asm-arm/time.h | 6 ++++++
3 files changed, 14 insertions(+)
diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
index 449fefc77d..b4ed479dc6 100644
--- a/xen/arch/arm/smpboot.c
+++ b/xen/arch/arm/smpboot.c
@@ -386,6 +386,7 @@ void __cpu_disable(void)
* in respective init interrupt functions called from start_secondary)
*/
deinit_maintenance_interrupt();
+ deinit_timer_interrupt();
/* It's now safe to remove this processor from the online map */
cpumask_clear_cpu(cpu, &cpu_online_map);
diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
index c11fcfeadd..1d9dc16f89 100644
--- a/xen/arch/arm/time.c
+++ b/xen/arch/arm/time.c
@@ -312,6 +312,13 @@ void init_timer_interrupt(void)
check_timer_irq_cfg(timer_irq[TIMER_PHYS_NONSECURE_PPI], "NS-physical");
}
+void deinit_timer_interrupt(void)
+{
+ release_irq(timer_irq[TIMER_HYP_PPI], NULL);
+ release_irq(timer_irq[TIMER_VIRT_PPI], NULL);
+ release_irq(timer_irq[TIMER_PHYS_NONSECURE_PPI], NULL);
+}
+
/* Wait a set number of microseconds */
void udelay(unsigned long usecs)
{
diff --git a/xen/include/asm-arm/time.h b/xen/include/asm-arm/time.h
index 5b9a31de91..6fa4c47532 100644
--- a/xen/include/asm-arm/time.h
+++ b/xen/include/asm-arm/time.h
@@ -34,6 +34,12 @@ unsigned int timer_get_irq(enum timer_ppi ppi);
/* Set up the timer interrupt on this CPU */
extern void init_timer_interrupt(void);
+/*
+ * Revert actions done in init_timer_interrupt that are required to properly
+ * disable this CPU.
+ */
+extern void deinit_timer_interrupt(void);
+
/* Counter value at boot time */
extern uint64_t boot_count;
--
2.13.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v2 08/10] xen/arm: Release timer interrupts when CPU is hot-unplugged
2018-04-20 12:25 ` [PATCH v2 08/10] xen/arm: Release timer interrupts " Mirela Simonovic
@ 2018-04-25 10:34 ` Mirela Simonovic
2018-04-25 10:38 ` Julien Grall
0 siblings, 1 reply; 45+ messages in thread
From: Mirela Simonovic @ 2018-04-25 10:34 UTC (permalink / raw)
To: Xen Devel
Cc: Edgar E. Iglesias, Julien Grall, Stefano Stabellini, Mirela Simonovic
Hi Julien,
You may have missed this one. Should we try using notifiers here as well?
Thanks,
Mirela
On Fri, Apr 20, 2018 at 2:25 PM, Mirela Simonovic
<mirela.simonovic@aggios.com> wrote:
> When a CPU is hot-unplugged timer interrupts have to be released
> in order to free the memory that was allocated when the interrupts
> were requested (using request_irq()). The request_irq is called
> for each timer interrupt when the CPU gets hotplugged
> (start_secondary->init_timer_interrupt->request_irq).
>
> Signed-off-by: Mirela Simonovic <mirela.simonovic@aggios.com>
>
> ---
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien.grall@arm.com>
> ---
> xen/arch/arm/smpboot.c | 1 +
> xen/arch/arm/time.c | 7 +++++++
> xen/include/asm-arm/time.h | 6 ++++++
> 3 files changed, 14 insertions(+)
>
> diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
> index 449fefc77d..b4ed479dc6 100644
> --- a/xen/arch/arm/smpboot.c
> +++ b/xen/arch/arm/smpboot.c
> @@ -386,6 +386,7 @@ void __cpu_disable(void)
> * in respective init interrupt functions called from start_secondary)
> */
> deinit_maintenance_interrupt();
> + deinit_timer_interrupt();
>
> /* It's now safe to remove this processor from the online map */
> cpumask_clear_cpu(cpu, &cpu_online_map);
> diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
> index c11fcfeadd..1d9dc16f89 100644
> --- a/xen/arch/arm/time.c
> +++ b/xen/arch/arm/time.c
> @@ -312,6 +312,13 @@ void init_timer_interrupt(void)
> check_timer_irq_cfg(timer_irq[TIMER_PHYS_NONSECURE_PPI], "NS-physical");
> }
>
> +void deinit_timer_interrupt(void)
> +{
> + release_irq(timer_irq[TIMER_HYP_PPI], NULL);
> + release_irq(timer_irq[TIMER_VIRT_PPI], NULL);
> + release_irq(timer_irq[TIMER_PHYS_NONSECURE_PPI], NULL);
> +}
> +
> /* Wait a set number of microseconds */
> void udelay(unsigned long usecs)
> {
> diff --git a/xen/include/asm-arm/time.h b/xen/include/asm-arm/time.h
> index 5b9a31de91..6fa4c47532 100644
> --- a/xen/include/asm-arm/time.h
> +++ b/xen/include/asm-arm/time.h
> @@ -34,6 +34,12 @@ unsigned int timer_get_irq(enum timer_ppi ppi);
> /* Set up the timer interrupt on this CPU */
> extern void init_timer_interrupt(void);
>
> +/*
> + * Revert actions done in init_timer_interrupt that are required to properly
> + * disable this CPU.
> + */
> +extern void deinit_timer_interrupt(void);
> +
> /* Counter value at boot time */
> extern uint64_t boot_count;
>
> --
> 2.13.0
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v2 08/10] xen/arm: Release timer interrupts when CPU is hot-unplugged
2018-04-25 10:34 ` Mirela Simonovic
@ 2018-04-25 10:38 ` Julien Grall
0 siblings, 0 replies; 45+ messages in thread
From: Julien Grall @ 2018-04-25 10:38 UTC (permalink / raw)
To: Mirela Simonovic, Xen Devel; +Cc: Edgar E. Iglesias, Stefano Stabellini
On 25/04/18 11:34, Mirela Simonovic wrote:
> Hi Julien,
>
> You may have missed this one. Should we try using notifiers here as well?
Yes, I assumed this was implied by my comments previously, so I skipped
the patch. Sorry for that.
Cheers,
>
> Thanks,
> Mirela
>
> On Fri, Apr 20, 2018 at 2:25 PM, Mirela Simonovic
> <mirela.simonovic@aggios.com> wrote:
>> When a CPU is hot-unplugged timer interrupts have to be released
>> in order to free the memory that was allocated when the interrupts
>> were requested (using request_irq()). The request_irq is called
>> for each timer interrupt when the CPU gets hotplugged
>> (start_secondary->init_timer_interrupt->request_irq).
>>
>> Signed-off-by: Mirela Simonovic <mirela.simonovic@aggios.com>
>>
>> ---
>> CC: Stefano Stabellini <sstabellini@kernel.org>
>> CC: Julien Grall <julien.grall@arm.com>
>> ---
>> xen/arch/arm/smpboot.c | 1 +
>> xen/arch/arm/time.c | 7 +++++++
>> xen/include/asm-arm/time.h | 6 ++++++
>> 3 files changed, 14 insertions(+)
>>
>> diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
>> index 449fefc77d..b4ed479dc6 100644
>> --- a/xen/arch/arm/smpboot.c
>> +++ b/xen/arch/arm/smpboot.c
>> @@ -386,6 +386,7 @@ void __cpu_disable(void)
>> * in respective init interrupt functions called from start_secondary)
>> */
>> deinit_maintenance_interrupt();
>> + deinit_timer_interrupt();
>>
>> /* It's now safe to remove this processor from the online map */
>> cpumask_clear_cpu(cpu, &cpu_online_map);
>> diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
>> index c11fcfeadd..1d9dc16f89 100644
>> --- a/xen/arch/arm/time.c
>> +++ b/xen/arch/arm/time.c
>> @@ -312,6 +312,13 @@ void init_timer_interrupt(void)
>> check_timer_irq_cfg(timer_irq[TIMER_PHYS_NONSECURE_PPI], "NS-physical");
>> }
>>
>> +void deinit_timer_interrupt(void)
>> +{
>> + release_irq(timer_irq[TIMER_HYP_PPI], NULL);
>> + release_irq(timer_irq[TIMER_VIRT_PPI], NULL);
>> + release_irq(timer_irq[TIMER_PHYS_NONSECURE_PPI], NULL);
>> +}
>> +
>> /* Wait a set number of microseconds */
>> void udelay(unsigned long usecs)
>> {
>> diff --git a/xen/include/asm-arm/time.h b/xen/include/asm-arm/time.h
>> index 5b9a31de91..6fa4c47532 100644
>> --- a/xen/include/asm-arm/time.h
>> +++ b/xen/include/asm-arm/time.h
>> @@ -34,6 +34,12 @@ unsigned int timer_get_irq(enum timer_ppi ppi);
>> /* Set up the timer interrupt on this CPU */
>> extern void init_timer_interrupt(void);
>>
>> +/*
>> + * Revert actions done in init_timer_interrupt that are required to properly
>> + * disable this CPU.
>> + */
>> +extern void deinit_timer_interrupt(void);
>> +
>> /* Counter value at boot time */
>> extern uint64_t boot_count;
>>
>> --
>> 2.13.0
>>
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v2 09/10] xen/arm: Free memory allocated for sibling/core maps on CPU hot-unplug
2018-04-20 12:25 [PATCH v2 00/10] xen/arm64: Suspend preconditions and CPU hotplug fixes Mirela Simonovic
` (7 preceding siblings ...)
2018-04-20 12:25 ` [PATCH v2 08/10] xen/arm: Release timer interrupts " Mirela Simonovic
@ 2018-04-20 12:25 ` Mirela Simonovic
2018-04-23 11:38 ` Julien Grall
2018-04-20 12:25 ` [PATCH v2 10/10] xen/arm: Call check_local_cpu_errata for secondary CPU only on boot Mirela Simonovic
9 siblings, 1 reply; 45+ messages in thread
From: Mirela Simonovic @ 2018-04-20 12:25 UTC (permalink / raw)
To: xen-devel; +Cc: edgar.iglesias, julien.grall, sstabellini, Mirela Simonovic
The memory allocated in setup_cpu_sibling_map() when a CPU is hotplugged
has to be freed when the CPU is hot-unplugged. This is done in
remove_cpu_sibling_map() and called from __cpu_disable() on CPU hot-unplug.
Signed-off-by: Mirela Simonovic <mirela.simonovic@aggios.com>
---
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>
---
xen/arch/arm/smpboot.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
index b4ed479dc6..d01b51592d 100644
--- a/xen/arch/arm/smpboot.c
+++ b/xen/arch/arm/smpboot.c
@@ -89,6 +89,12 @@ static void setup_cpu_sibling_map(int cpu)
cpumask_set_cpu(cpu, per_cpu(cpu_core_mask, cpu));
}
+static void remove_cpu_sibling_map(int cpu)
+{
+ free_cpumask_var(per_cpu(cpu_sibling_mask, cpu));
+ free_cpumask_var(per_cpu(cpu_core_mask, cpu));
+}
+
void __init
smp_clear_cpu_maps (void)
{
@@ -391,6 +397,8 @@ void __cpu_disable(void)
/* It's now safe to remove this processor from the online map */
cpumask_clear_cpu(cpu, &cpu_online_map);
+ remove_cpu_sibling_map(cpu);
+
if ( cpu_disable_scheduler(cpu) )
BUG();
smp_mb();
--
2.13.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v2 09/10] xen/arm: Free memory allocated for sibling/core maps on CPU hot-unplug
2018-04-20 12:25 ` [PATCH v2 09/10] xen/arm: Free memory allocated for sibling/core maps on CPU hot-unplug Mirela Simonovic
@ 2018-04-23 11:38 ` Julien Grall
2018-04-27 11:14 ` Mirela Simonovic
0 siblings, 1 reply; 45+ messages in thread
From: Julien Grall @ 2018-04-23 11:38 UTC (permalink / raw)
To: Mirela Simonovic, xen-devel; +Cc: edgar.iglesias, sstabellini
Hi,
On 20/04/18 13:25, Mirela Simonovic wrote:
> The memory allocated in setup_cpu_sibling_map() when a CPU is hotplugged
> has to be freed when the CPU is hot-unplugged. This is done in
> remove_cpu_sibling_map() and called from __cpu_disable() on CPU hot-unplug.
>
> Signed-off-by: Mirela Simonovic <mirela.simonovic@aggios.com>
>
> ---
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien.grall@arm.com>
> ---
> xen/arch/arm/smpboot.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
> index b4ed479dc6..d01b51592d 100644
> --- a/xen/arch/arm/smpboot.c
> +++ b/xen/arch/arm/smpboot.c
> @@ -89,6 +89,12 @@ static void setup_cpu_sibling_map(int cpu)
> cpumask_set_cpu(cpu, per_cpu(cpu_core_mask, cpu));
> }
>
> +static void remove_cpu_sibling_map(int cpu)
> +{
> + free_cpumask_var(per_cpu(cpu_sibling_mask, cpu));
> + free_cpumask_var(per_cpu(cpu_core_mask, cpu));
> +}
> +
> void __init
> smp_clear_cpu_maps (void)
> {
> @@ -391,6 +397,8 @@ void __cpu_disable(void)
> /* It's now safe to remove this processor from the online map */
> cpumask_clear_cpu(cpu, &cpu_online_map);
>
> + remove_cpu_sibling_map(cpu);
I don't think this is the right place. Those cpumask might be used by
the scheduler. So you want to free them only when the scheduler has been
disabled.
Looking at x86, they will free them when the CPU is completely dead (see
cpu_smpboot_callback). So I think a notifier would be the solution for
arm as well.
Cheers,
> +
> if ( cpu_disable_scheduler(cpu) )
> BUG();
> smp_mb();
>
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v2 09/10] xen/arm: Free memory allocated for sibling/core maps on CPU hot-unplug
2018-04-23 11:38 ` Julien Grall
@ 2018-04-27 11:14 ` Mirela Simonovic
0 siblings, 0 replies; 45+ messages in thread
From: Mirela Simonovic @ 2018-04-27 11:14 UTC (permalink / raw)
To: Julien Grall; +Cc: Edgar E. Iglesias, Stefano Stabellini, Xen Devel
Thanks Julien, I fixed this
On Mon, Apr 23, 2018 at 1:38 PM, Julien Grall <julien.grall@arm.com> wrote:
> Hi,
>
>
> On 20/04/18 13:25, Mirela Simonovic wrote:
>>
>> The memory allocated in setup_cpu_sibling_map() when a CPU is hotplugged
>> has to be freed when the CPU is hot-unplugged. This is done in
>> remove_cpu_sibling_map() and called from __cpu_disable() on CPU
>> hot-unplug.
>>
>> Signed-off-by: Mirela Simonovic <mirela.simonovic@aggios.com>
>>
>> ---
>> CC: Stefano Stabellini <sstabellini@kernel.org>
>> CC: Julien Grall <julien.grall@arm.com>
>> ---
>> xen/arch/arm/smpboot.c | 8 ++++++++
>> 1 file changed, 8 insertions(+)
>>
>> diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
>> index b4ed479dc6..d01b51592d 100644
>> --- a/xen/arch/arm/smpboot.c
>> +++ b/xen/arch/arm/smpboot.c
>> @@ -89,6 +89,12 @@ static void setup_cpu_sibling_map(int cpu)
>> cpumask_set_cpu(cpu, per_cpu(cpu_core_mask, cpu));
>> }
>> +static void remove_cpu_sibling_map(int cpu)
>> +{
>> + free_cpumask_var(per_cpu(cpu_sibling_mask, cpu));
>> + free_cpumask_var(per_cpu(cpu_core_mask, cpu));
>> +}
>> +
>> void __init
>> smp_clear_cpu_maps (void)
>> {
>> @@ -391,6 +397,8 @@ void __cpu_disable(void)
>> /* It's now safe to remove this processor from the online map */
>> cpumask_clear_cpu(cpu, &cpu_online_map);
>> + remove_cpu_sibling_map(cpu);
>
>
> I don't think this is the right place. Those cpumask might be used by the
> scheduler. So you want to free them only when the scheduler has been
> disabled.
>
> Looking at x86, they will free them when the CPU is completely dead (see
> cpu_smpboot_callback). So I think a notifier would be the solution for arm
> as well.
>
> Cheers,
>
>
>> +
>> if ( cpu_disable_scheduler(cpu) )
>> BUG();
>> smp_mb();
>>
>
> --
> Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v2 10/10] xen/arm: Call check_local_cpu_errata for secondary CPU only on boot
2018-04-20 12:25 [PATCH v2 00/10] xen/arm64: Suspend preconditions and CPU hotplug fixes Mirela Simonovic
` (8 preceding siblings ...)
2018-04-20 12:25 ` [PATCH v2 09/10] xen/arm: Free memory allocated for sibling/core maps on CPU hot-unplug Mirela Simonovic
@ 2018-04-20 12:25 ` Mirela Simonovic
2018-04-23 11:46 ` Julien Grall
9 siblings, 1 reply; 45+ messages in thread
From: Mirela Simonovic @ 2018-04-20 12:25 UTC (permalink / raw)
To: xen-devel; +Cc: edgar.iglesias, julien.grall, sstabellini, Mirela Simonovic
Checking CPU errata should be done only when a CPU is initially booted.
It is assumed that the CPU which is hotplugged after the system/Xen boots,
was initially hotplugged during the system/Xen boot, so errata is checked
by each CPU only once, on boot.
Signed-off-by: Mirela Simonovic <mirela.simonovic@aggios.com>
---
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>
---
xen/arch/arm/smpboot.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
index d01b51592d..5d6c6cadec 100644
--- a/xen/arch/arm/smpboot.c
+++ b/xen/arch/arm/smpboot.c
@@ -366,8 +366,8 @@ void start_secondary(unsigned long boot_phys_offset,
if ( system_state != SYS_STATE_boot )
setup_virt_paging_secondary();
-
- check_local_cpu_errata();
+ else
+ check_local_cpu_errata();
printk(XENLOG_DEBUG "CPU %u booted.\n", smp_processor_id());
--
2.13.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v2 10/10] xen/arm: Call check_local_cpu_errata for secondary CPU only on boot
2018-04-20 12:25 ` [PATCH v2 10/10] xen/arm: Call check_local_cpu_errata for secondary CPU only on boot Mirela Simonovic
@ 2018-04-23 11:46 ` Julien Grall
2018-04-25 15:13 ` Mirela Simonovic
0 siblings, 1 reply; 45+ messages in thread
From: Julien Grall @ 2018-04-23 11:46 UTC (permalink / raw)
To: Mirela Simonovic, xen-devel; +Cc: edgar.iglesias, sstabellini
Hi,
On 20/04/18 13:25, Mirela Simonovic wrote:
> Checking CPU errata should be done only when a CPU is initially booted.
> It is assumed that the CPU which is hotplugged after the system/Xen boots,
> was initially hotplugged during the system/Xen boot, so errata is checked
> by each CPU only once, on boot.
It is a good idea to document the assumption in the code. This will help
to know what is missing for other use case.
>
> Signed-off-by: Mirela Simonovic <mirela.simonovic@aggios.com>
>
> ---
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien.grall@arm.com>
> ---
> xen/arch/arm/smpboot.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
> index d01b51592d..5d6c6cadec 100644
> --- a/xen/arch/arm/smpboot.c
> +++ b/xen/arch/arm/smpboot.c
> @@ -366,8 +366,8 @@ void start_secondary(unsigned long boot_phys_offset,
>
> if ( system_state != SYS_STATE_boot )
> setup_virt_paging_secondary();
> -
> - check_local_cpu_errata();
> + else
> + check_local_cpu_errata();
No, check_local_cpu_errata should be called for everyone. This check
should be moved in the function with a TODO explaining what needs to be
done. Likely this will be go over the CPU errata and see if there are
any issue with the one currently selected.
Also, I just realized that any "cpu capability" (e.g spectre workaround)
that requires to be enabled will not be done on hotplugged CPU. You
likely need to implement a version of enable_errata_workaround for them.
Cheers,
>
> printk(XENLOG_DEBUG "CPU %u booted.\n", smp_processor_id());
>
>
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v2 10/10] xen/arm: Call check_local_cpu_errata for secondary CPU only on boot
2018-04-23 11:46 ` Julien Grall
@ 2018-04-25 15:13 ` Mirela Simonovic
2018-04-26 10:18 ` Julien Grall
0 siblings, 1 reply; 45+ messages in thread
From: Mirela Simonovic @ 2018-04-25 15:13 UTC (permalink / raw)
To: Julien Grall; +Cc: Edgar E. Iglesias, Stefano Stabellini, Xen Devel
Hi Julien,
On Mon, Apr 23, 2018 at 1:46 PM, Julien Grall <julien.grall@arm.com> wrote:
> Hi,
>
> On 20/04/18 13:25, Mirela Simonovic wrote:
>>
>> Checking CPU errata should be done only when a CPU is initially booted.
>> It is assumed that the CPU which is hotplugged after the system/Xen boots,
>> was initially hotplugged during the system/Xen boot, so errata is checked
>> by each CPU only once, on boot.
>
>
> It is a good idea to document the assumption in the code. This will help to
> know what is missing for other use case.
>
>>
>> Signed-off-by: Mirela Simonovic <mirela.simonovic@aggios.com>
>>
>> ---
>> CC: Stefano Stabellini <sstabellini@kernel.org>
>> CC: Julien Grall <julien.grall@arm.com>
>> ---
>> xen/arch/arm/smpboot.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
>> index d01b51592d..5d6c6cadec 100644
>> --- a/xen/arch/arm/smpboot.c
>> +++ b/xen/arch/arm/smpboot.c
>> @@ -366,8 +366,8 @@ void start_secondary(unsigned long boot_phys_offset,
>> if ( system_state != SYS_STATE_boot )
>> setup_virt_paging_secondary();
>> -
>> - check_local_cpu_errata();
>> + else
>> + check_local_cpu_errata();
>
>
> No, check_local_cpu_errata should be called for everyone. This check should
Could you please clarify what you meant with "for everyone"? My
understanding is that you suggested this in
https://lists.xenproject.org/archives/html/xen-devel/2018-01/msg00979.html
Did something change meanwhile?
> be moved in the function with a TODO explaining what needs to be done.
> Likely this will be go over the CPU errata and see if there are any issue
> with the one currently selected.
Please clarify, I don't follow this.
>
> Also, I just realized that any "cpu capability" (e.g spectre workaround)
> that requires to be enabled will not be done on hotplugged CPU. You likely
> need to implement a version of enable_errata_workaround for them.
>
Could you please point me to a place where this is done on boot?
Thanks,
Mirela
> Cheers,
>
>
>> printk(XENLOG_DEBUG "CPU %u booted.\n", smp_processor_id());
>>
>
>
> --
> Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v2 10/10] xen/arm: Call check_local_cpu_errata for secondary CPU only on boot
2018-04-25 15:13 ` Mirela Simonovic
@ 2018-04-26 10:18 ` Julien Grall
0 siblings, 0 replies; 45+ messages in thread
From: Julien Grall @ 2018-04-26 10:18 UTC (permalink / raw)
To: Mirela Simonovic; +Cc: Edgar E. Iglesias, Stefano Stabellini, Xen Devel
On 25/04/18 16:13, Mirela Simonovic wrote:
> Hi Julien,
Hi Mirela,
> On Mon, Apr 23, 2018 at 1:46 PM, Julien Grall <julien.grall@arm.com> wrote:
>> Hi,
>>
>> On 20/04/18 13:25, Mirela Simonovic wrote:
>>>
>>> Checking CPU errata should be done only when a CPU is initially booted.
>>> It is assumed that the CPU which is hotplugged after the system/Xen boots,
>>> was initially hotplugged during the system/Xen boot, so errata is checked
>>> by each CPU only once, on boot.
>>
>>
>> It is a good idea to document the assumption in the code. This will help to
>> know what is missing for other use case.
>>
>>>
>>> Signed-off-by: Mirela Simonovic <mirela.simonovic@aggios.com>
>>>
>>> ---
>>> CC: Stefano Stabellini <sstabellini@kernel.org>
>>> CC: Julien Grall <julien.grall@arm.com>
>>> ---
>>> xen/arch/arm/smpboot.c | 4 ++--
>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
>>> index d01b51592d..5d6c6cadec 100644
>>> --- a/xen/arch/arm/smpboot.c
>>> +++ b/xen/arch/arm/smpboot.c
>>> @@ -366,8 +366,8 @@ void start_secondary(unsigned long boot_phys_offset,
>>> if ( system_state != SYS_STATE_boot )
>>> setup_virt_paging_secondary();
>>> -
>>> - check_local_cpu_errata();
>>> + else
>>> + check_local_cpu_errata();
>>
>>
>> No, check_local_cpu_errata should be called for everyone. This check should
>
> Could you please clarify what you meant with "for everyone"? My
> understanding is that you suggested this in
> https://lists.xenproject.org/archives/html/xen-devel/2018-01/msg00979.html
> Did something change meanwhile?
We discussed a bit more how big.LITTLE fits in the picture since them.
Technically you have to check Xen has all the known workaround
associated to the processor so it can run safely. If not, you need to
park that CPU.
>
>> be moved in the function with a TODO explaining what needs to be done.
>> Likely this will be go over the CPU errata and see if there are any issue
>> with the one currently selected.
>
> Please clarify, I don't follow this.
Most of the errata requires to modify Xen code. This is done using the
alternative at Xen boot. If you boot a CPU with an errata that has not
been enabled, mostly likely you want to park that CPU.
In future, you could imagine to do the same with features (e.g atomics,
pointer authentification...). You want to know whether the processors
has a feature the hypervisor has enabled.
>
>>
>> Also, I just realized that any "cpu capability" (e.g spectre workaround)
>> that requires to be enabled will not be done on hotplugged CPU. You likely
>> need to implement a version of enable_errata_workaround for them.
>>
>
> Could you please point me to a place where this is done on boot?
Have a look at the function enable_errata_workarounds, this is done late
at boot and go through all CPU to enable the different errata.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread