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

[Ian Campbell <ian.campbell@citrix.com>]

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.

(note also s/to get/in a/ in the second sentence)

> Because of that a guest won't be able to modify the any ITARGETSR using
> byte access. Note that the first byte of each register will still be
> writeable.

"Because of that the guest will only be able to modify the first byte in
each ITARGETSR"

In your version the "any ITARGETSR" in the first sentence is immediately
contradicted by the second sentence with gives an example of an ITARGETSR
which it can modify.

> 
> 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.

> 
> Signed-off-by: Julien Grall <julien.grall@citrix.com>
> 
> ----
>     This change used to be embedded in "xen/arm: vgic: Optimize the way
>     to store the target vCPU in the rank". It has been moved out to
>     avoid having too much functional changes in a single patch.
> 
>     This patch is a good candidate to backport to Xen 4.6 and Xen 4.5.
>     Without it a guest won't be able migrate an IRQ from one vCPU to
>     another if it's using byte access to write in ITARGETSR.
> 
>     Changes in v4:
>         - Patch added
> ---
>  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?

>          /* ignore zero writes */
>          if ( !target )
>              goto write_ignore;
> @@ -374,7 +374,7 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info,
>  
>              if ( new_target != old_target )
>              {
> -                irq = gicd_reg - GICD_ITARGETSR + (i / 8);
> +                irq = (gicd_reg & ~0x3) - GICD_ITARGETSR + (i / 8);
>                  v_target = v->domain->vcpu[new_target];
>                  v_old = v->domain->vcpu[old_target];
>                  vgic_migrate_irq(v_old, v_target, irq);
> @@ -386,7 +386,7 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info,
>                                               DABT_WORD)] = target;
>          else
>              vgic_byte_write(&rank->v2.itargets[REG_RANK_INDEX(8,
> -                      gicd_reg - GICD_ITARGETSR, DABT_WORD)], target, gicd_reg);
> +                      gicd_reg - GICD_ITARGETSR, DABT_WORD)], r, gicd_reg);
>          vgic_unlock_rank(v, rank, flags);
>          return 1;
>      }