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 18:25:44 +0100 Message-ID: <20161105172543.GC10472@agordeev.lab.eng.brq.redhat.com> References: <20161104143352.gakhitj32hoojsfh@kamzik.brq.redhat.com> <20161104170741.4wpcvde6eq7uqcyk@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]:36094 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751438AbcKERRr (ORCPT ); Sat, 5 Nov 2016 13:17:47 -0400 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (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 8C39015561 for ; Sat, 5 Nov 2016 17:17:46 +0000 (UTC) Content-Disposition: inline In-Reply-To: <20161104170741.4wpcvde6eq7uqcyk@kamzik.brq.redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On Fri, Nov 04, 2016 at 06:07:41PM +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 > > 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. > > Another note on this PCI_COMMAND write. Why is it safe [correct?] to > use writel? This is a 16-bit register according to the comment in > lib/linux/pci_regs.h. Don't we need to introduce a pci_config_writew > and use that? If so, then you should grab Peter Xu's patch that > introduces pci_config_writeb/w. You are right, this is cleanly a bug. However, I can not put Peter's patch neither before nor after mine, since pci_config_write*() are both introduced and used in this patch. As Peter's patch needs a refreshment anyway (see my comment on it) I will add pci_config_writew() to this patch. Hope, that works for everyone. > > > > > + } > > > + > > > + return true; > > > +} > > Thanks, > > drew