[PATCH v4 1/3] hw: Model ASPEED's Hash and Crypto Engine
The HACE (Hash and Crypto Engine) is a device that offloads MD5, SHA1,
SHA2, RSA and other cryptographic algorithms.
This initial model implements a subset of the device's functionality;
currently only direct access (non-scatter gather) hashing.
Signed-off-by: Joel Stanley
---
v3:
- rebase on upstream to fix meson.build conflict
v2:
- reorder register defines
- mask src/dest/len registers according to hardware
v4:
- Fix typos in comments
- Remove sdram base address; new memory region fixes mean this is not
required
- Use PRIx64
- Add Object Classes for soc familiy specific features
- Convert big switch statement to a lookup in a struct
---
include/hw/misc/aspeed_hace.h | 43
hw/misc/aspeed_hace.c | 358 ++
hw/misc/meson.build | 1 +
3 files changed, 402 insertions(+)
create mode 100644 include/hw/misc/aspeed_hace.h
create mode 100644 hw/misc/aspeed_hace.c
diff --git a/include/hw/misc/aspeed_hace.h b/include/hw/misc/aspeed_hace.h
new file mode 100644
index ..94d5ada95fa2
--- /dev/null
+++ b/include/hw/misc/aspeed_hace.h
@@ -0,0 +1,43 @@
+/*
+ * ASPEED Hash and Crypto Engine
+ *
+ * Copyright (C) 2021 IBM Corp.
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#ifndef ASPEED_HACE_H
+#define ASPEED_HACE_H
+
+#include "hw/sysbus.h"
+
+#define TYPE_ASPEED_HACE "aspeed.hace"
+#define TYPE_ASPEED_AST2400_HACE TYPE_ASPEED_HACE "-ast2400"
+#define TYPE_ASPEED_AST2500_HACE TYPE_ASPEED_HACE "-ast2500"
+#define TYPE_ASPEED_AST2600_HACE TYPE_ASPEED_HACE "-ast2600"
+OBJECT_DECLARE_TYPE(AspeedHACEState, AspeedHACEClass, ASPEED_HACE)
+
+#define ASPEED_HACE_NR_REGS (0x64 >> 2)
+
+struct AspeedHACEState {
+SysBusDevice parent;
+
+MemoryRegion iomem;
+qemu_irq irq;
+
+uint32_t regs[ASPEED_HACE_NR_REGS];
+
+MemoryRegion *dram_mr;
+AddressSpace dram_as;
+};
+
+
+struct AspeedHACEClass {
+SysBusDeviceClass parent_class;
+
+uint32_t src_mask;
+uint32_t dest_mask;
+uint32_t hash_mask;
+};
+
+#endif /* _ASPEED_HACE_H_ */
diff --git a/hw/misc/aspeed_hace.c b/hw/misc/aspeed_hace.c
new file mode 100644
index ..a962ccea90e8
--- /dev/null
+++ b/hw/misc/aspeed_hace.c
@@ -0,0 +1,358 @@
+/*
+ * ASPEED Hash and Crypto Engine
+ *
+ * Copyright (C) 2021 IBM Corp.
+ *
+ * Joel Stanley
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/log.h"
+#include "qemu/error-report.h"
+#include "hw/misc/aspeed_hace.h"
+#include "qapi/error.h"
+#include "migration/vmstate.h"
+#include "crypto/hash.h"
+#include "hw/qdev-properties.h"
+#include "hw/irq.h"
+
+#define R_CRYPT_CMD (0x10 / 4)
+
+#define R_STATUS(0x1c / 4)
+#define HASH_IRQBIT(9)
+#define CRYPT_IRQ BIT(12)
+#define TAG_IRQ BIT(15)
+
+#define R_HASH_SRC (0x20 / 4)
+#define R_HASH_DEST (0x24 / 4)
+#define R_HASH_SRC_LEN (0x2c / 4)
+
+#define R_HASH_CMD (0x30 / 4)
+/* Hash algorithm selection */
+#define HASH_ALGO_MASK (BIT(4) | BIT(5) | BIT(6))
+#define HASH_ALGO_MD5 0
+#define HASH_ALGO_SHA1 BIT(5)
+#define HASH_ALGO_SHA224 BIT(6)
+#define HASH_ALGO_SHA256 (BIT(4) | BIT(6))
+#define HASH_ALGO_SHA512_SERIES(BIT(5) | BIT(6))
+/* SHA512 algorithm selection */
+#define SHA512_HASH_ALGO_MASK (BIT(10) | BIT(11) | BIT(12))
+#define HASH_ALGO_SHA512_SHA5120
+#define HASH_ALGO_SHA512_SHA384BIT(10)
+#define HASH_ALGO_SHA512_SHA256BIT(11)
+#define HASH_ALGO_SHA512_SHA224(BIT(10) | BIT(11))
+/* HMAC modes */
+#define HASH_HMAC_MASK (BIT(7) | BIT(8))
+#define HASH_DIGEST0
+#define HASH_DIGEST_HMAC BIT(7)
+#define HASH_DIGEST_ACCUM BIT(8)
+#define HASH_HMAC_KEY (BIT(7) | BIT(8))
+/* Cascaded operation modes */
+#define HASH_ONLY 0
+#define HASH_ONLY2 BIT(0)
+#define HASH_CRYPT_THEN_HASH BIT(1)
+#define HASH_HASH_THEN_CRYPT (BIT(0) | BIT(1))
+/* Other cmd bits */
+#define HASH_IRQ_ENBIT(9)
+#define HASH_SG_EN BIT(18)
+
+static const struct {
+uint32_t mask;
+QCryptoHashAlgorithm algo;
+} hash_algo_map[] = {
+{ HASH_ALGO_MD5, QCRYPTO_HASH_ALG_MD5 },
+{ HASH_ALGO_SHA1, QCRYPTO_HASH_ALG_SHA1 },
+{ HASH_ALGO_SHA224, QCRYPTO_HASH_ALG_SHA224 },
+{ HASH_ALGO_SHA256, QCRYPTO_HASH_ALG_SHA256 },
+{ HASH_ALGO_SHA512_SERIES | HASH_ALGO_SHA512_SHA512,
QCRYPTO_HASH_ALG_SHA512 },
+{ HASH_ALGO_SHA512_SERIES | HASH_ALGO_SHA512_SHA384,
QCRYPTO_HASH_ALG_SHA384 },
+{ HASH_ALGO_SHA512_SERIES | HASH_ALGO_SHA512_SHA256,
QCRYPTO_HASH_ALG_SHA256 },
+};
+
+static int hash_algo_lookup(uint32_t mask)
+{
+int i;
+
+for (i = 0; i < ARRAY_SIZE(hash_algo_map); i++) {
+if (mask == hash_algo_map[i].mask)
+return has
[PATCH v4 0/3] hw/misc: Model ASPEED hash and crypto engine
v4: Rebase on Philippe's memory region cleanup series [1] Address feedback from Cédric Rework qtest to run on ast2400, ast2500 and ast2600 v3: Rework qtest to not use libqtest-single.h, rebase to avoid LPC conflicts. v2: Address review from Andrew and Philippe. Adds a qtest. [1] https://lore.kernel.org/qemu-devel/20210312182851.1922972-1-f4...@amsat.org/ This adds a model for the ASPEED hash and crypto engine (HACE) found on all supported ASPEED SoCs. The model uses Qemu's gcrypto API to perform the SHA and MD5 hashing directly in the machine's emulated memory space, which I found a neat use of Qemu's features. It has been tested using u-boot and from Linux userspace, and adds a qtest for the model running as part of the ast2600-evb, ast2500-evb and palmetto-bmc (to test ast2400) machines. Note that the tests will fail without Philippe's series. Joel Stanley (3): hw: Model ASPEED's Hash and Crypto Engine aspeed: Integrate HACE tests/qtest: Add test for Aspeed HACE docs/system/arm/aspeed.rst | 2 +- include/hw/arm/aspeed_soc.h| 3 + include/hw/misc/aspeed_hace.h | 43 hw/arm/aspeed_ast2600.c| 15 ++ hw/arm/aspeed_soc.c| 16 ++ hw/misc/aspeed_hace.c | 358 + tests/qtest/aspeed_hace-test.c | 313 MAINTAINERS| 1 + hw/misc/meson.build| 1 + tests/qtest/meson.build| 3 + 10 files changed, 754 insertions(+), 1 deletion(-) create mode 100644 include/hw/misc/aspeed_hace.h create mode 100644 hw/misc/aspeed_hace.c create mode 100644 tests/qtest/aspeed_hace-test.c -- 2.30.2
[PATCH v4 3/3] tests/qtest: Add test for Aspeed HACE
This adds a test for the Aspeed Hash and Crypto (HACE) engine. It tests
the currently implemented behavior of the hash functionality.
The tests are similar, but are cut/pasted instead of broken out into a
common function so the assert machinery produces useful output when a
test fails.
Signed-off-by: Joel Stanley
---
v3: Write test without libqtest-single.h
v4: Run tests on all aspeed machines
---
tests/qtest/aspeed_hace-test.c | 313 +
MAINTAINERS| 1 +
tests/qtest/meson.build| 3 +
3 files changed, 317 insertions(+)
create mode 100644 tests/qtest/aspeed_hace-test.c
diff --git a/tests/qtest/aspeed_hace-test.c b/tests/qtest/aspeed_hace-test.c
new file mode 100644
index ..2b624b6b099b
--- /dev/null
+++ b/tests/qtest/aspeed_hace-test.c
@@ -0,0 +1,313 @@
+/*
+ * QTest testcase for the ASPEED Hash and Crypto Engine
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ * Copyright 2021 IBM Corp.
+ */
+
+#include "qemu/osdep.h"
+
+#include "libqos/libqtest.h"
+#include "qemu-common.h"
+#include "qemu/bitops.h"
+
+#define HACE_CMD 0x10
+#define HACE_SHA_BE_EN BIT(3)
+#define HACE_MD5_LE_EN BIT(2)
+#define HACE_ALGO_MD5 0
+#define HACE_ALGO_SHA1 BIT(5)
+#define HACE_ALGO_SHA224BIT(6)
+#define HACE_ALGO_SHA256(BIT(4) | BIT(6))
+#define HACE_ALGO_SHA512(BIT(5) | BIT(6))
+#define HACE_ALGO_SHA384(BIT(5) | BIT(6) | BIT(10))
+#define HACE_SG_EN BIT(18)
+
+#define HACE_STS 0x1c
+#define HACE_RSA_ISRBIT(13)
+#define HACE_CRYPTO_ISR BIT(12)
+#define HACE_HASH_ISR BIT(9)
+#define HACE_RSA_BUSY BIT(2)
+#define HACE_CRYPTO_BUSYBIT(1)
+#define HACE_HASH_BUSY BIT(0)
+#define HACE_HASH_SRC0x20
+#define HACE_HASH_DIGEST 0x24
+#define HACE_HASH_KEY_BUFF 0x28
+#define HACE_HASH_DATA_LEN 0x2c
+#define HACE_HASH_CMD0x30
+
+/*
+ * Test vector is the ascii "abc"
+ *
+ * Expected results were generated using command line utitiles:
+ *
+ * echo -n -e 'abc' | dd of=/tmp/test
+ * for hash in sha512sum sha256sum md5sum; do $hash /tmp/test; done
+ *
+ */
+static const uint8_t test_vector[] = {0x61, 0x62, 0x63};
+
+static const uint8_t test_result_sha512[] = {
+0xdd, 0xaf, 0x35, 0xa1, 0x93, 0x61, 0x7a, 0xba, 0xcc, 0x41, 0x73, 0x49,
+0xae, 0x20, 0x41, 0x31, 0x12, 0xe6, 0xfa, 0x4e, 0x89, 0xa9, 0x7e, 0xa2,
+0x0a, 0x9e, 0xee, 0xe6, 0x4b, 0x55, 0xd3, 0x9a, 0x21, 0x92, 0x99, 0x2a,
+0x27, 0x4f, 0xc1, 0xa8, 0x36, 0xba, 0x3c, 0x23, 0xa3, 0xfe, 0xeb, 0xbd,
+0x45, 0x4d, 0x44, 0x23, 0x64, 0x3c, 0xe8, 0x0e, 0x2a, 0x9a, 0xc9, 0x4f,
+0xa5, 0x4c, 0xa4, 0x9f};
+
+static const uint8_t test_result_sha256[] = {
+0xba, 0x78, 0x16, 0xbf, 0x8f, 0x01, 0xcf, 0xea, 0x41, 0x41, 0x40, 0xde,
+0x5d, 0xae, 0x22, 0x23, 0xb0, 0x03, 0x61, 0xa3, 0x96, 0x17, 0x7a, 0x9c,
+0xb4, 0x10, 0xff, 0x61, 0xf2, 0x00, 0x15, 0xad};
+
+static const uint8_t test_result_md5[] = {
+0x90, 0x01, 0x50, 0x98, 0x3c, 0xd2, 0x4f, 0xb0, 0xd6, 0x96, 0x3f, 0x7d,
+0x28, 0xe1, 0x7f, 0x72};
+
+
+static void write_regs(QTestState *s, uint32_t base, uint32_t src,
+ uint32_t length, uint32_t out, uint32_t method)
+{
+qtest_writel(s, base + HACE_HASH_SRC, src);
+qtest_writel(s, base + HACE_HASH_DIGEST, out);
+qtest_writel(s, base + HACE_HASH_DATA_LEN, length);
+qtest_writel(s, base + HACE_HASH_CMD, HACE_SHA_BE_EN | method);
+}
+
+static void test_md5(const char *machine, const uint32_t base,
+ const uint32_t src_addr)
+
+{
+QTestState *s = qtest_init(machine);
+
+uint32_t digest_addr = src_addr + 0x0100;
+uint8_t digest[16] = {0};
+
+/* Check engine is idle, no busy or irq bits set */
+g_assert_cmphex(qtest_readl(s, base + HACE_STS), ==, 0);
+
+/* Write test vector into memory */
+qtest_memwrite(s, src_addr, test_vector, sizeof(test_vector));
+
+write_regs(s, base, src_addr, sizeof(test_vector), digest_addr,
HACE_ALGO_MD5);
+
+/* Check hash IRQ status is asserted */
+g_assert_cmphex(qtest_readl(s, base + HACE_STS), ==, 0x0200);
+
+/* Clear IRQ status and check status is deasserted */
+qtest_writel(s, base + HACE_STS, 0x0200);
+g_assert_cmphex(qtest_readl(s, base + HACE_STS), ==, 0);
+
+/* Read computed digest from memory */
+qtest_memread(s, digest_addr, digest, sizeof(digest));
+
+/* Check result of computation */
+g_assert_cmpmem(digest, sizeof(digest),
+test_result_md5, sizeof(digest));
+}
+
+static void test_sha256(const char *machine, const uint32_t base,
+const uint32_t src_addr)
+{
+QTestState *s = qtest_init(machine);
+
+const uint32_t digest_addr = src_addr + 0x100;
+uint8_t digest[32] = {0};
+
+/* Check engine is idle, no busy or irq bi
[PATCH v4 2/3] aspeed: Integrate HACE
Add the hash and crypto engine model to the Aspeed socs.
Reviewed-by: Andrew Jeffery
Signed-off-by: Joel Stanley
---
v3: Rebase on upstream
v4: Update integration for soc-specific hace objects
---
docs/system/arm/aspeed.rst | 2 +-
include/hw/arm/aspeed_soc.h | 3 +++
hw/arm/aspeed_ast2600.c | 15 +++
hw/arm/aspeed_soc.c | 16
4 files changed, 35 insertions(+), 1 deletion(-)
diff --git a/docs/system/arm/aspeed.rst b/docs/system/arm/aspeed.rst
index d1fb8f25b39c..f9466e6d8245 100644
--- a/docs/system/arm/aspeed.rst
+++ b/docs/system/arm/aspeed.rst
@@ -49,6 +49,7 @@ Supported devices
* Ethernet controllers
* Front LEDs (PCA9552 on I2C bus)
* LPC Peripheral Controller (a subset of subdevices are supported)
+ * Hash/Crypto Engine (HACE) - Hash support only, no scatter-gather
Missing devices
@@ -59,7 +60,6 @@ Missing devices
* PWM and Fan Controller
* Slave GPIO Controller
* Super I/O Controller
- * Hash/Crypto Engine
* PCI-Express 1 Controller
* Graphic Display Controller
* PECI Controller
diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h
index 9359d6da336d..d9161d26d645 100644
--- a/include/hw/arm/aspeed_soc.h
+++ b/include/hw/arm/aspeed_soc.h
@@ -21,6 +21,7 @@
#include "hw/rtc/aspeed_rtc.h"
#include "hw/i2c/aspeed_i2c.h"
#include "hw/ssi/aspeed_smc.h"
+#include "hw/misc/aspeed_hace.h"
#include "hw/watchdog/wdt_aspeed.h"
#include "hw/net/ftgmac100.h"
#include "target/arm/cpu.h"
@@ -50,6 +51,7 @@ struct AspeedSoCState {
AspeedTimerCtrlState timerctrl;
AspeedI2CState i2c;
AspeedSCUState scu;
+AspeedHACEState hace;
AspeedXDMAState xdma;
AspeedSMCState fmc;
AspeedSMCState spi[ASPEED_SPIS_NUM];
@@ -133,6 +135,7 @@ enum {
ASPEED_DEV_XDMA,
ASPEED_DEV_EMMC,
ASPEED_DEV_KCS,
+ASPEED_DEV_HACE,
};
#endif /* ASPEED_SOC_H */
diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
index bc87e754a3cc..c149936e0b28 100644
--- a/hw/arm/aspeed_ast2600.c
+++ b/hw/arm/aspeed_ast2600.c
@@ -42,6 +42,7 @@ static const hwaddr aspeed_soc_ast2600_memmap[] = {
[ASPEED_DEV_ETH2] = 0x1E68,
[ASPEED_DEV_ETH4] = 0x1E69,
[ASPEED_DEV_VIC] = 0x1E6C,
+[ASPEED_DEV_HACE] = 0x1E6D,
[ASPEED_DEV_SDMC] = 0x1E6E,
[ASPEED_DEV_SCU] = 0x1E6E2000,
[ASPEED_DEV_XDMA] = 0x1E6E7000,
@@ -102,6 +103,7 @@ static const int aspeed_soc_ast2600_irqmap[] = {
[ASPEED_DEV_I2C] = 110, /* 110 -> 125 */
[ASPEED_DEV_ETH1] = 2,
[ASPEED_DEV_ETH2] = 3,
+[ASPEED_DEV_HACE] = 4,
[ASPEED_DEV_ETH3] = 32,
[ASPEED_DEV_ETH4] = 33,
[ASPEED_DEV_KCS] = 138, /* 138 -> 142 */
@@ -213,6 +215,9 @@ static void aspeed_soc_ast2600_init(Object *obj)
TYPE_SYSBUS_SDHCI);
object_initialize_child(obj, "lpc", &s->lpc, TYPE_ASPEED_LPC);
+
+snprintf(typename, sizeof(typename), "aspeed.hace-%s", socname);
+object_initialize_child(obj, "hace", &s->hace, typename);
}
/*
@@ -498,6 +503,16 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev,
Error **errp)
sysbus_connect_irq(SYS_BUS_DEVICE(&s->lpc), 1 + aspeed_lpc_kcs_4,
qdev_get_gpio_in(DEVICE(&s->a7mpcore),
sc->irqmap[ASPEED_DEV_KCS] +
aspeed_lpc_kcs_4));
+
+/* HACE */
+object_property_set_link(OBJECT(&s->hace), "dram", OBJECT(s->dram_mr),
+ &error_abort);
+if (!sysbus_realize(SYS_BUS_DEVICE(&s->hace), errp)) {
+return;
+}
+sysbus_mmio_map(SYS_BUS_DEVICE(&s->hace), 0, sc->memmap[ASPEED_DEV_HACE]);
+sysbus_connect_irq(SYS_BUS_DEVICE(&s->hace), 0,
+ aspeed_soc_get_irq(s, ASPEED_DEV_HACE));
}
static void aspeed_soc_ast2600_class_init(ObjectClass *oc, void *data)
diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
index 057d053c8478..c8c3bd233233 100644
--- a/hw/arm/aspeed_soc.c
+++ b/hw/arm/aspeed_soc.c
@@ -34,6 +34,7 @@ static const hwaddr aspeed_soc_ast2400_memmap[] = {
[ASPEED_DEV_VIC]= 0x1E6C,
[ASPEED_DEV_SDMC] = 0x1E6E,
[ASPEED_DEV_SCU]= 0x1E6E2000,
+[ASPEED_DEV_HACE] = 0x1E6E3000,
[ASPEED_DEV_XDMA] = 0x1E6E7000,
[ASPEED_DEV_VIDEO] = 0x1E70,
[ASPEED_DEV_ADC]= 0x1E6E9000,
@@ -65,6 +66,7 @@ static const hwaddr aspeed_soc_ast2500_memmap[] = {
[ASPEED_DEV_VIC]= 0x1E6C,
[ASPEED_DEV_SDMC] = 0x1E6E,
[ASPEED_DEV_SCU]= 0x1E6E2000,
+[ASPEED_DEV_HACE] = 0x1E6E3000,
[ASPEED_DEV_XDMA] = 0x1E6E7000,
[ASPEED_DEV_ADC]= 0x1E6E9000,
[ASPEED_DEV_VIDEO] = 0x1E70,
@@ -117,6 +119,7 @@ static const int aspeed_soc_ast2400_irqmap[] = {
[ASPEED_DEV_ETH2] = 3,
[ASPEED_DEV_XDMA] = 6,
[ASPEED_DEV_SDHCI] = 26,
+[ASPEED_DEV_HACE] = 4,
};
#define aspeed_soc_ast2500_irqmap aspeed_soc_ast2400_irqmap
@@ -
Re: [PATCH v4 3/3] tests/qtest: Add test for Aspeed HACE
On 3/24/21 8:09 AM, Joel Stanley wrote:
> This adds a test for the Aspeed Hash and Crypto (HACE) engine. It tests
> the currently implemented behavior of the hash functionality.
>
> The tests are similar, but are cut/pasted instead of broken out into a
> common function so the assert machinery produces useful output when a
> test fails.
>
> Signed-off-by: Joel Stanley
Reviewed-by: Cédric Le Goater
Thomas,
Can we keep your Acked-by ?
Thanks,
C.
> ---
> v3: Write test without libqtest-single.h
> v4: Run tests on all aspeed machines
> ---
> tests/qtest/aspeed_hace-test.c | 313 +
> MAINTAINERS| 1 +
> tests/qtest/meson.build| 3 +
> 3 files changed, 317 insertions(+)
> create mode 100644 tests/qtest/aspeed_hace-test.c
>
> diff --git a/tests/qtest/aspeed_hace-test.c b/tests/qtest/aspeed_hace-test.c
> new file mode 100644
> index ..2b624b6b099b
> --- /dev/null
> +++ b/tests/qtest/aspeed_hace-test.c
> @@ -0,0 +1,313 @@
> +/*
> + * QTest testcase for the ASPEED Hash and Crypto Engine
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + * Copyright 2021 IBM Corp.
> + */
> +
> +#include "qemu/osdep.h"
> +
> +#include "libqos/libqtest.h"
> +#include "qemu-common.h"
> +#include "qemu/bitops.h"
> +
> +#define HACE_CMD 0x10
> +#define HACE_SHA_BE_EN BIT(3)
> +#define HACE_MD5_LE_EN BIT(2)
> +#define HACE_ALGO_MD5 0
> +#define HACE_ALGO_SHA1 BIT(5)
> +#define HACE_ALGO_SHA224BIT(6)
> +#define HACE_ALGO_SHA256(BIT(4) | BIT(6))
> +#define HACE_ALGO_SHA512(BIT(5) | BIT(6))
> +#define HACE_ALGO_SHA384(BIT(5) | BIT(6) | BIT(10))
> +#define HACE_SG_EN BIT(18)
> +
> +#define HACE_STS 0x1c
> +#define HACE_RSA_ISRBIT(13)
> +#define HACE_CRYPTO_ISR BIT(12)
> +#define HACE_HASH_ISR BIT(9)
> +#define HACE_RSA_BUSY BIT(2)
> +#define HACE_CRYPTO_BUSYBIT(1)
> +#define HACE_HASH_BUSY BIT(0)
> +#define HACE_HASH_SRC0x20
> +#define HACE_HASH_DIGEST 0x24
> +#define HACE_HASH_KEY_BUFF 0x28
> +#define HACE_HASH_DATA_LEN 0x2c
> +#define HACE_HASH_CMD0x30
> +
> +/*
> + * Test vector is the ascii "abc"
> + *
> + * Expected results were generated using command line utitiles:
> + *
> + * echo -n -e 'abc' | dd of=/tmp/test
> + * for hash in sha512sum sha256sum md5sum; do $hash /tmp/test; done
> + *
> + */
> +static const uint8_t test_vector[] = {0x61, 0x62, 0x63};
> +
> +static const uint8_t test_result_sha512[] = {
> +0xdd, 0xaf, 0x35, 0xa1, 0x93, 0x61, 0x7a, 0xba, 0xcc, 0x41, 0x73, 0x49,
> +0xae, 0x20, 0x41, 0x31, 0x12, 0xe6, 0xfa, 0x4e, 0x89, 0xa9, 0x7e, 0xa2,
> +0x0a, 0x9e, 0xee, 0xe6, 0x4b, 0x55, 0xd3, 0x9a, 0x21, 0x92, 0x99, 0x2a,
> +0x27, 0x4f, 0xc1, 0xa8, 0x36, 0xba, 0x3c, 0x23, 0xa3, 0xfe, 0xeb, 0xbd,
> +0x45, 0x4d, 0x44, 0x23, 0x64, 0x3c, 0xe8, 0x0e, 0x2a, 0x9a, 0xc9, 0x4f,
> +0xa5, 0x4c, 0xa4, 0x9f};
> +
> +static const uint8_t test_result_sha256[] = {
> +0xba, 0x78, 0x16, 0xbf, 0x8f, 0x01, 0xcf, 0xea, 0x41, 0x41, 0x40, 0xde,
> +0x5d, 0xae, 0x22, 0x23, 0xb0, 0x03, 0x61, 0xa3, 0x96, 0x17, 0x7a, 0x9c,
> +0xb4, 0x10, 0xff, 0x61, 0xf2, 0x00, 0x15, 0xad};
> +
> +static const uint8_t test_result_md5[] = {
> +0x90, 0x01, 0x50, 0x98, 0x3c, 0xd2, 0x4f, 0xb0, 0xd6, 0x96, 0x3f, 0x7d,
> +0x28, 0xe1, 0x7f, 0x72};
> +
> +
> +static void write_regs(QTestState *s, uint32_t base, uint32_t src,
> + uint32_t length, uint32_t out, uint32_t method)
> +{
> +qtest_writel(s, base + HACE_HASH_SRC, src);
> +qtest_writel(s, base + HACE_HASH_DIGEST, out);
> +qtest_writel(s, base + HACE_HASH_DATA_LEN, length);
> +qtest_writel(s, base + HACE_HASH_CMD, HACE_SHA_BE_EN | method);
> +}
> +
> +static void test_md5(const char *machine, const uint32_t base,
> + const uint32_t src_addr)
> +
> +{
> +QTestState *s = qtest_init(machine);
> +
> +uint32_t digest_addr = src_addr + 0x0100;
> +uint8_t digest[16] = {0};
> +
> +/* Check engine is idle, no busy or irq bits set */
> +g_assert_cmphex(qtest_readl(s, base + HACE_STS), ==, 0);
> +
> +/* Write test vector into memory */
> +qtest_memwrite(s, src_addr, test_vector, sizeof(test_vector));
> +
> +write_regs(s, base, src_addr, sizeof(test_vector), digest_addr,
> HACE_ALGO_MD5);
> +
> +/* Check hash IRQ status is asserted */
> +g_assert_cmphex(qtest_readl(s, base + HACE_STS), ==, 0x0200);
> +
> +/* Clear IRQ status and check status is deasserted */
> +qtest_writel(s, base + HACE_STS, 0x0200);
> +g_assert_cmphex(qtest_readl(s, base + HACE_STS), ==, 0);
> +
> +/* Read computed digest from memory */
> +qtest_memread(s, digest_addr, digest, sizeof(digest));
> +
> +/* Check result of computation */
> +g_assert_cmpmem(d
Re: [PATCH 05/20] qobject: Change qobject_to_json()'s value to GString
On 11/12/2020 18.11, Markus Armbruster wrote: qobject_to_json() and qobject_to_json_pretty() build a GString, then covert it to QString. Just one of the callers actually needs a QString: qemu_rbd_parse_filename(). A few others need a string they can modify: qmp_send_response(), qga's send_response(), to_json_str(), and qmp_fd_vsend_fds(). The remainder just need a string. Change qobject_to_json() and qobject_to_json_pretty() to return the GString. qemu_rbd_parse_filename() now has to convert to QString. All others save a QString temporary. to_json_str() actually becomes a bit simpler, because GString provides more convenient modification functions. Signed-off-by: Markus Armbruster Hi Markus! This patch broke the output of default values in the device help. With commit eab3a4678b07267c39e72: $ ./qemu-system-x86_64 -device pvpanic,help pvpanic options: events= - (default: (null)) ioport=- (default: (null)) pvpanic[0]=> With the commit right before that one: $ ./qemu-system-x86_64 -device pvpanic,help pvpanic options: events= - (default: 3) ioport=- (default: 1285) pvpanic[0]=> Any ideas how to fix that? Thomas
Re: [PATCH v8] drivers/misc: sysgenid: add system generation id driver
On Tue, Mar 23, 2021 at 04:10:27PM +, Catangiu, Adrian Costin wrote: > Hi Greg, > > After your previous reply on this thread we started considering to provide > this interface and framework/functionality through a userspace service > instead of a kernel interface. > The latest iteration on this evolving patch-set doesn’t have strong reasons > for living in the kernel anymore - the only objectively strong advantage > would be easier driving of ecosystem integration; but I am not sure that's a > good enough reason to create a new kernel interface. > > I am now looking into adding this through Systemd. Either as a pluggable > service or maybe even a systemd builtin offering. > > What are your thoughts on it? Now dropped from my char-misc-testing branch. If you all decide you want this as a kernel driver, please resubmit it. Also next time, you might give me a heads-up that you don't want a patch merged... thanks, greg k-h
Re: [PATCH v4 2/3] aspeed: Integrate HACE
On 3/24/21 8:09 AM, Joel Stanley wrote:
> Add the hash and crypto engine model to the Aspeed socs.
>
> Reviewed-by: Andrew Jeffery
> Signed-off-by: Joel Stanley
Reviewed-by: Cédric Le Goater
> ---
> v3: Rebase on upstream
> v4: Update integration for soc-specific hace objects
> ---
> docs/system/arm/aspeed.rst | 2 +-
> include/hw/arm/aspeed_soc.h | 3 +++
> hw/arm/aspeed_ast2600.c | 15 +++
> hw/arm/aspeed_soc.c | 16
> 4 files changed, 35 insertions(+), 1 deletion(-)
>
> diff --git a/docs/system/arm/aspeed.rst b/docs/system/arm/aspeed.rst
> index d1fb8f25b39c..f9466e6d8245 100644
> --- a/docs/system/arm/aspeed.rst
> +++ b/docs/system/arm/aspeed.rst
> @@ -49,6 +49,7 @@ Supported devices
> * Ethernet controllers
> * Front LEDs (PCA9552 on I2C bus)
> * LPC Peripheral Controller (a subset of subdevices are supported)
> + * Hash/Crypto Engine (HACE) - Hash support only, no scatter-gather
>
>
> Missing devices
> @@ -59,7 +60,6 @@ Missing devices
> * PWM and Fan Controller
> * Slave GPIO Controller
> * Super I/O Controller
> - * Hash/Crypto Engine
> * PCI-Express 1 Controller
> * Graphic Display Controller
> * PECI Controller
> diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h
> index 9359d6da336d..d9161d26d645 100644
> --- a/include/hw/arm/aspeed_soc.h
> +++ b/include/hw/arm/aspeed_soc.h
> @@ -21,6 +21,7 @@
> #include "hw/rtc/aspeed_rtc.h"
> #include "hw/i2c/aspeed_i2c.h"
> #include "hw/ssi/aspeed_smc.h"
> +#include "hw/misc/aspeed_hace.h"
> #include "hw/watchdog/wdt_aspeed.h"
> #include "hw/net/ftgmac100.h"
> #include "target/arm/cpu.h"
> @@ -50,6 +51,7 @@ struct AspeedSoCState {
> AspeedTimerCtrlState timerctrl;
> AspeedI2CState i2c;
> AspeedSCUState scu;
> +AspeedHACEState hace;
> AspeedXDMAState xdma;
> AspeedSMCState fmc;
> AspeedSMCState spi[ASPEED_SPIS_NUM];
> @@ -133,6 +135,7 @@ enum {
> ASPEED_DEV_XDMA,
> ASPEED_DEV_EMMC,
> ASPEED_DEV_KCS,
> +ASPEED_DEV_HACE,
> };
>
> #endif /* ASPEED_SOC_H */
> diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
> index bc87e754a3cc..c149936e0b28 100644
> --- a/hw/arm/aspeed_ast2600.c
> +++ b/hw/arm/aspeed_ast2600.c
> @@ -42,6 +42,7 @@ static const hwaddr aspeed_soc_ast2600_memmap[] = {
> [ASPEED_DEV_ETH2] = 0x1E68,
> [ASPEED_DEV_ETH4] = 0x1E69,
> [ASPEED_DEV_VIC] = 0x1E6C,
> +[ASPEED_DEV_HACE] = 0x1E6D,
> [ASPEED_DEV_SDMC] = 0x1E6E,
> [ASPEED_DEV_SCU] = 0x1E6E2000,
> [ASPEED_DEV_XDMA] = 0x1E6E7000,
> @@ -102,6 +103,7 @@ static const int aspeed_soc_ast2600_irqmap[] = {
> [ASPEED_DEV_I2C] = 110, /* 110 -> 125 */
> [ASPEED_DEV_ETH1] = 2,
> [ASPEED_DEV_ETH2] = 3,
> +[ASPEED_DEV_HACE] = 4,
> [ASPEED_DEV_ETH3] = 32,
> [ASPEED_DEV_ETH4] = 33,
> [ASPEED_DEV_KCS] = 138, /* 138 -> 142 */
> @@ -213,6 +215,9 @@ static void aspeed_soc_ast2600_init(Object *obj)
> TYPE_SYSBUS_SDHCI);
>
> object_initialize_child(obj, "lpc", &s->lpc, TYPE_ASPEED_LPC);
> +
> +snprintf(typename, sizeof(typename), "aspeed.hace-%s", socname);
> +object_initialize_child(obj, "hace", &s->hace, typename);
> }
>
> /*
> @@ -498,6 +503,16 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev,
> Error **errp)
> sysbus_connect_irq(SYS_BUS_DEVICE(&s->lpc), 1 + aspeed_lpc_kcs_4,
> qdev_get_gpio_in(DEVICE(&s->a7mpcore),
> sc->irqmap[ASPEED_DEV_KCS] +
> aspeed_lpc_kcs_4));
> +
> +/* HACE */
> +object_property_set_link(OBJECT(&s->hace), "dram", OBJECT(s->dram_mr),
> + &error_abort);
> +if (!sysbus_realize(SYS_BUS_DEVICE(&s->hace), errp)) {
> +return;
> +}
> +sysbus_mmio_map(SYS_BUS_DEVICE(&s->hace), 0,
> sc->memmap[ASPEED_DEV_HACE]);
> +sysbus_connect_irq(SYS_BUS_DEVICE(&s->hace), 0,
> + aspeed_soc_get_irq(s, ASPEED_DEV_HACE));
> }
>
> static void aspeed_soc_ast2600_class_init(ObjectClass *oc, void *data)
> diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
> index 057d053c8478..c8c3bd233233 100644
> --- a/hw/arm/aspeed_soc.c
> +++ b/hw/arm/aspeed_soc.c
> @@ -34,6 +34,7 @@ static const hwaddr aspeed_soc_ast2400_memmap[] = {
> [ASPEED_DEV_VIC]= 0x1E6C,
> [ASPEED_DEV_SDMC] = 0x1E6E,
> [ASPEED_DEV_SCU]= 0x1E6E2000,
> +[ASPEED_DEV_HACE] = 0x1E6E3000,
> [ASPEED_DEV_XDMA] = 0x1E6E7000,
> [ASPEED_DEV_VIDEO] = 0x1E70,
> [ASPEED_DEV_ADC]= 0x1E6E9000,
> @@ -65,6 +66,7 @@ static const hwaddr aspeed_soc_ast2500_memmap[] = {
> [ASPEED_DEV_VIC]= 0x1E6C,
> [ASPEED_DEV_SDMC] = 0x1E6E,
> [ASPEED_DEV_SCU]= 0x1E6E2000,
> +[ASPEED_DEV_HACE] = 0x1E6E3000,
> [ASPEED_DEV_XDMA] = 0x1E6E7000,
>
Re: [PATCH v4 1/3] hw: Model ASPEED's Hash and Crypto Engine
On 3/24/21 8:09 AM, Joel Stanley wrote:
> The HACE (Hash and Crypto Engine) is a device that offloads MD5, SHA1,
> SHA2, RSA and other cryptographic algorithms.
>
> This initial model implements a subset of the device's functionality;
> currently only direct access (non-scatter gather) hashing.
>
> Signed-off-by: Joel Stanley
Reviewed-by: Cédric Le Goater
Thanks,
C.
> ---
> v3:
> - rebase on upstream to fix meson.build conflict
> v2:
> - reorder register defines
> - mask src/dest/len registers according to hardware
> v4:
> - Fix typos in comments
> - Remove sdram base address; new memory region fixes mean this is not
>required
> - Use PRIx64
> - Add Object Classes for soc familiy specific features
> - Convert big switch statement to a lookup in a struct
> ---
> include/hw/misc/aspeed_hace.h | 43
> hw/misc/aspeed_hace.c | 358 ++
> hw/misc/meson.build | 1 +
> 3 files changed, 402 insertions(+)
> create mode 100644 include/hw/misc/aspeed_hace.h
> create mode 100644 hw/misc/aspeed_hace.c
>
> diff --git a/include/hw/misc/aspeed_hace.h b/include/hw/misc/aspeed_hace.h
> new file mode 100644
> index ..94d5ada95fa2
> --- /dev/null
> +++ b/include/hw/misc/aspeed_hace.h
> @@ -0,0 +1,43 @@
> +/*
> + * ASPEED Hash and Crypto Engine
> + *
> + * Copyright (C) 2021 IBM Corp.
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#ifndef ASPEED_HACE_H
> +#define ASPEED_HACE_H
> +
> +#include "hw/sysbus.h"
> +
> +#define TYPE_ASPEED_HACE "aspeed.hace"
> +#define TYPE_ASPEED_AST2400_HACE TYPE_ASPEED_HACE "-ast2400"
> +#define TYPE_ASPEED_AST2500_HACE TYPE_ASPEED_HACE "-ast2500"
> +#define TYPE_ASPEED_AST2600_HACE TYPE_ASPEED_HACE "-ast2600"
> +OBJECT_DECLARE_TYPE(AspeedHACEState, AspeedHACEClass, ASPEED_HACE)
> +
> +#define ASPEED_HACE_NR_REGS (0x64 >> 2)
> +
> +struct AspeedHACEState {
> +SysBusDevice parent;
> +
> +MemoryRegion iomem;
> +qemu_irq irq;
> +
> +uint32_t regs[ASPEED_HACE_NR_REGS];
> +
> +MemoryRegion *dram_mr;
> +AddressSpace dram_as;
> +};
> +
> +
> +struct AspeedHACEClass {
> +SysBusDeviceClass parent_class;
> +
> +uint32_t src_mask;
> +uint32_t dest_mask;
> +uint32_t hash_mask;
> +};
> +
> +#endif /* _ASPEED_HACE_H_ */
> diff --git a/hw/misc/aspeed_hace.c b/hw/misc/aspeed_hace.c
> new file mode 100644
> index ..a962ccea90e8
> --- /dev/null
> +++ b/hw/misc/aspeed_hace.c
> @@ -0,0 +1,358 @@
> +/*
> + * ASPEED Hash and Crypto Engine
> + *
> + * Copyright (C) 2021 IBM Corp.
> + *
> + * Joel Stanley
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/log.h"
> +#include "qemu/error-report.h"
> +#include "hw/misc/aspeed_hace.h"
> +#include "qapi/error.h"
> +#include "migration/vmstate.h"
> +#include "crypto/hash.h"
> +#include "hw/qdev-properties.h"
> +#include "hw/irq.h"
> +
> +#define R_CRYPT_CMD (0x10 / 4)
> +
> +#define R_STATUS(0x1c / 4)
> +#define HASH_IRQBIT(9)
> +#define CRYPT_IRQ BIT(12)
> +#define TAG_IRQ BIT(15)
> +
> +#define R_HASH_SRC (0x20 / 4)
> +#define R_HASH_DEST (0x24 / 4)
> +#define R_HASH_SRC_LEN (0x2c / 4)
> +
> +#define R_HASH_CMD (0x30 / 4)
> +/* Hash algorithm selection */
> +#define HASH_ALGO_MASK (BIT(4) | BIT(5) | BIT(6))
> +#define HASH_ALGO_MD5 0
> +#define HASH_ALGO_SHA1 BIT(5)
> +#define HASH_ALGO_SHA224 BIT(6)
> +#define HASH_ALGO_SHA256 (BIT(4) | BIT(6))
> +#define HASH_ALGO_SHA512_SERIES(BIT(5) | BIT(6))
> +/* SHA512 algorithm selection */
> +#define SHA512_HASH_ALGO_MASK (BIT(10) | BIT(11) | BIT(12))
> +#define HASH_ALGO_SHA512_SHA5120
> +#define HASH_ALGO_SHA512_SHA384BIT(10)
> +#define HASH_ALGO_SHA512_SHA256BIT(11)
> +#define HASH_ALGO_SHA512_SHA224(BIT(10) | BIT(11))
> +/* HMAC modes */
> +#define HASH_HMAC_MASK (BIT(7) | BIT(8))
> +#define HASH_DIGEST0
> +#define HASH_DIGEST_HMAC BIT(7)
> +#define HASH_DIGEST_ACCUM BIT(8)
> +#define HASH_HMAC_KEY (BIT(7) | BIT(8))
> +/* Cascaded operation modes */
> +#define HASH_ONLY 0
> +#define HASH_ONLY2 BIT(0)
> +#define HASH_CRYPT_THEN_HASH BIT(1)
> +#define HASH_HASH_THEN_CRYPT (BIT(0) | BIT(1))
> +/* Other cmd bits */
> +#define HASH_IRQ_ENBIT(9)
> +#define HASH_SG_EN BIT(18)
> +
> +static const struct {
> +uint32_t mask;
> +QCryptoHashAlgorithm algo;
> +} hash_algo_map[] = {
> +{ HASH_ALGO_MD5, QCRYPTO_HASH_ALG_MD5 },
> +{ HASH_ALGO_SHA1, QCRYPTO_HASH_ALG_SHA1 },
> +{ HASH_ALGO_SHA224, QCRYPTO_HASH_ALG_SHA224 },
> +{ HASH_ALGO_SHA256, QCRYPTO_HASH_ALG_SHA256 },
> +{ HASH_ALGO_SHA512_SERIES | HASH_ALGO_SHA512_SHA512,
Re: [PATCH v4 0/3] hw/misc: Model ASPEED hash and crypto engine
Patchew URL: https://patchew.org/QEMU/20210324070955.125941-1-j...@jms.id.au/
Hi,
This series seems to have some coding style problems. See output below for
more information:
Type: series
Message-id: 20210324070955.125941-1-j...@jms.id.au
Subject: [PATCH v4 0/3] hw/misc: Model ASPEED hash and crypto engine
=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
* [new tag] patchew/20210324070955.125941-1-j...@jms.id.au ->
patchew/20210324070955.125941-1-j...@jms.id.au
Switched to a new branch 'test'
5ebc2c5 tests/qtest: Add test for Aspeed HACE
6fc1946 aspeed: Integrate HACE
fa3bbae hw: Model ASPEED's Hash and Crypto Engine
=== OUTPUT BEGIN ===
1/3 Checking commit fa3bbae71dd5 (hw: Model ASPEED's Hash and Crypto Engine)
Use of uninitialized value $acpi_testexpected in string eq at
./scripts/checkpatch.pl line 1529.
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#22:
new file mode 100644
WARNING: line over 80 characters
#95: FILE: hw/misc/aspeed_hace.c:69:
+{ HASH_ALGO_SHA512_SERIES | HASH_ALGO_SHA512_SHA512,
QCRYPTO_HASH_ALG_SHA512 },
WARNING: line over 80 characters
#96: FILE: hw/misc/aspeed_hace.c:70:
+{ HASH_ALGO_SHA512_SERIES | HASH_ALGO_SHA512_SHA384,
QCRYPTO_HASH_ALG_SHA384 },
WARNING: line over 80 characters
#97: FILE: hw/misc/aspeed_hace.c:71:
+{ HASH_ALGO_SHA512_SERIES | HASH_ALGO_SHA512_SHA256,
QCRYPTO_HASH_ALG_SHA256 },
ERROR: braces {} are necessary for all arms of this statement
#105: FILE: hw/misc/aspeed_hace.c:79:
+if (mask == hash_algo_map[i].mask)
[...]
WARNING: line over 80 characters
#212: FILE: hw/misc/aspeed_hace.c:186:
+ "%s: HMAC engine command mode %"PRIx64" not
implemented",
total: 1 errors, 5 warnings, 408 lines checked
Patch 1/3 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
2/3 Checking commit 6fc194601e1b (aspeed: Integrate HACE)
3/3 Checking commit 5ebc2c5642da (tests/qtest: Add test for Aspeed HACE)
WARNING: line over 80 characters
#130: FILE: tests/qtest/aspeed_hace-test.c:91:
+write_regs(s, base, src_addr, sizeof(test_vector), digest_addr,
HACE_ALGO_MD5);
WARNING: line over 80 characters
#161: FILE: tests/qtest/aspeed_hace-test.c:122:
+write_regs(s, base, src_addr, sizeof(test_vector), digest_addr,
HACE_ALGO_SHA256);
WARNING: line over 80 characters
#192: FILE: tests/qtest/aspeed_hace-test.c:153:
+write_regs(s, base, src_addr, sizeof(test_vector), digest_addr,
HACE_ALGO_SHA512);
WARNING: line over 80 characters
#253: FILE: tests/qtest/aspeed_hace-test.c:214:
+g_assert_cmphex(qtest_readl(s, base + HACE_HASH_DIGEST), ==,
expected->dest);
WARNING: line over 80 characters
#256: FILE: tests/qtest/aspeed_hace-test.c:217:
+g_assert_cmphex(qtest_readl(s, base + HACE_HASH_DATA_LEN), ==,
expected->len);
total: 0 errors, 5 warnings, 335 lines checked
Patch 3/3 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===
Test command exited with code: 1
The full log is available at
http://patchew.org/logs/20210324070955.125941-1-j...@jms.id.au/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-de...@redhat.com
[PATCH 0/5] virtio: Implement generic vhost-user-i2c backend
Hello, This is an initial implementation of a generic vhost-user backend for the I2C bus. This is based of the virtio specifications (already merged) for the I2C bus. The kernel virtio I2C driver is still under review, here is the latest version (v10): https://lore.kernel.org/lkml/226a8d5663b7bb6f5d06ede7701eedb18d1bafa1.1616493817.git.jie.d...@intel.com/ The backend is implemented as a vhost-user device because we want to experiment in making portable backends that can be used with multiple hypervisors. We also want to support backends isolated in their own separate service VMs with limited memory cross-sections with the principle guest. This is part of a wider initiative by Linaro called "project Stratos" for which you can find information here: https://collaborate.linaro.org/display/STR/Stratos+Home I mentioned this to explain the decision to write the daemon as a fairly pure glib application that just depends on libvhost-user. We are not sure where the vhost-user backend should get queued, qemu or a separate repository. Similar questions were raised by an earlier thread by Alex Bennée for his RPMB work: https://lore.kernel.org/qemu-devel/20200925125147.26943-1-alex.ben...@linaro.org/ Testing: --- I didn't have access to a real hardware where I can play with a I2C client device (like RTC, eeprom, etc) to verify the working of the backend daemon, so I decided to test it on my x86 box itself with hierarchy of two ARM64 guests. The first ARM64 guest was passed "-device ds1338,address=0x20" option, so it could emulate a ds1338 RTC device, which connects to an I2C bus. Once the guest came up, ds1338 device instance was created within the guest kernel by doing: echo ds1338 0x20 > /sys/bus/i2c/devices/i2c-0/new_device [ Note that this may end up binding the ds1338 device to its driver, which won't let our i2c daemon talk to the device. For that we need to manually unbind the device from the driver: echo 0-0020 > /sys/bus/i2c/devices/0-0020/driver/unbind ] After this is done, you will get /dev/rtc1. This is the device we wanted to emulate, which will be accessed by the vhost-user-i2c backend daemon via the /dev/i2c-0 file present in the guest VM. At this point we need to start the backend daemon and give it a socket-path to talk to from qemu (you can pass -v to it to get more detailed messages): vhost-user-i2c --socket-path=vi2c.sock --device-list 0:20 [ Here, 0:20 is the bus/device mapping, 0 for /dev/i2c-0 and 20 is client address of ds1338 that we used while creating the device. ] Now we need to start the second level ARM64 guest (from within the first guest) to get the i2c-virtio.c Linux driver up. The second level guest is passed the following options to connect to the same socket: -chardev socket,path=vi2c.sock,id=vi2c \ -device vhost-user-i2c-pci,chardev=vi2c,id=i2c Once the second level guest boots up, we will see the i2c-virtio bus at /sys/bus/i2c/devices/i2c-X/. From there we can now make it emulate the ds1338 device again by doing: echo ds1338 0x20 > /sys/bus/i2c/devices/i2c-0/new_device [ This time we want ds1338's driver to be bound to the device, so it should be enabled in the kernel as well. ] And we will get /dev/rtc1 device again here in the second level guest. Now we can play with the rtc device with help of hwclock utility and we can see the following sequence of transfers happening if we try to update rtc's time from system time. hwclock -w -f /dev/rtc1 (in guest2) -> Reaches i2c-virtio.c (Linux bus driver in guest2) -> transfer over virtio -> Reaches the qemu's vhost-i2c device emulation (running over guest1) -> Reaches the backend daemon vhost-user-i2c started earlier (in guest1) -> ioctl(/dev/i2c-0, I2C_RDWR, ..); (in guest1) -> reaches qemu's hw/rtc/ds1338.c (running over host) I hope I was able to give a clear picture of my test setup here :) Thanks. Viresh Kumar (5): hw/virtio: add boilerplate for vhost-user-i2c device hw/virtio: add vhost-user-i2c-pci boilerplate tools/vhost-user-i2c: Add backend driver docs: add a man page for vhost-user-i2c MAINTAINERS: Add entry for virtio-i2c MAINTAINERS | 9 + docs/tools/index.rst| 1 + docs/tools/vhost-user-i2c.rst | 75 +++ hw/virtio/Kconfig | 5 + hw/virtio/meson.build | 2 + hw/virtio/vhost-user-i2c-pci.c | 79 +++ hw/virtio/vhost-user-i2c.c | 286 + include/hw/virtio/vhost-user-i2c.h | 37 ++ include/standard-headers/linux/virtio_ids.h | 1 + tools/meson.build | 8 + tools/vhost-user-i2c/50-qemu-i2c.json.in| 5 + tools/vhost-user-i2c/main.c | 652 tools/vhost-user-i2c/meson.build| 10 + 13 files changed, 1170 insertions(+) create mode 100644 docs/tools/vhost-user-i2c.rst
[PATCH 2/5] hw/virtio: add vhost-user-i2c-pci boilerplate
This allows is to instantiate a vhost-user-i2c device as part of a PCI
bus. It is mostly boilerplate which looks pretty similar to the
vhost-user-fs-pci device.
Signed-off-by: Viresh Kumar
---
hw/virtio/meson.build | 1 +
hw/virtio/vhost-user-i2c-pci.c | 79 ++
2 files changed, 80 insertions(+)
create mode 100644 hw/virtio/vhost-user-i2c-pci.c
diff --git a/hw/virtio/meson.build b/hw/virtio/meson.build
index 1a0d736a0db5..bc352a600911 100644
--- a/hw/virtio/meson.build
+++ b/hw/virtio/meson.build
@@ -26,6 +26,7 @@ virtio_ss.add(when: 'CONFIG_VIRTIO_RNG', if_true:
files('virtio-rng.c'))
virtio_ss.add(when: 'CONFIG_VIRTIO_IOMMU', if_true: files('virtio-iommu.c'))
virtio_ss.add(when: 'CONFIG_VIRTIO_MEM', if_true: files('virtio-mem.c'))
virtio_ss.add(when: 'CONFIG_VHOST_USER_I2C', if_true:
files('vhost-user-i2c.c'))
+virtio_ss.add(when: ['CONFIG_VIRTIO_PCI', 'CONFIG_VHOST_USER_I2C'], if_true:
files('vhost-user-i2c-pci.c'))
virtio_pci_ss = ss.source_set()
virtio_pci_ss.add(when: 'CONFIG_VHOST_VSOCK', if_true:
files('vhost-vsock-pci.c'))
diff --git a/hw/virtio/vhost-user-i2c-pci.c b/hw/virtio/vhost-user-i2c-pci.c
new file mode 100644
index ..4bcfeafcb632
--- /dev/null
+++ b/hw/virtio/vhost-user-i2c-pci.c
@@ -0,0 +1,79 @@
+/*
+ * Vhost-user i2c virtio device PCI glue
+ *
+ * Copyright (c) 2021 Viresh Kumar
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "hw/qdev-properties.h"
+#include "hw/virtio/vhost-user-i2c.h"
+#include "virtio-pci.h"
+
+struct VHostUserI2CPCI {
+VirtIOPCIProxy parent_obj;
+VHostUserI2C vdev;
+};
+
+typedef struct VHostUserI2CPCI VHostUserI2CPCI;
+
+#define TYPE_VHOST_USER_I2C_PCI "vhost-user-i2c-pci-base"
+
+DECLARE_INSTANCE_CHECKER(VHostUserI2CPCI, VHOST_USER_I2C_PCI,
+ TYPE_VHOST_USER_I2C_PCI)
+
+static Property vhost_user_i2c_pci_properties[] = {
+DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors,
+ DEV_NVECTORS_UNSPECIFIED),
+DEFINE_PROP_END_OF_LIST(),
+};
+
+static void vhost_user_i2c_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
+{
+VHostUserI2CPCI *dev = VHOST_USER_I2C_PCI(vpci_dev);
+DeviceState *vdev = DEVICE(&dev->vdev);
+
+if (vpci_dev->nvectors == DEV_NVECTORS_UNSPECIFIED) {
+vpci_dev->nvectors = 1;
+}
+
+qdev_realize(vdev, BUS(&vpci_dev->bus), errp);
+}
+
+static void vhost_user_i2c_pci_class_init(ObjectClass *klass, void *data)
+{
+DeviceClass *dc = DEVICE_CLASS(klass);
+VirtioPCIClass *k = VIRTIO_PCI_CLASS(klass);
+PCIDeviceClass *pcidev_k = PCI_DEVICE_CLASS(klass);
+k->realize = vhost_user_i2c_pci_realize;
+set_bit(DEVICE_CATEGORY_INPUT, dc->categories);
+device_class_set_props(dc, vhost_user_i2c_pci_properties);
+pcidev_k->vendor_id = PCI_VENDOR_ID_REDHAT_QUMRANET;
+pcidev_k->device_id = 0; /* Set by virtio-pci based on virtio id */
+pcidev_k->revision = 0x00;
+pcidev_k->class_id = PCI_CLASS_COMMUNICATION_OTHER;
+}
+
+static void vhost_user_i2c_pci_instance_init(Object *obj)
+{
+VHostUserI2CPCI *dev = VHOST_USER_I2C_PCI(obj);
+
+virtio_instance_init_common(obj, &dev->vdev, sizeof(dev->vdev),
+TYPE_VHOST_USER_I2C);
+}
+
+static const VirtioPCIDeviceTypeInfo vhost_user_i2c_pci_info = {
+.base_name = TYPE_VHOST_USER_I2C_PCI,
+.non_transitional_name = "vhost-user-i2c-pci",
+.instance_size = sizeof(VHostUserI2CPCI),
+.instance_init = vhost_user_i2c_pci_instance_init,
+.class_init = vhost_user_i2c_pci_class_init,
+};
+
+static void vhost_user_i2c_pci_register(void)
+{
+virtio_pci_types_register(&vhost_user_i2c_pci_info);
+}
+
+type_init(vhost_user_i2c_pci_register);
--
2.25.0.rc1.19.g042ed3e048af
[PATCH 3/5] tools/vhost-user-i2c: Add backend driver
This adds the vhost-user backend driver to support virtio based I2C
devices.
vhost-user-i2c --help
Signed-off-by: Viresh Kumar
---
hw/virtio/vhost-user-i2c.c | 2 +-
tools/meson.build| 8 +
tools/vhost-user-i2c/50-qemu-i2c.json.in | 5 +
tools/vhost-user-i2c/main.c | 652 +++
tools/vhost-user-i2c/meson.build | 10 +
5 files changed, 676 insertions(+), 1 deletion(-)
create mode 100644 tools/vhost-user-i2c/50-qemu-i2c.json.in
create mode 100644 tools/vhost-user-i2c/main.c
create mode 100644 tools/vhost-user-i2c/meson.build
diff --git a/hw/virtio/vhost-user-i2c.c b/hw/virtio/vhost-user-i2c.c
index 7b0dc24412a4..c67c18ca00fc 100644
--- a/hw/virtio/vhost-user-i2c.c
+++ b/hw/virtio/vhost-user-i2c.c
@@ -218,7 +218,7 @@ static void vu_i2c_device_realize(DeviceState *dev, Error
**errp)
virtio_init(vdev, "vhost-user-i2c", VIRTIO_ID_I2C, 0);
-i2c->req_vq = virtio_add_queue(vdev, 3, vu_i2c_handle_output);
+i2c->req_vq = virtio_add_queue(vdev, 4, vu_i2c_handle_output);
i2c->vhost_dev.nvqs = 1;
i2c->vhost_dev.vqs = g_new0(struct vhost_virtqueue, i2c->vhost_dev.nvqs);
ret = vhost_dev_init(&i2c->vhost_dev, &i2c->vhost_user,
diff --git a/tools/meson.build b/tools/meson.build
index 3e5a0abfa29f..8271e110978b 100644
--- a/tools/meson.build
+++ b/tools/meson.build
@@ -24,3 +24,11 @@ endif
if have_virtiofsd
subdir('virtiofsd')
endif
+
+have_virtioi2c= (have_system and
+have_tools and
+'CONFIG_LINUX' in config_host)
+
+if have_virtioi2c
+ subdir('vhost-user-i2c')
+endif
diff --git a/tools/vhost-user-i2c/50-qemu-i2c.json.in
b/tools/vhost-user-i2c/50-qemu-i2c.json.in
new file mode 100644
index ..dafd1337fa9c
--- /dev/null
+++ b/tools/vhost-user-i2c/50-qemu-i2c.json.in
@@ -0,0 +1,5 @@
+{
+ "description": "QEMU vhost-user-i2c",
+ "type": "bridge",
+ "binary": "@libexecdir@/vhost-user-i2c"
+}
diff --git a/tools/vhost-user-i2c/main.c b/tools/vhost-user-i2c/main.c
new file mode 100644
index ..20942aeb189a
--- /dev/null
+++ b/tools/vhost-user-i2c/main.c
@@ -0,0 +1,652 @@
+/*
+ * VIRTIO I2C Emulation via vhost-user
+ *
+ * Copyright (c) 2021 Viresh Kumar
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#define G_LOG_DOMAIN "vhost-user-i2c"
+#define G_LOG_USE_STRUCTURED 1
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include "qemu/cutils.h"
+#include "subprojects/libvhost-user/libvhost-user-glib.h"
+#include "subprojects/libvhost-user/libvhost-user.h"
+
+/* Definitions from virtio-i2c specifications */
+#define VHOST_USER_I2C_MAX_QUEUES 1
+
+/* Status */
+#define VIRTIO_I2C_MSG_OK 0
+#define VIRTIO_I2C_MSG_ERR 1
+
+/* The bit 0 of the @virtio_i2c_out_hdr.@flags, used to group the requests */
+#define VIRTIO_I2C_FLAGS_FAIL_NEXT 0x0001
+
+/**
+ * struct virtio_i2c_out_hdr - the virtio I2C message OUT header
+ * @addr: the controlled device's address
+ * @padding: used to pad to full dword
+ * @flags: used for feature extensibility
+ */
+struct virtio_i2c_out_hdr {
+uint16_t addr;
+uint16_t padding;
+uint32_t flags;
+} __attribute__((packed));
+
+/**
+ * struct virtio_i2c_in_hdr - the virtio I2C message IN header
+ * @status: the processing result from the backend
+ */
+struct virtio_i2c_in_hdr {
+uint8_t status;
+} __attribute__((packed));
+
+/* vhost-user-i2c definitions */
+
+#ifndef container_of
+#define container_of(ptr, type, member) ({ \
+const typeof(((type *) 0)->member) *__mptr = (ptr); \
+(type *) ((char *) __mptr - offsetof(type, member));})
+#endif
+
+#define MAX_I2C_VDEV(1 << 7)
+#define MAX_I2C_ADAPTER 16
+
+typedef struct {
+int32_t fd;
+int32_t bus;
+bool clients[MAX_I2C_VDEV];
+} VI2cAdapter;
+
+typedef struct {
+VugDev dev;
+GMainLoop *loop;
+VI2cAdapter *adapter[MAX_I2C_ADAPTER];
+uint16_t adapter_map[MAX_I2C_VDEV];
+uint32_t adapter_num;
+} VuI2c;
+
+static gboolean print_cap, verbose;
+static gchar *socket_path, *device_list;
+static gint socket_fd = -1;
+
+static GOptionEntry options[] = {
+{ "socket-path", 's', 0, G_OPTION_ARG_FILENAME, &socket_path, "Location of
vhost-user Unix domain socket, incompatible with --fd", "PATH" },
+{ "fd", 'f', 0, G_OPTION_ARG_INT, &socket_fd, "Specify the file-descriptor
of the backend, incompatible with --socket-path", "FD" },
+{ "device-list", 'l', 0, G_OPTION_ARG_STRING, &device_list, "List of
i2c-dev bus and attached devices", "I2C Devices" },
+{ "print-capabilities", 'c', 0, G_OPTION_ARG_NONE, &print_cap, "Output to
stdout the backend capabilities in JSON format and exit", NULL},
+{ "verbose", 'v', 0, G_OPTION_ARG_NONE, &verbose, "Be more verbose in
o
[PATCH 5/5] MAINTAINERS: Add entry for virtio-i2c
This patch adds entry for virtio-i2c related files in MAINTAINERS. Signed-off-by: Viresh Kumar --- MAINTAINERS | 9 + 1 file changed, 9 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index 9147e9a429a0..3a80352fc85b 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -1967,6 +1967,15 @@ F: hw/virtio/virtio-mem-pci.h F: hw/virtio/virtio-mem-pci.c F: include/hw/virtio/virtio-mem.h +virtio-i2c +M: Viresh Kumar +S: Supported +F: docs/tools/vhost-user-i2c.rst +F: hw/virtio/vhost-user-i2c.c +F: hw/virtio/vhost-user-i2c-pci.c +F: include/hw/virtio/vhost-user-i2c.h +F: tools/vhost-user-i2c/* + nvme M: Keith Busch M: Klaus Jensen -- 2.25.0.rc1.19.g042ed3e048af
[PATCH 1/5] hw/virtio: add boilerplate for vhost-user-i2c device
This creates the QEMU side of the vhost-user-i2c device which connects
to the remote daemon. It is based of vhost-user-fs code.
Signed-off-by: Viresh Kumar
---
hw/virtio/Kconfig | 5 +
hw/virtio/meson.build | 1 +
hw/virtio/vhost-user-i2c.c | 286
include/hw/virtio/vhost-user-i2c.h | 37 +++
include/standard-headers/linux/virtio_ids.h | 1 +
5 files changed, 330 insertions(+)
create mode 100644 hw/virtio/vhost-user-i2c.c
create mode 100644 include/hw/virtio/vhost-user-i2c.h
diff --git a/hw/virtio/Kconfig b/hw/virtio/Kconfig
index 0eda25c4e1bf..35ab45e2095c 100644
--- a/hw/virtio/Kconfig
+++ b/hw/virtio/Kconfig
@@ -58,3 +58,8 @@ config VIRTIO_MEM
depends on LINUX
depends on VIRTIO_MEM_SUPPORTED
select MEM_DEVICE
+
+config VHOST_USER_I2C
+bool
+default y
+depends on VIRTIO && VHOST_USER
diff --git a/hw/virtio/meson.build b/hw/virtio/meson.build
index fbff9bc9d4de..1a0d736a0db5 100644
--- a/hw/virtio/meson.build
+++ b/hw/virtio/meson.build
@@ -25,6 +25,7 @@ virtio_ss.add(when: 'CONFIG_VHOST_USER_VSOCK', if_true:
files('vhost-user-vsock.
virtio_ss.add(when: 'CONFIG_VIRTIO_RNG', if_true: files('virtio-rng.c'))
virtio_ss.add(when: 'CONFIG_VIRTIO_IOMMU', if_true: files('virtio-iommu.c'))
virtio_ss.add(when: 'CONFIG_VIRTIO_MEM', if_true: files('virtio-mem.c'))
+virtio_ss.add(when: 'CONFIG_VHOST_USER_I2C', if_true:
files('vhost-user-i2c.c'))
virtio_pci_ss = ss.source_set()
virtio_pci_ss.add(when: 'CONFIG_VHOST_VSOCK', if_true:
files('vhost-vsock-pci.c'))
diff --git a/hw/virtio/vhost-user-i2c.c b/hw/virtio/vhost-user-i2c.c
new file mode 100644
index ..7b0dc24412a4
--- /dev/null
+++ b/hw/virtio/vhost-user-i2c.c
@@ -0,0 +1,286 @@
+/*
+ * Vhost-user i2c virtio device
+ *
+ * Copyright (c) 2021 Viresh Kumar
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "hw/qdev-properties.h"
+#include "hw/virtio/virtio-bus.h"
+#include "hw/virtio/vhost-user-i2c.h"
+#include "qemu/error-report.h"
+#include "standard-headers/linux/virtio_ids.h"
+
+static void vu_i2c_start(VirtIODevice *vdev)
+{
+VHostUserI2C *i2c = VHOST_USER_I2C(vdev);
+BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
+VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
+int ret;
+int i;
+
+if (!k->set_guest_notifiers) {
+error_report("binding does not support guest notifiers");
+return;
+}
+
+ret = vhost_dev_enable_notifiers(&i2c->vhost_dev, vdev);
+if (ret < 0) {
+error_report("Error enabling host notifiers: %d", -ret);
+return;
+}
+
+ret = k->set_guest_notifiers(qbus->parent, i2c->vhost_dev.nvqs, true);
+if (ret < 0) {
+error_report("Error binding guest notifier: %d", -ret);
+goto err_host_notifiers;
+}
+
+i2c->vhost_dev.acked_features = vdev->guest_features;
+ret = vhost_dev_start(&i2c->vhost_dev, vdev);
+if (ret < 0) {
+error_report("Error starting vhost-user-i2c: %d", -ret);
+goto err_guest_notifiers;
+}
+
+/*
+ * guest_notifier_mask/pending not used yet, so just unmask
+ * everything here. virtio-pci will do the right thing by
+ * enabling/disabling irqfd.
+ */
+for (i = 0; i < i2c->vhost_dev.nvqs; i++) {
+vhost_virtqueue_mask(&i2c->vhost_dev, vdev, i, false);
+}
+
+return;
+
+err_guest_notifiers:
+k->set_guest_notifiers(qbus->parent, i2c->vhost_dev.nvqs, false);
+err_host_notifiers:
+vhost_dev_disable_notifiers(&i2c->vhost_dev, vdev);
+}
+
+static void vu_i2c_stop(VirtIODevice *vdev)
+{
+VHostUserI2C *i2c = VHOST_USER_I2C(vdev);
+BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
+VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
+int ret;
+
+if (!k->set_guest_notifiers) {
+return;
+}
+
+vhost_dev_stop(&i2c->vhost_dev, vdev);
+
+ret = k->set_guest_notifiers(qbus->parent, i2c->vhost_dev.nvqs, false);
+if (ret < 0) {
+error_report("vhost guest notifier cleanup failed: %d", ret);
+return;
+}
+
+vhost_dev_disable_notifiers(&i2c->vhost_dev, vdev);
+}
+
+static void vu_i2c_set_status(VirtIODevice *vdev, uint8_t status)
+{
+VHostUserI2C *i2c = VHOST_USER_I2C(vdev);
+bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;
+
+if (!vdev->vm_running) {
+should_start = false;
+}
+
+if (i2c->vhost_dev.started == should_start) {
+return;
+}
+
+if (should_start) {
+vu_i2c_start(vdev);
+} else {
+vu_i2c_stop(vdev);
+}
+}
+
+static uint64_t vu_i2c_get_features(VirtIODevice *vdev,
+uint64_t requested_features, Error **errp)
+{
+/* No feature bits used yet */
+return requested_features;
+}
+
+static void vu_i2c_handle_output(VirtIODevice *vdev, VirtQueue *vq)
+{
+/*
+ * Not normally calle
Re: [PATCH v2 2/5] qemu-iotests: allow passing unittest.main arguments to the test scripts
24.03.2021 00:22, Paolo Bonzini wrote: On 23/03/21 20:17, Vladimir Sementsov-Ogievskiy wrote: + unittest.main(argv=argv, + testRunner=ReproducibleTestRunner, + verbosity=2 if debug else 1, + warnings=None if sys.warnoptions else 'ignore') def execute_setup_common(supported_fmts: Sequence[str] = (), supported_platforms: Sequence[str] = (), @@ -1350,7 +1354,7 @@ def execute_test(*args, test_function=None, **kwargs): debug = execute_setup_common(*args, **kwargs) if not test_function: - execute_unittest(debug) + execute_unittest(sys.argv, debug) else: test_function() If you decide to resend for some of my comments (or due to another reviewer be more careful), I think it would be nicer to merge part of this commit which moves us from passing object to passing ReproducibleTestRunner to the previous commit, to not remove line that we've added in the previous commit. And here only add argv argument. Well, it's the price to pay to make the previous commit as independent as possible. In particular in the previous patch there's no reason to add the complexity of warnings. I could make it three commits, but at some point too much splitting adds clutter, the patches are already pretty small. OK, they are good as is -- Best regards, Vladimir
Re: [PATCH v2 4/5] qemu-iotests: let "check" spawn an arbitrary test command
24.03.2021 00:20, Paolo Bonzini wrote: On 23/03/21 20:12, Vladimir Sementsov-Ogievskiy wrote: Move the trailing empty line to print_env(), since it always looks better and one caller was not adding it. Seems you've moved this fix from one unrelated commit to another.. And it touches two extra files. I'd just make it a separate commit. Nitpicking. Separate or as is: Well, now I add the third caller Ah yes. So it's related. Reviewed-by: Vladimir Sementsov-Ogievskiy -- Best regards, Vladimir
[PATCH 4/5] docs: add a man page for vhost-user-i2c
Basic usage and example invocation. Signed-off-by: Viresh Kumar --- docs/tools/index.rst | 1 + docs/tools/vhost-user-i2c.rst | 75 +++ 2 files changed, 76 insertions(+) create mode 100644 docs/tools/vhost-user-i2c.rst diff --git a/docs/tools/index.rst b/docs/tools/index.rst index 3a5829c17a54..af2519406ddf 100644 --- a/docs/tools/index.rst +++ b/docs/tools/index.rst @@ -17,3 +17,4 @@ QEMU Tools Guide qemu-trace-stap virtfs-proxy-helper virtiofsd + vhost-user-i2c diff --git a/docs/tools/vhost-user-i2c.rst b/docs/tools/vhost-user-i2c.rst new file mode 100644 index ..8471b39d8b1d --- /dev/null +++ b/docs/tools/vhost-user-i2c.rst @@ -0,0 +1,75 @@ +QEMU vhost-user-i2c - I2C emulation backend +=== + +Synopsis + + +**vhost-user-i2c** [*OPTIONS*] + +Description +--- + +This program is a vhost-user backend that emulates a VirtIO I2C bus. +This program takes the layout of the i2c bus and its devices on the host +OS and then talks to them via the /dev/i2c-X interface when a request +comes from the guest OS for an I2C device. + +This program is designed to work with QEMU's ``-device +vhost-user-i2c-pci`` but should work with any virtual machine monitor +(VMM) that supports vhost-user. See the Examples section below. + +Options +--- + +.. program:: vhost-user-i2c + +.. option:: -h, --help + + Print help. + +.. option:: -v, --verbose + + Increase verbosity of output + +.. option:: -s, --socket-path=PATH + + Listen on vhost-user UNIX domain socket at PATH. Incompatible with --fd. + +.. option:: -f, --fd=FDNUM + + Accept connections from vhost-user UNIX domain socket file descriptor FDNUM. + The file descriptor must already be listening for connections. + Incompatible with --socket-path. + +.. option:: -l, --device-list=I2C-DEVICES + + I2c device list at the host OS in the format: + :[:],[:[:]] + + Example: --device-list "2:1c:20,3:10:2c" + + Here, + bus (decimal): adatper bus number. e.g. 2 for /dev/i2c-2, 3 for /dev/i2c-3. + client_addr (hex): address for client device. e.g. 0x1C, 0x20, 0x10, 0x2C. + +Examples + + +The daemon should be started first: + +:: + + host# vhost-user-i2c --socket-path=vi2c.sock --device-list 0:20 + +The QEMU invocation needs to create a chardev socket the device can +use to communicate as well as share the guests memory over a memfd. + +:: + + host# qemu-system \ + -chardev socket,path=vi2c.sock,id=vi2c \ + -device vhost-user-i2c-pci,chardev=vi2c,id=i2c \ + -m 4096 \ + -object memory-backend-file,id=mem,size=4G,mem-path=/dev/shm,share=on \ + -numa node,memdev=mem \ + ... -- 2.25.0.rc1.19.g042ed3e048af
[Bug 1909247] Re: QEMU: use after free vulnerability in esp_do_dma() in hw/scsi/esp.c
On Wednesday, 17 March, 2021, 10:26:36 pm IST, Cheolwoo Myung
wrote:
> Hello PJP, Mauro
>
> Of course. you can post the details with our reproducers.
> I'm glad it helped you.
>
> Thank you.
> - Cheolwoo Myung
>
2021년 3월 17일 (수) 오후 10:30, P J P 님이 작성:
>
>On Monday, 15 March, 2021, 07:54:30 pm IST, Mauro Matteo Cascella
> wrote:
>>JFYI, CVE-2020-35506 was assigned to a very similar (if not the same)
>>issue, see https://bugs.launchpad.net/qemu/+bug/1909247.
>
> * From the QEMU command lines below they do look similar.
>
> * CVE bug above does not link to an upstream fix/patch. Maybe it's not fixed
> yet?
>
>
>On Mon, Mar 15, 2021 at 6:58 AM P J P wrote:
> >On Monday, 15 March, 2021, 11:11:14 am IST, Cheolwoo Myung
> > wrote:
> >Using hypervisor fuzzer, hyfuzz, I found a use-after-free issue in am53c974
> >emulator of QEMU enabled ASan.
> >
> ># To reproduce this issue, please run the QEMU process with the following
> >command line.
> >$ ./qemu-system-i386 -m 512 -drive
> >file=./hyfuzz.img,index=0,media=disk,format=raw \
> > -device am53c974,id=scsi -device scsi-hd,drive=SysDisk -drive
> > >id=SysDisk,if=none,file=./disk.img
> >
> >
> > Using hypervisor fuzzer, hyfuzz, I found a stack buffer overflow issue in
> > am53c974 emulator of QEMU enabled ASan.
> >
> ># To reproduce this issue, please run the QEMU process with the following
> >command line.
> >$ ./qemu-system-i386 -m 512 -drive
> >file=./hyfuzz.img,index=0,media=disk,format=raw \
> > -device am53c974,id=scsi -device scsi-hd,drive=SysDisk -drive
> > >id=SysDisk,if=none,file=./disk.img
> >
* I was able to reproduce these issues against the latest upstream git source
and following patch helps to fix above two issues.
===
$ git diff hw/scsi/
diff --git a/hw/scsi/esp-pci.c b/hw/scsi/esp-pci.c
index c3d3dab05e..4a6f208069 100644
--- a/hw/scsi/esp-pci.c
+++ b/hw/scsi/esp-pci.c
@@ -98,6 +98,7 @@ static void esp_pci_handle_abort(PCIESPState *pci, uint32_t
val)
trace_esp_pci_dma_abort(val);
if (s->current_req) {
scsi_req_cancel(s->current_req);
+s->async_len = 0;
}
}
diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 507ab363bc..99bee7bc66 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -564,7 +564,7 @@ static void esp_do_dma(ESPState *s)
int to_device = ((s->rregs[ESP_RSTAT] & 7) == STAT_DO);
uint8_t buf[ESP_CMDFIFO_SZ];
-len = esp_get_tc(s);
+len = MIN(esp_get_tc(s), sizeof(buf));
if (s->do_cmd) {
/*
===
> >Using hypervisor fuzzer, hyfuzz, I found a heap buffer overflow issue in
> >am53c974 emulator of QEMU enabled ASan.
> >
> ># To reproduce this issue, please run the QEMU process with the following
> >command line.
> >$ ./qemu-system-i386 -m 512 -drive
> >file=./hyfuzz.img,index=0,media=disk,format=raw \
> > -device am53c974,id=scsi -device scsi-hd,drive=SysDisk -drive
> > >id=SysDisk,if=none,file=./disk.img
* This heap OOB access issue seems to occur because
static void do_busid_cmd(...)
...
buf = (uint8_t *)fifo8_pop_buf(&s->cmdfifo, cmdlen, &n); <==
'buf' points towards an end of the 32 byte buffer allocated via
static void esp_init(Object *obj)
...
fifo8_create(&s->cmdfifo, ESP_CMDFIFO_SZ(=32)); <==
and the OOB access could occur at numerous places, one of which is
scsi_req_new
-> scsi_req_parse_cdb
-> memcpy(cmd->buf, buf, cmd->len); <== buf=27, cmd->len=6 <= 27+6 exceeds
limit 32.
* This one is quite tricky to fix. Because 'buf[]' is accessed at various
places with hard coded index values. It's not easy to check access
against 's->cmdfifo' object.
@Cheolwoo: is it okay with you if we post above details and your reproducers on
the upstream bug
-> https://bugs.launchpad.net/qemu/+bug/1909247
It'll help to discuss/prepare a proper fix patch.
Thank you.
---
-P J P
http://feedmug.com
** Attachment added: "hw-esp-oob-issues.zip"
https://bugs.launchpad.net/qemu/+bug/1909247/+attachment/5480385/+files/hw-esp-oob-issues.zip
--
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1909247
Title:
QEMU: use after free vulnerability in esp_do_dma() in hw/scsi/esp.c
Status in QEMU:
New
Bug description:
A use-after-free vulnerability was found in the am53c974 SCSI host bus
adapter emulation of QEMU. It could occur in the esp_do_dma() function
in hw/scsi/esp.c while handling the 'Information Transfer' command
(CMD_TI). A privileged guest user may abuse this flaw to crash the
QEMU process on the host, resulting in a denial of service or
potential code execution with the privileges of the QEMU process.
This issue was reported by Cheolwoo Myung (Seoul National University).
Original report:
Using hypervisor fuzzer, hyfuzz, I found a use-after-free issue in
am53c974 emulator of QEMU enabled ASan.
It occurs while transferring information, as it does not check the
buffer to be transferred.
Re: [PATCH 0/5] virtio: Implement generic vhost-user-i2c backend
Patchew URL:
https://patchew.org/QEMU/cover.1616570702.git.viresh.ku...@linaro.org/
Hi,
This series seems to have some coding style problems. See output below for
more information:
Type: series
Message-id: cover.1616570702.git.viresh.ku...@linaro.org
Subject: [PATCH 0/5] virtio: Implement generic vhost-user-i2c backend
=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
* [new tag] patchew/cover.1616570702.git.viresh.ku...@linaro.org ->
patchew/cover.1616570702.git.viresh.ku...@linaro.org
Switched to a new branch 'test'
40ccc27 MAINTAINERS: Add entry for virtio-i2c
9b632f4 docs: add a man page for vhost-user-i2c
5da2cca tools/vhost-user-i2c: Add backend driver
6716f9a hw/virtio: add vhost-user-i2c-pci boilerplate
d6439eb hw/virtio: add boilerplate for vhost-user-i2c device
=== OUTPUT BEGIN ===
1/5 Checking commit d6439ebcaa1d (hw/virtio: add boilerplate for vhost-user-i2c
device)
Use of uninitialized value $acpi_testexpected in string eq at
./scripts/checkpatch.pl line 1529.
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#46:
new file mode 100644
total: 0 errors, 1 warnings, 344 lines checked
Patch 1/5 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
2/5 Checking commit 6716f9a6b4c1 (hw/virtio: add vhost-user-i2c-pci boilerplate)
Use of uninitialized value $acpi_testexpected in string eq at
./scripts/checkpatch.pl line 1529.
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#31:
new file mode 100644
total: 0 errors, 1 warnings, 86 lines checked
Patch 2/5 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
3/5 Checking commit 5da2ccae459c (tools/vhost-user-i2c: Add backend driver)
Use of uninitialized value $acpi_testexpected in string eq at
./scripts/checkpatch.pl line 1529.
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#52:
new file mode 100644
ERROR: spaces required around that '*' (ctx:WxV)
#137: FILE: tools/vhost-user-i2c/main.c:70:
+const typeof(((type *) 0)->member) *__mptr = (ptr); \
^
ERROR: space required after that ';' (ctx:VxV)
#138: FILE: tools/vhost-user-i2c/main.c:71:
+(type *) ((char *) __mptr - offsetof(type, member));})
^
ERROR: line over 90 characters
#163: FILE: tools/vhost-user-i2c/main.c:96:
+{ "socket-path", 's', 0, G_OPTION_ARG_FILENAME, &socket_path, "Location of
vhost-user Unix domain socket, incompatible with --fd", "PATH" },
ERROR: line over 90 characters
#164: FILE: tools/vhost-user-i2c/main.c:97:
+{ "fd", 'f', 0, G_OPTION_ARG_INT, &socket_fd, "Specify the file-descriptor
of the backend, incompatible with --socket-path", "FD" },
ERROR: line over 90 characters
#165: FILE: tools/vhost-user-i2c/main.c:98:
+{ "device-list", 'l', 0, G_OPTION_ARG_STRING, &device_list, "List of
i2c-dev bus and attached devices", "I2C Devices" },
ERROR: line over 90 characters
#166: FILE: tools/vhost-user-i2c/main.c:99:
+{ "print-capabilities", 'c', 0, G_OPTION_ARG_NONE, &print_cap, "Output to
stdout the backend capabilities in JSON format and exit", NULL},
WARNING: line over 80 characters
#167: FILE: tools/vhost-user-i2c/main.c:100:
+{ "verbose", 'v', 0, G_OPTION_ARG_NONE, &verbose, "Be more verbose in
output", NULL},
WARNING: line over 80 characters
#209: FILE: tools/vhost-user-i2c/main.c:142:
+ * Adapter| adapter2 | none | adapter1 | adapter3 | none | none|
(val)
WARNING: line over 80 characters
#211: FILE: tools/vhost-user-i2c/main.c:144:
+ * Slave Address | addr 1 | none | addr 2 | addr 3 | none | none|
(idx)
ERROR: do not use assignment in if condition
#241: FILE: tools/vhost-user-i2c/main.c:174:
+if (addr < MAX_I2C_VDEV && ((idx = i2c->adapter_map[addr]) != 0)) {
ERROR: braces {} are necessary for all arms of this statement
#288: FILE: tools/vhost-user-i2c/main.c:221:
+if (bus < 0)
[...]
WARNING: line over 80 characters
#316: FILE: tools/vhost-user-i2c/main.c:249:
+g_printerr("client addr 0x%x repeat, not allowed.\n",
client_addr[i]);
ERROR: code indent should never use tabs
#340: FILE: tools/vhost-user-i2c/main.c:273:
+ * ^Ie.g. 2 for /dev/i2c-2$
ERROR: code indent should never use tabs
#342: FILE: tools/vhost-user-i2c/main.c:275:
+ * ^Ie.g. 0x1C or 1C$
ERROR: spaces required around that '==' (ctx:WxV)
#359: FILE: tools/vhost-user-i2c/main.c:292:
+if (!cp || *cp =='\0') {
Re: [PATCH v1 1/3] migration: Fix missing qemu_fflush() on buffer file in bg_migration_thread
On 23.03.2021 21:35, Peter Xu wrote: On Tue, Mar 23, 2021 at 08:21:43PM +0300, Andrey Gruzdev wrote: For the long term I think we'd better have a helper: qemu_put_qio_channel_buffer(QEMUFile *file, QIOChannelBuffer *bioc) So as to hide this flush operation, which is tricky. We'll have two users so far: bg_migration_completion colo_do_checkpoint_transaction IMHO it'll be nicer if you'd do it in this patch altogether! Thanks, Sorry, can't get the idea, what's wrong with the fix. I'm fine with the fix, but I've got one patch attached just to show what I meant, so without any testing for sure.. Looks more complicated than I thought, but again I think we should hide that buffer flush into another helper to avoid overlooking it. Thanks, Peter, now I've got what you meant - not to overlook flush on buffer channel. But it seems that adding a reasonable comment is enough here. Thanks,
Re: [PATCH qemu] spapr: Fix typo in the patb_entry comment
On Wed, Mar 24, 2021 at 03:30:57PM +1100, Alexey Kardashevskiy wrote: > > > On 22/03/2021 16:44, David Gibson wrote: > > On Thu, Feb 25, 2021 at 02:23:35PM +1100, Alexey Kardashevskiy wrote: > > > There is no H_REGISTER_PROCESS_TABLE, it is H_REGISTER_PROC_TBL handler > > > for which is still called h_register_process_table() though. > > > > > > Signed-off-by: Alexey Kardashevskiy > > > > Applied to ppc-for-6.0. > > > > In future, best to CC me directly, since I only occasionally peruse > > the lists. > > > > even the qemu-...@nongnu.org list? okay :) Yes. I do try to read qemu-...@nongnu.org weekly, but it's still easy for me to miss things. -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson signature.asc Description: PGP signature
[PATCH v3] i386/cpu_dump: support AVX512 ZMM regs dump
Since commit fa4518741e (target-i386: Rename struct XMMReg to ZMMReg),
CPUX86State.xmm_regs[] has already been extended to 512bit to support
AVX512.
Also, other qemu level supports for AVX512 registers are there for
years.
But in x86_cpu_dump_state(), still only dump XMM registers no matter
YMM/ZMM is enabled.
This patch is to complement this, let it dump XMM/YMM/ZMM accordingly.
Signed-off-by: Robert Hoo
---
Changelog:
v3: fix some coding style issue.
v2: dump XMM/YMM/ZMM according to XSAVE state-components enablement.
target/i386/cpu-dump.c | 55 --
target/i386/cpu.h | 11 ++
2 files changed, 51 insertions(+), 15 deletions(-)
diff --git a/target/i386/cpu-dump.c b/target/i386/cpu-dump.c
index aac21f1..00fb56f 100644
--- a/target/i386/cpu-dump.c
+++ b/target/i386/cpu-dump.c
@@ -499,21 +499,46 @@ void x86_cpu_dump_state(CPUState *cs, FILE *f, int flags)
else
qemu_fprintf(f, " ");
}
-if (env->hflags & HF_CS64_MASK)
-nb = 16;
-else
-nb = 8;
-for(i=0;ixmm_regs[i].ZMM_L(3),
- env->xmm_regs[i].ZMM_L(2),
- env->xmm_regs[i].ZMM_L(1),
- env->xmm_regs[i].ZMM_L(0));
-if ((i & 1) == 1)
-qemu_fprintf(f, "\n");
-else
-qemu_fprintf(f, " ");
+
+if ((env->xcr0 & XFEATURE_AVX512) == XFEATURE_AVX512) {
+/* XSAVE enabled AVX512 */
+nb = (env->hflags & HF_CS64_MASK) ? 32 : 8;
+for (i = 0; i < nb; i++) {
+qemu_fprintf(f, "ZMM%02d=0x%016lx %016lx %016lx %016lx %016lx "
+"%016lx %016lx %016lx\n",
+ i,
+ env->xmm_regs[i].ZMM_Q(7),
+ env->xmm_regs[i].ZMM_Q(6),
+ env->xmm_regs[i].ZMM_Q(5),
+ env->xmm_regs[i].ZMM_Q(4),
+ env->xmm_regs[i].ZMM_Q(3),
+ env->xmm_regs[i].ZMM_Q(2),
+ env->xmm_regs[i].ZMM_Q(1),
+ env->xmm_regs[i].ZMM_Q(0));
+}
+} else if (env->xcr0 & XFEATURE_AVX) {
+/* XSAVE enabled AVX */
+nb = env->hflags & HF_CS64_MASK ? 16 : 8;
+for (i = 0; i < nb; i++) {
+qemu_fprintf(f, "YMM%02d=0x%016lx %016lx %016lx %016lx\n",
+ i,
+ env->xmm_regs[i].ZMM_Q(3),
+ env->xmm_regs[i].ZMM_Q(2),
+ env->xmm_regs[i].ZMM_Q(1),
+ env->xmm_regs[i].ZMM_Q(0));
+}
+} else { /* SSE and below cases */
+nb = env->hflags & HF_CS64_MASK ? 16 : 8;
+for (i = 0; i < nb; i++) {
+qemu_fprintf(f, "XMM%02d=0x%016lx %016lx",
+ i,
+ env->xmm_regs[i].ZMM_Q(1),
+ env->xmm_regs[i].ZMM_Q(0));
+if ((i & 1) == 1)
+qemu_fprintf(f, "\n");
+else
+qemu_fprintf(f, " ");
+}
}
}
if (flags & CPU_DUMP_CODE) {
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 570f916..82f5d56 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -249,6 +249,17 @@ typedef enum X86Seg {
#define CR4_PKE_MASK (1U << 22)
#define CR4_PKS_MASK (1U << 24)
+#define XFEATURE_X87(1UL << 0)
+#define XFEATURE_SSE(1UL << 1)
+#define XFEATURE_AVX(1UL << 2)
+#define XFEATURE_AVX512_OPMASK (1UL << 5)
+#define XFEATURE_AVX512_ZMM_Hi256 (1UL << 6)
+#define XFEATURE_AVX512_Hi16_ZMM(1UL << 7)
+#define XFEATURE_AVX512 (XFEATURE_AVX512_OPMASK | \
+ XFEATURE_AVX512_ZMM_Hi256 | \
+ XFEATURE_AVX512_Hi16_ZMM)
+
+
#define DR6_BD (1 << 13)
#define DR6_BS (1 << 14)
#define DR6_BT (1 << 15)
--
1.8.3.1
Re: Crashes with qemu-system-ppc64
On Wed, 24 Mar 2021 00:35:05 +0100
Philippe Mathieu-Daudé wrote:
> On 3/24/21 12:00 AM, Greg Kurz wrote:
> > Cc'ing David
> >
> > On Tue, 23 Mar 2021 17:48:36 +0100
> > Thomas Huth wrote:
> >
> >>
> >> In case anyone is interested in fixing those, there are two regressions
> >> with
> >> qemu-system-ppc64 in the current master branch:
> >>
> >> $ ./qemu-system-ppc64 -M ppce500 -device macio-oldworld
> >> qemu-system-ppc64: ../../devel/qemu/softmmu/memory.c:2443:
> >> memory_region_add_subregion_common: Assertion `!subregion->container'
> >> failed.
> >>
> >> $ ./qemu-system-ppc64 -device power8_v2.0-spapr-cpu-core,help
> >> /home/thuth/devel/qemu/include/hw/boards.h:24:MACHINE: Object
> >> 0x5635bd53af10
> >> is not an instance of type machine
> >> Aborted (core dumped)
> >>
> >
> > I've bisected this one to:
> >
> > 3df261b6676b5850e93d6fab3f7a98f8ee8f19c5 is the first bad commit
> > commit 3df261b6676b5850e93d6fab3f7a98f8ee8f19c5
> > Author: Peter Maydell
> > Date: Fri Mar 13 17:24:47 2020 +
> >
> > softmmu/vl.c: Handle '-cpu help' and '-device help' before 'no default
> > machine'
> >
> > Currently if you try to ask for the list of CPUs for a target
> > architecture which does not specify a default machine type
> > you just get an error:
> >
> > $ qemu-system-arm -cpu help
> > qemu-system-arm: No machine specified, and there is no default
> > Use -machine help to list supported machines
> >
> > Since the list of CPUs doesn't depend on the machine, this is
> > unnecessarily unhelpful. "-device help" has a similar problem.
> >
> > Move the checks for "did the user ask for -cpu help or -device help"
> > up so they precede the select_machine() call which checks that the
> > user specified a valid machine type.
> >
> > Signed-off-by: Peter Maydell
> > Signed-off-by: Paolo Bonzini
> >
> > softmmu/vl.c | 26 --
> > 1 file changed, 16 insertions(+), 10 deletions(-)
> > bisect run success
> >
> > This change is fine but it unveils a bad assumption.
> >
> > 0 0x764a3708 in raise () at /lib64/power9/libc.so.6
> > #1 0x76483bcc in abort () at /lib64/power9/libc.so.6
> > #2 0x0001008db940 in object_dynamic_cast_assert
> > (obj=0x10126f670, typename=0x100c20380 "machine", file=0x100b34878
> > "/home/greg/Work/qemu/qemu-ppc/include/hw/boards.h", line=,
> > func=0x100bcd320 <__func__.30338> "MACHINE") at ../../qom/object.c:883
> > #3 0x000100456e00 in MACHINE (obj=) at
> > /home/greg/Work/qemu/qemu-ppc/include/hw/boards.h:24
> > #4 0x000100456e00 in cpu_core_instance_init (obj=0x10118e2c0) at
> > ../../hw/cpu/core.c:69
> > #5 0x0001008d9f44 in object_init_with_type (obj=obj@entry=0x10118e2c0,
> > ti=0x1011fd470) at ../../qom/object.c:375
> > #6 0x0001008d9f24 in object_init_with_type (obj=obj@entry=0x10118e2c0,
> > ti=0x101211ad0) at ../../qom/object.c:371
> > #7 0x0001008d9f24 in object_init_with_type (obj=obj@entry=0x10118e2c0,
> > ti=ti@entry=0x101212760) at ../../qom/object.c:371
> > #8 0x0001008dc474 in object_initialize_with_type
> > (obj=obj@entry=0x10118e2c0, size=size@entry=160,
> > type=type@entry=0x101212760) at ../../qom/object.c:517
> > #9 0x0001008dc678 in object_new_with_type (type=0x101212760) at
> > ../../qom/object.c:732
> > #10 0x0001009fbad8 in qmp_device_list_properties (typename= > out>, errp=) at ../../qom/qom-qmp-cmds.c:146
> > #11 0x0001005a4bf0 in qdev_device_help (opts=0x10126c200) at
> > ../../softmmu/qdev-monitor.c:285
> > #12 0x000100760afc in device_help_func (opaque=,
> > opts=, errp=) at ../../softmmu/vl.c:1204
> > #13 0x000100ad1050 in qemu_opts_foreach (list=,
> > func=0x100760ae0 , opaque=0x0, errp=0x0) at
> > ../../util/qemu-option.c:1167
> > #14 0x0001007653cc in qemu_process_help_options () at
> > ../../softmmu/vl.c:2451
> > #15 0x0001007653cc in qemu_init (argc=, argv= > out>, envp=) at ../../softmmu/vl.c:3521
> > #16 0x0001002f4f88 in main (argc=, argv=,
> > envp=) at ../../softmmu/main.c:49
> >
> > Basically, "-device power8_v2.0-spapr-cpu-core,help" ends up
> > instantiating an object of the "power8_v2.0-spapr-cpu-core" type,
> > which derives from "cpu-core". The "cpu-core" type has an instance
> > init function that assumes that qdev_get_machine() returns an object
> > of type "machine"...
> >
> > static void cpu_core_instance_init(Object *obj)
> > {
> > MachineState *ms = MACHINE(qdev_get_machine());
> > ^^
> > ...here.
> >
> > qdev_get_machine() cannot return a valid machine type since
> > select_machine() hasn't been called yet... an instance init
> > function is probably not the best place to use qdev_get_machine()
> > if any.
>
> Hmmm does this assert() matches your comment?
>
> -- >8 --
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index cefc5eaa0a9..41cbee77d14 100644
>
Re: [PATCH 05/20] qobject: Change qobject_to_json()'s value to GString
Thomas Huth writes:
> On 11/12/2020 18.11, Markus Armbruster wrote:
>> qobject_to_json() and qobject_to_json_pretty() build a GString, then
>> covert it to QString. Just one of the callers actually needs a
>> QString: qemu_rbd_parse_filename(). A few others need a string they
>> can modify: qmp_send_response(), qga's send_response(), to_json_str(),
>> and qmp_fd_vsend_fds(). The remainder just need a string.
>> Change qobject_to_json() and qobject_to_json_pretty() to return the
>> GString.
>> qemu_rbd_parse_filename() now has to convert to QString. All others
>> save a QString temporary. to_json_str() actually becomes a bit
>> simpler, because GString provides more convenient modification
>> functions.
>> Signed-off-by: Markus Armbruster
>
> Hi Markus!
>
> This patch broke the output of default values in the device help. With
> commit eab3a4678b07267c39e72:
>
> $ ./qemu-system-x86_64 -device pvpanic,help
> pvpanic options:
> events= - (default: (null))
> ioport=- (default: (null))
> pvpanic[0]=>
>
> With the commit right before that one:
>
> $ ./qemu-system-x86_64 -device pvpanic,help
> pvpanic options:
> events= - (default: 3)
> ioport=- (default: 1285)
> pvpanic[0]=>
>
> Any ideas how to fix that?
Yes:
diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
index c3324b0f86..bd8a947a63 100644
--- a/qom/object_interfaces.c
+++ b/qom/object_interfaces.c
@@ -159,7 +159,7 @@ char *object_property_help(const char *name, const char
*type,
}
if (defval) {
g_autofree char *def_json = g_string_free(qobject_to_json(defval),
- true);
+ false);
g_string_append_printf(str, " (default: %s)", def_json);
}
I'll post a proper patch. Sorry for the dumb mistake, and thanks for
pinpointing the commit!
[Bug 1909247] Re: QEMU: use after free vulnerability in esp_do_dma() in hw/scsi/esp.c
Can you confirm that this is fixed in the v2 of the above patchset? https://lists.gnu.org/archive/html/qemu-devel/2021-03/msg06550.html ATB, Mark. -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1909247 Title: QEMU: use after free vulnerability in esp_do_dma() in hw/scsi/esp.c Status in QEMU: New Bug description: A use-after-free vulnerability was found in the am53c974 SCSI host bus adapter emulation of QEMU. It could occur in the esp_do_dma() function in hw/scsi/esp.c while handling the 'Information Transfer' command (CMD_TI). A privileged guest user may abuse this flaw to crash the QEMU process on the host, resulting in a denial of service or potential code execution with the privileges of the QEMU process. This issue was reported by Cheolwoo Myung (Seoul National University). Original report: Using hypervisor fuzzer, hyfuzz, I found a use-after-free issue in am53c974 emulator of QEMU enabled ASan. It occurs while transferring information, as it does not check the buffer to be transferred. A malicious guest user/process could use this flaw to crash the QEMU process resulting in DoS scenario. To reproduce this issue, please run the QEMU with the following command line. # To enable ASan option, please set configuration with the following $ ./configure --target-list=i386-softmmu --disable-werror --enable-sanitizers $ make # To reproduce this issue, please run the QEMU process with the following command line $ ./qemu-system-i386 -m 512 -drive file=./hyfuzz.img,index=0,media=disk,format=raw \ -device am53c974,id=scsi -device scsi-hd,drive=SysDisk \ -drive id=SysDisk,if=none,file=./disk.img Please find attached the disk images to reproduce this issue. To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1909247/+subscriptions
Re: [PATCH v1 0/3] migration: Fixes to the 'background-snapshot' code
On 24.03.2021 01:21, Peter Xu wrote: On Fri, Mar 19, 2021 at 05:52:46PM +0300, Andrey Gruzdev wrote: Changes v0->v1: * Using qemu_real_host_page_size instead of TARGET_PAGE_SIZE for host page size in ram_block_populate_pages() * More elegant implementation of ram_block_populate_pages() This patch series contains: * Fix to the issue with occasionally truncated non-iterable device state * Solution to compatibility issues with virtio-balloon device * Fix to the issue when discarded or never populated pages miss UFFD write protection and get into migration stream in dirty state Andrey Gruzdev (3): migration: Fix missing qemu_fflush() on buffer file in bg_migration_thread migration: Inhibit virtio-balloon for the duration of background snapshot migration: Pre-fault memory before starting background snasphot Unless Andrey would like to respin a new version, this version looks good to me (I don't think the adding new helper issue in patch 1 is a blocker): Reviewed-by: Peter Xu Thanks. I'm also looking into introducing UFFD_FEATURE_WP_UNALLOCATED so as to wr-protect page holes too for a uffd-wp region when the feature bit is set. With that feature we should be able to avoid pre-fault as what we do in the last patch of this series. However even if that can work out, we'll still need this for old kernel anyways. I'm curious this new feature is based on adding wr-protection at the level of VMAs, so we won't miss write faults for missing pages? Thanks,
Re: [RFC v11 30/55] target/arm: wrap call to aarch64_sve_change_el in tcg_enabled()
On 3/23/21 11:50 PM, Alex Bennée wrote:
>
> Claudio Fontana writes:
>
>> After this patch it is possible to build only kvm:
>>
>> ./configure --disable-tcg --enable-kvm
It's possible to build, but tests will fail until all the test-related patches
are applied.
>
> FWIW at this point we get a different failure than later on:
>
> 21:10:25 [alex@aarch64-new:~/l/q/b/disable.tcg] (94e2abe0…)|… + make
> check-qtest
> GIT ui/keycodemapdb tests/fp/berkeley-testfloat-3
> tests/fp/berkeley-softfloat-3 meson dtc capstone slirp
> [1/19] Generating qemu-version.h with a meson_exe.py custom command
> Running test qtest-aarch64/qom-test
> qemu-system-aarch64: missing interface 'idau-interface' for object 'machine'
This one is broken by a recent commit in QEMU mainline, by removing the idau
interface from KVM cpus.
This is fixed by: Revert "target/arm: Restrict v8M IDAU to TCG" in the series.
> socket_accept failed: Resource temporarily unavailable
> **
> ERROR:../../tests/qtest/libqtest.c:319:qtest_init_without_qmp_handshake:
> assertion failed: (s->fd >= 0 && s->qmp_fd >= 0)
> ERROR qtest-aarch64/qom-test - Bail out!
> ERROR:../../tests/qtest/libqtest.c:319:qtest_init_without_qmp_handshake:
> assertion failed: (s->fd >= 0 && s->qmp_fd >= 0)
> make: *** [Makefile.mtest:24: run-test-1] Error 1
>
>
>>
>> Signed-off-by: Claudio Fontana
>> ---
>> target/arm/cpu-sysemu.c | 12 +++-
>> 1 file changed, 7 insertions(+), 5 deletions(-)
>>
>> diff --git a/target/arm/cpu-sysemu.c b/target/arm/cpu-sysemu.c
>> index eb928832a9..05d6e79ad9 100644
>> --- a/target/arm/cpu-sysemu.c
>> +++ b/target/arm/cpu-sysemu.c
>> @@ -820,11 +820,13 @@ static void arm_cpu_do_interrupt_aarch64(CPUState *cs)
>> unsigned int cur_el = arm_current_el(env);
>> int rt;
>>
>> -/*
>> - * Note that new_el can never be 0. If cur_el is 0, then
>> - * el0_a64 is is_a64(), else el0_a64 is ignored.
>> - */
>> -aarch64_sve_change_el(env, cur_el, new_el, is_a64(env));
>> +if (tcg_enabled()) {
>> +/*
>> + * Note that new_el can never be 0. If cur_el is 0, then
>> + * el0_a64 is is_a64(), else el0_a64 is ignored.
>> + */
>> +aarch64_sve_change_el(env, cur_el, new_el, is_a64(env));
>> +}
>>
>> if (cur_el < new_el) {
>> /* Entry vector offset depends on whether the implemented EL
>
>
Re: [PATCH v2 05/10] Acceptance Tests: add port redirection for ssh by default
Hi
On Wed, Mar 24, 2021 at 2:23 AM Cleber Rosa wrote:
> For users of the LinuxTest class, let's set up the VM with the port
> redirection for SSH, instead of requiring each test to set the same
> arguments.
>
> Signed-off-by: Cleber Rosa
> ---
> tests/acceptance/avocado_qemu/__init__.py | 4 +++-
> tests/acceptance/virtiofs_submounts.py| 4
> 2 files changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/tests/acceptance/avocado_qemu/__init__.py
> b/tests/acceptance/avocado_qemu/__init__.py
> index 67f75f66e5..e75b002c70 100644
> --- a/tests/acceptance/avocado_qemu/__init__.py
> +++ b/tests/acceptance/avocado_qemu/__init__.py
> @@ -309,10 +309,12 @@ class LinuxTest(Test, LinuxSSHMixIn):
> timeout = 900
> chksum = None
>
> -def setUp(self, ssh_pubkey=None):
> +def setUp(self, ssh_pubkey=None, network_device_type='virtio-net'):
> super(LinuxTest, self).setUp()
> self.vm.add_args('-smp', '2')
> self.vm.add_args('-m', '1024')
> +self.vm.add_args('-netdev', 'user,id=vnet,hostfwd=:127.0.0.1:0
> -:22',
> + '-device', '%s,netdev=vnet' %
> network_device_type)
> self.set_up_boot()
> if ssh_pubkey is None:
> ssh_pubkey, self.ssh_key = self.set_up_existing_ssh_keys()
> diff --git a/tests/acceptance/virtiofs_submounts.py
> b/tests/acceptance/virtiofs_submounts.py
> index bed8ce44df..e10a935ac4 100644
> --- a/tests/acceptance/virtiofs_submounts.py
> +++ b/tests/acceptance/virtiofs_submounts.py
> @@ -207,10 +207,6 @@ def setUp(self):
> self.vm.add_args('-kernel', vmlinuz,
> '-append', 'console=ttyS0 root=/dev/sda1')
>
> -# Allow us to connect to SSH
> -self.vm.add_args('-netdev', 'user,id=vnet,hostfwd=:127.0.0.1:0
> -:22',
> - '-device', 'virtio-net,netdev=vnet')
> -
> self.require_accelerator("kvm")
> self.vm.add_args('-accel', 'kvm')
>
> --
> 2.25.4
>
>
Looks fine, I suppose it could eventually be in LinuxSSHMixIn too for other
users.
Reviewed-by: Marc-André Lureau
--
Marc-André Lureau
Re: [PATCH v2 06/10] Acceptance Tests: make username/password configurable
On Wed, Mar 24, 2021 at 2:21 AM Cleber Rosa wrote: > This makes the username/password used for authentication configurable, > because some guest operating systems may have restrictions on accounts > to be used for logins, and it just makes it better documented. > > Signed-off-by: Cleber Rosa > Reviewed-by: Marc-André Lureau > --- > tests/acceptance/avocado_qemu/__init__.py | 6 -- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/tests/acceptance/avocado_qemu/__init__.py > b/tests/acceptance/avocado_qemu/__init__.py > index e75b002c70..535f63a48d 100644 > --- a/tests/acceptance/avocado_qemu/__init__.py > +++ b/tests/acceptance/avocado_qemu/__init__.py > @@ -308,6 +308,8 @@ class LinuxTest(Test, LinuxSSHMixIn): > > timeout = 900 > chksum = None > +username = 'root' > +password = 'password' > > def setUp(self, ssh_pubkey=None, network_device_type='virtio-net'): > super(LinuxTest, self).setUp() > @@ -370,8 +372,8 @@ def prepare_cloudinit(self, ssh_pubkey=None): > with open(ssh_pubkey) as pubkey: > pubkey_content = pubkey.read() > cloudinit.iso(cloudinit_iso, self.name, > - username='root', > - password='password', > + username=self.username, > + password=self.password, ># QEMU's hard coded usermode router address >phone_home_host='10.0.2.2', >phone_home_port=self.phone_home_port, > -- > 2.25.4 > > > -- Marc-André Lureau
Re: [PATCH v2 07/10] Acceptance Tests: set up SSH connection by default after boot for LinuxTest
On Wed, Mar 24, 2021 at 2:34 AM Cleber Rosa wrote:
> The LinuxTest specifically targets users that need to interact with Linux
> guests. So, it makes sense to give a connection by default, and avoid
> requiring it as boiler-plate code.
>
> Signed-off-by: Cleber Rosa
>
Reviewed-by: Marc-André Lureau
> ---
> tests/acceptance/avocado_qemu/__init__.py | 5 -
> tests/acceptance/virtiofs_submounts.py| 1 -
> 2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/tests/acceptance/avocado_qemu/__init__.py
> b/tests/acceptance/avocado_qemu/__init__.py
> index 535f63a48d..4960142bcc 100644
> --- a/tests/acceptance/avocado_qemu/__init__.py
> +++ b/tests/acceptance/avocado_qemu/__init__.py
> @@ -390,7 +390,7 @@ def set_up_cloudinit(self, ssh_pubkey=None):
> cloudinit_iso = self.prepare_cloudinit(ssh_pubkey)
> self.vm.add_args('-drive', 'file=%s,format=raw' % cloudinit_iso)
>
> -def launch_and_wait(self):
> +def launch_and_wait(self, set_up_ssh_connection=True):
> self.vm.set_console()
> self.vm.launch()
> console_drainer =
> datadrainer.LineLogger(self.vm.console_socket.fileno(),
> @@ -398,3 +398,6 @@ def launch_and_wait(self):
> console_drainer.start()
> self.log.info('VM launched, waiting for boot confirmation from
> guest')
> cloudinit.wait_for_phone_home(('0.0.0.0', self.phone_home_port),
> self.name)
> +if set_up_ssh_connection:
> +self.log.info('Setting up the SSH connection')
> +self.ssh_connect(self.username, self.ssh_key)
> diff --git a/tests/acceptance/virtiofs_submounts.py
> b/tests/acceptance/virtiofs_submounts.py
> index e10a935ac4..e019d3b896 100644
> --- a/tests/acceptance/virtiofs_submounts.py
> +++ b/tests/acceptance/virtiofs_submounts.py
> @@ -136,7 +136,6 @@ def set_up_virtiofs(self):
>
> def launch_vm(self):
> self.launch_and_wait()
> -self.ssh_connect('root', self.ssh_key)
>
> def set_up_nested_mounts(self):
> scratch_dir = os.path.join(self.shared_dir, 'scratch')
> --
> 2.25.4
>
>
>
--
Marc-André Lureau
Re: [PATCH v2 08/10] tests/acceptance/virtiofs_submounts.py: remove launch_vm()
On Wed, Mar 24, 2021 at 2:32 AM Cleber Rosa wrote: > The LinuxTest class' launch_and_wait() method now behaves the same way > as this test's custom launch_vm(), so let's just use the upper layer > (common) method. > > Signed-off-by: Cleber Rosa > Reviewed-by: Marc-André Lureau > --- > tests/acceptance/virtiofs_submounts.py | 13 + > 1 file changed, 5 insertions(+), 8 deletions(-) > > diff --git a/tests/acceptance/virtiofs_submounts.py > b/tests/acceptance/virtiofs_submounts.py > index e019d3b896..d77ee35674 100644 > --- a/tests/acceptance/virtiofs_submounts.py > +++ b/tests/acceptance/virtiofs_submounts.py > @@ -134,9 +134,6 @@ def set_up_virtiofs(self): > '-numa', > 'node,memdev=mem') > > -def launch_vm(self): > -self.launch_and_wait() > - > def set_up_nested_mounts(self): > scratch_dir = os.path.join(self.shared_dir, 'scratch') > try: > @@ -225,7 +222,7 @@ def test_pre_virtiofsd_set_up(self): > self.set_up_nested_mounts() > > self.set_up_virtiofs() > -self.launch_vm() > +self.launch_and_wait() > self.mount_in_guest() > self.check_in_guest() > > @@ -235,14 +232,14 @@ def test_pre_launch_set_up(self): > > self.set_up_nested_mounts() > > -self.launch_vm() > +self.launch_and_wait() > self.mount_in_guest() > self.check_in_guest() > > def test_post_launch_set_up(self): > self.set_up_shared_dir() > self.set_up_virtiofs() > -self.launch_vm() > +self.launch_and_wait() > > self.set_up_nested_mounts() > > @@ -252,7 +249,7 @@ def test_post_launch_set_up(self): > def test_post_mount_set_up(self): > self.set_up_shared_dir() > self.set_up_virtiofs() > -self.launch_vm() > +self.launch_and_wait() > self.mount_in_guest() > > self.set_up_nested_mounts() > @@ -265,7 +262,7 @@ def test_two_runs(self): > self.set_up_nested_mounts() > > self.set_up_virtiofs() > -self.launch_vm() > +self.launch_and_wait() > self.mount_in_guest() > self.check_in_guest() > > -- > 2.25.4 > > > -- Marc-André Lureau
[Bug 1921082] [NEW] VM crash when process broadcast MCE
Public bug reported: When i do memory SRAR test for VM, I meet the following issue: My VM has 16 vCPU, I will inject one UE error to memory which is accessed by VM, Then host MCE is raised and SIGBUS is send to VM, and qemu take control. Qemu will check the broadcast attribute by following cpu_x86_support_mca_broadcast(); Then Qemu may inject MCE to all vCPU, as vCPU is just one process for HOST, we can't guarantee all the vCPUs will enter MCE hander in 1S sync time, and the VM may panic. This issue will be easily fixed by expand monarch_timeout configuration, but the exact monarch_timeout can't be easily got, as it will depand on the num of vCPUs and current system schedule status. I am wondering why VM need broadcast attribute for MCE, When qeme process MCE event form host, it will always be signaled for one vCPU? If so, why does qemu need boradcast the MCE event to all vCPUs? Can weu just deliver LMCE to one specifc vCPU and make this behavior default? If anything wrong, Please point out. ** Affects: qemu Importance: Undecided Status: New -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1921082 Title: VM crash when process broadcast MCE Status in QEMU: New Bug description: When i do memory SRAR test for VM, I meet the following issue: My VM has 16 vCPU, I will inject one UE error to memory which is accessed by VM, Then host MCE is raised and SIGBUS is send to VM, and qemu take control. Qemu will check the broadcast attribute by following cpu_x86_support_mca_broadcast(); Then Qemu may inject MCE to all vCPU, as vCPU is just one process for HOST, we can't guarantee all the vCPUs will enter MCE hander in 1S sync time, and the VM may panic. This issue will be easily fixed by expand monarch_timeout configuration, but the exact monarch_timeout can't be easily got, as it will depand on the num of vCPUs and current system schedule status. I am wondering why VM need broadcast attribute for MCE, When qeme process MCE event form host, it will always be signaled for one vCPU? If so, why does qemu need boradcast the MCE event to all vCPUs? Can weu just deliver LMCE to one specifc vCPU and make this behavior default? If anything wrong, Please point out. To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1921082/+subscriptions
[PATCH] qom: Fix default values in help
Output of default values in device help is broken:
$ ./qemu-system-x86_64 -S -display none -monitor stdio
QEMU 5.2.50 monitor - type 'help' for more information
(qemu) device_add pvpanic,help
pvpanic options:
events= - (default: (null))
ioport=- (default: (null))
pvpanic[0]=>
The "(null)" is glibc printing a null pointer. Other systems crash
instead. Having a help request crash a running VM can really spoil
your day.
Root cause is a botched replacement of qstring_free() by
g_string_free(): to get the string back, we need to pass true to the
former, but false to the latter. Fix the argument.
Fixes: eab3a4678b07267c39e7290a6e9e7690b1d2a521
Reported-by: Thomas Huth
Signed-off-by: Markus Armbruster
---
qom/object_interfaces.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
index c3324b0f86..bd8a947a63 100644
--- a/qom/object_interfaces.c
+++ b/qom/object_interfaces.c
@@ -159,7 +159,7 @@ char *object_property_help(const char *name, const char
*type,
}
if (defval) {
g_autofree char *def_json = g_string_free(qobject_to_json(defval),
- true);
+ false);
g_string_append_printf(str, " (default: %s)", def_json);
}
--
2.26.3
[PATCH] iotests: Fix typo in iotest 051
There is an typo in iotest 051, correct it. Signed-off-by: Tao Xu --- tests/qemu-iotests/051| 2 +- tests/qemu-iotests/051.pc.out | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/qemu-iotests/051 b/tests/qemu-iotests/051 index f92161d8ef..1595babe82 100755 --- a/tests/qemu-iotests/051 +++ b/tests/qemu-iotests/051 @@ -209,7 +209,7 @@ case "$QEMU_DEFAULT_MACHINE" in # virtio-blk enables the iothread only when the driver initialises the # device, so a second virtio-blk device can't be added even with the # same iothread. virtio-scsi allows this. -run_qemu $iothread -device virtio-blk-pci,drive=disk,iohtread=iothread0,share-rw=on +run_qemu $iothread -device virtio-blk-pci,drive=disk,iothread=iothread0,share-rw=on run_qemu $iothread -device virtio-scsi,id=virtio-scsi1,iothread=thread0 -device scsi-hd,bus=virtio-scsi1.0,drive=disk,share-rw=on ;; *) diff --git a/tests/qemu-iotests/051.pc.out b/tests/qemu-iotests/051.pc.out index a28e3fc124..a43086bb41 100644 --- a/tests/qemu-iotests/051.pc.out +++ b/tests/qemu-iotests/051.pc.out @@ -183,9 +183,9 @@ Testing: -drive file=TEST_DIR/t.qcow2,if=none,node-name=disk -object iothread,id QEMU X.Y.Z monitor - type 'help' for more information (qemu) QEMU_PROG: -device scsi-hd,bus=virtio-scsi1.0,drive=disk,share-rw=on: Cannot change iothread of active block backend -Testing: -drive file=TEST_DIR/t.qcow2,if=none,node-name=disk -object iothread,id=thread0 -device virtio-scsi,iothread=thread0,id=virtio-scsi0 -device scsi-hd,bus=virtio-scsi0.0,drive=disk,share-rw=on -device virtio-blk-pci,drive=disk,iohtread=iothread0,share-rw=on +Testing: -drive file=TEST_DIR/t.qcow2,if=none,node-name=disk -object iothread,id=thread0 -device virtio-scsi,iothread=thread0,id=virtio-scsi0 -device scsi-hd,bus=virtio-scsi0.0,drive=disk,share-rw=on -device virtio-blk-pci,drive=disk,iothread=iothread0,share-rw=on QEMU X.Y.Z monitor - type 'help' for more information -(qemu) QEMU_PROG: -device virtio-blk-pci,drive=disk,iohtread=iothread0,share-rw=on: Cannot change iothread of active block backend +(qemu) QEMU_PROG: -device virtio-blk-pci,drive=disk,iothread=iothread0,share-rw=on: Cannot change iothread of active block backend Testing: -drive file=TEST_DIR/t.qcow2,if=none,node-name=disk -object iothread,id=thread0 -device virtio-scsi,iothread=thread0,id=virtio-scsi0 -device scsi-hd,bus=virtio-scsi0.0,drive=disk,share-rw=on -device virtio-scsi,id=virtio-scsi1,iothread=thread0 -device scsi-hd,bus=virtio-scsi1.0,drive=disk,share-rw=on QEMU X.Y.Z monitor - type 'help' for more information -- 2.25.1
Re: [PATCH v2 02/10] tests/acceptance/virtiofs_submounts.py: evaluate string not length
Hi Cleber,
On 3/23/21 11:15 PM, Cleber Rosa wrote:
> If the vmlinuz variable is set to anything that evaluates to True,
> then the respective arguments should be set. If the variable contains
> an empty string, than it will evaluate to False, and the extra
s/than/then
> arguments will not be set.>
> This keeps the same logic, but improves readability a bit.
>
> Signed-off-by: Cleber Rosa
> Reviewed-by: Beraldo Leal
> ---
> tests/acceptance/virtiofs_submounts.py | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tests/acceptance/virtiofs_submounts.py
> b/tests/acceptance/virtiofs_submounts.py
> index 5b74ce2929..ca64b76301 100644
> --- a/tests/acceptance/virtiofs_submounts.py
> +++ b/tests/acceptance/virtiofs_submounts.py
> @@ -251,7 +251,7 @@ def setUp(self):
>
> super(VirtiofsSubmountsTest, self).setUp(pubkey)
>
> -if len(vmlinuz) > 0:
> +if vmlinuz:
> self.vm.add_args('-kernel', vmlinuz,
> '-append', 'console=ttyS0 root=/dev/sda1')
>
>
Reviewed-by: Eric Auger
Thanks
Eric
Re: [PATCH v2 01/10] tests/acceptance/virtiofs_submounts.py: add missing accel tag
Hi Cleber, On 3/23/21 11:15 PM, Cleber Rosa wrote: > The tag is useful to select tests that depend/use a particular > feature. > > Signed-off-by: Cleber Rosa > Reviewed-by: Wainer dos Santos Moschetta > Reviewed-by: Willian Rampazzo > --- > tests/acceptance/virtiofs_submounts.py | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/tests/acceptance/virtiofs_submounts.py > b/tests/acceptance/virtiofs_submounts.py > index 46fa65392a..5b74ce2929 100644 > --- a/tests/acceptance/virtiofs_submounts.py > +++ b/tests/acceptance/virtiofs_submounts.py > @@ -70,6 +70,7 @@ def test_something_that_needs_cmd1_and_cmd2(self): > class VirtiofsSubmountsTest(LinuxTest): > """ > :avocado: tags=arch:x86_64 > +:avocado: tags=accel:kvm > """ > > def get_portfwd(self): > Reviewed-by: Eric Auger Thanks Eric
Re: [PATCH] qom: Fix default values in help
On Wed, Mar 24, 2021 at 12:41 PM Markus Armbruster
wrote:
> Output of default values in device help is broken:
>
> $ ./qemu-system-x86_64 -S -display none -monitor stdio
> QEMU 5.2.50 monitor - type 'help' for more information
> (qemu) device_add pvpanic,help
> pvpanic options:
> events= - (default: (null))
> ioport=- (default: (null))
> pvpanic[0]=>
>
> The "(null)" is glibc printing a null pointer. Other systems crash
> instead. Having a help request crash a running VM can really spoil
> your day.
>
> Root cause is a botched replacement of qstring_free() by
> g_string_free(): to get the string back, we need to pass true to the
> former, but false to the latter. Fix the argument.
>
> Fixes: eab3a4678b07267c39e7290a6e9e7690b1d2a521
> Reported-by: Thomas Huth
> Signed-off-by: Markus Armbruster
>
Reviewed-by: Marc-André Lureau
---
> qom/object_interfaces.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
> index c3324b0f86..bd8a947a63 100644
> --- a/qom/object_interfaces.c
> +++ b/qom/object_interfaces.c
> @@ -159,7 +159,7 @@ char *object_property_help(const char *name, const
> char *type,
> }
> if (defval) {
> g_autofree char *def_json = g_string_free(qobject_to_json(defval),
> - true);
> + false);
> g_string_append_printf(str, " (default: %s)", def_json);
> }
>
> --
> 2.26.3
>
>
Re: [PATCH v2 03/10] Python: add utility function for retrieving port redirection
Hi Cleber,
On 3/23/21 11:15 PM, Cleber Rosa wrote:
> Slightly different versions for the same utility code are currently
> present on different locations. This unifies them all, giving
> preference to the version from virtiofs_submounts.py, because of the
> last tweaks added to it.
>
> While at it, this adds a "qemu.utils" module to host the utility
> function and a test.
>
> Signed-off-by: Cleber Rosa
> Reviewed-by: Wainer dos Santos Moschetta
> ---
> python/qemu/utils.py | 35
> tests/acceptance/info_usernet.py | 29
> tests/acceptance/linux_ssh_mips_malta.py | 16 +--
> tests/acceptance/virtiofs_submounts.py | 21 --
> tests/vm/basevm.py | 7 ++---
> 5 files changed, 78 insertions(+), 30 deletions(-)
> create mode 100644 python/qemu/utils.py
> create mode 100644 tests/acceptance/info_usernet.py
>
> diff --git a/python/qemu/utils.py b/python/qemu/utils.py
> new file mode 100644
> index 00..89a246ab30
> --- /dev/null
> +++ b/python/qemu/utils.py
> @@ -0,0 +1,35 @@
> +"""
> +QEMU utility library
> +
> +This offers miscellaneous utility functions, which may not be easily
> +distinguishable or numerous to be in their own module.
> +"""
> +
> +# Copyright (C) 2021 Red Hat Inc.
> +#
> +# Authors:
> +# Cleber Rosa
> +#
> +# This work is licensed under the terms of the GNU GPL, version 2. See
> +# the COPYING file in the top-level directory.
> +#
> +
> +import re
> +from typing import Optional
> +> +
> +def get_info_usernet_hostfwd_port(info_usernet_output: str) -> Optional[int]:
> +"""
> +Returns the port given to the hostfwd parameter via info usernet
> +
> +:param info_usernet_output: output generated by hmp command "info
> usernet"
> +:param info_usernet_output: str
> +:return: the port number allocated by the hostfwd option
> +:rtype: int
> +"""
> +for line in info_usernet_output.split('\r\n'):
> +regex = r'TCP.HOST_FORWARD.*127\.0\.0\.1\s+(\d+)\s+10\.'
> +match = re.search(regex, line)
> +if match is not None:
> +return int(match[1])
> +return None
> diff --git a/tests/acceptance/info_usernet.py
> b/tests/acceptance/info_usernet.py
> new file mode 100644
> index 00..9c1fd903a0
> --- /dev/null
> +++ b/tests/acceptance/info_usernet.py
> @@ -0,0 +1,29 @@
> +# Test for the hmp command "info usernet"
> +#
> +# Copyright (c) 2021 Red Hat, Inc.
> +#
> +# Author:
> +# Cleber Rosa
> +#
> +# This work is licensed under the terms of the GNU GPL, version 2 or
> +# later. See the COPYING file in the top-level directory.
> +
> +from avocado_qemu import Test
> +
> +from qemu.utils import get_info_usernet_hostfwd_port
> +
> +
> +class InfoUsernet(Test):
> +
> +def test_hostfwd(self):
> +self.vm.add_args('-netdev', 'user,id=vnet,hostfwd=:127.0.0.1:0-:22')
> +self.vm.launch()
> +res = self.vm.command('human-monitor-command',
> + command_line='info usernet')
> +port = get_info_usernet_hostfwd_port(res)
> +self.assertIsNotNone(port,
> + ('"info usernet" output content does not seem
> to '
> + 'contain the redirected port'))
> +self.assertGreater(port, 0,
> + ('Found a redirected port that is not greater
> than'
> +' zero'))
> diff --git a/tests/acceptance/linux_ssh_mips_malta.py
> b/tests/acceptance/linux_ssh_mips_malta.py
> index 6dbd02d49d..052008f02d 100644
> --- a/tests/acceptance/linux_ssh_mips_malta.py
> +++ b/tests/acceptance/linux_ssh_mips_malta.py
> @@ -18,6 +18,8 @@
> from avocado.utils import archive
> from avocado.utils import ssh
>
> +from qemu.utils import get_info_usernet_hostfwd_port
> +
>
> class LinuxSSH(Test):
>
> @@ -70,18 +72,14 @@ def get_kernel_info(self, endianess, wordsize):
> def setUp(self):
> super(LinuxSSH, self).setUp()
>
> -def get_portfwd(self):
> +def ssh_connect(self, username, password):
> +self.ssh_logger = logging.getLogger('ssh')
> res = self.vm.command('human-monitor-command',
>command_line='info usernet')
> -line = res.split('\r\n')[2]
> -port = re.split(r'.*TCP.HOST_FORWARD.*127\.0\.0\.1 (\d+)\s+10\..*',
> -line)[1]
> +port = get_info_usernet_hostfwd_port(res)
> +if not port:
> +self.cancel("Failed to retrieve SSH port")
> self.log.debug("sshd listening on port:" + port)
> -return port
> -
> -def ssh_connect(self, username, password):
> -self.ssh_logger = logging.getLogger('ssh')
> -port = self.get_portfwd()
> self.ssh_session = ssh.Session(self.VM_IP, port=int(port),
> user=username, password=password)
> for i in range(10):
> diff -
Re: [PATCH] qom: Fix default values in help
On 24/03/2021 09.41, Markus Armbruster wrote:
Output of default values in device help is broken:
$ ./qemu-system-x86_64 -S -display none -monitor stdio
QEMU 5.2.50 monitor - type 'help' for more information
(qemu) device_add pvpanic,help
pvpanic options:
events= - (default: (null))
ioport=- (default: (null))
pvpanic[0]=>
The "(null)" is glibc printing a null pointer. Other systems crash
instead. Having a help request crash a running VM can really spoil
your day.
Root cause is a botched replacement of qstring_free() by
g_string_free(): to get the string back, we need to pass true to the
former, but false to the latter. Fix the argument.
Fixes: eab3a4678b07267c39e7290a6e9e7690b1d2a521
Reported-by: Thomas Huth
Signed-off-by: Markus Armbruster
---
qom/object_interfaces.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
index c3324b0f86..bd8a947a63 100644
--- a/qom/object_interfaces.c
+++ b/qom/object_interfaces.c
@@ -159,7 +159,7 @@ char *object_property_help(const char *name, const char
*type,
}
if (defval) {
g_autofree char *def_json = g_string_free(qobject_to_json(defval),
- true);
+ false);
g_string_append_printf(str, " (default: %s)", def_json);
}
Reviewed-by: Thomas Huth
Re: [PATCH v4 3/3] tests/qtest: Add test for Aspeed HACE
On 24/03/2021 08.21, Cédric Le Goater wrote: On 3/24/21 8:09 AM, Joel Stanley wrote: This adds a test for the Aspeed Hash and Crypto (HACE) engine. It tests the currently implemented behavior of the hash functionality. The tests are similar, but are cut/pasted instead of broken out into a common function so the assert machinery produces useful output when a test fails. Signed-off-by: Joel Stanley Reviewed-by: Cédric Le Goater Thomas, Can we keep your Acked-by ? Yes: Acked-by: Thomas Huth
Re: [PATCH v2 04/10] Acceptance Tests: move useful ssh methods to base class
Hi Cleber,
On 3/23/21 11:15 PM, Cleber Rosa wrote:
> Both the virtiofs submounts and the linux ssh mips malta tests
> contains useful methods related to ssh that deserve to be made
> available to other tests. Let's move them to the base LinuxTest
nit: strictly speaking they are moved to another class which is
inherited by LinuxTest, right?
> class.
>
> The method that helps with setting up an ssh connection will now
> support both key and password based authentication, defaulting to key
> based.
>
> Signed-off-by: Cleber Rosa
> Reviewed-by: Wainer dos Santos Moschetta
> Reviewed-by: Willian Rampazzo
> ---
> tests/acceptance/avocado_qemu/__init__.py | 48 ++-
> tests/acceptance/linux_ssh_mips_malta.py | 38 ++
> tests/acceptance/virtiofs_submounts.py| 37 -
> 3 files changed, 50 insertions(+), 73 deletions(-)
>
> diff --git a/tests/acceptance/avocado_qemu/__init__.py
> b/tests/acceptance/avocado_qemu/__init__.py
> index 83b1741ec8..67f75f66e5 100644
> --- a/tests/acceptance/avocado_qemu/__init__.py
> +++ b/tests/acceptance/avocado_qemu/__init__.py
> @@ -20,6 +20,7 @@
> from avocado.utils import cloudinit
> from avocado.utils import datadrainer
> from avocado.utils import network
> +from avocado.utils import ssh
> from avocado.utils import vmimage
> from avocado.utils.path import find_command
>
> @@ -43,6 +44,8 @@
> from qemu.accel import kvm_available
> from qemu.accel import tcg_available
> from qemu.machine import QEMUMachine
> +from qemu.utils import get_info_usernet_hostfwd_port
> +
>
> def is_readable_executable_file(path):
> return os.path.isfile(path) and os.access(path, os.R_OK | os.X_OK)
> @@ -253,7 +256,50 @@ def fetch_asset(self, name,
> cancel_on_missing=cancel_on_missing)
>
>
> -class LinuxTest(Test):
> +class LinuxSSHMixIn:
> +"""Contains utility methods for interacting with a guest via SSH."""
> +
> +def ssh_connect(self, username, credential, credential_is_key=True):
> +self.ssh_logger = logging.getLogger('ssh')
> +res = self.vm.command('human-monitor-command',
> + command_line='info usernet')
> +port = get_info_usernet_hostfwd_port(res)
> +self.assertIsNotNone(port)
> +self.assertGreater(port, 0)
> +self.log.debug('sshd listening on port: %d', port)
> +if credential_is_key:
> +self.ssh_session = ssh.Session('127.0.0.1', port=port,
> + user=username, key=credential)
> +else:
> +self.ssh_session = ssh.Session('127.0.0.1', port=port,
> + user=username,
> password=credential)
> +for i in range(10):
> +try:
> +self.ssh_session.connect()
> +return
> +except:
> +time.sleep(4)
> +pass
> +self.fail('ssh connection timeout')
> +
> +def ssh_command(self, command):
> +self.ssh_logger.info(command)
> +result = self.ssh_session.cmd(command)
> +stdout_lines = [line.rstrip() for line
> +in result.stdout_text.splitlines()]
> +for line in stdout_lines:
> +self.ssh_logger.info(line)
> +stderr_lines = [line.rstrip() for line
> +in result.stderr_text.splitlines()]
> +for line in stderr_lines:
> +self.ssh_logger.warning(line)
> +
> +self.assertEqual(result.exit_status, 0,
> + f'Guest command failed: {command}')
> +return stdout_lines, stderr_lines
> +
> +
> +class LinuxTest(Test, LinuxSSHMixIn):
> """Facilitates having a cloud-image Linux based available.
>
> For tests that indend to interact with guests, this is a better choice
> diff --git a/tests/acceptance/linux_ssh_mips_malta.py
> b/tests/acceptance/linux_ssh_mips_malta.py
> index 052008f02d..3f590a081f 100644
> --- a/tests/acceptance/linux_ssh_mips_malta.py
> +++ b/tests/acceptance/linux_ssh_mips_malta.py
> @@ -12,7 +12,7 @@
> import time
>
> from avocado import skipUnless
> -from avocado_qemu import Test
> +from avocado_qemu import Test, LinuxSSHMixIn
> from avocado_qemu import wait_for_console_pattern
> from avocado.utils import process
> from avocado.utils import archive
> @@ -21,7 +21,7 @@
> from qemu.utils import get_info_usernet_hostfwd_port
Can't you remove this now?
>
>
> -class LinuxSSH(Test):
> +class LinuxSSH(Test, LinuxSSHMixIn):
out of curiosity why can't it be migrated to a LinuxTest?
>
> timeout = 150 # Not for 'configure --enable-debug --enable-debug-tcg'
>
> @@ -72,41 +72,9 @@ def get_kernel_info(self, endianess, wordsize):
> def setUp(self):
> super(LinuxSSH, self).setUp()
>
> -def ssh_connect(self, username, password):
> -self.ssh_logger = logging.getLogger('ssh')
> -res = self.vm.command
Re: [PATCH v2 05/10] Acceptance Tests: add port redirection for ssh by default
Hi Cleber,
On 3/23/21 11:15 PM, Cleber Rosa wrote:
> For users of the LinuxTest class, let's set up the VM with the port
> redirection for SSH, instead of requiring each test to set the same
also sets the network device to virtio-net. This may be worth mentioning
here in the commit msg.
> arguments.
>
> Signed-off-by: Cleber Rosa
Reviewed-by: Eric Auger
Thanks
Eric
> ---
> tests/acceptance/avocado_qemu/__init__.py | 4 +++-
> tests/acceptance/virtiofs_submounts.py| 4
> 2 files changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/tests/acceptance/avocado_qemu/__init__.py
> b/tests/acceptance/avocado_qemu/__init__.py
> index 67f75f66e5..e75b002c70 100644
> --- a/tests/acceptance/avocado_qemu/__init__.py
> +++ b/tests/acceptance/avocado_qemu/__init__.py
> @@ -309,10 +309,12 @@ class LinuxTest(Test, LinuxSSHMixIn):
> timeout = 900
> chksum = None
>
> -def setUp(self, ssh_pubkey=None):
> +def setUp(self, ssh_pubkey=None, network_device_type='virtio-net'):
> super(LinuxTest, self).setUp()
> self.vm.add_args('-smp', '2')
> self.vm.add_args('-m', '1024')
> +self.vm.add_args('-netdev', 'user,id=vnet,hostfwd=:127.0.0.1:0-:22',
> + '-device', '%s,netdev=vnet' % network_device_type)
> self.set_up_boot()
> if ssh_pubkey is None:
> ssh_pubkey, self.ssh_key = self.set_up_existing_ssh_keys()
> diff --git a/tests/acceptance/virtiofs_submounts.py
> b/tests/acceptance/virtiofs_submounts.py
> index bed8ce44df..e10a935ac4 100644
> --- a/tests/acceptance/virtiofs_submounts.py
> +++ b/tests/acceptance/virtiofs_submounts.py
> @@ -207,10 +207,6 @@ def setUp(self):
> self.vm.add_args('-kernel', vmlinuz,
> '-append', 'console=ttyS0 root=/dev/sda1')
>
> -# Allow us to connect to SSH
> -self.vm.add_args('-netdev', 'user,id=vnet,hostfwd=:127.0.0.1:0-:22',
> - '-device', 'virtio-net,netdev=vnet')
> -
> self.require_accelerator("kvm")
> self.vm.add_args('-accel', 'kvm')
>
>
Re: Crashes with qemu-system-ppc64
On 24/03/21 00:35, Philippe Mathieu-Daudé wrote:
Hmmm does this assert() matches your comment?
-- >8 --
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index cefc5eaa0a9..41cbee77d14 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -1130,6 +1130,8 @@ Object *qdev_get_machine(void)
{
static Object *dev;
+assert(phase_check(PHASE_MACHINE_CREATED));
+
Very nice use of phase_check! Kudos.
Paolo
Re: [PATCH 1/1] linux-user/s390x: Apply h2g to address of sigreturn stub
On 24.03.21 09:51, Andreas Krebbel wrote:
The sigreturn SVC is put onto the stack by the emulation code. Hence
the address of it should not be subject to guest_base transformation
when fetching it.
The fix applies h2g to the address when writing it into the return
address register to nullify the transformation applied to it later.
Note: This only caused problems if Qemu has been built with
--disable-pie (as it is in distros nowadays). Otherwise guest_base
defaults to 0 hiding the actual problem.
Signed-off-by: Andreas Krebbel
---
linux-user/s390x/signal.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/linux-user/s390x/signal.c b/linux-user/s390x/signal.c
index ecfa2a14a9..1412376958 100644
--- a/linux-user/s390x/signal.c
+++ b/linux-user/s390x/signal.c
@@ -152,7 +152,7 @@ void setup_frame(int sig, struct target_sigaction *ka,
env->regs[14] = (unsigned long)
ka->sa_restorer | PSW_ADDR_AMODE;
} else {
-env->regs[14] = (frame_addr + offsetof(sigframe, retcode))
+env->regs[14] = h2g(frame_addr + offsetof(sigframe, retcode))
| PSW_ADDR_AMODE;
__put_user(S390_SYSCALL_OPCODE | TARGET_NR_sigreturn,
(uint16_t *)(frame->retcode));
@@ -213,7 +213,7 @@ void setup_rt_frame(int sig, struct target_sigaction *ka,
if (ka->sa_flags & TARGET_SA_RESTORER) {
env->regs[14] = (unsigned long) ka->sa_restorer | PSW_ADDR_AMODE;
} else {
-env->regs[14] = (unsigned long) frame->retcode | PSW_ADDR_AMODE;
+env->regs[14] = (unsigned long) h2g(frame->retcode) | PSW_ADDR_AMODE;
__put_user(S390_SYSCALL_OPCODE | TARGET_NR_rt_sigreturn,
(uint16_t *)(frame->retcode));
}
Sounds sane to me, although I am not an expert on that code :)
Acked-by: David Hildenbrand
--
Thanks,
David / dhildenb
Re: [PATCH v2 06/10] Acceptance Tests: make username/password configurable
On 3/23/21 11:15 PM, Cleber Rosa wrote: > This makes the username/password used for authentication configurable, > because some guest operating systems may have restrictions on accounts > to be used for logins, and it just makes it better documented. > > Signed-off-by: Cleber Rosa Reviewed-by: Eric Auger Eric > --- > tests/acceptance/avocado_qemu/__init__.py | 6 -- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/tests/acceptance/avocado_qemu/__init__.py > b/tests/acceptance/avocado_qemu/__init__.py > index e75b002c70..535f63a48d 100644 > --- a/tests/acceptance/avocado_qemu/__init__.py > +++ b/tests/acceptance/avocado_qemu/__init__.py > @@ -308,6 +308,8 @@ class LinuxTest(Test, LinuxSSHMixIn): > > timeout = 900 > chksum = None > +username = 'root' > +password = 'password' > > def setUp(self, ssh_pubkey=None, network_device_type='virtio-net'): > super(LinuxTest, self).setUp() > @@ -370,8 +372,8 @@ def prepare_cloudinit(self, ssh_pubkey=None): > with open(ssh_pubkey) as pubkey: > pubkey_content = pubkey.read() > cloudinit.iso(cloudinit_iso, self.name, > - username='root', > - password='password', > + username=self.username, > + password=self.password, ># QEMU's hard coded usermode router address >phone_home_host='10.0.2.2', >phone_home_port=self.phone_home_port, >
Re: [PATCH v2 07/10] Acceptance Tests: set up SSH connection by default after boot for LinuxTest
Hi Cleber,
On 3/23/21 11:15 PM, Cleber Rosa wrote:
> The LinuxTest specifically targets users that need to interact with Linux
> guests. So, it makes sense to give a connection by default, and avoid
> requiring it as boiler-plate code.
>
> Signed-off-by: Cleber Rosa
> ---
> tests/acceptance/avocado_qemu/__init__.py | 5 -
> tests/acceptance/virtiofs_submounts.py| 1 -
> 2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/tests/acceptance/avocado_qemu/__init__.py
> b/tests/acceptance/avocado_qemu/__init__.py
> index 535f63a48d..4960142bcc 100644
> --- a/tests/acceptance/avocado_qemu/__init__.py
> +++ b/tests/acceptance/avocado_qemu/__init__.py
> @@ -390,7 +390,7 @@ def set_up_cloudinit(self, ssh_pubkey=None):
> cloudinit_iso = self.prepare_cloudinit(ssh_pubkey)
> self.vm.add_args('-drive', 'file=%s,format=raw' % cloudinit_iso)
>
> -def launch_and_wait(self):
> +def launch_and_wait(self, set_up_ssh_connection=True):
> self.vm.set_console()
> self.vm.launch()
> console_drainer =
> datadrainer.LineLogger(self.vm.console_socket.fileno(),
> @@ -398,3 +398,6 @@ def launch_and_wait(self):
> console_drainer.start()
> self.log.info('VM launched, waiting for boot confirmation from
> guest')
> cloudinit.wait_for_phone_home(('0.0.0.0', self.phone_home_port),
> self.name)
> +if set_up_ssh_connection:
> +self.log.info('Setting up the SSH connection')
> +self.ssh_connect(self.username, self.ssh_key)
> diff --git a/tests/acceptance/virtiofs_submounts.py
> b/tests/acceptance/virtiofs_submounts.py
> index e10a935ac4..e019d3b896 100644
> --- a/tests/acceptance/virtiofs_submounts.py
> +++ b/tests/acceptance/virtiofs_submounts.py
> @@ -136,7 +136,6 @@ def set_up_virtiofs(self):
>
> def launch_vm(self):
> self.launch_and_wait()
> -self.ssh_connect('root', self.ssh_key)
>
> def set_up_nested_mounts(self):
> scratch_dir = os.path.join(self.shared_dir, 'scratch')
>
what about launch_and_wait calls in boot_linux.py. Don't you want to
force ssh connection off there?
Thanks
Eric
Re: [PATCH v2 08/10] tests/acceptance/virtiofs_submounts.py: remove launch_vm()
Hi, On 3/23/21 11:15 PM, Cleber Rosa wrote: > The LinuxTest class' launch_and_wait() method now behaves the same way > as this test's custom launch_vm(), so let's just use the upper layer > (common) method. > > Signed-off-by: Cleber Rosa Reviewed-by: Eric Auger Eric > --- > tests/acceptance/virtiofs_submounts.py | 13 + > 1 file changed, 5 insertions(+), 8 deletions(-) > > diff --git a/tests/acceptance/virtiofs_submounts.py > b/tests/acceptance/virtiofs_submounts.py > index e019d3b896..d77ee35674 100644 > --- a/tests/acceptance/virtiofs_submounts.py > +++ b/tests/acceptance/virtiofs_submounts.py > @@ -134,9 +134,6 @@ def set_up_virtiofs(self): > '-numa', > 'node,memdev=mem') > > -def launch_vm(self): > -self.launch_and_wait() > - > def set_up_nested_mounts(self): > scratch_dir = os.path.join(self.shared_dir, 'scratch') > try: > @@ -225,7 +222,7 @@ def test_pre_virtiofsd_set_up(self): > self.set_up_nested_mounts() > > self.set_up_virtiofs() > -self.launch_vm() > +self.launch_and_wait() > self.mount_in_guest() > self.check_in_guest() > > @@ -235,14 +232,14 @@ def test_pre_launch_set_up(self): > > self.set_up_nested_mounts() > > -self.launch_vm() > +self.launch_and_wait() > self.mount_in_guest() > self.check_in_guest() > > def test_post_launch_set_up(self): > self.set_up_shared_dir() > self.set_up_virtiofs() > -self.launch_vm() > +self.launch_and_wait() > > self.set_up_nested_mounts() > > @@ -252,7 +249,7 @@ def test_post_launch_set_up(self): > def test_post_mount_set_up(self): > self.set_up_shared_dir() > self.set_up_virtiofs() > -self.launch_vm() > +self.launch_and_wait() > self.mount_in_guest() > > self.set_up_nested_mounts() > @@ -265,7 +262,7 @@ def test_two_runs(self): > self.set_up_nested_mounts() > > self.set_up_virtiofs() > -self.launch_vm() > +self.launch_and_wait() > self.mount_in_guest() > self.check_in_guest() > >
Re: [PATCH v2 09/10] Acceptance Tests: add basic documentation on LinuxTest base class
Hi,
On 3/23/21 11:15 PM, Cleber Rosa wrote:
> Signed-off-by: Cleber Rosa
> Reviewed-by: Marc-André Lureau
> Reviewed-by: Willian Rampazzo
Reviewed-by: Eric Auger
Eric
> ---
> docs/devel/testing.rst | 25 +
> 1 file changed, 25 insertions(+)
>
> diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst
> index 1da4c4e4c4..ed2a06db28 100644
> --- a/docs/devel/testing.rst
> +++ b/docs/devel/testing.rst
> @@ -810,6 +810,31 @@ and hypothetical example follows:
> At test "tear down", ``avocado_qemu.Test`` handles all the QEMUMachines
> shutdown.
>
> +The ``avocado_qemu.LinuxTest`` base test class
> +~~
> +
> +The ``avocado_qemu.LinuxTest`` is further specialization of the
> +``avocado_qemu.Test`` class, so it contains all the characteristics of
> +the later plus some extra features.
> +
> +First of all, this base class is intended for tests that need to
> +interact with a fully booted and operational Linux guest. The most
> +basic example looks like this:
> +
> +.. code::
> +
> + from avocado_qemu import LinuxTest
> +
> +
> + class SomeTest(LinuxTest):
> +
> + def test(self):
> + self.launch_and_wait()
> + self.ssh_command('some_command_to_be_run_in_the_guest')
> +
> +Please refer to tests that use ``avocado_qemu.LinuxTest`` under
> +``tests/acceptance`` for more examples.
> +
> QEMUMachine
> ~~~
>
>
Re: [PATCH v2 10/10] Acceptance Tests: introduce CPU hotplug test
Hi Cleber,
On 3/23/21 11:15 PM, Cleber Rosa wrote:
> Even though there are qtest based tests for hotplugging CPUs (from
> which this test took some inspiration from), this one adds checks
> from a Linux guest point of view.
>
> It should also serve as an example for tests that follow a similar
> pattern and need to interact with QEMU (via qmp) and with the Linux
> guest via SSH.
>
> Signed-off-by: Cleber Rosa
> Reviewed-by: Marc-André Lureau
> Reviewed-by: Willian Rampazzo
Reviewed-by: Eric Auger
Eric
> ---
> tests/acceptance/hotplug_cpu.py | 37 +
> 1 file changed, 37 insertions(+)
> create mode 100644 tests/acceptance/hotplug_cpu.py
>
> diff --git a/tests/acceptance/hotplug_cpu.py b/tests/acceptance/hotplug_cpu.py
> new file mode 100644
> index 00..6374bf1b54
> --- /dev/null
> +++ b/tests/acceptance/hotplug_cpu.py
> @@ -0,0 +1,37 @@
> +# Functional test that hotplugs a CPU and checks it on a Linux guest
> +#
> +# Copyright (c) 2021 Red Hat, Inc.
> +#
> +# Author:
> +# Cleber Rosa
> +#
> +# This work is licensed under the terms of the GNU GPL, version 2 or
> +# later. See the COPYING file in the top-level directory.
> +
> +from avocado_qemu import LinuxTest
> +
> +
> +class HotPlugCPU(LinuxTest):
> +
> +def test(self):
> +"""
> +:avocado: tags=arch:x86_64
> +:avocado: tags=machine:q35
> +:avocado: tags=accel:kvm
> +"""
> +self.require_accelerator('kvm')
> +self.vm.add_args('-accel', 'kvm')
> +self.vm.add_args('-cpu', 'Haswell')
> +self.vm.add_args('-smp', '1,sockets=1,cores=2,threads=1,maxcpus=2')
> +self.launch_and_wait()
> +
> +self.ssh_command('test -e /sys/devices/system/cpu/cpu0')
> +with self.assertRaises(AssertionError):
> +self.ssh_command('test -e /sys/devices/system/cpu/cpu1')
> +
> +self.vm.command('device_add',
> +driver='Haswell-x86_64-cpu',
> +socket_id=0,
> +core_id=1,
> +thread_id=0)
> +self.ssh_command('test -e /sys/devices/system/cpu/cpu1')
>
[PATCH v2 0/2] vhost-user-blk: fix bug on device disconnection during initialization
v2: * split the initial patch into two (Raphael) * rename init to realized (Raphael) * remove unrelated comment (Raphael) When the vhost-user-blk device lose the connection to the daemon during the initialization phase it kills qemu because of the assert in the code. The series fixes the bug. 0001 is preparation for the fix 0002 fixes the bug, patch description has the full motivation for the series Denis Plotnikov (2): vhost-user-blk: use different event handlers on initialization vhost-user-blk: perform immediate cleanup if disconnect on initialization hw/block/vhost-user-blk.c | 79 --- 1 file changed, 48 insertions(+), 31 deletions(-) -- 2.25.1
[PATCH v2 2/2] vhost-user-blk: perform immediate cleanup if disconnect on initialization
Commit 4bcad76f4c39 ("vhost-user-blk: delay vhost_user_blk_disconnect")
introduced postponing vhost_dev cleanup aiming to eliminate qemu aborts
because of connection problems with vhost-blk daemon.
However, it introdues a new problem. Now, any communication errors
during execution of vhost_dev_init() called by vhost_user_blk_device_realize()
lead to qemu abort on assert in vhost_dev_get_config().
This happens because vhost_user_blk_disconnect() is postponed but
it should have dropped s->connected flag by the time
vhost_user_blk_device_realize() performs a new connection opening.
On the connection opening, vhost_dev initialization in
vhost_user_blk_connect() relies on s->connection flag and
if it's not dropped, it skips vhost_dev initialization and returns
with success. Then, vhost_user_blk_device_realize()'s execution flow
goes to vhost_dev_get_config() where it's aborted on the assert.
The connection/disconnection processing should happen
differently on initialization and operation of vhost-user-blk.
On initialization (in vhost_user_blk_device_realize()) we fully
control the initialization process. At that point, nobody can use the
device since it isn't initialized and we don't need to postpone any
cleanups, so we can do cleanup right away when there is communication
problems with the vhost-blk daemon.
On operation the disconnect may happen when the device is in use, so
the device users may want to use vhost_dev's data to do rollback before
vhost_dev is re-initialized (e.g. in vhost_dev_set_log()), so we
postpone the cleanup.
The patch splits those two cases, and performs the cleanup immediately on
initialization, and postpones cleanup when the device is initialized and
in use.
Signed-off-by: Denis Plotnikov
---
hw/block/vhost-user-blk.c | 48 +++
1 file changed, 24 insertions(+), 24 deletions(-)
diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index 1af95ec6aae7..4e215f71f152 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -402,38 +402,38 @@ static void vhost_user_blk_event(void *opaque,
QEMUChrEvent event,
break;
case CHR_EVENT_CLOSED:
/*
- * A close event may happen during a read/write, but vhost
- * code assumes the vhost_dev remains setup, so delay the
- * stop & clear. There are two possible paths to hit this
- * disconnect event:
- * 1. When VM is in the RUN_STATE_PRELAUNCH state. The
- * vhost_user_blk_device_realize() is a caller.
- * 2. In tha main loop phase after VM start.
- *
- * For p2 the disconnect event will be delayed. We can't
- * do the same for p1, because we are not running the loop
- * at this moment. So just skip this step and perform
- * disconnect in the caller function.
- *
- * TODO: maybe it is a good idea to make the same fix
- * for other vhost-user devices.
+ * Closing the connection should happen differently on device
+ * initialization and operation stages.
+ * On initalization, we want to re-start vhost_dev initialization
+ * from the very beginning right away when the connection is closed,
+ * so we clean up vhost_dev on each connection closing.
+ * On operation, we want to postpone vhost_dev cleanup to let the
+ * other code perform its own cleanup sequence using vhost_dev data
+ * (e.g. vhost_dev_set_log).
*/
if (realized) {
+/*
+ * A close event may happen during a read/write, but vhost
+ * code assumes the vhost_dev remains setup, so delay the
+ * stop & clear.
+ */
AioContext *ctx = qemu_get_current_aio_context();
qemu_chr_fe_set_handlers(&s->chardev, NULL, NULL, NULL, NULL,
NULL, NULL, false);
aio_bh_schedule_oneshot(ctx, vhost_user_blk_chr_closed_bh, opaque);
-}
-/*
- * Move vhost device to the stopped state. The vhost-user device
- * will be clean up and disconnected in BH. This can be useful in
- * the vhost migration code. If disconnect was caught there is an
- * option for the general vhost code to get the dev state without
- * knowing its type (in this case vhost-user).
- */
-s->dev.started = false;
+/*
+ * Move vhost device to the stopped state. The vhost-user device
+ * will be clean up and disconnected in BH. This can be useful in
+ * the vhost migration code. If disconnect was caught there is an
+ * option for the general vhost code to get the dev state without
+ * knowing its type (in this case vhost-user).
+ */
+s->dev.started = false;
+} else {
+vhost_user_blk_disconnect(dev);
+}
break;
case CHR_EVENT_BREAK:
ca
[PATCH v2 1/2] vhost-user-blk: use different event handlers on initialization
It is useful to use different connect/disconnect event handlers
on device initialization and operation as seen from the further
commit fixing a bug on device initialization.
The patch refactor the code to make use of them: we don't rely any
more on the VM state for choosing how to cleanup the device, instead
we explicitly use the proper event handler dependping on whether
the device has been initialized.
Signed-off-by: Denis Plotnikov
---
hw/block/vhost-user-blk.c | 31 ---
1 file changed, 24 insertions(+), 7 deletions(-)
diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index b870a50e6b20..1af95ec6aae7 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -362,7 +362,18 @@ static void vhost_user_blk_disconnect(DeviceState *dev)
vhost_dev_cleanup(&s->dev);
}
-static void vhost_user_blk_event(void *opaque, QEMUChrEvent event);
+static void vhost_user_blk_event(void *opaque, QEMUChrEvent event,
+ bool realized);
+
+static void vhost_user_blk_event_realize(void *opaque, QEMUChrEvent event)
+{
+vhost_user_blk_event(opaque, event, false);
+}
+
+static void vhost_user_blk_event_oper(void *opaque, QEMUChrEvent event)
+{
+vhost_user_blk_event(opaque, event, true);
+}
static void vhost_user_blk_chr_closed_bh(void *opaque)
{
@@ -371,11 +382,12 @@ static void vhost_user_blk_chr_closed_bh(void *opaque)
VHostUserBlk *s = VHOST_USER_BLK(vdev);
vhost_user_blk_disconnect(dev);
-qemu_chr_fe_set_handlers(&s->chardev, NULL, NULL, vhost_user_blk_event,
-NULL, opaque, NULL, true);
+qemu_chr_fe_set_handlers(&s->chardev, NULL, NULL,
+vhost_user_blk_event_oper, NULL, opaque, NULL, true);
}
-static void vhost_user_blk_event(void *opaque, QEMUChrEvent event)
+static void vhost_user_blk_event(void *opaque, QEMUChrEvent event,
+ bool realized)
{
DeviceState *dev = opaque;
VirtIODevice *vdev = VIRTIO_DEVICE(dev);
@@ -406,7 +418,7 @@ static void vhost_user_blk_event(void *opaque, QEMUChrEvent
event)
* TODO: maybe it is a good idea to make the same fix
* for other vhost-user devices.
*/
-if (runstate_is_running()) {
+if (realized) {
AioContext *ctx = qemu_get_current_aio_context();
qemu_chr_fe_set_handlers(&s->chardev, NULL, NULL, NULL, NULL,
@@ -473,8 +485,9 @@ static void vhost_user_blk_device_realize(DeviceState *dev,
Error **errp)
s->vhost_vqs = g_new0(struct vhost_virtqueue, s->num_queues);
s->connected = false;
-qemu_chr_fe_set_handlers(&s->chardev, NULL, NULL, vhost_user_blk_event,
- NULL, (void *)dev, NULL, true);
+qemu_chr_fe_set_handlers(&s->chardev, NULL, NULL,
+ vhost_user_blk_event_realize, NULL, (void *)dev,
+ NULL, true);
reconnect:
if (qemu_chr_fe_wait_connected(&s->chardev, &err) < 0) {
@@ -494,6 +507,10 @@ reconnect:
goto reconnect;
}
+/* we're fully initialized, now we can operate, so change the handler */
+qemu_chr_fe_set_handlers(&s->chardev, NULL, NULL,
+ vhost_user_blk_event_oper, NULL, (void *)dev,
+ NULL, true);
return;
virtio_err:
--
2.25.1
Re: [PATCH v4 1/3] hw: Model ASPEED's Hash and Crypto Engine
On 3/24/21 8:09 AM, Joel Stanley wrote:
> The HACE (Hash and Crypto Engine) is a device that offloads MD5, SHA1,
> SHA2, RSA and other cryptographic algorithms.
>
> This initial model implements a subset of the device's functionality;
> currently only direct access (non-scatter gather) hashing.
>
> Signed-off-by: Joel Stanley
> ---
> v3:
> - rebase on upstream to fix meson.build conflict
> v2:
> - reorder register defines
> - mask src/dest/len registers according to hardware
> v4:
> - Fix typos in comments
> - Remove sdram base address; new memory region fixes mean this is not
>required
> - Use PRIx64
> - Add Object Classes for soc familiy specific features
> - Convert big switch statement to a lookup in a struct
> ---
> include/hw/misc/aspeed_hace.h | 43
> hw/misc/aspeed_hace.c | 358 ++
> hw/misc/meson.build | 1 +
> 3 files changed, 402 insertions(+)
> create mode 100644 include/hw/misc/aspeed_hace.h
> create mode 100644 hw/misc/aspeed_hace.c
> +static int hash_algo_lookup(uint32_t mask)
> +{
> +int i;
> +
> +for (i = 0; i < ARRAY_SIZE(hash_algo_map); i++) {
> +if (mask == hash_algo_map[i].mask)
{
> +return hash_algo_map[i].algo;
}
> +}
> +
> +return -1;
> +}
> +
> +static int do_hash_operation(AspeedHACEState *s, int algo)
> +{
> +hwaddr src, len, dest;
> +uint8_t *digest_buf = NULL;
Eventually g_autofree,
> +size_t digest_len = 0;
> +char *src_buf;
> +int rc;
> +
> +src = s->regs[R_HASH_SRC];
> +len = s->regs[R_HASH_SRC_LEN];
> +dest = s->regs[R_HASH_DEST];
> +
> +src_buf = address_space_map(&s->dram_as, src, &len, false,
> +MEMTXATTRS_UNSPECIFIED);
> +if (!src_buf) {
> +qemu_log_mask(LOG_GUEST_ERROR, "%s: failed to map dram\n", __func__);
> +return -EACCES;
> +}
> +
> +rc = qcrypto_hash_bytes(algo, src_buf, len, &digest_buf, &digest_len,
> +&error_fatal);
> +if (rc < 0) {
> +qemu_log_mask(LOG_GUEST_ERROR, "%s: qcrypto failed\n", __func__);
> +return rc;
> +}
> +
> +rc = address_space_write(&s->dram_as, dest, MEMTXATTRS_UNSPECIFIED,
> + digest_buf, digest_len);
> +if (rc) {
> +qemu_log_mask(LOG_GUEST_ERROR,
> + "%s: address space write failed\n", __func__);
> +}
> +g_free(digest_buf);
removing g_free().
> +
> +address_space_unmap(&s->dram_as, src_buf, len, false, len);
> +
> +/*
> + * Set status bits to indicate completion. Testing shows hardware sets
> + * these irrespective of HASH_IRQ_EN.
> + */
> +s->regs[R_STATUS] |= HASH_IRQ;
> +
> +return 0;
> +}
Generic model LGTM.
Reviewed-by: Philippe Mathieu-Daudé
Re: [PATCH v2 2/7] ui/vdagent: core infrastructure
On Mon, Mar 22, 2021 at 11:27:17AM +0100, Gerd Hoffmann wrote:
> Hi,
>
> > > +if (vd->msgsize != msg->size + sizeof(*msg)) {
> > > +/* FIXME: handle parse messages splitted into multiple chunks */
> > > +fprintf(stderr, "%s: size mismatch: chunk %d, msg %d (+%zd)\n",
> > > +__func__, vd->msgsize, msg->size, sizeof(*msg));
> > >
> >
> > Not fixing? You handle chunking on sending but not on receiving?
>
> linux vdagent doesn't do chunking on send, so no need (and also no
> testcase) so far.
>
> Didn't try windows guests (yet), but that is next on my clipboard
> todo list.
Hmm, windows guest agent doesn't has VD_AGENT_CAP_CLIPBOARD_SELECTION,
so I have to deal with that.
Windows guests do actually send large messages in chunks, so I have
something to test with, good.
What are VD_AGENT_CAP_GUEST_LINEEND_LF + VD_AGENT_CAP_GUEST_LINEEND_CRLF
are good for btw? Are linefeeds converted automatically between dos and
unix conventions? If so, who is supposed to handle that?
take care,
Gerd
Re: [PATCH v4 2/3] aspeed: Integrate HACE
On 3/24/21 8:09 AM, Joel Stanley wrote: > Add the hash and crypto engine model to the Aspeed socs. > > Reviewed-by: Andrew Jeffery > Signed-off-by: Joel Stanley > --- > v3: Rebase on upstream > v4: Update integration for soc-specific hace objects > --- > docs/system/arm/aspeed.rst | 2 +- > include/hw/arm/aspeed_soc.h | 3 +++ > hw/arm/aspeed_ast2600.c | 15 +++ > hw/arm/aspeed_soc.c | 16 > 4 files changed, 35 insertions(+), 1 deletion(-) LGTM. Reviewed-by: Philippe Mathieu-Daudé
[Bug 1909247] Re: QEMU: use after free vulnerability in esp_do_dma() in hw/scsi/esp.c
Hello, Thank you all for your comments. Both patches (PJP/comment#8 - Mark/comment#9) seem to properly fix the UAF reported by Alexander in comment #6. However, I'm still able to reproduce the heap-bof from the above hw-esp-oob-issues.zip: ./x86_64-softmmu/qemu-system-x86_64 -m 512 \ -drive file=./atch2/hyfuzz.img,index=0,media=disk,format=raw \ -device am53c974,id=scsi -device scsi-hd,drive=SysDisk \ -drive id=SysDisk,if=none,file=./atch2/disk.img -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1909247 Title: QEMU: use after free vulnerability in esp_do_dma() in hw/scsi/esp.c Status in QEMU: New Bug description: A use-after-free vulnerability was found in the am53c974 SCSI host bus adapter emulation of QEMU. It could occur in the esp_do_dma() function in hw/scsi/esp.c while handling the 'Information Transfer' command (CMD_TI). A privileged guest user may abuse this flaw to crash the QEMU process on the host, resulting in a denial of service or potential code execution with the privileges of the QEMU process. This issue was reported by Cheolwoo Myung (Seoul National University). Original report: Using hypervisor fuzzer, hyfuzz, I found a use-after-free issue in am53c974 emulator of QEMU enabled ASan. It occurs while transferring information, as it does not check the buffer to be transferred. A malicious guest user/process could use this flaw to crash the QEMU process resulting in DoS scenario. To reproduce this issue, please run the QEMU with the following command line. # To enable ASan option, please set configuration with the following $ ./configure --target-list=i386-softmmu --disable-werror --enable-sanitizers $ make # To reproduce this issue, please run the QEMU process with the following command line $ ./qemu-system-i386 -m 512 -drive file=./hyfuzz.img,index=0,media=disk,format=raw \ -device am53c974,id=scsi -device scsi-hd,drive=SysDisk \ -drive id=SysDisk,if=none,file=./disk.img Please find attached the disk images to reproduce this issue. To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1909247/+subscriptions
Re: Crashes with qemu-system-ppc64
On 24/03/2021 00.35, Philippe Mathieu-Daudé wrote:
[...]
Hmmm does this assert() matches your comment?
-- >8 --
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index cefc5eaa0a9..41cbee77d14 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -1130,6 +1130,8 @@ Object *qdev_get_machine(void)
{
static Object *dev;
+assert(phase_check(PHASE_MACHINE_CREATED));
+
if (dev == NULL) {
dev = container_get(object_get_root(), "/machine");
}
Sounds like a good idea, but I think it should be sufficient to put it into
the if-statement instead.
Could you send it as a proper patch?
Thomas
[Bug 1910723] Re: NULL pointer dereference issues in am53c974 SCSI host bus adapter
I can confirm this is fixed now, thank you Mark. Patchset v2: https://lists.gnu.org/archive/html/qemu-devel/2021-03/msg06550.html -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1910723 Title: NULL pointer dereference issues in am53c974 SCSI host bus adapter Status in QEMU: New Bug description: Two NULL pointer dereference issues were found in the am53c974 SCSI host bus adapter emulation of QEMU. They could occur while handling the 'Information Transfer' command (CMD_TI) in function handle_ti() in hw/scsi/esp.c, and could be abused by a malicious guest to crash the QEMU process on the host resulting in a denial of service. Both issues were reported by Cheolwoo Myung (Seoul National University). To reproduce them, configure and run QEMU as follows. Please find attached the required disk images. $ ./configure --target-list=x86_64-softmmu --enable-kvm --enable-sanitizers $ make $ ./qemu-system-x86_64 -m 512 -drive file=./hyfuzz.img,index=0,media=disk,format=raw \ -device am53c974,id=scsi -device scsi-hd,drive=SysDisk \ -drive id=SysDisk,if=none,file=./disk.img Additional info: RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1909766 RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1909769 ASAN logs: ==672133== hw/scsi/scsi-bus.c:1385:12: runtime error: member access within null pointer of type 'struct SCSIRequest' AddressSanitizer:DEADLYSIGNAL = ==672133==ERROR: AddressSanitizer: SEGV on unknown address 0x0171 (pc 0x55bd63e20b85 bp 0x7f4b6fffdfa0 sp 0x7f4b6fffdf70 T7) ==672133==The signal is caused by a READ memory access. ==672133==Hint: address points to the zero page. #0 0x55bd63e20b85 in scsi_req_continue hw/scsi/scsi-bus.c:1385 #1 0x55bd63ab34fb in esp_do_dma hw/scsi/esp.c:453 #2 0x55bd63ab4b3c in handle_ti hw/scsi/esp.c:549 #3 0x55bd63ab72a9 in esp_reg_write hw/scsi/esp.c:691 #4 0x55bd63d7b5dd in esp_pci_io_write hw/scsi/esp-pci.c:206 #5 0x55bd645d55a3 in memory_region_write_accessor softmmu/memory.c:491 #6 0x55bd645d5a24 in access_with_adjusted_size softmmu/memory.c:552 #7 0x55bd645e2baa in memory_region_dispatch_write softmmu/memory.c:1501 #8 0x55bd646b75ff in flatview_write_continue softmmu/physmem.c:2759 #9 0x55bd646b79d1 in flatview_write softmmu/physmem.c:2799 #10 0x55bd646b8341 in address_space_write softmmu/physmem.c:2891 #11 0x55bd646b83f9 in address_space_rw softmmu/physmem.c:2901 #12 0x55bd648c4736 in kvm_handle_io accel/kvm/kvm-all.c:2285 #13 0x55bd648c69c8 in kvm_cpu_exec accel/kvm/kvm-all.c:2531 #14 0x55bd647b2413 in kvm_vcpu_thread_fn accel/kvm/kvm-cpus.c:49 #15 0x55bd64f560de in qemu_thread_start util/qemu-thread-posix.c:521 #16 0x7f4b981763f8 in start_thread (/lib64/libpthread.so.0+0x93f8) #17 0x7f4b980a3902 in __GI___clone (/lib64/libc.so.6+0x101902) --- ==672020== hw/scsi/esp.c:196:62: runtime error: member access within null pointer of type 'struct SCSIDevice' AddressSanitizer:DEADLYSIGNAL = ==672020==ERROR: AddressSanitizer: SEGV on unknown address 0x0098 (pc 0x559bc99946fd bp 0x7f08bd737fb0 sp 0x7f08bd737f70 T7) ==672020==The signal is caused by a READ memory access. ==672020==Hint: address points to the zero page. #0 0x559bc99946fd in do_busid_cmd hw/scsi/esp.c:196 #1 0x559bc9994e71 in do_cmd hw/scsi/esp.c:220 #2 0x559bc999ae81 in handle_ti hw/scsi/esp.c:555 #3 0x559bc999d2a9 in esp_reg_write hw/scsi/esp.c:691 #4 0x559bc9c615dd in esp_pci_io_write hw/scsi/esp-pci.c:206 #5 0x559bca4bb5a3 in memory_region_write_accessor softmmu/memory.c:491 #6 0x559bca4bba24 in access_with_adjusted_size softmmu/memory.c:552 #7 0x559bca4c8baa in memory_region_dispatch_write softmmu/memory.c:1501 #8 0x559bca59d5ff in flatview_write_continue softmmu/physmem.c:2759 #9 0x559bca59d9d1 in flatview_write softmmu/physmem.c:2799 #10 0x559bca59e341 in address_space_write softmmu/physmem.c:2891 #11 0x559bca59e3f9 in address_space_rw softmmu/physmem.c:2901 #12 0x559bca7aa736 in kvm_handle_io accel/kvm/kvm-all.c:2285 #13 0x559bca7ac9c8 in kvm_cpu_exec accel/kvm/kvm-all.c:2531 #14 0x559bca698413 in kvm_vcpu_thread_fn accel/kvm/kvm-cpus.c:49 #15 0x559bcae3c0de in qemu_thread_start util/
Re: [PATCH v2 7/7] ui/gtk: add clipboard support
Hi,
> > +if (gd->cbowner[s]) {
> > +/* ignore notifications about our own grabs */
> > +return;
> > +}
> > +
> > +
> > +switch (event->owner_change.reason) {
> > +case GDK_SETTING_ACTION_NEW:
> > +info = qemu_clipboard_info_new(&gd->cbpeer, s);
> > +if (gtk_clipboard_wait_is_text_available(clipboard)) {
> > +info->types[QEMU_CLIPBOARD_TYPE_TEXT].available = true;
> > +}
> >
>
> Same comment as v1:
> So after gtk_clipboard_set_text() the client side is actually taking
> the ownership away from the guest clipboard I presume. That might have some
> weird interaction issues. Hopefully the other side isn't playing the same
> game...
The cbowner check above should avoid that ...
> > +gd->gtkcb[QEMU_CLIPBOARD_SELECTION_CLIPBOARD] =
> > +gtk_clipboard_get(gdk_atom_intern("CLIPBOARD", FALSE));
>
> Why not use GDK_SELECTION_* ?
So I don't have to worry about converting GDK_SELECTION_* to
QEMU_CLIPBOARD_SELECTION_* ?
take care,
Gerd
Re: [PATCH v2 7/7] ui/gtk: add clipboard support
Hi
On Wed, Mar 24, 2021 at 2:16 PM Gerd Hoffmann wrote:
> Hi,
>
> > > +if (gd->cbowner[s]) {
> > > +/* ignore notifications about our own grabs */
> > > +return;
> > > +}
> > > +
> > > +
> > > +switch (event->owner_change.reason) {
> > > +case GDK_SETTING_ACTION_NEW:
> > > +info = qemu_clipboard_info_new(&gd->cbpeer, s);
> > > +if (gtk_clipboard_wait_is_text_available(clipboard)) {
> > > +info->types[QEMU_CLIPBOARD_TYPE_TEXT].available = true;
> > > +}
> > >
> >
> > Same comment as v1:
> > So after gtk_clipboard_set_text() the client side is actually taking
> > the ownership away from the guest clipboard I presume. That might have
> some
> > weird interaction issues. Hopefully the other side isn't playing the same
> > game...
>
> The cbowner check above should avoid that ...
>
I fail to see how that works, imagine the other end is the same code (qemu
in the guest), it will take clipboard ownership and it is in a endless
loop, isn't it?
>
> > > +gd->gtkcb[QEMU_CLIPBOARD_SELECTION_CLIPBOARD] =
> > > +gtk_clipboard_get(gdk_atom_intern("CLIPBOARD", FALSE));
> >
> > Why not use GDK_SELECTION_* ?
>
> So I don't have to worry about converting GDK_SELECTION_* to
> QEMU_CLIPBOARD_SELECTION_* ?
>
>
GDK_SELECTION* is gdk_atom_intern(*, FALSE)) afaik
--
Marc-André Lureau
Re: [PATCH 1/1] linux-user/s390x: Apply h2g to address of sigreturn stub
Le 24/03/2021 à 10:17, David Hildenbrand a écrit :
> On 24.03.21 09:51, Andreas Krebbel wrote:
>> The sigreturn SVC is put onto the stack by the emulation code. Hence
>> the address of it should not be subject to guest_base transformation
>> when fetching it.
>>
>> The fix applies h2g to the address when writing it into the return
>> address register to nullify the transformation applied to it later.
>>
>> Note: This only caused problems if Qemu has been built with
>> --disable-pie (as it is in distros nowadays). Otherwise guest_base
>> defaults to 0 hiding the actual problem.
>>
>> Signed-off-by: Andreas Krebbel
>> ---
>> linux-user/s390x/signal.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/linux-user/s390x/signal.c b/linux-user/s390x/signal.c
>> index ecfa2a14a9..1412376958 100644
>> --- a/linux-user/s390x/signal.c
>> +++ b/linux-user/s390x/signal.c
>> @@ -152,7 +152,7 @@ void setup_frame(int sig, struct target_sigaction *ka,
>> env->regs[14] = (unsigned long)
>> ka->sa_restorer | PSW_ADDR_AMODE;
>> } else {
>> - env->regs[14] = (frame_addr + offsetof(sigframe, retcode))
>> + env->regs[14] = h2g(frame_addr + offsetof(sigframe, retcode))
>> | PSW_ADDR_AMODE;
Well, it really doesn't sound good as frame_addr is a guest address (and
sa_restorer is too)
Where is the code that does the g2h() you want to nullify?
Thanks,
Laurent
Re: [PATCH v2 2/7] ui/vdagent: core infrastructure
On Wed, Mar 24, 2021 at 1:47 PM Gerd Hoffmann wrote:
> On Mon, Mar 22, 2021 at 11:27:17AM +0100, Gerd Hoffmann wrote:
> > Hi,
> >
> > > > +if (vd->msgsize != msg->size + sizeof(*msg)) {
> > > > +/* FIXME: handle parse messages splitted into multiple
> chunks */
> > > > +fprintf(stderr, "%s: size mismatch: chunk %d, msg %d
> (+%zd)\n",
> > > > +__func__, vd->msgsize, msg->size, sizeof(*msg));
> > > >
> > >
> > > Not fixing? You handle chunking on sending but not on receiving?
> >
> > linux vdagent doesn't do chunking on send, so no need (and also no
> > testcase) so far.
> >
> > Didn't try windows guests (yet), but that is next on my clipboard
> > todo list.
>
> Hmm, windows guest agent doesn't has VD_AGENT_CAP_CLIPBOARD_SELECTION,
> so I have to deal with that.
>
ok
> Windows guests do actually send large messages in chunks, so I have
> something to test with, good.
>
> What are VD_AGENT_CAP_GUEST_LINEEND_LF + VD_AGENT_CAP_GUEST_LINEEND_CRLF
> are good for btw? Are linefeeds converted automatically between dos and
> unix conventions? If so, who is supposed to handle that?
>
>
I think that means the guest agent uses a text clipboard with CRLF ending,
both ways. spice-gtk has dos2unix & unix2dos helpers to deal with that.
--
Marc-André Lureau
Re: Crashes with qemu-system-ppc64
On 24/03/2021 11.10, Thomas Huth wrote:
On 24/03/2021 00.35, Philippe Mathieu-Daudé wrote:
[...]
Hmmm does this assert() matches your comment?
-- >8 --
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index cefc5eaa0a9..41cbee77d14 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -1130,6 +1130,8 @@ Object *qdev_get_machine(void)
{
static Object *dev;
+ assert(phase_check(PHASE_MACHINE_CREATED));
+
if (dev == NULL) {
dev = container_get(object_get_root(), "/machine");
}
Sounds like a good idea, but I think it should be sufficient to put it into
the if-statement instead.
Scratch that. QEMU quickly asserts with that statement, which makes sense if
you consider that container_get also creates the container objects along the
way.
Thomas
[Bug 1921092] [NEW] qemu-system-arm multi core debug not working
Public bug reported: Working with Zephyr RTOS, running a multi core sample on mps2_an521 works fine. Both cpus start. Trying to debug with options -s -S the second core fails to boot. Posted with explanation also at: https://github.com/zephyrproject-rtos/zephyr/issues/33635 ** Affects: qemu Importance: Undecided Status: New ** Tags: arm debug -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1921092 Title: qemu-system-arm multi core debug not working Status in QEMU: New Bug description: Working with Zephyr RTOS, running a multi core sample on mps2_an521 works fine. Both cpus start. Trying to debug with options -s -S the second core fails to boot. Posted with explanation also at: https://github.com/zephyrproject-rtos/zephyr/issues/33635 To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1921092/+subscriptions
Re: [PATCH v2 05/10] Acceptance Tests: add port redirection for ssh by default
Hi Cleber,
On 3/23/21 11:15 PM, Cleber Rosa wrote:
> For users of the LinuxTest class, let's set up the VM with the port
> redirection for SSH, instead of requiring each test to set the same
> arguments.
>
> Signed-off-by: Cleber Rosa
> ---
> tests/acceptance/avocado_qemu/__init__.py | 4 +++-
> tests/acceptance/virtiofs_submounts.py| 4
> 2 files changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/tests/acceptance/avocado_qemu/__init__.py
> b/tests/acceptance/avocado_qemu/__init__.py
> index 67f75f66e5..e75b002c70 100644
> --- a/tests/acceptance/avocado_qemu/__init__.py
> +++ b/tests/acceptance/avocado_qemu/__init__.py
> @@ -309,10 +309,12 @@ class LinuxTest(Test, LinuxSSHMixIn):
> timeout = 900
> chksum = None
>
> -def setUp(self, ssh_pubkey=None):
> +def setUp(self, ssh_pubkey=None, network_device_type='virtio-net'):
I would be interested in testing with HW bridging too, when a bridge is
available. Do you think we could have the netdev configurable too?
This would be helpful to test vhost for instance.
With respect the network device type, I am currently working on SMMU
test and I need to call the parent setUp-) with the following args now:
super(IOMMU, self).setUp(pubkey,
'virtio-net-pci,iommu_platform=on,disable-modern=off,disable-legacy=on')
It works but I am not sure you had such kind of scenario in mind?
Thanks
Eric
> super(LinuxTest, self).setUp()
> self.vm.add_args('-smp', '2')
> self.vm.add_args('-m', '1024')
> +self.vm.add_args('-netdev', 'user,id=vnet,hostfwd=:127.0.0.1:0-:22',
> + '-device', '%s,netdev=vnet' % network_device_type)
> self.set_up_boot()
> if ssh_pubkey is None:
> ssh_pubkey, self.ssh_key = self.set_up_existing_ssh_keys()
> diff --git a/tests/acceptance/virtiofs_submounts.py
> b/tests/acceptance/virtiofs_submounts.py
> index bed8ce44df..e10a935ac4 100644
> --- a/tests/acceptance/virtiofs_submounts.py
> +++ b/tests/acceptance/virtiofs_submounts.py
> @@ -207,10 +207,6 @@ def setUp(self):
> self.vm.add_args('-kernel', vmlinuz,
> '-append', 'console=ttyS0 root=/dev/sda1')
>
> -# Allow us to connect to SSH
> -self.vm.add_args('-netdev', 'user,id=vnet,hostfwd=:127.0.0.1:0-:22',
> - '-device', 'virtio-net,netdev=vnet')
> -
> self.require_accelerator("kvm")
> self.vm.add_args('-accel', 'kvm')
>
>
Re: [PATCH V4 4/7] hmp-commands: Add new HMP command for COLO passthrough
* Zhang Chen (chen.zh...@intel.com) wrote:
> Add hmp_colo_passthrough_add and hmp_colo_passthrough_del make user
> can maintain COLO network passthrough list in human monitor.
>
> Signed-off-by: Zhang Chen
> ---
> hmp-commands.hx | 26 ++
> include/monitor/hmp.h | 2 ++
> monitor/hmp-cmds.c| 34 ++
> 3 files changed, 62 insertions(+)
>
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index d4001f9c5d..b67a5a04cb 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1335,6 +1335,32 @@ SRST
>Remove host network device.
> ERST
>
> +{
> +.name = "colo_passthrough_add",
> +.args_type =
> "protocol:s,id:s?,src_ip:s?,dst_ip:s?,src_port:i?,dst_port:i?",
> +.params = "protocol [id] [src_ip] [dst_ip] [src_port]
> [dst_port]",
That ordering is a bit unnatural; it's normal to keep ip and port
together; maybe this would be better as:
protocol:s,id:s,src:s?,dst:s?
then pass src and dst through inet_parse to get your hostname and port ?
Dave
> +.help = "Add network stream to colo passthrough list",
> +.cmd= hmp_colo_passthrough_add,
> +},
> +
> +SRST
> +``colo_passthrough_add``
> + Add network stream to colo passthrough list.
> +ERST
> +
> +{
> +.name = "colo_passthrough_del",
> +.args_type =
> "protocol:s,id:s?,src_ip:s?,dst_ip:s?,src_port:i?,dst_port:i?",
> +.params = "protocol [id] [src_ip] [dst_ip] [src_port]
> [dst_port]",
> +.help = "Delete network stream from colo passthrough list",
> +.cmd= hmp_colo_passthrough_del,
> +},
> +
> +SRST
> +``colo_passthrough_del``
> + Delete network stream from colo passthrough list.
> +ERST
> +
> {
> .name = "object_add",
> .args_type = "object:O",
> diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h
> index ed2913fd18..3c4943b09f 100644
> --- a/include/monitor/hmp.h
> +++ b/include/monitor/hmp.h
> @@ -81,6 +81,8 @@ void hmp_device_del(Monitor *mon, const QDict *qdict);
> void hmp_dump_guest_memory(Monitor *mon, const QDict *qdict);
> void hmp_netdev_add(Monitor *mon, const QDict *qdict);
> void hmp_netdev_del(Monitor *mon, const QDict *qdict);
> +void hmp_colo_passthrough_add(Monitor *mon, const QDict *qdict);
> +void hmp_colo_passthrough_del(Monitor *mon, const QDict *qdict);
> void hmp_getfd(Monitor *mon, const QDict *qdict);
> void hmp_closefd(Monitor *mon, const QDict *qdict);
> void hmp_sendkey(Monitor *mon, const QDict *qdict);
> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index 3c88a4faef..b57e3430ab 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -1668,6 +1668,40 @@ void hmp_netdev_del(Monitor *mon, const QDict *qdict)
> hmp_handle_error(mon, err);
> }
>
> +void hmp_colo_passthrough_add(Monitor *mon, const QDict *qdict)
> +{
> +const char *prot = qdict_get_str(qdict, "protocol");
> +L4_Connection *l4_conn = g_new0(L4_Connection, 1);
> +Error *err = NULL;
> +
> +l4_conn->id = g_strdup(qdict_get_try_str(qdict, "id"));
> +l4_conn->protocol = qapi_enum_parse(&IP_PROTOCOL_lookup, prot, -1, &err);
> +l4_conn->src_ip = g_strdup(qdict_get_try_str(qdict, "src_ip"));
> +l4_conn->dst_ip = g_strdup(qdict_get_try_str(qdict, "dst_ip"));
> +l4_conn->src_port = qdict_get_try_int(qdict, "src_port", 0);
> +l4_conn->dst_port = qdict_get_try_int(qdict, "dst_port", 0);
> +
> +qmp_colo_passthrough_add(l4_conn, &err);
> +hmp_handle_error(mon, err);
> +}
> +
> +void hmp_colo_passthrough_del(Monitor *mon, const QDict *qdict)
> +{
> +const char *prot = qdict_get_str(qdict, "protocol");
> +L4_Connection *l4_conn = g_new0(L4_Connection, 1);
> +Error *err = NULL;
> +
> +l4_conn->id = g_strdup(qdict_get_try_str(qdict, "id"));
> +l4_conn->protocol = qapi_enum_parse(&IP_PROTOCOL_lookup, prot, -1, &err);
> +l4_conn->src_ip = g_strdup(qdict_get_try_str(qdict, "src_ip"));
> +l4_conn->dst_ip = g_strdup(qdict_get_try_str(qdict, "dst_ip"));
> +l4_conn->src_port = qdict_get_try_int(qdict, "src_port", 0);
> +l4_conn->dst_port = qdict_get_try_int(qdict, "dst_port", 0);
> +
> +qmp_colo_passthrough_del(l4_conn, &err);
> +hmp_handle_error(mon, err);
> +}
> +
> void hmp_object_add(Monitor *mon, const QDict *qdict)
> {
> Error *err = NULL;
> --
> 2.25.1
>
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
[Bug 1921092] Re: qemu-system-arm multi core debug not working
** Description changed: Working with Zephyr RTOS, running a multi core sample on mps2_an521 works fine. Both cpus start. Trying to debug with options -s -S the second core fails to boot. Posted with explanation also at: https://github.com/zephyrproject-rtos/zephyr/issues/33635 + + only after closing the gdb server the second core boots and execution continues normally. + Is there no way to debug such a multi core environment? ** Description changed: Working with Zephyr RTOS, running a multi core sample on mps2_an521 works fine. Both cpus start. Trying to debug with options -s -S the second core fails to boot. Posted with explanation also at: https://github.com/zephyrproject-rtos/zephyr/issues/33635 - only after closing the gdb server the second core boots and execution continues normally. + only after quitting the debugging session the second core boots and execution continues normally. Is there no way to debug such a multi core environment? -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1921092 Title: qemu-system-arm multi core debug not working Status in QEMU: New Bug description: Working with Zephyr RTOS, running a multi core sample on mps2_an521 works fine. Both cpus start. Trying to debug with options -s -S the second core fails to boot. Posted with explanation also at: https://github.com/zephyrproject-rtos/zephyr/issues/33635 only after quitting the debugging session the second core boots and execution continues normally. Is there no way to debug such a multi core environment? To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1921092/+subscriptions
Re: [PATCH V4 5/7] net/colo-compare: Move data structure and define to .h file.
* Zhang Chen (chen.zh...@intel.com) wrote:
> Make other modules can reuse COLO code.
>
> Signed-off-by: Zhang Chen
> ---
> net/colo-compare.c | 106 -
> net/colo-compare.h | 106 +
> 2 files changed, 106 insertions(+), 106 deletions(-)
>
> diff --git a/net/colo-compare.h b/net/colo-compare.h
> index 22ddd512e2..2a9dcac0a7 100644
> --- a/net/colo-compare.h
> +++ b/net/colo-compare.h
> @@ -17,6 +17,112 @@
> #ifndef QEMU_COLO_COMPARE_H
> #define QEMU_COLO_COMPARE_H
>
> +#include "net/net.h"
> +#include "chardev/char-fe.h"
> +#include "migration/colo.h"
> +#include "migration/migration.h"
> +#include "sysemu/iothread.h"
> +#include "colo.h"
> +
> +#define TYPE_COLO_COMPARE "colo-compare"
> +typedef struct CompareState CompareState;
> +DECLARE_INSTANCE_CHECKER(CompareState, COLO_COMPARE,
> + TYPE_COLO_COMPARE)
> +
> +#define COMPARE_READ_LEN_MAX NET_BUFSIZE
> +#define MAX_QUEUE_SIZE 1024
> +#define COLO_COMPARE_FREE_PRIMARY 0x01
> +#define COLO_COMPARE_FREE_SECONDARY 0x02
> +
> +#define REGULAR_PACKET_CHECK_MS 1000
> +#define DEFAULT_TIME_OUT_MS 3000
> +
> +typedef struct SendCo {
> +Coroutine *co;
> +struct CompareState *s;
> +CharBackend *chr;
> +GQueue send_list;
> +bool notify_remote_frame;
> +bool done;
> +int ret;
> +} SendCo;
^
> +typedef struct SendEntry {
> +uint32_t size;
> +uint32_t vnet_hdr_len;
> +uint8_t *buf;
> +} SendEntry;
^
> +/*
> + * + CompareState ++
> + * | |
> + * +---+ +---+ +---+
> + * | conn list + - > conn + --- > conn + -- >
> ..
> + * +---+ +---+ +---+
> + * | | | | | |
> + * +---+ +---v+ +---v++---v+ +---v+
> + *|primary | |secondary|primary | |secondary
> + *|packet | |packet +|packet | |packet +
> + *++ ++++ ++
> + *| | | |
> + *+---v+ +---v++---v+ +---v+
> + *|primary | |secondary|primary | |secondary
> + *|packet | |packet +|packet | |packet +
> + *++ ++++ ++
> + *| | | |
> + *+---v+ +---v++---v+ +---v+
> + *|primary | |secondary|primary | |secondary
> + *|packet | |packet +|packet | |packet +
> + *++ ++++ ++
> + */
> +struct CompareState {
^
For a header, these are too generic names - they need to have Colo in
them; e.g. MAX_COLOQUEUE_SIZE and COLOSendEntry etc
Dave
> +Object parent;
> +
> +char *pri_indev;
> +char *sec_indev;
> +char *outdev;
> +char *notify_dev;
> +CharBackend chr_pri_in;
> +CharBackend chr_sec_in;
> +CharBackend chr_out;
> +CharBackend chr_notify_dev;
> +SocketReadState pri_rs;
> +SocketReadState sec_rs;
> +SocketReadState notify_rs;
> +SendCo out_sendco;
> +SendCo notify_sendco;
> +bool vnet_hdr;
> +uint64_t compare_timeout;
> +uint32_t expired_scan_cycle;
> +
> +/*
> + * Record the connection that through the NIC
> + * Element type: Connection
> + */
> +GQueue conn_list;
> +/* Record the connection without repetition */
> +GHashTable *connection_track_table;
> +
> +IOThread *iothread;
> +GMainContext *worker_context;
> +QEMUTimer *packet_check_timer;
> +
> +QEMUBH *event_bh;
> +enum colo_event event;
> +
> +QTAILQ_ENTRY(CompareState) next;
> +};
> +
> +typedef struct CompareClass {
> +ObjectClass parent_class;
> +} CompareClass;
> +
> +enum {
> +PRIMARY_IN = 0,
> +SECONDARY_IN,
> +};
> +
> void colo_notify_compares_event(void *opaque, int event, Error **errp);
> void colo_compare_register_notifier(Notifier *notify);
> void colo_compare_unregister_notifier(Notifier *notify);
> --
> 2.25.1
>
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
[PATCH] gitignore: Update with some filetypes
Update .gitignore to ignore .swp and .patch files. Signed-off-by: Viresh Kumar --- .gitignore | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.gitignore b/.gitignore index 75a4be07240f..eb2553026c5e 100644 --- a/.gitignore +++ b/.gitignore @@ -13,3 +13,5 @@ GTAGS *~ *.ast_raw *.depend_raw +*.swp +*.patch -- 2.25.0.rc1.19.g042ed3e048af
Re: [PATCH 0/5] virtio: Implement generic vhost-user-i2c backend
On 24-03-21, 00:42, no-re...@patchew.org wrote: > Patchew URL: > https://patchew.org/QEMU/cover.1616570702.git.viresh.ku...@linaro.org/ > > === TEST SCRIPT BEGIN === > #!/bin/bash > git rev-parse base > /dev/null || exit 0 > git config --local diff.renamelimit 0 > git config --local diff.renames True > git config --local diff.algorithm histogram > ./scripts/checkpatch.pl --mailback base.. I missed the fact that we have an implementation of checkpatch here as well. I have updated the patches already after running checkpatch at my end now, and will fix them while sending V2. -- viresh
Re: [PATCH v10 6/7] hw/pci-host: Add emulation of Marvell MV64361 PPC system controller
On Wed, 24 Mar 2021, David Gibson wrote:
On Tue, Mar 23, 2021 at 02:31:07PM +0100, BALATON Zoltan wrote:
On Tue, 23 Mar 2021, David Gibson wrote:
On Wed, Mar 17, 2021 at 02:17:51AM +0100, BALATON Zoltan wrote:
[snip]
+static void setup_mem_windows(MV64361State *s, uint32_t val)
+{
+MV64361PCIState *p;
+MemoryRegion *mr;
+uint32_t mask;
+int i;
+
+val &= 0x1f;
+for (mask = 1, i = 0; i < 21; i++, mask <<= 1) {
Having a loop, where nearly the entire body is a switch over the loop
variable seems a rather odd choice to me, compared to just unrolling
the whole thing. Or alternatively, maybe more can be be factored out
of the switch into common body code.
The loop is really over the bits in val that say which memory regions to
I see that, but it doesn't change the point.
enable so depending on this we need to touch different mem regions. For
selecting those memory regions and what to do with them a switch seems to be
obvious choice. I probably can't factor out anything as these lines in
switch cases are similar but all differ in some little details (like which
PCI bus, name of the region, etc.). Unrolling could be possible but would
just add lines between cases that's now in the loop head so I really don't
I see only 2 common lines, which basically balances about the case and
break lines in every switchcase.
Not sure what you mean by that. To me that means that these cannot be
factored out as they are in the middle so can't put them neither before
nor after the switch (without adding more local variables that would just
make the result longer than it is now).
Does that mean you prefer this to be unrolled then (written without a for
just repeating the if ((val & mask) != (s->base_addr_enable & mask)) at
every case)? That would also be longer by about 20 lines as we also log
regions that are not in the switch that would need their enable bits
checked separately if it's unrolled. Basically the trace is the common
part of the loop and handling of the individual regions are branching out
from the switch as they are different enough that factoring out the common
parts is not shorter than this way due to then needing local variables to
hold the different parts (name, address, size, base) the assigning of
which are as many or more lines than the map_pci_region call that could be
factored out that way.
I don't see why it is a problem to have a switch in a loop. If you insist
I can try to turn the switch into if else but I don't see how would that
be any better. Please explain a bit more what would you prefer here as I'm
not sure what to do with this. To me this is the shortest and simplest way
to write this.
Regards,
BALATON Zoltan
see why would that be better. Maybe renaming the loop variable from i to
something that makes the intent clearer could help but I don't know what
you'd like better. Can you suggest something?
+if ((val & mask) != (s->base_addr_enable & mask)) {
+trace_mv64361_region_enable(!(val & mask) ? "enable" : "disable",
i);
+switch (i) {
+/*
+ * 0-3 are SDRAM chip selects but we map all RAM directly
+ * 4-7 are device chip selects (not sure what those are)
+ * 8 is Boot device (ROM) chip select but we map that directly too
+ */
+case 9:
+p = &s->pci[0];
+mr = &s->cpu_win[i];
+unmap_region(mr);
+if (!(val & mask)) {
+map_pci_region(mr, &p->io, OBJECT(s), "pci0-io-win",
+ p->remap[4], (p->io_size + 1) << 16,
+ (p->io_base & 0xf) << 16);
+}
+break;
+case 10:
+p = &s->pci[0];
+mr = &s->cpu_win[i];
+unmap_region(mr);
+if (!(val & mask)) {
+map_pci_region(mr, &p->mem, OBJECT(s), "pci0-mem0-win",
+ p->remap[0], (p->mem_size[0] + 1) << 16,
+ (p->mem_base[0] & 0xf) << 16);
+}
+break;
+case 11:
+p = &s->pci[0];
+mr = &s->cpu_win[i];
+unmap_region(mr);
+if (!(val & mask)) {
+map_pci_region(mr, &p->mem, OBJECT(s), "pci0-mem1-win",
+ p->remap[1], (p->mem_size[1] + 1) << 16,
+ (p->mem_base[1] & 0xf) << 16);
+}
+break;
+case 12:
+p = &s->pci[0];
+mr = &s->cpu_win[i];
+unmap_region(mr);
+if (!(val & mask)) {
+map_pci_region(mr, &p->mem, OBJECT(s), "pci0-mem2-win",
+ p->remap[2], (p->mem_size[2] + 1) << 16,
+
qdev: Regarding lazy ISA bridge creation
Hi Cédric,
I'm trying to understand the comment you added in commit
3495b6b6101 ("ppc/pnv: add a ISA bus"):
/* let isa_bus_new() create its own bridge on SysBus otherwise
* devices specified on the command line won't find the bus and
* will fail to create.
*/
isa_bus = isa_bus_new(NULL, &lpc->isa_mem, &lpc->isa_io, &local_err);
Do you have an example so I can reproduce?
Thanks,
Phil.
Re: [PULL 0/5] Ui 20210323 patches
On Tue, 23 Mar 2021 at 15:39, Gerd Hoffmann wrote: > > The following changes since commit c95bd5ff1660883d15ad6e0005e4c8571604f51a: > > Merge remote-tracking branch 'remotes/philmd/tags/mips-fixes-20210322' into= > staging (2021-03-22 14:26:13 +) > > are available in the Git repository at: > > git://git.kraxel.org/qemu tags/ui-20210323-pull-request > > for you to fetch changes up to 40c503079ffcb5394be2b407e817de6104db9cfc: > > edid: prefer standard timings (2021-03-23 12:37:13 +0100) > > > fixes for 6.0 > > Applied, thanks. Please update the changelog at https://wiki.qemu.org/ChangeLog/6.0 for any user-visible changes. -- PMM
Re: [PATCH v10 7/7] hw/ppc: Add emulation of Genesi/bPlan Pegasos II
On Wed, 24 Mar 2021, David Gibson wrote: On Tue, Mar 23, 2021 at 02:01:27PM +0100, BALATON Zoltan wrote: On Tue, 23 Mar 2021, David Gibson wrote: On Wed, Mar 17, 2021 at 02:17:51AM +0100, BALATON Zoltan wrote: Add new machine called pegasos2 emulating the Genesi/bPlan Pegasos II, a PowerPC board based on the Marvell MV64361 system controller and the VIA VT8231 integrated south bridge/superio chips. It can run Linux, AmigaOS and a wide range of MorphOS versions. Currently a firmware ROM image is needed to boot and only MorphOS has a video driver to produce graphics output. Linux could work too but distros that supported this machine don't include usual video drivers so those only run with serial console for now. Signed-off-by: BALATON Zoltan Reviewed-by: Philippe Mathieu-Daudé --- MAINTAINERS | 10 ++ default-configs/devices/ppc-softmmu.mak | 2 + hw/ppc/Kconfig | 9 ++ hw/ppc/meson.build | 2 + hw/ppc/pegasos2.c | 144 5 files changed, 167 insertions(+) create mode 100644 hw/ppc/pegasos2.c diff --git a/MAINTAINERS b/MAINTAINERS index b6ab3d25a7..1c3c55ef09 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -1353,6 +1353,16 @@ F: pc-bios/canyonlands.dt[sb] F: pc-bios/u-boot-sam460ex-20100605.bin F: roms/u-boot-sam460ex +pegasos2 +M: BALATON Zoltan +R: David Gibson +L: qemu-...@nongnu.org +S: Maintained +F: hw/ppc/pegasos2.c +F: hw/pci-host/mv64361.c +F: hw/pci-host/mv643xx.h +F: include/hw/pci-host/mv64361.h Oh, sorry about the comment in the previous patch. RISC-V Machines --- OpenTitan diff --git a/default-configs/devices/ppc-softmmu.mak b/default-configs/devices/ppc-softmmu.mak index 61b78b844d..4535993d8d 100644 --- a/default-configs/devices/ppc-softmmu.mak +++ b/default-configs/devices/ppc-softmmu.mak @@ -14,5 +14,7 @@ CONFIG_SAM460EX=y CONFIG_MAC_OLDWORLD=y CONFIG_MAC_NEWWORLD=y +CONFIG_PEGASOS2=y I don't think we can have this default to enabled while it requires a non-free ROM to start. Not having it enabled though does not help those who might want to use it as they are not people who can compile their own QEMU but rely on binaries so adding it without also enabling it is like it wasn't there at all in practice. Not convinced, sorry. If it's not usable out of the box, having to build from source is kind of expected. Or you could convince someone I accept your point however there's a difference of only needing a ROM image to be able to use it from your distro's binary and having to rebuild it from source. So to me needing a ROM does not make it expected having to rebuild it. Needing to configure it some way would make that expected. (or do it yourself) to provide prebuild binaries for this purpose that have the right things enabled. There are people who provide binaries with patches for such purposes but that limits the availability of it compared to having it in all distro binaries without further effort. As I said I also plan to solve this eventually but I'd probably need VOF for that. Will that be merged at last? Other alternatives would be modifying SLOF, OpenBIOS or OpenFirmware or rewrite SmartFirmware to free it from its non-distributable parts but I think VOF would be a simpler way also avoiding adding another full OF implementation to QEMU that already has more than there should be. Regards, BALATON Zoltan I can attempt to make some guests boot without a ROM but since guests expect an OpenFirmware client interface, I'll need something to provide that. I'm waiting for VOF to be merged for this. Regards, BALATON Zoltan
Re: [PATCH 0/2] yank: Always link full yank code
On Tue, 23 Mar 2021 19:09:15 + Daniel P. Berrangé wrote: > On Tue, Mar 23, 2021 at 06:52:19PM +0100, Lukas Straub wrote: > > Hello Everyone, > > These patches remove yank's dependency on qiochannel and always link it in. > > Please Review. > > It would be useful if the cover letter or commit messages explained > to potential reviewers why this is being changed... The first patch > feels like a regression to me, without seeing an explanation why > it is desirable. Yes, sorry. There are two reasons for this patchset: -To exercise the full yank code (with checks for registering and unregistering of yank functions and instances) in existing tests and in the new yank test (https://lore.kernel.org/qemu-devel/cover.1616521487.git.lukasstra...@web.de/) -To replace "[PATCH] yank: Avoid linking into executables that don't want it" (https://lore.kernel.org/qemu-devel/20210316135907.3646901-1-arm...@redhat.com/) Now we always link in yank, but without pulling in other dependencies. Regards, Lukas Straub > > Regards, > Daniel -- pgpVK8yPpUWE3.pgp Description: OpenPGP digital signature
Re: [PATCH 0/2] yank: Always link full yank code
On Wed, Mar 24, 2021 at 12:22:42PM +0100, Lukas Straub wrote: > On Tue, 23 Mar 2021 19:09:15 + > Daniel P. Berrangé wrote: > > > On Tue, Mar 23, 2021 at 06:52:19PM +0100, Lukas Straub wrote: > > > Hello Everyone, > > > These patches remove yank's dependency on qiochannel and always link it > > > in. > > > Please Review. > > > > It would be useful if the cover letter or commit messages explained > > to potential reviewers why this is being changed... The first patch > > feels like a regression to me, without seeing an explanation why > > it is desirable. > > Yes, sorry. There are two reasons for this patchset: > -To exercise the full yank code (with checks for registering and unregistering > of yank functions and instances) in existing tests and in the new yank test > > (https://lore.kernel.org/qemu-devel/cover.1616521487.git.lukasstra...@web.de/) > -To replace "[PATCH] yank: Avoid linking into executables that don't want it" > > (https://lore.kernel.org/qemu-devel/20210316135907.3646901-1-arm...@redhat.com/) > Now we always link in yank, but without pulling in other dependencies. Conceptually, the real world usage of yank is in combination with parts of QEMU doing I/O, and so they will always have the "io" subsystem available. Thus it feels wrong to be refactoring this to avoid an "io" dependancy. I think this probably suggests that the yank code shouldn't be in libqemuutil.la, but instead part of the "io" subsystem to make the association/dependency explicit Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: qdev: Regarding lazy ISA bridge creation
On 3/24/21 12:13 PM, Philippe Mathieu-Daudé wrote:
> Hi Cédric,
>
> I'm trying to understand the comment you added in commit
> 3495b6b6101 ("ppc/pnv: add a ISA bus"):
>
> /* let isa_bus_new() create its own bridge on SysBus otherwise
> * devices specified on the command line won't find the bus and
> * will fail to create.
> */
> isa_bus = isa_bus_new(NULL, &lpc->isa_mem, &lpc->isa_io, &local_err);
>
> Do you have an example so I can reproduce?
I think this is related the IPMI BT device when using an external BMC :
https://qemu.readthedocs.io/en/latest/system/ppc/powernv.html
You could use an Aspeed machine for the remote end. But you need the
aspeed-6.0 branch for that because I did not send upstream the iBT model :
https://github.com/legoater/qemu/commit/4d2d9fe1211a25738ba5429d07a58c981845af92
C.
Re: [RFC v11 30/55] target/arm: wrap call to aarch64_sve_change_el in tcg_enabled()
Claudio Fontana writes:
> On 3/23/21 11:50 PM, Alex Bennée wrote:
>>
>> Claudio Fontana writes:
>>
>>> After this patch it is possible to build only kvm:
>>>
>>> ./configure --disable-tcg --enable-kvm
>
>
> It's possible to build, but tests will fail until all the test-related
> patches are applied.
So I think there has to be a change in ordering in the series so we
don't have differing failure modes as we enable. I'm not sure if that
means all tests need to be fixed before the first "--disable-tcg builds"
patch but I would expect at least a basic:
qemu-system-aarch64 -M virt,gic=host -cpu host -accel kvm -m 2048 -net none
-nographic -kernel ~/lsrc/linux.git/builds/arm64.virt/arch/arm64/boot/Image
-append "panic=-1"
--no-reboot
works - so at least we can track if any of the additional changes cause
regressions.
>>
>> FWIW at this point we get a different failure than later on:
>>
>> 21:10:25 [alex@aarch64-new:~/l/q/b/disable.tcg] (94e2abe0…)|… + make
>> check-qtest
>> GIT ui/keycodemapdb tests/fp/berkeley-testfloat-3
>> tests/fp/berkeley-softfloat-3 meson dtc capstone slirp
>> [1/19] Generating qemu-version.h with a meson_exe.py custom command
>> Running test qtest-aarch64/qom-test
>> qemu-system-aarch64: missing interface 'idau-interface' for object
>> 'machine'
>
> This one is broken by a recent commit in QEMU mainline, by removing the idau
> interface from KVM cpus.
>
> This is fixed by: Revert "target/arm: Restrict v8M IDAU to TCG" in the
> series.
The proper fix is probably to move the mps2tz machine type that brings
this in to TCG only.
Moving up the build chain to the revert I now get:
./qemu-system-aarch64 -M virt,gic=host -cpu host -accel kvm -m 2048 -net none
-nographic -kernel ~/lsrc/linux.git/builds/arm64.virt/arch/arm64/boot/Image
-append "panic=-1"
--no-reboot
qemu-system-aarch64: Property 'virt-6.0-machine.gic' not found
>
>> socket_accept failed: Resource temporarily unavailable
>> **
>> ERROR:../../tests/qtest/libqtest.c:319:qtest_init_without_qmp_handshake:
>> assertion failed: (s->fd >= 0 && s->qmp_fd >= 0)
>> ERROR qtest-aarch64/qom-test - Bail out!
>> ERROR:../../tests/qtest/libqtest.c:319:qtest_init_without_qmp_handshake:
>> assertion failed: (s->fd >= 0 && s->qmp_fd >= 0)
>> make: *** [Makefile.mtest:24: run-test-1] Error 1
>>
>>
>>>
>>> Signed-off-by: Claudio Fontana
>>> ---
>>> target/arm/cpu-sysemu.c | 12 +++-
>>> 1 file changed, 7 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/target/arm/cpu-sysemu.c b/target/arm/cpu-sysemu.c
>>> index eb928832a9..05d6e79ad9 100644
>>> --- a/target/arm/cpu-sysemu.c
>>> +++ b/target/arm/cpu-sysemu.c
>>> @@ -820,11 +820,13 @@ static void arm_cpu_do_interrupt_aarch64(CPUState *cs)
>>> unsigned int cur_el = arm_current_el(env);
>>> int rt;
>>>
>>> -/*
>>> - * Note that new_el can never be 0. If cur_el is 0, then
>>> - * el0_a64 is is_a64(), else el0_a64 is ignored.
>>> - */
>>> -aarch64_sve_change_el(env, cur_el, new_el, is_a64(env));
>>> +if (tcg_enabled()) {
>>> +/*
>>> + * Note that new_el can never be 0. If cur_el is 0, then
>>> + * el0_a64 is is_a64(), else el0_a64 is ignored.
>>> + */
>>> +aarch64_sve_change_el(env, cur_el, new_el, is_a64(env));
>>> +}
>>>
>>> if (cur_el < new_el) {
>>> /* Entry vector offset depends on whether the implemented EL
>>
>>
--
Alex Bennée
Re: [RFC v11 30/55] target/arm: wrap call to aarch64_sve_change_el in tcg_enabled()
Alex Bennée writes: > Claudio Fontana writes: > >> On 3/23/21 11:50 PM, Alex Bennée wrote: > Moving up the build chain to the revert I now get: > > ./qemu-system-aarch64 -M virt,gic=host -cpu host -accel kvm -m 2048 > -net none -nographic -kernel > ~/lsrc/linux.git/builds/arm64.virt/arch/arm64/boot/Image -append > "panic=-1" > --no-reboot > qemu-system-aarch64: Property 'virt-6.0-machine.gic' not found PEBKAC: The proper command line is "-M virt,gic-version=host" -- Alex Bennée
Re: [PATCH 0/2] yank: Always link full yank code
On Wed, 24 Mar 2021 11:36:13 + Daniel P. Berrangé wrote: > On Wed, Mar 24, 2021 at 12:22:42PM +0100, Lukas Straub wrote: > > On Tue, 23 Mar 2021 19:09:15 + > > Daniel P. Berrangé wrote: > > > > > On Tue, Mar 23, 2021 at 06:52:19PM +0100, Lukas Straub wrote: > > > > Hello Everyone, > > > > These patches remove yank's dependency on qiochannel and always link it > > > > in. > > > > Please Review. > > > > > > It would be useful if the cover letter or commit messages explained > > > to potential reviewers why this is being changed... The first patch > > > feels like a regression to me, without seeing an explanation why > > > it is desirable. > > > > Yes, sorry. There are two reasons for this patchset: > > -To exercise the full yank code (with checks for registering and > > unregistering > > of yank functions and instances) in existing tests and in the new yank test > > > > (https://lore.kernel.org/qemu-devel/cover.1616521487.git.lukasstra...@web.de/) > > -To replace "[PATCH] yank: Avoid linking into executables that don't want > > it" > > > > (https://lore.kernel.org/qemu-devel/20210316135907.3646901-1-arm...@redhat.com/) > > Now we always link in yank, but without pulling in other dependencies. > > Conceptually, the real world usage of yank is in combination with parts > of QEMU doing I/O, and so they will always have the "io" subsystem > available. Thus it feels wrong to be refactoring this to avoid an > "io" dependancy. I think this probably suggests that the yank code > shouldn't be in libqemuutil.la, but instead part of the "io" subsystem > to make the association/dependency explicit Yes, makes sense. On the other hand I think that the yank functions should be separate from the yank core anyway. Regards, Lukas Straub > > Regards, > Daniel -- pgpCbLq_skgIe.pgp Description: OpenPGP digital signature
Re: [PATCH v4 2/6] block: Allow changing bs->file on reopen
On Thu 18 Mar 2021 03:25:07 PM CET, Vladimir Sementsov-Ogievskiy
wrote:
>> static int bdrv_reopen_prepare(BDRVReopenState *reopen_state,
>> BlockReopenQueue *queue,
>> - Transaction *set_backings_tran, Error
>> **errp);
>> + Transaction *tran, Error **errp);
>
> I'd not call it just "tran" to not interfere with transaction
> actions. Of course, reopen should be finally refactored to work
> cleanly on Transaction API, but that is not done yet. And here we pass
> a transaction pointer only to keep children modification.. So, let's
> make it change_child_tran, or something like this.
The name change looks good to me.
>> +} else if (bdrv_recurse_has_child(new_child_bs, bs)) {
>> +error_setg(errp, "Making '%s' a %s of '%s' would create a
>> cycle",
>> + str, parse_file ? "file" : "backing file",
>
> maybe s/"file"/"file child"/
Ok.
>> default:
>> -/* 'backing' does not allow any other data type */
>> +/* The options QDict has been flattened, so 'backing' and 'file'
>> + * do not allow any other data type here. */
>
> checkpatch should complain that you didn't fix style of the comment...
I actually don't like to use the proposed style for 2-line comments in
many cases. I think it makes sense for big comment blocks but adds noise
for shorter comments.
>> +} else {
>> +/*
>> + * Ensure that @bs can really handle backing files, because we are
>> + * about to give it one (or swap the existing one)
>> + */
>> +if (bs->drv->is_filter) {
>> +/* Filters always have a file or a backing child */
>
> Probably we can assert bs->backing, as otherwise backing option should
> be unsupported [preexisting, not about this patch]
Yes, I see that this was added in commit 1d42f48c3a, maybe Max has good
reasons to keep it this way?
>> if (bdrv_is_backing_chain_frozen(overlay_bs,
>> - child_bs(overlay_bs->backing),
>> errp))
>> + bdrv_filter_or_cow_bs(overlay_bs),
>> + errp))
>> {
>> return -EPERM;
>> }
I just realized that this part is probably not ok if you want to change
bs->file on a node that is not a filter, because this would check
bs->backing->frozen and not bs->file->frozen.
>> +if (parse_file) {
>> +/* Store the old file bs, we'll need to refresh its permissions
>> */
>> +reopen_state->old_file_bs = bs->file->bs;
>> +
>> +/* And finally replace the child */
>> +bdrv_replace_child(bs->file, new_child_bs, tran);
>
> I think that actually, we need also to update inherits_from and do
> refresh_limits like in bdrv_set_backing_noperm().
Yes, I think you're right.
> Probably, bdrv_replace_child should do it. Probably not (there are
> still a lot of things to refactor in block.c :)..
>
> Hm. Also, using blockdev-reopen probably means that we are in a
>blockdev word, so we should not care about inherits_from here.
But with blockdev-reopen we do update inherits_from for backing files,
don't we?
> Also, you don't create reopen_state->replace_file_bs, like for
> backing.. On bdrv_reopen_comnmit replace_backing_bs is used to remove
> corresponding options.. Shouldn't we do the same with file options?
I think you're right.
>> -self.reopen(opts, {'file': 'not-found'}, "Cannot change the option
>> 'file'")
>> -self.reopen(opts, {'file': ''}, "Cannot change the option 'file'")
>> +self.reopen(opts, {'file': 'not-found'}, "Cannot find device='' nor
>> node-name='not-found'")
>
> Interesting that error-message say about device='', not 'not-found'...
That's because 'file' refers to a node name.
Thanks for reviewing,
Berto
[Bug 1920934] Re: Heap-use-after-free in io_writex / cputlb.c results in Linux kernel crashes
This suggests that the rcu_read in iotlb_to_section is not playing well with one of the g_renew calls in softmmu/physmem.c. Not sure which, since the sanitizer dump above doesn't trace back beyond glib itself. -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1920934 Title: Heap-use-after-free in io_writex / cputlb.c results in Linux kernel crashes Status in QEMU: New Bug description: qemu version: git 5ca634afcf83215a9a54ca6e66032325b5ffb5f6; 5.2.0 We've encountered that booting the Linux kernel in TCG mode, results in a racy heap-use-after-free. The bug can be detected by ASan [A], but in the majority of runs results in a crashing kernel [B]. To reproduce, the following command line was used: $> while ./qemu-system-x86_64 -no-reboot -smp 10 -m 2G -kernel arch/x86/boot/bzImage -nographic -append "oops=panic panic_on_warn=1 panic=1 kfence.sample_interval=1 nokaslr"; do sleep 0.5s; done The crashes in the kernel [B] appear to receive an interrupt in a code location where the instructions are periodically patched (via the jump_label infrastructure). [A]: = ==3552508==ERROR: AddressSanitizer: heap-use-after-free on address 0x6190007fef50 at pc 0x55885b0b4d1b bp 0x7f83baffb800 sp 0x7f83baffb7f8 READ of size 8 at 0x6190007fef50 thread T4 [4.616506][T1] pci :00:02.0: reg 0x18: [mem 0xfebf-0xfebf0fff] [4.670567][T1] pci :00:02.0: reg 0x30: [mem 0xfebe-0xfebe pref] [4.691345][T1] pci :00:03.0: [8086:100e] type 00 class 0x02 [4.701540][T1] pci :00:03.0: reg 0x10: [mem 0xfebc-0xfebd] [4.711076][T1] pci :00:03.0: reg 0x14: [io 0xc000-0xc03f] [4.746869][T1] pci :00:03.0: reg 0x30: [mem 0xfeb8-0xfebb pref] [4.813612][T1] ACPI: PCI Interrupt Link [LNKA] (IRQs 5 *10 11) #0 0x55885b0b4d1a in io_writex ../accel/tcg/cputlb.c:1408 #1 0x55885b0d3b9f in store_helper ../accel/tcg/cputlb.c:2444 #2 0x55885b0d3b9f in helper_le_stl_mmu ../accel/tcg/cputlb.c:2510 [4.820927][T1] ACPI: PCI Interrupt Link [LNKB] (IRQs 5 *10 11) #3 0x7f843cedf8dc ()
Re: [PATCH v2] x86/mce: fix wrong no-return-ip logic in do_machine_check()
On Wed, 24 Mar 2021 10:59:50 +0800
Aili Yao wrote:
> On Wed, 24 Feb 2021 10:39:21 +0800
> Aili Yao wrote:
>
> > On Tue, 23 Feb 2021 16:12:43 +
> > "Luck, Tony" wrote:
> >
> > > > What I think is qemu has not an easy to get the MCE signature from host
> > > > or currently no methods for this
> > > > So qemu treat all AR will be No RIPV, Do more is better than do less.
> > > >
> > >
> > > RIPV would be important in the guest in the case where the guest can fix
> > > the problem that caused
> > > the machine check and return to the failed instruction to continue.
> > >
> > > I think the only case where this happens is a fault in a read-only page
> > > mapped from a file (typically
> > > code page, but could be a data page). In this case memory-failure()
> > > unmaps the page with the posion
> > > but Linux can recover by reading data from the file into a new page.
> > >
> > > Other cases we send SIGBUS (so go to the signal handler instead of to the
> > > faulting instruction).
> > >
> > > So it would be good if the state of RIPV could be added to the signal
> > > state sent to qemu. If that
> > > isn't possible, then this full recovery case turns into another SIGBUS
> > > case.
> >
> > This KVM and VM case of failing recovery for SRAR is just one scenario I
> > think,
> > If Intel guarantee that when memory SRAR is triggered, RIPV will always be
> > set, then it's the job of qemu to
> > set the RIPV instead.
> > Or if When SRAR is triggered with RIPV cleared, the same issue will be true
> > for host.
> >
> > And I think it's better for VM to know the real RIPV value, It need more
> > work in qemu and kernel if possible.
> >
> > Thanks
> > Aili Yao
>
> ADD this topic to qemu list, this is really one bad issue.
>
> Issue report:
> when VM receive one SRAR memory failure from host, it all has RIPV cleared,
> and then vm process it and trigger one panic!
>
> Can any qemu maintainer fix this?
>
> Suggestion:
> qemu get the true value of RIPV from host, the inject it to VM accordingly.
Sorry for my previous description, I may not describe the issue clearly,
I found this issue when I do memory SRAR test for kvm virtual machine, the step
is:
1. Inject one uncorrectable error to one specific memory address A.
2. Then one user process in the VM access the address A and trigger a MCE
exception to host.
3. In do_machine_check() kernel will check the related register and do recovery
job from memory_failure();
4. Normally a BUS_MCEERR_AR SIGBUS is sent to the specifc core triggering this
error.
5. Qemu will take control, and will inject this event to VM, all infomation
qume can get currently is the Error code
BUS_MCEERR_AR and virtual address, in the qemu inject function:
if (code == BUS_MCEERR_AR) {
status |= MCI_STATUS_AR | 0x134;
mcg_status |= MCG_STATUS_EIPV;
} else {
status |= 0xc0;
mcg_status |= MCG_STATUS_RIPV;
}
For BUS_MCEERR_AR case, MCG_STATUS_RIPV will always be cleared.
6. Then in VM kernel, do_machine_check will got this:
if (!(m.mcgstatus & MCG_STATUS_RIPV))
kill_current_task = 1;
then go to force_sig(SIGBUS) without calling memory_failure();
so for now, the page is not marked hwpoison.
7 The VM kernel want to exit to user mode and then process the SIGBUS signal.
As SIGBUS is a fatal signal, the coredump related work will be called.
8. Then coredump will get the user space mapped memory dumped, include the
error page.
9. Then UE is triggered again, and qemu will take control again, then inject
this UE event to VM and
this time the error is triggered in kernel code, then VM panic.
I don't know how can this issue be fixed cleanly, maybe qemu developers may
help on this.
If qemu can fix this, that will be great!
--
Thanks!
Aili Yao
Re: [PATCH v2] x86/mce: fix wrong no-return-ip logic in do_machine_check()
On Wed, 24 Feb 2021 10:39:21 +0800 Aili Yao wrote: > On Tue, 23 Feb 2021 16:12:43 + > "Luck, Tony" wrote: > > > > What I think is qemu has not an easy to get the MCE signature from host > > > or currently no methods for this > > > So qemu treat all AR will be No RIPV, Do more is better than do less. > > > > RIPV would be important in the guest in the case where the guest can fix > > the problem that caused > > the machine check and return to the failed instruction to continue. > > > > I think the only case where this happens is a fault in a read-only page > > mapped from a file (typically > > code page, but could be a data page). In this case memory-failure() unmaps > > the page with the posion > > but Linux can recover by reading data from the file into a new page. > > > > Other cases we send SIGBUS (so go to the signal handler instead of to the > > faulting instruction). > > > > So it would be good if the state of RIPV could be added to the signal state > > sent to qemu. If that > > isn't possible, then this full recovery case turns into another SIGBUS > > case. > > This KVM and VM case of failing recovery for SRAR is just one scenario I > think, > If Intel guarantee that when memory SRAR is triggered, RIPV will always be > set, then it's the job of qemu to > set the RIPV instead. > Or if When SRAR is triggered with RIPV cleared, the same issue will be true > for host. > > And I think it's better for VM to know the real RIPV value, It need more work > in qemu and kernel if possible. > > Thanks > Aili Yao ADD this topic to qemu list, this is really one bad issue. Issue report: when VM receive one SRAR memory failure from host, it all has RIPV cleared, and then vm process it and trigger one panic! Can any qemu maintainer fix this? Suggestion: qemu get the true value of RIPV from host, the inject it to VM accordingly. Thanks Aili Yao!
Re: [PATCH v3 3/3] spapr: nvdimm: Enable sync-dax device property for nvdimm
On 3/24/21 8:39 AM, David Gibson wrote:
On Tue, Mar 23, 2021 at 09:47:55AM -0400, Shivaprasad G Bhat wrote:
The patch adds the 'sync-dax' property to the nvdimm device.
When the sync-dax is 'off', the device tree property
"hcall-flush-required" is added to the nvdimm node which makes the
guest to issue H_SCM_FLUSH hcalls to request for flushes explicitly.
This would be the default behaviour without sync-dax property set
for the nvdimm device.
The sync-dax="on" would mean the guest need not make flush requests
to the qemu. On previous machine versions the sync-dax is set to be
"on" by default using the hw_compat magic.
Signed-off-by: Shivaprasad G Bhat
---
hw/core/machine.c |1 +
hw/mem/nvdimm.c |1 +
hw/ppc/spapr_nvdimm.c | 17 +
include/hw/mem/nvdimm.h | 10 ++
include/hw/ppc/spapr.h |1 +
5 files changed, 30 insertions(+)
diff --git a/hw/core/machine.c b/hw/core/machine.c
index 257a664ea2..f843643574 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -41,6 +41,7 @@ GlobalProperty hw_compat_5_2[] = {
{ "PIIX4_PM", "smm-compat", "on"},
{ "virtio-blk-device", "report-discard-granularity", "off" },
{ "virtio-net-pci", "vectors", "3"},
+{ "nvdimm", "sync-dax", "on" },
};
const size_t hw_compat_5_2_len = G_N_ELEMENTS(hw_compat_5_2);
diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
index 7397b67156..8f0e29b191 100644
--- a/hw/mem/nvdimm.c
+++ b/hw/mem/nvdimm.c
@@ -229,6 +229,7 @@ static void nvdimm_write_label_data(NVDIMMDevice *nvdimm,
const void *buf,
static Property nvdimm_properties[] = {
DEFINE_PROP_BOOL(NVDIMM_UNARMED_PROP, NVDIMMDevice, unarmed, false),
+DEFINE_PROP_BOOL(NVDIMM_SYNC_DAX_PROP, NVDIMMDevice, sync_dax, false),
I'm a bit uncomfortable adding this base NVDIMM property without at
least some logic about how it's handled on non-PAPR platforms.
yes these should be specific to PAPR. These are there to handle
migration. with older guest. We can use the backing file to determine
synchronous dax support. if it is a file backed nvdimm on a fsdax mount
point, we can do synchronous dax. If it is one on a non dax file system
synchronous dax can be disabled.
DEFINE_PROP_END_OF_LIST(),
};
diff --git a/hw/ppc/spapr_nvdimm.c b/hw/ppc/spapr_nvdimm.c
index 883317c1ed..dd1c90251b 100644
--- a/hw/ppc/spapr_nvdimm.c
+++ b/hw/ppc/spapr_nvdimm.c
@@ -125,6 +125,9 @@ static int spapr_dt_nvdimm(SpaprMachineState *spapr, void
*fdt,
uint64_t lsize = nvdimm->label_size;
uint64_t size = object_property_get_int(OBJECT(nvdimm), PC_DIMM_SIZE_PROP,
NULL);
+bool sync_dax = object_property_get_bool(OBJECT(nvdimm),
+ NVDIMM_SYNC_DAX_PROP,
+ &error_abort);
drc = spapr_drc_by_id(TYPE_SPAPR_DRC_PMEM, slot);
g_assert(drc);
@@ -159,6 +162,11 @@ static int spapr_dt_nvdimm(SpaprMachineState *spapr, void
*fdt,
"operating-system")));
_FDT(fdt_setprop(fdt, child_offset, "ibm,cache-flush-required", NULL, 0));
+if (!sync_dax) {
+_FDT(fdt_setprop(fdt, child_offset, "ibm,hcall-flush-required",
+ NULL, 0));
+}
+
return child_offset;
}
@@ -567,10 +575,12 @@ static target_ulong h_scm_flush(PowerPCCPU *cpu, SpaprMachineState *spapr,
target_ulong opcode, target_ulong *args)
{
int ret;
+bool sync_dax;
uint32_t drc_index = args[0];
uint64_t continue_token = args[1];
SpaprDrc *drc = spapr_drc_by_index(drc_index);
PCDIMMDevice *dimm;
+NVDIMMDevice *nvdimm;
HostMemoryBackend *backend = NULL;
SpaprNVDIMMDeviceFlushState *state;
ThreadPool *pool = aio_get_thread_pool(qemu_get_aio_context());
@@ -580,6 +590,13 @@ static target_ulong h_scm_flush(PowerPCCPU *cpu,
SpaprMachineState *spapr,
return H_PARAMETER;
}
+nvdimm = NVDIMM(drc->dev);
+sync_dax = object_property_get_bool(OBJECT(nvdimm), NVDIMM_SYNC_DAX_PROP,
+&error_abort);
+if (sync_dax) {
+return H_UNSUPPORTED;
Do you want to return UNSUPPORTED here, or just H_SUCCESS, since the
flush should be a no-op in this case.
The reason to handle this as error is to indicate the OS that it is
using a wrong mechanism to flush.
+}
+
if (continue_token != 0) {
ret = spapr_nvdimm_get_flush_status(continue_token);
if (H_IS_LONG_BUSY(ret)) {
diff --git a/include/hw/mem/nvdimm.h b/include/hw/mem/nvdimm.h
index bcf62f825c..f82979cf2f 100644
--- a/include/hw/mem/nvdimm.h
+++ b/include/hw/mem/nvdimm.h
@@ -51,6 +51,7 @@ OBJECT_DECLARE_TYPE(NVDIMMDevice, NVDIMMClass, NVDIMM)
#define NVDIMM_LABEL_SIZE_PROP "label-size"
#define NVDIMM_UUID_PROP "uuid"
#define NVDIMM_UNARMED_PROP"unarmed"
+#de
Re: [PATCH 1/1] linux-user/s390x: Apply h2g to address of sigreturn stub
On 3/24/21 11:28 AM, Laurent Vivier wrote:
> Le 24/03/2021 à 10:17, David Hildenbrand a écrit :
>> On 24.03.21 09:51, Andreas Krebbel wrote:
>>> The sigreturn SVC is put onto the stack by the emulation code. Hence
>>> the address of it should not be subject to guest_base transformation
>>> when fetching it.
>>>
>>> The fix applies h2g to the address when writing it into the return
>>> address register to nullify the transformation applied to it later.
>>>
>>> Note: This only caused problems if Qemu has been built with
>>> --disable-pie (as it is in distros nowadays). Otherwise guest_base
>>> defaults to 0 hiding the actual problem.
>>>
>>> Signed-off-by: Andreas Krebbel
>>> ---
>>> linux-user/s390x/signal.c | 4 ++--
>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/linux-user/s390x/signal.c b/linux-user/s390x/signal.c
>>> index ecfa2a14a9..1412376958 100644
>>> --- a/linux-user/s390x/signal.c
>>> +++ b/linux-user/s390x/signal.c
>>> @@ -152,7 +152,7 @@ void setup_frame(int sig, struct target_sigaction *ka,
>>> env->regs[14] = (unsigned long)
>>> ka->sa_restorer | PSW_ADDR_AMODE;
>>> } else {
>>> - env->regs[14] = (frame_addr + offsetof(sigframe, retcode))
>>> + env->regs[14] = h2g(frame_addr + offsetof(sigframe, retcode))
>>> | PSW_ADDR_AMODE;
>
> Well, it really doesn't sound good as frame_addr is a guest address (and
> sa_restorer is too)
I would expect the sa_restorer address to actually point into the guest code
section.
>
> Where is the code that does the g2h() you want to nullify?
That's on the code path which usually fetches instructions from memory. In
cpu_lduw_code called via:
s390x_tr_translate_insn->translate_one->extract_insn->ld_code2->cpu_lduw_code
Btw. Power also uses h2g while setting up the trampoline address:
...
save_user_regs(env, mctx);
encode_trampoline(TARGET_NR_rt_sigreturn, trampptr);
/* The kernel checks for the presence of a VDSO here. We don't
emulate a vdso, so use a sigreturn system call. */
env->lr = (target_ulong) h2g(trampptr);
...
>
> Thanks,
> Laurent
>
Andreas
Issue Report: When VM has multiple vCPU and receive SIGBUS for SRAR, qemu will inject broadcast MCE to VM, VM may exceed 1S MCE sync time, and Panic.
Hi:
When i do memory SRAR test for VM, I meet the following issue:
My VM has 16 vCPU, I will inject one UE error to memory which is accessed by VM,
Then host MCE is raised and SIGBUS is send to VM, and qemu take control.
Qemu will check the broadcast attribute by following
cpu_x86_support_mca_broadcast();
Then Qemu may inject MCE to all vCPU, as vCPU is just one process for HOST, we
can't guarantee
all the vCPUs will enter MCE hander in 1S sync time, and the VM may panic.
This issue will be easily fixed by expand monarch_timeout configuration, but
the exact monarch_timeout can't be
easily got, as it will depand on the num of vCPUs and current system schedule
status.
I am wondering why VM need broadcast attribute for MCE, When qeme process MCE
event form host, it will always be
signaled for one vCPU? If so, why does qemu need boradcast the MCE event to all
vCPUs?
Can weu just deliver LMCE to one specifc vCPU and make this behavior default?
If anything wrong, Please point out.
Thanks!
Aili Yao
/* Broadcast MCA signal for processor version 06H_EH and above */
int cpu_x86_support_mca_broadcast(CPUX86State *env)
{
int family = 0;
int model = 0;
cpu_x86_version(env, &family, &model);
if ((family == 6 && model >= 14) || family > 6) {
return 1;
}
return 0;
}
Re: [PATCH v3 2/3] spapr: nvdimm: Implement H_SCM_FLUSH hcall
On 3/24/21 8:37 AM, David Gibson wrote: On Tue, Mar 23, 2021 at 09:47:38AM -0400, Shivaprasad G Bhat wrote: The patch adds support for the SCM flush hcall for the nvdimm devices. To be available for exploitation by guest through the next patch. The hcall expects the semantics such that the flush to return with H_BUSY when the operation is expected to take longer time along with a continue_token. The hcall to be called again providing the continue_token to get the status. So, all fresh requsts are put into a 'pending' list and flush worker is submitted to the thread pool. The thread pool completion callbacks move the requests to 'completed' list, which are cleaned up after reporting to guest in subsequent hcalls to get the status. The semantics makes it necessary to preserve the continue_tokens and their return status even across migrations. So, the pre_save handler for the device waits for the flush worker to complete and collects all the hcall states from 'completed' list. The necessary nvdimm flush specific vmstate structures are added to the spapr machine vmstate. Signed-off-by: Shivaprasad G Bhat An overal question: surely the same issue must arise on x86 with file-backed NVDIMMs. How do they handle this case? On x86 we have different ways nvdimm can be discovered. ACPI NFIT, e820 map and virtio_pmem. Among these virio_pmem always operated with synchronous dax disabled and both ACPI and e820 doesn't have the ability to differentiate support for synchronous dax. With that I would expect users to use virtio_pmem when using using file backed NVDIMMS -aneesh
[PATCH 1/1] linux-user/s390x: Apply h2g to address of sigreturn stub
The sigreturn SVC is put onto the stack by the emulation code. Hence
the address of it should not be subject to guest_base transformation
when fetching it.
The fix applies h2g to the address when writing it into the return
address register to nullify the transformation applied to it later.
Note: This only caused problems if Qemu has been built with
--disable-pie (as it is in distros nowadays). Otherwise guest_base
defaults to 0 hiding the actual problem.
Signed-off-by: Andreas Krebbel
---
linux-user/s390x/signal.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/linux-user/s390x/signal.c b/linux-user/s390x/signal.c
index ecfa2a14a9..1412376958 100644
--- a/linux-user/s390x/signal.c
+++ b/linux-user/s390x/signal.c
@@ -152,7 +152,7 @@ void setup_frame(int sig, struct target_sigaction *ka,
env->regs[14] = (unsigned long)
ka->sa_restorer | PSW_ADDR_AMODE;
} else {
-env->regs[14] = (frame_addr + offsetof(sigframe, retcode))
+env->regs[14] = h2g(frame_addr + offsetof(sigframe, retcode))
| PSW_ADDR_AMODE;
__put_user(S390_SYSCALL_OPCODE | TARGET_NR_sigreturn,
(uint16_t *)(frame->retcode));
@@ -213,7 +213,7 @@ void setup_rt_frame(int sig, struct target_sigaction *ka,
if (ka->sa_flags & TARGET_SA_RESTORER) {
env->regs[14] = (unsigned long) ka->sa_restorer | PSW_ADDR_AMODE;
} else {
-env->regs[14] = (unsigned long) frame->retcode | PSW_ADDR_AMODE;
+env->regs[14] = (unsigned long) h2g(frame->retcode) | PSW_ADDR_AMODE;
__put_user(S390_SYSCALL_OPCODE | TARGET_NR_rt_sigreturn,
(uint16_t *)(frame->retcode));
}
--
2.30.2
Re: [PATCH 2/8] virtiofds: Changed allocations of iovec to GLib's functions
On Tue, Mar 23, 2021 at 01:57:05PM +, Stefan Hajnoczi wrote: > On Fri, Mar 19, 2021 at 03:25:21PM +0200, Mahmoud Mandour wrote: > > @@ -629,9 +628,6 @@ int fuse_reply_ioctl_retry(fuse_req_t req, const struct > > iovec *in_iov, > > > > res = send_reply_iov(req, 0, iov, count); > > out: > > -free(in_fiov); > > -free(out_fiov); > > - > > return res; > > > > enomem: > > This hunk doesn't seem related to anything in this patch. Was it > included accidentally? Thanks for explaining that in_fiov/out_fiov are allocated by fuse_ioctl_iovec_copy() earlier in this function. I missed that. Please use Reply-All on mailing list emails so that the mailing like and all other CC email addresses are included in the discussion. Reviewed-by: Stefan Hajnoczi signature.asc Description: PGP signature
Re: [PATCH v2 7/7] ui/gtk: add clipboard support
Hi, > I fail to see how that works, imagine the other end is the same code (qemu > in the guest), it will take clipboard ownership and it is in a endless > loop, isn't it? Notifications on guest-triggered clipboard updates will not be sent back to the guest, exactly to avoid that kind of loop. See self_update checks in vdagent_clipboard_notify(). gtk and vnc notify callbacks have simliar checks for simliar reasons ;) take care, Gerd
Re: [PATCH 8/8] virtiofsd/fuse_virtio.c: Changed allocations of locals to GLib
On Wed, Mar 24, 2021 at 7:12 AM Mahmoud Mandour wrote: > > On Tue, Mar 23, 2021 at 4:15 PM Stefan Hajnoczi wrote: >> >> On Fri, Mar 19, 2021 at 03:25:27PM +0200, Mahmoud Mandour wrote: >> > @@ -588,7 +587,7 @@ out: >> > } >> > >> > pthread_mutex_destroy(&req->ch.lock); >> > -free(fbuf.mem); >> > +g_free(fbuf.mem); >> > free(req); >> >>^--- was FVRequest allocation changed in a previous patch? >> Maybe an earlier patch forgot to use g_free() here. >> >> Aside from this: >> >> Reviewed-by: Stefan Hajnoczi > > > I did not change the allocation of FVRequest. I believe that's why > this is left unchanged. Okay, I see it's allocated by libvhost-user and not directly by virtiofsd code. Thanks, Stefan
Re: [PATCH 1/1] linux-user/s390x: Apply h2g to address of sigreturn stub
Le 24/03/2021 à 12:26, Andreas Krebbel a écrit :
> On 3/24/21 11:28 AM, Laurent Vivier wrote:
>> Le 24/03/2021 à 10:17, David Hildenbrand a écrit :
>>> On 24.03.21 09:51, Andreas Krebbel wrote:
The sigreturn SVC is put onto the stack by the emulation code. Hence
the address of it should not be subject to guest_base transformation
when fetching it.
The fix applies h2g to the address when writing it into the return
address register to nullify the transformation applied to it later.
Note: This only caused problems if Qemu has been built with
--disable-pie (as it is in distros nowadays). Otherwise guest_base
defaults to 0 hiding the actual problem.
Signed-off-by: Andreas Krebbel
---
linux-user/s390x/signal.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/linux-user/s390x/signal.c b/linux-user/s390x/signal.c
index ecfa2a14a9..1412376958 100644
--- a/linux-user/s390x/signal.c
+++ b/linux-user/s390x/signal.c
@@ -152,7 +152,7 @@ void setup_frame(int sig, struct target_sigaction *ka,
env->regs[14] = (unsigned long)
ka->sa_restorer | PSW_ADDR_AMODE;
} else {
- env->regs[14] = (frame_addr + offsetof(sigframe, retcode))
+ env->regs[14] = h2g(frame_addr + offsetof(sigframe, retcode))
| PSW_ADDR_AMODE;
>>
>> Well, it really doesn't sound good as frame_addr is a guest address (and
>> sa_restorer is too)
>
> I would expect the sa_restorer address to actually point into the guest code
> section.
yes, it does. like frame_addr. The host address is frame, see:
if (!lock_user_struct(VERIFY_WRITE, frame, frame_addr, 0)) {
goto give_sigsegv;
}
So frame = g2h(frame_addr)
This line put the address of the next instruction to execute (guest address
space):
env->regs[14] = (frame_addr + offsetof(sigframe, retcode))
| PSW_ADDR_AMODE;
This line put at this address the NR_sigreturn syscall (but __put_user() uses
host address):
__put_user(S390_SYSCALL_OPCODE | TARGET_NR_sigreturn,
(uint16_t *)(frame->retcode));
In theory:
frame_addr + offsetof(sigframe, retcode) == h2g(frame->retcode)
So the next instruction executed after this function is the sigreturn() syscall.
I think the problem is elsewhere.
But I don't see what is the problem you are trying to solve.
>
>>
>> Where is the code that does the g2h() you want to nullify?
>
> That's on the code path which usually fetches instructions from memory. In
> cpu_lduw_code called via:
>
> s390x_tr_translate_insn->translate_one->extract_insn->ld_code2->cpu_lduw_code
cpu_lduw_code() takes a guest a address and needs to translate it to host
address. We need the g2h()
here because we have a guest address.
>
>
> Btw. Power also uses h2g while setting up the trampoline address:
>
> ...
> save_user_regs(env, mctx);
> encode_trampoline(TARGET_NR_rt_sigreturn, trampptr);
>
> /* The kernel checks for the presence of a VDSO here. We don't
>emulate a vdso, so use a sigreturn system call. */
> env->lr = (target_ulong) h2g(trampptr);
> ...
But here, it's correct because trampptr is an host address:
trampptr = &rt_sf->trampoline[0];
Thanks,
Laurent
[PATCH RFC v2 4/6] hw/arm/virt-acpi-build: Add explicit idmap info in IORT table
From: Xingang Wang
The idmap of smmuv3 and root complex covers the whole RID space for now,
this patch add explicit idmap info according to root bus number range.
This add smmuv3 idmap for certain bus which has enabled the iommu property.
Signed-off-by: Xingang Wang
Signed-off-by: Jiahui Cen
---
hw/arm/virt-acpi-build.c | 103 ++-
1 file changed, 81 insertions(+), 22 deletions(-)
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index f9c9df916c..e4a1ac3678 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -44,6 +44,7 @@
#include "hw/acpi/tpm.h"
#include "hw/pci/pcie_host.h"
#include "hw/pci/pci.h"
+#include "hw/pci/pci_bus.h"
#include "hw/pci-host/gpex.h"
#include "hw/arm/virt.h"
#include "hw/mem/nvdimm.h"
@@ -237,6 +238,41 @@ static void acpi_dsdt_add_tpm(Aml *scope, VirtMachineState
*vms)
aml_append(scope, dev);
}
+typedef
+struct AcpiIortMapping {
+AcpiIortIdMapping idmap;
+bool iommu;
+} AcpiIortMapping;
+
+/* For all PCI host bridges, walk and insert DMAR scope */
+static int
+iort_host_bridges(Object *obj, void *opaque)
+{
+GArray *map_blob = opaque;
+AcpiIortMapping map;
+AcpiIortIdMapping *idmap = &map.idmap;
+int bus_num, max_bus;
+
+if (object_dynamic_cast(obj, TYPE_PCI_HOST_BRIDGE)) {
+PCIBus *bus = PCI_HOST_BRIDGE(obj)->bus;
+
+if (bus) {
+bus_num = pci_bus_num(bus);
+max_bus = pci_root_bus_max_bus(bus);
+
+idmap->input_base = cpu_to_le32(bus_num << 8);
+idmap->id_count = cpu_to_le32((max_bus - bus_num + 1) << 8);
+idmap->output_base = cpu_to_le32(bus_num << 8);
+idmap->flags = cpu_to_le32(0);
+
+map.iommu = pci_root_bus_has_iommu(bus);
+g_array_append_val(map_blob, map);
+}
+}
+
+return 0;
+}
+
static void
build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
{
@@ -247,6 +283,21 @@ build_iort(GArray *table_data, BIOSLinker *linker,
VirtMachineState *vms)
AcpiIortSmmu3 *smmu;
size_t node_size, iort_node_offset, iort_length, smmu_offset = 0;
AcpiIortRC *rc;
+int smmu_mapping_count;
+GArray *map_blob = g_array_new(false, true, sizeof(AcpiIortMapping));
+AcpiIortMapping *map;
+
+/* pci_for_each_bus(vms->bus, insert_map, map_blob); */
+object_child_foreach_recursive(object_get_root(),
+ iort_host_bridges, map_blob);
+
+smmu_mapping_count = 0;
+for (int i = 0; i < map_blob->len; i++) {
+map = &g_array_index(map_blob, AcpiIortMapping, i);
+if (map->iommu) {
+smmu_mapping_count++;
+}
+}
iort = acpi_data_push(table_data, sizeof(*iort));
@@ -280,13 +331,13 @@ build_iort(GArray *table_data, BIOSLinker *linker,
VirtMachineState *vms)
/* SMMUv3 node */
smmu_offset = iort_node_offset + node_size;
-node_size = sizeof(*smmu) + sizeof(*idmap);
+node_size = sizeof(*smmu) + sizeof(*idmap) * smmu_mapping_count;
iort_length += node_size;
smmu = acpi_data_push(table_data, node_size);
smmu->type = ACPI_IORT_NODE_SMMU_V3;
smmu->length = cpu_to_le16(node_size);
-smmu->mapping_count = cpu_to_le32(1);
+smmu->mapping_count = cpu_to_le32(smmu_mapping_count);
smmu->mapping_offset = cpu_to_le32(sizeof(*smmu));
smmu->base_address = cpu_to_le64(vms->memmap[VIRT_SMMU].base);
smmu->flags = cpu_to_le32(ACPI_IORT_SMMU_V3_COHACC_OVERRIDE);
@@ -295,23 +346,28 @@ build_iort(GArray *table_data, BIOSLinker *linker,
VirtMachineState *vms)
smmu->gerr_gsiv = cpu_to_le32(irq + 2);
smmu->sync_gsiv = cpu_to_le32(irq + 3);
-/* Identity RID mapping covering the whole input RID range */
-idmap = &smmu->id_mapping_array[0];
-idmap->input_base = 0;
-idmap->id_count = cpu_to_le32(0x);
-idmap->output_base = 0;
-/* output IORT node is the ITS group node (the first node) */
-idmap->output_reference = cpu_to_le32(iort_node_offset);
+for (int i = 0, j = 0; i < map_blob->len; i++) {
+map = &g_array_index(map_blob, AcpiIortMapping, i);
+
+if (!map->iommu) {
+continue;
+}
+
+idmap = &smmu->id_mapping_array[j++];
+*idmap = map->idmap;
+/* output IORT node is the ITS group node (the first node) */
+idmap->output_reference = cpu_to_le32(iort_node_offset);
+}
}
/* Root Complex Node */
-node_size = sizeof(*rc) + sizeof(*idmap);
+node_size = sizeof(*rc) + sizeof(*idmap) * map_blob->len;
iort_length += node_size;
rc = acpi_data_push(table_data, node_size);
rc->type = ACPI_IORT_NODE_PCI_ROOT_COMPLEX;
rc->length = cpu_to_le16(node_size);
-rc->mapping_count = cpu_to_le32(1);
+rc->mapping_count = cpu_to_le32(map_
[PATCH RFC v2 3/6] hw/pci: Add pci_root_bus_max_bus
From: Xingang Wang
This helps to find max bus number of a root bus.
Signed-off-by: Xingang Wang
Signed-off-by: Jiahui Cen
---
hw/pci/pci.c | 34 ++
include/hw/pci/pci.h | 1 +
2 files changed, 35 insertions(+)
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index b82f39af10..ca26ab7750 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -537,6 +537,40 @@ int pci_bus_num(PCIBus *s)
return PCI_BUS_GET_CLASS(s)->bus_num(s);
}
+int pci_root_bus_max_bus(PCIBus *bus)
+{
+PCIHostState *host;
+PCIDevice *dev;
+int max_bus = 0;
+int type, devfn;
+uint8_t subordinate;
+
+if (!pci_bus_is_root(bus)) {
+return 0;
+}
+
+host = PCI_HOST_BRIDGE(BUS(bus)->parent);
+max_bus = pci_bus_num(host->bus);
+
+for (devfn = 0; devfn < ARRAY_SIZE(host->bus->devices); devfn++) {
+dev = host->bus->devices[devfn];
+
+if (!dev) {
+continue;
+}
+
+type = dev->config[PCI_HEADER_TYPE] & ~PCI_HEADER_TYPE_MULTI_FUNCTION;
+if (type == PCI_HEADER_TYPE_BRIDGE) {
+subordinate = dev->config[PCI_SUBORDINATE_BUS];
+if (subordinate > max_bus) {
+max_bus = subordinate;
+}
+}
+}
+
+return max_bus;
+}
+
int pci_bus_numa_node(PCIBus *bus)
{
return PCI_BUS_GET_CLASS(bus)->numa_node(bus);
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index 300332bc2c..96adf0220a 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -449,6 +449,7 @@ static inline PCIBus *pci_get_bus(const PCIDevice *dev)
return PCI_BUS(qdev_get_parent_bus(DEVICE(dev)));
}
int pci_bus_num(PCIBus *s);
+int pci_root_bus_max_bus(PCIBus *bus);
static inline int pci_dev_bus_num(const PCIDevice *dev)
{
return pci_bus_num(pci_get_bus(dev));
--
2.19.1
[PATCH RFC v2 2/6] hw/pci: Add iommu option for pci root bus
From: Xingang Wang
This add iommu option for pci root bus, including primary bus
and pxb root bus. The option is valid only if there is a virtual
iommu device.
Signed-off-by: Xingang Wang
Signed-off-by: Jiahui Cen
---
hw/arm/virt.c | 25 +
hw/i386/pc.c| 19 +++
hw/pci-bridge/pci_expander_bridge.c | 3 +++
hw/pci-host/q35.c | 1 +
include/hw/arm/virt.h | 1 +
include/hw/i386/pc.h| 1 +
6 files changed, 50 insertions(+)
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index aa2bbd14e0..446b3b867f 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1366,6 +1366,7 @@ static void create_pcie(VirtMachineState *vms)
}
pci = PCI_HOST_BRIDGE(dev);
+pci->iommu = vms->primary_bus_iommu;
vms->bus = pci->bus;
if (vms->bus) {
for (i = 0; i < nb_nics; i++) {
@@ -2319,6 +2320,20 @@ static void virt_set_iommu(Object *obj, const char
*value, Error **errp)
}
}
+static bool virt_get_primary_bus_iommu(Object *obj, Error **errp)
+{
+VirtMachineState *vms = VIRT_MACHINE(obj);
+
+return vms->primary_bus_iommu;
+}
+
+static void virt_set_primary_bus_iommu(Object *obj, bool value, Error **errp)
+{
+VirtMachineState *vms = VIRT_MACHINE(obj);
+
+vms->primary_bus_iommu = value;
+}
+
static CpuInstanceProperties
virt_cpu_index_to_props(MachineState *ms, unsigned cpu_index)
{
@@ -2652,6 +2667,13 @@ static void virt_machine_class_init(ObjectClass *oc,
void *data)
"Set the IOMMU type. "
"Valid values are none and smmuv3");
+object_class_property_add_bool(oc, "primary_bus_iommu",
+ virt_get_primary_bus_iommu,
+ virt_set_primary_bus_iommu);
+object_class_property_set_description(oc, "primary_bus_iommu",
+ "Set on/off to enable/disable "
+ "iommu for primary bus");
+
object_class_property_add_bool(oc, "ras", virt_get_ras,
virt_set_ras);
object_class_property_set_description(oc, "ras",
@@ -2719,6 +2741,9 @@ static void virt_instance_init(Object *obj)
/* Default disallows iommu instantiation */
vms->iommu = VIRT_IOMMU_NONE;
+/* The primary bus is attached to iommu by default */
+vms->primary_bus_iommu = true;
+
/* Default disallows RAS instantiation */
vms->ras = false;
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 410db9ef96..3426cd9c3f 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1531,6 +1531,21 @@ static void pc_machine_set_hpet(Object *obj, bool value,
Error **errp)
pcms->hpet_enabled = value;
}
+static bool pc_machine_get_primary_bus_iommu(Object *obj, Error **errp)
+{
+PCMachineState *pcms = PC_MACHINE(obj);
+
+return pcms->primary_bus_iommu;
+}
+
+static void pc_machine_set_primary_bus_iommu(Object *obj, bool value,
+ Error **errp)
+{
+PCMachineState *pcms = PC_MACHINE(obj);
+
+pcms->primary_bus_iommu = value;
+}
+
static void pc_machine_get_max_ram_below_4g(Object *obj, Visitor *v,
const char *name, void *opaque,
Error **errp)
@@ -1675,6 +1690,7 @@ static void pc_machine_initfn(Object *obj)
#ifdef CONFIG_HPET
pcms->hpet_enabled = true;
#endif
+pcms->primary_bus_iommu = true;
pc_system_flash_create(pcms);
pcms->pcspk = isa_new(TYPE_PC_SPEAKER);
@@ -1799,6 +1815,9 @@ static void pc_machine_class_init(ObjectClass *oc, void
*data)
object_class_property_add_bool(oc, "hpet",
pc_machine_get_hpet, pc_machine_set_hpet);
+object_class_property_add_bool(oc, "primary_bus_iommu",
+pc_machine_get_primary_bus_iommu, pc_machine_set_primary_bus_iommu);
+
object_class_property_add(oc, PC_MACHINE_MAX_FW_SIZE, "size",
pc_machine_get_max_fw_size, pc_machine_set_max_fw_size,
NULL, NULL);
diff --git a/hw/pci-bridge/pci_expander_bridge.c
b/hw/pci-bridge/pci_expander_bridge.c
index aedded1064..f1a0eadc03 100644
--- a/hw/pci-bridge/pci_expander_bridge.c
+++ b/hw/pci-bridge/pci_expander_bridge.c
@@ -57,6 +57,7 @@ struct PXBDev {
uint8_t bus_nr;
uint16_t numa_node;
+bool iommu;
};
static PXBDev *convert_to_pxb(PCIDevice *dev)
@@ -255,6 +256,7 @@ static void pxb_dev_realize_common(PCIDevice *dev, bool
pcie, Error **errp)
bus->map_irq = pxb_map_irq_fn;
PCI_HOST_BRIDGE(ds)->bus = bus;
+PCI_HOST_BRIDGE(ds)->iommu = pxb->iommu;
pxb_register_bus(dev, bus, &local_err);
if (local_err) {
@@ -301,6 +303,7 @@ static Property pxb_dev_properties[] = {
/* Note: 0 is not a legal PXB bus number. */
DEFINE_PROP_UINT8("bus_nr", PXBDev, bus_nr, 0),
DEFINE_PROP_UINT16("
[PATCH RFC v2 6/6] hw/i386/acpi-build: Add iommu filter in IVRS table
From: Xingang Wang
When building amd IVRS table, only devices attached to root bus with
IOMMU flag should be scanned.
Signed-off-by: Xingang Wang
Signed-off-by: Jiahui Cen
---
hw/i386/acpi-build.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 7729c96489..4e6b30d53a 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -2119,7 +2119,7 @@ ivrs_host_bridges(Object *obj, void *opaque)
if (object_dynamic_cast(obj, TYPE_PCI_HOST_BRIDGE)) {
PCIBus *bus = PCI_HOST_BRIDGE(obj)->bus;
-if (bus) {
+if (bus && pci_root_bus_has_iommu(bus)) {
pci_for_each_device(bus, pci_bus_num(bus), insert_ivhd, ivhd_blob);
}
}
--
2.19.1
[PATCH RFC v2 1/6] hw/pci/pci_host: Add iommu property for pci host
From: Xingang Wang
The pci host iommu property is useful to check whether
the iommu is enabled on the pci root bus.
Signed-off-by: Xingang Wang
Signed-off-by: Jiahui Cen
---
hw/pci/pci.c | 18 +-
hw/pci/pci_host.c | 2 ++
include/hw/pci/pci.h | 1 +
include/hw/pci/pci_host.h | 1 +
4 files changed, 21 insertions(+), 1 deletion(-)
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 0eadcdbc9e..b82f39af10 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -416,6 +416,22 @@ const char *pci_root_bus_path(PCIDevice *dev)
return rootbus->qbus.name;
}
+bool pci_root_bus_has_iommu(PCIBus *bus)
+{
+PCIBus *rootbus = bus;
+PCIHostState *host_bridge;
+
+if (!pci_bus_is_root(bus)) {
+rootbus = pci_device_root_bus(bus->parent_dev);
+}
+
+host_bridge = PCI_HOST_BRIDGE(rootbus->qbus.parent);
+
+assert(host_bridge->bus == rootbus);
+
+return host_bridge->iommu;
+}
+
static void pci_root_bus_init(PCIBus *bus, DeviceState *parent,
MemoryRegion *address_space_mem,
MemoryRegion *address_space_io,
@@ -2715,7 +2731,7 @@ AddressSpace *pci_device_iommu_address_space(PCIDevice
*dev)
iommu_bus = parent_bus;
}
-if (iommu_bus && iommu_bus->iommu_fn) {
+if (pci_root_bus_has_iommu(bus) && iommu_bus && iommu_bus->iommu_fn) {
return iommu_bus->iommu_fn(bus, iommu_bus->iommu_opaque, devfn);
}
return &address_space_memory;
diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c
index 8ca5fadcbd..92ce213b18 100644
--- a/hw/pci/pci_host.c
+++ b/hw/pci/pci_host.c
@@ -222,6 +222,8 @@ const VMStateDescription vmstate_pcihost = {
static Property pci_host_properties_common[] = {
DEFINE_PROP_BOOL("x-config-reg-migration-enabled", PCIHostState,
mig_enabled, true),
+DEFINE_PROP_BOOL("pci-host-iommu-enabled", PCIHostState,
+ iommu, true),
DEFINE_PROP_END_OF_LIST(),
};
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index 1bc231480f..300332bc2c 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -479,6 +479,7 @@ void pci_for_each_bus(PCIBus *bus,
PCIBus *pci_device_root_bus(const PCIDevice *d);
const char *pci_root_bus_path(PCIDevice *dev);
+bool pci_root_bus_has_iommu(PCIBus *bus);
PCIDevice *pci_find_device(PCIBus *bus, int bus_num, uint8_t devfn);
int pci_qdev_find_device(const char *id, PCIDevice **pdev);
void pci_bus_get_w64_range(PCIBus *bus, Range *range);
diff --git a/include/hw/pci/pci_host.h b/include/hw/pci/pci_host.h
index 52e038c019..64128e3a19 100644
--- a/include/hw/pci/pci_host.h
+++ b/include/hw/pci/pci_host.h
@@ -43,6 +43,7 @@ struct PCIHostState {
uint32_t config_reg;
bool mig_enabled;
PCIBus *bus;
+bool iommu;
QLIST_ENTRY(PCIHostState) next;
};
--
2.19.1
[PATCH RFC v2 5/6] hw/i386/acpi-build: Add explicit scope in DMAR table
From: Xingang Wang
In DMAR table, the drhd is set to cover all pci devices when intel_iommu
is on. This patch add explicit scope data, including only the pci devices
that go through iommu.
Signed-off-by: Xingang Wang
Signed-off-by: Jiahui Cen
---
hw/i386/acpi-build.c | 68 ++--
1 file changed, 66 insertions(+), 2 deletions(-)
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 442b4629a9..7729c96489 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1878,6 +1878,56 @@ build_srat(GArray *table_data, BIOSLinker *linker,
MachineState *machine)
pcms->oem_table_id);
}
+/*
+ * Insert DMAR scope for PCI bridges and endpoint devcie
+ */
+static void
+insert_scope(PCIBus *bus, PCIDevice *dev, void *opaque)
+{
+GArray *scope_blob = opaque;
+AcpiDmarDeviceScope *scope = NULL;
+
+if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_BRIDGE)) {
+/* Dmar Scope Type: 0x02 for PCI Bridge */
+build_append_int_noprefix(scope_blob, 0x02, 1);
+} else {
+/* Dmar Scope Type: 0x01 for PCI Endpoint Device */
+build_append_int_noprefix(scope_blob, 0x01, 1);
+}
+
+/* length */
+build_append_int_noprefix(scope_blob,
+ sizeof(*scope) + sizeof(scope->path[0]), 1);
+/* reserved */
+build_append_int_noprefix(scope_blob, 0, 2);
+/* enumeration_id */
+build_append_int_noprefix(scope_blob, 0, 1);
+/* bus */
+build_append_int_noprefix(scope_blob, pci_bus_num(bus), 1);
+/* device */
+build_append_int_noprefix(scope_blob, PCI_SLOT(dev->devfn), 1);
+/* function */
+build_append_int_noprefix(scope_blob, PCI_FUNC(dev->devfn), 1);
+}
+
+/* For all PCI host bridges, walk and insert DMAR scope */
+static int
+dmar_host_bridges(Object *obj, void *opaque)
+{
+GArray *scope_blob = opaque;
+
+if (object_dynamic_cast(obj, TYPE_PCI_HOST_BRIDGE)) {
+PCIBus *bus = PCI_HOST_BRIDGE(obj)->bus;
+
+if (bus && pci_root_bus_has_iommu(bus)) {
+pci_for_each_device(bus, pci_bus_num(bus), insert_scope,
+scope_blob);
+}
+}
+
+return 0;
+}
+
/*
* VT-d spec 8.1 DMA Remapping Reporting Structure
* (version Oct. 2014 or later)
@@ -1897,6 +1947,15 @@ build_dmar_q35(GArray *table_data, BIOSLinker *linker,
const char *oem_id,
/* Root complex IOAPIC use one path[0] only */
size_t ioapic_scope_size = sizeof(*scope) + sizeof(scope->path[0]);
IntelIOMMUState *intel_iommu = INTEL_IOMMU_DEVICE(iommu);
+GArray *scope_blob = g_array_new(false, true, 1);
+
+/*
+ * A PCI bus walk, for each PCI host bridge.
+ * Insert scope for each PCI bridge and endpoint device which
+ * is attached to a bus with iommu enabled.
+ */
+object_child_foreach_recursive(object_get_root(),
+ dmar_host_bridges, scope_blob);
assert(iommu);
if (x86_iommu_ir_supported(iommu)) {
@@ -1910,8 +1969,9 @@ build_dmar_q35(GArray *table_data, BIOSLinker *linker,
const char *oem_id,
/* DMAR Remapping Hardware Unit Definition structure */
drhd = acpi_data_push(table_data, sizeof(*drhd) + ioapic_scope_size);
drhd->type = cpu_to_le16(ACPI_DMAR_TYPE_HARDWARE_UNIT);
-drhd->length = cpu_to_le16(sizeof(*drhd) + ioapic_scope_size);
-drhd->flags = ACPI_DMAR_INCLUDE_PCI_ALL;
+drhd->length =
+cpu_to_le16(sizeof(*drhd) + ioapic_scope_size + scope_blob->len);
+drhd->flags = 0;/* Don't include all pci device */
drhd->pci_segment = cpu_to_le16(0);
drhd->address = cpu_to_le64(Q35_HOST_BRIDGE_IOMMU_ADDR);
@@ -1925,6 +1985,10 @@ build_dmar_q35(GArray *table_data, BIOSLinker *linker,
const char *oem_id,
scope->path[0].device = PCI_SLOT(Q35_PSEUDO_DEVFN_IOAPIC);
scope->path[0].function = PCI_FUNC(Q35_PSEUDO_DEVFN_IOAPIC);
+/* Add scope found above */
+g_array_append_vals(table_data, scope_blob->data, scope_blob->len);
+g_array_free(scope_blob, true);
+
if (iommu->dt_supported) {
atsr = acpi_data_push(table_data, sizeof(*atsr));
atsr->type = cpu_to_le16(ACPI_DMAR_TYPE_ATSR);
--
2.19.1
