Re: [PATCH v3 01/15] pcie: Add support for Single Root I/O Virtualization (SR/IOV)

["Łukasz Gieryk" <lukasz.gieryk@linux.intel.com>]

I'm sorry for the delayed response. We (I and the other Lukasz) somehow
had hoped that Knut, the original author of this patch, would have
responded.

Let me address your questions, up to my best knowledge.
  
> > -static pcibus_t pci_bar_address(PCIDevice *d,
> > -                                int reg, uint8_t type, pcibus_t size)
> > +static pcibus_t pci_config_get_bar_addr(PCIDevice *d, int reg,
> > +                                        uint8_t type, pcibus_t size)
> > +{
> > +    pcibus_t new_addr;
> > +    if (!pci_is_vf(d)) {
> > +        int bar = pci_bar(d, reg);
> > +        if (type & PCI_BASE_ADDRESS_MEM_TYPE_64) {
> > +            new_addr = pci_get_quad(d->config + bar);
> > +        } else {
> > +            new_addr = pci_get_long(d->config + bar);
> > +        }
> > +    } else {
> > +        PCIDevice *pf = d->exp.sriov_vf.pf;
> > +        uint16_t sriov_cap = pf->exp.sriov_cap;
> > +        int bar = sriov_cap + PCI_SRIOV_BAR + reg * 4;
> > +        uint16_t vf_offset = pci_get_word(pf->config + sriov_cap + PCI_SRIOV_VF_OFFSET);
> > +        uint16_t vf_stride = pci_get_word(pf->config + sriov_cap + PCI_SRIOV_VF_STRIDE);
> > +        uint32_t vf_num = (d->devfn - (pf->devfn + vf_offset)) / vf_stride;
> > +
> > +        if (type & PCI_BASE_ADDRESS_MEM_TYPE_64) {
> > +            new_addr = pci_get_quad(pf->config + bar);
> > +        } else {
> > +            new_addr = pci_get_long(pf->config + bar);
> > +        }
> > +        new_addr += vf_num * size;
> > +    }
> > +    if (reg != PCI_ROM_SLOT) {
> > +        /* Preserve the rom enable bit */
> > +        new_addr &= ~(size - 1);
> 
> This comment puzzles me. How does clearing low bits preserve
> any bits? Looks like this will clear low bits if any.
> 

I think the comment applies to (reg != PCI_ROM_SLOT), i.e., the bits are
cleared for BARs, but not for expansion ROM. I agree the placement of this
comment is slightly misleading. We will move it up and rephrase slightly.
 
> > +pcibus_t pci_bar_address(PCIDevice *d,
> > +                         int reg, uint8_t type, pcibus_t size)
> >  {
> >      pcibus_t new_addr, last_addr;
> > -    int bar = pci_bar(d, reg);
> >      uint16_t cmd = pci_get_word(d->config + PCI_COMMAND);
> >      Object *machine = qdev_get_machine();
> >      ObjectClass *oc = object_get_class(machine);
> > @@ -1309,7 +1363,7 @@ static pcibus_t pci_bar_address(PCIDevice *d,
> >          if (!(cmd & PCI_COMMAND_IO)) {
> >              return PCI_BAR_UNMAPPED;
> >          }
> > -        new_addr = pci_get_long(d->config + bar) & ~(size - 1);
> > +        new_addr = pci_config_get_bar_addr(d, reg, type, size);
> >          last_addr = new_addr + size - 1;
> >          /* Check if 32 bit BAR wraps around explicitly.
> >           * TODO: make priorities correct and remove this work around.
> > @@ -1324,11 +1378,7 @@ static pcibus_t pci_bar_address(PCIDevice *d,
> >      if (!(cmd & PCI_COMMAND_MEMORY)) {
> >          return PCI_BAR_UNMAPPED;
> >      }
> > -    if (type & PCI_BASE_ADDRESS_MEM_TYPE_64) {
> > -        new_addr = pci_get_quad(d->config + bar);
> > -    } else {
> > -        new_addr = pci_get_long(d->config + bar);
> > -    }
> > +    new_addr = pci_config_get_bar_addr(d, reg, type, size);
> >      /* the ROM slot has a specific enable bit */
> >      if (reg == PCI_ROM_SLOT && !(new_addr & PCI_ROM_ADDRESS_ENABLE)) {
> 
> And in fact here we check the low bit and handle it specially.

The code seems correct for me. The bit is preserved for ROM case.

> > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> > index d7d73a31e4..182a225054 100644
> > --- a/hw/pci/pcie.c
> > +++ b/hw/pci/pcie.c
> > @@ -446,6 +446,11 @@ void pcie_cap_slot_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
> >      PCIDevice *pci_dev = PCI_DEVICE(dev);
> >      uint32_t lnkcap = pci_get_long(exp_cap + PCI_EXP_LNKCAP);
> >  
> > +    if(pci_is_vf(pci_dev)) {
> > +        /* We don't want to change any state in hotplug_dev for SR/IOV virtual functions */
> > +        return;
> > +    }
> > +
> 
> Coding style violation here.  And pls document the why not the what.
> E.g. IIRC the reason is that VFs don't have an express capability,
> right?

I think the reason is that virtual functions don’t exist physically, so
they cannot be individually disconnected. Only PF should respond to
hotplug events, implicitly disconnecting (thus: destroying) all child
VFs.

Anyway, we will update this comment to state *why* and add the missing
space.

V4 with the mentioned changes will happen really soon.