On Tue, Aug 01, 2023, Michal Luczaj wrote:
> On 8/1/23 01:49, Sean Christopherson wrote:
> >> A note: when servicing kvm_run->kvm_dirty_regs, changes made by
> >> __set_sregs()/kvm_vcpu_ioctl_x86_set_vcpu_events() to on-stack copies of
> >> vcpu->run.s.regs will not be reflected back in vcpu->run.s.regs. Is this
> >> ok?
> >
> > I would be amazed if anyone cares. Given the justification and the author,
> >
> > This reduces ioctl overhead which is particularly important when userspace
> > is making synchronous guest state modifications (e.g. when emulating and/or
> > intercepting instructions).
> >
> > Signed-off-by: Ken Hofsass <hofsass@xxxxxxxxxx>
> >
> > I am pretty sure this was added to optimize a now-abandoned Google effort to do
> > emulation in uesrspace. I bring that up because I was going to suggest that we
> > might be able to get away with a straight revert, as QEMU doesn't use the flag
> > and AFAICT neither does our VMM, but there are a non-zero number of hits in e.g.
> > github, so sadly I think we're stuck with the feature :-(
>
> All right, so assuming the revert is not happening and the API is not misused
> (i.e. unless vcpu->run->kvm_valid_regs is set, no one is expecting up to date
> values in vcpu->run->s.regs), is assignment copying
>
> struct kvm_vcpu_events events = vcpu->run->s.regs.events;
>
> the right approach or should it be a memcpy(), like in ioctl handlers?
Both approaches are fine, though I am gaining a preference for the copy-by-value
method. With gcc-12 and probably most compilers, the code generation is identical
for both as the compiler generates a call to memcpy() to handle the the struct
assignment.
The advantage of copy-by-value for structs, and why I think I now prefer it, is
that it provides type safety. E.g. this compiles without complaint
memcpy(&events, &vcpu->run->s.regs.sregs, sizeof(events));
whereas this
struct kvm_vcpu_events events = vcpu->run->s.regs.sregs;
yields
arch/x86/kvm/x86.c: In function ‘sync_regs’:
arch/x86/kvm/x86.c:11793:49: error: invalid initializer
11793 | struct kvm_vcpu_events events = vcpu->run->s.regs.sregs;
| ^~~~
The downside is that it's less obvious when reading the code that there is a
large-ish memcpy happening, but IMO it's worth gaining the type safety.
