Re: [Xen-devel] [PATCH] xen/arm: implement GICD_I[S/C]ACTIVER reads

[Julien Grall <julien@xxxxxxx>]

Hi,

On 20/03/2020 01:03, Stefano Stabellini wrote:
> This is a simple implementation of GICD_ICACTIVER / GICD_ISACTIVER
> reads. It doesn't take into account the latest state of interrupts on
> other processors. Only the local processor is up-to-date.
The title and commit message suggests that GICD_I[S/C]ACTIVER will be 
implemented for all the vGIC we support. But the implementation is only 
for gicv3.
Technically, there is no difference between GICv2 and GICv3. So it 
should be trivial to implement in GICv2 (see how we deal with enabling 
IRQs).
Regarding the commit message itself:
  - IHMO, using "processor" is misleading. Can you make clear we are 
dealing with vCPU?
  - In the context of GICv3, it is not entirely clear what you mean by 
"local". Is it local as the resident vCPU or local as the vCPU 
associated to the re-distributor accessed?
Lastly, I am ok with a simple solution for ACTIVER, but I think the 
commit message should explain why a full solution is not possible.
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>
> Tested-by: Wei Xu <xuwei5@xxxxxxxxxxxxx>
> Tested-by: Peng Fan <peng.fan@xxxxxxx>
May I ask how this patch was tested?

> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
> index 4e60ba15cc..c9755ba45b 100644
> --- a/xen/arch/arm/vgic-v3.c
> +++ b/xen/arch/arm/vgic-v3.c
> @@ -713,9 +713,38 @@ static int __vgic_v3_distr_common_mmio_read(const char 
> *name, struct vcpu *v,
>           goto read_as_zero;
>   
>       /* Read the active status of an IRQ via GICD/GICR is not supported */
> -    case VRANGE32(GICD_ISACTIVER, GICD_ISACTIVER):
> +    case VRANGE32(GICD_ISACTIVER, GICD_ISACTIVERN):
>       case VRANGE32(GICD_ICACTIVER, GICD_ICACTIVERN):
> -        goto read_as_zero;
> +    { > +        bool invert = false;
> +        struct pending_irq *p;
> +        unsigned int start_irq, irq;
> +
> +        if ( reg < GICD_ISACTIVERN )
> +            start_irq = (reg - GICD_ISACTIVER) * 8;
> +        else
> +        {
> +            start_irq = (reg - GICD_ICACTIVER) * 8;
> +            invert = true;
The read value for ISACTIVER and ICACTIVER should be the same. So why do 
you need to invert the result?
> +        }
> +
> +        *r = 0;
> +
> +        /*
> +         * The following won't reflect the latest status of interrupts on
> +         * other vcpus.
> +         */
> +        for ( irq = start_irq; irq < start_irq + 32; irq++ )
You are assuming 32-bit access, but I can't find anywhere in your 
implementation forbidding 8-bit/16-bit access.
> +        {
> +            p = irq_to_pending(v, irq);
> +            if ( p != NULL && test_bit(GIC_IRQ_GUEST_ACTIVE, &p->status) )
irq_to_pending() will not return NULL if you are accessing a 
non-existing SPIs.
But I am not sure why you are not re-using the existing pattern to 
emulate registers. They are known to work and also make the code much 
easier to follow:
if ( datb.size != DABT_WORD ) goto bad_width;

rank = vgic_rank_offset(...)
if ( rank == NULL ) goto read_as_zero;
vgic_lock_rank(v, .... )

read active state for 32 interrupts

vgic_unlock_rank(v, ...)

*r = vreg_reg32_extract(..., info);

Note that the locking may not be necessary here. Also, I would like the 
"read activate state for 32 interrupts" to be part of a separate 
function so we can re-use it in the vGICv2 implementation.
> +                *r |= 1 << (irq - start_irq);
So this is going to introduced an undefined behavior because 1 << 31 
cannot be represented in an int. Please use 1U << ...
> +        }
> +        if ( invert )
> +            *r = ~(*r);
> +
> +        return 1;
> +    }
>   
>       case VRANGE32(GICD_IPRIORITYR, GICD_IPRIORITYRN):
>       {
Cheers,

--
Julien Grall

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