From owner-svn-src-all@freebsd.org Thu Mar 14 12:04:32 2019 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id A6371153FD49; Thu, 14 Mar 2019 12:04:32 +0000 (UTC) (envelope-from johalun@FreeBSD.org) Received: from smtp.freebsd.org (smtp.freebsd.org [IPv6:2610:1c1:1:606c::24b:4]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) server-signature RSA-PSS (4096 bits) client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "smtp.freebsd.org", Issuer "Let's Encrypt Authority X3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4CB6174761; Thu, 14 Mar 2019 12:04:32 +0000 (UTC) (envelope-from johalun@FreeBSD.org) Received: from [192.168.1.33] (unknown [81.174.250.12]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client did not present a certificate) (Authenticated sender: johalun) by smtp.freebsd.org (Postfix) with ESMTPSA id 8A70511CAC; Thu, 14 Mar 2019 12:04:31 +0000 (UTC) (envelope-from johalun@FreeBSD.org) Subject: Re: svn commit: r345103 - head/sys/compat/linuxkpi/common/include/linux To: Konstantin Belousov Cc: John Baldwin , cem@freebsd.org, Hans Petter Selasky , src-committers , svn-src-all , svn-src-head References: <201903131915.x2DJFbRk002502@repo.freebsd.org> <0521e2af-03ae-4642-f88c-60d8b8ede968@FreeBSD.org> <20190314114529.GE2492@kib.kiev.ua> From: Johannes Lundberg X-Tagtoolbar-Keys: D20190314120429942 Message-ID: <21dcbc21-e3a2-64a2-97ba-a612163ebb9d@FreeBSD.org> Date: Thu, 14 Mar 2019 12:04:30 +0000 User-Agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:60.0) Gecko/20100101 Thunderbird/60.4.0 MIME-Version: 1.0 In-Reply-To: <20190314114529.GE2492@kib.kiev.ua> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Content-Language: en-US X-Rspamd-Queue-Id: 4CB6174761 X-Spamd-Bar: ------ Authentication-Results: mx1.freebsd.org X-Spamd-Result: default: False [-6.98 / 15.00]; NEURAL_HAM_MEDIUM(-1.00)[-1.000,0]; NEURAL_HAM_SHORT(-0.98)[-0.981,0]; NEURAL_HAM_LONG(-1.00)[-1.000,0]; REPLY(-4.00)[] X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 14 Mar 2019 12:04:33 -0000 On 3/14/19 11:45 AM, Konstantin Belousov wrote: > On Thu, Mar 14, 2019 at 11:07:50AM +0000, Johannes Lundberg wrote: >> On 3/13/19 11:39 PM, John Baldwin wrote: >>> On 3/13/19 1:36 PM, Conrad Meyer wrote: >>>> Hi, >>>> >>>> A lot of the information about PCIe devices is read by PCI probe and >>>> cached on the (BSD) device. You could access it out of >>>> device_get_ivars(bsddev)->cfg.pcie and avoid the MMIO latency. >>> Agreed when possible, though I'm not sure caching lnkcap is really >>> all that useful. >>> >>>> On Wed, Mar 13, 2019 at 12:15 PM Hans Petter Selasky >>>> wrote: >>>>> +static inline enum pci_bus_speed >>>>> +pcie_get_speed_cap(struct pci_dev *dev) >>>>> +{ >>>>> + device_t root; >>>>> + uint32_t lnkcap, lnkcap2; >>>>> + int error, pos; >>>>> + >>>>> + root = device_get_parent(dev->dev.bsddev); >>>>> + if (root == NULL) >>>>> + return (PCI_SPEED_UNKNOWN); >>>>> + root = device_get_parent(root); >>>>> + if (root == NULL) >>>>> + return (PCI_SPEED_UNKNOWN); >>>>> + root = device_get_parent(root); >>>>> + if (root == NULL) >>>>> + return (PCI_SPEED_UNKNOWN); >>>> What is this mechanism trying to accomplish? It seems incredibly >>>> fragile. Looking for pci0? pcib0? >>> It does seem broken, or maybe that it only works for DRM drivers and nothing else. >>> For non-DRM drivers, 'bsddev' is a PCI-e endpoint. You would then go up two layers >>> (pciX and pcibX) to get to the parent bridge. However, this is assuming a DRM layout >>> where it has to go drm0 -> vgapci0 -> pciX -> pcibX due to the vgapci pseudo-driver. >>> As a result, this function can't possibly work on anything other than a DRM driver. >>> Since it would try to use pci ivars on some PCI bus instance (or worse), it's likely >>> to cause random panics if used from a non-DRM driver. >>> >>> Furthermore, it's not at all clear to me why you can't just read lnkcap/lnkcap2 from >>> the endpoint device directly vs heading up to the parent bridge though. Hmm, I guess >>> it's trying to infer the capabilities of the "slot", so that's why it needs the >>> grandparent. You will have to do something less fragile to find the grandparent. >>> The simplest way may be to walk up to the first "pcib" device you see and then >>> stop. You will still want to see if it's a "real" PCI device by seeing if it's >>> parent is a "pci" device (meaning that the "pcib" device will have valid PCI IVARs >>> and PCI config registers you can read). pci_find_pcie_root_port has some similar >>> code for this, but that function would walk too far up for what you want here, so you >>> can't reuse it as-is. >>> >>>>> + if ((error = pci_find_cap(root, PCIY_EXPRESS, &pos)) != 0) >>>>> + return (PCI_SPEED_UNKNOWN); >>>> Cached as non-zero cfg.pcie.pcie_location value in ivars. >>>> >>>>> + lnkcap2 = pci_read_config(root, pos + PCIER_LINK_CAP2, 4); >>>> This one we don't cache, unfortunately, but could. Ditto LINK_CAP >>>> below. Usually "pos + PCIEfoo" is spelled "pcie_read_config(..., >>>> PCIEfoo...)." >>> Yes, pcie_read_config is what you normally would use. That uses the cached >>> pcie_location for you. >>> >>> pcie_read_config(dev->dev.bsddev, PCIER_LINK_CAP2, 2); >>> >>> But also, you should be checking the PCIe version to see if this register >>> exists instead of reading it unconditionally. Specifically, you should read >>> the version field (PCIEM_FLAGS_VERSION) from PCIER_FLAGS. LINK_CAP2 only >>> exists if the version >= 2. >>> >>>>> + >>>>> + if (lnkcap2) { /* PCIe r3.0-compliant */ >>>>> + if (lnkcap2 & PCI_EXP_LNKCAP2_SLS_2_5GB) >>>>> + return (PCIE_SPEED_2_5GT); >>>> Seems like these definitions would be better suited as native >>>> PCIEM_LINK_CAP2_foo definitions in pcireg.h >>> I feel less strongly about those since we have to provide the constants >>> anyway for consumers to use. >>> >>>>> + if (lnkcap2 & PCI_EXP_LNKCAP2_SLS_5_0GB) >>>>> + return (PCIE_SPEED_5_0GT); >>>>> + if (lnkcap2 & PCI_EXP_LNKCAP2_SLS_8_0GB) >>>>> + return (PCIE_SPEED_8_0GT); >>>>> + if (lnkcap2 & PCI_EXP_LNKCAP2_SLS_16_0GB) >>>>> + return (PCIE_SPEED_16_0GT); >>>>> + } else { /* pre-r3.0 */ >>>>> + lnkcap = pci_read_config(root, pos + PCIER_LINK_CAP, 4); >>>>> + if (lnkcap & PCI_EXP_LNKCAP_SLS_2_5GB) >>>>> + return (PCIE_SPEED_2_5GT); >>>>> + if (lnkcap & PCI_EXP_LNKCAP_SLS_5_0GB) >>>>> + return (PCIE_SPEED_5_0GT); >>>>> + if (lnkcap & PCI_EXP_LNKCAP_SLS_8_0GB) >>>>> + return (PCIE_SPEED_8_0GT); >>>>> + if (lnkcap & PCI_EXP_LNKCAP_SLS_16_0GB) >>>>> + return (PCIE_SPEED_16_0GT); >>>>> + } >>>>> + return (PCI_SPEED_UNKNOWN); >>>>> +} >>>>> + >>>>> +static inline enum pcie_link_width >>>>> +pcie_get_width_cap(struct pci_dev *dev) >>>>> +{ >>>>> + uint32_t lnkcap; >>>>> + >>>>> + pcie_capability_read_dword(dev, PCI_EXP_LNKCAP, &lnkcap); >>>> Better spelled as PCIER_LINK_CAP. >>>> >>>>> + if (lnkcap) >>>>> + return ((lnkcap & PCI_EXP_LNKCAP_MLW) >> 4); >>>> And PCIEM_LINK_CAP_MAX_WIDTH. >>> Given that this is using a linuxkpi API (pcie_capability_read_dword >>> instead of pcie_read_config()) then the rest of this function is >>> written in the Linux KPI, so I think using Linux' native constants >>> is actually more consistent for this function. >> Hi >> >> Thanks for the feedback. >> >> This code used to exist in the drm driver but was moved into kernel pci >> code on Linux. It's more or less as it existed in the drm driver with >> FreeBSD patches having roots probably going back to drm2 from base. If > No, I assure you that it did not appeared in drm2. For a bit of history: drm2: https://github.com/freebsd/freebsd/blob/release/12.0.0/sys/dev/drm2/drm_pci.c#L439 later drm-next: https://github.com/FreeBSDDesktop/kms-drm/blob/drm-v4.16/drivers/gpu/drm/drm_pci.c#L441 and now linuxkpi: https://github.com/freebsd/freebsd/blob/master/sys/compat/linuxkpi/common/include/linux/pci.h#L866 Upstream patch: https://patchwork.kernel.org/patch/10487273/ > >> this function won't be compatible with anything but drm drivers, we >> could just as well move it to linuxkpi in ports instead. > It only works for GPUs which are either pseudo-PCIe devices, i.e. > they represent itself as PCIe-attached devices for configuration purposes > but really attached to the CPU ring directly. This is true for CPU- > integrated graphics. Or for lucky circumstances where external > GPU is plugged into CPU PCIe slot without any PCIe bridges. > > If PCIe GPU is connected through a switch (which happen quite often > these days), or into the slot managed by south bridge (I believe this > is theoretically possible) then the code would not work. > > Perhaps you want dmar_get_requester() which does exactly what you > (seems) try to get for PCIe devices attached without non-PCIe bridges. > >> I'll try to implement the suggested improvements. Keep on eye out for >> updates here https://reviews.freebsd.org/D19565 >> >> /Johannes >> >> >> >>