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

[Knut Omang <knuto@ifi.uio.no>]

On Wed, 2022-01-26 at 14:23 +0100, Łukasz Gieryk wrote:
> 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.

Yes, sorry - this one flushed past me here for some reason,

> 
> 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.

I agree - it's maybe better to just put the comment above the if(...) 
other than that I believe it is correct.

Knut

>  
> > > +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.
>