hw/pci/pci.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
When a device has an upstream PCIE port, we can only use slot 0. Non-zero slots
are invalid. This change ensures that we throw an error if the user
tries to hotplug a device with an upstream PCIE port to a non-zero slot.
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 | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
changelog:
v2: addressed issue with multifunction pcie root ports. Should allow
hotplug on functions other than function 0.
v3: improved commit message.
v4: improve commit message and code comments further. Some more
improvements might come in v5. No claims made here that this is
the final one :-)
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index bf38905b7d..30ce6a78cb 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 wasn't added yet.
+ */
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,18 @@ 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 has an upstream PCIE port, like a pcie root port,
+ * we only support functions on slot 0.
+ */
+ error_setg(errp, "PCI: slot %d is not valid for %s,"
+ " only functions on slot 0 is supported for devices"
+ " with an upstream PCIE port.",
+ PCI_SLOT(devfn), name);
+ return NULL;
}
pci_dev->devfn = devfn;
--
2.39.1just noticed a repeated "slot" in the subject btw.
On Tue, Jun 20, 2023 at 12:48:05PM +0530, Ani Sinha wrote:
> When a device has an upstream PCIE port, we can only use slot 0.
Actually, it's when device is plugged into a PCIE port.
So maybe:
PCI Express ports only have one slot, so
PCI Express devices can only be plugged into
slot 0 on a PCIE port
> Non-zero slots
> are invalid. This change ensures that we throw an error if the user
> tries to hotplug a device with an upstream PCIE port to a non-zero slot.
it also adds a comment explaining why function 0 must not exist
when function != 0 is added. or maybe split that part out.
> 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 | 18 ++++++++++++++++++
> 1 file changed, 18 insertions(+)
>
> changelog:
> v2: addressed issue with multifunction pcie root ports. Should allow
> hotplug on functions other than function 0.
> v3: improved commit message.
> v4: improve commit message and code comments further. Some more
> improvements might come in v5. No claims made here that this is
> the final one :-)
>
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index bf38905b7d..30ce6a78cb 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 wasn't added yet.
> + */
Pls capitalize populating. Also, comments like this should come
before the logic they document, not after. By the way it doesn't
have to be a bus scan - I'd just say "a scan" - with ACPI
guest knows what was added and can just probe the device functions.
> 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,18 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev,
> name);
>
> return NULL;
> + } else if (dev->hotplugged &&
why hotplugged? Doesn't the same rule apply to all devices?
> + !pci_is_vf(pci_dev) &&
Hmm. I think you copied it from here:
} else if (dev->hotplugged &&
!pci_is_vf(pci_dev) &&
pci_get_function_0(pci_dev)) {
it makes sense there because VFs are added later
after PF exists.
But here it makes no sense that I can see.
> + pcie_has_upstream_port(pci_dev) && PCI_SLOT(devfn)) {
> + /*
> + * If the device has an upstream PCIE port, like a pcie root port,
no, a root port can not be an upstream port.
> + * we only support functions on slot 0.
> + */
> + error_setg(errp, "PCI: slot %d is not valid for %s,"
> + " only functions on slot 0 is supported for devices"
> + " with an upstream PCIE port.",
something like:
error_setg(errp, "PCI: slot %d is not valid for %s:"
" PCI Express devices can only be plugged into slot 0")
and then you don't really need a comment.
> + PCI_SLOT(devfn), name);
> + return NULL;
> }
>
> pci_dev->devfn = devfn;
> --
> 2.39.1
> On 20-Jun-2023, at 4:13 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Tue, Jun 20, 2023 at 12:48:05PM +0530, Ani Sinha wrote:
>> When a device has an upstream PCIE port, we can only use slot 0.
>
> Actually, it's when device is plugged into a PCIE port.
> So maybe:
>
> PCI Express ports only have one slot, so
> PCI Express devices can only be plugged into
> slot 0 on a PCIE port
>
>> Non-zero slots
>> are invalid. This change ensures that we throw an error if the user
>> tries to hotplug a device with an upstream PCIE port to a non-zero slot.
>
> it also adds a comment explaining why function 0 must not exist
> when function != 0 is added. or maybe split that part out.
>
>> 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 | 18 ++++++++++++++++++
>> 1 file changed, 18 insertions(+)
>>
>> changelog:
>> v2: addressed issue with multifunction pcie root ports. Should allow
>> hotplug on functions other than function 0.
>> v3: improved commit message.
>> v4: improve commit message and code comments further. Some more
>> improvements might come in v5. No claims made here that this is
>> the final one :-)
>>
>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>> index bf38905b7d..30ce6a78cb 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 wasn't added yet.
>> + */
>
> Pls capitalize populating. Also, comments like this should come
> before the logic they document, not after. By the way it doesn't
> have to be a bus scan - I'd just say "a scan" - with ACPI
> guest knows what was added and can just probe the device functions.
>
>> 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,18 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev,
>> name);
>>
>> return NULL;
>> + } else if (dev->hotplugged &&
>
> why hotplugged? Doesn't the same rule apply to all devices?
>
>> + !pci_is_vf(pci_dev) &&
>
> Hmm. I think you copied it from here:
> } else if (dev->hotplugged &&
> !pci_is_vf(pci_dev) &&
> pci_get_function_0(pci_dev)) {
>
> it makes sense there because VFs are added later
> after PF exists.
>
> But here it makes no sense that I can see.
This patch with these changes causes failures in bios-tables-test which can be fixed easily. It also breaks hd-geo-test and I do not know enough of this test to fix them.
I think I will drop this patch for now.
>
>
>> + pcie_has_upstream_port(pci_dev) && PCI_SLOT(devfn)) {
>> + /*
>> + * If the device has an upstream PCIE port, like a pcie root port,
>
> no, a root port can not be an upstream port.
>
>
>> + * we only support functions on slot 0.
>> + */
>> + error_setg(errp, "PCI: slot %d is not valid for %s,"
>> + " only functions on slot 0 is supported for devices"
>> + " with an upstream PCIE port.",
>
>
> something like:
>
> error_setg(errp, "PCI: slot %d is not valid for %s:"
> " PCI Express devices can only be plugged into slot 0")
>
> and then you don't really need a comment.
>
>
>> + PCI_SLOT(devfn), name);
>> + return NULL;
>> }
>>
>> pci_dev->devfn = devfn;
>> --
>> 2.39.1
> On 21-Jun-2023, at 4:36 PM, Ani Sinha <anisinha@redhat.com> wrote:
>
>
>
>> On 20-Jun-2023, at 4:13 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>
>> On Tue, Jun 20, 2023 at 12:48:05PM +0530, Ani Sinha wrote:
>>> When a device has an upstream PCIE port, we can only use slot 0.
>>
>> Actually, it's when device is plugged into a PCIE port.
>> So maybe:
>>
>> PCI Express ports only have one slot, so
>> PCI Express devices can only be plugged into
>> slot 0 on a PCIE port
>>
>>> Non-zero slots
>>> are invalid. This change ensures that we throw an error if the user
>>> tries to hotplug a device with an upstream PCIE port to a non-zero slot.
>>
>> it also adds a comment explaining why function 0 must not exist
>> when function != 0 is added. or maybe split that part out.
>>
>>> 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 | 18 ++++++++++++++++++
>>> 1 file changed, 18 insertions(+)
>>>
>>> changelog:
>>> v2: addressed issue with multifunction pcie root ports. Should allow
>>> hotplug on functions other than function 0.
>>> v3: improved commit message.
>>> v4: improve commit message and code comments further. Some more
>>> improvements might come in v5. No claims made here that this is
>>> the final one :-)
>>>
>>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>>> index bf38905b7d..30ce6a78cb 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 wasn't added yet.
>>> + */
>>
>> Pls capitalize populating. Also, comments like this should come
>> before the logic they document, not after. By the way it doesn't
>> have to be a bus scan - I'd just say "a scan" - with ACPI
>> guest knows what was added and can just probe the device functions.
>>
>>> 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,18 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev,
>>> name);
>>>
>>> return NULL;
>>> + } else if (dev->hotplugged &&
>>
>> why hotplugged? Doesn't the same rule apply to all devices?
>>
>>> + !pci_is_vf(pci_dev) &&
>>
>> Hmm. I think you copied it from here:
>> } else if (dev->hotplugged &&
>> !pci_is_vf(pci_dev) &&
>> pci_get_function_0(pci_dev)) {
>>
>> it makes sense there because VFs are added later
>> after PF exists.
>>
>> But here it makes no sense that I can see.
>
> This patch with these changes causes failures in bios-tables-test which can be fixed easily. It also breaks hd-geo-test and I do not know enough of this test to fix them.
Specifically it breaks test_override_scsi_q35() and test_override_virtio_blk_q35().
I think these tests were wrong to begin with since they attach a pcie-to-pci bridge on a pcie root port and then attach a scsi controller not on the pcie-to-pci bridge but on the root port (which is not possible because we can only attach one device on a non-multifunction pcie root port). Even if I fix them, its failing somewhere else.
I have added Thomas and Laurent, maybe they can help fix these in this test.
I have pushed my patch here: https://gitlab.com/anisinha/qemu/-/commit/24b3eddb968a0739bff222bdf781f722365cc9b2
>
> I think I will drop this patch for now.
>
>>
>>
>>> + pcie_has_upstream_port(pci_dev) && PCI_SLOT(devfn)) {
>>> + /*
>>> + * If the device has an upstream PCIE port, like a pcie root port,
>>
>> no, a root port can not be an upstream port.
>>
>>
>>> + * we only support functions on slot 0.
>>> + */
>>> + error_setg(errp, "PCI: slot %d is not valid for %s,"
>>> + " only functions on slot 0 is supported for devices"
>>> + " with an upstream PCIE port.",
>>
>>
>> something like:
>>
>> error_setg(errp, "PCI: slot %d is not valid for %s:"
>> " PCI Express devices can only be plugged into slot 0")
>>
>> and then you don't really need a comment.
>>
>>
>>> + PCI_SLOT(devfn), name);
>>> + return NULL;
>>> }
>>>
>>> pci_dev->devfn = devfn;
>>> --
>>> 2.39.1
> On 21-Jun-2023, at 4:55 PM, Ani Sinha <anisinha@redhat.com> wrote:
>
>
>
>> On 21-Jun-2023, at 4:36 PM, Ani Sinha <anisinha@redhat.com> wrote:
>>
>>
>>
>>> On 20-Jun-2023, at 4:13 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>
>>> On Tue, Jun 20, 2023 at 12:48:05PM +0530, Ani Sinha wrote:
>>>> When a device has an upstream PCIE port, we can only use slot 0.
>>>
>>> Actually, it's when device is plugged into a PCIE port.
>>> So maybe:
>>>
>>> PCI Express ports only have one slot, so
>>> PCI Express devices can only be plugged into
>>> slot 0 on a PCIE port
>>>
>>>> Non-zero slots
>>>> are invalid. This change ensures that we throw an error if the user
>>>> tries to hotplug a device with an upstream PCIE port to a non-zero slot.
>>>
>>> it also adds a comment explaining why function 0 must not exist
>>> when function != 0 is added. or maybe split that part out.
>>>
>>>> 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 | 18 ++++++++++++++++++
>>>> 1 file changed, 18 insertions(+)
>>>>
>>>> changelog:
>>>> v2: addressed issue with multifunction pcie root ports. Should allow
>>>> hotplug on functions other than function 0.
>>>> v3: improved commit message.
>>>> v4: improve commit message and code comments further. Some more
>>>> improvements might come in v5. No claims made here that this is
>>>> the final one :-)
>>>>
>>>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>>>> index bf38905b7d..30ce6a78cb 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 wasn't added yet.
>>>> + */
>>>
>>> Pls capitalize populating. Also, comments like this should come
>>> before the logic they document, not after. By the way it doesn't
>>> have to be a bus scan - I'd just say "a scan" - with ACPI
>>> guest knows what was added and can just probe the device functions.
This commit essentially needs fixing:
commit c46b126088b5616d8b7cd3ff83aaf5d097c36633
Author: Michael Labiuk <michael.labiuk@virtuozzo.com>
Date: Fri Sep 30 01:35:42 2022 +0300
tests/x86: Add 'q35' machine type to override-tests in hd-geo-test
Signed-off-by: Michael Labiuk <michael.labiuk@virtuozzo.com>
Message-Id: <20220929223547.1429580-5-michael.labiuk@virtuozzo.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>
>>>> 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,18 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev,
>>>> name);
>>>>
>>>> return NULL;
>>>> + } else if (dev->hotplugged &&
>>>
>>> why hotplugged? Doesn't the same rule apply to all devices?
>>>
>>>> + !pci_is_vf(pci_dev) &&
>>>
>>> Hmm. I think you copied it from here:
>>> } else if (dev->hotplugged &&
>>> !pci_is_vf(pci_dev) &&
>>> pci_get_function_0(pci_dev)) {
>>>
>>> it makes sense there because VFs are added later
>>> after PF exists.
>>>
>>> But here it makes no sense that I can see.
>>
>> This patch with these changes causes failures in bios-tables-test which can be fixed easily. It also breaks hd-geo-test and I do not know enough of this test to fix them.
>
> Specifically it breaks test_override_scsi_q35() and test_override_virtio_blk_q35().
> I think these tests were wrong to begin with since they attach a pcie-to-pci bridge on a pcie root port and then attach a scsi controller not on the pcie-to-pci bridge but on the root port (which is not possible because we can only attach one device on a non-multifunction pcie root port). Even if I fix them, its failing somewhere else.
>
> I have added Thomas and Laurent, maybe they can help fix these in this test.
> I have pushed my patch here: https://gitlab.com/anisinha/qemu/-/commit/24b3eddb968a0739bff222bdf781f722365cc9b2
>
>
>>
>> I think I will drop this patch for now.
>>
>>>
>>>
>>>> + pcie_has_upstream_port(pci_dev) && PCI_SLOT(devfn)) {
>>>> + /*
>>>> + * If the device has an upstream PCIE port, like a pcie root port,
>>>
>>> no, a root port can not be an upstream port.
>>>
>>>
>>>> + * we only support functions on slot 0.
>>>> + */
>>>> + error_setg(errp, "PCI: slot %d is not valid for %s,"
>>>> + " only functions on slot 0 is supported for devices"
>>>> + " with an upstream PCIE port.",
>>>
>>>
>>> something like:
>>>
>>> error_setg(errp, "PCI: slot %d is not valid for %s:"
>>> " PCI Express devices can only be plugged into slot 0")
>>>
>>> and then you don't really need a comment.
>>>
>>>
>>>> + PCI_SLOT(devfn), name);
>>>> + return NULL;
>>>> }
>>>>
>>>> pci_dev->devfn = devfn;
>>>> --
>>>> 2.39.1
> On 21-Jun-2023, at 5:20 PM, Ani Sinha <anisinha@redhat.com> wrote:
>
>
>
>> On 21-Jun-2023, at 4:55 PM, Ani Sinha <anisinha@redhat.com> wrote:
>>
>>
>>
>>> On 21-Jun-2023, at 4:36 PM, Ani Sinha <anisinha@redhat.com> wrote:
>>>
>>>
>>>
>>>> On 20-Jun-2023, at 4:13 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>>
>>>> On Tue, Jun 20, 2023 at 12:48:05PM +0530, Ani Sinha wrote:
>>>>> When a device has an upstream PCIE port, we can only use slot 0.
>>>>
>>>> Actually, it's when device is plugged into a PCIE port.
>>>> So maybe:
>>>>
>>>> PCI Express ports only have one slot, so
>>>> PCI Express devices can only be plugged into
>>>> slot 0 on a PCIE port
>>>>
>>>>> Non-zero slots
>>>>> are invalid. This change ensures that we throw an error if the user
>>>>> tries to hotplug a device with an upstream PCIE port to a non-zero slot.
>>>>
>>>> it also adds a comment explaining why function 0 must not exist
>>>> when function != 0 is added. or maybe split that part out.
>>>>
>>>>> 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 | 18 ++++++++++++++++++
>>>>> 1 file changed, 18 insertions(+)
>>>>>
>>>>> changelog:
>>>>> v2: addressed issue with multifunction pcie root ports. Should allow
>>>>> hotplug on functions other than function 0.
>>>>> v3: improved commit message.
>>>>> v4: improve commit message and code comments further. Some more
>>>>> improvements might come in v5. No claims made here that this is
>>>>> the final one :-)
>>>>>
>>>>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>>>>> index bf38905b7d..30ce6a78cb 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 wasn't added yet.
>>>>> + */
>>>>
>>>> Pls capitalize populating. Also, comments like this should come
>>>> before the logic they document, not after. By the way it doesn't
>>>> have to be a bus scan - I'd just say "a scan" - with ACPI
>>>> guest knows what was added and can just probe the device functions.
>
> This commit essentially needs fixing:
>
> commit c46b126088b5616d8b7cd3ff83aaf5d097c36633
> Author: Michael Labiuk <michael.labiuk@virtuozzo.com>
> Date: Fri Sep 30 01:35:42 2022 +0300
>
> tests/x86: Add 'q35' machine type to override-tests in hd-geo-test
>
> Signed-off-by: Michael Labiuk <michael.labiuk@virtuozzo.com>
> Message-Id: <20220929223547.1429580-5-michael.labiuk@virtuozzo.com>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
I have sent a patch series with a proposed fix for this test (hd-geo-test) as well as fixes for bios-tables-test. The series also includes my QEMU patch that enforces proper PCIE slot usage.
The patches are part of one series and should be applied in the same sequence - test fixes first then the QEMU fix so that the QEMU fix does not break the unit tests.
Please review.
>
>>>>
>>>>> 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,18 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev,
>>>>> name);
>>>>>
>>>>> return NULL;
>>>>> + } else if (dev->hotplugged &&
>>>>
>>>> why hotplugged? Doesn't the same rule apply to all devices?
>>>>
>>>>> + !pci_is_vf(pci_dev) &&
>>>>
>>>> Hmm. I think you copied it from here:
>>>> } else if (dev->hotplugged &&
>>>> !pci_is_vf(pci_dev) &&
>>>> pci_get_function_0(pci_dev)) {
>>>>
>>>> it makes sense there because VFs are added later
>>>> after PF exists.
>>>>
>>>> But here it makes no sense that I can see.
>>>
>>> This patch with these changes causes failures in bios-tables-test which can be fixed easily. It also breaks hd-geo-test and I do not know enough of this test to fix them.
>>
>> Specifically it breaks test_override_scsi_q35() and test_override_virtio_blk_q35().
>> I think these tests were wrong to begin with since they attach a pcie-to-pci bridge on a pcie root port and then attach a scsi controller not on the pcie-to-pci bridge but on the root port (which is not possible because we can only attach one device on a non-multifunction pcie root port). Even if I fix them, its failing somewhere else.
>>
>> I have added Thomas and Laurent, maybe they can help fix these in this test.
>> I have pushed my patch here: https://gitlab.com/anisinha/qemu/-/commit/24b3eddb968a0739bff222bdf781f722365cc9b2
>>
>>
>>>
>>> I think I will drop this patch for now.
>>>
>>>>
>>>>
>>>>> + pcie_has_upstream_port(pci_dev) && PCI_SLOT(devfn)) {
>>>>> + /*
>>>>> + * If the device has an upstream PCIE port, like a pcie root port,
>>>>
>>>> no, a root port can not be an upstream port.
>>>>
>>>>
>>>>> + * we only support functions on slot 0.
>>>>> + */
>>>>> + error_setg(errp, "PCI: slot %d is not valid for %s,"
>>>>> + " only functions on slot 0 is supported for devices"
>>>>> + " with an upstream PCIE port.",
>>>>
>>>>
>>>> something like:
>>>>
>>>> error_setg(errp, "PCI: slot %d is not valid for %s:"
>>>> " PCI Express devices can only be plugged into slot 0")
>>>>
>>>> and then you don't really need a comment.
>>>>
>>>>
>>>>> + PCI_SLOT(devfn), name);
>>>>> + return NULL;
>>>>> }
>>>>>
>>>>> pci_dev->devfn = devfn;
>>>>> --
>>>>> 2.39.1
> On 20-Jun-2023, at 4:13 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Tue, Jun 20, 2023 at 12:48:05PM +0530, Ani Sinha wrote:
>> When a device has an upstream PCIE port, we can only use slot 0.
>
> Actually, it's when device is plugged into a PCIE port.
> So maybe:
>
> PCI Express ports only have one slot, so
> PCI Express devices can only be plugged into
> slot 0 on a PCIE port
>
>> Non-zero slots
>> are invalid. This change ensures that we throw an error if the user
>> tries to hotplug a device with an upstream PCIE port to a non-zero slot.
>
> it also adds a comment explaining why function 0 must not exist
> when function != 0 is added. or maybe split that part out.
>
>> 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 | 18 ++++++++++++++++++
>> 1 file changed, 18 insertions(+)
>>
>> changelog:
>> v2: addressed issue with multifunction pcie root ports. Should allow
>> hotplug on functions other than function 0.
>> v3: improved commit message.
>> v4: improve commit message and code comments further. Some more
>> improvements might come in v5. No claims made here that this is
>> the final one :-)
>>
>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>> index bf38905b7d..30ce6a78cb 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 wasn't added yet.
>> + */
>
> Pls capitalize populating. Also, comments like this should come
> before the logic they document, not after. By the way it doesn't
> have to be a bus scan - I'd just say "a scan" - with ACPI
> guest knows what was added and can just probe the device functions.
>
>> 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,18 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev,
>> name);
>>
>> return NULL;
>> + } else if (dev->hotplugged &&
>
> why hotplugged? Doesn't the same rule apply to all devices?
>
>> + !pci_is_vf(pci_dev) &&
>
> Hmm. I think you copied it from here:
> } else if (dev->hotplugged &&
> !pci_is_vf(pci_dev) &&
> pci_get_function_0(pci_dev)) {
>
> it makes sense there because VFs are added later
> after PF exists.
I thought PFs are handled only in the host OS and only VFs are passthrough into the guest? I thought this check was because VFs have a different domain address separate from other non-vf devices in the guest PCI tree.
>
> But here it makes no sense that I can see.
>
>
>> + pcie_has_upstream_port(pci_dev) && PCI_SLOT(devfn)) {
>> + /*
>> + * If the device has an upstream PCIE port, like a pcie root port,
>
> no, a root port can not be an upstream port.
>
>
>> + * we only support functions on slot 0.
>> + */
>> + error_setg(errp, "PCI: slot %d is not valid for %s,"
>> + " only functions on slot 0 is supported for devices"
>> + " with an upstream PCIE port.",
>
>
> something like:
>
> error_setg(errp, "PCI: slot %d is not valid for %s:"
> " PCI Express devices can only be plugged into slot 0")
>
> and then you don't really need a comment.
>
>
>> + PCI_SLOT(devfn), name);
>> + return NULL;
>> }
>>
>> pci_dev->devfn = devfn;
>> --
>> 2.39.1
On Wed, Jun 21, 2023 at 08:09:55AM +0530, Ani Sinha wrote:
>
>
> > On 20-Jun-2023, at 4:13 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Tue, Jun 20, 2023 at 12:48:05PM +0530, Ani Sinha wrote:
> >> When a device has an upstream PCIE port, we can only use slot 0.
> >
> > Actually, it's when device is plugged into a PCIE port.
> > So maybe:
> >
> > PCI Express ports only have one slot, so
> > PCI Express devices can only be plugged into
> > slot 0 on a PCIE port
> >
> >> Non-zero slots
> >> are invalid. This change ensures that we throw an error if the user
> >> tries to hotplug a device with an upstream PCIE port to a non-zero slot.
> >
> > it also adds a comment explaining why function 0 must not exist
> > when function != 0 is added. or maybe split that part out.
> >
> >> 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 | 18 ++++++++++++++++++
> >> 1 file changed, 18 insertions(+)
> >>
> >> changelog:
> >> v2: addressed issue with multifunction pcie root ports. Should allow
> >> hotplug on functions other than function 0.
> >> v3: improved commit message.
> >> v4: improve commit message and code comments further. Some more
> >> improvements might come in v5. No claims made here that this is
> >> the final one :-)
> >>
> >> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> >> index bf38905b7d..30ce6a78cb 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 wasn't added yet.
> >> + */
> >
> > Pls capitalize populating. Also, comments like this should come
> > before the logic they document, not after. By the way it doesn't
> > have to be a bus scan - I'd just say "a scan" - with ACPI
> > guest knows what was added and can just probe the device functions.
> >
> >> 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,18 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev,
> >> name);
> >>
> >> return NULL;
> >> + } else if (dev->hotplugged &&
> >
> > why hotplugged? Doesn't the same rule apply to all devices?
> >
> >> + !pci_is_vf(pci_dev) &&
> >
> > Hmm. I think you copied it from here:
> > } else if (dev->hotplugged &&
> > !pci_is_vf(pci_dev) &&
> > pci_get_function_0(pci_dev)) {
> >
> > it makes sense there because VFs are added later
> > after PF exists.
>
> I thought PFs are handled only in the host OS and only VFs are
> passthrough into the guest?
This is emulated SRIOV. host and guest would be nested L2.
> I thought this check was because VFs have
> a different domain address separate from other non-vf devices in the
> guest PCI tree.
Maybe take a look at the SRIOV spec then.
> >
> > But here it makes no sense that I can see.
> >
> >
> >> + pcie_has_upstream_port(pci_dev) && PCI_SLOT(devfn)) {
> >> + /*
> >> + * If the device has an upstream PCIE port, like a pcie root port,
> >
> > no, a root port can not be an upstream port.
> >
> >
> >> + * we only support functions on slot 0.
> >> + */
> >> + error_setg(errp, "PCI: slot %d is not valid for %s,"
> >> + " only functions on slot 0 is supported for devices"
> >> + " with an upstream PCIE port.",
> >
> >
> > something like:
> >
> > error_setg(errp, "PCI: slot %d is not valid for %s:"
> > " PCI Express devices can only be plugged into slot 0")
> >
> > and then you don't really need a comment.
> >
> >
> >> + PCI_SLOT(devfn), name);
> >> + return NULL;
> >> }
> >>
> >> pci_dev->devfn = devfn;
> >> --
> >> 2.39.1
On Tue, 20 Jun 2023 12:48:05 +0530
Ani Sinha <anisinha@redhat.com> wrote:
> When a device has an upstream PCIE port, we can only use slot 0. Non-zero slots
> are invalid.
> This change ensures that we throw an error if the user
> tries to hotplug a device with an upstream PCIE port to a non-zero slot.
Isn't the same true for coldplugged devices?
Why you limit it only to hotplug?
>
> 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 | 18 ++++++++++++++++++
> 1 file changed, 18 insertions(+)
>
> changelog:
> v2: addressed issue with multifunction pcie root ports. Should allow
> hotplug on functions other than function 0.
> v3: improved commit message.
> v4: improve commit message and code comments further. Some more
> improvements might come in v5. No claims made here that this is
> the final one :-)
>
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index bf38905b7d..30ce6a78cb 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 wasn't added yet.
> + */
> 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,18 @@ 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 has an upstream PCIE port, like a pcie root port,
> + * we only support functions on slot 0.
> + */
> + error_setg(errp, "PCI: slot %d is not valid for %s,"
> + " only functions on slot 0 is supported for devices"
> + " with an upstream PCIE port.",
upstream port language is confusing here and elsewhere you mention it.
It would be better to use root-port instead.
> + PCI_SLOT(devfn), name);
> + return NULL;
> }
>
> pci_dev->devfn = devfn;
On Tue, Jun 20, 2023 at 10:59:42AM +0200, Igor Mammedov wrote:
> On Tue, 20 Jun 2023 12:48:05 +0530
> Ani Sinha <anisinha@redhat.com> wrote:
>
> > When a device has an upstream PCIE port, we can only use slot 0. Non-zero slots
> > are invalid.
> > This change ensures that we throw an error if the user
> > tries to hotplug a device with an upstream PCIE port to a non-zero slot.
>
> Isn't the same true for coldplugged devices?
> Why you limit it only to hotplug?
>
> >
> > 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 | 18 ++++++++++++++++++
> > 1 file changed, 18 insertions(+)
> >
> > changelog:
> > v2: addressed issue with multifunction pcie root ports. Should allow
> > hotplug on functions other than function 0.
> > v3: improved commit message.
> > v4: improve commit message and code comments further. Some more
> > improvements might come in v5. No claims made here that this is
> > the final one :-)
> >
> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > index bf38905b7d..30ce6a78cb 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 wasn't added yet.
> > + */
> > 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,18 @@ 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 has an upstream PCIE port, like a pcie root port,
> > + * we only support functions on slot 0.
> > + */
> > + error_setg(errp, "PCI: slot %d is not valid for %s,"
> > + " only functions on slot 0 is supported for devices"
> > + " with an upstream PCIE port.",
>
> upstream port language is confusing here and elsewhere you mention it.
> It would be better to use root-port instead.
No i do not think this is specific to root ports.
it is technically any non-integrated express device but we also plug
pci devices into express ports as a hack.
so checking where device is plugged (this is what pcie_has_upstream_port
does) seems like a reasonable approach.
> > + PCI_SLOT(devfn), name);
> > + return NULL;
> > }
> >
> > pci_dev->devfn = devfn;
© 2016 - 2024 Red Hat, Inc.