From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH v4 1/5] xen/arm: vgic-v2: Handle correctly byte write in ITARGETSR Date: Fri, 23 Oct 2015 10:58:24 +0100 Message-ID: <562A04C0.7050109@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> <1445592803.2374.83.camel@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1ZpZ8i-0005Xj-Rh for xen-devel@lists.xenproject.org; Fri, 23 Oct 2015 10:00:01 +0000 In-Reply-To: <1445592803.2374.83.camel@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell , xen-devel@lists.xenproject.org Cc: stefano.stabellini@eu.citrix.com List-Id: xen-devel@lists.xenproject.org On 23/10/15 10:33, Ian Campbell wrote: > On Thu, 2015-10-22 at 17:36 +0100, Julien Grall wrote: >> On 22/10/15 16:53, Ian Campbell wrote: >>> On Mon, 2015-10-12 at 15:22 +0100, Julien Grall wrote: [...] >>>> >>>> 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. This code is already complex and I don't think the loop modification would have make the code easier to read. Although, my plan was to ask to backport the whole series once we exercise the code a bit in unstable. This is in order to fix 32-bit access on 64-bit register. >> >> [...] >> >>>> 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. FWIW, I wrote a Linux patch to exercise the code changed and I didn't see any possible issue: diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c index 982c09c..b6de74f 100644 --- a/drivers/irqchip/irq-gic.c +++ b/drivers/irqchip/irq-gic.c @@ -446,6 +446,7 @@ static void gic_cpu_if_up(struct gic_chip_data *gic) writel_relaxed(bypass | mode | GICC_ENABLE, cpu_base + GIC_CPU_CTRL); } +#include static void __init gic_dist_init(struct gic_chip_data *gic) { @@ -453,6 +454,7 @@ static void __init gic_dist_init(struct gic_chip_data *gic) u32 cpumask; unsigned int gic_irqs = gic->gic_irqs; void __iomem *base = gic_data_dist_base(gic); + void __iomem *target = base + GIC_DIST_TARGET; writel_relaxed(GICD_DISABLE, base + GIC_DIST_CTRL); @@ -465,6 +467,45 @@ static void __init gic_dist_init(struct gic_chip_data *gic) for (i = 32; i < gic_irqs; i += 4) writel_relaxed(cpumask, base + GIC_DIST_TARGET + i * 4 / 4); + for (i = 32; i < 34; i++) { + unsigned int n = i / 4; + unsigned int nb = i % 4; + int j; + void __iomem *regaddr = target + (i & ~0x3); + void __iomem *byte = target + i; + + xen_raw_printk("Exerce ITARGETSR for irq %u\n", i); + xen_raw_printk("\t 32-bit ITARGETSR%u = 0x%x\n", + n, readl_relaxed(regaddr)); + xen_raw_printk("\t 8-bit ITARGETSR%u[%u] = 0x%x\n", + n, nb, readb_relaxed(byte)); + for (j = 0; j < 5; j++) { + xen_raw_printk("switch to CPU%u\n", j); + writeb(1 << j, byte); + xen_raw_printk("\t 32-bit ITARGETSR%u = 0x%x\n", + n, readl_relaxed(regaddr)); + xen_raw_printk("\t 8-bit ITARGETSR%u[%u] = 0x%x\n", + n, nb, readb_relaxed(byte)); + } + } + + cpumask = 0x2; + cpumask |= cpumask << 8; + cpumask |= cpumask << 16; + xen_raw_printk("Excerce 32-bit access\n"); + for (i = 32; i < 38; i += 4) + { + unsigned int n = i / 4; + void __iomem *regaddr = target + i * 4 / 4; + + xen_raw_printk("Exerce 32-bit access on ITARGETSR%u\n", n); + xen_raw_printk("\t before = 0x%x\n", readl_relaxed(regaddr)); + writel_relaxed(cpumask, base + GIC_DIST_TARGET + i * 4 / 4); + xen_raw_printk("\t after = 0x%x\n", readl_relaxed(regaddr)); + } + + while (1); + gic_dist_config(base, gic_irqs, NULL); writel_relaxed(GICD_ENABLE, base + GIC_DIST_CTRL); Regards, -- Julien Grall