From mboxrd@z Thu Jan  1 00:00:00 1970
From: Julien Grall <julien.grall@linaro.org>
Subject: Re: [PATCH ARM v8 2/4] mini-os: arm: interrupt
	controller
Date: Wed, 22 Oct 2014 14:06:44 +0100
Message-ID: <5447ABE4.1030703@linaro.org>
References: <1412328051-20015-1-git-send-email-talex5@gmail.com>	<1412328051-20015-3-git-send-email-talex5@gmail.com>	<1413889218.23337.24.camel@citrix.com>	<20141021215406.GJ3481@type.youpi.perso.aquilenet.fr>
	<1413968619.20604.56.camel@citrix.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="iso-8859-1"
Content-Transfer-Encoding: quoted-printable
Return-path: <xen-devel-bounces@lists.xen.org>
Received: from mail6.bemta3.messagelabs.com ([195.245.230.39])
	by lists.xen.org with esmtp (Exim 4.72)
	(envelope-from <julien.grall@linaro.org>) id 1Xgvcs-0001go-98
	for xen-devel@lists.xenproject.org; Wed, 22 Oct 2014 13:06:54 +0000
Received: by mail-wi0-f172.google.com with SMTP id bs8so1310749wib.11
	for <xen-devel@lists.xenproject.org>;
	Wed, 22 Oct 2014 06:06:51 -0700 (PDT)
In-Reply-To: <1413968619.20604.56.camel@citrix.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Ian Campbell <Ian.Campbell@citrix.com>, Samuel Thibault <samuel.thibault@ens-lyon.org>
Cc: xen-devel@lists.xenproject.org, Thomas Leonard <talex5@gmail.com>, anil@recoil.org, Dave.Scott@eu.citrix.com, stefano.stabellini@eu.citrix.com
List-Id: xen-devel@lists.xenproject.org

On 10/22/2014 10:03 AM, Ian Campbell wrote:
> On Tue, 2014-10-21 at 23:54 +0200, Samuel Thibault wrote:
>> Ian Campbell, le Tue 21 Oct 2014 12:00:18 +0100, a =E9crit :
>>> On Fri, 2014-10-03 at 10:20 +0100, Thomas Leonard wrote:
>>>> +static inline uint32_t REG_READ32(volatile uint32_t *addr)
>>>> +{
>>>> +    uint32_t value;
>>>> +    __asm__ __volatile__("ldr %0, [%1]":"=3D&r"(value):"r"(addr));
>>>> +    rmb();
>>>
>>> I'm not 100% convinced that you need this rmb().

Most the GIC code doesn't require read barrier but...

>>>
>>>> +    return value;
>>>> +}
>>>> +
>>>> +static inline void REG_WRITE32(volatile uint32_t *addr, unsigned int =
value)
>>>> +{
>>>> +    __asm__ __volatile__("str %0, [%1]"::"r"(value), "r"(addr));
>>>> +    wmb();
>>>> +}

write barrier may be necessary on some, where we need to wait that all
write has been done before doing this one (such as enable the GIC ...).

So this function is buggy. It should be:

wmb();
__asm__ __volatile__(....).

>>
>> I don't really see why such barriers are needed indeed. Are they needed
>> to actually push the values out?
> =

> That would, I think, require an isb() (instruction barrier) whereas
> wmb() turns into a dsb() (data barrier). I expect you are write and
> these rmb/wmb are not needed, but an isb may be needed in the caller if
> they want to rely on the affect of a write (e.g. enabling the
> controller)

isb is useful for cache and system control registers. We don't use such
things in the GIC code, because the GIC register is memory mapped. We
only need to ensure that the write are ordered when it's necessary (only
few places).

Regards,

-- =

Julien Grall