From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alexander Gordeev Subject: Re: [kvm-unit-tests PATCH v4 04/12] pci: Rework pci_bar_addr() Date: Tue, 7 Jun 2016 08:38:55 +0200 Message-ID: <20160607063854.GA21204@dhcp-27-118.brq.redhat.com> References: <20160606141203.onxer2plqk4kh3vi@hawk.localdomain> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: kvm@vger.kernel.org, Thomas Huth To: Andrew Jones Return-path: Received: from mx1.redhat.com ([209.132.183.28]:34231 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753691AbcFGGi7 (ORCPT ); Tue, 7 Jun 2016 02:38:59 -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 B28B364D05 for ; Tue, 7 Jun 2016 06:38:58 +0000 (UTC) Content-Disposition: inline In-Reply-To: <20160606141203.onxer2plqk4kh3vi@hawk.localdomain> Sender: kvm-owner@vger.kernel.org List-ID: On Mon, Jun 06, 2016 at 04:12:03PM +0200, Andrew Jones wrote: > > +phys_addr_t pci_bar_size(pcidevaddr_t dev, int bar_num) > > { > > uint32_t bar = pci_config_readl(dev, PCI_BASE_ADDRESS_0 + bar_num * 4); > > + phys_addr_t size = (int32_t)pci_bar_size32(dev, bar_num); > > + phys_addr_t mask = (int32_t)pci_bar_mask(bar); > > > > - if (bar & PCI_BASE_ADDRESS_SPACE_IO) > > - return bar & PCI_BASE_ADDRESS_IO_MASK; > > - else > > - return bar & PCI_BASE_ADDRESS_MEM_MASK; > > + if (pci_bar_is64(dev, bar_num)) { > > + uint32_t size_high = pci_bar_size32(dev, bar_num + 1); > > + size = ((phys_addr_t)size_high << 32) | (uint32_t)size; > > + } > > + > > + return (~(size & mask)) + 1; > > All this casting of size and mask is pointless. Please rework it > similar to what I did above. This is the most compact and straight variant I was able to come up with: uint32_t bar = pci_bar_get(dev, bar_num); phys_addr_t size = (int32_t)pci_bar_size32(dev, bar_num); phys_addr_t mask = (int32_t)pci_bar_mask(bar); if (pci_bar_is64(dev, bar_num)) size |= (phys_addr_t)pci_bar_size32(dev, bar_num + 1) << 32; return (~(size & mask)) + 1; The casting is needed to avoid putting explicitly all 1s into higher bits of size and/or mask. Otherwise (~(size & mask)) + 1 expression would not bring correct results. I really struggle to make something better readable. > Thanks, > drew