From patchwork Fri May 19 16:00:16 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Halil Pasic X-Patchwork-Id: 9737623 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 447A7601A1 for ; Fri, 19 May 2017 16:03:51 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 3C2E828940 for ; Fri, 19 May 2017 16:03:51 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id A11F928944; Fri, 19 May 2017 16:03:49 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.9 required=2.0 tests=BAYES_00,RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 8348E28940 for ; Fri, 19 May 2017 16:03:48 +0000 (UTC) Received: from localhost ([::1]:59436 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dBkNX-00017t-3s for patchwork-qemu-devel@patchwork.kernel.org; Fri, 19 May 2017 12:03:47 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41114) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dBkKQ-000802-74 for qemu-devel@nongnu.org; Fri, 19 May 2017 12:00:36 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1dBkKM-0002to-8e for qemu-devel@nongnu.org; Fri, 19 May 2017 12:00:34 -0400 Received: from mx0a-001b2d01.pphosted.com ([148.163.156.1]:33736) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1dBkKL-0002tK-TN for qemu-devel@nongnu.org; Fri, 19 May 2017 12:00:30 -0400 Received: from pps.filterd (m0098399.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.0.20/8.16.0.20) with SMTP id v4JFwmSN006254 for ; Fri, 19 May 2017 12:00:27 -0400 Received: from e06smtp15.uk.ibm.com (e06smtp15.uk.ibm.com [195.75.94.111]) by mx0a-001b2d01.pphosted.com with ESMTP id 2ahw463n99-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for ; Fri, 19 May 2017 12:00:27 -0400 Received: from localhost by e06smtp15.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Fri, 19 May 2017 17:00:21 +0100 Received: from b06cxnps4074.portsmouth.uk.ibm.com (9.149.109.196) by e06smtp15.uk.ibm.com (192.168.101.145) with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted; Fri, 19 May 2017 17:00:17 +0100 Received: from d06av24.portsmouth.uk.ibm.com (d06av24.portsmouth.uk.ibm.com [9.149.105.60]) by b06cxnps4074.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id v4JG0Hhb37093602; Fri, 19 May 2017 16:00:17 GMT Received: from d06av24.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 0C05442041; Fri, 19 May 2017 16:58:35 +0100 (BST) Received: from d06av24.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id A4D894203F; Fri, 19 May 2017 16:58:34 +0100 (BST) Received: from oc3836556865.ibm.com (unknown [9.152.224.168]) by d06av24.portsmouth.uk.ibm.com (Postfix) with ESMTP; Fri, 19 May 2017 16:58:34 +0100 (BST) To: "Dr. David Alan Gilbert" References: <20170505173507.74077-1-pasic@linux.vnet.ibm.com> <20170505173507.74077-4-pasic@linux.vnet.ibm.com> <20170508164505.GG2120@work-vm> <3e0195e2-ada5-2efc-1feb-e2415761e7bc@linux.vnet.ibm.com> <20170515180129.GC2324@work-vm> <2bf9d9d3-86ed-da36-dc81-415065e08a91@linux.vnet.ibm.com> <20170519145502.GQ2081@work-vm> From: Halil Pasic Date: Fri, 19 May 2017 18:00:16 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: <20170519145502.GQ2081@work-vm> X-TM-AS-GCONF: 00 x-cbid: 17051916-0020-0000-0000-0000036E8889 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 17051916-0021-0000-0000-000041CFABE7 Message-Id: <5a2c4261-c97e-0b0c-24c5-0500948be0e9@linux.vnet.ibm.com> X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:, , definitions=2017-05-19_10:, , signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 suspectscore=0 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1703280000 definitions=main-1705190100 X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x [generic] [fuzzy] X-Received-From: 148.163.156.1 Subject: Re: [Qemu-devel] [PATCH 03/10] s390x/css: add vmstate entities for css X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Cornelia Huck , qemu-devel@nongnu.org, "Michael S. Tsirkin" Errors-To: qemu-devel-bounces+patchwork-qemu-devel=patchwork.kernel.org@nongnu.org Sender: "Qemu-devel" X-Virus-Scanned: ClamAV using ClamSMTP On 05/19/2017 04:55 PM, Dr. David Alan Gilbert wrote: > * Halil Pasic (pasic@linux.vnet.ibm.com) wrote: >> >> >> On 05/15/2017 08:01 PM, Dr. David Alan Gilbert wrote: >>> * Halil Pasic (pasic@linux.vnet.ibm.com) wrote: >>>> >>>> >>>> On 05/08/2017 06:45 PM, Dr. David Alan Gilbert wrote: >>>>> * Halil Pasic (pasic@linux.vnet.ibm.com) wrote: >>>>>> As a preparation for switching to a vmstate based migration let us >>>>>> introduce vmstate entities (e.g. VMStateDescription) for the css entities >>>>>> to be migrated. Alongside some comments explaining or indicating the not >>>>>> migration of certain members are introduced too. >>>>>> >>>>>> No changes in behavior, we just added some dead code -- which should >>>>>> rise to life soon. >>>>>> >>>>>> Signed-off-by: Halil Pasic >>>>>> --- >>>>>> hw/s390x/css.c | 276 +++++++++++++++++++++++++++++++++++++++++++++++++ >>>>>> include/hw/s390x/css.h | 10 +- >>>>>> 2 files changed, 285 insertions(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c >>>>>> index c03bb20..2bda7d0 100644 >>>>>> --- a/hw/s390x/css.c >>>>>> +++ b/hw/s390x/css.c >>>>>> @@ -20,29 +20,231 @@ >>>>>> #include "hw/s390x/css.h" >>>>>> #include "trace.h" >>>>>> #include "hw/s390x/s390_flic.h" >>>>>> +#include "hw/s390x/s390-virtio-ccw.h" >>>>>> >>>> >>>> [..] >>>> >>>>>> +static int css_get_ind_addr(QEMUFile *f, void *pv, size_t size, >>>>>> + VMStateField *field) >>>>>> +{ >>>>>> + int32_t len; >>>>>> + IndAddr **ind_addr = pv; >>>>>> + >>>>>> + len = qemu_get_be32(f); >>>>>> + if (len != 0) { >>>>>> + *ind_addr = get_indicator(qemu_get_be64(f), len); >>>>>> + } else { >>>>>> + qemu_get_be64(f); >>>>>> + *ind_addr = NULL; >>>>>> + } >>>>>> + return 0; >>>>>> +} >>>>>> + >>>>>> +static int css_put_ind_addr(QEMUFile *f, void *pv, size_t size, >>>>>> + VMStateField *field, QJSON *vmdesc) >>>>>> +{ >>>>>> + IndAddr *ind_addr = *(IndAddr **) pv; >>>>>> + >>>>>> + if (ind_addr != NULL) { >>>>>> + qemu_put_be32(f, ind_addr->len); >>>>>> + qemu_put_be64(f, ind_addr->addr); >>>>>> + } else { >>>>>> + qemu_put_be32(f, 0); >>>>>> + qemu_put_be64(f, 0UL); >>>>>> + } >>>>>> + return 0; >>>>>> +} >>>>>> + >>>>>> +const VMStateInfo vmstate_info_ind_addr = { >>>>>> + .name = "s390_ind_addr", >>>>>> + .get = css_get_ind_addr, >>>>>> + .put = css_put_ind_addr >>>>>> +}; >>>>> >>>>> You should be able to avoid this .get/.put by using VMSTATE_WITH_TMP, >>>>> declare a temporary struct something like: >>>>> struct tmp_ind_addr { >>>>> IndAddr *parent; >>>>> uint32_t len; >>>>> uint64_t addr; >>>>> } >>>>> >>>>> and then your .get/.put routines turn into pre_save/post_load >>>>> routines to just setup the len/addr. >>>>> >>>> >>>> I don't think this is going to work -- unfortunately! You can see below, >>>> how this IndAddr* migration stuff is supposed to be used: >>>> the client code just uses the VMSTATE_PTR_TO_IND_ADDR macro as a >>>> field when describing state which needs and IndAddr* migrated. >>>> >>>> The problem is, we do not know in what state will this field >>>> be embedded, the pre_save/post_load called by put_tmp/get_tmp >>>> is however copying the pointer to this state into the parent. >>>> So instead of having a pointer to IndAddr* in those functions >>>> and updating it accordingly, I would have to find the IndAddr* >>>> in some arbitrary state (in our case VirtioCcwDevice) first, >>>> and I lack information for that. >>>> >>>> If it's hard to follow I can give you the patch I was debugging >>>> to come to this conclusion. (By the way I ended up with 10 >>>> lines of code more than in this version, and although I think >>>> it looks nicer, it's simpler only if one knows how WITH_TMP >>>> works. My plan was to ask you which version do you like more >>>> and go with that before I realized it ain't gonna work.) >>>> >>> >>> Yes, I see - I've got some similar other cases; the challenge >>> is it's a custom allocator - 'get_indicator' - and it's used >>> as fields in a few places. Hmm. >>> >>> >> >> The problem can be worked around by wrapping the WITH_TMP into a another >> vmsd and using VMSTATE_STRUCT for describing the field in question. It's >> quite some boilerplate (+16 lines). Should I post the patch here? > > Yes please. > --------------------------------8<------------------------------------------ From a0b6c725b114745c8434f683133995c4e33d936e Mon Sep 17 00:00:00 2001 From: Halil Pasic Date: Tue, 9 May 2017 12:06:42 +0200 Subject: [PATCH 1/1] s390x/css: replace info with VMSTATE_WITH_TMP Convert s VMSatateInfo based solution manipulating the migration stream directly to VMSTATE_WITH_TMP witch keeps IO and transformation logic separate. Signed-off-by: Halil Pasic --- hw/s390x/css.c | 56 ++++++++++++++++++++++++++++++++------------------ include/hw/s390x/css.h | 4 ++-- 2 files changed, 38 insertions(+), 22 deletions(-) diff --git a/hw/s390x/css.c b/hw/s390x/css.c index 694365b395..71d1b95e3f 100644 --- a/hw/s390x/css.c +++ b/hw/s390x/css.c @@ -224,41 +224,57 @@ const VMStateDescription vmstate_subch_dev = { } }; -static int css_get_ind_addr(QEMUFile *f, void *pv, size_t size, - VMStateField *field) -{ +typedef struct IndAddrPtrTmp { + IndAddr **parent; + uint64_t addr; int32_t len; - IndAddr **ind_addr = pv; +} IndAddrPtrTmp; - len = qemu_get_be32(f); - if (len != 0) { - *ind_addr = get_indicator(qemu_get_be64(f), len); +static int post_load_ind_addr(void *opaque, int version_id) +{ + IndAddrPtrTmp *ptmp = opaque; + IndAddr **ind_addr = ptmp->parent; + + if (ptmp->len != 0) { + *ind_addr = get_indicator(ptmp->addr, ptmp->len); } else { - qemu_get_be64(f); *ind_addr = NULL; } return 0; } -static int css_put_ind_addr(QEMUFile *f, void *pv, size_t size, - VMStateField *field, QJSON *vmdesc) +static void pre_save_ind_addr(void *opaque) { - IndAddr *ind_addr = *(IndAddr **) pv; + IndAddrPtrTmp *ptmp = opaque; + IndAddr *ind_addr = *(ptmp->parent); if (ind_addr != NULL) { - qemu_put_be32(f, ind_addr->len); - qemu_put_be64(f, ind_addr->addr); + ptmp->len = ind_addr->len; + ptmp->addr = ind_addr->addr; } else { - qemu_put_be32(f, 0); - qemu_put_be64(f, 0UL); + ptmp->len = 0; + ptmp->addr = 0L; } - return 0; } -const VMStateInfo vmstate_info_ind_addr = { - .name = "s390_ind_addr", - .get = css_get_ind_addr, - .put = css_put_ind_addr +const VMStateDescription vmstate_ind_addr_tmp = { + .name = "s390_ind_addr_tmp", + .pre_save = pre_save_ind_addr, + .post_load = post_load_ind_addr, + + .fields = (VMStateField[]) { + VMSTATE_INT32(len, IndAddrPtrTmp), + VMSTATE_UINT64(addr, IndAddrPtrTmp), + VMSTATE_END_OF_LIST() + } +}; + +const VMStateDescription vmstate_ind_addr = { + .name = "s390_ind_addr_tmp", + .fields = (VMStateField[]) { + VMSTATE_WITH_TMP(IndAddr*, IndAddrPtrTmp, vmstate_ind_addr_tmp), + VMSTATE_END_OF_LIST() + } }; typedef struct CssImage { diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h index be5eb81ece..efe7cafef0 100644 --- a/include/hw/s390x/css.h +++ b/include/hw/s390x/css.h @@ -106,10 +106,10 @@ typedef struct IndAddr { QTAILQ_ENTRY(IndAddr) sibling; } IndAddr; -extern const VMStateInfo vmstate_info_ind_addr; +extern const VMStateDescription vmstate_ind_addr; #define VMSTATE_PTR_TO_IND_ADDR(_f, _s) \ - VMSTATE_SINGLE(_f, _s, 1 , vmstate_info_ind_addr, IndAddr*) + VMSTATE_STRUCT(_f, _s, 1, vmstate_ind_addr, IndAddr*) IndAddr *get_indicator(hwaddr ind_addr, int len); void release_indicator(AdapterInfo *adapter, IndAddr *indicator);