RE: [PATCH] PCI/IOV: Decrease VF memory BAR size to save host memory occupied by PTEs

[Deucher, Alexander]

[AMD Official Use Only - General]

> -----Original Message-----
> From: Ma, Rui <Rui.Ma@xxxxxxx>
> Sent: Tuesday, October 11, 2022 7:19 AM
> To: Bjorn Helgaas <helgaas@xxxxxxxxxx>
> Cc: bhelgaas@xxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; Deucher, Alexander
> <Alexander.Deucher@xxxxxxx>
> Subject: RE: [PATCH] PCI/IOV: Decrease VF memory BAR size to save host
> memory occupied by PTEs
> 
> [AMD Official Use Only - General]
> 
> Hi Helgass:
> 	Thank you very much for your suggestions on my patch!
> 
> 	The patch is a device-specific behavior. In our AMD device SR-IOV,
> the actual VF BAR size is dependent on NumVFs too. If only one VF is
> created, the VF BAR size will depend on BAR probing algorithm as described
> in Section 9.3.3.14, but when several VFs created our own driver will
> decrease BAR0 memory size  according to NumVFs. So I want to add this
> quirk to keep Linux code and certain driver code consistent.
> 
> > Except that this doesn't affect the *starting* address of each VF BAR,
> which I guess is what you mean by "BAR memory mapping is always based on
> the initial device physical device."
> Yes we should not change the starting address or the device cannot load
> well.
> 
> > Well, I guess the device still describes its worst-case BAR size; the quirk
> basically just optimizes space usage.  Right?
> Yes.
> 
> > Aren't both virt and phys contiguous and nicely aligned for this case?  It
> seems like the perfect application for huge pages.
> It cannot use huge page though address aligned.

+ KVM, Christian

I think this is just working around a bug in KVM.  This is to avoid wasting huge amounts of memory for page tables due to KVM using 4K pages for some reason.  I think we should figure out why KVM is not using huge pages for this rather than working around this in the PCI layer.

Alex

> 
> >> +		shift = 1;
> >> +		shift = virtfn_get_shift(dev, iov->num_VFs, i);
> >Maybe more of the fiddling could be hidden in the quirk, e.g.,
> >  size = quirk_vf_bar_size(dev, iov->num_VFs, i);
> >  if (!size)
> >    size = pci_iov_resource_size(dev, i + PCI_IOV_RESOURCES);
> If I hide get-shift func in the quirk is it concise to call pci_iov_resource_size()
> in quirk?
> 
> 	And I solved other issues in the patch sent later. Thank you for your
> patience!
> 
> 
> Regards,
> Rui
> 
> -----Original Message-----
> From: Bjorn Helgaas <helgaas@xxxxxxxxxx>
> Sent: Wednesday, September 28, 2022 6:07 AM
> To: Ma, Rui <Rui.Ma@xxxxxxx>
> Cc: bhelgaas@xxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH] PCI/IOV: Decrease VF memory BAR size to save host
> memory occupied by PTEs
> 
> On Mon, Sep 26, 2022 at 04:05:42PM +0800, Rui Ma wrote:
> > In SR_IOV scene, when the device physical space(such as Video RAM)is
> > fixed, as the number of VFs increases, the actual BAR memory space
> > used by each VF decreases.
> 
> s/SR_IOV/SR-IOV/ to match spec usage.
> 
> I think this is device-specific behavior, right?  I don't see anything in the PCIe
> spec about the BAR size being dependent on NumVFs.  If it's device-specific,
> it shouldn't be presented as "for all SR-IOV devices, the actual BAR memory
> space decreases as number of VFs increases."
> 
> > However, the BAR memory mapping is always based on the initial device
> > physical device. So do not map this unneeded memory can save host
> > memory occupied by PTEs. Although each PTE only occupies a few bytes
> > of space on its own, a large number of PTEs can still take up a lot of space.
> 
> So IIUC this is basically a quirk to override the "VF BAR aperture"
> size, which PCIe r6.0, sec 9.2.1.1.1 says is "determined by the usual BAR
> probing algorithm as described in Section 9.3.3.14."
> 
> Except that this doesn't affect the *starting* address of each VF BAR, which I
> guess is what you mean by "BAR memory mapping is always based on the
> initial device physical device."
> 
> Hmm.  This kind of breaks the "plug and play" model of PCI because the
> device is no longer self-describing.  Well, I guess the device still describes its
> worst-case BAR size; the quirk basically just optimizes space usage.  Right?
> 
> It's a shame if we can't reduce PTE usage by using hugeTLB pages for this.
> Aren't both virt and phys contiguous and nicely aligned for this case?  It
> seems like the perfect application for huge pages.
> 
> > Signed-off-by: Rui Ma <Rui.Ma@xxxxxxx>
> > ---
> >  drivers/pci/iov.c    | 14 ++++++++++++--
> >  drivers/pci/pci.h    | 15 +++++++++++++++
> >  drivers/pci/quirks.c | 37 +++++++++++++++++++++++++++++++++++++
> >  3 files changed, 64 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c index
> > 952217572113..6b9f9b6b9be1 100644
> > --- a/drivers/pci/iov.c
> > +++ b/drivers/pci/iov.c
> > @@ -296,6 +296,14 @@ int pci_iov_add_virtfn(struct pci_dev *dev, int id)
> >  	struct pci_sriov *iov = dev->sriov;
> >  	struct pci_bus *bus;
> >
> > +    /*
> > +     * Some SR-IOV device's BAR map range is larger than they can actually
> use.
> > +     * This extra BAR space occupy too much reverse mapping size(physical
> page
> > +     * back to the PTEs). So add a divisor shift parameter to resize the
> > +     * request resource of VF.
> > +     */
> > +	u16 shift = 1;
> > +
> >  	bus = virtfn_add_bus(dev->bus, pci_iov_virtfn_bus(dev, id));
> >  	if (!bus)
> >  		goto failed;
> > @@ -328,8 +336,10 @@ int pci_iov_add_virtfn(struct pci_dev *dev, int id)
> >  		virtfn->resource[i].name = pci_name(virtfn);
> >  		virtfn->resource[i].flags = res->flags;
> >  		size = pci_iov_resource_size(dev, i + PCI_IOV_RESOURCES);
> > +		shift = 1;
> > +		shift = virtfn_get_shift(dev, iov->num_VFs, i);
> 
> Maybe more of the fiddling could be hidden in the quirk, e.g.,
> 
>   size = quirk_vf_bar_size(dev, iov->num_VFs, i);
>   if (!size)
>     size = pci_iov_resource_size(dev, i + PCI_IOV_RESOURCES);
> 
> >  		virtfn->resource[i].start = res->start + size * id;
> > -		virtfn->resource[i].end = virtfn->resource[i].start + size - 1;
> > +		virtfn->resource[i].end = virtfn->resource[i].start + (size >>
> > +(shift - 1)) - 1;
> >  		rc = request_resource(res, &virtfn->resource[i]);
> >  		BUG_ON(rc);
> >  	}
> > @@ -680,12 +690,12 @@ static int sriov_enable(struct pci_dev *dev, int
> nr_virtfn)
> >  	msleep(100);
> >  	pci_cfg_access_unlock(dev);
> >
> > +	iov->num_VFs = nr_virtfn;
> >  	rc = sriov_add_vfs(dev, initial);
> >  	if (rc)
> >  		goto err_pcibios;
> >
> >  	kobject_uevent(&dev->dev.kobj, KOBJ_CHANGE);
> > -	iov->num_VFs = nr_virtfn;
> >
> >  	return 0;
> >
> > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h index
> > 3d60cabde1a1..befc67a280eb 100644
> > --- a/drivers/pci/pci.h
> > +++ b/drivers/pci/pci.h
> > @@ -603,6 +603,21 @@ static inline int pci_dev_specific_reset(struct
> > pci_dev *dev, bool probe)  }  #endif
> >
> > +struct virtfn_get_shift_methods {
> > +	u16 vendor;
> > +	u16 device;
> > +	u16 (*get_shift)(struct pci_dev *dev, u16 arg, int arg2); };
> > +
> > +#ifdef CONFIG_PCI_QUIRKS
> > +u16 virtfn_get_shift(struct pci_dev *dev, u16 arg1, int arg2); #else
> > +static inline u16 virtfn_get_shift(struct pci_dev *dev, u16 arg1, int
> > +arg2) {
> > +	return (u16)1;
> > +}
> > +#endif
> > +
> >  #if defined(CONFIG_PCI_QUIRKS) && defined(CONFIG_ARM64)  int
> > acpi_get_rc_resources(struct device *dev, const char *hid, u16 segment,
> >  			  struct resource *res);
> > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c index
> > da829274fc66..add587919705 100644
> > --- a/drivers/pci/quirks.c
> > +++ b/drivers/pci/quirks.c
> > @@ -4085,6 +4085,43 @@ int pci_dev_specific_reset(struct pci_dev *dev,
> bool probe)
> >  	return -ENOTTY;
> >  }
> >
> > +static u16 divided_by_VF(struct pci_dev *dev, u16 num_VFs, int
> > +bar_num)
> 
> This is clearly ATI specific or at the very least specific to devices that divvy up
> BAR0 in special ways, so the name is a bit too generic.
> 
> > +{
> > +	u16 shift = 1;
> > +
> > +	if (bar_num == 0) {
> > +		while ((1 << shift) <= num_VFs)
> > +			shift += 1;
> > +	}
> > +	pci_info(dev, "BAR %d get shift: %d.\n", bar_num, shift);
> 
> Drop the period at end.  If we're changing the size, I think it would be useful
> to know num_VFs, BAR #, and the new size.  IIUC, "dev" here is the PF, so
> this is the "VF BAR", not the BAR 0 of the PF.
> 
> > +	return shift;
> > +}
> > +
> > +static const struct virtfn_get_shift_methods virtfn_get_shift_methods[] =
> {
> > +	{ PCI_VENDOR_ID_ATI, 0x73a1, divided_by_VF},
> > +	{ 0 }
> > +};
> > +
> > +/*
> > + * Get shift num to calculate SR-IOV device BAR. Sometimes the BAR
> > +size for
> > + * SR-IOV device is too large and we want to calculate the size to
> > +define
> > + * the end of virtfn.
> > + */
> > +u16 virtfn_get_shift(struct pci_dev *dev, u16 arg1, int arg2) {
> > +	const struct virtfn_get_shift_methods *i;
> > +
> > +	for (i = virtfn_get_shift_methods; i->get_shift; i++) {
> > +		if ((i->vendor == dev->vendor ||
> > +		     i->vendor == (u16)PCI_ANY_ID) &&
> > +		    (i->device == dev->device ||
> > +		     i->device == (u16)PCI_ANY_ID))
> > +			return i->get_shift(dev, arg1, arg2);
> > +	}
> > +
> > +	return (u16)1;
> > +}
> > +
> >  static void quirk_dma_func0_alias(struct pci_dev *dev)  {
> >  	if (PCI_FUNC(dev->devfn) != 0)
> > --
> > 2.25.1
> >