lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <6cvrmn5try63xuv2xf3eqhwugvwavibxu6hec5z3ab4ni3oava@kzkumf5xv5li>
Date: Wed, 2 Apr 2025 12:23:53 +0200
From: Michał Winiarski <michal.winiarski@...el.com>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
CC: <linux-pci@...r.kernel.org>, <intel-xe@...ts.freedesktop.org>,
	<dri-devel@...ts.freedesktop.org>, LKML <linux-kernel@...r.kernel.org>,
	"Bjorn Helgaas" <bhelgaas@...gle.com>, Christian König
	<christian.koenig@....com>, Krzysztof Wilczyński
	<kw@...ux.com>, Rodrigo Vivi <rodrigo.vivi@...el.com>, Michal Wajdeczko
	<michal.wajdeczko@...el.com>, Lucas De Marchi <lucas.demarchi@...el.com>,
	Thomas Hellström <thomas.hellstrom@...ux.intel.com>,
	"Maarten Lankhorst" <maarten.lankhorst@...ux.intel.com>, Maxime Ripard
	<mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>, David Airlie
	<airlied@...il.com>, Simona Vetter <simona@...ll.ch>, Matt Roper
	<matthew.d.roper@...el.com>
Subject: Re: [PATCH v6 2/6] PCI: Add a helper to convert between VF BAR
 number and IOV resource

On Wed, Mar 26, 2025 at 04:46:19PM +0200, Ilpo Järvinen wrote:
> On Thu, 20 Mar 2025, Michał Winiarski wrote:
> 
> > There are multiple places where conversions between IOV resources and
> > corresponding VF BAR numbers are done.
> > 
> > Extract the logic to pci_resource_num_from_vf_bar() and
> > pci_resource_num_to_vf_bar() helpers.
> > 
> > Suggested-by: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
> > Signed-off-by: Michał Winiarski <michal.winiarski@...el.com>
> > Acked-by: Christian König <christian.koenig@....com>
> > ---
> >  drivers/pci/iov.c       | 22 ++++++++++++----------
> >  drivers/pci/pci.h       | 19 +++++++++++++++++++
> >  drivers/pci/setup-bus.c |  3 ++-
> >  3 files changed, 33 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> > index bf95387993cd5..985ea11339c45 100644
> > --- a/drivers/pci/iov.c
> > +++ b/drivers/pci/iov.c
> > @@ -151,7 +151,7 @@ resource_size_t pci_iov_resource_size(struct pci_dev *dev, int resno)
> >  	if (!dev->is_physfn)
> >  		return 0;
> >  
> > -	return dev->sriov->barsz[resno - PCI_IOV_RESOURCES];
> > +	return dev->sriov->barsz[pci_resource_num_to_vf_bar(resno)];
> >  }
> >  
> >  static void pci_read_vf_config_common(struct pci_dev *virtfn)
> > @@ -322,12 +322,13 @@ int pci_iov_add_virtfn(struct pci_dev *dev, int id)
> >  	virtfn->multifunction = 0;
> >  
> >  	for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) {
> > -		res = &dev->resource[i + PCI_IOV_RESOURCES];
> > +		res = &dev->resource[pci_resource_num_from_vf_bar(i)];
> >  		if (!res->parent)
> >  			continue;
> >  		virtfn->resource[i].name = pci_name(virtfn);
> >  		virtfn->resource[i].flags = res->flags;
> > -		size = pci_iov_resource_size(dev, i + PCI_IOV_RESOURCES);
> > +		size = pci_iov_resource_size(dev,
> > +					     pci_resource_num_from_vf_bar(i));
> >  		resource_set_range(&virtfn->resource[i],
> >  				   res->start + size * id, size);
> >  		rc = request_resource(res, &virtfn->resource[i]);
> > @@ -624,8 +625,8 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)
> >  
> >  	nres = 0;
> >  	for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) {
> > -		bars |= (1 << (i + PCI_IOV_RESOURCES));
> > -		res = &dev->resource[i + PCI_IOV_RESOURCES];
> > +		bars |= (1 << pci_resource_num_from_vf_bar(i));
> > +		res = &dev->resource[pci_resource_num_from_vf_bar(i)];
> >  		if (res->parent)
> >  			nres++;
> >  	}
> > @@ -791,8 +792,9 @@ static int sriov_init(struct pci_dev *dev, int pos)
> >  
> >  	nres = 0;
> >  	for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) {
> > -		res = &dev->resource[i + PCI_IOV_RESOURCES];
> > -		res_name = pci_resource_name(dev, i + PCI_IOV_RESOURCES);
> > +		res = &dev->resource[pci_resource_num_from_vf_bar(i)];
> > +		res_name = pci_resource_name(dev,
> > +					     pci_resource_num_from_vf_bar(i));
> 
> All these get easier to read if you add (same comment for the cases 
> above):
> 
> 		int idx = pci_resource_num_from_vf_bar(i);

Ok.

> 
> >  
> >  		/*
> >  		 * If it is already FIXED, don't change it, something
> > @@ -851,7 +853,7 @@ static int sriov_init(struct pci_dev *dev, int pos)
> >  	dev->is_physfn = 0;
> >  failed:
> >  	for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) {
> > -		res = &dev->resource[i + PCI_IOV_RESOURCES];
> > +		res = &dev->resource[pci_resource_num_from_vf_bar(i)];
> >  		res->flags = 0;
> >  	}
> >  
> > @@ -913,7 +915,7 @@ static void sriov_restore_state(struct pci_dev *dev)
> >  	pci_write_config_word(dev, iov->pos + PCI_SRIOV_CTRL, ctrl);
> >  
> >  	for (i = 0; i < PCI_SRIOV_NUM_BARS; i++)
> > -		pci_update_resource(dev, i + PCI_IOV_RESOURCES);
> > +		pci_update_resource(dev, pci_resource_num_from_vf_bar(i));
> >  
> >  	pci_write_config_dword(dev, iov->pos + PCI_SRIOV_SYS_PGSIZE, iov->pgsz);
> >  	pci_iov_set_numvfs(dev, iov->num_VFs);
> > @@ -979,7 +981,7 @@ void pci_iov_update_resource(struct pci_dev *dev, int resno)
> >  {
> >  	struct pci_sriov *iov = dev->is_physfn ? dev->sriov : NULL;
> >  	struct resource *res = pci_resource_n(dev, resno);
> > -	int vf_bar = resno - PCI_IOV_RESOURCES;
> > +	int vf_bar = pci_resource_num_to_vf_bar(resno);
> >  	struct pci_bus_region region;
> >  	u16 cmd;
> >  	u32 new;
> > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> > index adc54bb2c8b34..f44840ee3c327 100644
> > --- a/drivers/pci/pci.h
> > +++ b/drivers/pci/pci.h
> > @@ -693,6 +693,15 @@ static inline bool pci_resource_is_iov(int resno)
> >  {
> >  	return resno >= PCI_IOV_RESOURCES && resno <= PCI_IOV_RESOURCE_END;
> >  }
> > +static inline int pci_resource_num_from_vf_bar(int resno)
> > +{
> > +	return resno + PCI_IOV_RESOURCES;
> > +}
> > +
> > +static inline int pci_resource_num_to_vf_bar(int resno)
> > +{
> > +	return resno - PCI_IOV_RESOURCES;
> > +}
> >  extern const struct attribute_group sriov_pf_dev_attr_group;
> >  extern const struct attribute_group sriov_vf_dev_attr_group;
> >  #else
> > @@ -717,6 +726,16 @@ static inline bool pci_resource_is_iov(int resno)
> >  {
> >  	return false;
> >  }
> > +static inline int pci_resource_num_from_vf_bar(int resno)
> > +{
> > +	WARN_ON_ONCE(1);
> > +	return -ENODEV;
> > +}
> > +static inline int pci_resource_num_to_vf_bar(int resno)
> > +{
> > +	WARN_ON_ONCE(1);
> > +	return -ENODEV;
> > +}
> >  #endif /* CONFIG_PCI_IOV */
> >  
> >  #ifdef CONFIG_PCIE_TPH
> > diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> > index 54d6f4fa3ce16..55e91ba1e74a2 100644
> > --- a/drivers/pci/setup-bus.c
> > +++ b/drivers/pci/setup-bus.c
> > @@ -1885,7 +1885,8 @@ static int iov_resources_unassigned(struct pci_dev *dev, void *data)
> >  	bool *unassigned = data;
> >  
> >  	for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) {
> > -		struct resource *r = &dev->resource[i + PCI_IOV_RESOURCES];
> > +		struct resource *r =
> > +			&dev->resource[pci_resource_num_from_vf_bar(i)];
> 
> I'd add int idx here as well.

Ok, makes sense, I'll use the similar approach in the following patches
as well.

Thanks,
-Michał

> 
> >  		struct pci_bus_region region;
> >  
> >  		/* Not assigned or rejected by kernel? */
> > 
> 
> -- 
>  i.


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ