From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alexander Gordeev Subject: Re: [kvm-unit-tests PATCH v9 10/12] pci: Add generic ECAM host support Date: Sat, 5 Nov 2016 10:56:26 +0100 Message-ID: <20161105095625.GA10472@agordeev.lab.eng.brq.redhat.com> References: <20161104143352.gakhitj32hoojsfh@kamzik.brq.redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: kvm@vger.kernel.org, Thomas Huth , Peter Xu To: Andrew Jones Return-path: Received: from mx1.redhat.com ([209.132.183.28]:33292 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753329AbcKEJs2 (ORCPT ); Sat, 5 Nov 2016 05:48:28 -0400 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 2DC53155C3 for ; Sat, 5 Nov 2016 09:48:28 +0000 (UTC) Content-Disposition: inline In-Reply-To: <20161104143352.gakhitj32hoojsfh@kamzik.brq.redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On Fri, Nov 04, 2016 at 03:33:52PM +0100, Andrew Jones wrote: > > +bool pci_probe(void) > > +{ > > + pcidevaddr_t dev; > > + u8 header; > > + u32 cmd; > > + int i; > > + > > + assert(!pci_host_bridge); > > + pci_host_bridge = pci_dt_probe(); > > + if (!pci_host_bridge) > > + return false; > > + > > + for (dev = 0; dev < 256; dev++) { > > + if (!pci_dev_exists(dev)) > > + continue; > > + > > + /* We are only interested in normal PCI devices */ > > + header = pci_config_readb(dev, PCI_HEADER_TYPE); > > + if ((header & PCI_HEADER_TYPE_MASK) != PCI_HEADER_TYPE_NORMAL) > > + continue; > > + > > + cmd = PCI_COMMAND_SERR; > > + > > + for (i = 0; i < 6; i++) { > > + u64 addr; > > + > > + if (pci_alloc_resource(dev, i, &addr)) { > > + pci_bar_set_addr(dev, i, addr); > > + > > + if (pci_bar_is_memory(dev, i)) > > + cmd |= PCI_COMMAND_MEMORY; > > + else > > + cmd |= PCI_COMMAND_IO; > > + } > > + > > + if (pci_bar_is64(dev, i)) > > + i++; > > + } > > + > > + pci_config_writel(dev, PCI_COMMAND, cmd); > > As I asked in the last review, is this PCI_COMMAND_SERR safe/desired > to write, even when no resources are allocated? If not, then above Yes, it allows a device to report grave PCI bus errors. Moreover, I think I should have enabled parity errors check as well. I assume we want to notice such errors, including the case no resources were assigned. That would be more troubling even. In Linux it is up to device drivers to enable these, but I would just let all devices scream loudly by default. > we need to capture the return value of pci_alloc_resource, add > an 'else break', and then check the return value here before doing > the write. > > > + } > > + > > + return true; > > +} > Thanks, > drew