From patchwork Tue Jun 20 08:23:28 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Bharata B Rao X-Patchwork-Id: 9798725 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 2F5D0601BC for ; Tue, 20 Jun 2017 08:24:35 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 1E7C228464 for ; Tue, 20 Jun 2017 08:24:35 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 129ED28435; Tue, 20 Jun 2017 08:24:35 +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 4FE2F28435 for ; Tue, 20 Jun 2017 08:24:33 +0000 (UTC) Received: from localhost ([::1]:47259 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dNESe-0005Mg-RS for patchwork-qemu-devel@patchwork.kernel.org; Tue, 20 Jun 2017 04:24:32 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42531) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dNERu-0005HZ-2R for qemu-devel@nongnu.org; Tue, 20 Jun 2017 04:23:47 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1dNERp-0005vs-JT for qemu-devel@nongnu.org; Tue, 20 Jun 2017 04:23:46 -0400 Received: from mx0b-001b2d01.pphosted.com ([148.163.158.5]:43388 helo=mx0a-001b2d01.pphosted.com) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1dNERp-0005vX-Co for qemu-devel@nongnu.org; Tue, 20 Jun 2017 04:23:41 -0400 Received: from pps.filterd (m0098420.ppops.net [127.0.0.1]) by mx0b-001b2d01.pphosted.com (8.16.0.20/8.16.0.20) with SMTP id v5K8IgI8074327 for ; Tue, 20 Jun 2017 04:23:40 -0400 Received: from e23smtp03.au.ibm.com (e23smtp03.au.ibm.com [202.81.31.145]) by mx0b-001b2d01.pphosted.com with ESMTP id 2b6qenbanb-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for ; Tue, 20 Jun 2017 04:23:38 -0400 Received: from localhost by e23smtp03.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 20 Jun 2017 18:23:35 +1000 Received: from d23relay09.au.ibm.com (202.81.31.228) by e23smtp03.au.ibm.com (202.81.31.209) with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted; Tue, 20 Jun 2017 18:23:34 +1000 Received: from d23av03.au.ibm.com (d23av03.au.ibm.com [9.190.234.97]) by d23relay09.au.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id v5K8NXfv2032102; Tue, 20 Jun 2017 18:23:33 +1000 Received: from d23av03.au.ibm.com (localhost [127.0.0.1]) by d23av03.au.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id v5K8NOaJ015386; Tue, 20 Jun 2017 18:23:25 +1000 Received: from in.ibm.com ([9.124.35.35]) by d23av03.au.ibm.com (8.14.4/8.14.4/NCO v10.0 AVin) with ESMTP id v5K8NKnw015270 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO); Tue, 20 Jun 2017 18:23:23 +1000 Date: Tue, 20 Jun 2017 13:53:28 +0530 From: Bharata B Rao To: David Gibson References: <20170608050930.2613-1-david@gibson.dropbear.id.au> <20170608050930.2613-5-david@gibson.dropbear.id.au> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20170608050930.2613-5-david@gibson.dropbear.id.au> User-Agent: Mutt/1.7.1 (2016-10-04) X-TM-AS-MML: disable x-cbid: 17062008-0008-0000-0000-00000143D840 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 17062008-0009-0000-0000-000009736BFE Message-Id: <20170620082328.GA15438@in.ibm.com> X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:, , definitions=2017-06-20_04:, , 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-1706200151 X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x [generic] [fuzzy] X-Received-From: 148.163.158.5 Subject: Re: [Qemu-devel] [PATCH 4/6] spapr: Make DRC reset force DRC into known state 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: , Reply-To: bharata@linux.vnet.ibm.com Cc: qemu-devel@nongnu.org, sursingh@redhat.com, qemu-ppc@nongnu.org, mdroth@linux.vnet.ibm.com, groug@kaod.org Errors-To: qemu-devel-bounces+patchwork-qemu-devel=patchwork.kernel.org@nongnu.org Sender: "Qemu-devel" X-Virus-Scanned: ClamAV using ClamSMTP On Thu, Jun 08, 2017 at 03:09:28PM +1000, David Gibson wrote: > The reset handler for DRCs attempts several state transitions which are > subject to various checks and restrictions. But at reset time we know > there is no guest, so we can ignore most of the usual sequencing rules and > just set the DRC back to a known state. In fact, it's safer to do so. > > The existing code also has several redundant checks for > drc->awaiting_release inside a block which has already tested that. This > patch removes those and sets the DRC to a fixed initial state based only > on whether a device is currently plugged or not. > > With DRCs correctly reset to a state based on device presence, we don't > need to force state transitions as cold plugged devices are processed. > This allows us to remove all the callers of the set_*_state() methods from > outside spapr_drc.c. > > Signed-off-by: David Gibson > --- > hw/ppc/spapr.c | 15 --------------- > hw/ppc/spapr_drc.c | 28 ++++++++-------------------- > 2 files changed, 8 insertions(+), 35 deletions(-) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index 01dda9e..c988e38 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -2545,12 +2545,6 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size, > > spapr_drc_attach(drc, dev, fdt, fdt_offset, !dev->hotplugged, errp); > addr += SPAPR_MEMORY_BLOCK_SIZE; > - if (!dev->hotplugged) { > - sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc); > - /* guests expect coldplugged LMBs to be pre-allocated */ > - drck->set_allocation_state(drc, SPAPR_DR_ALLOCATION_STATE_USABLE); > - drck->set_isolation_state(drc, SPAPR_DR_ISOLATION_STATE_UNISOLATED); > - } > } > /* send hotplug notification to the > * guest only in case of hotplugged memory > @@ -2901,15 +2895,6 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev, > * of hotplugged CPUs. > */ > spapr_hotplug_req_add_by_index(drc); > - } else { > - /* > - * Set the right DRC states for cold plugged CPU. > - */ > - if (drc) { > - sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc); > - drck->set_allocation_state(drc, SPAPR_DR_ALLOCATION_STATE_USABLE); > - drck->set_isolation_state(drc, SPAPR_DR_ISOLATION_STATE_UNISOLATED); So here you are removing the initial state setting for cold plugged CPUs and ... > - } > } > core_slot->cpu = OBJECT(dev); > } > diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c > index dc4ac77..7e17f34 100644 > --- a/hw/ppc/spapr_drc.c > +++ b/hw/ppc/spapr_drc.c > @@ -393,7 +393,6 @@ static bool release_pending(sPAPRDRConnector *drc) > static void reset(DeviceState *d) > { > sPAPRDRConnector *drc = SPAPR_DR_CONNECTOR(d); > - sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc); > > trace_spapr_drc_reset(spapr_drc_index(drc)); > > @@ -401,28 +400,17 @@ static void reset(DeviceState *d) > drc->ccs = NULL; > > /* immediately upon reset we can safely assume DRCs whose devices > - * are pending removal can be safely removed, and that they will > - * subsequently be left in an ISOLATED state. move the DRC to this > - * state in these cases (which will in turn complete any pending > - * device removals) > + * are pending removal can be safely removed. > */ > if (drc->awaiting_release) { > - drck->set_isolation_state(drc, SPAPR_DR_ISOLATION_STATE_ISOLATED); > - /* generally this should also finalize the removal, but if the device > - * hasn't yet been configured we normally defer removal under the > - * assumption that this transition is taking place as part of device > - * configuration. so check if we're still waiting after this, and > - * force removal if we are > - */ > - if (drc->awaiting_release) { > - spapr_drc_detach(drc, DEVICE(drc->dev), NULL); > - } > + spapr_drc_release(drc); > + } > > - /* non-PCI devices may be awaiting a transition to UNUSABLE */ > - if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI && > - drc->awaiting_release) { > - drck->set_allocation_state(drc, SPAPR_DR_ALLOCATION_STATE_UNUSABLE); > - } > + drc->isolation_state = drc->dev ? SPAPR_DR_ISOLATION_STATE_UNISOLATED > + : SPAPR_DR_ISOLATION_STATE_ISOLATED; > + if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI) { > + drc->allocation_state = drc->dev ? SPAPR_DR_ALLOCATION_STATE_USABLE > + : SPAPR_DR_ALLOCATION_STATE_UNUSABLE; > } ... adding it via reset. However you are setting drc->isolation_state and drc->allocation_state directly rather than going via drck->set_isolation_state() and drck->set_allocation_state() routines. This results in awaiting_allocation not geting cleared for cold plugged CPUs. So the effect after this commit is that we can't remove the CPUs that are specified on cmdline using -device. The following fixes the issue for me: Regards, Bharata. diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c index fd9e07d..da6979a 100644 --- a/hw/ppc/spapr_drc.c +++ b/hw/ppc/spapr_drc.c @@ -450,13 +450,13 @@ static void reset(DeviceState *d) /* A device present at reset is coldplugged */ drc->isolation_state = SPAPR_DR_ISOLATION_STATE_UNISOLATED; if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI) { - drc->allocation_state = SPAPR_DR_ALLOCATION_STATE_USABLE; + drc_set_usable(drc); } } else { /* Otherwise device is absent, but might be hotplugged */ drc->isolation_state = SPAPR_DR_ISOLATION_STATE_ISOLATED; if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI) { - drc->allocation_state = SPAPR_DR_ALLOCATION_STATE_UNUSABLE; + drc_set_unusable(drc); } } } However I thought this will restore the previous behaviour completely: diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c index fd9e07d..b7726ef 100644 --- a/hw/ppc/spapr_drc.c +++ b/hw/ppc/spapr_drc.c @@ -433,6 +433,7 @@ static bool release_pending(sPAPRDRConnector *drc) static void reset(DeviceState *d) { sPAPRDRConnector *drc = SPAPR_DR_CONNECTOR(d); + sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc); trace_spapr_drc_reset(spapr_drc_index(drc)); @@ -448,15 +449,15 @@ static void reset(DeviceState *d) if (drc->dev) { /* A device present at reset is coldplugged */ - drc->isolation_state = SPAPR_DR_ISOLATION_STATE_UNISOLATED; + drck->unisolate(drc); if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI) { - drc->allocation_state = SPAPR_DR_ALLOCATION_STATE_USABLE; + drc_set_usable(drc); } } else { /* Otherwise device is absent, but might be hotplugged */ - drc->isolation_state = SPAPR_DR_ISOLATION_STATE_ISOLATED; + drck->isolate(drc); if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI) { - drc->allocation_state = SPAPR_DR_ALLOCATION_STATE_UNUSABLE; + drc_set_unusable(drc); } } }