From: Igor Mammedov <imammedo@redhat.com>
To: Ani Sinha <anisinha@redhat.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>,
Marcel Apfelbaum <marcel.apfelbaum@gmail.com>,
Julia Suvorova <jusual@redhat.com>,
qemu-devel@nongnu.org
Subject: Re: [PATCH v2] hw/pci: prevent hotplug of devices on pcie-root-ports on the wrong slot
Date: Fri, 16 Jun 2023 10:32:27 +0200 [thread overview]
Message-ID: <20230616103227.0028f4fa@imammedo.users.ipa.redhat.com> (raw)
In-Reply-To: <D505615B-F22E-441A-9F6C-566E46C714E8@redhat.com>
On Fri, 16 Jun 2023 13:06:06 +0530
Ani Sinha <anisinha@redhat.com> wrote:
> > On 15-Jun-2023, at 4:56 PM, Igor Mammedov <imammedo@redhat.com> wrote:
> >
> > On Thu, 15 Jun 2023 10:46:45 +0530
> > Ani Sinha <anisinha@redhat.com> wrote:
> >
> >> PCIE root ports and other upstream ports only allow one device on slot 0.
> >> When hotplugging a device on a pcie root port, make sure that the device
> >> address passed always represents slot 0. Any other slot value would be
> >> illegal on a root port.
> >>
> >> CC: jusual@redhat.com
> >> CC: imammedo@redhat.com
> >> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2128929
> >> Signed-off-by: Ani Sinha <anisinha@redhat.com>
> >> ---
> >> hw/pci/pci.c | 16 ++++++++++++++++
> >> 1 file changed, 16 insertions(+)
> >>
> >> changelog:
> >> v2: feedback from mst included.
> >>
> >> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> >> index bf38905b7d..66999352cc 100644
> >> --- a/hw/pci/pci.c
> >> +++ b/hw/pci/pci.c
> >> @@ -64,6 +64,7 @@ bool pci_available = true;
> >> static char *pcibus_get_dev_path(DeviceState *dev);
> >> static char *pcibus_get_fw_dev_path(DeviceState *dev);
> >> static void pcibus_reset(BusState *qbus);
> >> +static bool pcie_has_upstream_port(PCIDevice *dev);
> >>
> >> static Property pci_props[] = {
> >> DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1),
> >> @@ -1182,6 +1183,11 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev,
> >> } else if (dev->hotplugged &&
> >> !pci_is_vf(pci_dev) &&
> >> pci_get_function_0(pci_dev)) {
> >> + /*
> >> + * populating function 0 triggers a bus scan from the guest that
> >> + * exposes other non-zero functions. Hence we need to ensure that
> >> + * function 0 is available.
> >> + */
> >> error_setg(errp, "PCI: slot %d function 0 already occupied by %s,"
> >> " new func %s cannot be exposed to guest.",
> >> PCI_SLOT(pci_get_function_0(pci_dev)->devfn),
> >> @@ -1189,6 +1195,16 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev,
> >> name);
> >>
> >> return NULL;
> >> + } else if (dev->hotplugged &&
> >> + !pci_is_vf(pci_dev) &&
> >> + pcie_has_upstream_port(pci_dev) && PCI_SLOT(devfn)) {
> >> + /*
> >> + * If the device is being plugged into an upstream PCIE port,
> >
> > you probably mixing up downstream port with upstream one,
> > the only thing that could be plugged into upstream port
> > is PCIE switch.
> >
> > Also I'm not sure that we should do this at all.
> > Looking at BZ it seems that QEMU crashes inside backend
> > and tear down/cleanup sequence is broken somewhere.
> > And that is the root cause, so I'd rather fix that 1st
> > and only after that consider adding workarounds if any
> > were necessary.
>
> I have added more details in the ticket. I still believe that my approach is in the right direction.
eject is _optional_ and guest is free to ignore eject request.
That shall not cause improper QEMU behavior.
Preventing bug trigger is ok if we can't fix root cause but
then one should explain in commit message what root cause
is and why it can't be fixed.
does it work for similar coldplugged setup without unplug
(if yes then why)?
>
> >
> >
> >> + * like a pcie root port, we only support one device at slot 0
> >> + */
> >> + error_setg(errp, "PCI: slot %d is not valid for %s",
> >> + PCI_SLOT(devfn), name);
> >> + return NULL;
> >> }
> >>
> >> pci_dev->devfn = devfn;
>
next prev parent reply other threads:[~2023-06-16 8:33 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-06-15 5:16 [PATCH v2] hw/pci: prevent hotplug of devices on pcie-root-ports on the wrong slot Ani Sinha
2023-06-15 11:26 ` Igor Mammedov
2023-06-16 7:36 ` Ani Sinha
2023-06-16 8:32 ` Igor Mammedov [this message]
2023-06-19 12:01 ` Ani Sinha
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=20230616103227.0028f4fa@imammedo.users.ipa.redhat.com \
--to=imammedo@redhat.com \
--cc=anisinha@redhat.com \
--cc=jusual@redhat.com \
--cc=marcel.apfelbaum@gmail.com \
--cc=mst@redhat.com \
--cc=qemu-devel@nongnu.org \
/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).