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.