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

[Akihiko Odaki]

On 2024/08/03 1:52, Michael S. Tsirkin wrote:
> 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_SRIOV_TOTAL_VF gives always a valid value, and it is the only value 
known to valid when PCI_SRIOV_CTRL gets written with a value clearing 
PCI_SRIOV_CTRL_VFE.
> > > >            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.
I masked writes to PCI_SRIOV_NUM_VF when PCI_SRIOV_CTRL_VFE is set in v12.

Regards,
Akihiko Odaki