From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alexander Gordeev Subject: Re: [kvm-unit-tests PATCH v4 6/5] pci: Add BAR sanity checks Date: Tue, 7 Mar 2017 20:39:06 +0100 Message-ID: <20170307193905.GA27018@agordeev.lab.eng.brq.redhat.com> References: <20170306200622.GA27507@dhcp-27-118.brq.redhat.com> <20170307142253.o6f4etsnd23m2f7o@hawk.localdomain> 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]:45964 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933045AbdCGUZC (ORCPT ); Tue, 7 Mar 2017 15:25:02 -0500 Received: from smtp.corp.redhat.com (int-mx16.intmail.prod.int.phx2.redhat.com [10.5.11.28]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 9274161D27 for ; Tue, 7 Mar 2017 19:29:02 +0000 (UTC) Content-Disposition: inline In-Reply-To: <20170307142253.o6f4etsnd23m2f7o@hawk.localdomain> Sender: kvm-owner@vger.kernel.org List-ID: On Tue, Mar 07, 2017 at 03:22:53PM +0100, Andrew Jones wrote: > Hi Alex, > > This should be its own patch, the series you've added it to was > already committed. > > On Mon, Mar 06, 2017 at 09:06:23PM +0100, Alexander Gordeev wrote: > > Cc: Thomas Huth > > Cc: Andrew Jones > > Cc: Peter Xu > > Signed-off-by: Alexander Gordeev > > --- > > lib/pci.c | 54 ++++++++++++++++++++++++++++++++++++++++-------------- > > lib/pci.h | 21 +++++++++++++-------- > > 2 files changed, 53 insertions(+), 22 deletions(-) > > > > diff --git a/lib/pci.c b/lib/pci.c > > index daf398100b7e..98fc3849d297 100644 > > --- a/lib/pci.c > > +++ b/lib/pci.c > > @@ -110,13 +110,14 @@ uint32_t pci_bar_mask(uint32_t bar) > > PCI_BASE_ADDRESS_IO_MASK : PCI_BASE_ADDRESS_MEM_MASK; > > } > > > > -uint32_t pci_bar_get(struct pci_dev *dev, int bar_num) > > +uint32_t pci_bar_get(struct pci_dev *dev, unsigned int bar_num) > > { > > - return pci_config_readl(dev->bdf, PCI_BASE_ADDRESS_0 + > > - bar_num * 4); > > + CHECK_BAR_NUM(bar_num); > > + > > + return pci_config_readl(dev->bdf, PCI_BASE_ADDRESS_0 + bar_num * 4); > > } > > > > -static phys_addr_t __pci_bar_get_addr(struct pci_dev *dev, int bar_num) > > +static phys_addr_t __pci_bar_get_addr(struct pci_dev *dev, unsigned int bar_num) > > { > > uint32_t bar = pci_bar_get(dev, bar_num); > > uint32_t mask = pci_bar_mask(bar); > > @@ -132,15 +133,26 @@ static phys_addr_t __pci_bar_get_addr(struct pci_dev *dev, int bar_num) > > return phys_addr; > > } > > > > -phys_addr_t pci_bar_get_addr(struct pci_dev *dev, int bar_num) > > +phys_addr_t pci_bar_get_addr(struct pci_dev *dev, unsigned int bar_num) > > { > > + CHECK_BAR_NUM(bar_num); > > + > > return dev->resource[bar_num]; > > } > > > > -void pci_bar_set_addr(struct pci_dev *dev, int bar_num, phys_addr_t addr) > > +void pci_bar_set_addr(struct pci_dev *dev, > > + unsigned int bar_num, phys_addr_t addr) > > { > > int off = PCI_BASE_ADDRESS_0 + bar_num * 4; > > > > + CHECK_BAR_NUM(bar_num); > > + assert(addr != INVALID_PHYS_ADDR); > > Shouldn't this be something like > > assert((addr & PHYS_MASK) == addr) Nope. At this stage PCI bus is scanned and pci_dev::resource[] is initialized with proper values. A value of INVALID_PHYS_ADDR in dev->resource[bar_num] indicates BAR #bar_num is not implemented. Thus, we do not want to screw up this assumption with a test case trying to write INVALID_PHYS_ADDR to a the BAR. > But that means we need to ensure all architectures define PHYS_MASK... > > > + assert(dev->resource[bar_num] != INVALID_PHYS_ADDR); > > + if (pci_bar_is64(dev, bar_num)) { > > + assert(bar_num + 1 < PCI_BAR_NUM); > > Use CHECK_BAR_NUM(bar_num + 1) > > > + assert(dev->resource[bar_num] == dev->resource[bar_num + 1]); > > Can this ever happen without the unit test explicitly messing things up? > > > + } > > + > > pci_config_writel(dev->bdf, off, (uint32_t)addr); > > dev->resource[bar_num] = addr; > > > > @@ -161,7 +173,7 @@ void pci_bar_set_addr(struct pci_dev *dev, int bar_num, phys_addr_t addr) > > * The following pci_bar_size_helper() and pci_bar_size() functions > > * implement the algorithm. > > */ > > -static uint32_t pci_bar_size_helper(struct pci_dev *dev, int bar_num) > > +static uint32_t pci_bar_size_helper(struct pci_dev *dev, unsigned int bar_num) > > { > > int off = PCI_BASE_ADDRESS_0 + bar_num * 4; > > uint16_t bdf = dev->bdf; > > @@ -175,10 +187,12 @@ static uint32_t pci_bar_size_helper(struct pci_dev *dev, int bar_num) > > return val; > > } > > > > -phys_addr_t pci_bar_size(struct pci_dev *dev, int bar_num) > > +phys_addr_t pci_bar_size(struct pci_dev *dev, unsigned int bar_num) > > { > > uint32_t bar, size; > > > > + CHECK_BAR_NUM(bar_num); > > + > > size = pci_bar_size_helper(dev, bar_num); > > if (!size) > > return 0; > > @@ -196,21 +210,31 @@ phys_addr_t pci_bar_size(struct pci_dev *dev, int bar_num) > > } > > } > > > > -bool pci_bar_is_memory(struct pci_dev *dev, int bar_num) > > +bool pci_bar_is_memory(struct pci_dev *dev, unsigned int bar_num) > > { > > - uint32_t bar = pci_bar_get(dev, bar_num); > > + uint32_t bar; > > + > > + CHECK_BAR_NUM(bar_num); > > + > > + bar = pci_bar_get(dev, bar_num); > > This hunk is unnecessary, pci_bar_get already calls CHECK_BAR_NUM Right, it is superfluous. I put it here intentionally to avoid the check to go away in case pci_bar_get() is changed somehow. Pure paranoia. I can remove it if you want here and elsewhere. > > return !(bar & PCI_BASE_ADDRESS_SPACE_IO); > > } > > > > -bool pci_bar_is_valid(struct pci_dev *dev, int bar_num) > > +bool pci_bar_is_valid(struct pci_dev *dev, unsigned int bar_num) > > { > > + CHECK_BAR_NUM(bar_num); > > + > > return dev->resource[bar_num] != INVALID_PHYS_ADDR; > > } > > > > -bool pci_bar_is64(struct pci_dev *dev, int bar_num) > > +bool pci_bar_is64(struct pci_dev *dev, unsigned int bar_num) > > { > > - uint32_t bar = pci_bar_get(dev, bar_num); > > + uint32_t bar; > > + > > + CHECK_BAR_NUM(bar_num); > > + > > + bar = pci_bar_get(dev, bar_num); > > Same as above (unnecessary hunk) > > > > > if (bar & PCI_BASE_ADDRESS_SPACE_IO) > > return false; > > @@ -219,11 +243,13 @@ bool pci_bar_is64(struct pci_dev *dev, int bar_num) > > PCI_BASE_ADDRESS_MEM_TYPE_64; > > } > > > > -void pci_bar_print(struct pci_dev *dev, int bar_num) > > +void pci_bar_print(struct pci_dev *dev, unsigned int bar_num) > > { > > phys_addr_t size, start, end; > > uint32_t bar; > > > > + CHECK_BAR_NUM(bar_num); > > + > > Unnecessary, pci_bar_is_valid already checks > > > if (!pci_bar_is_valid(dev, bar_num)) > > return; > > > > diff --git a/lib/pci.h b/lib/pci.h > > index 03cc0a72d48d..c770def840da 100644 > > --- a/lib/pci.h > > +++ b/lib/pci.h > > @@ -18,6 +18,9 @@ enum { > > #define PCI_BAR_NUM 6 > > #define PCI_DEVFN_MAX 256 > > > > +#define CHECK_BAR_NUM(bar_num) \ > > + do { assert(bar_num < PCI_BAR_NUM); } while (0) > > Just add 'bar_num >= 0 &&' to this macro and we don't need to > change bar_num to unsigned everywhere > > > + > > #define PCI_BDF_GET_DEVFN(x) ((x) & 0xff) > > #define PCI_BDF_GET_BUS(x) (((x) >> 8) & 0xff) > > > > @@ -54,15 +57,17 @@ extern pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id); > > * It is expected the caller is aware of the device BAR layout and never > > * tries to address the middle of a 64-bit register. > > */ > > -extern phys_addr_t pci_bar_get_addr(struct pci_dev *dev, int bar_num); > > -extern void pci_bar_set_addr(struct pci_dev *dev, int bar_num, phys_addr_t addr); > > -extern phys_addr_t pci_bar_size(struct pci_dev *dev, int bar_num); > > -extern uint32_t pci_bar_get(struct pci_dev *dev, int bar_num); > > + > > +extern phys_addr_t pci_bar_get_addr(struct pci_dev *dev, unsigned int bar_num); > > +extern void pci_bar_set_addr(struct pci_dev *dev, unsigned int bar_num, > > + phys_addr_t addr); > > +extern phys_addr_t pci_bar_size(struct pci_dev *dev, unsigned int bar_num); > > +extern uint32_t pci_bar_get(struct pci_dev *dev, unsigned int bar_num); > > extern uint32_t pci_bar_mask(uint32_t bar); > > -extern bool pci_bar_is64(struct pci_dev *dev, int bar_num); > > -extern bool pci_bar_is_memory(struct pci_dev *dev, int bar_num); > > -extern bool pci_bar_is_valid(struct pci_dev *dev, int bar_num); > > -extern void pci_bar_print(struct pci_dev *dev, int bar_num); > > +extern bool pci_bar_is64(struct pci_dev *dev, unsigned int bar_num); > > +extern bool pci_bar_is_memory(struct pci_dev *dev, unsigned int bar_num); > > +extern bool pci_bar_is_valid(struct pci_dev *dev, unsigned int bar_num); > > +extern void pci_bar_print(struct pci_dev *dev, unsigned int bar_num); > > extern void pci_dev_print_id(struct pci_dev *dev); > > extern void pci_dev_print(struct pci_dev *dev); > > extern uint8_t pci_intx_line(struct pci_dev *dev); > > -- > > 1.8.3.1 > > > > Thanks, > drew