On Tue, Feb 24, 2026 at 09:33:28AM -0800, David Matlack wrote:
> 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.
>
Ack. If we aren't supporting preservation for hot-plug at this point.
Let's mention that somewhere? Maybe just a little comment or the kdoc?
> > > +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.
>
I agree, the snippet was just an example. What I'm trying to say here
is, what if the retval is -ENOMEM / -ENODATA, the existing code will
treat it as a fresh boot because it believes there are no incoming
devices. However, since this was an incoming device which failed to be
retrieved, there's a chance that it's IOMMU mapping was preserved too.
By returning 0, the PCI core will feel free to rebalance bus numbers or
reassign BARs. For instance, if the IOMMU already inherited mappings for
BDF 02:00.0, but the PCI core (due to this masked error) reassigns a
different device to that BDF, we face DMA aliasing or IOMMU faults.
Am I missing some context here?
> > > +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.
>
No strong feelings there, I see both being used in drivers/pci.
> > > @@ -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.
If you have pahole handy, it would be great to see if these can slide
into an existing hole. If not, no big deal for v3.. we can keep it as is
Thanks,
Praan
