| Message ID | 20210601161118.18986-8-olaf@aepfle.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | leftover from 2020 | expand |
On 01.06.21 18:10, Olaf Hering wrote: > Introduce a helper which decides if a given pfn type has data > for the migration stream. > > No change in behavior intended. > > Signed-off-by: Olaf Hering <olaf@aepfle.de> > --- > tools/libs/saverestore/common.h | 17 ++++++++++++++++ > tools/libs/saverestore/restore.c | 34 +++++--------------------------- > tools/libs/saverestore/save.c | 14 ++----------- > 3 files changed, 24 insertions(+), 41 deletions(-) > > diff --git a/tools/libs/saverestore/common.h b/tools/libs/saverestore/common.h > index 36946e5d48..50a8479d39 100644 > --- a/tools/libs/saverestore/common.h > +++ b/tools/libs/saverestore/common.h > @@ -467,6 +467,23 @@ int populate_pfns(struct xc_sr_context *ctx, unsigned int count, > /* Handle a STATIC_DATA_END record. */ > int handle_static_data_end(struct xc_sr_context *ctx); > > +static inline bool page_type_has_stream_data(uint32_t type) > +{ > + bool ret; > + > + switch (type) > + { > + case XEN_DOMCTL_PFINFO_XTAB: > + case XEN_DOMCTL_PFINFO_XALLOC: > + case XEN_DOMCTL_PFINFO_BROKEN: > + ret = false; > + break; > + default: > + ret = true; > + break; > + } > + return ret; > +} > #endif > /* > * Local variables: > diff --git a/tools/libs/saverestore/restore.c b/tools/libs/saverestore/restore.c > index cccb0dcb71..700f9e74b5 100644 > --- a/tools/libs/saverestore/restore.c > +++ b/tools/libs/saverestore/restore.c > @@ -152,9 +152,8 @@ int populate_pfns(struct xc_sr_context *ctx, unsigned int count, > > for ( i = 0; i < count; ++i ) > { > - if ( (!types || (types && > - (types[i] != XEN_DOMCTL_PFINFO_XTAB && > - types[i] != XEN_DOMCTL_PFINFO_BROKEN))) && > + if ( (!types || > + (types && page_type_has_stream_data(types[i]) == true)) && What about XEN_DOMCTL_PFINFO_XALLOC? Is this case impossible here, or are you changing behavior? Juergen
Am Wed, 2 Jun 2021 08:59:13 +0200 schrieb Juergen Gross <jgross@suse.com>: > What about XEN_DOMCTL_PFINFO_XALLOC? Is this case impossible here, or > are you changing behavior? I think XEN_DOMCTL_PFINFO_XALLOC is a type that does not exist in practice. So, no change in behavior. Olaf
On 02.06.21 13:21, Olaf Hering wrote: > Am Wed, 2 Jun 2021 08:59:13 +0200 > schrieb Juergen Gross <jgross@suse.com>: > >> What about XEN_DOMCTL_PFINFO_XALLOC? Is this case impossible here, or >> are you changing behavior? > > I think XEN_DOMCTL_PFINFO_XALLOC is a type that does not exist in practice. Oh, indeed. It was used ages ago by migration code (I could find it being used e.g. in Xen 4.2, but not in 4.7). BTW, the use case was to not shatter superpages on migration... > So, no change in behavior. Maybe XEN_DOMCTL_PFINFO_XALLOC should no longer be supported, i.e. a stream containing it should be rejected? This might be a worthwhile modification of patch 5. Juergen
Am Wed, 2 Jun 2021 14:03:40 +0200 schrieb Juergen Gross <jgross@suse.com>: > Maybe XEN_DOMCTL_PFINFO_XALLOC should no longer be supported, i.e. > a stream containing it should be rejected? This might be a > worthwhile modification of patch 5. If there is a desire to actively break incoming domUs from pre-4.6 hosts, this should probably be done in a separate change. I have not checked if such migration works today with unmodified xen.git#staging. Olaf
On 07.06.21 12:12, Olaf Hering wrote: > Am Wed, 2 Jun 2021 14:03:40 +0200 > schrieb Juergen Gross <jgross@suse.com>: > >> Maybe XEN_DOMCTL_PFINFO_XALLOC should no longer be supported, i.e. >> a stream containing it should be rejected? This might be a >> worthwhile modification of patch 5. > > If there is a desire to actively break incoming domUs from pre-4.6 hosts, this should probably be done in a separate change. > > I have not checked if such migration works today with unmodified xen.git#staging. At least with your patch this might change now, as you are modifying the behavior in case of XEN_DOMCTL_PFINFO_XALLOC. Officially pre-4.6 hosts are not supported regarding LM. So your patch should be fine in this regard. OTH I'd rather have a clear failure than a maybe weird behavior in case a stream is containing a record with XEN_DOMCTL_PFINFO_XALLOC. Juergen
Am Wed, 2 Jun 2021 08:59:13 +0200 schrieb Juergen Gross <jgross@suse.com>: > > @@ -152,9 +152,8 @@ int populate_pfns(struct xc_sr_context *ctx, unsigned int count, > > > > for ( i = 0; i < count; ++i ) > > { > > - if ( (!types || (types && > > - (types[i] != XEN_DOMCTL_PFINFO_XTAB && > > - types[i] != XEN_DOMCTL_PFINFO_BROKEN))) && > > + if ( (!types || > > + (types && page_type_has_stream_data(types[i]) == true)) && > > What about XEN_DOMCTL_PFINFO_XALLOC? Is this case impossible here, or > are you changing behavior? I guess this needs to be handled somehow, a large enough HVM domU will have XEN_DOMCTL_PFINFO_XALLOC in the stream. Not sure why this was thrown away with the v2 format. Olaf
diff --git a/tools/libs/saverestore/common.h b/tools/libs/saverestore/common.h index 36946e5d48..50a8479d39 100644 --- a/tools/libs/saverestore/common.h +++ b/tools/libs/saverestore/common.h @@ -467,6 +467,23 @@ int populate_pfns(struct xc_sr_context *ctx, unsigned int count, /* Handle a STATIC_DATA_END record. */ int handle_static_data_end(struct xc_sr_context *ctx); +static inline bool page_type_has_stream_data(uint32_t type) +{ + bool ret; + + switch (type) + { + case XEN_DOMCTL_PFINFO_XTAB: + case XEN_DOMCTL_PFINFO_XALLOC: + case XEN_DOMCTL_PFINFO_BROKEN: + ret = false; + break; + default: + ret = true; + break; + } + return ret; +} #endif /* * Local variables: diff --git a/tools/libs/saverestore/restore.c b/tools/libs/saverestore/restore.c index cccb0dcb71..700f9e74b5 100644 --- a/tools/libs/saverestore/restore.c +++ b/tools/libs/saverestore/restore.c @@ -152,9 +152,8 @@ int populate_pfns(struct xc_sr_context *ctx, unsigned int count, for ( i = 0; i < count; ++i ) { - if ( (!types || (types && - (types[i] != XEN_DOMCTL_PFINFO_XTAB && - types[i] != XEN_DOMCTL_PFINFO_BROKEN))) && + if ( (!types || + (types && page_type_has_stream_data(types[i]) == true)) && !pfn_is_populated(ctx, original_pfns[i]) ) { rc = pfn_set_populated(ctx, original_pfns[i]); @@ -233,25 +232,8 @@ static int process_page_data(struct xc_sr_context *ctx, unsigned int count, { ctx->restore.ops.set_page_type(ctx, pfns[i], types[i]); - switch ( types[i] ) - { - case XEN_DOMCTL_PFINFO_NOTAB: - - case XEN_DOMCTL_PFINFO_L1TAB: - case XEN_DOMCTL_PFINFO_L1TAB | XEN_DOMCTL_PFINFO_LPINTAB: - - case XEN_DOMCTL_PFINFO_L2TAB: - case XEN_DOMCTL_PFINFO_L2TAB | XEN_DOMCTL_PFINFO_LPINTAB: - - case XEN_DOMCTL_PFINFO_L3TAB: - case XEN_DOMCTL_PFINFO_L3TAB | XEN_DOMCTL_PFINFO_LPINTAB: - - case XEN_DOMCTL_PFINFO_L4TAB: - case XEN_DOMCTL_PFINFO_L4TAB | XEN_DOMCTL_PFINFO_LPINTAB: - + if ( page_type_has_stream_data(types[i]) == true ) mfns[nr_pages++] = ctx->restore.ops.pfn_to_gfn(ctx, pfns[i]); - break; - } } /* Nothing to do? */ @@ -271,14 +253,8 @@ static int process_page_data(struct xc_sr_context *ctx, unsigned int count, for ( i = 0, j = 0; i < count; ++i ) { - switch ( types[i] ) - { - case XEN_DOMCTL_PFINFO_XTAB: - case XEN_DOMCTL_PFINFO_BROKEN: - case XEN_DOMCTL_PFINFO_XALLOC: - /* No page data to deal with. */ + if ( page_type_has_stream_data(types[i]) == false ) continue; - } if ( map_errs[j] ) { @@ -413,7 +389,7 @@ static int handle_page_data(struct xc_sr_context *ctx, struct xc_sr_record *rec) goto err; } - if ( type < XEN_DOMCTL_PFINFO_BROKEN ) + if ( page_type_has_stream_data(type) == true ) /* NOTAB and all L1 through L4 tables (including pinned) should * have a page worth of data in the record. */ pages_of_data++; diff --git a/tools/libs/saverestore/save.c b/tools/libs/saverestore/save.c index 8d449ee0ae..bcff2d28f5 100644 --- a/tools/libs/saverestore/save.c +++ b/tools/libs/saverestore/save.c @@ -153,13 +153,8 @@ static int write_batch(struct xc_sr_context *ctx) goto err; } - switch ( types[i] ) - { - case XEN_DOMCTL_PFINFO_BROKEN: - case XEN_DOMCTL_PFINFO_XALLOC: - case XEN_DOMCTL_PFINFO_XTAB: + if ( page_type_has_stream_data(types[i]) == false ) continue; - } mfns[nr_pages++] = mfns[i]; } @@ -177,13 +172,8 @@ static int write_batch(struct xc_sr_context *ctx) for ( i = 0, p = 0; i < nr_pfns; ++i ) { - switch ( types[i] ) - { - case XEN_DOMCTL_PFINFO_BROKEN: - case XEN_DOMCTL_PFINFO_XALLOC: - case XEN_DOMCTL_PFINFO_XTAB: + if ( page_type_has_stream_data(types[i]) == false ) continue; - } if ( errors[p] ) {
Introduce a helper which decides if a given pfn type has data for the migration stream. No change in behavior intended. Signed-off-by: Olaf Hering <olaf@aepfle.de> --- tools/libs/saverestore/common.h | 17 ++++++++++++++++ tools/libs/saverestore/restore.c | 34 +++++--------------------------- tools/libs/saverestore/save.c | 14 ++----------- 3 files changed, 24 insertions(+), 41 deletions(-)