Re: [PATCH for-9.2 v11 08/11] pcie_sriov: Remove num_vfs from PCIESriovPF

[Michael S. Tsirkin]

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.odaki@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