Re: [Xen-devel] [PATCH 40/57] ARM: new VGIC: Add PRIORITY registers handlers

[Julien Grall <julien.grall@xxxxxxx>]

On 05/03/18 16:03, Andre Przywara wrote:
> The priority 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.
> There is a corner case when we change the priority of a pending
> interrupt which we don't handle at the moment.
I don't believe it is a corner case. The spec (8.9.12 ARM IHI 0069d) says:

"Implementations must ensure that an interrupt that is pending at the 
time of the write uses either the old value or the new value and must 
ensure that the interrupt is neither lost nor handled more than once. 
The effect of the change must be visible in finite time."
So the current implementation looks compliant to the spec.

> This is based on Linux commit dd238ec2b87b, written by Andre Przywara.
Using short commit ID is usually a pretty bad idea because it may not be 
uniq. For instance, a git show on my Linux tree will not be able to find it.
The full commit ID can feel a bit too long, so usually I give the short 
commit ID and the title.
But in that context, the commit message looks wrong. From my tree, it 
seems it is 055658bf48fcc6afdf90810e7e8f4e98f486c0d2.
> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxxxxx>
> ---
> Changelog RFC ... v1:
> - use 32 bit register types
>
>   xen/arch/arm/vgic/vgic-mmio-v2.c |  2 +-
>   xen/arch/arm/vgic/vgic-mmio.c    | 47 ++++++++++++++++++++++++++++++++++++++++
>   xen/arch/arm/vgic/vgic-mmio.h    |  7 ++++++
>   xen/arch/arm/vgic/vgic.h         |  2 ++
>   4 files changed, 57 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/vgic/vgic-mmio-v2.c b/xen/arch/arm/vgic/vgic-mmio-v2.c
> index c93455fbb2..29db9dec6f 100644
> --- a/xen/arch/arm/vgic/vgic-mmio-v2.c
> +++ b/xen/arch/arm/vgic/vgic-mmio-v2.c
> @@ -98,7 +98,7 @@ static const struct vgic_register_region 
> vgic_v2_dist_registers[] = {
>           vgic_mmio_read_active, vgic_mmio_write_cactive, 1,
>           VGIC_ACCESS_32bit),
>       REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_IPRIORITYR,
> -        vgic_mmio_read_raz, vgic_mmio_write_wi, 8,
> +        vgic_mmio_read_priority, vgic_mmio_write_priority, 8,
>           VGIC_ACCESS_32bit | VGIC_ACCESS_8bit),
>       REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_ITARGETSR,
>           vgic_mmio_read_raz, vgic_mmio_write_wi, 8,
> diff --git a/xen/arch/arm/vgic/vgic-mmio.c b/xen/arch/arm/vgic/vgic-mmio.c
> index c44d67082f..538f08bc66 100644
> --- a/xen/arch/arm/vgic/vgic-mmio.c
> +++ b/xen/arch/arm/vgic/vgic-mmio.c
> @@ -384,6 +384,53 @@ void vgic_mmio_write_sactive(struct vcpu *vcpu,
>       }
>   }
>   
> +unsigned long vgic_mmio_read_priority(struct vcpu *vcpu,
> +                                      paddr_t addr, unsigned int len)
> +{
> +    uint32_t intid = VGIC_ADDR_TO_INTID(addr, 8);
> +    unsigned int i;
> +    uint32_t val = 0;
> +
> +    for ( i = 0; i < len; i++ )
> +    {
> +        struct vgic_irq *irq = vgic_get_irq(vcpu->domain, vcpu, intid + i);
> +
> +        val |= (uint32_t)irq->priority << (i * 8);
> +
> +        vgic_put_irq(vcpu->domain, irq);
> +    }
> +
> +    return val;
> +}
> +
> +/*
> + * We currently don't handle changing the priority of an interrupt that
> + * is already pending on a VCPU. If there is a need for this, we would
> + * need to make this VCPU exit and re-evaluate the priorities, potentially
> + * leading to this interrupt getting presented now to the guest (if it has
> + * been masked by the priority mask before).
> + */
> +void vgic_mmio_write_priority(struct vcpu *vcpu,
> +                              paddr_t addr, unsigned int len,
> +                              unsigned long val)
> +{
> +    uint32_t intid = VGIC_ADDR_TO_INTID(addr, 8);
> +    unsigned int i;
> +    unsigned long flags;
> +
> +    for ( i = 0; i < len; i++ )
> +    {
> +        struct vgic_irq *irq = vgic_get_irq(vcpu->domain, vcpu, intid + i);
> +
> +        spin_lock_irqsave(&irq->irq_lock, flags);
> +        /* Narrow the priority range to what we actually support */
> +        irq->priority = (val >> (i * 8)) & GENMASK(7, 8 - VGIC_PRI_BITS);
> +        spin_unlock_irqrestore(&irq->irq_lock, flags);
> +
> +        vgic_put_irq(vcpu->domain, irq);
> +    }
> +}
> +
>   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 8604720628..e3f9029344 100644
> --- a/xen/arch/arm/vgic/vgic-mmio.h
> +++ b/xen/arch/arm/vgic/vgic-mmio.h
> @@ -129,6 +129,13 @@ void vgic_mmio_write_sactive(struct vcpu *vcpu,
>                                paddr_t addr, unsigned int len,
>                                unsigned long val);
>   
> +unsigned long vgic_mmio_read_priority(struct vcpu *vcpu,
> +                      paddr_t addr, unsigned int len);
> +
> +void vgic_mmio_write_priority(struct vcpu *vcpu,
> +                  paddr_t addr, unsigned int len,
> +                  unsigned long val);
> +
>   unsigned int vgic_v2_init_dist_iodev(struct vgic_io_device *dev);
>   
>   #endif
> diff --git a/xen/arch/arm/vgic/vgic.h b/xen/arch/arm/vgic/vgic.h
> index 68e205d10a..b294b04391 100644
> --- a/xen/arch/arm/vgic/vgic.h
> +++ b/xen/arch/arm/vgic/vgic.h
> @@ -20,6 +20,8 @@
>   #define PRODUCT_ID_XEN      0x58    /* ASCII code X */
>   #define IMPLEMENTER_ARM     0x43b
>   
> +#define VGIC_PRI_BITS       5
> +
>   #define vgic_irq_is_sgi(intid) ((intid) < VGIC_NR_SGIS)
>   
>   static inline bool irq_is_pending(struct vgic_irq *irq)
Cheers,

--
Julien Grall

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