On Tue, Feb 24, 2026 at 1:18 AM Pranjal Shrivastava <[email protected]> wrote:
> On Thu, Jan 29, 2026 at 09:24:49PM +0000, David Matlack wrote:
> > + * Copyright (c) 2025, Google LLC.
>
> Nit: Should these be 2026 now?
Yes! Thanks for catching that.
> > +int pci_liveupdate_outgoing_preserve(struct pci_dev *dev)
> > +{
> > + struct pci_dev_ser new = INIT_PCI_DEV_SER(dev);
> > + struct pci_ser *ser;
> > + int i, ret;
> > +
> > + /* Preserving VFs is not supported yet. */
> > + if (dev->is_virtfn)
> > + return -EINVAL;
> > +
> > + guard(mutex)(&pci_flb_outgoing_lock);
> > +
> > + if (dev->liveupdate_outgoing)
> > + return -EBUSY;
> > +
> > + ret = liveupdate_flb_get_outgoing(&pci_liveupdate_flb, (void **)&ser);
> > + if (ret)
> > + return ret;
> > +
> > + if (ser->nr_devices == ser->max_nr_devices)
> > + return -E2BIG;
>
> I'm wondering how (or if) this handles hot-plugged devices?
> max_nr_devices is calculated based on for_each_pci_dev at the time of
> the first preservation.. what happens if a device is hotplugged after
> the first device is preserved but before the second one is, does
> max_nr_devices become stale? Since ser->max_nr_devices will not reflect
> the actual possible device count, potentially leading to an unnecessary
> -E2BIG failure?
Yes, it's possible to run out space to preserve devices if devices are
hot-plugged and then preserved. But I think it's better to defer
handling such a use-case exists (unless you see an obvious simple
solution). So far I am not seeing preserving hot-plugged devices
across Live Update as a high priority use-case to support.
> > +u32 pci_liveupdate_incoming_nr_devices(void)
> > +{
> > + struct pci_ser *ser;
> > + int ret;
> > +
> > + ret = liveupdate_flb_get_incoming(&pci_liveupdate_flb, (void **)&ser);
> > + if (ret)
> > + return 0;
>
> Masking this error looks troubled, in the following patch, I see that
> the retval 0 is treated as a fresh boot, but the IOMMU mappings for that
> BDF might still be preserved? Which could lead to DMA aliasing issues,
> without a hint of what happened since we don't even log anything.
All fo the non-0 errors indicate there are 0 incoming devices at the
time of the call, so I think returning 0 is appropriate.
- EOPNOTSUPP: Live Update is not enabled.
- ENODATA: Live Update is finished (all incoming devices have been restored).
- ENOTENT: No PCI data was preserved across the Live Update.
None of these cover the case where an IOMMU mapping for BDF X is
preserved, but device X is not preserved. This is a case we should
handle in some way... but here is not that place.
>
> Maybe we could have something like the following:
>
> int pci_liveupdate_incoming_nr_devices(void)
> {
> struct pci_ser *ser;
> int ret;
>
> ret = liveupdate_flb_get_incoming(&pci_liveupdate_flb, (void **)&ser);
> if (ret) {
> if (ret != -ENOENT)
> pr_warn("PCI: Failed to retrieve preservation list:
> %d\n", ret);
This would cause this warning to get printed if Live Update was
disabled, or if no PCI devices were preserved. But both of those are
not error scenarios.
> > +void pci_liveupdate_setup_device(struct pci_dev *dev)
> > +{
> > + struct pci_ser *ser;
> > + int ret;
> > +
> > + ret = liveupdate_flb_get_incoming(&pci_liveupdate_flb, (void **)&ser);
> > + if (ret)
> > + return;
>
> We should log something here either at info / debug level since the
> error isn't bubbled up and the luo_core doesn't scream about it either.
Any error from liveupdate_flb_get_incoming() simply means there are no
incoming devices. So I don't think there's any error to report in
dmesg.
> > + dev->liveupdate_incoming = !!pci_ser_find(ser, dev);
>
> This feels a little hacky, shall we go for something like:
>
> dev->liveupdate_incoming = (pci_ser_find(ser, dev) != NULL); ?
In my experience in the kernel (mostly from KVM), explicity comparison
to NULL is less preferred to treating a pointer as a boolean. But I'm
ok with following whatever is the locally preferred style for this
kind of check.
> > @@ -582,6 +583,10 @@ struct pci_dev {
> > u8 tph_mode; /* TPH mode */
> > u8 tph_req_type; /* TPH requester type */
> > #endif
> > +#ifdef CONFIG_LIVEUPDATE
> > + unsigned int liveupdate_incoming:1; /* Preserved by previous
> > kernel */
> > + unsigned int liveupdate_outgoing:1; /* Preserved for next kernel
> > */
> > +#endif
> > };
>
> This would start another anon bitfield container, should we move this
> above within the existing bitfield? If we've run pahole and found this
> to be better, then this should be fine.
Yeah I simply appended these new fields to the very end of the struct.
If we care about optimizing the packing of struct pci_dev I can find a
better place to put it.
