1. Patchew
  2. QEMU
  3. [PATCH v4 0/9] hw/pci: SR-IOV related fixes and improvements
  4. View patch

[PATCH v4 8/9] pcie_sriov: Do not reset NumVFs after unregistering VFs

Akihiko Odaki posted 9 patches 1 year, 12 months ago
Maintainers: "Michael S. Tsirkin" <mst@redhat.com>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, Paolo Bonzini <pbonzini@redhat.com>, "Daniel P. Berrangé" <berrange@redhat.com>, Eduardo Habkost <eduardo@habkost.net>, Akihiko Odaki <akihiko.odaki@daynix.com>, Sriram Yagnaraman <sriram.yagnaraman@est.tech>, Jason Wang <jasowang@redhat.com>, Keith Busch <kbusch@kernel.org>, Klaus Jensen <its@irrelevant.dk>, Alex Williamson <alex.williamson@redhat.com>, "Cédric Le Goater" <clg@redhat.com>
There is a newer version of this series
[PATCH v4 8/9] pcie_sriov: Do not reset NumVFs after unregistering VFs
Posted by Akihiko Odaki 1 year, 12 months ago 
I couldn't find such a behavior specified.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 hw/pci/pcie_sriov.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/pci/pcie_sriov.c b/hw/pci/pcie_sriov.c
index 9d668b8d6c17..410bc090fc58 100644
--- a/hw/pci/pcie_sriov.c
+++ b/hw/pci/pcie_sriov.c
@@ -209,7 +209,6 @@ static void unregister_vfs(PCIDevice *dev)
         pci_set_enabled(dev->exp.sriov_pf.vf[i], false);
     }
     dev->exp.sriov_pf.num_vfs = 0;
-    pci_set_word(dev->config + dev->exp.sriov_cap + PCI_SRIOV_NUM_VF, 0);
 }
 
 void pcie_sriov_config_write(PCIDevice *dev, uint32_t address,

-- 
2.43.0
Re: [PATCH v4 8/9] pcie_sriov: Do not reset NumVFs after unregistering VFs
Posted by Michael S. Tsirkin 1 year, 12 months ago 
On Wed, Feb 14, 2024 at 02:13:46PM +0900, Akihiko Odaki wrote:
> I couldn't find such a behavior specified.

Is it fixing a bug or just removing unnecessary code?
Is this guest visible at all?

> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
>  hw/pci/pcie_sriov.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/hw/pci/pcie_sriov.c b/hw/pci/pcie_sriov.c
> index 9d668b8d6c17..410bc090fc58 100644
> --- a/hw/pci/pcie_sriov.c
> +++ b/hw/pci/pcie_sriov.c
> @@ -209,7 +209,6 @@ static void unregister_vfs(PCIDevice *dev)
>          pci_set_enabled(dev->exp.sriov_pf.vf[i], false);
>      }
>      dev->exp.sriov_pf.num_vfs = 0;
> -    pci_set_word(dev->config + dev->exp.sriov_cap + PCI_SRIOV_NUM_VF, 0);
>  }
>  
>  void pcie_sriov_config_write(PCIDevice *dev, uint32_t address,
> 
> -- 
> 2.43.0
Re: [PATCH v4 8/9] pcie_sriov: Do not reset NumVFs after unregistering VFs
Posted by Akihiko Odaki 1 year, 12 months ago 
On 2024/02/14 15:53, Michael S. Tsirkin wrote:
> On Wed, Feb 14, 2024 at 02:13:46PM +0900, Akihiko Odaki wrote:
>> I couldn't find such a behavior specified.
> 
> Is it fixing a bug or just removing unnecessary code?
> Is this guest visible at all?

My intention is just to remove unnecessary code, but it is 
guest-visible. The original behavior causes a problem and it should be 
considered as a bug fix if a guest expects VFs can be restored by 
setting VF Enable after clearing it, but I don't know such an example.

> 
>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>> ---
>>   hw/pci/pcie_sriov.c | 1 -
>>   1 file changed, 1 deletion(-)
>>
>> diff --git a/hw/pci/pcie_sriov.c b/hw/pci/pcie_sriov.c
>> index 9d668b8d6c17..410bc090fc58 100644
>> --- a/hw/pci/pcie_sriov.c
>> +++ b/hw/pci/pcie_sriov.c
>> @@ -209,7 +209,6 @@ static void unregister_vfs(PCIDevice *dev)
>>           pci_set_enabled(dev->exp.sriov_pf.vf[i], false);
>>       }
>>       dev->exp.sriov_pf.num_vfs = 0;
>> -    pci_set_word(dev->config + dev->exp.sriov_cap + PCI_SRIOV_NUM_VF, 0);
>>   }
>>   
>>   void pcie_sriov_config_write(PCIDevice *dev, uint32_t address,
>>
>> -- 
>> 2.43.0
>
Re: [PATCH v4 8/9] pcie_sriov: Do not reset NumVFs after unregistering VFs
Posted by Michael S. Tsirkin 1 year, 12 months ago 
On Wed, Feb 14, 2024 at 11:32:11PM +0900, Akihiko Odaki wrote:
> On 2024/02/14 15:53, Michael S. Tsirkin wrote:
> > On Wed, Feb 14, 2024 at 02:13:46PM +0900, Akihiko Odaki wrote:
> > > I couldn't find such a behavior specified.
> > 
> > Is it fixing a bug or just removing unnecessary code?
> > Is this guest visible at all?
> 
> My intention is just to remove unnecessary code, but it is guest-visible.
> The original behavior causes a problem and it should be considered as a bug
> fix if a guest expects VFs can be restored by setting VF Enable after
> clearing it, but I don't know such an example.

Ah ok.  So:

	According to the spec, PCI_SRIOV_NUM_VF isn't reset when
	VFs are disabled. Clearing it is guest visible and out of spec,
	even though guests mostly don't rely on this value being
	preserved, so we never noticed.

I wonder however what happens on reset.
How come we don't have a reset callback for sriov cap?
I suspect this hack serves as a work around to avoid leaking
this register, and we really should fix that too here?

> > 
> > > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> > > ---
> > >   hw/pci/pcie_sriov.c | 1 -
> > >   1 file changed, 1 deletion(-)
> > > 
> > > diff --git a/hw/pci/pcie_sriov.c b/hw/pci/pcie_sriov.c
> > > index 9d668b8d6c17..410bc090fc58 100644
> > > --- a/hw/pci/pcie_sriov.c
> > > +++ b/hw/pci/pcie_sriov.c
> > > @@ -209,7 +209,6 @@ static void unregister_vfs(PCIDevice *dev)
> > >           pci_set_enabled(dev->exp.sriov_pf.vf[i], false);
> > >       }
> > >       dev->exp.sriov_pf.num_vfs = 0;
> > > -    pci_set_word(dev->config + dev->exp.sriov_cap + PCI_SRIOV_NUM_VF, 0);
> > >   }
> > >   void pcie_sriov_config_write(PCIDevice *dev, uint32_t address,
> > > 
> > > -- 
> > > 2.43.0
> >
Re: [PATCH v4 8/9] pcie_sriov: Do not reset NumVFs after unregistering VFs
Posted by Akihiko Odaki 1 year, 12 months ago 
On 2024/02/15 0:51, Michael S. Tsirkin wrote:
> On Wed, Feb 14, 2024 at 11:32:11PM +0900, Akihiko Odaki wrote:
>> On 2024/02/14 15:53, Michael S. Tsirkin wrote:
>>> On Wed, Feb 14, 2024 at 02:13:46PM +0900, Akihiko Odaki wrote:
>>>> I couldn't find such a behavior specified.
>>>
>>> Is it fixing a bug or just removing unnecessary code?
>>> Is this guest visible at all?
>>
>> My intention is just to remove unnecessary code, but it is guest-visible.
>> The original behavior causes a problem and it should be considered as a bug
>> fix if a guest expects VFs can be restored by setting VF Enable after
>> clearing it, but I don't know such an example.
> 
> Ah ok.  So:
> 
> 	According to the spec, PCI_SRIOV_NUM_VF isn't reset when
> 	VFs are disabled. Clearing it is guest visible and out of spec,
> 	even though guests mostly don't rely on this value being
> 	preserved, so we never noticed.
> 
> I wonder however what happens on reset.
> How come we don't have a reset callback for sriov cap?
> I suspect this hack serves as a work around to avoid leaking
> this register, and we really should fix that too here?

pcie_sriov_pf_disable_vfs() is meant to serve for that purpose, but it 
just disable VFs and does not update registers. I'll change it to update 
registers and rename it to pcie_sriov_pf_reset().

> 
>>>
>>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>>>> ---
>>>>    hw/pci/pcie_sriov.c | 1 -
>>>>    1 file changed, 1 deletion(-)
>>>>
>>>> diff --git a/hw/pci/pcie_sriov.c b/hw/pci/pcie_sriov.c
>>>> index 9d668b8d6c17..410bc090fc58 100644
>>>> --- a/hw/pci/pcie_sriov.c
>>>> +++ b/hw/pci/pcie_sriov.c
>>>> @@ -209,7 +209,6 @@ static void unregister_vfs(PCIDevice *dev)
>>>>            pci_set_enabled(dev->exp.sriov_pf.vf[i], false);
>>>>        }
>>>>        dev->exp.sriov_pf.num_vfs = 0;
>>>> -    pci_set_word(dev->config + dev->exp.sriov_cap + PCI_SRIOV_NUM_VF, 0);
>>>>    }
>>>>    void pcie_sriov_config_write(PCIDevice *dev, uint32_t address,
>>>>
>>>> -- 
>>>> 2.43.0
>>>
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.