From nobody Thu May 2 02:39:26 2024 Delivered-To: importer@patchew.org Authentication-Results: mx.zohomail.com; spf=pass (zohomail.com: domain of gnu.org designates 209.51.188.17 as permitted sender) smtp.mailfrom=qemu-devel-bounces+importer=patchew.org@nongnu.org ARC-Seal: i=1; a=rsa-sha256; t=1594314842; cv=none; d=zohomail.com; s=zohoarc; b=NvFzTd65OK2OoKpuW/sg+S2Y6Yl9k6Ki6mPhSSbacuvLcXDkse6R29QhwAFfcsvMKq0r7IrFTtOLCRK8cIzVJOjCN2yPOzuqwIGtZtZ7J8WJSNvbGLL3Ig92E35m7rT+FLMSuXy5gF4QhNvbQEPHHiPTxSjgiZl4A8eKn1YfbAY= ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=zohomail.com; s=zohoarc; t=1594314842; h=Content-Type:Content-Transfer-Encoding:Cc:Date:From:List-Subscribe:List-Post:List-Id:List-Archive:List-Help:List-Unsubscribe:MIME-Version:Message-ID:Sender:Subject:To; bh=4F6TzEwUmnNJyke2ZaZJKm9+5l/lArztHRENmh0okYE=; b=OhxCXRiqoSjJoNmOJFmOswsxBwgNLGMuMRa44EgquXdPh4Hi/yS5uMirqg5JVILEGjDZ2R5St/eZTiw2GLdWd3Kg2MLVhFG81NHUVcOuFzqZAf/bInzD7Y70RZyO2Z10J0zAJn4iBupxQExUIgv3tdY9PCQUVw4+7hh9Z20b2N4= ARC-Authentication-Results: i=1; mx.zohomail.com; spf=pass (zohomail.com: domain of gnu.org designates 209.51.188.17 as permitted sender) smtp.mailfrom=qemu-devel-bounces+importer=patchew.org@nongnu.org Return-Path: Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) by mx.zohomail.com with SMTPS id 1594314841910831.4635473903109; Thu, 9 Jul 2020 10:14:01 -0700 (PDT) Received: from localhost ([::1]:42078 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1jta7U-00071b-NI for importer@patchew.org; Thu, 09 Jul 2020 13:14:00 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:49046) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1jta6c-000601-QL for qemu-devel@nongnu.org; Thu, 09 Jul 2020 13:13:06 -0400 Received: from us-smtp-delivery-1.mimecast.com ([207.211.31.120]:52403 helo=us-smtp-1.mimecast.com) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.90_1) (envelope-from ) id 1jta6a-0003Nr-Pm for qemu-devel@nongnu.org; Thu, 09 Jul 2020 13:13:06 -0400 Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-27-F5zskHOVOiC5Ca40pSp7Fw-1; Thu, 09 Jul 2020 13:12:59 -0400 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 074CF107ACCA; Thu, 9 Jul 2020 17:12:58 +0000 (UTC) Received: from bahia.lan (ovpn-112-37.ams2.redhat.com [10.36.112.37]) by smtp.corp.redhat.com (Postfix) with ESMTP id 5B0F119D61; Thu, 9 Jul 2020 17:12:48 +0000 (UTC) X-MC-Unique: F5zskHOVOiC5Ca40pSp7Fw-1 Subject: [PATCH] spapr_pci: Robustify support of PCI bridges From: Greg Kurz To: David Gibson Date: Thu, 09 Jul 2020 19:12:47 +0200 Message-ID: <159431476748.407044.16711294833569014964.stgit@bahia.lan> User-Agent: StGit/0.21 MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: kaod.org Content-Transfer-Encoding: quoted-printable Received-SPF: pass (zohomail.com: domain of gnu.org designates 209.51.188.17 as permitted sender) client-ip=209.51.188.17; envelope-from=qemu-devel-bounces+importer=patchew.org@nongnu.org; helo=lists.gnu.org; Received-SPF: softfail client-ip=207.211.31.120; envelope-from=groug@kaod.org; helo=us-smtp-1.mimecast.com X-detected-operating-system: by eggs.gnu.org: First seen = 2020/07/09 11:02:45 X-ACL-Warn: Detected OS = Linux 2.2.x-3.x [generic] [fuzzy] X-Spam_score_int: -21 X-Spam_score: -2.2 X-Spam_bar: -- X-Spam_report: (-2.2 / 5.0 requ) BAYES_00=-1.9, RCVD_IN_DNSWL_NONE=-0.0001, RCVD_IN_MSPIKE_H2=-1, SPF_HELO_NONE=0.001, SPF_SOFTFAIL=0.665 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Markus Armbruster , Thomas Huth , qemu-ppc@nongnu.org, qemu-devel@nongnu.org, "Michael S. Tsirkin" Errors-To: qemu-devel-bounces+importer=patchew.org@nongnu.org Sender: "Qemu-devel" Content-Type: text/plain; charset="utf-8" Some recent error handling cleanups unveiled issues with our support of PCI bridges: 1) QEMU aborts when using non-standard PCI bridge types, unveiled by commit 7ef1553dac "spapr_pci: Drop some dead error handling" $ qemu-system-ppc64 -M pseries -device pcie-pci-bridge Unexpected error in object_property_find() at qom/object.c:1240: qemu-system-ppc64: -device pcie-pci-bridge: Property '.chassis_nr' not found Aborted (core dumped) This happens because we assume all PCI bridge types to have a "chassis_nr" property. This property only exists with the standard PCI bridge type "pci-bridge" actually. We could possibly revert 7ef1553dac but it seems much simpler to check the presence of "chassis_nr" earlier. 2) QEMU abort if same "chassis_nr" value is used several times, unveiled by commit d2623129a7de "qom: Drop parameter @errp of object_property_add() & friends" $ qemu-system-ppc64 -M pseries -device pci-bridge,chassis_nr=3D1 \ -device pci-bridge,chassis_nr=3D1 Unexpected error in object_property_try_add() at qom/object.c:1167: qemu-system-ppc64: -device pci-bridge,chassis_nr=3D1: attempt to add duplic= ate property '40000100' to object (type 'container') Aborted (core dumped) This happens because we assume that "chassis_nr" values are unique, but nobody enforces that and we end up generating duplicate DRC ids. The PCI code doesn't really care for duplicate "chassis_nr" properties since it is only used to initialize the "Chassis Number Register" of the bridge, with no functional impact on QEMU. So, even if passing the same value several times might look weird, it never broke anything before, so I guess we don't necessarily want to enforce strict checking in the PCI code now. Workaround both issues in the PAPR code: check that the bridge has a unique and non null "chassis_nr" when plugging it into its parent bus. Fixes: 05929a6c5dfe ("spapr: Don't use bus number for building DRC ids") Reported-by: Thomas Huth Signed-off-by: Greg Kurz Acked-by: Michael S. Tsirkin --- hw/ppc/spapr_pci.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++++= ++++ 1 file changed, 57 insertions(+) diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c index 329002ac040e..09d52ef7954d 100644 --- a/hw/ppc/spapr_pci.c +++ b/hw/ppc/spapr_pci.c @@ -1480,6 +1480,57 @@ static void spapr_pci_bridge_plug(SpaprPhbState *phb, add_drcs(phb, bus); } =20 +/* Returns non-zero if the value of "chassis_nr" is already in use */ +static int check_chassis_nr(Object *obj, void *opaque) +{ + int new_chassis_nr =3D + object_property_get_uint(opaque, "chassis_nr", &error_abort); + int chassis_nr =3D + object_property_get_uint(obj, "chassis_nr", NULL); + + if (!object_dynamic_cast(obj, TYPE_PCI_BRIDGE)) { + return 0; + } + + /* Skip unsupported bridge types */ + if (!chassis_nr) { + return 0; + } + + /* Skip self */ + if (obj =3D=3D opaque) { + return 0; + } + + return chassis_nr =3D=3D new_chassis_nr; +} + +static bool bridge_has_valid_chassis_nr(Object *bridge, Error **errp) +{ + int chassis_nr =3D + object_property_get_uint(bridge, "chassis_nr", NULL); + + /* + * slotid_cap_init() already ensures that "chassis_nr" isn't null for + * standard PCI bridges, so this really tells if "chassis_nr" is prese= nt + * or not. + */ + if (!chassis_nr) { + error_setg(errp, "PCI Bridge lacks a \"chassis_nr\" property"); + error_append_hint(errp, "Try -device pci-bridge instead.\n"); + return false; + } + + /* We want unique values for "chassis_nr" */ + if (object_child_foreach_recursive(object_get_root(), check_chassis_nr, + bridge)) { + error_setg(errp, "Bridge chassis %d already in use", chassis_nr); + return false; + } + + return true; +} + static void spapr_pci_plug(HotplugHandler *plug_handler, DeviceState *plugged_dev, Error **errp) { @@ -1491,6 +1542,12 @@ static void spapr_pci_plug(HotplugHandler *plug_hand= ler, PCIBus *bus =3D PCI_BUS(qdev_get_parent_bus(DEVICE(pdev))); uint32_t slotnr =3D PCI_SLOT(pdev->devfn); =20 + if (pc->is_bridge) { + if (!bridge_has_valid_chassis_nr(OBJECT(plugged_dev), errp)) { + return; + } + } + /* if DR is disabled we don't need to do anything in the case of * hotplug or coldplug callbacks */