[PATCH for 4.5] xen/arm: Initialize the domain vgic lock

[PATCH for 4.5] xen/arm: Initialize the domain vgic lock

[Julien Grall]

The domain vgic lock is used uninitialized.

Signed-off-by: Julien Grall <julien.grall@linaro.org>

---
    This is a bug fix for Xen 4.5 and Xen 4.4. The vgic lock is used
    unitialized. Luckily we only use the field "raw" which is reset to 0
    during the domain allocation.

    There is no harm to apply for Xen 4.5 because it will correctly set
    the spin_lock structure for a later usage.
---
 xen/arch/arm/vgic.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 97061ce..b8bd38b 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -90,6 +90,8 @@ int domain_vgic_init(struct domain *d)
         return -ENODEV;
     }
 
+    spin_lock_init(&d->arch.vgic.lock);
+
     d->arch.vgic.shared_irqs =
         xzalloc_array(struct vgic_irq_rank, DOMAIN_NR_RANKS(d));
     if ( d->arch.vgic.shared_irqs == NULL )
-- 
2.1.3

Re: [PATCH for 4.5] xen/arm: Initialize the domain vgic lock

[Ian Campbell]

On Wed, 2014-12-17 at 15:40 +0000, Julien Grall wrote:
> The domain vgic lock is used uninitialized.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

> ---
>     This is a bug fix for Xen 4.5 and Xen 4.4. The vgic lock is used
>     unitialized. Luckily we only use the field "raw" which is reset to 0
>     during the domain allocation.
> 
>     There is no harm to apply for Xen 4.5 because it will correctly set
>     the spin_lock structure for a later usage.

By your above reasoning there is also no point, is there? That said, I
think we should take this since as you say it is harmless and good
practice to initialise spinlocks even if not strictly necessary.

> ---
>  xen/arch/arm/vgic.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index 97061ce..b8bd38b 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -90,6 +90,8 @@ int domain_vgic_init(struct domain *d)
>          return -ENODEV;
>      }
>  
> +    spin_lock_init(&d->arch.vgic.lock);
> +
>      d->arch.vgic.shared_irqs =
>          xzalloc_array(struct vgic_irq_rank, DOMAIN_NR_RANKS(d));
>      if ( d->arch.vgic.shared_irqs == NULL )

Re: [PATCH for 4.5] xen/arm: Initialize the domain vgic lock

[Julien Grall]

Hi Ian,

On 18/12/2014 09:47, Ian Campbell wrote:
> On Wed, 2014-12-17 at 15:40 +0000, Julien Grall wrote:
>> The domain vgic lock is used uninitialized.
>>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
>
>> ---
>>      This is a bug fix for Xen 4.5 and Xen 4.4. The vgic lock is used
>>      unitialized. Luckily we only use the field "raw" which is reset to 0
>>      during the domain allocation.
>>
>>      There is no harm to apply for Xen 4.5 because it will correctly set
>>      the spin_lock structure for a later usage.
>
> By your above reasoning there is also no point, is there? That said, I
> think we should take this since as you say it is harmless and good
> practice to initialise spinlocks even if not strictly necessary.

It's necessary to initialize spinlocks, not all the field of the 
spinlock is using the value 0 at initialization time.

What I meant if the current usage is fine. But if we want to debug 
spinlock, it won't work.

Regards,

-- 
Julien Grall

Re: [PATCH for 4.5] xen/arm: Initialize the domain vgic lock

[Ian Campbell]

On Thu, 2014-12-18 at 12:05 +0000, Julien Grall wrote:
> Hi Ian,
> 
> On 18/12/2014 09:47, Ian Campbell wrote:
> > On Wed, 2014-12-17 at 15:40 +0000, Julien Grall wrote:
> >> The domain vgic lock is used uninitialized.
> >>
> >> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> >
> > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> >
> >> ---
> >>      This is a bug fix for Xen 4.5 and Xen 4.4. The vgic lock is used
> >>      unitialized. Luckily we only use the field "raw" which is reset to 0
> >>      during the domain allocation.
> >>
> >>      There is no harm to apply for Xen 4.5 because it will correctly set
> >>      the spin_lock structure for a later usage.
> >
> > By your above reasoning there is also no point, is there? That said, I
> > think we should take this since as you say it is harmless and good
> > practice to initialise spinlocks even if not strictly necessary.
> 
> It's necessary to initialize spinlocks, not all the field of the 
> spinlock is using the value 0 at initialization time.

You said "...we only use the field "raw" which is reset to 0...". Was
that statement inaccurate?

> What I meant if the current usage is fine. But if we want to debug 
> spinlock, it won't work.
> 
> Regards,
> 

Re: [PATCH for 4.5] xen/arm: Initialize the domain vgic lock

[Julien Grall]

On 18/12/2014 12:12, Ian Campbell wrote:
> On Thu, 2014-12-18 at 12:05 +0000, Julien Grall wrote:
>> Hi Ian,
>>
>> On 18/12/2014 09:47, Ian Campbell wrote:
>>> On Wed, 2014-12-17 at 15:40 +0000, Julien Grall wrote:
>>>> The domain vgic lock is used uninitialized.
>>>>
>>>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>>>
>>> Acked-by: Ian Campbell <ian.campbell@citrix.com>
>>>
>>>> ---
>>>>       This is a bug fix for Xen 4.5 and Xen 4.4. The vgic lock is used
>>>>       unitialized. Luckily we only use the field "raw" which is reset to 0
>>>>       during the domain allocation.
>>>>
>>>>       There is no harm to apply for Xen 4.5 because it will correctly set
>>>>       the spin_lock structure for a later usage.
>>>
>>> By your above reasoning there is also no point, is there? That said, I
>>> think we should take this since as you say it is harmless and good
>>> practice to initialise spinlocks even if not strictly necessary.
>>
>> It's necessary to initialize spinlocks, not all the field of the
>> spinlock is using the value 0 at initialization time.
>
> You said "...we only use the field "raw" which is reset to 0...". Was
> that statement inaccurate?

It's accurate in the current. I should have give more details earlier, 
sorry.

Regards,

-- 
Julien Grall

Re: [PATCH for 4.5] xen/arm: Initialize the domain vgic lock

[Konrad Rzeszutek Wilk]

On Thu, Dec 18, 2014 at 09:47:37AM +0000, Ian Campbell wrote:
> On Wed, 2014-12-17 at 15:40 +0000, Julien Grall wrote:
> > The domain vgic lock is used uninitialized.
> > 
> > Signed-off-by: Julien Grall <julien.grall@linaro.org>
> 
> Acked-by: Ian Campbell <ian.campbell@citrix.com>

Release-Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

> 
> > ---
> >     This is a bug fix for Xen 4.5 and Xen 4.4. The vgic lock is used
> >     unitialized. Luckily we only use the field "raw" which is reset to 0
> >     during the domain allocation.
> > 
> >     There is no harm to apply for Xen 4.5 because it will correctly set
> >     the spin_lock structure for a later usage.
> 
> By your above reasoning there is also no point, is there? That said, I
> think we should take this since as you say it is harmless and good
> practice to initialise spinlocks even if not strictly necessary.
> 
> > ---
> >  xen/arch/arm/vgic.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> > index 97061ce..b8bd38b 100644
> > --- a/xen/arch/arm/vgic.c
> > +++ b/xen/arch/arm/vgic.c
> > @@ -90,6 +90,8 @@ int domain_vgic_init(struct domain *d)
> >          return -ENODEV;
> >      }
> >  
> > +    spin_lock_init(&d->arch.vgic.lock);
> > +
> >      d->arch.vgic.shared_irqs =
> >          xzalloc_array(struct vgic_irq_rank, DOMAIN_NR_RANKS(d));
> >      if ( d->arch.vgic.shared_irqs == NULL )
> 
> 
> 

Re: [PATCH for 4.5] xen/arm: Initialize the domain vgic lock

[Ian Campbell]

On Fri, 2014-12-19 at 09:23 -0500, Konrad Rzeszutek Wilk wrote:
> On Thu, Dec 18, 2014 at 09:47:37AM +0000, Ian Campbell wrote:
> > On Wed, 2014-12-17 at 15:40 +0000, Julien Grall wrote:
> > > The domain vgic lock is used uninitialized.
> > > 
> > > Signed-off-by: Julien Grall <julien.grall@linaro.org>
> > 
> > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> 
> Release-Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Applied.