From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:56231) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TEiZr-0003pl-Ti for qemu-devel@nongnu.org; Thu, 20 Sep 2012 11:22:09 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TEiZn-00050t-LB for qemu-devel@nongnu.org; Thu, 20 Sep 2012 11:22:07 -0400 Received: from e06smtp13.uk.ibm.com ([195.75.94.109]:43995) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TEiZn-00050m-Bu for qemu-devel@nongnu.org; Thu, 20 Sep 2012 11:22:03 -0400 Received: from /spool/local by e06smtp13.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 20 Sep 2012 16:22:02 +0100 Received: from d06av10.portsmouth.uk.ibm.com (d06av10.portsmouth.uk.ibm.com [9.149.37.251]) by b06cxnps3075.portsmouth.uk.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id q8KFLkig52822194 for ; Thu, 20 Sep 2012 15:21:46 GMT Received: from d06av10.portsmouth.uk.ibm.com (loopback [127.0.0.1]) by d06av10.portsmouth.uk.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id q8KEiZqE002625 for ; Thu, 20 Sep 2012 10:44:36 -0400 Message-ID: <505B348E.7010808@de.ibm.com> Date: Thu, 20 Sep 2012 17:21:50 +0200 From: Christian Borntraeger MIME-Version: 1.0 References: <1345636483-6581-1-git-send-email-jfrei@linux.vnet.ibm.com> <24857C68-EAED-4C11-B931-4AEC31DB43D4@suse.de> <505B10D6.3060606@de.ibm.com> <2101761A-84C3-4B1A-9C85-9176F5774196@suse.de> In-Reply-To: <2101761A-84C3-4B1A-9C85-9176F5774196@suse.de> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] s390: use sync regs for register transfer List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Graf Cc: Cornelia Huck , Jens Freimann , Heinz Graalfs , qemu-devel , Einar Lueck On 20/09/12 16:13, Alexander Graf wrote: > > > On 20.09.2012, at 14:49, Christian Borntraeger 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