Re: [PATCH 01/15] xen/commom/dt-overlay: Fix missing lock when remove the device
On 24.04.2024 05:34, Henry Wang wrote:
> --- a/xen/common/dt-overlay.c
> +++ b/xen/common/dt-overlay.c
> @@ -381,9 +381,14 @@ static int remove_node_resources(struct dt_device_node
> *device_node)
> {
> if ( dt_device_is_protected(device_node) )
> {
> +write_lock(_host_lock);
> rc = iommu_remove_dt_device(device_node);
Any particular reason you add two call sites to the unlock function,
instead of putting it here?
Jan
> if ( rc < 0 )
> +{
> +write_unlock(_host_lock);
> return rc;
> +}
> +write_unlock(_host_lock);
> }
> }
>
Re: [PATCH 5/6] x86/alternative: Relocate all insn-relative fields
On 23.04.2024 16:59, Jan Beulich wrote:
> On 22.04.2024 20:14, Andrew Cooper wrote:
>> --- a/xen/arch/x86/alternative.c
>> +++ b/xen/arch/x86/alternative.c
>> @@ -244,10 +244,31 @@ static void init_or_livepatch
>> _apply_alternatives(struct alt_instr *start,
>>
>> memcpy(buf, repl, a->repl_len);
>>
>> +/* Walk buf[] and adjust any insn-relative operands. */
>> +if ( a->repl_len )
>> {
>> -/* 0xe8/0xe9 are relative branches; fix the offset. */
>> -if ( a->repl_len >= 5 && (*buf & 0xfe) == 0xe8 )
>> +uint8_t *ip = buf, *end = ip + a->repl_len;
>> +
>> +for ( x86_decode_lite_t res; ip < end; ip += res.len )
>> {
>> +int32_t *d32;
>> +uint8_t *target;
>> +
>> +res = x86_decode_lite(ip, end);
>> +
>> +if ( res.len <= 0 )
>> +{
>> +printk("Alternative for %ps [%*ph]\n",
>> + ALT_ORIG_PTR(a), a->repl_len, repl);
>> +printk("Unable to decode instruction in alternative -
>> ignoring.\n");
>> +goto skip_this_alternative;
>
> Can this really be just a log message? There are cases where patching has
> to happen for things to operate correctly. Hence if not panic()ing, I'd
> say we at least want to taint the hypervisor.
Actually, after some further thought, I don't even think we should skip
such alternatives. Think of e.g. cases where in principle we could get
away with just patching the prefix of an insn. Yet even without such
trickery - there's a fair chance that the alternative doesn't need
fiddling with, and hence putting it in unaltered is likely the best we
can do here.
Jan
[xen-unstable-smoke test] 185781: tolerable all pass - PUSHED
flight 185781 xen-unstable-smoke real [real] http://logs.test-lab.xenproject.org/osstest/logs/185781/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 16 saverestore-support-checkfail never pass test-armhf-armhf-xl 15 migrate-support-checkfail never pass test-armhf-armhf-xl 16 saverestore-support-checkfail never pass version targeted for testing: xen 410ef3343924b5a3928bbe8e392491992b322cf0 baseline version: xen 77e25f0e30ddd11e043e6fce84bf108ce7de5b6f Last test of basis 185775 2024-04-23 20:00:26 Z0 days Testing same since 185781 2024-04-24 02:00:26 Z0 days1 attempts People who touched revisions under test: Petr Beneš Tamas K Lengyel jobs: build-arm64-xsm pass build-amd64 pass build-armhf pass build-amd64-libvirt pass test-armhf-armhf-xl pass test-arm64-arm64-xl-xsm pass test-amd64-amd64-xl-qemuu-debianhvm-amd64pass test-amd64-amd64-libvirt pass sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master Test harness code can be found at http://xenbits.xen.org/gitweb?p=osstest.git;a=summary Pushing revision : To xenbits.xen.org:/home/xen/git/xen.git 77e25f0e30..410ef33439 410ef3343924b5a3928bbe8e392491992b322cf0 -> smoke
[PATCH 13/15] xl/overlay: add remove operation to xenstore
From: Vikram Garhwal
Add 3 new command line parameters to the xl overlay command: overlay
name, type and partial. Pass these paramters to the domU via xenstore.
Also introduce support for "operation" in xenstore: it can be "add" or
"remove". In case of "remove", the overlay is to be removed from the
domU device tree.
Signed-off-by: Vikram Garhwal
Signed-off-by: Stefano Stabellini
Signed-off-by: Henry Wang
---
tools/xl/xl_vmcontrol.c | 184 +---
1 file changed, 173 insertions(+), 11 deletions(-)
diff --git a/tools/xl/xl_vmcontrol.c b/tools/xl/xl_vmcontrol.c
index 2bf76dd389..ddd6e9e370 100644
--- a/tools/xl/xl_vmcontrol.c
+++ b/tools/xl/xl_vmcontrol.c
@@ -1466,8 +1466,123 @@ static uint32_t get_num_pages(struct xs_handle *xs,
const char *xs_path)
return num_pages;
}
+static bool write_overlay_operation(struct xs_handle *xs, char *operation,
+ char *path)
+{
+xs_transaction_t xs_trans = XBT_NULL;
+char buf[128];
+char ref[64];
+
+retry_transaction:
+xs_trans = xs_transaction_start(xs);
+if (!xs_trans)
+return false;
+
+snprintf(ref, sizeof(ref), "%s", operation);
+snprintf(buf, sizeof(buf), "%s/overlay-operation", path);
+
+if (!xs_write(xs, xs_trans, buf, ref, strlen(ref)))
+return false;
+
+if (!xs_transaction_end(xs, xs_trans, 0)) {
+if (errno == EAGAIN)
+goto retry_transaction;
+else
+return false;
+}
+
+return true;
+}
+
+static bool write_overlay_name(struct xs_handle *xs, char *name,
+ char *path)
+{
+xs_transaction_t xs_trans = XBT_NULL;
+char buf[128];
+char ref[64];
+
+retry_transaction:
+xs_trans = xs_transaction_start(xs);
+if (!xs_trans)
+return false;
+
+snprintf(ref, sizeof(ref), "%s", name);
+snprintf(buf, sizeof(buf), "%s/overlay-name", path);
+
+if (!xs_write(xs, xs_trans, buf, ref, strlen(ref)))
+return false;
+
+if (!xs_transaction_end(xs, xs_trans, 0)) {
+if (errno == EAGAIN)
+goto retry_transaction;
+else
+return false;
+}
+
+return true;
+}
+
+static bool write_overlay_type(struct xs_handle *xs, char *type,
+ char *path)
+{
+xs_transaction_t xs_trans = XBT_NULL;
+char buf[128];
+char ref[64];
+
+retry_transaction:
+xs_trans = xs_transaction_start(xs);
+if (!xs_trans)
+return false;
+
+snprintf(ref, sizeof(ref), "%s", type);
+snprintf(buf, sizeof(buf), "%s/overlay-type", path);
+
+if (!xs_write(xs, xs_trans, buf, ref, strlen(ref)))
+return false;
+
+if (!xs_transaction_end(xs, xs_trans, 0)) {
+if (errno == EAGAIN)
+goto retry_transaction;
+else
+return false;
+}
+
+return true;
+}
+
+static bool write_overlay_partial(struct xs_handle *xs, bool is_partial,
+ char *path)
+{
+xs_transaction_t xs_trans = XBT_NULL;
+char buf[128];
+char ref[4];
+
+retry_transaction:
+xs_trans = xs_transaction_start(xs);
+if (!xs_trans)
+return false;
+
+snprintf(ref, sizeof(ref), "%d", is_partial);
+snprintf(buf, sizeof(buf), "%s/overlay-partial", path);
+
+if (!xs_write(xs, xs_trans, buf, ref, strlen(ref)))
+return false;
+
+if (!xs_transaction_end(xs, xs_trans, 0)) {
+if (errno == EAGAIN)
+goto retry_transaction;
+else
+return false;
+}
+
+return true;
+}
+
+
static int share_overlay_with_domu(void *overlay_dt_domU, int overlay_dt_size,
- int domain_id)
+ int domain_id, char *overlay_ops,
+ char *overlay_name,
+ char *overlay_type, bool is_overlay_partial)
{
struct xs_handle *xs = NULL;
char *path = NULL;
@@ -1574,6 +1689,34 @@ static int share_overlay_with_domu(void
*overlay_dt_domU, int overlay_dt_size,
goto out;
}
+/* write overlay ops */
+if (!write_overlay_operation(xs, overlay_ops, path)) {
+err = ERROR_FAIL;
+fprintf(stderr,"Writing overlay_ops ready failed\n");
+goto out;
+}
+
+/* Write the overlay-name. */
+if (!write_overlay_name(xs, overlay_name, path)) {
+err = ERROR_FAIL;
+fprintf(stderr,"Writing overlay_name ready failed\n");
+goto out;
+}
+
+/* Write the overlay-type. */
+if (!write_overlay_type(xs, overlay_type, path)) {
+err = ERROR_FAIL;
+fprintf(stderr,"Writing overlay_type ready failed\n");
+goto out;
+}
+
+/* Write the overlay-partial. */
+if (!write_overlay_partial(xs, is_overlay_partial, path)) {
+err = ERROR_FAIL;
+fprintf(stderr,"Writing overlay_partial ready failed\n");
+goto out;
+}
+
/* Write the status
[PATCH 12/15] get_overlay: remove domU overlay
From: Vikram Garhwal
Retrieve 4 new parameters from xenstore: overlay name, type, whether it
is a partial overlay and operation. Operation can be "add" or "remove".
Add correspond to existing mode of operation. Remove introduces support
for removing an overlay from a domU.
Signed-off-by: Vikram Garhwal
Signed-off-by: Stewart Hildebrand
Signed-off-by: Stefano Stabellini
Signed-off-by: Henry Wang
---
tools/helpers/get_overlay.c | 132 +---
1 file changed, 123 insertions(+), 9 deletions(-)
diff --git a/tools/helpers/get_overlay.c b/tools/helpers/get_overlay.c
index ca3007570e..daa697ca04 100644
--- a/tools/helpers/get_overlay.c
+++ b/tools/helpers/get_overlay.c
@@ -66,6 +66,33 @@ retry_transaction:
snprintf(ref, sizeof(ref), "%s", "not_ready");
snprintf(buf, sizeof(buf), "%s/sender-status", xs_base);
+if (!xs_write(xs, xs_trans, buf, ref, strlen(ref)))
+goto fail_xs_transaction;
+if (!xs_set_permissions(xs, xs_trans, buf, perms, 2))
+goto fail_xs_transaction;
+
+/* Create overlay-name node. */
+snprintf(ref, sizeof(ref), "%s", "overlay_node");
+snprintf(buf, sizeof(buf), "%s/overlay-name", xs_base);
+
+if (!xs_write(xs, xs_trans, buf, ref, strlen(ref)))
+goto fail_xs_transaction;
+if (!xs_set_permissions(xs, xs_trans, buf, perms, 2))
+goto fail_xs_transaction;
+
+/* Create overlay-type node. */
+snprintf(ref, sizeof(ref), "%s", "type");
+snprintf(buf, sizeof(buf), "%s/overlay-type", xs_base);
+
+if (!xs_write(xs, xs_trans, buf, ref, strlen(ref)))
+goto fail_xs_transaction;
+if (!xs_set_permissions(xs, xs_trans, buf, perms, 2))
+goto fail_xs_transaction;
+
+/* Create overlay-partial node. */
+snprintf(ref, sizeof(ref), "%d", 0);
+snprintf(buf, sizeof(buf), "%s/overlay-partial", xs_base);
+
if (!xs_write(xs, xs_trans, buf, ref, strlen(ref)))
goto fail_xs_transaction;
if (!xs_set_permissions(xs, xs_trans, buf, perms, 2))
@@ -174,7 +201,7 @@ static bool wait_for_status(struct xs_handle *xs, int fd,
char *status_path,
}
static bool write_page_ref(struct xs_handle *xs, uint32_t *page_ref,
- uint32_t num_pages, char *path)
+ uint32_t num_pages, const char *path)
{
xs_transaction_t xs_trans = XBT_NULL;
char buf[128];
@@ -249,12 +276,69 @@ retry_transaction:
return true;
}
+static char *get_overlay_ops(struct xs_handle *xs, const char *xs_path)
+{
+char buf[128];
+char *ref = NULL;
+unsigned int len;
+
+snprintf(buf, sizeof(buf), "%s/overlay-operation", xs_path);
+
+ref = xs_read(xs, XBT_NULL, buf, );
+
+return ref;
+}
+static char *get_overlay_name(struct xs_handle *xs, const char *xs_path)
+{
+char buf[128];
+char *ref = NULL;
+unsigned int len;
+
+snprintf(buf, sizeof(buf), "%s/overlay-name", xs_path);
+
+ref = xs_read(xs, XBT_NULL, buf, );
+
+return ref;
+}
+
+static char *get_overlay_type(struct xs_handle *xs, const char *xs_path)
+{
+char buf[128];
+char *ref = NULL;
+unsigned int len;
+
+snprintf(buf, sizeof(buf), "%s/overlay-type", xs_path);
+
+ref = xs_read(xs, XBT_NULL, buf, );
+
+return ref;
+}
+
+static bool get_overlay_partial(struct xs_handle *xs, const char *xs_path)
+{
+char buf[128];
+char *ref = NULL;
+unsigned int len;
+
+snprintf(buf, sizeof(buf), "%s/overlay-partial", xs_path);
+
+ref = xs_read(xs, XBT_NULL, buf, );
+
+if (ref) {
+bool is_partial = atoi(ref);
+free(ref);
+return is_partial;
+}
+
+return false;
+}
+
int main(int argc, char **argv)
{
void *buffer = NULL;
int domain ;
uint32_t *page_refs = NULL;
-FILE *fptr;
+FILE *fptr = NULL;
int dtbo_size = 0;
const char *path = "data/overlay";
char receiver_status_path[64] = { };
@@ -263,7 +347,11 @@ int main(int argc, char **argv)
int rc = 0;
int fd = 0;
uint32_t num_pages = 0;
-xengntshr_handle *gntshr;
+xengntshr_handle *gntshr = NULL;
+char *overlay_ops = NULL;
+char *name = NULL;
+char *type = NULL;
+bool is_partial = false;
if (argc < 2) {
fprintf(stderr,"Please enter domain_id.\n");
@@ -357,16 +445,33 @@ int main(int argc, char **argv)
goto out;
}
-if ((fptr = fopen("overlay.dtbo","wb")) == NULL) {
-fprintf(stderr,"Error! opening file");
+overlay_ops = get_overlay_ops(xs, path);
+name = get_overlay_name(xs, path);
+type = get_overlay_type(xs, path);
+is_partial = get_overlay_partial(xs, path);
+
+if (overlay_ops == NULL || name == NULL || type == NULL)
goto out;
-}
-printf("Writing to file overlay.dtbo.\n");
+printf("%s %s %s", overlay_ops, name, type);
+if (is_partial)
+printf(" %d", is_partial);
+
+printf("\n");
-fwrite(buffer, dtbo_size, 1, fptr);
+if
[PATCH 11/15] tools/helpers: Add get_overlay
From: Vikram Garhwal
This user level application copies the overlay dtbo shared by dom0 while doing
overlay node assignment operation. It uses xenstore to communicate with dom0.
More information on the protocol is writtien in docs/misc/overlay.txt file.
Signed-off-by: Vikram Garhwal
Signed-off-by: Stefano Stabellini
Signed-off-by: Henry Wang
---
tools/helpers/Makefile | 8 +
tools/helpers/get_overlay.c | 393
2 files changed, 401 insertions(+)
create mode 100644 tools/helpers/get_overlay.c
diff --git a/tools/helpers/Makefile b/tools/helpers/Makefile
index 09590eb5b6..dfe17ef269 100644
--- a/tools/helpers/Makefile
+++ b/tools/helpers/Makefile
@@ -12,6 +12,7 @@ TARGETS += init-xenstore-domain
endif
ifeq ($(CONFIG_ARM),y)
TARGETS += init-dom0less
+TARGETS += get_overlay
endif
endif
@@ -39,6 +40,9 @@ $(INIT_DOM0LESS_OBJS): CFLAGS += $(CFLAGS_libxenctrl)
$(INIT_DOM0LESS_OBJS): CFLAGS += $(CFLAGS_libxenevtchn)
init-dom0less: LDLIBS += $(call xenlibs-ldlibs,ctrl evtchn toollog store light
guest foreignmemory)
+SHARE_OVERLAY_OBJS = get_overlay.o
+$(SHARE_OVERLAY_OBJS): CFLAGS += $(CFLAGS_libxengnttab) $(CFLAGS_libxenstore)
$(CFLAGS_libxenctrl)
+
.PHONY: all
all: $(TARGETS)
@@ -51,6 +55,10 @@ init-xenstore-domain: $(INIT_XENSTORE_DOMAIN_OBJS)
init-dom0less: $(INIT_DOM0LESS_OBJS)
$(CC) $(LDFLAGS) -o $@ $(INIT_DOM0LESS_OBJS) $(LDLIBS) $(APPEND_LDFLAGS)
+get_overlay: $(SHARE_OVERLAY_OBJS)
+ $(CC) $(LDFLAGS) -o $@ $< $(LDLIBS_libxenvchan) $(LDLIBS_libxenstore)
$(LDLIBS_libxenctrl) $(LDLIBS_libxengnttab) $(APPEND_LDFLAGS)
+
+
.PHONY: install
install: all
$(INSTALL_DIR) $(DESTDIR)$(LIBEXEC_BIN)
diff --git a/tools/helpers/get_overlay.c b/tools/helpers/get_overlay.c
new file mode 100644
index 00..ca3007570e
--- /dev/null
+++ b/tools/helpers/get_overlay.c
@@ -0,0 +1,393 @@
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define PAGE_SIZE 4096
+
+static int xs_create_overlay_node(int domain, const char *xs_base,
+ struct xs_handle *xs)
+{
+int ret = -1;
+struct xs_permissions perms[2];
+char buf[64];
+char ref[16];
+char *domid_str = NULL;
+int overlay_size = 0;
+
+xs_transaction_t xs_trans = XBT_NULL;
+
+domid_str = xs_read(xs, XBT_NULL, "domid", NULL);
+
+if (!domid_str)
+return ret;
+
+/* owner domain is us */
+perms[0].id = atoi(domid_str);
+/* permissions for domains not listed = none. */
+perms[0].perms = XS_PERM_NONE;
+/* other domains i.e. domain provided by user gets r/w permissions. */
+perms[1].id = domain;
+perms[1].perms = XS_PERM_READ | XS_PERM_WRITE;
+
+retry_transaction:
+
+xs_trans = xs_transaction_start(xs);
+if (!xs_trans)
+goto fail_xs_transaction;
+
+/* Create overlay-size node. */
+snprintf(ref, sizeof(ref), "%d", overlay_size);
+snprintf(buf, sizeof(buf), "%s/overlay-size", xs_base);
+
+if (!xs_write(xs, xs_trans, buf, ref, strlen(ref)))
+goto fail_xs_transaction;
+if (!xs_set_permissions(xs, xs_trans, buf, perms, 2))
+goto fail_xs_transaction;
+
+/* Create domU status node. */
+snprintf(ref, sizeof(ref), "%s", "waiting");
+snprintf(buf, sizeof(buf), "%s/receiver-status", xs_base);
+
+if (!xs_write(xs, xs_trans, buf, ref, strlen(ref)))
+goto fail_xs_transaction;
+if (!xs_set_permissions(xs, xs_trans, buf, perms, 2))
+goto fail_xs_transaction;
+
+/* Create dom0 status node. */
+snprintf(ref, sizeof(ref), "%s", "not_ready");
+snprintf(buf, sizeof(buf), "%s/sender-status", xs_base);
+
+if (!xs_write(xs, xs_trans, buf, ref, strlen(ref)))
+goto fail_xs_transaction;
+if (!xs_set_permissions(xs, xs_trans, buf, perms, 2))
+goto fail_xs_transaction;
+
+if (!xs_transaction_end(xs, xs_trans, 0)) {
+if (errno == EAGAIN)
+goto retry_transaction;
+else
+goto fail_xs_transaction;
+} else
+ret = 0;
+
+fail_xs_transaction:
+free(domid_str);
+
+return ret;
+}
+
+static int get_overlay_size(struct xs_handle *xs, int domain,
+const char *xs_path)
+{
+char buf[128];
+char *ref;
+unsigned int len;
+int dt_size = 0;
+
+snprintf(buf, sizeof(buf), "%s/overlay-size", xs_path);
+
+ref = xs_read(xs, XBT_NULL, buf, );
+
+if (!ref)
+return dt_size;
+
+dt_size = atoi(ref);
+
+free(ref);
+
+return dt_size;
+}
+
+static uint32_t get_num_pages(int dtbo_size)
+{
+int num_pages = 1;
+
+while (dtbo_size > PAGE_SIZE) {
+dtbo_size = dtbo_size - PAGE_SIZE;
+num_pages++;
+}
+
+return num_pages;
+}
+
+static void *create_shared_buffer(int domain, uint32_t *refs, uint32_t pages,
+ xengntshr_handle *gntshr)
+{
+return
[PATCH 14/15] add a domU script to fetch overlays and applying them to linux
From: Vikram Garhwal Introduce a shell script that runs in the background and calls get_overlay to retrive overlays and add them (or remove them) to Linux device tree (running as a domU). Signed-off-by: Vikram Garhwal Signed-off-by: Stefano Stabellini Signed-off-by: Henry Wang --- tools/helpers/Makefile | 2 +- tools/helpers/get_overlay.sh | 81 2 files changed, 82 insertions(+), 1 deletion(-) create mode 100755 tools/helpers/get_overlay.sh diff --git a/tools/helpers/Makefile b/tools/helpers/Makefile index dfe17ef269..2d0558aeb8 100644 --- a/tools/helpers/Makefile +++ b/tools/helpers/Makefile @@ -58,7 +58,6 @@ init-dom0less: $(INIT_DOM0LESS_OBJS) get_overlay: $(SHARE_OVERLAY_OBJS) $(CC) $(LDFLAGS) -o $@ $< $(LDLIBS_libxenvchan) $(LDLIBS_libxenstore) $(LDLIBS_libxenctrl) $(LDLIBS_libxengnttab) $(APPEND_LDFLAGS) - .PHONY: install install: all $(INSTALL_DIR) $(DESTDIR)$(LIBEXEC_BIN) @@ -67,6 +66,7 @@ install: all .PHONY: uninstall uninstall: for i in $(TARGETS); do rm -f $(DESTDIR)$(LIBEXEC_BIN)/$$i; done + $(RM) $(DESTDIR)$(LIBEXEC_BIN)/get_overlay.sh .PHONY: clean clean: diff --git a/tools/helpers/get_overlay.sh b/tools/helpers/get_overlay.sh new file mode 100755 index 00..2e8c6ecefd --- /dev/null +++ b/tools/helpers/get_overlay.sh @@ -0,0 +1,81 @@ +#!/bin/sh + +modprobe xen_gntalloc +modprobe xen_gntdev + +while : +do +overlay_node_name="" +type_overlay="normal" +is_partial_dtb="" + +output=`/usr/lib/xen/bin/get_overlay 0` + +if test $? -ne 0 +then +echo error +exit 1 +fi + +if test -z "$output" +then +echo "" +exit 1 +fi + +# output: add overlay-name normal partial +operation=`echo $output | cut -d " " -f 1` +overlay_node_name=`echo $output | cut -d " " -f 2` +type_overlay=`echo $output | cut -d " " -f 3` +is_partial_dtb=`echo $output | cut -d " " -f 4` + +if test -z "$operation" || test -z "$overlay_node_name" +then +echo "invalid ops" +exit 1 +fi + +if test $operation = "add" +then +echo "Overlay received" + +if test "$type_overlay" = "normal" +then + final_path="/sys/kernel/config/device-tree/overlays/$overlay_node_name" +mkdir -p $final_path +cat overlay.dtbo > $final_path/dtbo +else +# fpga overlay +cp overlay.dtbo lib/firmware/ +mkdir /configfs +mount -t configfs configfs /configfs +cd /configfs/device-tree/overlays/ + +if test "$is_partial_dtb" +then +mkdir partial +echo 1 > /sys/class/fpga_manager/fpga0/flags +echo -n "overlay.dtbo" > /configfs/device-tree/overlays/partial +else +mkdir full +echo -n "overlay.dtbo" > /configfs/device-tree/overlays/full +fi +fi +elif test $operation = "remove" +then +if test "$type_overlay" = "normal" +then +# implement remove + path=/sys/kernel/config/device-tree/overlays/$overlay_node_name/dtbo +if ! test -f $path +then +echo "error: path doesn't exist" +exit 1 +fi +rm $path +fi +else +echo "operation unsupported" +exit 1 +fi +done -- 2.34.1
[PATCH 07/15] xen/overlay: Enable device tree overlay assignment to running domains
From: Vikram Garhwal
Following changes are done to enable dtbo assignment to a running vm with given
domain_id:
1. Support for irq map and unmap for running domain. We store the IRQ nums for
each overlay and while removing those IRQ nums are removed.
2. Support iommu assign/reassign to running domains.
3. Added "map_nodes" options to support two modes:
a. Add the nodes in Xen device tree and map the nodes to given VM
b. Just add them in xen device tree without mapping.
Change device.c: handle_device() function with input domain_mapping flag.
Signed-off-by: Vikram Garhwal
Signed-off-by: Michal Orzel
Signed-off-by: Henry Wang
---
xen/arch/arm/device.c| 8 +-
xen/arch/arm/domain_build.c | 2 +-
xen/arch/arm/include/asm/setup.h | 3 +-
xen/common/dt-overlay.c | 218 ++-
xen/include/public/sysctl.h | 4 +-
5 files changed, 199 insertions(+), 36 deletions(-)
diff --git a/xen/arch/arm/device.c b/xen/arch/arm/device.c
index 3e02cff008..df5035f9a8 100644
--- a/xen/arch/arm/device.c
+++ b/xen/arch/arm/device.c
@@ -317,7 +317,8 @@ static int map_device_children(const struct dt_device_node
*dev,
* - Map the IRQs and iomem regions to DOM0
*/
int handle_device(struct domain *d, struct dt_device_node *dev, p2m_type_t
p2mt,
- struct rangeset *iomem_ranges, struct rangeset *irq_ranges)
+ struct rangeset *iomem_ranges, struct rangeset *irq_ranges,
+ bool device_mapping)
{
unsigned int naddr;
unsigned int i;
@@ -334,9 +335,10 @@ int handle_device(struct domain *d, struct dt_device_node
*dev, p2m_type_t p2mt,
struct map_range_data mr_data = {
.d = d,
.p2mt = p2mt,
-.skip_mapping = !own_device ||
+.skip_mapping = (!own_device ||
(is_pci_passthrough_enabled() &&
-(device_get_class(dev) == DEVICE_PCI_HOSTBRIDGE)),
+(device_get_class(dev) == DEVICE_PCI_HOSTBRIDGE))) &&
+!device_mapping,
.iomem_ranges = iomem_ranges,
.irq_ranges = irq_ranges
};
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 54232ed4cb..a525aed175 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1699,7 +1699,7 @@ static int __init handle_node(struct domain *d, struct
kernel_info *kinfo,
"WARNING: Path %s is reserved, skip the node as we may re-use
the path.\n",
path);
-res = handle_device(d, node, p2mt, NULL, NULL);
+res = handle_device(d, node, p2mt, NULL, NULL, false);
if ( res)
return res;
diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h
index d15a88d2e0..d6d8c28d02 100644
--- a/xen/arch/arm/include/asm/setup.h
+++ b/xen/arch/arm/include/asm/setup.h
@@ -172,7 +172,8 @@ u32 device_tree_get_u32(const void *fdt, int node,
const char *prop_name, u32 dflt);
int handle_device(struct domain *d, struct dt_device_node *dev, p2m_type_t
p2mt,
- struct rangeset *iomem_ranges, struct rangeset *irq_ranges);
+ struct rangeset *iomem_ranges, struct rangeset *irq_ranges,
+ bool device_mapping);
int map_device_irqs_to_domain(struct domain *d, struct dt_device_node *dev,
bool need_mapping, struct rangeset *irq_ranges);
diff --git a/xen/common/dt-overlay.c b/xen/common/dt-overlay.c
index 39e4ba59dd..8840ea756b 100644
--- a/xen/common/dt-overlay.c
+++ b/xen/common/dt-overlay.c
@@ -356,24 +356,133 @@ static int overlay_get_nodes_info(const void *fdto, char
**nodes_full_path)
return 0;
}
+static int remove_all_irqs(struct rangeset *irq_ranges,
+ struct domain *d, bool domain_mapping)
+{
+struct range *x;
+int rc = 0;
+
+read_lock(_ranges->lock);
+
+for ( x = first_range(irq_ranges); x != NULL;
+ x = next_range(irq_ranges, x) )
+{
+/*
+ * Handle invalid use cases 1:
+ * Where user assigned the nodes to dom0 along with their irq
+ * mappings but now just wants to remove the nodes entry from Xen
device
+ * device tree without unmapping the irq.
+ */
+if ( !domain_mapping && vgic_get_hw_irq_desc(d, NULL, x->s) )
+{
+printk(XENLOG_ERR "Removing node from device tree without
releasing it's IRQ/IOMMU is not allowed\n");
+rc = -EINVAL;
+goto out;
+}
+
+/*
+ * IRQ should always have access unless there are duplication of
+ * of irqs in device tree. There are few cases of xen device tree
+ * where there are duplicate interrupts for the same node.
+ */
+if (!irq_access_permitted(d, x->s))
+continue;
+/*
+ * TODO: We don't handle shared IRQs for now. So, it is assumed that
+
[PATCH 15/15] docs: add device tree overlay documentation
From: Vikram Garhwal Signed-off-by: Vikram Garhwal Signed-off-by: Stefano Stabellini Signed-off-by: Henry Wang --- docs/misc/arm/overlay.txt | 172 ++ 1 file changed, 172 insertions(+) create mode 100644 docs/misc/arm/overlay.txt diff --git a/docs/misc/arm/overlay.txt b/docs/misc/arm/overlay.txt new file mode 100644 index 00..0351f82a19 --- /dev/null +++ b/docs/misc/arm/overlay.txt @@ -0,0 +1,172 @@ +# Device Tree Overlays support in Xen + +Xen now supports dynamic device assignment to running domains i.e. +adding/removing nodes (using .dtbo) in Xen device tree, and assigning +them to a running domain with given $domid. + +Xen supports two modes of operation: normal mode and expert mode for +assigning nodes to domU. More on this below. + +Dynamic node assignment works in following ways: + +1. Xen tools check the dtbo given and parse all other user provided arguments +2. Xen tools pass the dtbo to Xen hypervisor via hypercall. +3. Xen hypervisor applies the dtbo to Xen device tree and assign the + dbto's node resources to the user-provided $domid. +4. For normal mode, Xen tools share the modified dtbo with domU. domU needs + to run get_overlay.sh to get the dtbo from dom0 and apply the + overlay. get_overlay.sh uses get_overlay application for data transfer + between dom0 and domU. + + +# Overlay Sharing protocol based on Xenstore + +The overlay sharing protocol with domU works in the following ways: + +1. get_overlay creates Xenstore path data/overlay and creates the + following nodes under data/overlay path: + a. receiver-status + b. sender-status + c. overlay-size + d. overlay-name + e. overlay-type + f. overlay-partial + and write "waiting" on receiver-status and "not_ready" to sender_status. + +2. libxl waits for "waiting" status on receiver-status, then writes + "overlay-size" with dtbo size and "ready" on "sender-status". + +3. get_overlay waits for "sender-status" to "ready", then allocates the + pages, next it shares the pages with dom0 (the page-ref num) by creating + page-ref node under /data/overlay and finally writes "page-refs" to + "receiver_status". + +4. libxl waits for "receiver-status" to become "page-refs" and copies + the data to buffer allocated with page_refs. libxl also writes the + "overlay-name", "overlay-type", and "overlay-partial" nodes with + user-provided information. Lastly, libxl writes "done" to + "sender-status". + +6. get_overlay waits for "sender-status" to be "done". + +7. get_overlay copies the data and writes it to file. + +8. Finally, get_overlay unshares the pages with dom0. + +Note: get_overlay application needs two drivers xen_gntdev and xen_gntalloc in +Linux. These can be loaded using modprobe xen_gntalloc and modprobe xen_gntdev. + + +# Examples + +Here are a few examples on how to use it. + +## Dom0 device add + +For assigning a device tree overlay to dom0, enter the following: + +(dom0) xl dt-overlay add overlay.dtbo 0 + +This will allocate the devices mentioned in overlay.dtbo to Xen device +tree and will assign these devices to dom0. + +Next, if the user wants to add the same device tree overlay to dom0 +Linux, execute the following: + +(dom0) mkdir -p /sys/kernel/config/device-tree/overlays/new_overlay +(dom0) cat overlay.dtbo > /sys/kernel/config/device-tree/overlays/new_overlay/dtbo + +Finally if needed, the relevant Linux kernel drive can be loaded using: + +(dom0) modprobe module_name.ko + + +## Dom0 device remove + +For removing the device from dom0, do the following: + +(dom0) xl dt-overlay remove overlay.dtbo + +NOTE: The user is expected to unload any Linux kernel modules which +might be accessing the devices in overlay.dtbo. Removing devices without +unloading the modules might result in a crash. + +The following is an incorrect sequence: + +(dom0) xl dt-overlay add overlay.dtbo 0 +(dom0) xl dt-overlay remove overlay.dtbo + +The last command only removed the nodes from the Xen dtb but it did not +deassigning irq/iommus from dom0. This will result in unhandled +behavior. The correct sequence is to deassign the nodes from dom0: + +(dom0) xl dt-overlay remove overlay.dtbo 0 + + +## DomU device add/remove + +There are two modes supported for domU use cases: expert mode and normal +mode. + + +### Expert mode + +All the nodes in dtbo will be assigned to a domain; the user will need +to prepare the dtb for the domU. User will also need to modprobe the +relevant drivers. + +Example for domU device add: + +(dom0) xl dt-overlay add overlay.dtbo $domid +(dom0) xl console $domid # to access $domid console + +Next, if the user needs to modify/prepare the overlay.dtbo suitable for +the domU: + +(domU) mkdir -p /sys/kernel/config/device-tree/overlays/new_overlay +(domU) cat overlay_linux.dtbo > /sys/kernel/config/device-tree/overlays/new_overlay/dtbo + +Finally, if needed, the relevant Linux
[PATCH 06/15] rangeset: Move struct range and struct rangeset to headerfile
From: Vikram Garhwal
Move struct range, rangeset and removed static from first_range and
next_range().
IRQs and IOMEMs for nodes are stored as rangeset in the dynamic node addition
part. While removing the nodes we need to access every IRQ and IOMEM ranges to
unmap IRQ and IOMEM from the domain.
Signed-off-by: Vikram Garhwal
Signed-off-by: Henry Wang
---
xen/common/rangeset.c | 31 ++-
xen/include/xen/rangeset.h | 32 +++-
2 files changed, 33 insertions(+), 30 deletions(-)
diff --git a/xen/common/rangeset.c b/xen/common/rangeset.c
index b75590f907..d3f4297e41 100644
--- a/xen/common/rangeset.c
+++ b/xen/common/rangeset.c
@@ -12,31 +12,6 @@
#include
#include
-/* An inclusive range [s,e] and pointer to next range in ascending order. */
-struct range {
-struct list_head list;
-unsigned long s, e;
-};
-
-struct rangeset {
-/* Owning domain and threaded list of rangesets. */
-struct list_head rangeset_list;
-struct domain *domain;
-
-/* Ordered list of ranges contained in this set, and protecting lock. */
-struct list_head range_list;
-
-/* Number of ranges that can be allocated */
-long nr_ranges;
-rwlock_t lock;
-
-/* Pretty-printing name. */
-char name[32];
-
-/* RANGESETF flags. */
-unsigned int flags;
-};
-
/*
* Private range functions hide the underlying linked-list implemnetation.
*/
@@ -57,8 +32,7 @@ static struct range *find_range(
return x;
}
-/* Return the lowest range in the set r, or NULL if r is empty. */
-static struct range *first_range(
+struct range *first_range(
struct rangeset *r)
{
if ( list_empty(>range_list) )
@@ -66,8 +40,7 @@ static struct range *first_range(
return list_entry(r->range_list.next, struct range, list);
}
-/* Return range following x in ascending order, or NULL if x is the highest. */
-static struct range *next_range(
+struct range *next_range(
struct rangeset *r, struct range *x)
{
if ( x->list.next == >range_list )
diff --git a/xen/include/xen/rangeset.h b/xen/include/xen/rangeset.h
index 96c9180825..cd80fd9179 100644
--- a/xen/include/xen/rangeset.h
+++ b/xen/include/xen/rangeset.h
@@ -13,7 +13,37 @@
#include
struct domain;
-struct rangeset;
+
+struct rangeset {
+/* Owning domain and threaded list of rangesets. */
+struct list_head rangeset_list;
+struct domain *domain;
+
+/* Ordered list of ranges contained in this set, and protecting lock. */
+struct list_head range_list;
+
+/* Number of ranges that can be allocated */
+long nr_ranges;
+rwlock_t lock;
+
+/* Pretty-printing name. */
+char name[32];
+
+/* RANGESETF flags. */
+unsigned int flags;
+};
+
+/* An inclusive range [s,e] and pointer to next range in ascending order. */
+struct range {
+struct list_head list;
+unsigned long s, e;
+};
+
+/* Return the lowest range in the set r, or NULL if r is empty. */
+struct range *first_range(struct rangeset *r);
+
+/* Return range following x in ascending order, or NULL if x is the highest. */
+struct range *next_range(struct rangeset *r, struct range *x);
/*
* Initialise/destroy per-domain rangeset information.
--
2.34.1
[PATCH 09/15] tools/libs/light: Modify dtbo to domU linux dtbo format
From: Vikram Garhwal
Add support for modifying dtbo to make it suitable for DomU Linux. This is
done for '-e expert' mode.
Signed-off-by: Vikram Garhwal
Signed-off-by: Stefano Stabellini
Signed-off-by: Henry Wang
---
tools/libs/light/libxl_dt_overlay.c | 73 +
1 file changed, 73 insertions(+)
diff --git a/tools/libs/light/libxl_dt_overlay.c
b/tools/libs/light/libxl_dt_overlay.c
index cdb62b28cf..eaf11a0f9c 100644
--- a/tools/libs/light/libxl_dt_overlay.c
+++ b/tools/libs/light/libxl_dt_overlay.c
@@ -17,6 +17,7 @@
#include "libxl_internal.h"
#include
#include
+#include
static int check_overlay_fdt(libxl__gc *gc, void *fdt, size_t size)
{
@@ -41,6 +42,69 @@ static int check_overlay_fdt(libxl__gc *gc, void *fdt,
size_t size)
return 0;
}
+static int modify_overlay_for_domU(libxl__gc *gc, void *overlay_dt_domU,
+ size_t size)
+{
+int rc = 0;
+int virtual_interrupt_parent = GUEST_PHANDLE_GIC;
+const struct fdt_property *fdt_prop_node = NULL;
+int overlay;
+int prop_len = 0;
+int subnode = 0;
+int fragment;
+const char *prop_name;
+const char *target_path = "/";
+
+fdt_for_each_subnode(fragment, overlay_dt_domU, 0) {
+prop_name = fdt_getprop(overlay_dt_domU, fragment, "target-path",
+_len);
+if (prop_name == NULL) {
+LOG(ERROR, "target-path property not found\n");
+rc = ERROR_FAIL;
+goto err;
+}
+
+/* Change target path for domU dtb. */
+rc = fdt_setprop_string(overlay_dt_domU, fragment, "target-path",
+target_path);
+if (rc) {
+LOG(ERROR, "Setting interrupt parent property failed for %s\n",
+prop_name);
+goto err;
+}
+
+overlay = fdt_subnode_offset(overlay_dt_domU, fragment, "__overlay__");
+
+fdt_for_each_subnode(subnode, overlay_dt_domU, overlay)
+{
+const char *node_name = fdt_get_name(overlay_dt_domU, subnode,
+ NULL);
+
+fdt_prop_node = fdt_getprop(overlay_dt_domU, subnode,
+"interrupt-parent", _len);
+if (fdt_prop_node == NULL) {
+LOG(DETAIL, "%s property not found for %s. Skip to next
node\n",
+"interrupt-parent", node_name);
+continue;
+}
+
+rc = fdt_setprop_inplace_u32(overlay_dt_domU, subnode,
+ "interrupt-parent",
+ virtual_interrupt_parent);
+if (rc) {
+LOG(ERROR, "Setting interrupt parent property failed for %s\n",
+"interrupt-parent");
+goto err;
+}
+}
+}
+
+return 0;
+
+err:
+return rc;
+}
+
int libxl_dt_overlay(libxl_ctx *ctx, uint32_t domid, void *overlay_dt,
uint32_t overlay_dt_size, uint8_t overlay_op,
bool auto_mode, bool domain_mapping)
@@ -73,6 +137,15 @@ int libxl_dt_overlay(libxl_ctx *ctx, uint32_t domid, void
*overlay_dt,
rc = ERROR_FAIL;
}
+/*
+ * auto_mode doesn't apply to dom0 as dom0 can get the physical
+ * description of the hardware.
+ */
+if (domid && auto_mode) {
+if (overlay_op == LIBXL_DT_OVERLAY_ADD)
+rc = modify_overlay_for_domU(gc, overlay_dt, overlay_dt_size);
+}
+
out:
GC_FREE;
return rc;
--
2.34.1
[PATCH 10/15] tools/xl: Share overlay with domU
From: Vikram Garhwal
Add domid to enable assigning the node to a running VMs.
Add expert mode option to share the overlay dtbo with domU. In this mode dom0
communicates with domid domain to share the overlay dtbo. This is done via
xenstore.
Signed-off-by: Vikram Garhwal
Signed-off-by: Stewart Hildebrand
Signed-off-by: Stefano Stabellini
Signed-off-by: Henry Wang
---
tools/xl/Makefile | 2 +-
tools/xl/xl_cmdtable.c | 2 +-
tools/xl/xl_vmcontrol.c | 343 +++-
3 files changed, 342 insertions(+), 5 deletions(-)
diff --git a/tools/xl/Makefile b/tools/xl/Makefile
index d742e96a5b..122dfb9346 100644
--- a/tools/xl/Makefile
+++ b/tools/xl/Makefile
@@ -32,7 +32,7 @@ $(XL_OBJS): CFLAGS += -include $(XEN_ROOT)/tools/config.h #
libxl_json.h needs i
all: xl
xl: $(XL_OBJS)
- $(CC) $(LDFLAGS) -o $@ $(XL_OBJS) $(LDLIBS_libxenutil)
$(LDLIBS_libxenlight) $(LDLIBS_libxentoollog) -lyajl $(APPEND_LDFLAGS)
+ $(CC) $(LDFLAGS) -o $@ $(XL_OBJS) $(LDLIBS_libxenutil)
$(LDLIBS_libxenlight) $(LDLIBS_libxentoollog) $(LDLIBS_libxenstore)
$(LDLIBS_libxengnttab) -lyajl $(APPEND_LDFLAGS)
.PHONY: install
install: all
diff --git a/tools/xl/xl_cmdtable.c b/tools/xl/xl_cmdtable.c
index 62bdb2aeaa..2531e8517b 100644
--- a/tools/xl/xl_cmdtable.c
+++ b/tools/xl/xl_cmdtable.c
@@ -635,7 +635,7 @@ const struct cmd_spec cmd_table[] = {
{ "dt-overlay",
_dt_overlay, 0, 1,
"Add/Remove a device tree overlay",
- "add/remove <.dtbo>"
+ "add/remove <.dtbo> domain_id -e[expert_mode]",
"-h print this help\n"
},
#endif
diff --git a/tools/xl/xl_vmcontrol.c b/tools/xl/xl_vmcontrol.c
index 9674383ec3..2bf76dd389 100644
--- a/tools/xl/xl_vmcontrol.c
+++ b/tools/xl/xl_vmcontrol.c
@@ -16,6 +16,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -29,6 +30,8 @@
#include "xl.h"
#include "xl_utils.h"
#include "xl_parse.h"
+#include
+#include
static int fd_lock = -1;
@@ -1266,6 +1269,335 @@ int main_create(int argc, char **argv)
}
#ifdef LIBXL_HAVE_DT_OVERLAY
+static int copy_overlay_to_domU(uint32_t domain_id, void *overlay_dt_domU,
+uint32_t overlay_dt_size, uint32_t *grant_ref,
+uint32_t num_pages)
+{
+void *buffer = NULL;
+xengnttab_handle *gnttab = xengnttab_open(NULL, 0);
+
+if (!gnttab) {
+fprintf(stderr,"opening gnttab failed for domain %d\n", domain_id);
+return -1;
+}
+
+buffer = xengnttab_map_domain_grant_refs(gnttab, num_pages, domain_id,
+ grant_ref, PROT_READ|PROT_WRITE);
+
+if (buffer == NULL) {
+fprintf(stderr, "Getting the buffer failed for grant_refs\n");
+
+return -1;
+}
+
+/* buffer is contiguous allocated. */
+memcpy(buffer, overlay_dt_domU, overlay_dt_size);
+
+xengnttab_unmap(gnttab, buffer, num_pages);
+
+return 0;
+}
+
+static bool wait_for_status(struct xs_handle *xs, int fd, char *status_path,
+const char *status)
+{
+unsigned int num_strings;
+char *buf = NULL;
+char **vec = NULL;
+bool ret = false;
+unsigned int len;
+int rc = 0;
+fd_set set;
+
+while (1)
+{
+FD_ZERO();
+FD_SET(fd, );
+
+rc = select(fd + 1, , NULL, NULL, NULL);
+/* Poll for data: Blocking. */
+if (rc <= 0)
+break;
+
+if (FD_ISSET(fd, )) {
+/*
+ * num_strings will be set to the number of elements in vec
+ * (2 - the watched path and the overlay_watch)
+ */
+vec = xs_read_watch(xs, _strings);
+if (!vec) {
+break;
+}
+
+/* do a read. */
+buf = xs_read(xs, XBT_NULL, status_path, );
+if (buf) {
+if (!strcmp(buf, status)) {
+ret = true;
+break;
+}
+}
+}
+}
+
+free(vec);
+free(buf);
+
+return ret;
+}
+
+static bool write_overlay_size(struct xs_handle *xs, uint32_t overlay_size,
+ char *path)
+{
+xs_transaction_t xs_trans = XBT_NULL;
+char buf[128];
+char ref[16];
+
+retry_transaction:
+xs_trans = xs_transaction_start(xs);
+if (!xs_trans)
+return false;
+
+snprintf(ref, sizeof(ref), "%u", overlay_size);
+snprintf(buf, sizeof(buf), "%s/overlay-size", path);
+
+if (!xs_write(xs, xs_trans, buf, ref, strlen(ref)))
+return false;
+
+if (!xs_transaction_end(xs, xs_trans, 0)) {
+if (errno == EAGAIN)
+goto retry_transaction;
+else
+return false;
+}
+
+return true;
+}
+
+static bool write_status(struct xs_handle *xs, char *status, char *status_path)
+{
+xs_transaction_t xs_trans = XBT_NULL;
+
+retry_transaction:
+xs_trans =
[PATCH 08/15] tools: Add domain_id and expert mode for overlay operations
From: Vikram Garhwal
Add domain_id and expert mode for overlay assignment. This enables dynamic
programming of nodes during runtime.
Take the opportunity to fix the name mismatch in the xl command, the
command name should be "dt-overlay" instead of "dt_overlay".
Signed-off-by: Vikram Garhwal
Signed-off-by: Stefano Stabellini
Signed-off-by: Henry Wang
---
tools/include/libxl.h | 8 +--
tools/include/xenctrl.h | 5 +++--
tools/libs/ctrl/xc_dt_overlay.c | 7 --
tools/libs/light/libxl_dt_overlay.c | 17 +++
tools/xl/xl_vmcontrol.c | 34 ++---
5 files changed, 58 insertions(+), 13 deletions(-)
diff --git a/tools/include/libxl.h b/tools/include/libxl.h
index 62cb07dea6..59a3e1b37c 100644
--- a/tools/include/libxl.h
+++ b/tools/include/libxl.h
@@ -2549,8 +2549,12 @@ libxl_device_pci *libxl_device_pci_list(libxl_ctx *ctx,
uint32_t domid,
void libxl_device_pci_list_free(libxl_device_pci* list, int num);
#if defined(__arm__) || defined(__aarch64__)
-int libxl_dt_overlay(libxl_ctx *ctx, void *overlay,
- uint32_t overlay_size, uint8_t overlay_op);
+#define LIBXL_DT_OVERLAY_ADD 1
+#define LIBXL_DT_OVERLAY_REMOVE2
+
+int libxl_dt_overlay(libxl_ctx *ctx, uint32_t domain_id, void *overlay,
+ uint32_t overlay_size, uint8_t overlay_op, bool auto_mode,
+ bool domain_mapping);
#endif
/*
diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index 2ef8b4e054..3861d0a23f 100644
--- a/tools/include/xenctrl.h
+++ b/tools/include/xenctrl.h
@@ -2654,8 +2654,9 @@ int xc_domain_cacheflush(xc_interface *xch, uint32_t
domid,
xen_pfn_t start_pfn, xen_pfn_t nr_pfns);
#if defined(__arm__) || defined(__aarch64__)
-int xc_dt_overlay(xc_interface *xch, void *overlay_fdt,
- uint32_t overlay_fdt_size, uint8_t overlay_op);
+int xc_dt_overlay(xc_interface *xch, uint32_t domain_id, void *overlay_fdt,
+ uint32_t overlay_fdt_size, uint8_t overlay_op,
+ bool domain_mapping);
#endif
/* Compat shims */
diff --git a/tools/libs/ctrl/xc_dt_overlay.c b/tools/libs/ctrl/xc_dt_overlay.c
index c2224c4d15..a1dc549915 100644
--- a/tools/libs/ctrl/xc_dt_overlay.c
+++ b/tools/libs/ctrl/xc_dt_overlay.c
@@ -20,15 +20,18 @@
#include "xc_private.h"
-int xc_dt_overlay(xc_interface *xch, void *overlay_fdt,
- uint32_t overlay_fdt_size, uint8_t overlay_op)
+int xc_dt_overlay(xc_interface *xch, uint32_t domid, void *overlay_fdt,
+ uint32_t overlay_fdt_size, uint8_t overlay_op,
+ bool domain_mapping)
{
int err;
struct xen_sysctl sysctl = {
.cmd = XEN_SYSCTL_dt_overlay,
.u.dt_overlay = {
+.domain_id = domid,
.overlay_op = overlay_op,
.overlay_fdt_size = overlay_fdt_size,
+.domain_mapping = domain_mapping,
}
};
diff --git a/tools/libs/light/libxl_dt_overlay.c
b/tools/libs/light/libxl_dt_overlay.c
index a6c709a6dc..cdb62b28cf 100644
--- a/tools/libs/light/libxl_dt_overlay.c
+++ b/tools/libs/light/libxl_dt_overlay.c
@@ -41,8 +41,9 @@ static int check_overlay_fdt(libxl__gc *gc, void *fdt, size_t
size)
return 0;
}
-int libxl_dt_overlay(libxl_ctx *ctx, void *overlay_dt, uint32_t
overlay_dt_size,
- uint8_t overlay_op)
+int libxl_dt_overlay(libxl_ctx *ctx, uint32_t domid, void *overlay_dt,
+ uint32_t overlay_dt_size, uint8_t overlay_op,
+ bool auto_mode, bool domain_mapping)
{
int rc;
int r;
@@ -57,10 +58,18 @@ int libxl_dt_overlay(libxl_ctx *ctx, void *overlay_dt,
uint32_t overlay_dt_size,
rc = 0;
}
-r = xc_dt_overlay(ctx->xch, overlay_dt, overlay_dt_size, overlay_op);
+/* Check if user entered a valid domain id. */
+rc = libxl_domain_info(CTX, NULL, domid);
+if (rc == ERROR_DOMAIN_NOTFOUND) {
+LOGD(ERROR, domid, "Non-existant domain.");
+return ERROR_FAIL;
+}
+
+r = xc_dt_overlay(ctx->xch, domid, overlay_dt, overlay_dt_size, overlay_op,
+ domain_mapping);
if (r) {
-LOG(ERROR, "%s: Adding/Removing overlay dtb failed.", __func__);
+LOG(ERROR, "domain%d: Adding/Removing overlay dtb failed.", domid);
rc = ERROR_FAIL;
}
diff --git a/tools/xl/xl_vmcontrol.c b/tools/xl/xl_vmcontrol.c
index 98f6bd2e76..9674383ec3 100644
--- a/tools/xl/xl_vmcontrol.c
+++ b/tools/xl/xl_vmcontrol.c
@@ -1270,21 +1270,48 @@ int main_dt_overlay(int argc, char **argv)
{
const char *overlay_ops = NULL;
const char *overlay_config_file = NULL;
+uint32_t domain_id = 0;
void *overlay_dtb = NULL;
int rc;
+bool auto_mode = true;
+bool domain_mapping = false;
uint8_t op;
int overlay_dtb_size = 0;
const int overlay_add_op = 1;
const
[PATCH 05/15] tools/libs/light: Increase nr_spi to 160
From: Vikram Garhwal
Increase number of spi to 160 i.e. gic_number_lines() for Xilinx ZynqMP - 32.
This was done to allocate and assign IRQs to a running domain.
Signed-off-by: Vikram Garhwal
Signed-off-by: Stefano Stabellini
Signed-off-by: Henry Wang
---
tools/libs/light/libxl_arm.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
index dd5c9f4917..50dbd0f2a9 100644
--- a/tools/libs/light/libxl_arm.c
+++ b/tools/libs/light/libxl_arm.c
@@ -181,7 +181,8 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
LOG(DEBUG, "Configure the domain");
-config->arch.nr_spis = nr_spis;
+/* gic_number_lines() is 192 for Xilinx ZynqMP. min nr_spis = 192 - 32. */
+config->arch.nr_spis = MAX(nr_spis, 160);
LOG(DEBUG, " - Allocate %u SPIs", nr_spis);
switch (d_config->b_info.arch_arm.gic_version) {
--
2.34.1
[PATCH 04/15] tools/libs/light: Always enable IOMMU
From: Vikram Garhwal
For overlay with iommu functionality to work with running VMs, we need
to enable IOMMU when iomem presents for the domains.
Signed-off-by: Vikram Garhwal
Signed-off-by: Henry Wang
---
tools/libs/light/libxl_arm.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
index 1cb89fa584..dd5c9f4917 100644
--- a/tools/libs/light/libxl_arm.c
+++ b/tools/libs/light/libxl_arm.c
@@ -222,6 +222,12 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
config->arch.sve_vl = d_config->b_info.arch_arm.sve_vl / 128U;
}
+#ifdef LIBXL_HAVE_DT_OVERLAY
+if (d_config->b_info.num_iomem) {
+config->flags |= XEN_DOMCTL_CDF_iommu;
+}
+#endif
+
return 0;
}
--
2.34.1
[PATCH 03/15] xen/arm: Always enable IOMMU
From: Vikram Garhwal
For overlay with iommu functionality to work with running VMs, we need to enable
IOMMU by default for the domains.
Signed-off-by: Vikram Garhwal
Signed-off-by: Stefano Stabellini
Signed-off-by: Henry Wang
---
xen/arch/arm/dom0less-build.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c
index fb63ec6fd1..2d1fd1e214 100644
--- a/xen/arch/arm/dom0less-build.c
+++ b/xen/arch/arm/dom0less-build.c
@@ -894,7 +894,8 @@ void __init create_domUs(void)
panic("Missing property 'cpus' for domain %s\n",
dt_node_name(node));
-if ( dt_find_compatible_node(node, NULL, "multiboot,device-tree") &&
+if ( (IS_ENABLED(CONFIG_OVERLAY_DTB) ||
+ dt_find_compatible_node(node, NULL, "multiboot,device-tree")) &&
iommu_enabled )
d_cfg.flags |= XEN_DOMCTL_CDF_iommu;
--
2.34.1
[PATCH 01/15] xen/commom/dt-overlay: Fix missing lock when remove the device
If CONFIG_DEBUG=y, below assertion will be triggered:
(XEN) Assertion 'rw_is_locked(_host_lock)' failed at
drivers/passthrough/device_tree.c:146
(XEN) [ Xen-4.19-unstable arm64 debug=y Not tainted ]
(XEN) CPU: 0
(XEN) PC: 0a257418 iommu_remove_dt_device+0x8c/0xd4
(XEN) LR: 0a2573a0
(XEN) SP: 8000fff7fb30
(XEN) CPSR: 0249 MODE:64-bit EL2h (Hypervisor, handler)
[...]
(XEN) Xen call trace:
(XEN) [<0a257418>] iommu_remove_dt_device+0x8c/0xd4 (PC)
(XEN) [<0a2573a0>] iommu_remove_dt_device+0x14/0xd4 (LR)
(XEN) [<0a20797c>] dt-overlay.c#remove_node_resources+0x8c/0x90
(XEN) [<0a207f14>] dt-overlay.c#remove_nodes+0x524/0x648
(XEN) [<0a208460>] dt_overlay_sysctl+0x428/0xc68
(XEN) [<0a2707f8>] arch_do_sysctl+0x1c/0x2c
(XEN) [<0a230b40>] do_sysctl+0x96c/0x9ec
(XEN) [<0a271e08>] traps.c#do_trap_hypercall+0x1e8/0x288
(XEN) [<0a273490>] do_trap_guest_sync+0x448/0x63c
(XEN) [<0a25c480>] entry.o#guest_sync_slowpath+0xa8/0xd8
(XEN)
(XEN)
(XEN)
(XEN) Panic on CPU 0:
(XEN) Assertion 'rw_is_locked(_host_lock)' failed at
drivers/passthrough/device_tree.c:146
(XEN)
This is because iommu_remove_dt_device() is called without taking the
dt_host_lock. Fix the issue by taking and releasing the lock properly.
Fixes: 7e5c4a8b86f1 ("xen/arm: Implement device tree node removal
functionalities")
Signed-off-by: Henry Wang
---
xen/common/dt-overlay.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/xen/common/dt-overlay.c b/xen/common/dt-overlay.c
index 1b197381f6..39e4ba59dd 100644
--- a/xen/common/dt-overlay.c
+++ b/xen/common/dt-overlay.c
@@ -381,9 +381,14 @@ static int remove_node_resources(struct dt_device_node
*device_node)
{
if ( dt_device_is_protected(device_node) )
{
+write_lock(_host_lock);
rc = iommu_remove_dt_device(device_node);
if ( rc < 0 )
+{
+write_unlock(_host_lock);
return rc;
+}
+write_unlock(_host_lock);
}
}
--
2.34.1
[PATCH 00/15] Remaining patches for dynamic node programming using overlay dtbo
Hi all, This is the remaining series for the full functional "dynamic node programming using overlay dtbo" feature. The first part [1] has already been merged. Quoting from the original series, the first part has already made Xen aware of new device tree node which means updating the dt_host with overlay node information, and in this series, the goal is to map IRQ and IOMMU during runtime, where we will do the actual IOMMU and IRQ mapping to a running domain and will call unmap_mmio_regions() to remove the mapping. Also, documentation of the "dynamic node programming using overlay dtbo" feature is added. Patch 1 is a fix of [1] which is noticed during my local test, details please see the commit message. Gitlab CI for this series can be found in [2]. [1] https://lore.kernel.org/xen-devel/20230906011631.30310-1-vikram.garh...@amd.com/ [2] https://gitlab.com/xen-project/people/henryw/xen/-/pipelines/1265297506 Henry Wang (1): xen/commom/dt-overlay: Fix missing lock when remove the device Vikram Garhwal (14): xen/arm/gic: Enable interrupt assignment to running VM xen/arm: Always enable IOMMU tools/libs/light: Always enable IOMMU tools/libs/light: Increase nr_spi to 160 rangeset: Move struct range and struct rangeset to headerfile xen/overlay: Enable device tree overlay assignment to running domains tools: Add domain_id and expert mode for overlay operations tools/libs/light: Modify dtbo to domU linux dtbo format tools/xl: Share overlay with domU tools/helpers: Add get_overlay get_overlay: remove domU overlay xl/overlay: add remove operation to xenstore add a domU script to fetch overlays and applying them to linux docs: add device tree overlay documentation docs/misc/arm/overlay.txt | 172 + tools/helpers/Makefile | 8 + tools/helpers/get_overlay.c | 507 ++ tools/helpers/get_overlay.sh| 81 + tools/include/libxl.h | 8 +- tools/include/xenctrl.h | 5 +- tools/libs/ctrl/xc_dt_overlay.c | 7 +- tools/libs/light/libxl_arm.c| 9 +- tools/libs/light/libxl_dt_overlay.c | 90 - tools/xl/Makefile | 2 +- tools/xl/xl_cmdtable.c | 2 +- tools/xl/xl_vmcontrol.c | 539 +++- xen/arch/arm/device.c | 8 +- xen/arch/arm/dom0less-build.c | 3 +- xen/arch/arm/domain_build.c | 2 +- xen/arch/arm/gic.c | 4 + xen/arch/arm/include/asm/setup.h| 3 +- xen/common/dt-overlay.c | 223 ++-- xen/common/rangeset.c | 31 +- xen/include/public/sysctl.h | 4 +- xen/include/xen/rangeset.h | 32 +- 21 files changed, 1654 insertions(+), 86 deletions(-) create mode 100644 docs/misc/arm/overlay.txt create mode 100644 tools/helpers/get_overlay.c create mode 100755 tools/helpers/get_overlay.sh -- 2.34.1
[PATCH 02/15] xen/arm/gic: Enable interrupt assignment to running VM
From: Vikram Garhwal Enable interrupt assign/remove for running VMs in CONFIG_OVERLAY_DTB. Currently, irq_route and mapping is only allowed at the domain creation. Adding exception for CONFIG_OVERLAY_DTB. Signed-off-by: Vikram Garhwal Signed-off-by: Stefano Stabellini Signed-off-by: Henry Wang --- xen/arch/arm/gic.c | 4 1 file changed, 4 insertions(+) diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c index 44c40e86de..a775f886ed 100644 --- a/xen/arch/arm/gic.c +++ b/xen/arch/arm/gic.c @@ -140,8 +140,10 @@ int gic_route_irq_to_guest(struct domain *d, unsigned int virq, * back to the physical IRQ. To prevent get unsync, restrict the * routing to when the Domain is been created. */ +#ifndef CONFIG_OVERLAY_DTB if ( d->creation_finished ) return -EBUSY; +#endif ret = vgic_connect_hw_irq(d, NULL, virq, desc, true); if ( ret ) @@ -171,8 +173,10 @@ int gic_remove_irq_from_guest(struct domain *d, unsigned int virq, * Removing an interrupt while the domain is running may have * undesirable effect on the vGIC emulation. */ +#ifndef CONFIG_OVERLAY_DTB if ( !d->is_dying ) return -EBUSY; +#endif desc->handler->shutdown(desc); -- 2.34.1
[linux-5.4 test] 185769: regressions - FAIL
flight 185769 linux-5.4 real [real] http://logs.test-lab.xenproject.org/osstest/logs/185769/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: build-arm64-pvopsbroken in 185433 build-arm64-xsm broken in 185433 build-arm64-libvirt broken in 185433 build-arm64-libvirt 5 host-build-prep fail in 185433 REGR. vs. 185168 build-arm64-pvops5 host-build-prep fail in 185433 REGR. vs. 185168 build-arm64-xsm 5 host-build-prep fail in 185433 REGR. vs. 185168 test-arm64-arm64-libvirt-raw 17 guest-start/debian.repeat fail in 185765 REGR. vs. 185168 Tests which are failing intermittently (not blocking): test-amd64-i386-qemut-rhel6hvm-amd 14 guest-start/redhat.repeat fail in 185433 pass in 185769 test-amd64-i386-libvirt-raw 12 debian-di-install fail pass in 185433 test-amd64-i386-libvirt-qcow2 12 debian-di-install fail pass in 185433 test-amd64-i386-xl-vhd 12 debian-di-install fail pass in 185433 test-armhf-armhf-examine 8 reboot fail pass in 185765 test-arm64-arm64-libvirt-raw 13 guest-startfail pass in 185765 test-amd64-amd64-xl-qcow221 guest-start/debian.repeat fail pass in 185765 Tests which did not succeed, but are not blocking: test-arm64-arm64-xl-credit1 1 build-check(1) blocked in 185433 n/a test-arm64-arm64-libvirt-raw 1 build-check(1) blocked in 185433 n/a test-arm64-arm64-xl-thunderx 1 build-check(1) blocked in 185433 n/a test-arm64-arm64-examine 1 build-check(1) blocked in 185433 n/a test-arm64-arm64-xl-credit2 1 build-check(1) blocked in 185433 n/a test-arm64-arm64-xl-vhd 1 build-check(1) blocked in 185433 n/a test-arm64-arm64-xl 1 build-check(1) blocked in 185433 n/a test-arm64-arm64-libvirt-xsm 1 build-check(1) blocked in 185433 n/a test-arm64-arm64-xl-xsm 1 build-check(1) blocked in 185433 n/a test-armhf-armhf-xl-credit1 19 guest-start.2 fail blocked in 185168 test-armhf-armhf-xl-credit2 19 guest-start.2 fail in 185433 blocked in 185168 test-armhf-armhf-xl-credit1 14 guest-start fail in 185433 like 185168 test-amd64-i386-libvirt-raw 14 migrate-support-check fail in 185433 never pass test-amd64-i386-libvirt-qcow2 14 migrate-support-check fail in 185433 never pass test-armhf-armhf-xl-credit1 18 guest-start/debian.repeat fail in 185765 blocked in 185168 test-arm64-arm64-libvirt-raw 14 migrate-support-check fail in 185765 never pass test-arm64-arm64-libvirt-raw 15 saverestore-support-check fail in 185765 never pass test-armhf-armhf-libvirt 16 saverestore-support-checkfail like 185168 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 185168 test-amd64-i386-xl-qemut-win7-amd64 19 guest-stop fail like 185168 test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 185168 test-armhf-armhf-xl-credit2 18 guest-start/debian.repeatfail like 185168 test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 185168 test-armhf-armhf-xl-multivcpu 18 guest-start/debian.repeatfail like 185168 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 185168 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 185168 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 185168 test-amd64-i386-xl-qemut-ws16-amd64 19 guest-stop fail like 185168 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 185168 test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-i386-xl-pvshim14 guest-start fail never pass test-amd64-i386-libvirt 15 migrate-support-checkfail never pass test-amd64-i386-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-arm64-arm64-xl 15 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 15 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 16 saverestore-support-checkfail never pass test-arm64-arm64-xl 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit2 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit1 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 16 saverestore-support-checkfail never pass
[xen-unstable-smoke test] 185775: tolerable all pass - PUSHED
flight 185775 xen-unstable-smoke real [real] http://logs.test-lab.xenproject.org/osstest/logs/185775/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 16 saverestore-support-checkfail never pass test-armhf-armhf-xl 15 migrate-support-checkfail never pass test-armhf-armhf-xl 16 saverestore-support-checkfail never pass version targeted for testing: xen 77e25f0e30ddd11e043e6fce84bf108ce7de5b6f baseline version: xen cccb7878f386fb8691b7e28957a562a66d9b875f Last test of basis 185760 2024-04-22 14:02:06 Z1 days Testing same since 185770 2024-04-23 13:00:23 Z0 days2 attempts People who touched revisions under test: Anthony PERARD Jan Beulich Leigh Brown Roger Pau Monné Ross Lagerwall jobs: build-arm64-xsm pass build-amd64 pass build-armhf pass build-amd64-libvirt pass test-armhf-armhf-xl pass test-arm64-arm64-xl-xsm pass test-amd64-amd64-xl-qemuu-debianhvm-amd64pass test-amd64-amd64-libvirt pass sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master Test harness code can be found at http://xenbits.xen.org/gitweb?p=osstest.git;a=summary Pushing revision : To xenbits.xen.org:/home/xen/git/xen.git cccb7878f3..77e25f0e30 77e25f0e30ddd11e043e6fce84bf108ce7de5b6f -> smoke
[linux-linus test] 185768: tolerable FAIL - PUSHED
flight 185768 linux-linus real [real] flight 185778 linux-linus real-retest [real] http://logs.test-lab.xenproject.org/osstest/logs/185768/ http://logs.test-lab.xenproject.org/osstest/logs/185778/ Failures :-/ but no regressions. Tests which are failing intermittently (not blocking): test-amd64-amd64-xl-qemut-debianhvm-i386-xsm 12 debian-hvm-install fail pass in 185778-retest Tests which did not succeed, but are not blocking: test-armhf-armhf-libvirt 16 saverestore-support-check fail blocked in 185750 test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 185750 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 185750 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 185750 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 185750 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 185750 test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt-qcow2 14 migrate-support-checkfail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-arm64-arm64-xl 15 migrate-support-checkfail never pass test-arm64-arm64-xl 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-thunderx 15 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 16 saverestore-support-checkfail never pass test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit1 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit2 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-multivcpu 15 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit2 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 16 saverestore-support-checkfail never pass test-armhf-armhf-xl 15 migrate-support-checkfail never pass test-armhf-armhf-xl 16 saverestore-support-checkfail never pass test-armhf-armhf-libvirt 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt-raw 14 migrate-support-checkfail never pass test-amd64-amd64-libvirt-vhd 14 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 16 saverestore-support-checkfail never pass test-arm64-arm64-libvirt-raw 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-raw 15 saverestore-support-checkfail never pass test-arm64-arm64-xl-vhd 14 migrate-support-checkfail never pass test-arm64-arm64-xl-vhd 15 saverestore-support-checkfail never pass test-armhf-armhf-xl-qcow214 migrate-support-checkfail never pass test-armhf-armhf-xl-qcow215 saverestore-support-checkfail never pass test-armhf-armhf-xl-raw 14 migrate-support-checkfail never pass test-armhf-armhf-xl-raw 15 saverestore-support-checkfail never pass test-armhf-armhf-xl-rtds 15 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 16 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-vhd 14 migrate-support-checkfail never pass test-armhf-armhf-libvirt-vhd 15 saverestore-support-checkfail never pass test-armhf-armhf-xl-arndale 15 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 16 saverestore-support-checkfail never pass version targeted for testing: linux71b1543c83d65af8215d7558d70fc2ecbee77dcf baseline version: linux977b1ef51866aa7170409af80740788d4f9c4841 Last test of basis 185750 2024-04-21 12:13:57 Z2 days Failing since185753 2024-04-21 22:44:09 Z2 days4 attempts Testing same since 185768 2024-04-23 07:46:10 Z0 days1 attempts People who touched revisions under test: Alan Stern Alexander Usyskin Andy Shevchenko Andy Shevchenko AngeloGioacchino Del Regno bolan wang Borislav Petkov (AMD) Carlos Llamas Christian A. Ehrhardt Chuanhong Guo Chuck Lever Coia Prant Dan Carpenter Daniele Palmas Dave Hansen Dave Hansen #
[ovmf test] 185776: all pass - PUSHED
flight 185776 ovmf real [real] http://logs.test-lab.xenproject.org/osstest/logs/185776/ Perfect :-) All tests in this flight passed as required version targeted for testing: ovmf d97f964f7ce063f9861f4d21cc6352f6861f95a8 baseline version: ovmf e3fa6986ae8521275fc6ca161f7394a3809f8723 Last test of basis 185773 2024-04-23 17:41:11 Z0 days Testing same since 185776 2024-04-23 21:41:06 Z0 days1 attempts People who touched revisions under test: Gua Guo jobs: build-amd64-xsm pass build-i386-xsm pass build-amd64 pass build-i386 pass build-amd64-libvirt pass build-i386-libvirt pass build-amd64-pvopspass build-i386-pvops pass test-amd64-amd64-xl-qemuu-ovmf-amd64 pass sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master Test harness code can be found at http://xenbits.xen.org/gitweb?p=osstest.git;a=summary Pushing revision : To xenbits.xen.org:/home/xen/git/osstest/ovmf.git e3fa6986ae..d97f964f7c d97f964f7ce063f9861f4d21cc6352f6861f95a8 -> xen-tested-master
Re: [PATCH] x86/monitor: allow fast-singlestepping without enabling singlestep monitor
On Sun, Apr 14, 2024 at 2:21 PM Petr Beneš wrote: > > From: Petr Beneš > > Reorder the condition checks within the HVM_MONITOR_SINGLESTEP_BREAKPOINT > case to enable fast singlestepping independently of the singlestep monitor > being enabled. Previously, fast singlestepping required the singlestep > monitor to be explicitly enabled through xc_monitor_singlestep, even though > it operates entirely within Xen and does not generate external events. > > Signed-off-by: Petr Beneš Acked-by: Tamas K Lengyel
Re: [PATCH 1/4] x86/P2M: write_p2m_entry() is HVM-only anyway
On 23/04/2024 3:31 pm, Jan Beulich wrote:
> The latest as of e2b2ff677958 ("x86/P2M: split out init/teardown
> functions") the function is obviously unreachable for PV guests.
This doesn't parse. Do you mean "Since e2b2ff677958 ..." ?
> Hence
> the paging_mode_enabled(d) check is pointless.
>
> Further host mode of a vCPU is always set, by virtue of
> paging_vcpu_init() being part of vCPU creation. Hence the
> paging_get_hostmode() check is pointless.
>
> With that the v local variable is unnecessary too. Drop the "if()"
> conditional and its corresponding "else".
>
> Signed-off-by: Jan Beulich
> ---
> I have to confess that this if() has been puzzling me before.
Puzzling yes, but it can't blindly be dropped.
This is the "did the toolstack initiate this update" check. i.e. I
think it's "bypass the normal side effects of making this update".
I suspect it exists because of improper abstraction between the guest
physmap and the shadow pagetables as-were - which were/are tighly
coupled to vCPUs even for aspects where they shouldn't have been.
For better or worse, the toolstack can add_to_physmap() before it
creates vCPUs, and it will take this path you're trying to delete.
There may be other cases too; I could see foreign mapping ending up
ticking this too.
Whether we ought to permit a toolstack to do this is a different
question, but seeing as we explicitly intend (eventually for AMX) have a
set_policy call between domain_create() and vcpu_create(), I don't think
we can reasably restrict other hypercalls too in this period.
~Andrew
[ovmf test] 185773: all pass - PUSHED
flight 185773 ovmf real [real] http://logs.test-lab.xenproject.org/osstest/logs/185773/ Perfect :-) All tests in this flight passed as required version targeted for testing: ovmf e3fa6986ae8521275fc6ca161f7394a3809f8723 baseline version: ovmf 86c8d69146310f24069701053a27153ae536ebba Last test of basis 185764 2024-04-22 23:12:58 Z0 days Testing same since 185773 2024-04-23 17:41:11 Z0 days1 attempts People who touched revisions under test: Adam Dunlap jobs: build-amd64-xsm pass build-i386-xsm pass build-amd64 pass build-i386 pass build-amd64-libvirt pass build-i386-libvirt pass build-amd64-pvopspass build-i386-pvops pass test-amd64-amd64-xl-qemuu-ovmf-amd64 pass sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master Test harness code can be found at http://xenbits.xen.org/gitweb?p=osstest.git;a=summary Pushing revision : To xenbits.xen.org:/home/xen/git/osstest/ovmf.git 86c8d69146..e3fa6986ae e3fa6986ae8521275fc6ca161f7394a3809f8723 -> xen-tested-master
[xen-unstable-smoke test] 185770: regressions - FAIL
flight 185770 xen-unstable-smoke real [real]
flight 185772 xen-unstable-smoke real-retest [real]
http://logs.test-lab.xenproject.org/osstest/logs/185770/
http://logs.test-lab.xenproject.org/osstest/logs/185772/
Regressions :-(
Tests which did not succeed and are blocking,
including tests which could not be run:
test-armhf-armhf-xl 8 xen-boot fail REGR. vs. 185760
Tests which did not succeed, but are not blocking:
test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass
test-arm64-arm64-xl-xsm 15 migrate-support-checkfail never pass
test-arm64-arm64-xl-xsm 16 saverestore-support-checkfail never pass
version targeted for testing:
xen 77e25f0e30ddd11e043e6fce84bf108ce7de5b6f
baseline version:
xen cccb7878f386fb8691b7e28957a562a66d9b875f
Last test of basis 185760 2024-04-22 14:02:06 Z1 days
Testing same since 185770 2024-04-23 13:00:23 Z0 days1 attempts
People who touched revisions under test:
Anthony PERARD
Jan Beulich
Leigh Brown
Roger Pau Monné
Ross Lagerwall
jobs:
build-arm64-xsm pass
build-amd64 pass
build-armhf pass
build-amd64-libvirt pass
test-armhf-armhf-xl fail
test-arm64-arm64-xl-xsm pass
test-amd64-amd64-xl-qemuu-debianhvm-amd64pass
test-amd64-amd64-libvirt pass
sg-report-flight on osstest.test-lab.xenproject.org
logs: /home/logs/logs
images: /home/logs/images
Logs, config files, etc. are available at
http://logs.test-lab.xenproject.org/osstest/logs
Explanation of these reports, and of osstest in general, is at
http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master
http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master
Test harness code can be found at
http://xenbits.xen.org/gitweb?p=osstest.git;a=summary
Not pushing.
commit 77e25f0e30ddd11e043e6fce84bf108ce7de5b6f
Author: Jan Beulich
Date: Tue Apr 23 14:13:48 2024 +0200
x86/MTRR: correct inadvertently inverted WC check
The ! clearly got lost by mistake.
Fixes: e9e0eb30d4d6 ("x86/MTRR: avoid several indirect calls")
Reported-by: Marek Marczykowski-Górecki
Signed-off-by: Jan Beulich
Acked-by: Roger Pau Monné
commit f82c43a384913460892d4917d2a2f8c2b9399a5e
Author: Roger Pau Monné
Date: Tue Apr 23 14:12:04 2024 +0200
xen: introduce header file with section related symbols
Start by declaring the beginning and end of the init section.
No functional change intended.
Requested-by: Andrew Cooper
Signed-off-by: Roger Pau Monné
Reviewed-by: Andrew Cooper
commit 70d46b51e2f80f808c91f264f501cca1ca7af2b5
Author: Leigh Brown
Date: Tue Apr 23 14:11:14 2024 +0200
docs/man: Add xenwatchdog manual page
Add a manual page for xenwatchdogd.
Signed-off-by: Leigh Brown
Reviewed-by: Anthony PERARD
commit 9c872b5766cde6cd9ebeb724a346aaee16e87c84
Author: Leigh Brown
Date: Tue Apr 23 14:10:16 2024 +0200
tools/misc: Add xenwatchdogd.c copyright notice
Add copyright notice and description of the program.
Signed-off-by: Leigh Brown
Acked-by: Anthony PERARD
commit e906bfae48b2f71607159e7f5589cb71f961351c
Author: Leigh Brown
Date: Tue Apr 23 14:10:03 2024 +0200
tools/misc: xenwatchdogd enhancements
Add usage() function, the ability to run in the foreground, and
the ability to disarm the watchdog timer when exiting.
Add enhanced parameter parsing and validation, making use of
getopt_long(). Check the number of parameters are correct, the
timeout is at least two seconds (to allow a minimum sleep time of
one second), and that the sleep time is at least one and less
than the watchdog timeout.
With these changes, the daemon will no longer instantly reboot
the domain if you enter a zero timeout (or non-numeric parameter),
and prevent the daemon consuming 100% of a CPU due to zero sleep
time.
Signed-off-by: Leigh Brown
Reviewed-by: Anthony PERARD
commit f0fa75c9ea9f03cf2c6f5e63d07664ca4b224dd6
Author: Leigh Brown
Date: Tue Apr 23 14:09:50 2024 +0200
tools/misc: xenwatchdogd: add parse_secs()
Create a new parse_secs() function to parse the timeout and sleep
parameters. This ensures that non-numeric parameters are not
accidentally treated as numbers.
Signed-off-by: Leigh Brown
[xen-unstable test] 185767: tolerable FAIL
flight 185767 xen-unstable real [real] http://logs.test-lab.xenproject.org/osstest/logs/185767/ Failures :-/ but no regressions. Tests which are failing intermittently (not blocking): test-amd64-amd64-xl-pvshim 13 debian-fixup fail in 185762 pass in 185767 test-armhf-armhf-xl-rtds 8 xen-boot fail pass in 185762 Tests which did not succeed, but are not blocking: test-armhf-armhf-xl-rtds15 migrate-support-check fail in 185762 never pass test-armhf-armhf-xl-rtds 16 saverestore-support-check fail in 185762 never pass test-armhf-armhf-libvirt 16 saverestore-support-checkfail like 185762 test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 185762 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 185762 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 185762 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 185762 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 185762 test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-arm64-arm64-xl 15 migrate-support-checkfail never pass test-arm64-arm64-xl 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit2 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-xsm 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-thunderx 15 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-qcow2 14 migrate-support-checkfail never pass test-armhf-armhf-xl 15 migrate-support-checkfail never pass test-armhf-armhf-xl 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit2 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-multivcpu 15 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 16 saverestore-support-checkfail never pass test-armhf-armhf-libvirt 15 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 15 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-raw 14 migrate-support-checkfail never pass test-amd64-amd64-libvirt-vhd 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-raw 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-raw 15 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit1 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-vhd 14 migrate-support-checkfail never pass test-arm64-arm64-xl-vhd 15 saverestore-support-checkfail never pass test-armhf-armhf-xl-qcow214 migrate-support-checkfail never pass test-armhf-armhf-xl-qcow215 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit1 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 16 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-vhd 14 migrate-support-checkfail never pass test-armhf-armhf-libvirt-vhd 15 saverestore-support-checkfail never pass test-armhf-armhf-xl-raw 14 migrate-support-checkfail never pass test-armhf-armhf-xl-raw 15 saverestore-support-checkfail never pass version targeted for testing: xen cccb7878f386fb8691b7e28957a562a66d9b875f baseline version: xen cccb7878f386fb8691b7e28957a562a66d9b875f Last test of basis 185767 2024-04-23 05:03:58 Z0 days Testing same since (not found) 0 attempts jobs: build-amd64-xsm pass build-arm64-xsm pass build-i386-xsm pass build-amd64-xtf pass build-amd64 pass build-arm64 pass build-armhf pass
Re: [PATCH v2 5/5] x86: Add --force option to xen-ucode to override microcode version check
On Tue, Apr 23, 2024 at 04:26:53PM +0100, Fouad Hilly wrote: > On Mon, Apr 22, 2024 at 6:57 PM Anthony PERARD > wrote: > > On Tue, Apr 16, 2024 at 10:15:46AM +0100, Fouad Hilly wrote: > > > diff --git a/tools/misc/xen-ucode.c b/tools/misc/xen-ucode.c > > > index e3c1943e3633..4178fd2221ea 100644 > > > --- a/tools/misc/xen-ucode.c > > > +++ b/tools/misc/xen-ucode.c > > > @@ -24,7 +26,8 @@ static void usage(const char *name) > > > "Usage: %s [microcode file] [options]\n" > > > > Now, that usage line is wrong. The options needs to go before the file. > I am not sure what you mean "wrong"? from parsing perspective, both > scenarios can be successfully executed: > xen-ucode firmware-file --force > xen-ucode --force firmware-file > it becomes wrong if there is an argument expects the file as an input. Oh, sorry, I didn't know that getopt() would look for options on all arguments, even when separated by nonoptions. I'm used to CLI programs that takes options first, then files, even if that might work with a different order. Cheers, -- Anthony PERARD
Re: [PATCH v2 4/5] x86: Use getopt to handle command line args
On Tue, Apr 23, 2024 at 04:16:10PM +0100, Fouad Hilly wrote: > On Mon, Apr 22, 2024 at 6:48 PM Anthony PERARD > wrote: > > On Tue, Apr 16, 2024 at 10:15:45AM +0100, Fouad Hilly wrote: > > > +if ( argc != 2 ) > > > +goto ext_err; > > > > Why only two arguments allowed? And why check `argc` before calling > > getopt_long() ? > At this stage of the patch series, xen-ucode expects either firmware > file or a single argument, that is why argc should be 2. > If there is no argument or many arguments that are not supposed to be, > we exit with error other than try to parse the arguments. Right, but what's the point of introducing getopt_long() if we are not going to use it? The patch tries to make it nicer to introduce more options to the tool but then put an extra check that make this actually harder. This condition is even change in the next patch, that's one more reason that says that it's a wrong check. You can let getopt_long() parse all the options, deal with them, then you can deal with the nonoptions after that, and check that's there's only one nonoption. > > > + ext_err: > > > +fprintf(stderr, > > > +"xen-ucode: Xen microcode updating tool\n" > > > +"Usage: %s [microcode file] [options]\n", argv[0]); > > > > Isn't there a usage() function that we could use? > As there is an error, stderr should be used, which was a comment on V1. > > > > > +show_curr_cpu(stderr); > > > > Why call show_curr_cpu() on the error path? > Informative, but could be removed. We are on the error path, it's looks like the usage message is printed before the cpu information, so if the error is due to wrong options then that information is lost. We should print why there's an error, not much else would be useful. Error messages should be as late as possible or they are getting lost in the scroll of the terminal. So yes, it's probably best to remove. Thanks, -- Anthony PERARD
[PATCH 3/3] automation: Add arm64 test for running Xen with GICv3
At the moment, all the Arm64 Qemu tests use GICv2 which is the default
GIC version used by Qemu. Improve the coverage by adding a new test in
which Qemu will be configured to have GICv3.
Rename host device tree name to "virt.dtb" to be GIC version agnostic.
Use "gic-version" Qemu option to select the version to use. Unless the
test variant is set to "gicv3", version 2 will be used.
Signed-off-by: Michal Orzel
---
automation/gitlab-ci/test.yaml| 8
.../scripts/qemu-smoke-dom0less-arm64.sh | 19 ++-
2 files changed, 22 insertions(+), 5 deletions(-)
diff --git a/automation/gitlab-ci/test.yaml b/automation/gitlab-ci/test.yaml
index 1e5d86763f6c..ad249fa0a5d9 100644
--- a/automation/gitlab-ci/test.yaml
+++ b/automation/gitlab-ci/test.yaml
@@ -255,6 +255,14 @@ qemu-smoke-dom0less-arm64-gcc-debug:
- *arm64-test-needs
- alpine-3.18-gcc-debug-arm64
+qemu-smoke-dom0less-arm64-gcc-debug-gicv3:
+ extends: .qemu-arm64
+ script:
+- ./automation/scripts/qemu-smoke-dom0less-arm64.sh gicv3 2>&1 | tee
${LOGFILE}
+ needs:
+- *arm64-test-needs
+- alpine-3.18-gcc-debug-arm64
+
qemu-smoke-dom0less-arm64-gcc-debug-staticmem:
extends: .qemu-arm64
script:
diff --git a/automation/scripts/qemu-smoke-dom0less-arm64.sh
b/automation/scripts/qemu-smoke-dom0less-arm64.sh
index fc943a1a622c..292c38a56147 100755
--- a/automation/scripts/qemu-smoke-dom0less-arm64.sh
+++ b/automation/scripts/qemu-smoke-dom0less-arm64.sh
@@ -4,6 +4,9 @@ set -ex
test_variant=$1
+# Default GIC version
+gic_version="2"
+
if [ -z "${test_variant}" ]; then
passed="ping test passed"
domU_check="
@@ -66,16 +69,22 @@ if [[ "${test_variant}" == "earlyprintk" ]]; then
passed="\- Ready \-"
fi
+if [[ "${test_variant}" == "gicv3" ]]; then
+gic_version=3
+passed="${test_variant} test passed"
+domU_check="echo \"${passed}\""
+fi
+
# XXX QEMU looks for "efi-virtio.rom" even if it is unneeded
curl -fsSLO https://github.com/qemu/qemu/raw/v5.2.0/pc-bios/efi-virtio.rom
./binaries/qemu-system-aarch64 \
-machine virtualization=true \
- -cpu cortex-a57 -machine type=virt \
+ -cpu cortex-a57 -machine type=virt,gic-version=$gic_version \
-m 2048 -smp 2 -display none \
- -machine dumpdtb=binaries/virt-gicv2.dtb
+ -machine dumpdtb=binaries/virt.dtb
# XXX disable pl061 to avoid Linux crash
-fdtput binaries/virt-gicv2.dtb -p -t s /pl061@903 status disabled
+fdtput binaries/virt.dtb -p -t s /pl061@903 status disabled
# Busybox
mkdir -p initrd
@@ -138,7 +147,7 @@ cd ..
echo 'MEMORY_START="0x4000"
MEMORY_END="0x5000"
-DEVICE_TREE="virt-gicv2.dtb"
+DEVICE_TREE="virt.dtb"
XEN="xen"
DOM0_KERNEL="Image"
DOM0_RAMDISK="dom0-rootfs.cpio.gz"
@@ -200,7 +209,7 @@ echo " virtio scan; dhcp; tftpb 0x4000 boot.scr;
source 0x4000"| \
timeout -k 1 240 \
./binaries/qemu-system-aarch64 \
-machine virtualization=true \
--cpu cortex-a57 -machine type=virt \
+-cpu cortex-a57 -machine type=virt,gic-version=$gic_version \
-m 2048 -monitor none -serial stdio \
-smp 2 \
-no-reboot \
--
2.25.1
[PATCH 2/3] automation: Add arm{64,32} earlyprintk jobs
Introduce qemu based Arm earlyprintk test and build jobs to cover this
feature in debug variant. The tests simply check for the presence of the
last message printed by the bootstrap code before entering the C world.
Signed-off-by: Michal Orzel
---
automation/gitlab-ci/build.yaml | 17 +
automation/gitlab-ci/test.yaml | 16
automation/scripts/qemu-smoke-dom0less-arm32.sh | 7 +++
automation/scripts/qemu-smoke-dom0less-arm64.sh | 5 +
4 files changed, 45 insertions(+)
diff --git a/automation/gitlab-ci/build.yaml b/automation/gitlab-ci/build.yaml
index f3c934471f32..49d6265ad5b4 100644
--- a/automation/gitlab-ci/build.yaml
+++ b/automation/gitlab-ci/build.yaml
@@ -402,6 +402,15 @@ debian-bookworm-gcc-arm32-debug-staticmem:
CONFIG_UNSUPPORTED=y
CONFIG_STATIC_MEMORY=y
+debian-bookworm-gcc-arm32-debug-earlyprintk:
+ extends: .gcc-arm32-cross-build-debug
+ variables:
+CONTAINER: debian:bookworm-arm64v8-arm32-gcc
+HYPERVISOR_ONLY: y
+EXTRA_XEN_CONFIG: |
+ CONFIG_EARLY_UART_CHOICE_PL011=y
+ CONFIG_EARLY_UART_BASE_ADDRESS=0x900
+
# Arm builds
debian-bookworm-gcc-arm64:
@@ -473,6 +482,14 @@ alpine-3.18-gcc-debug-arm64-boot-cpupools:
EXTRA_XEN_CONFIG: |
CONFIG_BOOT_TIME_CPUPOOLS=y
+alpine-3.18-gcc-debug-arm64-earlyprintk:
+ extends: .gcc-arm64-build-debug
+ variables:
+CONTAINER: alpine:3.18-arm64v8
+EXTRA_XEN_CONFIG: |
+ CONFIG_EARLY_UART_CHOICE_PL011=y
+ CONFIG_EARLY_UART_BASE_ADDRESS=0x900
+
# RISC-V 64 cross-build
.riscv-fixed-randconfig:
variables:
diff --git a/automation/gitlab-ci/test.yaml b/automation/gitlab-ci/test.yaml
index 55a7831ad292..1e5d86763f6c 100644
--- a/automation/gitlab-ci/test.yaml
+++ b/automation/gitlab-ci/test.yaml
@@ -287,6 +287,14 @@ qemu-smoke-dom0less-arm64-gcc-debug-boot-cpupools:
- *arm64-test-needs
- alpine-3.18-gcc-debug-arm64-boot-cpupools
+qemu-smoke-dom0less-arm64-gcc-debug-earlyprintk:
+ extends: .qemu-arm64
+ script:
+- ./automation/scripts/qemu-smoke-dom0less-arm64.sh earlyprintk 2>&1 | tee
${LOGFILE}
+ needs:
+- *arm64-test-needs
+- alpine-3.18-gcc-debug-arm64-earlyprintk
+
qemu-xtf-dom0less-arm64-gcc-hyp-xen-version:
extends: .qemu-arm64
script:
@@ -359,6 +367,14 @@ qemu-smoke-dom0less-arm32-gcc-debug-without-dom0:
- *arm32-test-needs
- debian-bookworm-gcc-arm32-debug
+qemu-smoke-dom0less-arm32-gcc-debug-earlyprintk:
+ extends: .qemu-arm32
+ script:
+- ./automation/scripts/qemu-smoke-dom0less-arm32.sh earlyprintk 2>&1 | tee
${LOGFILE}
+ needs:
+- *arm32-test-needs
+- debian-bookworm-gcc-arm32-debug-earlyprintk
+
qemu-alpine-x86_64-gcc:
extends: .qemu-x86-64
script:
diff --git a/automation/scripts/qemu-smoke-dom0less-arm32.sh
b/automation/scripts/qemu-smoke-dom0less-arm32.sh
index e31b6b9014e1..1e2b939aadf7 100755
--- a/automation/scripts/qemu-smoke-dom0less-arm32.sh
+++ b/automation/scripts/qemu-smoke-dom0less-arm32.sh
@@ -53,6 +53,13 @@ echo \"${passed}\"
"
fi
+if [[ "${test_variant}" == "earlyprintk" ]]; then
+# Clear dom0 prompt
+dom0_prompt=""
+# Last early printk message before entering C world
+passed="\- Ready \-"
+fi
+
# dom0/domU rootfs
# We are using the same rootfs for dom0 and domU. The only difference is
# that for the former, we set explictly rdinit to /bin/sh, whereas for the
diff --git a/automation/scripts/qemu-smoke-dom0less-arm64.sh
b/automation/scripts/qemu-smoke-dom0less-arm64.sh
index e748b8ef1699..fc943a1a622c 100755
--- a/automation/scripts/qemu-smoke-dom0less-arm64.sh
+++ b/automation/scripts/qemu-smoke-dom0less-arm64.sh
@@ -61,6 +61,11 @@ fi
"
fi
+if [[ "${test_variant}" == "earlyprintk" ]]; then
+# Last early printk message before entering C world
+passed="\- Ready \-"
+fi
+
# XXX QEMU looks for "efi-virtio.rom" even if it is unneeded
curl -fsSLO https://github.com/qemu/qemu/raw/v5.2.0/pc-bios/efi-virtio.rom
./binaries/qemu-system-aarch64 \
--
2.25.1
[PATCH 0/3] automation: improve Arm coverage
This came up as part of the following discussion:
https://lore.kernel.org/xen-devel/alpine.DEB.2.22.394.2402291108290.853156@ubuntu-linux-20-04-desktop/
Michal Orzel (3):
automation: Drop some of the non-debug variants of the same Arm jobs
automation: Add arm{64,32} earlyprintk jobs
automation: Add arm64 test for running Xen with GICv3
automation/gitlab-ci/build.yaml | 41 --
automation/gitlab-ci/test.yaml| 56 ++-
.../scripts/qemu-smoke-dom0less-arm32.sh | 7 +++
.../scripts/qemu-smoke-dom0less-arm64.sh | 24 ++--
4 files changed, 52 insertions(+), 76 deletions(-)
--
2.25.1
[PATCH 1/3] automation: Drop some of the non-debug variants of the same Arm jobs
To save some bandwith that can be later on used to increase the test
coverage by adding new tests, drop the following non-debug test/build
jobs existing in both debug and non-debug variants:
- static memory (arm64, arm32)
- static shared memory (arm64)
- static heap (arm64)
- boot cpupools (arm64)
- gzip (arm32)
More generic tests existing in both variants were left unmodified.
Signed-off-by: Michal Orzel
---
automation/gitlab-ci/build.yaml | 38 --
automation/gitlab-ci/test.yaml | 48 -
2 files changed, 86 deletions(-)
diff --git a/automation/gitlab-ci/build.yaml b/automation/gitlab-ci/build.yaml
index aac29ee13ab6..f3c934471f32 100644
--- a/automation/gitlab-ci/build.yaml
+++ b/automation/gitlab-ci/build.yaml
@@ -392,16 +392,6 @@ debian-bookworm-gcc-arm32-debug-randconfig:
HYPERVISOR_ONLY: y
RANDCONFIG: y
-debian-bookworm-gcc-arm32-staticmem:
- extends: .gcc-arm32-cross-build
- variables:
-CONTAINER: debian:bookworm-arm64v8-arm32-gcc
-HYPERVISOR_ONLY: y
-EXTRA_XEN_CONFIG: |
- CONFIG_EXPERT=y
- CONFIG_UNSUPPORTED=y
- CONFIG_STATIC_MEMORY=y
-
debian-bookworm-gcc-arm32-debug-staticmem:
extends: .gcc-arm32-cross-build-debug
variables:
@@ -458,15 +448,6 @@ alpine-3.18-gcc-debug-arm64-randconfig:
CONTAINER: alpine:3.18-arm64v8
RANDCONFIG: y
-alpine-3.18-gcc-arm64-staticmem:
- extends: .gcc-arm64-build
- variables:
-CONTAINER: alpine:3.18-arm64v8
-EXTRA_XEN_CONFIG: |
- CONFIG_EXPERT=y
- CONFIG_UNSUPPORTED=y
- CONFIG_STATIC_MEMORY=y
-
alpine-3.18-gcc-debug-arm64-staticmem:
extends: .gcc-arm64-build-debug
variables:
@@ -476,15 +457,6 @@ alpine-3.18-gcc-debug-arm64-staticmem:
CONFIG_UNSUPPORTED=y
CONFIG_STATIC_MEMORY=y
-alpine-3.18-gcc-arm64-static-shared-mem:
- extends: .gcc-arm64-build
- variables:
-CONTAINER: alpine:3.18-arm64v8
-EXTRA_XEN_CONFIG: |
- CONFIG_UNSUPPORTED=y
- CONFIG_STATIC_MEMORY=y
- CONFIG_STATIC_SHM=y
-
alpine-3.18-gcc-debug-arm64-static-shared-mem:
extends: .gcc-arm64-build-debug
variables:
@@ -494,16 +466,6 @@ alpine-3.18-gcc-debug-arm64-static-shared-mem:
CONFIG_STATIC_MEMORY=y
CONFIG_STATIC_SHM=y
-alpine-3.18-gcc-arm64-boot-cpupools:
- extends: .gcc-arm64-build
- variables:
-CONTAINER: alpine:3.18-arm64v8
-EXTRA_XEN_CONFIG: |
- CONFIG_EXPERT=y
- CONFIG_UNSUPPORTED=y
- CONFIG_SCHED_NULL=y
- CONFIG_BOOT_TIME_CPUPOOLS=y
-
alpine-3.18-gcc-debug-arm64-boot-cpupools:
extends: .gcc-arm64-build-debug
variables:
diff --git a/automation/gitlab-ci/test.yaml b/automation/gitlab-ci/test.yaml
index 8b7b2e4da92d..55a7831ad292 100644
--- a/automation/gitlab-ci/test.yaml
+++ b/automation/gitlab-ci/test.yaml
@@ -255,14 +255,6 @@ qemu-smoke-dom0less-arm64-gcc-debug:
- *arm64-test-needs
- alpine-3.18-gcc-debug-arm64
-qemu-smoke-dom0less-arm64-gcc-staticmem:
- extends: .qemu-arm64
- script:
-- ./automation/scripts/qemu-smoke-dom0less-arm64.sh static-mem 2>&1 | tee
${LOGFILE}
- needs:
-- *arm64-test-needs
-- alpine-3.18-gcc-arm64-staticmem
-
qemu-smoke-dom0less-arm64-gcc-debug-staticmem:
extends: .qemu-arm64
script:
@@ -271,14 +263,6 @@ qemu-smoke-dom0less-arm64-gcc-debug-staticmem:
- *arm64-test-needs
- alpine-3.18-gcc-debug-arm64-staticmem
-qemu-smoke-dom0less-arm64-gcc-staticheap:
- extends: .qemu-arm64
- script:
- - ./automation/scripts/qemu-smoke-dom0less-arm64.sh static-heap 2>&1 | tee
${LOGFILE}
- needs:
- - *arm64-test-needs
- - alpine-3.18-gcc-arm64
-
qemu-smoke-dom0less-arm64-gcc-debug-staticheap:
extends: .qemu-arm64
script:
@@ -287,14 +271,6 @@ qemu-smoke-dom0less-arm64-gcc-debug-staticheap:
- *arm64-test-needs
- alpine-3.18-gcc-debug-arm64
-qemu-smoke-dom0less-arm64-gcc-static-shared-mem:
- extends: .qemu-arm64
- script:
-- ./automation/scripts/qemu-smoke-dom0less-arm64.sh static-shared-mem 2>&1
| tee ${LOGFILE}
- needs:
-- *arm64-test-needs
-- alpine-3.18-gcc-arm64-static-shared-mem
-
qemu-smoke-dom0less-arm64-gcc-debug-static-shared-mem:
extends: .qemu-arm64
script:
@@ -303,14 +279,6 @@ qemu-smoke-dom0less-arm64-gcc-debug-static-shared-mem:
- *arm64-test-needs
- alpine-3.18-gcc-debug-arm64-static-shared-mem
-qemu-smoke-dom0less-arm64-gcc-boot-cpupools:
- extends: .qemu-arm64
- script:
-- ./automation/scripts/qemu-smoke-dom0less-arm64.sh boot-cpupools 2>&1 |
tee ${LOGFILE}
- needs:
-- *arm64-test-needs
-- alpine-3.18-gcc-arm64-boot-cpupools
-
qemu-smoke-dom0less-arm64-gcc-debug-boot-cpupools:
extends: .qemu-arm64
script:
@@ -359,14 +327,6 @@ qemu-smoke-dom0less-arm32-gcc-debug:
- *arm32-test-needs
- debian-bookworm-gcc-arm32-debug
-qemu-smoke-dom0less-arm32-gcc-staticmem:
- extends: .qemu-arm32
- script:
-- ./automation/scripts/qemu-smoke-dom0less-arm32.sh static-mem 2>&1 | tee
${LOGFILE}
- needs:
Re: [PATCH 2/5] x86: Refactor microcode_update() hypercall with flags field
On 23.04.2024 16:53, Fouad Hilly wrote:
> On Mon, Apr 8, 2024 at 10:16 AM Jan Beulich wrote:
>> On 05.04.2024 14:11, Fouad Hilly wrote:
>>> @@ -708,11 +712,13 @@ static long cf_check microcode_update_helper(void
>>> *data)
>>> return ret;
>>> }
>>>
>>> -int microcode_update(XEN_GUEST_HANDLE(const_void) buf, unsigned long len)
>>> +int microcode_update(XEN_GUEST_HANDLE(const_void) buf, unsigned long len,
>>> unsigned int flags)
>>> {
>>> int ret;
>>> struct ucode_buf *buffer;
>>>
>>> +ucode_force_flag = (flags == XENPF_UCODE_FLAG_FORCE_SET)? 1: 0;
>>
>> No need for ?: when the lhs has type bool.
>>
>> But - do we really need to resort to parameter passing via static variables
>> here? If it's unavoidable, its setting needs to move inside a locked region
>> (with that region covering everything up to all consumption of the value).
> There are many function calls and checks of the firmware between
> microcode_update() and the actual update, which makes static variable
> the viable option.
> In V2 I broke it down between the actual update_flags (static) and
> force_flag (local to firmware update function), I understand that
> might not be enough, I will look into further improvement for
> microcode_update flags in V3.
>>
>> Further, to avoid the same issue again when another flag wants adding, you
>> want to check that all other bits in the flags field are clear.
> The above check is checking all bits in the flags field. Are you
> referring to flag per bit where multiple flags can be set
> simultaneously?
No. What you do is treat a flags value of, say, 2 the same as a flags
value of 0. That's wrong when considering that the value 2 may gain a
meaning going forward. At this point you want to refuse flags values
other than 0 or 1.
Jan
Re: [PATCH v3 3/4] livepatch: refuse to resolve symbols that belong to init sections
On 23.04.2024 17:03, Roger Pau Monné wrote:
> On Tue, Apr 23, 2024 at 04:28:59PM +0200, Jan Beulich wrote:
>> On 23.04.2024 16:26, Roger Pau Monné wrote:
>>> On Tue, Apr 23, 2024 at 03:44:42PM +0200, Jan Beulich wrote:
On 23.04.2024 15:12, Roger Pau Monne wrote:
> Livepatch payloads containing symbols that belong to init sections can
> only
> lead to page faults later on, as by the time the livepatch is loaded init
> sections have already been freed.
>
> Refuse to resolve such symbols and return an error instead.
>
> Note such resolutions are only relevant for symbols that point to
> undefined
> sections (SHN_UNDEF), as that implies the symbol is not in the current
> payload
> and hence must either be a Xen or a different livepatch payload symbol.
>
> Do not allow to resolve symbols that point to __init_begin, as that
> address is
> also unmapped. On the other hand, __init_end is not unmapped, and hence
> allow
> resolutions against it.
>
> Since __init_begin can alias other symbols (like _erodata for example)
> allow the force flag to override the check and resolve the symbol anyway.
>
> Signed-off-by: Roger Pau Monné
In principle, as promised (and just to indicate earlier concerns were
addressed, as this is meaningless for other purposes)
Acked-by: Jan Beulich
However, ...
> @@ -310,6 +311,21 @@ int livepatch_elf_resolve_symbols(struct
> livepatch_elf *elf)
> break;
> }
> }
> +
> +/*
> + * Ensure not an init symbol. Only applicable to Xen
> symbols, as
> + * livepatch payloads don't have init sections or equivalent.
> + */
> +else if ( st_value >= (uintptr_t)&__init_begin &&
> + st_value < (uintptr_t)&__init_end && !force )
> +{
> +printk(XENLOG_ERR LIVEPATCH
> + "%s: symbol %s is in init section, not
> resolving\n",
> + elf->name, elf->sym[i].name);
> +rc = -ENXIO;
> +break;
> +}
... wouldn't it make sense to still warn in this case when "force" is set?
>>>
>>> Pondered it, I was thinking that a user would first run without
>>> --force, and use the option as a result of seeing the first failure.
>>>
>>> However if there is more than one check that's bypassed, further ones
>>> won't be noticed, so:
>>>
>>> else if ( st_value >= (uintptr_t)&__init_begin &&
>>> st_value < (uintptr_t)&__init_end )
>>> {
>>> printk(XENLOG_ERR LIVEPATCH
>>>"%s: symbol %s is in init section, not resolving\n",
>>>elf->name, elf->sym[i].name);
>>> if ( !force )
>>> {
>>> rc = -ENXIO;
>>> break;
>>> }
>>> }
>>>
>>> Would be OK then?
>>
>> Perhaps. "not resolving" isn't quite true when "force" is true, and warnings
>> would also better not be issued with XENLOG_ERR.
>
> I was assuming that printing as XENLOG_ERR level would still be OK -
> even if bypassed because of the usage of --force. The "not resolving"
> part should indeed go away. Maybe this is better:
>
> else if ( st_value >= (uintptr_t)&__init_begin &&
> st_value < (uintptr_t)&__init_end )
> {
> printk("%s" LIVEPATCH "%s: symbol %s is in init section%s\n",
>force ? XENLOG_WARNING : XENLOG_ERR,
>elf->name, elf->sym[i].name,
>force ? "" : ", not resolving");
> if ( !force )
> {
> rc = -ENXIO;
> break;
> }
> }
I'd be okay with this; the livepatch maintainers will have the ultimate say.
Jan
Re: [XEN PATCH] automation/eclair: add deviations for MISRA C:2012 Rule 16.4
On 23.04.2024 17:52, Federico Serafini wrote:
> On 23/04/24 12:26, Jan Beulich wrote:
>> On 23.04.2024 12:02, Federico Serafini wrote:
>>> --- a/docs/misra/deviations.rst
>>> +++ b/docs/misra/deviations.rst
>>> @@ -302,6 +302,19 @@ Deviations related to MISRA C:2012 Rules:
>>> leave such files as is.
>>>- Tagged as `deliberate` for ECLAIR.
>>>
>>> + * - R16.4
>>> + - Switch statements having a controlling expression of enum type
>>> + deliberately do not have a default case: gcc -Wall enables -Wswitch
>>> + which warns (and breaks the build as we use -Werror) if one of the
>>> enum
>>> + labels is missing from the switch.
>>> + - Tagged as `deliberate` for ECLAIR.
>>> +
>>> + * - R16.4
>>> + - A switch statement with a single switch clause and no default label
>>> may
>>> + be used in place of an equivalent if statement if it is considered
>>> to
>>> + improve readability."
>
> (I placed Rule 16.4 before Rule 16.3.
> I will propose a new version with the correct ordering.)
>
>>
>> First a terminology related comment here: I'm afraid "switch clause" can be
>> interpreted multiple ways, when I think we want to leave no room for
>> interpretation here. It's not even clear to me whether
>>
>> switch ( x )
>> {
>> case 1: case 2: case 3: case 4:
>> ...
>> break;
>> }
>>
>> would be covered by the deviation, or whether the multiple case labels
>> wouldn't already be too much.
>
> The MISRA C document, within Rule 16.1 ("A switch statement shall be
> well-formed") defines the syntax rules that can be used to define a
> "well formed" switch statement.
> When I say "switch clause", I refer to the same entity the MISRA
> document refers to in the definition of such syntax rules.
> In the example above, we have a single switch clause with multiple
> labels and no default label: this is a violation of Rule 16.4
> ("Every `switch' statement shall have a `default' label") which will
> be covered by the deviation.
> Do you think inserting the example in rules.rst or deviations.rst could
> be useful?
No, I don't think there should be examples in those documents. But those
documents should also not (blindly) rely on terminology in the Misra
spec, as not everyone has access to that (licensed copies had to be
obtained for quite a few of us).
Jan
Re: [PATCH v3 2/4] livepatch: introduce --force option
On Tue, Apr 23, 2024 at 03:12:47PM +0200, Roger Pau Monne wrote: > Introduce a xen-livepatch tool --force option, that's propagated into the > hyerpvisor for livepatch operations. The intention is for the option to be > used to bypass some checks that would otherwise prevent the patch from being > loaded. > > Re purpose the pad field in xen_sysctl_livepatch_op to be a flags field that > applies to all livepatch operations. The flag is currently only set by the > hypercall wrappers for the XEN_SYSCTL_LIVEPATCH_UPLOAD operation, as that's so > far the only one where it will be used initially. Other uses can be added as > required. > > Note that helpers would set the .pad field to 0, that's been removed since the > structure is already zero initialized at definition. > > No functional usages of the new flag introduced in this patch. > > Signed-off-by: Roger Pau Monné For the tools: Acked-by: Anthony PERARD Thanks, -- Anthony PERARD
Re: [XEN PATCH] automation/eclair: add deviations for MISRA C:2012 Rule 16.4
On 23/04/24 12:26, Jan Beulich wrote:
On 23.04.2024 12:02, Federico Serafini wrote:
--- a/docs/misra/deviations.rst
+++ b/docs/misra/deviations.rst
@@ -302,6 +302,19 @@ Deviations related to MISRA C:2012 Rules:
leave such files as is.
- Tagged as `deliberate` for ECLAIR.
+ * - R16.4
+ - Switch statements having a controlling expression of enum type
+ deliberately do not have a default case: gcc -Wall enables -Wswitch
+ which warns (and breaks the build as we use -Werror) if one of the enum
+ labels is missing from the switch.
+ - Tagged as `deliberate` for ECLAIR.
+
+ * - R16.4
+ - A switch statement with a single switch clause and no default label may
+ be used in place of an equivalent if statement if it is considered to
+ improve readability."
(I placed Rule 16.4 before Rule 16.3.
I will propose a new version with the correct ordering.)
First a terminology related comment here: I'm afraid "switch clause" can be
interpreted multiple ways, when I think we want to leave no room for
interpretation here. It's not even clear to me whether
switch ( x )
{
case 1: case 2: case 3: case 4:
...
break;
}
would be covered by the deviation, or whether the multiple case labels
wouldn't already be too much.
The MISRA C document, within Rule 16.1 ("A switch statement shall be
well-formed") defines the syntax rules that can be used to define a
"well formed" switch statement.
When I say "switch clause", I refer to the same entity the MISRA
document refers to in the definition of such syntax rules.
In the example above, we have a single switch clause with multiple
labels and no default label: this is a violation of Rule 16.4
("Every `switch' statement shall have a `default' label") which will
be covered by the deviation.
Do you think inserting the example in rules.rst or deviations.rst could
be useful?
And then it is not clear to me why
switch ( x )
{
case 1:
...
break;
default:
...
break;
}
shouldn't also be covered, as potentially a readability improvement /
future change simplification over
if ( x == 1 )
{
...
}
else
{
...
}
Here there are two switch clauses,
each of them terminated by a break statement,
and the default label is present:
the switch is well formed, no violations of series 16 will
be reported.
--
Federico Serafini, M.Sc.
Software Engineer, BUGSENG (http://bugseng.com)
Re: [XEN PATCH v2 5/5] xen/arm: ffa: support notification
Hi Julien,
On Mon, Apr 22, 2024 at 1:40 PM Julien Grall wrote:
>
> Hi Jens,
>
> This is not a full review of the code. I will let Bertrand doing it.
>
> On 22/04/2024 08:37, Jens Wiklander wrote:
> > +void ffa_notif_init(void)
> > +{
> > +const struct arm_smccc_1_2_regs arg = {
> > +.a0 = FFA_FEATURES,
> > +.a1 = FFA_FEATURE_SCHEDULE_RECV_INTR,
> > +};
> > +struct arm_smccc_1_2_regs resp;
> > +unsigned int irq;
> > +int ret;
> > +
> > +arm_smccc_1_2_smc(, );
> > +if ( resp.a0 != FFA_SUCCESS_32 )
> > +return;
> > +
> > +irq = resp.a2;
> > +if ( irq >= NR_GIC_SGI )
> > +irq_set_type(irq, IRQ_TYPE_EDGE_RISING);
> > +ret = request_irq(irq, 0, notif_irq_handler, "FF-A notif", NULL);
>
> If I am not mistaken, ffa_notif_init() is only called once on the boot
> CPU. However, request_irq() needs to be called on every CPU so the
> callback is registered every where and the interrupt enabled.
>
> I know the name of the function is rather confusing. So can you confirm
> this is what you expected?
Good catch, no this wasn't what I expected. I'll need to change this.
>
> [...]
>
> > diff --git a/xen/arch/arm/tee/ffa_private.h b/xen/arch/arm/tee/ffa_private.h
> > index 98236cbf14a3..ef8ffd4526bd 100644
> > --- a/xen/arch/arm/tee/ffa_private.h
> > +++ b/xen/arch/arm/tee/ffa_private.h
> > @@ -25,6 +25,7 @@
> > #define FFA_RET_DENIED -6
> > #define FFA_RET_RETRY -7
> > #define FFA_RET_ABORTED -8
> > +#define FFA_RET_NO_DATA -9
> >
> > /* FFA_VERSION helpers */
> > #define FFA_VERSION_MAJOR_SHIFT 16U
> > @@ -97,6 +98,18 @@
> >*/
> > #define FFA_MAX_SHM_COUNT 32
> >
> > +/*
> > + * TODO How to manage the available SGIs? SGI 8-15 seem to be entirely
> > + * unused, but that may change.
>
> Are the value below intended for the guests? If so, can they be moved in
> public/arch-arm.h along with the others guest interrupts?
Yes, I'll move it.
>
> > + *
> > + * SGI is the preferred delivery mechanism. SGIs 8-15 are normally not used
> > + * by a guest as they in a non-virtualized system typically are assigned to
> > + * the secure world. Here we're free to use SGI 8-15 since they are virtual
> > + * and have nothing to do with the secure world.
>
> Do you have a pointer to the specification?
There's one at the top of arch/arm/tee/ffa.c,
https://developer.arm.com/documentation/den0077/e
Do you want the link close to the defines when I've moved them to
public/arch-arm.h?
Or is it perhaps better to give a link to "Arm Base System
Architecture v1.0C", https://developer.arm.com/documentation/den0094/
instead?
Thanks,
Jens
[XEN PATCH 08/10] x86/hvm: hpet: address violations of MISRA C Rule 20.7
MISRA C Rule 20.7 states: "Expressions resulting from the expansion of macro parameters shall be enclosed in parentheses". Therefore, some macro definitions should gain additional parentheses to ensure that all current and future users will be safe with respect to expansions that can possibly alter the semantics of the passed-in macro parameter. No functional change. Signed-off-by: Nicola Vetrini --- xen/arch/x86/hvm/hpet.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/xen/arch/x86/hvm/hpet.c b/xen/arch/x86/hvm/hpet.c index 1db9c0b60ee0..5f456221cbfb 100644 --- a/xen/arch/x86/hvm/hpet.c +++ b/xen/arch/x86/hvm/hpet.c @@ -43,11 +43,11 @@ ((s_time_t)tick) > (h)->hpet_to_ns_limit) ? \ ~0ULL : (tick) * (h)->hpet_to_ns_scale) >> 10)) -#define timer_config(h, n) (h->hpet.timers[n].config) +#define timer_config(h, n) ((h)->hpet.timers[n].config) #define timer_enabled(h, n) (timer_config(h, n) & HPET_TN_ENABLE) #define timer_is_periodic(h, n) (timer_config(h, n) & HPET_TN_PERIODIC) #define timer_is_32bit(h, n) (timer_config(h, n) & HPET_TN_32BIT) -#define hpet_enabled(h) (h->hpet.config & HPET_CFG_ENABLE) +#define hpet_enabled(h) ((h)->hpet.config & HPET_CFG_ENABLE) #define timer_level(h, n)(timer_config(h, n) & HPET_TN_LEVEL) #define timer_int_route(h, n)MASK_EXTR(timer_config(h, n), HPET_TN_ROUTE) -- 2.34.1
Re: [PATCH v2 5/5] x86: Add --force option to xen-ucode to override microcode version check
On Mon, Apr 22, 2024 at 6:57 PM Anthony PERARD wrote: > > On Tue, Apr 16, 2024 at 10:15:46AM +0100, Fouad Hilly wrote: > > Introduce --force option to xen-ucode to force skipping microcode version > > check, which allows the user to update x86 microcode even if both versions > > are the same. > > > > [v2] > > 1- Changed data type from uint32_t to unsigned int. > > 2- Corrected line length. > > 3- Removed XENPF_UCODE_FLAG_FORCE_NOT_SET. > > 4- Corrected indentations. > > 5- Changed command line options to have the file name as first argument > > when applicable. > > 6- --force option doesn't require an argument anymore. > > 7- Used optint to access filename in argv. > > > > Signed-off-by: Fouad Hilly > > --- > > > > Suggested-by: Andrew Cooper > > You might want to move that tag before the '---' separation otherwise it > wont be part of the commit message. `git am` discard every thing after > the '---' line. Also add the tag before your SOB. Noted. > > > --- > > tools/include/xenctrl.h | 3 ++- > > tools/libs/ctrl/xc_misc.c | 13 +++-- > > tools/misc/xen-ucode.c| 18 +- > > 3 files changed, 26 insertions(+), 8 deletions(-) > > > > diff --git a/tools/misc/xen-ucode.c b/tools/misc/xen-ucode.c > > index e3c1943e3633..4178fd2221ea 100644 > > --- a/tools/misc/xen-ucode.c > > +++ b/tools/misc/xen-ucode.c > > @@ -24,7 +26,8 @@ static void usage(const char *name) > > "Usage: %s [microcode file] [options]\n" > > Now, that usage line is wrong. The options needs to go before the file. I am not sure what you mean "wrong"? from parsing perspective, both scenarios can be successfully executed: xen-ucode firmware-file --force xen-ucode --force firmware-file it becomes wrong if there is an argument expects the file as an input. > That could be fix on the previous patch. With that usage line, we would > want to run `./xen-ucode ucode.bin --force`, but I don't think that > would work. > > > "Options:\n" > > " -h, --helpdisplay this help and exit\n" > > - " -s, --show-cpu-info show CPU information and exit\n", > > + " -s, --show-cpu-info show CPU information and exit\n" > > + " -f, --force force to skip micorocde version > > check\n", > > name, name); > > } > > > > Thanks, > > -- > Anthony PERARD Thanks, Fouad
Re: [XEN PATCH v2 5/5] xen/arm: ffa: support notification
Hi Julien,
> On 23 Apr 2024, at 17:16, Julien Grall wrote:
>
> Hi Bertrand,
>
> On 23/04/2024 16:12, Bertrand Marquis wrote:
>> Hi Julien,
>>> On 22 Apr 2024, at 13:40, Julien Grall wrote:
>>>
>>> Hi Jens,
>>>
>>> This is not a full review of the code. I will let Bertrand doing it.
>>>
>>> On 22/04/2024 08:37, Jens Wiklander wrote:
+void ffa_notif_init(void)
+{
+const struct arm_smccc_1_2_regs arg = {
+.a0 = FFA_FEATURES,
+.a1 = FFA_FEATURE_SCHEDULE_RECV_INTR,
+};
+struct arm_smccc_1_2_regs resp;
+unsigned int irq;
+int ret;
+
+arm_smccc_1_2_smc(, );
+if ( resp.a0 != FFA_SUCCESS_32 )
+return;
+
+irq = resp.a2;
+if ( irq >= NR_GIC_SGI )
+irq_set_type(irq, IRQ_TYPE_EDGE_RISING);
+ret = request_irq(irq, 0, notif_irq_handler, "FF-A notif", NULL);
>>>
>>> If I am not mistaken, ffa_notif_init() is only called once on the boot CPU.
>>> However, request_irq() needs to be called on every CPU so the callback is
>>> registered every where and the interrupt enabled.
>>>
>>> I know the name of the function is rather confusing. So can you confirm
>>> this is what you expected?
>>>
>>> [...]
>>>
diff --git a/xen/arch/arm/tee/ffa_private.h
b/xen/arch/arm/tee/ffa_private.h
index 98236cbf14a3..ef8ffd4526bd 100644
--- a/xen/arch/arm/tee/ffa_private.h
+++ b/xen/arch/arm/tee/ffa_private.h
@@ -25,6 +25,7 @@
#define FFA_RET_DENIED -6
#define FFA_RET_RETRY -7
#define FFA_RET_ABORTED -8
+#define FFA_RET_NO_DATA -9
/* FFA_VERSION helpers */
#define FFA_VERSION_MAJOR_SHIFT 16U
@@ -97,6 +98,18 @@
*/
#define FFA_MAX_SHM_COUNT 32
+/*
+ * TODO How to manage the available SGIs? SGI 8-15 seem to be entirely
+ * unused, but that may change.
>>>
>>> Are the value below intended for the guests? If so, can they be moved in
>>> public/arch-arm.h along with the others guest interrupts?
>> The values are to be used by the guest but they will discover them through
>> the FFA_FEATURES ABI so I do not think those
>> should belong the public headers.
>
> Sorry I should have been clearer. The values in the public headers are not
> meant for the guest. They are meant for a common understanding between the
> toolstack and Xen of the guest layout (memory + interrupts).
>
> I know that some of the guests started to rely on it. But this is against our
> policy:
>
> * These are defined for consistency between the tools and the
> * hypervisor. Guests must not rely on these hardcoded values but
> * should instead use the FDT.
>
> In this case, even if this is only used by Xen (today), I would argue they
> should defined at the same place because it is easier to understand the
> layout if it is in one place.
I see, that makes sense then to add them there.
Cheers
Bertrand
>
> Cheers,
>
> --
> Julien Grall
Re: [PATCH v2 4/5] x86: Use getopt to handle command line args
On Mon, Apr 22, 2024 at 6:48 PM Anthony PERARD wrote:
>
> On Tue, Apr 16, 2024 at 10:15:45AM +0100, Fouad Hilly wrote:
> > diff --git a/tools/misc/xen-ucode.c b/tools/misc/xen-ucode.c
> > index 0c0b2337b4ea..e3c1943e3633 100644
> > --- a/tools/misc/xen-ucode.c
> > +++ b/tools/misc/xen-ucode.c
> > @@ -11,6 +11,7 @@
> > #include
> > #include
> > #include
> > +#include
> >
> > static xc_interface *xch;
> >
> > @@ -22,7 +23,8 @@ static void usage(const char *name)
> > printf("%s: Xen microcode updating tool\n"
> > "Usage: %s [microcode file] [options]\n"
> > "Options:\n"
> > - "show-cou-info show CPU information and exit\n",
> > + " -h, --helpdisplay this help and exit\n"
> > + " -s, --show-cpu-info show CPU information and exit\n",
> > name, name);
> > }
> >
> > @@ -86,6 +88,13 @@ int main(int argc, char *argv[])
> > char *filename, *buf;
> > size_t len;
> > struct stat st;
> > +int opt;
> > +
> > +static const struct option options[] = {
> > +{"help", no_argument, NULL, 'h'},
> > +{"show-cpu-info", no_argument, NULL, 's'},
> > +{NULL, no_argument, NULL, 0}
> > +};
> >
> > xch = xc_interface_open(NULL, NULL, 0);
> > if ( xch == NULL )
> > @@ -95,19 +104,22 @@ int main(int argc, char *argv[])
> > exit(1);
> > }
> >
> > -if ( argc < 2 )
> > -{
> > -fprintf(stderr,
> > -"xen-ucode: Xen microcode updating tool\n"
> > -"Usage: %s [ | show-cpu-info]\n", argv[0]);
> > -show_curr_cpu(stderr);
> > -exit(2);
> > -}
> > +if ( argc != 2 )
> > +goto ext_err;
>
> Why only two arguments allowed? And why check `argc` before calling
> getopt_long() ?
At this stage of the patch series, xen-ucode expects either firmware
file or a single argument, that is why argc should be 2.
If there is no argument or many arguments that are not supposed to be,
we exit with error other than try to parse the arguments.
>
>
> >
> > -if ( !strcmp(argv[1], "show-cpu-info") )
> > +while ( (opt = getopt_long(argc, argv, "hs", options, NULL)) != -1 )
> > {
> > -show_curr_cpu(stdout);
> > -return 0;
> > +switch (opt)
> > +{
> > +case 'h':
> > +usage(argv[0]);
> > +exit(EXIT_SUCCESS);
> > +case 's':
> > +show_curr_cpu(stdout);
> > +exit(EXIT_SUCCESS);
> > +default:
> > +goto ext_err;
> > +}
> > }
> >
> > filename = argv[1];
>
> So, after calling getopt_long(), the variable `optind` point to the next
> argument that getopt_long() didn't recognize as an argument. It would be
> a good time to start using it, and check that the are actually argument
> left on the command line, even if in the current patch the only possible
> outcome is that argv[1] has something that isn't an option.
>
> > @@ -152,4 +164,11 @@ int main(int argc, char *argv[])
> > close(fd);
> >
> > return 0;
> > +
> > + ext_err:
> > +fprintf(stderr,
> > +"xen-ucode: Xen microcode updating tool\n"
> > +"Usage: %s [microcode file] [options]\n", argv[0]);
>
> Isn't there a usage() function that we could use?
As there is an error, stderr should be used, which was a comment on V1.
>
> > +show_curr_cpu(stderr);
>
> Why call show_curr_cpu() on the error path?
Informative, but could be removed.
>
> > +exit(STDERR_FILENO);
>
> Still not an exit value.
>
> Thanks,
>
> --
> Anthony PERARD
Thanks,
Fouad
Re: [XEN PATCH v2 5/5] xen/arm: ffa: support notification
Hi Bertrand,
On 23/04/2024 16:12, Bertrand Marquis wrote:
Hi Julien,
On 22 Apr 2024, at 13:40, Julien Grall wrote:
Hi Jens,
This is not a full review of the code. I will let Bertrand doing it.
On 22/04/2024 08:37, Jens Wiklander wrote:
+void ffa_notif_init(void)
+{
+const struct arm_smccc_1_2_regs arg = {
+.a0 = FFA_FEATURES,
+.a1 = FFA_FEATURE_SCHEDULE_RECV_INTR,
+};
+struct arm_smccc_1_2_regs resp;
+unsigned int irq;
+int ret;
+
+arm_smccc_1_2_smc(, );
+if ( resp.a0 != FFA_SUCCESS_32 )
+return;
+
+irq = resp.a2;
+if ( irq >= NR_GIC_SGI )
+irq_set_type(irq, IRQ_TYPE_EDGE_RISING);
+ret = request_irq(irq, 0, notif_irq_handler, "FF-A notif", NULL);
If I am not mistaken, ffa_notif_init() is only called once on the boot CPU.
However, request_irq() needs to be called on every CPU so the callback is
registered every where and the interrupt enabled.
I know the name of the function is rather confusing. So can you confirm this is
what you expected?
[...]
diff --git a/xen/arch/arm/tee/ffa_private.h b/xen/arch/arm/tee/ffa_private.h
index 98236cbf14a3..ef8ffd4526bd 100644
--- a/xen/arch/arm/tee/ffa_private.h
+++ b/xen/arch/arm/tee/ffa_private.h
@@ -25,6 +25,7 @@
#define FFA_RET_DENIED -6
#define FFA_RET_RETRY -7
#define FFA_RET_ABORTED -8
+#define FFA_RET_NO_DATA -9
/* FFA_VERSION helpers */
#define FFA_VERSION_MAJOR_SHIFT 16U
@@ -97,6 +98,18 @@
*/
#define FFA_MAX_SHM_COUNT 32
+/*
+ * TODO How to manage the available SGIs? SGI 8-15 seem to be entirely
+ * unused, but that may change.
Are the value below intended for the guests? If so, can they be moved in
public/arch-arm.h along with the others guest interrupts?
The values are to be used by the guest but they will discover them through the
FFA_FEATURES ABI so I do not think those
should belong the public headers.
Sorry I should have been clearer. The values in the public headers are
not meant for the guest. They are meant for a common understanding
between the toolstack and Xen of the guest layout (memory + interrupts).
I know that some of the guests started to rely on it. But this is
against our policy:
* These are defined for consistency between the tools and the
* hypervisor. Guests must not rely on these hardcoded values but
* should instead use the FDT.
In this case, even if this is only used by Xen (today), I would argue
they should defined at the same place because it is easier to understand
the layout if it is in one place.
Cheers,
--
Julien Grall
[XEN PATCH] automation/eclair: reorganize pipelines
From: Simone Ballarin
Introduce accepted_guidelines.sh: a script to autogenerate the
configuration file accepted.ecl from docs/misra/rules.rst which enables
all accepted guidelines.
Introduce monitored.ecl: a manual selection of accepted guidelines
which are clean or almost clean, it is intended to be used for the
analyses triggered by commits.
Reorganize tagging.ecl:
-Remove "accepted" tags: keeping track of accepted guidelines tagging
them as "accepted" in the configuration file tagging.ecl is no
longer needed since docs/rules.rst is keeping track of them.
-Tag more guidelines as clean.
Reorganize eclair pipelines:
- Set1, Set2, Set3 are now obsolete: remove the corresponding
pipelines and ecl files.
- Amend scheduled eclair pipeline to use accepted.ecl.
- Amend triggered eclair pipeline to use monitored.ecl.
Rename and improve action_check_clean_regressions.sh to print a
diagnostic in case a commit introduces a violation of a clean guideline.
An example of diagnostic is the following:
Failure: 13 regressions found for clean guidelines
service MC3R1.R8.2: (required) Function types shall be in prototype form with
named parameters:
violation: 13
Signed-off-by: Simone Ballarin
Signed-off-by: Federico Serafini
Signed-off-by: Alessandro Zucchelli
---
automation/eclair_analysis/ECLAIR/Set2.ecl| 25 ---
automation/eclair_analysis/ECLAIR/Set3.ecl| 67 ---
.../ECLAIR/accepted_guidelines.sh | 13
.../eclair_analysis/ECLAIR/action.helpers | 3 +-
.../eclair_analysis/ECLAIR/action.settings| 1 +
.../ECLAIR/action_check_clean_regressions.sh | 38 +++
.../ECLAIR/action_clean_added.sh | 36 --
automation/eclair_analysis/ECLAIR/analyze.sh | 2 +-
.../eclair_analysis/ECLAIR/generate_ecl.sh| 4 ++
.../ECLAIR/{Set1.ecl => monitored.ecl}| 57 +++-
automation/eclair_analysis/ECLAIR/tagging.ecl | 15 +
automation/gitlab-ci/analyze.yaml | 48 ++---
automation/scripts/eclair | 8 +--
13 files changed, 108 insertions(+), 209 deletions(-)
delete mode 100644 automation/eclair_analysis/ECLAIR/Set2.ecl
delete mode 100644 automation/eclair_analysis/ECLAIR/Set3.ecl
create mode 100755 automation/eclair_analysis/ECLAIR/accepted_guidelines.sh
create mode 100755
automation/eclair_analysis/ECLAIR/action_check_clean_regressions.sh
delete mode 100755 automation/eclair_analysis/ECLAIR/action_clean_added.sh
rename automation/eclair_analysis/ECLAIR/{Set1.ecl => monitored.ecl} (67%)
diff --git a/automation/eclair_analysis/ECLAIR/Set2.ecl
b/automation/eclair_analysis/ECLAIR/Set2.ecl
deleted file mode 100644
index 7608335cf4..00
--- a/automation/eclair_analysis/ECLAIR/Set2.ecl
+++ /dev/null
@@ -1,25 +0,0 @@
--doc_begin="Set 2 of Xen MISRA C guidelines"
--enable=MC3R1.R10.1
--enable=MC3R1.R10.2
--enable=MC3R1.R10.3
--enable=MC3R1.R10.4
--enable=MC3R1.R10.6
--enable=MC3R1.R10.7
--enable=MC3R1.R10.8
--enable=MC3R1.R11.1
--enable=MC3R1.R11.2
--enable=MC3R1.R11.3
--enable=MC3R1.R11.6
--enable=MC3R1.R11.7
--enable=MC3R1.R11.8
--enable=MC3R1.R11.9
--enable=MC3R1.R12.2
--enable=MC3R1.R13.1
--enable=MC3R1.R13.2
--enable=MC3R1.R13.5
--enable=MC3R1.R13.6
--enable=MC3R1.R14.1
--enable=MC3R1.R14.2
--enable=MC3R1.R14.3
--enable=MC3R1.R14.4
--doc_end
diff --git a/automation/eclair_analysis/ECLAIR/Set3.ecl
b/automation/eclair_analysis/ECLAIR/Set3.ecl
deleted file mode 100644
index d2c2c4b21f..00
--- a/automation/eclair_analysis/ECLAIR/Set3.ecl
+++ /dev/null
@@ -1,67 +0,0 @@
--doc_begin="Set 3 of Xen MISRA C guidelines"
--enable=MC3R1.D4.12
--enable=MC3R1.R5.5
--enable=MC3R1.R5.7
--enable=MC3R1.R5.8
--enable=MC3R1.R15.2
--enable=MC3R1.R15.3
--enable=MC3R1.R15.6
--enable=MC3R1.R15.7
--enable=MC3R1.R16.1
--enable=MC3R1.R16.2
--enable=MC3R1.R16.3
--enable=MC3R1.R16.4
--enable=MC3R1.R16.5
--enable=MC3R1.R16.6
--enable=MC3R1.R16.7
--enable=MC3R1.R17.1
--enable=MC3R1.R17.2
--enable=MC3R1.R17.5
--enable=MC3R1.R17.7
--enable=MC3R1.R18.1
--enable=MC3R1.R18.2
--enable=MC3R1.R18.3
--enable=MC3R1.R18.6
--enable=MC3R1.R18.7
--enable=MC3R1.R18.8
--enable=MC3R1.R20.2
--enable=MC3R1.R20.3
--enable=MC3R1.R20.4
--enable=MC3R1.R20.6
--enable=MC3R1.R20.7
--enable=MC3R1.R20.8
--enable=MC3R1.R20.9
--enable=MC3R1.R20.11
--enable=MC3R1.R20.12
--enable=MC3R1.R20.13
--enable=MC3R1.R20.14
--enable=MC3R1.R21.1
--enable=MC3R1.R21.2
--enable=MC3R1.R21.3
--enable=MC3R1.R21.4
--enable=MC3R1.R21.5
--enable=MC3R1.R21.6
--enable=MC3R1.R21.7
--enable=MC3R1.R21.8
--enable=MC3R1.R21.9
--enable=MC3R1.R21.10
--enable=MC3R1.R21.12
--enable=MC3R1.R21.14
--enable=MC3R1.R21.15
--enable=MC3R1.R21.16
--enable=MC3R1.R22.1
--enable=MC3R1.R22.3
--enable=MC3R1.R22.7
--enable=MC3R1.R22.8
--enable=MC3R1.R22.9
--enable=MC3R1.R22.10
--enable=MC3R1.R2.6
--enable=MC3R1.R4.2
--doc_end
-
--doc_begin="Guidelines added with Xen MISRA C Task (a): Xen Coding Guidelines
v1.1, June 1, 2023"
[XEN PATCH 09/10] x86/debugreg: address violation of MISRA C Rule 20.7
MISRA C Rule 20.7 states: "Expressions resulting from the expansion
of macro parameters shall be enclosed in parentheses". Therefore, some
macro definitions should gain additional parentheses to ensure that all
current and future users will be safe with respect to expansions that
can possibly alter the semantics of the passed-in macro parameter.
No functional change.
Signed-off-by: Nicola Vetrini
---
xen/arch/x86/include/asm/debugreg.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/xen/arch/x86/include/asm/debugreg.h
b/xen/arch/x86/include/asm/debugreg.h
index 2bdaf5d9aa11..96c406ad53c8 100644
--- a/xen/arch/x86/include/asm/debugreg.h
+++ b/xen/arch/x86/include/asm/debugreg.h
@@ -67,7 +67,7 @@
#define DR_GENERAL_DETECT(0x2000UL) /* General detect enable */
#define write_debugreg(reg, val) do { \
-unsigned long __val = val; \
+unsigned long __val = (val);\
asm volatile ( "mov %0,%%db" #reg : : "r" (__val) );\
} while (0)
#define read_debugreg(reg) ({ \
--
2.34.1
[XEN PATCH 06/10] x86/pci: address violation of MISRA C Rule 20.7
MISRA C Rule 20.7 states: "Expressions resulting from the expansion
of macro parameters shall be enclosed in parentheses". Therefore, some
macro definitions should gain additional parentheses to ensure that all
current and future users will be safe with respect to expansions that
can possibly alter the semantics of the passed-in macro parameter.
No functional change.
Signed-off-by: Nicola Vetrini
---
xen/arch/x86/include/asm/pci.h | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/xen/arch/x86/include/asm/pci.h b/xen/arch/x86/include/asm/pci.h
index 6bfe87e2780b..fd5480d67d43 100644
--- a/xen/arch/x86/include/asm/pci.h
+++ b/xen/arch/x86/include/asm/pci.h
@@ -8,10 +8,10 @@
#define CF8_ADDR_HI(cf8) ( ((cf8) & 0x0f00U) >> 16)
#define CF8_ENABLED(cf8) (!!((cf8) & 0x8000U))
-#define IS_SNB_GFX(id) (id == 0x01068086 || id == 0x01168086 \
-|| id == 0x01268086 || id == 0x01028086 \
-|| id == 0x01128086 || id == 0x01228086 \
-|| id == 0x010A8086 )
+#define IS_SNB_GFX(id) ((id) == 0x01068086 || (id) == 0x01168086 \
+|| (id) == 0x01268086 || (id) == 0x01028086 \
+|| (id) == 0x01128086 || (id) == 0x01228086 \
+|| (id) == 0x010A8086 )
struct arch_pci_dev {
vmask_t used_vectors;
--
2.34.1
[XEN PATCH 04/10] drivers: char: address violation of MISRA C Rule 20.7
MISRA C Rule 20.7 states: "Expressions resulting from the expansion of macro parameters shall be enclosed in parentheses". Therefore, some macro definitions should gain additional parentheses to ensure that all current and future users will be safe with respect to expansions that can possibly alter the semantics of the passed-in macro parameter. No functional chage. Signed-off-by: Nicola Vetrini --- xen/drivers/char/omap-uart.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/xen/drivers/char/omap-uart.c b/xen/drivers/char/omap-uart.c index 03b5b66e7acb..9e1abf306ace 100644 --- a/xen/drivers/char/omap-uart.c +++ b/xen/drivers/char/omap-uart.c @@ -48,8 +48,9 @@ /* System configuration register */ #define UART_OMAP_SYSC_DEF_CONF 0x0d /* autoidle mode, wakeup is enabled */ -#define omap_read(uart, off) readl((uart)->regs + (offregs + \ + ((off) << REG_SHIFT)) static struct omap_uart { u32 baud, clock_hz, data_bits, parity, stop_bits, fifo_size; -- 2.34.1
[XEN PATCH 10/10] x86/mm: address violations of MISRA C Rule 20.7
MISRA C Rule 20.7 states: "Expressions resulting from the expansion of macro parameters shall be enclosed in parentheses". Therefore, some macro definitions should gain additional parentheses to ensure that all current and future users will be safe with respect to expansions that can possibly alter the semantics of the passed-in macro parameter. No functional change. Signed-off-by: Nicola Vetrini --- xen/arch/x86/mm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c index 9141912ae52d..87529db7d136 100644 --- a/xen/arch/x86/mm.c +++ b/xen/arch/x86/mm.c @@ -162,7 +162,7 @@ static uint32_t base_disallow_mask; #define L4_DISALLOW_MASK (base_disallow_mask) #define l1_disallow_mask(d) \ -((d != dom_io) && \ +(((d) != dom_io) && \ (rangeset_is_empty((d)->iomem_caps) && \ rangeset_is_empty((d)->arch.ioport_caps) && \ !has_arch_pdevs(d) && \ -- 2.34.1
[XEN PATCH 07/10] x86/acpi: power: address violations of MISRA Rule 20.7
MISRA C Rule 20.7 states: "Expressions resulting from the expansion
of macro parameters shall be enclosed in parentheses". Therefore, some
macro definitions should gain additional parentheses to ensure that all
current and future users will be safe with respect to expansions that
can possibly alter the semantics of the passed-in macro parameter.
No functional change.
Signed-off-by: Nicola Vetrini
---
xen/arch/x86/acpi/power.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/xen/arch/x86/acpi/power.c b/xen/arch/x86/acpi/power.c
index c6fa810a6b13..610937f42e95 100644
--- a/xen/arch/x86/acpi/power.c
+++ b/xen/arch/x86/acpi/power.c
@@ -422,12 +422,12 @@ static void tboot_sleep(u8 sleep_state)
{
uint32_t shutdown_type;
-#define TB_COPY_GAS(tbg, g) \
-tbg.space_id = g.space_id; \
-tbg.bit_width = g.bit_width;\
-tbg.bit_offset = g.bit_offset; \
-tbg.access_width = g.access_width; \
-tbg.address = g.address;
+#define TB_COPY_GAS(tbg, g) \
+(tbg).space_id = (g).space_id; \
+(tbg).bit_width = (g).bit_width;\
+(tbg).bit_offset = (g).bit_offset; \
+(tbg).access_width = (g).access_width; \
+(tbg).address = (g).address;
/* sizes are not same (due to packing) so copy each one */
TB_COPY_GAS(g_tboot_shared->acpi_sinfo.pm1a_cnt_blk,
--
2.34.1
[XEN PATCH 02/10] xen/page-defs: address violation of MISRA C Rule 20.7
MISRA C Rule 20.7 states: "Expressions resulting from the expansion of macro parameters shall be enclosed in parentheses". Therefore, some macro definitions should gain additional parentheses to ensure that all current and future users will be safe with respect to expansions that can possibly alter the semantics of the passed-in macro parameter. No functional change. Signed-off-by: Nicola Vetrini --- xen/include/xen/page-defs.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/xen/include/xen/page-defs.h b/xen/include/xen/page-defs.h index 540f8b0b6452..682da6b7b476 100644 --- a/xen/include/xen/page-defs.h +++ b/xen/include/xen/page-defs.h @@ -4,7 +4,8 @@ /* Helpers for different page granularities. */ #define PAGE_SIZE_GRAN(gran)((paddr_t)1 << PAGE_SHIFT_##gran) #define PAGE_MASK_GRAN(gran)(-PAGE_SIZE_GRAN(gran)) -#define PAGE_ALIGN_GRAN(gran, addr) ((addr + ~PAGE_MASK_##gran) & PAGE_MASK_##gran) +#define PAGE_ALIGN_GRAN(gran, addr) (((addr) + ~PAGE_MASK_##gran) & \ + PAGE_MASK_##gran) #define PAGE_SHIFT_4K 12 #define PAGE_SIZE_4KPAGE_SIZE_GRAN(4K) -- 2.34.1
[XEN PATCH 05/10] xen/spinlock: address violations of MISRA C Rule 20.7
MISRA C Rule 20.7 states: "Expressions resulting from the expansion
of macro parameters shall be enclosed in parentheses". Therefore, some
macro definitions should gain additional parentheses to ensure that all
current and future users will be safe with respect to expansions that
can possibly alter the semantics of the passed-in macro parameter.
No functional change.
Signed-off-by: Nicola Vetrini
---
xen/common/spinlock.c | 2 +-
xen/include/xen/spinlock.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/xen/common/spinlock.c b/xen/common/spinlock.c
index 5aa9ba618859..558ea7ac3518 100644
--- a/xen/common/spinlock.c
+++ b/xen/common/spinlock.c
@@ -269,7 +269,7 @@ void spin_debug_disable(void)
profile->lock_cnt++; \
}
#define LOCK_PROFILE_VAR(var, val)s_time_t var = (val)
-#define LOCK_PROFILE_BLOCK(var) var = var ? : NOW()
+#define LOCK_PROFILE_BLOCK(var) (var) = (var) ? : NOW()
#define LOCK_PROFILE_BLKACC(tst, val)\
if ( tst ) \
{\
diff --git a/xen/include/xen/spinlock.h b/xen/include/xen/spinlock.h
index 18793c5e29cb..8825affb25ca 100644
--- a/xen/include/xen/spinlock.h
+++ b/xen/include/xen/spinlock.h
@@ -141,7 +141,7 @@ struct lock_profile_qhead {
} \
prof->name = #l; \
prof->ptr.lockptr = &(s)->l; \
-prof->is_rlock = isr; \
+prof->is_rlock = (isr); \
prof->next = (s)->profile_head.elem_q;\
(s)->profile_head.elem_q = prof; \
} while( 0 )
--
2.34.1
[XEN PATCH 00/10] Address violations of MISRA C Rule 20.7
Hi all,
this series aims to refactor some macros that cause violations of MISRA C Rule
20.7 ("Expressions resulting from the expansion of macro parameters shall be
enclosed in parentheses"). All the macros touched by these patches are in some
way involved in violations, and the strategy adopted to bring them into
compliance is to add parentheses around macro arguments where needed.
Nicola Vetrini (10):
libelf: address violations of MISRA C Rule 20.7
xen/page-defs: address violation of MISRA C Rule 20.7
automation/eclair_analysis: deviate macro count_args_ for MISRA Rule
20.7
drivers: char: address violation of MISRA C Rule 20.7
xen/spinlock: address violations of MISRA C Rule 20.7
x86/pci: address violation of MISRA C Rule 20.7
x86/acpi: power: address violations of MISRA Rule 20.7
x86/hvm: hpet: address violations of MISRA C Rule 20.7
x86/debugreg: address violation of MISRA C Rule 20.7
x86/mm: address violations of MISRA C Rule 20.7
automation/eclair_analysis/ECLAIR/deviations.ecl | 6 ++
docs/misra/deviations.rst| 6 ++
xen/arch/x86/acpi/power.c| 12 ++--
xen/arch/x86/hvm/hpet.c | 4 ++--
xen/arch/x86/include/asm/debugreg.h | 2 +-
xen/arch/x86/include/asm/pci.h | 8
xen/arch/x86/mm.c| 2 +-
xen/common/libelf/libelf-private.h | 2 +-
xen/common/spinlock.c| 2 +-
xen/drivers/char/omap-uart.c | 5 +++--
xen/include/xen/libelf.h | 2 +-
xen/include/xen/page-defs.h | 3 ++-
xen/include/xen/spinlock.h | 2 +-
13 files changed, 35 insertions(+), 21 deletions(-)
--
2.34.1
[XEN PATCH 03/10] automation/eclair_analysis: deviate macro count_args_ for MISRA Rule 20.7
The count_args_ macro violates Rule 20.7, but it can't be made
compliant with Rule 20.7 without breaking its functionality. Since
it's very unlikely for this macro to be misused, it is deviated.
No functional change.
Signed-off-by: Nicola Vetrini
---
automation/eclair_analysis/ECLAIR/deviations.ecl | 6 ++
docs/misra/deviations.rst| 6 ++
2 files changed, 12 insertions(+)
diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl
b/automation/eclair_analysis/ECLAIR/deviations.ecl
index d21f112a9b94..c17e2f5a0886 100644
--- a/automation/eclair_analysis/ECLAIR/deviations.ecl
+++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
@@ -445,6 +445,12 @@ not to parenthesize their arguments."
-config=MC3R1.R20.7,reports+={safe,
"any_area(any_loc(any_exp(macro(^alternative_(v)?call[0-9]$"}
-doc_end
+-doc_begin="The argument 'x' of the count_args_ macro can't be parenthesized as
+the rule would require, without breaking the functionality of the macro. The
uses
+of this macro do not lead to developer confusion, and can thus be deviated."
+-config=MC3R1.R20.7,reports+={safe,
"any_area(any_loc(any_exp(macro(^count_args_$"}
+-doc_end
+
-doc_begin="Uses of variadic macros that have one of their arguments defined as
a macro and used within the body for both ordinary parameter expansion and as
an
operand to the # or ## operators have a behavior that is well-understood and
diff --git a/docs/misra/deviations.rst b/docs/misra/deviations.rst
index ed0c1e8ed0bf..e228ae2e715f 100644
--- a/docs/misra/deviations.rst
+++ b/docs/misra/deviations.rst
@@ -371,6 +371,12 @@ Deviations related to MISRA C:2012 Rules:
sanity checks in place.
- Tagged as `safe` for ECLAIR.
+ * - R20.7
+ - The macro `count_args_` is not compliant with the rule, but is not
likely
+ to incur in the risk of being misused or lead to developer confusion,
and
+ refactoring it to add parentheses breaks its functionality.
+ - Tagged as `safe` for ECLAIR.
+
* - R20.12
- Variadic macros that use token pasting often employ the gcc extension
`ext_paste_comma`, as detailed in `C-language-toolchain.rst`, which is
--
2.34.1
[XEN PATCH 01/10] libelf: address violations of MISRA C Rule 20.7
MISRA C Rule 20.7 states: "Expressions resulting from the expansion
of macro parameters shall be enclosed in parentheses". Therefore, some
macro definitions should gain additional parentheses to ensure that all
current and future users will be safe with respect to expansions that
can possibly alter the semantics of the passed-in macro parameter.
No functional change.
Signed-off-by: Nicola Vetrini
---
xen/common/libelf/libelf-private.h | 2 +-
xen/include/xen/libelf.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/xen/common/libelf/libelf-private.h
b/xen/common/libelf/libelf-private.h
index 98cac65bc50d..197d7a7623a3 100644
--- a/xen/common/libelf/libelf-private.h
+++ b/xen/common/libelf/libelf-private.h
@@ -26,7 +26,7 @@
/* we would like to use elf->log_callback but we can't because
* there is no vprintk in Xen */
#define elf_msg(elf, fmt, args ... ) \
- if (elf->verbose) printk(fmt, ## args )
+ if ((elf)->verbose) printk(fmt, ## args )
#define elf_err(elf, fmt, args ... ) \
printk(fmt, ## args )
diff --git a/xen/include/xen/libelf.h b/xen/include/xen/libelf.h
index 9ac530acc2a8..a0675a4dc352 100644
--- a/xen/include/xen/libelf.h
+++ b/xen/include/xen/libelf.h
@@ -288,7 +288,7 @@ bool elf_access_ok(struct elf_binary * elf,
#define elf_store_val(elf, type, ptr, val) \
({ \
typeof(type) elf_store__val = (val);\
-elf_ptrval elf_store__targ = ptr; \
+elf_ptrval elf_store__targ = (ptr); \
if (elf_access_ok((elf), elf_store__targ, \
sizeof(elf_store__val))) { \
elf_memcpy_unchecked((void*)elf_store__targ, _store__val, \
--
2.34.1
Re: [XEN PATCH v2 5/5] xen/arm: ffa: support notification
Hi Julien,
> On 22 Apr 2024, at 13:40, Julien Grall wrote:
>
> Hi Jens,
>
> This is not a full review of the code. I will let Bertrand doing it.
>
> On 22/04/2024 08:37, Jens Wiklander wrote:
>> +void ffa_notif_init(void)
>> +{
>> +const struct arm_smccc_1_2_regs arg = {
>> +.a0 = FFA_FEATURES,
>> +.a1 = FFA_FEATURE_SCHEDULE_RECV_INTR,
>> +};
>> +struct arm_smccc_1_2_regs resp;
>> +unsigned int irq;
>> +int ret;
>> +
>> +arm_smccc_1_2_smc(, );
>> +if ( resp.a0 != FFA_SUCCESS_32 )
>> +return;
>> +
>> +irq = resp.a2;
>> +if ( irq >= NR_GIC_SGI )
>> +irq_set_type(irq, IRQ_TYPE_EDGE_RISING);
>> +ret = request_irq(irq, 0, notif_irq_handler, "FF-A notif", NULL);
>
> If I am not mistaken, ffa_notif_init() is only called once on the boot CPU.
> However, request_irq() needs to be called on every CPU so the callback is
> registered every where and the interrupt enabled.
>
> I know the name of the function is rather confusing. So can you confirm this
> is what you expected?
>
> [...]
>
>> diff --git a/xen/arch/arm/tee/ffa_private.h b/xen/arch/arm/tee/ffa_private.h
>> index 98236cbf14a3..ef8ffd4526bd 100644
>> --- a/xen/arch/arm/tee/ffa_private.h
>> +++ b/xen/arch/arm/tee/ffa_private.h
>> @@ -25,6 +25,7 @@
>> #define FFA_RET_DENIED -6
>> #define FFA_RET_RETRY -7
>> #define FFA_RET_ABORTED -8
>> +#define FFA_RET_NO_DATA -9
>>/* FFA_VERSION helpers */
>> #define FFA_VERSION_MAJOR_SHIFT 16U
>> @@ -97,6 +98,18 @@
>> */
>> #define FFA_MAX_SHM_COUNT 32
>> +/*
>> + * TODO How to manage the available SGIs? SGI 8-15 seem to be entirely
>> + * unused, but that may change.
>
> Are the value below intended for the guests? If so, can they be moved in
> public/arch-arm.h along with the others guest interrupts?
The values are to be used by the guest but they will discover them through the
FFA_FEATURES ABI so I do not think those
should belong the public headers.
Cheers
Bertrand
>
>> + *
>> + * SGI is the preferred delivery mechanism. SGIs 8-15 are normally not used
>> + * by a guest as they in a non-virtualized system typically are assigned to
>> + * the secure world. Here we're free to use SGI 8-15 since they are virtual
>> + * and have nothing to do with the secure world.
>
> Do you have a pointer to the specification?
>
> [...]
>
> Cheers,
>
> --
> Julien Grall
Re: [PATCH v3 3/4] livepatch: refuse to resolve symbols that belong to init sections
On Tue, Apr 23, 2024 at 04:28:59PM +0200, Jan Beulich wrote:
> On 23.04.2024 16:26, Roger Pau Monné wrote:
> > On Tue, Apr 23, 2024 at 03:44:42PM +0200, Jan Beulich wrote:
> >> On 23.04.2024 15:12, Roger Pau Monne wrote:
> >>> Livepatch payloads containing symbols that belong to init sections can
> >>> only
> >>> lead to page faults later on, as by the time the livepatch is loaded init
> >>> sections have already been freed.
> >>>
> >>> Refuse to resolve such symbols and return an error instead.
> >>>
> >>> Note such resolutions are only relevant for symbols that point to
> >>> undefined
> >>> sections (SHN_UNDEF), as that implies the symbol is not in the current
> >>> payload
> >>> and hence must either be a Xen or a different livepatch payload symbol.
> >>>
> >>> Do not allow to resolve symbols that point to __init_begin, as that
> >>> address is
> >>> also unmapped. On the other hand, __init_end is not unmapped, and hence
> >>> allow
> >>> resolutions against it.
> >>>
> >>> Since __init_begin can alias other symbols (like _erodata for example)
> >>> allow the force flag to override the check and resolve the symbol anyway.
> >>>
> >>> Signed-off-by: Roger Pau Monné
> >>
> >> In principle, as promised (and just to indicate earlier concerns were
> >> addressed, as this is meaningless for other purposes)
> >> Acked-by: Jan Beulich
> >> However, ...
> >>
> >>> @@ -310,6 +311,21 @@ int livepatch_elf_resolve_symbols(struct
> >>> livepatch_elf *elf)
> >>> break;
> >>> }
> >>> }
> >>> +
> >>> +/*
> >>> + * Ensure not an init symbol. Only applicable to Xen
> >>> symbols, as
> >>> + * livepatch payloads don't have init sections or equivalent.
> >>> + */
> >>> +else if ( st_value >= (uintptr_t)&__init_begin &&
> >>> + st_value < (uintptr_t)&__init_end && !force )
> >>> +{
> >>> +printk(XENLOG_ERR LIVEPATCH
> >>> + "%s: symbol %s is in init section, not
> >>> resolving\n",
> >>> + elf->name, elf->sym[i].name);
> >>> +rc = -ENXIO;
> >>> +break;
> >>> +}
> >>
> >> ... wouldn't it make sense to still warn in this case when "force" is set?
> >
> > Pondered it, I was thinking that a user would first run without
> > --force, and use the option as a result of seeing the first failure.
> >
> > However if there is more than one check that's bypassed, further ones
> > won't be noticed, so:
> >
> > else if ( st_value >= (uintptr_t)&__init_begin &&
> > st_value < (uintptr_t)&__init_end )
> > {
> > printk(XENLOG_ERR LIVEPATCH
> >"%s: symbol %s is in init section, not resolving\n",
> >elf->name, elf->sym[i].name);
> > if ( !force )
> > {
> > rc = -ENXIO;
> > break;
> > }
> > }
> >
> > Would be OK then?
>
> Perhaps. "not resolving" isn't quite true when "force" is true, and warnings
> would also better not be issued with XENLOG_ERR.
I was assuming that printing as XENLOG_ERR level would still be OK -
even if bypassed because of the usage of --force. The "not resolving"
part should indeed go away. Maybe this is better:
else if ( st_value >= (uintptr_t)&__init_begin &&
st_value < (uintptr_t)&__init_end )
{
printk("%s" LIVEPATCH "%s: symbol %s is in init section%s\n",
force ? XENLOG_WARNING : XENLOG_ERR,
elf->name, elf->sym[i].name,
force ? "" : ", not resolving");
if ( !force )
{
rc = -ENXIO;
break;
}
}
Thanks, Roger.
Re: [PATCH 4/6] x86/alternative: Replace a continue with a goto
On 22.04.2024 20:14, Andrew Cooper wrote: > A subsequent patch is going to insert a loop, which interferes with the > continue in the devirtualisation logic. > > Replace it with a goto, and a paragraph explaining why we intentionally avoid > setting a->priv = 1. > > No functional change. > > Signed-off-by: Andrew Cooper Acked-by: Jan Beulich
Re: [PATCH 3/6] x86/alternative: Intend the relocation logic
On 22.04.2024 20:14, Andrew Cooper wrote:
> ... to make subsequent patches legible.
>
> No functional change.
>
> Signed-off-by: Andrew Cooper
Acked-by: Jan Beulich
even if (perhaps due to changes in the "real" patch) some of this re-
indenting wants doing differently, just with ...
> --- a/xen/arch/x86/alternative.c
> +++ b/xen/arch/x86/alternative.c
> @@ -244,78 +244,80 @@ static void init_or_livepatch
> _apply_alternatives(struct alt_instr *start,
>
> memcpy(buf, repl, a->repl_len);
>
> -/* 0xe8/0xe9 are relative branches; fix the offset. */
> -if ( a->repl_len >= 5 && (*buf & 0xfe) == 0xe8 )
> {
> -/*
> - * Detect the special case of indirect-to-direct branch patching:
> - * - replacement is a direct CALL/JMP (opcodes 0xE8/0xE9; already
> - * checked above),
> - * - replacement's displacement is -5 (pointing back at the very
> - * insn, which makes no sense in a real replacement insn),
> - * - original is an indirect CALL/JMP (opcodes 0xFF/2 or 0xFF/4)
> - * using RIP-relative addressing.
> - * Some branch destinations may still be NULL when we come here
> - * the first time. Defer patching of those until the post-presmp-
> - * initcalls re-invocation (with force set to true). If at that
> - * point the branch destination is still NULL, insert "UD2; UD0"
> - * (for ease of recognition) instead of CALL/JMP.
> - */
> -if ( a->cpuid == X86_FEATURE_ALWAYS &&
> - *(int32_t *)(buf + 1) == -5 &&
> - a->orig_len >= 6 &&
> - orig[0] == 0xff &&
> - orig[1] == (*buf & 1 ? 0x25 : 0x15) )
> +/* 0xe8/0xe9 are relative branches; fix the offset. */
> +if ( a->repl_len >= 5 && (*buf & 0xfe) == 0xe8 )
> {
> -long disp = *(int32_t *)(orig + 2);
> -const uint8_t *dest = *(void **)(orig + 6 + disp);
> -
> -if ( dest )
> +/*
> + * Detect the special case of indirect-to-direct branch
> patching:
> + * - replacement is a direct CALL/JMP (opcodes 0xE8/0xE9;
> already
> + * checked above),
> + * - replacement's displacement is -5 (pointing back at the
> very
> + * insn, which makes no sense in a real replacement insn),
> + * - original is an indirect CALL/JMP (opcodes 0xFF/2 or
> 0xFF/4)
> + * using RIP-relative addressing.
> + * Some branch destinations may still be NULL when we come
> here
> + * the first time. Defer patching of those until the
> post-presmp-
> + * initcalls re-invocation (with force set to true). If at
> that
> + * point the branch destination is still NULL, insert "UD2;
> UD0"
> + * (for ease of recognition) instead of CALL/JMP.
> + */
> +if ( a->cpuid == X86_FEATURE_ALWAYS &&
> + *(int32_t *)(buf + 1) == -5 &&
> + a->orig_len >= 6 &&
> + orig[0] == 0xff &&
> + orig[1] == (*buf & 1 ? 0x25 : 0x15) )
> {
> -/*
> - * When building for CET-IBT, all function pointer
> targets
> - * should have an endbr64 instruction.
> - *
> - * If this is not the case, leave a warning because
> - * something is probably wrong with the build. A CET-IBT
> - * enabled system might have exploded already.
> - *
> - * Otherwise, skip the endbr64 instruction. This is a
> - * marginal perf improvement which saves on instruction
> - * decode bandwidth.
> - */
> -if ( IS_ENABLED(CONFIG_XEN_IBT) )
> +long disp = *(int32_t *)(orig + 2);
> +const uint8_t *dest = *(void **)(orig + 6 + disp);
> +
> +if ( dest )
> {
> -if ( is_endbr64(dest) )
> -dest += ENDBR64_LEN;
> -else
> -printk(XENLOG_WARNING
> - "altcall %ps dest %ps has no endbr64\n",
> - orig, dest);
> +/*
> + * When building for CET-IBT, all function pointer
> targets
> + * should have an endbr64 instruction.
> + *
> + * If this is not the case, leave a warning because
> + * something is probably wrong with the build.
Re: [PATCH v2 1/5] x86: Update x86 low level version check of microcode
On Thu, Apr 18, 2024 at 11:05 AM Andrew Cooper wrote: > > On 16/04/2024 10:15 am, Fouad Hilly wrote: > > Update microcode version check at Intel and AMD Level by: > > Preventing the low level code from sending errors if the microcode > > version provided is not a newer version. Other errors will be sent like > > before. > > When the provided microcode version is the same as the current one, code > > to point to microcode provided. > > Microcode version check happens at higher and common level in core.c. > > Keep all the required code at low level that checks for signature and CPU > > compatibility > > > > [v2] > > Update message description to better describe the changes > > > > Signed-off-by: Fouad Hilly > > --- > > > As a general note, your v2/v3/etc changelog needs to go under this --- line. Noted. > > ~Andrew Thanks, Fouad
Re: [PATCH v2 1/5] x86: Update x86 low level version check of microcode
On Mon, Apr 22, 2024 at 3:09 PM Jan Beulich wrote:
>
> On 16.04.2024 11:15, Fouad Hilly wrote:
> > Update microcode version check at Intel and AMD Level by:
> > Preventing the low level code from sending errors if the microcode
> > version provided is not a newer version.
>
> And why is this change (a) wanted and (b) correct?
I will improve the message description to cover more details and reasoning.
>
> > Other errors will be sent like before.
> > When the provided microcode version is the same as the current one, code
> > to point to microcode provided.
>
> I'm afraid I can't interpret this sentence.
"provided" is the firmware presented\provided to the code for firmware
flashing. As above, I will provide more comprehensive description.
>
> > Microcode version check happens at higher and common level in core.c.
> > Keep all the required code at low level that checks for signature and CPU
> > compatibility
> >
> > [v2]
> > Update message description to better describe the changes
>
> This belongs ...
>
> > Signed-off-by: Fouad Hilly
> > ---
>
> ... below the separator.
>
> > --- a/xen/arch/x86/cpu/microcode/amd.c
> > +++ b/xen/arch/x86/cpu/microcode/amd.c
> > @@ -383,12 +383,8 @@ static struct microcode_patch *cf_check
> > cpu_request_microcode(
> > goto skip;
> > }
> >
> > -/*
> > - * If the new ucode covers current CPU, compare ucodes and
> > store the
> > - * one with higher revision.
> > - */
> > -if ( (microcode_fits(mc->patch) != MIS_UCODE) &&
> > - (!saved || (compare_header(mc->patch, saved) ==
> > NEW_UCODE)) )
> > +/* If the provided ucode covers current CPU, then store its
> > revision. */
> > +if ( (microcode_fits(mc->patch) != MIS_UCODE) && !saved )
> > {
> > saved = mc->patch;
> > saved_size = mc->len;
> > --- a/xen/arch/x86/cpu/microcode/intel.c
> > +++ b/xen/arch/x86/cpu/microcode/intel.c
> > @@ -294,8 +294,7 @@ static int cf_check apply_microcode(const struct
> > microcode_patch *patch)
> >
> > result = microcode_update_match(patch);
> >
> > -if ( result != NEW_UCODE &&
> > - !(opt_ucode_allow_same && result == SAME_UCODE) )
> > +if ( result != NEW_UCODE && result != SAME_UCODE )
> > return -EINVAL;
>
> Unlike the other two adjustments this one results in still permitting
> only same-or-newer. How does this fit with the AMD change above and
> the other Intel change ...
To be fixed in V3
>
> > @@ -354,12 +353,8 @@ static struct microcode_patch *cf_check
> > cpu_request_microcode(
> > if ( error )
> > break;
> >
> > -/*
> > - * If the new update covers current CPU, compare updates and store
> > the
> > - * one with higher revision.
> > - */
> > -if ( (microcode_update_match(mc) != MIS_UCODE) &&
> > - (!saved || compare_revisions(saved->rev, mc->rev) ==
> > NEW_UCODE) )
> > +/* If the provided ucode covers current CPU, then store its
> > revision. */
> > +if ( (microcode_update_match(mc) != MIS_UCODE) && !saved )
> > saved = mc;
>
> ... here?
I assume this refers to the previous comment? Which will be fixed in V3
>
> Jan
Thanks,
Fouad
Re: [PATCH 5/6] x86/alternative: Relocate all insn-relative fields
On 22.04.2024 20:14, Andrew Cooper wrote:
> --- a/xen/arch/x86/alternative.c
> +++ b/xen/arch/x86/alternative.c
> @@ -244,10 +244,31 @@ static void init_or_livepatch
> _apply_alternatives(struct alt_instr *start,
>
> memcpy(buf, repl, a->repl_len);
>
> +/* Walk buf[] and adjust any insn-relative operands. */
> +if ( a->repl_len )
> {
> -/* 0xe8/0xe9 are relative branches; fix the offset. */
> -if ( a->repl_len >= 5 && (*buf & 0xfe) == 0xe8 )
> +uint8_t *ip = buf, *end = ip + a->repl_len;
> +
> +for ( x86_decode_lite_t res; ip < end; ip += res.len )
> {
> +int32_t *d32;
> +uint8_t *target;
> +
> +res = x86_decode_lite(ip, end);
> +
> +if ( res.len <= 0 )
> +{
> +printk("Alternative for %ps [%*ph]\n",
> + ALT_ORIG_PTR(a), a->repl_len, repl);
> +printk("Unable to decode instruction in alternative -
> ignoring.\n");
> +goto skip_this_alternative;
Can this really be just a log message? There are cases where patching has
to happen for things to operate correctly. Hence if not panic()ing, I'd
say we at least want to taint the hypervisor.
> @@ -317,14 +338,23 @@ static void init_or_livepatch
> _apply_alternatives(struct alt_instr *start,
> */
> goto skip_this_alternative;
> }
> +
> +continue;
> }
> -else if ( force && system_state < SYS_STATE_active )
> -ASSERT_UNREACHABLE();
This (and the other one below) is related to altcall patching, which you
say you mean to leave alone: During the 2nd pass, no un-processed CALL /
JMP should occur anymore that aren't altcall related.
Jan
Re: [XEN PATCH v2 4/5] xen/arm: allow dynamically assigned SGI handlers
On Tue, Apr 23, 2024 at 1:05 PM Julien Grall wrote:
>
>
>
> On 23/04/2024 10:35, Jens Wiklander wrote:
> > Hi Julien,
>
> Hi Jens,
>
> > On Mon, Apr 22, 2024 at 12:57 PM Julien Grall wrote:
> >>
> >> Hi Jens,
> >>
> >> On 22/04/2024 08:37, Jens Wiklander wrote:
> >>> Updates so request_irq() can be used with a dynamically assigned SGI irq
> >>> as input. This prepares for a later patch where an FF-A schedule
> >>> receiver interrupt handler is installed for an SGI generated by the
> >>> secure world.
> >>
> >> I would like to understand the use-case a bit more. Who is responsible
> >> to decide the SGI number? Is it Xen or the firmware?
> >>
> >> If the later, how can we ever guarantee the ID is not going to clash
> >> with what the OS/hypervisor is using? Is it described in a
> >> specification? If so, please give a pointer.
> >
> > The firmware decides the SGI number. Given that the firmware doesn't
> > know which SGIs Xen is using it typically needs to donate one of the
> > secure SGIs, but that is transparent to Xen.
>
> Right this is my concern. The firmware decides the number, but at the
> same time Xen thinks that all the SGIs are available (AFAIK there is
> only one set).
>
> What I would like to see is some wording from a spec indicating that the
> SGIs ID reserved by the firmware will not be clashing with the one used
> by Xen.
>
> >
> >
> >>
> >>>
> >>> gic_route_irq_to_xen() don't gic_set_irq_type() for SGIs since they are
> >>> always edge triggered.
> >>>
> >>> gic_interrupt() is updated to route the dynamically assigned SGIs to
> >>> do_IRQ() instead of do_sgi(). The latter still handles the statically
> >>> assigned SGI handlers like for instance GIC_SGI_CALL_FUNCTION.
> >>>
> >>> Signed-off-by: Jens Wiklander
> >>> ---
> >>> v1->v2
> >>> - Update patch description as requested
> >>> ---
> >>>xen/arch/arm/gic.c | 5 +++--
> >>>xen/arch/arm/irq.c | 7 +--
> >>
> >> I am not sure where to write the comment. But I think the comment on top
> >> of irq_set_affinity() in setup_irq() should also be updated.
> >>
> >>>2 files changed, 8 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> >>> index 44c40e86defe..e9aeb7138455 100644
> >>> --- a/xen/arch/arm/gic.c
> >>> +++ b/xen/arch/arm/gic.c
> >>> @@ -117,7 +117,8 @@ void gic_route_irq_to_xen(struct irq_desc *desc,
> >>> unsigned int priority)
> >>>
> >>>desc->handler = gic_hw_ops->gic_host_irq_type;
> >>>
> >>> -gic_set_irq_type(desc, desc->arch.type);
> >>> +if ( desc->irq >= NR_GIC_SGI)
> >>> +gic_set_irq_type(desc, desc->arch.type);
> >>
> >> So above, you say that the SGIs are always edge-triggered interrupt. So
> >> I assume desc->arch.type. So are you skipping the call because it is
> >> unnessary or it could do the wrong thing?
> >>
> >> Ideally, the outcome of the answer be part of the comment on top of the
> >> check.
> >
> > gic_set_irq_type() has an assert "ASSERT(type != IRQ_TYPE_INVALID)"
> > which is triggered without this check.
> > So it's both unnecessary and wrong. I suppose we could update the
> > bookkeeping of all SGIs to be edge-triggered instead of
> > IRQ_TYPE_INVALID. It would still be unnecessary though. What do you
> > suggest?
>
> I would rather prefer if we update the book-keeping for all the SGIs.
I'll update the code.
>
> [...]
>
> >>
> >>>{
> >>>isb();
> >>>do_IRQ(regs, irq, is_fiq);
> >>> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> >>> index bcce80a4d624..fdb214560978 100644
> >>> --- a/xen/arch/arm/irq.c
> >>> +++ b/xen/arch/arm/irq.c
> >>> @@ -224,9 +224,12 @@ void do_IRQ(struct cpu_user_regs *regs, unsigned int
> >>> irq, int is_fiq)
> >>>
> >>>perfc_incr(irqs);
> >>>
> >>> -ASSERT(irq >= 16); /* SGIs do not come down this path */
> >>> +/* Statically assigned SGIs do not come down this path */
> >>> +ASSERT(irq >= GIC_SGI_MAX);
> >>
> >>
> >> With this change, I think the path with vgic_inject_irq() now needs to
> >> gain an ASSERT(irq >= NR_GIC_SGI) because the path is not supposed to be
> >> taken for SGIs.
> >
> > I'm sorry, I don't see the connection. If I add
> > ASSERT(virq >= NR_GIC_SGI);
> > at the top of vgic_inject_irq() it will panic when injecting a
> > Schedule Receiver or Notification Pending Interrupt for a guest.
>
> If you look at do_IRQ(), we have the following code:
>
> if ( test_bit(_IRQ_GUEST, >status) )
> {
> struct irq_guest *info = irq_get_guest_info(desc);
>
> perfc_incr(guest_irqs);
> desc->handler->end(desc);
>
> set_bit(_IRQ_INPROGRESS, >status);
>
> /*
> * The irq cannot be a PPI, we only support delivery of SPIs to
> * guests.
> */
> vgic_inject_irq(info->d, NULL, info->virq, true);
> goto out_no_end;
> }
>
> What I suggesting is to add an ASSERT(irq >= NR_GIC_SGI) just before the
> call because now do_IRQ() can
Re: [PATCH 2/5] x86: Refactor microcode_update() hypercall with flags field
On Mon, Apr 8, 2024 at 10:16 AM Jan Beulich wrote:
>
> On 05.04.2024 14:11, Fouad Hilly wrote:
> > @@ -708,11 +712,13 @@ static long cf_check microcode_update_helper(void
> > *data)
> > return ret;
> > }
> >
> > -int microcode_update(XEN_GUEST_HANDLE(const_void) buf, unsigned long len)
> > +int microcode_update(XEN_GUEST_HANDLE(const_void) buf, unsigned long len,
> > unsigned int flags)
> > {
> > int ret;
> > struct ucode_buf *buffer;
> >
> > +ucode_force_flag = (flags == XENPF_UCODE_FLAG_FORCE_SET)? 1: 0;
>
> No need for ?: when the lhs has type bool.
>
> But - do we really need to resort to parameter passing via static variables
> here? If it's unavoidable, its setting needs to move inside a locked region
> (with that region covering everything up to all consumption of the value).
There are many function calls and checks of the firmware between
microcode_update() and the actual update, which makes static variable
the viable option.
In V2 I broke it down between the actual update_flags (static) and
force_flag (local to firmware update function), I understand that
might not be enough, I will look into further improvement for
microcode_update flags in V3.
>
> Further, to avoid the same issue again when another flag wants adding, you
> want to check that all other bits in the flags field are clear.
The above check is checking all bits in the flags field. Are you
referring to flag per bit where multiple flags can be set
simultaneously?
>
> > --- a/xen/arch/x86/include/asm/microcode.h
> > +++ b/xen/arch/x86/include/asm/microcode.h
> > @@ -22,7 +22,7 @@ struct cpu_signature {
> > DECLARE_PER_CPU(struct cpu_signature, cpu_sig);
> >
> > void microcode_set_module(unsigned int idx);
> > -int microcode_update(XEN_GUEST_HANDLE(const_void) buf, unsigned long len);
> > +int microcode_update(XEN_GUEST_HANDLE(const_void) buf, unsigned long len,
> > unsigned int flags);
>
> Nit: Too long line.
>
> > --- a/xen/include/public/platform.h
> > +++ b/xen/include/public/platform.h
> > @@ -99,6 +99,10 @@ struct xenpf_microcode_update {
> > /* IN variables. */
> > XEN_GUEST_HANDLE(const_void) data;/* Pointer to microcode data */
> > uint32_t length; /* Length of microcode data. */
> > +uint32_t flags; /* Flags to be passed with ucode. */
> > +/* Force to skip microcode version check when set */
> > +#define XENPF_UCODE_FLAG_FORCE_NOT_SET 0
> > +#define XENPF_UCODE_FLAG_FORCE_SET 1
> > };
>
> The safety of this growing of an existing stable ABI struct wants at least
> briefly mentioning in the description.
>
> > @@ -624,6 +628,10 @@ struct xenpf_ucode_revision {
> > typedef struct xenpf_ucode_revision xenpf_ucode_revision_t;
> > DEFINE_XEN_GUEST_HANDLE(xenpf_ucode_revision_t);
> >
> > +/* Hypercall to microcode_update with flags */
> > +#define XENPF_microcode_update266
> > +
> > +
>
> No double blank lines please.
>
> Jan
Thanks,
Fouad
Re: [XEN PATCH v2 4/5] xen/arm: allow dynamically assigned SGI handlers
Hi, On Tue, Apr 23, 2024 at 4:28 PM Julien Grall wrote: > > Hi Bertrand, > > On 23/04/2024 14:23, Bertrand Marquis wrote: > > Hi Julien, > > > >> On 23 Apr 2024, at 14:37, Bertrand Marquis > >> wrote: > >> > >> Hi Julien, > >> > >>> On 23 Apr 2024, at 13:05, Julien Grall wrote: > >>> > >>> > >>> > >>> On 23/04/2024 10:35, Jens Wiklander wrote: > Hi Julien, > >>> > >>> Hi Jens, > >>> > On Mon, Apr 22, 2024 at 12:57 PM Julien Grall wrote: > > > > Hi Jens, > > > > On 22/04/2024 08:37, Jens Wiklander wrote: > >> Updates so request_irq() can be used with a dynamically assigned SGI > >> irq > >> as input. This prepares for a later patch where an FF-A schedule > >> receiver interrupt handler is installed for an SGI generated by the > >> secure world. > > > > I would like to understand the use-case a bit more. Who is responsible > > to decide the SGI number? Is it Xen or the firmware? > > > > If the later, how can we ever guarantee the ID is not going to clash > > with what the OS/hypervisor is using? Is it described in a > > specification? If so, please give a pointer. > The firmware decides the SGI number. Given that the firmware doesn't > know which SGIs Xen is using it typically needs to donate one of the > secure SGIs, but that is transparent to Xen. > >>> > >>> Right this is my concern. The firmware decides the number, but at the > >>> same time Xen thinks that all the SGIs are available (AFAIK there is only > >>> one set). > >>> > >>> What I would like to see is some wording from a spec indicating that the > >>> SGIs ID reserved by the firmware will not be clashing with the one used > >>> by Xen. > >> > >> The idea is that the only SGI reserved for secure are used by the secure > >> world (in fact it is the SPMC in the secure world who tells us which SGI > >> it will generate). > >> So in theory that means it will always use an SGI between 8 and 15. > >> > >> Now it could make sense in fact to check that the number returned by the > >> firmware (or SPMC) is not clashing with Xen as it is a recommendation in > >> the spec and > >> in fact an implementation might do something different. > >> > >> Right now there is no spec that will say that it will never clash with the > >> one used by Xen as the FF-A spec is not enforcing anything here so it > >> would be a good idea > >> to check and disable FF-A with a proper error message if this happens. > > > > > > After some more digging here is what is recommended by Arm in the Arm Base > > System Architecture v1.0C [1]: > > > > "The system shall implement at least eight Non-secure SGIs, assigned to > > interrupt IDs 0-7." > > Thanks! Can we provide a link to the specification in the commit message? Sure, I'll add a link. > > > > > So basically as long as Xen is using SGIs 0-7 it is safe as those shall > > never be used by the secure world. > > Now i do agree that we should check that whatever is returned by the > > firmware is not conflicting with what > > is used by Xen. > +1. That makes sense, I'll add a check. Thanks, Jens
Re: [PATCH io_uring-next/net-next v2 1/4] net: extend ubuf_info callback to ops structure
Pavel Begunkov wrote: > We'll need to associate additional callbacks with ubuf_info, introduce > a structure holding ubuf_info callbacks. Apart from a more smarter > io_uring notification management introduced in next patches, it can be > used to generalise msg_zerocopy_put_abort() and also store > ->sg_from_iter, which is currently passed in struct msghdr. > > Reviewed-by: Jens Axboe > Reviewed-by: David Ahern > Signed-off-by: Pavel Begunkov Reviewed-by: Willem de Bruijn
Re: [PATCH io_uring-next/net-next v2 2/4] net: add callback for setting a ubuf_info to skb
Pavel Begunkov wrote: > At the moment an skb can only have one ubuf_info associated with it, > which might be a performance problem for zerocopy sends in cases like > TCP via io_uring. Add a callback for assigning ubuf_info to skb, this > way we will implement smarter assignment later like linking ubuf_info > together. > > Note, it's an optional callback, which should be compatible with > skb_zcopy_set(), that's because the net stack might potentially decide > to clone an skb and take another reference to ubuf_info whenever it > wishes. Also, a correct implementation should always be able to bind to > an skb without prior ubuf_info, otherwise we could end up in a situation > when the send would not be able to progress. > > Reviewed-by: Jens Axboe > Reviewed-by: David Ahern > Signed-off-by: Pavel Begunkov Reviewed-by: Willem de Bruijn
Re: [PATCH 2/6] x86/alternative: Walk all replacements in debug builds
On 22.04.2024 20:14, Andrew Cooper wrote:
> In debug builds, walk all alternative replacements with x86_decode_lite().
>
> This checks that we can decode all instructions, and also lets us check that
> disp8's don't leave the replacement block.
>
> Signed-off-by: Andrew Cooper
With pointed-to types consistently constified, technically:
Reviewed-by: Jan Beulich
One further suggestion and a question, though:
> @@ -464,6 +465,54 @@ static void __init _alternative_instructions(bool force)
> void __init alternative_instructions(void)
> {
> arch_init_ideal_nops();
> +
> +/*
> + * Walk all replacement instructions with x86_decode_lite(). This checks
> + * both that we can decode all instructions within the replacement, and
> + * that any near branch with a disp8 stays within the alternative itself.
> + */
> +if ( IS_ENABLED(CONFIG_DEBUG) )
> +{
> +struct alt_instr *a;
> +
> +for ( a = __alt_instructions;
> + a < __alt_instructions_end; ++a )
> +{
> +void *repl = ALT_REPL_PTR(a);
> +void *ip = repl, *end = ip + a->repl_len;
> +
> +if ( !a->repl_len )
> +continue;
> +
> +for ( x86_decode_lite_t res; ip < end; ip += res.len )
> +{
> +res = x86_decode_lite(ip, end);
> +
> +if ( res.len <= 0 )
> +{
> +printk("Alternative for %ps [%*ph]\n",
> + ALT_ORIG_PTR(a), a->repl_len, repl);
> +panic("Unable to decode instruction at +%u in
> alternative\n",
> + (unsigned int)(unsigned long)(ip - repl));
Instead of the double cast, maybe better use +%lu? And really we ought to
have support for %tu, allowing the other cast t be dropped here, too.
> +}
> +
> +if ( res.rel_type == REL_TYPE_d8 )
> +{
> +int8_t *d8 = res.rel;
> +void *target = ip + res.len + *d8;
> +
> +if ( target < repl || target > end )
> +{
> +printk("Alternative for %ps [%*ph]\n",
> + ALT_ORIG_PTR(a), a->repl_len, repl);
> +panic("'JMP/Jcc disp8' at +%u leaves alternative
> block\n",
> + (unsigned int)(unsigned long)(ip - repl));
> +}
> +}
Why's Disp8 more important to check than Disp32? A bad CALL in a
replacement can't possibly be encoded with Disp8, and both JMP and Jcc
are also more likely to be encoded with Disp32 when their target isn't
in the same blob (but e.g. in a different section).
Jan
Re: [PATCH v2 2/5] x86: Refactor microcode_update() hypercall with flags
On Mon, Apr 22, 2024 at 3:18 PM Jan Beulich wrote: > > On 16.04.2024 11:15, Fouad Hilly wrote: > > Refactor microcode_update() hypercall by adding flags field. > > Introduce XENPF_microcode_update2 hypercall to handle flags field. > > struct xenpf_microcode_update updated to have uint32_t flags at > > the end of the sturcture. > > > > [v2] > > 1- Update message description to highlight interface change. > > 2- Removed extra empty lines. > > 3- removed unnecessary define. > > 4- Corrected long lines. > > 5- Removed ternary operator. > > 6- Introduced static ucode_update_flags, which will be used later to > > determine local ucode_force_flag. > > Non-style comments on v1 have remained un-addressed. Specifically, to > give an example, while 1 says you now highlight the interface change, > the request was to explain why changing an existing structure is okay > (hint: it likely isn't, as the structure size changes for compat [aka > 32-bit] callers). I see your point now, I will keep the stable ABI as is. > > I'm not going to give the same comments again; I'll rather expect you to > respond to them by either adjustments to the patch (or its description), > or by verbal replies. I will respond to your V1 comment on the previous email to keep things inlined > > Jan Thanks, Fouad
Re: [PATCH v3 4/4] x86/livepatch: perform sanity checks on the payload exception table contents
On 23.04.2024 16:31, Roger Pau Monné wrote:
> On Tue, Apr 23, 2024 at 03:51:31PM +0200, Jan Beulich wrote:
>> On 23.04.2024 15:12, Roger Pau Monne wrote:
>>> Ensure the entries of a payload exception table only apply to text regions
>>> in
>>> the payload itself. Since the payload exception table needs to be loaded
>>> and
>>> active even before a patch is applied (because hooks might already rely on
>>> it),
>>> make sure the exception table (if any) only contains fixups for the payload
>>> text section.
>>>
>>> Signed-off-by: Roger Pau Monné
>>
>> In principle
>> Reviewed-by: Jan Beulich
>> Still two comments:
>>
>>> --- a/xen/arch/x86/extable.c
>>> +++ b/xen/arch/x86/extable.c
>>> @@ -228,3 +228,21 @@ unsigned long asmlinkage
>>> search_pre_exception_table(struct cpu_user_regs *regs)
>>> }
>>> return fixup;
>>> }
>>> +
>>> +#ifdef CONFIG_LIVEPATCH
>>> +bool extable_is_between_bounds(const struct exception_table_entry
>>> *ex_start,
>>
>> s/between/in/ or even s/is_between/in/? "Between", to me at least, reads
>> very much like meaning "exclusive at both ends".
>
> Oh, OK, I don't associate any boundary inclusion with 'between' or
> 'in'. The result is shorter, so I like it.
>
>>> + const struct exception_table_entry *ex_end,
>>> + const void *start, const void *end)
>>> +{
>>> +for ( ; ex_start < ex_end; ex_start++ )
>>> +{
>>> +const void *addr = (void *)ex_addr(ex_start);
>>> +const void *cont = (void *)ex_cont(ex_start);
>>
>> Might be nicer to use _p() here, or not do the comparisons with pointers, but
>> instead with unsigned long-s.
>
> No strong opinion regarding whether to use unsigned longs or pointers.
> I've used pointers because I think the function parameters should be
> pointers, and that avoided doing a cast in the comparison with
> obfuscates it (or introducing yet another local variable).
>
> I can switch to _p(), that's indeed better.
>
> Let me know if you have a strong opinion for using unsigned longs,
> otherwise my preference would be to leave it with pointers.
Especially if you want to stick to pointer function arguments - no, no
strong opinion.
Jan
[PATCH 4/4] x86/shadow: correct shadow_vcpu_init()'s comment
As of the commit referenced below the update_paging_modes() hook is per-
domain and hence also set (already) during domain construction.
Fixes: d0816a9085b5 ("x86/paging: move update_paging_modes() hook")
Signed-off-by: Jan Beulich
--- a/xen/arch/x86/mm/shadow/common.c
+++ b/xen/arch/x86/mm/shadow/common.c
@@ -99,11 +99,12 @@ int shadow_domain_init(struct domain *d)
return 0;
}
-/* Setup the shadow-specfic parts of a vcpu struct. Note: The most important
- * job is to initialize the update_paging_modes() function pointer, which is
- * used to initialized the rest of resources. Therefore, it really does not
- * matter to have v->arch.paging.mode pointing to any mode, as long as it can
- * be compiled.
+/*
+ * Setup the shadow-specific parts of a vcpu struct. Note: The
+ * update_paging_modes() function pointer, which is used to initialize other
+ * resources, was already set during domain creation. Therefore it really does
+ * not matter to have v->arch.paging.mode pointing to any (legitimate) mode,
+ * as long as it can be compiled.
*/
void shadow_vcpu_init(struct vcpu *v)
{
[PATCH 3/4] x86/paging: vCPU host mode is always set
... thanks to paging_vcpu_init() being part of vCPU creation. Further
if paging is enabled on a domain, it's also guaranteed to be either HAP
or shadow. Drop respective unnecessary (parts of) conditionals.
Signed-off-by: Jan Beulich
--- a/xen/arch/x86/mm/paging.c
+++ b/xen/arch/x86/mm/paging.c
@@ -937,19 +937,12 @@ void paging_dump_vcpu_info(struct vcpu *
{
printk("paging assistance: ");
if ( paging_mode_shadow(v->domain) )
-{
-if ( paging_get_hostmode(v) )
-printk("shadowed %u-on-%u\n",
- paging_get_hostmode(v)->guest_levels,
- paging_get_hostmode(v)->shadow.shadow_levels);
-else
-printk("not shadowed\n");
-}
-else if ( paging_mode_hap(v->domain) && paging_get_hostmode(v) )
+printk("shadowed %u-on-%u\n",
+ paging_get_hostmode(v)->guest_levels,
+ paging_get_hostmode(v)->shadow.shadow_levels);
+else
printk("hap, %u levels\n",
paging_get_hostmode(v)->guest_levels);
-else
-printk("none\n");
}
}
[PATCH 2/4] x86/P2M: un-indent write_p2m_entry()
Drop the inner scope that was left from earlier if/else removal. Take
the opportunity and make the paging_unlock() invocation common to
success and error paths, though.
Signed-off-by: Jan Beulich
--- a/xen/arch/x86/mm/p2m-pt.c
+++ b/xen/arch/x86/mm/p2m-pt.c
@@ -110,49 +110,43 @@ static int write_p2m_entry(struct p2m_do
unsigned int level)
{
struct domain *d = p2m->domain;
-
+unsigned int oflags;
+mfn_t omfn;
+int rc;
+
+paging_lock(d);
+
+if ( p2m->write_p2m_entry_pre && gfn != gfn_x(INVALID_GFN) )
+p2m->write_p2m_entry_pre(d, gfn, *p, new, level);
+
+oflags = l1e_get_flags(*p);
+omfn = l1e_get_mfn(*p);
+
+rc = p2m_entry_modify(p2m, p2m_flags_to_type(l1e_get_flags(new)),
+ p2m_flags_to_type(oflags), l1e_get_mfn(new),
+ omfn, level);
+if ( !rc )
{
-unsigned int oflags;
-mfn_t omfn;
-int rc;
-
-paging_lock(d);
-
-if ( p2m->write_p2m_entry_pre && gfn != gfn_x(INVALID_GFN) )
-p2m->write_p2m_entry_pre(d, gfn, *p, new, level);
-
-oflags = l1e_get_flags(*p);
-omfn = l1e_get_mfn(*p);
-
-rc = p2m_entry_modify(p2m, p2m_flags_to_type(l1e_get_flags(new)),
- p2m_flags_to_type(oflags), l1e_get_mfn(new),
- omfn, level);
-if ( rc )
-{
-paging_unlock(d);
-return rc;
-}
-
safe_write_pte(p, new);
if ( p2m->write_p2m_entry_post )
p2m->write_p2m_entry_post(p2m, oflags);
+}
-paging_unlock(d);
+paging_unlock(d);
-if ( nestedhvm_enabled(d) && !p2m_is_nestedp2m(p2m) &&
- (oflags & _PAGE_PRESENT) &&
- !p2m_get_hostp2m(d)->defer_nested_flush &&
- /*
- * We are replacing a valid entry so we need to flush nested p2ms,
- * unless the only change is an increase in access rights.
- */
- (!mfn_eq(omfn, l1e_get_mfn(new)) ||
- !perms_strictly_increased(oflags, l1e_get_flags(new))) )
-p2m_flush_nestedp2m(d);
-}
+if ( !rc && nestedhvm_enabled(d) && !p2m_is_nestedp2m(p2m) &&
+ (oflags & _PAGE_PRESENT) &&
+ !p2m_get_hostp2m(d)->defer_nested_flush &&
+ /*
+ * We are replacing a valid entry so we need to flush nested p2ms,
+ * unless the only change is an increase in access rights.
+ */
+ (!mfn_eq(omfn, l1e_get_mfn(new)) ||
+ !perms_strictly_increased(oflags, l1e_get_flags(new))) )
+p2m_flush_nestedp2m(d);
-return 0;
+return rc;
}
// Find the next level's P2M entry, checking for out-of-range gfn's...
[PATCH 1/4] x86/P2M: write_p2m_entry() is HVM-only anyway
The latest as of e2b2ff677958 ("x86/P2M: split out init/teardown
functions") the function is obviously unreachable for PV guests. Hence
the paging_mode_enabled(d) check is pointless.
Further host mode of a vCPU is always set, by virtue of
paging_vcpu_init() being part of vCPU creation. Hence the
paging_get_hostmode() check is pointless.
With that the v local variable is unnecessary too. Drop the "if()"
conditional and its corresponding "else".
Signed-off-by: Jan Beulich
---
I have to confess that this if() has been puzzling me before.
--- a/xen/arch/x86/mm/p2m-pt.c
+++ b/xen/arch/x86/mm/p2m-pt.c
@@ -110,12 +110,7 @@ static int write_p2m_entry(struct p2m_do
unsigned int level)
{
struct domain *d = p2m->domain;
-const struct vcpu *v = current;
-if ( v->domain != d )
-v = d->vcpu ? d->vcpu[0] : NULL;
-if ( likely(v && paging_mode_enabled(d) && paging_get_hostmode(v)) ||
- p2m_is_nestedp2m(p2m) )
{
unsigned int oflags;
mfn_t omfn;
@@ -156,8 +151,6 @@ static int write_p2m_entry(struct p2m_do
!perms_strictly_increased(oflags, l1e_get_flags(new))) )
p2m_flush_nestedp2m(d);
}
-else
-safe_write_pte(p, new);
return 0;
}
Re: [PATCH v3 4/4] x86/livepatch: perform sanity checks on the payload exception table contents
On Tue, Apr 23, 2024 at 03:51:31PM +0200, Jan Beulich wrote:
> On 23.04.2024 15:12, Roger Pau Monne wrote:
> > Ensure the entries of a payload exception table only apply to text regions
> > in
> > the payload itself. Since the payload exception table needs to be loaded
> > and
> > active even before a patch is applied (because hooks might already rely on
> > it),
> > make sure the exception table (if any) only contains fixups for the payload
> > text section.
> >
> > Signed-off-by: Roger Pau Monné
>
> In principle
> Reviewed-by: Jan Beulich
> Still two comments:
>
> > --- a/xen/arch/x86/extable.c
> > +++ b/xen/arch/x86/extable.c
> > @@ -228,3 +228,21 @@ unsigned long asmlinkage
> > search_pre_exception_table(struct cpu_user_regs *regs)
> > }
> > return fixup;
> > }
> > +
> > +#ifdef CONFIG_LIVEPATCH
> > +bool extable_is_between_bounds(const struct exception_table_entry
> > *ex_start,
>
> s/between/in/ or even s/is_between/in/? "Between", to me at least, reads
> very much like meaning "exclusive at both ends".
Oh, OK, I don't associate any boundary inclusion with 'between' or
'in'. The result is shorter, so I like it.
> > + const struct exception_table_entry *ex_end,
> > + const void *start, const void *end)
> > +{
> > +for ( ; ex_start < ex_end; ex_start++ )
> > +{
> > +const void *addr = (void *)ex_addr(ex_start);
> > +const void *cont = (void *)ex_cont(ex_start);
>
> Might be nicer to use _p() here, or not do the comparisons with pointers, but
> instead with unsigned long-s.
No strong opinion regarding whether to use unsigned longs or pointers.
I've used pointers because I think the function parameters should be
pointers, and that avoided doing a cast in the comparison with
obfuscates it (or introducing yet another local variable).
I can switch to _p(), that's indeed better.
Let me know if you have a strong opinion for using unsigned longs,
otherwise my preference would be to leave it with pointers.
Thanks, Roger.
[PATCH 0/4] x86/mm: assorted adjustments
1: P2M: write_p2m_entry() is HVM-only anyway 2: P2M: un-indent write_p2m_entry() 3: paging: vCPU host mode is always set 4: shadow: correct shadow_vcpu_init()'s comment Jan
Re: [PATCH v3 3/4] livepatch: refuse to resolve symbols that belong to init sections
On 23.04.2024 16:26, Roger Pau Monné wrote:
> On Tue, Apr 23, 2024 at 03:44:42PM +0200, Jan Beulich wrote:
>> On 23.04.2024 15:12, Roger Pau Monne wrote:
>>> Livepatch payloads containing symbols that belong to init sections can only
>>> lead to page faults later on, as by the time the livepatch is loaded init
>>> sections have already been freed.
>>>
>>> Refuse to resolve such symbols and return an error instead.
>>>
>>> Note such resolutions are only relevant for symbols that point to undefined
>>> sections (SHN_UNDEF), as that implies the symbol is not in the current
>>> payload
>>> and hence must either be a Xen or a different livepatch payload symbol.
>>>
>>> Do not allow to resolve symbols that point to __init_begin, as that address
>>> is
>>> also unmapped. On the other hand, __init_end is not unmapped, and hence
>>> allow
>>> resolutions against it.
>>>
>>> Since __init_begin can alias other symbols (like _erodata for example)
>>> allow the force flag to override the check and resolve the symbol anyway.
>>>
>>> Signed-off-by: Roger Pau Monné
>>
>> In principle, as promised (and just to indicate earlier concerns were
>> addressed, as this is meaningless for other purposes)
>> Acked-by: Jan Beulich
>> However, ...
>>
>>> @@ -310,6 +311,21 @@ int livepatch_elf_resolve_symbols(struct livepatch_elf
>>> *elf)
>>> break;
>>> }
>>> }
>>> +
>>> +/*
>>> + * Ensure not an init symbol. Only applicable to Xen symbols,
>>> as
>>> + * livepatch payloads don't have init sections or equivalent.
>>> + */
>>> +else if ( st_value >= (uintptr_t)&__init_begin &&
>>> + st_value < (uintptr_t)&__init_end && !force )
>>> +{
>>> +printk(XENLOG_ERR LIVEPATCH
>>> + "%s: symbol %s is in init section, not resolving\n",
>>> + elf->name, elf->sym[i].name);
>>> +rc = -ENXIO;
>>> +break;
>>> +}
>>
>> ... wouldn't it make sense to still warn in this case when "force" is set?
>
> Pondered it, I was thinking that a user would first run without
> --force, and use the option as a result of seeing the first failure.
>
> However if there is more than one check that's bypassed, further ones
> won't be noticed, so:
>
> else if ( st_value >= (uintptr_t)&__init_begin &&
> st_value < (uintptr_t)&__init_end )
> {
> printk(XENLOG_ERR LIVEPATCH
>"%s: symbol %s is in init section, not resolving\n",
>elf->name, elf->sym[i].name);
> if ( !force )
> {
> rc = -ENXIO;
> break;
> }
> }
>
> Would be OK then?
Perhaps. "not resolving" isn't quite true when "force" is true, and warnings
would also better not be issued with XENLOG_ERR.
Jan
Re: [XEN PATCH v2 4/5] xen/arm: allow dynamically assigned SGI handlers
Hi Bertrand, On 23/04/2024 14:23, Bertrand Marquis wrote: Hi Julien, On 23 Apr 2024, at 14:37, Bertrand Marquis wrote: Hi Julien, On 23 Apr 2024, at 13:05, Julien Grall wrote: On 23/04/2024 10:35, Jens Wiklander wrote: Hi Julien, Hi Jens, On Mon, Apr 22, 2024 at 12:57 PM Julien Grall wrote: Hi Jens, On 22/04/2024 08:37, Jens Wiklander wrote: Updates so request_irq() can be used with a dynamically assigned SGI irq as input. This prepares for a later patch where an FF-A schedule receiver interrupt handler is installed for an SGI generated by the secure world. I would like to understand the use-case a bit more. Who is responsible to decide the SGI number? Is it Xen or the firmware? If the later, how can we ever guarantee the ID is not going to clash with what the OS/hypervisor is using? Is it described in a specification? If so, please give a pointer. The firmware decides the SGI number. Given that the firmware doesn't know which SGIs Xen is using it typically needs to donate one of the secure SGIs, but that is transparent to Xen. Right this is my concern. The firmware decides the number, but at the same time Xen thinks that all the SGIs are available (AFAIK there is only one set). What I would like to see is some wording from a spec indicating that the SGIs ID reserved by the firmware will not be clashing with the one used by Xen. The idea is that the only SGI reserved for secure are used by the secure world (in fact it is the SPMC in the secure world who tells us which SGI it will generate). So in theory that means it will always use an SGI between 8 and 15. Now it could make sense in fact to check that the number returned by the firmware (or SPMC) is not clashing with Xen as it is a recommendation in the spec and in fact an implementation might do something different. Right now there is no spec that will say that it will never clash with the one used by Xen as the FF-A spec is not enforcing anything here so it would be a good idea to check and disable FF-A with a proper error message if this happens. After some more digging here is what is recommended by Arm in the Arm Base System Architecture v1.0C [1]: "The system shall implement at least eight Non-secure SGIs, assigned to interrupt IDs 0-7." Thanks! Can we provide a link to the specification in the commit message? So basically as long as Xen is using SGIs 0-7 it is safe as those shall never be used by the secure world. Now i do agree that we should check that whatever is returned by the firmware is not conflicting with what is used by Xen. +1. Cheers, -- Julien Grall
Re: [PATCH v3 3/4] livepatch: refuse to resolve symbols that belong to init sections
On Tue, Apr 23, 2024 at 03:44:42PM +0200, Jan Beulich wrote:
> On 23.04.2024 15:12, Roger Pau Monne wrote:
> > Livepatch payloads containing symbols that belong to init sections can only
> > lead to page faults later on, as by the time the livepatch is loaded init
> > sections have already been freed.
> >
> > Refuse to resolve such symbols and return an error instead.
> >
> > Note such resolutions are only relevant for symbols that point to undefined
> > sections (SHN_UNDEF), as that implies the symbol is not in the current
> > payload
> > and hence must either be a Xen or a different livepatch payload symbol.
> >
> > Do not allow to resolve symbols that point to __init_begin, as that address
> > is
> > also unmapped. On the other hand, __init_end is not unmapped, and hence
> > allow
> > resolutions against it.
> >
> > Since __init_begin can alias other symbols (like _erodata for example)
> > allow the force flag to override the check and resolve the symbol anyway.
> >
> > Signed-off-by: Roger Pau Monné
>
> In principle, as promised (and just to indicate earlier concerns were
> addressed, as this is meaningless for other purposes)
> Acked-by: Jan Beulich
> However, ...
>
> > @@ -310,6 +311,21 @@ int livepatch_elf_resolve_symbols(struct livepatch_elf
> > *elf)
> > break;
> > }
> > }
> > +
> > +/*
> > + * Ensure not an init symbol. Only applicable to Xen symbols,
> > as
> > + * livepatch payloads don't have init sections or equivalent.
> > + */
> > +else if ( st_value >= (uintptr_t)&__init_begin &&
> > + st_value < (uintptr_t)&__init_end && !force )
> > +{
> > +printk(XENLOG_ERR LIVEPATCH
> > + "%s: symbol %s is in init section, not resolving\n",
> > + elf->name, elf->sym[i].name);
> > +rc = -ENXIO;
> > +break;
> > +}
>
> ... wouldn't it make sense to still warn in this case when "force" is set?
Pondered it, I was thinking that a user would first run without
--force, and use the option as a result of seeing the first failure.
However if there is more than one check that's bypassed, further ones
won't be noticed, so:
else if ( st_value >= (uintptr_t)&__init_begin &&
st_value < (uintptr_t)&__init_end )
{
printk(XENLOG_ERR LIVEPATCH
"%s: symbol %s is in init section, not resolving\n",
elf->name, elf->sym[i].name);
if ( !force )
{
rc = -ENXIO;
break;
}
}
Would be OK then?
Thanks, Roger.
Re: [PATCH v3 1/4] xen-livepatch: fix parameter name parsing
On Tue, Apr 23, 2024 at 03:33:36PM +0200, Jan Beulich wrote: > On 23.04.2024 15:12, Roger Pau Monne wrote: > > It's incorrect to restrict strncmp to the length of the command line input > > parameter, as then a user passing a rune like: > > > > % xen-livepatch up foo.livepatch > > > > Would match against the "upload" command, because the string comparison has > > been truncated to the length of the input argument. Instead the truncation > > should be done based on the length of the command name stored in the > > internal > > array of actions. > > But then "xen-livepatch upload-or-not foo.livepatch" would still wrongly > match. Why strncmp() at all, rather than strcmp()? Bah, indeed, how dumb of me. Will switch to strcmp in the next version. Thanks, Roger.-
Re: [PATCH v3 4/4] x86/livepatch: perform sanity checks on the payload exception table contents
On 23.04.2024 15:12, Roger Pau Monne wrote:
> Ensure the entries of a payload exception table only apply to text regions in
> the payload itself. Since the payload exception table needs to be loaded and
> active even before a patch is applied (because hooks might already rely on
> it),
> make sure the exception table (if any) only contains fixups for the payload
> text section.
>
> Signed-off-by: Roger Pau Monné
In principle
Reviewed-by: Jan Beulich
Still two comments:
> --- a/xen/arch/x86/extable.c
> +++ b/xen/arch/x86/extable.c
> @@ -228,3 +228,21 @@ unsigned long asmlinkage
> search_pre_exception_table(struct cpu_user_regs *regs)
> }
> return fixup;
> }
> +
> +#ifdef CONFIG_LIVEPATCH
> +bool extable_is_between_bounds(const struct exception_table_entry *ex_start,
s/between/in/ or even s/is_between/in/? "Between", to me at least, reads
very much like meaning "exclusive at both ends".
> + const struct exception_table_entry *ex_end,
> + const void *start, const void *end)
> +{
> +for ( ; ex_start < ex_end; ex_start++ )
> +{
> +const void *addr = (void *)ex_addr(ex_start);
> +const void *cont = (void *)ex_cont(ex_start);
Might be nicer to use _p() here, or not do the comparisons with pointers, but
instead with unsigned long-s.
Jan
Re: [PATCH v3 3/4] livepatch: refuse to resolve symbols that belong to init sections
On 23.04.2024 15:12, Roger Pau Monne wrote:
> Livepatch payloads containing symbols that belong to init sections can only
> lead to page faults later on, as by the time the livepatch is loaded init
> sections have already been freed.
>
> Refuse to resolve such symbols and return an error instead.
>
> Note such resolutions are only relevant for symbols that point to undefined
> sections (SHN_UNDEF), as that implies the symbol is not in the current payload
> and hence must either be a Xen or a different livepatch payload symbol.
>
> Do not allow to resolve symbols that point to __init_begin, as that address is
> also unmapped. On the other hand, __init_end is not unmapped, and hence allow
> resolutions against it.
>
> Since __init_begin can alias other symbols (like _erodata for example)
> allow the force flag to override the check and resolve the symbol anyway.
>
> Signed-off-by: Roger Pau Monné
In principle, as promised (and just to indicate earlier concerns were
addressed, as this is meaningless for other purposes)
Acked-by: Jan Beulich
However, ...
> @@ -310,6 +311,21 @@ int livepatch_elf_resolve_symbols(struct livepatch_elf
> *elf)
> break;
> }
> }
> +
> +/*
> + * Ensure not an init symbol. Only applicable to Xen symbols, as
> + * livepatch payloads don't have init sections or equivalent.
> + */
> +else if ( st_value >= (uintptr_t)&__init_begin &&
> + st_value < (uintptr_t)&__init_end && !force )
> +{
> +printk(XENLOG_ERR LIVEPATCH
> + "%s: symbol %s is in init section, not resolving\n",
> + elf->name, elf->sym[i].name);
> +rc = -ENXIO;
> +break;
> +}
... wouldn't it make sense to still warn in this case when "force" is set?
Jan
Re: [PATCH v3 2/4] livepatch: introduce --force option
On 23.04.2024 15:12, Roger Pau Monne wrote:
> --- a/xen/common/livepatch.c
> +++ b/xen/common/livepatch.c
> @@ -2125,7 +2125,8 @@ int livepatch_op(struct xen_sysctl_livepatch_op
> *livepatch)
> {
> int rc;
>
> -if ( livepatch->pad )
> +if ( livepatch->flags & ~LIVEPATCH_FLAGS_MASK &&
With parentheses added here (which presumably could be done while
committing)
Acked-by: Jan Beulich
Jan
Re: [PATCH v3 1/4] xen-livepatch: fix parameter name parsing
On 23.04.2024 15:12, Roger Pau Monne wrote:
> It's incorrect to restrict strncmp to the length of the command line input
> parameter, as then a user passing a rune like:
>
> % xen-livepatch up foo.livepatch
>
> Would match against the "upload" command, because the string comparison has
> been truncated to the length of the input argument. Instead the truncation
> should be done based on the length of the command name stored in the internal
> array of actions.
But then "xen-livepatch upload-or-not foo.livepatch" would still wrongly
match. Why strncmp() at all, rather than strcmp()?
Jan
> Fixes: 05bb8afedede ('xen-xsplice: Tool to manipulate xsplice payloads')
> Signed-off-by: Roger Pau Monné
> ---
> Changes since v2:
> - New in this version.
> ---
> tools/misc/xen-livepatch.c | 6 --
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/tools/misc/xen-livepatch.c b/tools/misc/xen-livepatch.c
> index 5bf9d9a32b65..a246e5dfd38e 100644
> --- a/tools/misc/xen-livepatch.c
> +++ b/tools/misc/xen-livepatch.c
> @@ -572,13 +572,15 @@ int main(int argc, char *argv[])
> return 0;
> }
> for ( i = 0; i < ARRAY_SIZE(main_options); i++ )
> -if (!strncmp(main_options[i].name, argv[1], strlen(argv[1])))
> +if (!strncmp(main_options[i].name, argv[1],
> + strlen(main_options[i].name)))
> break;
>
> if ( i == ARRAY_SIZE(main_options) )
> {
> for ( j = 0; j < ARRAY_SIZE(action_options); j++ )
> -if (!strncmp(action_options[j].name, argv[1], strlen(argv[1])))
> +if (!strncmp(action_options[j].name, argv[1],
> + strlen(action_options[j].name)))
> break;
>
> if ( j == ARRAY_SIZE(action_options) )
Re: [XEN PATCH v2 4/5] xen/arm: allow dynamically assigned SGI handlers
Hi Julien, > On 23 Apr 2024, at 14:37, Bertrand Marquis wrote: > > Hi Julien, > >> On 23 Apr 2024, at 13:05, Julien Grall wrote: >> >> >> >> On 23/04/2024 10:35, Jens Wiklander wrote: >>> Hi Julien, >> >> Hi Jens, >> >>> On Mon, Apr 22, 2024 at 12:57 PM Julien Grall wrote: Hi Jens, On 22/04/2024 08:37, Jens Wiklander wrote: > Updates so request_irq() can be used with a dynamically assigned SGI irq > as input. This prepares for a later patch where an FF-A schedule > receiver interrupt handler is installed for an SGI generated by the > secure world. I would like to understand the use-case a bit more. Who is responsible to decide the SGI number? Is it Xen or the firmware? If the later, how can we ever guarantee the ID is not going to clash with what the OS/hypervisor is using? Is it described in a specification? If so, please give a pointer. >>> The firmware decides the SGI number. Given that the firmware doesn't >>> know which SGIs Xen is using it typically needs to donate one of the >>> secure SGIs, but that is transparent to Xen. >> >> Right this is my concern. The firmware decides the number, but at the same >> time Xen thinks that all the SGIs are available (AFAIK there is only one >> set). >> >> What I would like to see is some wording from a spec indicating that the >> SGIs ID reserved by the firmware will not be clashing with the one used by >> Xen. > > The idea is that the only SGI reserved for secure are used by the secure > world (in fact it is the SPMC in the secure world who tells us which SGI it > will generate). > So in theory that means it will always use an SGI between 8 and 15. > > Now it could make sense in fact to check that the number returned by the > firmware (or SPMC) is not clashing with Xen as it is a recommendation in the > spec and > in fact an implementation might do something different. > > Right now there is no spec that will say that it will never clash with the > one used by Xen as the FF-A spec is not enforcing anything here so it would > be a good idea > to check and disable FF-A with a proper error message if this happens. After some more digging here is what is recommended by Arm in the Arm Base System Architecture v1.0C [1]: "The system shall implement at least eight Non-secure SGIs, assigned to interrupt IDs 0-7." So basically as long as Xen is using SGIs 0-7 it is safe as those shall never be used by the secure world. Now i do agree that we should check that whatever is returned by the firmware is not conflicting with what is used by Xen. Cheers Bertrand [1] https://developer.arm.com/documentation/den0094/
Re: [PATCH v2 1/2] xen: introduce header file with section related symbols
On Tue, Apr 23, 2024 at 02:25:16PM +0200, Jan Beulich wrote: > On 19.04.2024 12:02, Roger Pau Monne wrote: > Then I wonder why it was this rather than ... > > > --- a/xen/arch/x86/setup.c > > +++ b/xen/arch/x86/setup.c > > @@ -5,6 +5,7 @@ > > #include > > #include > > #include > > +#include > > #include > > #include > > #include > > @@ -309,8 +310,6 @@ void __init discard_initial_images(void) > > initial_images = NULL; > > } > > > > -extern unsigned char __init_begin[], __init_end[]; > > ... this ... > > > --- /dev/null > > +++ b/xen/include/xen/sections.h > > @@ -0,0 +1,17 @@ > > +/* SPDX-License-Identifier: GPL-2.0-only */ > > + > > +#ifndef __XEN_SECTIONS_H__ > > +#define __XEN_SECTIONS_H__ > > + > > +/* SAF-0-safe */ > > +extern char __init_begin[], __init_end[]; > > ... that was moved. "unsigned char" is generally preferred over > declaring binary stuff "strings". OK, noted. > I further wonder why the opportunity > wasn't taken to make both array-of-const. Hm, yes. The sections are mapped RWX IIRC, but there's no reason to not make the markers const. > And finally I'm slightly > puzzled by the SAF comment appearing with no mention at all in the > description; of course I don't mind its addition. I could have noted it in the commit message indeed, sorry. Thanks, Roger.
[PATCH v3 1/4] xen-livepatch: fix parameter name parsing
It's incorrect to restrict strncmp to the length of the command line input
parameter, as then a user passing a rune like:
% xen-livepatch up foo.livepatch
Would match against the "upload" command, because the string comparison has
been truncated to the length of the input argument. Instead the truncation
should be done based on the length of the command name stored in the internal
array of actions.
Fixes: 05bb8afedede ('xen-xsplice: Tool to manipulate xsplice payloads')
Signed-off-by: Roger Pau Monné
---
Changes since v2:
- New in this version.
---
tools/misc/xen-livepatch.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/tools/misc/xen-livepatch.c b/tools/misc/xen-livepatch.c
index 5bf9d9a32b65..a246e5dfd38e 100644
--- a/tools/misc/xen-livepatch.c
+++ b/tools/misc/xen-livepatch.c
@@ -572,13 +572,15 @@ int main(int argc, char *argv[])
return 0;
}
for ( i = 0; i < ARRAY_SIZE(main_options); i++ )
-if (!strncmp(main_options[i].name, argv[1], strlen(argv[1])))
+if (!strncmp(main_options[i].name, argv[1],
+ strlen(main_options[i].name)))
break;
if ( i == ARRAY_SIZE(main_options) )
{
for ( j = 0; j < ARRAY_SIZE(action_options); j++ )
-if (!strncmp(action_options[j].name, argv[1], strlen(argv[1])))
+if (!strncmp(action_options[j].name, argv[1],
+ strlen(action_options[j].name)))
break;
if ( j == ARRAY_SIZE(action_options) )
--
2.44.0
[PATCH v3 3/4] livepatch: refuse to resolve symbols that belong to init sections
Livepatch payloads containing symbols that belong to init sections can only
lead to page faults later on, as by the time the livepatch is loaded init
sections have already been freed.
Refuse to resolve such symbols and return an error instead.
Note such resolutions are only relevant for symbols that point to undefined
sections (SHN_UNDEF), as that implies the symbol is not in the current payload
and hence must either be a Xen or a different livepatch payload symbol.
Do not allow to resolve symbols that point to __init_begin, as that address is
also unmapped. On the other hand, __init_end is not unmapped, and hence allow
resolutions against it.
Since __init_begin can alias other symbols (like _erodata for example)
allow the force flag to override the check and resolve the symbol anyway.
Signed-off-by: Roger Pau Monné
---
Changes since v2:
- Allow bypassing added check with the force flag.
Changes since v1:
- Fix off-by-one in range checking.
---
xen/common/livepatch.c | 13 -
xen/common/livepatch_elf.c | 18 +-
xen/include/xen/livepatch_elf.h | 2 +-
3 files changed, 26 insertions(+), 7 deletions(-)
diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index 1503a84457e4..36cf4bee8b8a 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -1080,7 +1080,8 @@ static void free_payload(struct payload *data)
xfree(data);
}
-static int load_payload_data(struct payload *payload, void *raw, size_t len)
+static int load_payload_data(struct payload *payload, void *raw, size_t len,
+ bool force)
{
struct livepatch_elf elf = { .name = payload->name, .len = len };
int rc = 0;
@@ -1093,7 +1094,7 @@ static int load_payload_data(struct payload *payload,
void *raw, size_t len)
if ( rc )
goto out;
-rc = livepatch_elf_resolve_symbols();
+rc = livepatch_elf_resolve_symbols(, force);
if ( rc )
goto out;
@@ -1133,7 +1134,8 @@ static int load_payload_data(struct payload *payload,
void *raw, size_t len)
return rc;
}
-static int livepatch_upload(struct xen_sysctl_livepatch_upload *upload)
+static int livepatch_upload(struct xen_sysctl_livepatch_upload *upload,
+bool force)
{
struct payload *data, *found;
char n[XEN_LIVEPATCH_NAME_SIZE];
@@ -1162,7 +1164,7 @@ static int livepatch_upload(struct
xen_sysctl_livepatch_upload *upload)
{
memcpy(data->name, n, strlen(n));
-rc = load_payload_data(data, raw_data, upload->size);
+rc = load_payload_data(data, raw_data, upload->size, force);
if ( rc )
goto out;
@@ -2132,7 +2134,8 @@ int livepatch_op(struct xen_sysctl_livepatch_op
*livepatch)
switch ( livepatch->cmd )
{
case XEN_SYSCTL_LIVEPATCH_UPLOAD:
-rc = livepatch_upload(>u.upload);
+rc = livepatch_upload(>u.upload,
+ livepatch->flags & LIVEPATCH_FLAG_FORCE);
break;
case XEN_SYSCTL_LIVEPATCH_GET:
diff --git a/xen/common/livepatch_elf.c b/xen/common/livepatch_elf.c
index 45d73912a3cd..0436f2d5fcbd 100644
--- a/xen/common/livepatch_elf.c
+++ b/xen/common/livepatch_elf.c
@@ -4,6 +4,7 @@
#include
#include
+#include
#include
#include
#include
@@ -276,7 +277,7 @@ static int elf_get_sym(struct livepatch_elf *elf, const
void *data)
return 0;
}
-int livepatch_elf_resolve_symbols(struct livepatch_elf *elf)
+int livepatch_elf_resolve_symbols(struct livepatch_elf *elf, bool force)
{
unsigned int i;
int rc = 0;
@@ -310,6 +311,21 @@ int livepatch_elf_resolve_symbols(struct livepatch_elf
*elf)
break;
}
}
+
+/*
+ * Ensure not an init symbol. Only applicable to Xen symbols, as
+ * livepatch payloads don't have init sections or equivalent.
+ */
+else if ( st_value >= (uintptr_t)&__init_begin &&
+ st_value < (uintptr_t)&__init_end && !force )
+{
+printk(XENLOG_ERR LIVEPATCH
+ "%s: symbol %s is in init section, not resolving\n",
+ elf->name, elf->sym[i].name);
+rc = -ENXIO;
+break;
+}
+
dprintk(XENLOG_DEBUG, LIVEPATCH "%s: Undefined symbol resolved: %s
=> %#"PRIxElfAddr"\n",
elf->name, elf->sym[i].name, st_value);
break;
diff --git a/xen/include/xen/livepatch_elf.h b/xen/include/xen/livepatch_elf.h
index 7116deaddc28..84e9c5eb7be5 100644
--- a/xen/include/xen/livepatch_elf.h
+++ b/xen/include/xen/livepatch_elf.h
@@ -44,7 +44,7 @@ livepatch_elf_sec_by_name(const struct livepatch_elf *elf,
int livepatch_elf_load(struct livepatch_elf *elf, const void *data);
void livepatch_elf_free(struct livepatch_elf *elf);
-int livepatch_elf_resolve_symbols(struct livepatch_elf *elf);
+int
[PATCH v3 4/4] x86/livepatch: perform sanity checks on the payload exception table contents
Ensure the entries of a payload exception table only apply to text regions in
the payload itself. Since the payload exception table needs to be loaded and
active even before a patch is applied (because hooks might already rely on it),
make sure the exception table (if any) only contains fixups for the payload
text section.
Signed-off-by: Roger Pau Monné
---
Changes since v2:
- New in this version.
---
xen/arch/x86/extable.c | 18 ++
xen/arch/x86/include/asm/uaccess.h | 4
xen/common/livepatch.c | 9 +
3 files changed, 31 insertions(+)
diff --git a/xen/arch/x86/extable.c b/xen/arch/x86/extable.c
index 8415cd1fa249..9e91e8234e71 100644
--- a/xen/arch/x86/extable.c
+++ b/xen/arch/x86/extable.c
@@ -228,3 +228,21 @@ unsigned long asmlinkage search_pre_exception_table(struct
cpu_user_regs *regs)
}
return fixup;
}
+
+#ifdef CONFIG_LIVEPATCH
+bool extable_is_between_bounds(const struct exception_table_entry *ex_start,
+ const struct exception_table_entry *ex_end,
+ const void *start, const void *end)
+{
+for ( ; ex_start < ex_end; ex_start++ )
+{
+const void *addr = (void *)ex_addr(ex_start);
+const void *cont = (void *)ex_cont(ex_start);
+
+if ( addr < start || addr >= end || cont < start || cont >= end )
+return false;
+}
+
+return true;
+}
+#endif
diff --git a/xen/arch/x86/include/asm/uaccess.h
b/xen/arch/x86/include/asm/uaccess.h
index 48b684c19d44..0dad61e21a9c 100644
--- a/xen/arch/x86/include/asm/uaccess.h
+++ b/xen/arch/x86/include/asm/uaccess.h
@@ -426,5 +426,9 @@ extern unsigned long search_exception_table(const struct
cpu_user_regs *regs,
extern void sort_exception_tables(void);
extern void sort_exception_table(struct exception_table_entry *start,
const struct exception_table_entry *stop);
+extern bool extable_is_between_bounds(
+const struct exception_table_entry *ex_start,
+const struct exception_table_entry *ex_end,
+const void *start, const void *end);
#endif /* __X86_UACCESS_H__ */
diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index 36cf4bee8b8a..67b6815d87ac 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -912,6 +912,15 @@ static int prepare_payload(struct payload *payload,
s = sec->load_addr;
e = sec->load_addr + sec->sec->sh_size;
+if ( !extable_is_between_bounds(s, e, payload->text_addr,
+payload->text_addr + payload->text_size) )
+{
+printk(XENLOG_ERR LIVEPATCH
+ "%s: Invalid exception table with out of bounds entries\n",
+ elf->name);
+return -EINVAL;
+}
+
sort_exception_table(s ,e);
region->ex = s;
--
2.44.0
[PATCH v3 2/4] livepatch: introduce --force option
Introduce a xen-livepatch tool --force option, that's propagated into the
hyerpvisor for livepatch operations. The intention is for the option to be
used to bypass some checks that would otherwise prevent the patch from being
loaded.
Re purpose the pad field in xen_sysctl_livepatch_op to be a flags field that
applies to all livepatch operations. The flag is currently only set by the
hypercall wrappers for the XEN_SYSCTL_LIVEPATCH_UPLOAD operation, as that's so
far the only one where it will be used initially. Other uses can be added as
required.
Note that helpers would set the .pad field to 0, that's been removed since the
structure is already zero initialized at definition.
No functional usages of the new flag introduced in this patch.
Signed-off-by: Roger Pau Monné
---
Changes since v2:
- New in this version.
---
tools/include/xenctrl.h | 3 ++-
tools/libs/ctrl/xc_misc.c | 7 +++
tools/misc/xen-livepatch.c | 21 +++--
xen/common/livepatch.c | 3 ++-
xen/include/public/sysctl.h | 4 +++-
5 files changed, 29 insertions(+), 9 deletions(-)
diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index 2ef8b4e05422..499685594427 100644
--- a/tools/include/xenctrl.h
+++ b/tools/include/xenctrl.h
@@ -2555,7 +2555,8 @@ int xc_psr_get_hw_info(xc_interface *xch, uint32_t socket,
#endif
int xc_livepatch_upload(xc_interface *xch,
-char *name, unsigned char *payload, uint32_t size);
+char *name, unsigned char *payload, uint32_t size,
+bool force);
int xc_livepatch_get(xc_interface *xch,
char *name,
diff --git a/tools/libs/ctrl/xc_misc.c b/tools/libs/ctrl/xc_misc.c
index 5ecdfa2c7934..50282fd60dcc 100644
--- a/tools/libs/ctrl/xc_misc.c
+++ b/tools/libs/ctrl/xc_misc.c
@@ -576,7 +576,8 @@ int xc_getcpuinfo(xc_interface *xch, int max_cpus,
int xc_livepatch_upload(xc_interface *xch,
char *name,
unsigned char *payload,
-uint32_t size)
+uint32_t size,
+bool force)
{
int rc;
struct xen_sysctl sysctl = {};
@@ -612,7 +613,7 @@ int xc_livepatch_upload(xc_interface *xch,
sysctl.cmd = XEN_SYSCTL_livepatch_op;
sysctl.u.livepatch.cmd = XEN_SYSCTL_LIVEPATCH_UPLOAD;
-sysctl.u.livepatch.pad = 0;
+sysctl.u.livepatch.flags = force ? LIVEPATCH_FLAG_FORCE : 0;
sysctl.u.livepatch.u.upload.size = size;
set_xen_guest_handle(sysctl.u.livepatch.u.upload.payload, local);
@@ -656,7 +657,6 @@ int xc_livepatch_get(xc_interface *xch,
sysctl.cmd = XEN_SYSCTL_livepatch_op;
sysctl.u.livepatch.cmd = XEN_SYSCTL_LIVEPATCH_GET;
-sysctl.u.livepatch.pad = 0;
sysctl.u.livepatch.u.get.status.state = 0;
sysctl.u.livepatch.u.get.status.rc = 0;
@@ -985,7 +985,6 @@ static int _xc_livepatch_action(xc_interface *xch,
sysctl.cmd = XEN_SYSCTL_livepatch_op;
sysctl.u.livepatch.cmd = XEN_SYSCTL_LIVEPATCH_ACTION;
-sysctl.u.livepatch.pad = 0;
sysctl.u.livepatch.u.action.cmd = action;
sysctl.u.livepatch.u.action.timeout = timeout;
sysctl.u.livepatch.u.action.flags = flags;
diff --git a/tools/misc/xen-livepatch.c b/tools/misc/xen-livepatch.c
index a246e5dfd38e..cf2e5fada12d 100644
--- a/tools/misc/xen-livepatch.c
+++ b/tools/misc/xen-livepatch.c
@@ -19,11 +19,15 @@
static xc_interface *xch;
+/* Global option to disable checks. */
+static bool force;
+
void show_help(void)
{
fprintf(stderr,
"xen-livepatch: live patching tool\n"
-"Usage: xen-livepatch [args] [command-flags]\n"
+"Usage: xen-livepatch [--force] [args] [command-flags]\n"
+" Use --force option to bypass some checks.\n"
" An unique name of payload. Up to %d characters.\n"
"Commands:\n"
" help display this help\n"
@@ -240,7 +244,7 @@ static int upload_func(int argc, char *argv[])
return saved_errno;
}
printf("Uploading %s... ", filename);
-rc = xc_livepatch_upload(xch, name, fbuf, len);
+rc = xc_livepatch_upload(xch, name, fbuf, len, force);
if ( rc )
{
rc = errno;
@@ -571,6 +575,19 @@ int main(int argc, char *argv[])
show_help();
return 0;
}
+
+if ( !strncmp("--force", argv[1], strlen("--force")) )
+{
+if ( argc <= 2 )
+{
+show_help();
+return EXIT_FAILURE;
+}
+force = true;
+argv++;
+argc--;
+}
+
for ( i = 0; i < ARRAY_SIZE(main_options); i++ )
if (!strncmp(main_options[i].name, argv[1],
strlen(main_options[i].name)))
diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index 351a3e0b9a60..1503a84457e4 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -2125,7 +2125,8 @@ int livepatch_op(struct
[PATCH v3 0/4] livepatch: minor bug fixes and improvements
Hello, Following series contain some minor bugfixes and improvements for livepatch logic, both inside the hypervisor and on the command line tool. Thanks, Roger. Roger Pau Monne (4): xen-livepatch: fix parameter name parsing livepatch: introduce --force option livepatch: refuse to resolve symbols that belong to init sections x86/livepatch: perform sanity checks on the payload exception table contents tools/include/xenctrl.h| 3 ++- tools/libs/ctrl/xc_misc.c | 7 +++ tools/misc/xen-livepatch.c | 27 +++ xen/arch/x86/extable.c | 18 ++ xen/arch/x86/include/asm/uaccess.h | 4 xen/common/livepatch.c | 25 +++-- xen/common/livepatch_elf.c | 18 +- xen/include/public/sysctl.h| 4 +++- xen/include/xen/livepatch_elf.h| 2 +- 9 files changed, 90 insertions(+), 18 deletions(-) -- 2.44.0
[RFC XEN PATCH v1] xen/public: Add initial files for PV-IOMMU
Hello, I am introducing a proposal for a PV-IOMMU hypercall interface. Some operating systems want to use IOMMU to implement various features (e.g VFIO) or DMA protection. This proposal aims to provide to guests (notably Dom0) a way to access a paravirtualized one. This proposal is based on what presented on xcp-ng blog [1] with some notable changes : - it is now possible to specify a number of contigous pages to map/unmap, it replaces the "sub-operation count" parameter of the hypercall that allowed batching operations but I found it too complex and not really practical - it is now possible from the guest to query PV-IOMMU capabilities (max iova, maximum context number, max pages in a single operation) This patch includes a design document describing the main ideas and a public header of the interface to use. Teddy --- [1] https://xcp-ng.org/blog/2024/04/18/iommu-paravirtualization-for-xen/ Cc: Andrew Cooper Cc: Roger Pau Monné Cc: Jan Beulich Cc: Bertrand Marquis Cc: Rahul Singh --- docs/designs/pv-iommu.md | 105 +++ xen/include/public/pv-iommu.h | 114 ++ 2 files changed, 219 insertions(+) create mode 100644 docs/designs/pv-iommu.md create mode 100644 xen/include/public/pv-iommu.h diff --git a/docs/designs/pv-iommu.md b/docs/designs/pv-iommu.md new file mode 100644 index 00..c01062a3ad --- /dev/null +++ b/docs/designs/pv-iommu.md @@ -0,0 +1,105 @@ +# IOMMU paravirtualization for Dom0 + +Status: Experimental + +# Background + +By default, Xen only uses the IOMMU for itself, either to make device adress +space coherent with guest adress space (x86 HVM/PVH) or to prevent devices +from doing DMA outside it's expected memory regions including the hypervisor +(x86 PV). + +A limitation is that guests (especially privildged ones) may want to use +IOMMU hardware in order to implement features such as DMA protection and +VFIO [1] as IOMMU functionality is not available outside of the hypervisor +currently. + +[1] VFIO - "Virtual Function I/O" - https://www.kernel.org/doc/html/latest/driver-api/vfio.html + +# Design + +The operating system may want to have access to various IOMMU features such as +context management and DMA remapping. We can create a new hypercall that allows +the guest to have access to a new paravirtualized IOMMU interface. + +This feature is only meant to be available for the Dom0, as DomU have some +emulated devices that can't be managed on Xen side and are not hardware, we +can't rely on the hardware IOMMU to enforce DMA remapping. + +This interface is exposed under the `iommu_op` hypercall. + +In addition, Xen domains are modified in order to allow existence of several +IOMMU context including a default one that implement default behavior (e.g +hardware assisted paging) and can't be modified by guest. DomU cannot have +contexts, and therefore act as if they only have the default domain. + +Each IOMMU context within a Xen domain is identified using a domain-specific +context number that is used in the Xen IOMMU subsystem and the hypercall +interface. + +The number of IOMMU context a domain can use is predetermined at domain creation +and is configurable through `dom0-iommu=nb-ctx=N` xen cmdline. + +# IOMMU operations + +## Alloc context + +Create a new IOMMU context for the guest and return the context number to the +guest. +Fail if the IOMMU context limit of the guest is reached. + +A flag can be specified to create a identity mapping. + +## Free context + +Destroy a IOMMU context created previously. +It is not possible to free the default context. + +Reattach context devices to default context if specified by the guest. + +Fail if there is a device in the context and reattach-to-default flag is not +specified. + +## Reattach device + +Reattach a device to another IOMMU context (including the default one). +The target IOMMU context number must be valid and the context allocated. + +The guest needs to specify a PCI SBDF of a device he has access to. + +## Map/unmap page + +Map/unmap a page on a context. +The guest needs to specify a gfn and target dfn to map. + +Refuse to create the mapping if one already exist for the same dfn. + +## Lookup page + +Get the gfn mapped by a specific dfn. + +# Implementation considerations + +## Hypercall batching + +In order to prevent unneeded hypercalls and IOMMU flushing, it is advisable to +be able to batch some critical IOMMU operations (e.g map/unmap multiple pages). + +## Hardware without IOMMU support + +Operating system needs to be aware on PV-IOMMU capability, and whether it is +able to make contexts. However, some operating system may critically fail in +case they are able to make a new IOMMU context. Which is supposed to happen +if no IOMMU hardware is available. + +The hypercall interface needs a interface to advertise the ability to create +and manage IOMMU contexts including the amount of context the guest is able +to use. Using these informations,
[RFC XEN v1] docs/designs: Introduce IOMMU context management
Hello,
This RFC patch introduce a design proposal for changes in the IOMMU subsystem
to allow existence of "IOMMU contexts" (aka hardware IOMMU domains) within
a single Xen domain.
In addition to this, redesign and simplify the existing Xen IOMMU subsystem
with these new uses in mind, using IOMMU contexts as main objects and
reimplementing current features in a more platform agnostic way (e.g
quarantine).
The implementation of this design is needed for the PV-IOMMU interface
implementation. [1]
This design is still incomplete on some aspects (notably platforms that uses
device-tree nodes), and I am looking for some feedback regarding the whole
approach, and if there is anything worth noting I missed out.
Teddy
---
[1] https://xcp-ng.org/blog/2024/04/18/iommu-paravirtualization-for-xen/
Cc: Andrew Cooper
Cc: Roger Pau Monné
Cc: Jan Beulich
Cc: Bertrand Marquis
Cc: Rahul Singh
---
docs/designs/iommu-contexts.md | 373 +
1 file changed, 373 insertions(+)
create mode 100644 docs/designs/iommu-contexts.md
diff --git a/docs/designs/iommu-contexts.md b/docs/designs/iommu-contexts.md
new file mode 100644
index 00..fb9301c398
--- /dev/null
+++ b/docs/designs/iommu-contexts.md
@@ -0,0 +1,373 @@
+# IOMMU context management in Xen
+
+Status: Experimental
+Revision: 0
+
+# Background
+
+The design for *IOMMU paravirtualization for Dom0* [1] explains that some
guests may
+want to access to IOMMU features. In order to implement this in Xen, several
adjustments
+needs to be made to the IOMMU subsystem.
+
+This "hardware IOMMU domain" is currently implemented on a per-domain basis
such as each
+domain actually has a specific *hardware IOMMU domain*, this design aims to
allow a
+single Xen domain to manage several "IOMMU context", and allow some domains
(e.g Dom0
+[1]) to modify their IOMMU contexts.
+
+In addition to this, quarantine feature can be refactored into using IOMMU
contexts
+to reduce the complexity of platform-specific implementations and ensuring more
+consistency across platforms.
+
+# IOMMU context
+
+We define a "IOMMU context" as being a *hardware IOMMU domain*, but named as a
context
+to avoid confusion with Xen domains.
+It represents some hardware-specific data structure that contains mappings
from a device
+frame-number to a machine frame-number (e.g using a pagetable) that can be
applied to
+a device using IOMMU hardware.
+
+This structure is bound to a Xen domain, but a Xen domain may have several
IOMMU context.
+These contexts may be modifiable using the interface as defined in [1] aside
some
+specific cases (e.g modifying default context).
+
+This is implemented in Xen as a new structure that will hold context-specific
+data.
+
+```c
+struct iommu_context {
+u16 id; /* Context id (0 means default context) */
+struct list_head devices;
+
+struct arch_iommu_context arch;
+
+bool opaque; /* context can't be modified nor accessed (e.g HAP) */
+};
+```
+
+A context is identified by a number that is domain-specific and may be used by
IOMMU
+users such as PV-IOMMU by the guest.
+
+struct arch_iommu_context is splited from struct arch_iommu
+
+```c
+struct arch_iommu_context
+{
+spinlock_t pgtables_lock;
+struct page_list_head pgtables;
+
+union {
+/* Intel VT-d */
+struct {
+uint64_t pgd_maddr; /* io page directory machine address */
+domid_t *didmap; /* per-iommu DID */
+unsigned long *iommu_bitmap; /* bitmap of iommu(s) that the
context uses */
+} vtd;
+/* AMD IOMMU */
+struct {
+struct page_info *root_table;
+} amd;
+};
+};
+
+struct arch_iommu
+{
+spinlock_t mapping_lock; /* io page table lock */
+struct {
+struct page_list_head list;
+spinlock_t lock;
+} pgtables;
+
+struct list_head identity_maps;
+
+union {
+/* Intel VT-d */
+struct {
+/* no more context-specific values */
+unsigned int agaw; /* adjusted guest address width, 0 is level 2
30-bit */
+} vtd;
+/* AMD IOMMU */
+struct {
+unsigned int paging_mode;
+struct guest_iommu *g_iommu;
+} amd;
+};
+};
+```
+
+IOMMU context information is now carried by iommu_context rather than being
integrated to
+struct arch_iommu.
+
+# Xen domain IOMMU structure
+
+`struct domain_iommu` is modified to allow multiples context within a single
Xen domain
+to exist :
+
+```c
+struct iommu_context_list {
+uint16_t count; /* Context count excluding default context */
+
+/* if count > 0 */
+
+uint64_t *bitmap; /* bitmap of context allocation */
+struct iommu_context *map; /* Map of contexts */
+};
+
+struct domain_iommu {
+/* ... */
+
+struct iommu_context default_ctx;
+struct iommu_context_list other_contexts;
+
+/* ... */
+}
+```
+
+default_ctx is a special context with id=0 that holds the page table
[linux-5.4 test] 185765: regressions - FAIL
flight 185765 linux-5.4 real [real] http://logs.test-lab.xenproject.org/osstest/logs/185765/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: build-arm64-pvopsbroken in 185433 build-arm64-xsm broken in 185433 build-arm64-libvirt broken in 185433 test-arm64-arm64-libvirt-raw 17 guest-start/debian.repeat fail REGR. vs. 185168 build-arm64-libvirt 5 host-build-prep fail in 185433 REGR. vs. 185168 build-arm64-pvops5 host-build-prep fail in 185433 REGR. vs. 185168 build-arm64-xsm 5 host-build-prep fail in 185433 REGR. vs. 185168 Tests which are failing intermittently (not blocking): test-amd64-i386-qemut-rhel6hvm-amd 14 guest-start/redhat.repeat fail in 185433 pass in 185765 test-amd64-i386-libvirt-raw 12 debian-di-install fail pass in 185433 test-amd64-i386-libvirt-qcow2 12 debian-di-install fail pass in 185433 test-amd64-i386-xl-vhd 12 debian-di-install fail pass in 185433 Tests which did not succeed, but are not blocking: test-arm64-arm64-libvirt-xsm 1 build-check(1) blocked in 185433 n/a test-arm64-arm64-xl-vhd 1 build-check(1) blocked in 185433 n/a test-arm64-arm64-xl-credit2 1 build-check(1) blocked in 185433 n/a test-arm64-arm64-xl-credit1 1 build-check(1) blocked in 185433 n/a test-arm64-arm64-xl-xsm 1 build-check(1) blocked in 185433 n/a test-arm64-arm64-examine 1 build-check(1) blocked in 185433 n/a test-arm64-arm64-xl 1 build-check(1) blocked in 185433 n/a test-arm64-arm64-xl-thunderx 1 build-check(1) blocked in 185433 n/a test-arm64-arm64-libvirt-raw 1 build-check(1) blocked in 185433 n/a test-armhf-armhf-xl-credit1 18 guest-start/debian.repeat fail blocked in 185168 test-armhf-armhf-xl-credit2 19 guest-start.2 fail in 185433 blocked in 185168 test-armhf-armhf-xl-credit1 14 guest-start fail in 185433 like 185168 test-amd64-i386-libvirt-raw 14 migrate-support-check fail in 185433 never pass test-amd64-i386-libvirt-qcow2 14 migrate-support-check fail in 185433 never pass test-armhf-armhf-libvirt 16 saverestore-support-checkfail like 185168 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 185168 test-amd64-i386-xl-qemut-win7-amd64 19 guest-stop fail like 185168 test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 185168 test-armhf-armhf-xl-credit2 18 guest-start/debian.repeatfail like 185168 test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 185168 test-armhf-armhf-xl-multivcpu 18 guest-start/debian.repeatfail like 185168 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 185168 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 185168 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 185168 test-amd64-i386-xl-qemut-ws16-amd64 19 guest-stop fail like 185168 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 185168 test-amd64-i386-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-i386-xl-pvshim14 guest-start fail never pass test-amd64-i386-libvirt 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-arm64-arm64-xl 15 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 15 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 16 saverestore-support-checkfail never pass test-arm64-arm64-xl 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit2 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit1 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 16 saverestore-support-checkfail never pass test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit1 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-rtds 15
Re: [XEN PATCH v2 4/5] xen/arm: allow dynamically assigned SGI handlers
Hi Julien,
> On 23 Apr 2024, at 13:05, Julien Grall wrote:
>
>
>
> On 23/04/2024 10:35, Jens Wiklander wrote:
>> Hi Julien,
>
> Hi Jens,
>
>> On Mon, Apr 22, 2024 at 12:57 PM Julien Grall wrote:
>>>
>>> Hi Jens,
>>>
>>> On 22/04/2024 08:37, Jens Wiklander wrote:
Updates so request_irq() can be used with a dynamically assigned SGI irq
as input. This prepares for a later patch where an FF-A schedule
receiver interrupt handler is installed for an SGI generated by the
secure world.
>>>
>>> I would like to understand the use-case a bit more. Who is responsible
>>> to decide the SGI number? Is it Xen or the firmware?
>>>
>>> If the later, how can we ever guarantee the ID is not going to clash
>>> with what the OS/hypervisor is using? Is it described in a
>>> specification? If so, please give a pointer.
>> The firmware decides the SGI number. Given that the firmware doesn't
>> know which SGIs Xen is using it typically needs to donate one of the
>> secure SGIs, but that is transparent to Xen.
>
> Right this is my concern. The firmware decides the number, but at the same
> time Xen thinks that all the SGIs are available (AFAIK there is only one set).
>
> What I would like to see is some wording from a spec indicating that the SGIs
> ID reserved by the firmware will not be clashing with the one used by Xen.
The idea is that the only SGI reserved for secure are used by the secure world
(in fact it is the SPMC in the secure world who tells us which SGI it will
generate).
So in theory that means it will always use an SGI between 8 and 15.
Now it could make sense in fact to check that the number returned by the
firmware (or SPMC) is not clashing with Xen as it is a recommendation in the
spec and
in fact an implementation might do something different.
Right now there is no spec that will say that it will never clash with the one
used by Xen as the FF-A spec is not enforcing anything here so it would be a
good idea
to check and disable FF-A with a proper error message if this happens.
Cheers
Bertrand
>
>>>
gic_route_irq_to_xen() don't gic_set_irq_type() for SGIs since they are
always edge triggered.
gic_interrupt() is updated to route the dynamically assigned SGIs to
do_IRQ() instead of do_sgi(). The latter still handles the statically
assigned SGI handlers like for instance GIC_SGI_CALL_FUNCTION.
Signed-off-by: Jens Wiklander
---
v1->v2
- Update patch description as requested
---
xen/arch/arm/gic.c | 5 +++--
xen/arch/arm/irq.c | 7 +--
>>>
>>> I am not sure where to write the comment. But I think the comment on top
>>> of irq_set_affinity() in setup_irq() should also be updated.
>>>
2 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 44c40e86defe..e9aeb7138455 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -117,7 +117,8 @@ void gic_route_irq_to_xen(struct irq_desc *desc,
unsigned int priority)
desc->handler = gic_hw_ops->gic_host_irq_type;
-gic_set_irq_type(desc, desc->arch.type);
+if ( desc->irq >= NR_GIC_SGI)
+gic_set_irq_type(desc, desc->arch.type);
>>>
>>> So above, you say that the SGIs are always edge-triggered interrupt. So
>>> I assume desc->arch.type. So are you skipping the call because it is
>>> unnessary or it could do the wrong thing?
>>>
>>> Ideally, the outcome of the answer be part of the comment on top of the
>>> check.
>> gic_set_irq_type() has an assert "ASSERT(type != IRQ_TYPE_INVALID)"
>> which is triggered without this check.
>> So it's both unnecessary and wrong. I suppose we could update the
>> bookkeeping of all SGIs to be edge-triggered instead of
>> IRQ_TYPE_INVALID. It would still be unnecessary though. What do you
>> suggest?
>
> I would rather prefer if we update the book-keeping for all the SGIs.
>
> [...]
>
>>>
{
isb();
do_IRQ(regs, irq, is_fiq);
diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
index bcce80a4d624..fdb214560978 100644
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -224,9 +224,12 @@ void do_IRQ(struct cpu_user_regs *regs, unsigned int
irq, int is_fiq)
perfc_incr(irqs);
-ASSERT(irq >= 16); /* SGIs do not come down this path */
+/* Statically assigned SGIs do not come down this path */
+ASSERT(irq >= GIC_SGI_MAX);
>>>
>>>
>>> With this change, I think the path with vgic_inject_irq() now needs to
>>> gain an ASSERT(irq >= NR_GIC_SGI) because the path is not supposed to be
>>> taken for SGIs.
>> I'm sorry, I don't see the connection. If I add
>> ASSERT(virq >= NR_GIC_SGI);
>> at the top of vgic_inject_irq() it will panic when injecting a
>> Schedule Receiver or Notification Pending Interrupt for a guest.
>
> If you look at
Re: [PATCH v2 1/2] xen: introduce header file with section related symbols
On 19.04.2024 12:02, Roger Pau Monne wrote: > Start by declaring the beginning and end of the init section. > > No functional change intended. > > Requested-by: Andrew Cooper > Signed-off-by: Roger Pau Monné > --- > xen/arch/arm/mmu/setup.c | 3 +-- > xen/arch/x86/setup.c | 3 +-- > xen/include/xen/sections.h | 17 + > 3 files changed, 19 insertions(+), 4 deletions(-) > create mode 100644 xen/include/xen/sections.h Noticing a few things only after committing / pushing: For one, I should have waited for an Arm ack. I hope that's not a big issue. > --- a/xen/arch/arm/mmu/setup.c > +++ b/xen/arch/arm/mmu/setup.c > @@ -7,6 +7,7 @@ > > #include > #include > +#include > #include > #include > > @@ -62,8 +63,6 @@ vaddr_t directmap_virt_start __read_mostly; > unsigned long directmap_base_pdx __read_mostly; > #endif > > -extern char __init_begin[], __init_end[]; Then I wonder why it was this rather than ... > --- a/xen/arch/x86/setup.c > +++ b/xen/arch/x86/setup.c > @@ -5,6 +5,7 @@ > #include > #include > #include > +#include > #include > #include > #include > @@ -309,8 +310,6 @@ void __init discard_initial_images(void) > initial_images = NULL; > } > > -extern unsigned char __init_begin[], __init_end[]; ... this ... > --- /dev/null > +++ b/xen/include/xen/sections.h > @@ -0,0 +1,17 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > + > +#ifndef __XEN_SECTIONS_H__ > +#define __XEN_SECTIONS_H__ > + > +/* SAF-0-safe */ > +extern char __init_begin[], __init_end[]; ... that was moved. "unsigned char" is generally preferred over declaring binary stuff "strings". I further wonder why the opportunity wasn't taken to make both array-of-const. And finally I'm slightly puzzled by the SAF comment appearing with no mention at all in the description; of course I don't mind its addition. Jan
[PATCH v2] x86/xen/pvh: Enable PAE mode for 32-bit guest only when CONFIG_X86_PAE is set
The PVH entry is available for 32-bit KVM guests, and 32-bit KVM guests do not depend on CONFIG_X86_PAE. However, mk_early_pgtbl_32() builds different pagetables depending on whether CONFIG_X86_PAE is set. Therefore, enabling PAE mode for 32-bit KVM guests without CONFIG_X86_PAE being set would result in a boot failure during CR3 loading. Signed-off-by: Hou Wenlong Reviewed-by: Juergen Gross --- v1 -> v2: Add the "Reviewed-by" tag from Juergen v1: https://lore.kernel.org/lkml/8c5448eeebbba998a7fff9ed9b2f7e7f3e437967.1697792461.git.houwenlong@antgroup.com --- arch/x86/platform/pvh/head.S | 2 ++ 1 file changed, 2 insertions(+) diff --git a/arch/x86/platform/pvh/head.S b/arch/x86/platform/pvh/head.S index f7235ef87bc3..18c69d213458 100644 --- a/arch/x86/platform/pvh/head.S +++ b/arch/x86/platform/pvh/head.S @@ -71,10 +71,12 @@ SYM_CODE_START_LOCAL(pvh_start_xen) mov $_pa(early_stack_end), %esp +#if defined(CONFIG_X86_64) || defined(CONFIG_X86_PAE) /* Enable PAE mode. */ mov %cr4, %eax orl $X86_CR4_PAE, %eax mov %eax, %cr4 +#endif #ifdef CONFIG_X86_64 /* Enable Long mode. */ -- 2.31.1
Re: [XEN PATCH v2 4/5] xen/arm: allow dynamically assigned SGI handlers
On 23/04/2024 10:35, Jens Wiklander wrote:
Hi Julien,
Hi Jens,
On Mon, Apr 22, 2024 at 12:57 PM Julien Grall wrote:
Hi Jens,
On 22/04/2024 08:37, Jens Wiklander wrote:
Updates so request_irq() can be used with a dynamically assigned SGI irq
as input. This prepares for a later patch where an FF-A schedule
receiver interrupt handler is installed for an SGI generated by the
secure world.
I would like to understand the use-case a bit more. Who is responsible
to decide the SGI number? Is it Xen or the firmware?
If the later, how can we ever guarantee the ID is not going to clash
with what the OS/hypervisor is using? Is it described in a
specification? If so, please give a pointer.
The firmware decides the SGI number. Given that the firmware doesn't
know which SGIs Xen is using it typically needs to donate one of the
secure SGIs, but that is transparent to Xen.
Right this is my concern. The firmware decides the number, but at the
same time Xen thinks that all the SGIs are available (AFAIK there is
only one set).
What I would like to see is some wording from a spec indicating that the
SGIs ID reserved by the firmware will not be clashing with the one used
by Xen.
gic_route_irq_to_xen() don't gic_set_irq_type() for SGIs since they are
always edge triggered.
gic_interrupt() is updated to route the dynamically assigned SGIs to
do_IRQ() instead of do_sgi(). The latter still handles the statically
assigned SGI handlers like for instance GIC_SGI_CALL_FUNCTION.
Signed-off-by: Jens Wiklander
---
v1->v2
- Update patch description as requested
---
xen/arch/arm/gic.c | 5 +++--
xen/arch/arm/irq.c | 7 +--
I am not sure where to write the comment. But I think the comment on top
of irq_set_affinity() in setup_irq() should also be updated.
2 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 44c40e86defe..e9aeb7138455 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -117,7 +117,8 @@ void gic_route_irq_to_xen(struct irq_desc *desc, unsigned
int priority)
desc->handler = gic_hw_ops->gic_host_irq_type;
-gic_set_irq_type(desc, desc->arch.type);
+if ( desc->irq >= NR_GIC_SGI)
+gic_set_irq_type(desc, desc->arch.type);
So above, you say that the SGIs are always edge-triggered interrupt. So
I assume desc->arch.type. So are you skipping the call because it is
unnessary or it could do the wrong thing?
Ideally, the outcome of the answer be part of the comment on top of the
check.
gic_set_irq_type() has an assert "ASSERT(type != IRQ_TYPE_INVALID)"
which is triggered without this check.
So it's both unnecessary and wrong. I suppose we could update the
bookkeeping of all SGIs to be edge-triggered instead of
IRQ_TYPE_INVALID. It would still be unnecessary though. What do you
suggest?
I would rather prefer if we update the book-keeping for all the SGIs.
[...]
{
isb();
do_IRQ(regs, irq, is_fiq);
diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
index bcce80a4d624..fdb214560978 100644
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -224,9 +224,12 @@ void do_IRQ(struct cpu_user_regs *regs, unsigned int irq,
int is_fiq)
perfc_incr(irqs);
-ASSERT(irq >= 16); /* SGIs do not come down this path */
+/* Statically assigned SGIs do not come down this path */
+ASSERT(irq >= GIC_SGI_MAX);
With this change, I think the path with vgic_inject_irq() now needs to
gain an ASSERT(irq >= NR_GIC_SGI) because the path is not supposed to be
taken for SGIs.
I'm sorry, I don't see the connection. If I add
ASSERT(virq >= NR_GIC_SGI);
at the top of vgic_inject_irq() it will panic when injecting a
Schedule Receiver or Notification Pending Interrupt for a guest.
If you look at do_IRQ(), we have the following code:
if ( test_bit(_IRQ_GUEST, >status) )
{
struct irq_guest *info = irq_get_guest_info(desc);
perfc_incr(guest_irqs);
desc->handler->end(desc);
set_bit(_IRQ_INPROGRESS, >status);
/*
* The irq cannot be a PPI, we only support delivery of SPIs to
* guests.
*/
vgic_inject_irq(info->d, NULL, info->virq, true);
goto out_no_end;
}
What I suggesting is to add an ASSERT(irq >= NR_GIC_SGI) just before the
call because now do_IRQ() can be called with SGIs yet we don't allow HW
SGIs to assigned to a guest.
Cheers,
--
Julien Grall
Re: [PATCH 1/6] x86: Introduce x86_decode_lite()
On 23.04.2024 12:27, Andrew Cooper wrote:
> On 23/04/2024 10:17 am, Jan Beulich wrote:
>> On 22.04.2024 20:14, Andrew Cooper wrote:
>>> --- /dev/null
>>> +++ b/xen/arch/x86/x86_emulate/decode-lite.c
>>> @@ -0,0 +1,245 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>> +
>>> +#include "private.h"
>>> +
>>> +#define Imm8 (1 << 0)
>>> +#define Imm(1 << 1)
>>> +#define Branch (1 << 5) /* ... that we care about */
>>> +/* ModRM (1 << 6) */
>>> +#define Known (1 << 7)
>>> +
>>> +#define ALU_OPS \
>>> +(Known|ModRM), \
>>> +(Known|ModRM), \
>>> +(Known|ModRM), \
>>> +(Known|ModRM), \
>>> +(Known|Imm8), \
>>> +(Known|Imm)
>>> +
>>> +static const uint8_t init_or_livepatch_const onebyte[256] = {
>>> +[0x00] = ALU_OPS, /* ADD */ [0x08] = ALU_OPS, /* OR */
>>> +[0x10] = ALU_OPS, /* ADC */ [0x18] = ALU_OPS, /* SBB */
>>> +[0x20] = ALU_OPS, /* AND */ [0x28] = ALU_OPS, /* SUB */
>>> +[0x30] = ALU_OPS, /* XOR */ [0x38] = ALU_OPS, /* CMP */
>>> +
>>> +[0x50 ... 0x5f] = (Known), /* PUSH/POP %reg */
>>> +
>>> +[0x70 ... 0x7f] = (Known|Branch|Imm8), /* Jcc disp8 */
>>> +[0x80] = (Known|ModRM|Imm8),
>>> +[0x81] = (Known|ModRM|Imm),
>>> +
>>> +[0x83] = (Known|ModRM|Imm8),
>>> +[0x84 ... 0x8e] = (Known|ModRM), /* TEST/XCHG/MOV/MOV-SREG/LEA
>>> r/rm */
>>> +
>>> +[0xb0 ... 0xb7] = (Known|Imm8),/* MOV $imm8, %reg */
>> I'm surprised you get away without at least NOP also marked as known.
>> Imo the whole 0x90 row should be marked so.
>>
>> Similarly I'm not convinced that leaving the 0xA0 row unpopulated is a
>> good idea. It's at best a trap set up for somebody to fall into rather
>> sooner than later.
>>
>>> +[0xb8 ... 0xbf] = (Known|Imm), /* MOV $imm32, %reg */
>>> +
>>> +[0xcc] = (Known), /* INT3 */
>>> +[0xcd] = (Known|Imm8),/* INT $imm8 */
>> Like above, what about in particular any of the shifts/rotates and the
>> MOV that's in the 0xC0 row?
>>
>> While the last sentence in the description is likely meant to cover
>> that, I think the description wants to go further as to any pretty
>> common but omitted insns. Already "x86: re-work memset()" and "x86: re-
>> work memcpy()" (v2 pending for, soon, 3 years) would make it necessary
>> to touch this table, thus increasing complexity of those changes to an
>> area they shouldn't be concerned about at all.
>>
>>> +[0xe8 ... 0xe9] = (Known|Branch|Imm), /* CALL/JMP disp32 */
>>> +[0xeb] = (Known|Branch|Imm8), /* JMP disp8 */
>> 0xe0 ... 0xe7 and 0xec ... 0xef would imo also better be covered, as
>> they easily can be (much like you also cover e.g. CMC despite it
>> appearing pretty unlikely that we use that insn anywhere).
>>
>>> +[0xf1] = (Known), /* ICEBP */
>>> +[0xf4] = (Known), /* HLT */
>>> +[0xf5] = (Known), /* CMC */
>>> +[0xf6 ... 0xf7] = (Known|ModRM), /* Grp3 */
>>> +[0xf8 ... 0xfd] = (Known), /* CLC ... STD */
>>> +[0xfe ... 0xff] = (Known|ModRM), /* Grp4 */
>>> +};
>>> +static const uint8_t init_or_livepatch_const twobyte[256] = {
>>> +[0x00 ... 0x01] = (Known|ModRM), /* Grp6/Grp7 */
>> LAR / LSL? CLTS? WBINVD? UD2?
>>
>>> +[0x18 ... 0x1f] = (Known|ModRM), /* Grp16 (Hint Nop) */
>>> +
>>> +[0x20 ... 0x23] = (Known|ModRM), /* MOV CR/DR */
>>> +
>>> +[0x30 ... 0x34] = (Known), /* WRMSR ... RDPMC */
>> 0x34 is SYSENTER, isn't it?
>>
>>> +[0x40 ... 0x4f] = (Known|ModRM), /* CMOVcc */
>>> +
>>> +[0x80 ... 0x8f] = (Known|Branch|Imm), /* Jcc disp32 */
>> What about things like VMREAD/VMWRITE?
>>
>>> +[0x90 ... 0x9f] = (Known|ModRM), /* SETcc */
>> PUSH/POP fs/gs? CPUID?
>>
>>> +[0xab] = (Known|ModRM), /* BTS */
>> BTS (and BTC below) but not BT and BTR?
>>
>>> +[0xac] = (Known|ModRM|Imm8), /* SHRD $imm8 */
>>> +[0xad ... 0xaf] = (Known|ModRM), /* SHRD %cl / Grp15 / IMUL */
>> SHRD but not SHLD?
>>
>> CMPXCHG?
>>
>>> +[0xb8 ... 0xb9] = (Known|ModRM), /* POPCNT/Grp10 (UD1) */
>>> +[0xba] = (Known|ModRM|Imm8), /* Grp8 */
>>> +[0xbb ... 0xbf] = (Known|ModRM), /* BSR/BSF/BSR/MOVSX */
>> Nit (comment only): 0xbb is BTC.
>>
>> MOVSX but not MOVZX and also not MOVSXD (in the 1-byte table)?
>>
>>> +};
>> XADD, MOVNTI, and the whole 0xc7-based group?
>
> As you may have guessed, I filled in the opcode table until I could
> parse all replacements.
>
> When, at the end of this, I didn't need the osize=8 movs, I took the
> decoding complexity back out.
While I can see that this requiring extra logic makes it desirable to
leave out, it'll
