[Skiboot-stable] [PATCH] pci/iov: Remove skiboot VF tracking

Sergey Miroshnichenko s.miroshnichenko at yadro.com
Wed May 15 00:44:24 AEST 2019


On 5/7/19 1:36 PM, Oliver wrote:
> On Wed, May 1, 2019 at 6:30 PM Oliver O'Halloran <oohall at gmail.com> wrote:
>>
>> This feature was added a few years ago in response to a request to make
>> the MaxPayloadSize (MPS) field of a Virtual Function match the MPS of the
>> Physical Function that hosts it.
>>
>> The SR-IOV specification states the the MPS field of the VF is "ResvP".
>> This indicates the VF will use whatever MPS is configured on the PF and
>> that the field should be treated as a reserved field in the config space
>> of the VF. In other words, a SR-IOV spec compliant VF should always return
>> zero in the MPS field.  Adding hacks in OPAL to make it non-zero is...
>> misguided at best.
>>
>> Additionally, there is a bug in the way pci_device structures are handled
>> by VFs that results in a crash on fast-reboot that occurs if VFs are
>> enabled and then disabled prior to rebooting. This patch fixes the bug by
>> removing the code entirely. This patch has no impact on SR-IOV support on
>> the host operating system.
>>
>> Cc: Sergey Miroshnichenko <s.miroshnichenko at yadro.com>
>> Cc: skiboot-stable at lists.ozlabs.org
>> Signed-off-by: Oliver O'Halloran <oohall at gmail.com>
> 
> The test team smoke tested it and it came back clean. Merged to master
> as of 22057f868f3b2b1fd02647a738f6da0858b5eb6c
> 

Works fine on our setup as well (platforms/vesnin).

And it lets me gladly drop two patches from the "core/pci: Sync VFs and the changes of
bdfns between the firmware and the OS" patchset.

Serge

> Vasant, you can pick this up for stable now if you like.
> 
>> ---
>>  core/Makefile.inc |   2 +-
>>  core/pci-iov.c    | 265 ----------------------------------------------
>>  core/pci.c        |   2 -
>>  include/pci-iov.h |  37 -------
>>  4 files changed, 1 insertion(+), 305 deletions(-)
>>  delete mode 100644 core/pci-iov.c
>>  delete mode 100644 include/pci-iov.h
>>
>> diff --git a/core/Makefile.inc b/core/Makefile.inc
>> index 3b4387081a62..21c12fb8db31 100644
>> --- a/core/Makefile.inc
>> +++ b/core/Makefile.inc
>> @@ -3,7 +3,7 @@
>>  SUBDIRS += core
>>  CORE_OBJS = relocate.o console.o stack.o init.o chip.o mem_region.o
>>  CORE_OBJS += malloc.o lock.o cpu.o utils.o fdt.o opal.o interrupts.o timebase.o
>> -CORE_OBJS += opal-msg.o pci.o pci-iov.o pci-virt.o pci-slot.o pcie-slot.o
>> +CORE_OBJS += opal-msg.o pci.o pci-virt.o pci-slot.o pcie-slot.o
>>  CORE_OBJS += pci-opal.o fast-reboot.o device.o exceptions.o trace.o affinity.o
>>  CORE_OBJS += vpd.o hostservices.o platform.o nvram.o nvram-format.o hmi.o
>>  CORE_OBJS += console-log.o ipmi.o time-utils.o pel.o pool.o errorlog.o
>> diff --git a/core/pci-iov.c b/core/pci-iov.c
>> deleted file mode 100644
>> index 8fecebc1292a..000000000000
>> --- a/core/pci-iov.c
>> +++ /dev/null
>> @@ -1,265 +0,0 @@
>> -/* Copyright 2013-2016 IBM Corp.
>> - *
>> - * Licensed under the Apache License, Version 2.0 (the "License");
>> - * you may not use this file except in compliance with the License.
>> - * You may obtain a copy of the License at
>> - *
>> - *      http://www.apache.org/licenses/LICENSE-2.0
>> - *
>> - * Unless required by applicable law or agreed to in writing, software
>> - * distributed under the License is distributed on an "AS IS" BASIS,
>> - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
>> - * implied.
>> - * See the License for the specific language governing permissions and
>> - * limitations under the License.
>> - */
>> -
>> -#include <skiboot.h>
>> -#include <pci.h>
>> -#include <pci-cfg.h>
>> -#include <pci-slot.h>
>> -#include <pci-iov.h>
>> -
>> -/*
>> - * Tackle the VF's MPS in PCIe capability. The field is read only.
>> - * This function caches what is written and returns the cached
>> - * MPS on read.
>> - */
>> -static int64_t pci_iov_vf_devctl(void *dev, struct pci_cfg_reg_filter *pcrf,
>> -                                uint32_t offset, uint32_t len,
>> -                                uint32_t *data, bool write)
>> -{
>> -       struct pci_device *vf = (struct pci_device *)dev;
>> -       uint32_t pos = pci_cap(vf, PCI_CFG_CAP_ID_EXP, false);
>> -       uint8_t *pcache;
>> -
>> -       if (offset != (pos + PCICAP_EXP_DEVCTL))
>> -               return OPAL_PARTIAL;
>> -
>> -       pcache = &pcrf->data[0];
>> -       if (write) {
>> -               *pcache = ((uint8_t)(*data >> (8 * (4 - len)))) &
>> -                          PCICAP_EXP_DEVCTL_MPS;
>> -       } else {
>> -               *data &= ~(PCICAP_EXP_DEVCTL_MPS << (8 * (4 - len)));
>> -               *data |= (((uint32_t)(*pcache & PCICAP_EXP_DEVCTL_MPS))
>> -                         << (8 * (4 - len)));
>> -       }
>> -
>> -       return OPAL_SUCCESS;
>> -}
>> -
>> -static void pci_iov_vf_quirk(struct phb *phb, struct pci_device *vf)
>> -{
>> -       struct pci_cfg_reg_filter *pcrf;
>> -       uint32_t pos;
>> -
>> -       if (!pci_has_cap(vf, PCI_CFG_CAP_ID_EXP, false))
>> -               return;
>> -
>> -       /*
>> -        * On Mellanox MT27500 Family [ConnectX-3], its VF's MPS field in
>> -        * the corresponding config register is readonly. The MPS for PF/VF
>> -        * are usually different. We are introducing a quirk to make them
>> -        * look same to avoid confusion.
>> -        */
>> -       if (vf->vdid != 0x100315b3)
>> -               return;
>> -
>> -       pos = pci_cap(vf, PCI_CFG_CAP_ID_EXP, false);
>> -       pcrf = pci_add_cfg_reg_filter(vf, pos + PCICAP_EXP_DEVCTL, 4,
>> -                                     PCI_REG_FLAG_MASK, pci_iov_vf_devctl);
>> -       if (!pcrf)
>> -               prlog(PR_WARNING, "%s: Missed DEVCTL filter on %04x:%02x:%02x.%01x\n",
>> -                     __func__, phb->opal_id, (vf->bdfn >> 8),
>> -                     ((vf->bdfn >> 3) & 0x1f), (vf->bdfn & 0x7));
>> -}
>> -
>> -/*
>> - * Update the SRIOV parameters that change when the number of
>> - * VFs is configured.
>> - */
>> -static bool pci_iov_update_parameters(struct pci_iov *iov)
>> -{
>> -       struct phb *phb = iov->phb;
>> -       uint16_t bdfn = iov->pd->bdfn;
>> -       uint32_t pos = iov->pos;
>> -       uint16_t val;
>> -       bool enabled;
>> -
>> -       pci_cfg_read16(phb, bdfn, pos + PCIECAP_SRIOV_CTRL, &val);
>> -       enabled = !!(val & PCIECAP_SRIOV_CTRL_VFE);
>> -       if (iov->enabled == enabled)
>> -               return false;
>> -
>> -       if (enabled) {
>> -               pci_cfg_read16(phb, bdfn, pos + PCIECAP_SRIOV_INITIAL_VF,
>> -                              &iov->init_VFs);
>> -               pci_cfg_read16(phb, bdfn, pos + PCIECAP_SRIOV_NUM_VF,
>> -                              &iov->num_VFs);
>> -               pci_cfg_read16(phb, bdfn, pos + PCIECAP_SRIOV_VF_OFFSET,
>> -                              &iov->offset);
>> -               pci_cfg_read16(phb, bdfn, pos + PCIECAP_SRIOV_VF_STRIDE,
>> -                              &iov->stride);
>> -       } else {
>> -               iov->init_VFs   = 0;
>> -               iov->num_VFs    = 0;
>> -               iov->offset     = 0;
>> -               iov->stride     = 0;
>> -       }
>> -
>> -       iov->enabled = enabled;
>> -       return true;
>> -}
>> -
>> -static int64_t pci_iov_change(void *dev __unused,
>> -                             struct pci_cfg_reg_filter *pcrf,
>> -                             uint32_t offset __unused,
>> -                             uint32_t len __unused,
>> -                             uint32_t *data __unused,
>> -                             bool write __unused)
>> -{
>> -       struct pci_iov *iov = (struct pci_iov *)pcrf->data;
>> -       struct phb *phb = iov->phb;
>> -       struct pci_device *pd = iov->pd;
>> -       struct pci_device *vf, *tmp;
>> -       uint32_t i;
>> -       bool changed;
>> -
>> -       /* Update SRIOV variable parameters */
>> -       changed = pci_iov_update_parameters(iov);
>> -       if (!changed)
>> -               return OPAL_PARTIAL;
>> -
>> -       /* Remove all VFs that have been attached to the parent */
>> -       if (!iov->enabled) {
>> -               list_for_each_safe(&pd->children, vf, tmp, link)
>> -                       list_del(&vf->link);
>> -               return OPAL_PARTIAL;
>> -       }
>> -
>> -       /* Initialize the VFs and attach them to parent */
>> -       for (changed = false, i = 0; i < iov->num_VFs; i++) {
>> -               vf = &iov->VFs[i];
>> -               vf->bdfn = pd->bdfn + iov->offset + iov->stride * i;
>> -               list_add_tail(&pd->children, &vf->link);
>> -
>> -               /*
>> -                * We don't populate the capabilities again if they have
>> -                * been existing, to save time. Also, we need delay for
>> -                * 100ms before the VF's config space becomes ready.
>> -                */
>> -               if (!pci_has_cap(vf, PCI_CFG_CAP_ID_EXP, false)) {
>> -                       if (!changed) {
>> -                               changed = !changed;
>> -                               time_wait_ms(100);
>> -                       }
>> -
>> -                       pci_init_capabilities(phb, vf);
>> -                       pci_iov_vf_quirk(phb, vf);
>> -               }
>> -
>> -               /* Call PHB hook */
>> -               if (phb->ops->device_init)
>> -                       phb->ops->device_init(phb, pd, NULL);
>> -       }
>> -
>> -       return OPAL_PARTIAL;
>> -}
>> -
>> -/*
>> - * This function is called with disabled SRIOV capability. So the VF's
>> - * config address isn't finalized and its config space isn't accessible.
>> - */
>> -static void pci_iov_init_VF(struct pci_device *pd, struct pci_device *vf)
>> -{
>> -       vf->is_bridge           = false;
>> -       vf->is_multifunction    = false;
>> -       vf->is_vf               = true;
>> -       vf->dev_type            = PCIE_TYPE_ENDPOINT;
>> -       vf->scan_map            = -1;
>> -       vf->vdid                = pd->vdid;
>> -       vf->sub_vdid            = pd->sub_vdid;
>> -       vf->class               = pd->class;
>> -       vf->dn                  = NULL;
>> -       vf->slot                = NULL;
>> -       vf->parent              = pd;
>> -       vf->phb                 = pd->phb;
>> -       list_head_init(&vf->pcrf);
>> -       list_head_init(&vf->children);
>> -}
>> -
>> -static void pci_free_iov_cap(void *data)
>> -{
>> -       struct pci_iov *iov = data;
>> -       free(iov->VFs);
>> -       free(iov);
>> -}
>> -
>> -void pci_init_iov_cap(struct phb *phb, struct pci_device *pd)
>> -{
>> -       int64_t pos;
>> -       struct pci_iov *iov;
>> -       struct pci_cfg_reg_filter *pcrf;
>> -       uint32_t i;
>> -
>> -       /* Search for SRIOV capability */
>> -       if (!pci_has_cap(pd, PCI_CFG_CAP_ID_EXP, false))
>> -               return;
>> -
>> -       pos = pci_find_ecap(phb, pd->bdfn, PCIECAP_ID_SRIOV, NULL);
>> -       if (pos <= 0)
>> -               return;
>> -
>> -       /* Allocate IOV */
>> -       iov = zalloc(sizeof(*iov));
>> -       if (!iov) {
>> -               prlog(PR_ERR, "%s: Cannot alloc IOV for %04x:%02x:%02x.%01x\n",
>> -                     __func__, phb->opal_id, (pd->bdfn >> 8),
>> -                     ((pd->bdfn >> 3) & 0x1f), (pd->bdfn & 0x7));
>> -               return;
>> -       }
>> -
>> -       /* Allocate VFs */
>> -       pci_cfg_read16(phb, pd->bdfn, pos + PCIECAP_SRIOV_TOTAL_VF,
>> -                      &iov->total_VFs);
>> -       iov->VFs = zalloc(sizeof(*iov->VFs) * iov->total_VFs);
>> -       if (!iov->VFs) {
>> -               prlog(PR_ERR, "%s: Cannot alloc %d VFs for %04x:%02x:%02x.%01x\n",
>> -                     __func__, iov->total_VFs, phb->opal_id,
>> -                     (pd->bdfn >> 8), ((pd->bdfn >> 3) & 0x1f),
>> -                     (pd->bdfn & 0x7));
>> -               free(iov);
>> -               return;
>> -       }
>> -
>> -       /* Initialize VFs */
>> -       for (i = 0; i < iov->total_VFs; i++)
>> -               pci_iov_init_VF(pd, &iov->VFs[i]);
>> -
>> -       /* Register filter for enabling or disabling SRIOV capability */
>> -       pcrf = pci_add_cfg_reg_filter(pd, pos + PCIECAP_SRIOV_CTRL, 2,
>> -                                     PCI_REG_FLAG_WRITE, pci_iov_change);
>> -       if (!pcrf) {
>> -               prlog(PR_ERR, "%s: Cannot set filter on %04x:%02x:%02x.%01x\n",
>> -                     __func__, phb->opal_id, (pd->bdfn >> 8),
>> -                     ((pd->bdfn >> 3) & 0x1f), (pd->bdfn & 0x7));
>> -               free(iov->VFs);
>> -               free(iov);
>> -               return;
>> -       }
>> -
>> -       /* Associate filter and IOV capability */
>> -       pcrf->data = (void *)iov;
>> -
>> -       /*
>> -        * Retrieve the number of VFs and other information if applicable.
>> -        * Register the SRIOV capability in the mean while.
>> -        */
>> -       iov->phb = phb;
>> -       iov->pd = pd;
>> -       iov->pos = pos;
>> -       iov->enabled = false;
>> -       pci_iov_update_parameters(iov);
>> -       pci_set_cap(pd, PCIECAP_ID_SRIOV, pos, iov, pci_free_iov_cap, true);
>> -}
>> diff --git a/core/pci.c b/core/pci.c
>> index ee563c2fc577..02f5caa8d901 100644
>> --- a/core/pci.c
>> +++ b/core/pci.c
>> @@ -18,7 +18,6 @@
>>  #include <cpu.h>
>>  #include <pci.h>
>>  #include <pci-cfg.h>
>> -#include <pci-iov.h>
>>  #include <pci-slot.h>
>>  #include <pci-quirk.h>
>>  #include <timebase.h>
>> @@ -193,7 +192,6 @@ void pci_init_capabilities(struct phb *phb, struct pci_device *pd)
>>  {
>>         pci_init_pcie_cap(phb, pd);
>>         pci_init_aer_cap(phb, pd);
>> -       pci_init_iov_cap(phb, pd);
>>         pci_init_pm_cap(phb, pd);
>>  }
>>
>> diff --git a/include/pci-iov.h b/include/pci-iov.h
>> deleted file mode 100644
>> index 787b2cd1da69..000000000000
>> --- a/include/pci-iov.h
>> +++ /dev/null
>> @@ -1,37 +0,0 @@
>> -/* Copyright 2013-2016 IBM Corp.
>> - *
>> - * Licensed under the Apache License, Version 2.0 (the "License");
>> - * you may not use this file except in compliance with the License.
>> - * You may obtain a copy of the License at
>> - *
>> - *      http://www.apache.org/licenses/LICENSE-2.0
>> - *
>> - * Unless required by applicable law or agreed to in writing, software
>> - * distributed under the License is distributed on an "AS IS" BASIS,
>> - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
>> - * implied.
>> - * See the License for the specific language governing permissions and
>> - * limitations under the License.
>> - */
>> -
>> -#ifndef __PCI_IOV_H
>> -#define __PCI_IOV_H
>> -
>> -struct pci_iov {
>> -       struct phb                      *phb;
>> -       struct pci_device               *pd;
>> -       struct pci_device               *VFs;
>> -       uint32_t                        pos;
>> -       bool                            enabled;
>> -       struct pci_cfg_reg_filter       pcrf;
>> -
>> -       uint16_t                        init_VFs;
>> -       uint16_t                        total_VFs;
>> -       uint16_t                        num_VFs;
>> -       uint16_t                        offset;
>> -       uint16_t                        stride;
>> -};
>> -
>> -extern void pci_init_iov_cap(struct phb *phb, struct pci_device *pd);
>> -
>> -#endif
>> --
>> 2.20.1
>>


More information about the Skiboot-stable mailing list