Re: [Xen-devel] [RFC PATCH 29/49] ARM: new VGIC: Add CTLR, TYPER and IIDR handlers

[Julien Grall <julien.grall@xxxxxxx>]

Hi,

On 09/02/18 14:39, Andre Przywara wrote:
> Those three registers are v2 emulation specific, so their implementation
> lives entirely in vgic-mmio-v2.c. Also they are handled in one function,
> as their implementation is pretty simple.
> When the guest enables the distributor, we kick all VCPUs to get
> potentially pending interrupts serviced.
>
> This is based on Linux commit 2b0cda878965, written by Marc Zyngier.
>
> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxxxxx>
> ---
>   xen/arch/arm/vgic/vgic-mmio-v2.c | 48 +++++++++++++++++++++++++++++++++++++++-
>   xen/arch/arm/vgic/vgic.c         | 15 +++++++++++++
>   xen/arch/arm/vgic/vgic.h         |  4 ++++
>   3 files changed, 66 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/vgic/vgic-mmio-v2.c b/xen/arch/arm/vgic/vgic-mmio-v2.c
> index ee685a5a07..0926b3243e 100644
> --- a/xen/arch/arm/vgic/vgic-mmio-v2.c
> +++ b/xen/arch/arm/vgic/vgic-mmio-v2.c
> @@ -20,9 +20,55 @@
>   #include "vgic.h"
>   #include "vgic-mmio.h"
>   
> +static unsigned long vgic_mmio_read_v2_misc(struct vcpu *vcpu,
> +                        paddr_t addr, unsigned int len)
Indentation.

> +{
> +    u32 value;
Please use uint32_t.

> +
> +    switch (addr & 0x0c)
Coding style switch ( ... ). But I do admit the mask is actually quite 
confusing. You rely on the caller only call it with the right reg here.
It is probably worth to have a comment explaining it.

> +    {
> +    case GICD_CTLR:
> +        value = vcpu->domain->arch.vgic.enabled ? GICD_CTL_ENABLE : 0;
> +        break;
> +    case GICD_TYPER:
> +        value = vcpu->domain->arch.vgic.nr_spis + VGIC_NR_PRIVATE_IRQS;
> +        value = (value >> 5) - 1;
> +        value |= (vcpu->domain->max_vcpus - 1) << 5;
> +        break;
> +    case GICD_IIDR:
> +        value = (PRODUCT_ID_KVM << 24) | (IMPLEMENTER_ARM << 0);
We probably want to use _ID_XEN here.

> +        break;
> +    default:
> +        return 0;
> +    }
> +
> +    return value;
> +}
> +
> +static void vgic_mmio_write_v2_misc(struct vcpu *vcpu,
> +                    paddr_t addr, unsigned int len,
> +                    unsigned long val)
Indentation.

> +{
> +    struct vgic_dist *dist = &vcpu->domain->arch.vgic;
> +    bool was_enabled = dist->enabled;
> +
> +    switch (addr & 0x0c)
Ditto for coding style switch and the mask.

> +    {
> +    case GICD_CTLR:
> +        dist->enabled = val & GICD_CTL_ENABLE;
> +        if ( !was_enabled && dist->enabled )
> +            vgic_kick_vcpus(vcpu->domain);
On Xen, this code is definitely not atomic if you have multiple callers.

> +        break;
> +    case GICD_TYPER:
> +    case GICD_IIDR:
> +        /* Nothing to do */
> +        return;
> +    }
> +}
> +
>   static const struct vgic_register_region vgic_v2_dist_registers[] = {
>       REGISTER_DESC_WITH_LENGTH(GICD_CTLR,
> -        vgic_mmio_read_raz, vgic_mmio_write_wi, 12,
> +        vgic_mmio_read_v2_misc, vgic_mmio_write_v2_misc, 12,
>           VGIC_ACCESS_32bit),
>       REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_IGROUPR,
>           vgic_mmio_read_rao, vgic_mmio_write_wi, NULL, NULL, 1,
> diff --git a/xen/arch/arm/vgic/vgic.c b/xen/arch/arm/vgic/vgic.c
> index 9e7fb1edcb..dc5e011fa3 100644
> --- a/xen/arch/arm/vgic/vgic.c
> +++ b/xen/arch/arm/vgic/vgic.c
> @@ -678,6 +678,21 @@ int gic_events_need_delivery(void)
>       return vgic_vcpu_pending_irq(current);
>   }
>   
> +void vgic_kick_vcpus(struct domain *d)
> +{
> +    struct vcpu *vcpu;
> +
> +    /*
> +     * We've injected an interrupt, time to find out who deserves
> +     * a good kick...
> +     */
> +    for_each_vcpu( d, vcpu )
> +    {
> +        if ( vgic_vcpu_pending_irq(vcpu) )
> +            vcpu_unblock(vcpu);
Unblock will not notify a vCPU running. You want also want to send SGI 
if the CPU is running.
> +    }
> +}
> +
>   /*
>    * Local variables:
>    * mode: C
> diff --git a/xen/arch/arm/vgic/vgic.h b/xen/arch/arm/vgic/vgic.h
> index 7747d3f3e0..82fe902e26 100644
> --- a/xen/arch/arm/vgic/vgic.h
> +++ b/xen/arch/arm/vgic/vgic.h
> @@ -17,6 +17,9 @@
>   #ifndef __XEN_ARM_VGIC_NEW_H__
>   #define __XEN_ARM_VGIC_NEW_H__
>   
> +#define PRODUCT_ID_KVM      0x4b    /* ASCII code K */
> +#define IMPLEMENTER_ARM     0x43b
> +
>   #define vgic_irq_is_sgi(intid) ((intid) < VGIC_NR_SGIS)
>   
>   static inline bool irq_is_pending(struct vgic_irq *irq)
> @@ -36,6 +39,7 @@ struct vgic_irq *vgic_get_irq(struct domain *d, struct vcpu 
> *vcpu,
>   void vgic_put_irq(struct domain *d, struct vgic_irq *irq);
>   bool vgic_queue_irq_unlock(struct domain *d, struct vgic_irq *irq,
>                  unsigned long flags);
> +void vgic_kick_vcpus(struct domain *d);
>   
>   static inline void vgic_get_irq_kref(struct vgic_irq *irq)
>   {
Cheers,

--
Julien Grall

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