> On 21-Jun-2023, at 5:20 PM, Ani Sinha <anisi...@redhat.com> wrote:
>
>
>
>> On 21-Jun-2023, at 4:55 PM, Ani Sinha <anisi...@redhat.com> wrote:
>>
>>
>>
>>> On 21-Jun-2023, at 4:36 PM, Ani Sinha <anisi...@redhat.com> wrote:
>>>
>>>
>>>
>>>> On 20-Jun-2023, at 4:13 PM, Michael S. Tsirkin <m...@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: jus...@redhat.com
>>>>> CC: imamm...@redhat.com
>>>>> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2128929
>>>>> Signed-off-by: Ani Sinha <anisi...@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.lab...@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.lab...@virtuozzo.com>
> Message-Id: <20220929223547.1429580-5-michael.lab...@virtuozzo.com>
> Signed-off-by: Thomas Huth <th...@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
