Re: [Xen-devel] [PATCH v5 2/6] xen/arm: vgic-v2: Handle correctly byte write in ITARGETSR

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Mon, 9 Nov 2015, Julien Grall wrote:
> During a store, the byte is always in the low part of the register (i.e
> [0:7]).
> 
> We are incorrectly masking the register by using a shift of the byte
> offset in the ITARGETSR while the byte is alwasy in r[0:7]. This will
> result in a target list equal to 0 which is ignored by the emulation.
> 
> Because of that the guest will only be able to modify the first byte in
> each ITARGETSR.
> 
> 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.
> 
> Signed-off-by: Julien Grall <julien.grall@xxxxxxxxxx>

Acked-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>



>     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.
> 
>     Note that if we backport this patch alone, the resulting code in
>     earlier version of Xen will be complex to read. As the last patch
>     of this serie should also be backported, I'm planning to request
>     backport for the whole series.
> 
>     Changes in v5:
>         - Update commit message based on Ian's suggestion
> 
>     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 041291c..486e497 100644
> --- a/xen/arch/arm/vgic-v2.c
> +++ b/xen/arch/arm/vgic-v2.c
> @@ -353,11 +353,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)));
>          /* ignore zero writes */
>          if ( !target )
>              goto write_ignore;
> @@ -381,7 +381,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);
> @@ -393,7 +393,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;
>      }
> -- 
> 2.1.4
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel