[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 4/5] common/domain: add a domain context record for shared_info...
HI Paul,
On 27/03/2020 18:50, Paul Durrant wrote:
... and update xen-ctx to dump some information describing the record.
The commit message should contain a little bit more context why we are
saving/restore the shared_info.
NOTE: To allow a sensible definition of the record in public/save.h
this patch also adds a definition of the Xen ABI's de-facto page
size into public/xen.h.
Signed-off-by: Paul Durrant <paul@xxxxxxx>
---
Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
Cc: Wei Liu <wl@xxxxxxx>
Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
Cc: George Dunlap <george.dunlap@xxxxxxxxxx>
Cc: Jan Beulich <jbeulich@xxxxxxxx>
Cc: Julien Grall <julien@xxxxxxx>
Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
---
tools/misc/xen-ctx.c | 8 ++++++
xen/common/domain.c | 55 +++++++++++++++++++++++++++++++++++++++
xen/include/public/save.h | 10 ++++++-
xen/include/public/xen.h | 3 +++
4 files changed, 75 insertions(+), 1 deletion(-)
diff --git a/tools/misc/xen-ctx.c b/tools/misc/xen-ctx.c
index c31dd5d8e9..8f9692843b 100644
--- a/tools/misc/xen-ctx.c
+++ b/tools/misc/xen-ctx.c
@@ -57,6 +57,13 @@ static void dump_header(void)
h.magic, h.version);
}
+static void dump_shared_info(void)
+{
+ DOMAIN_SAVE_TYPE(SHARED_INFO) s;
+ READ(s);
+ printf(" SHARED_INFO: field_width %u\n", s.field_width);
+}
+
static void dump_end(void)
{
DOMAIN_SAVE_TYPE(END) e;
@@ -124,6 +131,7 @@ int main(int argc, char **argv)
switch (desc.typecode)
{
case DOMAIN_SAVE_CODE(HEADER): dump_header(); break;
+ case DOMAIN_SAVE_CODE(SHARED_INFO): dump_shared_info(); break;
case DOMAIN_SAVE_CODE(END): dump_end(); return 0;
default:
printf("Unknown type %u: skipping\n", desc.typecode);
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 3dcd73f67c..484f6bde13 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>
@@ -1646,6 +1647,60 @@ int continue_hypercall_on_cpu(
return 0;
}
+static int save_shared_info(const struct vcpu *v, struct domain_context *c,
+ bool dry_run)
+{
+ struct domain *d = v->domain;
+ struct domain_shared_info_context ctxt = {};
IHMO, storing 4K worth of data on the stack is a pretty bad idea.
Looking at the code...
+
+ if ( !dry_run )
+ {
+ memcpy(ctxt.buffer, d->shared_info, PAGE_SIZE);
... it feels to me the content of the shared_info should be directly
written in the stream.
+ ctxt.field_width = has_32bit_shinfo(d) ? 4 : 8;
+ }
+
+ return DOMAIN_SAVE_ENTRY(SHARED_INFO, c, v, &ctxt, sizeof(ctxt));
+}
+
+static int load_shared_info(struct vcpu *v, struct domain_context *c)
+{
+ struct domain *d = v->domain;
+ struct domain_shared_info_context ctxt;
+ unsigned int i;
+ int rc;
+
+ rc = DOMAIN_LOAD_ENTRY(SHARED_INFO, c, v, &ctxt, sizeof(ctxt), true);
+ if ( rc )
+ return rc;
+
+ for ( i = 0; i < ARRAY_SIZE(ctxt.pad); i++ )
+ if ( ctxt.pad[i] )
+ return -EINVAL;
+
+ switch ( ctxt.field_width )
+ {
+ case 4:
+ d->arch.has_32bit_shinfo = 1;
I don't think this will compile on Arm.
+ break;
+
+ case 8:
+ d->arch.has_32bit_shinfo = 0;
+ break;
+
+ default:
+ rc = -EINVAL;
+ break;
+ }
+
+ if ( !rc )
+ memcpy(d->shared_info, ctxt.buffer, PAGE_SIZE);
+
+ return rc;
+}
+
+DOMAIN_REGISTER_SAVE_RESTORE(SHARED_INFO, false, 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 84981cd0f6..ff804a7dbf 100644
--- a/xen/include/public/save.h
+++ b/xen/include/public/save.h
@@ -69,6 +69,14 @@ struct domain_save_header {
};
DECLARE_DOMAIN_SAVE_TYPE(HEADER, 1, struct domain_save_header);
-#define DOMAIN_SAVE_CODE_MAX 1
+struct domain_shared_info_context {
+ uint8_t buffer[XEN_PAGE_SIZE];
In the load/save code, you are using PAGE_SIZE but here you are using
XEN_PAGE_SIZE. I don't think there are any promise they will be the same
value. In particular...
+ uint8_t field_width;
+ uint8_t pad[7];
+};
+
+DECLARE_DOMAIN_SAVE_TYPE(SHARED_INFO, 2, struct domain_shared_info_context);
+
+#define DOMAIN_SAVE_CODE_MAX 2
#endif /* __XEN_PUBLIC_SAVE_H__ */
diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
index 75b1619d0d..cbf603f289 100644
--- a/xen/include/public/xen.h
+++ b/xen/include/public/xen.h
@@ -37,6 +37,9 @@
#error "Unsupported architecture"
#endif
+/* The Xen ABI assumes a page size of 4k. */
+#define XEN_PAGE_SIZE 4096
... this is not correct. Xen ABI is based on the page granularity used
by Xen. It happens to be 4KB today but that's because no-one build yet a
64KB/16KB hypervisor.
If someone tomorrow decides to add support for 64KB, then using 4KB in
the ABI by default is not going to fly.
Imagine the guest only give access to 4KB region, how do you handle it
if your minimum granularity in the hypervisor is 64KB?
In theory, it will not require much effort to get Xen on Arm built with
64KB/16KB support as long as guest is using a page granularity higher
than what Xen is using.
I would much prefer if we encode the page size in the stream. We can
then use to retrieve data based on the page size.
Regarding the shared_info itself. Why do you need to know the size in
tools? Would not it be enough to save the real size in
domain_shared_info_context?
Cheers,
--
Julien Grall
|
