diff mbox series

[RFC] KVM: arm64/vgic: Populate GICR_TYPER with Aff3

Message ID 20240205184326.78814-1-sauravsc@amazon.com
State New
Headers
Series [RFC] KVM: arm64/vgic: Populate GICR_TYPER with Aff3 |

Commit Message

Saurav Sachidanand Feb. 5, 2024, 6:43 p.m. UTC 
  According to spec, bits [63:56] of the GICR_TYPER register are supposed
to contain Affinity level 3 (Aff3) bits of the Processing Element (PE)
associated with its GIC redistributor. Linux guests on boot match PEs
with their redistributor using all four Affinity level bits from this
register.

Currently, vGIC populates GICR_TYPER with just the first three Affinity
levels of a vCPU's MPIDR. This works fine for a Linux guest that boots
with KVM's default vCPU MPIDR assignment, which also only populates till
the first three Affinity levels.

However, a hypervisor can override KVM's default MPIDR assignment by
writing directly to a vCPU's MPIDR_EL1 register. If such a hypervisor
were to populate Aff3 bits for a VM, a Linux guest booting there would
fail to match vCPUs with their vGIC redistributors, since their virtual
GICR_TYPER registers would be missing the respective Aff3 bits.

To change that, let's populate GICR_TYPER using Aff3 bits [39:32] from
the vCPU's MPIDR.

Signed-off-by: Saurav Sachidanand <sauravsc@amazon.com>
---
 arch/arm64/kvm/vgic/vgic-mmio-v3.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Oliver Upton Feb. 6, 2024, 10:28 a.m. UTC | #1 
Hi Saurav,

On Mon, Feb 05, 2024 at 06:43:26PM +0000, Saurav Sachidanand wrote:

[...]

> diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> index c15ee1df036a..26bc838ce14c 100644
> --- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> +++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> @@ -324,6 +324,7 @@ static unsigned long vgic_mmio_read_v3r_typer(struct kvm_vcpu *vcpu,
>  	u64 value;
>  
>  	value = (u64)(mpidr & GENMASK(23, 0)) << 32;
> +	value |= (u64)((mpidr >> 32) & GENMASK(7, 0)) << 56;

This looks suspiciously similar to what MPIDR_AFFINITY_LEVEL()
accomplishes. I think we can tolerate a few extra lines of code to make
the entire thing more self-documenting, like:

	value = ((u64)MPIDR_AFFINITY_LEVEL(mpidr, 3) << 56 |
		MPIDR_AFFINITY_LEVEL(mpidr, 2) << 48 |
		MPIDR_AFFINITY_LEVEL(mpidr, 1) << 40 |
		MPIDR_AFFINITY_LEVEL(mpidr, 0) << 32);
Marc Zyngier Feb. 6, 2024, 10:29 a.m. UTC | #2 
Hi Saurav,

On Mon, 05 Feb 2024 18:43:26 +0000,
Saurav Sachidanand <sauravsc@amazon.com> wrote:
> 
> According to spec, bits [63:56] of the GICR_TYPER register are supposed
> to contain Affinity level 3 (Aff3) bits of the Processing Element (PE)
> associated with its GIC redistributor. Linux guests on boot match PEs
> with their redistributor using all four Affinity level bits from this
> register.

If you read the spec carefully, you will notice that Aff3 support is
*optional*, and that it is on purpose that we don't support Aff3: this
cannot work for 32bit guests.

The other thing is that there is no guarantee that the HW actually
supports that either: ICH_VTR_EL2.A3V tells you whether the CPU
interface actually supports it.

> 
> Currently, vGIC populates GICR_TYPER with just the first three Affinity
> levels of a vCPU's MPIDR. This works fine for a Linux guest that boots
> with KVM's default vCPU MPIDR assignment, which also only populates till
> the first three Affinity levels.
> 
> However, a hypervisor can override KVM's default MPIDR assignment by
> writing directly to a vCPU's MPIDR_EL1 register. If such a hypervisor
> were to populate Aff3 bits for a VM, a Linux guest booting there would
> fail to match vCPUs with their vGIC redistributors, since their virtual
> GICR_TYPER registers would be missing the respective Aff3 bits.
> 
> To change that, let's populate GICR_TYPER using Aff3 bits [39:32] from
> the vCPU's MPIDR.
> 
> Signed-off-by: Saurav Sachidanand <sauravsc@amazon.com>
> ---
>  arch/arm64/kvm/vgic/vgic-mmio-v3.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> index c15ee1df036a..26bc838ce14c 100644
> --- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> +++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> @@ -324,6 +324,7 @@ static unsigned long vgic_mmio_read_v3r_typer(struct kvm_vcpu *vcpu,
>  	u64 value;
>  
>  	value = (u64)(mpidr & GENMASK(23, 0)) << 32;
> +	value |= (u64)((mpidr >> 32) & GENMASK(7, 0)) << 56;
>  	value |= ((target_vcpu_id & 0xffff) << 8);
>  
>  	if (vgic_has_its(vcpu->kvm))

So if you consider the above remarks, this is neither sufficient nor
guaranteed to work. I'm not opposed to having full Aff3 support (most
of it is already there), but this needs to be done in a consistent
way, and consider both the case of 32bit guests (which are not going
away) and the capabilities of the HW (the current code being rather
inconsistent in this regard).

If you are willing to work on this, I'll happily review a more
complete series of patches.

Thanks,

	M.
diff mbox series

Patch

diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
index c15ee1df036a..26bc838ce14c 100644
--- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
+++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
@@ -324,6 +324,7 @@  static unsigned long vgic_mmio_read_v3r_typer(struct kvm_vcpu *vcpu,
 	u64 value;
 
 	value = (u64)(mpidr & GENMASK(23, 0)) << 32;
+	value |= (u64)((mpidr >> 32) & GENMASK(7, 0)) << 56;
 	value |= ((target_vcpu_id & 0xffff) << 8);
 
 	if (vgic_has_its(vcpu->kvm))