Re: [PATCH v2 2/9] docs/specs, tools: define the PAGE_DATA flags field

[Teddy Astie <teddy.astie@xxxxxxxxxx>]

Le 20/10/2026 à 22:05, Marcus Granado a écrit :
> The 32-bit field after count in a PAGE_DATA record is reserved, and
> libxenguest and the Python stream validator reject a record which sets
> it.  The record's page_data is page_size bytes of uncompressed
> contents per present page, and nothing in the record can say that it
> is encoded otherwise, so pages cannot be sent compressed.
>
> Turn that field into a flags field.  Bits 7-0 identify the algorithm
> used to encode page_data, 0x00 meaning none, and a restoring side must
> fail on a value it does not implement.  Bits 31-8 stay reserved, and a
> restoring side must still reject a record which sets any of them,
> overriding the general rule for reserved fields.  Make libxenguest and
> the Python validator enforce both rules, and check the length of
> page_data according to the algorithm.  Add test_page_data_flags for the
> Python checks.
>
> The specification's Future Extensions section requires a new record
> type for any extension of an existing one.  Also allow an existing type
> to be extended in place when a receiver which predates the extension is
> guaranteed to reject the extended record rather than misread it.  For
> PAGE_DATA, this holds for an encoding which leaves count and the pfn
> array in plaintext and shortens page_data: an older libxenguest which
> ignores the field fails its exact length check.
>
> The stream version is not bumped.  A zero flags field means what the
> reserved field meant, and no other value is accepted.
>
> Assisted-by: Claude:claude-opus-5-5
> Signed-off-by: Marcus Granado <marcus.granado@xxxxxxxxxx>
> ---
>
> Notes:
>      Changes in v2:
>      - Replaces v1 1/6, which added a PAGE_DATA_LZ4 record type: PAGE_DATA's
>        reserved field becomes a flags field, following review by Teddy
>        Astie and Frediano Ziglio.  LZ4 is assigned in patch 3.
>
>   docs/specs/libxc-migration-stream.pandoc | 28 +++++++++++++++++------
>   tools/libs/guest/xg_sr_restore.c         | 29 ++++++++++++++++--------
>   tools/libs/guest/xg_sr_stream_format.h   |  8 ++++++-
>   tools/python/xen/migration/libxc.py      | 28 ++++++++++++++++++-----
>   tools/python/xen/migration/tests.py      | 25 +++++++++++++++++++-
>   5 files changed, 94 insertions(+), 24 deletions(-)
>
> diff --git a/docs/specs/libxc-migration-stream.pandoc 
> b/docs/specs/libxc-migration-stream.pandoc
> index b1c81b5d68..fd44dd6e0a 100644
(...)

> diff --git a/tools/libs/guest/xg_sr_stream_format.h 
> b/tools/libs/guest/xg_sr_stream_format.h
> index 1e6e50a710..f839d874c8 100644
> --- a/tools/libs/guest/xg_sr_stream_format.h
> +++ b/tools/libs/guest/xg_sr_stream_format.h
> @@ -83,10 +83,16 @@ struct xc_sr_rhdr
>   struct xc_sr_rec_page_data_header
>   {
>       uint32_t count;
> -    uint32_t _res1;
> +    uint32_t flags;
>       uint64_t pfn[0];
>   };
>   
> +/* PAGE_DATA flags: bits 7-0 are the algorithm, bits 31-8 are reserved. */
> +#define PAGE_DATA_ALGO_MASK       0x000000ffU
> +#define PAGE_DATA_FLAGS_RESZ_MASK 0xffffff00U
> +
> +#define PAGE_DATA_ALGO_NONE       0x00000000U
> +
nit: I would prefer having each algorithm as a explicit 8-bit number,
i.e 0x00U for ALGO_NONE, given that they can't be larger than it.

>   #define PAGE_DATA_PFN_MASK      0x000fffffffffffffULL
>   #define PAGE_DATA_PFN_RESZ_MASK 0x0ff0000000000000ULL
>   #define PAGE_DATA_TYPE_MASK     0xf000000000000000ULL
> diff --git a/tools/python/xen/migration/libxc.py 
> b/tools/python/xen/migration/libxc.py
> index e52e632cb1..2906b78d36 100644
> --- a/tools/python/xen/migration/libxc.py
> +++ b/tools/python/xen/migration/libxc.py
> @@ -86,6 +86,11 @@ PAGE_DATA_FORMAT             = "II"
>   PAGE_DATA_PFN_MASK           = (1 << 52) - 1
>   PAGE_DATA_PFN_RESZ_MASK      = ((1 << 60) - 1) & ~((1 << 52) - 1)
>   
> +# page_data flags: bits 7-0 algorithm, bits 31-8 reserved (must reject)
> +PAGE_DATA_ALGO_MASK          = 0x000000ff
> +PAGE_DATA_FLAGS_RESZ_MASK    = 0xffffff00
> +PAGE_DATA_ALGO_NONE          = 0x00
> +
>   # flags from xen/public/domctl.h: XEN_DOMCTL_PFINFO_* shifted by 32 bits
>   PAGE_DATA_TYPE_SHIFT         = 60
>   PAGE_DATA_TYPE_LTABTYPE_MASK = (0x7 << PAGE_DATA_TYPE_SHIFT)
> @@ -259,11 +264,14 @@ class VerifyLibxc(VerifyBase):
>               raise RecordError(
>                   "PAGE_DATA record must be at least %d bytes long" % (minsz, ))
>   
> -        count, res1 = unpack(PAGE_DATA_FORMAT, content[:minsz])
> +        count, flags = unpack(PAGE_DATA_FORMAT, content[:minsz])
>   
> -        if res1 != 0:
> +        if flags & PAGE_DATA_FLAGS_RESZ_MASK:
>               raise StreamError(
> -                "Reserved bits set in PAGE_DATA record 0x%04x" % (res1, ))
> +                "Reserved bits set in PAGE_DATA record 0x%08x"
> +                % (flags & PAGE_DATA_FLAGS_RESZ_MASK, ))
> +
> +        algo = flags & PAGE_DATA_ALGO_MASK
>   
>           pfnsz = count * 8
>           if (len(content) - minsz) < pfnsz:
> @@ -289,9 +297,17 @@ class VerifyLibxc(VerifyBase):
>                   nr_pages += 1
>   
>           pagesz = nr_pages * 4096
> -        if len(content) != minsz + pfnsz + pagesz:
> -            raise RecordError("Expected %u + %u + %u, got %u" %
> -                              (minsz, pfnsz, pagesz, len(content)))
> +        datasz = len(content) - minsz - pfnsz
> +
> +        if algo == PAGE_DATA_ALGO_NONE:
> +
> +            if datasz != pagesz:
> +                raise RecordError("Expected %u + %u + %u, got %u" %
> +                                  (minsz, pfnsz, pagesz, len(content)))
> +
> +        else:
> +            raise StreamError(
> +                "Unsupported algorithm %u in PAGE_DATA record" % (algo, ))
>   
>   
>       def verify_record_x86_pv_info(self, content):
> diff --git a/tools/python/xen/migration/tests.py 
> b/tools/python/xen/migration/tests.py
> index fcf94b0bb2..1944ae09b3 100644
> --- a/tools/python/xen/migration/tests.py
> +++ b/tools/python/xen/migration/tests.py
> @@ -6,7 +6,7 @@ Unit tests for migration v2 streams
>   
>   import unittest
>   
> -from struct import calcsize
> +from struct import calcsize, pack
>   
>   from xen.migration import libxc, libxl
>   
> @@ -30,6 +30,29 @@ class TestLibxc(unittest.TestCase):
>                            ):
>               self.assertEqual(calcsize(fmt), sz)
>   
> +    def test_page_data_flags(self):
> +        """ PAGE_DATA flags: algorithm dispatch and the reserved bits """
> +
> +        v = libxc.VerifyLibxc(lambda *_: None, lambda *_: b"")
> +        PAGE = b"\x00" * 4096
> +        NOTAB = libxc.PAGE_DATA_TYPE_NOTAB | 0x1   # one normal page, pfn 1
> +
> +        def rec(flags, data):
> +            return pack(libxc.PAGE_DATA_FORMAT, 1, flags) + \
> +                   pack("=Q", NOTAB) + data
> +
> +        # Uncompressed: exactly one page of data, and nothing else accepted.
> +        v.verify_record_page_data(rec(libxc.PAGE_DATA_ALGO_NONE, PAGE))
> +        self.assertRaises(libxc.RecordError, v.verify_record_page_data,
> +                          rec(libxc.PAGE_DATA_ALGO_NONE, PAGE[:-1]))
> +
> +        # An unimplemented algorithm, and any reserved bit, are refused.
> +        self.assertRaises(libxc.StreamError, v.verify_record_page_data,
> +                          rec(libxc.PAGE_DATA_ALGO_NONE + 1, PAGE))
> +        for bit in range(8, 32):
> +            self.assertRaises(libxc.StreamError, v.verify_record_page_data,
> +                              rec(1 << bit, PAGE))
> +
>   
>   class TestLibxl(unittest.TestCase):
>   
The rest looks good to me
With the change on number formatting

Reviewed-by: Teddy Astie <teddy.astie@xxxxxxxxxx>

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature