Introduce struct dev_liveupdate and add it to struct device.

Use the new struct to track a device's liveupdate states.
- flags: If not zero, the device participate the live update.

Currently the "flags" has two possible bit:
LU_BUSMASTER: The device is requested for perserving the bus master.
LU_BUSMASTER_BRIDGE: A child device is requested for preserving the
	bus master. The bridge will need to preserve bus master as well.

In the PCI subsystem prepare callback, create the requested device list
 as per the following rules:
- If the device is requested for liveupdate LU_BUSMASTER, then the parent
  bridge will be set LU_BUSMASTER_BRIDGE

The list of PCI root bus and its children bus lists form a tree of all
PCI buses. The tree is walked in postorder traversal, so that the device
on the child bus can mark the parent bridge for LU_BUSMASTER_BRIDGE.

After the postorder traversal of the bus tree then reverse order
enumerates the devices in the bus, all device marks either requested or
depended will be added to the requested device list.

This list of devices will be used in the next change to forward the
liveupdate call back into individual devices.

Note that collect_liveupdate_devices() returns the number of devices it
added to request_devices. This will be used in a subsequent commit so that
the PCI subsystem can calculate what size folio to allocate for its save

Signed-off-by: Chris Li <chrisl@kernel.org>
---
 MAINTAINERS                    |  1 +
 drivers/pci/liveupdate.c       | 80 ++++++++++++++++++++++++++++++++++++++++++
 drivers/pci/pcie/portdrv.c     |  1 +
 drivers/pci/probe.c            |  4 ++-
 include/linux/dev_liveupdate.h | 44 +++++++++++++++++++++++
 include/linux/device.h         | 15 ++++++++
 6 files changed, 144 insertions(+), 1 deletion(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 85749a5da69f88544ccc749e9d723b1b54c0e3b7..1ae3d166cd35ec5c7818f202079ed5d10c09144b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14021,6 +14021,7 @@ F:	Documentation/ABI/testing/sysfs-kernel-liveupdate
 F:	Documentation/admin-guide/liveupdate.rst
 F:	drivers/misc/liveupdate/
 F:	drivers/pci/liveupdate/
+F:	include/linux/dev_liveupdate.h
 F:	include/linux/liveupdate.h
 F:	include/uapi/linux/liveupdate.h
 F:	tools/testing/selftests/liveupdate/
diff --git a/drivers/pci/liveupdate.c b/drivers/pci/liveupdate.c
index 86b4f3a2fb44781c6e323ba029db510450556fa9..e8891844b8194dabf8d1e8e2d74d9c701bd741ca 100644
--- a/drivers/pci/liveupdate.c
+++ b/drivers/pci/liveupdate.c
@@ -6,14 +6,94 @@
  */
 
 #define pr_fmt(fmt) "PCI liveupdate: " fmt
+#define dev_fmt(fmt) "PCI liveupdate: " fmt
 
+#include <linux/types.h>
 #include <linux/liveupdate.h>
+#include "pci.h"
 
 #define PCI_SUBSYSTEM_NAME "pci"
 
+static void stack_push_buses(struct list_head *stack, struct list_head *buses)
+{
+	struct pci_bus *bus;
+
+	list_for_each_entry(bus, buses, node)
+		list_move_tail(&bus->dev.lu.lu_next, stack);
+}
+
+static void liveupdate_add_dev(struct device *dev, struct list_head *head)
+{
+	dev_info(dev, "collect liveupdate device: flags %x\n", dev->lu.flags);
+	list_move_tail(&dev->lu.lu_next, head);
+}
+
+static int collect_bus_devices_reverse(struct pci_bus *bus, struct list_head *head)
+{
+	struct pci_dev *pdev;
+	int count = 0;
+
+	list_for_each_entry_reverse(pdev, &bus->devices, bus_list) {
+		if (pdev->dev.lu.flags & LU_BUSMASTER &&  pdev->dev.parent)
+			pdev->dev.parent->lu.flags |= LU_BUSMASTER_BRIDGE;
+		if (pdev->dev.lu.flags) {
+			liveupdate_add_dev(&pdev->dev, head);
+			count++;
+		}
+	}
+	return count;
+}
+
+static int build_liveupdate_devices(struct list_head *head)
+{
+	LIST_HEAD(bus_stack);
+	int count = 0;
+
+	stack_push_buses(&bus_stack, &pci_root_buses);
+
+	while (!list_empty(&bus_stack)) {
+		struct device *busdev;
+		struct pci_bus *bus;
+
+		busdev = list_last_entry(&bus_stack, struct device, lu.lu_next);
+		bus = to_pci_bus(busdev);
+		if (!busdev->lu.visited && !list_empty(&bus->children)) {
+			stack_push_buses(&bus_stack, &bus->children);
+			busdev->lu.visited = 1;
+			continue;
+		}
+
+		count += collect_bus_devices_reverse(bus, head);
+		busdev->lu.visited = 0;
+		list_del_init(&busdev->lu.lu_next);
+	}
+	return count;
+}
+
+static void cleanup_liveupdate_devices(struct list_head *head)
+{
+	struct device *d, *n;
+
+	list_for_each_entry_safe(d, n, head, lu.lu_next) {
+		d->lu.flags &= ~LU_DEPENDED;
+		list_del_init(&d->lu.lu_next);
+	}
+}
+
 static int pci_liveupdate_prepare(void *arg, u64 *data)
 {
+	LIST_HEAD(requested_devices);
+
 	pr_info("prepare data[%llx]\n", *data);
+
+	pci_lock_rescan_remove();
+	down_write(&pci_bus_sem);
+
+	build_liveupdate_devices(&requested_devices);
+	cleanup_liveupdate_devices(&requested_devices);
+
+	up_write(&pci_bus_sem);
+	pci_unlock_rescan_remove();
 	return 0;
 }
 
diff --git a/drivers/pci/pcie/portdrv.c b/drivers/pci/pcie/portdrv.c
index e8318fd5f6ed537a1b236a3a0f054161d5710abd..0e9ef387182856771d857181d88f376632b46f0d 100644
--- a/drivers/pci/pcie/portdrv.c
+++ b/drivers/pci/pcie/portdrv.c
@@ -304,6 +304,7 @@ static int pcie_device_init(struct pci_dev *pdev, int service, int irq)
 	device = &pcie->device;
 	device->bus = &pcie_port_bus_type;
 	device->release = release_pcie_device;	/* callback to free pcie dev */
+	dev_liveupdate_init(device);
 	dev_set_name(device, "%s:pcie%03x",
 		     pci_name(pdev),
 		     get_descriptor_id(pci_pcie_type(pdev), service));
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 4b8693ec9e4c67fc1655e0057b3b96b4098e6630..dddd7ebc03d1a6e6ee456e0bf02ab9833a819509 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -614,6 +614,7 @@ static struct pci_bus *pci_alloc_bus(struct pci_bus *parent)
 	INIT_LIST_HEAD(&b->devices);
 	INIT_LIST_HEAD(&b->slots);
 	INIT_LIST_HEAD(&b->resources);
+	dev_liveupdate_init(&b->dev);
 	b->max_bus_speed = PCI_SPEED_UNKNOWN;
 	b->cur_bus_speed = PCI_SPEED_UNKNOWN;
 #ifdef CONFIG_PCI_DOMAINS_GENERIC
@@ -1985,6 +1986,7 @@ int pci_setup_device(struct pci_dev *dev)
 	dev->sysdata = dev->bus->sysdata;
 	dev->dev.parent = dev->bus->bridge;
 	dev->dev.bus = &pci_bus_type;
+	dev_liveupdate_init(&dev->dev);
 	dev->hdr_type = hdr_type & 0x7f;
 	dev->multifunction = !!(hdr_type & 0x80);
 	dev->error_state = pci_channel_io_normal;
@@ -3184,7 +3186,7 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
 		return NULL;
 
 	bridge->dev.parent = parent;
-
+	dev_liveupdate_init(&bridge->dev);
 	list_splice_init(resources, &bridge->windows);
 	bridge->sysdata = sysdata;
 	bridge->busnr = bus;
diff --git a/include/linux/dev_liveupdate.h b/include/linux/dev_liveupdate.h
new file mode 100644
index 0000000000000000000000000000000000000000..72297cba08a999e89f7bc0997dabdbe14e0aa12c
--- /dev/null
+++ b/include/linux/dev_liveupdate.h
@@ -0,0 +1,44 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+/*
+ * Copyright (c) 2025, Google LLC.
+ * Pasha Tatashin <pasha.tatashin@soleen.com>
+ * Chris Li <chrisl@kernel.org>
+ */
+#ifndef _LINUX_DEV_LIVEUPDATE_H
+#define _LINUX_DEV_LIVEUPDATE_H
+
+#include <linux/liveupdate.h>
+
+#ifdef CONFIG_LIVEUPDATE
+
+enum liveupdate_flag {
+	LU_BUSMASTER = 1 << 0,
+	LU_BUSMASTER_BRIDGE = 2 << 0,
+};
+
+#define	LU_REQUESTED	(LU_BUSMASTER)
+#define	LU_DEPENDED	(LU_BUSMASTER_BRIDGE)
+
+/**
+ * struct dev_liveupdate - Device state for live update operations
+ * @lu_next:	List head for linking the device into live update
+ *		related lists (e.g., a list of devices participating
+ *		in a live update sequence).
+ * @flags:	Indicate what liveupdate feature does the device
+ *		participtate.
+ * @visited:	Only used by the bus devices when travese the PCI buses
+ *		to build the liveupdate devices list. Set if the child
+ *		buses have been pushed into the pending stack.
+ *
+ * This structure holds the state information required for performing
+ * live update operations on a device. It is embedded within a struct device.
+ */
+struct dev_liveupdate {
+	struct list_head lu_next;
+	enum liveupdate_flag flags;
+	bool visited:1;
+};
+
+#endif /* CONFIG_LIVEUPDATE */
+#endif /* _LINUX_DEV_LIVEUPDATE_H */
diff --git a/include/linux/device.h b/include/linux/device.h
index 4940db137fffff4ceacf819b32433a0f4898b125..e0b35c723239f1254a3b6152f433e0412cd3fb34 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -21,6 +21,7 @@
 #include <linux/lockdep.h>
 #include <linux/compiler.h>
 #include <linux/types.h>
+#include <linux/dev_liveupdate.h>
 #include <linux/mutex.h>
 #include <linux/pm.h>
 #include <linux/atomic.h>
@@ -508,6 +509,7 @@ struct device_physical_location {
  * @pm_domain:	Provide callbacks that are executed during system suspend,
  * 		hibernation, system resume and during runtime PM transitions
  * 		along with subsystem-level and driver-level callbacks.
+ * @lu:		Live update state.
  * @em_pd:	device's energy model performance domain
  * @pins:	For device pin management.
  *		See Documentation/driver-api/pin-control.rst for details.
@@ -603,6 +605,10 @@ struct device {
 	struct dev_pm_info	power;
 	struct dev_pm_domain	*pm_domain;
 
+#ifdef CONFIG_LIVEUPDATE
+	struct dev_liveupdate	lu;
+#endif
+
 #ifdef CONFIG_ENERGY_MODEL
 	struct em_perf_domain	*em_pd;
 #endif
@@ -1168,4 +1174,13 @@ void device_link_wait_removal(void);
 #define MODULE_ALIAS_CHARDEV_MAJOR(major) \
 	MODULE_ALIAS("char-major-" __stringify(major) "-*")
 
+#ifdef CONFIG_LIVEUPDATE
+static inline void dev_liveupdate_init(struct device *dev)
+{
+	INIT_LIST_HEAD(&dev->lu.lu_next);
+}
+#else
+static inline void dev_liveupdate_init(struct device *dev) {}
+#endif
+
 #endif /* _DEVICE_H_ */

-- 
2.51.0.384.g4c02a37b29-goog

On Tue, Sep 16, 2025 at 12:45:10AM -0700, Chris Li wrote:
>  #define pr_fmt(fmt) "PCI liveupdate: " fmt
> +#define dev_fmt(fmt) "PCI liveupdate: " fmt

Please no.  Use the default dev_ formatting so that people can correct
track the devices spitting out messages here.

> +#include <linux/types.h>
>  #include <linux/liveupdate.h>
> +#include "pci.h"
>  
>  #define PCI_SUBSYSTEM_NAME "pci"

I still don't know why this is needed, why?

>  
> +static void stack_push_buses(struct list_head *stack, struct list_head *buses)
> +{
> +	struct pci_bus *bus;
> +
> +	list_for_each_entry(bus, buses, node)
> +		list_move_tail(&bus->dev.lu.lu_next, stack);
> +}
> +
> +static void liveupdate_add_dev(struct device *dev, struct list_head *head)
> +{
> +	dev_info(dev, "collect liveupdate device: flags %x\n", dev->lu.flags);

Debugging code can go away please.

> +	list_move_tail(&dev->lu.lu_next, head);
> +}
> +
> +static int collect_bus_devices_reverse(struct pci_bus *bus, struct list_head *head)
> +{
> +	struct pci_dev *pdev;
> +	int count = 0;
> +
> +	list_for_each_entry_reverse(pdev, &bus->devices, bus_list) {

Why are you allowed to walk the pci bus list here?  Shouldn't there be
some type of core function to do that?

And why in reverse?

> +		if (pdev->dev.lu.flags & LU_BUSMASTER &&  pdev->dev.parent)
> +			pdev->dev.parent->lu.flags |= LU_BUSMASTER_BRIDGE;
> +		if (pdev->dev.lu.flags) {
> +			liveupdate_add_dev(&pdev->dev, head);
> +			count++;

No locking?

> +		}
> +	}
> +	return count;

What prevents this value from changing right after you return it?

> +}
> +
> +static int build_liveupdate_devices(struct list_head *head)
> +{
> +	LIST_HEAD(bus_stack);
> +	int count = 0;
> +
> +	stack_push_buses(&bus_stack, &pci_root_buses);
> +
> +	while (!list_empty(&bus_stack)) {
> +		struct device *busdev;
> +		struct pci_bus *bus;
> +
> +		busdev = list_last_entry(&bus_stack, struct device, lu.lu_next);
> +		bus = to_pci_bus(busdev);
> +		if (!busdev->lu.visited && !list_empty(&bus->children)) {
> +			stack_push_buses(&bus_stack, &bus->children);
> +			busdev->lu.visited = 1;
> +			continue;
> +		}
> +
> +		count += collect_bus_devices_reverse(bus, head);
> +		busdev->lu.visited = 0;
> +		list_del_init(&busdev->lu.lu_next);
> +	}
> +	return count;

A comment here about what you are trying to do with walking the list of
devices.  Somehow.  Are you sure that's right?  It feels backwards, and
the lack of any locking makes me very nervous.  How is this integrating
into the normal driver model lists?

> +}
> +
> +static void cleanup_liveupdate_devices(struct list_head *head)
> +{
> +	struct device *d, *n;
> +
> +	list_for_each_entry_safe(d, n, head, lu.lu_next) {
> +		d->lu.flags &= ~LU_DEPENDED;
> +		list_del_init(&d->lu.lu_next);
> +	}
> +}

What does "cleanup" mean?

> +
>  static int pci_liveupdate_prepare(void *arg, u64 *data)
>  {
> +	LIST_HEAD(requested_devices);
> +
>  	pr_info("prepare data[%llx]\n", *data);

Addresses written to the kernel log?

> +
> +	pci_lock_rescan_remove();
> +	down_write(&pci_bus_sem);
> +
> +	build_liveupdate_devices(&requested_devices);

Ah, you lock here.  Document the heck out of this and put the proper
build macros in there so we know what is going on.

> +	cleanup_liveupdate_devices(&requested_devices);
> +
> +	up_write(&pci_bus_sem);

Why is it a write?  You aren't modifying the list, are you?

> +	pci_unlock_rescan_remove();
>  	return 0;
>  }
>  
> diff --git a/drivers/pci/pcie/portdrv.c b/drivers/pci/pcie/portdrv.c
> index e8318fd5f6ed537a1b236a3a0f054161d5710abd..0e9ef387182856771d857181d88f376632b46f0d 100644
> --- a/drivers/pci/pcie/portdrv.c
> +++ b/drivers/pci/pcie/portdrv.c
> @@ -304,6 +304,7 @@ static int pcie_device_init(struct pci_dev *pdev, int service, int irq)
>  	device = &pcie->device;
>  	device->bus = &pcie_port_bus_type;
>  	device->release = release_pcie_device;	/* callback to free pcie dev */
> +	dev_liveupdate_init(device);

Why here?

>  	dev_set_name(device, "%s:pcie%03x",
>  		     pci_name(pdev),
>  		     get_descriptor_id(pci_pcie_type(pdev), service));
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 4b8693ec9e4c67fc1655e0057b3b96b4098e6630..dddd7ebc03d1a6e6ee456e0bf02ab9833a819509 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -614,6 +614,7 @@ static struct pci_bus *pci_alloc_bus(struct pci_bus *parent)
>  	INIT_LIST_HEAD(&b->devices);
>  	INIT_LIST_HEAD(&b->slots);
>  	INIT_LIST_HEAD(&b->resources);
> +	dev_liveupdate_init(&b->dev);

Same, why here?  Shouldn't the driver core be doing this all for you
automatically?  Are you going to make each bus do this manually?

>  	b->max_bus_speed = PCI_SPEED_UNKNOWN;
>  	b->cur_bus_speed = PCI_SPEED_UNKNOWN;
>  #ifdef CONFIG_PCI_DOMAINS_GENERIC
> @@ -1985,6 +1986,7 @@ int pci_setup_device(struct pci_dev *dev)
>  	dev->sysdata = dev->bus->sysdata;
>  	dev->dev.parent = dev->bus->bridge;
>  	dev->dev.bus = &pci_bus_type;
> +	dev_liveupdate_init(&dev->dev);

Looks like you are :(

Do it in one place please.

>  	dev->hdr_type = hdr_type & 0x7f;
>  	dev->multifunction = !!(hdr_type & 0x80);
>  	dev->error_state = pci_channel_io_normal;
> @@ -3184,7 +3186,7 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
>  		return NULL;
>  
>  	bridge->dev.parent = parent;
> -
> +	dev_liveupdate_init(&bridge->dev);

Again, one place.

>  	list_splice_init(resources, &bridge->windows);
>  	bridge->sysdata = sysdata;
>  	bridge->busnr = bus;
> diff --git a/include/linux/dev_liveupdate.h b/include/linux/dev_liveupdate.h
> new file mode 100644
> index 0000000000000000000000000000000000000000..72297cba08a999e89f7bc0997dabdbe14e0aa12c
> --- /dev/null
> +++ b/include/linux/dev_liveupdate.h
> @@ -0,0 +1,44 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +/*
> + * Copyright (c) 2025, Google LLC.
> + * Pasha Tatashin <pasha.tatashin@soleen.com>
> + * Chris Li <chrisl@kernel.org>
> + */
> +#ifndef _LINUX_DEV_LIVEUPDATE_H
> +#define _LINUX_DEV_LIVEUPDATE_H
> +
> +#include <linux/liveupdate.h>
> +
> +#ifdef CONFIG_LIVEUPDATE
> +
> +enum liveupdate_flag {
> +	LU_BUSMASTER = 1 << 0,
> +	LU_BUSMASTER_BRIDGE = 2 << 0,

BIT() please.

> +};
> +
> +#define	LU_REQUESTED	(LU_BUSMASTER)
> +#define	LU_DEPENDED	(LU_BUSMASTER_BRIDGE)

Why 2 names for the same thing?

> +
> +/**
> + * struct dev_liveupdate - Device state for live update operations
> + * @lu_next:	List head for linking the device into live update
> + *		related lists (e.g., a list of devices participating
> + *		in a live update sequence).
> + * @flags:	Indicate what liveupdate feature does the device
> + *		participtate.
> + * @visited:	Only used by the bus devices when travese the PCI buses
> + *		to build the liveupdate devices list. Set if the child
> + *		buses have been pushed into the pending stack.
> + *
> + * This structure holds the state information required for performing
> + * live update operations on a device. It is embedded within a struct device.
> + */
> +struct dev_liveupdate {
> +	struct list_head lu_next;

Another list?

> +	enum liveupdate_flag flags;
> +	bool visited:1;

You shouldn't need this, you "know" you only touch one device at a time
when walking a bus, don't try to manually keep track of it on your own.

And again, why is the pci core doing this, the driver core should be
doing all of this, PLEASE do not bury driver-model-core-changes down in
a "PCI" patch.  That will make the driver core maintainers very grumpy
when they run across stuff like this (as it did here...)

> +};
> +
> +#endif /* CONFIG_LIVEUPDATE */
> +#endif /* _LINUX_DEV_LIVEUPDATE_H */
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 4940db137fffff4ceacf819b32433a0f4898b125..e0b35c723239f1254a3b6152f433e0412cd3fb34 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -21,6 +21,7 @@
>  #include <linux/lockdep.h>
>  #include <linux/compiler.h>
>  #include <linux/types.h>
> +#include <linux/dev_liveupdate.h>

Look, driver core changes.  Please do this all in stuff that is NOT for
just PCI.


>  #include <linux/mutex.h>
>  #include <linux/pm.h>
>  #include <linux/atomic.h>
> @@ -508,6 +509,7 @@ struct device_physical_location {
>   * @pm_domain:	Provide callbacks that are executed during system suspend,
>   * 		hibernation, system resume and during runtime PM transitions
>   * 		along with subsystem-level and driver-level callbacks.
> + * @lu:		Live update state.

You have more letters, please use them.  "lu" is too short.

>   * @em_pd:	device's energy model performance domain
>   * @pins:	For device pin management.
>   *		See Documentation/driver-api/pin-control.rst for details.
> @@ -603,6 +605,10 @@ struct device {
>  	struct dev_pm_info	power;
>  	struct dev_pm_domain	*pm_domain;
>  
> +#ifdef CONFIG_LIVEUPDATE
> +	struct dev_liveupdate	lu;
> +#endif

Why not a pointer?

> +
>  #ifdef CONFIG_ENERGY_MODEL
>  	struct em_perf_domain	*em_pd;
>  #endif
> @@ -1168,4 +1174,13 @@ void device_link_wait_removal(void);
>  #define MODULE_ALIAS_CHARDEV_MAJOR(major) \
>  	MODULE_ALIAS("char-major-" __stringify(major) "-*")
>  
> +#ifdef CONFIG_LIVEUPDATE
> +static inline void dev_liveupdate_init(struct device *dev)
> +{
> +	INIT_LIST_HEAD(&dev->lu.lu_next);

Why does this have to be in device.h?  The driver core should do this
for you (as I say above).

thanks,

greg k-h

On Tue, Sep 16, 2025 at 12:45:10AM -0700, Chris Li wrote:
>  static int pci_liveupdate_prepare(void *arg, u64 *data)
>  {
> +	LIST_HEAD(requested_devices);
> +
>  	pr_info("prepare data[%llx]\n", *data);
> +
> +	pci_lock_rescan_remove();
> +	down_write(&pci_bus_sem);
> +
> +	build_liveupdate_devices(&requested_devices);
> +	cleanup_liveupdate_devices(&requested_devices);
> +
> +	up_write(&pci_bus_sem);
> +	pci_unlock_rescan_remove();
>  	return 0;
>  }

This doesn't seem conceptually right, PCI should not be preserving
everything. Only devices and their related hierarchy that are opted
into live update by iommufd should be preserved.

Jason

On Mon, Sep 29, 2025 at 10:47 AM Jason Gunthorpe <jgg@ziepe.ca> wrote:
>
> On Tue, Sep 16, 2025 at 12:45:10AM -0700, Chris Li wrote:
> >  static int pci_liveupdate_prepare(void *arg, u64 *data)
> >  {
> > +     LIST_HEAD(requested_devices);
> > +
> >       pr_info("prepare data[%llx]\n", *data);
> > +
> > +     pci_lock_rescan_remove();
> > +     down_write(&pci_bus_sem);
> > +
> > +     build_liveupdate_devices(&requested_devices);
> > +     cleanup_liveupdate_devices(&requested_devices);
> > +
> > +     up_write(&pci_bus_sem);
> > +     pci_unlock_rescan_remove();
> >       return 0;
> >  }
>
> This doesn't seem conceptually right, PCI should not be preserving
> everything. Only devices and their related hierarchy that are opted
> into live update by iommufd should be preserved.

Can you elaborate? This is not preserving everything, for repserveding
bus master, only the device and the parent PCI bridge are added to the
requested_devies list. That is done in the
build_liveupdate_devices(), the device is added to the listhead pass
into the function. So it matches the "their related hierarchy" part.
Can you explain what unnecessary device was preserved in this?

It is not using iommufd, just the dev->lu.flags flags, the flags are
set when VFIO adds a device to VM. Here the PCI LU test driver just
adds the flag in the probe function to simulate that VFIO ioctl.

Iommufd is a separate series, not this one.

Chris


Chris

On Mon, Sep 29, 2025 at 07:13:51PM -0700, Chris Li wrote:
> On Mon, Sep 29, 2025 at 10:47 AM Jason Gunthorpe <jgg@ziepe.ca> wrote:
> >
> > On Tue, Sep 16, 2025 at 12:45:10AM -0700, Chris Li wrote:
> > >  static int pci_liveupdate_prepare(void *arg, u64 *data)
> > >  {
> > > +     LIST_HEAD(requested_devices);
> > > +
> > >       pr_info("prepare data[%llx]\n", *data);
> > > +
> > > +     pci_lock_rescan_remove();
> > > +     down_write(&pci_bus_sem);
> > > +
> > > +     build_liveupdate_devices(&requested_devices);
> > > +     cleanup_liveupdate_devices(&requested_devices);
> > > +
> > > +     up_write(&pci_bus_sem);
> > > +     pci_unlock_rescan_remove();
> > >       return 0;
> > >  }
> >
> > This doesn't seem conceptually right, PCI should not be preserving
> > everything. Only devices and their related hierarchy that are opted
> > into live update by iommufd should be preserved.
> 
> Can you elaborate? This is not preserving everything, for repserveding
> bus master, only the device and the parent PCI bridge are added to the
> requested_devies list. That is done in the
> build_liveupdate_devices(), the device is added to the listhead pass
> into the function. So it matches the "their related hierarchy" part.
> Can you explain what unnecessary device was preserved in this?

I expected an exported function to request a pci device be preserved
and to populate a tracking list linked to a luo session when that
function is called.

This flags and then search over all the buses seems, IDK, strange and
should probably be justified.

Jason