From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alexander Gordeev Subject: [kvm-unit-tests PATCH v4 2/5] pci: Accomodate 64 bit BARs in pci_dev::resource[] Date: Tue, 28 Feb 2017 19:08:27 +0100 Message-ID: <9184b3fc479b6d6ee41219c7419b4830819e5f29.1488304691.git.agordeev@redhat.com> References: Cc: Alexander Gordeev , Thomas Huth , Andrew Jones , Peter Xu To: kvm@vger.kernel.org Return-path: Received: from mx1.redhat.com ([209.132.183.28]:59819 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751344AbdB1SI4 (ORCPT ); Tue, 28 Feb 2017 13:08:56 -0500 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 AF40081F01 for ; Tue, 28 Feb 2017 18:08:37 +0000 (UTC) In-Reply-To: In-Reply-To: References: Sender: kvm-owner@vger.kernel.org List-ID: Array pci_dev::resource[] is ambiguous wrt what is actually stored in its elements and how to interpret the index into the array. It is simple in case a device has only 32-bit BARs - an element pci_dev::resource[bar_num] contains the decoded address of BAR # bar_num. But what if a device has 64-bit BAR starting at bar_num? Curretnly pci_dev::resource[bar_num] contains the decoded address of the BAR, while pci_dev::resource[bar_num + 1] contains 0. That makes meaning of (bar_num + 1) index difficult to understand. On the one hand a testcase should not address a 64-bit BAR using high part BAR number. Particularly, when it tries to obtan BAR size or address. On the other hand a testcase should be able to access a low and high parts separately if it needs to for whatever reason. The rest of the API allows that as well. pci_dev::resource[] contains decoded 32/64-bit BAR addresses, not low/high parts of underlying 32-bit device BARs. Yet, indexes into this array correspond to raw 32-bit BARs in the PCI device configuration space. Thus, a question arises - what should be stored in array elements that correspond to high-parts of 64-bit BARs? Zero is particularly bad choice, because: - it is a valid address in PIO address space, so it can not stand for "no value" or NULL or whatever marker could be used to indicate a high part; - the high part of underlying 64-bit address is (could be) non-zero. So there is inconsistency also; By placing the same 64-bit address in both bar_num and (bar_num + 1) elements the ambiguity is less striking, since: - the meaning of bar_num kept consistent with the rest of the functions (where it refers 32-bit BAR in terms of the device configuration address space); - pci_dev::resource[bar_num + 1] contains a valid address rather than vague value of 0. - both bar_num and (bar_num + 1) indexes refer to the same 64-bit BAR and therefore return the same address; The notion of low and high parts of a 64-bit address is ignored, but that is fine, since pci_dev::resource[] contain only full addresses; Cc: Thomas Huth Cc: Andrew Jones Cc: Peter Xu Reviewed-by: Andrew Jones Signed-off-by: Alexander Gordeev --- lib/pci.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/lib/pci.c b/lib/pci.c index 62b1c6b0b7c5..9acc9652cb25 100644 --- a/lib/pci.c +++ b/lib/pci.c @@ -331,12 +331,15 @@ void pci_scan_bars(struct pci_dev *dev) int i; for (i = 0; i < PCI_BAR_NUM; i++) { - if (!pci_bar_is_valid(dev, i)) - continue; - dev->resource[i] = pci_bar_get_addr(dev, i); - if (pci_bar_is64(dev, i)) { - i++; - dev->resource[i] = (phys_addr_t)0; + if (pci_bar_size(dev, i)) { + dev->resource[i] = pci_bar_get_addr(dev, i); + if (pci_bar_is64(dev, i)) { + assert(i + 1 < PCI_BAR_NUM); + dev->resource[i + 1] = dev->resource[i]; + i++; + } + } else { + dev->resource[i] = INVALID_PHYS_ADDR; } } } -- 1.8.3.1