[PATCH] xen/arm: Make local_events_need_delivery working with idle VPCU

[PATCH] xen/arm: Make local_events_need_delivery working with idle VPCU

[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) ****************************************

As an idle VCPU can never receive an event, return 0 when the current
VCPU is an idle VCPU in local_events_need_delivery.

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.

Although, I'm not sure this is the right fix for the bug.
---
 xen/include/asm-arm/event.h | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/xen/include/asm-arm/event.h b/xen/include/asm-arm/event.h
index 5330dfe..0149d06 100644
--- a/xen/include/asm-arm/event.h
+++ b/xen/include/asm-arm/event.h
@@ -39,7 +39,12 @@ static inline int local_events_need_delivery_nomask(void)
 
 static inline int local_events_need_delivery(void)
 {
-    if ( !vcpu_event_delivery_is_enabled(current) )
+    struct vcpu *v = current;
+
+    if ( unlikely(is_idle_vcpu(v)) )
+        return 0;
+
+    if ( !vcpu_event_delivery_is_enabled(v) )
         return 0;
     return local_events_need_delivery_nomask();
 }
-- 
2.1.4

Re: [PATCH] xen/arm: Make local_events_need_delivery working with idle VPCU

[Stefano Stabellini]

On Mon, 27 Apr 2015, 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) ****************************************
> 
> As an idle VCPU can never receive an event, return 0 when the current
> VCPU is an idle VCPU in local_events_need_delivery.
> 
> 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.
> 
> Although, I'm not sure this is the right fix for the bug.
> ---
>  xen/include/asm-arm/event.h | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/include/asm-arm/event.h b/xen/include/asm-arm/event.h
> index 5330dfe..0149d06 100644
> --- a/xen/include/asm-arm/event.h
> +++ b/xen/include/asm-arm/event.h
> @@ -39,7 +39,12 @@ static inline int local_events_need_delivery_nomask(void)
>  
>  static inline int local_events_need_delivery(void)
>  {
> -    if ( !vcpu_event_delivery_is_enabled(current) )
> +    struct vcpu *v = current;
> +
> +    if ( unlikely(is_idle_vcpu(v)) )
> +        return 0;
> +
> +    if ( !vcpu_event_delivery_is_enabled(v) )
>          return 0;
>      return local_events_need_delivery_nomask();
>  }

Is it actually considered correct in Xen to call hypercall_preempt_check
and/or local_events_need_delivery on the idle vcpu?

Shouldn't it be avoided and maybe a BUG_ON added here instead?

Re: [PATCH] xen/arm: Make local_events_need_delivery working with idle VPCU

[Julien Grall]

Hi Stefano,

On 27/04/15 16:36, Stefano Stabellini wrote:
> On Mon, 27 Apr 2015, 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) ****************************************
>>
>> As an idle VCPU can never receive an event, return 0 when the current
>> VCPU is an idle VCPU in local_events_need_delivery.
>>
>> 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.
>>
>> Although, I'm not sure this is the right fix for the bug.
>> ---
>>  xen/include/asm-arm/event.h | 7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/include/asm-arm/event.h b/xen/include/asm-arm/event.h
>> index 5330dfe..0149d06 100644
>> --- a/xen/include/asm-arm/event.h
>> +++ b/xen/include/asm-arm/event.h
>> @@ -39,7 +39,12 @@ static inline int local_events_need_delivery_nomask(void)
>>  
>>  static inline int local_events_need_delivery(void)
>>  {
>> -    if ( !vcpu_event_delivery_is_enabled(current) )
>> +    struct vcpu *v = current;
>> +
>> +    if ( unlikely(is_idle_vcpu(v)) )
>> +        return 0;
>> +
>> +    if ( !vcpu_event_delivery_is_enabled(v) )
>>          return 0;
>>      return local_events_need_delivery_nomask();
>>  }
> 
> Is it actually considered correct in Xen to call hypercall_preempt_check
> and/or local_events_need_delivery on the idle vcpu?

It seems that the x86 version of hypercall_preempt_check is able to cope
with idle VCPU. Although, I'm not sure if there is path where
hypercall_preempt_check can be called on an idle VCPU (cc x86
maintainers for this purpose).

> Shouldn't it be avoided and maybe a BUG_ON added here instead?

This patch was the simple way to fix the bug. I have other ideas in mind
which require some rework in apply_p2m_changes.

I'll wait to see what x86 maintainers think.

Regards,

-- 
Julien Grall

Re: [PATCH] xen/arm: Make local_events_need_delivery working with idle VPCU

[Riku Voipio]

Hi,

On 27 April 2015 at 19:32, Julien Grall <julien.grall@citrix.com> wrote:
> Hi Stefano,
>
> On 27/04/15 16:36, Stefano Stabellini wrote:
>> On Mon, 27 Apr 2015, 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) ****************************************
>>>
>>> As an idle VCPU can never receive an event, return 0 when the current
>>> VCPU is an idle VCPU in local_events_need_delivery.
>>>
>>> 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.
>>>
>>> Although, I'm not sure this is the right fix for the bug.
>>> ---
>>>  xen/include/asm-arm/event.h | 7 ++++++-
>>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/xen/include/asm-arm/event.h b/xen/include/asm-arm/event.h
>>> index 5330dfe..0149d06 100644
>>> --- a/xen/include/asm-arm/event.h
>>> +++ b/xen/include/asm-arm/event.h
>>> @@ -39,7 +39,12 @@ static inline int local_events_need_delivery_nomask(void)
>>>
>>>  static inline int local_events_need_delivery(void)
>>>  {
>>> -    if ( !vcpu_event_delivery_is_enabled(current) )
>>> +    struct vcpu *v = current;
>>> +
>>> +    if ( unlikely(is_idle_vcpu(v)) )
>>> +        return 0;
>>> +
>>> +    if ( !vcpu_event_delivery_is_enabled(v) )
>>>          return 0;
>>>      return local_events_need_delivery_nomask();
>>>  }
>>
>> Is it actually considered correct in Xen to call hypercall_preempt_check
>> and/or local_events_need_delivery on the idle vcpu?
>
> It seems that the x86 version of hypercall_preempt_check is able to cope
> with idle VCPU. Although, I'm not sure if there is path where
> hypercall_preempt_check can be called on an idle VCPU (cc x86
> maintainers for this purpose).
>
>> Shouldn't it be avoided and maybe a BUG_ON added here instead?
>
> This patch was the simple way to fix the bug. I have other ideas in mind
> which require some rework in apply_p2m_changes.

> I'll wait to see what x86 maintainers think.

Any news on this front? The head Xen is still failing in LAVA. We are
risking that
new regressions get introduced undetected while we wait for the fix for this...

Riku

Re: [PATCH] xen/arm: Make local_events_need_delivery working with idle VPCU

[Julien Grall]

Hi,

On 04/05/2015 12:44, Riku Voipio wrote:
> On 27 April 2015 at 19:32, Julien Grall <julien.grall@citrix.com> wrote:
>> Hi Stefano,
>>
>> On 27/04/15 16:36, Stefano Stabellini wrote:
>>> On Mon, 27 Apr 2015, 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) ****************************************
>>>>
>>>> As an idle VCPU can never receive an event, return 0 when the current
>>>> VCPU is an idle VCPU in local_events_need_delivery.
>>>>
>>>> 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.
>>>>
>>>> Although, I'm not sure this is the right fix for the bug.
>>>> ---
>>>>   xen/include/asm-arm/event.h | 7 ++++++-
>>>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/xen/include/asm-arm/event.h b/xen/include/asm-arm/event.h
>>>> index 5330dfe..0149d06 100644
>>>> --- a/xen/include/asm-arm/event.h
>>>> +++ b/xen/include/asm-arm/event.h
>>>> @@ -39,7 +39,12 @@ static inline int local_events_need_delivery_nomask(void)
>>>>
>>>>   static inline int local_events_need_delivery(void)
>>>>   {
>>>> -    if ( !vcpu_event_delivery_is_enabled(current) )
>>>> +    struct vcpu *v = current;
>>>> +
>>>> +    if ( unlikely(is_idle_vcpu(v)) )
>>>> +        return 0;
>>>> +
>>>> +    if ( !vcpu_event_delivery_is_enabled(v) )
>>>>           return 0;
>>>>       return local_events_need_delivery_nomask();
>>>>   }
>>>
>>> Is it actually considered correct in Xen to call hypercall_preempt_check
>>> and/or local_events_need_delivery on the idle vcpu?
>>
>> It seems that the x86 version of hypercall_preempt_check is able to cope
>> with idle VCPU. Although, I'm not sure if there is path where
>> hypercall_preempt_check can be called on an idle VCPU (cc x86
>> maintainers for this purpose).
>>
>>> Shouldn't it be avoided and maybe a BUG_ON added here instead?
>>
>> This patch was the simple way to fix the bug. I have other ideas in mind
>> which require some rework in apply_p2m_changes.
>
>> I'll wait to see what x86 maintainers think.
>
> Any news on this front? The head Xen is still failing in LAVA. We are
> risking that
> new regressions get introduced undetected while we wait for the fix for this...

Most of the maintainers were at the Xen hackathon last week and it's a 
bank holiday in England. I expect to have feedback during this week.

Although, no big changes has been pushed to Xen upstream as the 
committers were away too. Therefore, we should not see any major regression.

Regards,

-- 
Julien Grall

Re: [PATCH] xen/arm: Make local_events_need_delivery working with idle VPCU

[Ian Campbell]

On Mon, 2015-04-27 at 17:32 +0100, Julien Grall wrote:
> >> diff --git a/xen/include/asm-arm/event.h b/xen/include/asm-arm/event.h
> >> index 5330dfe..0149d06 100644
> >> --- a/xen/include/asm-arm/event.h
> >> +++ b/xen/include/asm-arm/event.h
> >> @@ -39,7 +39,12 @@ static inline int local_events_need_delivery_nomask(void)
> >>  
> >>  static inline int local_events_need_delivery(void)
> >>  {
> >> -    if ( !vcpu_event_delivery_is_enabled(current) )
> >> +    struct vcpu *v = current;
> >> +
> >> +    if ( unlikely(is_idle_vcpu(v)) )
> >> +        return 0;
> >> +
> >> +    if ( !vcpu_event_delivery_is_enabled(v) )
> >>          return 0;
> >>      return local_events_need_delivery_nomask();
> >>  }
> > 
> > Is it actually considered correct in Xen to call hypercall_preempt_check
> > and/or local_events_need_delivery on the idle vcpu?
> 
> It seems that the x86 version of hypercall_preempt_check is able to cope
> with idle VCPU. 

AFAICT that's just a coincidence, since an idle vcpu won't ever have a
pending up call.

> Although, I'm not sure if there is path where
> hypercall_preempt_check can be called on an idle VCPU (cc x86
> maintainers for this purpose).
> 
> > Shouldn't it be avoided and maybe a BUG_ON added here instead?
> 
> This patch was the simple way to fix the bug. I have other ideas in mind
> which require some rework in apply_p2m_changes.
> 
> I'll wait to see what x86 maintainers think.

I'm inclined to just go with this patch for now, unless Stefano is
nacking it.

One question first: What aspect of local_events_need_delivery relies on
the vcpu not being an idle one? I suppose something is not initialised,
but what.

Ian.

Re: [PATCH] xen/arm: Make local_events_need_delivery working with idle VPCU

[Stefano Stabellini]

On Tue, 5 May 2015, Ian Campbell wrote:
> On Mon, 2015-04-27 at 17:32 +0100, Julien Grall wrote:
> > >> diff --git a/xen/include/asm-arm/event.h b/xen/include/asm-arm/event.h
> > >> index 5330dfe..0149d06 100644
> > >> --- a/xen/include/asm-arm/event.h
> > >> +++ b/xen/include/asm-arm/event.h
> > >> @@ -39,7 +39,12 @@ static inline int local_events_need_delivery_nomask(void)
> > >>  
> > >>  static inline int local_events_need_delivery(void)
> > >>  {
> > >> -    if ( !vcpu_event_delivery_is_enabled(current) )
> > >> +    struct vcpu *v = current;
> > >> +
> > >> +    if ( unlikely(is_idle_vcpu(v)) )
> > >> +        return 0;
> > >> +
> > >> +    if ( !vcpu_event_delivery_is_enabled(v) )
> > >>          return 0;
> > >>      return local_events_need_delivery_nomask();
> > >>  }
> > > 
> > > Is it actually considered correct in Xen to call hypercall_preempt_check
> > > and/or local_events_need_delivery on the idle vcpu?
> > 
> > It seems that the x86 version of hypercall_preempt_check is able to cope
> > with idle VCPU. 
> 
> AFAICT that's just a coincidence, since an idle vcpu won't ever have a
> pending up call.
> 
> > Although, I'm not sure if there is path where
> > hypercall_preempt_check can be called on an idle VCPU (cc x86
> > maintainers for this purpose).
> > 
> > > Shouldn't it be avoided and maybe a BUG_ON added here instead?
> > 
> > This patch was the simple way to fix the bug. I have other ideas in mind
> > which require some rework in apply_p2m_changes.
> > 
> > I'll wait to see what x86 maintainers think.
> 
> I'm inclined to just go with this patch for now, unless Stefano is
> nacking it.

I won't nack it.


> One question first: What aspect of local_events_need_delivery relies on
> the vcpu not being an idle one? I suppose something is not initialised,
> but what.
> 
> Ian.
> 
> 

Re: [PATCH] xen/arm: Make local_events_need_delivery working with idle VPCU

[Julien Grall]

Hi Ian,

On 05/05/15 12:40, Ian Campbell wrote:
> On Mon, 2015-04-27 at 17:32 +0100, Julien Grall wrote:
>>>> diff --git a/xen/include/asm-arm/event.h b/xen/include/asm-arm/event.h
>>>> index 5330dfe..0149d06 100644
>>>> --- a/xen/include/asm-arm/event.h
>>>> +++ b/xen/include/asm-arm/event.h
>>>> @@ -39,7 +39,12 @@ static inline int local_events_need_delivery_nomask(void)
>>>>  
>>>>  static inline int local_events_need_delivery(void)
>>>>  {
>>>> -    if ( !vcpu_event_delivery_is_enabled(current) )
>>>> +    struct vcpu *v = current;
>>>> +
>>>> +    if ( unlikely(is_idle_vcpu(v)) )
>>>> +        return 0;
>>>> +
>>>> +    if ( !vcpu_event_delivery_is_enabled(v) )
>>>>          return 0;
>>>>      return local_events_need_delivery_nomask();
>>>>  }
>>>
>>> Is it actually considered correct in Xen to call hypercall_preempt_check
>>> and/or local_events_need_delivery on the idle vcpu?
>>
>> It seems that the x86 version of hypercall_preempt_check is able to cope
>> with idle VCPU. 
> 
> AFAICT that's just a coincidence, since an idle vcpu won't ever have a
> pending up call.
> 
>> Although, I'm not sure if there is path where
>> hypercall_preempt_check can be called on an idle VCPU (cc x86
>> maintainers for this purpose).
>>
>>> Shouldn't it be avoided and maybe a BUG_ON added here instead?
>>
>> This patch was the simple way to fix the bug. I have other ideas in mind
>> which require some rework in apply_p2m_changes.
>>
>> I'll wait to see what x86 maintainers think.
> 
> I'm inclined to just go with this patch for now, unless Stefano is
> nacking it.

This patch seem to turn into a workaround, would it be better to move
check idle_check in apply_p2m_check?

I will prepare a follow-up to avoid properly the call
hypercall_preempt_check with idle_vcpu.

> One question first: What aspect of local_events_need_delivery relies on
> the vcpu not being an idle one? I suppose something is not initialised,
> but what.

Everything related to the vGIC is not initialized. It's used in
local_event_need_delivery_nomask (see irq_to_pending and
gic_events_need_devlivery).

-- 
Julien Grall

Re: [PATCH] xen/arm: Make local_events_need_delivery working with idle VPCU

[Ian Campbell]

On Tue, 2015-05-05 at 13:00 +0100, Julien Grall wrote:
> > I'm inclined to just go with this patch for now, unless Stefano is
> > nacking it.
> 
> This patch seem to turn into a workaround, would it be better to move
> check idle_check in apply_p2m_check?
> 
> I will prepare a follow-up to avoid properly the call
> hypercall_preempt_check with idle_vcpu.
> 
> > One question first: What aspect of local_events_need_delivery relies on
> > the vcpu not being an idle one? I suppose something is not initialised,
> > but what.
> 
> Everything related to the vGIC is not initialized. It's used in
> local_event_need_delivery_nomask (see irq_to_pending and
> gic_events_need_devlivery).

Would it be better to arrange that vcpu_event_delivery_is_enabled is
never true for an idle vcpu? Either with an explicit check or by
arranging regs->cpsr to say that?

Ian.

> 

Re: [PATCH] xen/arm: Make local_events_need_delivery working with idle VPCU

[Julien Grall]

On 05/05/15 13:29, Ian Campbell wrote:
> On Tue, 2015-05-05 at 13:00 +0100, Julien Grall wrote:
>>> I'm inclined to just go with this patch for now, unless Stefano is
>>> nacking it.
>>
>> This patch seem to turn into a workaround, would it be better to move
>> check idle_check in apply_p2m_check?
>>
>> I will prepare a follow-up to avoid properly the call
>> hypercall_preempt_check with idle_vcpu.
>>
>>> One question first: What aspect of local_events_need_delivery relies on
>>> the vcpu not being an idle one? I suppose something is not initialised,
>>> but what.
>>
>> Everything related to the vGIC is not initialized. It's used in
>> local_event_need_delivery_nomask (see irq_to_pending and
>> gic_events_need_devlivery).
> 
> Would it be better to arrange that vcpu_event_delivery_is_enabled is
> never true for an idle vcpu? Either with an explicit check or by
> arranging regs->cpsr to say that?

That could work.

Although, as discussed IRL, I will move the check of idle_vcpu in
apply_p2m_changes in a preempt variable.

Regards,

-- 
Julien Grall

Re: [PATCH] xen/arm: Make local_events_need_delivery working with idle VPCU

[Tamas K Lengyel]

[-- Attachment #1.1: Type: text/plain, Size: 2767 bytes --]

IMHO we should check if op==RELINQUISH || op==MEMACCESS before calling
hypercall_preempt_check.
That was a change I made - previously it was only called if the op was
RELINQUISH
and not the other way around.

Tamas

On Mon, Apr 27, 2015 at 4:39 PM, Julien Grall <julien.grall@citrix.com>
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) ****************************************
>
> As an idle VCPU can never receive an event, return 0 when the current
> VCPU is an idle VCPU in local_events_need_delivery.
>
> 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.
>
> Although, I'm not sure this is the right fix for the bug.
> ---
>  xen/include/asm-arm/event.h | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/xen/include/asm-arm/event.h b/xen/include/asm-arm/event.h
> index 5330dfe..0149d06 100644
> --- a/xen/include/asm-arm/event.h
> +++ b/xen/include/asm-arm/event.h
> @@ -39,7 +39,12 @@ static inline int
> local_events_need_delivery_nomask(void)
>
>  static inline int local_events_need_delivery(void)
>  {
> -    if ( !vcpu_event_delivery_is_enabled(current) )
> +    struct vcpu *v = current;
> +
> +    if ( unlikely(is_idle_vcpu(v)) )
> +        return 0;
> +
> +    if ( !vcpu_event_delivery_is_enabled(v) )
>          return 0;
>      return local_events_need_delivery_nomask();
>  }
> --
> 2.1.4
>
>

[-- Attachment #1.2: Type: text/html, Size: 3837 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH] xen/arm: Make local_events_need_delivery working with idle VPCU

[Julien Grall]

Hi Tamas,

On 27/04/15 16:40, Tamas K Lengyel wrote:
> IMHO we should check if op==RELINQUISH || op==MEMACCESS before
> calling hypercall_preempt_check. That was a change I made - previously
> it was only called if the op was RELINQUISH and not the other way around.

I though about it but it make the code a lot uglier. It would be worst
if we decide to preempt more operations in the future...

Regards,

-- 
Julien Grall

Re: [PATCH] xen/arm: Make local_events_need_delivery working with idle VPCU

[Tamas K Lengyel]

[-- Attachment #1.1: Type: text/plain, Size: 602 bytes --]

On Mon, Apr 27, 2015 at 6:13 PM, Julien Grall <julien.grall@citrix.com>
wrote:

> Hi Tamas,
>
> On 27/04/15 16:40, Tamas K Lengyel wrote:
> > IMHO we should check if op==RELINQUISH || op==MEMACCESS before
> > calling hypercall_preempt_check. That was a change I made - previously
> > it was only called if the op was RELINQUISH and not the other way around.
>
> I though about it but it make the code a lot uglier. It would be worst
> if we decide to preempt more operations in the future...
>
> Regards,
>
> --
> Julien Grall
>

Ack, that was my original reasoning as well for switching it up.

Tamas

[-- Attachment #1.2: Type: text/html, Size: 1131 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel