* [PATCH v10 01/11] docs / include: introduce a new framework for 'domain context' records
2020-10-08 18:57 [PATCH v10 00/11] domain context infrastructure Paul Durrant
@ 2020-10-08 18:57 ` Paul Durrant
2020-10-19 13:46 ` Jan Beulich
2021-01-25 18:18 ` Andrew Cooper
2020-10-08 18:57 ` [PATCH v10 02/11] xen: introduce implementation of save/restore of 'domain context' Paul Durrant
` (10 subsequent siblings)
11 siblings, 2 replies; 30+ messages in thread
From: Paul Durrant @ 2020-10-08 18:57 UTC (permalink / raw)
To: xen-devel
Cc: Paul Durrant, Andrew Cooper, George Dunlap, Ian Jackson,
Jan Beulich, Julien Grall, Stefano Stabellini, Wei Liu,
Volodymyr Babchuk, Roger Pau Monné
From: Paul Durrant <pdurrant@amazon.com>
To allow enlightened HVM guests (i.e. those that have PV drivers) to be
migrated without their co-operation it will be necessary to transfer 'PV'
state such as event channel state, grant entry state, etc.
Currently there is a framework (entered via the hvm_save/load() functions)
that allows a domain's 'HVM' (architectural) state to be transferred but
'PV' state is also common with pure PV guests and so this framework is not
really suitable.
This patch adds the specification of a new common 'domain context' image and
the supporting public header. Subsequent patches will introduce other parts of
the framework, and code that will make use of it.
This patch also marks the HVM-only framework as deprecated in favour of the
new framework.
Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <george.dunlap@citrix.com>
Cc: Ian Jackson <iwj@xenproject.org>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Julien Grall <julien@xen.org>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Wei Liu <wl@xen.org>
Cc: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>
v10:
- New in v10
---
docs/specs/domain-context.md | 137 +++++++++++++++++++++++++
xen/include/public/arch-arm/hvm/save.h | 5 +
xen/include/public/arch-x86/hvm/save.h | 5 +
xen/include/public/save.h | 66 ++++++++++++
4 files changed, 213 insertions(+)
create mode 100644 docs/specs/domain-context.md
create mode 100644 xen/include/public/save.h
diff --git a/docs/specs/domain-context.md b/docs/specs/domain-context.md
new file mode 100644
index 0000000000..f177cf24b3
--- /dev/null
+++ b/docs/specs/domain-context.md
@@ -0,0 +1,137 @@
+# Domain Context (v1)
+
+## Background
+
+The design for *Non-Cooperative Migration of Guests*[1] explains that extra
+information is required in the migration stream to allow a guest running PV
+drivers to be migrated without its co-operation. This information includes
+hypervisor state such as the set of event channels in operation and the
+content of the grant table.
+
+There already exists in Xen a mechanism to save and restore *HVM context*[2].
+This specification describes a new framework that will replace that
+mechanism and be suitable for transferring additional *PV state* records
+conveying the information mentioned above. There is also on-going work to
+implement *live update* of Xen where hypervisor state must be transferred in
+memory from one incarnation to the next. The framework described is designed
+to also be suitable for this purpose.
+
+## Image format
+
+The format will read from or written to the hypervisor in a single virtually
+contiguous buffer segmented into **context records** specified in the following
+sections.
+
+Fields within the records will always be aligned to their size. Padding and
+reserved fields will always be set to zero when the context buffer is read
+from the hypervisor and will be verified when written.
+The endianness of numerical values will the native endianness of the
+hypervisor. In the case of migration, that endianness is specified in the
+*libxenctrl (libxc) Domain Image Format*[3].
+
+### Record format
+
+All records have the following format:
+
+```
+ 0 1 2 3 4 5 6 7 octet
++-------+-------+-------+-------+-------+-------+-------+-------+
+| type | instance |
++-------------------------------+-------------------------------+
+| length |
++---------------------------------------------------------------+
+| body
+...
+| | padding (0 to 7 octets) |
++-------+-------------------------------------------------------+
+```
+
+\pagebreak
+The fields are defined as follows:
+
+
+| Field | Description |
+|------------|--------------------------------------------------|
+| `type` | A code which determines the layout and semantics |
+| | of `body` |
+| | |
+| `instance` | The instance of the record |
+| | |
+| `length` | The length (in octets) of `body` |
+| | |
+| `body` | Zero or more octets of record data |
+| | |
+| `padding` | Zero to seven octets of zero-filled padding to |
+| | bring the total record length up to the next |
+| | 64-bit boundary |
+
+The `instance` field is present to distinguish multiple occurences of
+a record. E.g. state that is per-vcpu may need to be described in multiple
+records.
+
+The first record in the image is always a **START** record. The version of
+the image format can be inferred from the `type` of this record.
+
+## Image content
+
+The following records are defined for the v1 image format. This set may be
+extended in newer versions of the hypervisor. It is not expected that an image
+saved on a newer version of Xen will need to be restored on an older version.
+Therefore an image containing unrecognized record types should be rejected.
+
+### START
+
+```
+ 0 1 2 3 4 5 6 7 octet
++-------+-------+-------+-------+-------+-------+-------+-------+
+| type == 1 | instance == 0 |
++-------------------------------+-------------------------------+
+| length == 8 |
++-------------------------------+-------------------------------+
+| xen_major | xen_minor |
++-------------------------------+-------------------------------+
+```
+
+A type 1 **START** record implies a v1 image. If a new image format version
+is needed in future then this can be indicated by a new type value for this
+(first) record in the image.
+
+\pagebreak
+The record body contains the following fields:
+
+| Field | Description |
+|-------------|-------------------------------------------------|
+| `xen_major` | The major version of Xen that created this |
+| | image |
+| | |
+| `xen_minor` | The minor version of Xen that created this |
+| | image |
+
+The version of Xen that created the image can be useful to the version that
+is restoring the image to determine whether certain records are expected to
+be present in the image. For example, a version of Xen prior to X.Y may not
+generate a FOO record but Xen X.Y+ can infer its content. But Xen X.Y+1
+**must** generate the FOO record as, from that version onward, its content
+can no longer be safely inferred.
+
+### END
+
+```
+ 0 1 2 3 4 5 6 7 octet
++-------+-------+-------+-------+-------+-------+-------+-------+
+| type == 0 | instance == 0 |
++-------------------------------+-------------------------------+
+| length == 0 |
++---------------------------------------------------------------+
+```
+
+A record of this type terminates the image. No further data from the buffer
+should be consumed.
+
+* * *
+
+[1] See https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=docs/designs/non-cooperative-migration.md
+
+[2] See https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=xen/include/public/hvm/save.h
+
+[3] See https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=docs/specs/libxc-migration-stream.pandoc
diff --git a/xen/include/public/arch-arm/hvm/save.h b/xen/include/public/arch-arm/hvm/save.h
index 75b8e65bcb..d5b0c15203 100644
--- a/xen/include/public/arch-arm/hvm/save.h
+++ b/xen/include/public/arch-arm/hvm/save.h
@@ -26,6 +26,11 @@
#ifndef __XEN_PUBLIC_HVM_SAVE_ARM_H__
#define __XEN_PUBLIC_HVM_SAVE_ARM_H__
+/*
+ * Further use of HVM state is deprecated. New state records should only
+ * be added to the domain state header: public/save.h
+ */
+
#endif
/*
diff --git a/xen/include/public/arch-x86/hvm/save.h b/xen/include/public/arch-x86/hvm/save.h
index 773a380bc2..e61e2dbcd7 100644
--- a/xen/include/public/arch-x86/hvm/save.h
+++ b/xen/include/public/arch-x86/hvm/save.h
@@ -648,6 +648,11 @@ struct hvm_msr {
*/
#define HVM_SAVE_CODE_MAX 20
+/*
+ * Further use of HVM state is deprecated. New state records should only
+ * be added to the domain state header: public/save.h
+ */
+
#endif /* __XEN_PUBLIC_HVM_SAVE_X86_H__ */
/*
diff --git a/xen/include/public/save.h b/xen/include/public/save.h
new file mode 100644
index 0000000000..c4be9f570c
--- /dev/null
+++ b/xen/include/public/save.h
@@ -0,0 +1,66 @@
+/*
+ * save.h
+ *
+ * Structure definitions for common PV/HVM domain state that is held by Xen.
+ *
+ * Copyright Amazon.com Inc. or its affiliates.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to
+ * deal in the Software without restriction, including without limitation the
+ * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
+ * sell copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS IN THE SOFTWARE.
+ */
+
+#ifndef XEN_PUBLIC_SAVE_H
+#define XEN_PUBLIC_SAVE_H
+
+#if defined(__XEN__) || defined(__XEN_TOOLS__)
+
+#include "xen.h"
+
+/*
+ * C structures for the Domain Context v1 format.
+ * See docs/specs/domain-context.md
+ */
+
+struct domain_context_record {
+ uint32_t type;
+ uint32_t instance;
+ uint64_t length;
+ uint8_t body[XEN_FLEX_ARRAY_DIM];
+};
+
+#define _DOMAIN_CONTEXT_RECORD_ALIGN 3
+#define DOMAIN_CONTEXT_RECORD_ALIGN (1U << _DOMAIN_CONTEXT_RECORD_ALIGN)
+
+enum {
+ DOMAIN_CONTEXT_END,
+ DOMAIN_CONTEXT_START,
+ /* New types go here */
+ DOMAIN_CONTEXT_NR_TYPES
+};
+
+/* Initial entry */
+struct domain_context_start {
+ uint32_t xen_major, xen_minor;
+};
+
+/* Terminating entry */
+struct domain_context_end {};
+
+#endif /* defined(__XEN__) || defined(__XEN_TOOLS__) */
+
+#endif /* XEN_PUBLIC_SAVE_H */
--
2.20.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH v10 01/11] docs / include: introduce a new framework for 'domain context' records
2020-10-08 18:57 ` [PATCH v10 01/11] docs / include: introduce a new framework for 'domain context' records Paul Durrant
@ 2020-10-19 13:46 ` Jan Beulich
2021-01-25 18:25 ` Andrew Cooper
2021-01-25 18:18 ` Andrew Cooper
1 sibling, 1 reply; 30+ messages in thread
From: Jan Beulich @ 2020-10-19 13:46 UTC (permalink / raw)
To: Paul Durrant
Cc: xen-devel, Paul Durrant, Andrew Cooper, George Dunlap,
Ian Jackson, Julien Grall, Stefano Stabellini, Wei Liu,
Volodymyr Babchuk, Roger Pau Monné
On 08.10.2020 20:57, Paul Durrant wrote:
> --- /dev/null
> +++ b/xen/include/public/save.h
> @@ -0,0 +1,66 @@
> +/*
> + * save.h
> + *
> + * Structure definitions for common PV/HVM domain state that is held by Xen.
> + *
> + * Copyright Amazon.com Inc. or its affiliates.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to
> + * deal in the Software without restriction, including without limitation the
> + * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
> + * sell copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
> + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> + * DEALINGS IN THE SOFTWARE.
> + */
> +
> +#ifndef XEN_PUBLIC_SAVE_H
> +#define XEN_PUBLIC_SAVE_H
> +
> +#if defined(__XEN__) || defined(__XEN_TOOLS__)
> +
> +#include "xen.h"
> +
> +/*
> + * C structures for the Domain Context v1 format.
> + * See docs/specs/domain-context.md
> + */
> +
> +struct domain_context_record {
> + uint32_t type;
> + uint32_t instance;
> + uint64_t length;
Should this be uint64_aligned_t, such that alignof() will
produce consistent values regardless of bitness of the invoking
domain?
Jan
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH v10 01/11] docs / include: introduce a new framework for 'domain context' records
2020-10-19 13:46 ` Jan Beulich
@ 2021-01-25 18:25 ` Andrew Cooper
2021-01-26 8:55 ` Jan Beulich
0 siblings, 1 reply; 30+ messages in thread
From: Andrew Cooper @ 2021-01-25 18:25 UTC (permalink / raw)
To: Jan Beulich, Paul Durrant
Cc: xen-devel, Paul Durrant, George Dunlap, Ian Jackson,
Julien Grall, Stefano Stabellini, Wei Liu, Volodymyr Babchuk,
Roger Pau Monné
On 19/10/2020 14:46, Jan Beulich wrote:
> On 08.10.2020 20:57, Paul Durrant wrote:
>> --- /dev/null
>> +++ b/xen/include/public/save.h
>> @@ -0,0 +1,66 @@
>> +/*
>> + * save.h
>> + *
>> + * Structure definitions for common PV/HVM domain state that is held by Xen.
>> + *
>> + * Copyright Amazon.com Inc. or its affiliates.
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a copy
>> + * of this software and associated documentation files (the "Software"), to
>> + * deal in the Software without restriction, including without limitation the
>> + * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
>> + * sell copies of the Software, and to permit persons to whom the Software is
>> + * furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice shall be included in
>> + * all copies or substantial portions of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
>> + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
>> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
>> + * DEALINGS IN THE SOFTWARE.
>> + */
>> +
>> +#ifndef XEN_PUBLIC_SAVE_H
>> +#define XEN_PUBLIC_SAVE_H
>> +
>> +#if defined(__XEN__) || defined(__XEN_TOOLS__)
>> +
>> +#include "xen.h"
>> +
>> +/*
>> + * C structures for the Domain Context v1 format.
>> + * See docs/specs/domain-context.md
>> + */
>> +
>> +struct domain_context_record {
>> + uint32_t type;
>> + uint32_t instance;
>> + uint64_t length;
> Should this be uint64_aligned_t, such that alignof() will
> produce consistent values regardless of bitness of the invoking
> domain?
Does it matter? Its just a bitstream, and can appear in the migration
fd at any arbitrary alignment.
What matters is that the structure is aligned appropriately for the
bitness of code operating on these fields.
Even with the tools ABI fixed to allow a 32-on-64-on-64 toolstack to
function, I'm not sure that excess alignment would be appropriate. Sure
- it would be more efficient for 32bit code to align to the 8 byte
boundary for the benefit of a 64bit Xen's copy_from_user(), but this
alignment happens anyway because of how hypercall buffers work.
~Andrew
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH v10 01/11] docs / include: introduce a new framework for 'domain context' records
2021-01-25 18:25 ` Andrew Cooper
@ 2021-01-26 8:55 ` Jan Beulich
0 siblings, 0 replies; 30+ messages in thread
From: Jan Beulich @ 2021-01-26 8:55 UTC (permalink / raw)
To: Andrew Cooper
Cc: xen-devel, Paul Durrant, George Dunlap, Ian Jackson,
Julien Grall, Stefano Stabellini, Wei Liu, Volodymyr Babchuk,
Roger Pau Monné,
Paul Durrant
On 25.01.2021 19:25, Andrew Cooper wrote:
> On 19/10/2020 14:46, Jan Beulich wrote:
>> On 08.10.2020 20:57, Paul Durrant wrote:
>>> --- /dev/null
>>> +++ b/xen/include/public/save.h
>>> @@ -0,0 +1,66 @@
>>> +/*
>>> + * save.h
>>> + *
>>> + * Structure definitions for common PV/HVM domain state that is held by Xen.
>>> + *
>>> + * Copyright Amazon.com Inc. or its affiliates.
>>> + *
>>> + * Permission is hereby granted, free of charge, to any person obtaining a copy
>>> + * of this software and associated documentation files (the "Software"), to
>>> + * deal in the Software without restriction, including without limitation the
>>> + * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
>>> + * sell copies of the Software, and to permit persons to whom the Software is
>>> + * furnished to do so, subject to the following conditions:
>>> + *
>>> + * The above copyright notice and this permission notice shall be included in
>>> + * all copies or substantial portions of the Software.
>>> + *
>>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
>>> + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
>>> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
>>> + * DEALINGS IN THE SOFTWARE.
>>> + */
>>> +
>>> +#ifndef XEN_PUBLIC_SAVE_H
>>> +#define XEN_PUBLIC_SAVE_H
>>> +
>>> +#if defined(__XEN__) || defined(__XEN_TOOLS__)
>>> +
>>> +#include "xen.h"
>>> +
>>> +/*
>>> + * C structures for the Domain Context v1 format.
>>> + * See docs/specs/domain-context.md
>>> + */
>>> +
>>> +struct domain_context_record {
>>> + uint32_t type;
>>> + uint32_t instance;
>>> + uint64_t length;
>> Should this be uint64_aligned_t, such that alignof() will
>> produce consistent values regardless of bitness of the invoking
>> domain?
>
> Does it matter? Its just a bitstream, and can appear in the migration
> fd at any arbitrary alignment.
>
> What matters is that the structure is aligned appropriately for the
> bitness of code operating on these fields.
>
> Even with the tools ABI fixed to allow a 32-on-64-on-64 toolstack to
> function, I'm not sure that excess alignment would be appropriate. Sure
> - it would be more efficient for 32bit code to align to the 8 byte
> boundary for the benefit of a 64bit Xen's copy_from_user(), but this
> alignment happens anyway because of how hypercall buffers work.
It _probably_ doesn't matter (hence me having put this as a
question), but a struct in the ABI doesn't mandate how it is
going to be used. I don't expect this to become the (or part
of an) argument to a hypercall. I also don't expect anyone
needing to use alignof() on it. But by not following the
common model for the tools-only parts of the public
interface we'd outright exclude such uses down the road.
Jan
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v10 01/11] docs / include: introduce a new framework for 'domain context' records
2020-10-08 18:57 ` [PATCH v10 01/11] docs / include: introduce a new framework for 'domain context' records Paul Durrant
2020-10-19 13:46 ` Jan Beulich
@ 2021-01-25 18:18 ` Andrew Cooper
2021-01-26 9:31 ` Jan Beulich
1 sibling, 1 reply; 30+ messages in thread
From: Andrew Cooper @ 2021-01-25 18:18 UTC (permalink / raw)
To: Paul Durrant, xen-devel
Cc: Paul Durrant, George Dunlap, Ian Jackson, Jan Beulich,
Julien Grall, Stefano Stabellini, Wei Liu, Volodymyr Babchuk,
Roger Pau Monné
On 08/10/2020 19:57, Paul Durrant wrote:
> diff --git a/xen/include/public/save.h b/xen/include/public/save.h
> new file mode 100644
> index 0000000000..c4be9f570c
> --- /dev/null
> +++ b/xen/include/public/save.h
> @@ -0,0 +1,66 @@
> +/*
> + * save.h
> + *
> + * Structure definitions for common PV/HVM domain state that is held by Xen.
What exactly is, and is not in scope, for this new stream? The PV above
I think refers to "paravirtual state", not PV guests.
> +#define _DOMAIN_CONTEXT_RECORD_ALIGN 3
> +#define DOMAIN_CONTEXT_RECORD_ALIGN (1U << _DOMAIN_CONTEXT_RECORD_ALIGN)
Do we need the logarithm version?
> +
> +enum {
> + DOMAIN_CONTEXT_END,
> + DOMAIN_CONTEXT_START,
> + /* New types go here */
> + DOMAIN_CONTEXT_NR_TYPES
> +};
Does this enum ever end up in an API?
We might be ok as we're inside __XEN_TOOLS__, but enums normally cannot
be used in ABI's because their size is implementation defined, and not
always 4 bytes.
~Andrew
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH v10 01/11] docs / include: introduce a new framework for 'domain context' records
2021-01-25 18:18 ` Andrew Cooper
@ 2021-01-26 9:31 ` Jan Beulich
0 siblings, 0 replies; 30+ messages in thread
From: Jan Beulich @ 2021-01-26 9:31 UTC (permalink / raw)
To: Andrew Cooper
Cc: Paul Durrant, George Dunlap, Ian Jackson, Julien Grall,
Stefano Stabellini, Wei Liu, Volodymyr Babchuk,
Roger Pau Monné,
xen-devel, Paul Durrant
On 25.01.2021 19:18, Andrew Cooper wrote:
>> +enum {
>> + DOMAIN_CONTEXT_END,
>> + DOMAIN_CONTEXT_START,
>> + /* New types go here */
>> + DOMAIN_CONTEXT_NR_TYPES
>> +};
>
> Does this enum ever end up in an API?
>
> We might be ok as we're inside __XEN_TOOLS__, but enums normally cannot
> be used in ABI's because their size is implementation defined, and not
> always 4 bytes.
The only way to use this as a type (e.g. to declare a struct field)
would be via typeof(), which isn't something we'd allow in the
public interface (for being an extension). Hence without a tag I
would think an enum is fine to have here?
Jan
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v10 02/11] xen: introduce implementation of save/restore of 'domain context'
2020-10-08 18:57 [PATCH v10 00/11] domain context infrastructure Paul Durrant
2020-10-08 18:57 ` [PATCH v10 01/11] docs / include: introduce a new framework for 'domain context' records Paul Durrant
@ 2020-10-08 18:57 ` Paul Durrant
2020-10-19 14:07 ` Jan Beulich
2021-01-25 18:36 ` Andrew Cooper
2020-10-08 18:57 ` [PATCH v10 03/11] xen/common/domctl: introduce XEN_DOMCTL_get/set_domain_context Paul Durrant
` (9 subsequent siblings)
11 siblings, 2 replies; 30+ messages in thread
From: Paul Durrant @ 2020-10-08 18:57 UTC (permalink / raw)
To: xen-devel
Cc: Paul Durrant, Andrew Cooper, George Dunlap, Ian Jackson,
Jan Beulich, Julien Grall, Stefano Stabellini, Wei Liu
From: Paul Durrant <pdurrant@amazon.com>
A previous patch introduced the specification of the 'domain context' image
and the supporting public header. This patch introduces the code necessary
to generate and consume such an image. The entry points to the code are
domain_save_ctxt() and domain_load_ctxt(). Code to call these functions will
be introduced in a subsequent patch.
Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <george.dunlap@citrix.com>
Cc: Ian Jackson <iwj@xenproject.org>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Julien Grall <julien@xen.org>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Wei Liu <wl@xen.org>
v10:
- New in v10
- Largely derived from patch #1 of the v9 series, but the code has been
re-worked both functionally and cosmetically
---
xen/common/Makefile | 1 +
xen/common/save.c | 339 +++++++++++++++++++++++++++++++++++++++++
xen/include/xen/save.h | 160 +++++++++++++++++++
3 files changed, 500 insertions(+)
create mode 100644 xen/common/save.c
create mode 100644 xen/include/xen/save.h
diff --git a/xen/common/Makefile b/xen/common/Makefile
index b3b60a1ba2..3e6f21714a 100644
--- a/xen/common/Makefile
+++ b/xen/common/Makefile
@@ -37,6 +37,7 @@ obj-y += radix-tree.o
obj-y += rbtree.o
obj-y += rcupdate.o
obj-y += rwlock.o
+obj-y += save.o
obj-y += shutdown.o
obj-y += softirq.o
obj-y += sort.o
diff --git a/xen/common/save.c b/xen/common/save.c
new file mode 100644
index 0000000000..9287b20198
--- /dev/null
+++ b/xen/common/save.c
@@ -0,0 +1,339 @@
+/*
+ * save.c: save and load state common to all domain types.
+ *
+ * Copyright Amazon.com Inc. or its affiliates.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program; If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <xen/compile.h>
+#include <xen/save.h>
+
+struct domain_ctxt_state {
+ struct domain *d;
+ struct domain_context_record rec;
+ size_t len; /* for internal accounting */
+ union {
+ const struct domain_save_ctxt_ops *save;
+ const struct domain_load_ctxt_ops *load;
+ } ops;
+ void *priv;
+ bool dry_run;
+};
+
+static struct {
+ const char *name;
+ domain_save_ctxt_type save;
+ domain_load_ctxt_type load;
+} fns[DOMAIN_CONTEXT_NR_TYPES];
+
+void __init domain_register_ctxt_type(unsigned int type, const char *name,
+ domain_save_ctxt_type save,
+ domain_load_ctxt_type load)
+{
+ BUG_ON(type >= ARRAY_SIZE(fns));
+
+ ASSERT(!fns[type].save);
+ ASSERT(!fns[type].load);
+
+ fns[type].name = name;
+ fns[type].save = save;
+ fns[type].load = load;
+}
+
+int domain_save_ctxt_rec_begin(struct domain_ctxt_state *c,
+ unsigned int type, unsigned int instance)
+{
+ struct domain_context_record rec = { .type = type, .instance = instance };
+ int rc;
+
+ c->rec = rec;
+ c->len = 0;
+
+ rc = c->ops.save->begin(c->priv, &c->rec);
+ if ( rc )
+ return rc;
+
+ return 0;
+}
+
+int domain_save_ctxt_rec_data(struct domain_ctxt_state *c, const void *src,
+ size_t len)
+{
+ int rc = c->ops.save->append(c->priv, src, len);
+
+ if ( !rc )
+ c->len += len;
+
+ return rc;
+}
+
+int domain_save_ctxt_rec_end(struct domain_ctxt_state *c)
+{
+ size_t len = c->len;
+ size_t pad = ROUNDUP(len, DOMAIN_CONTEXT_RECORD_ALIGN) - len;
+ int rc;
+
+ if ( pad )
+ {
+ static const uint8_t zeroes[DOMAIN_CONTEXT_RECORD_ALIGN] = {};
+
+ rc = c->ops.save->append(c->priv, zeroes, pad);
+ if ( rc )
+ return rc;
+ }
+
+ if ( !c->dry_run )
+ gdprintk(XENLOG_DEBUG, "%pd save: %s[%u] +%zu (+%zu)\n", c->d,
+ fns[c->rec.type].name, c->rec.instance,
+ len, pad);
+
+ rc = c->ops.save->end(c->priv, c->len);
+
+ return rc;
+}
+
+int domain_save_ctxt(struct domain *d, const struct domain_save_ctxt_ops *ops,
+ void *priv, bool dry_run)
+{
+ struct domain_ctxt_state c = {
+ .d = d,
+ .ops.save = ops,
+ .priv = priv,
+ .dry_run = dry_run,
+ };
+ domain_save_ctxt_type save;
+ unsigned int type;
+ int rc;
+
+ ASSERT(d != current->domain);
+
+ save = fns[DOMAIN_CONTEXT_START].save;
+ BUG_ON(!save);
+
+ rc = save(d, &c, dry_run);
+ if ( rc )
+ return rc;
+
+ domain_pause(d);
+
+ for ( type = DOMAIN_CONTEXT_START + 1; type < ARRAY_SIZE(fns); type++ )
+ {
+ save = fns[type].save;
+ if ( !save )
+ continue;
+
+ rc = save(d, &c, dry_run);
+ if ( rc )
+ break;
+ }
+
+ domain_unpause(d);
+
+ if ( rc )
+ return rc;
+
+ save = fns[DOMAIN_CONTEXT_END].save;
+ BUG_ON(!save);
+
+ return save(d, &c, dry_run);
+}
+
+int domain_load_ctxt_rec_begin(struct domain_ctxt_state *c,
+ unsigned int type, unsigned int *instance)
+{
+ if ( type != c->rec.type )
+ {
+ ASSERT_UNREACHABLE();
+ return -EINVAL;
+ }
+
+ *instance = c->rec.instance;
+ c->len = 0;
+
+ return 0;
+}
+
+int domain_load_ctxt_rec_data(struct domain_ctxt_state *c, void *dst,
+ size_t len)
+{
+ int rc = 0;
+
+ c->len += len;
+ if (c->len > c->rec.length)
+ return -ENODATA;
+
+ if ( dst )
+ rc = c->ops.load->read(c->priv, dst, len);
+ else /* sink data */
+ {
+ uint8_t ignore;
+
+ while ( !rc && len-- )
+ rc = c->ops.load->read(c->priv, &ignore, sizeof(ignore));
+ }
+
+ return rc;
+}
+
+int domain_load_ctxt_rec_end(struct domain_ctxt_state *c, bool ignore_data)
+{
+ size_t len = c->len;
+ size_t pad = ROUNDUP(len, DOMAIN_CONTEXT_RECORD_ALIGN) - len;
+
+ gdprintk(XENLOG_DEBUG, "%pd load: %s[%u] +%zu (+%zu)\n", c->d,
+ fns[c->rec.type].name, c->rec.instance,
+ len, pad);
+
+ while ( pad-- )
+ {
+ uint8_t zero;
+ int rc = c->ops.load->read(c->priv, &zero, sizeof(zero));
+
+ if ( rc )
+ return rc;
+
+ if ( zero )
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+int domain_load_ctxt(struct domain *d, const struct domain_load_ctxt_ops *ops,
+ void *priv)
+{
+ struct domain_ctxt_state c = { .d = d, .ops.load = ops, .priv = priv, };
+ domain_load_ctxt_type load;
+ int rc;
+
+ ASSERT(d != current->domain);
+
+ rc = c.ops.load->read(c.priv, &c.rec, sizeof(c.rec));
+ if ( rc )
+ return rc;
+
+ load = fns[DOMAIN_CONTEXT_START].load;
+ BUG_ON(!load);
+
+ rc = load(d, &c);
+ if ( rc )
+ return rc;
+
+ domain_pause(d);
+
+ for (;;)
+ {
+ unsigned int type;
+
+ rc = c.ops.load->read(c.priv, &c.rec, sizeof(c.rec));
+ if ( rc )
+ break;
+
+ type = c.rec.type;
+ if ( type == DOMAIN_CONTEXT_END )
+ break;
+
+ rc = -EINVAL;
+ if ( type >= ARRAY_SIZE(fns) )
+ break;
+
+ load = fns[type].load;
+ if ( !load )
+ break;
+
+ rc = load(d, &c);
+ if ( rc )
+ break;
+ }
+
+ domain_unpause(d);
+
+ if ( rc )
+ return rc;
+
+ load = fns[DOMAIN_CONTEXT_END].load;
+ BUG_ON(!load);
+
+ return load(d, &c);
+}
+
+static int save_start(struct domain *d, struct domain_ctxt_state *c,
+ bool dry_run)
+{
+ static const struct domain_context_start s = {
+ .xen_major = XEN_VERSION,
+ .xen_minor = XEN_SUBVERSION,
+ };
+
+ return domain_save_ctxt_rec(c, DOMAIN_CONTEXT_START, 0, &s, sizeof(s));
+}
+
+static int load_start(struct domain *d, struct domain_ctxt_state *c)
+{
+ static struct domain_context_start s;
+ unsigned int i;
+ int rc = domain_load_ctxt_rec(c, DOMAIN_CONTEXT_START, &i, &s, sizeof(s));
+
+ if ( rc )
+ return rc;
+
+ if ( i )
+ return -EINVAL;
+
+ /*
+ * Make sure we are not attempting to load an image generated by a newer
+ * version of Xen.
+ */
+ if ( s.xen_major > XEN_VERSION && s.xen_minor > XEN_SUBVERSION )
+ return -EOPNOTSUPP;
+
+ return 0;
+}
+
+DOMAIN_REGISTER_CTXT_TYPE(START, save_start, load_start);
+
+static int save_end(struct domain *d, struct domain_ctxt_state *c,
+ bool dry_run)
+{
+ static const struct domain_context_end e = {};
+
+ return domain_save_ctxt_rec(c, DOMAIN_CONTEXT_END, 0, &e, sizeof(e));
+}
+
+static int load_end(struct domain *d, struct domain_ctxt_state *c)
+{
+ unsigned int i;
+ int rc = domain_load_ctxt_rec(c, DOMAIN_CONTEXT_END, &i, NULL,
+ sizeof(struct domain_context_end));
+
+ if ( rc )
+ return rc;
+
+ if ( i )
+ return -EINVAL;
+
+ return 0;
+}
+
+DOMAIN_REGISTER_CTXT_TYPE(END, save_end, load_end);
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/include/xen/save.h b/xen/include/xen/save.h
new file mode 100644
index 0000000000..358cf2f700
--- /dev/null
+++ b/xen/include/xen/save.h
@@ -0,0 +1,160 @@
+/*
+ * save.h: support routines for save/restore
+ *
+ * Copyright Amazon.com Inc. or its affiliates.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program; If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef XEN_SAVE_H
+#define XEN_SAVE_H
+
+#include <xen/init.h>
+#include <xen/sched.h>
+#include <xen/types.h>
+
+#include <public/save.h>
+
+struct domain_ctxt_state;
+
+int domain_save_ctxt_rec_begin(struct domain_ctxt_state *c, unsigned int type,
+ unsigned int instance);
+int domain_save_ctxt_rec_data(struct domain_ctxt_state *c, const void *data,
+ size_t len);
+int domain_save_ctxt_rec_end(struct domain_ctxt_state *c);
+
+static inline int domain_save_ctxt_rec(struct domain_ctxt_state *c,
+ unsigned int type, unsigned int instance,
+ const void *src, size_t len)
+{
+ int rc;
+
+ rc = domain_save_ctxt_rec_begin(c, type, instance);
+ if ( rc )
+ return rc;
+
+ rc = domain_save_ctxt_rec_data(c, src, len);
+ if ( rc )
+ return rc;
+
+ return domain_save_ctxt_rec_end(c);
+}
+
+int domain_load_ctxt_rec_begin(struct domain_ctxt_state *c, unsigned int type,
+ unsigned int *instance);
+int domain_load_ctxt_rec_data(struct domain_ctxt_state *c, void *data,
+ size_t len);
+int domain_load_ctxt_rec_end(struct domain_ctxt_state *c, bool ignore_data);
+
+static inline int domain_load_ctxt_rec(struct domain_ctxt_state *c,
+ unsigned int type,
+ unsigned int *instance, void *dst,
+ size_t len)
+{
+ int rc;
+
+ rc = domain_load_ctxt_rec_begin(c, type, instance);
+ if ( rc )
+ return rc;
+
+ rc = domain_load_ctxt_rec_data(c, dst, len);
+ if ( rc )
+ return rc;
+
+ return domain_load_ctxt_rec_end(c, false);
+}
+
+/*
+ * The 'dry_run' flag indicates that the caller of domain_save_ctxt() (see below)
+ * is not trying to actually acquire the data, only the size of the data.
+ * The save handler can therefore limit work to only that which is necessary
+ * to call domain_save_ctxt_rec_data() the correct number of times with accurate
+ * values for 'len'.
+ *
+ * NOTE: the domain pointer argument to domain_save_ctxt_type is not const as
+ * some handlers may need to acquire locks.
+ */
+typedef int (*domain_save_ctxt_type)(struct domain *d,
+ struct domain_ctxt_state *c,
+ bool dry_run);
+typedef int (*domain_load_ctxt_type)(struct domain *d,
+ struct domain_ctxt_state *c);
+
+void domain_register_ctxt_type(unsigned int type, const char *name,
+ domain_save_ctxt_type save,
+ domain_load_ctxt_type load);
+
+/*
+ * Register save and load handlers for a record type.
+ *
+ * Save handlers will be invoked in an order which copes with any inter-
+ * entry dependencies. For now this means that HEADER will come first and
+ * END will come last, all others being invoked in order of 'typecode'.
+ *
+ * Load handlers will be invoked in the order of entries present in the
+ * buffer.
+ */
+#define DOMAIN_REGISTER_CTXT_TYPE(x, s, l) \
+ static int __init __domain_register_##x##_ctxt_type(void) \
+ { \
+ domain_register_ctxt_type( \
+ DOMAIN_CONTEXT_ ## x, \
+ #x, \
+ &(s), \
+ &(l)); \
+ \
+ return 0; \
+ } \
+ __initcall(__domain_register_##x##_ctxt_type);
+
+/* Callback functions */
+struct domain_save_ctxt_ops {
+ /*
+ * Begin a new entry with the given record (only type and instance are
+ * valid).
+ */
+ int (*begin)(void *priv, const struct domain_context_record *rec);
+ /* Append data/padding to the buffer */
+ int (*append)(void *priv, const void *data, size_t len);
+ /*
+ * Complete the entry by updating the record with the total length of the
+ * appended data (not including padding).
+ */
+ int (*end)(void *priv, size_t len);
+};
+
+struct domain_load_ctxt_ops {
+ /* Read data/padding from the buffer */
+ int (*read)(void *priv, void *data, size_t len);
+};
+
+/*
+ * Entry points:
+ *
+ * ops: These are callback functions provided by the caller that will
+ * be used to write to (in the save case) or read from (in the
+ * load case) the context buffer. See above for more detail.
+ * priv: This is a pointer that will be passed to the copy function to
+ * allow it to identify the context buffer and the current state
+ * of the save or load operation.
+ * dry_run: If this is set then the caller of domain_save() is only trying
+ * to acquire the total size of the data, not the data itself.
+ * In this case the caller may supply different ops to avoid doing
+ * unnecessary work.
+ */
+int domain_save_ctxt(struct domain *d, const struct domain_save_ctxt_ops *ops,
+ void *priv, bool dry_run);
+int domain_load_ctxt(struct domain *d, const struct domain_load_ctxt_ops *ops,
+ void *priv);
+
+#endif /* XEN_SAVE_H */
--
2.20.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH v10 02/11] xen: introduce implementation of save/restore of 'domain context'
2020-10-08 18:57 ` [PATCH v10 02/11] xen: introduce implementation of save/restore of 'domain context' Paul Durrant
@ 2020-10-19 14:07 ` Jan Beulich
2021-01-25 18:36 ` Andrew Cooper
1 sibling, 0 replies; 30+ messages in thread
From: Jan Beulich @ 2020-10-19 14:07 UTC (permalink / raw)
To: Paul Durrant
Cc: xen-devel, Paul Durrant, Andrew Cooper, George Dunlap,
Ian Jackson, Julien Grall, Stefano Stabellini, Wei Liu
On 08.10.2020 20:57, Paul Durrant wrote:
> +void __init domain_register_ctxt_type(unsigned int type, const char *name,
> + domain_save_ctxt_type save,
> + domain_load_ctxt_type load)
> +{
> + BUG_ON(type >= ARRAY_SIZE(fns));
> +
> + ASSERT(!fns[type].save);
> + ASSERT(!fns[type].load);
> +
> + fns[type].name = name;
I expect I merely didn't spot this (perhaps just latent) issue in
earlier versions: If the caller lives in code getting built into
*.init.o, the string passed in will live in .init.rodata. That's a
general shortcoming (if you like) of the .o -> .init.o
tranformation, but I see no good alternative (or else all format
strings passed to printk() and alike won't get moved either).
Therefore I wonder whether it wouldn't be safer to have the struct
field be e.g. char[16], assuming 15 characters will allow for
meaningful names.
> +int domain_load_ctxt_rec_data(struct domain_ctxt_state *c, void *dst,
> + size_t len)
> +{
> + int rc = 0;
> +
> + c->len += len;
> + if (c->len > c->rec.length)
Nit: Missing blanks.
> +int domain_load_ctxt(struct domain *d, const struct domain_load_ctxt_ops *ops,
> + void *priv)
> +{
> + struct domain_ctxt_state c = { .d = d, .ops.load = ops, .priv = priv, };
> + domain_load_ctxt_type load;
> + int rc;
> +
> + ASSERT(d != current->domain);
> +
> + rc = c.ops.load->read(c.priv, &c.rec, sizeof(c.rec));
> + if ( rc )
> + return rc;
> +
> + load = fns[DOMAIN_CONTEXT_START].load;
> + BUG_ON(!load);
> +
> + rc = load(d, &c);
> + if ( rc )
> + return rc;
> +
> + domain_pause(d);
> +
> + for (;;)
Nit: Missing blanks again.
> + {
> + unsigned int type;
> +
> + rc = c.ops.load->read(c.priv, &c.rec, sizeof(c.rec));
> + if ( rc )
> + break;
> +
> + type = c.rec.type;
> + if ( type == DOMAIN_CONTEXT_END )
> + break;
> +
> + rc = -EINVAL;
> + if ( type >= ARRAY_SIZE(fns) )
> + break;
> +
> + load = fns[type].load;
While this is meant to be used by Dom0 only, I think it would be
better if it nevertheless used array_access_nospec().
> +static int load_start(struct domain *d, struct domain_ctxt_state *c)
> +{
> + static struct domain_context_start s;
> + unsigned int i;
> + int rc = domain_load_ctxt_rec(c, DOMAIN_CONTEXT_START, &i, &s, sizeof(s));
> +
> + if ( rc )
> + return rc;
> +
> + if ( i )
> + return -EINVAL;
> +
> + /*
> + * Make sure we are not attempting to load an image generated by a newer
> + * version of Xen.
> + */
> + if ( s.xen_major > XEN_VERSION && s.xen_minor > XEN_SUBVERSION )
> + return -EOPNOTSUPP;
Are you sure this needs to be excluded here and unilaterally?
And if this is to stay, then it wants to be
if ( s.xen_major > XEN_VERSION ||
(s.xen_major == XEN_VERSION && s.xen_minor > XEN_SUBVERSION) )
return -EOPNOTSUPP;
> +/*
> + * Register save and load handlers for a record type.
> + *
> + * Save handlers will be invoked in an order which copes with any inter-
> + * entry dependencies. For now this means that HEADER will come first and
> + * END will come last, all others being invoked in order of 'typecode'.
> + *
> + * Load handlers will be invoked in the order of entries present in the
> + * buffer.
> + */
> +#define DOMAIN_REGISTER_CTXT_TYPE(x, s, l) \
> + static int __init __domain_register_##x##_ctxt_type(void) \
> + { \
> + domain_register_ctxt_type( \
> + DOMAIN_CONTEXT_ ## x, \
> + #x, \
> + &(s), \
> + &(l)); \
I don't think there's a need for each of these to consume a separate
line.
Jan
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH v10 02/11] xen: introduce implementation of save/restore of 'domain context'
2020-10-08 18:57 ` [PATCH v10 02/11] xen: introduce implementation of save/restore of 'domain context' Paul Durrant
2020-10-19 14:07 ` Jan Beulich
@ 2021-01-25 18:36 ` Andrew Cooper
1 sibling, 0 replies; 30+ messages in thread
From: Andrew Cooper @ 2021-01-25 18:36 UTC (permalink / raw)
To: Paul Durrant, xen-devel
Cc: Paul Durrant, George Dunlap, Ian Jackson, Jan Beulich,
Julien Grall, Stefano Stabellini, Wei Liu
On 08/10/2020 19:57, Paul Durrant wrote:
> diff --git a/xen/common/save.c b/xen/common/save.c
> new file mode 100644
> index 0000000000..9287b20198
> --- /dev/null
> +++ b/xen/common/save.c
> @@ -0,0 +1,339 @@
>
> +static int load_start(struct domain *d, struct domain_ctxt_state *c)
> +{
> + static struct domain_context_start s;
> + unsigned int i;
> + int rc = domain_load_ctxt_rec(c, DOMAIN_CONTEXT_START, &i, &s, sizeof(s));
> +
> + if ( rc )
> + return rc;
> +
> + if ( i )
> + return -EINVAL;
> +
> + /*
> + * Make sure we are not attempting to load an image generated by a newer
> + * version of Xen.
> + */
> + if ( s.xen_major > XEN_VERSION && s.xen_minor > XEN_SUBVERSION )
major > XEN_VERSON || (major == XEN_VERSION && minor > XEN_SUBVERSION)
~Andrew
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v10 03/11] xen/common/domctl: introduce XEN_DOMCTL_get/set_domain_context
2020-10-08 18:57 [PATCH v10 00/11] domain context infrastructure Paul Durrant
2020-10-08 18:57 ` [PATCH v10 01/11] docs / include: introduce a new framework for 'domain context' records Paul Durrant
2020-10-08 18:57 ` [PATCH v10 02/11] xen: introduce implementation of save/restore of 'domain context' Paul Durrant
@ 2020-10-08 18:57 ` Paul Durrant
2020-10-19 14:30 ` Jan Beulich
2020-10-08 18:57 ` [PATCH v10 04/11] tools/misc: add xen-domctx to present domain context Paul Durrant
` (8 subsequent siblings)
11 siblings, 1 reply; 30+ messages in thread
From: Paul Durrant @ 2020-10-08 18:57 UTC (permalink / raw)
To: xen-devel
Cc: Paul Durrant, Julien Grall, Wei Liu, Daniel De Graaf,
Ian Jackson, Andrew Cooper, George Dunlap, Jan Beulich,
Stefano Stabellini
These domctls provide a mechanism to get and set 'domain context' from
the toolstack. The implementation calls the domain_save_ctxt() and
domain_load_ctxt() functions introduced in a previous patch.
Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
Cc: Julien Grall <julien@xen.org>
Cc: Wei Liu <wl@xen.org>
Cc: Daniel De Graaf <dgdegra@tycho.nsa.gov>
Cc: Ian Jackson <iwj@xenproject.org>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <george.dunlap@citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
v10:
- Re-base
- Add underscores and move to 64-bit image size as requested by Andrew
- Add a couple of record alignment ASSERTions
- Dropped R-b and A-b since changes are not entirely cosmetic
v4:
- Add missing zero pad checks
v3:
- Addressed comments from Julien and Jan
- Use vmalloc() rather than xmalloc_bytes()
v2:
- drop mask parameter
- const-ify some more buffers
---
tools/flask/policy/modules/xen.if | 4 +-
tools/libs/ctrl/include/xenctrl.h | 5 +
tools/libs/ctrl/xc_domain.c | 56 +++++++++
xen/common/domctl.c | 173 ++++++++++++++++++++++++++++
xen/include/public/domctl.h | 39 +++++++
xen/xsm/flask/hooks.c | 6 +
xen/xsm/flask/policy/access_vectors | 4 +
7 files changed, 285 insertions(+), 2 deletions(-)
diff --git a/tools/flask/policy/modules/xen.if b/tools/flask/policy/modules/xen.if
index 5e2aa472b6..2e2303d684 100644
--- a/tools/flask/policy/modules/xen.if
+++ b/tools/flask/policy/modules/xen.if
@@ -53,7 +53,7 @@ define(`create_domain_common', `
allow $1 $2:domain2 { set_cpu_policy settsc setscheduler setclaim
set_vnumainfo get_vnumainfo cacheflush
psr_cmt_op psr_alloc soft_reset
- resource_map get_cpu_policy };
+ resource_map get_cpu_policy set_context };
allow $1 $2:security check_context;
allow $1 $2:shadow enable;
allow $1 $2:mmu { map_read map_write adjust memorymap physmap pinpage mmuext_op updatemp };
@@ -97,7 +97,7 @@ define(`migrate_domain_out', `
allow $1 $2:hvm { gethvmc getparam };
allow $1 $2:mmu { stat pageinfo map_read };
allow $1 $2:domain { getaddrsize getvcpucontext pause destroy };
- allow $1 $2:domain2 gettsc;
+ allow $1 $2:domain2 { gettsc get_context };
allow $1 $2:shadow { enable disable logdirty };
')
diff --git a/tools/libs/ctrl/include/xenctrl.h b/tools/libs/ctrl/include/xenctrl.h
index 3796425e1e..754a00c67b 100644
--- a/tools/libs/ctrl/include/xenctrl.h
+++ b/tools/libs/ctrl/include/xenctrl.h
@@ -867,6 +867,11 @@ int xc_domain_hvm_setcontext(xc_interface *xch,
uint8_t *hvm_ctxt,
uint32_t size);
+int xc_domain_get_context(xc_interface *xch, uint32_t domid,
+ void *ctxt_buf, size_t *size);
+int xc_domain_set_context(xc_interface *xch, uint32_t domid,
+ const void *ctxt_buf, size_t size);
+
/**
* This function will return guest IO ABI protocol
*
diff --git a/tools/libs/ctrl/xc_domain.c b/tools/libs/ctrl/xc_domain.c
index e7cea4a17d..f35c1d2a28 100644
--- a/tools/libs/ctrl/xc_domain.c
+++ b/tools/libs/ctrl/xc_domain.c
@@ -536,6 +536,62 @@ int xc_domain_hvm_setcontext(xc_interface *xch,
return ret;
}
+int xc_domain_get_context(xc_interface *xch, uint32_t domid,
+ void *ctxt_buf, size_t *size)
+{
+ int ret;
+ DECLARE_DOMCTL = {
+ .cmd = XEN_DOMCTL_get_domain_context,
+ .domain = domid,
+ .u.get_domain_context.size = *size,
+ };
+ DECLARE_HYPERCALL_BOUNCE(ctxt_buf, *size, XC_HYPERCALL_BUFFER_BOUNCE_OUT);
+
+ if ( xc_hypercall_bounce_pre(xch, ctxt_buf) )
+ return -1;
+
+ set_xen_guest_handle(domctl.u.set_domain_context.buffer, ctxt_buf);
+
+ ret = do_domctl(xch, &domctl);
+
+ xc_hypercall_bounce_post(xch, ctxt_buf);
+
+ if ( ret )
+ return ret;
+
+ *size = domctl.u.get_domain_context.size;
+ if ( *size != domctl.u.get_domain_context.size )
+ {
+ errno = EOVERFLOW;
+ return -1;
+ }
+
+ return 0;
+}
+
+int xc_domain_set_context(xc_interface *xch, uint32_t domid,
+ const void *ctxt_buf, size_t size)
+{
+ int ret;
+ DECLARE_DOMCTL = {
+ .cmd = XEN_DOMCTL_set_domain_context,
+ .domain = domid,
+ .u.set_domain_context.size = size,
+ };
+ DECLARE_HYPERCALL_BOUNCE_IN(ctxt_buf, size);
+
+ if ( xc_hypercall_bounce_pre(xch, ctxt_buf) )
+ return -1;
+
+ set_xen_guest_handle(domctl.u.set_domain_context.buffer, ctxt_buf);
+
+ ret = do_domctl(xch, &domctl);
+
+ xc_hypercall_bounce_post(xch, ctxt_buf);
+
+ return ret;
+}
+
int xc_vcpu_getcontext(xc_interface *xch,
uint32_t domid,
uint32_t vcpu,
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index af044e2eda..6dbbe7f08a 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -25,6 +25,8 @@
#include <xen/hypercall.h>
#include <xen/vm_event.h>
#include <xen/monitor.h>
+#include <xen/save.h>
+#include <xen/vmap.h>
#include <asm/current.h>
#include <asm/irq.h>
#include <asm/page.h>
@@ -273,6 +275,168 @@ static struct vnuma_info *vnuma_init(const struct xen_domctl_vnuma *uinfo,
return ERR_PTR(ret);
}
+struct domctl_context
+{
+ void *buffer;
+ struct domain_context_record *rec;
+ size_t len;
+ size_t cur;
+};
+
+static int dry_run_append(void *priv, const void *data, size_t len)
+{
+ struct domctl_context *c = priv;
+
+ if ( c->len + len < c->len )
+ return -EOVERFLOW;
+
+ c->len += len;
+
+ return 0;
+}
+
+static int dry_run_begin(void *priv, const struct domain_context_record *rec)
+{
+ return dry_run_append(priv, NULL, sizeof(*rec));
+}
+
+static int dry_run_end(void *priv, size_t len)
+{
+ struct domctl_context *c = priv;
+
+ ASSERT(IS_ALIGNED(c->len, DOMAIN_CONTEXT_RECORD_ALIGN));
+
+ return 0;
+}
+
+static struct domain_save_ctxt_ops dry_run_ops = {
+ .begin = dry_run_begin,
+ .append = dry_run_append,
+ .end = dry_run_end,
+};
+
+static int save_begin(void *priv, const struct domain_context_record *rec)
+{
+ struct domctl_context *c = priv;
+
+ ASSERT(IS_ALIGNED(c->cur, DOMAIN_CONTEXT_RECORD_ALIGN));
+
+ if ( c->len - c->cur < sizeof(*rec) )
+ return -ENOSPC;
+
+ c->rec = c->buffer + c->cur; /* stash pointer to record */
+ *c->rec = *rec;
+
+ c->cur += sizeof(*rec);
+
+ return 0;
+}
+
+static int save_append(void *priv, const void *data, size_t len)
+{
+ struct domctl_context *c = priv;
+
+ if ( c->len - c->cur < len )
+ return -ENOSPC;
+
+ memcpy(c->buffer + c->cur, data, len);
+ c->cur += len;
+
+ return 0;
+}
+
+static int save_end(void *priv, size_t len)
+{
+ struct domctl_context *c = priv;
+
+ c->rec->length = len;
+
+ return 0;
+}
+
+static struct domain_save_ctxt_ops save_ops = {
+ .begin = save_begin,
+ .append = save_append,
+ .end = save_end,
+};
+
+static int get_domain_context(struct domain *d,
+ struct xen_domctl_get_domain_context *gdc)
+{
+ struct domctl_context c = { .buffer = ZERO_BLOCK_PTR };
+ int rc;
+
+ if ( d == current->domain )
+ return -EPERM;
+
+ if ( guest_handle_is_null(gdc->buffer) ) /* query for buffer size */
+ {
+ if ( gdc->size )
+ return -EINVAL;
+
+ /* dry run to acquire buffer size */
+ rc = domain_save_ctxt(d, &dry_run_ops, &c, true);
+ if ( rc )
+ return rc;
+
+ gdc->size = c.len;
+ return 0;
+ }
+
+ c.len = gdc->size;
+ c.buffer = vmalloc(c.len);
+ if ( !c.buffer )
+ return -ENOMEM;
+
+ rc = domain_save_ctxt(d, &save_ops, &c, false);
+
+ gdc->size = c.cur;
+ if ( !rc && copy_to_guest(gdc->buffer, c.buffer, gdc->size) )
+ rc = -EFAULT;
+
+ vfree(c.buffer);
+
+ return rc;
+}
+
+static int load_read(void *priv, void *data, size_t len)
+{
+ struct domctl_context *c = priv;
+
+ if ( c->len - c->cur < len )
+ return -ENODATA;
+
+ memcpy(data, c->buffer + c->cur, len);
+ c->cur += len;
+
+ return 0;
+}
+
+static struct domain_load_ctxt_ops load_ops = {
+ .read = load_read,
+};
+
+static int set_domain_context(struct domain *d,
+ const struct xen_domctl_set_domain_context *sdc)
+{
+ struct domctl_context c = { .buffer = ZERO_BLOCK_PTR, .len = sdc->size };
+ int rc;
+
+ if ( d == current->domain )
+ return -EPERM;
+
+ c.buffer = vmalloc(c.len);
+ if ( !c.buffer )
+ return -ENOMEM;
+
+ rc = !copy_from_guest(c.buffer, sdc->buffer, c.len) ?
+ domain_load_ctxt(d, &load_ops, &c) : -EFAULT;
+
+ vfree(c.buffer);
+
+ return rc;
+}
+
long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
{
long ret = 0;
@@ -867,6 +1031,15 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
copyback = 1;
break;
+ case XEN_DOMCTL_get_domain_context:
+ ret = get_domain_context(d, &op->u.get_domain_context);
+ copyback = !ret;
+ break;
+
+ case XEN_DOMCTL_set_domain_context:
+ ret = set_domain_context(d, &op->u.set_domain_context);
+ break;
+
default:
ret = arch_do_domctl(op, d, u_domctl);
break;
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index 666aeb71bf..a3e10c03f1 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -1132,6 +1132,41 @@ struct xen_domctl_vuart_op {
*/
};
+/*
+ * XEN_DOMCTL_get_domain_context
+ * -----------------------------
+ *
+ * buffer (IN): The buffer into which the context data should be
+ * copied, or NULL to query the buffer size that should
+ * be allocated.
+ * size (IN/OUT): If 'buffer' is NULL then the value passed in must be
+ * zero, and the value passed out will be the size of the
+ * buffer to allocate.
+ * If 'buffer' is non-NULL then the value passed in must
+ * be the size of the buffer into which data may be copied.
+ * The value passed out will be the size of data written.
+ */
+struct xen_domctl_get_domain_context {
+ uint64_t size;
+ XEN_GUEST_HANDLE_64(void) buffer;
+};
+
+/* XEN_DOMCTL_set_domain_context
+ * -----------------------------
+ *
+ * buffer (IN): The buffer from which the context data should be
+ * copied.
+ * size (IN): The size of the buffer from which data may be copied.
+ * This data must include DOMAIN_SAVE_CODE_HEADER at the
+ * start and terminate with a DOMAIN_SAVE_CODE_END record.
+ * Any data beyond the DOMAIN_SAVE_CODE_END record will be
+ * ignored.
+ */
+struct xen_domctl_set_domain_context {
+ uint64_t size;
+ XEN_GUEST_HANDLE_64(const_void) buffer;
+};
+
struct xen_domctl {
uint32_t cmd;
#define XEN_DOMCTL_createdomain 1
@@ -1216,6 +1251,8 @@ struct xen_domctl {
#define XEN_DOMCTL_vuart_op 81
#define XEN_DOMCTL_get_cpu_policy 82
#define XEN_DOMCTL_set_cpu_policy 83
+#define XEN_DOMCTL_get_domain_context 84
+#define XEN_DOMCTL_set_domain_context 85
#define XEN_DOMCTL_gdbsx_guestmemio 1000
#define XEN_DOMCTL_gdbsx_pausevcpu 1001
#define XEN_DOMCTL_gdbsx_unpausevcpu 1002
@@ -1276,6 +1313,8 @@ struct xen_domctl {
struct xen_domctl_monitor_op monitor_op;
struct xen_domctl_psr_alloc psr_alloc;
struct xen_domctl_vuart_op vuart_op;
+ struct xen_domctl_get_domain_context get_domain_context;
+ struct xen_domctl_set_domain_context set_domain_context;
uint8_t pad[128];
} u;
};
diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
index de050cc9fe..3c6217e4ac 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -754,6 +754,12 @@ static int flask_domctl(struct domain *d, int cmd)
case XEN_DOMCTL_get_cpu_policy:
return current_has_perm(d, SECCLASS_DOMAIN2, DOMAIN2__GET_CPU_POLICY);
+ case XEN_DOMCTL_set_domain_context:
+ return current_has_perm(d, SECCLASS_DOMAIN2, DOMAIN2__SET_CONTEXT);
+
+ case XEN_DOMCTL_get_domain_context:
+ return current_has_perm(d, SECCLASS_DOMAIN2, DOMAIN2__GET_CONTEXT);
+
default:
return avc_unknown_permission("domctl", cmd);
}
diff --git a/xen/xsm/flask/policy/access_vectors b/xen/xsm/flask/policy/access_vectors
index 1aa0bb501c..fea0c9f143 100644
--- a/xen/xsm/flask/policy/access_vectors
+++ b/xen/xsm/flask/policy/access_vectors
@@ -245,6 +245,10 @@ class domain2
resource_map
# XEN_DOMCTL_get_cpu_policy
get_cpu_policy
+# XEN_DOMCTL_set_domain_context
+ set_context
+# XEN_DOMCTL_get_domain_context
+ get_context
}
# Similar to class domain, but primarily contains domctls related to HVM domains
--
2.20.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH v10 03/11] xen/common/domctl: introduce XEN_DOMCTL_get/set_domain_context
2020-10-08 18:57 ` [PATCH v10 03/11] xen/common/domctl: introduce XEN_DOMCTL_get/set_domain_context Paul Durrant
@ 2020-10-19 14:30 ` Jan Beulich
2020-10-19 15:06 ` Jan Beulich
0 siblings, 1 reply; 30+ messages in thread
From: Jan Beulich @ 2020-10-19 14:30 UTC (permalink / raw)
To: Paul Durrant
Cc: xen-devel, Paul Durrant, Julien Grall, Wei Liu, Daniel De Graaf,
Ian Jackson, Andrew Cooper, George Dunlap, Stefano Stabellini
On 08.10.2020 20:57, Paul Durrant wrote:
> +static int dry_run_end(void *priv, size_t len)
> +{
> + struct domctl_context *c = priv;
> +
> + ASSERT(IS_ALIGNED(c->len, DOMAIN_CONTEXT_RECORD_ALIGN));
> +
> + return 0;
> +}
> +
> +static struct domain_save_ctxt_ops dry_run_ops = {
const? (same for save_ops and load_ops then)
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -1132,6 +1132,41 @@ struct xen_domctl_vuart_op {
> */
> };
>
> +/*
> + * XEN_DOMCTL_get_domain_context
> + * -----------------------------
> + *
> + * buffer (IN): The buffer into which the context data should be
> + * copied, or NULL to query the buffer size that should
> + * be allocated.
> + * size (IN/OUT): If 'buffer' is NULL then the value passed in must be
> + * zero, and the value passed out will be the size of the
> + * buffer to allocate.
> + * If 'buffer' is non-NULL then the value passed in must
> + * be the size of the buffer into which data may be copied.
> + * The value passed out will be the size of data written.
> + */
> +struct xen_domctl_get_domain_context {
> + uint64_t size;
uint64_aligned_t (also again below)?
With these adjusted
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Jan
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH v10 03/11] xen/common/domctl: introduce XEN_DOMCTL_get/set_domain_context
2020-10-19 14:30 ` Jan Beulich
@ 2020-10-19 15:06 ` Jan Beulich
0 siblings, 0 replies; 30+ messages in thread
From: Jan Beulich @ 2020-10-19 15:06 UTC (permalink / raw)
To: Paul Durrant
Cc: xen-devel, Paul Durrant, Julien Grall, Wei Liu, Daniel De Graaf,
Ian Jackson, Andrew Cooper, George Dunlap, Stefano Stabellini
On 19.10.2020 16:30, Jan Beulich wrote:
> On 08.10.2020 20:57, Paul Durrant wrote:
>> +static int dry_run_end(void *priv, size_t len)
>> +{
>> + struct domctl_context *c = priv;
>> +
>> + ASSERT(IS_ALIGNED(c->len, DOMAIN_CONTEXT_RECORD_ALIGN));
>> +
>> + return 0;
>> +}
>> +
>> +static struct domain_save_ctxt_ops dry_run_ops = {
>
> const? (same for save_ops and load_ops then)
>
>> --- a/xen/include/public/domctl.h
>> +++ b/xen/include/public/domctl.h
>> @@ -1132,6 +1132,41 @@ struct xen_domctl_vuart_op {
>> */
>> };
>>
>> +/*
>> + * XEN_DOMCTL_get_domain_context
>> + * -----------------------------
>> + *
>> + * buffer (IN): The buffer into which the context data should be
>> + * copied, or NULL to query the buffer size that should
>> + * be allocated.
>> + * size (IN/OUT): If 'buffer' is NULL then the value passed in must be
>> + * zero, and the value passed out will be the size of the
>> + * buffer to allocate.
>> + * If 'buffer' is non-NULL then the value passed in must
>> + * be the size of the buffer into which data may be copied.
>> + * The value passed out will be the size of data written.
>> + */
>> +struct xen_domctl_get_domain_context {
>> + uint64_t size;
>
> uint64_aligned_t (also again below)?
>
> With these adjusted
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
FAOD: Non-XSM hypervisor pieces only.
Jan
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v10 04/11] tools/misc: add xen-domctx to present domain context
2020-10-08 18:57 [PATCH v10 00/11] domain context infrastructure Paul Durrant
` (2 preceding siblings ...)
2020-10-08 18:57 ` [PATCH v10 03/11] xen/common/domctl: introduce XEN_DOMCTL_get/set_domain_context Paul Durrant
@ 2020-10-08 18:57 ` Paul Durrant
2020-10-08 18:57 ` [PATCH v10 05/11] common/domain: add a domain context record for shared_info Paul Durrant
` (7 subsequent siblings)
11 siblings, 0 replies; 30+ messages in thread
From: Paul Durrant @ 2020-10-08 18:57 UTC (permalink / raw)
To: xen-devel; +Cc: Paul Durrant, Ian Jackson, Wei Liu, Andrew Cooper
This tool is analogous to 'xen-hvmctx' which presents HVM context.
Subsequent patches will add 'dump' functions when new records are
introduced.
Signed-off-by: Paul Durrant <pdurrant@amazon.com>
Acked-by: Ian Jackson <iwj@xenproject.org>
Acked-by: Wei Liu <wl@xen.org>
---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
v10:
- Re-base
- Use err[x]()
- Keep existing A-b since modifications are trivial
v3:
- Re-worked to avoid copying onto stack
- Added optional typecode and instance arguments
v2:
- Change name from 'xen-ctx' to 'xen-domctx'
---
.gitignore | 1 +
tools/misc/Makefile | 4 +
tools/misc/xen-domctx.c | 172 ++++++++++++++++++++++++++++++++++++++++
3 files changed, 177 insertions(+)
create mode 100644 tools/misc/xen-domctx.c
diff --git a/.gitignore b/.gitignore
index 188495783e..4d9a61124d 100644
--- a/.gitignore
+++ b/.gitignore
@@ -243,6 +243,7 @@ tools/misc/xen_cpuperf
tools/misc/xen-cpuid
tools/misc/xen-detect
tools/misc/xen-diag
+tools/misc/xen-domctx
tools/misc/xen-tmem-list-parse
tools/misc/xen-livepatch
tools/misc/xenperf
diff --git a/tools/misc/Makefile b/tools/misc/Makefile
index 7d37f297a9..fb673d0ff6 100644
--- a/tools/misc/Makefile
+++ b/tools/misc/Makefile
@@ -32,6 +32,7 @@ INSTALL_SBIN += xenpm
INSTALL_SBIN += xenwatchdogd
INSTALL_SBIN += xen-livepatch
INSTALL_SBIN += xen-diag
+INSTALL_SBIN += xen-domctx
INSTALL_SBIN += $(INSTALL_SBIN-y)
# Everything to be installed in a private bin/
@@ -111,6 +112,9 @@ xen-livepatch: xen-livepatch.o
xen-diag: xen-diag.o
$(CC) $(LDFLAGS) -o $@ $< $(LDLIBS_libxenctrl) $(APPEND_LDFLAGS)
+xen-domctx: xen-domctx.o
+ $(CC) $(LDFLAGS) -o $@ $< $(LDLIBS_libxenctrl) $(APPEND_LDFLAGS)
+
xen-lowmemd: xen-lowmemd.o
$(CC) $(LDFLAGS) -o $@ $< $(LDLIBS_libxenevtchn) $(LDLIBS_libxenctrl) $(LDLIBS_libxenstore) $(APPEND_LDFLAGS)
diff --git a/tools/misc/xen-domctx.c b/tools/misc/xen-domctx.c
new file mode 100644
index 0000000000..ca135b9a28
--- /dev/null
+++ b/tools/misc/xen-domctx.c
@@ -0,0 +1,172 @@
+/*
+ * xen-domctx.c
+ *
+ * Print out domain save records in a human-readable way.
+ *
+ * Copyright Amazon.com Inc. or its affiliates.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS IN THE SOFTWARE.
+ */
+
+#include <inttypes.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <errno.h>
+#include <err.h>
+
+#include <xenctrl.h>
+#include <xen-tools/libs.h>
+#include <xen/xen.h>
+#include <xen/domctl.h>
+#include <xen/save.h>
+
+static void *buf = NULL;
+static size_t len, off;
+
+#define GET_PTR(_x) \
+ do { \
+ if ( len - off < sizeof(*(_x)) ) \
+ errx(1, "error: need another %lu bytes, only %lu available", \
+ sizeof(*(_x)), len - off); \
+ (_x) = buf + off; \
+ } while (false);
+
+static void dump_start(void)
+{
+ struct domain_context_start *s;
+
+ GET_PTR(s);
+
+ printf(" START: Xen %u.%u\n", s->xen_major, s->xen_minor);
+}
+
+static void dump_end(void)
+{
+ struct domain_context_end *e;
+
+ GET_PTR(e);
+
+ printf(" END\n");
+}
+
+static void usage(void)
+{
+ errx(1, "usage: <domid> [ <type> [ <instance> ]]");
+}
+
+int main(int argc, char **argv)
+{
+ char *s, *e;
+ long domid;
+ long type = -1;
+ long instance = -1;
+ unsigned int entry;
+ xc_interface *xch;
+ int rc;
+
+ if ( argc < 2 || argc > 4 )
+ usage();
+
+ s = e = argv[1];
+ domid = strtol(s, &e, 0);
+
+ if ( *s == '\0' || *e != '\0' ||
+ domid < 0 || domid >= DOMID_FIRST_RESERVED )
+ errx(1, "invalid domid '%s'", s);
+
+ if ( argc >= 3 )
+ {
+ s = e = argv[2];
+ type = strtol(s, &e, 0);
+
+ if ( *s == '\0' || *e != '\0' )
+ errx(1, "invalid type '%s'", s);
+ }
+
+ if ( argc == 4 )
+ {
+ s = e = argv[3];
+ instance = strtol(s, &e, 0);
+
+ if ( *s == '\0' || *e != '\0' )
+ errx(1, "invalid instance '%s'", s);
+ }
+
+ xch = xc_interface_open(0, 0, 0);
+ if ( !xch )
+ err(1, "can't open libxc handle");
+
+ rc = xc_domain_get_context(xch, domid, NULL, &len);
+ if ( rc < 0 )
+ err(1, "can't get context length for dom %lu", domid);
+
+ buf = malloc(len);
+ if ( !buf )
+ err(1, "can't allocate %lu bytes", len);
+
+ rc = xc_domain_get_context(xch, domid, buf, &len);
+ if ( rc < 0 )
+ err(1, "can't get context for dom %lu", domid);
+
+ off = 0;
+
+ entry = 0;
+ for ( ;; )
+ {
+ struct domain_context_record *rec;
+
+ GET_PTR(rec);
+
+ off += sizeof(*rec);
+
+ if ( (type < 0 || type == rec->type) &&
+ (instance < 0 || instance == rec->instance) )
+ {
+ printf("[%u] type: %u instance: %u length: %"PRIu64"\n", entry++,
+ rec->type, rec->instance, rec->length);
+
+ switch (rec->type)
+ {
+ case DOMAIN_CONTEXT_START: dump_start(); break;
+ case DOMAIN_CONTEXT_END: dump_end(); break;
+ default:
+ printf("Unknown type %u: skipping\n", rec->type);
+ break;
+ }
+ }
+
+ if ( rec->type == DOMAIN_CONTEXT_END )
+ break;
+
+ off += ROUNDUP(rec->length, _DOMAIN_CONTEXT_RECORD_ALIGN);
+ }
+
+ return 0;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
--
2.20.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH v10 05/11] common/domain: add a domain context record for shared_info...
2020-10-08 18:57 [PATCH v10 00/11] domain context infrastructure Paul Durrant
` (3 preceding siblings ...)
2020-10-08 18:57 ` [PATCH v10 04/11] tools/misc: add xen-domctx to present domain context Paul Durrant
@ 2020-10-08 18:57 ` Paul Durrant
2020-10-19 15:25 ` Jan Beulich
2021-01-25 19:11 ` Andrew Cooper
2020-10-08 18:57 ` [PATCH v10 06/11] x86/time: add a domain context record for tsc_info Paul Durrant
` (6 subsequent siblings)
11 siblings, 2 replies; 30+ messages in thread
From: Paul Durrant @ 2020-10-08 18:57 UTC (permalink / raw)
To: xen-devel
Cc: Paul Durrant, Jan Beulich, Ian Jackson, Wei Liu, Andrew Cooper,
George Dunlap, Julien Grall, Stefano Stabellini
From: Paul Durrant <pdurrant@amazon.com>
... and update xen-domctx to dump some information describing the record.
NOTE: Processing of the content during restore is currently limited to
PV domains, and matches processing of the PV-only SHARED_INFO record
done by libxc. All content is, however, saved such that restore
processing can be modified in future without requiring a new record
format.
Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Ian Jackson <iwj@xenproject.org>
Cc: Wei Liu <wl@xen.org>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <george.dunlap@citrix.com>
Cc: Julien Grall <julien@xen.org>
Cc: Stefano Stabellini <sstabellini@kernel.org>
v10:
- Re-base
- Amend the specification now there is one
- Dropped Jan's R-b as modifications are not completely trivial
v9:
- Use macros to make the code less verbose
- Add missing check for allocation failure
v8:
- Incorporate zero-ing out of shared info fields that would be done in
processing of SHARED_INFO from older stream versions
v7:
- Only restore vcpu_info and arch sub-structures for PV domains, to match
processing of SHARED_INFO in xc_sr_restore_x86_pv.c
- Use additional option to domain_load_end() to ignore the record for
HVM domains
v6:
- Only save compat_shared_info buffer if has_32bit_shinfo is set
- Validate flags field in load handler
v5:
- Addressed comments from Julien
v4:
- Addressed comments from Jan
v3:
- Actually dump some of the content of shared_info
v2:
- Drop the header change to define a 'Xen' page size and instead use a
variable length struct now that the framework makes this is feasible
- Guard use of 'has_32bit_shinfo' in common code with CONFIG_COMPAT
---
docs/specs/domain-context.md | 29 +++++++++
tools/misc/xen-domctx.c | 80 +++++++++++++++++++++++++
xen/common/domain.c | 113 +++++++++++++++++++++++++++++++++++
xen/include/public/save.h | 11 ++++
4 files changed, 233 insertions(+)
diff --git a/docs/specs/domain-context.md b/docs/specs/domain-context.md
index f177cf24b3..95e9f9d1ab 100644
--- a/docs/specs/domain-context.md
+++ b/docs/specs/domain-context.md
@@ -128,6 +128,33 @@ can no longer be safely inferred.
A record of this type terminates the image. No further data from the buffer
should be consumed.
+### SHARED_INFO
+
+```
+ 0 1 2 3 4 5 6 7 octet
++-------+-------+-------+-------+-------+-------+-------+-------+
+| type == 2 | instance == 0 |
++-------------------------------+-------------------------------+
+| length |
++-------------------------------+-------------------------------+
+| flags | buffer
++-------------------------------+
+...
+```
+
+\pagebreak
+The record body contains the following fields:
+
+| Field | Description |
+|-------------|-------------------------------------------------|
+| `flags` | A bit-wise OR of the following: |
+| | |
+| | 0x00000001: The domain has 32-bit (compat) |
+| | shared info |
+| | |
+| `buffer` | The shared info (`length` being architecture |
+| | dependent[4]) |
+
* * *
[1] See https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=docs/designs/non-cooperative-migration.md
@@ -135,3 +162,5 @@ should be consumed.
[2] See https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=xen/include/public/hvm/save.h
[3] See https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=docs/specs/libxc-migration-stream.pandoc
+
+[4] See https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=tools/include/xen-foreign/reference.size
diff --git a/tools/misc/xen-domctx.c b/tools/misc/xen-domctx.c
index ca135b9a28..5ea6de50d1 100644
--- a/tools/misc/xen-domctx.c
+++ b/tools/misc/xen-domctx.c
@@ -57,6 +57,85 @@ static void dump_start(void)
printf(" START: Xen %u.%u\n", s->xen_major, s->xen_minor);
}
+static void print_binary(const char *prefix, const void *val, size_t size,
+ const char *suffix)
+{
+ printf("%s", prefix);
+
+ while ( size-- )
+ {
+ uint8_t octet = *(const uint8_t *)val++;
+ unsigned int i;
+
+ for ( i = 0; i < 8; i++ )
+ {
+ printf("%u", octet & 1);
+ octet >>= 1;
+ }
+ }
+
+ printf("%s", suffix);
+}
+
+static void dump_shared_info(void)
+{
+ struct domain_context_shared_info *s;
+ bool has_32bit_shinfo;
+ shared_info_any_t *info;
+ unsigned int i, n;
+
+ GET_PTR(s);
+ has_32bit_shinfo = s->flags & DOMAIN_CONTEXT_32BIT_SHARED_INFO;
+
+ printf(" SHARED_INFO: has_32bit_shinfo: %s\n",
+ has_32bit_shinfo ? "true" : "false");
+
+ info = (shared_info_any_t *)s->buffer;
+
+#define GET_FIELD_PTR(_f) \
+ (has_32bit_shinfo ? \
+ (const void *)&(info->x32._f) : \
+ (const void *)&(info->x64._f))
+#define GET_FIELD_SIZE(_f) \
+ (has_32bit_shinfo ? sizeof(info->x32._f) : sizeof(info->x64._f))
+#define GET_FIELD(_f) \
+ (has_32bit_shinfo ? info->x32._f : info->x64._f)
+
+ n = has_32bit_shinfo ?
+ ARRAY_SIZE(info->x32.evtchn_pending) :
+ ARRAY_SIZE(info->x64.evtchn_pending);
+
+ for ( i = 0; i < n; i++ )
+ {
+ const char *prefix = !i ?
+ " evtchn_pending: " :
+ " ";
+
+ print_binary(prefix, GET_FIELD_PTR(evtchn_pending[0]),
+ GET_FIELD_SIZE(evtchn_pending[0]), "\n");
+ }
+
+ for ( i = 0; i < n; i++ )
+ {
+ const char *prefix = !i ?
+ " evtchn_mask: " :
+ " ";
+
+ print_binary(prefix, GET_FIELD_PTR(evtchn_mask[0]),
+ GET_FIELD_SIZE(evtchn_mask[0]), "\n");
+ }
+
+ printf(" wc: version: %u sec: %u nsec: %u",
+ GET_FIELD(wc_version), GET_FIELD(wc_sec), GET_FIELD(wc_nsec));
+ if ( !has_32bit_shinfo )
+ printf(" sec_hi: %u", info->x64.xen_wc_sec_hi);
+ printf("\n");
+
+#undef GET_FIELD
+#undef GET_FIELD_SIZE
+#undef GET_FIELD_PTR
+}
+
static void dump_end(void)
{
struct domain_context_end *e;
@@ -145,6 +224,7 @@ int main(int argc, char **argv)
switch (rec->type)
{
case DOMAIN_CONTEXT_START: dump_start(); break;
+ case DOMAIN_CONTEXT_SHARED_INFO: dump_shared_info(); break;
case DOMAIN_CONTEXT_END: dump_end(); break;
default:
printf("Unknown type %u: skipping\n", rec->type);
diff --git a/xen/common/domain.c b/xen/common/domain.c
index f748806a45..6c223dae38 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -33,6 +33,7 @@
#include <xen/xenoprof.h>
#include <xen/irq.h>
#include <xen/argo.h>
+#include <xen/save.h>
#include <asm/debugger.h>
#include <asm/p2m.h>
#include <asm/processor.h>
@@ -1671,6 +1672,118 @@ int continue_hypercall_on_cpu(
return 0;
}
+static int save_shared_info(struct domain *d, struct domain_ctxt_state *c,
+ bool dry_run)
+{
+#ifdef CONFIG_COMPAT
+ struct domain_context_shared_info s = {
+ .flags = has_32bit_shinfo(d) ? DOMAIN_CONTEXT_32BIT_SHARED_INFO : 0,
+ };
+ size_t size = has_32bit_shinfo(d) ?
+ sizeof(struct compat_shared_info) :
+ sizeof(struct shared_info);
+#else
+ struct domain_context_shared_info s = {};
+ size_t size = sizeof(struct shared_info);
+#endif
+ int rc;
+
+ rc = domain_save_ctxt_rec_begin(c, DOMAIN_CONTEXT_SHARED_INFO, 0);
+ if ( rc )
+ return rc;
+
+ rc = domain_save_ctxt_rec_data(c, &s, offsetof(typeof(s), buffer));
+ if ( rc )
+ return rc;
+
+ rc = domain_save_ctxt_rec_data(c, d->shared_info, size);
+ if ( rc )
+ return rc;
+
+ return domain_save_ctxt_rec_end(c);
+}
+
+static int load_shared_info(struct domain *d, struct domain_ctxt_state *c)
+{
+ struct domain_context_shared_info s = {};
+ size_t size;
+ unsigned int i;
+ int rc;
+
+ rc = domain_load_ctxt_rec_begin(c, DOMAIN_CONTEXT_SHARED_INFO, &i);
+ if ( rc )
+ return rc;
+
+ if ( i ) /* expect only a single instance */
+ return -ENXIO;
+
+ rc = domain_load_ctxt_rec_data(c, &s, offsetof(typeof(s), buffer));
+ if ( rc )
+ return rc;
+
+ if ( s.flags & ~DOMAIN_CONTEXT_32BIT_SHARED_INFO )
+ return -EINVAL;
+
+ if ( s.flags & DOMAIN_CONTEXT_32BIT_SHARED_INFO )
+ {
+#ifdef CONFIG_COMPAT
+ d->arch.has_32bit_shinfo = true;
+ size = sizeof(struct compat_shared_info);
+#else
+ return -EINVAL;
+#endif
+ }
+ else
+ size = sizeof(struct shared_info);
+
+ if ( is_pv_domain(d) )
+ {
+ shared_info_t *shinfo = xzalloc(shared_info_t);
+
+ if ( !shinfo )
+ return -ENOMEM;
+
+ rc = domain_load_ctxt_rec_data(c, shinfo, size);
+ if ( rc )
+ goto out;
+
+ memcpy(&shared_info(d, vcpu_info), &__shared_info(d, shinfo, vcpu_info),
+ sizeof(shared_info(d, vcpu_info)));
+ memcpy(&shared_info(d, arch), &__shared_info(d, shinfo, arch),
+ sizeof(shared_info(d, arch)));
+
+ memset(&shared_info(d, evtchn_pending), 0,
+ sizeof(shared_info(d, evtchn_pending)));
+ memset(&shared_info(d, evtchn_mask), 0xff,
+ sizeof(shared_info(d, evtchn_mask)));
+
+#ifdef CONFIG_X86
+ shared_info(d, arch.pfn_to_mfn_frame_list_list) = 0;
+#endif
+ for ( i = 0; i < XEN_LEGACY_MAX_VCPUS; i++ )
+ shared_info(d, vcpu_info[i].evtchn_pending_sel) = 0;
+
+ rc = domain_load_ctxt_rec_end(c, false);
+
+ out:
+ xfree(shinfo);
+ }
+ else
+ {
+ /*
+ * No modifications to shared_info are required for restoring non-PV
+ * domains.
+ */
+ rc = domain_load_ctxt_rec_data(c, NULL, size);
+ if ( !rc )
+ rc = domain_load_ctxt_rec_end(c, true);
+ }
+
+ return rc;
+}
+
+DOMAIN_REGISTER_CTXT_TYPE(SHARED_INFO, save_shared_info, load_shared_info);
+
/*
* Local variables:
* mode: C
diff --git a/xen/include/public/save.h b/xen/include/public/save.h
index c4be9f570c..bccbaadd0b 100644
--- a/xen/include/public/save.h
+++ b/xen/include/public/save.h
@@ -49,6 +49,7 @@ struct domain_context_record {
enum {
DOMAIN_CONTEXT_END,
DOMAIN_CONTEXT_START,
+ DOMAIN_CONTEXT_SHARED_INFO,
/* New types go here */
DOMAIN_CONTEXT_NR_TYPES
};
@@ -58,6 +59,16 @@ struct domain_context_start {
uint32_t xen_major, xen_minor;
};
+struct domain_context_shared_info {
+ uint32_t flags;
+
+#define _DOMAIN_CONTEXT_32BIT_SHARED_INFO 0
+#define DOMAIN_CONTEXT_32BIT_SHARED_INFO \
+ (1U << _DOMAIN_CONTEXT_32BIT_SHARED_INFO)
+
+ uint8_t buffer[XEN_FLEX_ARRAY_DIM];
+};
+
/* Terminating entry */
struct domain_context_end {};
--
2.20.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH v10 05/11] common/domain: add a domain context record for shared_info...
2020-10-08 18:57 ` [PATCH v10 05/11] common/domain: add a domain context record for shared_info Paul Durrant
@ 2020-10-19 15:25 ` Jan Beulich
2021-01-25 19:11 ` Andrew Cooper
1 sibling, 0 replies; 30+ messages in thread
From: Jan Beulich @ 2020-10-19 15:25 UTC (permalink / raw)
To: Paul Durrant
Cc: xen-devel, Paul Durrant, Ian Jackson, Wei Liu, Andrew Cooper,
George Dunlap, Julien Grall, Stefano Stabellini
On 08.10.2020 20:57, Paul Durrant wrote:
> @@ -1671,6 +1672,118 @@ int continue_hypercall_on_cpu(
> return 0;
> }
>
> +static int save_shared_info(struct domain *d, struct domain_ctxt_state *c,
> + bool dry_run)
> +{
> +#ifdef CONFIG_COMPAT
> + struct domain_context_shared_info s = {
> + .flags = has_32bit_shinfo(d) ? DOMAIN_CONTEXT_32BIT_SHARED_INFO : 0,
> + };
> + size_t size = has_32bit_shinfo(d) ?
> + sizeof(struct compat_shared_info) :
> + sizeof(struct shared_info);
> +#else
> + struct domain_context_shared_info s = {};
> + size_t size = sizeof(struct shared_info);
All of these would imo better be expressed in terms of d->shared_info.
While chances are zero that these types will change in any way, it
still sets a bad precedent for people seeing this and then introducing
similar disconnects elsewhere. (Same in the load handling then.)
> +static int load_shared_info(struct domain *d, struct domain_ctxt_state *c)
> +{
> + struct domain_context_shared_info s = {};
> + size_t size;
> + unsigned int i;
> + int rc;
> +
> + rc = domain_load_ctxt_rec_begin(c, DOMAIN_CONTEXT_SHARED_INFO, &i);
> + if ( rc )
> + return rc;
> +
> + if ( i ) /* expect only a single instance */
> + return -ENXIO;
> +
> + rc = domain_load_ctxt_rec_data(c, &s, offsetof(typeof(s), buffer));
> + if ( rc )
> + return rc;
> +
> + if ( s.flags & ~DOMAIN_CONTEXT_32BIT_SHARED_INFO )
> + return -EINVAL;
> +
> + if ( s.flags & DOMAIN_CONTEXT_32BIT_SHARED_INFO )
> + {
> +#ifdef CONFIG_COMPAT
> + d->arch.has_32bit_shinfo = true;
> + size = sizeof(struct compat_shared_info);
I realize this has been more or less this way already in prior
versions, but aren't you introducing a way to have a degenerate
64-bit PV guest with 32-bit shared info (or vice versa), in that
shared info bitness isn't strictly tied to guest bitness anymore?
Rejecting this case may not need to live here, but it needs to be
present / added somewhere.
> +#else
> + return -EINVAL;
> +#endif
> + }
> + else
> + size = sizeof(struct shared_info);
> +
> + if ( is_pv_domain(d) )
> + {
> + shared_info_t *shinfo = xzalloc(shared_info_t);
> +
> + if ( !shinfo )
> + return -ENOMEM;
> +
> + rc = domain_load_ctxt_rec_data(c, shinfo, size);
> + if ( rc )
> + goto out;
> +
> + memcpy(&shared_info(d, vcpu_info), &__shared_info(d, shinfo, vcpu_info),
> + sizeof(shared_info(d, vcpu_info)));
> + memcpy(&shared_info(d, arch), &__shared_info(d, shinfo, arch),
> + sizeof(shared_info(d, arch)));
> +
> + memset(&shared_info(d, evtchn_pending), 0,
> + sizeof(shared_info(d, evtchn_pending)));
> + memset(&shared_info(d, evtchn_mask), 0xff,
> + sizeof(shared_info(d, evtchn_mask)));
> +
> +#ifdef CONFIG_X86
> + shared_info(d, arch.pfn_to_mfn_frame_list_list) = 0;
> +#endif
> + for ( i = 0; i < XEN_LEGACY_MAX_VCPUS; i++ )
> + shared_info(d, vcpu_info[i].evtchn_pending_sel) = 0;
Again I realize this has been this way in earlier versions, and
it was also me to ask for streamlining the code, but is this
actually correct? I ask in particular in light of this comment
/*
* Compat field is never larger than native field, so cast to that as it
* is the largest memory range it is safe for the caller to modify without
* further discrimination between compat and native cases.
*/
in xen/shared.h, next to the __shared_info() #define. I can't
help thinking that you'll fill only half of some of the fields
in the 64-bit case.
> @@ -58,6 +59,16 @@ struct domain_context_start {
> uint32_t xen_major, xen_minor;
> };
>
> +struct domain_context_shared_info {
> + uint32_t flags;
> +
> +#define _DOMAIN_CONTEXT_32BIT_SHARED_INFO 0
Is this separate constant actually needed for anything?
Speaking of which - wouldn't all your additions to this header
better be proper name spacing citizens, by having xen_ / XEN_
prefixes?
Jan
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH v10 05/11] common/domain: add a domain context record for shared_info...
2020-10-08 18:57 ` [PATCH v10 05/11] common/domain: add a domain context record for shared_info Paul Durrant
2020-10-19 15:25 ` Jan Beulich
@ 2021-01-25 19:11 ` Andrew Cooper
1 sibling, 0 replies; 30+ messages in thread
From: Andrew Cooper @ 2021-01-25 19:11 UTC (permalink / raw)
To: Paul Durrant, xen-devel
Cc: Paul Durrant, Jan Beulich, Ian Jackson, Wei Liu, George Dunlap,
Julien Grall, Stefano Stabellini
On 08/10/2020 19:57, Paul Durrant wrote:
> diff --git a/xen/include/public/save.h b/xen/include/public/save.h
> index c4be9f570c..bccbaadd0b 100644
> --- a/xen/include/public/save.h
> +++ b/xen/include/public/save.h
> @@ -58,6 +59,16 @@ struct domain_context_start {
> uint32_t xen_major, xen_minor;
> };
>
> +struct domain_context_shared_info {
> + uint32_t flags;
> +
> +#define _DOMAIN_CONTEXT_32BIT_SHARED_INFO 0
> +#define DOMAIN_CONTEXT_32BIT_SHARED_INFO \
> + (1U << _DOMAIN_CONTEXT_32BIT_SHARED_INFO)
There is no need for the logarithm version of this constant.
You do however want an explicit uint32_t _rsvd; so buffer[] doesn't
start at the wrong alignment for an efficient memcpy() in 64bit builds
of Xen.
~Andrew
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v10 06/11] x86/time: add a domain context record for tsc_info...
2020-10-08 18:57 [PATCH v10 00/11] domain context infrastructure Paul Durrant
` (4 preceding siblings ...)
2020-10-08 18:57 ` [PATCH v10 05/11] common/domain: add a domain context record for shared_info Paul Durrant
@ 2020-10-08 18:57 ` Paul Durrant
2021-01-25 19:24 ` Andrew Cooper
2020-10-08 18:57 ` [PATCH v10 07/11] docs/specs: add missing definitions to libxc-migration-stream Paul Durrant
` (5 subsequent siblings)
11 siblings, 1 reply; 30+ messages in thread
From: Paul Durrant @ 2020-10-08 18:57 UTC (permalink / raw)
To: xen-devel
Cc: Paul Durrant, Jan Beulich, Ian Jackson, Wei Liu, Andrew Cooper,
George Dunlap, Julien Grall, Stefano Stabellini,
Roger Pau Monné
From: Paul Durrant <pdurrant@amazon.com>
... and update xen-domctx to dump some information describing the record.
NOTE: Whilst the record is x86 specific, it is visible directly in the common
header as context record numbers should be unique across all
architectures.
Signed-off-by: Paul Durrant <pdurrant@amazon.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
Cc: Ian Jackson <iwj@xenproject.org>
Cc: Wei Liu <wl@xen.org>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <george.dunlap@citrix.com>
Cc: Julien Grall <julien@xen.org>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>
v10:
- Re-base
- Amend the specification now there is one
- Kept Jan's R-b as the changes are cosmetic
v8:
- Removed stray blank line
v7:
- New in v7
---
docs/specs/domain-context.md | 32 ++++++++++++++++++++++++++++++++
tools/misc/xen-domctx.c | 12 ++++++++++++
xen/arch/x86/time.c | 30 ++++++++++++++++++++++++++++++
xen/include/public/save.h | 8 ++++++++
4 files changed, 82 insertions(+)
diff --git a/docs/specs/domain-context.md b/docs/specs/domain-context.md
index 95e9f9d1ab..8aa3466d96 100644
--- a/docs/specs/domain-context.md
+++ b/docs/specs/domain-context.md
@@ -155,6 +155,38 @@ The record body contains the following fields:
| `buffer` | The shared info (`length` being architecture |
| | dependent[4]) |
+### TSC_INFO
+
+```
+ 0 1 2 3 4 5 6 7 octet
++-------+-------+-------+-------+-------+-------+-------+-------+
+| type == 3 | instance == 0 |
++-------------------------------+-------------------------------+
+| length == 20 |
++-------------------------------+-------------------------------+
+| mode | khz |
++-------------------------------+-------------------------------+
+| nsec |
++-------------------------------+-------------------------------+
+| incarnation |
++-------------------------------+
+```
+
+The record body contains the following fields:
+
+| Field | Description |
+|---------------|-----------------------------------------------|
+| `mode` | 0x00000000: Default (emulate if necessary) |
+| | 0x00000001: Always emulate |
+| | 0x00000002: Never emulate |
+| | |
+| `khz` | The TSC frequency in kHz |
+| | |
+| `nsec` | Elapsed time in nanoseconds |
+| | |
+| `incarnation` | Incarnation (counter indicating how many |
+| | times the TSC value has been set) |
+
* * *
[1] See https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=docs/designs/non-cooperative-migration.md
diff --git a/tools/misc/xen-domctx.c b/tools/misc/xen-domctx.c
index 5ea6de50d1..1a5dfb9d5a 100644
--- a/tools/misc/xen-domctx.c
+++ b/tools/misc/xen-domctx.c
@@ -136,6 +136,17 @@ static void dump_shared_info(void)
#undef GET_FIELD_PTR
}
+static void dump_tsc_info(void)
+{
+ struct domain_context_tsc_info *t;
+
+ GET_PTR(t);
+
+ printf(" TSC_INFO: mode: %u incarnation: %u\n"
+ " khz %u nsec: %"PRIu64"\n",
+ t->mode, t->incarnation, t->khz, t->nsec);
+}
+
static void dump_end(void)
{
struct domain_context_end *e;
@@ -225,6 +236,7 @@ int main(int argc, char **argv)
{
case DOMAIN_CONTEXT_START: dump_start(); break;
case DOMAIN_CONTEXT_SHARED_INFO: dump_shared_info(); break;
+ case DOMAIN_CONTEXT_TSC_INFO: dump_tsc_info(); break;
case DOMAIN_CONTEXT_END: dump_end(); break;
default:
printf("Unknown type %u: skipping\n", rec->type);
diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
index 8938c0f435..aec4bfb0f3 100644
--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -26,6 +26,7 @@
#include <xen/symbols.h>
#include <xen/keyhandler.h>
#include <xen/guest_access.h>
+#include <xen/save.h>
#include <asm/io.h>
#include <asm/iocap.h>
#include <asm/msr.h>
@@ -2451,6 +2452,35 @@ int tsc_set_info(struct domain *d,
return 0;
}
+static int save_tsc_info(struct domain *d, struct domain_ctxt_state *c,
+ bool dry_run)
+{
+ struct domain_context_tsc_info t = {};
+
+ if ( !dry_run )
+ tsc_get_info(d, &t.mode, &t.nsec, &t.khz, &t.incarnation);
+
+ return domain_save_ctxt_rec(c, DOMAIN_CONTEXT_TSC_INFO, 0, &t, sizeof(t));
+}
+
+static int load_tsc_info(struct domain *d, struct domain_ctxt_state *c)
+{
+ struct domain_context_tsc_info t;
+ unsigned int i;
+ int rc;
+
+ rc = domain_load_ctxt_rec(c, DOMAIN_CONTEXT_TSC_INFO, &i, &t, sizeof(t));
+ if ( rc )
+ return rc;
+
+ if ( i ) /* expect only a single instance */
+ return -ENXIO;
+
+ return tsc_set_info(d, t.mode, t.nsec, t.khz, t.incarnation);
+}
+
+DOMAIN_REGISTER_CTXT_TYPE(TSC_INFO, save_tsc_info, load_tsc_info);
+
/* vtsc may incur measurable performance degradation, diagnose with this */
static void dump_softtsc(unsigned char key)
{
diff --git a/xen/include/public/save.h b/xen/include/public/save.h
index bccbaadd0b..86881864cf 100644
--- a/xen/include/public/save.h
+++ b/xen/include/public/save.h
@@ -50,6 +50,7 @@ enum {
DOMAIN_CONTEXT_END,
DOMAIN_CONTEXT_START,
DOMAIN_CONTEXT_SHARED_INFO,
+ DOMAIN_CONTEXT_TSC_INFO,
/* New types go here */
DOMAIN_CONTEXT_NR_TYPES
};
@@ -69,6 +70,13 @@ struct domain_context_shared_info {
uint8_t buffer[XEN_FLEX_ARRAY_DIM];
};
+struct domain_context_tsc_info {
+ uint32_t mode;
+ uint32_t khz;
+ uint64_t nsec;
+ uint32_t incarnation;
+};
+
/* Terminating entry */
struct domain_context_end {};
--
2.20.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH v10 06/11] x86/time: add a domain context record for tsc_info...
2020-10-08 18:57 ` [PATCH v10 06/11] x86/time: add a domain context record for tsc_info Paul Durrant
@ 2021-01-25 19:24 ` Andrew Cooper
0 siblings, 0 replies; 30+ messages in thread
From: Andrew Cooper @ 2021-01-25 19:24 UTC (permalink / raw)
To: Paul Durrant, xen-devel
Cc: Paul Durrant, Jan Beulich, Ian Jackson, Wei Liu, George Dunlap,
Julien Grall, Stefano Stabellini, Roger Pau Monné
On 08/10/2020 19:57, Paul Durrant wrote:
> +The record body contains the following fields:
> +
> +| Field | Description |
> +|---------------|-----------------------------------------------|
> +| `mode` | 0x00000000: Default (emulate if necessary) |
> +| | 0x00000001: Always emulate |
> +| | 0x00000002: Never emulate |
> +| | |
> +| `khz` | The TSC frequency in kHz |
> +| | |
> +| `nsec` | Elapsed time in nanoseconds |
> +| | |
> +| `incarnation` | Incarnation (counter indicating how many |
> +| | times the TSC value has been set) |
It is how many set_tsc_info() (hyper)calls have been made, not how many
times the guest has written to the TSC MSR.
That said, it is totally useless now that PVRDTSCP mode has gone, other
than the fact that it appears in guest CPUID as an approximation of "how
many times has this domain been migrated".
I.e. its a number you'll want to actively squash in your usecase.
I'm not sure whether to suggest dropping the field entirely, or not. I
highly doubt any user exists - IIRC, it was specifically for PVRDTSCP
userspace to notice that the frequency may have changed, and to
re-adjust its calculations.
> diff --git a/xen/include/public/save.h b/xen/include/public/save.h
> index bccbaadd0b..86881864cf 100644
> --- a/xen/include/public/save.h
> +++ b/xen/include/public/save.h
> @@ -50,6 +50,7 @@ enum {
> DOMAIN_CONTEXT_END,
> DOMAIN_CONTEXT_START,
> DOMAIN_CONTEXT_SHARED_INFO,
> + DOMAIN_CONTEXT_TSC_INFO,
At a minimum, this wants an /* x86 only */ comment. Possibly an X86 infix.
~Andrew
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v10 07/11] docs/specs: add missing definitions to libxc-migration-stream
2020-10-08 18:57 [PATCH v10 00/11] domain context infrastructure Paul Durrant
` (5 preceding siblings ...)
2020-10-08 18:57 ` [PATCH v10 06/11] x86/time: add a domain context record for tsc_info Paul Durrant
@ 2020-10-08 18:57 ` Paul Durrant
2021-01-25 19:28 ` Andrew Cooper
2020-10-08 18:57 ` [PATCH v10 08/11] docs / tools: specify migration v4 to include DOMAIN_CONTEXT Paul Durrant
` (4 subsequent siblings)
11 siblings, 1 reply; 30+ messages in thread
From: Paul Durrant @ 2020-10-08 18:57 UTC (permalink / raw)
To: xen-devel
Cc: Paul Durrant, Wei Liu, Andrew Cooper, George Dunlap, Ian Jackson,
Jan Beulich, Julien Grall, Stefano Stabellini
From: Paul Durrant <pdurrant@amazon.com>
The STATIC_DATA_END, X86_CPUID_POLICY and X86_MSR_POLICY record types have
sections explaining what they are but their values are not defined. Indeed
their values are defined as "Reserved for future mandatory records."
Also, the spec revision is adjusted to match the migration stream version
and an END record is added to the description of a 'typical save record for
and x86 HVM guest.'
Signed-off-by: Paul Durrant <pdurrant@amazon.com>
Fixes: 6f71b5b1506 ("docs/migration Specify migration v3 and STATIC_DATA_END")
Fixes: ddd273d8863 ("docs/migration: Specify X86_{CPUID,MSR}_POLICY records")
Acked-by: Wei Liu <wl@xen.org>
---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <george.dunlap@citrix.com>
Cc: Ian Jackson <iwj@xenproject.org>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Julien Grall <julien@xen.org>
Cc: Stefano Stabellini <sstabellini@kernel.org>
v7:
- New in v7
---
docs/specs/libxc-migration-stream.pandoc | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/docs/specs/libxc-migration-stream.pandoc b/docs/specs/libxc-migration-stream.pandoc
index 6b0c49e97a..8aeab3b11b 100644
--- a/docs/specs/libxc-migration-stream.pandoc
+++ b/docs/specs/libxc-migration-stream.pandoc
@@ -3,7 +3,7 @@
Andrew Cooper <<andrew.cooper3@citrix.com>>
Wen Congyang <<wency@cn.fujitsu.com>>
Yang Hongyang <<hongyang.yang@easystack.cn>>
-% Revision 2
+% Revision 3
Introduction
============
@@ -227,7 +227,13 @@ type 0x00000000: END
0x0000000F: CHECKPOINT_DIRTY_PFN_LIST (Secondary -> Primary)
- 0x00000010 - 0x7FFFFFFF: Reserved for future _mandatory_
+ 0x00000010: STATIC_DATA_END
+
+ 0x00000011: X86_CPUID_POLICY
+
+ 0x00000012: X86_MSR_POLICY
+
+ 0x00000013 - 0x7FFFFFFF: Reserved for future _mandatory_
records.
0x80000000 - 0xFFFFFFFF: Reserved for future _optional_
@@ -732,6 +738,7 @@ A typical save record for an x86 HVM guest image would look like:
* X86_TSC_INFO
* HVM_PARAMS
* HVM_CONTEXT
+* END record
HVM_PARAMS must precede HVM_CONTEXT, as certain parameters can affect
the validity of architectural state in the context.
--
2.20.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH v10 07/11] docs/specs: add missing definitions to libxc-migration-stream
2020-10-08 18:57 ` [PATCH v10 07/11] docs/specs: add missing definitions to libxc-migration-stream Paul Durrant
@ 2021-01-25 19:28 ` Andrew Cooper
0 siblings, 0 replies; 30+ messages in thread
From: Andrew Cooper @ 2021-01-25 19:28 UTC (permalink / raw)
To: Paul Durrant, xen-devel
Cc: Paul Durrant, Wei Liu, George Dunlap, Ian Jackson, Jan Beulich,
Julien Grall, Stefano Stabellini
On 08/10/2020 19:57, Paul Durrant wrote:
> From: Paul Durrant <pdurrant@amazon.com>
>
> The STATIC_DATA_END, X86_CPUID_POLICY and X86_MSR_POLICY record types have
> sections explaining what they are but their values are not defined. Indeed
> their values are defined as "Reserved for future mandatory records."
>
> Also, the spec revision is adjusted to match the migration stream version
> and an END record is added to the description of a 'typical save record for
> and x86 HVM guest.'
>
> Signed-off-by: Paul Durrant <pdurrant@amazon.com>
> Fixes: 6f71b5b1506 ("docs/migration Specify migration v3 and STATIC_DATA_END")
> Fixes: ddd273d8863 ("docs/migration: Specify X86_{CPUID,MSR}_POLICY records")
> Acked-by: Wei Liu <wl@xen.org>
I've committed this. I have no idea where these hunks got lost, because
I definitely did have them at some point during the mig-v3 work.
~Andrew
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v10 08/11] docs / tools: specify migration v4 to include DOMAIN_CONTEXT
2020-10-08 18:57 [PATCH v10 00/11] domain context infrastructure Paul Durrant
` (6 preceding siblings ...)
2020-10-08 18:57 ` [PATCH v10 07/11] docs/specs: add missing definitions to libxc-migration-stream Paul Durrant
@ 2020-10-08 18:57 ` Paul Durrant
2021-01-25 19:43 ` Andrew Cooper
2020-10-08 18:57 ` [PATCH v10 09/11] tools/python: modify libxc.py to verify v4 stream Paul Durrant
` (3 subsequent siblings)
11 siblings, 1 reply; 30+ messages in thread
From: Paul Durrant @ 2020-10-08 18:57 UTC (permalink / raw)
To: xen-devel
Cc: Paul Durrant, Wei Liu, Andrew Cooper, George Dunlap, Ian Jackson,
Jan Beulich, Julien Grall, Stefano Stabellini,
Marek Marczykowski-Górecki
From: Paul Durrant <pdurrant@amazon.com>
A new 'domain context' framework was recently introduced to facilitate
transfer of state for both PV and HVM guests. Hence this patch mandates the
presence of a new DOMAIN_CONTEXT record in version 4 of the libxc migration
stream.
This record will incorprate the content of the domain's 'shared_info' page
and the TSC informition so the SHARED_INFO and TSC_INFO records are deprecated.
It is intended that, in future, this record will contain state currently
present in the HVM_CONTEXT record. However, for compatibility with earlier
migration streams, the version 4 stream format continues to specify an
HVM_CONTEXT record and XEN_DOMCTL_sethvmcontext will continue to accept all
content of that record that may be present in a version 3 stream.
Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
Cc: Wei Liu <wl@xen.org>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <george.dunlap@citrix.com>
Cc: Ian Jackson <iwj@xenproject.org>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Julien Grall <julien@xen.org>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: "Marek Marczykowski-Górecki" <marmarek@invisiblethingslab.com>
NOTE: Wei requested ack from Andrew
v10:
- Removed changes to xg_sr_common.c and libxc.py to make this a just
documentation and header patch
- Dropped Wei's A-b in light of change
v7:
- New in v7
---
docs/specs/libxc-migration-stream.pandoc | 62 ++++++++++++++++++------
tools/libs/guest/xg_sr_stream_format.h | 1 +
2 files changed, 49 insertions(+), 14 deletions(-)
diff --git a/docs/specs/libxc-migration-stream.pandoc b/docs/specs/libxc-migration-stream.pandoc
index 8aeab3b11b..aa6fe284f3 100644
--- a/docs/specs/libxc-migration-stream.pandoc
+++ b/docs/specs/libxc-migration-stream.pandoc
@@ -3,7 +3,7 @@
Andrew Cooper <<andrew.cooper3@citrix.com>>
Wen Congyang <<wency@cn.fujitsu.com>>
Yang Hongyang <<hongyang.yang@easystack.cn>>
-% Revision 3
+% Revision 4
Introduction
============
@@ -127,7 +127,7 @@ marker 0xFFFFFFFFFFFFFFFF.
id 0x58454E46 ("XENF" in ASCII).
-version 0x00000003. The version of this specification.
+version 0x00000004. The version of this specification.
options bit 0: Endianness. 0 = little-endian, 1 = big-endian.
@@ -209,9 +209,9 @@ type 0x00000000: END
0x00000006: X86_PV_VCPU_XSAVE
- 0x00000007: SHARED_INFO
+ 0x00000007: SHARED_INFO (deprecated)
- 0x00000008: X86_TSC_INFO
+ 0x00000008: X86_TSC_INFO (deprecated)
0x00000009: HVM_CONTEXT
@@ -233,7 +233,9 @@ type 0x00000000: END
0x00000012: X86_MSR_POLICY
- 0x00000013 - 0x7FFFFFFF: Reserved for future _mandatory_
+ 0x00000013: DOMAIN_CONTEXT
+
+ 0x00000014 - 0x7FFFFFFF: Reserved for future _mandatory_
records.
0x80000000 - 0xFFFFFFFF: Reserved for future _optional_
@@ -442,10 +444,11 @@ X86_PV_VCPU_MSRS XEN_DOMCTL_{get,set}\_vcpu_msrs
\clearpage
-SHARED_INFO
------------
+SHARED_INFO (deprecated)
+------------------------
-The content of the Shared Info page.
+The content of the Shared Info page. This is incorporated into the
+DOMAIN_CONTEXT record as of specification version 4.
0 1 2 3 4 5 6 7 octet
+-------------------------------------------------+
@@ -462,11 +465,12 @@ shared_info Contents of the shared info page. This record
\clearpage
-X86_TSC_INFO
-------------
+X86_TSC_INFO (deprecated)
+-------------------------
Domain TSC information, as accessed by the
-XEN_DOMCTL_{get,set}tscinfo hypercall sub-ops.
+XEN_DOMCTL_{get,set}tscinfo hypercall sub-ops. This is incorporated into the
+DOMAIN_CONTEXT record as of specification version 4.
0 1 2 3 4 5 6 7 octet
+------------------------+------------------------+
@@ -680,6 +684,25 @@ MSR_policy Array of xen_msr_entry_t[]'s
\clearpage
+DOMAIN_CONTEXT
+--------------
+
+Domain context, as accessed by the
+XEN_DOMCTL_{get,set}_domain_context hypercall sub-ops.
+
+ 0 1 2 3 4 5 6 7 octet
+ +-------------------------------------------------+
+ | dom_ctx |
+ ...
+ +-------------------------------------------------+
+
+--------------------------------------------------------------------
+Field Description
+----------- ---------------------------------------------------
+dom_ctx The Domain Context blob from Xen.
+--------------------------------------------------------------------
+
+\clearpage
Layout
======
@@ -706,8 +729,7 @@ A typical save record for an x86 PV guest image would look like:
* STATIC_DATA_END
* X86_PV_P2M_FRAMES record
* Many PAGE_DATA records
-* X86_TSC_INFO
-* SHARED_INFO record
+* DOMAIN_CONTEXT
* VCPU context records for each online VCPU
* X86_PV_VCPU_BASIC record
* X86_PV_VCPU_EXTENDED record
@@ -735,7 +757,7 @@ A typical save record for an x86 HVM guest image would look like:
* X86_{CPUID,MSR}_POLICY
* STATIC_DATA_END
* Many PAGE_DATA records
-* X86_TSC_INFO
+* DOMAIN_CONTEXT
* HVM_PARAMS
* HVM_CONTEXT
* END record
@@ -746,6 +768,18 @@ the validity of architectural state in the context.
Compatibility with older versions
=================================
+v4 compat with v3
+-----------------
+
+A v4 stream is compatible with a v3 stream, but mandates the presence of a
+DOMAIN_CONTEXT record. This incorporates context such as the content of
+the domain's Shared Info page and the TSC information, hence the SHARED_INFO
+and TSC_INFO records are deprecated.
+It also supercedes HVM_CONTEXT and, over time, data that is currently part of
+the HVM_CONTEXT blob will move to the DOMAIN_CONTEXT blob. Xen, however, will
+continue to accept all defined HVM_CONTEXT records so a v4-compatible
+receiver can still accept an unmodified v3 stream.
+
v3 compat with v2
-----------------
diff --git a/tools/libs/guest/xg_sr_stream_format.h b/tools/libs/guest/xg_sr_stream_format.h
index 8a0da26f75..bc538bc192 100644
--- a/tools/libs/guest/xg_sr_stream_format.h
+++ b/tools/libs/guest/xg_sr_stream_format.h
@@ -76,6 +76,7 @@ struct xc_sr_rhdr
#define REC_TYPE_STATIC_DATA_END 0x00000010U
#define REC_TYPE_X86_CPUID_POLICY 0x00000011U
#define REC_TYPE_X86_MSR_POLICY 0x00000012U
+#define REC_TYPE_DOMAIN_CONTEXT 0x00000013U
#define REC_TYPE_OPTIONAL 0x80000000U
--
2.20.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH v10 08/11] docs / tools: specify migration v4 to include DOMAIN_CONTEXT
2020-10-08 18:57 ` [PATCH v10 08/11] docs / tools: specify migration v4 to include DOMAIN_CONTEXT Paul Durrant
@ 2021-01-25 19:43 ` Andrew Cooper
0 siblings, 0 replies; 30+ messages in thread
From: Andrew Cooper @ 2021-01-25 19:43 UTC (permalink / raw)
To: Paul Durrant, xen-devel
Cc: Paul Durrant, Wei Liu, George Dunlap, Ian Jackson, Jan Beulich,
Julien Grall, Stefano Stabellini,
Marek Marczykowski-Górecki
On 08/10/2020 19:57, Paul Durrant wrote:
> diff --git a/docs/specs/libxc-migration-stream.pandoc b/docs/specs/libxc-migration-stream.pandoc
> index 8aeab3b11b..aa6fe284f3 100644
> --- a/docs/specs/libxc-migration-stream.pandoc
> +++ b/docs/specs/libxc-migration-stream.pandoc
> @@ -127,7 +127,7 @@ marker 0xFFFFFFFFFFFFFFFF.
>
> id 0x58454E46 ("XENF" in ASCII).
>
> -version 0x00000003. The version of this specification.
> +version 0x00000004. The version of this specification.
>
> options bit 0: Endianness. 0 = little-endian, 1 = big-endian.
>
> @@ -209,9 +209,9 @@ type 0x00000000: END
>
> 0x00000006: X86_PV_VCPU_XSAVE
>
> - 0x00000007: SHARED_INFO
> + 0x00000007: SHARED_INFO (deprecated)
"in v4" ?
> @@ -442,10 +444,11 @@ X86_PV_VCPU_MSRS XEN_DOMCTL_{get,set}\_vcpu_msrs
>
> \clearpage
>
> -SHARED_INFO
> ------------
> +SHARED_INFO (deprecated)
> +------------------------
>
> -The content of the Shared Info page.
> +The content of the Shared Info page. This is incorporated into the
> +DOMAIN_CONTEXT record as of specification version 4.
This needs to be stricter. A SHARED_INFO frame must not be present in a
v4 stream.
Absolutely nothing good can come from having the state twice. Moreover, ...
>
> 0 1 2 3 4 5 6 7 octet
> +-------------------------------------------------+
> @@ -462,11 +465,12 @@ shared_info Contents of the shared info page. This record
>
> \clearpage
>
> -X86_TSC_INFO
> -------------
> +X86_TSC_INFO (deprecated)
> +-------------------------
>
> Domain TSC information, as accessed by the
> -XEN_DOMCTL_{get,set}tscinfo hypercall sub-ops.
> +XEN_DOMCTL_{get,set}tscinfo hypercall sub-ops. This is incorporated into the
> +DOMAIN_CONTEXT record as of specification version 4.
... it is actively problematic for this one, as incarnation counts the
number of set_tsc_info() hypercalls.
~Andrew
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v10 09/11] tools/python: modify libxc.py to verify v4 stream
2020-10-08 18:57 [PATCH v10 00/11] domain context infrastructure Paul Durrant
` (7 preceding siblings ...)
2020-10-08 18:57 ` [PATCH v10 08/11] docs / tools: specify migration v4 to include DOMAIN_CONTEXT Paul Durrant
@ 2020-10-08 18:57 ` Paul Durrant
2021-01-25 19:45 ` Andrew Cooper
2020-10-08 18:57 ` [PATCH v10 10/11] tools/libs/guest: add code to restore a v4 libxc stream Paul Durrant
` (2 subsequent siblings)
11 siblings, 1 reply; 30+ messages in thread
From: Paul Durrant @ 2020-10-08 18:57 UTC (permalink / raw)
To: xen-devel
Cc: Paul Durrant, Andrew Cooper, Marek Marczykowski-Górecki,
Ian Jackson, Wei Liu
From: Paul Durrant <pdurrant@amazon.com>
This patch adds code to verify the presence of a REC_TYPE_domain_context
record in a v4 stream, as well as absence of REC_TYPE_shared_info and
REC_TYPE_tsc_info records.
Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: "Marek Marczykowski-Górecki" <marmarek@invisiblethingslab.com>
Cc: Ian Jackson <iwj@xenproject.org>
Cc: Wei Liu <wl@xen.org>
v10:
- New in v10
---
tools/python/xen/migration/libxc.py | 22 ++++++++++++++++++++--
1 file changed, 20 insertions(+), 2 deletions(-)
diff --git a/tools/python/xen/migration/libxc.py b/tools/python/xen/migration/libxc.py
index 9881f5ced4..24fb50cbda 100644
--- a/tools/python/xen/migration/libxc.py
+++ b/tools/python/xen/migration/libxc.py
@@ -59,6 +59,7 @@ REC_TYPE_checkpoint_dirty_pfn_list = 0x0000000f
REC_TYPE_static_data_end = 0x00000010
REC_TYPE_x86_cpuid_policy = 0x00000011
REC_TYPE_x86_msr_policy = 0x00000012
+REC_TYPE_domain_context = 0x00000013
rec_type_to_str = {
REC_TYPE_end : "End",
@@ -80,6 +81,7 @@ rec_type_to_str = {
REC_TYPE_static_data_end : "Static data end",
REC_TYPE_x86_cpuid_policy : "x86 CPUID policy",
REC_TYPE_x86_msr_policy : "x86 MSR policy",
+ REC_TYPE_domain_context : "Domain context",
}
# page_data
@@ -156,9 +158,9 @@ class VerifyLibxc(VerifyBase):
raise StreamError("Bad image id: Expected 0x%x, got 0x%x" %
(IHDR_IDENT, ident))
- if not (2 <= version <= 3):
+ if not (2 <= version <= 4):
raise StreamError(
- "Unknown image version: Expected 2 <= ver <= 3, got %d" %
+ "Unknown image version: Expected 2 <= ver <= 4, got %d" %
(version, ))
self.version = version
@@ -362,6 +364,9 @@ class VerifyLibxc(VerifyBase):
def verify_record_shared_info(self, content):
""" shared info record """
+ if self.version >= 4:
+ raise RecordError("Shared info record found in v4 stream")
+
contentsz = len(content)
if contentsz != 4096:
raise RecordError("Length expected to be 4906 bytes, not %d" %
@@ -371,6 +376,9 @@ class VerifyLibxc(VerifyBase):
def verify_record_tsc_info(self, content):
""" tsc info record """
+ if self.version >= 4:
+ raise RecordError("TSC info record found in v4 stream")
+
sz = calcsize(X86_TSC_INFO_FORMAT)
if len(content) != sz:
@@ -476,6 +484,14 @@ class VerifyLibxc(VerifyBase):
raise RecordError("Record length %u, expected multiple of %u" %
(contentsz, sz))
+ def verify_record_domain_context(self, content):
+ """ domain context record """
+
+ if self.version < 4:
+ raise RecordError("Domain context record found in v3 stream")
+
+ if len(content) == 0:
+ raise RecordError("Zero length domain context")
record_verifiers = {
REC_TYPE_end:
@@ -526,4 +542,6 @@ record_verifiers = {
VerifyLibxc.verify_record_x86_cpuid_policy,
REC_TYPE_x86_msr_policy:
VerifyLibxc.verify_record_x86_msr_policy,
+ REC_TYPE_domain_context:
+ VerifyLibxc.verify_record_domain_context,
}
--
2.20.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH v10 09/11] tools/python: modify libxc.py to verify v4 stream
2020-10-08 18:57 ` [PATCH v10 09/11] tools/python: modify libxc.py to verify v4 stream Paul Durrant
@ 2021-01-25 19:45 ` Andrew Cooper
0 siblings, 0 replies; 30+ messages in thread
From: Andrew Cooper @ 2021-01-25 19:45 UTC (permalink / raw)
To: Paul Durrant, xen-devel
Cc: Paul Durrant, Marek Marczykowski-Górecki, Ian Jackson, Wei Liu
On 08/10/2020 19:57, Paul Durrant wrote:
> @@ -476,6 +484,14 @@ class VerifyLibxc(VerifyBase):
> raise RecordError("Record length %u, expected multiple of %u" %
> (contentsz, sz))
>
> + def verify_record_domain_context(self, content):
> + """ domain context record """
> +
> + if self.version < 4:
> + raise RecordError("Domain context record found in v3 stream")
> +
> + if len(content) == 0:
> + raise RecordError("Zero length domain context")
This needs a recursive dissector to validate the domain context format,
as it is not a private ABI within Xen.
~Andrew
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v10 10/11] tools/libs/guest: add code to restore a v4 libxc stream
2020-10-08 18:57 [PATCH v10 00/11] domain context infrastructure Paul Durrant
` (8 preceding siblings ...)
2020-10-08 18:57 ` [PATCH v10 09/11] tools/python: modify libxc.py to verify v4 stream Paul Durrant
@ 2020-10-08 18:57 ` Paul Durrant
2021-01-25 20:04 ` Andrew Cooper
2020-10-08 18:57 ` [PATCH v10 11/11] tools/libs/guest: add code to save " Paul Durrant
2021-01-25 20:15 ` [PATCH v10 00/11] domain context infrastructure Andrew Cooper
11 siblings, 1 reply; 30+ messages in thread
From: Paul Durrant @ 2020-10-08 18:57 UTC (permalink / raw)
To: xen-devel; +Cc: Paul Durrant, Andrew Cooper, Ian Jackson, Wei Liu
From: Paul Durrant <pdurrant@amazon.com>
This patch adds the necessary code to accept a v4 stream, and to recognise and
restore a REC_TYPE_DOMAIN_CONTEXT record.
Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Ian Jackson <iwj@xenproject.org>
Cc: Wei Liu <wl@xen.org>
v10:
- New in v10
- Derived from patch #8 of the v9 series
- Fix memory leak
---
tools/libs/guest/xg_sr_common.c | 1 +
tools/libs/guest/xg_sr_common.h | 3 +++
tools/libs/guest/xg_sr_restore.c | 24 ++++++++++++++++++++++--
tools/libs/guest/xg_sr_restore_x86_hvm.c | 9 +++++++++
tools/libs/guest/xg_sr_restore_x86_pv.c | 9 +++++++++
5 files changed, 44 insertions(+), 2 deletions(-)
diff --git a/tools/libs/guest/xg_sr_common.c b/tools/libs/guest/xg_sr_common.c
index 17567ab133..f813320202 100644
--- a/tools/libs/guest/xg_sr_common.c
+++ b/tools/libs/guest/xg_sr_common.c
@@ -39,6 +39,7 @@ static const char *const mandatory_rec_types[] =
[REC_TYPE_STATIC_DATA_END] = "Static data end",
[REC_TYPE_X86_CPUID_POLICY] = "x86 CPUID policy",
[REC_TYPE_X86_MSR_POLICY] = "x86 MSR policy",
+ [REC_TYPE_DOMAIN_CONTEXT] = "Domain context",
};
const char *rec_type_to_str(uint32_t type)
diff --git a/tools/libs/guest/xg_sr_common.h b/tools/libs/guest/xg_sr_common.h
index cc3ad1c394..ba9e5b0a84 100644
--- a/tools/libs/guest/xg_sr_common.h
+++ b/tools/libs/guest/xg_sr_common.h
@@ -297,6 +297,9 @@ struct xc_sr_context
/* Sender has invoked verify mode on the stream. */
bool verify;
+
+ /* Domain context blob. */
+ struct xc_sr_blob dom_ctx;
} restore;
};
diff --git a/tools/libs/guest/xg_sr_restore.c b/tools/libs/guest/xg_sr_restore.c
index b57a787519..9d2bbdfaa3 100644
--- a/tools/libs/guest/xg_sr_restore.c
+++ b/tools/libs/guest/xg_sr_restore.c
@@ -35,9 +35,9 @@ static int read_headers(struct xc_sr_context *ctx)
return -1;
}
- if ( ihdr.version < 2 || ihdr.version > 3 )
+ if ( ihdr.version < 2 || ihdr.version > 4 )
{
- ERROR("Invalid Version: Expected 2 <= ver <= 3, Got %d",
+ ERROR("Invalid Version: Expected 2 <= ver <= 4, Got %d",
ihdr.version);
return -1;
}
@@ -682,6 +682,21 @@ int handle_static_data_end(struct xc_sr_context *ctx)
return rc;
}
+/*
+ * Process a DOMAIN_CONTEXT record from the stream.
+ */
+static int handle_domain_context(struct xc_sr_context *ctx,
+ struct xc_sr_record *rec)
+{
+ xc_interface *xch = ctx->xch;
+ int rc = update_blob(&ctx->restore.dom_ctx, rec->data, rec->length);
+
+ if ( rc )
+ ERROR("Unable to allocate %u bytes for domain context", rec->length);
+
+ return rc;
+}
+
static int process_record(struct xc_sr_context *ctx, struct xc_sr_record *rec)
{
xc_interface *xch = ctx->xch;
@@ -709,6 +724,10 @@ static int process_record(struct xc_sr_context *ctx, struct xc_sr_record *rec)
rc = handle_static_data_end(ctx);
break;
+ case REC_TYPE_DOMAIN_CONTEXT:
+ rc = handle_domain_context(ctx, rec);
+ break;
+
default:
rc = ctx->restore.ops.process_record(ctx, rec);
break;
@@ -784,6 +803,7 @@ static void cleanup(struct xc_sr_context *ctx)
free(ctx->restore.buffered_records);
free(ctx->restore.populated_pfns);
+ free(ctx->restore.dom_ctx.ptr);
if ( ctx->restore.ops.cleanup(ctx) )
PERROR("Failed to clean up");
diff --git a/tools/libs/guest/xg_sr_restore_x86_hvm.c b/tools/libs/guest/xg_sr_restore_x86_hvm.c
index d6ea6f3012..6bb164b9f0 100644
--- a/tools/libs/guest/xg_sr_restore_x86_hvm.c
+++ b/tools/libs/guest/xg_sr_restore_x86_hvm.c
@@ -225,6 +225,15 @@ static int x86_hvm_stream_complete(struct xc_sr_context *ctx)
return rc;
}
+ rc = xc_domain_set_context(xch, ctx->domid,
+ ctx->restore.dom_ctx.ptr,
+ ctx->restore.dom_ctx.size);
+ if ( rc )
+ {
+ PERROR("Unable to restore Domain context");
+ return rc;
+ }
+
rc = xc_dom_gnttab_seed(xch, ctx->domid, true,
ctx->restore.console_gfn,
ctx->restore.xenstore_gfn,
diff --git a/tools/libs/guest/xg_sr_restore_x86_pv.c b/tools/libs/guest/xg_sr_restore_x86_pv.c
index dc50b0f5a8..2dafad7b83 100644
--- a/tools/libs/guest/xg_sr_restore_x86_pv.c
+++ b/tools/libs/guest/xg_sr_restore_x86_pv.c
@@ -1134,6 +1134,15 @@ static int x86_pv_stream_complete(struct xc_sr_context *ctx)
if ( rc )
return rc;
+ rc = xc_domain_set_context(xch, ctx->domid,
+ ctx->restore.dom_ctx.ptr,
+ ctx->restore.dom_ctx.size);
+ if ( rc )
+ {
+ PERROR("Unable to restore Domain context");
+ return rc;
+ }
+
rc = xc_dom_gnttab_seed(xch, ctx->domid, false,
ctx->restore.console_gfn,
ctx->restore.xenstore_gfn,
--
2.20.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH v10 10/11] tools/libs/guest: add code to restore a v4 libxc stream
2020-10-08 18:57 ` [PATCH v10 10/11] tools/libs/guest: add code to restore a v4 libxc stream Paul Durrant
@ 2021-01-25 20:04 ` Andrew Cooper
0 siblings, 0 replies; 30+ messages in thread
From: Andrew Cooper @ 2021-01-25 20:04 UTC (permalink / raw)
To: Paul Durrant, xen-devel; +Cc: Paul Durrant, Ian Jackson, Wei Liu
On 08/10/2020 19:57, Paul Durrant wrote:
> From: Paul Durrant <pdurrant@amazon.com>
>
> This patch adds the necessary code to accept a v4 stream, and to recognise and
> restore a REC_TYPE_DOMAIN_CONTEXT record.
>
> Signed-off-by: Paul Durrant <pdurrant@amazon.com>
Somewhere within this needs to be logic to reject the forbidden records
in relevant stream versions.
> diff --git a/tools/libs/guest/xg_sr_restore_x86_hvm.c b/tools/libs/guest/xg_sr_restore_x86_hvm.c
> index d6ea6f3012..6bb164b9f0 100644
> --- a/tools/libs/guest/xg_sr_restore_x86_hvm.c
> +++ b/tools/libs/guest/xg_sr_restore_x86_hvm.c
> @@ -225,6 +225,15 @@ static int x86_hvm_stream_complete(struct xc_sr_context *ctx)
> return rc;
> }
>
> + rc = xc_domain_set_context(xch, ctx->domid,
> + ctx->restore.dom_ctx.ptr,
> + ctx->restore.dom_ctx.size);
> + if ( rc )
> + {
> + PERROR("Unable to restore Domain context");
> + return rc;
> + }
This doesn't match where you specified the record to live in the stream,
and in particular is reordered WRT HVMCONTEXT restoration.
Also, it appears to be in the middle of a block of code which needs to
become `if ( guest-aware )`.
> +
> rc = xc_dom_gnttab_seed(xch, ctx->domid, true,
> ctx->restore.console_gfn,
> ctx->restore.xenstore_gfn,
> diff --git a/tools/libs/guest/xg_sr_restore_x86_pv.c b/tools/libs/guest/xg_sr_restore_x86_pv.c
> index dc50b0f5a8..2dafad7b83 100644
> --- a/tools/libs/guest/xg_sr_restore_x86_pv.c
> +++ b/tools/libs/guest/xg_sr_restore_x86_pv.c
> @@ -1134,6 +1134,15 @@ static int x86_pv_stream_complete(struct xc_sr_context *ctx)
> if ( rc )
> return rc;
>
> + rc = xc_domain_set_context(xch, ctx->domid,
> + ctx->restore.dom_ctx.ptr,
> + ctx->restore.dom_ctx.size);
> + if ( rc )
> + {
> + PERROR("Unable to restore Domain context");
> + return rc;
> + }
Similar comment as HVM for the reordering. PV guests in particular tend
to be far more sensitive to the restoration order.
~Andrew
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v10 11/11] tools/libs/guest: add code to save a v4 libxc stream
2020-10-08 18:57 [PATCH v10 00/11] domain context infrastructure Paul Durrant
` (9 preceding siblings ...)
2020-10-08 18:57 ` [PATCH v10 10/11] tools/libs/guest: add code to restore a v4 libxc stream Paul Durrant
@ 2020-10-08 18:57 ` Paul Durrant
2021-01-25 20:11 ` Andrew Cooper
2021-01-25 20:15 ` [PATCH v10 00/11] domain context infrastructure Andrew Cooper
11 siblings, 1 reply; 30+ messages in thread
From: Paul Durrant @ 2020-10-08 18:57 UTC (permalink / raw)
To: xen-devel; +Cc: Paul Durrant, Andrew Cooper, Ian Jackson, Wei Liu
From: Paul Durrant <pdurrant@amazon.com>
This patch adds the necessary code to save a REC_TYPE_DOMAIN_CONTEXT record,
and stop saving the now obsolete REC_TYPE_SHARED_INFO and REC_TYPE_TSC_INFO
records for PV guests.
Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Ian Jackson <iwj@xenproject.org>
Cc: Wei Liu <wl@xen.org>
v10:
- New in v10
- Derived from patch #8 of the v9 series
---
tools/libs/guest/xg_sr_common_x86.c | 20 -----------
tools/libs/guest/xg_sr_common_x86.h | 6 ----
tools/libs/guest/xg_sr_save.c | 52 ++++++++++++++++++++++++++-
tools/libs/guest/xg_sr_save_x86_hvm.c | 5 ---
tools/libs/guest/xg_sr_save_x86_pv.c | 22 ------------
5 files changed, 51 insertions(+), 54 deletions(-)
diff --git a/tools/libs/guest/xg_sr_common_x86.c b/tools/libs/guest/xg_sr_common_x86.c
index 6f12483907..10a35b998e 100644
--- a/tools/libs/guest/xg_sr_common_x86.c
+++ b/tools/libs/guest/xg_sr_common_x86.c
@@ -1,25 +1,5 @@
#include "xg_sr_common_x86.h"
-int write_x86_tsc_info(struct xc_sr_context *ctx)
-{
- xc_interface *xch = ctx->xch;
- struct xc_sr_rec_x86_tsc_info tsc = {};
- struct xc_sr_record rec = {
- .type = REC_TYPE_X86_TSC_INFO,
- .length = sizeof(tsc),
- .data = &tsc,
- };
-
- if ( xc_domain_get_tsc_info(xch, ctx->domid, &tsc.mode,
- &tsc.nsec, &tsc.khz, &tsc.incarnation) < 0 )
- {
- PERROR("Unable to obtain TSC information");
- return -1;
- }
-
- return write_record(ctx, &rec);
-}
-
int handle_x86_tsc_info(struct xc_sr_context *ctx, struct xc_sr_record *rec)
{
xc_interface *xch = ctx->xch;
diff --git a/tools/libs/guest/xg_sr_common_x86.h b/tools/libs/guest/xg_sr_common_x86.h
index b55758c96d..e504169705 100644
--- a/tools/libs/guest/xg_sr_common_x86.h
+++ b/tools/libs/guest/xg_sr_common_x86.h
@@ -3,12 +3,6 @@
#include "xg_sr_common.h"
-/*
- * Obtains a domains TSC information from Xen and writes a X86_TSC_INFO record
- * into the stream.
- */
-int write_x86_tsc_info(struct xc_sr_context *ctx);
-
/*
* Parses a X86_TSC_INFO record and applies the result to the domain.
*/
diff --git a/tools/libs/guest/xg_sr_save.c b/tools/libs/guest/xg_sr_save.c
index 2ba7c3200c..3eecc3e987 100644
--- a/tools/libs/guest/xg_sr_save.c
+++ b/tools/libs/guest/xg_sr_save.c
@@ -13,7 +13,7 @@ static int write_headers(struct xc_sr_context *ctx, uint16_t guest_type)
struct xc_sr_ihdr ihdr = {
.marker = IHDR_MARKER,
.id = htonl(IHDR_ID),
- .version = htonl(3),
+ .version = htonl(4),
.options = htons(IHDR_OPT_LITTLE_ENDIAN),
};
struct xc_sr_dhdr dhdr = {
@@ -44,6 +44,52 @@ static int write_headers(struct xc_sr_context *ctx, uint16_t guest_type)
return 0;
}
+/*
+ * Writes a DOMAIN_CONTEXT record into the stream.
+ */
+static int write_domain_context_record(struct xc_sr_context *ctx)
+{
+ xc_interface *xch = ctx->xch;
+ struct xc_sr_record rec = {
+ .type = REC_TYPE_DOMAIN_CONTEXT,
+ };
+ size_t len = 0;
+ int rc;
+
+ rc = xc_domain_get_context(xch, ctx->domid, NULL, &len);
+ if ( rc < 0 )
+ {
+ PERROR("can't get record length for dom %u\n", ctx->domid);
+ goto out;
+ }
+
+ rec.data = malloc(len);
+
+ rc = -1;
+ if ( !rec.data )
+ {
+ PERROR("can't allocate %lu bytes\n", len);
+ goto out;
+ }
+
+ rc = xc_domain_get_context(xch, ctx->domid, rec.data, &len);
+ if ( rc < 0 )
+ {
+ PERROR("can't get domain record for dom %u\n", ctx->domid);
+ goto out;
+ }
+
+ rec.length = len;
+ rc = write_record(ctx, &rec);
+ if ( rc < 0 )
+ PERROR("failed to write DOMAIN_CONTEXT record");
+
+ out:
+ free(rec.data);
+
+ return rc;
+}
+
/*
* Writes an END record into the stream.
*/
@@ -905,6 +951,10 @@ static int save(struct xc_sr_context *ctx, uint16_t guest_type)
goto err;
}
+ rc = write_domain_context_record(ctx);
+ if ( rc )
+ goto err;
+
rc = ctx->save.ops.end_of_checkpoint(ctx);
if ( rc )
goto err;
diff --git a/tools/libs/guest/xg_sr_save_x86_hvm.c b/tools/libs/guest/xg_sr_save_x86_hvm.c
index 1634a7bc43..d44fb3fc4f 100644
--- a/tools/libs/guest/xg_sr_save_x86_hvm.c
+++ b/tools/libs/guest/xg_sr_save_x86_hvm.c
@@ -193,11 +193,6 @@ static int x86_hvm_end_of_checkpoint(struct xc_sr_context *ctx)
{
int rc;
- /* Write the TSC record. */
- rc = write_x86_tsc_info(ctx);
- if ( rc )
- return rc;
-
/* Write the HVM_CONTEXT record. */
rc = write_hvm_context(ctx);
if ( rc )
diff --git a/tools/libs/guest/xg_sr_save_x86_pv.c b/tools/libs/guest/xg_sr_save_x86_pv.c
index 4964f1f7b8..3de7b19f54 100644
--- a/tools/libs/guest/xg_sr_save_x86_pv.c
+++ b/tools/libs/guest/xg_sr_save_x86_pv.c
@@ -849,20 +849,6 @@ static int write_x86_pv_p2m_frames(struct xc_sr_context *ctx)
return rc;
}
-/*
- * Writes an SHARED_INFO record into the stream.
- */
-static int write_shared_info(struct xc_sr_context *ctx)
-{
- struct xc_sr_record rec = {
- .type = REC_TYPE_SHARED_INFO,
- .length = PAGE_SIZE,
- .data = ctx->x86.pv.shinfo,
- };
-
- return write_record(ctx, &rec);
-}
-
/*
* Normalise a pagetable for the migration stream. Performs mfn->pfn
* conversions on the ptes.
@@ -1093,14 +1079,6 @@ static int x86_pv_end_of_checkpoint(struct xc_sr_context *ctx)
{
int rc;
- rc = write_x86_tsc_info(ctx);
- if ( rc )
- return rc;
-
- rc = write_shared_info(ctx);
- if ( rc )
- return rc;
-
rc = write_all_vcpu_information(ctx);
if ( rc )
return rc;
--
2.20.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH v10 11/11] tools/libs/guest: add code to save a v4 libxc stream
2020-10-08 18:57 ` [PATCH v10 11/11] tools/libs/guest: add code to save " Paul Durrant
@ 2021-01-25 20:11 ` Andrew Cooper
0 siblings, 0 replies; 30+ messages in thread
From: Andrew Cooper @ 2021-01-25 20:11 UTC (permalink / raw)
To: Paul Durrant, xen-devel; +Cc: Paul Durrant, Ian Jackson, Wei Liu
On 08/10/2020 19:57, Paul Durrant wrote:
> From: Paul Durrant <pdurrant@amazon.com>
>
> This patch adds the necessary code to save a REC_TYPE_DOMAIN_CONTEXT record,
> and stop saving the now obsolete REC_TYPE_SHARED_INFO and REC_TYPE_TSC_INFO
> records for PV guests.
>
> Signed-off-by: Paul Durrant <pdurrant@amazon.com>
Looks broadly ok.
> diff --git a/tools/libs/guest/xg_sr_common_x86.h b/tools/libs/guest/xg_sr_common_x86.h
> index b55758c96d..e504169705 100644
> --- a/tools/libs/guest/xg_sr_common_x86.h
> +++ b/tools/libs/guest/xg_sr_common_x86.h
> @@ -44,6 +44,52 @@ static int write_headers(struct xc_sr_context *ctx, uint16_t guest_type)
> return 0;
> }
>
> +/*
> + * Writes a DOMAIN_CONTEXT record into the stream.
> + */
> +static int write_domain_context_record(struct xc_sr_context *ctx)
> +{
> + xc_interface *xch = ctx->xch;
> + struct xc_sr_record rec = {
> + .type = REC_TYPE_DOMAIN_CONTEXT,
> + };
> + size_t len = 0;
> + int rc;
> +
> + rc = xc_domain_get_context(xch, ctx->domid, NULL, &len);
> + if ( rc < 0 )
> + {
> + PERROR("can't get record length for dom %u\n", ctx->domid);
> + goto out;
> + }
> +
> + rec.data = malloc(len);
> +
> + rc = -1;
> + if ( !rec.data )
> + {
> + PERROR("can't allocate %lu bytes\n", len);
%zu, because not all versions of C have size_t the same as unsigned long.
> + goto out;
> + }
> +
> + rc = xc_domain_get_context(xch, ctx->domid, rec.data, &len);
> + if ( rc < 0 )
> + {
> + PERROR("can't get domain record for dom %u\n", ctx->domid);
"domain context", and above.
> diff --git a/tools/libs/guest/xg_sr_save_x86_pv.c b/tools/libs/guest/xg_sr_save_x86_pv.c
> index 4964f1f7b8..3de7b19f54 100644
> --- a/tools/libs/guest/xg_sr_save_x86_pv.c
> +++ b/tools/libs/guest/xg_sr_save_x86_pv.c
> @@ -849,20 +849,6 @@ static int write_x86_pv_p2m_frames(struct xc_sr_context *ctx)
> return rc;
> }
>
> -/*
> - * Writes an SHARED_INFO record into the stream.
> - */
> -static int write_shared_info(struct xc_sr_context *ctx)
> -{
> - struct xc_sr_record rec = {
> - .type = REC_TYPE_SHARED_INFO,
> - .length = PAGE_SIZE,
> - .data = ctx->x86.pv.shinfo,
> - };
> -
> - return write_record(ctx, &rec);
> -}
This change also wants to strip out ctx->x86.pv.shinfo, and the mapping
logic.
~Andrew
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v10 00/11] domain context infrastructure
2020-10-08 18:57 [PATCH v10 00/11] domain context infrastructure Paul Durrant
` (10 preceding siblings ...)
2020-10-08 18:57 ` [PATCH v10 11/11] tools/libs/guest: add code to save " Paul Durrant
@ 2021-01-25 20:15 ` Andrew Cooper
11 siblings, 0 replies; 30+ messages in thread
From: Andrew Cooper @ 2021-01-25 20:15 UTC (permalink / raw)
To: Paul Durrant, xen-devel
Cc: Paul Durrant, Daniel De Graaf, George Dunlap, Ian Jackson,
Jan Beulich, Julien Grall, Marek Marczykowski-Górecki,
Roger Pau Monné,
Stefano Stabellini, Volodymyr Babchuk, Wei Liu
On 08/10/2020 19:57, Paul Durrant wrote:
> From: Paul Durrant <pdurrant@amazon.com>
>
> Paul Durrant (11):
> docs / include: introduce a new framework for 'domain context' records
> xen: introduce implementation of save/restore of 'domain context'
> xen/common/domctl: introduce XEN_DOMCTL_get/set_domain_context
> tools/misc: add xen-domctx to present domain context
> common/domain: add a domain context record for shared_info...
> x86/time: add a domain context record for tsc_info...
> docs/specs: add missing definitions to libxc-migration-stream
> docs / tools: specify migration v4 to include DOMAIN_CONTEXT
> tools/python: modify libxc.py to verify v4 stream
> tools/libs/guest: add code to restore a v4 libxc stream
> tools/libs/guest: add code to save a v4 libxc stream
Thanks - this is much better when it comes to the public API/ABI.
However, my concerns still stands. What *else* is going in the domain
context record, because you can pull the "bump the interface version and
deprecated new fields" exactly once, as the libxg logic doesn't delve
into domain context stream.
At the moment this does increase the downtime of the VM for no gain.
What I'm trying to understand is whether this is "no gain (yet)" or
whether you consider this "done" as far as cooperative migrate is concerned.
~Andrew
^ permalink raw reply [flat|nested] 30+ messages in thread