Re: [PATCH v7 5/6] hw/pci: ensure PCIE devices are plugged into only slot 0 of PCIE port

[Igor Mammedov <imammedo@redhat.com>]

On Tue, 4 Jul 2023 19:20:00 +0530
Ani Sinha <anisinha@redhat.com> wrote:

> > On 04-Jul-2023, at 6:18 PM, Igor Mammedov <imammedo@redhat.com> wrote:
> > 
> > On Tue, 4 Jul 2023 21:02:09 +0900
> > Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
> >   
> >> On 2023/07/04 20:59, Ani Sinha wrote:  
> >>> 
> >>>   
> >>>> On 04-Jul-2023, at 5:24 PM, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
> >>>> 
> >>>> On 2023/07/04 20:25, Ani Sinha wrote:    
> >>>>> PCI Express ports only have one slot, so PCI Express devices can only be
> >>>>> plugged into slot 0 on a PCIE port. Add a warning to let users know when the
> >>>>> invalid configuration is used. We may enforce this more strongly later on once
> >>>>> we get more clarity on whether we are introducing a bad regression for users
> >>>>> currenly using the wrong configuration.
> >>>>> The change has been tested to not break or alter behaviors of ARI capable
> >>>>> devices by instantiating seven vfs on an emulated igb device (the maximum
> >>>>> number of vfs the linux igb driver supports). The vfs instantiated correctly
> >>>>> and are seen to have non-zero device/slot numbers in the conventional PCI BDF
> >>>>> representation.
> >>>>> CC: jusual@redhat.com
> >>>>> CC: imammedo@redhat.com
> >>>>> CC: mst@redhat.com
> >>>>> CC: akihiko.odaki@daynix.com
> >>>>> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2128929
> >>>>> Signed-off-by: Ani Sinha <anisinha@redhat.com>
> >>>>> Reviewed-by: Julia Suvorova <jusual@redhat.com>
> >>>>> ---
> >>>>>  hw/pci/pci.c | 15 +++++++++++++++
> >>>>>  1 file changed, 15 insertions(+)
> >>>>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> >>>>> index e2eb4c3b4a..47517ba3db 100644
> >>>>> --- a/hw/pci/pci.c
> >>>>> +++ b/hw/pci/pci.c
> >>>>> @@ -65,6 +65,7 @@ bool pci_available = true;
> >>>>>  static char *pcibus_get_dev_path(DeviceState *dev);
> >>>>>  static char *pcibus_get_fw_dev_path(DeviceState *dev);
> >>>>>  static void pcibus_reset(BusState *qbus);
> >>>>> +static bool pcie_has_upstream_port(PCIDevice *dev);
> >>>>>    static Property pci_props[] = {
> >>>>>      DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1),
> >>>>> @@ -2121,6 +2122,20 @@ static void pci_qdev_realize(DeviceState *qdev, Error **errp)
> >>>>>          }
> >>>>>      }
> >>>>>  +    /*
> >>>>> +     * With SRIOV and ARI, vfs can have non-zero slot in the conventional
> >>>>> +     * PCI interpretation as all five bits reserved for slot addresses are
> >>>>> +     * also used for function bits for the various vfs. Ignore that case.    
> >>>> 
> >>>> You don't have to mention SR/IOV; it affects all ARI-capable devices. A PF can also have non-zero slot number in the conventional interpretation so you shouldn't call it vf either.    
> >>> 
> >>> Can you please help write a comment that explains this properly for all cases - ARI/non-ARI, PFs and VFs? Once everyone agrees that its clear and correct, I will re-spin.    
> >> 
> >> Simply, you can say:
> >> With ARI, the slot number field in the conventional PCI interpretation 
> >> can have a non-zero value as the field bits are reused to extend the 
> >> function number bits. Ignore that case.  
> > 
> > mentioning 'conventional PCI interpretation' in comment and then immediately
> > checking 'pci_is_express(pci_dev)' is confusing. Since comment belongs
> > only to PCIE branch it would be better to talk in only about PCIe stuff
> > and referring to relevant portions of spec.  
> 
> Ok so how about this?
> 
>    * With ARI, devices can have non-zero slot in the traditional BDF                                                                                  
>      * representation as all five bits reserved for slot addresses are                                                                                  
>      * also used for function bits. Ignore that case.  

you still refer to traditional (which I misread as 'conventional'),
steal the linux comment and argument it with ARI if necessary,
something like this (probably needs some more massaging):


         /*                                                                       
         * A PCIe Downstream Port normally leads to a Link with only Device      
         * 0 on it (PCIe spec r3.1, sec 7.3.1). 
          However PCI_SLOT() is broken if ARI is enabled, hence work around it
          by skipping check if the later cap is present.                                  
         */
                     
> 
> 
> > (for example see how it's done in kernel code: only_one_child(...)
> > 
> > PS:
> > kernel can be forced  to scan for !0 device numbers, but that's rather
> > a hack, so we shouldn't really care about that.
> >   
> >>   
> >>>   
> >>>>   
> >>>>> +     */
> >>>>> +    if (pci_is_express(pci_dev) &&
> >>>>> +        !pcie_find_capability(pci_dev, PCI_EXT_CAP_ID_ARI) &&
> >>>>> +        pcie_has_upstream_port(pci_dev) &&
> >>>>> +        PCI_SLOT(pci_dev->devfn)) {
> >>>>> +        warn_report("PCI: slot %d is not valid for %s,"
> >>>>> +                    " parent device only allows plugging into slot 0.",
> >>>>> +                    PCI_SLOT(pci_dev->devfn), pci_dev->name);
> >>>>> +    }
> >>>>> +
> >>>>>      if (pci_dev->failover_pair_id) {
> >>>>>          if (!pci_bus_is_express(pci_get_bus(pci_dev))) {
> >>>>>              error_setg(errp, "failover primary device must be on "    
>