[PATCH] xen/arm: p2m: Restrict preemption check in apply_p2m_changes

[PATCH] xen/arm: p2m: Restrict preemption check in apply_p2m_changes

[Julien Grall]

The commit 569fb6c "xen/arm: Data abort exception (R/W) mem_access
events" makes apply_p2m_changes to call hypercall_preempt_check for any
operation rather than for relinquish.

The function hypercall_preempt_check call local_events_need_delivery
which rely on the current VCPU is not an idle VCPU.
Although, during DOM0 building the current VCPU is an idle one. This
would make Xen crash with the following stack trace:

(XEN) CPU0: Unexpected Trap: Data Abort
[...]
(XEN) Xen call trace:
(XEN)    [<00256ef4>] apply_p2m_changes+0x210/0x1190 (PC)
(XEN)    [<002506b4>] gic_events_need_delivery+0x5c/0x13c (LR)
(XEN)    [<002580ec>] map_mmio_regions+0x64/0x74
(XEN)    [<00251958>] gicv2v_setup+0xf8/0x150
(XEN)    [<00250964>] gicv_setup+0x20/0x30
(XEN)    [<0024cb3c>] arch_domain_create+0x170/0x244
(XEN)    [<00207df0>] domain_create+0x2ac/0x4d8
(XEN)    [<0028e3d0>] start_xen+0xcbc/0xee4
(XEN)    [<00200540>] paging+0x94/0xd8
(XEN)
(XEN)
(XEN) ****************************************
(XEN) Panic on CPU 0:
(XEN) CPU0: Unexpected Trap: Data Abort
(XEN)
(XEN) ****************************************

hypercall_preempt_check is expecting to be call only when the current
VCPU belong to a real domain (see x86 behavior).

As the bug prevents Xen booting on some platform, fix it by only check
preemption when the current VCPU is an idle one for now. We could
improve it later.

Reported-by: Riku Voipio <riku.voipio@linaro.org>
Signed-off-by: Julien Grall <julien.grall@citrix.com>
CC: Tamas K Lengyel <tklengyel@sec.in.tum.de>

---

This bug has been catched during boot on Mustang. This is because we
have to map large chunk of PCI memory region.

I was able to reproduce the bug on midway by lowering down
preempt_count_limit to 16 in apply_p2m_changes.

Note: This patch superseeds "Make local_events_need_delivery working with idle
VPCU"
---
 xen/arch/arm/p2m.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 65efa94..59dd23a 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -948,6 +948,7 @@ static int apply_p2m_changes(struct domain *d,
     const unsigned long sgfn = paddr_to_pfn(start_gpaddr),
                         egfn = paddr_to_pfn(end_gpaddr);
     const unsigned int preempt_count_limit = (op == MEMACCESS) ? 1 : 0x2000;
+    const bool_t preempt = !is_idle_vcpu(current);
     bool_t flush = false;
     bool_t flush_pt;
 
@@ -980,7 +981,8 @@ static int apply_p2m_changes(struct domain *d,
          * always make at least one pass as long as preempt_count_limit is
          * initialized with a value >= 1.
          */
-        if ( count >= preempt_count_limit && hypercall_preempt_check() )
+        if ( preempt && count >= preempt_count_limit
+             && hypercall_preempt_check() )
         {
             switch ( op )
             {
-- 
2.1.4

Re: [PATCH] xen/arm: p2m: Restrict preemption check in apply_p2m_changes

[Ian Campbell]

On Tue, 2015-05-05 at 16:02 +0100, Julien Grall wrote:
> The commit 569fb6c "xen/arm: Data abort exception (R/W) mem_access
> events" makes apply_p2m_changes to call hypercall_preempt_check for any
> operation rather than for relinquish.
> 
> The function hypercall_preempt_check call local_events_need_delivery
> which rely on the current VCPU is not an idle VCPU.
> Although, during DOM0 building the current VCPU is an idle one. This
> would make Xen crash with the following stack trace:
> 
> (XEN) CPU0: Unexpected Trap: Data Abort
> [...]
> (XEN) Xen call trace:
> (XEN)    [<00256ef4>] apply_p2m_changes+0x210/0x1190 (PC)
> (XEN)    [<002506b4>] gic_events_need_delivery+0x5c/0x13c (LR)
> (XEN)    [<002580ec>] map_mmio_regions+0x64/0x74
> (XEN)    [<00251958>] gicv2v_setup+0xf8/0x150
> (XEN)    [<00250964>] gicv_setup+0x20/0x30
> (XEN)    [<0024cb3c>] arch_domain_create+0x170/0x244
> (XEN)    [<00207df0>] domain_create+0x2ac/0x4d8
> (XEN)    [<0028e3d0>] start_xen+0xcbc/0xee4
> (XEN)    [<00200540>] paging+0x94/0xd8
> (XEN)
> (XEN)
> (XEN) ****************************************
> (XEN) Panic on CPU 0:
> (XEN) CPU0: Unexpected Trap: Data Abort
> (XEN)
> (XEN) ****************************************
> 
> hypercall_preempt_check is expecting to be call only when the current
> VCPU belong to a real domain (see x86 behavior).
> 
> As the bug prevents Xen booting on some platform, fix it by only check
> preemption when the current VCPU is an idle one for now. We could
> improve it later.
> 
> Reported-by: Riku Voipio <riku.voipio@linaro.org>
> Signed-off-by: Julien Grall <julien.grall@citrix.com>
> CC: Tamas K Lengyel <tklengyel@sec.in.tum.de>

Acked + applied, thanks.

Re: [PATCH] xen/arm: p2m: Restrict preemption check in apply_p2m_changes

[Konrad Rzeszutek Wilk]

On Tue, May 05, 2015 at 04:02:09PM +0100, Julien Grall wrote:
> The commit 569fb6c "xen/arm: Data abort exception (R/W) mem_access
> events" makes apply_p2m_changes to call hypercall_preempt_check for any
> operation rather than for relinquish.
> 
> The function hypercall_preempt_check call local_events_need_delivery
> which rely on the current VCPU is not an idle VCPU.
> Although, during DOM0 building the current VCPU is an idle one. This
> would make Xen crash with the following stack trace:
> 
> (XEN) CPU0: Unexpected Trap: Data Abort
> [...]
> (XEN) Xen call trace:
> (XEN)    [<00256ef4>] apply_p2m_changes+0x210/0x1190 (PC)
> (XEN)    [<002506b4>] gic_events_need_delivery+0x5c/0x13c (LR)
> (XEN)    [<002580ec>] map_mmio_regions+0x64/0x74
> (XEN)    [<00251958>] gicv2v_setup+0xf8/0x150
> (XEN)    [<00250964>] gicv_setup+0x20/0x30
> (XEN)    [<0024cb3c>] arch_domain_create+0x170/0x244
> (XEN)    [<00207df0>] domain_create+0x2ac/0x4d8
> (XEN)    [<0028e3d0>] start_xen+0xcbc/0xee4
> (XEN)    [<00200540>] paging+0x94/0xd8
> (XEN)
> (XEN)
> (XEN) ****************************************
> (XEN) Panic on CPU 0:
> (XEN) CPU0: Unexpected Trap: Data Abort
> (XEN)
> (XEN) ****************************************
> 
> hypercall_preempt_check is expecting to be call only when the current
> VCPU belong to a real domain (see x86 behavior).
> 
> As the bug prevents Xen booting on some platform, fix it by only check
> preemption when the current VCPU is an idle one for now. We could
> improve it later.
> 
> Reported-by: Riku Voipio <riku.voipio@linaro.org>
> Signed-off-by: Julien Grall <julien.grall@citrix.com>
> CC: Tamas K Lengyel <tklengyel@sec.in.tum.de>
> 
> ---
> 
> This bug has been catched during boot on Mustang. This is because we
> have to map large chunk of PCI memory region.
> 
> I was able to reproduce the bug on midway by lowering down
> preempt_count_limit to 16 in apply_p2m_changes.
> 
> Note: This patch superseeds "Make local_events_need_delivery working with idle
> VPCU"
> ---
>  xen/arch/arm/p2m.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 65efa94..59dd23a 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -948,6 +948,7 @@ static int apply_p2m_changes(struct domain *d,
>      const unsigned long sgfn = paddr_to_pfn(start_gpaddr),
>                          egfn = paddr_to_pfn(end_gpaddr);
>      const unsigned int preempt_count_limit = (op == MEMACCESS) ? 1 : 0x2000;
> +    const bool_t preempt = !is_idle_vcpu(current);
>      bool_t flush = false;
>      bool_t flush_pt;
>  
> @@ -980,7 +981,8 @@ static int apply_p2m_changes(struct domain *d,
>           * always make at least one pass as long as preempt_count_limit is
>           * initialized with a value >= 1.
>           */
> -        if ( count >= preempt_count_limit && hypercall_preempt_check() )
> +        if ( preempt && count >= preempt_count_limit
> +             && hypercall_preempt_check() )

Could you  use the softirq_pending() check to deal when there are no domains?

>          {
>              switch ( op )
>              {
> -- 
> 2.1.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH] xen/arm: p2m: Restrict preemption check in apply_p2m_changes

[Julien Grall]

Hi Konrad,

On 05/05/15 16:50, Konrad Rzeszutek Wilk wrote:
>> ---
>>  xen/arch/arm/p2m.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>> index 65efa94..59dd23a 100644
>> --- a/xen/arch/arm/p2m.c
>> +++ b/xen/arch/arm/p2m.c
>> @@ -948,6 +948,7 @@ static int apply_p2m_changes(struct domain *d,
>>      const unsigned long sgfn = paddr_to_pfn(start_gpaddr),
>>                          egfn = paddr_to_pfn(end_gpaddr);
>>      const unsigned int preempt_count_limit = (op == MEMACCESS) ? 1 : 0x2000;
>> +    const bool_t preempt = !is_idle_vcpu(current);
>>      bool_t flush = false;
>>      bool_t flush_pt;
>>  
>> @@ -980,7 +981,8 @@ static int apply_p2m_changes(struct domain *d,
>>           * always make at least one pass as long as preempt_count_limit is
>>           * initialized with a value >= 1.
>>           */
>> -        if ( count >= preempt_count_limit && hypercall_preempt_check() )
>> +        if ( preempt && count >= preempt_count_limit
>> +             && hypercall_preempt_check() )
> 
> Could you  use the softirq_pending() check to deal when there are no domains?

apply_p2m_changes will only be called with no current domain (i.e idle
vcpu) during DOM0 construction.

If we check softirq_pending for this case, you will Xen will fail to
build DOM0 but the preemption is not supported.

I suspect this is the same on x86.

Regards,

-- 
Julien Grall

Re: [PATCH] xen/arm: p2m: Restrict preemption check in apply_p2m_changes

[Ian Campbell]

On Tue, 2015-05-05 at 17:31 +0100, Julien Grall wrote:
> Hi Konrad,
> 
> On 05/05/15 16:50, Konrad Rzeszutek Wilk wrote:
> >> ---
> >>  xen/arch/arm/p2m.c | 4 +++-
> >>  1 file changed, 3 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> >> index 65efa94..59dd23a 100644
> >> --- a/xen/arch/arm/p2m.c
> >> +++ b/xen/arch/arm/p2m.c
> >> @@ -948,6 +948,7 @@ static int apply_p2m_changes(struct domain *d,
> >>      const unsigned long sgfn = paddr_to_pfn(start_gpaddr),
> >>                          egfn = paddr_to_pfn(end_gpaddr);
> >>      const unsigned int preempt_count_limit = (op == MEMACCESS) ? 1 : 0x2000;
> >> +    const bool_t preempt = !is_idle_vcpu(current);
> >>      bool_t flush = false;
> >>      bool_t flush_pt;
> >>  
> >> @@ -980,7 +981,8 @@ static int apply_p2m_changes(struct domain *d,
> >>           * always make at least one pass as long as preempt_count_limit is
> >>           * initialized with a value >= 1.
> >>           */
> >> -        if ( count >= preempt_count_limit && hypercall_preempt_check() )
> >> +        if ( preempt && count >= preempt_count_limit
> >> +             && hypercall_preempt_check() )
> > 
> > Could you  use the softirq_pending() check to deal when there are no domains?
> 
> apply_p2m_changes will only be called with no current domain (i.e idle
> vcpu) during DOM0 construction.

I wonder if we could/should try and switch to d0:v0 sooner during the
build process? Is that even feasible?

> If we check softirq_pending for this case, you will Xen will fail to
> build DOM0 but the preemption is not supported.

I also don't really understand how softirq_pending would help here.

Ian.

Re: [PATCH] xen/arm: p2m: Restrict preemption check in apply_p2m_changes

[Julien Grall]

Hi Ian,

On 06/05/15 09:31, Ian Campbell wrote:
> On Tue, 2015-05-05 at 17:31 +0100, Julien Grall wrote:
>> Hi Konrad,
>>
>> On 05/05/15 16:50, Konrad Rzeszutek Wilk wrote:
>>>> ---
>>>>  xen/arch/arm/p2m.c | 4 +++-
>>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>>>> index 65efa94..59dd23a 100644
>>>> --- a/xen/arch/arm/p2m.c
>>>> +++ b/xen/arch/arm/p2m.c
>>>> @@ -948,6 +948,7 @@ static int apply_p2m_changes(struct domain *d,
>>>>      const unsigned long sgfn = paddr_to_pfn(start_gpaddr),
>>>>                          egfn = paddr_to_pfn(end_gpaddr);
>>>>      const unsigned int preempt_count_limit = (op == MEMACCESS) ? 1 : 0x2000;
>>>> +    const bool_t preempt = !is_idle_vcpu(current);
>>>>      bool_t flush = false;
>>>>      bool_t flush_pt;
>>>>  
>>>> @@ -980,7 +981,8 @@ static int apply_p2m_changes(struct domain *d,
>>>>           * always make at least one pass as long as preempt_count_limit is
>>>>           * initialized with a value >= 1.
>>>>           */
>>>> -        if ( count >= preempt_count_limit && hypercall_preempt_check() )
>>>> +        if ( preempt && count >= preempt_count_limit
>>>> +             && hypercall_preempt_check() )
>>>
>>> Could you  use the softirq_pending() check to deal when there are no domains?
>>
>> apply_p2m_changes will only be called with no current domain (i.e idle
>> vcpu) during DOM0 construction.
> 
> I wonder if we could/should try and switch to d0:v0 sooner during the
> build process? Is that even feasible?

We would hide out the problem and rely on hypercall_preempt_check to
always return 0 in non-hypercall context (i.e during DOM0 construction).

The problem may come back if we decide to change the behavior of
local_events_need_delivery or the interrupt subsystem.

I think this should be properly fix by calling hypercall_preempt_check
only when it's necessary.

Regards,

-- 
Julien Grall