On Thu, Aug 13, 2020 at 6:33 AM Alex Williamson
<alex.williamson@xxxxxxxxxx> wrote:
>
> On Wed, 12 Aug 2020 15:21:11 -0400
> Matthew Rosato <mjrosato@xxxxxxxxxxxxx> wrote:
>
> > @@ -521,7 +522,8 @@ static int vfio_basic_config_read(struct vfio_pci_device *vdev, int pos,
> > count = vfio_default_config_read(vdev, pos, count, perm, offset, val);
> >
> > /* Mask in virtual memory enable for SR-IOV devices */
> > - if (offset == PCI_COMMAND && vdev->pdev->is_virtfn) {
> > + if ((offset == PCI_COMMAND) &&
> > + (vdev->pdev->is_virtfn || vdev->pdev->detached_vf)) {
> > u16 cmd = le16_to_cpu(*(__le16 *)&vdev->vconfig[PCI_COMMAND]);
> > u32 tmp_val = le32_to_cpu(*val);
> >
> > @@ -1734,7 +1736,8 @@ int vfio_config_init(struct vfio_pci_device *vdev)
> > vconfig[PCI_INTERRUPT_PIN]);
> >
> > vconfig[PCI_INTERRUPT_PIN] = 0; /* Gratuitous for good VFs */
> > -
> > + }
> > + if (pdev->is_virtfn || pdev->detached_vf) {
> > /*
> > * VFs do no implement the memory enable bit of the COMMAND
> > * register therefore we'll not have it set in our initial
> > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > index 8355306..23a6972 100644
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -445,6 +445,7 @@ struct pci_dev {
> > unsigned int is_probed:1; /* Device probing in progress */
> > unsigned int link_active_reporting:1;/* Device capable of reporting link active */
> > unsigned int no_vf_scan:1; /* Don't scan for VFs after IOV enablement */
> > + unsigned int detached_vf:1; /* VF without local PF access */
>
> Is there too much implicit knowledge in defining a "detached VF"? For
> example, why do we know that we can skip the portion of
> vfio_config_init() that copies the vendor and device IDs from the
> struct pci_dev into the virtual config space? It's true on s390x, but
> I think that's because we know that firmware emulates those registers
> for us.
>
> We also skip the INTx pin register sanity checking. Do we do
> that because we haven't installed the broken device into an s390x
> system? Because we know firmware manages that for us too? Or simply
> because s390x doesn't support INTx anyway, and therefore it's another
> architecture implicit decision?
Agreed. Any hacks we put in for normal VFs are going to be needed for
the passed-though VF case. Only applying the memory space enable
workaround doesn't make sense to me either.
> If detached_vf is really equivalent to is_virtfn for all cases that
> don't care about referencing physfn on the pci_dev, then we should
> probably have a macro to that effect.
A pci_is_virtfn() helper would be better than open coding both checks
everywhere. That said, it might be solving the wrong problem. The
union between ->physfn and ->sriov has always seemed like a footgun to
me so we might be better off switching the users who want a physfn to
a helper instead. i.e.
struct pci_dev *pci_get_vf_physfn(struct pci_dev *vf)
{
if (!vf->is_virtfn)
return NULL;
return vf->physfn;
}
...
pf = pci_get_vf_physfn(vf)
if (pf)
/* do pf things */
Then we can just use ->is_virtfn for the normal and detached cases.
Oliver
