From: "Oliver O'Halloran" <oohall@gmail.com>
To: Alex Williamson <alex.williamson@redhat.com>
Cc: Matthew Rosato <mjrosato@linux.ibm.com>,
Bjorn Helgaas <bhelgaas@google.com>,
schnelle@linux.ibm.com, pmorel@linux.ibm.com,
Michael Ellerman <mpe@ellerman.id.au>,
linux-s390@vger.kernel.org,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
KVM list <kvm@vger.kernel.org>,
linux-pci <linux-pci@vger.kernel.org>
Subject: Re: [PATCH v2] PCI: Introduce flag for detached virtual functions
Date: Thu, 13 Aug 2020 11:59:40 +1000 [thread overview]
Message-ID: <CAOSf1CFh4ygZeeqpjpbWFWxJJEpDjHD+Q_L4dUaU_3wx7_35pg@mail.gmail.com> (raw)
In-Reply-To: <20200812143254.2f080c38@x1.home>
On Thu, Aug 13, 2020 at 6:33 AM Alex Williamson
<alex.williamson@redhat.com> wrote:
>
> On Wed, 12 Aug 2020 15:21:11 -0400
> Matthew Rosato <mjrosato@linux.ibm.com> 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
next prev parent reply other threads:[~2020-08-13 1:59 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-08-12 19:21 [PATCH v2] PCI: Identifying detached virtual functions Matthew Rosato
2020-08-12 19:21 ` [PATCH v2] PCI: Introduce flag for " Matthew Rosato
2020-08-12 20:32 ` Alex Williamson
2020-08-13 1:59 ` Oliver O'Halloran [this message]
2020-08-13 7:54 ` Niklas Schnelle
2020-08-13 13:09 ` Matthew Rosato
2020-08-13 1:55 ` Oliver O'Halloran
2020-08-13 9:00 ` Niklas Schnelle
2020-08-13 9:59 ` Oliver O'Halloran
2020-08-13 10:40 ` Niklas Schnelle
2020-08-13 12:34 ` Niklas Schnelle
2020-08-13 13:11 ` Matthew Rosato
2020-08-13 13:28 ` Niklas Schnelle
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAOSf1CFh4ygZeeqpjpbWFWxJJEpDjHD+Q_L4dUaU_3wx7_35pg@mail.gmail.com \
--to=oohall@gmail.com \
--cc=alex.williamson@redhat.com \
--cc=bhelgaas@google.com \
--cc=kvm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=linux-s390@vger.kernel.org \
--cc=mjrosato@linux.ibm.com \
--cc=mpe@ellerman.id.au \
--cc=pmorel@linux.ibm.com \
--cc=schnelle@linux.ibm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).