[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v5 3/6] xen/arm: vgic-v2: Don't ignore a write in ITARGETSR if one field is 0



On Mon, 9 Nov 2015, Julien Grall wrote:
> The current implementation ignores the whole write if one of the field is
> 0. Although, based on the spec (4.3.12 IHI 0048B.b), 0 is a valid value
> when:
>     - The interrupt is not wired in the distributor. From the Xen
>     point of view, it means that the corresponding bit is not set in
>     d->arch.vgic.allocated_irqs.
>     - The user wants to disable the IRQ forwarding in the distributor.
>     I.e the IRQ stays pending in the distributor and never received by
>     the guest.
> 
> Implementing the later will require more work in Xen because we always
> assume the interrupt is forwarded to a valid vCPU. So for now, ignore
> any field where the value is 0.
> 
> The emulation of the write access of ITARGETSR has been reworked and
> moved to a new function because it would have been difficult to
> implement properly the behavior with the current code.
> 
> The new implementation is breaking the register in 4 distinct bytes. For
> each byte, it will check the validity of the target list, find the new
> target, migrate the interrupt and store the value if necessary.
> 
> In the new implementation there is nearly no distinction of the access
> size to avoid having too many different path which is harder to test.
> 
> Signed-off-by: Julien Grall <julien.grall@xxxxxxxxxx>
> 
> ---
>     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.
> 
>     I'm not sure if this patch should be backported to Xen 4.6. Without
>     it any guest writing 0 in one the field won't be able to migrate
>     other interrupts. Although, in all the use case I've seen, the guest
>     is read ITARGETSR first and write-back the value with the
>     corresponding byte changed.
> 
>     Note that NR_BITS_PER_TARGET has been kept as a define because it
>     will be use in another function in the next patch and I wanted to
>     avoid having to duplicate const var = ...
> 
>     Changes in v5:
>         - s/NR_TARGET_PER_REG/NR_TARGETS_PER_ITARGETSR/
>         - s/NR_BIT_PER_TARGET/NR_BITS_PER_TARGET/
>         - Compute NR_BITS_PER_TARGET based on NR_TARGETS_PER_ITARGETSR
>         - Typoes in the comments
>         - Compute the shift for itargetsr on every loop
>         - Use gprintk(XENLOG_WARNING,...)
>         - Clarify commit message
> 
>     Changes in v4:
>         - Patch added
> ---
>  xen/arch/arm/vgic-v2.c | 145 
> ++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 102 insertions(+), 43 deletions(-)
> 
> diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
> index 486e497..ad2ea0a 100644
> --- a/xen/arch/arm/vgic-v2.c
> +++ b/xen/arch/arm/vgic-v2.c
> @@ -57,6 +57,98 @@ void vgic_v2_setup_hw(paddr_t dbase, paddr_t cbase, 
> paddr_t csize,
>      vgic_v2_hw.aliased_offset = aliased_offset;
>  }
>  
> +#define NR_TARGETS_PER_ITARGETSR    4U
> +#define NR_BITS_PER_TARGET  (32U / NR_TARGETS_PER_ITARGETSR)
> +
> +/*
> + * Store an ITARGETSR register. This function only deals with ITARGETSR8
> + * and onwards.
> + *
> + * Note the offset will be aligned to the appropriate boundary.
> + */
> +static void vgic_store_itargetsr(struct domain *d, struct vgic_irq_rank 
> *rank,
> +                                 unsigned int offset, uint32_t itargetsr)
> +{
> +    unsigned int i;
> +    unsigned int regidx = REG_RANK_INDEX(8, offset, DABT_WORD);
> +    unsigned int virq;
> +
> +    ASSERT(spin_is_locked(&rank->lock));
> +
> +    /*
> +     * The ITARGETSR0-7, used for SGIs/PPIs, are implemented RO in the
> +     * emulation and should never call this function.
> +     *
> +     * They all live in the first rank.
> +     */
> +    BUILD_BUG_ON(NR_INTERRUPT_PER_RANK != 32);
> +    ASSERT(rank->index >= 1);
> +
> +    offset &= INTERRUPT_RANK_MASK;
> +    offset &= ~(NR_TARGETS_PER_ITARGETSR - 1);
> +
> +    virq = rank->index * NR_INTERRUPT_PER_RANK + offset;

The patch looks good, but these three lines I think could be replaced
with:

virq = offset & ~(NR_TARGETS_PER_ITARGETSR - 1);

isn't it right?



> +    for ( i = 0; i < NR_TARGETS_PER_ITARGETSR; i++, offset++, virq++ )

offset is not needed in the loop


> +    {
> +        unsigned int new_target, old_target;
> +        uint8_t new_mask, old_mask;
> +
> +        /*
> +         * Don't need to mask as we rely on new_mask to fit for only one
> +         * target.
> +         */
> +        BUILD_BUG_ON((sizeof (new_mask) * 8) != NR_BITS_PER_TARGET);
> +
> +        new_mask = itargetsr >> (i * NR_BITS_PER_TARGET);
> +        old_mask = vgic_byte_read(rank->v2.itargets[regidx], i);
> +
> +        /*
> +         * SPIs are using the 1-N model (see 1.4.3 in ARM IHI 0048B).
> +         * While the interrupt could be set pending to all the vCPUs in
> +         * target list, it's not guaranteed by the spec.
> +         * For simplicity, always route the vIRQ to the first interrupt
> +         * in the target list
> +         */
> +        new_target = ffs(new_mask);
> +        old_target = ffs(old_mask);
> +
> +        /* The current target should always be valid */
> +        ASSERT(old_target && (old_target <= d->max_vcpus));
> +
> +        /*
> +         * Ignore the write request for this interrupt if the new target
> +         * is invalid.
> +         * XXX: From the spec, if the target list is not valid, the
> +         * interrupt should be ignored (i.e not forwarded to the
> +         * guest).
> +         */
> +        if ( !new_target || (new_target > d->max_vcpus) )
> +        {
> +            gprintk(XENLOG_WARNING,
> +                   "No valid vCPU found for vIRQ%u in the target list (%#x). 
> Skip it\n",
> +                   virq, new_mask);
> +            continue;
> +        }
> +
> +        /* The vCPU ID always starts from 0 */
> +        new_target--;
> +        old_target--;
> +
> +        /* Only migrate the vIRQ if the target vCPU has changed */
> +        if ( new_target != old_target )
> +        {
> +            vgic_migrate_irq(d->vcpu[old_target],
> +                             d->vcpu[new_target],
> +                             virq);
> +        }
> +
> +        /* Bit corresponding to unimplemented CPU is write-ignore. */
> +        new_mask &= (1 << d->max_vcpus) - 1;
> +        vgic_byte_write(&rank->v2.itargets[regidx], new_mask, i);
> +    }
> +}
> +
>  static int vgic_v2_distr_mmio_read(struct vcpu *v, mmio_info_t *info,
>                                     register_t *r, void *priv)
>  {
> @@ -344,56 +436,23 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, 
> mmio_info_t *info,
>  
>      case GICD_ITARGETSR8 ... GICD_ITARGETSRN:
>      {
> -        /* unsigned long needed for find_next_bit */
> -        unsigned long target;
> -        int i;
> +        uint32_t itargetsr;
> +
>          if ( dabt.size != DABT_BYTE && dabt.size != DABT_WORD ) goto 
> bad_width;
>          rank = vgic_rank_offset(v, 8, gicd_reg - GICD_ITARGETSR, DABT_WORD);
>          if ( rank == NULL) goto write_ignore;
> -        /* 8-bit vcpu mask for this domain */
> -        BUG_ON(v->domain->max_vcpus > 8);
> -        target = (1 << v->domain->max_vcpus) - 1;
> -        target = target | (target << 8) | (target << 16) | (target << 24);
> +        vgic_lock_rank(v, rank, flags);
>          if ( dabt.size == DABT_WORD )
> -            target &= r;
> +            itargetsr = r;
>          else
> -            target &= (r << (8 * (gicd_reg & 0x3)));
> -        /* ignore zero writes */
> -        if ( !target )
> -            goto write_ignore;
> -        /* For word reads ignore writes where any single byte is zero */
> -        if ( dabt.size == 2 &&
> -            !((target & 0xff) && (target & (0xff << 8)) &&
> -             (target & (0xff << 16)) && (target & (0xff << 24))))
> -            goto write_ignore;
> -        vgic_lock_rank(v, rank, flags);
> -        i = 0;
> -        while ( (i = find_next_bit(&target, 32, i)) < 32 )
>          {
> -            unsigned int irq, new_target, old_target;
> -            unsigned long old_target_mask;
> -            struct vcpu *v_target, *v_old;
> -
> -            new_target = i % 8;
> -            old_target_mask = 
> vgic_byte_read(rank->v2.itargets[REG_RANK_INDEX(8,
> -                                             gicd_reg - GICD_ITARGETSR, 
> DABT_WORD)], i/8);
> -            old_target = find_first_bit(&old_target_mask, 8);
> -
> -            if ( new_target != old_target )
> -            {
> -                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);
> -            }
> -            i += 8 - new_target;
> +            itargetsr = rank->v2.itargets[REG_RANK_INDEX(8,
> +                                    gicd_reg - GICD_ITARGETSR,
> +                                    DABT_WORD)];
> +            vgic_byte_write(&itargetsr, r, gicd_reg);
>          }
> -        if ( dabt.size == DABT_WORD )
> -            rank->v2.itargets[REG_RANK_INDEX(8, gicd_reg - GICD_ITARGETSR,
> -                                             DABT_WORD)] = target;
> -        else
> -            vgic_byte_write(&rank->v2.itargets[REG_RANK_INDEX(8,
> -                      gicd_reg - GICD_ITARGETSR, DABT_WORD)], r, gicd_reg);
> +        vgic_store_itargetsr(v->domain, rank, gicd_reg - GICD_ITARGETSR,
> +                             itargetsr);
>          vgic_unlock_rank(v, rank, flags);
>          return 1;
>      }
> -- 
> 2.1.4
> 

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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.