On Sat, Aug 03, 2024 at 12:38:10AM +0900, Akihiko Odaki wrote:
> On 2024/08/02 21:58, Michael S. Tsirkin wrote:
> > On Fri, Aug 02, 2024 at 02:17:58PM +0900, Akihiko Odaki wrote:
> > > num_vfs is not migrated so use PCI_SRIOV_CTRL_VFE and PCI_SRIOV_NUM_VF
> > > instead.
> > >
> > > Signed-off-by: Akihiko Odaki <akihiko.od...@daynix.com>
> > > ---
> > > include/hw/pci/pcie_sriov.h | 1 -
> > > hw/pci/pcie_sriov.c | 28 ++++++++++++++++++++--------
> > > hw/pci/trace-events | 2 +-
> > > 3 files changed, 21 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/include/hw/pci/pcie_sriov.h b/include/hw/pci/pcie_sriov.h
> > > index 70649236c18a..5148c5b77dd1 100644
> > > --- a/include/hw/pci/pcie_sriov.h
> > > +++ b/include/hw/pci/pcie_sriov.h
> > > @@ -16,7 +16,6 @@
> > > #include "hw/pci/pci.h"
> > > typedef struct PCIESriovPF {
> > > - uint16_t num_vfs; /* Number of virtual functions created */
> > > uint8_t vf_bar_type[PCI_NUM_REGIONS]; /* Store type for each VF
> > > bar */
> > > PCIDevice **vf; /* Pointer to an array of num_vfs VF devices */
> > > } PCIESriovPF;
> > > diff --git a/hw/pci/pcie_sriov.c b/hw/pci/pcie_sriov.c
> > > index 9bd7f8acc3f4..fae6acea4acb 100644
> > > --- a/hw/pci/pcie_sriov.c
> > > +++ b/hw/pci/pcie_sriov.c
> > > @@ -57,7 +57,6 @@ bool pcie_sriov_pf_init(PCIDevice *dev, uint16_t offset,
> > > pcie_add_capability(dev, PCI_EXT_CAP_ID_SRIOV, 1,
> > > offset, PCI_EXT_CAP_SRIOV_SIZEOF);
> > > dev->exp.sriov_cap = offset;
> > > - dev->exp.sriov_pf.num_vfs = 0;
> > > dev->exp.sriov_pf.vf = NULL;
> > > pci_set_word(cfg + PCI_SRIOV_VF_OFFSET, vf_offset);
> > > @@ -186,6 +185,12 @@ void pcie_sriov_vf_register_bar(PCIDevice *dev, int
> > > region_num,
> > > }
> > > }
> > > +static void clear_ctrl_vfe(PCIDevice *dev)
> > > +{
> > > + uint8_t *ctrl = dev->config + dev->exp.sriov_cap + PCI_SRIOV_CTRL;
> >
> > space here, after definition
> >
> > > + pci_set_word(ctrl, pci_get_word(ctrl) & ~PCI_SRIOV_CTRL_VFE);
> > > +}
> > > +
> >
> > Pls use pci_word_test_and_clear_mask
>
> That sounds good. I'll do so with the next version.
>
> >
> >
> > > static void register_vfs(PCIDevice *dev)
> > > {
> > > uint16_t num_vfs;
> > > @@ -195,6 +200,7 @@ static void register_vfs(PCIDevice *dev)
> > > assert(sriov_cap > 0);
> > > num_vfs = pci_get_word(dev->config + sriov_cap + PCI_SRIOV_NUM_VF);
> > > if (num_vfs > pci_get_word(dev->config + sriov_cap +
> > > PCI_SRIOV_TOTAL_VF)) {
> > > + clear_ctrl_vfe(dev);
> > > return;
> > > }
> > > @@ -203,20 +209,18 @@ static void register_vfs(PCIDevice *dev)
> > > for (i = 0; i < num_vfs; i++) {
> > > pci_set_enabled(dev->exp.sriov_pf.vf[i], true);
> > > }
> > > - dev->exp.sriov_pf.num_vfs = num_vfs;
> > > }
> > > static void unregister_vfs(PCIDevice *dev)
> > > {
> > > - uint16_t num_vfs = dev->exp.sriov_pf.num_vfs;
> > > uint16_t i;
> > > + uint8_t *cfg = dev->config + dev->exp.sriov_cap;
> > > trace_sriov_unregister_vfs(dev->name, PCI_SLOT(dev->devfn),
> > > - PCI_FUNC(dev->devfn), num_vfs);
> > > - for (i = 0; i < num_vfs; i++) {
> > > + PCI_FUNC(dev->devfn));
> > > + for (i = 0; i < pci_get_word(cfg + PCI_SRIOV_TOTAL_VF); i++) {
> >
> > Why PCI_SRIOV_TOTAL_VF not PCI_SRIOV_NUM_VF/pcie_sriov_num_vfs?
>
> Because PCI_SRIOV_NUM_VF is overwritten when unregister_vfs() is called.
maybe this function should get the range of VFs to unregister, then.
> >
> >
> > > pci_set_enabled(dev->exp.sriov_pf.vf[i], false);
> > > }
> > > - dev->exp.sriov_pf.num_vfs = 0;
> > > }
> > > void pcie_sriov_config_write(PCIDevice *dev, uint32_t address,
> > > @@ -242,6 +246,9 @@ void pcie_sriov_config_write(PCIDevice *dev, uint32_t
> > > address,
> > > } else {
> > > unregister_vfs(dev);
> > > }
> > > + } else if (range_covers_byte(off, len, PCI_SRIOV_NUM_VF)) {
> > > + clear_ctrl_vfe(dev);
> > > + unregister_vfs(dev);
> >
> > So any write into PCI_SRIOV_NUM_VF automatically clears VFE?
> >
> > Yes writing into PCI_SRIOV_NUM_VF should not happen when VFE
> > is set, but spec does not say we need to clear it automatically.
> > Why come up with random rules? just don't special case it,
> > whatever happens, let it happen.
> >
> > And what does this change have to do with getting rid of
> > num_vfs?
>
> Keeping VFs working requires to know the number of VFs, but we do no longer
> know it because PCI_SRIOV_NUM_VF is overwritten. This disables all VFs
> instead of trying to keep VFs alive.
>
> Regards,
> Akihiko Odaki
However, we then get into a situation where VFE is set but
PCI_SRIOV_NUM_VF no longer reflects the # of registered VFs.
Given you removed num_vfs which was exactly
the # of registered VFs, it is hard to say if that will lead to
confusion now or later.
--
MST
