Re: [Xen-devel] [PATCH V3 08/10] Introduce Xen PCI Passthrough, PCI config space helpers (2/3)

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

> > > +    case PCI_CAP_ID_EXP:
> > > +        /* The PCI Express Capability Structure of the VF of Intel 82599 10GbE
> > > +         * Controller looks trivial, e.g., the PCI Express Capabilities
> > > +         * Register is 0. We should not try to expose it to guest.
> >
> > Why not?
> 
> Because (an old commit):
> 
> passthrough: support the assignment of the VF of Intel 82599 10GbE Controller
> 
> The datasheet is available at
> http://download.intel.com/design/network/datashts/82599_datasheet.pdf
> 
> See 'Table 9.7. VF PCIe Configuration Space' of the datasheet, the PCI
> Express Capability Structure of the VF of Intel 82599 10GbE Controller looks
> trivial, e.g., the PCI Express Capabilities Register is 0, so the Capability
> Version is 0 and pt_pcie_size_init() would fail.
> 
> We should not try to expose the PCIe cap of the device to guest.
> 
> a patch from Dexuan Cui <dexuan.cui@intel.com>

Lets inlude that in the description here..
> 
> > > +         */
> > > +        if (d->vendor_id == PCI_VENDOR_ID_INTEL &&
> > > +                d->device_id == PCI_DEVICE_ID_INTEL_82599_VF) {
> > > +            return 1;
> > > +        }
> > > +        break;
> > > +    }
> > > +    return 0;
> > > +}
> > > +
> > > +/*   find emulate register group entry */
> > > +XenPTRegGroup *pt_find_reg_grp(XenPCIPassthroughState *s, uint32_t address)
> > > +{
> > > +    XenPTRegGroup *entry = NULL;
> > > +
> > > +    /* find register group entry */
> > > +    QLIST_FOREACH(entry, &s->reg_grp_tbl, entries) {
> > > +        /* check address */
> > > +        if ((entry->base_offset <= address)
> > > +            && ((entry->base_offset + entry->size) > address)) {
> > > +            return entry;
> > > +        }
> > > +    }
> > > +
> > > +    /* group entry not found */
> > > +    return NULL;
> > > +}
> > > +
> > > +/* find emulate register entry */
> > > +XenPTReg *pt_find_reg(XenPTRegGroup *reg_grp, uint32_t address)
> > > +{
> > > +    XenPTReg *reg_entry = NULL;
> > > +    XenPTRegInfo *reg = NULL;
> > > +    uint32_t real_offset = 0;
> > > +
> > > +    /* find register entry */
> > > +    QLIST_FOREACH(reg_entry, &reg_grp->reg_tbl_list, entries) {
> > > +        reg = reg_entry->reg;
> > > +        real_offset = reg_grp->base_offset + reg->offset;
> > > +        /* check address */
> > > +        if ((real_offset <= address)
> > > +            && ((real_offset + reg->size) > address)) {
> > > +            return reg_entry;
> > > +        }
> > > +    }
> > > +
> > > +    return NULL;
> > > +}
> > > +
> > > +/* parse BAR */
> > > +static PTBarFlag pt_bar_reg_parse(XenPCIPassthroughState *s, XenPTRegInfo *reg)
> > > +{
> > > +    PCIDevice *d = &s->dev;
> > > +    XenPTRegion *region = NULL;
> > > +    PCIIORegion *r;
> > > +    int index = 0;
> > > +
> > > +    /* check 64bit BAR */
> > > +    index = pt_bar_offset_to_index(reg->offset);
> > > +    if ((0 < index) && (index < PCI_ROM_SLOT)) {
> >
> > This is  a bit confusing. Can you make the index be on the same
> > side, like
> >
> > if ((0 < index) && (PCI_ROM_SLOT > index)
> >
> > or better:
> >
> > if ((index < 0) && (index < PCI_ROM_SLOT))
> >
> > um, which looks wrong. Should it be 'index > 0' ?
> 
> Every other form is a bit confusing to me. I'd like to write
> 0 < index < ROM_SLOT, so I know that index is between 0 and ROM_SLOT.
> But, it's C and not math, so I wrote the closest way I can.
> 
> > > +        int flags = s->real_device->io_regions[index - 1].flags;
> >
> > Do we want to check the index - 1 to make sure it is not negative?
> 
> We have:
>   0 < index < ROM_SLOT
> so (index - 1) give us:
>   0 <= index - 1 < ROM_SLOT - 1
> 
> So (index - 1) can be 0, but under 0.
> ;)

Right! Ok, then please ignore my comment.

.. snip..
> > > +    cap_ver = pci_get_byte(s->dev.config + real_offset - reg->offset
> > > +                           + PCI_EXP_FLAGS)
> > > +        & PCI_EXP_FLAGS_VERS;
> > > +    dev_type = (pci_get_byte(s->dev.config + real_offset - reg->offset
> > > +                             + PCI_EXP_FLAGS)
> > > +                & PCI_EXP_FLAGS_TYPE) >> 4;
> > > +
> > > +    /* no need to initialize in case of Root Complex Integrated Endpoint
> > > +     * with cap_ver 1.x
> >
> > Why?
> 
> Who knows? I don't. And `git log` does not give me more information.

<laughs> OK, could the earlier author provide some ideas? Or perhaps
there is something akin in the Linux code.

.. snip..
> > > +    cap_ver = pci_get_byte(s->dev.config + real_offset - reg->offset
> > > +                           + PCI_EXP_FLAGS)
> > > +        & PCI_EXP_FLAGS_VERS;
> >
> > This looks like a weird tab issue, but it might be just my mailer.
> 
> Nop, there is no tab.
> 
> Maybe writing it like that:
> > cap_ver = pci_get_byte(s->dev.config + real_offset - reg->offset
> >                        + PCI_EXP_FLAGS);
> > cap_ver &= PCI_EXP_FLAGS_VERS;
> would be better.

OK.
> 
> > > +
> > > +    /* no need to initialize in case of cap_ver 1.x */
> > > +    if (cap_ver == 1) {
> > > +        return PT_INVALID_REG;

.. snip..
> > > +    case 2:
> > > +        PT_LOG("Power state transition D2 -> D0active\n");
> > > +        host_pci_set_word(s->real_device, real_offset, 0);
> > > +        usleep(200);
> >
> > Heheh..
> 
> I don't know if I can remove it safely, or not.

Probably not. One of my machines reguarly gets confused when the SSD disk
returns VPD information way to fast and it ends up using the name of a
previous disk.. So the usleep is probably very much required (and in
all likehood defined in the PCI spec).