From mboxrd@z Thu Jan  1 00:00:00 1970
From: Ian Campbell <ian.campbell@citrix.com>
Subject: Re: [PATCH v4 1/5] xen/arm: vgic-v2: Handle correctly
 byte write in ITARGETSR
Date: Fri, 23 Oct 2015 10:33:23 +0100
Message-ID: <1445592803.2374.83.camel@citrix.com>
References: <1444659760-24123-1-git-send-email-julien.grall@citrix.com>
	<1444659760-24123-2-git-send-email-julien.grall@citrix.com>
	<1445529185.2374.25.camel@citrix.com> <562910A1.5000606@citrix.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
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 <prvs=7311588c8=Ian.Campbell@citrix.com>)
	id 1ZpYjQ-0001CH-Fx
	for xen-devel@lists.xenproject.org; Fri, 23 Oct 2015 09:33:52 +0000
In-Reply-To: <562910A1.5000606@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: Julien Grall <julien.grall@citrix.com>, xen-devel@lists.xenproject.org
Cc: stefano.stabellini@eu.citrix.com
List-Id: xen-devel@lists.xenproject.org

On Thu, 2015-10-22 at 17:36 +0100, Julien Grall wrote:
> Hi Ian,
> 
> On 22/10/15 16:53, Ian Campbell wrote:
> > On Mon, 2015-10-12 at 15:22 +0100, Julien Grall wrote:
> > 
> > Subject: "correctly handle" and "writes"
> > 
> > > During a store, the byte is always in the low part of the register
> > > (i.e
> > > [0:7]).
> > > 
> > > Although, we are masking the register by using a shift of the
> > > byte offset in the ITARGETSR. This will result to get a target list
> > > equal to 0 which is ignored by the emulation.
> > 
> > I'm afraid I can't parse this.
> > 
> > I think instead of "Although" you might mean "incorrectly" as in "we
> > are
> > incorrectly...", but that would really then want the sentence to end
> > "instead of <the right thing>". So perhaps:
> > 
> >     We are incorrectly masking the register by using a shift of the
> > byte
> >     offset in the ITARGETSR instead of <...something...>. This will
> > result
> >     in a target list equal to 0 which is ignored by the emulation.
> 
> Rather than "instead of..." what about "while the byte is always in
> r[0:7]"?

Yes, I think that's ok.


> > > 
> > > Furthermore, the body of the loop is retrieving the old target list
> > > using the index of the byte.
> > > 
> > > To avoid modifying too much the loop, shift the byte stored to the
> > > correct
> > > offset.
> > 
> > That might have meant a smaller patch, but it's a lot harder to
> > understand
> > either the result or the diff.
> 
> The size of the patch would have been the same. Although, it requires to
> modify the call to vgic_byte_read in the loop to access the correct
> interrupt.
> 
> I didn't try to spend to much time to modify the loop because the
> follow-up patch (#2) will rewrite the loop.

Since this patch is marked for backport then if we decided to take #2 then
that's probably ok, otherwise the state of the tree after just this patch
is more relevant.

That's in itself probably a stronger argument for taking #2 than the actual
functionality of #2 is.


> 
> [...]
> 
> > >  xen/arch/arm/vgic-v2.c | 12 ++++++------
> > >  1 file changed, 6 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
> > > index 2d63e12..665afeb 100644
> > > --- a/xen/arch/arm/vgic-v2.c
> > > +++ b/xen/arch/arm/vgic-v2.c
> > > @@ -346,11 +346,11 @@ static int vgic_v2_distr_mmio_write(struct vcpu
> > > *v, mmio_info_t *info,
> > >          /* 8-bit vcpu mask for this domain */
> > >          BUG_ON(v->domain->max_vcpus > 8);
> > >          target = (1 << v->domain->max_vcpus) - 1;
> > > -        if ( dabt.size == 2 )
> > > -            target = target | (target << 8) | (target << 16) |
> > > (target << 24);
> > > +        target = target | (target << 8) | (target << 16) | (target
> > > << 24);
> > > +        if ( dabt.size == DABT_WORD )
> > > +            target &= r;
> > >          else
> > > -            target = (target << (8 * (gicd_reg & 0x3)));
> > > -        target &= r;
> > > +            target &= (r << (8 * (gicd_reg & 0x3)));
> > 
> > At this point do you not now have 3 bytes of
> >     (1 << v->domain->max_vcpus) - 1;
> > and 1 byte of that masked with the write?
> > 
> > IOW isn't this modifying the 3 bytes which aren't written?
> 
> No, the current loop search for bit set to 1. As the target variable
> will only contain one byte with some bit set to 1, only the IRQ
> associated to this byte will be updated.

Um, OK, I'll take your word for that.

Ian.