Re: [PATCH v4 1/5] xen/arm: vgic-v2: Handle correctly byte write in ITARGETSR

[Julien Grall <julien.grall@citrix.com>]

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 <xen/hvc-console.h>
 
 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