RE: [PATCH V2 1/2] xen/arm: Add i.MX lpuart driver
Hi Peng,
> -Original Message-
> From: Xen-devel On Behalf Of
> Peng Fan (OSS)
> Sent: Saturday, April 2, 2022 1:42 PM
> To: sstabell...@kernel.org; jul...@xen.org; volodymyr_babc...@epam.com;
> Bertrand Marquis
> Cc: andrew.coop...@citrix.com; george.dun...@citrix.com;
> jbeul...@suse.com; w...@xen.org; xen-devel@lists.xenproject.org;
> van.free...@gmail.com; Peng Fan
> Subject: [PATCH V2 1/2] xen/arm: Add i.MX lpuart driver
>
> From: Peng Fan
>
> The i.MX LPUART Documentation:
> https://www.nxp.com/webapp/Download?colCode=IMX8QMIEC
> Chatper 13.6 Low Power Universal Asynchronous Receiver/
> Transmitter (LPUART)
>
> Signed-off-by: Peng Fan
For the whole series of patch, I tested that the series will not break
current arm64 boot of Xen with and without the CONFIG_HAS_IMX_LPUART.
Unfortunately I cannot test the functionality as I do not have the board :))
So, for this series:
Tested-by: Henry Wang
> ---
> xen/drivers/char/Kconfig | 8 +
> xen/drivers/char/Makefile | 1 +
> xen/drivers/char/imx-lpuart.c | 275
> ++
> xen/include/xen/imx-lpuart.h | 64
> 4 files changed, 348 insertions(+)
> create mode 100644 xen/drivers/char/imx-lpuart.c
> create mode 100644 xen/include/xen/imx-lpuart.h
>
> diff --git a/xen/drivers/char/Kconfig b/xen/drivers/char/Kconfig
> index 2ff5b288e2..0efdb2128f 100644
> --- a/xen/drivers/char/Kconfig
> +++ b/xen/drivers/char/Kconfig
> @@ -13,6 +13,14 @@ config HAS_CADENCE_UART
> This selects the Xilinx Zynq Cadence UART. If you have a Xilinx Zynq
> based board, say Y.
>
> +config HAS_IMX_LPUART
> + bool "i.MX LPUART driver"
> + default y
> + depends on ARM_64
> + help
> + This selects the i.MX LPUART. If you have a i.MX8QM based board,
> + say Y.
> +
> config HAS_MVEBU
> bool "Marvell MVEBU UART driver"
> default y
> diff --git a/xen/drivers/char/Makefile b/xen/drivers/char/Makefile
> index 7c646d771c..14e67cf072 100644
> --- a/xen/drivers/char/Makefile
> +++ b/xen/drivers/char/Makefile
> @@ -8,6 +8,7 @@ obj-$(CONFIG_HAS_MVEBU) += mvebu-uart.o
> obj-$(CONFIG_HAS_OMAP) += omap-uart.o
> obj-$(CONFIG_HAS_SCIF) += scif-uart.o
> obj-$(CONFIG_HAS_EHCI) += ehci-dbgp.o
> +obj-$(CONFIG_HAS_IMX_LPUART) += imx-lpuart.o
> obj-$(CONFIG_ARM) += arm-uart.o
> obj-y += serial.o
> obj-$(CONFIG_XEN_GUEST) += xen_pv_console.o
> diff --git a/xen/drivers/char/imx-lpuart.c b/xen/drivers/char/imx-lpuart.c
> new file mode 100644
> index 00..49330fd2f8
> --- /dev/null
> +++ b/xen/drivers/char/imx-lpuart.c
> @@ -0,0 +1,275 @@
> +/*
> + * xen/drivers/char/imx-lpuart.c
> + *
> + * Driver for i.MX LPUART.
> + *
> + * Peng Fan
> + * Copyright 2022 NXP
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define imx_lpuart_read(uart, off) readl((uart)->regs + off)
> +#define imx_lpuart_write(uart, off, val) writel((val), (uart)->regs + off)
> +
> +static struct imx_lpuart {
> +uint32_t baud, clock_hz, data_bits, parity, stop_bits, fifo_size;
> +uint32_t irq;
> +char __iomem *regs;
> +struct irqaction irqaction;
> +struct vuart_info vuart;
> +} imx8_com;
> +
> +static void imx_lpuart_interrupt(int irq, void *data,
> + struct cpu_user_regs *regs)
> +{
> +struct serial_port *port = data;
> +struct imx_lpuart *uart = port->uart;
> +uint32_t sts, rxcnt;
> +
> +sts = imx_lpuart_read(uart, UARTSTAT);
> +rxcnt = imx_lpuart_read(uart, UARTWATER) >> UARTWATER_RXCNT_OFF;
> +
> +if ( (sts & UARTSTAT_RDRF) || (rxcnt > 0) )
> + serial_rx_interrupt(port, regs);
> +
> +if ( sts & UARTSTAT_TDRE )
> + serial_tx_interrupt(port, regs);
> +
> +imx_lpuart_write(uart, UARTSTAT, sts);
> +}
> +
> +static void __init imx_lpuart_init_preirq(struct serial_port *port)
> +{
> +struct imx_lpuart *uart = port->uart;
> +uint32_t ctrl, old_ctrl, bd;
> +
> +ctrl = old_ctrl = imx_lpuart_read(uart, UARTCTRL);
> +ctrl = (old_ctrl & ~UARTCTRL_M) | UARTCTRL_TE | UARTCTRL_RE;
> +bd = imx_lpuart_read(uart, UARTBAUD);
> +
> +while ( !(imx_lpuart_read(uart, UARTSTAT) & UARTSTAT_TC) )
> + cpu_relax();
> +
> +/* Disable transmit and receive */
> +imx_lpuart_write(uart, UARTCTRL, old_ctrl & ~(UARTCTRL_TE |
> UARTCTRL_RE));
> +
> +/* Reuse firmware baudrate settings, only di
[PATCH V2 1/2] xen/arm: Add i.MX lpuart driver
From: Peng Fan
The i.MX LPUART Documentation:
https://www.nxp.com/webapp/Download?colCode=IMX8QMIEC
Chatper 13.6 Low Power Universal Asynchronous Receiver/
Transmitter (LPUART)
Signed-off-by: Peng Fan
---
xen/drivers/char/Kconfig | 8 +
xen/drivers/char/Makefile | 1 +
xen/drivers/char/imx-lpuart.c | 275 ++
xen/include/xen/imx-lpuart.h | 64
4 files changed, 348 insertions(+)
create mode 100644 xen/drivers/char/imx-lpuart.c
create mode 100644 xen/include/xen/imx-lpuart.h
diff --git a/xen/drivers/char/Kconfig b/xen/drivers/char/Kconfig
index 2ff5b288e2..0efdb2128f 100644
--- a/xen/drivers/char/Kconfig
+++ b/xen/drivers/char/Kconfig
@@ -13,6 +13,14 @@ config HAS_CADENCE_UART
This selects the Xilinx Zynq Cadence UART. If you have a Xilinx Zynq
based board, say Y.
+config HAS_IMX_LPUART
+ bool "i.MX LPUART driver"
+ default y
+ depends on ARM_64
+ help
+ This selects the i.MX LPUART. If you have a i.MX8QM based board,
+ say Y.
+
config HAS_MVEBU
bool "Marvell MVEBU UART driver"
default y
diff --git a/xen/drivers/char/Makefile b/xen/drivers/char/Makefile
index 7c646d771c..14e67cf072 100644
--- a/xen/drivers/char/Makefile
+++ b/xen/drivers/char/Makefile
@@ -8,6 +8,7 @@ obj-$(CONFIG_HAS_MVEBU) += mvebu-uart.o
obj-$(CONFIG_HAS_OMAP) += omap-uart.o
obj-$(CONFIG_HAS_SCIF) += scif-uart.o
obj-$(CONFIG_HAS_EHCI) += ehci-dbgp.o
+obj-$(CONFIG_HAS_IMX_LPUART) += imx-lpuart.o
obj-$(CONFIG_ARM) += arm-uart.o
obj-y += serial.o
obj-$(CONFIG_XEN_GUEST) += xen_pv_console.o
diff --git a/xen/drivers/char/imx-lpuart.c b/xen/drivers/char/imx-lpuart.c
new file mode 100644
index 00..49330fd2f8
--- /dev/null
+++ b/xen/drivers/char/imx-lpuart.c
@@ -0,0 +1,275 @@
+/*
+ * xen/drivers/char/imx-lpuart.c
+ *
+ * Driver for i.MX LPUART.
+ *
+ * Peng Fan
+ * Copyright 2022 NXP
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define imx_lpuart_read(uart, off) readl((uart)->regs + off)
+#define imx_lpuart_write(uart, off, val) writel((val), (uart)->regs + off)
+
+static struct imx_lpuart {
+uint32_t baud, clock_hz, data_bits, parity, stop_bits, fifo_size;
+uint32_t irq;
+char __iomem *regs;
+struct irqaction irqaction;
+struct vuart_info vuart;
+} imx8_com;
+
+static void imx_lpuart_interrupt(int irq, void *data,
+ struct cpu_user_regs *regs)
+{
+struct serial_port *port = data;
+struct imx_lpuart *uart = port->uart;
+uint32_t sts, rxcnt;
+
+sts = imx_lpuart_read(uart, UARTSTAT);
+rxcnt = imx_lpuart_read(uart, UARTWATER) >> UARTWATER_RXCNT_OFF;
+
+if ( (sts & UARTSTAT_RDRF) || (rxcnt > 0) )
+ serial_rx_interrupt(port, regs);
+
+if ( sts & UARTSTAT_TDRE )
+ serial_tx_interrupt(port, regs);
+
+imx_lpuart_write(uart, UARTSTAT, sts);
+}
+
+static void __init imx_lpuart_init_preirq(struct serial_port *port)
+{
+struct imx_lpuart *uart = port->uart;
+uint32_t ctrl, old_ctrl, bd;
+
+ctrl = old_ctrl = imx_lpuart_read(uart, UARTCTRL);
+ctrl = (old_ctrl & ~UARTCTRL_M) | UARTCTRL_TE | UARTCTRL_RE;
+bd = imx_lpuart_read(uart, UARTBAUD);
+
+while ( !(imx_lpuart_read(uart, UARTSTAT) & UARTSTAT_TC) )
+ cpu_relax();
+
+/* Disable transmit and receive */
+imx_lpuart_write(uart, UARTCTRL, old_ctrl & ~(UARTCTRL_TE | UARTCTRL_RE));
+
+/* Reuse firmware baudrate settings, only disable DMA here */
+bd &= ~(UARTBAUD_TDMAE | UARTBAUD_RDMAE);
+
+imx_lpuart_write(uart, UARTMODIR, 0);
+imx_lpuart_write(uart, UARTBAUD, bd);
+imx_lpuart_write(uart, UARTCTRL, ctrl);
+}
+
+static void __init imx_lpuart_init_postirq(struct serial_port *port)
+{
+struct imx_lpuart *uart = port->uart;
+uint32_t temp;
+
+uart->irqaction.handler = imx_lpuart_interrupt;
+uart->irqaction.name = "imx_lpuart";
+uart->irqaction.dev_id = port;
+
+if ( setup_irq(uart->irq, 0, &uart->irqaction) != 0 )
+{
+dprintk(XENLOG_ERR, "Failed to allocate imx_lpuart IRQ %d\n",
+uart->irq);
+return;
+}
+
+/* Enable interrupts */
+temp = imx_lpuart_read(uart, UARTCTRL);
+temp |= (UARTCTRL_RIE | UARTCTRL_TIE);
+temp |= UARTCTRL_ILIE;
+imx_lpuart_write(uart, UARTCTRL, temp);
+}
+
+static void imx_lpuart_suspend(struct serial_port *port)
+{
+BUG();
+}
+
+s
[PATCH V2 2/2] xen/arm: Add i.MX lpuart early printk support
From: Peng Fan Signed-off-by: Peng Fan --- xen/arch/arm/Kconfig.debug | 14 +++ xen/arch/arm/arm64/debug-imx-lpuart.inc | 52 + 2 files changed, 66 insertions(+) create mode 100644 xen/arch/arm/arm64/debug-imx-lpuart.inc diff --git a/xen/arch/arm/Kconfig.debug b/xen/arch/arm/Kconfig.debug index 35ccd13273..842d768280 100644 --- a/xen/arch/arm/Kconfig.debug +++ b/xen/arch/arm/Kconfig.debug @@ -58,6 +58,16 @@ choice This option is preferred over the platform specific options; the platform specific options are deprecated and will soon be removed. + config EARLY_UART_CHOICE_IMX_LPUART + select EARLY_UART_IMX_LPUART + depends on ARM_64 + bool "Early printk via i.MX LPUART" + help + Say Y here if you wish the early printk to direct their + output to a i.MX LPUART. You can use this option to + provide the parameters for the i.MX LPUART rather than + selecting one of the platform specific options below if + you know the parameters for the port. config EARLY_UART_CHOICE_MESON select EARLY_UART_MESON depends on ARM_64 @@ -186,6 +196,9 @@ config EARLY_UART_CADENCE config EARLY_UART_EXYNOS4210 select EARLY_PRINTK bool +config EARLY_UART_IMX_LPUART + select EARLY_PRINTK + bool config EARLY_UART_MESON select EARLY_PRINTK bool @@ -283,6 +296,7 @@ config EARLY_PRINTK_INC default "debug-8250.inc" if EARLY_UART_8250 default "debug-cadence.inc" if EARLY_UART_CADENCE default "debug-exynos4210.inc" if EARLY_UART_EXYNOS4210 + default "debug-imx-lpuart.inc" if EARLY_UART_IMX_LPUART default "debug-meson.inc" if EARLY_UART_MESON default "debug-mvebu.inc" if EARLY_UART_MVEBU default "debug-pl011.inc" if EARLY_UART_PL011 diff --git a/xen/arch/arm/arm64/debug-imx-lpuart.inc b/xen/arch/arm/arm64/debug-imx-lpuart.inc new file mode 100644 index 00..f7ac78a781 --- /dev/null +++ b/xen/arch/arm/arm64/debug-imx-lpuart.inc @@ -0,0 +1,52 @@ +/* + * xen/arch/arm/arm64/debug-imx8qm.inc + * + * i.MX8QM specific debug code + * + * Peng Fan + * Copyright (C) 2016 Freescale Inc. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include + +.macro early_uart_init wb wc wd +/* Already initialized in bootloader */ +.endm + +/* + * Wait LPUART to be ready to transmit + * rb: register which contains the UART base address + * rc: scratch register + */ +.macro early_uart_ready xb, c +1: +ldr w\c, [\xb, #UARTSTAT] /* <- Flag register */ +tst w\c, #UARTSTAT_TDRE /* Check FIFO EMPTY bit */ +beq 1b /* Wait for the UART to be ready */ +.endm + +/* + * LPUART transmit character + * rb: register which contains the UART base address + * rt: register which contains the character to transmit + */ +.macro early_uart_transmit xb, wt +str \wt, [\xb, #UARTDATA] /* -> Data Register */ +.endm + +/* + * Local variables: + * mode: ASM + * indent-tabs-mode: nil + * End: + */ -- 2.35.1
[PATCH V2 0/2] xen/arm: add i.MX lpuart and i.MX8QM initial support
From: Peng Fan V2: Per Julien's comments, fix coding style issue, drop unneeded code Add i.MX lpuart driver and i.MX8QM platform support. - lpuart is the uart IP used in i.MX8QM/QXP/93. - Very basic i.MX8QM platform support. Peng Fan (2): xen/arm: Add i.MX lpuart driver xen/arm: Add i.MX lpuart early printk support xen/arch/arm/Kconfig.debug | 14 ++ xen/arch/arm/arm64/debug-imx-lpuart.inc | 52 + xen/drivers/char/Kconfig| 8 + xen/drivers/char/Makefile | 1 + xen/drivers/char/imx-lpuart.c | 275 xen/include/xen/imx-lpuart.h| 64 ++ 6 files changed, 414 insertions(+) create mode 100644 xen/arch/arm/arm64/debug-imx-lpuart.inc create mode 100644 xen/drivers/char/imx-lpuart.c create mode 100644 xen/include/xen/imx-lpuart.h -- 2.35.1
[xen-unstable test] 169106: tolerable FAIL - PUSHED
flight 169106 xen-unstable real [real] http://logs.test-lab.xenproject.org/osstest/logs/169106/ Failures :-/ but no regressions. Regressions which are regarded as allowable (not blocking): test-armhf-armhf-xl-rtds 14 guest-start fail REGR. vs. 169063 Tests which did not succeed, but are not blocking: test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 169063 test-armhf-armhf-libvirt 16 saverestore-support-checkfail like 169063 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 169063 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 169063 test-amd64-i386-xl-qemut-ws16-amd64 19 guest-stop fail like 169063 test-amd64-i386-xl-qemut-win7-amd64 19 guest-stop fail like 169063 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check fail like 169063 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail like 169063 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 169063 test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 169063 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 169063 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 169063 test-arm64-arm64-xl-seattle 15 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 16 saverestore-support-checkfail never pass test-amd64-i386-libvirt-xsm 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-amd64-libvirt 15 migrate-support-checkfail never pass test-amd64-i386-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-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-thunderx 15 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 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-credit1 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 16 saverestore-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-amd64-i386-libvirt-raw 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-armhf-armhf-xl-cubietruck 15 migrate-support-checkfail never pass test-armhf-armhf-xl-cubietruck 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-amd64-amd64-libvirt-vhd 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-libvirt 15 migrate-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-arm64-arm64-xl-vhd 14 migrate-support-checkfail never pass test-arm64-arm64-xl-vhd 15 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-libvirt-qcow2 14 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-armhf-armhf-xl-vhd 14 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 15 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-raw 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 version targeted for testing: xen e7cfcdc6719d586eb7cdb62d40275a7d17fe6760 baseline version: xen 8eec96b7b8d937d40e2e7988edb8bbd0859
[linux-linus test] 169100: tolerable FAIL - PUSHED
flight 169100 linux-linus real [real] flight 169121 linux-linus real-retest [real] http://logs.test-lab.xenproject.org/osstest/logs/169100/ http://logs.test-lab.xenproject.org/osstest/logs/169121/ Failures :-/ but no regressions. Tests which are failing intermittently (not blocking): test-amd64-amd64-libvirt-qcow2 8 xen-boot fail pass in 169121-retest Tests which did not succeed, but are not blocking: test-amd64-amd64-libvirt-qcow2 14 migrate-support-check fail in 169121 never pass test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 169055 test-armhf-armhf-libvirt 16 saverestore-support-checkfail like 169055 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 169055 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 169055 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 169055 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 169055 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check fail like 169055 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail like 169055 test-arm64-arm64-xl-seattle 15 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 16 saverestore-support-checkfail never pass 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-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-thunderx 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-thunderx 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-xl 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 16 saverestore-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-amd64-amd64-libvirt-raw 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-armhf-armhf-xl 15 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 15 migrate-support-checkfail never pass test-armhf-armhf-xl 16 saverestore-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-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-credit1 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 16 saverestore-support-checkfail never pass test-armhf-armhf-libvirt 15 migrate-support-checkfail never pass test-armhf-armhf-xl-cubietruck 15 migrate-support-checkfail never pass test-armhf-armhf-xl-cubietruck 16 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-qcow2 14 migrate-support-checkfail never pass test-armhf-armhf-libvirt-raw 14 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-armhf-armhf-xl-vhd 14 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 15 saverestore-support-checkfail never pass version targeted for testing: linuxe8b767f5e04097aaedcd6e06e2270f9fe5282696 baseline version: linux787af64d05cd528aac9ad16752d11bb1c6061bb9 Last test of basis 169055 2022-03-31 02:32:32 Z1 days Testing same since 169100 2022-04-01 04:55:02 Z0 days1 attempts People who touched revisions under test: Akira Yokosawa Alexander Lobakin Alexei Starovoitov Alistair Francis Andreas Gruenbacher Andrew Melnychenko Andrew Price And
Re: [PATCH v3 10/19] xen/arm32: mm: Re-implement setup_xenheap_mappings() using map_pages_to_xen()
On Mon, 21 Feb 2022, Julien Grall wrote:
> From: Julien Grall
>
> Now that map_pages_to_xen() has been extended to support 2MB mappings,
> we can replace the create_mappings() call by map_pages_to_xen() call.
>
> Signed-off-by: Julien Grall
Reviewed-by: Stefano Stabellini
> ---
> Changes in v3:
> - Fix build when CONFIG_DEBUG=y
>
> Changes in v2:
> - New patch
>
> TODOs:
> - add support for contiguous mapping
> ---
> xen/arch/arm/mm.c | 7 ++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index f18f65745595..1e5c2c45dcf9 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -809,7 +809,12 @@ void mmu_init_secondary_cpu(void)
> void __init setup_xenheap_mappings(unsigned long base_mfn,
> unsigned long nr_mfns)
> {
> -create_mappings(xen_second, XENHEAP_VIRT_START, base_mfn, nr_mfns,
> MB(32));
> +int rc;
> +
> +rc = map_pages_to_xen(XENHEAP_VIRT_START, _mfn(base_mfn), nr_mfns,
> + PAGE_HYPERVISOR_RW | _PAGE_BLOCK);
> +if ( rc )
> +panic("Unable to setup the xenheap mappings.\n");
>
> /* Record where the xenheap is, for translation routines. */
> xenheap_virt_end = XENHEAP_VIRT_START + nr_mfns * PAGE_SIZE;
> --
> 2.32.0
>
Re: [PATCH v3 08/19] xen/arm: mm: Re-implement early_fdt_map() using map_pages_to_xen()
On Mon, 21 Feb 2022, Julien Grall wrote:
> From: Julien Grall
>
> Now that map_pages_to_xen() has been extended to support 2MB mappings,
> we can replace the create_mappings() calls by map_pages_to_xen() calls.
>
> The mapping can also be marked read-only has Xen as no business to
> modify the host Device Tree.
>
> Signed-off-by: Julien Grall
> Signed-off-by: Julien Grall
>
> ---
> Changes in v2:
> - Add my AWS signed-off-by
> - Fix typo in the commit message
> ---
> xen/arch/arm/mm.c | 18 +-
> 1 file changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index f088a4b2de96..24de8dcb9042 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -559,6 +559,7 @@ void * __init early_fdt_map(paddr_t fdt_paddr)
> paddr_t offset;
> void *fdt_virt;
> uint32_t size;
> +int rc;
>
> /*
> * Check whether the physical FDT address is set and meets the minimum
> @@ -574,8 +575,12 @@ void * __init early_fdt_map(paddr_t fdt_paddr)
> /* The FDT is mapped using 2MB superpage */
> BUILD_BUG_ON(BOOT_FDT_VIRT_START % SZ_2M);
>
> -create_mappings(xen_second, BOOT_FDT_VIRT_START,
> paddr_to_pfn(base_paddr),
> -SZ_2M >> PAGE_SHIFT, SZ_2M);
> +rc = map_pages_to_xen(BOOT_FDT_VIRT_START, maddr_to_mfn(base_paddr),
> + SZ_2M >> PAGE_SHIFT,
> + PAGE_HYPERVISOR_RO | _PAGE_BLOCK);
> +if ( rc )
> +panic("Unable to map the device-tree.\n");
> +
>
> offset = fdt_paddr % SECOND_SIZE;
> fdt_virt = (void *)BOOT_FDT_VIRT_START + offset;
> @@ -589,9 +594,12 @@ void * __init early_fdt_map(paddr_t fdt_paddr)
>
> if ( (offset + size) > SZ_2M )
> {
> -create_mappings(xen_second, BOOT_FDT_VIRT_START + SZ_2M,
> -paddr_to_pfn(base_paddr + SZ_2M),
> -SZ_2M >> PAGE_SHIFT, SZ_2M);
> +rc = map_pages_to_xen(BOOT_FDT_VIRT_START + SZ_2M,
> + maddr_to_mfn(base_paddr + SZ_2M),
> + SZ_2M >> PAGE_SHIFT,
> + PAGE_HYPERVISOR_RO | _PAGE_BLOCK);
> +if ( rc )
> +panic("Unable to map the device-tree\n");
> }
Very good! :-)
I have a small preference for making the change to PAGE_HYPERVISOR_RO in
a separate patch because it would make it easier to revert in the
future if we need so (e.g. overlays...). But it is OK either way.
Re: [PATCH v3 07/19] xen/arm: mm: Don't open-code Xen PT update in remove_early_mappings()
On Mon, 21 Feb 2022, Julien Grall wrote:
> From: Julien Grall
>
> Now that xen_pt_update_entry() is able to deal with different mapping
> size, we can replace the open-coding of the page-tables update by a call
> to modify_xen_mappings().
>
> As the function is not meant to fail, a BUG_ON() is added to check the
> return.
>
> Signed-off-by: Julien Grall
> Signed-off-by: Julien Grall
Nice!
> ---
> Changes in v2:
> - Stay consistent with how function name are used in the commit
> message
> - Add my AWS signed-off-by
> ---
> xen/arch/arm/mm.c | 10 +-
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 7b4b9de8693e..f088a4b2de96 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -599,11 +599,11 @@ void * __init early_fdt_map(paddr_t fdt_paddr)
>
> void __init remove_early_mappings(void)
> {
> -lpae_t pte = {0};
> -write_pte(xen_second + second_table_offset(BOOT_FDT_VIRT_START), pte);
> -write_pte(xen_second + second_table_offset(BOOT_FDT_VIRT_START + SZ_2M),
> - pte);
> -flush_xen_tlb_range_va(BOOT_FDT_VIRT_START, BOOT_FDT_SLOT_SIZE);
> +int rc;
> +
> +rc = modify_xen_mappings(BOOT_FDT_VIRT_START, BOOT_FDT_VIRT_END,
> + _PAGE_BLOCK);
> +BUG_ON(rc);
Am I right that we are actually destroying the mapping, which usually is
done by calling destroy_xen_mappings, but we cannot call
destroy_xen_mappings in this case because it doesn't take a flags
parameter?
If so, then I would add a flags parameter to destroy_xen_mappings
instead of calling modify_xen_mappings just to pass _PAGE_BLOCK.
But I don't feel strongly about it so if you don't feel like making the
change to destroy_xen_mappings, you can add my acked-by here anyway.
Re: [PATCH v3 06/19] xen/arm: mm: Avoid flushing the TLBs when mapping are inserted
On Mon, 21 Feb 2022, Julien Grall wrote:
> From: Julien Grall
>
> Currently, the function xen_pt_update() will flush the TLBs even when
> the mappings are inserted. This is a bit wasteful because we don't
> allow mapping replacement. Even if we were, the flush would need to
> happen earlier because mapping replacement should use Break-Before-Make
> when updating the entry.
>
> A single call to xen_pt_update() can perform a single action. IOW, it
> is not possible to, for instance, mix inserting and removing mappings.
> Therefore, we can use `flags` to determine what action is performed.
>
> This change will be particularly help to limit the impact of switching
> boot time mapping to use xen_pt_update().
>
> Signed-off-by: Julien Grall
>
> ---
> Changes in v2:
> - New patch
> ---
> xen/arch/arm/mm.c | 17 ++---
> 1 file changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index fd16c1541ce2..7b4b9de8693e 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -1104,7 +1104,13 @@ static bool xen_pt_check_entry(lpae_t entry, mfn_t
> mfn, unsigned int level,
> /* We should be here with a valid MFN. */
> ASSERT(!mfn_eq(mfn, INVALID_MFN));
>
> -/* We don't allow replacing any valid entry. */
> +/*
> + * We don't allow replacing any valid entry.
> + *
> + * Note that the function xen_pt_update() relies on this
> + * assumption and will skip the TLB flush. The function will need
> + * to be updated if the check is relaxed.
> + */
> if ( lpae_is_valid(entry) )
> {
> if ( lpae_is_mapping(entry, level) )
> @@ -1417,11 +1423,16 @@ static int xen_pt_update(unsigned long virt,
> }
>
> /*
> - * Flush the TLBs even in case of failure because we may have
> + * The TLBs flush can be safely skipped when a mapping is inserted
> + * as we don't allow mapping replacement (see xen_pt_check_entry()).
> + *
> + * For all the other cases, the TLBs will be flushed unconditionally
> + * even if the mapping has failed. This is because we may have
> * partially modified the PT. This will prevent any unexpected
> * behavior afterwards.
> */
> -flush_xen_tlb_range_va(virt, PAGE_SIZE * nr_mfns);
> +if ( !(flags & _PAGE_PRESENT) || mfn_eq(mfn, INVALID_MFN) )
> +flush_xen_tlb_range_va(virt, PAGE_SIZE * nr_mfns);
I am trying to think of a care where the following wouldn't be enough
but I cannot come up with one:
if ( mfn_eq(mfn, INVALID_MFN) )
flush_xen_tlb_range_va(virt, PAGE_SIZE * nr_mfns);
Re: [PATCH v3 05/19] xen/arm: mm: Add support for the contiguous bit
On Mon, 21 Feb 2022, Julien Grall wrote:
> From: Julien Grall
>
> In follow-up patches, we will use xen_pt_update() (or its callers)
> to handle large mappings (e.g. frametable, xenheap). They are also
> not going to be modified once created.
>
> The page-table entries have an hint to indicate that whether an
> entry is contiguous to another 16 entries (assuming 4KB). When the
> processor support the hint, one TLB entry will be created per
> contiguous region.
>
> For now this is tied to _PAGE_BLOCK. We can untie it in the future
> if there are use-cases where we may want to use _PAGE_BLOCK without
> setting the contiguous (couldn't think of any yet).
>
> Note that to avoid extra complexity, mappings with the contiguous
> bit set cannot be removed. Given the expected use, this restriction
> ought to be fine.
>
> Signed-off-by: Julien Grall
>
> ---
> Changes in v3:
> - New patch
> ---
> xen/arch/arm/include/asm/page.h | 4 ++
> xen/arch/arm/mm.c | 80 ++---
> 2 files changed, 77 insertions(+), 7 deletions(-)
>
> diff --git a/xen/arch/arm/include/asm/page.h b/xen/arch/arm/include/asm/page.h
> index 07998df47bac..e7cd62190c7f 100644
> --- a/xen/arch/arm/include/asm/page.h
> +++ b/xen/arch/arm/include/asm/page.h
> @@ -70,6 +70,7 @@
> * [5] Page present
> * [6] Only populate page tables
> * [7] Superpage mappings is allowed
> + * [8] Set contiguous bit (internal flag)
> */
> #define PAGE_AI_MASK(x) ((x) & 0x7U)
>
> @@ -86,6 +87,9 @@
> #define _PAGE_BLOCK_BIT 7
> #define _PAGE_BLOCK (1U << _PAGE_BLOCK_BIT)
>
> +#define _PAGE_CONTIG_BIT8
> +#define _PAGE_CONTIG(1U << _PAGE_CONTIG_BIT)
> +
> /*
> * _PAGE_DEVICE and _PAGE_NORMAL are convenience defines. They are not
> * meant to be used outside of this header.
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 3af69b396bd1..fd16c1541ce2 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -1237,6 +1237,8 @@ static int xen_pt_update_entry(mfn_t root, unsigned
> long virt,
> /* Set permission */
> pte.pt.ro = PAGE_RO_MASK(flags);
> pte.pt.xn = PAGE_XN_MASK(flags);
> +/* Set contiguous bit */
> +pte.pt.contig = !!(flags & _PAGE_CONTIG);
> }
>
> write_pte(entry, pte);
> @@ -1289,6 +1291,51 @@ static int xen_pt_mapping_level(unsigned long vfn,
> mfn_t mfn, unsigned long nr,
> return level;
> }
>
> +#define XEN_PT_4K_NR_CONTIG 16
> +
> +/*
> + * Check whether the contiguous bit can be set. Return the number of
> + * contiguous entry allowed. If not allowed, return 1.
> + */
> +static unsigned int xen_pt_check_contig(unsigned long vfn, mfn_t mfn,
> +unsigned int level, unsigned long
> left,
> +unsigned int flags)
> +{
> +unsigned long nr_contig;
> +
> +/*
> + * Allow the contiguous bit to set when the caller requests block
> + * mapping.
> + */
> +if ( !(flags & _PAGE_BLOCK) )
> +return 1;
> +
> +/*
> + * We don't allow to remove mapping with the contiguous bit set.
> + * So shortcut the logic and directly return 1.
> + */
> +if ( mfn_eq(mfn, INVALID_MFN) )
> +return 1;
> +
> +/*
> + * The number of contiguous entries varies depending on the page
> + * granularity used. The logic below assumes 4KB.
> + */
> +BUILD_BUG_ON(PAGE_SIZE != SZ_4K);
> +
> +/*
> + * In order to enable the contiguous bit, we should have enough entries
> + * to map left and both the virtual and physical address should be
> + * aligned to the size of 16 translation tables entries.
> + */
> +nr_contig = BIT(XEN_PT_LEVEL_ORDER(level), UL) * XEN_PT_4K_NR_CONTIG;
> +
> +if ( (left < nr_contig) || ((mfn_x(mfn) | vfn) & (nr_contig - 1)) )
> +return 1;
> +
> +return XEN_PT_4K_NR_CONTIG;
> +}
> +
> static DEFINE_SPINLOCK(xen_pt_lock);
>
> static int xen_pt_update(unsigned long virt,
> @@ -1322,6 +1369,12 @@ static int xen_pt_update(unsigned long virt,
> return -EINVAL;
> }
>
> +if ( flags & _PAGE_CONTIG )
> +{
> +mm_printk("_PAGE_CONTIG is an internal only flag.\n");
> +return -EINVAL;
> +}
> +
> if ( !IS_ALIGNED(virt, PAGE_SIZE) )
> {
> mm_printk("The virtual address is not aligned to the page-size.\n");
> @@ -1333,21 +1386,34 @@ static int xen_pt_update(unsigned long virt,
> while ( left )
> {
> unsigned int order, level;
> +unsigned int nr_contig;
> +unsigned int new_flags;
>
> level = xen_pt_mapping_level(vfn, mfn, left, flags);
> order = XEN_PT_LEVEL_ORDER(level);
>
> ASSERT(left >= BIT(order, UL));
>
> -rc = xen_pt_update_entry(root, pfn_to_paddr(vfn), mfn, level, flags);
> -if ( rc )
> -break;
> +/*
> + * Check if we can set the
Re: [PATCH v3 04/19] xen/arm: mm: Allow other mapping size in xen_pt_update_entry()
On Mon, 21 Feb 2022, Julien Grall wrote:
> From: Julien Grall
>
> At the moment, xen_pt_update_entry() only supports mapping at level 3
> (i.e 4KB mapping). While this is fine for most of the runtime helper,
> the boot code will require to use superpage mapping.
>
> We don't want to allow superpage mapping by default as some of the
> callers may expect small mappings (i.e populate_pt_range()) or even
> expect to unmap only a part of a superpage.
>
> To keep the code simple, a new flag _PAGE_BLOCK is introduced to
> allow the caller to enable superpage mapping.
>
> As the code doesn't support all the combinations, xen_pt_check_entry()
> is extended to take into account the cases we don't support when
> using block mapping:
> - Replacing a table with a mapping. This may happen if region was
> first mapped with 4KB mapping and then later on replaced with a 2MB
> (or 1GB mapping).
> - Removing/modifying a table. This may happen if a caller try to
> remove a region with _PAGE_BLOCK set when it was created without it.
>
> Note that the current restriction means that the caller must ensure that
> _PAGE_BLOCK is consistently set/cleared across all the updates on a
> given virtual region. This ought to be fine with the expected use-cases.
>
> More rework will be necessary if we wanted to remove the restrictions.
>
> Note that nr_mfns is now marked const as it is used for flushing the
> TLBs and we don't want it to be modified.
>
> Signed-off-by: Julien Grall
> Signed-off-by: Julien Grall
>
> ---
> Changes in v3:
> - Fix clash after prefixing the PT macros with XEN_PT_
> - Fix typoes in the commit message
> - Support superpage mappings even if nr is not suitably aligned
> - Move the logic to find the level in a separate function
>
> Changes in v2:
> - Pass the target level rather than the order to
> xen_pt_update_entry()
> - Update some comments
> - Open-code paddr_to_pfn()
> - Add my AWS signed-off-by
> ---
> xen/arch/arm/include/asm/page.h | 4 ++
> xen/arch/arm/mm.c | 108 ++--
> 2 files changed, 94 insertions(+), 18 deletions(-)
>
> diff --git a/xen/arch/arm/include/asm/page.h b/xen/arch/arm/include/asm/page.h
> index c6f9fb0d4e0c..07998df47bac 100644
> --- a/xen/arch/arm/include/asm/page.h
> +++ b/xen/arch/arm/include/asm/page.h
> @@ -69,6 +69,7 @@
> * [3:4] Permission flags
> * [5] Page present
> * [6] Only populate page tables
> + * [7] Superpage mappings is allowed
> */
> #define PAGE_AI_MASK(x) ((x) & 0x7U)
>
> @@ -82,6 +83,9 @@
> #define _PAGE_PRESENT(1U << 5)
> #define _PAGE_POPULATE (1U << 6)
>
> +#define _PAGE_BLOCK_BIT 7
> +#define _PAGE_BLOCK (1U << _PAGE_BLOCK_BIT)
> +
> /*
> * _PAGE_DEVICE and _PAGE_NORMAL are convenience defines. They are not
> * meant to be used outside of this header.
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 515d0906f85b..3af69b396bd1 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -1063,9 +1063,10 @@ static int xen_pt_next_level(bool read_only, unsigned
> int level,
> }
>
> /* Sanity check of the entry */
> -static bool xen_pt_check_entry(lpae_t entry, mfn_t mfn, unsigned int flags)
> +static bool xen_pt_check_entry(lpae_t entry, mfn_t mfn, unsigned int level,
> + unsigned int flags)
> {
> -/* Sanity check when modifying a page. */
> +/* Sanity check when modifying an entry. */
> if ( (flags & _PAGE_PRESENT) && mfn_eq(mfn, INVALID_MFN) )
> {
> /* We don't allow modifying an invalid entry. */
> @@ -1075,6 +1076,13 @@ static bool xen_pt_check_entry(lpae_t entry, mfn_t
> mfn, unsigned int flags)
> return false;
> }
>
> +/* We don't allow modifying a table entry */
> +if ( !lpae_is_mapping(entry, level) )
> +{
> +mm_printk("Modifying a table entry is not allowed.\n");
> +return false;
> +}
> +
> /* We don't allow changing memory attributes. */
> if ( entry.pt.ai != PAGE_AI_MASK(flags) )
> {
> @@ -1090,7 +1098,7 @@ static bool xen_pt_check_entry(lpae_t entry, mfn_t mfn,
> unsigned int flags)
> return false;
> }
> }
> -/* Sanity check when inserting a page */
> +/* Sanity check when inserting a mapping */
> else if ( flags & _PAGE_PRESENT )
> {
> /* We should be here with a valid MFN. */
> @@ -1099,18 +1107,28 @@ static bool xen_pt_check_entry(lpae_t entry, mfn_t
> mfn, unsigned int flags)
> /* We don't allow replacing any valid entry. */
> if ( lpae_is_valid(entry) )
> {
> -mm_printk("Changing MFN for a valid entry is not allowed
> (%#"PRI_mfn" -> %#"PRI_mfn").\n",
> - mfn_x(lpae_get_mfn(entry)), mfn_x(mfn));
> +if ( lpae_is_mapping(entry, level) )
> +
[PATCH v2] Grab the EFI System Resource Table and check it
The EFI System Resource Table (ESRT) is necessary for fwupd to identify
firmware updates to install. According to the UEFI specification §23.4,
the table shall be stored in memory of type EfiBootServicesData.
Therefore, Xen must avoid reusing that memory for other purposes, so
that Linux can access the ESRT. Additionally, Xen must mark the memory
as reserved, so that Linux knows accessing it is safe.
See https://lore.kernel.org/xen-devel/20200818184018.GN1679@mail-itl/T/
for details.
Signed-off-by: Demi Marie Obenour
---
xen/arch/arm/efi/efi-boot.h | 9 +++--
xen/arch/x86/efi/efi-boot.h | 5 ++-
xen/common/efi/boot.c | 77 +++--
xen/common/efi/efi.h| 2 +-
xen/common/efi/runtime.c| 5 ++-
xen/include/efi/efiapi.h| 3 ++
6 files changed, 89 insertions(+), 12 deletions(-)
diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
index ae8627134e..767b2c9154 100644
--- a/xen/arch/arm/efi/efi-boot.h
+++ b/xen/arch/arm/efi/efi-boot.h
@@ -176,7 +176,8 @@ static bool __init meminfo_add_bank(struct meminfo *mem,
static EFI_STATUS __init efi_process_memory_map_bootinfo(EFI_MEMORY_DESCRIPTOR
*map,
UINTN mmap_size,
-UINTN desc_size)
+UINTN desc_size,
+const EFI_MEMORY_DESCRIPTOR
*const esrt_desc)
{
int Index;
EFI_MEMORY_DESCRIPTOR *desc_ptr = map;
@@ -188,6 +189,7 @@ static EFI_STATUS __init
efi_process_memory_map_bootinfo(EFI_MEMORY_DESCRIPTOR *
desc_ptr->Type == EfiLoaderCode ||
desc_ptr->Type == EfiLoaderData ||
(!map_bs &&
+ desc_ptr != esrt_desc &&
(desc_ptr->Type == EfiBootServicesCode ||
desc_ptr->Type == EfiBootServicesData))) )
{
@@ -393,11 +395,12 @@ static void __init
efi_arch_process_memory_map(EFI_SYSTEM_TABLE *SystemTable,
void *map,
UINTN map_size,
UINTN desc_size,
- UINT32 desc_ver)
+ UINT32 desc_ver,
+ const EFI_MEMORY_DESCRIPTOR
*const esrt_desc)
{
EFI_STATUS status;
-status = efi_process_memory_map_bootinfo(map, map_size, desc_size);
+status = efi_process_memory_map_bootinfo(map, map_size, desc_size,
esrt_desc);
if ( EFI_ERROR(status) )
blexit(L"EFI memory map processing failed");
diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
index d91eb5a537..6e6a5d2224 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -154,7 +154,8 @@ static void __init
efi_arch_process_memory_map(EFI_SYSTEM_TABLE *SystemTable,
void *map,
UINTN map_size,
UINTN desc_size,
- UINT32 desc_ver)
+ UINT32 desc_ver,
+ const EFI_MEMORY_DESCRIPTOR
*const esrt_desc)
{
struct e820entry *e;
unsigned int i;
@@ -171,7 +172,7 @@ static void __init
efi_arch_process_memory_map(EFI_SYSTEM_TABLE *SystemTable,
{
case EfiBootServicesCode:
case EfiBootServicesData:
-if ( map_bs )
+if ( map_bs || desc == esrt_desc )
{
default:
type = E820_RESERVED;
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 4dd5ea6a06..ad5883133d 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -93,6 +93,23 @@ typedef struct _EFI_LOAD_OPTION {
CHAR16 Description[];
} EFI_LOAD_OPTION;
+struct esrt_entry {
+EFI_GUID fw_class;
+UINT32 fw_type;
+UINT32 fw_version;
+UINT32 fw_lowest_supported_version;
+UINT32 fw_capsule_flags;
+UINT32 fw_last_attempt_version;
+UINT32 fw_last_attempt_status;
+};
+
+struct esrt {
+UINT32 esrt_count;
+UINT32 esrt_max;
+UINT64 esrt_version;
+struct esrt_entry esrt_entries[];
+};
+
#define LOAD_OPTION_ACTIVE 0x0001
union string {
@@ -567,6 +584,38 @@ static int __init efi_check_dt_boot(const EFI_LOADED_IMAGE
*loaded_image)
}
#endif
+static UINTN __initdata esrt;
+
+static bool __init is_esrt_valid(
+const EFI_MEMORY_DESCRIPTOR *const desc)
+{
+size_t available_len, esrt_len, len;
+const UINTN physical_start = desc->PhysicalStart;
+const struct esrt *esrt_ptr;
+
+len = desc->NumberOfPages << EFI_PAGE_SHIFT;
+if ( esrt == EFI_INVALID_TABLE_ADDR )
+return false;
+if ( physical_start > esrt ||
Re: [PATCH] of: of_property_read_string return -ENODATA when !length
On Fri, 1 Apr 2022, Rob Herring wrote:
> On Fri, Apr 1, 2022 at 3:49 PM Stefano Stabellini
> wrote:
> >
> > On Fri, 1 Apr 2022, Rob Herring wrote:
> > > On Thu, Mar 31, 2022 at 7:46 PM Stefano Stabellini
> > > wrote:
> > > >
> > > > From: Stefano Stabellini
> > > >
> > > > When the length of the string is zero of_property_read_string should
> > > > return -ENODATA according to the description of the function.
> > >
> > > Perhaps it is a difference of:
> > >
> > > prop;
> > >
> > > vs.
> > >
> > > prop = "";
> > >
> > > Both are 0 length by some definition. The description, '-ENODATA if
> > > property does not have a value', matches the first case.
> > >
> > > >
> > > > However, of_property_read_string doesn't check pp->length. If pp->length
> > > > is zero, return -ENODATA.
> > > >
> > > > Without this patch the following command in u-boot:
> > > >
> > > > fdt set /chosen/node property-name
> > > >
> > > > results in of_property_read_string returning -EILSEQ when attempting to
> > > > read property-name. With this patch, it returns -ENODATA as expected.
> > >
> > > Why do you care? Do you have a user? There could be an in tree user
> > > that doesn't like this change.
> >
> > During review of a Xen patch series (we have libfdt is Xen too, synced
> > with the kernel) Julien noticed a check for -EILSEQ. I added the check
> > so that Xen would behave correctly in cases like the u-boot example in
> > the patch description.
> >
> > Looking more into it, it seemed to be a mismatch between the description
> > of of_property_read_string and the behavior (e.g. -ENODATA would seem to
> > be the right return value, not -EILSEQ.)
> >
> > I added a printk to confirm what was going on when -EILSEQ was returned:
> >
> > printk("DEBUG %s %d value=%s value[0]=%d length=%u
> > len=%lu\n",__func__,__LINE__,(char*)pp->value,
> > *((char*)pp->value),pp->length,
> > strlen(pp->value));
> >
> > This is the output:
> > DEBUG of_property_read_string 205 value= value[0]=0 length=0 len=0
>
> It turns out that we never set pp->value to NULL when unflattening
> (and libfdt always returns a value). This function is assuming we do.
> >
> > As the description says:
> >
> > *
> > * Return: 0 on success, -EINVAL if the property does not exist, -ENODATA if
> > * property does not have a value, and -EILSEQ if the string is not
> > * null-terminated within the length of the property data.
> > *
> >
> > It seems that this case matches "property does not have a value" which
> > is expected to be -ENODATA instead of -EILSEQ. I guess one could also
> > say that length is zero, so the string cannot be null-terminated,
> > thus -EILSEQ?
> >
> > I am happy to go with your interpretation but -ENODATA seems to be the
> > best match in my opinion.
>
> I agree. I just think empty property should have a NULL value and 0
> length, but we should only have to check one. I don't want check
> length as that could be different for Sparc or non-FDT. So I think we
> need this instead:
>
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index ec315b060cd5..d6b2b0d49d89 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -165,7 +165,7 @@ static void populate_properties(const void *blob,
>
> pp->name = (char *)pname;
> pp->length = sz;
> - pp->value = (__be32 *)val;
> + pp->value = sz ? (__be32 *)val : NULL;
> *pprev = pp;
> pprev = &pp->next;
> }
>
>
> It looks like setting 'value' has been like this at least since 2010.
Yep, fixing old bugs nobody cares about, that's me :-)
I made the corresponding change to Xen to check that it fixes the
original issue (I am using Xen only for convenience because I already
have a reproducer setup for it.)
Unfortunately it breaks the boot: the reason is that of_get_property is
implemented as:
return pp ? pp->value : NULL;
and at least in Xen (maybe in Linux too) there are instances of callers
doing:
if (!of_get_property(node, "interrupt-controller", NULL))
continue;
This would now take the wrong code path because value is returned as
NULL.
So, although your patch is technically correct, it comes with higher
risk of breaking existing code. How do you want to proceed?
Re: [PATCH] of: of_property_read_string return -ENODATA when !length
On Fri, Apr 1, 2022 at 3:49 PM Stefano Stabellini
wrote:
>
> On Fri, 1 Apr 2022, Rob Herring wrote:
> > On Thu, Mar 31, 2022 at 7:46 PM Stefano Stabellini
> > wrote:
> > >
> > > From: Stefano Stabellini
> > >
> > > When the length of the string is zero of_property_read_string should
> > > return -ENODATA according to the description of the function.
> >
> > Perhaps it is a difference of:
> >
> > prop;
> >
> > vs.
> >
> > prop = "";
> >
> > Both are 0 length by some definition. The description, '-ENODATA if
> > property does not have a value', matches the first case.
> >
> > >
> > > However, of_property_read_string doesn't check pp->length. If pp->length
> > > is zero, return -ENODATA.
> > >
> > > Without this patch the following command in u-boot:
> > >
> > > fdt set /chosen/node property-name
> > >
> > > results in of_property_read_string returning -EILSEQ when attempting to
> > > read property-name. With this patch, it returns -ENODATA as expected.
> >
> > Why do you care? Do you have a user? There could be an in tree user
> > that doesn't like this change.
>
> During review of a Xen patch series (we have libfdt is Xen too, synced
> with the kernel) Julien noticed a check for -EILSEQ. I added the check
> so that Xen would behave correctly in cases like the u-boot example in
> the patch description.
>
> Looking more into it, it seemed to be a mismatch between the description
> of of_property_read_string and the behavior (e.g. -ENODATA would seem to
> be the right return value, not -EILSEQ.)
>
> I added a printk to confirm what was going on when -EILSEQ was returned:
>
> printk("DEBUG %s %d value=%s value[0]=%d length=%u
> len=%lu\n",__func__,__LINE__,(char*)pp->value, *((char*)pp->value),pp->length,
> strlen(pp->value));
>
> This is the output:
> DEBUG of_property_read_string 205 value= value[0]=0 length=0 len=0
It turns out that we never set pp->value to NULL when unflattening
(and libfdt always returns a value). This function is assuming we do.
>
> As the description says:
>
> *
> * Return: 0 on success, -EINVAL if the property does not exist, -ENODATA if
> * property does not have a value, and -EILSEQ if the string is not
> * null-terminated within the length of the property data.
> *
>
> It seems that this case matches "property does not have a value" which
> is expected to be -ENODATA instead of -EILSEQ. I guess one could also
> say that length is zero, so the string cannot be null-terminated,
> thus -EILSEQ?
>
> I am happy to go with your interpretation but -ENODATA seems to be the
> best match in my opinion.
I agree. I just think empty property should have a NULL value and 0
length, but we should only have to check one. I don't want check
length as that could be different for Sparc or non-FDT. So I think we
need this instead:
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index ec315b060cd5..d6b2b0d49d89 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -165,7 +165,7 @@ static void populate_properties(const void *blob,
pp->name = (char *)pname;
pp->length = sz;
- pp->value = (__be32 *)val;
+ pp->value = sz ? (__be32 *)val : NULL;
*pprev = pp;
pprev = &pp->next;
}
It looks like setting 'value' has been like this at least since 2010.
Rob
[xen-4.16-testing test] 169086: regressions - FAIL
flight 169086 xen-4.16-testing real [real] http://logs.test-lab.xenproject.org/osstest/logs/169086/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: build-arm64-xsm 6 xen-buildfail REGR. vs. 168513 test-amd64-i386-xl-qemut-debianhvm-i386-xsm 12 debian-hvm-install fail REGR. vs. 168513 Tests which did not succeed, but are not blocking: test-arm64-arm64-libvirt-xsm 1 build-check(1) blocked n/a test-arm64-arm64-xl-xsm 1 build-check(1) blocked n/a test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 168513 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 168513 test-armhf-armhf-libvirt 16 saverestore-support-checkfail like 168513 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 168513 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 168513 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 168513 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail like 168513 test-amd64-i386-xl-qemut-win7-amd64 19 guest-stop fail like 168513 test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 168513 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check fail like 168513 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 168513 test-amd64-i386-xl-qemut-ws16-amd64 19 guest-stop fail like 168513 test-amd64-i386-xl-pvshim14 guest-start fail never pass test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-i386-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-i386-libvirt 15 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 15 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-amd64-i386-libvirt-raw 14 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-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-thunderx 15 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 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-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail 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-vhd 14 migrate-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-libvirt 15 migrate-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 15 migrate-support-checkfail never pass test-armhf-armhf-xl 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-cubietruck 15 migrate-support-checkfail never pass test-armhf-armhf-xl-cubietruck 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-rtds 15 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-vhd 14 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 15 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-raw 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-armhf-armhf-libvirt-qcow2 14 migrate-support-checkfail never pass version targeted for testing: xen e34c16cc6ee029fa75c35bd
[xen-unstable-smoke test] 169114: tolerable all pass - PUSHED
flight 169114 xen-unstable-smoke real [real] http://logs.test-lab.xenproject.org/osstest/logs/169114/ 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 d62a34423a1a98aefd7c30e22d2d82d198f077c8 baseline version: xen e7cfcdc6719d586eb7cdb62d40275a7d17fe6760 Last test of basis 169090 2022-04-01 01:01:50 Z0 days Testing same since 169114 2022-04-01 17:01:54 Z0 days1 attempts People who touched revisions under test: Jason Andryuk 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 e7cfcdc671..d62a34423a d62a34423a1a98aefd7c30e22d2d82d198f077c8 -> smoke
Re: [PATCH] of: of_property_read_string return -ENODATA when !length
On Fri, 1 Apr 2022, Rob Herring wrote:
> On Thu, Mar 31, 2022 at 7:46 PM Stefano Stabellini
> wrote:
> >
> > From: Stefano Stabellini
> >
> > When the length of the string is zero of_property_read_string should
> > return -ENODATA according to the description of the function.
>
> Perhaps it is a difference of:
>
> prop;
>
> vs.
>
> prop = "";
>
> Both are 0 length by some definition. The description, '-ENODATA if
> property does not have a value', matches the first case.
>
> >
> > However, of_property_read_string doesn't check pp->length. If pp->length
> > is zero, return -ENODATA.
> >
> > Without this patch the following command in u-boot:
> >
> > fdt set /chosen/node property-name
> >
> > results in of_property_read_string returning -EILSEQ when attempting to
> > read property-name. With this patch, it returns -ENODATA as expected.
>
> Why do you care? Do you have a user? There could be an in tree user
> that doesn't like this change.
During review of a Xen patch series (we have libfdt is Xen too, synced
with the kernel) Julien noticed a check for -EILSEQ. I added the check
so that Xen would behave correctly in cases like the u-boot example in
the patch description.
Looking more into it, it seemed to be a mismatch between the description
of of_property_read_string and the behavior (e.g. -ENODATA would seem to
be the right return value, not -EILSEQ.)
I added a printk to confirm what was going on when -EILSEQ was returned:
printk("DEBUG %s %d value=%s value[0]=%d length=%u
len=%lu\n",__func__,__LINE__,(char*)pp->value, *((char*)pp->value),pp->length,
strlen(pp->value));
This is the output:
DEBUG of_property_read_string 205 value= value[0]=0 length=0 len=0
As the description says:
*
* Return: 0 on success, -EINVAL if the property does not exist, -ENODATA if
* property does not have a value, and -EILSEQ if the string is not
* null-terminated within the length of the property data.
*
It seems that this case matches "property does not have a value" which
is expected to be -ENODATA instead of -EILSEQ. I guess one could also
say that length is zero, so the string cannot be null-terminated,
thus -EILSEQ?
I am happy to go with your interpretation but -ENODATA seems to be the
best match in my opinion.
> > Signed-off-by: Stefano Stabellini
> >
> > diff --git a/drivers/of/property.c b/drivers/of/property.c
> > index 8e90071de6ed..da0f02c98bb2 100644
> > --- a/drivers/of/property.c
> > +++ b/drivers/of/property.c
> > @@ -439,7 +439,7 @@ int of_property_read_string(const struct device_node
> > *np, const char *propname,
> > const struct property *prop = of_find_property(np, propname, NULL);
> > if (!prop)
> > return -EINVAL;
> > - if (!prop->value)
> > + if (!prop->value || !pp->length)
> > return -ENODATA;
> > if (strnlen(prop->value, prop->length) >= prop->length)
> > return -EILSEQ;
>
[ovmf test] 169102: regressions - FAIL
flight 169102 ovmf real [real] http://logs.test-lab.xenproject.org/osstest/logs/169102/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: build-amd64 6 xen-buildfail REGR. vs. 168254 build-amd64-xsm 6 xen-buildfail REGR. vs. 168254 build-i3866 xen-buildfail REGR. vs. 168254 build-i386-xsm6 xen-buildfail REGR. vs. 168254 Tests which did not succeed, but are not blocking: build-amd64-libvirt 1 build-check(1) blocked n/a build-i386-libvirt1 build-check(1) blocked n/a test-amd64-amd64-xl-qemuu-ovmf-amd64 1 build-check(1) blocked n/a test-amd64-i386-xl-qemuu-ovmf-amd64 1 build-check(1) blocked n/a version targeted for testing: ovmf 55637a2894babca97945eeca1da0d431f74f8627 baseline version: ovmf b1b89f9009f2390652e0061bd7b24fc40732bc70 Last test of basis 168254 2022-02-28 10:41:46 Z 32 days Failing since168258 2022-03-01 01:55:31 Z 31 days 266 attempts Testing same since 169004 2022-03-30 02:24:42 Z2 days4 attempts People who touched revisions under test: Abdul Lateef Attar Abdul Lateef Attar via groups.io Abner Chang Akihiko Odaki Bandaru, Purna Chandra Rao Bob Feng Gerd Hoffmann Guo Dong Guomin Jiang Hao A Wu Hua Ma Huang, Li-Xia Jagadeesh Ujja Jason Jason Lou Ken Lautner Kenneth Lautner Kuo, Ted Li, Zhihao Lixia Huang Lou, Yun Ma, Hua Mara Sophie Grosch Mara Sophie Grosch via groups.io Matt DeVillier Michael Kubacki Patrick Rudolph Purna Chandra Rao Bandaru Ray Ni Sami Mujawar Sean Rhodes Sean Rhodes sean@starlabs.systems Sebastien Boeuf Sunny Wang Ted Kuo Wenyi Xie wenyi,xie via groups.io Xiaolu.Jiang Yi Li Zhihao Li jobs: build-amd64-xsm fail build-i386-xsm fail build-amd64 fail build-i386 fail build-amd64-libvirt blocked build-i386-libvirt blocked build-amd64-pvopspass build-i386-pvops pass test-amd64-amd64-xl-qemuu-ovmf-amd64 blocked test-amd64-i386-xl-qemuu-ovmf-amd64 blocked 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. (No revision log; it would be 1236 lines long.)
[libvirt test] 169098: regressions - FAIL
flight 169098 libvirt real [real] http://logs.test-lab.xenproject.org/osstest/logs/169098/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: build-armhf-libvirt 6 libvirt-buildfail REGR. vs. 151777 build-amd64-libvirt 6 libvirt-buildfail REGR. vs. 151777 build-i386-libvirt6 libvirt-buildfail REGR. vs. 151777 build-arm64-libvirt 6 libvirt-buildfail REGR. vs. 151777 Tests which did not succeed, but are not blocking: test-amd64-amd64-libvirt 1 build-check(1) blocked n/a test-amd64-amd64-libvirt-pair 1 build-check(1) blocked n/a test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 1 build-check(1) blocked n/a test-amd64-amd64-libvirt-vhd 1 build-check(1) blocked n/a test-amd64-amd64-libvirt-xsm 1 build-check(1) blocked n/a test-amd64-i386-libvirt 1 build-check(1) blocked n/a test-amd64-i386-libvirt-pair 1 build-check(1) blocked n/a test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 1 build-check(1) blocked n/a test-amd64-i386-libvirt-raw 1 build-check(1) blocked n/a test-amd64-i386-libvirt-xsm 1 build-check(1) blocked n/a test-arm64-arm64-libvirt 1 build-check(1) blocked n/a test-arm64-arm64-libvirt-qcow2 1 build-check(1) blocked n/a test-arm64-arm64-libvirt-raw 1 build-check(1) blocked n/a test-armhf-armhf-libvirt-raw 1 build-check(1) blocked n/a test-arm64-arm64-libvirt-xsm 1 build-check(1) blocked n/a test-armhf-armhf-libvirt 1 build-check(1) blocked n/a test-armhf-armhf-libvirt-qcow2 1 build-check(1) blocked n/a version targeted for testing: libvirt d5f81479a6e51eaf9842d05df40c631653c6230d baseline version: libvirt 2c846fa6bcc11929c9fb857a22430fb9945654ad Last test of basis 151777 2020-07-10 04:19:19 Z 630 days Failing since151818 2020-07-11 04:18:52 Z 629 days 611 attempts Testing same since 169098 2022-04-01 04:20:07 Z0 days1 attempts People who touched revisions under test: Adolfo Jayme Barrientos Aleksandr Alekseev Aleksei Zakharov Andika Triwidada Andrea Bolognani Ani Sinha Balázs Meskó Barrett Schonefeld Bastian Germann Bastien Orivel BiaoXiang Ye Bihong Yu Binfeng Wu Bjoern Walk Boris Fiuczynski Brad Laue Brian Turek Bruno Haible Chris Mayo Christian Borntraeger Christian Ehrhardt Christian Kirbach Christian Schoenebeck Christophe Fergeau Claudio Fontana Cole Robinson Collin Walling Cornelia Huck Cédric Bosdonnat Côme Borsoi Daniel Henrique Barboza Daniel Letai Daniel P. Berrange Daniel P. Berrangé Didik Supriadi dinglimin Divya Garg Dmitrii Shcherbakov Dmytro Linkin Eiichi Tsukata Emilio Herrera Eric Farman Erik Skultety Fabian Affolter Fabian Freyer Fabiano Fidêncio Fangge Jin Farhan Ali Fedora Weblate Translation Franck Ridel Gavi Teitz gongwei Guoyi Tu Göran Uddeborg Halil Pasic Han Han Hao Wang Haonan Wang Hela Basa Helmut Grohne Hiroki Narukawa Hyman Huang(黄勇) Ian Wienand Ioanna Alifieraki Ivan Teterevkov Jakob Meng Jamie Strandboge Jamie Strandboge Jan Kuparinen jason lee Jean-Baptiste Holcroft Jia Zhou Jianan Gao Jim Fehlig Jin Yan Jing Qi Jinsheng Zhang Jiri Denemark Joachim Falk John Ferlan John Levon John Levon Jonathan Watt Jonathon Jongsma Julio Faracco Justin Gatzen Ján Tomko Kashyap Chamarthy Kevin Locke Kim InSoo Koichi Murase Kristina Hanicova Laine Stump Laszlo Ersek Lee Yarwood Lei Yang Liao Pingfang Lin Ma Lin Ma Lin Ma Liu Yiding Lubomir Rintel Luke Yue Luyao Zhong Marc Hartmayer Marc-André Lureau Marek Marczykowski-Górecki Markus Schade Martin Kletzander Martin Pitt Masayoshi Mizuma Matej Cepl Matt Coleman Matt Coleman Mauro Matteo Cascella Meina Li Michal Privoznik Michał Smyk Milo Casagrande Moshe Levi Muha Aliss Nathan Neal Gompa Nick Chevsky Nick Shyrokovskiy Nickys Music Group Nico Pache Nicolas Lécureuil Nicolas Lécureuil Nikolay Shirokovskiy Olaf Hering Olesya Gerasimenko Or Ozeri Orion Poplawski Pany Paolo Bonzini Patrick Magauran Paulo de Rezende Pinatti Pavel Hrdina Peng Liang Peter Krempa Pino Toscano Pino Toscano Piotr Drąg Prathamesh Chavan Praveen K Paladugu Richard W.M. Jones Ricky Tigg Robin Lee Rohit Kumar Roman Bogorodskiy Roman Bolshakov Ryan Gahagan Ryan Schmidt Sam Hartma
Re: [PATCH v1 3/5] xen/arm: unpopulate memory when domain on static allocation
Hi Penny, On 31/03/2022 11:30, Penny Zheng wrote: Another reason I want to keep page allocated is that if putting pages in resv_page_list upon dropping the last ref, we need to do a lot things on pages to totally let it free, like set its owner to NULL, changing page state from in_use to free, etc. This is not only about optimization here. Bad things can happen if you let a page in state free that is not meant to be used by the buddy allocator (e.g. it was reserved for static memory). I discovered it earlier this year when trying to optimize init_heap_pages() for Live-Update. It was quite hard to debug because the corruption very rarely happened. So let me explain it before you face the same issue :). free_heap_pages() will try to merge the about-to-be-freed chunk with the predecessor and/or successor. To know if the page can be merged, the algorithm is looking at whether the chunk is suitably aligned (e.g. same order) and if the page is in state free. AFAICT, the pages belonging to the buddy allocator could be right next to region reserved memory. So there is a very slim chance that free_heap_pages() may decide to merge a chunk from the static region with the about-to-be-free chunk. Nothing very good will ensue. Technically, this is already a bug in the already merged implementation of the static memory allocator. I can see two ways to fix it: 1) Update free_heap_pages() to check whether the page has PGC_reserved set. 2) Use a different state for pages used by the static allocator. So far my preference is leaning towards 1. But I would like to hear other opinions. Cheers, -- Julien Grall
Re: [PATCH v1 3/5] xen/arm: unpopulate memory when domain on static allocation
Hi Jan, On 30/03/2022 10:52, Jan Beulich wrote: On 30.03.2022 11:36, Penny Zheng wrote: --- a/xen/common/memory.c +++ b/xen/common/memory.c @@ -35,6 +35,10 @@ #include #endif +#ifndef is_domain_on_static_allocation +#define is_domain_on_static_allocation(d) 0 Nit: "false", not "0". I think we also want to evaluate d so the behavior on x86 and arm is the same. Something like ((void)(d), false). Cheers, -- Julien Grall
Re: [PATCH v1 2/5] xen/arm: introduce CDF_staticmem
Hi Penny,
On 30/03/2022 10:36, Penny Zheng wrote:
In order to have an easy and quick way to find out whether this domain is
on static allocation, this commit introduces a new flag CDF_staticmem and a
new helper is_domain_on_static_allocation.
Signed-off-by: Penny Zheng
---
xen/arch/arm/domain_build.c | 5 -
xen/arch/arm/include/asm/domain.h | 2 ++
xen/include/xen/domain.h | 2 ++
3 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 8be01678de..4e62fd0bf1 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -3191,9 +3191,12 @@ void __init create_domUs(void)
if ( !dt_device_is_compatible(node, "xen,domain") )
continue;
+if ( dt_find_property(node, "xen,static-mem", NULL) )
+flags |= CDF_staticmem;
+
if ( dt_property_read_bool(node, "direct-map") )
{
-if ( !IS_ENABLED(CONFIG_STATIC_MEMORY) || !dt_find_property(node,
"xen,static-mem", NULL) )
+if ( !IS_ENABLED(CONFIG_STATIC_MEMORY) || !(flags & CDF_staticmem)
)
panic("direct-map is not valid for domain %s without static
allocation.\n",
dt_node_name(node));
diff --git a/xen/arch/arm/include/asm/domain.h b/xen/arch/arm/include/asm/domain.h
index 95fef29111..4379f20a12 100644
--- a/xen/arch/arm/include/asm/domain.h
+++ b/xen/arch/arm/include/asm/domain.h
@@ -31,6 +31,8 @@ enum domain_type {
#define is_domain_direct_mapped(d) (((d)->arch.flags) & CDF_directmap)
+#define is_domain_on_static_allocation(d) (((d)->arch.flags) & CDF_staticmem)
"on" sounds strange to me and the name is bit long. How about
"is_domain_using_staticmem()" or maybe "is_domain_static()"?
I will have a look at the rest of the series before giving my ack on
this patch.
+
/*
* Is the domain using the host memory layout?
*
diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h
index 1c3c88a14d..35dc7143a4 100644
--- a/xen/include/xen/domain.h
+++ b/xen/include/xen/domain.h
@@ -34,6 +34,8 @@ void arch_get_domain_info(const struct domain *d,
#ifdef CONFIG_ARM
/* Should domain memory be directly mapped? */
#define CDF_directmap(1U << 1)
+/* Is domain memory on static allocation? */
+#define CDF_staticmem(1U << 2)
#endif
/*
Cheers,
--
Julien Grall
Re: [PATCH v1 1/5] xen/arm: field "flags" to cover all internal CDF_XXX
Hi Penny,
On 30/03/2022 10:36, Penny Zheng wrote:
With more and more CDF_xxx internal flags in and to save the space, this
commit introduces a new field "flags" to store CDF_* internal flags
directly.
Another new CDF_xxx will be introduced in the next patch.
Signed-off-by: Penny Zheng
---
xen/arch/arm/domain.c | 3 ++-
xen/arch/arm/include/asm/domain.h | 5 +++--
2 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 8110c1df86..35c157d499 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -709,7 +709,8 @@ int arch_domain_create(struct domain *d,
ioreq_domain_init(d);
#endif
-d->arch.directmap = flags & CDF_directmap;
+/* Holding CDF_* internal flags. */
+d->arch.flags = flags;
/* p2m_init relies on some value initialized by the IOMMU subsystem */
if ( (rc = iommu_domain_init(d, config->iommu_opts)) != 0 )
diff --git a/xen/arch/arm/include/asm/domain.h
b/xen/arch/arm/include/asm/domain.h
index ed63c2b6f9..95fef29111 100644
--- a/xen/arch/arm/include/asm/domain.h
+++ b/xen/arch/arm/include/asm/domain.h
@@ -29,7 +29,7 @@ enum domain_type {
#define is_64bit_domain(d) (0)
#endif
-#define is_domain_direct_mapped(d) (d)->arch.directmap
+#define is_domain_direct_mapped(d) (((d)->arch.flags) & CDF_directmap)
The () around (d)->arch.flags are not necessary.
/*
* Is the domain using the host memory layout?
@@ -103,7 +103,8 @@ struct arch_domain
void *tee;
#endif
-bool directmap;
+/* Holding CDF_* constant. Internal flags for domain creation. */
+uint32_t flags;
I think this wants to live in the struct domain. So other arch can take
advantage of it in the future.
} __cacheline_aligned;
struct arch_vcpu
Cheers,
--
Julien Grall
Re: [PATCH 1/2] xsm: add ability to elevate a domain to privileged
Hi Daniel,
On 31/03/2022 00:05, Daniel P. Smith wrote:
There are now instances where internal hypervisor logic needs to make resource
allocation calls that are protected by XSM checks. The internal hypervisor logic
is represented a number of system domains which by designed are represented by
non-privileged struct domain instances. To enable these logic blocks to
function correctly but in a controlled manner, this commit introduces a pair
of privilege escalation and demotion functions that will make a system domain
privileged and then remove that privilege.
Signed-off-by: Daniel P. Smith
---
xen/include/xsm/xsm.h | 22 ++
1 file changed, 22 insertions(+)
diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
index e22d6160b5..157e57151e 100644
--- a/xen/include/xsm/xsm.h
+++ b/xen/include/xsm/xsm.h
@@ -189,6 +189,28 @@ struct xsm_operations {
#endif
};
+static always_inline int xsm_elevate_priv(struct domain *d)
+{
+if ( is_system_domain(d) )
+{
+d->is_privileged = true;
The call for xsm_elevate_priv() cannot be nested. So I would suggest to
check if d->is_privileged is already true and return -EBUSY in this case.
+return 0;
+}
+
+return -EPERM;
+}
+
+static always_inline int xsm_demote_priv(struct domain *d)
+{
+if ( is_system_domain(d) )
+{
+d->is_privileged = false;
+return 0;
+}
+
+return -EPERM;
+}
+
#ifdef CONFIG_XSM
extern struct xsm_operations *xsm_ops;
Cheers,
--
Julien Grall
Re: [PATCH 1/2] xsm: add ability to elevate a domain to privileged
Hi,
On 31/03/2022 13:36, Roger Pau Monné wrote:
On Wed, Mar 30, 2022 at 07:05:48PM -0400, Daniel P. Smith wrote:
There are now instances where internal hypervisor logic needs to make resource
allocation calls that are protected by XSM checks. The internal hypervisor logic
is represented a number of system domains which by designed are represented by
non-privileged struct domain instances. To enable these logic blocks to
function correctly but in a controlled manner, this commit introduces a pair
of privilege escalation and demotion functions that will make a system domain
privileged and then remove that privilege.
Signed-off-by: Daniel P. Smith
---
xen/include/xsm/xsm.h | 22 ++
I'm not sure this needs to be in xsm code, AFAICT it could live in a
more generic file.
1 file changed, 22 insertions(+)
diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
index e22d6160b5..157e57151e 100644
--- a/xen/include/xsm/xsm.h
+++ b/xen/include/xsm/xsm.h
@@ -189,6 +189,28 @@ struct xsm_operations {
#endif
};
+static always_inline int xsm_elevate_priv(struct domain *d)
I don't think it needs to be always_inline, using just inline would be
fine IMO.
Also this needs to be __init.
Hmmm I thought adding __init on function defined in header was
pointless. In particular, if the compiler decides to inline it.
In any case, I think it would be good to check that the system_state <
SYS_state_active (could potentially be an ASSERT()) to prevent any misuse.
Cheers,
--
Julien Grall
[xen-4.15-testing test] 169079: regressions - FAIL
flight 169079 xen-4.15-testing real [real] http://logs.test-lab.xenproject.org/osstest/logs/169079/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: test-amd64-i386-livepatch13 livepatch-runfail REGR. vs. 168502 test-amd64-amd64-livepatch 13 livepatch-runfail REGR. vs. 168502 Tests which are failing intermittently (not blocking): test-amd64-i386-xl-qemut-debianhvm-i386-xsm 12 debian-hvm-install fail in 169040 pass in 169079 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 18 guest-start/debianhvm.repeat fail in 169040 pass in 169079 test-armhf-armhf-xl-rtds 18 guest-start/debian.repeat fail pass in 169040 Tests which did not succeed, but are not blocking: test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 168502 test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 168502 test-armhf-armhf-libvirt 16 saverestore-support-checkfail like 168502 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 168502 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 168502 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 168502 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 168502 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check fail like 168502 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail like 168502 test-amd64-i386-xl-qemut-win7-amd64 19 guest-stop fail like 168502 test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 168502 test-amd64-i386-xl-qemut-ws16-amd64 19 guest-stop fail like 168502 test-amd64-i386-libvirt-xsm 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-arm64-arm64-xl-seattle 15 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 16 saverestore-support-checkfail never pass test-amd64-i386-xl-pvshim14 guest-start fail never pass test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-amd64-i386-libvirt 15 migrate-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-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-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt-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-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail 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-i386-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-amd64-amd64-libvirt-vhd 14 migrate-support-checkfail never pass test-amd64-i386-libvirt-raw 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-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-credit1 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 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-arm64-arm64-xl-vhd 14 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 15 migrate-support-checkfail never pass test-arm64-arm64-xl-vhd 15 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit2 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-cubietruck 15 migrate-support-checkfail never pass test-armhf-armhf-xl-cubietruck 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-rtds 15 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 16 saverestore-sup
Re: [PATCH v4 3/9] xen/arm: temporarily elevate idle_domain privileged during create_domUs
Hi,
On 01/04/2022 01:38, Stefano Stabellini wrote:
From: Stefano Stabellini
create_domUs might call functions that perform XSM checks on the current
domain, which is idle_domain at this time. Temporarily elevate
idle_domain privileges in create_domUs.
Signed-off-by: Stefano Stabellini
---
xen/arch/arm/domain_build.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index b6189b935d..100a4959a8 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -27,6 +27,7 @@
#include
#include
#include
+#include
#include
#include
@@ -3210,6 +3211,8 @@ void __init create_domUs(void)
struct dt_device_node *node;
const struct dt_device_node *chosen = dt_find_node_by_path("/chosen");
+xsm_elevate_priv(current->domain);
Please check the return of this function and...
+
BUG_ON(chosen == NULL);
dt_for_each_child_node(chosen, node)
{
@@ -3291,6 +3294,8 @@ void __init create_domUs(void)
if ( construct_domU(d, node) != 0 )
panic("Could not set up domain %s\n", dt_node_name(node));
}
+
+xsm_demote_priv(current->domain);
... this. For us, it should hopefully be 0. But it is a good practice to
confirm.
Cheers,
--
Julien Grall
Re: [PATCH v12] xen/arm64: io: Handle data abort due to cache maintenance instructions
Hi Ayan,
On 24/03/2022 13:37, Ayan Kumar Halder wrote:
/*
* At this point, we know that the instruction is either valid or has been
* decoded successfully. Thus, Xen should be allowed to execute the
diff --git a/xen/arch/arm/ioreq.c b/xen/arch/arm/ioreq.c
index 54167aebcb..87a6240f2a 100644
--- a/xen/arch/arm/ioreq.c
+++ b/xen/arch/arm/ioreq.c
@@ -47,7 +47,7 @@ enum io_state try_fwd_ioserv(struct cpu_user_regs *regs,
struct vcpu *v, mmio_info_t *info)
{
struct vcpu_io *vio = &v->io;
-struct instr_details instr = info->dabt_instr;
+const struct instr_details instr = info->dabt_instr;
struct hsr_dabt dabt = info->dabt;
ioreq_t p = {
.type = IOREQ_TYPE_COPY,
@@ -62,7 +62,6 @@ enum io_state try_fwd_ioserv(struct cpu_user_regs *regs,
* memory access. So for now, we can safely always set to 0.
*/
.df = 0,
-.data = get_user_reg(regs, info->dabt.reg),
.state = STATE_IOREQ_READY,
};
struct ioreq_server *s = NULL;
@@ -74,12 +73,23 @@ enum io_state try_fwd_ioserv(struct cpu_user_regs *regs,
return IO_ABORT;
}
+if ( instr.state == INSTR_CACHE )
+p.size = dcache_line_bytes;
I think it would be best to only set the p.size when instr.state !=
INSTR_CACHE in the else here.
I can do that on commit. I will also give a chance to Stefano to reply.
Cheers,
--
Julien Grall
[xen-4.14-testing test] 169074: regressions - FAIL
flight 169074 xen-4.14-testing real [real] flight 169111 xen-4.14-testing real-retest [real] http://logs.test-lab.xenproject.org/osstest/logs/169074/ http://logs.test-lab.xenproject.org/osstest/logs/169111/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: test-amd64-amd64-livepatch 13 livepatch-runfail REGR. vs. 168506 test-amd64-i386-livepatch13 livepatch-runfail REGR. vs. 168506 Tests which did not succeed, but are not blocking: test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 168506 test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 168506 test-armhf-armhf-libvirt 16 saverestore-support-checkfail like 168506 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 168506 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 168506 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 168506 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 168506 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check fail like 168506 test-amd64-i386-xl-qemut-ws16-amd64 19 guest-stop fail like 168506 test-amd64-i386-xl-qemut-win7-amd64 19 guest-stop fail like 168506 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail like 168506 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 168506 test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-amd64-i386-libvirt 15 migrate-support-checkfail never pass test-amd64-i386-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 15 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 16 saverestore-support-checkfail never pass test-amd64-i386-xl-pvshim14 guest-start fail never pass test-amd64-amd64-libvirt-xsm 15 migrate-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-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-credit1 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 16 saverestore-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-armhf-armhf-xl-arndale 15 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 16 saverestore-support-checkfail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-amd64-amd64-libvirt-vhd 14 migrate-support-checkfail never pass test-amd64-i386-libvirt-raw 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-armhf-armhf-libvirt 15 migrate-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-xl-multivcpu 15 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 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-credit1 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-cubietruck 15 migrate-support-checkfail never pass test-armhf-armhf-xl-cubietruck 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-qcow2 14 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 14 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 15 saverestore-support-checkf
Re: [PATCH 1/2] tools/firmware: fix setting of fcf-protection=none
On Fri, Apr 01, 2022 at 03:04:44PM +, Andrew Cooper wrote: > On 01/04/2022 15:50, Anthony PERARD wrote: > > On Fri, Apr 01, 2022 at 04:37:18PM +0200, Roger Pau Monne wrote: > >> +EMBEDDED_EXTRA_CFLAGS += -fcf-protection=none > >> + > > I think making modification to $(EMBEDDED_EXTRA_CFLAGS) outside of > > "Config.mk" is confusing and would be better be avoided. > > EMBEDDED_EXTRA_CFLAGS in the root Config.mk is conceptually broken and > needs deleting. I'm asking to remove "-fcf-protection=none" from the broken variable EMBEDDED_EXTRA_CFLAGS, and instead only modify the CFLAGS variable of "tools/firmware/". As this patch show, making modification to EMBEDDED_EXTRA_CFLAGS outside of Config.mk doesn't work because changes doesn't propagate. So the modification I've ask for the patch goes toward deleting EMBEDDED_EXTRA_CFLAGS, like you want. Cheers, -- Anthony PERARD
Re: [PATCH 1/2] tools/firmware: fix setting of fcf-protection=none
On 01/04/2022 15:48, Andrew Cooper wrote:
> On 01/04/2022 15:37, Roger Pau Monne wrote:
>> Setting the fcf-protection=none option in EMBEDDED_EXTRA_CFLAGS in the
>> Makefile doesn't get it propagated to the subdirectories, so instead
>> set the flag in firmware/Rules.mk, like it's done for other compiler
>> flags.
>>
>> Fixes: 3667f7f8f7 ('x86: Introduce support for CET-IBT')
>> Signed-off-by: Roger Pau Monné
> Acked-by: Andrew Cooper
This also needs backporting with the XSA-398 CET-IBT fixes.
~Andrew
Re: [PATCH 1/2] tools/firmware: fix setting of fcf-protection=none
On 01/04/2022 15:50, Anthony PERARD wrote:
> On Fri, Apr 01, 2022 at 04:37:18PM +0200, Roger Pau Monne wrote:
>> Setting the fcf-protection=none option in EMBEDDED_EXTRA_CFLAGS in the
>> Makefile doesn't get it propagated to the subdirectories, so instead
>> set the flag in firmware/Rules.mk, like it's done for other compiler
>> flags.
>>
>> Fixes: 3667f7f8f7 ('x86: Introduce support for CET-IBT')
>> Signed-off-by: Roger Pau Monné
>> ---
>> tools/firmware/Makefile | 2 --
>> tools/firmware/Rules.mk | 2 ++
>> 2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/firmware/Makefile b/tools/firmware/Makefile
>> index 53ed4f161e..345037b93b 100644
>> --- a/tools/firmware/Makefile
>> +++ b/tools/firmware/Makefile
>> @@ -6,8 +6,6 @@ TARGET := hvmloader/hvmloader
>> INST_DIR := $(DESTDIR)$(XENFIRMWAREDIR)
>> DEBG_DIR := $(DESTDIR)$(DEBUG_DIR)$(XENFIRMWAREDIR)
>>
>> -EMBEDDED_EXTRA_CFLAGS += -fcf-protection=none
>> -
>> SUBDIRS-y :=
>> SUBDIRS-$(CONFIG_OVMF) += ovmf-dir
>> SUBDIRS-$(CONFIG_SEABIOS) += seabios-dir
>> diff --git a/tools/firmware/Rules.mk b/tools/firmware/Rules.mk
>> index 9f78a7dec9..efbbc73a45 100644
>> --- a/tools/firmware/Rules.mk
>> +++ b/tools/firmware/Rules.mk
>> @@ -13,6 +13,8 @@ endif
>>
>> CFLAGS += -Werror
>>
>> +EMBEDDED_EXTRA_CFLAGS += -fcf-protection=none
>> +
> I think making modification to $(EMBEDDED_EXTRA_CFLAGS) outside of
> "Config.mk" is confusing and would be better be avoided.
EMBEDDED_EXTRA_CFLAGS in the root Config.mk is conceptually broken and
needs deleting.
Yes, xen/ and tools/firmware/ are freestanding from C's point of view,
and embedded from many peoples points of view, but this doesn't mean
they have shared build requirements.
-nopie isn't even a CFLAG. It's spelt -no-pie and is an LDFLAG. This
bug is hidden by everything being cc-option'd behind the scenes.
Stack protector we'd absolutely have in Xen if it weren't for a quirk of
supporting PV guests.
-fno-exceptions is C++ only so not relevant for anything in xen.git
~Andrew
Re: [PATCH] of: of_property_read_string return -ENODATA when !length
On Thu, Mar 31, 2022 at 7:46 PM Stefano Stabellini wrote: > > From: Stefano Stabellini > > When the length of the string is zero of_property_read_string should > return -ENODATA according to the description of the function. Perhaps it is a difference of: prop; vs. prop = ""; Both are 0 length by some definition. The description, '-ENODATA if property does not have a value', matches the first case. > > However, of_property_read_string doesn't check pp->length. If pp->length > is zero, return -ENODATA. > > Without this patch the following command in u-boot: > > fdt set /chosen/node property-name > > results in of_property_read_string returning -EILSEQ when attempting to > read property-name. With this patch, it returns -ENODATA as expected. Why do you care? Do you have a user? There could be an in tree user that doesn't like this change. > > Signed-off-by: Stefano Stabellini > > diff --git a/drivers/of/property.c b/drivers/of/property.c > index 8e90071de6ed..da0f02c98bb2 100644 > --- a/drivers/of/property.c > +++ b/drivers/of/property.c > @@ -439,7 +439,7 @@ int of_property_read_string(const struct device_node *np, > const char *propname, > const struct property *prop = of_find_property(np, propname, NULL); > if (!prop) > return -EINVAL; > - if (!prop->value) > + if (!prop->value || !pp->length) > return -ENODATA; > if (strnlen(prop->value, prop->length) >= prop->length) > return -EILSEQ;
Re: [PATCH 2/2] tools/firmware: do not add a .note.gnu.property section
On 01/04/2022 15:37, Roger Pau Monne wrote: > Prevent the assembler from creating a .note.gnu.property section on > the output objects, as it's not useful for firmware related binaries, > and breaks the resulting rombios image. > > This requires modifying the cc-option Makefile macro so it can test > assembler options (by replacing the usage of the -S flag with -c) and > also stripping the -Wa, prefix if present when checking for the test > output. > > Signed-off-by: Roger Pau Monné Acked-by: Andrew Cooper It's worth saying that this was found from XenServer's testing, but there's no obvious Fixes: tag to use.
Re: [PATCH 1/2] tools/firmware: fix setting of fcf-protection=none
On Fri, Apr 01, 2022 at 04:37:18PM +0200, Roger Pau Monne wrote:
> Setting the fcf-protection=none option in EMBEDDED_EXTRA_CFLAGS in the
> Makefile doesn't get it propagated to the subdirectories, so instead
> set the flag in firmware/Rules.mk, like it's done for other compiler
> flags.
>
> Fixes: 3667f7f8f7 ('x86: Introduce support for CET-IBT')
> Signed-off-by: Roger Pau Monné
> ---
> tools/firmware/Makefile | 2 --
> tools/firmware/Rules.mk | 2 ++
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tools/firmware/Makefile b/tools/firmware/Makefile
> index 53ed4f161e..345037b93b 100644
> --- a/tools/firmware/Makefile
> +++ b/tools/firmware/Makefile
> @@ -6,8 +6,6 @@ TARGET := hvmloader/hvmloader
> INST_DIR := $(DESTDIR)$(XENFIRMWAREDIR)
> DEBG_DIR := $(DESTDIR)$(DEBUG_DIR)$(XENFIRMWAREDIR)
>
> -EMBEDDED_EXTRA_CFLAGS += -fcf-protection=none
> -
> SUBDIRS-y :=
> SUBDIRS-$(CONFIG_OVMF) += ovmf-dir
> SUBDIRS-$(CONFIG_SEABIOS) += seabios-dir
> diff --git a/tools/firmware/Rules.mk b/tools/firmware/Rules.mk
> index 9f78a7dec9..efbbc73a45 100644
> --- a/tools/firmware/Rules.mk
> +++ b/tools/firmware/Rules.mk
> @@ -13,6 +13,8 @@ endif
>
> CFLAGS += -Werror
>
> +EMBEDDED_EXTRA_CFLAGS += -fcf-protection=none
> +
I think making modification to $(EMBEDDED_EXTRA_CFLAGS) outside of
"Config.mk" is confusing and would be better be avoided.
Could you instead call $(cc-option-add ) just for that extra CFLAGS?
> $(call cc-options-add,CFLAGS,CC,$(EMBEDDED_EXTRA_CFLAGS))
>
> # Extra CFLAGS suitable for an embedded type of environment.
Thanks,
--
Anthony PERARD
Re: [PATCH 1/2] tools/firmware: fix setting of fcf-protection=none
On 01/04/2022 15:37, Roger Pau Monne wrote:
> Setting the fcf-protection=none option in EMBEDDED_EXTRA_CFLAGS in the
> Makefile doesn't get it propagated to the subdirectories, so instead
> set the flag in firmware/Rules.mk, like it's done for other compiler
> flags.
>
> Fixes: 3667f7f8f7 ('x86: Introduce support for CET-IBT')
> Signed-off-by: Roger Pau Monné
Acked-by: Andrew Cooper
Re: [PATCH v2] libxl: Don't segfault on soft-reset failure
On Fri, Apr 01, 2022 at 10:32:56AM -0400, Jason Andryuk wrote:
> If domain_soft_reset_cb can't rename the save file, it doesn't call
> initiate_domain_create() and calls domcreate_complete().
>
> Skipping initiate_domain_create() means dcs->console_wait is
> uninitialized and all 0s.
>
> We have:
> domcreate_complete()
> libxl__xswait_stop()
> libxl__ev_xswatch_deregister().
>
> The uninitialized slotnum 0 is considered valid (-1 is the invalid
> sentinel), so the NULL pointer path to passed to xs_unwatch() which
> segfaults.
>
> libxl__ev_xswatch_deregister:watch w=0x12bc250 wpath=(null) token=0/0:
> deregister slotnum=0
>
> Move dcs->console_xswait initialization into the callers of
> initiate_domain_create, do_domain_create() and do_domain_soft_reset(),
> so it is initialized along with the other dcs state.
>
> Fixes: c57e6ebd8c3e ("(lib)xl: soft reset support")
> Signed-off-by: Jason Andryuk
Reviewed-by: Anthony PERARD
Thanks,
--
Anthony PERARD
Re: [PATCH v2] libxl: Re-scope qmp_proxy_spawn.ao usage
On Fri, Apr 01, 2022 at 10:33:10AM -0400, Jason Andryuk wrote:
> I've observed this failed assertion:
> libxl_event.c:2057: libxl__ao_inprogress_gc: Assertion `ao' failed.
>
> AFAICT, this is happening in qmp_proxy_spawn_outcome where
> sdss->qmp_proxy_spawn.ao is NULL.
>
> The out label of spawn_stub_launch_dm() calls qmp_proxy_spawn_outcome(),
> but it is only in the success path that sdss->qmp_proxy_spawn.ao gets
> set to the current ao.
>
> qmp_proxy_spawn_outcome() should instead use sdss->dm.spawn.ao, which is
> the already in-use ao when spawn_stub_launch_dm() is called. The same
> is true for spawn_qmp_proxy().
>
> With this, move sdss->qmp_proxy_spawn.ao initialization to
> spawn_qmp_proxy() since its use is for libxl__spawn_spawn() and it can
> be initialized along with the rest of sdss->qmp_proxy_spawn.
>
> Fixes: 83c845033dc8 ("libxl: use vchan for QMP access with Linux stubdomain")
> Signed-off-by: Jason Andryuk
Reviewed-by: Anthony PERARD
Thanks,
--
Anthony PERARD
[PATCH 1/2] tools/firmware: fix setting of fcf-protection=none
Setting the fcf-protection=none option in EMBEDDED_EXTRA_CFLAGS in the
Makefile doesn't get it propagated to the subdirectories, so instead
set the flag in firmware/Rules.mk, like it's done for other compiler
flags.
Fixes: 3667f7f8f7 ('x86: Introduce support for CET-IBT')
Signed-off-by: Roger Pau Monné
---
tools/firmware/Makefile | 2 --
tools/firmware/Rules.mk | 2 ++
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/firmware/Makefile b/tools/firmware/Makefile
index 53ed4f161e..345037b93b 100644
--- a/tools/firmware/Makefile
+++ b/tools/firmware/Makefile
@@ -6,8 +6,6 @@ TARGET := hvmloader/hvmloader
INST_DIR := $(DESTDIR)$(XENFIRMWAREDIR)
DEBG_DIR := $(DESTDIR)$(DEBUG_DIR)$(XENFIRMWAREDIR)
-EMBEDDED_EXTRA_CFLAGS += -fcf-protection=none
-
SUBDIRS-y :=
SUBDIRS-$(CONFIG_OVMF) += ovmf-dir
SUBDIRS-$(CONFIG_SEABIOS) += seabios-dir
diff --git a/tools/firmware/Rules.mk b/tools/firmware/Rules.mk
index 9f78a7dec9..efbbc73a45 100644
--- a/tools/firmware/Rules.mk
+++ b/tools/firmware/Rules.mk
@@ -13,6 +13,8 @@ endif
CFLAGS += -Werror
+EMBEDDED_EXTRA_CFLAGS += -fcf-protection=none
+
$(call cc-options-add,CFLAGS,CC,$(EMBEDDED_EXTRA_CFLAGS))
# Extra CFLAGS suitable for an embedded type of environment.
--
2.35.1
[PATCH 2/2] tools/firmware: do not add a .note.gnu.property section
Prevent the assembler from creating a .note.gnu.property section on the output objects, as it's not useful for firmware related binaries, and breaks the resulting rombios image. This requires modifying the cc-option Makefile macro so it can test assembler options (by replacing the usage of the -S flag with -c) and also stripping the -Wa, prefix if present when checking for the test output. Signed-off-by: Roger Pau Monné --- Config.mk | 2 +- tools/firmware/Rules.mk | 4 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/Config.mk b/Config.mk index f56f7dc334..82832945e5 100644 --- a/Config.mk +++ b/Config.mk @@ -91,7 +91,7 @@ PYTHON_PREFIX_ARG ?= --prefix="$(prefix)" # # Usage: cflags-y += $(call cc-option,$(CC),-march=winchip-c6,-march=i586) cc-option = $(shell if test -z "`echo 'void*p=1;' | \ - $(1) $(2) -S -o /dev/null -x c - 2>&1 | grep -- $(2) -`"; \ + $(1) $(2) -c -o /dev/null -x c - 2>&1 | grep -- $(2:-Wa$(comma)%=%) -`"; \ then echo "$(2)"; else echo "$(3)"; fi ;) # cc-option-add: Add an option to compilation flags, but only if supported. diff --git a/tools/firmware/Rules.mk b/tools/firmware/Rules.mk index efbbc73a45..1cbe666f5e 100644 --- a/tools/firmware/Rules.mk +++ b/tools/firmware/Rules.mk @@ -15,6 +15,10 @@ CFLAGS += -Werror EMBEDDED_EXTRA_CFLAGS += -fcf-protection=none +# Do not add the .note.gnu.property section to any of the firmware objects: it +# breaks the rombios binary and is not useful for firmware anyway. +EMBEDDED_EXTRA_CFLAGS += -Wa$$(comma)-mx86-used-note=no + $(call cc-options-add,CFLAGS,CC,$(EMBEDDED_EXTRA_CFLAGS)) # Extra CFLAGS suitable for an embedded type of environment. -- 2.35.1
[PATCH 0/2] firmware: build fixes with gcc-11
Hello, The following fixes some firmware build issues with gcc-11. Note that dropping of .note.gnu.property section could likely be done in the linker script in the hvmloader case, but rombios has no linker script and such note is causing a non-working image. Other options could be using objcopy to drop the section, but those seems more complicated than just using the compiler command line option. Thanks, Roger. Roger Pau Monne (2): tools/firmware: fix setting of fcf-protection=none tools/firmware: do not add a .note.gnu.property section Config.mk | 2 +- tools/firmware/Makefile | 2 -- tools/firmware/Rules.mk | 6 ++ 3 files changed, 7 insertions(+), 3 deletions(-) -- 2.35.1
Re: [PATCH v4 2/9] xen/arm: implement domU extended regions
Hi,
On 01/04/2022 01:38, Stefano Stabellini wrote:
From: Stefano Stabellini
Implement extended regions for dom0less domUs. The implementation is
based on the libxl implementation.
Signed-off-by: Stefano Stabellini
---
xen/arch/arm/domain_build.c | 42 ++---
1 file changed, 39 insertions(+), 3 deletions(-)
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 8be01678de..b6189b935d 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1324,6 +1324,35 @@ out:
return res;
}
+#define ALIGN_UP_TO_2MB(x) (((x) + MB(2) - 1) & (~(MB(2) - 1)))
I think this is the same as ROUNDUP(x, SZ_2M).
+
+static int __init find_domU_holes(const struct kernel_info *kinfo,
+ struct meminfo *ext_regions)
+{
+unsigned int i;
+uint64_t bankend[GUEST_RAM_BANKS];
Looking, you only seem to use one bankend at the time. So why do you
need to store all the bankend?
This should also be s/uint64_t/paddr_t/. Same for two other instances below.
+const uint64_t bankbase[] = GUEST_RAM_BANK_BASES;
+const uint64_t banksize[] = GUEST_RAM_BANK_SIZES;
+
+for ( i = 0; i < GUEST_RAM_BANKS; i++ )
+{
+ext_regions->bank[ext_regions->nr_banks].start =
The code would be easier to read if you define a local variable ext_bank
that points to &(ext_regions->bank[ext_regions->nr_banks]).
+ALIGN_UP_TO_2MB(bankbase[i] + kinfo->mem.bank[i].size);
+
+bankend[i] = ~0ULL >> (64 - p2m_ipa_bits);
+bankend[i] = min(bankend[i], bankbase[i] + banksize[i] - 1); > +if
(bankend[i] > ext_regions->bank[ext_regions->nr_banks].start)
Coding style:
if ( ... )
+ext_regions->bank[ext_regions->nr_banks].size =
+bankend[i] - ext_regions->bank[ext_regions->nr_banks].start +
1;
This is one of the line that could greatly benefits from the local
variable I suggested above. It would look like:
ext_bank->size = bankend[i] - ext_bank->start + 1;
+
+/* 64MB is the minimum size of an extended region */
+if ( ext_regions->bank[ext_regions->nr_banks].size < MB(64) )
+continue;
+ext_regions->nr_banks++;
+}
NIT: We tend to add a newline before the last return.
+return 0;
find_memory_holes() and find_unallocated_memory() will return an error
if there are no banks allocated. I think we should do the same here at
least for consistency.
In which case, the check should be moved in make_hypervisor_node().
+}
+
static int __init make_hypervisor_node(struct domain *d,
const struct kernel_info *kinfo,
int addrcells, int sizecells)
@@ -1374,10 +1403,17 @@ static int __init make_hypervisor_node(struct domain *d,
if ( !ext_regions )
return -ENOMEM;
-if ( !is_iommu_enabled(d) )
-res = find_unallocated_memory(kinfo, ext_regions);
+if ( is_domain_direct_mapped(d) )
I believe the code in the 'if' part would work properly for a dom0 that
is not direct mapped (e.g. in the cache coloring case).
If it doesn't, I think we need...
+{
+if ( !is_iommu_enabled(d) )
+res = find_unallocated_memory(kinfo, ext_regions);
+else
+res = find_memory_holes(kinfo, ext_regions);
+}
else
-res = find_memory_holes(kinfo, ext_regions);
+{
... and ASSERT() here so we the person that will introduce non direct
mapped dom0 can easily notice before the domain get corrupted.
+res = find_domU_holes(kinfo, ext_regions);
+}
if ( res )
printk(XENLOG_WARNING "Failed to allocate extended regions\n");
This printk() and the others in the function should print the domain.
Cheers,
--
Julien Grall
[PATCH v2] libxl: Re-scope qmp_proxy_spawn.ao usage
I've observed this failed assertion:
libxl_event.c:2057: libxl__ao_inprogress_gc: Assertion `ao' failed.
AFAICT, this is happening in qmp_proxy_spawn_outcome where
sdss->qmp_proxy_spawn.ao is NULL.
The out label of spawn_stub_launch_dm() calls qmp_proxy_spawn_outcome(),
but it is only in the success path that sdss->qmp_proxy_spawn.ao gets
set to the current ao.
qmp_proxy_spawn_outcome() should instead use sdss->dm.spawn.ao, which is
the already in-use ao when spawn_stub_launch_dm() is called. The same
is true for spawn_qmp_proxy().
With this, move sdss->qmp_proxy_spawn.ao initialization to
spawn_qmp_proxy() since its use is for libxl__spawn_spawn() and it can
be initialized along with the rest of sdss->qmp_proxy_spawn.
Fixes: 83c845033dc8 ("libxl: use vchan for QMP access with Linux stubdomain")
Signed-off-by: Jason Andryuk
---
v2:
Change subject
Add Fixes
Change to using sdss->dm.spawn.ao
tools/libs/light/libxl_dm.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/tools/libs/light/libxl_dm.c b/tools/libs/light/libxl_dm.c
index 9a8ddbe188..1864ee30f0 100644
--- a/tools/libs/light/libxl_dm.c
+++ b/tools/libs/light/libxl_dm.c
@@ -2567,7 +2567,6 @@ static void spawn_stub_launch_dm(libxl__egc *egc,
goto out;
}
-sdss->qmp_proxy_spawn.ao = ao;
if (libxl__stubdomain_is_linux(&guest_config->b_info)) {
spawn_qmp_proxy(egc, sdss);
} else {
@@ -2584,7 +2583,7 @@ out:
static void spawn_qmp_proxy(libxl__egc *egc,
libxl__stub_dm_spawn_state *sdss)
{
-STATE_AO_GC(sdss->qmp_proxy_spawn.ao);
+STATE_AO_GC(sdss->dm.spawn.ao);
const uint32_t guest_domid = sdss->dm.guest_domid;
const uint32_t dm_domid = sdss->pvqemu.guest_domid;
const char *dom_path = libxl__xs_get_dompath(gc, dm_domid);
@@ -2598,6 +2597,7 @@ static void spawn_qmp_proxy(libxl__egc *egc,
goto out;
}
+sdss->qmp_proxy_spawn.ao = ao;
sdss->qmp_proxy_spawn.what = GCSPRINTF("domain %d device model qmp proxy",
guest_domid);
sdss->qmp_proxy_spawn.pidpath = GCSPRINTF("%s/image/qmp-proxy-pid",
dom_path);
sdss->qmp_proxy_spawn.xspath = DEVICE_MODEL_XS_PATH(gc,
LIBXL_TOOLSTACK_DOMID,
@@ -2685,7 +2685,7 @@ static void qmp_proxy_spawn_outcome(libxl__egc *egc,
libxl__stub_dm_spawn_state *sdss,
int rc)
{
-STATE_AO_GC(sdss->qmp_proxy_spawn.ao);
+STATE_AO_GC(sdss->dm.spawn.ao);
/*
* Until xenconsoled learns how to handle multiple consoles, require qemu
* in dom0 to serve consoles for a stubdomain - it require at least 3 of
them.
--
2.35.1
[PATCH v2] libxl: Don't segfault on soft-reset failure
If domain_soft_reset_cb can't rename the save file, it doesn't call
initiate_domain_create() and calls domcreate_complete().
Skipping initiate_domain_create() means dcs->console_wait is
uninitialized and all 0s.
We have:
domcreate_complete()
libxl__xswait_stop()
libxl__ev_xswatch_deregister().
The uninitialized slotnum 0 is considered valid (-1 is the invalid
sentinel), so the NULL pointer path to passed to xs_unwatch() which
segfaults.
libxl__ev_xswatch_deregister:watch w=0x12bc250 wpath=(null) token=0/0:
deregister slotnum=0
Move dcs->console_xswait initialization into the callers of
initiate_domain_create, do_domain_create() and do_domain_soft_reset(),
so it is initialized along with the other dcs state.
Fixes: c57e6ebd8c3e ("(lib)xl: soft reset support")
Signed-off-by: Jason Andryuk
---
v2:
Add Fixes
Drop NULL check
Re-position libxl__xswait_init in callers
tools/libs/light/libxl_create.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/libs/light/libxl_create.c b/tools/libs/light/libxl_create.c
index 15ed021f41..885675591f 100644
--- a/tools/libs/light/libxl_create.c
+++ b/tools/libs/light/libxl_create.c
@@ -1255,8 +1255,6 @@ static void initiate_domain_create(libxl__egc *egc,
libxl_domain_config *const d_config = dcs->guest_config;
libxl__domain_build_state *dbs = &dcs->build_state;
-libxl__xswait_init(&dcs->console_xswait);
-
domid = dcs->domid;
libxl__domain_build_state_init(dbs);
dbs->restore = dcs->restore_fd >= 0;
@@ -2072,6 +2070,7 @@ static int do_domain_create(libxl_ctx *ctx,
libxl_domain_config *d_config,
cdcs->dcs.callback = domain_create_cb;
cdcs->dcs.domid = INVALID_DOMID;
cdcs->dcs.soft_reset = false;
+libxl__xswait_init(&cdcs->dcs.console_xswait);
if (cdcs->dcs.restore_params.checkpointed_stream ==
LIBXL_CHECKPOINTED_STREAM_COLO) {
@@ -2172,6 +2171,7 @@ static int do_domain_soft_reset(libxl_ctx *ctx,
cdcs->dcs.domid = domid;
cdcs->dcs.soft_reset = true;
cdcs->dcs.callback = domain_create_cb;
+libxl__xswait_init(&cdcs->dcs.console_xswait);
libxl__ao_progress_gethow(&srs->cdcs.dcs.aop_console_how,
aop_console_how);
cdcs->domid_out = &domid_out;
--
2.35.1
[PATCH AUTOSEL 5.17 137/149] xen/usb: harden xen_hcd against malicious backends
From: Juergen Gross
[ Upstream commit aff477cb8f94613f501d386d10f20019e294bc35 ]
Make sure a malicious backend can't cause any harm other than wrong
I/O data.
Missing are verification of the request id in a response, sanitizing
the reported actual I/O length, and protection against interrupt storms
from the backend.
Signed-off-by: Juergen Gross
Link: https://lore.kernel.org/r/20220311103509.12908-1-jgr...@suse.com
Signed-off-by: Greg Kroah-Hartman
Signed-off-by: Sasha Levin
---
drivers/usb/host/xen-hcd.c | 57 --
1 file changed, 43 insertions(+), 14 deletions(-)
diff --git a/drivers/usb/host/xen-hcd.c b/drivers/usb/host/xen-hcd.c
index 19b8c7ed74cb..4ed3ee328a4a 100644
--- a/drivers/usb/host/xen-hcd.c
+++ b/drivers/usb/host/xen-hcd.c
@@ -51,6 +51,7 @@ struct vdevice_status {
struct usb_shadow {
struct xenusb_urb_request req;
struct urb *urb;
+ bool in_flight;
};
struct xenhcd_info {
@@ -722,6 +723,12 @@ static void xenhcd_gnttab_done(struct xenhcd_info *info,
unsigned int id)
int nr_segs = 0;
int i;
+ if (!shadow->in_flight) {
+ xenhcd_set_error(info, "Illegal request id");
+ return;
+ }
+ shadow->in_flight = false;
+
nr_segs = shadow->req.nr_buffer_segs;
if (xenusb_pipeisoc(shadow->req.pipe))
@@ -805,6 +812,7 @@ static int xenhcd_do_request(struct xenhcd_info *info,
struct urb_priv *urbp)
info->urb_ring.req_prod_pvt++;
info->shadow[id].urb = urb;
+ info->shadow[id].in_flight = true;
RING_PUSH_REQUESTS_AND_CHECK_NOTIFY(&info->urb_ring, notify);
if (notify)
@@ -933,10 +941,27 @@ static int xenhcd_unlink_urb(struct xenhcd_info *info,
struct urb_priv *urbp)
return ret;
}
-static int xenhcd_urb_request_done(struct xenhcd_info *info)
+static void xenhcd_res_to_urb(struct xenhcd_info *info,
+ struct xenusb_urb_response *res, struct urb *urb)
+{
+ if (unlikely(!urb))
+ return;
+
+ if (res->actual_length > urb->transfer_buffer_length)
+ urb->actual_length = urb->transfer_buffer_length;
+ else if (res->actual_length < 0)
+ urb->actual_length = 0;
+ else
+ urb->actual_length = res->actual_length;
+ urb->error_count = res->error_count;
+ urb->start_frame = res->start_frame;
+ xenhcd_giveback_urb(info, urb, res->status);
+}
+
+static int xenhcd_urb_request_done(struct xenhcd_info *info,
+ unsigned int *eoiflag)
{
struct xenusb_urb_response res;
- struct urb *urb;
RING_IDX i, rp;
__u16 id;
int more_to_do = 0;
@@ -963,16 +988,12 @@ static int xenhcd_urb_request_done(struct xenhcd_info
*info)
xenhcd_gnttab_done(info, id);
if (info->error)
goto err;
- urb = info->shadow[id].urb;
- if (likely(urb)) {
- urb->actual_length = res.actual_length;
- urb->error_count = res.error_count;
- urb->start_frame = res.start_frame;
- xenhcd_giveback_urb(info, urb, res.status);
- }
+ xenhcd_res_to_urb(info, &res, info->shadow[id].urb);
}
xenhcd_add_id_to_freelist(info, id);
+
+ *eoiflag = 0;
}
info->urb_ring.rsp_cons = i;
@@ -990,7 +1011,7 @@ static int xenhcd_urb_request_done(struct xenhcd_info
*info)
return 0;
}
-static int xenhcd_conn_notify(struct xenhcd_info *info)
+static int xenhcd_conn_notify(struct xenhcd_info *info, unsigned int *eoiflag)
{
struct xenusb_conn_response res;
struct xenusb_conn_request *req;
@@ -1035,6 +1056,8 @@ static int xenhcd_conn_notify(struct xenhcd_info *info)
info->conn_ring.req_prod_pvt);
req->id = id;
info->conn_ring.req_prod_pvt++;
+
+ *eoiflag = 0;
}
if (rc != info->conn_ring.req_prod_pvt)
@@ -1057,14 +1080,19 @@ static int xenhcd_conn_notify(struct xenhcd_info *info)
static irqreturn_t xenhcd_int(int irq, void *dev_id)
{
struct xenhcd_info *info = (struct xenhcd_info *)dev_id;
+ unsigned int eoiflag = XEN_EOI_FLAG_SPURIOUS;
- if (unlikely(info->error))
+ if (unlikely(info->error)) {
+ xen_irq_lateeoi(irq, XEN_EOI_FLAG_SPURIOUS);
return IRQ_HANDLED;
+ }
- while (xenhcd_urb_request_done(info) | xenhcd_conn_notify(info))
+ while (xenhcd_urb_request_done(info, &eoiflag) |
+ xenhcd_conn_notify(info, &eoiflag))
/* Yield point for this unbounded loop. */
cond_resched();
+ xen_irq_lateeoi(irq, eoiflag
Re: [PATCH v4 8/9] tools: add example application to initialize dom0less PV drivers
On 01.04.22 16:04, Julien Grall wrote:
Hi Juergen,
On 01/04/2022 14:52, Juergen Gross wrote:
On 01.04.22 15:35, Julien Grall wrote:
Hi Juergen,
On 01/04/2022 11:46, Juergen Gross wrote:
On 01.04.22 12:21, Julien Grall wrote:
Hi,
I have posted some comments in v3 after you sent this version. Please have
a look.
On 01/04/2022 01:38, Stefano Stabellini wrote:
+static int init_domain(struct xs_handle *xsh, libxl_dominfo *info)
+{
+ struct xc_interface_core *xch;
+ libxl_uuid uuid;
+ uint64_t xenstore_evtchn, xenstore_pfn;
+ int rc;
+
+ printf("Init dom0less domain: %u\n", info->domid);
+ xch = xc_interface_open(0, 0, 0);
+
+ rc = xc_hvm_param_get(xch, info->domid, HVM_PARAM_STORE_EVTCHN,
+ &xenstore_evtchn);
+ if (rc != 0) {
+ printf("Failed to get HVM_PARAM_STORE_EVTCHN\n");
+ return 1;
+ }
+
+ /* Alloc xenstore page */
+ if (alloc_xs_page(xch, info, &xenstore_pfn) != 0) {
+ printf("Error on alloc magic pages\n");
+ return 1;
+ }
+
+ rc = xc_dom_gnttab_seed(xch, info->domid, true,
+ (xen_pfn_t)-1, xenstore_pfn, 0, 0);
+ if (rc)
+ err(1, "xc_dom_gnttab_seed");
+
+ libxl_uuid_generate(&uuid);
+ xc_domain_sethandle(xch, info->domid, libxl_uuid_bytearray(&uuid));
+
+ rc = gen_stub_json_config(info->domid, &uuid);
+ if (rc)
+ err(1, "gen_stub_json_config");
+
+ /* Now everything is ready: set HVM_PARAM_STORE_PFN */
+ rc = xc_hvm_param_set(xch, info->domid, HVM_PARAM_STORE_PFN,
+ xenstore_pfn);
On patch #1, you told me you didn't want to allocate the page in Xen
because it wouldn't be initialized by Xenstored. But this is what we are
doing here.
Xenstore (at least the C variant) is only using the fixed grant ref
GNTTAB_RESERVED_XENSTORE, so it doesn't need the page to be advertised
to the guest. And the mapping is done only when the domain is being
introduced to Xenstore.
And we don't expect the guest to use the grant entry to find the xenstore page?
This would be a problem if Linux is still booting and hasn't yet call
xenbus_probe_initcall().
I understand we need to have the page setup before raising the event
channel. I don't think we can allow Xenstored to set the HVM_PARAM (it may
run in a domain with less privilege). So I think we may need to create a
separate command to kick the client (not great).
Juergen, any thoughts?
I think it should work like that:
- setup the grant via xc_dom_gnttab_seed()
- introduce the domain to Xenstore
- call xc_hvm_param_set()
When the guest is receiving the event, it should wait for the xenstore
page to appear.
IIUC, this would mean the guest would need to do some sort of busy loop until
the xenstore page to appear.
Looking for it every second or so would be enough.
This is a better than a busy loop but not by much. I would argue a design that
requires to poll after receiving an interrupt is broken.
If so, this doesn't sound great to me. I think it would be better to have a
flag in the page to indicate whether the page is not ready.
Xenstored could then clear the flag before raising the event channel.
Hmm, the "connection" field could be used for that.
I thought about the field but the description doesn't entirely match what we
want. In particular, the spec says only the guest should set the value to 1
(i.e. reconnect). Maybe this could be relaxed?
It would mean, though, that e.g. libxl would need to initialize the
page accordingly before calling xs_introduce()
libxl only create domain paused. So I don't think it would be necessary to
update it.
Maybe not libxl, but whoever is calling xc_dom_gnttab_seed(),
xc_hvm_param_set() and/or xs_introduce() needs to set the field, in
order to have an effect of Xenstore resetting it.
Juergen
OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key
OpenPGP_signature
Description: OpenPGP digital signature
Re: [PATCH v4 8/9] tools: add example application to initialize dom0less PV drivers
Hi Juergen,
On 01/04/2022 14:52, Juergen Gross wrote:
On 01.04.22 15:35, Julien Grall wrote:
Hi Juergen,
On 01/04/2022 11:46, Juergen Gross wrote:
On 01.04.22 12:21, Julien Grall wrote:
Hi,
I have posted some comments in v3 after you sent this version.
Please have a look.
On 01/04/2022 01:38, Stefano Stabellini wrote:
+static int init_domain(struct xs_handle *xsh, libxl_dominfo *info)
+{
+ struct xc_interface_core *xch;
+ libxl_uuid uuid;
+ uint64_t xenstore_evtchn, xenstore_pfn;
+ int rc;
+
+ printf("Init dom0less domain: %u\n", info->domid);
+ xch = xc_interface_open(0, 0, 0);
+
+ rc = xc_hvm_param_get(xch, info->domid, HVM_PARAM_STORE_EVTCHN,
+ &xenstore_evtchn);
+ if (rc != 0) {
+ printf("Failed to get HVM_PARAM_STORE_EVTCHN\n");
+ return 1;
+ }
+
+ /* Alloc xenstore page */
+ if (alloc_xs_page(xch, info, &xenstore_pfn) != 0) {
+ printf("Error on alloc magic pages\n");
+ return 1;
+ }
+
+ rc = xc_dom_gnttab_seed(xch, info->domid, true,
+ (xen_pfn_t)-1, xenstore_pfn, 0, 0);
+ if (rc)
+ err(1, "xc_dom_gnttab_seed");
+
+ libxl_uuid_generate(&uuid);
+ xc_domain_sethandle(xch, info->domid,
libxl_uuid_bytearray(&uuid));
+
+ rc = gen_stub_json_config(info->domid, &uuid);
+ if (rc)
+ err(1, "gen_stub_json_config");
+
+ /* Now everything is ready: set HVM_PARAM_STORE_PFN */
+ rc = xc_hvm_param_set(xch, info->domid, HVM_PARAM_STORE_PFN,
+ xenstore_pfn);
On patch #1, you told me you didn't want to allocate the page in Xen
because it wouldn't be initialized by Xenstored. But this is what we
are doing here.
Xenstore (at least the C variant) is only using the fixed grant ref
GNTTAB_RESERVED_XENSTORE, so it doesn't need the page to be advertised
to the guest. And the mapping is done only when the domain is being
introduced to Xenstore.
And we don't expect the guest to use the grant entry to find the
xenstore page?
This would be a problem if Linux is still booting and hasn't yet
call xenbus_probe_initcall().
I understand we need to have the page setup before raising the event
channel. I don't think we can allow Xenstored to set the HVM_PARAM
(it may run in a domain with less privilege). So I think we may need
to create a separate command to kick the client (not great).
Juergen, any thoughts?
I think it should work like that:
- setup the grant via xc_dom_gnttab_seed()
- introduce the domain to Xenstore
- call xc_hvm_param_set()
When the guest is receiving the event, it should wait for the xenstore
page to appear.
IIUC, this would mean the guest would need to do some sort of busy
loop until the xenstore page to appear.
Looking for it every second or so would be enough.
This is a better than a busy loop but not by much. I would argue a
design that requires to poll after receiving an interrupt is broken.
If so, this doesn't sound great to me. I think it would be better to
have a flag in the page to indicate whether the page is not ready.
Xenstored could then clear the flag before raising the event channel.
Hmm, the "connection" field could be used for that.
I thought about the field but the description doesn't entirely match
what we want. In particular, the spec says only the guest should set the
value to 1 (i.e. reconnect). Maybe this could be relaxed?
It would mean, though, that e.g. libxl would need to initialize the
page accordingly before calling xs_introduce()
libxl only create domain paused. So I don't think it would be necessary
to update it.
Cheers,
--
Julien Grall
Re: [PATCH v4 2/9] xen/arm: implement domU extended regions
Hi Stefano,
> +#define ALIGN_UP_TO_2MB(x) (((x) + MB(2) - 1) & (~(MB(2) - 1)))
> +
> +static int __init find_domU_holes(const struct kernel_info *kinfo,
> + struct meminfo *ext_regions)
> +{
> +unsigned int i;
> +uint64_t bankend[GUEST_RAM_BANKS];
> +const uint64_t bankbase[] = GUEST_RAM_BANK_BASES;
> +const uint64_t banksize[] = GUEST_RAM_BANK_SIZES;
> +
> +for ( i = 0; i < GUEST_RAM_BANKS; i++ )
> +{
> +ext_regions->bank[ext_regions->nr_banks].start =
> +ALIGN_UP_TO_2MB(bankbase[i] + kinfo->mem.bank[i].size);
> +
> +bankend[i] = ~0ULL >> (64 - p2m_ipa_bits);
> +bankend[i] = min(bankend[i], bankbase[i] + banksize[i] - 1);
> +if (bankend[i] > ext_regions->bank[ext_regions->nr_banks].start)
Just a code style issue, the if needs a space before and after the condition
With this fixed:
Reviewed-by: Luca Fancellu
Cheers,
Luca
Re: [PATCH v4 8/9] tools: add example application to initialize dom0less PV drivers
On 01.04.22 15:35, Julien Grall wrote:
Hi Juergen,
On 01/04/2022 11:46, Juergen Gross wrote:
On 01.04.22 12:21, Julien Grall wrote:
Hi,
I have posted some comments in v3 after you sent this version. Please have a
look.
On 01/04/2022 01:38, Stefano Stabellini wrote:
+static int init_domain(struct xs_handle *xsh, libxl_dominfo *info)
+{
+ struct xc_interface_core *xch;
+ libxl_uuid uuid;
+ uint64_t xenstore_evtchn, xenstore_pfn;
+ int rc;
+
+ printf("Init dom0less domain: %u\n", info->domid);
+ xch = xc_interface_open(0, 0, 0);
+
+ rc = xc_hvm_param_get(xch, info->domid, HVM_PARAM_STORE_EVTCHN,
+ &xenstore_evtchn);
+ if (rc != 0) {
+ printf("Failed to get HVM_PARAM_STORE_EVTCHN\n");
+ return 1;
+ }
+
+ /* Alloc xenstore page */
+ if (alloc_xs_page(xch, info, &xenstore_pfn) != 0) {
+ printf("Error on alloc magic pages\n");
+ return 1;
+ }
+
+ rc = xc_dom_gnttab_seed(xch, info->domid, true,
+ (xen_pfn_t)-1, xenstore_pfn, 0, 0);
+ if (rc)
+ err(1, "xc_dom_gnttab_seed");
+
+ libxl_uuid_generate(&uuid);
+ xc_domain_sethandle(xch, info->domid, libxl_uuid_bytearray(&uuid));
+
+ rc = gen_stub_json_config(info->domid, &uuid);
+ if (rc)
+ err(1, "gen_stub_json_config");
+
+ /* Now everything is ready: set HVM_PARAM_STORE_PFN */
+ rc = xc_hvm_param_set(xch, info->domid, HVM_PARAM_STORE_PFN,
+ xenstore_pfn);
On patch #1, you told me you didn't want to allocate the page in Xen because
it wouldn't be initialized by Xenstored. But this is what we are doing here.
Xenstore (at least the C variant) is only using the fixed grant ref
GNTTAB_RESERVED_XENSTORE, so it doesn't need the page to be advertised
to the guest. And the mapping is done only when the domain is being
introduced to Xenstore.
And we don't expect the guest to use the grant entry to find the xenstore page?
This would be a problem if Linux is still booting and hasn't yet call
xenbus_probe_initcall().
I understand we need to have the page setup before raising the event channel.
I don't think we can allow Xenstored to set the HVM_PARAM (it may run in a
domain with less privilege). So I think we may need to create a separate
command to kick the client (not great).
Juergen, any thoughts?
I think it should work like that:
- setup the grant via xc_dom_gnttab_seed()
- introduce the domain to Xenstore
- call xc_hvm_param_set()
When the guest is receiving the event, it should wait for the xenstore
page to appear.
IIUC, this would mean the guest would need to do some sort of busy loop until
the xenstore page to appear.
Looking for it every second or so would be enough.
If so, this doesn't sound great to me. I think it would be better to have a flag
in the page to indicate whether the page is not ready.
Xenstored could then clear the flag before raising the event channel.
Hmm, the "connection" field could be used for that.
It would mean, though, that e.g. libxl would need to initialize the
page accordingly before calling xs_introduce().
Juergen
OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key
OpenPGP_signature
Description: OpenPGP digital signature
Re: [XEN PATCH] tools/libs/light/libxl_pci.c: explicitly grant access to Intel IGD opregion
On Fri, Apr 1, 2022 at 9:41 AM Chuck Zmudzinski wrote: > > On 4/1/22 9:21 AM, Chuck Zmudzinski wrote: > > On 3/30/22 2:45 PM, Jason Andryuk wrote: > >> On Fri, Mar 18, 2022 at 4:13 AM Jan Beulich wrote: > >>> On 14.03.2022 04:41, Chuck Zmudzinski wrote: > When gfx_passthru is enabled for the Intel IGD, hvmloader maps the IGD > opregion to the guest but libxl does not grant the guest permission to > access the mapped memory region. This results in a crash of the > i915.ko > kernel module in a Linux HVM guest when it needs to access the IGD > opregion: > > Oct 23 11:36:33 domU kernel: Call Trace: > Oct 23 11:36:33 domU kernel: ? idr_alloc+0x39/0x70 > Oct 23 11:36:33 domU kernel: drm_get_last_vbltimestamp+0xaa/0xc0 [drm] > Oct 23 11:36:33 domU kernel: drm_reset_vblank_timestamp+0x5b/0xd0 > [drm] > Oct 23 11:36:33 domU kernel: drm_crtc_vblank_on+0x7b/0x130 [drm] > Oct 23 11:36:33 domU kernel: > intel_modeset_setup_hw_state+0xbd4/0x1900 [i915] > Oct 23 11:36:33 domU kernel: ? _cond_resched+0x16/0x40 > Oct 23 11:36:33 domU kernel: ? ww_mutex_lock+0x15/0x80 > Oct 23 11:36:33 domU kernel: intel_modeset_init_nogem+0x867/0x1d30 > [i915] > Oct 23 11:36:33 domU kernel: ? gen6_write32+0x4b/0x1c0 [i915] > Oct 23 11:36:33 domU kernel: ? intel_irq_postinstall+0xb9/0x670 > [i915] > Oct 23 11:36:33 domU kernel: i915_driver_probe+0x5c2/0xc90 [i915] > Oct 23 11:36:33 domU kernel: ? > vga_switcheroo_client_probe_defer+0x1f/0x40 > Oct 23 11:36:33 domU kernel: ? i915_pci_probe+0x3f/0x150 [i915] > Oct 23 11:36:33 domU kernel: local_pci_probe+0x42/0x80 > Oct 23 11:36:33 domU kernel: ? _cond_resched+0x16/0x40 > Oct 23 11:36:33 domU kernel: pci_device_probe+0xfd/0x1b0 > Oct 23 11:36:33 domU kernel: really_probe+0x222/0x480 > Oct 23 11:36:33 domU kernel: driver_probe_device+0xe1/0x150 > Oct 23 11:36:33 domU kernel: device_driver_attach+0xa1/0xb0 > Oct 23 11:36:33 domU kernel: __driver_attach+0x8a/0x150 > Oct 23 11:36:33 domU kernel: ? device_driver_attach+0xb0/0xb0 > Oct 23 11:36:33 domU kernel: ? device_driver_attach+0xb0/0xb0 > Oct 23 11:36:33 domU kernel: bus_for_each_dev+0x78/0xc0 > Oct 23 11:36:33 domU kernel: bus_add_driver+0x12b/0x1e0 > Oct 23 11:36:33 domU kernel: driver_register+0x8b/0xe0 > Oct 23 11:36:33 domU kernel: ? 0xc06b8000 > Oct 23 11:36:33 domU kernel: i915_init+0x5d/0x70 [i915] > Oct 23 11:36:33 domU kernel: do_one_initcall+0x44/0x1d0 > Oct 23 11:36:33 domU kernel: ? do_init_module+0x23/0x260 > Oct 23 11:36:33 domU kernel: ? kmem_cache_alloc_trace+0xf5/0x200 > Oct 23 11:36:33 domU kernel: do_init_module+0x5c/0x260 > Oct 23 11:36:33 domU kernel: __do_sys_finit_module+0xb1/0x110 > Oct 23 11:36:33 domU kernel: do_syscall_64+0x33/0x80 > Oct 23 11:36:33 domU kernel: entry_SYSCALL_64_after_hwframe+0x44/0xa9 > >>> The call trace alone leaves open where exactly the crash occurred. > >>> Looking at 5.17 I notice that the first thing the driver does > >>> after mapping the range it to check the signature (both in > >>> intel_opregion_setup()). As the signature can't possibly match > >>> with no access granted to the underlying mappings, there shouldn't > >>> be any further attempts to use the region in the driver; if there > >>> are, I'd view this as a driver bug. > >> Yes. i915_driver_hw_probe does not check the return value of > >> intel_opregion_setup(dev_priv) and just continues on. > >> > >> Chuck, the attached patch may help if you want to test it. > >> > >> Regards, > >> Jason > > > > I tested the patch - it made no noticeable difference.I still > > get the same crash and call trace with the patch. Actually, > > the call trace I posted here is only the first of three call > > traces, and I still see all three call traces with the patch. Thanks for testing. Sorry it didn't help. > It is probably necessary to patch intet_opregion_setup to > return from it with an error sooner if the goal is to suppress > the call traces that occur when the driver cannot access > the opregion. It looks correct for 5.17 running in your domU. I thought the opregion signature check would fail. A failure in intel_opregion_setup would percolate up through i915_driver_hw_probe to i915_driver_probe. In i915_driver_probe the error should goto out_cleanup_mmio and skip intel_modeset_init_nogem which is in your backtrace. Regards, Jason
Re: preparations for 4.14.5 ?
On Wed, Mar 30, 2022 at 12:16:00PM +0200, Jan Beulich wrote: > All, > > while 4.14's general support period ended in January, we're considering > to cut an out-of-band release due to the relatively large number of > security relevant backports which has accumulated in just 2 months. By > doing this we would _not_ be promising to do so again in the future. > Please voice objections to the plan (or support for it) in the next > couple of days. > > Since it's a little easier to "batch" releases, I would intend to keep > 4.14.5 aligned with 4.16.1. > > Commits I have queued but not committed to the branch yet (and I won't > until in a couple of days time, to allow for objections to the plan to > be raised): > > dd6c062a7a4a tools/libxl: Correctly align the ACPI tables > aa390d513a67 build: fix exported variable name CFLAGS_stack_boundary > e62cc29f9b6c tools/libs: Fix build dependencies > eddf13b5e940 x86emul: fix VPBLENDMW with mask and memory operand > 6bd1b4d35c05 x86/console: process softirqs between warning prints > 07449ecfa425 tools/libxl: don't allow IOMMU usage with PoD > 10454f381f91 xz: avoid overlapping memcpy() with invalid input with in-place > decompression > 0a21660515c2 xz: validate the value before assigning it to an enum variable > b4f211606011 vpci/msix: fix PBA accesses > > Please point out backports you find missing from both the respective > staging branch and the list above, but which you consider relevant. I'm not sure if "just" bugfix qualify for 4.14 at this point, but if so, I'd propose: 0a20a53df158 tools/libs/light: set video_mem for PVH guests In any case, the above should be backported to 4.15 and 4.16. -- Best Regards, Marek Marczykowski-Górecki Invisible Things Lab signature.asc Description: PGP signature
Re: [XEN PATCH] tools/libs/light/libxl_pci.c: explicitly grant access to Intel IGD opregion
On 4/1/22 9:21 AM, Chuck Zmudzinski wrote: On 3/30/22 2:45 PM, Jason Andryuk wrote: On Fri, Mar 18, 2022 at 4:13 AM Jan Beulich wrote: On 14.03.2022 04:41, Chuck Zmudzinski wrote: When gfx_passthru is enabled for the Intel IGD, hvmloader maps the IGD opregion to the guest but libxl does not grant the guest permission to access the mapped memory region. This results in a crash of the i915.ko kernel module in a Linux HVM guest when it needs to access the IGD opregion: Oct 23 11:36:33 domU kernel: Call Trace: Oct 23 11:36:33 domU kernel: ? idr_alloc+0x39/0x70 Oct 23 11:36:33 domU kernel: drm_get_last_vbltimestamp+0xaa/0xc0 [drm] Oct 23 11:36:33 domU kernel: drm_reset_vblank_timestamp+0x5b/0xd0 [drm] Oct 23 11:36:33 domU kernel: drm_crtc_vblank_on+0x7b/0x130 [drm] Oct 23 11:36:33 domU kernel: intel_modeset_setup_hw_state+0xbd4/0x1900 [i915] Oct 23 11:36:33 domU kernel: ? _cond_resched+0x16/0x40 Oct 23 11:36:33 domU kernel: ? ww_mutex_lock+0x15/0x80 Oct 23 11:36:33 domU kernel: intel_modeset_init_nogem+0x867/0x1d30 [i915] Oct 23 11:36:33 domU kernel: ? gen6_write32+0x4b/0x1c0 [i915] Oct 23 11:36:33 domU kernel: ? intel_irq_postinstall+0xb9/0x670 [i915] Oct 23 11:36:33 domU kernel: i915_driver_probe+0x5c2/0xc90 [i915] Oct 23 11:36:33 domU kernel: ? vga_switcheroo_client_probe_defer+0x1f/0x40 Oct 23 11:36:33 domU kernel: ? i915_pci_probe+0x3f/0x150 [i915] Oct 23 11:36:33 domU kernel: local_pci_probe+0x42/0x80 Oct 23 11:36:33 domU kernel: ? _cond_resched+0x16/0x40 Oct 23 11:36:33 domU kernel: pci_device_probe+0xfd/0x1b0 Oct 23 11:36:33 domU kernel: really_probe+0x222/0x480 Oct 23 11:36:33 domU kernel: driver_probe_device+0xe1/0x150 Oct 23 11:36:33 domU kernel: device_driver_attach+0xa1/0xb0 Oct 23 11:36:33 domU kernel: __driver_attach+0x8a/0x150 Oct 23 11:36:33 domU kernel: ? device_driver_attach+0xb0/0xb0 Oct 23 11:36:33 domU kernel: ? device_driver_attach+0xb0/0xb0 Oct 23 11:36:33 domU kernel: bus_for_each_dev+0x78/0xc0 Oct 23 11:36:33 domU kernel: bus_add_driver+0x12b/0x1e0 Oct 23 11:36:33 domU kernel: driver_register+0x8b/0xe0 Oct 23 11:36:33 domU kernel: ? 0xc06b8000 Oct 23 11:36:33 domU kernel: i915_init+0x5d/0x70 [i915] Oct 23 11:36:33 domU kernel: do_one_initcall+0x44/0x1d0 Oct 23 11:36:33 domU kernel: ? do_init_module+0x23/0x260 Oct 23 11:36:33 domU kernel: ? kmem_cache_alloc_trace+0xf5/0x200 Oct 23 11:36:33 domU kernel: do_init_module+0x5c/0x260 Oct 23 11:36:33 domU kernel: __do_sys_finit_module+0xb1/0x110 Oct 23 11:36:33 domU kernel: do_syscall_64+0x33/0x80 Oct 23 11:36:33 domU kernel: entry_SYSCALL_64_after_hwframe+0x44/0xa9 The call trace alone leaves open where exactly the crash occurred. Looking at 5.17 I notice that the first thing the driver does after mapping the range it to check the signature (both in intel_opregion_setup()). As the signature can't possibly match with no access granted to the underlying mappings, there shouldn't be any further attempts to use the region in the driver; if there are, I'd view this as a driver bug. Yes. i915_driver_hw_probe does not check the return value of intel_opregion_setup(dev_priv) and just continues on. Chuck, the attached patch may help if you want to test it. Regards, Jason I tested the patch - it made no noticeable difference.I still get the same crash and call trace with the patch. Actually, the call trace I posted here is only the first of three call traces, and I still see all three call traces with the patch. It is probably necessary to patch intet_opregion_setup to return from it with an error sooner if the goal is to suppress the call traces that occur when the driver cannot access the opregion. Regards, Chuck
Re: [PATCH v4 8/9] tools: add example application to initialize dom0less PV drivers
Hi Juergen,
On 01/04/2022 11:46, Juergen Gross wrote:
On 01.04.22 12:21, Julien Grall wrote:
Hi,
I have posted some comments in v3 after you sent this version. Please
have a look.
On 01/04/2022 01:38, Stefano Stabellini wrote:
+static int init_domain(struct xs_handle *xsh, libxl_dominfo *info)
+{
+ struct xc_interface_core *xch;
+ libxl_uuid uuid;
+ uint64_t xenstore_evtchn, xenstore_pfn;
+ int rc;
+
+ printf("Init dom0less domain: %u\n", info->domid);
+ xch = xc_interface_open(0, 0, 0);
+
+ rc = xc_hvm_param_get(xch, info->domid, HVM_PARAM_STORE_EVTCHN,
+ &xenstore_evtchn);
+ if (rc != 0) {
+ printf("Failed to get HVM_PARAM_STORE_EVTCHN\n");
+ return 1;
+ }
+
+ /* Alloc xenstore page */
+ if (alloc_xs_page(xch, info, &xenstore_pfn) != 0) {
+ printf("Error on alloc magic pages\n");
+ return 1;
+ }
+
+ rc = xc_dom_gnttab_seed(xch, info->domid, true,
+ (xen_pfn_t)-1, xenstore_pfn, 0, 0);
+ if (rc)
+ err(1, "xc_dom_gnttab_seed");
+
+ libxl_uuid_generate(&uuid);
+ xc_domain_sethandle(xch, info->domid, libxl_uuid_bytearray(&uuid));
+
+ rc = gen_stub_json_config(info->domid, &uuid);
+ if (rc)
+ err(1, "gen_stub_json_config");
+
+ /* Now everything is ready: set HVM_PARAM_STORE_PFN */
+ rc = xc_hvm_param_set(xch, info->domid, HVM_PARAM_STORE_PFN,
+ xenstore_pfn);
On patch #1, you told me you didn't want to allocate the page in Xen
because it wouldn't be initialized by Xenstored. But this is what we
are doing here.
Xenstore (at least the C variant) is only using the fixed grant ref
GNTTAB_RESERVED_XENSTORE, so it doesn't need the page to be advertised
to the guest. And the mapping is done only when the domain is being
introduced to Xenstore.
And we don't expect the guest to use the grant entry to find the
xenstore page?
This would be a problem if Linux is still booting and hasn't yet call
xenbus_probe_initcall().
I understand we need to have the page setup before raising the event
channel. I don't think we can allow Xenstored to set the HVM_PARAM (it
may run in a domain with less privilege). So I think we may need to
create a separate command to kick the client (not great).
Juergen, any thoughts?
I think it should work like that:
- setup the grant via xc_dom_gnttab_seed()
- introduce the domain to Xenstore
- call xc_hvm_param_set()
When the guest is receiving the event, it should wait for the xenstore
page to appear.
IIUC, this would mean the guest would need to do some sort of busy loop
until the xenstore page to appear.
If so, this doesn't sound great to me. I think it would be better to
have a flag in the page to indicate whether the page is not ready.
Xenstored could then clear the flag before raising the event channel.
Cheers,
--
Julien Grall
[qemu-mainline test] 169072: tolerable FAIL - PUSHED
flight 169072 qemu-mainline real [real] flight 169109 qemu-mainline real-retest [real] http://logs.test-lab.xenproject.org/osstest/logs/169072/ http://logs.test-lab.xenproject.org/osstest/logs/169109/ Failures :-/ but no regressions. Tests which are failing intermittently (not blocking): test-amd64-amd64-qemuu-nested-intel 20 debian-hvm-install/l1/l2 fail pass in 169109-retest Tests which did not succeed, but are not blocking: test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 169017 test-armhf-armhf-libvirt 16 saverestore-support-checkfail like 169017 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 169017 test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 169017 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check fail like 169017 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail like 169017 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 169017 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 169017 test-amd64-i386-xl-pvshim14 guest-start fail never pass test-arm64-arm64-xl-seattle 15 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 16 saverestore-support-checkfail never pass test-amd64-i386-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-amd64-i386-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-arm64-arm64-xl-credit2 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit1 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-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-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 16 saverestore-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-amd64-i386-libvirt-raw 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-amd64-amd64-libvirt-vhd 14 migrate-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-credit1 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 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-xl-cubietruck 15 migrate-support-checkfail never pass test-armhf-armhf-xl-cubietruck 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-rtds 15 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 16 saverestore-support-checkfail never pass test-armhf-armhf-libvirt 15 migrate-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-libvirt-qcow2 14 migrate-support-checkfail never pass test-armhf-armhf-libvirt-raw 14 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 14 migrate-support-checkfail never pass test-armhf-armhf-xl-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: qemuucace6c6f3aca7b88afc42995f90bbefb37a0ed57 baseline version: qemuuaea6e471085f39ada1ccd637043c3ee3dfd88750 Last test of basis 169017 2022-03-30
Re: [XEN PATCH] tools/libs/light/libxl_pci.c: explicitly grant access to Intel IGD opregion
On 3/30/22 2:45 PM, Jason Andryuk wrote: On Fri, Mar 18, 2022 at 4:13 AM Jan Beulich wrote: On 14.03.2022 04:41, Chuck Zmudzinski wrote: When gfx_passthru is enabled for the Intel IGD, hvmloader maps the IGD opregion to the guest but libxl does not grant the guest permission to access the mapped memory region. This results in a crash of the i915.ko kernel module in a Linux HVM guest when it needs to access the IGD opregion: Oct 23 11:36:33 domU kernel: Call Trace: Oct 23 11:36:33 domU kernel: ? idr_alloc+0x39/0x70 Oct 23 11:36:33 domU kernel: drm_get_last_vbltimestamp+0xaa/0xc0 [drm] Oct 23 11:36:33 domU kernel: drm_reset_vblank_timestamp+0x5b/0xd0 [drm] Oct 23 11:36:33 domU kernel: drm_crtc_vblank_on+0x7b/0x130 [drm] Oct 23 11:36:33 domU kernel: intel_modeset_setup_hw_state+0xbd4/0x1900 [i915] Oct 23 11:36:33 domU kernel: ? _cond_resched+0x16/0x40 Oct 23 11:36:33 domU kernel: ? ww_mutex_lock+0x15/0x80 Oct 23 11:36:33 domU kernel: intel_modeset_init_nogem+0x867/0x1d30 [i915] Oct 23 11:36:33 domU kernel: ? gen6_write32+0x4b/0x1c0 [i915] Oct 23 11:36:33 domU kernel: ? intel_irq_postinstall+0xb9/0x670 [i915] Oct 23 11:36:33 domU kernel: i915_driver_probe+0x5c2/0xc90 [i915] Oct 23 11:36:33 domU kernel: ? vga_switcheroo_client_probe_defer+0x1f/0x40 Oct 23 11:36:33 domU kernel: ? i915_pci_probe+0x3f/0x150 [i915] Oct 23 11:36:33 domU kernel: local_pci_probe+0x42/0x80 Oct 23 11:36:33 domU kernel: ? _cond_resched+0x16/0x40 Oct 23 11:36:33 domU kernel: pci_device_probe+0xfd/0x1b0 Oct 23 11:36:33 domU kernel: really_probe+0x222/0x480 Oct 23 11:36:33 domU kernel: driver_probe_device+0xe1/0x150 Oct 23 11:36:33 domU kernel: device_driver_attach+0xa1/0xb0 Oct 23 11:36:33 domU kernel: __driver_attach+0x8a/0x150 Oct 23 11:36:33 domU kernel: ? device_driver_attach+0xb0/0xb0 Oct 23 11:36:33 domU kernel: ? device_driver_attach+0xb0/0xb0 Oct 23 11:36:33 domU kernel: bus_for_each_dev+0x78/0xc0 Oct 23 11:36:33 domU kernel: bus_add_driver+0x12b/0x1e0 Oct 23 11:36:33 domU kernel: driver_register+0x8b/0xe0 Oct 23 11:36:33 domU kernel: ? 0xc06b8000 Oct 23 11:36:33 domU kernel: i915_init+0x5d/0x70 [i915] Oct 23 11:36:33 domU kernel: do_one_initcall+0x44/0x1d0 Oct 23 11:36:33 domU kernel: ? do_init_module+0x23/0x260 Oct 23 11:36:33 domU kernel: ? kmem_cache_alloc_trace+0xf5/0x200 Oct 23 11:36:33 domU kernel: do_init_module+0x5c/0x260 Oct 23 11:36:33 domU kernel: __do_sys_finit_module+0xb1/0x110 Oct 23 11:36:33 domU kernel: do_syscall_64+0x33/0x80 Oct 23 11:36:33 domU kernel: entry_SYSCALL_64_after_hwframe+0x44/0xa9 The call trace alone leaves open where exactly the crash occurred. Looking at 5.17 I notice that the first thing the driver does after mapping the range it to check the signature (both in intel_opregion_setup()). As the signature can't possibly match with no access granted to the underlying mappings, there shouldn't be any further attempts to use the region in the driver; if there are, I'd view this as a driver bug. Yes. i915_driver_hw_probe does not check the return value of intel_opregion_setup(dev_priv) and just continues on. Chuck, the attached patch may help if you want to test it. Regards, Jason I tested the patch - it made no noticeable difference. I still get the same crash and call trace with the patch. Actually, the call trace I posted here is only the first of three call traces, and I still see all three call traces with the patch. The patch is harmless and the i915 module with the patch works normally when it can access the intel opregion. Regards, Chuck
Re: [PATCH v4 7/9] xenstored: send an evtchn notification on introduce_domain
On 01.04.22 02:38, Stefano Stabellini wrote: From: Luca Miccio When xs_introduce_domain is called, send out a notification on the xenstore event channel so that any (dom0less) domain waiting for the xenstore interface to be ready can continue with the initialization. The extra notification is harmless for domains that don't require it. Signed-off-by: Luca Miccio Signed-off-by: Stefano Stabellini Reviewed-by: Bertrand Marquis Reviewed-by: Juergen Gross Juergen OpenPGP_0xB0DE9DD628BF132F.asc Description: OpenPGP public key OpenPGP_signature Description: OpenPGP digital signature
Re: [PATCH v2 00/14] x86/mm: large parts of P2M code and struct p2m_domain are HVM-only
> On Feb 23, 2022, at 3:55 PM, Jan Beulich wrote:
>
> The primary goal of this series is to leave p2m.c with, as its leading
> comment suggests, just code for "physical-to-machine mappings for
> automatically-translated domains". This requires splitting a few
> functions, with their non-HVM parts moved elsewhere.
>
> There aren't many changes in v2, mostly from re-basing. See individual
> patches for details.
>
> 01: x86/P2M: rename p2m_remove_page()
> 02: x86/P2M: introduce p2m_{add,remove}_page()
> 03: x86/mm: move guest_physmap_{add,remove}_page()
> 04: x86/mm: split set_identity_p2m_entry() into PV and HVM parts
> 05: x86/P2M: p2m_{alloc,free}_ptp() and p2m_alloc_table() are HVM-only
> 06: x86/P2M: PoD, altp2m, and nested-p2m are HVM-only
> 07: x86/P2M: split out init/teardown functions
> 08: x86/P2M: p2m_get_page_from_gfn() is HVM-only
> 09: x86/P2M: derive a HVM-only variant from __get_gfn_type_access()
> 10: x86/p2m: re-arrange {,__}put_gfn()
> 11: shr_pages field is MEM_SHARING-only
> 12: paged_pages field is MEM_PAGING-only
> 13: x86/P2M: p2m.c is HVM-only
> 14: x86/P2M: the majority for struct p2m_domain's fields are HVM-only
OK, I think every patch has an R-b from me on it now — let me know if I missed
anything.
-George
signature.asc
Description: Message signed with OpenPGP
Re: [PATCH v2 06/14] x86/P2M: PoD, altp2m, and nested-p2m are HVM-only
> On Feb 23, 2022, at 4:01 PM, Jan Beulich wrote:
>
> There's no need to initialize respective data for PV domains. Note that
> p2m_teardown_{alt,nested}p2m() will handle the lack-of-initialization
> case fine.
>
> As a result, despite PV domains having a host P2M associated with them
> and hence using XENMEM_get_pod_target on such may not be a real problem,
> calling p2m_pod_set_mem_target() for a PV domain is surely wrong, even
> if benign at present. Add a guard there as well.
>
> In p2m_pod_demand_populate() the situation is a little different: This
> function is reachable only for HVM domains anyway, but following from
> other PoD functions only ever acting on the host P2M (and hence PoD
> entries only ever existing in host P2Ms), assert and bail from there for
> non-host-P2Ms.
>
> Signed-off-by: Jan Beulich
Thanks,
Reviewed-by: George Dunlap
> Perhaps p2m_pod_init() could be invoked from p2m_init_hostp2m(), leaving
> all other p2m's PoD state uninitialized. Of course at that point the
> question would be whether the PoD pieces of struct p2m_domain wouldn't
> better move into a separate structure, present only for host P2Ms.
> Together with the p2m_pod_demand_populate() adjustment this might then
> better be a separate change ...
I’d certainly be tempted to do that kind of clean-up.
I would just check this patch in as-is, but if you really want to pull the
p2m_pod_demand_populate() adjustment into a separate patch to keep everything
in the same place, feel free to drop that while retaining my R-b.
-George
signature.asc
Description: Message signed with OpenPGP
Re: [PATCH v2 04/14] x86/mm: split set_identity_p2m_entry() into PV and HVM parts
> On Feb 23, 2022, at 3:59 PM, Jan Beulich wrote: > > ..., moving the former into the new physmap.c. Also call the new > functions directly from arch_iommu_hwdom_init() and > vpci_make_msix_hole(), as the PV/HVM split is explicit there. > > Signed-off-by: Jan Beulich > Reviewed-by: George Dunlap “citrix” is spelled wrong in the email address — not sure whether it’s my typo or yours. :-) > --- > v2: Change arch_iommu_hwdom_init() and vpci_make_msix_hole(). I think the R-b should probably have been dropped for this change, but in any case: Reviewed-by: George Dunlap signature.asc Description: Message signed with OpenPGP
Re: [PATCH v2 02/14] x86/P2M: introduce p2m_{add,remove}_page()
> On Feb 23, 2022, at 3:58 PM, Jan Beulich wrote: > > Rename guest_physmap_add_entry() to p2m_add_page(); make > guest_physmap_remove_page() a trivial wrapper around p2m_remove_page(). > This way callers can use suitable pairs of functions (previously > violated by hvm/grant_table.c). > > In HVM-specific code further avoid going through the guest_physmap_*() > layer, and instead use the two new/renamed functions directly. > > Ultimately the goal is to have guest_physmap_...() functions cover all > types of guests, but p2m_...() dealing only with translated ones. > > Signed-off-by: Jan Beulich > Reviewed-by: Paul Durrant Description reads much better to me — thanks! Reviewed-by: George Dunlap signature.asc Description: Message signed with OpenPGP
Re: [PATCH v2 01/14] x86/P2M: rename p2m_remove_page()
> On Feb 23, 2022, at 3:57 PM, Jan Beulich wrote: > > This is in preparation to re-using the original name. > > Signed-off-by: Jan Beulich Reviewed-by: George Dunlap signature.asc Description: Message signed with OpenPGP
Re: [PATCH v4 9/9] docs: document dom0less + PV drivers
> On 1 Apr 2022, at 01:38, Stefano Stabellini wrote: > > From: Stefano Stabellini > > Document how to use the feature and how the implementation works. > > Signed-off-by: Stefano Stabellini Hi Stefano, Reviewed-by: Luca Fancellu
[PATCH v9 1/2] xen+tools: Report Interrupt Controller Virtualization capabilities on x86
Add XEN_SYSCTL_PHYSCAP_X86_ASSISTED_XAPIC and
XEN_SYSCTL_PHYSCAP_X86_ASSISTED_X2APIC to report accelerated xAPIC and
x2APIC, on x86 hardware. This is so that xAPIC and x2APIC virtualization
can subsequently be enabled on a per-domain basis.
No such features are currently implemented on AMD hardware.
HW assisted xAPIC virtualization will be reported if HW, at the
minimum, supports virtualize_apic_accesses as this feature alone means
that an access to the APIC page will cause an APIC-access VM exit. An
APIC-access VM exit provides a VMM with information about the access
causing the VM exit, unlike a regular EPT fault, thus simplifying some
internal handling.
HW assisted x2APIC virtualization will be reported if HW supports
virtualize_x2apic_mode and, at least, either apic_reg_virt or
virtual_intr_delivery. This also means that
sysctl follows the conditionals in vmx_vlapic_msr_changed().
For that purpose, also add an arch-specific "capabilities" parameter
to struct xen_sysctl_physinfo.
Note that this interface is intended to be compatible with AMD so that
AVIC support can be introduced in a future patch. Unlike Intel that
has multiple controls for APIC Virtualization, AMD has one global
'AVIC Enable' control bit, so fine-graining of APIC virtualization
control cannot be done on a common interface.
Suggested-by: Andrew Cooper
Signed-off-by: Jane Malalane
---
CC: Wei Liu
CC: Anthony PERARD
CC: Juergen Gross
CC: Andrew Cooper
CC: George Dunlap
CC: Jan Beulich
CC: Julien Grall
CC: Stefano Stabellini
CC: Volodymyr Babchuk
CC: Bertrand Marquis
CC: Jun Nakajima
CC: Kevin Tian
CC: "Roger Pau Monné"
v9:
* Move assisted_x{2}apic_available to vmx_vmcs_init() so they get
declared at boot time, after vmx_secondary_exec_control is set
v8:
* Improve commit message
v7:
* Make sure assisted_x{2}apic_available evaluates to false, to ensure
Xen builds, when !CONFIG_HVM
* Fix coding style issues
v6:
* Limit abi check to x86
* Fix coding style issue
v5:
* Have assisted_xapic_available solely depend on
cpu_has_vmx_virtualize_apic_accesses and assisted_x2apic_available
depend on cpu_has_vmx_virtualize_x2apic_mode and
cpu_has_vmx_apic_reg_virt OR cpu_has_vmx_virtual_intr_delivery
v4:
* Fallback to the original v2/v1 conditions for setting
assisted_xapic_available and assisted_x2apic_available so that in
the future APIC virtualization can be exposed on AMD hardware
since fine-graining of "AVIC" is not supported, i.e., AMD solely
uses "AVIC Enable". This also means that sysctl mimics what's
exposed in CPUID
v3:
* Define XEN_SYSCTL_PHYSCAP_ARCH_MAX for ABI checking and actually
set "arch_capbilities", via a call to c_bitmap_to_ocaml_list()
* Have assisted_x2apic_available only depend on
cpu_has_vmx_virtualize_x2apic_mode
v2:
* Use one macro LIBXL_HAVE_PHYSINFO_ASSISTED_APIC instead of two
* Pass xcpyshinfo as a pointer in libxl__arch_get_physinfo
* Set assisted_x{2}apic_available to be conditional upon "bsp" and
annotate it with __ro_after_init
* Change XEN_SYSCTL_PHYSCAP_ARCH_ASSISTED_X{2}APIC to
_X86_ASSISTED_X{2}APIC
* Keep XEN_SYSCTL_PHYSCAP_X86_ASSISTED_X{2}APIC contained within
sysctl.h
* Fix padding introduced in struct xen_sysctl_physinfo and bump
XEN_SYSCTL_INTERFACE_VERSION
---
tools/golang/xenlight/helpers.gen.go | 4
tools/golang/xenlight/types.gen.go | 2 ++
tools/include/libxl.h| 7 +++
tools/libs/light/libxl.c | 3 +++
tools/libs/light/libxl_arch.h| 4
tools/libs/light/libxl_arm.c | 5 +
tools/libs/light/libxl_types.idl | 2 ++
tools/libs/light/libxl_x86.c | 11 +++
tools/ocaml/libs/xc/xenctrl.ml | 5 +
tools/ocaml/libs/xc/xenctrl.mli | 5 +
tools/ocaml/libs/xc/xenctrl_stubs.c | 15 +--
tools/xl/xl_info.c | 6 --
xen/arch/x86/hvm/hvm.c | 3 +++
xen/arch/x86/hvm/vmx/vmcs.c | 6 ++
xen/arch/x86/include/asm/hvm/hvm.h | 5 +
xen/arch/x86/sysctl.c| 4
xen/include/public/sysctl.h | 11 ++-
17 files changed, 93 insertions(+), 5 deletions(-)
diff --git a/tools/golang/xenlight/helpers.gen.go
b/tools/golang/xenlight/helpers.gen.go
index b746ff1081..dd4e6c9f14 100644
--- a/tools/golang/xenlight/helpers.gen.go
+++ b/tools/golang/xenlight/helpers.gen.go
@@ -3373,6 +3373,8 @@ x.CapVmtrace = bool(xc.cap_vmtrace)
x.CapVpmu = bool(xc.cap_vpmu)
x.CapGnttabV1 = bool(xc.cap_gnttab_v1)
x.CapGnttabV2 = bool(xc.cap_gnttab_v2)
+x.CapAssistedXapic = bool(xc.cap_assisted_xapic)
+x.CapAssistedX2Apic = bool(xc.cap_assisted_x2apic)
return nil}
@@ -3407,6 +3409,8 @@ xc.cap_vmtrace = C.bool(x.CapVmtrace)
xc.cap_vpmu = C.bool(x.CapVpmu)
xc.cap_gnttab_v1 = C.bool(x.CapGnttabV1)
xc.cap_gnttab_v2 = C.bool(x.CapGnttabV2)
+xc.cap_assisted_xapic = C.bool(x.CapAssistedXapic)
+xc.cap_assisted_x2apic = C.bool(x.CapAssistedX2Apic)
ret
[PATCH v9 0/2] xen: Report and use hardware APIC virtualization capabilities
Jane Malalane (2): xen+tools: Report Interrupt Controller Virtualization capabilities on x86 x86/xen: Allow per-domain usage of hardware virtualized APIC docs/man/xl.cfg.5.pod.in | 15 ++ docs/man/xl.conf.5.pod.in | 12 +++ tools/golang/xenlight/helpers.gen.go | 16 ++ tools/golang/xenlight/types.gen.go| 4 tools/include/libxl.h | 14 + tools/libs/light/libxl.c | 3 +++ tools/libs/light/libxl_arch.h | 9 ++-- tools/libs/light/libxl_arm.c | 14 ++--- tools/libs/light/libxl_create.c | 22 tools/libs/light/libxl_types.idl | 4 tools/libs/light/libxl_x86.c | 39 +-- tools/ocaml/libs/xc/xenctrl.ml| 7 +++ tools/ocaml/libs/xc/xenctrl.mli | 7 +++ tools/ocaml/libs/xc/xenctrl_stubs.c | 17 --- tools/xl/xl.c | 8 +++ tools/xl/xl.h | 2 ++ tools/xl/xl_info.c| 6 -- tools/xl/xl_parse.c | 19 + xen/arch/x86/domain.c | 29 +- xen/arch/x86/hvm/hvm.c| 3 +++ xen/arch/x86/hvm/vmx/vmcs.c | 10 + xen/arch/x86/hvm/vmx/vmx.c| 13 xen/arch/x86/include/asm/hvm/domain.h | 6 ++ xen/arch/x86/include/asm/hvm/hvm.h| 10 + xen/arch/x86/sysctl.c | 4 xen/arch/x86/traps.c | 5 +++-- xen/include/public/arch-x86/xen.h | 5 + xen/include/public/sysctl.h | 11 +- 28 files changed, 280 insertions(+), 34 deletions(-) -- 2.11.0
[PATCH v9 2/2] x86/xen: Allow per-domain usage of hardware virtualized APIC
Introduce a new per-domain creation x86 specific flag to
select whether hardware assisted virtualization should be used for
x{2}APIC.
A per-domain option is added to xl in order to select the usage of
x{2}APIC hardware assisted virtualization, as well as a global
configuration option.
Having all APIC interaction exit to Xen for emulation is slow and can
induce much overhead. Hardware can speed up x{2}APIC by decoding the
APIC access and providing a VM exit with a more specific exit reason
than a regular EPT fault or by altogether avoiding a VM exit.
On the other hand, being able to disable x{2}APIC hardware assisted
virtualization can be useful for testing and debugging purposes.
Note:
- vmx_install_vlapic_mapping doesn't require modifications regardless
of whether the guest has "Virtualize APIC accesses" enabled or not,
i.e., setting the APIC_ACCESS_ADDR VMCS field is fine so long as
virtualize_apic_accesses is supported by the CPU.
- Both per-domain and global assisted_x{2}apic options are not part of
the migration stream, unless explicitly set in the configuration file,
so it is safe to migrate a guest that doesn't have assisted_x{2}apic
set in its config file between hosts that have different support for
hardware assisted x{2}APIC virtualization.
Suggested-by: Andrew Cooper
Signed-off-by: Jane Malalane
---
CC: Wei Liu
CC: Anthony PERARD
CC: Juergen Gross
CC: Andrew Cooper
CC: George Dunlap
CC: Jan Beulich
CC: Julien Grall
CC: Stefano Stabellini
CC: Christian Lindig
CC: David Scott
CC: Volodymyr Babchuk
CC: "Roger Pau Monné"
v9:
* Fix style issues
* Fix exit() logic for assisted_x{2}apic parsing
* Add and use XEN_X86_MISC_FLAGS_MAX for ABI checking instead of
using XEN_X86_ASSISTED_X2APIC directly
* Expand commit message to mention migration is safe
v8:
* Widen assisted_x{2}apic parsing to PVH guests in
parse_config_data()
v7:
* Fix void return in libxl__arch_domain_build_info_setdefault
* Fix style issues
* Use EINVAL when rejecting assisted_x{2}apic for PV guests and
ENODEV otherwise, when assisted_x{2}apic isn't supported
* Define has_assisted_x{2}apic macros for when !CONFIG_HVM
* Replace "EPT" fault reference with "p2m" fault since the former is
Intel-specific
v6:
* Use ENODEV instead of EINVAL when rejecting assisted_x{2}apic
for PV guests
* Move has_assisted_x{2}apic macros out of an Intel specific header
* Remove references to Intel specific features in documentation
v5:
* Revert v4 changes in vmx_vlapic_msr_changed(), preserving the use of
the has_assisted_x{2}apic macros
* Following changes in assisted_x{2}apic_available definitions in
patch 1, retighten conditionals for setting
XEN_HVM_CPUID_APIC_ACCESS_VIRT and XEN_HVM_CPUID_X2APIC_VIRT in
cpuid_hypervisor_leaves()
v4:
* Add has_assisted_x{2}apic macros and use them where appropriate
* Replace CPU checks with per-domain assisted_x{2}apic control
options in vmx_vlapic_msr_changed() and cpuid_hypervisor_leaves(),
following edits to assisted_x{2}apic_available definitions in
patch 1
Note: new assisted_x{2}apic_available definitions make later
cpu_has_vmx_apic_reg_virt and cpu_has_vmx_virtual_intr_delivery
checks redundant in vmx_vlapic_msr_changed()
v3:
* Change info in xl.cfg to better express reality and fix
capitalization of x{2}apic
* Move "physinfo" variable definition to the beggining of
libxl__domain_build_info_setdefault()
* Reposition brackets in if statement to match libxl coding style
* Shorten logic in libxl__arch_domain_build_info_setdefault()
* Correct dprintk message in arch_sanitise_domain_config()
* Make appropriate changes in vmx_vlapic_msr_changed() and
cpuid_hypervisor_leaves() for amended "assisted_x2apic" bit
* Remove unneeded parantheses
v2:
* Add a LIBXL_HAVE_ASSISTED_APIC macro
* Pass xcpyshinfo as a pointer in libxl__arch_get_physinfo
* Add a return statement in now "int"
libxl__arch_domain_build_info_setdefault()
* Preserve libxl__arch_domain_build_info_setdefault 's location in
libxl_create.c
* Correct x{2}apic default setting logic in
libxl__arch_domain_prepare_config()
* Correct logic for parsing assisted_x{2}apic host/guest options in
xl_parse.c and initialize them to -1 in xl.c
* Use guest options directly in vmx_vlapic_msr_changed
* Fix indentation of bool assisted_x{2}apic in struct hvm_domain
* Add a change in xenctrl_stubs.c to pass xenctrl ABI checks
---
docs/man/xl.cfg.5.pod.in | 15 +++
docs/man/xl.conf.5.pod.in | 12
tools/golang/xenlight/helpers.gen.go | 12
tools/golang/xenlight/types.gen.go| 2 ++
tools/include/libxl.h | 7 +++
tools/libs/light/libxl_arch.h | 5 +++--
tools/libs/light/libxl_arm.c | 9 ++---
tools/libs/light/libxl_create.c | 22 +-
tools/libs/light/libxl_types.idl | 2 ++
tools/libs/light/libxl_x86.c | 28 +++
Re: [PATCH v4 8/9] tools: add example application to initialize dom0less PV drivers
On 01.04.22 12:21, Julien Grall wrote:
Hi,
I have posted some comments in v3 after you sent this version. Please have a
look.
On 01/04/2022 01:38, Stefano Stabellini wrote:
+static int init_domain(struct xs_handle *xsh, libxl_dominfo *info)
+{
+ struct xc_interface_core *xch;
+ libxl_uuid uuid;
+ uint64_t xenstore_evtchn, xenstore_pfn;
+ int rc;
+
+ printf("Init dom0less domain: %u\n", info->domid);
+ xch = xc_interface_open(0, 0, 0);
+
+ rc = xc_hvm_param_get(xch, info->domid, HVM_PARAM_STORE_EVTCHN,
+ &xenstore_evtchn);
+ if (rc != 0) {
+ printf("Failed to get HVM_PARAM_STORE_EVTCHN\n");
+ return 1;
+ }
+
+ /* Alloc xenstore page */
+ if (alloc_xs_page(xch, info, &xenstore_pfn) != 0) {
+ printf("Error on alloc magic pages\n");
+ return 1;
+ }
+
+ rc = xc_dom_gnttab_seed(xch, info->domid, true,
+ (xen_pfn_t)-1, xenstore_pfn, 0, 0);
+ if (rc)
+ err(1, "xc_dom_gnttab_seed");
+
+ libxl_uuid_generate(&uuid);
+ xc_domain_sethandle(xch, info->domid, libxl_uuid_bytearray(&uuid));
+
+ rc = gen_stub_json_config(info->domid, &uuid);
+ if (rc)
+ err(1, "gen_stub_json_config");
+
+ /* Now everything is ready: set HVM_PARAM_STORE_PFN */
+ rc = xc_hvm_param_set(xch, info->domid, HVM_PARAM_STORE_PFN,
+ xenstore_pfn);
On patch #1, you told me you didn't want to allocate the page in Xen because it
wouldn't be initialized by Xenstored. But this is what we are doing here.
Xenstore (at least the C variant) is only using the fixed grant ref
GNTTAB_RESERVED_XENSTORE, so it doesn't need the page to be advertised
to the guest. And the mapping is done only when the domain is being
introduced to Xenstore.
This would be a problem if Linux is still booting and hasn't yet call
xenbus_probe_initcall().
I understand we need to have the page setup before raising the event channel. I
don't think we can allow Xenstored to set the HVM_PARAM (it may run in a domain
with less privilege). So I think we may need to create a separate command to
kick the client (not great).
Juergen, any thoughts?
I think it should work like that:
- setup the grant via xc_dom_gnttab_seed()
- introduce the domain to Xenstore
- call xc_hvm_param_set()
When the guest is receiving the event, it should wait for the xenstore
page to appear.
Juergen
OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key
OpenPGP_signature
Description: OpenPGP digital signature
Re: [PATCH v3 5/5] tools: add example application to initialize dom0less PV drivers
On 01.04.22 12:02, Julien Grall wrote:
Hi Stefano,
On 01/04/2022 01:35, Stefano Stabellini wrote:
+
+ /* Alloc magic pages */
+ if (alloc_magic_pages(info, &dom) != 0) {
+ printf("Error on alloc magic pages\n");
+ return 1;
+ }
+
+ xc_dom_gnttab_init(&dom);
This call as the risk to break the guest if the dom0 Linux doesn't support
the
acquire interface. This is because it will punch a hole in the domain
memory
where the grant-table may have already been mapped.
Also, this function could fails.
I'll check for return errors. Dom0less is for fully static
configurations so I think it is OK to return error and abort if
something unexpected happens: dom0less' main reason for being is that
there is nothing unexpected :-)
Does this mean the caller will have to reboot the system if there is an error?
IOW, we don't expect them to call ./init-dom0less twice.
Yes, exactly. I think init-dom0less could even panic. My mental model is
that this is an "extension" of construct_domU. Over there we just panic
if something is wrong and here it would be similar. The user provided a
wrong config and should fix it.
Ok. I think we should make explicit how it can be used.
+
+ libxl_uuid_generate(&uuid);
+ xc_domain_sethandle(dom.xch, info->domid,
libxl_uuid_bytearray(&uuid));
+
+ rc = gen_stub_json_config(info->domid, &uuid);
+ if (rc)
+ err(1, "gen_stub_json_config");
+
+ rc = restore_xenstore(xsh, info, uuid, dom.xenstore_evtchn);
+ if (rc)
+ err(1, "writing to xenstore");
+
+ xs_introduce_domain(xsh, info->domid,
+ (GUEST_MAGIC_BASE >> XC_PAGE_SHIFT) + XENSTORE_PFN_OFFSET,
+ dom.xenstore_evtchn);
xs_introduce_domain() can technically fails.
OK
+ return 0;
+}
+
+/* Check if domain has been configured in XS */
+static bool domain_exists(struct xs_handle *xsh, int domid)
+{
+ return xs_is_domain_introduced(xsh, domid);
+}
Would not this lead to initialize a domain with PV driver disabled?
I am not sure I understood your question, but I'll try to answer anyway.
This check is purely to distinguish dom0less guests, which needs further
initializations, from regular guests (e.g. xl guests) that don't need
any actions taken here.
Dom0less domUs can be divided in two categories based on whether they are xen
aware (e.g. xen,enhanced is set).
Looking at this script, it seems to assume that all dom0less domUs are Xen
aware. So it will end up to allocate Xenstore ring and call
xs_introduce_domain(). I suspect the call will end up to fail because the
event channel would be 0.
So did you try to use this script on a platform where there only xen aware
domU and/or a mix?
Good idea of asking for this test. I thought I already ran that test,
but I did it again to be sure. Everything works OK (although the
xenstore page allocation is unneeded). xs_introduce_domain does not
> fail:
Are you sure? If I pass 0 as the 4th argument (event channel), the command will
return EINVAL. However, looking at the code you are not checking the return for
the call. So you will continue as if it were successful.
So you will end up to write nodes for a domain Xenstored is not aware and also
set HVM_PARAM_STORE_PFN which may further confuse the guest as it may try to
initialize Xenstored it discovers the page.
I think that's because it is usually called on all domains by the
toolstack, even the ones without xenstore support in the kernel.
The toolstack will always allocate the event channel irrespective to whether the
guest will use Xenstore. So both the shared page and the event channel are
always valid today.
With your series, this will change as the event channel will not be allocated
when "xen,enhanced" is not set.
In your case, I think we may want to register the domain to xenstore but say
there are no connection available for the domain. Juergen, what do you think?
In my opinion such a domain should not be registered with Xenstore.
At least C-xenstored should work mostly correctly. I think the
"introduced" status is tested everywhere it should be tested.
Basically this is similar to today's status of xenstore-stubdom: it
is never introduced, but Xenstore itself is happy with it existing. :-)
And even today the first nodes for a new domain are being created
before the domain is officially introduced.
Juergen
OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key
OpenPGP_signature
Description: OpenPGP digital signature
Re: [PATCH v3 5/5] tools: add example application to initialize dom0less PV drivers
On 01/04/2022 11:02, Julien Grall wrote:
Hi Stefano,
On 01/04/2022 01:35, Stefano Stabellini wrote:
+
+ /* Alloc magic pages */
+ if (alloc_magic_pages(info, &dom) != 0) {
+ printf("Error on alloc magic pages\n");
+ return 1;
+ }
+
+ xc_dom_gnttab_init(&dom);
This call as the risk to break the guest if the dom0 Linux doesn't
support
the
acquire interface. This is because it will punch a hole in the domain
memory
where the grant-table may have already been mapped.
Also, this function could fails.
I'll check for return errors. Dom0less is for fully static
configurations so I think it is OK to return error and abort if
something unexpected happens: dom0less' main reason for being is that
there is nothing unexpected :-)
Does this mean the caller will have to reboot the system if there is
an error?
IOW, we don't expect them to call ./init-dom0less twice.
Yes, exactly. I think init-dom0less could even panic. My mental model is
that this is an "extension" of construct_domU. Over there we just panic
if something is wrong and here it would be similar. The user provided a
wrong config and should fix it.
Ok. I think we should make explicit how it can be used.
+
+ libxl_uuid_generate(&uuid);
+ xc_domain_sethandle(dom.xch, info->domid,
libxl_uuid_bytearray(&uuid));
+
+ rc = gen_stub_json_config(info->domid, &uuid);
+ if (rc)
+ err(1, "gen_stub_json_config");
+
+ rc = restore_xenstore(xsh, info, uuid, dom.xenstore_evtchn);
+ if (rc)
+ err(1, "writing to xenstore");
+
+ xs_introduce_domain(xsh, info->domid,
+ (GUEST_MAGIC_BASE >> XC_PAGE_SHIFT) +
XENSTORE_PFN_OFFSET,
+ dom.xenstore_evtchn);
xs_introduce_domain() can technically fails.
OK
+ return 0;
+}
+
+/* Check if domain has been configured in XS */
+static bool domain_exists(struct xs_handle *xsh, int domid)
+{
+ return xs_is_domain_introduced(xsh, domid);
+}
Would not this lead to initialize a domain with PV driver disabled?
I am not sure I understood your question, but I'll try to answer
anyway.
This check is purely to distinguish dom0less guests, which needs
further
initializations, from regular guests (e.g. xl guests) that don't need
any actions taken here.
Dom0less domUs can be divided in two categories based on whether they
are xen
aware (e.g. xen,enhanced is set).
Looking at this script, it seems to assume that all dom0less domUs
are Xen
aware. So it will end up to allocate Xenstore ring and call
xs_introduce_domain(). I suspect the call will end up to fail because
the
event channel would be 0.
So did you try to use this script on a platform where there only xen
aware
domU and/or a mix?
Good idea of asking for this test. I thought I already ran that test,
but I did it again to be sure. Everything works OK (although the
xenstore page allocation is unneeded). xs_introduce_domain does not
> fail:
Are you sure? If I pass 0 as the 4th argument (event channel), the
command will return EINVAL. However, looking at the code you are not
checking the return for the call. So you will continue as if it were
successful.
So you will end up to write nodes for a domain Xenstored is not aware
and also set HVM_PARAM_STORE_PFN which may further confuse the guest as
it may try to initialize Xenstored it discovers the page.
^ if it discovers
I think that's because it is usually called on all domains by the
toolstack, even the ones without xenstore support in the kernel.
The toolstack will always allocate the event channel irrespective to
whether the guest will use Xenstore. So both the shared page and the
event channel are always valid today.
With your series, this will change as the event channel will not be
allocated when "xen,enhanced" is not set.
In your case, I think we may want to register the domain to xenstore but
say there are no connection available for the domain. Juergen, what do
you think?
Cheers,
--
Julien Grall
Re: [PATCH v4 8/9] tools: add example application to initialize dom0less PV drivers
Hi,
I have posted some comments in v3 after you sent this version. Please
have a look.
On 01/04/2022 01:38, Stefano Stabellini wrote:
+static int init_domain(struct xs_handle *xsh, libxl_dominfo *info)
+{
+struct xc_interface_core *xch;
+libxl_uuid uuid;
+uint64_t xenstore_evtchn, xenstore_pfn;
+int rc;
+
+printf("Init dom0less domain: %u\n", info->domid);
+xch = xc_interface_open(0, 0, 0);
+
+rc = xc_hvm_param_get(xch, info->domid, HVM_PARAM_STORE_EVTCHN,
+ &xenstore_evtchn);
+if (rc != 0) {
+printf("Failed to get HVM_PARAM_STORE_EVTCHN\n");
+return 1;
+}
+
+/* Alloc xenstore page */
+if (alloc_xs_page(xch, info, &xenstore_pfn) != 0) {
+printf("Error on alloc magic pages\n");
+return 1;
+}
+
+rc = xc_dom_gnttab_seed(xch, info->domid, true,
+(xen_pfn_t)-1, xenstore_pfn, 0, 0);
+if (rc)
+err(1, "xc_dom_gnttab_seed");
+
+libxl_uuid_generate(&uuid);
+xc_domain_sethandle(xch, info->domid, libxl_uuid_bytearray(&uuid));
+
+rc = gen_stub_json_config(info->domid, &uuid);
+if (rc)
+err(1, "gen_stub_json_config");
+
+/* Now everything is ready: set HVM_PARAM_STORE_PFN */
+rc = xc_hvm_param_set(xch, info->domid, HVM_PARAM_STORE_PFN,
+ xenstore_pfn);
On patch #1, you told me you didn't want to allocate the page in Xen
because it wouldn't be initialized by Xenstored. But this is what we are
doing here.
This would be a problem if Linux is still booting and hasn't yet call
xenbus_probe_initcall().
I understand we need to have the page setup before raising the event
channel. I don't think we can allow Xenstored to set the HVM_PARAM (it
may run in a domain with less privilege). So I think we may need to
create a separate command to kick the client (not great).
Juergen, any thoughts?
Cheers,
--
Julien Grall
Re: [PATCH v4 3/8] x86/EFI: retrieve EDID
> On 31 Mar 2022, at 10:45, Jan Beulich wrote: > > When booting directly from EFI, obtaining this information from EFI is > the only possible way. And even when booting with a boot loader > interposed, it's more clean not to use legacy BIOS calls for this > purpose. (The downside being that there are no "capabilities" that we > can retrieve the EFI way.) > > To achieve this we need to propagate the handle used to obtain the > EFI_GRAPHICS_OUTPUT_PROTOCOL instance for further obtaining an > EFI_EDID_*_PROTOCOL instance, which has been part of the spec since 2.5. > > Signed-off-by: Jan Beulich Hi Jan, For the arm and common part, the changes looks good to me. Reviewed-by: Luca Fancellu I tested also the whole serie in the sense that it boots properly on arm, unfortunately I could not test the functionality. Cheers, Luca
Re: [PATCH v3 5/5] tools: add example application to initialize dom0less PV drivers
Hi Stefano,
On 01/04/2022 01:35, Stefano Stabellini wrote:
+
+/* Alloc magic pages */
+if (alloc_magic_pages(info, &dom) != 0) {
+printf("Error on alloc magic pages\n");
+return 1;
+}
+
+xc_dom_gnttab_init(&dom);
This call as the risk to break the guest if the dom0 Linux doesn't support
the
acquire interface. This is because it will punch a hole in the domain
memory
where the grant-table may have already been mapped.
Also, this function could fails.
I'll check for return errors. Dom0less is for fully static
configurations so I think it is OK to return error and abort if
something unexpected happens: dom0less' main reason for being is that
there is nothing unexpected :-)
Does this mean the caller will have to reboot the system if there is an error?
IOW, we don't expect them to call ./init-dom0less twice.
Yes, exactly. I think init-dom0less could even panic. My mental model is
that this is an "extension" of construct_domU. Over there we just panic
if something is wrong and here it would be similar. The user provided a
wrong config and should fix it.
Ok. I think we should make explicit how it can be used.
+
+libxl_uuid_generate(&uuid);
+xc_domain_sethandle(dom.xch, info->domid,
libxl_uuid_bytearray(&uuid));
+
+rc = gen_stub_json_config(info->domid, &uuid);
+if (rc)
+err(1, "gen_stub_json_config");
+
+rc = restore_xenstore(xsh, info, uuid, dom.xenstore_evtchn);
+if (rc)
+err(1, "writing to xenstore");
+
+xs_introduce_domain(xsh, info->domid,
+(GUEST_MAGIC_BASE >> XC_PAGE_SHIFT) + XENSTORE_PFN_OFFSET,
+dom.xenstore_evtchn);
xs_introduce_domain() can technically fails.
OK
+return 0;
+}
+
+/* Check if domain has been configured in XS */
+static bool domain_exists(struct xs_handle *xsh, int domid)
+{
+return xs_is_domain_introduced(xsh, domid);
+}
Would not this lead to initialize a domain with PV driver disabled?
I am not sure I understood your question, but I'll try to answer anyway.
This check is purely to distinguish dom0less guests, which needs further
initializations, from regular guests (e.g. xl guests) that don't need
any actions taken here.
Dom0less domUs can be divided in two categories based on whether they are xen
aware (e.g. xen,enhanced is set).
Looking at this script, it seems to assume that all dom0less domUs are Xen
aware. So it will end up to allocate Xenstore ring and call
xs_introduce_domain(). I suspect the call will end up to fail because the
event channel would be 0.
So did you try to use this script on a platform where there only xen aware
domU and/or a mix?
Good idea of asking for this test. I thought I already ran that test,
but I did it again to be sure. Everything works OK (although the
xenstore page allocation is unneeded). xs_introduce_domain does not
> fail:
Are you sure? If I pass 0 as the 4th argument (event channel), the
command will return EINVAL. However, looking at the code you are not
checking the return for the call. So you will continue as if it were
successful.
So you will end up to write nodes for a domain Xenstored is not aware
and also set HVM_PARAM_STORE_PFN which may further confuse the guest as
it may try to initialize Xenstored it discovers the page.
I think that's because it is usually called on all domains by the
toolstack, even the ones without xenstore support in the kernel.
The toolstack will always allocate the event channel irrespective to
whether the guest will use Xenstore. So both the shared page and the
event channel are always valid today.
With your series, this will change as the event channel will not be
allocated when "xen,enhanced" is not set.
In your case, I think we may want to register the domain to xenstore but
say there are no connection available for the domain. Juergen, what do
you think?
Cheers,
--
Julien Grall
[xen-4.14-testing bisection] complete test-amd64-i386-livepatch
branch xen-4.14-testing xenbranch xen-4.14-testing job test-amd64-i386-livepatch testid livepatch-run Tree: linux git://xenbits.xen.org/linux-pvops.git Tree: linuxfirmware git://xenbits.xen.org/osstest/linux-firmware.git Tree: ovmf git://xenbits.xen.org/osstest/ovmf.git Tree: qemu git://xenbits.xen.org/qemu-xen-traditional.git Tree: qemuu git://xenbits.xen.org/qemu-xen.git Tree: seabios git://xenbits.xen.org/osstest/seabios.git Tree: xen git://xenbits.xen.org/xen.git *** Found and reproduced problem changeset *** Bug is in tree: xen git://xenbits.xen.org/xen.git Bug introduced: 72a75b9c2ce36ed573a2eb201ac63ca22bedb889 Bug not present: 6db64187700d6d1ce23f49e667c24f1c7ebe11f8 Last fail repro: http://logs.test-lab.xenproject.org/osstest/logs/169105/ commit 72a75b9c2ce36ed573a2eb201ac63ca22bedb889 Author: Bjoern Doebel Date: Thu Mar 10 07:35:36 2022 + xen/x86: Livepatch: support patching CET-enhanced functions Xen enabled CET for supporting architectures. The control flow aspect of CET require functions that can be called indirectly (i.e., via function pointers) to start with an ENDBR64 instruction. Otherwise a control flow exception is raised. This expectation breaks livepatching flows because we patch functions by overwriting their first 5 bytes with a JMP + , thus breaking the ENDBR64. We fix this by checking the start of a patched function for being ENDBR64. In the positive case we move the livepatch JMP to start behind the ENDBR64 instruction. To avoid having to guess the ENDBR64 offset again on patch reversal (which might race with other mechanisms adding/removing ENDBR dynamically), use the livepatch metadata to store the computed offset along with the saved bytes of the overwritten function. Signed-off-by: Bjoern Doebel Acked-by: Konrad Rzeszutek Wilk Reviewed-by: Ross Lagerwall Tested-by: Jiamei Xie (cherry picked from commit 6974c75180f1aad44e5428eabf2396b2b50fb0e4) Note: For backports to 4.14 thru 4.16, there is no endbr-clobbering, hence no is_endbr64_poison() logic. For bisection revision-tuple graph see: http://logs.test-lab.xenproject.org/osstest/results/bisect/xen-4.14-testing/test-amd64-i386-livepatch.livepatch-run.html Revision IDs in each graph node refer, respectively, to the Trees above. Running cs-bisection-step --graph-out=/home/logs/results/bisect/xen-4.14-testing/test-amd64-i386-livepatch.livepatch-run --summary-out=tmp/169105.bisection-summary --basis-template=168506 --blessings=real,real-bisect,real-retry xen-4.14-testing test-amd64-i386-livepatch livepatch-run Searching for failure / basis pass: 169028 fail [host=elbling1] / 168506 [host=fiano0] 168493 [host=huxelrebe0] 168061 [host=albana0] 168013 [host=chardonnay0] 167964 [host=albana1] 167908 [host=elbling0] 167629 [host=fiano0] 167415 ok. Failure / basis pass flights: 169028 / 167415 (tree with no url: minios) Tree: linux git://xenbits.xen.org/linux-pvops.git Tree: linuxfirmware git://xenbits.xen.org/osstest/linux-firmware.git Tree: ovmf git://xenbits.xen.org/osstest/ovmf.git Tree: qemu git://xenbits.xen.org/qemu-xen-traditional.git Tree: qemuu git://xenbits.xen.org/qemu-xen.git Tree: seabios git://xenbits.xen.org/osstest/seabios.git Tree: xen git://xenbits.xen.org/xen.git Latest c3038e718a19fc596f7b1baba0f83d5146dc7784 c530a75c1e6a472b0eb9558310b518f0dfcd8860 b1b89f9009f2390652e0061bd7b24fc40732bc70 3c659044118e34603161457db9934a34f816d78b 83aebe33dc76760f911162f9e7a4b98a4929776b d239552ce7220e448ae81f41515138f7b9e3c4db 1e595d9c2b8608bcef48d6a69cc2f6135780bcc0 Basis pass c3038e718a19fc596f7b1baba0f83d5146dc7784 c530a75c1e6a472b0eb9558310b518f0dfcd8860 c039fa7ff09729de07bc7ebcdd4878340bfaf252 3c659044118e34603161457db9934a34f816d78b d7d6a60e73ee21e82f0bac2036153f996e6c 2dd4b9b3f84019668719344b40dba79d681be41c cbadf67bcab4e29c883410db393f4f5ef34df04a Generating revisions with ./adhoc-revtuple-generator git://xenbits.xen.org/linux-pvops.git#c3038e718a19fc596f7b1baba0f83d5146dc7784-c3038e718a19fc596f7b1baba0f83d5146dc7784 git://xenbits.xen.org/osstest/linux-firmware.git#c530a75c1e6a472b0eb9558310b518f0dfcd8860-c530a75c1e6a472b0eb9558310b518f0dfcd8860 git://xenbits.xen.org/osstest/ovmf.git#c039fa7ff09729de07bc7ebcdd4878340bfaf252-b1b89f9009f2390652e0061bd7b24fc40732bc70 git://xenbits.xen.org/qemu-xen-traditional.git#3c659044118e34603161457db99\ 34a34f816d78b-3c659044118e34603161457db9934a34f816d78b git://xenbits.xen.org/qemu-xen.git#d7d6a60e73ee21e82f0bac2036153f996e6c-83aebe33dc76760f911162f9e7a4b98a4929776b git://xenbits.xen.org/osstest/seabios.git#2dd4b9b3f84019668719344b40dba79d681be41c-d239552ce7220e448ae81f41515138f7b9e3c4db git://xenbits.xen.org/xen.git#cbadf67bcab4e29c883410db393f4f5ef34df04a-1e595d9c2b8608bcef48d6a69cc2f6135780bcc0 Loade
[xen-unstable test] 169063: tolerable FAIL
flight 169063 xen-unstable real [real] http://logs.test-lab.xenproject.org/osstest/logs/169063/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 169003 test-armhf-armhf-libvirt 16 saverestore-support-checkfail like 169003 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 169003 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 169003 test-amd64-i386-xl-qemut-ws16-amd64 19 guest-stop fail like 169003 test-amd64-i386-xl-qemut-win7-amd64 19 guest-stop fail like 169003 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check fail like 169003 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail like 169003 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 169003 test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 169003 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 169003 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 169003 test-arm64-arm64-xl-seattle 15 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 16 saverestore-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-amd64-libvirt 15 migrate-support-checkfail never pass test-amd64-i386-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-i386-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-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-thunderx 15 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 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-credit1 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 16 saverestore-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-amd64-i386-libvirt-raw 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-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-cubietruck 15 migrate-support-checkfail never pass test-armhf-armhf-xl-cubietruck 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-amd64-amd64-libvirt-vhd 14 migrate-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-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-arm64-arm64-xl-vhd 14 migrate-support-checkfail never pass test-arm64-arm64-xl-vhd 15 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-libvirt-qcow2 14 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 14 migrate-support-checkfail never pass test-armhf-armhf-xl-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 test-armhf-armhf-libvirt-raw 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 version targeted for testing: xen 8eec96b7b8d937d40e2e7988edb8bbd08598c715 baseline version: xen 8eec96b7b8d937d4
Re: [PATCH v3 3/5] xen/arm: configure dom0less domain for enabling xenstore after boot
Hi Stefano, On 01/04/2022 01:34, Stefano Stabellini wrote: Because (as you noted as a comment to the following patch) as soon as d->arch.hvm.params[HVM_PARAM_STORE_PFN] is set the guest can continue with the initialization and will expect the right data to be set on the page. I think you misunderstood my question. From my understanding, at the moment, Linux would break with your proposal. So you need to modify the kernel in order to support what you are doing. Linux does not break with this proposal. I wrote a longer explanation [1] some time ago. I guess I should not have written "broken" here. But instead point out that... In short: the master branch and any supported versions of Linux boots fine with this proposal without changes, however it wouldn't be able to use PV drivers when started as dom0less kernel. To be able to use the new feature, this patch is required [2]. ... without the extra patch is Linux, you would not be able to take advantage of this feature. IOW, we have room to decide the approach here. Xenstore protocol has a way to allow (re)connection (see docs/mics/xenstore-ring.txt). This feature looks quite suited to what you are trying to do here (we want to delay the connection). The main advantage with this approach is the resources allocation for Xenstore will be done in the place and the work in Linux could be re-used for non-dom0less domain. Have you explored it? Luca (CCed) is the original author. I don't know if he explored that approach. I have not looked into it in any details but I think there might be challenges: in this case there is nothing on the shared page. There are no feature bits as it has not been initialized (xenstored is the one initializating it). I agree there is nothing today. But here, we have the liberty to initialize the feature bits in Xen. Keep in mind that Luca and I have done many tests on this approach, both the Xen side, the Linux side (very many different kernel versions) and complex configurations (both network and block PV drivers, DMA mastering devices, etc.) If we changed approach now we would lose some of the value of the past efforts. I appreciate the effort you put into testing this approach. However, this is an external interface that we will consider stable as soon as the two sides (Xen and Linux) are committed. So I want to make sure we are not putting ourself in a corner. One major issue I can forsee with your approach is the xenstore initialization seem to only works for HVM. In theory, we may have the same need for PV (e.g. in the case of Hyperlaunch). How would your proposal work for PV guest? Note that I now that PV guest may not work without Xenstore. But I don't see any reason why we could not start them before Xenstored. Cheers, -- Julien Grall
RE: [PATCH v4 1/9] xen/dt: dt_property_read_string should return -ENODATA on !length
Hi Stefano, > -Original Message- > From: Xen-devel On Behalf Of > Stefano Stabellini > Sent: Friday, April 1, 2022 8:39 AM > To: xen-devel@lists.xenproject.org > Cc: sstabell...@kernel.org; jgr...@suse.com; Bertrand Marquis > ; jul...@xen.org; > volodymyr_babc...@epam.com; Stefano Stabellini > > Subject: [PATCH v4 1/9] xen/dt: dt_property_read_string should return - > ENODATA on !length > > From: Stefano Stabellini > > When the length is zero (pp->length == 0), dt_property_read_string > should return -ENODATA, but actually currently returns -EILSEQ because > there is no specific check for lenght == 0. > > Add a check now. > > Signed-off-by: Stefano Stabellini Reviewed-by: Henry Wang > --- > xen/common/device_tree.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c > index 4aae281e89..db67fb5fb4 100644 > --- a/xen/common/device_tree.c > +++ b/xen/common/device_tree.c > @@ -198,7 +198,7 @@ int dt_property_read_string(const struct > dt_device_node *np, > > if ( !pp ) > return -EINVAL; > -if ( !pp->value ) > +if ( !pp->value || !pp->length ) > return -ENODATA; > if ( strnlen(pp->value, pp->length) >= pp->length ) > return -EILSEQ; > -- > 2.25.1 >
