Re: [PATCH] pcie_sriov: Fix broken MMIO accesses from SR-IOV VFs

["Michael S. Tsirkin" <mst@redhat.com>]

On Tue, Aug 26, 2025 at 03:11:58PM +0000, CLEMENT MATHIEU--DRIF wrote:
> Hi all,
> 
> Kindly ping,  
> do you think we could have a look at this issue after the release of 10.1?
> 
> Thanks  
> \>cmd

indeed.

> On Wed, 2025-08-20 at 08:41 +0000, CLEMENT MATHIEU--DRIF wrote:
> > From: Damien Bergamini <[damien.bergamini@eviden.com](mailto:damien.bergamini@eviden.com)>
> > 
> > Starting with commit cab1398a60eb, SR-IOV VFs are realized as soon as  
> > pcie_sriov_pf_init() is called.  Because pcie_sriov_pf_init() must be  
> > called before pcie_sriov_pf_init_vf_bar(), the VF BARs types won't be  
> > known when the VF realize function calls pcie_sriov_vf_register_bar().
> > 
> > This breaks the memory regions of the VFs (for instance with igbvf):
> > 
> > $ lspci  
> > ...  
> >     Region 0: Memory at 281a00000 (64-bit, prefetchable) [virtual] [size=16K]  
> >     Region 3: Memory at 281a20000 (64-bit, prefetchable) [virtual] [size=16K]
> > 
> > $ info mtree  
> > ...  
> > address-space: pci_bridge_pci_mem  
> >   0000000000000000-ffffffffffffffff (prio 0, i/o): pci_bridge_pci  
> >     0000000081a00000-0000000081a03fff (prio 1, i/o): igbvf-mmio  
> >     0000000081a20000-0000000081a23fff (prio 1, i/o): igbvf-msix
> > 
> > and causes MMIO accesses to fail:
> > 
> >     Invalid write at addr 0x281A01520, size 4, region '(null)', reason: rejected  
> >     Invalid read at addr 0x281A00C40, size 4, region '(null)', reason: rejected
> > 
> > To fix this, a type parameter is added to pcie_sriov_vf_register_bar()  
> > to indicate the BAR type. It should be set to the same value as in the  
> > pcie_sriov_pf_init_vf_bar() call.
> > 
> > Fixes: cab1398a60eb ("pcie_sriov: Reuse SR-IOV VF device instances")  
> > Signed-off-by: Damien Bergamini <[damien.bergamini@eviden.com](mailto:damien.bergamini@eviden.com)>  
> > Signed-off-by: Clement Mathieu--Drif <[clement.mathieu--drif@eviden.com](mailto:clement.mathieu--drif@eviden.com)>  
> > ---  
> >  docs/pcie_sriov.txt         | 2 +-  
> >  hw/net/igbvf.c              | 8 ++++++--  
> >  hw/nvme/ctrl.c              | 4 +++-  
> >  hw/pci/pci.c                | 3 ---  
> >  hw/pci/pcie_sriov.c         | 6 ++----  
> >  include/hw/pci/pcie_sriov.h | 2 +-  
> >  6 files changed, 13 insertions(+), 12 deletions(-)
> > 
> > diff --git a/docs/pcie_sriov.txt b/docs/pcie_sriov.txt  
> > index ab2142807f..77d618b36f 100644  
> > --- a/docs/pcie_sriov.txt  
> > +++ b/docs/pcie_sriov.txt  
> > @@ -83,7 +83,7 @@ setting up a BAR for a VF.  
> >        pcie_ari_init(d, 0x100);  
> >        ...  
> >        memory_region_init(mr, ... )  
> > -      pcie_sriov_vf_register_bar(d, bar_nr, mr);  
> > +      pcie_sriov_vf_register_bar(d, bar_nr, bar_type, mr);  
> >        ...  
> >     }  
> >    
> > diff --git a/hw/net/igbvf.c b/hw/net/igbvf.c  
> > index 31d72c4977..88dd8fb516 100644  
> > --- a/hw/net/igbvf.c  
> > +++ b/hw/net/igbvf.c  
> > @@ -251,10 +251,14 @@ static void igbvf_pci_realize(PCIDevice *dev, Error **errp)  
> >    
> >      memory_region_init_io(&s->mmio, OBJECT(dev), &mmio_ops, s, "igbvf-mmio",  
> >          IGBVF_MMIO_SIZE);  
> > -    pcie_sriov_vf_register_bar(dev, IGBVF_MMIO_BAR_IDX, &s->mmio);  
> > +    pcie_sriov_vf_register_bar(dev, IGBVF_MMIO_BAR_IDX,  
> > +                               PCI_BASE_ADDRESS_MEM_TYPE_64 |  
> > +                               PCI_BASE_ADDRESS_MEM_PREFETCH, &s->mmio);  
> >    
> >      memory_region_init(&s->msix, OBJECT(dev), "igbvf-msix", IGBVF_MSIX_SIZE);  
> > -    pcie_sriov_vf_register_bar(dev, IGBVF_MSIX_BAR_IDX, &s->msix);  
> > +    pcie_sriov_vf_register_bar(dev, IGBVF_MSIX_BAR_IDX,  
> > +                               PCI_BASE_ADDRESS_MEM_TYPE_64 |  
> > +                               PCI_BASE_ADDRESS_MEM_PREFETCH, &s->msix);  
> >    
> >      ret = msix_init(dev, IGBVF_MSIX_VEC_NUM, &s->msix, IGBVF_MSIX_BAR_IDX, 0,  
> >          &s->msix, IGBVF_MSIX_BAR_IDX, 0x2000, 0x70, errp);  
> > diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c  
> > index f5ee6bf260..35a82d2037 100644  
> > --- a/hw/nvme/ctrl.c  
> > +++ b/hw/nvme/ctrl.c  
> > @@ -8709,7 +8709,9 @@ static bool nvme_init_pci(NvmeCtrl *n, PCIDevice *pci_dev, Error **errp)  
> >          memory_region_add_subregion(&n->bar0, 0, &n->iomem);  
> >    
> >          if (pci_is_vf(pci_dev)) {  
> > -            pcie_sriov_vf_register_bar(pci_dev, 0, &n->bar0);  
> > +            pcie_sriov_vf_register_bar(pci_dev, 0,  
> > +                                       PCI_BASE_ADDRESS_SPACE_MEMORY |  
> > +                                       PCI_BASE_ADDRESS_MEM_TYPE_64, &n->bar0);  
> >          } else {  
> >              pci_register_bar(pci_dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY |  
> >                               PCI_BASE_ADDRESS_MEM_TYPE_64, &n->bar0);  
> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c  
> > index c70b5ceeba..4fe2626f9e 100644  
> > --- a/hw/pci/pci.c  
> > +++ b/hw/pci/pci.c  
> > @@ -1490,9 +1490,6 @@ void pci_register_bar(PCIDevice *pci_dev, int region_num,  
> >                          : pci_get_bus(pci_dev)->address_space_mem;  
> >    
> >      if (pci_is_vf(pci_dev)) {  
> > -        PCIDevice *pf = pci_dev->exp.sriov_vf.pf;  
> > -        assert(!pf || type == pf->exp.sriov_pf.vf_bar_type[region_num]);  
> > -  
> >          r->addr = pci_bar_address(pci_dev, region_num, r->type, r->size);  
> >          if (r->addr != PCI_BAR_UNMAPPED) {  
> >              memory_region_add_subregion_overlap(r->address_space,  
> > diff --git a/hw/pci/pcie_sriov.c b/hw/pci/pcie_sriov.c  
> > index 8a4bf0d6f7..eedce6be1d 100644  
> > --- a/hw/pci/pcie_sriov.c  
> > +++ b/hw/pci/pcie_sriov.c  
> > @@ -242,13 +242,11 @@ void pcie_sriov_pf_init_vf_bar(PCIDevice *dev, int region_num,  
> >      dev->exp.sriov_pf.vf_bar_type[region_num] = type;  
> >  }  
> >    
> > +/* `type` must match the type passed to pcie_sriov_pf_init_vf_bar() */  
> >  void pcie_sriov_vf_register_bar(PCIDevice *dev, int region_num,  
> > -                                MemoryRegion *memory)  
> > +                                uint8_t type, MemoryRegion *memory)  
> >  {  
> > -    uint8_t type;  
> > -  
> >      assert(dev->exp.sriov_vf.pf);  
> > -    type = dev->exp.sriov_vf.pf->exp.sriov_pf.vf_bar_type[region_num];  
> >    
> >      return pci_register_bar(dev, region_num, type, memory);  
> >  }  
> > diff --git a/include/hw/pci/pcie_sriov.h b/include/hw/pci/pcie_sriov.h  
> > index aeaa38cf34..b67449f8ba 100644  
> > --- a/include/hw/pci/pcie_sriov.h  
> > +++ b/include/hw/pci/pcie_sriov.h  
> > @@ -39,7 +39,7 @@ void pcie_sriov_pf_init_vf_bar(PCIDevice *dev, int region_num,  
> >    
> >  /* Instantiate a bar for a VF */  
> >  void pcie_sriov_vf_register_bar(PCIDevice *dev, int region_num,  
> > -                                MemoryRegion *memory);  
> > +                                uint8_t type, MemoryRegion *memory);  
> >    
> >  /**  
> >   * pcie_sriov_pf_init_from_user_created_vfs() - Initialize PF with user-created