diff mbox series

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

Message ID 20240214-reuse-v4-8-89ad093a07f4@daynix.com
State New
Headers show
Series hw/pci: SR-IOV related fixes and improvements | expand

Commit Message

Akihiko Odaki Feb. 14, 2024, 5:13 a.m. UTC
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(-)

Comments

Michael S. Tsirkin Feb. 14, 2024, 6:53 a.m. UTC | #1
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
Akihiko Odaki Feb. 14, 2024, 2:32 p.m. UTC | #2
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
>
Michael S. Tsirkin Feb. 14, 2024, 3:51 p.m. UTC | #3
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
> >
Akihiko Odaki Feb. 14, 2024, 4:10 p.m. UTC | #4
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
>>>
>
diff mbox series

Patch

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,