On Wed, May 25, 2022, Venkatesh Srinivas wrote:
> From: Marc Orr <marcorr@xxxxxxxxxx>
>
> From: Venkatesh Srinivas <venkateshs@xxxxxxxxxxxx>
Heh, something is wonky in your setup, only Marc should get an explicit From:
> The upper bytes of any x2APIC register are reserved. Inject a #GP
> into the guest if any of these reserved bits are set.
>
> Signed-off-by: Marc Orr <marcorr@xxxxxxxxxx>
> Signed-off-by: Venkatesh Srinivas <venkateshs@xxxxxxxxxxxx>
> ---
> arch/x86/kvm/lapic.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 6f8522e8c492..617e4936c5cc 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -2907,6 +2907,8 @@ int kvm_x2apic_msr_write(struct kvm_vcpu *vcpu, u32 msr, u64 data)
>
> if (!lapic_in_kernel(vcpu) || !apic_x2apic_mode(apic))
> return 1;
> + else if (data >> 32)
> + return 1;
This is incorrect, ICR is a 64-bit value. Marc's changelog for the internal patch
was wrong, although the code was correct.
The correct upstream change is:
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 39b805666a18..54d0f350acdf 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -2892,6 +2892,9 @@ static int kvm_lapic_msr_write(struct kvm_lapic *apic, u32 reg, u64 data)
if (reg == APIC_ICR)
return kvm_x2apic_icr_write(apic, data);
+ if (data >> 32)
+ return 1;
+
return kvm_lapic_reg_write(apic, reg, (u32)data);
}
As penance for not testing, can you write a KUT testcase to hit all the registers?
You could use e.g. SIPI to test that a 64-bit ICR value is _not_ rejected.
One thought would be to base your testcase on top of similar changes to msr.c to
blast all of the MCE MSRs, e.g. add a path/helper to enable x2APIC and iterate over
all registers.
[*] https://lore.kernel.org/all/20220512233045.4125471-1-seanjc@xxxxxxxxxx
