From mboxrd@z Thu Jan  1 00:00:00 1970
From: Ian Campbell <ian.campbell@citrix.com>
Subject: Re: [RFC PATCH v3 07/18] xen/arm: ITS: implement
 hw_irq_controller for LPIs
Date: Thu, 2 Jul 2015 13:59:11 +0100
Message-ID: <1435841951.21469.343.camel@citrix.com>
References: <1434974517-12136-1-git-send-email-vijay.kilari@gmail.com>
	<1434974517-12136-8-git-send-email-vijay.kilari@gmail.com>
	<55896DEC.2030101@citrix.com> <1435579158.32500.288.camel@citrix.com>
	<CALicx6sc_PBm900=tV7HDWhY8-b5hSd28dn4eo5U4fnoQ+=V4A@mail.gmail.com>
	<1435840506.21469.337.camel@citrix.com>
	<CALicx6s-2j6e8EPHnnNdyE6D0oZmzG4VnKy5jak_WRC4NooCXw@mail.gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <CALicx6s-2j6e8EPHnnNdyE6D0oZmzG4VnKy5jak_WRC4NooCXw@mail.gmail.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: Vijay Kilari <vijay.kilari@gmail.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>, Prasun Kapoor <Prasun.Kapoor@caviumnetworks.com>, Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>, Tim Deegan <tim@xen.org>, "xen-devel@lists.xen.org" <xen-devel@lists.xen.org>, Julien Grall <julien.grall@citrix.com>, Stefano Stabellini <stefano.stabellini@citrix.com>, manish.jaggi@caviumnetworks.com
List-Id: xen-devel@lists.xenproject.org

On Thu, 2015-07-02 at 18:14 +0530, Vijay Kilari wrote:
> On Thu, Jul 2, 2015 at 6:05 PM, Ian Campbell <ian.campbell@citrix.com> wrote:
> > On Thu, 2015-07-02 at 17:51 +0530, Vijay Kilari wrote:
> >> On Mon, Jun 29, 2015 at 5:29 PM, Ian Campbell <ian.campbell@citrix.com> wrote:
> >> > On Tue, 2015-06-23 at 15:32 +0100, Julien Grall wrote:
> >> > [...]
> >> >> > +{
> >> >> > +    struct its_collection *col;
> >> >> > +    struct its_device *its_dev = get_irq_device(desc);
> >> >> > +    u8 *cfg;
> >> >> > +    u32 virq = irq_to_virq(desc);
> >> >> > +
> >> >> > +    ASSERT(virq < its_dev->nr_lpis);
> >> >> > +
> >> >> > +    cfg = gic_rdists->prop_page + desc->irq - NR_GIC_LPI;
> >> >> > +    if ( enable )
> >> >> > +        *cfg |= LPI_PROP_ENABLED;
> >> >> > +    else
> >> >> > +        *cfg &= ~LPI_PROP_ENABLED;
> >> >> > +
> >> >> > +    /*
> >> >> > +     * Make the above write visible to the redistributors.
> >> >> > +     * And yes, we're flushing exactly: One. Single. Byte.
> >> >> > +     * Humpf...
> >> >> > +     */
> >> >> > +    if ( gic_rdists->flags & RDIST_FLAGS_PROPBASE_NEEDS_FLUSHING )
> >> >> > +        clean_and_invalidate_dcache_va_range(cfg, sizeof(*cfg));
> >> >> > +    else
> >> >> > +        dsb(ishst);
> >> >> > +
> >> >> > +    /* Get collection id for this event id */
> >> >> > +    col = &its_dev->its->collections[virq % num_online_cpus()];
> >> >>
> >> >> This is fragile, you are assuming that num_online_cpus() will never
> >> >> change. Why don't you store the collection in every irq_desc?
> >> >
> >> > The original Linux code upon which this is based doesn't seem to need to
> >> > lookup the collection here, why is flushing needed for us but not Linux?
> >>
> >> We are writing to lpi property table. Even linux code flushes it.
> >
> > Sorry I was referring to the collection look up and inv, not the cache
> > flush, i.e. this bit:
> >
> > +    /* Get collection id for this event id */
> > +    col = &its_dev->its->collections[virq % num_online_cpus()];
> > +    its_send_inv(its_dev, col, virq);
> >
> > Linux doesn't seem to do that INV there.
> 
> Linux does INV.
> 
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/irqchip/irq-gic-v3-its.c?id=refs/tags/v4.1
> 
> line 555

So it does, not sure how I missed that when I first looked.

Linux's approach of saving collection in the its_dev seems preferable to
looking it up like this here though.

Ian.