Re: [PATCH] KVM: arm/arm64: vgic: Handle GICR_PENDBASER.PTZ filed as RAZ

From: Marc Zyngier <maz@kernel.org>
To: Zenghui Yu <yuzenghui@huawei.com>
Cc: andre.przywara@arm.com, linux-kernel@vger.kernel.org,
	eric.auger@redhat.com, wanghaibin.wang@huawei.com,
	kvmarm@lists.cs.columbia.edu,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH] KVM: arm/arm64: vgic: Handle GICR_PENDBASER.PTZ filed as RAZ
Date: Fri, 20 Dec 2019 13:07:05 +0000	[thread overview]
Message-ID: <71e3dcc00ad5ab8dffd732bfe7381705@www.loen.fr> (raw)
In-Reply-To: <20191220111833.1422-1-yuzenghui@huawei.com>

On 2019-12-20 11:18, Zenghui Yu wrote:
> Although guest will hardly read and use the PTZ (Pending Table Zero)
> bit in GICR_PENDBASER, let us emulate the architecture strictly.
> As per IHI 0069E 9.11.30, PTZ field is WO, and reads as 0.
>
> Signed-off-by: Zenghui Yu <yuzenghui@huawei.com>
> ---
>
> Noticed when checking all fields of GICR_PENDBASER register.
> But _not_ sure whether it's worth a fix, as Linux never sets
> the PTZ bit before enabling LPI (set GICR_CTLR_ENABLE_LPIS).
>
> And I wonder under which scenarios can this bit be written as 1.
> It seems difficult for software to determine whether the pending
> table contains all zeros when writing this bit.

This is a useless HW optimization, where it can avoid reading the
pending table the very first time you write to this register if
it is told that it is all zero. A decent ITS implementation
already has a mechanism to find out about the pending bits by
looking into the IMPDEF area (the first 1kB) of the pending table.
PTZ is just yet another way to do the same thing.

This can only happen once in the lifetime of the system (when 
allocating
the table), and Linux doesn't really care. As usual, the GIC is setting
the level of useless complexity pretty high...

>
>  virt/kvm/arm/vgic/vgic-mmio-v3.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c
> b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> index 7dfd15dbb308..ebc218840fc2 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> @@ -414,8 +414,11 @@ static unsigned long
> vgic_mmio_read_pendbase(struct kvm_vcpu *vcpu,
>  					     gpa_t addr, unsigned int len)
>  {
>  	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
> +	u64 value = vgic_cpu->pendbaser;
>
> -	return extract_bytes(vgic_cpu->pendbaser, addr & 7, len);
> +	value &= ~GICR_PENDBASER_PTZ;
> +
> +	return extract_bytes(value, addr & 7, len);
>  }
>
>  static void vgic_mmio_write_pendbase(struct kvm_vcpu *vcpu,

Otherwise looks good. I'll queue it with Eric's correction
to the subject line.

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel