Re: [PATCH 2/3] KVM: arm64: Replace vgic_v3_uaccess_read_pending with vgic_uaccess_read_pending
On Thu, 02 Jun 2022 21:06:42 +0100,
Eric Auger wrote:
>
> Hi Marc,
> On 6/2/22 10:30, Marc Zyngier wrote:
> > Now that GICv2 has a proper userspace accessor for the pending state,
> > switch GICv3 over to it, dropping the local version.
> >
> > Signed-off-by: Marc Zyngier
> > ---
> > arch/arm64/kvm/vgic/vgic-mmio-v3.c | 40 ++
> > 1 file changed, 2 insertions(+), 38 deletions(-)
> >
> > diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> > b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> > index f7aa7bcd6fb8..f15e29cc63ce 100644
> > --- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> > +++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> > @@ -353,42 +353,6 @@ static unsigned long vgic_mmio_read_v3_idregs(struct
> > kvm_vcpu *vcpu,
> > return 0;
> > }
> >
> > -static unsigned long vgic_v3_uaccess_read_pending(struct kvm_vcpu *vcpu,
> > - gpa_t addr, unsigned int len)
> > -{
> > - u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
> > - u32 value = 0;
> > - int i;
>
> > -
> > - /*
> > -* pending state of interrupt is latched in pending_latch variable.
> > -* Userspace will save and restore pending state and line_level
> > -* separately.
> > -* Refer to Documentation/virt/kvm/devices/arm-vgic-v3.rst
> > -* for handling of ISPENDR and ICPENDR.
> Don't know if you want a derivative of this comment in
> vgic_uaccess_read_pending()?
I don't find it specially helpful, but at the same time, it doesn't
hurt to move it around.
> > -*/
> > - for (i = 0; i < len * 8; i++) {
> > - struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
> > - bool state = irq->pending_latch;
> > -
> > - if (irq->hw && vgic_irq_is_sgi(irq->intid)) {
> > - int err;
> > -
> in __read_pending(), irq->irq_lock is hold which looks safer at 1st
> sight. If potentially fixing something this can be documented in the
> commit msg.
I don't think it fixes anything. The idea is that if you are
accessing the state from userspace, you already have stopped the VM,
and thus there is no concurrent modifications if the state.
> > - err = irq_get_irqchip_state(irq->host_irq,
> > - IRQCHIP_STATE_PENDING,
> > - );
> > - WARN_ON(err);
> > - }
> > -
> in __read_pending(), irq_is_pending(irq) is used instead of
> irq->pending_latch. for level sensitive IRQ this is not identical. This
> may also deserve some comment. The nuance may be related to the above
> comment.
That is a good point, and we should unify the userspace behaviours
between GICv2 and v3.
I'll respin the series shortly.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH 2/3] KVM: arm64: Replace vgic_v3_uaccess_read_pending with vgic_uaccess_read_pending
Hi Marc,
On 6/2/22 10:30, Marc Zyngier wrote:
> Now that GICv2 has a proper userspace accessor for the pending state,
> switch GICv3 over to it, dropping the local version.
>
> Signed-off-by: Marc Zyngier
> ---
> arch/arm64/kvm/vgic/vgic-mmio-v3.c | 40 ++
> 1 file changed, 2 insertions(+), 38 deletions(-)
>
> diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> index f7aa7bcd6fb8..f15e29cc63ce 100644
> --- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> +++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> @@ -353,42 +353,6 @@ static unsigned long vgic_mmio_read_v3_idregs(struct
> kvm_vcpu *vcpu,
> return 0;
> }
>
> -static unsigned long vgic_v3_uaccess_read_pending(struct kvm_vcpu *vcpu,
> - gpa_t addr, unsigned int len)
> -{
> - u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
> - u32 value = 0;
> - int i;
> -
> - /*
> - * pending state of interrupt is latched in pending_latch variable.
> - * Userspace will save and restore pending state and line_level
> - * separately.
> - * Refer to Documentation/virt/kvm/devices/arm-vgic-v3.rst
> - * for handling of ISPENDR and ICPENDR.
Don't know if you want a derivative of this comment in
vgic_uaccess_read_pending()?
> - */
> - for (i = 0; i < len * 8; i++) {
> - struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
> - bool state = irq->pending_latch;
> -
> - if (irq->hw && vgic_irq_is_sgi(irq->intid)) {
> - int err;
> -
in __read_pending(), irq->irq_lock is hold which looks safer at 1st
sight. If potentially fixing something this can be documented in the
commit msg.
> - err = irq_get_irqchip_state(irq->host_irq,
> - IRQCHIP_STATE_PENDING,
> - );
> - WARN_ON(err);
> - }
> -
in __read_pending(), irq_is_pending(irq) is used instead of
irq->pending_latch. for level sensitive IRQ this is not identical. This
may also deserve some comment. The nuance may be related to the above
comment.
Thanks
Eric
> - if (state)
> - value |= (1U << i);
> -
> - vgic_put_irq(vcpu->kvm, irq);
> - }
> -
> - return value;
> -}
> -
> static int vgic_v3_uaccess_write_pending(struct kvm_vcpu *vcpu,
>gpa_t addr, unsigned int len,
>unsigned long val)
> @@ -666,7 +630,7 @@ static const struct vgic_register_region
> vgic_v3_dist_registers[] = {
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ISPENDR,
> vgic_mmio_read_pending, vgic_mmio_write_spending,
> - vgic_v3_uaccess_read_pending, vgic_v3_uaccess_write_pending, 1,
> + vgic_uaccess_read_pending, vgic_v3_uaccess_write_pending, 1,
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ICPENDR,
> vgic_mmio_read_pending, vgic_mmio_write_cpending,
> @@ -750,7 +714,7 @@ static const struct vgic_register_region
> vgic_v3_rd_registers[] = {
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_LENGTH_UACCESS(SZ_64K + GICR_ISPENDR0,
> vgic_mmio_read_pending, vgic_mmio_write_spending,
> - vgic_v3_uaccess_read_pending, vgic_v3_uaccess_write_pending, 4,
> + vgic_uaccess_read_pending, vgic_v3_uaccess_write_pending, 4,
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_LENGTH_UACCESS(SZ_64K + GICR_ICPENDR0,
> vgic_mmio_read_pending, vgic_mmio_write_cpending,
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
[PATCH 2/3] KVM: arm64: Replace vgic_v3_uaccess_read_pending with vgic_uaccess_read_pending
Now that GICv2 has a proper userspace accessor for the pending state,
switch GICv3 over to it, dropping the local version.
Signed-off-by: Marc Zyngier
---
arch/arm64/kvm/vgic/vgic-mmio-v3.c | 40 ++
1 file changed, 2 insertions(+), 38 deletions(-)
diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
index f7aa7bcd6fb8..f15e29cc63ce 100644
--- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
+++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
@@ -353,42 +353,6 @@ static unsigned long vgic_mmio_read_v3_idregs(struct
kvm_vcpu *vcpu,
return 0;
}
-static unsigned long vgic_v3_uaccess_read_pending(struct kvm_vcpu *vcpu,
- gpa_t addr, unsigned int len)
-{
- u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
- u32 value = 0;
- int i;
-
- /*
-* pending state of interrupt is latched in pending_latch variable.
-* Userspace will save and restore pending state and line_level
-* separately.
-* Refer to Documentation/virt/kvm/devices/arm-vgic-v3.rst
-* for handling of ISPENDR and ICPENDR.
-*/
- for (i = 0; i < len * 8; i++) {
- struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
- bool state = irq->pending_latch;
-
- if (irq->hw && vgic_irq_is_sgi(irq->intid)) {
- int err;
-
- err = irq_get_irqchip_state(irq->host_irq,
- IRQCHIP_STATE_PENDING,
- );
- WARN_ON(err);
- }
-
- if (state)
- value |= (1U << i);
-
- vgic_put_irq(vcpu->kvm, irq);
- }
-
- return value;
-}
-
static int vgic_v3_uaccess_write_pending(struct kvm_vcpu *vcpu,
gpa_t addr, unsigned int len,
unsigned long val)
@@ -666,7 +630,7 @@ static const struct vgic_register_region
vgic_v3_dist_registers[] = {
VGIC_ACCESS_32bit),
REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ISPENDR,
vgic_mmio_read_pending, vgic_mmio_write_spending,
- vgic_v3_uaccess_read_pending, vgic_v3_uaccess_write_pending, 1,
+ vgic_uaccess_read_pending, vgic_v3_uaccess_write_pending, 1,
VGIC_ACCESS_32bit),
REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ICPENDR,
vgic_mmio_read_pending, vgic_mmio_write_cpending,
@@ -750,7 +714,7 @@ static const struct vgic_register_region
vgic_v3_rd_registers[] = {
VGIC_ACCESS_32bit),
REGISTER_DESC_WITH_LENGTH_UACCESS(SZ_64K + GICR_ISPENDR0,
vgic_mmio_read_pending, vgic_mmio_write_spending,
- vgic_v3_uaccess_read_pending, vgic_v3_uaccess_write_pending, 4,
+ vgic_uaccess_read_pending, vgic_v3_uaccess_write_pending, 4,
VGIC_ACCESS_32bit),
REGISTER_DESC_WITH_LENGTH_UACCESS(SZ_64K + GICR_ICPENDR0,
vgic_mmio_read_pending, vgic_mmio_write_cpending,
--
2.34.1
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
