Hi Marc,
On 17/04/2020 09:33, Marc Zyngier wrote:
> There is no point in accessing the HW when writing to any of the
> ISPENDR/ICPENDR registers from userspace, as only the guest should
> be allowed to change the HW state.
>
> Introduce new userspace-specific accessors that deal solely with
> the virtual state. Note that the API differs from that of GICv3,
> where userspace exclusively uses ISPENDR to set the state. Too
> bad we can't reuse it.
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> index f51c6e939c76..a016f07adc28 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> @@ -417,10 +417,12 @@ static const struct vgic_register_region vgic_v2_dist_registers[] = {
> NULL, vgic_uaccess_write_cenable, 1,
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_PENDING_SET,
> - vgic_mmio_read_pending, vgic_mmio_write_spending, NULL, NULL, 1,
> + vgic_mmio_read_pending, vgic_mmio_write_spending,
> + NULL, vgic_uaccess_write_spending, 1,
> VGIC_ACCESS_32bit),
vgic_mmio_write_spending() has some homebrew detection for is_uaccess, which causes
vgic_hw_irq_spending() to do nothing. Isn't that now dead-code with this change?
> diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
> index 6e30034d1464..f1927ae02d2e 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio.c
> @@ -321,6 +321,27 @@ void vgic_mmio_write_spending(struct kvm_vcpu *vcpu,
> +int vgic_uaccess_write_spending(struct kvm_vcpu *vcpu,
> + gpa_t addr, unsigned int len,
> + unsigned long val)
> +{
> + u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
> + int i;
> + unsigned long flags;
> +
> + for_each_set_bit(i, &val, len * 8) {
> + struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
vgic_mmio_write_spending() has:
| /* GICD_ISPENDR0 SGI bits are WI *
and bales out early. Is GIC_DIST_PENDING_SET the same register?
(If so, shouldn't that be true for PPI too?)
> + raw_spin_lock_irqsave(&irq->irq_lock, flags);
> + irq->pending_latch = true;
> + vgic_queue_irq_unlock(vcpu->kvm, irq, flags);
> +
> + vgic_put_irq(vcpu->kvm, irq);
> + }
> +
> + return 0;
> +}
> @@ -390,6 +411,26 @@ void vgic_mmio_write_cpending(struct kvm_vcpu *vcpu,
> +int vgic_uaccess_write_cpending(struct kvm_vcpu *vcpu,
> + gpa_t addr, unsigned int len,
> + unsigned long val)
> +{
> + u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
> + int i;
> + unsigned long flags;
> +
> + for_each_set_bit(i, &val, len * 8) {
> + struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
Same dumb question about GICD_ICPENDR0!?
> + raw_spin_lock_irqsave(&irq->irq_lock, flags);
> + irq->pending_latch = false;
> + raw_spin_unlock_irqrestore(&irq->irq_lock, flags);
> +
> + vgic_put_irq(vcpu->kvm, irq);
> + }
> +
> + return 0;
> +}
Thanks,
James
