| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <82416d60-36ec-4aac-b36c-83073b8354bd@gmail.com>
Date: Fri, 11 Oct 2024 11:23:03 +0200
From: Christian König <ckoenig.leichtzumerken@...il.com>
To: Michał Winiarski <michal.winiarski@...el.com>
Cc: linux-pci@...r.kernel.org, intel-xe@...ts.freedesktop.org,
dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
Bjorn Helgaas <bhelgaas@...gle.com>, Krzysztof Wilczyński
<kw@...ux.com>, Rodrigo Vivi <rodrigo.vivi@...el.com>,
Michal Wajdeczko <michal.wajdeczko@...el.com>,
Lucas De Marchi <lucas.demarchi@...el.com>,
Thomas Hellström <thomas.hellstrom@...ux.intel.com>,
Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>,
David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
Matt Roper <matthew.d.roper@...el.com>
Subject: Re: [PATCH v2 2/3] PCI: Allow extending VF BAR within original
resource boundary
Re-sending this as text from my private mail account since the AMD
servers now seem to convert everything to HTML ^^.
Christian.
Am 11.10.24 um 10:57 schrieb Christian König:
> Am 10.10.24 um 10:59 schrieb Michał Winiarski:
>> On Fri, Sep 20, 2024 at 12:07:34PM +0200, Christian König wrote:
>>> Am 20.09.24 um 00:35 schrieb Michał Winiarski:
>>>> [SNIP]
>>>> @@ -487,6 +567,11 @@ static ssize_t sriov_numvfs_store(struct device *dev,
>>>> goto exit;
>>>> }
>>>> + for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) {
>>>> + if (pdev->sriov->rebar_extend[i])
>>>> + pci_iov_resource_do_extend(pdev, i + PCI_IOV_RESOURCES, num_vfs);
>>>> + }
>>>> +
>>> That sounds like a really bad idea to me.
>>>
>>> Basically the suggestion is here that the PCI subsystem should silently
>>> extend and shrink the VF BARs when the number of VFs change?
>> Why do you think it's a bad idea? Everything is under PCI subsystem
>> control and the driver in charge has to explicitly opt-in to this
>> behavior on a per-BAR basis.
>
> And exactly that's a bad idea. The PCI subsystem shouldn't control
> this, the driver should.
>
> At least for some devices we have tons of interactions with ACPI and
> EFI. Only the driver does know for example when platform drivers which
> might be in the way for a resize have been unloaded.
>
> From the past experience BAR resize should only be triggered by the
> driver and never from the PCI subsystem while scanning the bus or
> probing devices.
>
>>> Bjorn has the last word on that but I would say that instead the driver
>>> owning the PCIe device as hypervisor should resize the VF BARs to a desired
>>> size and that in turn restricts the number of VFs you can enable.
>> Then the PCI subsystem would silently change the driver_max_VFs (or new
>> variable, as driver_max_VFs is under PF control, so it's either new var
>> or checking VF BAR size in pci_sriov_set_totalvfs).
>
> Nope, the PCI subsystem should not magically adjust anything.
>
> What should happen instead is that the driver would call
> pci_enable_sriov() with the number of virtual functions to enable and
> the PCI subsystem then validates that number and return -EINVAL or
> -ENOSPC if it won't work.
>
>> It also means that we have to do the maths to calculate the new VF limit
>> in both PCI subsystem and the caller.
>
> Well the point is that those calculations are different.
>
> What the subsystem does is to validate if with the number of requested
> virtual functions the necessary resources will fit into the allocate
> space.
>
> What the driver does previously is to either change the allocate space
> or calculate the other way around and determine the maximum virtual
> functions from the space available.
>
>> We can go this route as well - I just think it's cleaner to keep this
>> all under PCI subsystem control.
>
> I think that would be much cleaner, especially the PCI subsystem
> shouldn't adjust any values given from the driver or even more general
> overrule decisions the driver made.
>
> Instead proper error codes should be returned if some values don't
> make sense or the subsystem isn't able to move around BARs currently
> in use etc...
>
> Regards,
> Christian.
>
>> I'll keep the current behavior in v3, but I'm open to changing it.
>>
>> Thanks,
>> -Michał
>>
>>> Regards,
>>> Christian.
>>>
>>>> ret = pdev->driver->sriov_configure(pdev, num_vfs);
>>>> if (ret < 0)
>>>> goto exit;
>>>> @@ -881,8 +966,13 @@ static int sriov_init(struct pci_dev *dev, int pos)
>>>> static void sriov_release(struct pci_dev *dev)
>>>> {
>>>> + int i;
>>>> +
>>>> BUG_ON(dev->sriov->num_VFs);
>>>> + for (i = 0; i < PCI_SRIOV_NUM_BARS; i++)
>>>> + pci_iov_resource_do_restore(dev, i + PCI_IOV_RESOURCES);
>>>> +
>>>> if (dev != dev->sriov->dev)
>>>> pci_dev_put(dev->sriov->dev);
>>>> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
>>>> index e763b3fd4c7a2..47ed2633232aa 100644
>>>> --- a/drivers/pci/pci.h
>>>> +++ b/drivers/pci/pci.h
>>>> @@ -385,6 +385,7 @@ struct pci_sriov {
>>>> u16 subsystem_vendor; /* VF subsystem vendor */
>>>> u16 subsystem_device; /* VF subsystem device */
>>>> resource_size_t barsz[PCI_SRIOV_NUM_BARS]; /* VF BAR size */
>>>> + bool rebar_extend[PCI_SRIOV_NUM_BARS]; /* Resize VF BAR */
>>>> bool drivers_autoprobe; /* Auto probing of VFs by driver */
>>>> };
>>>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>>>> index 4cf89a4b4cbcf..c007119da7b3d 100644
>>>> --- a/include/linux/pci.h
>>>> +++ b/include/linux/pci.h
>>>> @@ -2364,6 +2364,7 @@ int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs);
>>>> int pci_sriov_get_totalvfs(struct pci_dev *dev);
>>>> int pci_sriov_configure_simple(struct pci_dev *dev, int nr_virtfn);
>>>> resource_size_t pci_iov_resource_size(struct pci_dev *dev, int resno);
>>>> +int pci_iov_resource_extend(struct pci_dev *dev, int resno, bool enable);
>>>> void pci_vf_drivers_autoprobe(struct pci_dev *dev, bool probe);
>>>> /* Arch may override these (weak) */
>>>> @@ -2416,6 +2417,8 @@ static inline int pci_sriov_get_totalvfs(struct pci_dev *dev)
>>>> #define pci_sriov_configure_simple NULL
>>>> static inline resource_size_t pci_iov_resource_size(struct pci_dev *dev, int resno)
>>>> { return 0; }
>>>> +static inline void pci_iov_resource_extend(struct pci_dev *dev, int resno, bool enable)
>>>> +{ return -ENODEV; }
>>>> static inline void pci_vf_drivers_autoprobe(struct pci_dev *dev, bool probe) { }
>>>> #endif
>
Powered by blists - more mailing lists