Re: [PATCH RFC v2 03/15] vfio/nvgrace-gpu: track GPUs associated with the EGM regions

[Alex Williamson]

On Thu, 26 Feb 2026 14:55:37 +0000
Shameer Kolothum Thodi <skolothumtho@xxxxxxxxxx> wrote:

> > -----Original Message-----
> > From: Ankit Agrawal <ankita@xxxxxxxxxx>
> > Sent: 23 February 2026 15:55
> > To: Ankit Agrawal <ankita@xxxxxxxxxx>; Vikram Sethi <vsethi@xxxxxxxxxx>;
> > Jason Gunthorpe <jgg@xxxxxxxxxx>; Matt Ochs <mochs@xxxxxxxxxx>;
> > jgg@xxxxxxxx; Shameer Kolothum Thodi <skolothumtho@xxxxxxxxxx>;
> > alex@xxxxxxxxxxx
> > Cc: Neo Jia <cjia@xxxxxxxxxx>; Zhi Wang <zhiw@xxxxxxxxxx>; Krishnakant
> > Jaju <kjaju@xxxxxxxxxx>; Yishai Hadas <yishaih@xxxxxxxxxx>;
> > kevin.tian@xxxxxxxxx; kvm@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> > Subject: [PATCH RFC v2 03/15] vfio/nvgrace-gpu: track GPUs associated with
> > the EGM regions
> > 
> > From: Ankit Agrawal <ankita@xxxxxxxxxx>
> > 
> > Grace Blackwell systems could have multiple GPUs on a socket and
> > thus are associated with the corresponding EGM region for that
> > socket. Track the GPUs as a list.
> > 
> > On the device probe, the device pci_dev struct is added to a
> > linked list of the appropriate EGM region.
> > 
> > Similarly on device remove, the pci_dev struct for the GPU
> > is removed from the EGM region.
> > 
> > Since the GPUs on a socket have the same EGM region, they have
> > the have the same set of EGM region information. Skip the EGM
> > region information fetch if already done through a differnt
> > GPU on the same socket.
> > 
> > Signed-off-by: Ankit Agrawal <ankita@xxxxxxxxxx>
> > ---
> >  drivers/vfio/pci/nvgrace-gpu/egm_dev.c | 29 ++++++++++++++++++++
> >  drivers/vfio/pci/nvgrace-gpu/egm_dev.h |  4 +++
> >  drivers/vfio/pci/nvgrace-gpu/main.c    | 37 +++++++++++++++++++++++---
> >  include/linux/nvgrace-egm.h            |  6 +++++
> >  4 files changed, 72 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/vfio/pci/nvgrace-gpu/egm_dev.c
> > b/drivers/vfio/pci/nvgrace-gpu/egm_dev.c
> > index faf658723f7a..0bf95688a486 100644
> > --- a/drivers/vfio/pci/nvgrace-gpu/egm_dev.c
> > +++ b/drivers/vfio/pci/nvgrace-gpu/egm_dev.c
> > @@ -17,6 +17,33 @@ int nvgrace_gpu_has_egm_property(struct pci_dev
> > *pdev, u64 *pegmpxm)
> >  					pegmpxm);
> >  }
> > 
> > +int add_gpu(struct nvgrace_egm_dev *egm_dev, struct pci_dev *pdev)
> > +{
> > +	struct gpu_node *node;
> > +
> > +	node = kzalloc(sizeof(*node), GFP_KERNEL);
> > +	if (!node)
> > +		return -ENOMEM;
> > +
> > +	node->pdev = pdev;
> > +
> > +	list_add_tail(&node->list, &egm_dev->gpus);
> > +
> > +	return 0;
> > +}
> > +
> > +void remove_gpu(struct nvgrace_egm_dev *egm_dev, struct pci_dev *pdev)
> > +{
> > +	struct gpu_node *node, *tmp;
> > +
> > +	list_for_each_entry_safe(node, tmp, &egm_dev->gpus, list) {  
> 
> Looks like this gpu list also will require a lock.

+1

> Can we get rid of this gpu list by having a refcount_t in struct nvgrace_egm_dev?

+1

In this implementation, a reference count seems sufficient and the
egm_dev list could be moved to egm_dev.c, where a get_or_create
function could handle the de-dupe and refcount and a put function could
deference and free.

We'd only need reference to the GPU pci_dev if we needed to invalidate
mappings across a GPU reset, or perhaps if we were exposing multiple
EGM devices per socket, one for each GPU route.

> > +		if (node->pdev == pdev) {
> > +			list_del(&node->list);
> > +			kfree(node);
> > +		}

Also why do we continue searching the list after a match is found?
Thanks,

Alex

> > +	}
> > +}
> > +
> >  static void nvgrace_gpu_release_aux_device(struct device *device)
> >  {
> >  	struct auxiliary_device *aux_dev = container_of(device, struct
> > auxiliary_device, dev);
> > @@ -37,6 +64,8 @@ nvgrace_gpu_create_aux_device(struct pci_dev *pdev,
> > const char *name,
> >  		goto create_err;
> > 
> >  	egm_dev->egmpxm = egmpxm;
> > +	INIT_LIST_HEAD(&egm_dev->gpus);
> > +
> >  	egm_dev->aux_dev.id = egmpxm;
> >  	egm_dev->aux_dev.name = name;
> >  	egm_dev->aux_dev.dev.release = nvgrace_gpu_release_aux_device;
> > diff --git a/drivers/vfio/pci/nvgrace-gpu/egm_dev.h
> > b/drivers/vfio/pci/nvgrace-gpu/egm_dev.h
> > index c00f5288f4e7..1635753c9e50 100644
> > --- a/drivers/vfio/pci/nvgrace-gpu/egm_dev.h
> > +++ b/drivers/vfio/pci/nvgrace-gpu/egm_dev.h
> > @@ -10,6 +10,10 @@
> > 
> >  int nvgrace_gpu_has_egm_property(struct pci_dev *pdev, u64 *pegmpxm);
> > 
> > +int add_gpu(struct nvgrace_egm_dev *egm_dev, struct pci_dev *pdev);
> > +
> > +void remove_gpu(struct nvgrace_egm_dev *egm_dev, struct pci_dev *pdev);
> > +
> >  struct nvgrace_egm_dev *
> >  nvgrace_gpu_create_aux_device(struct pci_dev *pdev, const char *name,
> >  			      u64 egmphys);
> > diff --git a/drivers/vfio/pci/nvgrace-gpu/main.c b/drivers/vfio/pci/nvgrace-
> > gpu/main.c
> > index 23028e6e7192..3dd0c57e5789 100644
> > --- a/drivers/vfio/pci/nvgrace-gpu/main.c
> > +++ b/drivers/vfio/pci/nvgrace-gpu/main.c
> > @@ -77,9 +77,10 @@ static struct list_head egm_dev_list;
> > 
> >  static int nvgrace_gpu_create_egm_aux_device(struct pci_dev *pdev)
> >  {
> > -	struct nvgrace_egm_dev_entry *egm_entry;
> > +	struct nvgrace_egm_dev_entry *egm_entry = NULL;
> >  	u64 egmpxm;
> >  	int ret = 0;
> > +	bool is_new_region = false;
> > 
> >  	/*
> >  	 * EGM is an optional feature enabled in SBIOS. If disabled, there
> > @@ -90,6 +91,19 @@ static int nvgrace_gpu_create_egm_aux_device(struct
> > pci_dev *pdev)
> >  	if (nvgrace_gpu_has_egm_property(pdev, &egmpxm))
> >  		goto exit;
> > 
> > +	list_for_each_entry(egm_entry, &egm_dev_list, list) {
> > +		/*
> > +		 * A system could have multiple GPUs associated with an
> > +		 * EGM region and will have the same set of EGM region
> > +		 * information. Skip the EGM region information fetch if
> > +		 * already done through a differnt GPU on the same socket.
> > +		 */
> > +		if (egm_entry->egm_dev->egmpxm == egmpxm)
> > +			goto add_gpu;
> > +	}
> > +
> > +	is_new_region = true;
> > +
> >  	egm_entry = kzalloc(sizeof(*egm_entry), GFP_KERNEL);
> >  	if (!egm_entry)
> >  		return -ENOMEM;
> > @@ -98,13 +112,24 @@ static int
> > nvgrace_gpu_create_egm_aux_device(struct pci_dev *pdev)
> >  		nvgrace_gpu_create_aux_device(pdev,
> > NVGRACE_EGM_DEV_NAME,
> >  					      egmpxm);
> >  	if (!egm_entry->egm_dev) {
> > -		kvfree(egm_entry);
> >  		ret = -EINVAL;
> > -		goto exit;
> > +		goto free_egm_entry;
> >  	}
> > 
> > -	list_add_tail(&egm_entry->list, &egm_dev_list);
> > +add_gpu:
> > +	ret = add_gpu(egm_entry->egm_dev, pdev);
> > +	if (ret)
> > +		goto free_dev;
> > 
> > +	if (is_new_region)
> > +		list_add_tail(&egm_entry->list, &egm_dev_list);  
> 
> So this is where you address the previous patch comment I suppose...
> If so, need to change the commit description there.
> 
> > +	return 0;
> > +
> > +free_dev:
> > +	if (is_new_region)
> > +		auxiliary_device_destroy(&egm_entry->egm_dev->aux_dev);
> > +free_egm_entry:
> > +	kvfree(egm_entry);  
> 
> Suppose the add_gpu() above fails, then you will end up here with an existing 
> egm_entry which might be in use.
> 
> Thanks,
> Shameer
> 
>