Re: [PATCH v2 22/36] KVM: arm64: gic-v5: Trap and mask guest PPI register accesses

[Jonathan Cameron <jonathan.cameron@huawei.com>]

On Fri, 19 Dec 2025 15:52:43 +0000
Sascha Bischoff <Sascha.Bischoff@arm.com> wrote:

> A guest should not be able to detect if a PPI that is not exposed to
> the guest is implemented or not. If the writes to the PPI registers
> are not masked, it becomes possible for the guest to detect the
> presence of all implemented PPIs on the host.
> 
> Guest writes to the following registers are masked:
> 
> ICC_CACTIVERx_EL1
> ICC_SACTIVERx_EL1
> ICC_CPENDRx_EL1
> ICC_SPENDRx_EL1
> ICC_ENABLERx_EL1
> ICC_PRIORITYRx_EL1
> 
> When a guest writes these registers, the write is masked with the set
> of PPIs actually exposed to the guest, and the state is written back
> to KVM's shadow state..

One . seems enough.

> 
> Reads for the above registers are not masked. When the guest is
> running and reads from the above registers, it is presented with what
> KVM provides in the ICH_PPI_x_EL2 registers, which is the masked
> version of what the guest last wrote.
> 
> The ICC_PPI_HMRx_EL1 register is used to determine which PPIs use
> Level-sensitive semantics, and which use Edge. For a GICv5 guest, the
> correct view of the virtual PPIs must be provided to the guest, and
> hence this must also be trapped, but only for reads. The content of
> the HMRs is calculated and masked when finalising the PPI state for
> the guest.
> 
> Signed-off-by: Sascha Bischoff <sascha.bischoff@arm.com>
A few bits inline but nothing significant so I'll assume you tidy those up
Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>

> ---
>  arch/arm64/kvm/config.c   |  22 ++++++-
>  arch/arm64/kvm/sys_regs.c | 133 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 153 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/kvm/config.c b/arch/arm64/kvm/config.c
> index eb0c6f4d95b6d..f81bfdadd12fb 100644
> --- a/arch/arm64/kvm/config.c
> +++ b/arch/arm64/kvm/config.c
> @@ -1586,8 +1586,26 @@ static void __compute_ich_hfgrtr(struct kvm_vcpu *vcpu)
>  {
>  	__compute_fgt(vcpu, ICH_HFGRTR_EL2);
>  
> -	/* ICC_IAFFIDR_EL1 *always* needs to be trapped when running a guest */
> +	/*
> +	 * ICC_IAFFIDR_EL1 and ICH_PPI_HMRx_EL1 *always* needs to be

need to be

> +	 * trapped when running a guest.
> +	 **/

*/

>  	*vcpu_fgt(vcpu, ICH_HFGRTR_EL2) &= ~ICH_HFGRTR_EL2_ICC_IAFFIDR_EL1;
> +	*vcpu_fgt(vcpu, ICH_HFGRTR_EL2) &= ~ICH_HFGRTR_EL2_ICC_PPI_HMRn_EL1;
> +}

> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 383ada0d75922..cef13bf6bb3a1 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -696,6 +696,111 @@ static bool access_gicv5_iaffid(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
>  	return true;
>  }
>  
> +static bool access_gicv5_ppi_hmr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
> +				 const struct sys_reg_desc *r)
> +{
> +	if (p->is_write)
> +		return ignore_write(vcpu, p);
> +
> +	if (p->Op2 == 0) {	/* ICC_PPI_HMR0_EL1 */
> +		p->regval = vcpu->arch.vgic_cpu.vgic_v5.vgic_ppi_hmr[0];
> +	} else {		/* ICC_PPI_HMR1_EL1 */
> +		p->regval = vcpu->arch.vgic_cpu.vgic_v5.vgic_ppi_hmr[1];
> +	}

No {} as single line statements in all legs.

However, I'd be tempted to use a local variable for the index like you've
done in many other cases
	
	unsigned int index;

...

	index = p->Op2 == 0 ? 0 : 1;
	p->regval = vcpu->arch.vgic_cpu.vgic_v5.vgic_ppi_hrm[index];

Or use the p->Op2 % 2 as you do in ppi_enabler.


> +
> +	return true;
> +}
> +
> +static bool access_gicv5_ppi_enabler(struct kvm_vcpu *vcpu,
> +				     struct sys_reg_params *p,
> +				     const struct sys_reg_desc *r)
> +{
> +	struct vgic_v5_cpu_if *cpu_if = &vcpu->arch.vgic_cpu.vgic_v5;
> +	u64 masked_write;
> +
> +	/* We never expect to get here with a read! */
> +	if (WARN_ON_ONCE(!p->is_write))
> +		return undef_access(vcpu, p, r);
> +
> +	masked_write = p->regval & cpu_if->vgic_ppi_mask[p->Op2 % 2];
> +	cpu_if->vgic_ich_ppi_enabler_entry[p->Op2 % 2] = masked_write;
> +
> +	return true;
> +}
> +
> +static bool access_gicv5_ppi_pendr(struct kvm_vcpu *vcpu,
> +				   struct sys_reg_params *p,
> +				   const struct sys_reg_desc *r)
> +{
> +	struct vgic_v5_cpu_if *cpu_if = &vcpu->arch.vgic_cpu.vgic_v5;
> +	u64 masked_write;
> +
> +	/* We never expect to get here with a read! */
> +	if (WARN_ON_ONCE(!p->is_write))
> +		return undef_access(vcpu, p, r);
> +
> +	masked_write = p->regval & cpu_if->vgic_ppi_mask[p->Op2 % 2];
> +
> +	if (p->Op2 & 0x2) {	/* SPENDRx */
> +		cpu_if->vgic_ppi_pendr_entry[p->Op2 % 2] |= masked_write;
> +	} else {		/* CPENDRx */
> +		cpu_if->vgic_ppi_pendr_entry[p->Op2 % 2] &= ~masked_write;
> +	}

No {} wanted in kernel style when all legs are single line statements.
Same applies in a few other cases that follow.

> +
> +	return true;
> +}
> +