From owner-dev-commits-src-all@freebsd.org Thu Apr 8 19:26:46 2021 Return-Path: Delivered-To: dev-commits-src-all@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id BCE605BCEB8; Thu, 8 Apr 2021 19:26:46 +0000 (UTC) (envelope-from jhb@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) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "smtp.freebsd.org", Issuer "R3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4FGWTy54D8z4TpX; Thu, 8 Apr 2021 19:26:46 +0000 (UTC) (envelope-from jhb@FreeBSD.org) Received: from John-Baldwins-MacBook-Pro.local (ralph.baldwin.cx [66.234.199.215]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client did not present a certificate) (Authenticated sender: jhb) by smtp.freebsd.org (Postfix) with ESMTPSA id 2F51D6904; Thu, 8 Apr 2021 19:26:46 +0000 (UTC) (envelope-from jhb@FreeBSD.org) Subject: Re: git: 5a898b2b78ce - main - Set PCIe device's Max_Payload_Size to match PCIe root's. To: Alexander Motin , src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org References: <202104051440.135EeeTZ095177@gitrepo.freebsd.org> <4091ea80-5269-9c78-9fe6-6bba2ed85fbd@FreeBSD.org> <6239ed61-4d35-2cfa-69a7-16da733d8091@FreeBSD.org> From: John Baldwin Message-ID: Date: Thu, 8 Apr 2021 12:26:41 -0700 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:78.0) Gecko/20100101 Thunderbird/78.9.0 MIME-Version: 1.0 In-Reply-To: <6239ed61-4d35-2cfa-69a7-16da733d8091@FreeBSD.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit X-BeenThere: dev-commits-src-all@freebsd.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Commit messages for all branches of the src repository List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 08 Apr 2021 19:26:46 -0000 On 4/8/21 11:02 AM, Alexander Motin wrote: > On 08.04.2021 13:36, John Baldwin wrote: >> On 4/5/21 7:40 AM, Alexander Motin wrote: >>> +    rmps = pcie_read_config(root, PCIER_DEVICE_CTL, 2) & >>> +        PCIEM_CTL_MAX_PAYLOAD; >>> +    mps = pcie_read_config(dev, PCIER_DEVICE_CTL, 2) & >>> +        PCIEM_CTL_MAX_PAYLOAD; >>> +    if (mps == rmps) >>> +        return; >>> +    /* Check whether the device is capable of the root's MPS. */ >>> +    mmps = (pcie_read_config(dev, PCIER_DEVICE_CAP, 2) & >>> +        PCIEM_CAP_MAX_PAYLOAD) << 5; >>> +    if (rmps > mmps) { >>> +        /* >>> +         * The device is unable to handle root's MPS.  Limit root. >>> +         * XXX: We should traverse through all the tree, applying >>> +         * it to all the devices. >>> +         */ >> >> Hmmm, limiting the root seems very dubious here.  Do you really need this? > > All devices under the same root (at least ones that talk to each other) > must have the same MPS value, otherwise some may consider larger > transfer as an error. In case of direct PCIe connection the root is the > only other device, so this code should be sufficient. I mean, had you seen any cases where you needed to adjust the root? I do worry about this breaking other systems due to it not doing the walk of the full sub-tree. Maybe only do it if the root port is the grandparent of the device in question since that is the safe case here and punt if there are switches in the middle? >> If not, I'd put it behind a tunable sysctl that defaults to off.  Ideally >> what you'd do here is use an and of the two masks and select one of those >> bits to choose the value rather than assuming the root can do the device's >> value. > > It is not a bitmask, it is a power-of-2 between 128 bytes and the > maximum device capability. So if the root is already configured for > higher value, then it must support the lover one too. I thought the DEVICE_CAP is a bitmask, and that is what I was thinking of using the and with. Nominally I think we should be doing the AND of the DEVICE_CAP field for all the devices under a port and then programming them to some value remaining in the mask. Most devices just DMA to/from memory though rather than to each other, so generally you just care about the path from a device to a root port. -- John Baldwin