Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH] KVM: arm64: vgic-v3: Drop the reporting of
 GICR_TYPER.Last for userspace
To:     Zenghui Yu <yuzenghui@huawei.com>, kvmarm@lists.cs.columbia.edu,
        maz@kernel.org
Cc:     linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        james.morse@arm.com, julien.thierry.kdev@gmail.com,
        suzuki.poulose@arm.com, wanghaibin.wang@huawei.com,
        Keqian Zhu <zhukeqian1@huawei.com>
References: <20201117151629.1738-1-yuzenghui@huawei.com>
From:   Auger Eric <eric.auger@redhat.com>
Message-ID: <4c9c10f9-fc6d-2725-168e-db6442b74574@redhat.com>
Date:   Tue, 17 Nov 2020 18:29:50 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.5.0
MIME-Version: 1.0
In-Reply-To: <20201117151629.1738-1-yuzenghui@huawei.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Zenghui,

On 11/17/20 4:16 PM, Zenghui Yu wrote:
> It was recently reported that if GICR_TYPER is accessed before the RD base
> address is set, we'll suffer from the unset @rdreg dereferencing. Oops...
> 
> 	gpa_t last_rdist_typer = rdreg->base + GICR_TYPER +
> 			(rdreg->free_index - 1) * KVM_VGIC_V3_REDIST_SIZE;
> 
> It's "expected" that users will access registers in the redistributor if
> the RD has been properly configured (e.g., the RD base address is set). But
> it hasn't yet been covered by the existing documentation.
> 
> Per discussion on the list [1], the reporting of the GICR_TYPER.Last bit
> for userspace never actually worked. And it's difficult for us to emulate
> it correctly given that userspace has the flexibility to access it any
> time. Let's just drop the reporting of the Last bit for userspace for now
> (userspace should have full knowledge about it anyway) and it at least
> prevents kernel from panic ;-)
> 
> [1] https://lore.kernel.org/kvmarm/c20865a267e44d1e2c0d52ce4e012263@kernel.org/
> 
> Fixes: ba7b3f1275fd ("KVM: arm/arm64: Revisit Redistributor TYPER last bit computation")
> Reported-by: Keqian Zhu <zhukeqian1@huawei.com>
> Signed-off-by: Zenghui Yu <yuzenghui@huawei.com>
Given the state of last bit, it looks sensible atm.

Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric


> ---
> 
> This may be the easiest way to fix the issue and to get the fix backported
> to stable tree. There is still some work can be done since (at least) we
> have code duplicates between the MMIO and uaccess callbacks.
> 
>  arch/arm64/kvm/vgic/vgic-mmio-v3.c | 22 ++++++++++++++++++++--
>  1 file changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> index 52d6f24f65dc..15a6c98ee92f 100644
> --- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> +++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> @@ -273,6 +273,23 @@ static unsigned long vgic_mmio_read_v3r_typer(struct kvm_vcpu *vcpu,
>  	return extract_bytes(value, addr & 7, len);
>  }
>  
> +static unsigned long vgic_uaccess_read_v3r_typer(struct kvm_vcpu *vcpu,
> +						 gpa_t addr, unsigned int len)
> +{
> +	unsigned long mpidr = kvm_vcpu_get_mpidr_aff(vcpu);
> +	int target_vcpu_id = vcpu->vcpu_id;
> +	u64 value;
> +
> +	value = (u64)(mpidr & GENMASK(23, 0)) << 32;
> +	value |= ((target_vcpu_id & 0xffff) << 8);
> +
> +	if (vgic_has_its(vcpu->kvm))
> +		value |= GICR_TYPER_PLPIS;
> +
> +	/* reporting of the Last bit is not supported for userspace */
> +	return extract_bytes(value, addr & 7, len);
> +}
> +
>  static unsigned long vgic_mmio_read_v3r_iidr(struct kvm_vcpu *vcpu,
>  					     gpa_t addr, unsigned int len)
>  {
> @@ -593,8 +610,9 @@ static const struct vgic_register_region vgic_v3_rd_registers[] = {
>  	REGISTER_DESC_WITH_LENGTH(GICR_IIDR,
>  		vgic_mmio_read_v3r_iidr, vgic_mmio_write_wi, 4,
>  		VGIC_ACCESS_32bit),
> -	REGISTER_DESC_WITH_LENGTH(GICR_TYPER,
> -		vgic_mmio_read_v3r_typer, vgic_mmio_write_wi, 8,
> +	REGISTER_DESC_WITH_LENGTH_UACCESS(GICR_TYPER,
> +		vgic_mmio_read_v3r_typer, vgic_mmio_write_wi,
> +		vgic_uaccess_read_v3r_typer, vgic_mmio_uaccess_write_wi, 8,
>  		VGIC_ACCESS_64bit | VGIC_ACCESS_32bit),
>  	REGISTER_DESC_WITH_LENGTH(GICR_WAKER,
>  		vgic_mmio_read_raz, vgic_mmio_write_wi, 4,
>