On 20/09/12 16:13, Alexander Graf wrote: > > > On 20.09.2012, at 14:49, Christian Borntraeger <borntrae...@de.ibm.com> wrote: > >> [...] >>>> --- a/target-s390x/kvm.c >>>> +++ b/target-s390x/kvm.c >>>> @@ -88,50 +88,77 @@ void kvm_arch_reset_vcpu(CPUS390XState *env) >>>> /* FIXME: add code to reset vcpu. */ >>>> } >>>> >>>> +/* we want to have the prefix, the GPRS, the ACRS and the CRS up to date >>>> */ >>>> +#define QEMU_NEEDED_REGS (KVM_SYNC_PREFIX | KVM_SYNC_GPRS | \ >>>> + KVM_SYNC_ACRS | KVM_SYNC_CRS) >>>> + >>>> +/* But qemu only changes the GPRS */ >>>> +#define QEMU_DIRTY_REGS (KVM_SYNC_GPRS) >>>> + >>>> int kvm_arch_put_registers(CPUS390XState *env, int level) >>>> { >>>> struct kvm_regs regs; >>>> int ret; >>>> int i; >>>> >>>> - ret = kvm_vcpu_ioctl(env, KVM_GET_REGS, ®s); >>>> - if (ret < 0) { >>>> - return ret; >>>> - } >>>> - >>>> - for (i = 0; i < 16; i++) { >>>> - regs.gprs[i] = env->regs[i]; >>>> - } >>>> - >>>> - ret = kvm_vcpu_ioctl(env, KVM_SET_REGS, ®s); >>>> - if (ret < 0) { >>>> - return ret; >>>> - } >>>> - >>>> env->kvm_run->psw_addr = env->psw.addr; >>>> env->kvm_run->psw_mask = env->psw.mask; >>>> >>>> - return ret; >>>> + if ((env->kvm_run->kvm_valid_regs & QEMU_NEEDED_REGS) == >>>> QEMU_NEEDED_REGS) { >>> >>> Isn't this also missing a check for KVM_CAP_SYNC_REGS? >> >> I ommitted the check for two reasons: >> - since kvm_check_extension is an ioctl by itself, this would counter the >> original idea of sync regs >> - the kvm_run structure contains zeroes at the sync reg location in kernels >> that dont support that, >> (the area was unused and the page was allocated with GFP_ZERO) >> So by having any of the kvm_valid_regs != 0 we know that SYNC REGS must be >> supported > > But the spec explicitly says that the fields are only valid when the CAP is > present. So if you think that is unnecessary, please patch the spec.
Maybe I should do both. Update the spec that the CAP defines if this is any useful, if not it will start out with zeroes,but also having a single CAP check during startup to have the code cleaner. > The way we get around constant ioctls for cap checks is that we usually check > only once and remember the result in a global variable. Please check other > arch's kvm.c files for reference. I looked for that in i386, but seems that only ppc does it. So I will look more into ppc.... > >> >> >>> Also, on reset we probably also want to write the other registers back, >>> right? >> >> Well, on reset we call the initial cpu reset ioctl, which does reset the >> other registers mandated >> by a cpu reset. Furthermore, it is not different to the current >> implementation.... > > The difference is that get_registers fetches more registers, so not putting > them looks awkward. Yep, obviously the commenting was not enough to explain the why. > >> >> What we might want to do is to also consider a clear reset (which also >> zeroes the FPRs and ARs as >> well as memory etc) as an additional option. But this should not be the >> default, e.g. to make >> kdump or standalone dump possible. >> I think this would then be an addon-patch by itself. > > What does reset on normal systems look like? Does it clear register state? A reset does not, a reset clear does. But you just got me convinced, that we should really follow the level statement for the put function and simply write everything for KVM_PUT_FULL_STATE and KVM_PUT_RESET_STATE. The KVM_EXIT_S390_RESET handler must then do the right thing (e.g. making sure that the general purpose register content for a non-clear reset is not changed). Will look if that works out. Christian