| Message ID | 20240214-reuse-v4-8-89ad093a07f4@daynix.com |
|---|---|
| State | New |
| Headers | show |
| Series | hw/pci: SR-IOV related fixes and improvements | expand |
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
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 >
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 > >
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 --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,
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(-)