Re: [PATCH v4 9/9] hw/nvme: Refer to dev->exp.sriov_pf.num_vfs

[Akihiko Odaki]

On 2024/02/14 16:07, Michael S. Tsirkin wrote:
> On Wed, Feb 14, 2024 at 02:13:47PM +0900, Akihiko Odaki wrote:
> > NumVFs may not equal to the current effective number of VFs because VF
> > Enable is cleared, NumVFs is set after VF Enable is set, or NumVFs is
> > greater than TotalVFs.
> >
> > Fixes: 11871f53ef8e ("hw/nvme: Add support for the Virtualization Management 
> > command")
> > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>
> I don't get what this is saying about VF enable.
> This code will not trigger on numVFs write when VF enable is set.
> Generally this commit makes no sense on its own, squash it with
> the pci core change pls.

This code is meant to run when it is clearing VF Enable, and its 
functionality is to change the state of VFs currently enabled so that we 
can disable them.

However, NumVFs does not necessarily represent VFs currently being 
enabled, and have a different value in the case described above. Such 
cases exist even before the earlier patches and this fix is 
independently meaningful.

> > ---
> >   hw/nvme/ctrl.c | 5 ++---
> >   1 file changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> > index f8df622fe590..daedda5d326f 100644
> > --- a/hw/nvme/ctrl.c
> > +++ b/hw/nvme/ctrl.c
> > @@ -8481,7 +8481,7 @@ static void nvme_sriov_pre_write_ctrl(PCIDevice *dev, 
> > uint32_t address,
> >       NvmeSecCtrlEntry *sctrl;
> >       uint16_t sriov_cap = dev->exp.sriov_cap;
> >       uint32_t off = address - sriov_cap;
> > -    int i, num_vfs;
> > +    int i;
> >   
> >       if (!sriov_cap) {
> >           return;
> > @@ -8489,8 +8489,7 @@ static void nvme_sriov_pre_write_ctrl(PCIDevice *dev, 
> > uint32_t address,
> >   
> >       if (range_covers_byte(off, len, PCI_SRIOV_CTRL)) {
> >           if (!(val & PCI_SRIOV_CTRL_VFE)) {
> > -            num_vfs = pci_get_word(dev->config + sriov_cap + PCI_SRIOV_NUM_VF);
> > -            for (i = 0; i < num_vfs; i++) {
> > +            for (i = 0; i < dev->exp.sriov_pf.num_vfs; i++) {
> >                   sctrl = &n->sec_ctrl_list.sec[i];
> >                   nvme_virt_set_state(n, le16_to_cpu(sctrl->scid), false);
> >               }
> >
> > --
> > 2.43.0