Re: [PATCH v4] hw/pci: enforce use of slot only slot 0 when devices have an upstream PCIE port

[Ani Sinha]

> 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