* [Xen-devel] [PATCH] xen/arm: implement GICD_I[S/C]ACTIVER reads
@ 2020-03-20 1:03 Stefano Stabellini
2020-03-21 11:42 ` Julien Grall
0 siblings, 1 reply; 2+ messages in thread
From: Stefano Stabellini @ 2020-03-20 1:03 UTC (permalink / raw)
To: julien; +Cc: xen-devel, peng.fan, sstabellini, xuwei5
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.
Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
Tested-by: Wei Xu <xuwei5@hisilicon.com>
Tested-by: Peng Fan <peng.fan@nxp.com>
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;
+ }
+
+ *r = 0;
+
+ /*
+ * The following won't reflect the latest status of interrupts on
+ * other vcpus.
+ */
+ for ( irq = start_irq; irq < start_irq + 32; irq++ )
+ {
+ p = irq_to_pending(v, irq);
+ if ( p != NULL && test_bit(GIC_IRQ_GUEST_ACTIVE, &p->status) )
+ *r |= 1 << (irq - start_irq);
+ }
+ if ( invert )
+ *r = ~(*r);
+
+ return 1;
+ }
case VRANGE32(GICD_IPRIORITYR, GICD_IPRIORITYRN):
{
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [Xen-devel] [PATCH] xen/arm: implement GICD_I[S/C]ACTIVER reads
2020-03-20 1:03 [Xen-devel] [PATCH] xen/arm: implement GICD_I[S/C]ACTIVER reads Stefano Stabellini
@ 2020-03-21 11:42 ` Julien Grall
0 siblings, 0 replies; 2+ messages in thread
From: Julien Grall @ 2020-03-21 11:42 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: xen-devel, peng.fan, xuwei5
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@xilinx.com>
> Tested-by: Wei Xu <xuwei5@hisilicon.com>
> Tested-by: Peng Fan <peng.fan@nxp.com>
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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2020-03-21 11:42 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-20 1:03 [Xen-devel] [PATCH] xen/arm: implement GICD_I[S/C]ACTIVER reads Stefano Stabellini
2020-03-21 11:42 ` Julien Grall
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).