Re: [PATCH] kvm: initialize all of the kvm_debugregs structure before sending it to userspace

[Sean Christopherson <seanjc@google.com>]

On Mon, Apr 03, 2023, Mathias Krause wrote:
> On 20.02.23 11:40, Mathias Krause wrote:
> > VirtualBox and QEMU, OTOH, assume that the array is properly filled,
> > i.e. indices 0..3 map to DR0..3. This means, these users are currently
> > (and *always* have been) broken when trying to set DR1..3. Time to get
> > them fixed before x86-32 vanishes into irrelevance.

Practically speaking, KVM support for 32-bit host kernels has been irrelevant for
years.

> > [1] https://www.virtualbox.org/browser/vbox/trunk/src/VBox/VMM/VMMR3/NEMR3Native-linux.cpp?rev=98193#L1735
> > [2] https://gitlab.com/qemu-project/qemu/-/blob/v7.2.0/target/i386/kvm/kvm.c#L4480-4522
> > [3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/testing/selftests/kvm/include/x86_64/processor.h?h=v6.2#n722
> > 
> > An ABI-breaking^Wfixing change like below might be worth to apply on top
> > to get that long standing bug fixed:
> > 
> > -- >8 --
> > Subject: [PATCH] KVM: x86: Fix broken debugregs ABI for 32 bit kernels
> > 
> > The ioctl()s to get and set KVM's debug registers are broken for 32 bit
> > kernels as they'd only copy half of the user register state because of
> > the UAPI and in-kernel type mismatch (__u64 vs. unsigned long; 8 vs. 4
> > bytes).
> > 
> > This makes it impossible for userland to set anything but DR0 without
> > resorting to bit folding tricks.
> > 
> > Switch to a loop for copying debug registers that'll implicitly do the
> > type conversion for us, if needed.
> > 
> > This ABI breaking change actually fixes known users [1,2] that have been
> > broken since the API's introduction in commit a1efbe77c1fd ("KVM: x86:
> > Add support for saving&restoring debug registers").

Are there actually real users?  VMMs that invoke the ioctls(), sure.  But I highly
doubt there are actual deployments/users that run VMs on top of 32-bit kernels.

I like the patch, but would prefer not to mark it for stable, and definitely don't
want the changelog to incorrectly assert that there actually users that would
benefit from the fix.

The only reason we haven't deprecated support for KVM on 32-bit kernels is because
we want to be able to test nested TDP with a 32-bit L1 hypervisor, but I'm starting
to think even that is a weak excuse.   The only potential problem with using an old
kernel in L1 is that we _might_ not be able to test newfangled features.

> > Also take 'dr6' from the arch part directly, as we do for 'dr7'. There's
> > no need to take the clunky route via kvm_get_dr().

This belongs in a separate patch.