Re: [RFC PATCH 32/49] ARM: new VGIC: Add ACTIVE registers handlers

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

Hi Andre,

On 09/02/18 14:39, Andre Przywara wrote:
> The active register handlers are shared between the v2 and v3 emulation,
> so their implementation goes into vgic-mmio.c, to be easily referenced
> from the v3 emulation as well later.
> Since activation/deactivation of an interrupt may happen entirely in the
> guest without it ever exiting, we need some extra logic to properly track
> the active state.
> For clearing the active state, we would basically have to halt the guest
> to make sure this is properly propagated into the respective VCPUs.
> This is not yet implemented in Xen.

I don't mind if it is not implemented in the first series. But I think 
we need to make it clearer in the code.

> 
> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
> ---
>   xen/arch/arm/vgic/vgic-mmio-v2.c |  4 +-
>   xen/arch/arm/vgic/vgic-mmio.c    | 94 ++++++++++++++++++++++++++++++++++++++++
>   xen/arch/arm/vgic/vgic-mmio.h    | 11 +++++
>   3 files changed, 107 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/vgic/vgic-mmio-v2.c b/xen/arch/arm/vgic/vgic-mmio-v2.c
> index ceb86900a0..eba24d9866 100644
> --- a/xen/arch/arm/vgic/vgic-mmio-v2.c
> +++ b/xen/arch/arm/vgic/vgic-mmio-v2.c
> @@ -86,10 +86,10 @@ static const struct vgic_register_region vgic_v2_dist_registers[] = {
>           vgic_mmio_read_pending, vgic_mmio_write_cpending, NULL, NULL, 1,
>           VGIC_ACCESS_32bit),
>       REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_ISACTIVER,
> -        vgic_mmio_read_raz, vgic_mmio_write_wi, NULL, NULL, 1,
> +        vgic_mmio_read_active, vgic_mmio_write_sactive, NULL, NULL, 1,
>           VGIC_ACCESS_32bit),
>       REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_ICACTIVER,
> -        vgic_mmio_read_raz, vgic_mmio_write_wi, NULL, NULL, 1,
> +        vgic_mmio_read_active, vgic_mmio_write_cactive, NULL, NULL, 1,
>           VGIC_ACCESS_32bit),
>       REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_IPRIORITYR,
>           vgic_mmio_read_raz, vgic_mmio_write_wi, NULL, NULL, 8,
> diff --git a/xen/arch/arm/vgic/vgic-mmio.c b/xen/arch/arm/vgic/vgic-mmio.c
> index 9a65e39d78..ac3aa03fbc 100644
> --- a/xen/arch/arm/vgic/vgic-mmio.c
> +++ b/xen/arch/arm/vgic/vgic-mmio.c
> @@ -215,6 +215,100 @@ void vgic_mmio_write_cpending(struct vcpu *vcpu,
>       }
>   }
>   
> +unsigned long vgic_mmio_read_active(struct vcpu *vcpu,
> +                    paddr_t addr, unsigned int len)
> +{
> +    u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
> +    u32 value = 0;
> +    int i;
> +
> +    /* Loop over all IRQs affected by this read */
> +    for ( i = 0; i < len * 8; i++ )
> +    {
> +        struct vgic_irq *irq = vgic_get_irq(vcpu->domain, vcpu, intid + i);
> +
> +        if ( irq->active )
> +            value |= (1U << i);
> +
> +        vgic_put_irq(vcpu->domain, irq);
> +    }
> +
> +    return value;
> +}
> +
> +static void vgic_mmio_change_active(struct vcpu *vcpu, struct vgic_irq *irq,
> +                    bool new_active_state)
> +{

Can we add a gdprintk here? So if we don't implement it in the first 
series, we still keep the current warning?

> +}
> +
> +static void vgic_change_active_prepare(struct vcpu *vcpu, u32 intid)
> +{
> +}
> +
> +static void vgic_change_active_finish(struct vcpu *vcpu, u32 intid)
> +{
> +}
> +
> +static void __vgic_mmio_write_cactive(struct vcpu *vcpu,
> +                      paddr_t addr, unsigned int len,
> +                      unsigned long val)

Indentation.

> +{
> +    u32 intid = VGIC_ADDR_TO_INTID(addr, 1);

uint32_t

> +    int i;

unsigned int

> +
> +    for_each_set_bit( i, &val, len * 8 )
> +    {
> +        struct vgic_irq *irq = vgic_get_irq(vcpu->domain, vcpu, intid + i);

Newline here please.

> +        vgic_mmio_change_active(vcpu, irq, false);
> +        vgic_put_irq(vcpu->domain, irq);
> +    }
> +}
> +
> +void vgic_mmio_write_cactive(struct vcpu *vcpu,
> +                 paddr_t addr, unsigned int len,
> +                 unsigned long val)

Indentation

> +{
> +    u32 intid = VGIC_ADDR_TO_INTID(addr, 1);

uint32_t

> +
> +    spin_lock(&vcpu->domain->domain_lock);

If you want to use the domain lock, then please use domain_lock(..) and 
domain_unlock(..) macros. But I am not sure whether this is the right 
lock here and how long it will be taken.

> +    vgic_change_active_prepare(vcpu, intid);
> +
> +    __vgic_mmio_write_cactive(vcpu, addr, len, val);
> +
> +    vgic_change_active_finish(vcpu, intid);
> +    spin_unlock(&vcpu->domain->domain_lock);
> +}
> +
> +static void __vgic_mmio_write_sactive(struct vcpu *vcpu,
> +                      paddr_t addr, unsigned int len,
> +                      unsigned long val)
> +{
> +    u32 intid = VGIC_ADDR_TO_INTID(addr, 1);

uint32_t

> +    int i;

unsigned int.

> +
> +    for_each_set_bit( i, &val, len * 8 )
> +    {
> +        struct vgic_irq *irq = vgic_get_irq(vcpu->domain, vcpu, intid + i);
> +        vgic_mmio_change_active(vcpu, irq, true);
> +        vgic_put_irq(vcpu->domain, irq);
> +    }
> +}
> +
> +void vgic_mmio_write_sactive(struct vcpu *vcpu,
> +                 paddr_t addr, unsigned int len,
> +                 unsigned long val)
> +{
> +    u32 intid = VGIC_ADDR_TO_INTID(addr, 1);

uint32_t

> +
> +    spin_lock(&vcpu->domain->domain_lock);

See my remark on the domain_lock above.

> +    vgic_change_active_prepare(vcpu, intid);
> +
> +    __vgic_mmio_write_sactive(vcpu, addr, len, val);
> +
> +    vgic_change_active_finish(vcpu, intid);
> +    spin_unlock(&vcpu->domain->domain_lock);
> +}
> +
>   static int match_region(const void *key, const void *elt)
>   {
>       const unsigned int offset = (unsigned long)key;
> diff --git a/xen/arch/arm/vgic/vgic-mmio.h b/xen/arch/arm/vgic/vgic-mmio.h
> index 209afbbb9a..39e854232e 100644
> --- a/xen/arch/arm/vgic/vgic-mmio.h
> +++ b/xen/arch/arm/vgic/vgic-mmio.h
> @@ -159,6 +159,17 @@ void vgic_mmio_write_cpending(struct vcpu *vcpu,
>                     paddr_t addr, unsigned int len,
>                     unsigned long val);
>   
> +unsigned long vgic_mmio_read_active(struct vcpu *vcpu,
> +                    paddr_t addr, unsigned int len);

Indentation and belows

> +
> +void vgic_mmio_write_cactive(struct vcpu *vcpu,
> +                 paddr_t addr, unsigned int len,
> +                 unsigned long val);
> +
> +void vgic_mmio_write_sactive(struct vcpu *vcpu,
> +                 paddr_t addr, unsigned int len,
> +                 unsigned long val);
> +
>   unsigned int vgic_v2_init_dist_iodev(struct vgic_io_device *dev);
>   
>   /* Find the proper register handler entry given a certain address offset */
> 

Cheers,

-- 
Julien Grall

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