Re: [PATCH v6 05/27] PCI: liveupdate: Add helpers to preserve struct pci_saved_state

[Bjorn Helgaas <helgaas@kernel.org>]

On Mon, Nov 02, 2026 at 07:36:34AM -0800, Vipin Sharma wrote:
> Drivers that keep a device running across Live Update need to hand the
> state they captured with pci_store_saved_state() to the next kernel,
> since the next kernel never sees the device in that state. VFIO, for
> example, captures it at first open and uses it to restore the device
> when the last user closes it.
> 
> struct pci_saved_state is private to the PCI core and its in-memory
> layout is not a stable ABI. Add helpers to serialize it into a
> caller-provided buffer and rebuild it in the next kernel with a stable
> struct pci_saved_state_ser ABI. Using caller-provided buffers allows
> drivers to embed the serialized saved state directly in their own
> KHO-preserved data with a single allocation and lifetime.
> 
> Validate the serialized buffer and verify that the saved standard
> configuration header matches the live device before rebuilding the
> state: the vendor and device IDs must match, only Type 0 (Normal)
> headers are accepted, and BAR0-5 and the expansion ROM BAR address must
> equal the current hardware values. This ensures that restoring the
> saved state cannot write stale BAR addresses if resources were
> reassigned in the new kernel. Only the address bits of the ROM BAR are
> compared, since its enable and validation status bits may legitimately
> differ.
> 
> Signed-off-by: Vipin Sharma <vipinsh@google.com>

Acked-by: Bjorn Helgaas <bhelgaas@google.com>

A few nits below.

> +static const struct pci_cap_saved_data *pci_saved_cap_next(const struct pci_cap_saved_data *cap)
> +{
> +	return (const void *)cap + sizeof(*cap) + cap->size;
> +}
> +
> +/**
> + * pci_liveupdate_saved_state_ser_size() - Size needed to serialize a saved state

Wrap to fit in 80 columns (75 or so is even nicer because it's a
little more relaxed to read and typo fixes are less likely to rewrap).

> + * @state: Saved state returned by pci_store_saved_state(), or NULL.
> + *
> + * Return: Number of bytes needed by pci_liveupdate_serialize_saved_state(), or
> + * 0 if @state is NULL.
> + */
> +size_t pci_liveupdate_saved_state_ser_size(const struct pci_saved_state *state)

I think pci_liveupdate_saved_state_ser_size() is connected with
pci_liveupdate_serialize_saved_state() and
pci_liveupdate_deserialize_saved_state().

Would be nice if that were obvious from the names.

They're also pretty long, which makes prototypes and uses very likely
to wrap in awkward spots.

> + * struct pci_cap_saved_data_ser - A saved capability in a struct pci_saved_state_ser
> + * @cap_nr: Capability ID (PCI_CAP_ID_* or PCI_EXT_CAP_ID_*).
> + * @extended: 1 if @cap_nr is an extended capability ID, 0 otherwise.
> + * @size: Size of @data in bytes, never 0.
> + * @data: The saved registers.
> + */
> +struct pci_cap_saved_data_ser {
> +	u16 cap_nr;

Maybe "cap_id" to avoid confusion with "nr_caps" or indexes into
"caps[]" below?

> +	u16 extended;
> +	u32 size;
> +	u8 data[];
> +} __packed;
> +
> +/**
> + * struct pci_saved_state_ser - Serialized struct pci_saved_state
> + * @size: Size in bytes of this struct, including all entries in @caps.
> + * @flags: Features a kernel must support to restore the state. None defined.
> + * @nr_caps: Number of entries in @caps.
> + * @config_space: The saved standard configuration header.
> + * @caps: @nr_caps struct pci_cap_saved_data_ser entries.
> + */
> +struct pci_saved_state_ser {
> +	u32 size;
> +	u32 flags;
> +	u32 nr_caps;
> +	u32 config_space[16];
> +	u8 caps[];

I think it would be helpful if the names of "config_space[]" and
"caps[]" hinted at the connection between them (both are in config
space).  "config_space[]" by itself suggests it's complete, which it's
not, so maybe there's a place for "hdr" or similar in it.