[{"id":1750416,"web_url":"http://patchwork.ozlabs.org/comment/1750416/","msgid":"<2163ef83-3633-a8cf-3416-0e37a6c500db@ozlabs.ru>","list_archive_url":null,"date":"2017-08-17T22:05:42","subject":"Re: [PATCH kernel] PCI: Disable IOV before pcibios_sriov_disable()","submitter":{"id":7621,"url":"http://patchwork.ozlabs.org/api/people/7621/","name":"Alexey Kardashevskiy","email":"aik@ozlabs.ru"},"content":"On 11/08/17 18:19, Alexey Kardashevskiy wrote:\n> From: Gavin Shan \n> \n> The PowerNV platform is the only user of pcibios_sriov_disable().\n> The IOV BAR could be shifted by pci_iov_update_resource(). The\n> warning message in the function is printed if the IOV capability\n> is in enabled (PCI_SRIOV_CTRL_VFE && PCI_SRIOV_CTRL_MSE) state.\n> \n> This is the backtrace of what is happening:\n> pci_disable_sriov\n> sriov_disable\n> pnv_pci_sriov_disable\n> pnv_pci_vf_resource_shift\n> pci_update_resource\n> pci_iov_update_resource\n> \n> This fixes the issue by disabling IOV capability before calling\n> pcibios_sriov_disable(). With it, the disabling path matches\n> the enabling path: pcibios_sriov_enable() is called before the\n> IOV capability is enabled.\n> \n> Cc: shan.gavin@gmail.com\n> Cc: Benjamin Herrenschmidt \n> Cc: Paul Mackerras \n> Reported-by: Carol L Soto \n> Signed-off-by: Gavin Shan \n> Tested-by: Carol L Soto \n> Signed-off-by: Alexey Kardashevskiy \n> ---\n> \n> This is repost. Since Gavin left the team, I am trying to push it out.\n> The previos converstion is here: https://patchwork.ozlabs.org/patch/732653/\n> \n> Two questions were raised then. I'll try to comment on this below.\n\nBjorn, ping? Thanks.\n\n> \n>> 1) \"res\" is already in the resource tree, so we shouldn't be changing\n>> its start address, because that may make the tree inconsistent,\n>> e.g., the resource may no longer be completely contained in its\n>> parent, it may conflict with a sibling, etc.\n> \n> We should not, yes. But...\n> \n> At the boot time IOV BAR gets as much MMIO space as it can possibly use.\n> (Embarassingly I cannot trace where this is coming from, 8GB is selected\n> via pci_assign_unassigned_root_bus_resources() path somehow).\n> For example, it is 256*32MB=8GB where 256 is maximum PEs number and 32MB\n> is a PF/VF BAR size. Whatever shifting we do afterwards, the boudaries of\n> that 8GB area do not change and we test it in pnv_pci_vf_resource_shift():\n> \n> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/powerpc/platforms/powernv/pci-ioda.c#n987\n> \n>> 2) If we update \"res->start\", shouldn't we update \"res->end\"\n>> correspondingly?\n> \n> We have to update the PF's IOV BAR address as we allocate PEs dynamically\n> and we do not know in advance where our VF numbers start in that\n> 8GB window. So we change IOV BASR start. Changing the end may make it\n> look more like there is a free area to use but in reality it won't be\n> usable as well as the area we \"release\" by shifting the start address.\n> \n> We could probably move that M64 MMIO window by the same delta in\n> opposite direction so the IOV BAR start address would remain the same\n> but its VF#0 would be mapped to let's say PF#5. I am just afraid there\n> is an alignment requirement for these M64 window start address; and this\n> would be even more tricky to manage.\n> \n> We could also create reserved areas for the amount of space \"release\" by\n> moving the start address, not sure how though.\n> \n> So how do we proceed with this particular patch now? Thanks.\n> ---\n> drivers/pci/iov.c | 7 ++++---\n> 1 file changed, 4 insertions(+), 3 deletions(-)\n> \n> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c\n> index 120485d6f352..ac41c8be9200 100644\n> --- a/drivers/pci/iov.c\n> +++ b/drivers/pci/iov.c\n> @@ -331,7 +331,6 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)\n> \twhile (i--)\n> \t\tpci_iov_remove_virtfn(dev, i, 0);\n> \n> -\tpcibios_sriov_disable(dev);\n> err_pcibios:\n> \tiov->ctrl &= ~(PCI_SRIOV_CTRL_VFE | PCI_SRIOV_CTRL_MSE);\n> \tpci_cfg_access_lock(dev);\n> @@ -339,6 +338,8 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)\n> \tssleep(1);\n> \tpci_cfg_access_unlock(dev);\n> \n> +\tpcibios_sriov_disable(dev);\n> +\n> \tif (iov->link != dev->devfn)\n> \t\tsysfs_remove_link(&dev->dev.kobj, \"dep_link\");\n> \n> @@ -357,14 +358,14 @@ static void sriov_disable(struct pci_dev *dev)\n> \tfor (i = 0; i < iov->num_VFs; i++)\n> \t\tpci_iov_remove_virtfn(dev, i, 0);\n> \n> -\tpcibios_sriov_disable(dev);\n> -\n> \tiov->ctrl &= ~(PCI_SRIOV_CTRL_VFE | PCI_SRIOV_CTRL_MSE);\n> \tpci_cfg_access_lock(dev);\n> \tpci_write_config_word(dev, iov->pos + PCI_SRIOV_CTRL, iov->ctrl);\n> \tssleep(1);\n> \tpci_cfg_access_unlock(dev);\n> \n> +\tpcibios_sriov_disable(dev);\n> +\n> \tif (iov->link != dev->devfn)\n> \t\tsysfs_remove_link(&dev->dev.kobj, \"dep_link\");\n> \n>","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=linux-pci-owner@vger.kernel.org;\n\treceiver=)","ozlabs.org; dkim=pass (2048-bit key;\n\tunprotected) header.d=ozlabs-ru.20150623.gappssmtp.com\n\theader.i=@ozlabs-ru.20150623.gappssmtp.com\n\theader.b=\"1HuLrJ5W\"; dkim-atps=neutral"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xYL7j33gQz9t3m\n\tfor ;\n\tFri, 18 Aug 2017 08:13:05 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1753763AbdHQWMv (ORCPT );\n\tThu, 17 Aug 2017 18:12:51 -0400","from mail-pg0-f66.google.com ([74.125.83.66]:33591 \"EHLO\n\tmail-pg0-f66.google.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1753849AbdHQWFw (ORCPT\n\t); Thu, 17 Aug 2017 18:05:52 -0400","by mail-pg0-f66.google.com with SMTP id u185so11818084pgb.0\n\tfor ; Thu, 17 Aug 2017 15:05:51 -0700 (PDT)","from [192.168.10.22] (124-171-136-142.dyn.iinet.net.au.\n\t[124.171.136.142]) by smtp.googlemail.com with ESMTPSA id\n\tr82sm9077818pfe.0.2017.08.17.15.05.44\n\t(version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128);\n\tThu, 17 Aug 2017 15:05:50 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ozlabs-ru.20150623.gappssmtp.com; s=20150623;\n\th=subject:to:cc:references:from:message-id:date:user-agent\n\t:mime-version:in-reply-to:content-language:content-transfer-encoding; \n\tbh=k3607myWMHiEHryn8aDiRRKKwAa9CnzuAnffO5aieoM=;\n\tb=1HuLrJ5WyScAflS5VMpwhOtNbOzgT8a++mGf/Svk4l5Ysx+rt429tHx7OZ2ELPhZKL\n\tg+qNLd4QdmWkOIHMyyK1iqrsd1nAFspovqfArHpFAXj46yP0mzLBSjcC/SXcuBDTnE2B\n\tEN0RfuoLchafSeDYbmvhlZCytlngPVllb5HvWQAEvV1uj0ugYMVAI/1X9+y37dyjb3dC\n\tX/nLBESgDILl1aTqMsPrm7Xami7U183VwmBaEXkRTC0yFTjt9X6IicPyXtBm/H21qMna\n\tL83mn7wowY6s6HujEs4W37RwjWz0u/JY9p3OHZ/T8arCz2KDzLNRfLOBBFI/NsHoWICO\n\tPUkQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:subject:to:cc:references:from:message-id:date\n\t:user-agent:mime-version:in-reply-to:content-language\n\t:content-transfer-encoding;\n\tbh=k3607myWMHiEHryn8aDiRRKKwAa9CnzuAnffO5aieoM=;\n\tb=p6/Eb/N5/tdwk3T8LN4l5qDvXGxv3rt0+CY6un6+ldxilfZtzLcDiJPOadXfY7tNuk\n\tG/LrvwRzfPTjBM2bqWoXZIAXA95ZmPkUSpv0lZHjlFFScO/BRSl+PsDge8YYQb5r2S9+\n\tJQON/Q+DZcRIhQKU8PMHu1kef/FXQkrxXCiD4vyoKcQg8omw+jlhK3dU9+ANPLu3Tei+\n\thYXERdNPpgmuEc18BTIvvdKEHcAm0FYrjaMZ+Cw49W+dH8o3Kar9388jiwoYoM5ZHahH\n\tPDvplvEGBzOcNKx2dReH7yDUZ6TvFJlVsY/lRnyJdADqKFZ3lunFoqWrd3fSjKavmoBT\n\tqBRg==","X-Gm-Message-State":"AHYfb5gqM6rAXV5Pn3TQDUbtB4QT79i9p2l0x/g4DSrkFgqLqD5TkKwC\n\tMhTAmuSNfM9qkECtHtA=","X-Received":"by 10.98.200.151 with SMTP id i23mr6718052pfk.281.1503007551256; \n\tThu, 17 Aug 2017 15:05:51 -0700 (PDT)","Subject":"Re: [PATCH kernel] PCI: Disable IOV before pcibios_sriov_disable()","To":"linuxppc-dev@lists.ozlabs.org, Bjorn Helgaas ","Cc":"kvm@vger.kernel.org, Yongji Xie ,\n\tEric Auger ,\n\tlinux-kernel@vger.kernel.org, linux-pci@vger.kernel.org,\n\tshan.gavin@gmail.com, Benjamin Herrenschmidt ,\n\tPaul Mackerras , Gavin Shan ","References":"<20170811081933.17474-1-aik@ozlabs.ru>","From":"Alexey Kardashevskiy ","Message-ID":"<2163ef83-3633-a8cf-3416-0e37a6c500db@ozlabs.ru>","Date":"Fri, 18 Aug 2017 08:05:42 +1000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.2.1","MIME-Version":"1.0","In-Reply-To":"<20170811081933.17474-1-aik@ozlabs.ru>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-AU","Content-Transfer-Encoding":"7bit","Sender":"linux-pci-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-pci@vger.kernel.org"}},{"id":1751193,"web_url":"http://patchwork.ozlabs.org/comment/1751193/","msgid":"<20170818152740.GL28977@bhelgaas-glaptop.roam.corp.google.com>","list_archive_url":null,"date":"2017-08-18T15:27:40","subject":"Re: [PATCH kernel] PCI: Disable IOV before pcibios_sriov_disable()","submitter":{"id":67298,"url":"http://patchwork.ozlabs.org/api/people/67298/","name":"Bjorn Helgaas","email":"helgaas@kernel.org"},"content":"On Fri, Aug 18, 2017 at 08:05:42AM +1000, Alexey Kardashevskiy wrote:\n> On 11/08/17 18:19, Alexey Kardashevskiy wrote:\n> > From: Gavin Shan \n> > \n> > The PowerNV platform is the only user of pcibios_sriov_disable().\n> > The IOV BAR could be shifted by pci_iov_update_resource(). The\n> > warning message in the function is printed if the IOV capability\n> > is in enabled (PCI_SRIOV_CTRL_VFE && PCI_SRIOV_CTRL_MSE) state.\n> > \n> > This is the backtrace of what is happening:\n> > pci_disable_sriov\n> > sriov_disable\n> > pnv_pci_sriov_disable\n> > pnv_pci_vf_resource_shift\n> > pci_update_resource\n> > pci_iov_update_resource\n> > \n> > This fixes the issue by disabling IOV capability before calling\n> > pcibios_sriov_disable(). With it, the disabling path matches\n> > the enabling path: pcibios_sriov_enable() is called before the\n> > IOV capability is enabled.\n> > \n> > Cc: shan.gavin@gmail.com\n> > Cc: Benjamin Herrenschmidt \n> > Cc: Paul Mackerras \n> > Reported-by: Carol L Soto \n> > Signed-off-by: Gavin Shan \n> > Tested-by: Carol L Soto \n> > Signed-off-by: Alexey Kardashevskiy \n> > ---\n> > \n> > This is repost. Since Gavin left the team, I am trying to push it out.\n> > The previos converstion is here: https://patchwork.ozlabs.org/patch/732653/\n> > \n> > Two questions were raised then. I'll try to comment on this below.\n> \n> Bjorn, ping? Thanks.\n\nThanks for the reminder. This is in patchwork, so it's on my to-do\nlist.\n\nMy last response in the thread above was:\n\n I'm not going to merge this without a comment in\n pnv_pci_vf_resource_shift() that addresses the two questions I\n raised in my very first response. I don't think the existing\n comment about \"After doing so, there would be a 'hole'\" is\n sufficient. If it were sufficient, I wouldn't have raised the\n questions in the first place.\n\nThe problem here is that I'm looking for a comment *in the code*, and\nyou and Gavin are giving responses and clarifications in email.\n\nWhat we need to do is transfer this email information into something\nuseful when reading the code, i.e., a comment in the code.\n\n> >> 1) \"res\" is already in the resource tree, so we shouldn't be changing\n> >> its start address, because that may make the tree inconsistent,\n> >> e.g., the resource may no longer be completely contained in its\n> >> parent, it may conflict with a sibling, etc.\n> > \n> > We should not, yes. But...\n> > \n> > At the boot time IOV BAR gets as much MMIO space as it can possibly use.\n> > (Embarassingly I cannot trace where this is coming from, 8GB is selected\n> > via pci_assign_unassigned_root_bus_resources() path somehow).\n> > For example, it is 256*32MB=8GB where 256 is maximum PEs number and 32MB\n> > is a PF/VF BAR size. Whatever shifting we do afterwards, the boudaries of\n> > that 8GB area do not change and we test it in pnv_pci_vf_resource_shift():\n> > \n> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/powerpc/platforms/powernv/pci-ioda.c#n987\n> > \n> >> 2) If we update \"res->start\", shouldn't we update \"res->end\"\n> >> correspondingly?\n> > \n> > We have to update the PF's IOV BAR address as we allocate PEs dynamically\n> > and we do not know in advance where our VF numbers start in that\n> > 8GB window. So we change IOV BASR start. Changing the end may make it\n> > look more like there is a free area to use but in reality it won't be\n> > usable as well as the area we \"release\" by shifting the start address.\n> > \n> > We could probably move that M64 MMIO window by the same delta in\n> > opposite direction so the IOV BAR start address would remain the same\n> > but its VF#0 would be mapped to let's say PF#5. I am just afraid there\n> > is an alignment requirement for these M64 window start address; and this\n> > would be even more tricky to manage.\n> > \n> > We could also create reserved areas for the amount of space \"release\" by\n> > moving the start address, not sure how though.\n> > \n> > So how do we proceed with this particular patch now? Thanks.\n> > ---\n> > drivers/pci/iov.c | 7 ++++---\n> > 1 file changed, 4 insertions(+), 3 deletions(-)\n> > \n> > diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c\n> > index 120485d6f352..ac41c8be9200 100644\n> > --- a/drivers/pci/iov.c\n> > +++ b/drivers/pci/iov.c\n> > @@ -331,7 +331,6 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)\n> > \twhile (i--)\n> > \t\tpci_iov_remove_virtfn(dev, i, 0);\n> > \n> > -\tpcibios_sriov_disable(dev);\n> > err_pcibios:\n> > \tiov->ctrl &= ~(PCI_SRIOV_CTRL_VFE | PCI_SRIOV_CTRL_MSE);\n> > \tpci_cfg_access_lock(dev);\n> > @@ -339,6 +338,8 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)\n> > \tssleep(1);\n> > \tpci_cfg_access_unlock(dev);\n> > \n> > +\tpcibios_sriov_disable(dev);\n> > +\n> > \tif (iov->link != dev->devfn)\n> > \t\tsysfs_remove_link(&dev->dev.kobj, \"dep_link\");\n> > \n> > @@ -357,14 +358,14 @@ static void sriov_disable(struct pci_dev *dev)\n> > \tfor (i = 0; i < iov->num_VFs; i++)\n> > \t\tpci_iov_remove_virtfn(dev, i, 0);\n> > \n> > -\tpcibios_sriov_disable(dev);\n> > -\n> > \tiov->ctrl &= ~(PCI_SRIOV_CTRL_VFE | PCI_SRIOV_CTRL_MSE);\n> > \tpci_cfg_access_lock(dev);\n> > \tpci_write_config_word(dev, iov->pos + PCI_SRIOV_CTRL, iov->ctrl);\n> > \tssleep(1);\n> > \tpci_cfg_access_unlock(dev);\n> > \n> > +\tpcibios_sriov_disable(dev);\n> > +\n> > \tif (iov->link != dev->devfn)\n> > \t\tsysfs_remove_link(&dev->dev.kobj, \"dep_link\");\n> > \n> > \n> \n> \n> -- \n> Alexey","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=linux-pci-owner@vger.kernel.org;\n\treceiver=)","mail.kernel.org;\n\tdmarc=none (p=none dis=none) header.from=kernel.org","mail.kernel.org;\n\tspf=none smtp.mailfrom=helgaas@kernel.org"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xYn5Z4WxSz9s4s\n\tfor ;\n\tSat, 19 Aug 2017 01:27:46 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751489AbdHRP1o (ORCPT );\n\tFri, 18 Aug 2017 11:27:44 -0400","from mail.kernel.org ([198.145.29.99]:43076 \"EHLO mail.kernel.org\"\n\trhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP\n\tid S1750966AbdHRP1n (ORCPT );\n\tFri, 18 Aug 2017 11:27:43 -0400","from localhost (unknown [69.71.4.159])\n\t(using TLSv1.2 with cipher DHE-RSA-AES128-SHA (128/128 bits))\n\t(No client certificate requested)\n\tby mail.kernel.org (Postfix) with ESMTPSA id AC99823695;\n\tFri, 18 Aug 2017 15:27:42 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mail.kernel.org AC99823695","Date":"Fri, 18 Aug 2017 10:27:40 -0500","From":"Bjorn Helgaas ","To":"Alexey Kardashevskiy ","Cc":"linuxppc-dev@lists.ozlabs.org, Bjorn Helgaas ,\n\tkvm@vger.kernel.org, Yongji Xie ,\n\tEric Auger ,\n\tlinux-kernel@vger.kernel.org, linux-pci@vger.kernel.org,\n\tshan.gavin@gmail.com, Benjamin Herrenschmidt ,\n\tPaul Mackerras , Gavin Shan ","Subject":"Re: [PATCH kernel] PCI: Disable IOV before pcibios_sriov_disable()","Message-ID":"<20170818152740.GL28977@bhelgaas-glaptop.roam.corp.google.com>","References":"<20170811081933.17474-1-aik@ozlabs.ru>\n\t<2163ef83-3633-a8cf-3416-0e37a6c500db@ozlabs.ru>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<2163ef83-3633-a8cf-3416-0e37a6c500db@ozlabs.ru>","User-Agent":"Mutt/1.5.21 (2010-09-15)","Sender":"linux-pci-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-pci@vger.kernel.org"}},{"id":1752372,"web_url":"http://patchwork.ozlabs.org/comment/1752372/","msgid":"","list_archive_url":null,"date":"2017-08-21T04:00:28","subject":"Re: [PATCH kernel] PCI: Disable IOV before pcibios_sriov_disable()","submitter":{"id":7621,"url":"http://patchwork.ozlabs.org/api/people/7621/","name":"Alexey Kardashevskiy","email":"aik@ozlabs.ru"},"content":"On 19/08/17 01:27, Bjorn Helgaas wrote:\n> On Fri, Aug 18, 2017 at 08:05:42AM +1000, Alexey Kardashevskiy wrote:\n>> On 11/08/17 18:19, Alexey Kardashevskiy wrote:\n>>> From: Gavin Shan \n>>>\n>>> The PowerNV platform is the only user of pcibios_sriov_disable().\n>>> The IOV BAR could be shifted by pci_iov_update_resource(). The\n>>> warning message in the function is printed if the IOV capability\n>>> is in enabled (PCI_SRIOV_CTRL_VFE && PCI_SRIOV_CTRL_MSE) state.\n>>>\n>>> This is the backtrace of what is happening:\n>>> pci_disable_sriov\n>>> sriov_disable\n>>> pnv_pci_sriov_disable\n>>> pnv_pci_vf_resource_shift\n>>> pci_update_resource\n>>> pci_iov_update_resource\n>>>\n>>> This fixes the issue by disabling IOV capability before calling\n>>> pcibios_sriov_disable(). With it, the disabling path matches\n>>> the enabling path: pcibios_sriov_enable() is called before the\n>>> IOV capability is enabled.\n>>>\n>>> Cc: shan.gavin@gmail.com\n>>> Cc: Benjamin Herrenschmidt \n>>> Cc: Paul Mackerras \n>>> Reported-by: Carol L Soto \n>>> Signed-off-by: Gavin Shan \n>>> Tested-by: Carol L Soto \n>>> Signed-off-by: Alexey Kardashevskiy \n>>> ---\n>>>\n>>> This is repost. Since Gavin left the team, I am trying to push it out.\n>>> The previos converstion is here: https://patchwork.ozlabs.org/patch/732653/\n>>>\n>>> Two questions were raised then. I'll try to comment on this below.\n>>\n>> Bjorn, ping? Thanks.\n> \n> Thanks for the reminder. This is in patchwork, so it's on my to-do\n> list.\n> \n> My last response in the thread above was:\n> \n> I'm not going to merge this without a comment in\n> pnv_pci_vf_resource_shift() that addresses the two questions I\n> raised in my very first response. I don't think the existing\n> comment about \"After doing so, there would be a 'hole'\" is\n> sufficient. If it were sufficient, I wouldn't have raised the\n> questions in the first place.\n> \n> The problem here is that I'm looking for a comment *in the code*, and\n> you and Gavin are giving responses and clarifications in email.\n\nI understand.\n\n> What we need to do is transfer this email information into something\n> useful when reading the code, i.e., a comment in the code.\n\nI agree. Here are few problems though.\n\n1. I am not sure if there is nothing to fix (like adding dummy regions to\nprevent these \"holes\" to be used for something else)\n\n2. I do not really know how exactly to update the comment in the code to\nmake it clearer for the future readers, and before doing this, I'd like to\nunderstand about 1).\n\n\nAn example of the comment which would make sense to me:\n\n@@ -1002,9 +1016,13 @@ static int pnv_pci_vf_resource_shift(struct pci_dev\n*dev, int offset)\n }\n\n /*\n- * After doing so, there would be a \"hole\" in the /proc/iomem when\n- * offset is a positive value. It looks like the device return some\n- * mmio back to the system, which actually no one could use it.\n+ * Since M64 BAR shares segments among all possible 256 PEs,\n+ * we have to shift the beginning of PF IOV BAR to make it start from\n+ * the segment which belongs to the PE number assigned to the first VF.\n+ * This creates a \"hole\" in the /proc/iomem which could be used for\n+ * allocating other resources, however this is not expected to happen\n+ * on IODA as the only possibility would be a PCI hotplug and IODA\n+ * hardware only allows it on a slot with dedicated PHB.\n */\n\n\nDoes this make sense? Thanks.\n\n\n\n> \n>>>> 1) \"res\" is already in the resource tree, so we shouldn't be changing\n>>>> its start address, because that may make the tree inconsistent,\n>>>> e.g., the resource may no longer be completely contained in its\n>>>> parent, it may conflict with a sibling, etc.\n>>>\n>>> We should not, yes. But...\n>>>\n>>> At the boot time IOV BAR gets as much MMIO space as it can possibly use.\n>>> (Embarassingly I cannot trace where this is coming from, 8GB is selected\n>>> via pci_assign_unassigned_root_bus_resources() path somehow).\n>>> For example, it is 256*32MB=8GB where 256 is maximum PEs number and 32MB\n>>> is a PF/VF BAR size. Whatever shifting we do afterwards, the boudaries of\n>>> that 8GB area do not change and we test it in pnv_pci_vf_resource_shift():\n>>>\n>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/powerpc/platforms/powernv/pci-ioda.c#n987\n>>>\n>>>> 2) If we update \"res->start\", shouldn't we update \"res->end\"\n>>>> correspondingly?\n>>>\n>>> We have to update the PF's IOV BAR address as we allocate PEs dynamically\n>>> and we do not know in advance where our VF numbers start in that\n>>> 8GB window. So we change IOV BASR start. Changing the end may make it\n>>> look more like there is a free area to use but in reality it won't be\n>>> usable as well as the area we \"release\" by shifting the start address.\n>>>\n>>> We could probably move that M64 MMIO window by the same delta in\n>>> opposite direction so the IOV BAR start address would remain the same\n>>> but its VF#0 would be mapped to let's say PF#5. I am just afraid there\n>>> is an alignment requirement for these M64 window start address; and this\n>>> would be even more tricky to manage.\n>>>\n>>> We could also create reserved areas for the amount of space \"release\" by\n>>> moving the start address, not sure how though.\n>>>\n>>> So how do we proceed with this particular patch now? Thanks.\n>>> ---\n>>> drivers/pci/iov.c | 7 ++++---\n>>> 1 file changed, 4 insertions(+), 3 deletions(-)\n>>>\n>>> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c\n>>> index 120485d6f352..ac41c8be9200 100644\n>>> --- a/drivers/pci/iov.c\n>>> +++ b/drivers/pci/iov.c\n>>> @@ -331,7 +331,6 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)\n>>> \twhile (i--)\n>>> \t\tpci_iov_remove_virtfn(dev, i, 0);\n>>> \n>>> -\tpcibios_sriov_disable(dev);\n>>> err_pcibios:\n>>> \tiov->ctrl &= ~(PCI_SRIOV_CTRL_VFE | PCI_SRIOV_CTRL_MSE);\n>>> \tpci_cfg_access_lock(dev);\n>>> @@ -339,6 +338,8 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)\n>>> \tssleep(1);\n>>> \tpci_cfg_access_unlock(dev);\n>>> \n>>> +\tpcibios_sriov_disable(dev);\n>>> +\n>>> \tif (iov->link != dev->devfn)\n>>> \t\tsysfs_remove_link(&dev->dev.kobj, \"dep_link\");\n>>> \n>>> @@ -357,14 +358,14 @@ static void sriov_disable(struct pci_dev *dev)\n>>> \tfor (i = 0; i < iov->num_VFs; i++)\n>>> \t\tpci_iov_remove_virtfn(dev, i, 0);\n>>> \n>>> -\tpcibios_sriov_disable(dev);\n>>> -\n>>> \tiov->ctrl &= ~(PCI_SRIOV_CTRL_VFE | PCI_SRIOV_CTRL_MSE);\n>>> \tpci_cfg_access_lock(dev);\n>>> \tpci_write_config_word(dev, iov->pos + PCI_SRIOV_CTRL, iov->ctrl);\n>>> \tssleep(1);\n>>> \tpci_cfg_access_unlock(dev);\n>>> \n>>> +\tpcibios_sriov_disable(dev);\n>>> +\n>>> \tif (iov->link != dev->devfn)\n>>> \t\tsysfs_remove_link(&dev->dev.kobj, \"dep_link\");\n>>> \n>>>\n>>\n>>\n>> -- \n>> Alexey","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=linux-pci-owner@vger.kernel.org;\n\treceiver=)","ozlabs.org; dkim=pass (2048-bit key;\n\tunprotected) header.d=ozlabs-ru.20150623.gappssmtp.com\n\theader.i=@ozlabs-ru.20150623.gappssmtp.com\n\theader.b=\"S9sFVLEZ\"; dkim-atps=neutral"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xbKjf4G8bz9tWX\n\tfor ;\n\tMon, 21 Aug 2017 14:00:54 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751048AbdHUEAh (ORCPT );\n\tMon, 21 Aug 2017 00:00:37 -0400","from mail-pg0-f68.google.com ([74.125.83.68]:33657 \"EHLO\n\tmail-pg0-f68.google.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1750850AbdHUEAf (ORCPT\n\t); Mon, 21 Aug 2017 00:00:35 -0400","by mail-pg0-f68.google.com with SMTP id n4so6955551pgn.0\n\tfor ; Sun, 20 Aug 2017 21:00:35 -0700 (PDT)","from [10.61.2.175] ([122.99.82.10])\n\tby smtp.googlemail.com with ESMTPSA id\n\tr9sm3965836pfd.126.2017.08.20.21.00.30\n\t(version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128);\n\tSun, 20 Aug 2017 21:00:33 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ozlabs-ru.20150623.gappssmtp.com; s=20150623;\n\th=from:subject:to:cc:references:message-id:date:user-agent\n\t:mime-version:in-reply-to:content-language:content-transfer-encoding; \n\tbh=cEqo3iamGkSNeDtpcuFL77UvJ6AqmrnSxi85gTRSv2w=;\n\tb=S9sFVLEZmmSfEp3uLm1h69Kl/SRxUYrLsPWZfigHqcWblNR0kWn5eqHsg/8KseIRuG\n\t/kOHrMh8GZ83AYrsPo9MZid4JY0JZOWtTrteXsFG8dxk7jfm9qGHIDSwvsKFSaByG+3t\n\tnhEwOGwVso/PAu3+RsKOS+hTkUAw68S3zcaNuP2hUTdlYqHaMBKc8GbeLdf34/PtC1EL\n\tx7sUdGsbLvMN4RIJoANjR2c75PPW60pV2wBWUdytQoi3jYaNGV9FXjVHPwlxEcIoL46x\n\tG8iCPyyu0aF1EEE9nISdhVLkDY7O7N5hZ8Hv1uqnSonhRbjRXqyMGjPp6PjYmYO6OOJG\n\trvVA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:from:subject:to:cc:references:message-id:date\n\t:user-agent:mime-version:in-reply-to:content-language\n\t:content-transfer-encoding;\n\tbh=cEqo3iamGkSNeDtpcuFL77UvJ6AqmrnSxi85gTRSv2w=;\n\tb=dwA617NXJ6H6/RhZgcSbdMDcPruASqNbsjVo9r2QPooFQZIIBGrQIvPBzvAFkkF+6o\n\tMLyV0OkSlUXoN9Lx2ZImC2zycvh3tk6uclQhMeyYmatGdb8MqVELlPkIs4lO1zWKN+hJ\n\tyV/NsWvWD1kN96oBCznYbiIBhEnjF2xTuis8OcVOVxW6Cmd55N1tJ/5RZloFsTYchTXc\n\tHLYVhOrf3YBdVbgAE/Q3SuzAAh0AqYGPaUB+Ju9sIOi67YGgVAzZYimzw/qVufS6xsc9\n\tOk9j8/naVydyTKcIJmJbfwgtxAE7b5rezh8euvlyXTAq68AyPtdnE/btzFx3oCGcHwfu\n\twIWA==","X-Gm-Message-State":"AHYfb5g2UZLZ+xFkMk2GLyHc5exiRNcic7rQTMM5oPb+NTmZSwQNLqMd\n\tw6ExEQ2+us2cfMF3","X-Received":"by 10.84.210.3 with SMTP id z3mr14687641plh.114.1503288035171;\n\tSun, 20 Aug 2017 21:00:35 -0700 (PDT)","From":"Alexey Kardashevskiy ","Subject":"Re: [PATCH kernel] PCI: Disable IOV before pcibios_sriov_disable()","To":"Bjorn Helgaas ","Cc":"linuxppc-dev@lists.ozlabs.org, Bjorn Helgaas ,\n\tkvm@vger.kernel.org, Yongji Xie ,\n\tEric Auger ,\n\tlinux-kernel@vger.kernel.org, linux-pci@vger.kernel.org,\n\tshan.gavin@gmail.com, Benjamin Herrenschmidt ,\n\tPaul Mackerras ","References":"<20170811081933.17474-1-aik@ozlabs.ru>\n\t<2163ef83-3633-a8cf-3416-0e37a6c500db@ozlabs.ru>\n\t<20170818152740.GL28977@bhelgaas-glaptop.roam.corp.google.com>","Message-ID":"","Date":"Mon, 21 Aug 2017 14:00:28 +1000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.2.1","MIME-Version":"1.0","In-Reply-To":"<20170818152740.GL28977@bhelgaas-glaptop.roam.corp.google.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-AU","Content-Transfer-Encoding":"7bit","Sender":"linux-pci-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-pci@vger.kernel.org"}},{"id":1760414,"web_url":"http://patchwork.ozlabs.org/comment/1760414/","msgid":"<20170830190206.GU8154@bhelgaas-glaptop.roam.corp.google.com>","list_archive_url":null,"date":"2017-08-30T19:02:06","subject":"Re: [PATCH kernel] PCI: Disable IOV before pcibios_sriov_disable()","submitter":{"id":67298,"url":"http://patchwork.ozlabs.org/api/people/67298/","name":"Bjorn Helgaas","email":"helgaas@kernel.org"},"content":"On Fri, Aug 11, 2017 at 06:19:33PM +1000, Alexey Kardashevskiy wrote:\n> From: Gavin Shan \n> \n> The PowerNV platform is the only user of pcibios_sriov_disable().\n> The IOV BAR could be shifted by pci_iov_update_resource(). The\n> warning message in the function is printed if the IOV capability\n> is in enabled (PCI_SRIOV_CTRL_VFE && PCI_SRIOV_CTRL_MSE) state.\n> \n> This is the backtrace of what is happening:\n> pci_disable_sriov\n> sriov_disable\n> pnv_pci_sriov_disable\n> pnv_pci_vf_resource_shift\n> pci_update_resource\n> pci_iov_update_resource\n> \n> This fixes the issue by disabling IOV capability before calling\n> pcibios_sriov_disable(). With it, the disabling path matches\n> the enabling path: pcibios_sriov_enable() is called before the\n> IOV capability is enabled.\n> \n> Cc: shan.gavin@gmail.com\n> Cc: Benjamin Herrenschmidt \n> Cc: Paul Mackerras \n> Reported-by: Carol L Soto \n> Signed-off-by: Gavin Shan \n> Tested-by: Carol L Soto \n> Signed-off-by: Alexey Kardashevskiy \n> ---\n> \n> This is repost. Since Gavin left the team, I am trying to push it out.\n> The previos converstion is here: https://patchwork.ozlabs.org/patch/732653/\n\nI gave up on the previous issue. I think this patch makes sense as-is\nat least as far as the fact that we can't update a struct resource\nwhile the device is still consuming it. I reworked the changelog to\nemphasize that.\n\nI assume the fact that pci_iov_update_resource() dropped the resource\nupdate caused some user-visible issue later on, and I might mention\nthat, too, if I knew what it was.\n\nHere's what I would consider putting on pci/virtualization (the diff\nis unchanged from your post):\n\n\ncommit 08132e7759b3929bea0ccdf8afe81ebf05351389\nAuthor: Gavin Shan \nDate: Fri Aug 11 18:19:33 2017 +1000\n\n PCI: Disable VF decoding before updating resources in pcibios_sriov_disable()\n \n A struct resource represents the address space consumed by a device. We\n should not modify that resource while the device is actively using the\n address space. For VFs, pci_iov_update_resource() enforces this by\n printing a warning and doing nothing if the VFE (VF Enable) and MSE (VF\n Memory Space Enable) bits are set.\n \n Previously, both sriov_enable() and sriov_disable() called the\n pcibios_sriov_disable() arch hook, which may update the struct resource,\n while VFE and MSE were enabled. This effectively dropped the resource\n update pcibios_sriov_disable() intended to do.\n \n Disable VF memory decoding before calling pcibios_sriov_disable().\n \n Reported-by: Carol L Soto \n Tested-by: Carol L Soto \n Signed-off-by: Gavin Shan \n Signed-off-by: Alexey Kardashevskiy \n [bhelgaas: changelog]\n Signed-off-by: Bjorn Helgaas \n Cc: shan.gavin@gmail.com\n Cc: Benjamin Herrenschmidt \n Cc: Paul Mackerras \n\ndiff --git a/drivers/pci/iov.c b/drivers/pci/iov.c\nindex 120485d6f352..ac41c8be9200 100644\n--- a/drivers/pci/iov.c\n+++ b/drivers/pci/iov.c\n@@ -331,7 +331,6 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)\n \twhile (i--)\n \t\tpci_iov_remove_virtfn(dev, i, 0);\n \n-\tpcibios_sriov_disable(dev);\n err_pcibios:\n \tiov->ctrl &= ~(PCI_SRIOV_CTRL_VFE | PCI_SRIOV_CTRL_MSE);\n \tpci_cfg_access_lock(dev);\n@@ -339,6 +338,8 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)\n \tssleep(1);\n \tpci_cfg_access_unlock(dev);\n \n+\tpcibios_sriov_disable(dev);\n+\n \tif (iov->link != dev->devfn)\n \t\tsysfs_remove_link(&dev->dev.kobj, \"dep_link\");\n \n@@ -357,14 +358,14 @@ static void sriov_disable(struct pci_dev *dev)\n \tfor (i = 0; i < iov->num_VFs; i++)\n \t\tpci_iov_remove_virtfn(dev, i, 0);\n \n-\tpcibios_sriov_disable(dev);\n-\n \tiov->ctrl &= ~(PCI_SRIOV_CTRL_VFE | PCI_SRIOV_CTRL_MSE);\n \tpci_cfg_access_lock(dev);\n \tpci_write_config_word(dev, iov->pos + PCI_SRIOV_CTRL, iov->ctrl);\n \tssleep(1);\n \tpci_cfg_access_unlock(dev);\n \n+\tpcibios_sriov_disable(dev);\n+\n \tif (iov->link != dev->devfn)\n \t\tsysfs_remove_link(&dev->dev.kobj, \"dep_link\");","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=linux-pci-owner@vger.kernel.org;\n\treceiver=)","mail.kernel.org;\n\tdmarc=none (p=none dis=none) header.from=kernel.org","mail.kernel.org;\n\tspf=none smtp.mailfrom=helgaas@kernel.org"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xjFHj66Btz9t2S\n\tfor ;\n\tThu, 31 Aug 2017 05:02:25 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1750803AbdH3TCJ (ORCPT );\n\tWed, 30 Aug 2017 15:02:09 -0400","from mail.kernel.org ([198.145.29.99]:57850 \"EHLO mail.kernel.org\"\n\trhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP\n\tid S1750757AbdH3TCI (ORCPT );\n\tWed, 30 Aug 2017 15:02:08 -0400","from localhost (unknown [69.55.156.165])\n\t(using TLSv1.2 with cipher DHE-RSA-AES128-SHA (128/128 bits))\n\t(No client certificate requested)\n\tby mail.kernel.org (Postfix) with ESMTPSA id 333DB21A92;\n\tWed, 30 Aug 2017 19:02:07 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mail.kernel.org 333DB21A92","Date":"Wed, 30 Aug 2017 14:02:06 -0500","From":"Bjorn Helgaas ","To":"Alexey Kardashevskiy ","Cc":"linuxppc-dev@lists.ozlabs.org, kvm@vger.kernel.org,\n\tYongji Xie , Eric Auger ,\n\tBjorn Helgaas ,\n\tlinux-kernel@vger.kernel.org, linux-pci@vger.kernel.org,\n\tshan.gavin@gmail.com, Benjamin Herrenschmidt ,\n\tPaul Mackerras , Gavin Shan ","Subject":"Re: [PATCH kernel] PCI: Disable IOV before pcibios_sriov_disable()","Message-ID":"<20170830190206.GU8154@bhelgaas-glaptop.roam.corp.google.com>","References":"<20170811081933.17474-1-aik@ozlabs.ru>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20170811081933.17474-1-aik@ozlabs.ru>","User-Agent":"Mutt/1.5.21 (2010-09-15)","Sender":"linux-pci-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-pci@vger.kernel.org"}},{"id":1760607,"web_url":"http://patchwork.ozlabs.org/comment/1760607/","msgid":"<830677fa-cba8-1339-197e-07b5a0400144@ozlabs.ru>","list_archive_url":null,"date":"2017-08-31T03:08:31","subject":"Re: [PATCH kernel] PCI: Disable IOV before pcibios_sriov_disable()","submitter":{"id":7621,"url":"http://patchwork.ozlabs.org/api/people/7621/","name":"Alexey Kardashevskiy","email":"aik@ozlabs.ru"},"content":"On 31/08/17 05:02, Bjorn Helgaas wrote:\n> On Fri, Aug 11, 2017 at 06:19:33PM +1000, Alexey Kardashevskiy wrote:\n>> From: Gavin Shan \n>>\n>> The PowerNV platform is the only user of pcibios_sriov_disable().\n>> The IOV BAR could be shifted by pci_iov_update_resource(). The\n>> warning message in the function is printed if the IOV capability\n>> is in enabled (PCI_SRIOV_CTRL_VFE && PCI_SRIOV_CTRL_MSE) state.\n>>\n>> This is the backtrace of what is happening:\n>> pci_disable_sriov\n>> sriov_disable\n>> pnv_pci_sriov_disable\n>> pnv_pci_vf_resource_shift\n>> pci_update_resource\n>> pci_iov_update_resource\n>>\n>> This fixes the issue by disabling IOV capability before calling\n>> pcibios_sriov_disable(). With it, the disabling path matches\n>> the enabling path: pcibios_sriov_enable() is called before the\n>> IOV capability is enabled.\n>>\n>> Cc: shan.gavin@gmail.com\n>> Cc: Benjamin Herrenschmidt \n>> Cc: Paul Mackerras \n>> Reported-by: Carol L Soto \n>> Signed-off-by: Gavin Shan \n>> Tested-by: Carol L Soto \n>> Signed-off-by: Alexey Kardashevskiy \n>> ---\n>>\n>> This is repost. Since Gavin left the team, I am trying to push it out.\n>> The previos converstion is here: https://patchwork.ozlabs.org/patch/732653/\n> \n> I gave up on the previous issue. I think this patch makes sense as-is\n> at least as far as the fact that we can't update a struct resource\n> while the device is still consuming it. I reworked the changelog to\n> emphasize that.\n> \n> I assume the fact that pci_iov_update_resource() dropped the resource\n> update caused some user-visible issue later on, and I might mention\n> that, too, if I knew what it was.\n\nI could not identify any issue so far in my test setup - I recreated VFs\nseveral times, run some traffic through them on one of mellanox'es so the\nmessage+backtrace seems to be the only issue for now.\n\n\n> Here's what I would consider putting on pci/virtualization (the diff\n> is unchanged from your post):\n\n\nThis sounds good to me, thanks for updating the commit log.\n\nI'll still try and finish my homework with updating that comment about the\nhole in arch/powerpc/platforms/powernv/pci-ioda.c.\n\nCheers.\n\n> \n> \n> commit 08132e7759b3929bea0ccdf8afe81ebf05351389\n> Author: Gavin Shan \n> Date: Fri Aug 11 18:19:33 2017 +1000\n> \n> PCI: Disable VF decoding before updating resources in pcibios_sriov_disable()\n> \n> A struct resource represents the address space consumed by a device. We\n> should not modify that resource while the device is actively using the\n> address space. For VFs, pci_iov_update_resource() enforces this by\n> printing a warning and doing nothing if the VFE (VF Enable) and MSE (VF\n> Memory Space Enable) bits are set.\n> \n> Previously, both sriov_enable() and sriov_disable() called the\n> pcibios_sriov_disable() arch hook, which may update the struct resource,\n> while VFE and MSE were enabled. This effectively dropped the resource\n> update pcibios_sriov_disable() intended to do.\n> \n> Disable VF memory decoding before calling pcibios_sriov_disable().\n> \n> Reported-by: Carol L Soto \n> Tested-by: Carol L Soto \n> Signed-off-by: Gavin Shan \n> Signed-off-by: Alexey Kardashevskiy \n> [bhelgaas: changelog]\n> Signed-off-by: Bjorn Helgaas \n> Cc: shan.gavin@gmail.com\n> Cc: Benjamin Herrenschmidt \n> Cc: Paul Mackerras \n\n> \n> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c\n> index 120485d6f352..ac41c8be9200 100644\n> --- a/drivers/pci/iov.c\n> +++ b/drivers/pci/iov.c\n> @@ -331,7 +331,6 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)\n> \twhile (i--)\n> \t\tpci_iov_remove_virtfn(dev, i, 0);\n> \n> -\tpcibios_sriov_disable(dev);\n> err_pcibios:\n> \tiov->ctrl &= ~(PCI_SRIOV_CTRL_VFE | PCI_SRIOV_CTRL_MSE);\n> \tpci_cfg_access_lock(dev);\n> @@ -339,6 +338,8 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)\n> \tssleep(1);\n> \tpci_cfg_access_unlock(dev);\n> \n> +\tpcibios_sriov_disable(dev);\n> +\n> \tif (iov->link != dev->devfn)\n> \t\tsysfs_remove_link(&dev->dev.kobj, \"dep_link\");\n> \n> @@ -357,14 +358,14 @@ static void sriov_disable(struct pci_dev *dev)\n> \tfor (i = 0; i < iov->num_VFs; i++)\n> \t\tpci_iov_remove_virtfn(dev, i, 0);\n> \n> -\tpcibios_sriov_disable(dev);\n> -\n> \tiov->ctrl &= ~(PCI_SRIOV_CTRL_VFE | PCI_SRIOV_CTRL_MSE);\n> \tpci_cfg_access_lock(dev);\n> \tpci_write_config_word(dev, iov->pos + PCI_SRIOV_CTRL, iov->ctrl);\n> \tssleep(1);\n> \tpci_cfg_access_unlock(dev);\n> \n> +\tpcibios_sriov_disable(dev);\n> +\n> \tif (iov->link != dev->devfn)\n> \t\tsysfs_remove_link(&dev->dev.kobj, \"dep_link\");\n> \n>","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=linux-pci-owner@vger.kernel.org;\n\treceiver=)","ozlabs.org; dkim=pass (2048-bit key;\n\tunprotected) header.d=ozlabs-ru.20150623.gappssmtp.com\n\theader.i=@ozlabs-ru.20150623.gappssmtp.com\n\theader.b=\"FFJD4y0x\"; dkim-atps=neutral"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xjS543gQ8z9s83\n\tfor ;\n\tThu, 31 Aug 2017 13:08:56 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751127AbdHaDIk (ORCPT );\n\tWed, 30 Aug 2017 23:08:40 -0400","from mail-pf0-f195.google.com ([209.85.192.195]:38239 \"EHLO\n\tmail-pf0-f195.google.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1750968AbdHaDIi (ORCPT\n\t); Wed, 30 Aug 2017 23:08:38 -0400","by mail-pf0-f195.google.com with SMTP id r187so5421078pfr.5\n\tfor ; Wed, 30 Aug 2017 20:08:38 -0700 (PDT)","from [10.61.2.175] ([122.99.82.10])\n\tby smtp.googlemail.com with ESMTPSA id\n\to17sm13330835pge.41.2017.08.30.20.08.33\n\t(version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128);\n\tWed, 30 Aug 2017 20:08:37 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ozlabs-ru.20150623.gappssmtp.com; s=20150623;\n\th=subject:to:cc:references:from:message-id:date:user-agent\n\t:mime-version:in-reply-to:content-language:content-transfer-encoding; \n\tbh=CJ3CqoiPbbKggS9fl1skm9AC2E9VQJFomL4uyXgMRyo=;\n\tb=FFJD4y0xvp4oQfpibklfmrAIbsg/tcFPFyKtlMVTBRl5wJ/0YO64H90utpMVvGFaB5\n\tmE5An0b6qjdyhjaq3H9vmkc2fqFLKhMxmQlHEQTcwBb/vmaxCf+lEsIP5Gwb55ubeDZI\n\txJ4FeBZoU4HwN3uXbOD+T8hfsria0qrG9r+eGZiHS1aD1hpUrLaWhd630hmfDY6v7VmG\n\tNnNWsJpUPeCtgbdvQRFhOkTTGq+eS7vwGncNWByUbWXKOYootBPqf66CDL97qVDX2PcM\n\t4kdbwP7TiDNH7fzTAZk9j4vzjzcuWYQR2HMV5hT+F+ZZgaNR3KvqdnA8ee5VA2+F+B6J\n\tkB0A==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:subject:to:cc:references:from:message-id:date\n\t:user-agent:mime-version:in-reply-to:content-language\n\t:content-transfer-encoding;\n\tbh=CJ3CqoiPbbKggS9fl1skm9AC2E9VQJFomL4uyXgMRyo=;\n\tb=jnAQupN1BdW4WCundcabTSVW9ZBCoKomOwG/8oPvyS+r2OVAHukFzTp3ZYqZrGsAA3\n\tbVFTKiht9dlSqjWsOYoh5G6CQ99EN7ihcr2QBgSciQv+xlQAFxoJHidfbqN4IWly2vnJ\n\tmRtWM1YJa1MPJ4UrL2RsSo9kBxER1Dpx1+vJ/gGY8b4Yxvuq7WjleLu8EF+ElTaJu+Zs\n\t5x2xJOGuVngPAA/Q6EHG1jJ6ygmsWXtSOblG68h+e9Qir+h6X4jQT9Gkm/46/p08DQyV\n\tOTDDOeKGYlFGKQgJ7huINgn17MjpANOjK0Z8aUYE1nmuHOtfTOjWauy9ydLXVMKJyCt/\n\t5Oaw==","X-Gm-Message-State":"AHYfb5ioA5VmjA6a8a7/aKHEChBRtP/5HxE65cP32L/Fmt7r3RneBvYX\n\tJgaqxCvphj/bVYuy","X-Received":"by 10.99.122.69 with SMTP id j5mr859265pgn.12.1504148918181;\n\tWed, 30 Aug 2017 20:08:38 -0700 (PDT)","Subject":"Re: [PATCH kernel] PCI: Disable IOV before pcibios_sriov_disable()","To":"Bjorn Helgaas ","Cc":"linuxppc-dev@lists.ozlabs.org, kvm@vger.kernel.org,\n\tYongji Xie , Eric Auger ,\n\tBjorn Helgaas ,\n\tlinux-kernel@vger.kernel.org, linux-pci@vger.kernel.org,\n\tshan.gavin@gmail.com, Benjamin Herrenschmidt ,\n\tPaul Mackerras , Gavin Shan ","References":"<20170811081933.17474-1-aik@ozlabs.ru>\n\t<20170830190206.GU8154@bhelgaas-glaptop.roam.corp.google.com>","From":"Alexey Kardashevskiy ","Message-ID":"<830677fa-cba8-1339-197e-07b5a0400144@ozlabs.ru>","Date":"Thu, 31 Aug 2017 13:08:31 +1000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.2.1","MIME-Version":"1.0","In-Reply-To":"<20170830190206.GU8154@bhelgaas-glaptop.roam.corp.google.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-AU","Content-Transfer-Encoding":"7bit","Sender":"linux-pci-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-pci@vger.kernel.org"}},{"id":1761199,"web_url":"http://patchwork.ozlabs.org/comment/1761199/","msgid":"<20170831170410.GE18250@bhelgaas-glaptop.roam.corp.google.com>","list_archive_url":null,"date":"2017-08-31T17:04:10","subject":"Re: [PATCH kernel] PCI: Disable IOV before pcibios_sriov_disable()","submitter":{"id":67298,"url":"http://patchwork.ozlabs.org/api/people/67298/","name":"Bjorn Helgaas","email":"helgaas@kernel.org"},"content":"On Wed, Aug 30, 2017 at 02:02:06PM -0500, Bjorn Helgaas wrote:\n> On Fri, Aug 11, 2017 at 06:19:33PM +1000, Alexey Kardashevskiy wrote:\n> > From: Gavin Shan \n> > \n> > The PowerNV platform is the only user of pcibios_sriov_disable().\n> > The IOV BAR could be shifted by pci_iov_update_resource(). The\n> > warning message in the function is printed if the IOV capability\n> > is in enabled (PCI_SRIOV_CTRL_VFE && PCI_SRIOV_CTRL_MSE) state.\n> > \n> > This is the backtrace of what is happening:\n> > pci_disable_sriov\n> > sriov_disable\n> > pnv_pci_sriov_disable\n> > pnv_pci_vf_resource_shift\n> > pci_update_resource\n> > pci_iov_update_resource\n> > \n> > This fixes the issue by disabling IOV capability before calling\n> > pcibios_sriov_disable(). With it, the disabling path matches\n> > the enabling path: pcibios_sriov_enable() is called before the\n> > IOV capability is enabled.\n> > \n> > Cc: shan.gavin@gmail.com\n> > Cc: Benjamin Herrenschmidt \n> > Cc: Paul Mackerras \n> > Reported-by: Carol L Soto \n> > Signed-off-by: Gavin Shan \n> > Tested-by: Carol L Soto \n> > Signed-off-by: Alexey Kardashevskiy \n> > ---\n> > \n> > This is repost. Since Gavin left the team, I am trying to push it out.\n> > The previos converstion is here: https://patchwork.ozlabs.org/patch/732653/\n> \n> I gave up on the previous issue. I think this patch makes sense as-is\n> at least as far as the fact that we can't update a struct resource\n> while the device is still consuming it. I reworked the changelog to\n> emphasize that.\n> \n> I assume the fact that pci_iov_update_resource() dropped the resource\n> update caused some user-visible issue later on, and I might mention\n> that, too, if I knew what it was.\n> \n> Here's what I would consider putting on pci/virtualization (the diff\n> is unchanged from your post):\n\nI applied the patch below on pci/virtualization for v4.14.\n\n> commit 08132e7759b3929bea0ccdf8afe81ebf05351389\n> Author: Gavin Shan \n> Date: Fri Aug 11 18:19:33 2017 +1000\n> \n> PCI: Disable VF decoding before updating resources in pcibios_sriov_disable()\n> \n> A struct resource represents the address space consumed by a device. We\n> should not modify that resource while the device is actively using the\n> address space. For VFs, pci_iov_update_resource() enforces this by\n> printing a warning and doing nothing if the VFE (VF Enable) and MSE (VF\n> Memory Space Enable) bits are set.\n> \n> Previously, both sriov_enable() and sriov_disable() called the\n> pcibios_sriov_disable() arch hook, which may update the struct resource,\n> while VFE and MSE were enabled. This effectively dropped the resource\n> update pcibios_sriov_disable() intended to do.\n> \n> Disable VF memory decoding before calling pcibios_sriov_disable().\n> \n> Reported-by: Carol L Soto \n> Tested-by: Carol L Soto \n> Signed-off-by: Gavin Shan \n> Signed-off-by: Alexey Kardashevskiy \n> [bhelgaas: changelog]\n> Signed-off-by: Bjorn Helgaas \n> Cc: shan.gavin@gmail.com\n> Cc: Benjamin Herrenschmidt \n> Cc: Paul Mackerras \n> \n> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c\n> index 120485d6f352..ac41c8be9200 100644\n> --- a/drivers/pci/iov.c\n> +++ b/drivers/pci/iov.c\n> @@ -331,7 +331,6 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)\n> \twhile (i--)\n> \t\tpci_iov_remove_virtfn(dev, i, 0);\n> \n> -\tpcibios_sriov_disable(dev);\n> err_pcibios:\n> \tiov->ctrl &= ~(PCI_SRIOV_CTRL_VFE | PCI_SRIOV_CTRL_MSE);\n> \tpci_cfg_access_lock(dev);\n> @@ -339,6 +338,8 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)\n> \tssleep(1);\n> \tpci_cfg_access_unlock(dev);\n> \n> +\tpcibios_sriov_disable(dev);\n> +\n> \tif (iov->link != dev->devfn)\n> \t\tsysfs_remove_link(&dev->dev.kobj, \"dep_link\");\n> \n> @@ -357,14 +358,14 @@ static void sriov_disable(struct pci_dev *dev)\n> \tfor (i = 0; i < iov->num_VFs; i++)\n> \t\tpci_iov_remove_virtfn(dev, i, 0);\n> \n> -\tpcibios_sriov_disable(dev);\n> -\n> \tiov->ctrl &= ~(PCI_SRIOV_CTRL_VFE | PCI_SRIOV_CTRL_MSE);\n> \tpci_cfg_access_lock(dev);\n> \tpci_write_config_word(dev, iov->pos + PCI_SRIOV_CTRL, iov->ctrl);\n> \tssleep(1);\n> \tpci_cfg_access_unlock(dev);\n> \n> +\tpcibios_sriov_disable(dev);\n> +\n> \tif (iov->link != dev->devfn)\n> \t\tsysfs_remove_link(&dev->dev.kobj, \"dep_link\");\n>","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=linux-pci-owner@vger.kernel.org;\n\treceiver=)","mail.kernel.org;\n\tdmarc=none (p=none dis=none) header.from=kernel.org","mail.kernel.org;\n\tspf=none smtp.mailfrom=helgaas@kernel.org"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xjpcx3V24z9sD5\n\tfor ;\n\tFri, 1 Sep 2017 03:04:17 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751948AbdHaREP (ORCPT );\n\tThu, 31 Aug 2017 13:04:15 -0400","from mail.kernel.org ([198.145.29.99]:37388 \"EHLO mail.kernel.org\"\n\trhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP\n\tid S1750813AbdHaREN (ORCPT );\n\tThu, 31 Aug 2017 13:04:13 -0400","from localhost (unknown [69.55.156.165])\n\t(using TLSv1.2 with cipher DHE-RSA-AES128-SHA (128/128 bits))\n\t(No client certificate requested)\n\tby mail.kernel.org (Postfix) with ESMTPSA id 5FD082199C;\n\tThu, 31 Aug 2017 17:04:12 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mail.kernel.org 5FD082199C","Date":"Thu, 31 Aug 2017 12:04:10 -0500","From":"Bjorn Helgaas ","To":"Alexey Kardashevskiy ","Cc":"linuxppc-dev@lists.ozlabs.org, kvm@vger.kernel.org,\n\tYongji Xie , Eric Auger ,\n\tBjorn Helgaas ,\n\tlinux-kernel@vger.kernel.org, linux-pci@vger.kernel.org,\n\tshan.gavin@gmail.com, Benjamin Herrenschmidt ,\n\tPaul Mackerras , Gavin Shan ","Subject":"Re: [PATCH kernel] PCI: Disable IOV before pcibios_sriov_disable()","Message-ID":"<20170831170410.GE18250@bhelgaas-glaptop.roam.corp.google.com>","References":"<20170811081933.17474-1-aik@ozlabs.ru>\n\t<20170830190206.GU8154@bhelgaas-glaptop.roam.corp.google.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20170830190206.GU8154@bhelgaas-glaptop.roam.corp.google.com>","User-Agent":"Mutt/1.5.21 (2010-09-15)","Sender":"linux-pci-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-pci@vger.kernel.org"}}]