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: Wed, 8 Mar 2017 12:40:59 +0100 Message-ID: <20170308114059.GB27018@agordeev.lab.eng.brq.redhat.com> References: <20170306200622.GA27507@dhcp-27-118.brq.redhat.com> <20170307142253.o6f4etsnd23m2f7o@hawk.localdomain> <20170307193905.GA27018@agordeev.lab.eng.brq.redhat.com> <20170308101026.6oc2hgnhv3cod6gu@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]:49546 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751929AbdCHMH3 (ORCPT ); Wed, 8 Mar 2017 07:07:29 -0500 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) (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 EC453C04BD3A for ; Wed, 8 Mar 2017 11:30:55 +0000 (UTC) Content-Disposition: inline In-Reply-To: <20170308101026.6oc2hgnhv3cod6gu@hawk.localdomain> Sender: kvm-owner@vger.kernel.org List-ID: On Wed, Mar 08, 2017 at 11:10:26AM +0100, Andrew Jones wrote: > > > > -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. > > Of course, by why would any other invalid phys addr be OK? Hmm, maybe > it should be allowed for test case purposes. Yeah, I think it should be allowed. > > > 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? No, unless there is really something wrong with the code. > No answer for this. I don't see the point of an assert that can never > fail. So this is code integrity assert, not test case input check. Do you want me to remove it? > > > Thanks, > > > drew