Re: [Xen-devel] [RFC 15/16] hack: move gicv2 LRs reads and writes out of spinlocks

[Julien Grall <Julien.Grall@xxxxxxx>]

Hi Andrii,

I can't comment on the cover letter because it is in-existent...
This series needs a lot of explanation why you are doing this way
and the performance improvement over multiple use case (not only
graphics benchmark). But...

On 28/11/2018 21:32, Andrii Anisov wrote:
> From: Andrii Anisov <andrii_anisov@xxxxxxxx>
> 
> Use values cached in `struct vcpu` for guest IRQs processing.
> This also reduces LR stores and restores on VCPU switch.

... I am trying to understand what is the benefits to improve the old 
vGIC when ideally we would like to get the new vGIC used in more place
as it is more compliant with the GIC spec. The more that this patch 
looks very similar to what the new vGIC does...

Cheers,

> 
> Signed-off-by: Andrii Anisov <andrii_anisov@xxxxxxxx>
> ---
>   xen/arch/arm/gic-v2.c     | 56 
> +++++++++++++++++++++++++++++++----------------
>   xen/arch/arm/gic-vgic.c   |  8 +++++--
>   xen/include/asm-arm/gic.h |  5 ++++-
>   3 files changed, 47 insertions(+), 22 deletions(-)
> 
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index 25147bd..a2eb0a7 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -176,31 +176,18 @@ static unsigned int gicv2_cpu_mask(const cpumask_t 
> *cpumask)
>   
>   static void gicv2_save_state(struct vcpu *v)
>   {
> -    int i;
> -
> -    /* No need for spinlocks here because interrupts are disabled around
> -     * this call and it only accesses struct vcpu fields that cannot be
> -     * accessed simultaneously by another pCPU.
> -     */
> -    for ( i = 0; i < gicv2_info.nr_lrs; i++ )
> -        v->arch.gic.v2.lr[i] = readl_gich(GICH_LR + i * 4);
> -
>       v->arch.gic.v2.apr = readl_gich(GICH_APR);
>       v->arch.gic.v2.vmcr = readl_gich(GICH_VMCR);
>       /* Disable until next VCPU scheduled */
>       writel_gich(0, GICH_HCR);
>   }
>   
> -static void gicv2_restore_state(const struct vcpu *v)
> +static void gicv2_restore_state(struct vcpu *v)
>   {
> -    int i;
> -
> -    for ( i = 0; i < gicv2_info.nr_lrs; i++ )
> -        writel_gich(v->arch.gic.v2.lr[i], GICH_LR + i * 4);
> -
>       writel_gich(v->arch.gic.v2.apr, GICH_APR);
>       writel_gich(v->arch.gic.v2.vmcr, GICH_VMCR);
>       writel_gich(GICH_HCR_EN, GICH_HCR);
> +    v->arch.gic.v2.lr_update_mask = (1 << gic_hw_ops->info->nr_lrs) - 1;
>   }
>   
>   static void gicv2_dump_state(const struct vcpu *v)
> @@ -493,6 +480,7 @@ static void gicv2_disable_interface(void)
>   static void gicv2_update_lr(int lr, unsigned int virq, uint8_t priority,
>                               unsigned int hw_irq, unsigned int state)
>   {
> +    struct vcpu *v = current;
>       uint32_t lr_reg;
>   
>       BUG_ON(lr >= gicv2_info.nr_lrs);
> @@ -507,19 +495,23 @@ static void gicv2_update_lr(int lr, unsigned int virq, 
> uint8_t priority,
>           lr_reg |= GICH_V2_LR_HW | ((hw_irq & GICH_V2_LR_PHYSICAL_MASK )
>                                      << GICH_V2_LR_PHYSICAL_SHIFT);
>   
> -    writel_gich(lr_reg, GICH_LR + lr * 4);
> +    v->arch.gic.v2.lr[lr] = lr_reg;
> +    v->arch.gic.v2.lr_update_mask |= 1<<lr;
>   }
>   
>   static void gicv2_clear_lr(int lr)
>   {
> -    writel_gich(0, GICH_LR + lr * 4);
> +    struct vcpu *v = current;
> +
> +    v->arch.gic.v2.lr[lr] = 0;
> +    v->arch.gic.v2.lr_update_mask |= 1<<lr;
>   }
>   
>   static void gicv2_read_lr(int lr, struct gic_lr *lr_reg)
>   {
>       uint32_t lrv;
>   
> -    lrv          = readl_gich(GICH_LR + lr * 4);
> +    lrv = current->arch.gic.v2.lr[lr];
>       lr_reg->virq = (lrv >> GICH_V2_LR_VIRTUAL_SHIFT) & 
> GICH_V2_LR_VIRTUAL_MASK;
>       lr_reg->priority = (lrv >> GICH_V2_LR_PRIORITY_SHIFT) & 
> GICH_V2_LR_PRIORITY_MASK;
>       lr_reg->pending = lrv & GICH_V2_LR_PENDING;
> @@ -546,6 +538,7 @@ static void gicv2_read_lr(int lr, struct gic_lr *lr_reg)
>   static void gicv2_write_lr(int lr, const struct gic_lr *lr_reg)
>   {
>       uint32_t lrv = 0;
> +    struct vcpu *v = current;
>   
>       lrv = (((lr_reg->virq & GICH_V2_LR_VIRTUAL_MASK) << 
> GICH_V2_LR_VIRTUAL_SHIFT)   |
>             ((uint32_t)(lr_reg->priority & GICH_V2_LR_PRIORITY_MASK)
> @@ -574,7 +567,30 @@ static void gicv2_write_lr(int lr, const struct gic_lr 
> *lr_reg)
>           lrv |= (uint32_t)lr_reg->virt.source << GICH_V2_LR_CPUID_SHIFT;
>       }
>   
> -    writel_gich(lrv, GICH_LR + lr * 4);
> +    v->arch.gic.v2.lr[lr] = lrv;
> +    v->arch.gic.v2.lr_update_mask |= 1<<lr;
> +}
> +
> +static void gicv2_fetch_lrs(struct vcpu *v)
> +{
> +    int i;
> +
> +    for ( i = 0; i < gicv2_info.nr_lrs; i++ )
> +        if ( this_cpu(lr_mask) & 1<<i )
> +            v->arch.gic.v2.lr[i] = readl_gich(GICH_LR + i * 4);
> +        else
> +            v->arch.gic.v2.lr[i] = 0;
> +}
> +
> +static void gicv2_push_lrs(struct vcpu *v)
> +{
> +    int i;
> +    uint64_t mask = v->arch.gic.v2.lr_update_mask;
> +
> +    for ( i = 0; i < gicv2_info.nr_lrs; i++ )
> +        if ( mask & 1<<i )
> +            writel_gich(v->arch.gic.v2.lr[i], GICH_LR + i * 4);
> +    v->arch.gic.v2.lr_update_mask = 0;
>   }
>   
>   static void gicv2_hcr_status(uint32_t flag, bool status)
> @@ -1360,6 +1376,8 @@ const static struct gic_hw_operations gicv2_ops = {
>       .map_hwdom_extra_mappings = gicv2_map_hwdown_extra_mappings,
>       .iomem_deny_access   = gicv2_iomem_deny_access,
>       .do_LPI              = gicv2_do_LPI,
> +    .fetch_lrs           = gicv2_fetch_lrs,
> +    .push_lrs            = gicv2_push_lrs,
>   };
>   
>   /* Set up the GIC */
> diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c
> index e511f91..86c8ae2 100644
> --- a/xen/arch/arm/gic-vgic.c
> +++ b/xen/arch/arm/gic-vgic.c
> @@ -277,6 +277,7 @@ void vgic_sync_from_lrs(struct vcpu *v)
>           return;
>   
>       gic_hw_ops->update_hcr_status(GICH_HCR_UIE, false);
> +    gic_hw_ops->fetch_lrs(v);
>   
>       spin_lock(&v->arch.vgic.lock);
>   
> @@ -403,11 +404,14 @@ int vgic_vcpu_pending_irq(struct vcpu *v)
>   
>   void vgic_sync_to_lrs(void)
>   {
> +    struct vcpu *v = current;
>       ASSERT(!local_irq_is_enabled());
>   
> -    gic_restore_pending_irqs(current);
> +    gic_restore_pending_irqs(v);
>   
> -    if ( !list_empty(&current->arch.vgic.lr_pending) && lr_all_full() )
> +    gic_hw_ops->push_lrs(v);
> +
> +    if ( !list_empty(&v->arch.vgic.lr_pending) && lr_all_full() )
>           gic_hw_ops->update_hcr_status(GICH_HCR_UIE, true);
>   }
>   
> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index 3d7394d..add2566 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -187,6 +187,7 @@ struct gic_v2 {
>       uint32_t vmcr;
>       uint32_t apr;
>       uint32_t lr[64];
> +    uint64_t lr_update_mask;
>   };
>   
>   /*
> @@ -323,7 +324,7 @@ struct gic_hw_operations {
>       /* Save GIC registers */
>       void (*save_state)(struct vcpu *);
>       /* Restore GIC registers */
> -    void (*restore_state)(const struct vcpu *);
> +    void (*restore_state)(struct vcpu *);
>       /* Dump GIC LR register information */
>       void (*dump_state)(const struct vcpu *);
>   
> @@ -384,6 +385,8 @@ struct gic_hw_operations {
>       int (*iomem_deny_access)(const struct domain *d);
>       /* Handle LPIs, which require special handling */
>       void (*do_LPI)(unsigned int lpi);
> +    void (*fetch_lrs) (struct vcpu *v);
> +    void (*push_lrs) (struct vcpu *v);
>   };
>   
>   extern const struct gic_hw_operations *gic_hw_ops;
> 

-- 
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel