[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


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 */
-        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;
+    }


Julien Grall

Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.