From owner-freebsd-current@FreeBSD.ORG Wed May 6 23:12:17 2015 Return-Path: Delivered-To: freebsd-current@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id B97D22D5; Wed, 6 May 2015 23:12:17 +0000 (UTC) Received: from aussmtpmrkps320.us.dell.com (aussmtpmrkps320.us.dell.com [143.166.224.254]) (using TLSv1 with cipher RC4-SHA (128/128 bits)) (Client CN "ausxipmktps31.us.dell.com", Issuer "Verizon Public SureServer CA G14-SHA1" (not verified)) by mx1.freebsd.org (Postfix) with ESMTPS id 5486D1D83; Wed, 6 May 2015 23:12:17 +0000 (UTC) X-Loopcount0: from 64.238.244.148 X-IronPort-AV: E=Sophos;i="5.13,381,1427778000"; d="scan'208,217";a="270085008" Message-ID: <554A9FBD.4010908@compellent.com> Date: Wed, 6 May 2015 18:11:57 -0500 From: Eric Badger User-Agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:31.0) Gecko/20100101 Thunderbird/31.3.0 MIME-Version: 1.0 To: Ryan Stone , FreeBSD Current , John Baldwin Subject: Re: PCI PF memory decode disable when sizing VF BARs References: <11092809.7nmbPfKl0V@ralph.baldwin.cx> <2106775.8KnH0oLhUZ@ralph.baldwin.cx> <554A6457.4060702@compellent.com> In-Reply-To: Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit X-Content-Filtered-By: Mailman/MimeDel 2.1.20 X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 06 May 2015 23:12:17 -0000 On 05/06/15 14:54, Ryan Stone wrote: > On Wed, May 6, 2015 at 2:33 PM, John Baldwin > wrote: > > Ah, sorry, I didn't know you did it in the caller already. Perhaps > then something more like your previous patch, but using the test you > added here (PCIR_IS_IOV) instead of your previous check against BAR > values to decide when to frob the command register? > > I think that I prefer the current version, as it keeps the interface > consistent. It's redundant now, but caller could evolve in the > future. Given that this is just being run during initialization a > couple of extra register accesses are irrelevant anyway. > > On Wed, May 6, 2015 at 2:58 PM, Eric Badger > > wrote: > > Does the disabling of VF MSE in pci_iov_config actually protect > anything else beyond what happens in pci_read_bar? I gave a read > through which suggests "no", but I might have missed something. > Just thinking that the code would be a bit more hardy if it were > done the same way for both VFs and other devices. > > Eric > > > I think that it inherently has to be done differently. For real PCI > devices the device might be important during the boot process (e.g. > the video card) so we need to stay working. For VFs the devices don't > even exist until I enable the VF Enable bit is set, so setting MSE > before that point is irrelevant (it's allowed by the spec, but any > access to a VF memory space with MSE set and VF Enable clear just gets > an Unsupported Request response). Sure; what I meant was to leave the disabling of VF MSE when sizing VF BARs in pci_read_bar (as in your second patch) for consistency and, if possible, not bother disabling VF MSE in pci_iov_config. But if it's not worth nixing the latter (or not possible), it's no big deal. I've been testing out the second patch in my environment and it looks good. I might suggest something like the below (which I find more readable) as a cosmetic change: @@ -2627,9 +2635,18 @@ pci_read_bar(device_t dev, int reg, pci_addr_t *mapp, pci_addr_t *testvalp, * determining the BAR's length since we will be placing it in * a weird state. */ - cmd = pci_read_config(dev, PCIR_COMMAND, 2); - pci_write_config(dev, PCIR_COMMAND, - cmd & ~(PCI_BAR_MEM(map) ? PCIM_CMD_MEMEN : PCIM_CMD_PORTEN), 2); +#ifdef PCI_IOV + if (PCIR_IS_IOV(&dinfo->cfg, reg)) { + restore_reg = dinfo->cfg.iov->iov_pos + PCIR_SRIOV_CTL; + mask = PCIM_SRIOV_VF_MSE; + } else +#endif + { + restore_reg = PCIR_COMMAND; + mask = PCI_BAR_MEM(map) ? PCIM_CMD_MEMEN : PCIM_CMD_PORTEN; + } + cmd = pci_read_config(dev, restore_reg, 2); + pci_write_config(dev, restore_reg, cmd & ~mask, 2); Thanks, Eric