Re: [PATCH 0/3] Disable vhost device IOTLB is IOMMU is not enabled
在 2021/8/4 上午11:48, Jason Wang 写道: Hi: We currently try to enable device IOTLB when iommu_platform is set. This may lead unnecessary trasnsactions between qemu and vhost when vIOMMU is not used (which is the typical case for the encrypted VM). So patch tries to use transport specific method to detect the enalbing of vIOMMU and enable the device IOTLB only if vIOMMU is enalbed. Please review. Hi Michael: Does this looks good for you? If yes, would you like to pick this series? Thanks Thanks Jason Wang (3): virtio-bus: introduce iommu_enabled() virtio-pci: implement iommu_enabled() vhost: correctly detect the enabling IOMMU hw/virtio/vhost.c | 2 +- hw/virtio/virtio-bus.c | 14 ++ hw/virtio/virtio-pci.c | 14 ++ include/hw/virtio/virtio-bus.h | 4 +++- 4 files changed, 32 insertions(+), 2 deletions(-)
[PATCH] virtio-net: fix use after unmap/free for sg
When mergeable buffer is enabled, we try to set the num_buffers after
the virtqueue elem has been unmapped. This will lead several issues,
E.g a use after free when the descriptor has an address which belongs
to the non direct access region. In this case we use bounce buffer
that is allocated during address_space_map() and freed during
address_space_unmap().
Fixing this by storing the elems temporarily in an array and delay the
unmap after we set the the num_buffers.
This addresses CVE-2021-3748.
Reported-by: Alexander Bulekov
Fixes: fbe78f4f55c6 ("virtio-net support")
Cc: qemu-sta...@nongnu.org
Signed-off-by: Jason Wang
---
hw/net/virtio-net.c | 39 ---
1 file changed, 32 insertions(+), 7 deletions(-)
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 16d20cdee5..f205331dcf 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -1746,10 +1746,13 @@ static ssize_t virtio_net_receive_rcu(NetClientState
*nc, const uint8_t *buf,
VirtIONet *n = qemu_get_nic_opaque(nc);
VirtIONetQueue *q = virtio_net_get_subqueue(nc);
VirtIODevice *vdev = VIRTIO_DEVICE(n);
+VirtQueueElement *elems[VIRTQUEUE_MAX_SIZE];
+size_t lens[VIRTQUEUE_MAX_SIZE];
struct iovec mhdr_sg[VIRTQUEUE_MAX_SIZE];
struct virtio_net_hdr_mrg_rxbuf mhdr;
unsigned mhdr_cnt = 0;
-size_t offset, i, guest_offset;
+size_t offset, i, guest_offset, j;
+ssize_t err;
if (!virtio_net_can_receive(nc)) {
return -1;
@@ -1780,6 +1783,12 @@ static ssize_t virtio_net_receive_rcu(NetClientState
*nc, const uint8_t *buf,
total = 0;
+if (i == VIRTQUEUE_MAX_SIZE) {
+virtio_error(vdev, "virtio-net unexpected long buffer chain");
+err = size;
+goto err;
+}
+
elem = virtqueue_pop(q->rx_vq, sizeof(VirtQueueElement));
if (!elem) {
if (i) {
@@ -1791,7 +1800,8 @@ static ssize_t virtio_net_receive_rcu(NetClientState *nc,
const uint8_t *buf,
n->guest_hdr_len, n->host_hdr_len,
vdev->guest_features);
}
-return -1;
+err = -1;
+goto err;
}
if (elem->in_num < 1) {
@@ -1799,7 +1809,8 @@ static ssize_t virtio_net_receive_rcu(NetClientState *nc,
const uint8_t *buf,
"virtio-net receive queue contains no in buffers");
virtqueue_detach_element(q->rx_vq, elem, 0);
g_free(elem);
-return -1;
+err = -1;
+goto err;
}
sg = elem->in_sg;
@@ -1836,12 +1847,13 @@ static ssize_t virtio_net_receive_rcu(NetClientState
*nc, const uint8_t *buf,
if (!n->mergeable_rx_bufs && offset < size) {
virtqueue_unpop(q->rx_vq, elem, total);
g_free(elem);
-return size;
+err = size;
+goto err;
}
-/* signal other side */
-virtqueue_fill(q->rx_vq, elem, total, i++);
-g_free(elem);
+elems[i] = elem;
+lens[i] = total;
+i++;
}
if (mhdr_cnt) {
@@ -1851,10 +1863,23 @@ static ssize_t virtio_net_receive_rcu(NetClientState
*nc, const uint8_t *buf,
_buffers, sizeof mhdr.num_buffers);
}
+for (j = 0; j < i; j++) {
+/* signal other side */
+virtqueue_fill(q->rx_vq, elems[j], lens[j], j);
+g_free(elems[j]);
+}
+
virtqueue_flush(q->rx_vq, i);
virtio_notify(vdev, q->rx_vq);
return size;
+
+err:
+for (j = 0; j < i; j++) {
+g_free(elems[j]);
+}
+
+return err;
}
static ssize_t virtio_net_do_receive(NetClientState *nc, const uint8_t *buf,
--
2.25.1
Re: [PATCH v3 0/6] hw/arm: xilinx_zynq: Fix upstream U-Boot boot failure
On Wed, Sep 01, 2021 at 08:45:15PM +0800, Bin Meng wrote:
> As of today, when booting upstream U-Boot for Xilinx Zynq, the UART
> does not receive anything. Debugging shows that the UART input clock
> frequency is zero which prevents the UART from receiving anything as.
> per the logic in uart_receive().
>
> Note the U-Boot can still output data to the UART tx fifo, which should
> not happen, as the design seems to prevent the data transmission when
> clock is not enabled but somehow it only applies to the Rx side.
>
> For anyone who is interested to give a try, here is the U-Boot defconfig:
> $ make xilinx_zynq_virt_defconfig
>
> and QEMU commands to test U-Boot:
> $ qemu-system-arm -M xilinx-zynq-a9 -m 1G -display none -serial null -serial
> stdio \
> -device loader,file=u-boot-dtb.bin,addr=0x400,cpu-num=0
>
> Note U-Boot used to boot properly in QEMU 4.2.0 which is the QEMU
> version used in current U-Boot's CI testing. The UART clock changes
> were introduced by the following 3 commits:
>
> 38867cb7ec90 ("hw/misc/zynq_slcr: add clock generation for uarts")
> b636db306e06 ("hw/char/cadence_uart: add clock support")
> 5b49a34c6800 ("hw/arm/xilinx_zynq: connect uart clocks to slcr")
Thanks Bin,
On the entire series:
Reviewed-by: Edgar E. Iglesias
Re: [PATCH v1 1/1] target/riscv: Update the ePMP CSR address
On Thu, Sep 2, 2021 at 11:57 AM Bin Meng wrote: > > On Thu, Sep 2, 2021 at 8:40 AM Alistair Francis > wrote: > > > > From: Alistair Francis > > > > Update the ePMP CSRs to match the 0.9.3 ePMP spec > > https://github.com/riscv/riscv-tee/blob/61455747230a26002d741f64879dd78cc9689323/Smepmp/Smepmp.pdf > > > > Signed-off-by: Alistair Francis > > --- > > target/riscv/cpu_bits.h | 4 ++-- > > target/riscv/cpu.c | 1 + > > 2 files changed, 3 insertions(+), 2 deletions(-) > > > > Reviewed-by: Bin Meng Thanks! Applied to riscv-to-apply.next Alistair
Re: [PATCH] s390x: Replace PAGE_SIZE, PAGE_SHIFT and PAGE_MASK
On Wed, 1 Sep 2021 14:58:00 +0200 Thomas Huth wrote: > The PAGE_SIZE macro is causing trouble on Alpine Linux since it > clashes with a macro from a system header there. We already have > the TARGET_PAGE_SIZE, TARGET_PAGE_MASK and TARGET_PAGE_BITS macros > in QEMU anyway, so let's simply replace the PAGE_SIZE, PAGE_MASK > and PAGE_SHIFT macro with their TARGET_* counterparts. > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/572 > Signed-off-by: Thomas Huth Reviewed-by: Halil Pasic
Re: [PATCH 8/8] ppc/pnv: Rename "id" to "quad-id" in PnvQuad
On Wed, Sep 01, 2021 at 11:41:53AM +0200, Cédric Le Goater wrote:
> This to avoid possible conflicts with the "id" property of QOM objects.
>
> Signed-off-by: Cédric Le Goater
Applied to ppc-for-6.2, thanks.
> ---
> include/hw/ppc/pnv_core.h | 2 +-
> hw/ppc/pnv.c | 4 ++--
> hw/ppc/pnv_core.c | 4 ++--
> 3 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/include/hw/ppc/pnv_core.h b/include/hw/ppc/pnv_core.h
> index 6ecee98a76ed..c22eab2e1f69 100644
> --- a/include/hw/ppc/pnv_core.h
> +++ b/include/hw/ppc/pnv_core.h
> @@ -67,7 +67,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(PnvQuad, PNV_QUAD)
> struct PnvQuad {
> DeviceState parent_obj;
>
> -uint32_t id;
> +uint32_t quad_id;
> MemoryRegion xscom_regs;
> };
> #endif /* PPC_PNV_CORE_H */
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 761b82be7401..93f76738fc94 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -1370,10 +1370,10 @@ static void pnv_chip_quad_realize(Pnv9Chip *chip9,
> Error **errp)
> sizeof(*eq), TYPE_PNV_QUAD,
> _fatal, NULL);
>
> -object_property_set_int(OBJECT(eq), "id", core_id, _fatal);
> +object_property_set_int(OBJECT(eq), "quad-id", core_id,
> _fatal);
> qdev_realize(DEVICE(eq), NULL, _fatal);
>
> -pnv_xscom_add_subregion(chip, PNV9_XSCOM_EQ_BASE(eq->id),
> +pnv_xscom_add_subregion(chip, PNV9_XSCOM_EQ_BASE(eq->quad_id),
> >xscom_regs);
> }
> }
> diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
> index 4de8414df212..19e8eb885f71 100644
> --- a/hw/ppc/pnv_core.c
> +++ b/hw/ppc/pnv_core.c
> @@ -407,13 +407,13 @@ static void pnv_quad_realize(DeviceState *dev, Error
> **errp)
> PnvQuad *eq = PNV_QUAD(dev);
> char name[32];
>
> -snprintf(name, sizeof(name), "xscom-quad.%d", eq->id);
> +snprintf(name, sizeof(name), "xscom-quad.%d", eq->quad_id);
> pnv_xscom_region_init(>xscom_regs, OBJECT(dev), _quad_xscom_ops,
>eq, name, PNV9_XSCOM_EQ_SIZE);
> }
>
> static Property pnv_quad_properties[] = {
> -DEFINE_PROP_UINT32("id", PnvQuad, id, 0),
> +DEFINE_PROP_UINT32("quad-id", PnvQuad, quad_id, 0),
> DEFINE_PROP_END_OF_LIST(),
> };
>
--
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
RE: [PATCH 1/2] Use EGL device extension in display initialization.
Thanks for the comment. I will submit another patch.
Best regards,
Eugene
From: Marc-André Lureau
Sent: Monday, August 30, 2021 7:01 AM
To: Eugene Huang ; Gerd Hoffmann
Cc: QEMU
Subject: Re: [PATCH 1/2] Use EGL device extension in display initialization.
External email: Use caution opening links or attachments
On Wed, Aug 25, 2021 at 2:22 AM Eugene Huang
mailto:euge...@nvidia.com>> wrote:
Signed-off-by: Eugene Huang mailto:euge...@nvidia.com>>
---
ui/egl-helpers.c | 41 +
1 file changed, 37 insertions(+), 4 deletions(-)
diff --git a/ui/egl-helpers.c b/ui/egl-helpers.c
index 6d0cb2b5cb..ce0971422b 100644
--- a/ui/egl-helpers.c
+++ b/ui/egl-helpers.c
@@ -1,6 +1,8 @@
/*
* Copyright (C) 2015-2016 Gerd Hoffmann
mailto:kra...@redhat.com>>
*
+ * Copyright (c) 2021, NVIDIA CORPORATION & AFFILIATES. All rights reserved.
+ *
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
* License as published by the Free Software Foundation; either
@@ -351,11 +353,26 @@ static EGLDisplay
qemu_egl_get_display(EGLNativeDisplayType native,
EGLDisplay dpy = EGL_NO_DISPLAY;
/* In practise any EGL 1.5 implementation would support the EXT extension
*/
-if (epoxy_has_egl_extension(NULL, "EGL_EXT_platform_base")) {
+if (epoxy_has_egl_extension(NULL, "EGL_EXT_platform_base")
+&& epoxy_has_egl_extension(NULL, "EGL_EXT_platform_device")
+&& (epoxy_has_egl_extension(NULL, "EGL_EXT_device_base")
+|| epoxy_has_egl_extension(NULL, "EGL_EXT_device_enumeration"))) {
PFNEGLGETPLATFORMDISPLAYEXTPROC getPlatformDisplayEXT =
(void *) eglGetProcAddress("eglGetPlatformDisplayEXT");
if (getPlatformDisplayEXT && platform != 0) {
-dpy = getPlatformDisplayEXT(platform, native, NULL);
+if (platform == EGL_PLATFORM_DEVICE_EXT) {
+static const int MAX_DEVICES = 4;
+EGLDeviceEXT eglDevs[MAX_DEVICES];
+EGLint numDevices;
+
+PFNEGLQUERYDEVICESEXTPROC eglQueryDevicesEXT =
+(PFNEGLQUERYDEVICESEXTPROC)
+eglGetProcAddress("eglQueryDevicesEXT");
It should handle the case where returned eglQueryDevicesEXT is NULL.
+eglQueryDevicesEXT(MAX_DEVICES, eglDevs, );
You need to handle the success return value, as well as the case where
numDevices == 0.
(surprisingly, we don't need to care about memory of eglDevs?)
+dpy = getPlatformDisplayEXT(platform, eglDevs[0], 0);
+} else {
+dpy = getPlatformDisplayEXT(platform, native, NULL);
+}
}
}
@@ -388,6 +405,17 @@ static int qemu_egl_init_dpy(EGLNativeDisplayType dpy,
EGL_ALPHA_SIZE, 0,
EGL_NONE,
};
+
+static const EGLint conf_att_pbuffer[] = {
+EGL_SURFACE_TYPE, EGL_PBUFFER_BIT,
+EGL_RED_SIZE, 8,
+EGL_GREEN_SIZE, 8,
+EGL_BLUE_SIZE, 8,
+EGL_DEPTH_SIZE, 1,
+EGL_RENDERABLE_TYPE, EGL_OPENGL_BIT,
+EGL_NONE
+};
+
EGLint major, minor;
EGLBoolean b;
EGLint n;
@@ -413,8 +441,8 @@ static int qemu_egl_init_dpy(EGLNativeDisplayType dpy,
}
b = eglChooseConfig(qemu_egl_display,
-gles ? conf_att_gles : conf_att_core,
-_egl_config, 1, );
+gles ? conf_att_gles : (platform == EGL_PLATFORM_DEVICE_EXT ?
conf_att_pbuffer : conf_att_core),
+_egl_config, 1, );
if (b == EGL_FALSE || n != 1) {
error_report("egl: eglChooseConfig failed (%s mode)",
gles ? "gles" : "core");
@@ -436,6 +464,11 @@ int qemu_egl_init_dpy_x11(EGLNativeDisplayType dpy,
DisplayGLMode mode)
int qemu_egl_init_dpy_mesa(EGLNativeDisplayType dpy, DisplayGLMode mode)
{
+// Try EGL Device Extension
+if (qemu_egl_init_dpy(dpy, EGL_PLATFORM_DEVICE_EXT, mode) == 0) {
+return 0;
+}
+
#ifdef EGL_MESA_platform_gbm
return qemu_egl_init_dpy(dpy, EGL_PLATFORM_GBM_MESA, mode);
#else
--
2.17.1
--
Marc-André Lureau
Re: [PATCH 7/8] ppc/xive: Export xive_tctx_word2() helper
On Wed, Sep 01, 2021 at 11:41:52AM +0200, Cédric Le Goater wrote:
> Signed-off-by: Cédric Le Goater
Applied to ppc-for-6.2
> ---
> include/hw/ppc/xive.h | 5 +
> hw/intc/xive.c| 5 -
> 2 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
> index 29b130eaea59..252c58a1d691 100644
> --- a/include/hw/ppc/xive.h
> +++ b/include/hw/ppc/xive.h
> @@ -335,6 +335,11 @@ struct XiveTCTX {
> XivePresenter *xptr;
> };
>
> +static inline uint32_t xive_tctx_word2(uint8_t *ring)
> +{
> +return *((uint32_t *) [TM_WORD2]);
> +}
> +
> /*
> * XIVE Router
> */
> diff --git a/hw/intc/xive.c b/hw/intc/xive.c
> index b0c4f76b1d4b..6c82326ec768 100644
> --- a/hw/intc/xive.c
> +++ b/hw/intc/xive.c
> @@ -141,11 +141,6 @@ void xive_tctx_ipb_update(XiveTCTX *tctx, uint8_t ring,
> uint8_t ipb)
> xive_tctx_notify(tctx, ring);
> }
>
> -static inline uint32_t xive_tctx_word2(uint8_t *ring)
> -{
> -return *((uint32_t *) [TM_WORD2]);
> -}
> -
> /*
> * XIVE Thread Interrupt Management Area (TIMA)
> */
--
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
Re: [PATCH 2/8] ppc/spapr: Add a POWER10 DD2 CPU
On Wed, Sep 01, 2021 at 11:41:47AM +0200, Cédric Le Goater wrote:
> Signed-off-by: Cédric Le Goater
Applied to ppc-for-6.2, thanks.
> ---
> hw/ppc/spapr_cpu_core.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 4f316a6f9d31..58e7341cb784 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -382,6 +382,7 @@ static const TypeInfo spapr_cpu_core_type_infos[] = {
> DEFINE_SPAPR_CPU_CORE_TYPE("power9_v1.0"),
> DEFINE_SPAPR_CPU_CORE_TYPE("power9_v2.0"),
> DEFINE_SPAPR_CPU_CORE_TYPE("power10_v1.0"),
> +DEFINE_SPAPR_CPU_CORE_TYPE("power10_v2.0"),
> #ifdef CONFIG_KVM
> DEFINE_SPAPR_CPU_CORE_TYPE("host"),
> #endif
--
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
Re: [PATCH 6/8] ppc/xive: Export priority_to_ipb() helper
On Wed, Sep 01, 2021 at 11:41:51AM +0200, Cédric Le Goater wrote:
> Signed-off-by: Cédric Le Goater
Applied to ppc-for-6.2.
> ---
> include/hw/ppc/xive.h | 11 +++
> hw/intc/xive.c| 21 ++---
> 2 files changed, 17 insertions(+), 15 deletions(-)
>
> diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
> index db7641165484..29b130eaea59 100644
> --- a/include/hw/ppc/xive.h
> +++ b/include/hw/ppc/xive.h
> @@ -458,6 +458,17 @@ struct XiveENDSource {
> */
> #define XIVE_PRIORITY_MAX 7
>
> +/*
> + * Convert a priority number to an Interrupt Pending Buffer (IPB)
> + * register, which indicates a pending interrupt at the priority
> + * corresponding to the bit number
> + */
> +static inline uint8_t xive_priority_to_ipb(uint8_t priority)
> +{
> +return priority > XIVE_PRIORITY_MAX ?
> +0 : 1 << (XIVE_PRIORITY_MAX - priority);
> +}
> +
> /*
> * XIVE Thread Interrupt Management Aera (TIMA)
> *
> diff --git a/hw/intc/xive.c b/hw/intc/xive.c
> index b817ee8e3704..b0c4f76b1d4b 100644
> --- a/hw/intc/xive.c
> +++ b/hw/intc/xive.c
> @@ -27,17 +27,6 @@
> * XIVE Thread Interrupt Management context
> */
>
> -/*
> - * Convert a priority number to an Interrupt Pending Buffer (IPB)
> - * register, which indicates a pending interrupt at the priority
> - * corresponding to the bit number
> - */
> -static uint8_t priority_to_ipb(uint8_t priority)
> -{
> -return priority > XIVE_PRIORITY_MAX ?
> -0 : 1 << (XIVE_PRIORITY_MAX - priority);
> -}
> -
> /*
> * Convert an Interrupt Pending Buffer (IPB) register to a Pending
> * Interrupt Priority Register (PIPR), which contains the priority of
> @@ -89,7 +78,7 @@ static uint64_t xive_tctx_accept(XiveTCTX *tctx, uint8_t
> ring)
> regs[TM_CPPR] = cppr;
>
> /* Reset the pending buffer bit */
> -regs[TM_IPB] &= ~priority_to_ipb(cppr);
> +regs[TM_IPB] &= ~xive_priority_to_ipb(cppr);
> regs[TM_PIPR] = ipb_to_pipr(regs[TM_IPB]);
>
> /* Drop Exception bit */
> @@ -353,7 +342,7 @@ static void xive_tm_set_os_cppr(XivePresenter *xptr,
> XiveTCTX *tctx,
> static void xive_tm_set_os_pending(XivePresenter *xptr, XiveTCTX *tctx,
> hwaddr offset, uint64_t value, unsigned
> size)
> {
> -xive_tctx_ipb_update(tctx, TM_QW1_OS, priority_to_ipb(value & 0xff));
> +xive_tctx_ipb_update(tctx, TM_QW1_OS, xive_priority_to_ipb(value &
> 0xff));
> }
>
> static void xive_os_cam_decode(uint32_t cam, uint8_t *nvt_blk,
> @@ -1535,7 +1524,8 @@ bool xive_presenter_notify(XiveFabric *xfb, uint8_t
> format,
> /* handle CPU exception delivery */
> if (count) {
> trace_xive_presenter_notify(nvt_blk, nvt_idx, match.ring);
> -xive_tctx_ipb_update(match.tctx, match.ring,
> priority_to_ipb(priority));
> +xive_tctx_ipb_update(match.tctx, match.ring,
> + xive_priority_to_ipb(priority));
> }
>
> return !!count;
> @@ -1682,7 +1672,8 @@ static void xive_router_end_notify(XiveRouter *xrtr,
> uint8_t end_blk,
> * use. The presenter will resend the interrupt when the vCPU
> * is dispatched again on a HW thread.
> */
> -ipb = xive_get_field32(NVT_W4_IPB, nvt.w4) |
> priority_to_ipb(priority);
> +ipb = xive_get_field32(NVT_W4_IPB, nvt.w4) |
> +xive_priority_to_ipb(priority);
> nvt.w4 = xive_set_field32(NVT_W4_IPB, nvt.w4, ipb);
> xive_router_write_nvt(xrtr, nvt_blk, nvt_idx, , 4);
>
--
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
Re: [PATCH v2 07/19] target/ppc: Move REQUIRE_ALTIVEC/VECTOR to translate.c
On Tue, Aug 31, 2021 at 01:39:55PM -0300, Luis Pires wrote:
> From: Bruno Larsen
>
> Move REQUIRE_ALTIVEC to translate.c and rename it to REQUIRE_VECTOR.
>
> Signed-off-by: Bruno Larsen
> Signed-off-by: Matheus Ferst
> Signed-off-by: Fernando Valle
> Signed-off-by: Luis Pires
Acked-by: David Gibson
> ---
> target/ppc/translate.c | 8
> target/ppc/translate/vector-impl.c.inc | 10 +-
> 2 files changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index 171b216e17..4749ecdaa9 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -7453,6 +7453,14 @@ static int times_4(DisasContext *ctx, int x)
> # define REQUIRE_64BIT(CTX) REQUIRE_INSNS_FLAGS(CTX, 64B)
> #endif
>
> +#define REQUIRE_VECTOR(CTX) \
> +do {\
> +if (unlikely(!(CTX)->altivec_enabled)) {\
> +gen_exception((CTX), POWERPC_EXCP_VPU); \
> +return true;\
> +} \
> +} while (0)
> +
> /*
> * Helpers for implementing sets of trans_* functions.
> * Defer the implementation of NAME to FUNC, with optional extra arguments.
> diff --git a/target/ppc/translate/vector-impl.c.inc
> b/target/ppc/translate/vector-impl.c.inc
> index 117ce9b137..197e903337 100644
> --- a/target/ppc/translate/vector-impl.c.inc
> +++ b/target/ppc/translate/vector-impl.c.inc
> @@ -17,20 +17,12 @@
> * License along with this library; if not, see
>
Re: [PATCH 4/8] ppc/pnv: Remove useless variable
On Wed, Sep 01, 2021 at 11:41:49AM +0200, Cédric Le Goater wrote:
> Signed-off-by: Cédric Le Goater
Applied to ppc-for-6.2.
> ---
> hw/ppc/pnv.c | 7 +++
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 2f5358b70c95..a62e90b15e27 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -838,8 +838,7 @@ static void pnv_init(MachineState *machine)
> for (i = 0; i < pnv->num_chips; i++) {
> char chip_name[32];
> Object *chip = OBJECT(qdev_new(chip_typename));
> -int chip_id = i;
> -uint64_t chip_ram_size = pnv_chip_get_ram_size(pnv, chip_id);
> +uint64_t chip_ram_size = pnv_chip_get_ram_size(pnv, i);
>
> pnv->chips[i] = PNV_CHIP(chip);
>
> @@ -850,9 +849,9 @@ static void pnv_init(MachineState *machine)
> _fatal);
> chip_ram_start += chip_ram_size;
>
> -snprintf(chip_name, sizeof(chip_name), "chip[%d]", chip_id);
> +snprintf(chip_name, sizeof(chip_name), "chip[%d]", i);
> object_property_add_child(OBJECT(pnv), chip_name, chip);
> -object_property_set_int(chip, "chip-id", chip_id, _fatal);
> +object_property_set_int(chip, "chip-id", i, _fatal);
> object_property_set_int(chip, "nr-cores", machine->smp.cores,
> _fatal);
> object_property_set_int(chip, "nr-threads", machine->smp.threads,
--
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
Re: [PATCH 3/8] ppc/pnv: Add a comment on the "primary-topology-index" property
On Wed, Sep 01, 2021 at 11:41:48AM +0200, Cédric Le Goater wrote: > On P10, the chip id is calculated from the "Primary topology table > index". See skiboot commits for more information [1]. > > This information is extracted from the hdata on real systems which > QEMU needs to emulate. Add this property for all machines even if it > is only used on POWER10. > > [1] https://github.com/open-power/skiboot/commit/2ce3f083f399 > https://github.com/open-power/skiboot/commit/a2d4d7f9e14a > > Signed-off-by: Cédric Le Goater Applied to ppc-for-6.2, thanks. > --- > hw/ppc/pnv_xscom.c | 4 > 1 file changed, 4 insertions(+) > > diff --git a/hw/ppc/pnv_xscom.c b/hw/ppc/pnv_xscom.c > index faa488e3117a..9ce018dbc279 100644 > --- a/hw/ppc/pnv_xscom.c > +++ b/hw/ppc/pnv_xscom.c > @@ -284,6 +284,10 @@ int pnv_dt_xscom(PnvChip *chip, void *fdt, int > root_offset, > _FDT(xscom_offset); > g_free(name); > _FDT((fdt_setprop_cell(fdt, xscom_offset, "ibm,chip-id", > chip->chip_id))); > +/* > + * On P10, the xscom bus id has been deprecated and the chip id is > + * calculated from the "Primary topology table index". See skiboot. > + */ > _FDT((fdt_setprop_cell(fdt, xscom_offset, "ibm,primary-topology-index", > chip->chip_id))); > _FDT((fdt_setprop_cell(fdt, xscom_offset, "#address-cells", 1))); -- 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
Re: [PATCH 5/8] ppc/pnv: Add an assert when calculating the RAM distribution on chips
On Wed, Sep 01, 2021 at 11:41:50AM +0200, Cédric Le Goater wrote: > Signed-off-by: Cédric Le Goater Uh.. I thought the proposed assert was about making it clear there wouldn't be a divide by zero, which would want > 1, not < 2. > --- > hw/ppc/pnv.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c > index a62e90b15e27..761b82be7401 100644 > --- a/hw/ppc/pnv.c > +++ b/hw/ppc/pnv.c > @@ -723,6 +723,8 @@ static uint64_t pnv_chip_get_ram_size(PnvMachineState > *pnv, int chip_id) > return QEMU_ALIGN_DOWN(ram_per_chip, 1 * MiB); > } > > +assert(pnv->num_chips < 2); > + > ram_per_chip = (machine->ram_size - 1 * GiB) / (pnv->num_chips - 1); > return chip_id == 0 ? 1 * GiB : QEMU_ALIGN_DOWN(ram_per_chip, 1 * MiB); > } -- 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
Re: [Internet]Re: [PATCH] .mailmap: Fix more contributor entries
On Sep 1, 2021, at 23:05, Philippe Mathieu-Daudé mailto:f4...@amsat.org>> wrote: On 8/20/21 10:04 AM, Philippe Mathieu-Daudé wrote: These authors have some incorrect author email field. For each of them, there is one commit with the replaced entry. Cc: Alex Chen mailto:alex.c...@huawei.com>> Cc: Bibo Mao mailto:maob...@loongson.cn>> Cc: Guoyi Tu mailto:tu.gu...@h3c.com>> Cc: Haibin Zhang mailto:haibinzh...@tencent.com>> Cc: Hyman Huang mailto:huang...@chinatelecom.cn>> Cc: Lichang Zhao mailto:zhaolich...@huawei.com>> Cc: Yuanjun Gong mailto:ruc_gongyuan...@163.com>> Signed-off-by: Philippe Mathieu-Daudé mailto:f4...@amsat.org>> --- If you are Cc'ed and agree with this change, please reply with a "Reviewed-by: Name " line. I'll wait 2 weeks and consider no reply as a disagreement and will remove your entry from this patch. last call ;) Reviewed-by: Haibin Zhang mailto:haibinzh...@tencent.com>> --- .mailmap | 7 +++ 1 file changed, 7 insertions(+) diff --git a/.mailmap b/.mailmap index f029d1c21fe..11b259e7d07 100644 --- a/.mailmap +++ b/.mailmap @@ -69,6 +69,7 @@ Yongbok Kim mailto:yongbok@mips.com>> mailto:yongbok@imgtec.com>> # git author config, or had utf8/latin1 encoding issues. Aaron Lindsay mailto:aa...@os.amperecomputing.com>> Alexey Gerasimenko mailto:x19...@gmail.com>> +Alex Chen mailto:alex.c...@huawei.com>> Alex Ivanov mailto:v...@aleksoft.net>> Andreas Färber mailto:afaer...@suse.de>> Bandan Das mailto:b...@redhat.com>> @@ -76,6 +77,7 @@ Benjamin MARSILI mailto:mlspira...@gmail.com>> Benoît Canet mailto:benoit.ca...@gmail.com>> Benoît Canet mailto:benoit.ca...@irqsave.net>> Benoît Canet mailto:benoit.ca...@nodalink.com>> +Bibo Mao mailto:maob...@loongson.cn>> Boqun Feng mailto:boqun.f...@gmail.com>> Boqun Feng mailto:boqun.f...@intel.com>> Brad Smith mailto:b...@comstyle.com>> @@ -99,9 +101,12 @@ Gautham R. Shenoy mailto:e...@in.ibm.com>> Gautham R. Shenoy mailto:e...@linux.vnet.ibm.com>> Gonglei (Arei) mailto:arei.gong...@huawei.com>> Guang Wang mailto:wang.guan...@zte.com.cn>> +Guoyi Tu mailto:tu.gu...@h3c.com>> +Haibin Zhang mailto:haibinzh...@tencent.com>> Hailiang Zhang mailto:zhang.zhanghaili...@huawei.com>> Hanna Reitz mailto:hre...@redhat.com>> mailto:mre...@redhat.com>> Hervé Poussineau mailto:hpous...@reactos.org>> +Hyman Huang mailto:huang...@chinatelecom.cn>> Jakub Jermář mailto:ja...@jermar.eu>> Jakub Jermář mailto:jakub.jer...@kernkonzept.com>> Jean-Christophe Dubois mailto:j...@tribudubois.net>> @@ -113,6 +118,7 @@ Jun Li mailto:junm...@gmail.com>> Laurent Vivier mailto:laur...@lvivier.info>> Leandro Lupori mailto:leandro.lup...@gmail.com>> Li Guang mailto:lig.f...@cn.fujitsu.com>> +Lichang Zhao mailto:zhaolich...@huawei.com>> Liming Wang mailto:walimis...@gmail.com>> linzhecheng mailto:li...@zju.edu.cn>> Liran Schour mailto:lir...@il.ibm.com>> @@ -171,6 +177,7 @@ Xiong Zhang mailto:xiong.y.zh...@intel.com>> Yin Yin mailto:yin@cs2c.com.cn>> Yu-Chen Lin mailto:npes87...@gmail.com>> Yu-Chen Lin mailto:npes87...@gmail.com>> mailto:yuchen...@synology.com>> +Yuanjun Gong mailto:ruc_gongyuan...@163.com>> YunQiang Su mailto:s...@debian.org>> YunQiang Su mailto:y...@wavecomp.com>> Yuri Pudgorodskiy mailto:y...@virtuozzo.com>>
Re: [PATCH v3 5/6] hw/char: cadence_uart: Ignore access when unclocked or in reset for uart_{read, write}()
On Wed, Sep 1, 2021 at 10:49 PM Bin Meng wrote:
>
> Read or write to uart registers when unclocked or in reset should be
> ignored. Add the check there, and as a result of this, the check in
> uart_write_tx_fifo() is now unnecessary.
>
> Signed-off-by: Bin Meng
Reviewed-by: Alistair Francis
Alistair
>
> ---
>
> (no changes since v2)
>
> Changes in v2:
> - new patch: hw/char: cadence_uart: Ignore access when unclocked or in reset
> for uart_{read,write}()
>
> hw/char/cadence_uart.c | 15 ++-
> 1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c
> index 8bcf2b718a..5f5a4645ac 100644
> --- a/hw/char/cadence_uart.c
> +++ b/hw/char/cadence_uart.c
> @@ -335,11 +335,6 @@ static gboolean cadence_uart_xmit(void *do_not_use,
> GIOCondition cond,
> static void uart_write_tx_fifo(CadenceUARTState *s, const uint8_t *buf,
> int size)
> {
> -/* ignore characters when unclocked or in reset */
> -if (!clock_is_enabled(s->refclk) || device_is_in_reset(DEVICE(s))) {
> -return;
> -}
> -
> if ((s->r[R_CR] & UART_CR_TX_DIS) || !(s->r[R_CR] & UART_CR_TX_EN)) {
> return;
> }
> @@ -416,6 +411,11 @@ static MemTxResult uart_write(void *opaque, hwaddr
> offset,
> {
> CadenceUARTState *s = opaque;
>
> +/* ignore access when unclocked or in reset */
> +if (!clock_is_enabled(s->refclk) || device_is_in_reset(DEVICE(s))) {
> +return MEMTX_ERROR;
> +}
> +
> DB_PRINT(" offset:%x data:%08x\n", (unsigned)offset, (unsigned)value);
> offset >>= 2;
> if (offset >= CADENCE_UART_R_MAX) {
> @@ -476,6 +476,11 @@ static MemTxResult uart_read(void *opaque, hwaddr offset,
> CadenceUARTState *s = opaque;
> uint32_t c = 0;
>
> +/* ignore access when unclocked or in reset */
> +if (!clock_is_enabled(s->refclk) || device_is_in_reset(DEVICE(s))) {
> +return MEMTX_ERROR;
> +}
> +
> offset >>= 2;
> if (offset >= CADENCE_UART_R_MAX) {
> return MEMTX_DECODE_ERROR;
> --
> 2.25.1
>
>
Re: [PATCH v3 6/6] hw/char: cadence_uart: Log a guest error when device is unclocked or in reset
On Wed, Sep 1, 2021 at 10:52 PM Bin Meng wrote:
>
> We've got SW that expects FSBL (Bootlooader) to setup clocks and
> resets. It's quite common that users run that SW on QEMU without
> FSBL (FSBL typically requires the Xilinx tools installed). That's
> fine, since users can stil use -device loader to enable clocks etc.
>
> To help folks understand what's going, a log (guest-error) message
> would be helpful here. In particular with the serial port since
> things will go very quiet if they get things wrong.
>
> Suggested-by: Edgar E. Iglesias
> Signed-off-by: Bin Meng
Reviewed-by: Alistair Francis
Alistair
>
> ---
>
> Changes in v3:
> - new patch: hw/char: cadence_uart: Log a guest error when unclocked or in
> reset
>
> hw/char/cadence_uart.c | 8
> 1 file changed, 8 insertions(+)
>
> diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c
> index 5f5a4645ac..c069a30842 100644
> --- a/hw/char/cadence_uart.c
> +++ b/hw/char/cadence_uart.c
> @@ -240,6 +240,8 @@ static int uart_can_receive(void *opaque)
>
> /* ignore characters when unclocked or in reset */
> if (!clock_is_enabled(s->refclk) || device_is_in_reset(DEVICE(s))) {
> +qemu_log_mask(LOG_GUEST_ERROR, "%s: uart is unclocked or in reset\n",
> + __func__);
> return 0;
> }
>
> @@ -376,6 +378,8 @@ static void uart_event(void *opaque, QEMUChrEvent event)
>
> /* ignore characters when unclocked or in reset */
> if (!clock_is_enabled(s->refclk) || device_is_in_reset(DEVICE(s))) {
> +qemu_log_mask(LOG_GUEST_ERROR, "%s: uart is unclocked or in reset\n",
> + __func__);
> return;
> }
>
> @@ -413,6 +417,8 @@ static MemTxResult uart_write(void *opaque, hwaddr offset,
>
> /* ignore access when unclocked or in reset */
> if (!clock_is_enabled(s->refclk) || device_is_in_reset(DEVICE(s))) {
> +qemu_log_mask(LOG_GUEST_ERROR, "%s: uart is unclocked or in reset\n",
> + __func__);
> return MEMTX_ERROR;
> }
>
> @@ -478,6 +484,8 @@ static MemTxResult uart_read(void *opaque, hwaddr offset,
>
> /* ignore access when unclocked or in reset */
> if (!clock_is_enabled(s->refclk) || device_is_in_reset(DEVICE(s))) {
> +qemu_log_mask(LOG_GUEST_ERROR, "%s: uart is unclocked or in reset\n",
> + __func__);
> return MEMTX_ERROR;
> }
>
> --
> 2.25.1
>
>
Re: [PATCH v3 4/6] hw/char: cadence_uart: Convert to memop_with_attrs() ops
On Wed, Sep 1, 2021 at 10:46 PM Bin Meng wrote:
>
> This converts uart_read() and uart_write() to memop_with_attrs() ops.
>
> Signed-off-by: Bin Meng
Reviewed-by: Alistair Francis
Alistair
>
> ---
>
> (no changes since v2)
>
> Changes in v2:
> - new patch: hw/char: cadence_uart: Convert to memop_with_attrs() ops
>
> hw/char/cadence_uart.c | 26 +++---
> 1 file changed, 15 insertions(+), 11 deletions(-)
>
> diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c
> index fff8be3619..8bcf2b718a 100644
> --- a/hw/char/cadence_uart.c
> +++ b/hw/char/cadence_uart.c
> @@ -411,15 +411,15 @@ static void uart_read_rx_fifo(CadenceUARTState *s,
> uint32_t *c)
> uart_update_status(s);
> }
>
> -static void uart_write(void *opaque, hwaddr offset,
> - uint64_t value, unsigned size)
> +static MemTxResult uart_write(void *opaque, hwaddr offset,
> + uint64_t value, unsigned size, MemTxAttrs
> attrs)
> {
> CadenceUARTState *s = opaque;
>
> DB_PRINT(" offset:%x data:%08x\n", (unsigned)offset, (unsigned)value);
> offset >>= 2;
> if (offset >= CADENCE_UART_R_MAX) {
> -return;
> +return MEMTX_DECODE_ERROR;
> }
> switch (offset) {
> case R_IER: /* ier (wts imr) */
> @@ -466,30 +466,34 @@ static void uart_write(void *opaque, hwaddr offset,
> break;
> }
> uart_update_status(s);
> +
> +return MEMTX_OK;
> }
>
> -static uint64_t uart_read(void *opaque, hwaddr offset,
> -unsigned size)
> +static MemTxResult uart_read(void *opaque, hwaddr offset,
> + uint64_t *value, unsigned size, MemTxAttrs
> attrs)
> {
> CadenceUARTState *s = opaque;
> uint32_t c = 0;
>
> offset >>= 2;
> if (offset >= CADENCE_UART_R_MAX) {
> -c = 0;
> -} else if (offset == R_TX_RX) {
> +return MEMTX_DECODE_ERROR;
> +}
> +if (offset == R_TX_RX) {
> uart_read_rx_fifo(s, );
> } else {
> - c = s->r[offset];
> +c = s->r[offset];
> }
>
> DB_PRINT(" offset:%x data:%08x\n", (unsigned)(offset << 2), (unsigned)c);
> -return c;
> +*value = c;
> +return MEMTX_OK;
> }
>
> static const MemoryRegionOps uart_ops = {
> -.read = uart_read,
> -.write = uart_write,
> +.read_with_attrs = uart_read,
> +.write_with_attrs = uart_write,
> .endianness = DEVICE_NATIVE_ENDIAN,
> };
>
> --
> 2.25.1
>
>
Re: [PATCH v3 3/6] hw/char: cadence_uart: Move clock/reset check to uart_can_receive()
On Wed, Sep 1, 2021 at 10:49 PM Bin Meng wrote:
>
> Currently the clock/reset check is done in uart_receive(), but we
> can move the check to uart_can_receive() which is earlier.
>
> Signed-off-by: Bin Meng
Reviewed-by: Alistair Francis
Alistair
>
> ---
>
> (no changes since v2)
>
> Changes in v2:
> - avoid declaring variables mid-scope
>
> hw/char/cadence_uart.c | 17 ++---
> 1 file changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c
> index 154be34992..fff8be3619 100644
> --- a/hw/char/cadence_uart.c
> +++ b/hw/char/cadence_uart.c
> @@ -235,8 +235,16 @@ static void uart_parameters_setup(CadenceUARTState *s)
> static int uart_can_receive(void *opaque)
> {
> CadenceUARTState *s = opaque;
> -int ret = MAX(CADENCE_UART_RX_FIFO_SIZE, CADENCE_UART_TX_FIFO_SIZE);
> -uint32_t ch_mode = s->r[R_MR] & UART_MR_CHMODE;
> +int ret;
> +uint32_t ch_mode;
> +
> +/* ignore characters when unclocked or in reset */
> +if (!clock_is_enabled(s->refclk) || device_is_in_reset(DEVICE(s))) {
> +return 0;
> +}
> +
> +ret = MAX(CADENCE_UART_RX_FIFO_SIZE, CADENCE_UART_TX_FIFO_SIZE);
> +ch_mode = s->r[R_MR] & UART_MR_CHMODE;
>
> if (ch_mode == NORMAL_MODE || ch_mode == ECHO_MODE) {
> ret = MIN(ret, CADENCE_UART_RX_FIFO_SIZE - s->rx_count);
> @@ -358,11 +366,6 @@ static void uart_receive(void *opaque, const uint8_t
> *buf, int size)
> CadenceUARTState *s = opaque;
> uint32_t ch_mode = s->r[R_MR] & UART_MR_CHMODE;
>
> -/* ignore characters when unclocked or in reset */
> -if (!clock_is_enabled(s->refclk) || device_is_in_reset(DEVICE(s))) {
> -return;
> -}
> -
> if (ch_mode == NORMAL_MODE || ch_mode == ECHO_MODE) {
> uart_write_rx_fifo(opaque, buf, size);
> }
> --
> 2.25.1
>
>
Re: [PATCH] target/riscv: Fix satp write
On Thu, Sep 2, 2021 at 11:59 AM Bin Meng wrote: > > On Thu, Sep 2, 2021 at 9:02 AM LIU Zhiwei wrote: > > > > > > On 2021/9/1 下午9:05, Bin Meng wrote: > > > On Wed, Sep 1, 2021 at 8:51 PM LIU Zhiwei wrote: > > >> These variables should be target_ulong. If truncated to int, > > >> the bool conditions they indicate will be wrong. > > >> > > >> As satp is very important for Linux, this bug almost fails every boot. > > > Could you please describe which Linux configuration is broken? > > > > I use the image from: > > > > https://gitlab.com/c-sky/buildroot/-/jobs/1251564514/artifacts/browse/output/images/ > > > > > I have > > > a 64-bit 5.10 kernel and it boots fine. > > > > The login is mostly OK for me. But the busybox can't run properly. > > Which kernel version is this? Could you please investigate and > indicate in the commit message? > > I just tested current qemu-system-riscv64 can boot to Ubuntu 20.04 > distro user space. I also have never seen any issues. Looking at this `vm` is set from a `static const char valid_vm_1_10_64` so an int is fine. It probably is a good idea for `mask` and `asid` to be target_ulong as they are set by bit operations on target_ulong's. I guess if your host int is 32-bits SATP64_ASID will overflow that. Anyway with 128-bit RISC-V and maybe the ability to run 64-bit guests no 32-bit hosts this seems like a good step Reviewed-by: Alistair Francis Alistair > > Regards, > Bin >
Re: [PATCH] target/riscv: Fix satp write
On Thu, Sep 2, 2021 at 10:44 AM LIU Zhiwei wrote: > > > On 2021/9/2 上午9:59, Bin Meng wrote: > > On Thu, Sep 2, 2021 at 9:02 AM LIU Zhiwei wrote: > >> > >> On 2021/9/1 下午9:05, Bin Meng wrote: > >>> On Wed, Sep 1, 2021 at 8:51 PM LIU Zhiwei wrote: > These variables should be target_ulong. If truncated to int, > the bool conditions they indicate will be wrong. > > As satp is very important for Linux, this bug almost fails every boot. > >>> Could you please describe which Linux configuration is broken? > >> I use the image from: > >> > >> https://gitlab.com/c-sky/buildroot/-/jobs/1251564514/artifacts/browse/output/images/ > >> > >>>I have > >>> a 64-bit 5.10 kernel and it boots fine. > >> The login is mostly OK for me. But the busybox can't run properly. > > Which kernel version is this? > 5.10.4 > > Could you please investigate and > > indicate in the commit message? > > > > I just tested current qemu-system-riscv64 can boot to Ubuntu 20.04 > > distro user space. > > Very strange. This will cause tlb_flush can't be called in this function. > Did your kernel enable asid? Regards, Bin
Re: [PATCH] target/riscv: Fix satp write
On 2021/9/2 上午9:59, Bin Meng wrote: On Thu, Sep 2, 2021 at 9:02 AM LIU Zhiwei wrote: On 2021/9/1 下午9:05, Bin Meng wrote: On Wed, Sep 1, 2021 at 8:51 PM LIU Zhiwei wrote: These variables should be target_ulong. If truncated to int, the bool conditions they indicate will be wrong. As satp is very important for Linux, this bug almost fails every boot. Could you please describe which Linux configuration is broken? I use the image from: https://gitlab.com/c-sky/buildroot/-/jobs/1251564514/artifacts/browse/output/images/ I have a 64-bit 5.10 kernel and it boots fine. The login is mostly OK for me. But the busybox can't run properly. Which kernel version is this? 5.10.4 Could you please investigate and indicate in the commit message? I just tested current qemu-system-riscv64 can boot to Ubuntu 20.04 distro user space. Very strange. This will cause tlb_flush can't be called in this function. Thanks, Zhiwei Regards, Bin
Re: [PATCH v2 4/5] hw/char: cadence_uart: Convert to memop_with_attrs() ops
On Wed, Sep 1, 2021 at 1:30 PM Bin Meng wrote:
>
> This converts uart_read() and uart_write() to memop_with_attrs() ops.
>
> Signed-off-by: Bin Meng
Reviewed-by: Alistair Francis
Alistair
>
> ---
>
> Changes in v2:
> - new patch: hw/char: cadence_uart: Convert to memop_with_attrs() ops
>
> hw/char/cadence_uart.c | 26 +++---
> 1 file changed, 15 insertions(+), 11 deletions(-)
>
> diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c
> index fff8be3619..8bcf2b718a 100644
> --- a/hw/char/cadence_uart.c
> +++ b/hw/char/cadence_uart.c
> @@ -411,15 +411,15 @@ static void uart_read_rx_fifo(CadenceUARTState *s,
> uint32_t *c)
> uart_update_status(s);
> }
>
> -static void uart_write(void *opaque, hwaddr offset,
> - uint64_t value, unsigned size)
> +static MemTxResult uart_write(void *opaque, hwaddr offset,
> + uint64_t value, unsigned size, MemTxAttrs
> attrs)
> {
> CadenceUARTState *s = opaque;
>
> DB_PRINT(" offset:%x data:%08x\n", (unsigned)offset, (unsigned)value);
> offset >>= 2;
> if (offset >= CADENCE_UART_R_MAX) {
> -return;
> +return MEMTX_DECODE_ERROR;
> }
> switch (offset) {
> case R_IER: /* ier (wts imr) */
> @@ -466,30 +466,34 @@ static void uart_write(void *opaque, hwaddr offset,
> break;
> }
> uart_update_status(s);
> +
> +return MEMTX_OK;
> }
>
> -static uint64_t uart_read(void *opaque, hwaddr offset,
> -unsigned size)
> +static MemTxResult uart_read(void *opaque, hwaddr offset,
> + uint64_t *value, unsigned size, MemTxAttrs
> attrs)
> {
> CadenceUARTState *s = opaque;
> uint32_t c = 0;
>
> offset >>= 2;
> if (offset >= CADENCE_UART_R_MAX) {
> -c = 0;
> -} else if (offset == R_TX_RX) {
> +return MEMTX_DECODE_ERROR;
> +}
> +if (offset == R_TX_RX) {
> uart_read_rx_fifo(s, );
> } else {
> - c = s->r[offset];
> +c = s->r[offset];
> }
>
> DB_PRINT(" offset:%x data:%08x\n", (unsigned)(offset << 2), (unsigned)c);
> -return c;
> +*value = c;
> +return MEMTX_OK;
> }
>
> static const MemoryRegionOps uart_ops = {
> -.read = uart_read,
> -.write = uart_write,
> +.read_with_attrs = uart_read,
> +.write_with_attrs = uart_write,
> .endianness = DEVICE_NATIVE_ENDIAN,
> };
>
> --
> 2.25.1
>
>
Re: [PATCH v2 3/5] hw/char: cadence_uart: Move clock/reset check to uart_can_receive()
On Wed, Sep 1, 2021 at 1:28 PM Bin Meng wrote:
>
> Currently the clock/reset check is done in uart_receive(), but we
> can move the check to uart_can_receive() which is earlier.
>
> Signed-off-by: Bin Meng
Reviewed-by: Alistair Francis
Alistair
>
> ---
>
> Changes in v2:
> - avoid declaring variables mid-scope
>
> hw/char/cadence_uart.c | 17 ++---
> 1 file changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c
> index 154be34992..fff8be3619 100644
> --- a/hw/char/cadence_uart.c
> +++ b/hw/char/cadence_uart.c
> @@ -235,8 +235,16 @@ static void uart_parameters_setup(CadenceUARTState *s)
> static int uart_can_receive(void *opaque)
> {
> CadenceUARTState *s = opaque;
> -int ret = MAX(CADENCE_UART_RX_FIFO_SIZE, CADENCE_UART_TX_FIFO_SIZE);
> -uint32_t ch_mode = s->r[R_MR] & UART_MR_CHMODE;
> +int ret;
> +uint32_t ch_mode;
> +
> +/* ignore characters when unclocked or in reset */
> +if (!clock_is_enabled(s->refclk) || device_is_in_reset(DEVICE(s))) {
> +return 0;
> +}
> +
> +ret = MAX(CADENCE_UART_RX_FIFO_SIZE, CADENCE_UART_TX_FIFO_SIZE);
> +ch_mode = s->r[R_MR] & UART_MR_CHMODE;
>
> if (ch_mode == NORMAL_MODE || ch_mode == ECHO_MODE) {
> ret = MIN(ret, CADENCE_UART_RX_FIFO_SIZE - s->rx_count);
> @@ -358,11 +366,6 @@ static void uart_receive(void *opaque, const uint8_t
> *buf, int size)
> CadenceUARTState *s = opaque;
> uint32_t ch_mode = s->r[R_MR] & UART_MR_CHMODE;
>
> -/* ignore characters when unclocked or in reset */
> -if (!clock_is_enabled(s->refclk) || device_is_in_reset(DEVICE(s))) {
> -return;
> -}
> -
> if (ch_mode == NORMAL_MODE || ch_mode == ECHO_MODE) {
> uart_write_rx_fifo(opaque, buf, size);
> }
> --
> 2.25.1
>
>
Re: [PATCH v7 14/14] disas/riscv: Add Zb[abcs] instructions
On Mon, Aug 30, 2021 at 9:22 PM Philipp Tomsich
wrote:
>
> With the addition of Zb[abcs], we also need to add disassembler
> support for these new instructions.
>
> Signed-off-by: Philipp Tomsich
Acked-by: Alistair Francis
Alistair
>
> ---
>
> (no changes since v2)
>
> Changes in v2:
> - Fix missing ';' from last-minute whitespace cleanups.
>
> disas/riscv.c | 157 +-
> 1 file changed, 154 insertions(+), 3 deletions(-)
>
> diff --git a/disas/riscv.c b/disas/riscv.c
> index 278d9be924..793ad14c27 100644
> --- a/disas/riscv.c
> +++ b/disas/riscv.c
> @@ -478,6 +478,49 @@ typedef enum {
> rv_op_fsflags = 316,
> rv_op_fsrmi = 317,
> rv_op_fsflagsi = 318,
> +rv_op_bseti = 319,
> +rv_op_bclri = 320,
> +rv_op_binvi = 321,
> +rv_op_bexti = 322,
> +rv_op_rori = 323,
> +rv_op_clz = 324,
> +rv_op_ctz = 325,
> +rv_op_cpop = 326,
> +rv_op_sext_h = 327,
> +rv_op_sext_b = 328,
> +rv_op_xnor = 329,
> +rv_op_orn = 330,
> +rv_op_andn = 331,
> +rv_op_rol = 332,
> +rv_op_ror = 333,
> +rv_op_sh1add = 334,
> +rv_op_sh2add = 335,
> +rv_op_sh3add = 336,
> +rv_op_sh1add_uw = 337,
> +rv_op_sh2add_uw = 338,
> +rv_op_sh3add_uw = 339,
> +rv_op_clmul = 340,
> +rv_op_clmulr = 341,
> +rv_op_clmulh = 342,
> +rv_op_min = 343,
> +rv_op_minu = 344,
> +rv_op_max = 345,
> +rv_op_maxu = 346,
> +rv_op_clzw = 347,
> +rv_op_ctzw = 348,
> +rv_op_cpopw = 349,
> +rv_op_slli_uw = 350,
> +rv_op_add_uw = 351,
> +rv_op_rolw = 352,
> +rv_op_rorw = 353,
> +rv_op_rev8 = 354,
> +rv_op_zext_h = 355,
> +rv_op_roriw = 356,
> +rv_op_orc_b = 357,
> +rv_op_bset = 358,
> +rv_op_bclr = 359,
> +rv_op_binv = 360,
> +rv_op_bext = 361,
> } rv_op;
>
> /* structures */
> @@ -1117,6 +1160,49 @@ const rv_opcode_data opcode_data[] = {
> { "fsflags", rv_codec_i_csr, rv_fmt_rd_rs1, NULL, 0, 0, 0 },
> { "fsrmi", rv_codec_i_csr, rv_fmt_rd_zimm, NULL, 0, 0, 0 },
> { "fsflagsi", rv_codec_i_csr, rv_fmt_rd_zimm, NULL, 0, 0, 0 },
> +{ "bseti", rv_codec_i_sh7, rv_fmt_rd_rs1_imm, NULL, 0, 0, 0 },
> +{ "bclri", rv_codec_i_sh7, rv_fmt_rd_rs1_imm, NULL, 0, 0, 0 },
> +{ "binvi", rv_codec_i_sh7, rv_fmt_rd_rs1_imm, NULL, 0, 0, 0 },
> +{ "bexti", rv_codec_i_sh7, rv_fmt_rd_rs1_imm, NULL, 0, 0, 0 },
> +{ "rori", rv_codec_i_sh7, rv_fmt_rd_rs1_imm, NULL, 0, 0, 0 },
> +{ "clz", rv_codec_r, rv_fmt_rd_rs1, NULL, 0, 0, 0 },
> +{ "ctz", rv_codec_r, rv_fmt_rd_rs1, NULL, 0, 0, 0 },
> +{ "cpop", rv_codec_r, rv_fmt_rd_rs1, NULL, 0, 0, 0 },
> +{ "sext.h", rv_codec_r, rv_fmt_rd_rs1, NULL, 0, 0, 0 },
> +{ "sext.b", rv_codec_r, rv_fmt_rd_rs1, NULL, 0, 0, 0 },
> +{ "xnor", rv_codec_r, rv_fmt_rd_rs1, NULL, 0, 0, 0 },
> +{ "orn", rv_codec_r, rv_fmt_rd_rs1, NULL, 0, 0, 0 },
> +{ "andn", rv_codec_r, rv_fmt_rd_rs1, NULL, 0, 0, 0 },
> +{ "rol", rv_codec_r, rv_fmt_rd_rs1_rs2, NULL, 0, 0, 0 },
> +{ "ror", rv_codec_r, rv_fmt_rd_rs1_rs2, NULL, 0, 0, 0 },
> +{ "sh1add", rv_codec_r, rv_fmt_rd_rs1_rs2, NULL, 0, 0, 0 },
> +{ "sh2add", rv_codec_r, rv_fmt_rd_rs1_rs2, NULL, 0, 0, 0 },
> +{ "sh3add", rv_codec_r, rv_fmt_rd_rs1_rs2, NULL, 0, 0, 0 },
> +{ "sh1add.uw", rv_codec_r, rv_fmt_rd_rs1_rs2, NULL, 0, 0, 0 },
> +{ "sh2add.uw", rv_codec_r, rv_fmt_rd_rs1_rs2, NULL, 0, 0, 0 },
> +{ "sh3add.uw", rv_codec_r, rv_fmt_rd_rs1_rs2, NULL, 0, 0, 0 },
> +{ "clmul", rv_codec_r, rv_fmt_rd_rs1_rs2, NULL, 0, 0, 0 },
> +{ "clmulr", rv_codec_r, rv_fmt_rd_rs1_rs2, NULL, 0, 0, 0 },
> +{ "clmulh", rv_codec_r, rv_fmt_rd_rs1_rs2, NULL, 0, 0, 0 },
> +{ "min", rv_codec_r, rv_fmt_rd_rs1_rs2, NULL, 0, 0, 0 },
> +{ "minu", rv_codec_r, rv_fmt_rd_rs1_rs2, NULL, 0, 0, 0 },
> +{ "max", rv_codec_r, rv_fmt_rd_rs1_rs2, NULL, 0, 0, 0 },
> +{ "maxu", rv_codec_r, rv_fmt_rd_rs1_rs2, NULL, 0, 0, 0 },
> +{ "clzw", rv_codec_r, rv_fmt_rd_rs1, NULL, 0, 0, 0 },
> +{ "clzw", rv_codec_r, rv_fmt_rd_rs1, NULL, 0, 0, 0 },
> +{ "cpopw", rv_codec_r, rv_fmt_rd_rs1, NULL, 0, 0, 0 },
> +{ "slli.uw", rv_codec_r, rv_fmt_rd_rs1_rs2, NULL, 0, 0, 0 },
> +{ "add.uw", rv_codec_r, rv_fmt_rd_rs1_rs2, NULL, 0, 0, 0 },
> +{ "rolw", rv_codec_r, rv_fmt_rd_rs1_rs2, NULL, 0, 0, 0 },
> +{ "rorw", rv_codec_r, rv_fmt_rd_rs1_rs2, NULL, 0, 0, 0 },
> +{ "rev8", rv_codec_r, rv_fmt_rd_rs1, NULL, 0, 0, 0 },
> +{ "zext.h", rv_codec_r, rv_fmt_rd_rs1, NULL, 0, 0, 0 },
> +{ "roriw", rv_codec_i_sh5, rv_fmt_rd_rs1_imm, NULL, 0, 0, 0 },
> +{ "orc.b", rv_codec_r, rv_fmt_rd_rs1, NULL, 0, 0, 0 },
> +{ "bset", rv_codec_r, rv_fmt_rd_rs1_rs2, NULL, 0, 0, 0 },
> +{ "bclr", rv_codec_r, rv_fmt_rd_rs1_rs2, NULL, 0, 0, 0 },
> +{ "binv", rv_codec_r, rv_fmt_rd_rs1_rs2, NULL, 0, 0, 0 },
> +{ "bext", rv_codec_r, rv_fmt_rd_rs1_rs2, NULL, 0, 0, 0 },
> };
>
> /* CSR names */
> @@ -1507,7 +1593,20 @@
Re: [RFC PATCH 0/2] riscv: Adding custom CSR related Kconfig options
On Fri, Aug 27, 2021 at 1:16 AM Ruinland Chuan-Tzu Tsai wrote: > > From: Ruinland ChuanTzu Tsai > > During my modification on my previous patch series for custom CSR support, I > believe this issue deserves its own discussion (or debate) because it's _not_ > as simple as "just put those options in Kconfig". > > The obstables I've encountered and the kluges I came up is listed as follow : > > (1) Due to the design of top-level meson.build, all Kconfig options will land > into `*-config-devices.h` since minikconf will be only used after > config_target > being processed. This will let to the fact that linux-users won't be able to > use custom CSR code properly becuase they only includes `*-config-devices.h`. > And that is reasonble due to the fact that changes on cpu.c and csr.c is a > target-related matter and linux-user mode shouldn't include device related > headers in most of cases. > > So, modify meson.build to parse target/riscv/Kconfig during config_target > phase > is without doubts necessary. > > (2) Kconfig option `RISCV_CUSTOM_CSR` is introduced for RISC-V cpu models to > toggle it at its will. Yet due to the fact that csr.o and cpu.o are linked > altogether for all CPU models, the suffer will be shared without option. > The only reasonable way to seperate build the fire lane which seperates vendor > flavored cpu and spec-conformed ones, is to build them seperately with options > toggled diffrently, just like RV32 and RV64 shares almost the same source > base, > yet the sources are compiled with differnt flags/definitions. > > To achieve that, miraculously, we can just put *.mak files into `target` > directoy, because that's how `configure` enumerates what targets are > supported. > > (3) The longest days are not over yet, if we take a good look at how the > minikconf > is invoked during config_devices and in what way *.mak presented its options > inside `default-configs/devices`, we can see that *.mak files there is > formated > in `CONFIG_*` style and the minikconf is reading directly during config_device > phase. That's totally different from *.mak files presented in > `default-configs/targets`. To make the parsing logic consistent, I > introduce a rv_custom directory inside which contains minikconf-parsable > mak files. > > With this patches, ones can build a A25/AX25 linux-user platform by : > $ ./configure --target-list=riscv64-andes-linux-user,riscv32-andes-linux-user Hey! Thanks for the patches I'm not convinced that we want this though. Right now we are trying to head towards a riscv64-softmmu binary being able to run all RISC-V code. That include 32-bit cpus (qemu-riscv64-softmmu -cpu r32...) and 64-bit CPUs. We shouldn't be splitting out more targets. It also goes against the general idea of RISC-V in that everyone has a standard compliant implementation, they can then add extra functionality. In terms of Kconfig options. It doesn't seem like a bad idea to have an option to fully disable custom CSRs. That way if someone really wants performance and doesn't want custom CSRs they can disable the switch. Otherwise we leave it on and all custom CSRs are available in the build and then controlled by the CPU selection at runtime. If that ends up being too difficult to implement though then we don't have to have it. Thanks again for working on this. Alistair > $ make > > P.S. The pacthes from : > https://lists.gnu.org/archive/html/qemu-devel/2021-08/msg00913.html > is needed. A clean-up and modified version will be sent out soon. > > P.P.S. > I know these parts won't be easy to digest, and the further iterations will be > needed, so I didn't ask my colleagues to sign-off for now. > > Cordially yours, > Ruinland ChuanTzu Tsai > > Ruinland ChuanTzu Tsai (2): > Adding Kconfig options for custom CSR support and Andes CPU model > Adding necessary files for Andes platforms, cores to enable custom CSR > support > > Kconfig | 1 + > .../devices/riscv32-andes-softmmu.mak | 17 > .../devices/riscv64-andes-softmmu.mak | 17 > .../targets/riscv32-andes-linux-user.mak | 1 + > .../targets/riscv32-andes-softmmu.mak | 1 + > .../targets/riscv64-andes-linux-user.mak | 1 + > .../targets/riscv64-andes-softmmu.mak | 1 + > .../targets/rv_custom/no_custom.mak | 0 > .../rv_custom/riscv32-andes-linux-user.mak| 1 + > .../rv_custom/riscv32-andes-softmmu.mak | 1 + > .../targets/rv_custom/riscv32-linux-user.mak | 1 + > .../targets/rv_custom/riscv32-softmmu.mak | 1 + > .../rv_custom/riscv64-andes-linux-user.mak| 1 + > .../rv_custom/riscv64-andes-softmmu.mak | 1 + > .../targets/rv_custom/riscv64-linux-user.mak | 1 + > .../targets/rv_custom/riscv64-softmmu.mak | 1 + > meson.build | 26 +++ > target/riscv/Kconfig | 6 + > 18 files changed, 79
Re: How does qemu detect the completion of interrupt execution？
Hi,
thank you for your response.
As you say
> "
> *End of interrupt handling is entirely dependent on what the*
> *guest hardware being emulated is. Usually the guest software*
> *will indicate "interrupt handled" back to the interrupt*
> *controller (perhaps by writing a register; depends on the*
> *interrupt controller), and the interrupt controller will*
> *then look at what the next highest priority pending interrupt*
> *is and signal that back to the CPU, or do nothing if there's*
> *no new interrupt. So the second interrupt will automatically*
> *be taken and handled once the first one has finished,*
> *as a result of this interrupt controller and guest OS**interaction*."
I agree with that. I has try some method, But Still have some problems.
Q1:
My guest(target) cpu seem don't have a * "interrupt handled" , *And I
don't know How/When to program the "* interrupt controller" *to check
the second interrupt when the first over.
Q2:
Also I found the new problem(maybe bug) , when first interrupt not over,
the second interrupt may occure, this case Interrupt nesting ，if I check
Interrupt flag in the code，the second interrupt losed。
I don't know the interrupt mechanism of qemu very well. If you have any
suggestions, I am very happy to receive them.
this is my code：
/* set irq for device */
> static void stm8_cpu_set_irq(void *opaque, int irq, int level)
> {
> //printf("%s\n",__func__);
> STM8CPU *cpu = opaque;
> CPUSTM8State *env = >env;
> CPUState *cs = CPU(cpu);
> uint64_t mask = (1ull << irq);
>
> //printf("irq:%d , level:%d\n",irq,level);
> if (level) {
> env->intsrc |= mask;
> cpu_interrupt(cs, CPU_INTERRUPT_HARD);
> } else {
> env->intsrc &= ~mask;
> if (env->intsrc == 0) {
> cpu_reset_interrupt(cs, CPU_INTERRUPT_HARD);
> }
> }
> }
>
bool stm8_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
> {
> //printf("%s\n",__func__);
> CPUClass *cpu_cc = CPU_GET_CLASS(cs);
> STM8CPU *cpu = STM8_CPU(cs);
> CPUSTM8State *env = >env;
> int idx = -1;
>
> /* Check interrupt */
> if (!cpu_interrupts_enabled(env)){
> qemu_log_mask(LOG_GUEST_ERROR,"[CPU] cpu_interrupts_enabled =
> false.\n");
> //return false;
> }
> if (interrupt_request & CPU_INTERRUPT_RESET)
> {
> idx = 0;
> cs->interrupt_request &= ~CPU_INTERRUPT_RESET;
> }
> if (interrupt_request & CPU_INTERRUPT_HARD) {
> if(env->intsrc != 0){
> idx = ctz32(env->intsrc);
> env->intsrc &= ~(1<
> cs->interrupt_request &= ~CPU_INTERRUPT_HARD;
> /* Map interrupt */
> idx = EXCP_INT(idx);
> }
> }
> if (idx >= 0) {
> cs->exception_index = idx;
> cpu_cc->do_interrupt(cs);
> return true;
> }
> return false;
> }
>
Peter Maydell 于2021年8月6日周五 下午6:16写道：
> On Fri, 6 Aug 2021 at 07:24, Duo jia wrote:
> > I am simulating a device. When an interrupt occurs, another interrupt
> > comes, and the second interrupt will not be triggered because the
> > first interrupt has not yet finished.
> >
> > I want to know whether qemu can detect whether the interrupt has been
> > executed, will there be a callback here?
> > Or how can I deal with this situation?
>
> End of interrupt handling is entirely dependent on what the
> guest hardware being emulated is. Usually the guest software
> will indicate "interrupt handled" back to the interrupt
> controller (perhaps by writing a register; depends on the
> interrupt controller), and the interrupt controller will
> then look at what the next highest priority pending interrupt
> is and signal that back to the CPU, or do nothing if there's
> no new interrupt. So the second interrupt will automatically
> be taken and handled once the first one has finished,
> as a result of this interrupt controller and guest OS
> interaction.
>
> The original device usually doesn't get told when this
> happens, and it doesn't need to know. For example, one common
> form of device interrupt is level-triggered. Here the device
> has some condition (perhaps "FIFO full") that causes an
> interrupt. So it raises its outbound IRQ line when the FIFO
> is full, and it doesn't lower it again until whatever the
> device specification says is the condition (eg when the
> guest reads from the FIFO, or if the guest writes to some
> 'clear interrupt' register on the device). It's the job of
> the guest software to make sure that when it gets an interrupt
> from the device that it handles it such that the device has
> been satisfied and lowered the interrupt.
>
> More rarely, some devices are specified to pulse their interrupt
> line when a condition occurs.
>
> In summary, you need to look at the specification of the device
> you're emulating to find out when and how it is supposed to
> raise or lower its interrupt line. ("I didn't get a second
> interrupt" bugs
RE: [PATCH v5 2/4] hw/arm/smmuv3: Update implementation of CFGI commands based on device SID
Hi,
> -Original Message-
> From: Eric Auger [mailto:eric.au...@redhat.com]
> Sent: Wednesday, September 01, 2021 8:05 PM
> To: Li, Chunming; chunming; peter.mayd...@linaro.org
> Cc: qemu-...@nongnu.org; qemu-devel@nongnu.org; Wen, Jianxian; Liu,
> Renwei
> Subject: Re: [PATCH v5 2/4] hw/arm/smmuv3: Update implementation of
> CFGI commands based on device SID
>
> Hi,
>
> On 9/1/21 8:51 AM, Li, Chunming wrote:
> >
> >> -Original Message-
> >> From: Eric Auger [mailto:eric.au...@redhat.com]
> >> Sent: Tuesday, August 31, 2021 10:02 PM
> >> To: chunming; peter.mayd...@linaro.org
> >> Cc: qemu-...@nongnu.org; qemu-devel@nongnu.org; Wen, Jianxian; Liu,
> >> Renwei; Li, Chunming
> >> Subject: Re: [PATCH v5 2/4] hw/arm/smmuv3: Update implementation of
> >> CFGI commands based on device SID
> >>
> >> Hi Chunming
> >>
> >> On 8/25/21 10:08 AM, chunming wrote:
> >>> From: chunming
> >>>
> >>> Replace "smmuv3_flush_config" with "g_hash_table_foreach_remove".
> >> this replacement may have a potential negative impact on the
> >> performance
> >> for PCIe support, which is the main use case: a unique
> >> g_hash_table_remove() is replaced by an iteration over all the
> config
> >> hash keys.
> >>
> >> I wonder if you couldn't just adapt smmu_iommu_mr() and it case this
> >> latter returns NULL for the current PCIe search, look up in the
> >> platform
> >> device list:
> >>
> >> peri_sdev_list?
> > I think there are 2 scenes:
> > 1. PCIe devices share same SID with peripheral devices.
> > 2. Multi peripheral devices share same SID.
> > If we search PCIe 1st then search peri_sdev_list, there are 2
> problems:
> > 1. The code is complex.
> > 2. We may need to search peri_sdev_list multi times. It may
> has performance impact.
> why multiple times? 1st you look for a PCIe RID and then you look for
> platform devices? Still I am dubious about the duplicate streamid case.
> what I wanted to emphasize is at the moment I do not have a clear view
> about your use case and I don't want to degrade the perf of the main
> use
> case to support a yet to be defined one ;-)
Yes, you are right. I agree with you after re-checking the code.
I will update it in next version.
> >
> > The CFGI commands are only called when the SMMU device is removed.
> > So we think there is no big performance impact.
>
> Nevertheless I think this is not a major issue indeed.
>
> Thanks
>
> Eric
> >
> >> Thanks
> >>
> >> Eric
> >>
> >>
> >>
> >>> "smmu_iommu_mr" function can't get MR according to SID for non
> >> PCI/PCIe devices.
> >>> Signed-off-by: chunming
> >>> ---
> >>> hw/arm/smmuv3.c | 35 ++--
> --
> >> -
> >>> include/hw/arm/smmu-common.h | 5 -
> >>> 2 files changed, 14 insertions(+), 26 deletions(-)
> >>>
> >>> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> >>> index 11d7fe8423..9f3f13fb8e 100644
> >>> --- a/hw/arm/smmuv3.c
> >>> +++ b/hw/arm/smmuv3.c
> >>> @@ -613,14 +613,6 @@ static SMMUTransCfg
> >> *smmuv3_get_config(SMMUDevice *sdev, SMMUEventInfo *event)
> >>> return cfg;
> >>> }
> >>>
> >>> -static void smmuv3_flush_config(SMMUDevice *sdev)
> >>> -{
> >>> -SMMUv3State *s = sdev->smmu;
> >>> -SMMUState *bc = >smmu_state;
> >>> -
> >>> -trace_smmuv3_config_cache_inv(smmu_get_sid(sdev));
> >>> -g_hash_table_remove(bc->configs, sdev);
> >>> -}
> >>>
> >>> static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion *mr,
> hwaddr
> >> addr,
> >>>IOMMUAccessFlags flag, int
> >> iommu_idx)
> >>> @@ -964,22 +956,18 @@ static int smmuv3_cmdq_consume(SMMUv3State
> *s)
> >>> case SMMU_CMD_CFGI_STE:
> >>> {
> >>> uint32_t sid = CMD_SID();
> >>> -IOMMUMemoryRegion *mr = smmu_iommu_mr(bs, sid);
> >>> -SMMUDevice *sdev;
> >>> +SMMUSIDRange sid_range;
> >>>
> >>> if (CMD_SSEC()) {
> >>> cmd_error = SMMU_CERROR_ILL;
> >>> break;
> >>> }
> >>>
> >>> -if (!mr) {
> >>> -break;
> >>> -}
> >>> -
> >>> +sid_range.start = sid;
> >>> +sid_range.end = sid;
> >>> trace_smmuv3_cmdq_cfgi_ste(sid);
> >>> -sdev = container_of(mr, SMMUDevice, iommu);
> >>> -smmuv3_flush_config(sdev);
> >>> -
> >>> +g_hash_table_foreach_remove(bs->configs,
> >> smmuv3_invalidate_ste,
> >>> +_range);
> >>> break;
> >>> }
> >>> case SMMU_CMD_CFGI_STE_RANGE: /* same as SMMU_CMD_CFGI_ALL
> >> */
> >>> @@ -1006,21 +994,18 @@ static int smmuv3_cmdq_consume(SMMUv3State
> *s)
> >>> case SMMU_CMD_CFGI_CD_ALL:
> >>> {
> >>> uint32_t sid = CMD_SID();
> >>> -IOMMUMemoryRegion *mr = smmu_iommu_mr(bs, sid);
> >>> -SMMUDevice *sdev;
> >>> +SMMUSIDRange sid_range;
> >>>
> >>>
Re: [PATCH] target/riscv: Fix satp write
On Thu, Sep 2, 2021 at 9:02 AM LIU Zhiwei wrote: > > > On 2021/9/1 下午9:05, Bin Meng wrote: > > On Wed, Sep 1, 2021 at 8:51 PM LIU Zhiwei wrote: > >> These variables should be target_ulong. If truncated to int, > >> the bool conditions they indicate will be wrong. > >> > >> As satp is very important for Linux, this bug almost fails every boot. > > Could you please describe which Linux configuration is broken? > > I use the image from: > > https://gitlab.com/c-sky/buildroot/-/jobs/1251564514/artifacts/browse/output/images/ > > > I have > > a 64-bit 5.10 kernel and it boots fine. > > The login is mostly OK for me. But the busybox can't run properly. Which kernel version is this? Could you please investigate and indicate in the commit message? I just tested current qemu-system-riscv64 can boot to Ubuntu 20.04 distro user space. Regards, Bin
Re: [PATCH v1 1/1] target/riscv: Update the ePMP CSR address
On Thu, Sep 2, 2021 at 8:40 AM Alistair Francis wrote: > > From: Alistair Francis > > Update the ePMP CSRs to match the 0.9.3 ePMP spec > https://github.com/riscv/riscv-tee/blob/61455747230a26002d741f64879dd78cc9689323/Smepmp/Smepmp.pdf > > Signed-off-by: Alistair Francis > --- > target/riscv/cpu_bits.h | 4 ++-- > target/riscv/cpu.c | 1 + > 2 files changed, 3 insertions(+), 2 deletions(-) > Reviewed-by: Bin Meng
RE: [PATCH v2] hw/arm/smmuv3: Simplify range invalidation
> -Original Message-
> From: Eric Auger [mailto:eric.au...@redhat.com]
> Sent: Wednesday, September 01, 2021 9:14 PM
> To: Liu, Renwei; Peter Maydell
> Cc: qemu-...@nongnu.org; qemu-devel@nongnu.org; Li, Chunming; Wen,
> Jianxian
> Subject: Re: [PATCH v2] hw/arm/smmuv3: Simplify range invalidation
>
> Hi,
>
> On 9/1/21 8:33 AM, Liu, Renwei wrote:
> >> -Original Message-
> >> From: Eric Auger [mailto:eric.au...@redhat.com]
> >> Sent: Tuesday, August 31, 2021 10:46 PM
> >> To: Liu, Renwei; Peter Maydell
> >> Cc: qemu-...@nongnu.org; qemu-devel@nongnu.org; Li, Chunming; Wen,
> >> Jianxian
> >> Subject: Re: [PATCH v2] hw/arm/smmuv3: Simplify range invalidation
> >>
> >> Hi Liu,
> >>
> >> On 8/23/21 9:50 AM, Liu, Renwei wrote:
> >>> Simplify range invalidation which can avoid to iterate over all
> >>> iotlb entries multi-times. For instance invalidations patterns like
> >>> "invalidate 32 4kB pages starting from 0xffacd000" need to iterate
> >> over
> >>> all iotlb entries 6 times (num_pages: 1, 2, 16, 8, 4, 1). It only
> >> needs
> >>> to iterate over all iotlb entries once with new implementation.
> >> This wouldn't work. This reverts commit
> >> 6d9cd115b9df ("hw/arm/smmuv3: Enforce invalidation on a power of two
> >> range")
> >> which is mandated for VFIO and virtio to work. IOTLB invalidations
> must
> >> be naturally aligned and with a power of 2 range, hence this
> iteration.
> >>
> >> Thanks
> >>
> >> Eric
> > Hi Eric,
> >
> > Could you try the patch firstly? I want to know whether it's failed
> > in your application scenario with this implementation.
> There are many test cases, virtio-pci, vhost, VFIO, ...
> > I agree with you that IOTLB entry must be naturally aligned and
> > with a power of 2 range. But we can invalidate multi IOTLB entries
> > in one iteration. We check the overlap between invalidation range
> > and IOTLB range, not check mask.
> This smmu_hash_remove_by_asid_iova() change only affects the internal
> SMMUv3 IOTLB hash table lookup. However there are also IOTLB
> invalidation notifications sent to components who registered notifiers,
> ie. smmuv3_notify_iova path.
> > The final result is same with
> > your implementation (split to multi times with a power of 2 range).
> > I wonder why we can't implement it directly when the application can
> > send an invalidation command with a non power of 2 range.
> > We have tested it in our application scenario and not find any fail.
> Assume the driver invalidates 5 * 4kB pages =0x5000 range. Without the
> loop you removed
>
> in smmuv3_notify_iova() event.entry.addr_mask = num_pages * (1 <<
> granule) - 1 = 0x4FFF. This addr_mask is an invalid mask
> this entry is passed to components who registered invalidation
> notifiers
> such as vhost or vfio. for instance in VFIO you have '&' ops on the
> addr_mask.
> addr_mask is expected to be a mask of a power of 2 range.
>
> Does it clarify?
>
> Thanks
>
> Eric
Hi Eric
Got it, thanks a lot for your clarification.
I don't consider the further notifier from vhost or vfio indeed,
because they are not registered in our application scenario.
Let's keep the previous implementation and ignore this patch.
Thanks
Renwei Liu
> >
> > In addition, from the code implementation, smmu_iotlb_inv_iova()
> > should be OK. In another call smmuv3_inv_notifiers_iova() ->
> > smmuv3_notify_iova() -> memory_region_notify_iommu_one(),
> > it also checks range overlap. So it should be OK if the range
> > is not a power of 2.
> >
> > Could you take a look at it again?
> >
> > Thanks
> > Renwei Liu
Re: [PATCH] target/riscv: Fix satp write
On 2021/9/1 下午9:05, Bin Meng wrote:
On Wed, Sep 1, 2021 at 8:51 PM LIU Zhiwei wrote:
These variables should be target_ulong. If truncated to int,
the bool conditions they indicate will be wrong.
As satp is very important for Linux, this bug almost fails every boot.
Could you please describe which Linux configuration is broken?
I use the image from:
https://gitlab.com/c-sky/buildroot/-/jobs/1251564514/artifacts/browse/output/images/
I have
a 64-bit 5.10 kernel and it boots fine.
The login is mostly OK for me. But the busybox can't run properly.
Thanks,
Zhiwei
Please add:
Fixes: 419ddf00ed78 ("target/riscv: Remove the hardcoded SATP_MODE macro")
Signed-off-by: LIU Zhiwei
---
target/riscv/csr.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 50a2c3a3b4..ba9818f6a5 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -986,7 +986,7 @@ static RISCVException read_satp(CPURISCVState *env, int
csrno,
static RISCVException write_satp(CPURISCVState *env, int csrno,
target_ulong val)
{
-int vm, mask, asid;
+target_ulong vm, mask, asid;
if (!riscv_feature(env, RISCV_FEATURE_MMU)) {
return RISCV_EXCP_NONE;
Reviewed-by: Bin Meng
[PATCH v1 1/1] target/riscv: Update the ePMP CSR address
From: Alistair Francis
Update the ePMP CSRs to match the 0.9.3 ePMP spec
https://github.com/riscv/riscv-tee/blob/61455747230a26002d741f64879dd78cc9689323/Smepmp/Smepmp.pdf
Signed-off-by: Alistair Francis
---
target/riscv/cpu_bits.h | 4 ++--
target/riscv/cpu.c | 1 +
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
index 7330ff5a19..ce9dcc030c 100644
--- a/target/riscv/cpu_bits.h
+++ b/target/riscv/cpu_bits.h
@@ -210,8 +210,8 @@
#define CSR_MTVAL2 0x34b
/* Enhanced Physical Memory Protection (ePMP) */
-#define CSR_MSECCFG 0x390
-#define CSR_MSECCFGH0x391
+#define CSR_MSECCFG 0x747
+#define CSR_MSECCFGH0x757
/* Physical Memory Protection */
#define CSR_PMPCFG0 0x3a0
#define CSR_PMPCFG1 0x3a1
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 1a2b03d579..8ecb8df780 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -599,6 +599,7 @@ static Property riscv_cpu_properties[] = {
DEFINE_PROP_UINT16("elen", RISCVCPU, cfg.elen, 64),
DEFINE_PROP_BOOL("mmu", RISCVCPU, cfg.mmu, true),
DEFINE_PROP_BOOL("pmp", RISCVCPU, cfg.pmp, true),
+/* ePMP 0.9.3 */
DEFINE_PROP_BOOL("x-epmp", RISCVCPU, cfg.epmp, false),
DEFINE_PROP_UINT64("resetvec", RISCVCPU, cfg.resetvec, DEFAULT_RSTVEC),
--
2.31.1
[PATCH 1/1] hw/arm/aspeed: Add Fuji machine type
From: Peter Delevoryas
This adds a new machine type "fuji-bmc" that's equivalent to
"ast2600-evb" except that it uses MAC3 and UART1. It might be
appropriate to change other aspects of this machine type in the future,
but so far this is all the specificity necessary to get a Fuji OpenBMC
image booting and networking within QEMU.
Here's a link to the device tree:
https://github.com/facebook/openbmc-uboot/blob/openbmc/helium/v2019.04/arch/arm/dts/aspeed-bmc-facebook-fuji.dts
I tested this by building a Fuji image from Facebook's OpenBMC repo,
booting it, and ssh'ing into it.
git clone https://github.com/facebook/openbmc
cd openbmc
./sync_yocto.sh
source openbmc-init-build-env fuji build-fuji
bitbake fuji-image
dd if=/dev/zero of=/tmp/fuji.mtd bs=1M count=128
dd if=./tmp/deploy/images/fuji/flash-fuji of=/tmp/fuji.mtd \
bs=1k conv=notrunc
git clone --branch aspeed-next https://github.com/peterdelevoryas/qemu
cd qemu
./configure --target-list=arm-softmmu --disable-vnc
make -j $(nproc)
./build/arm-softmmu/qemu-system-arm \
-machine fuji-bmc \
-drive file=/tmp/fuji.mtd,format=raw,if=mtd \
-serial stdio \
-nic user,hostfwd=::-:22
U-Boot 2019.04 fuji-b9c651226b (Aug 25 2021 - 17:27:02 +)
SOC: AST2600-A3
eSPI Mode: SIO:Enable : SuperIO-2e
Eth: MAC0: RGMII, MAC1: RGMII, MAC2: RGMII, MAC3: RGMII
Model: Aspeed BMC
DRAM: 896 MiB (capacity:1024 MiB, VGA:64 MiB, ECC:on, ECC size:896 MiB)
MMC: emmc_slot0@100: 0
...
sshpass -p 0penBmc ssh root@localhost -p
Warning: Permanently added '[localhost]:' (ECDSA) to the list of known
Last login: Fri Mar 9 04:36:31 2018
root@bmc-oob:~# exit
logout
Connection to localhost closed.
I also created a Github release with the Fuji mtd image and added an
acceptance test utilizing it.
https://github.com/peterdelevoryas/openbmc/releases/tag/fuji-v0.1-alpha
Signed-off-by: Peter Delevoryas
---
hw/arm/aspeed.c| 14 ++
tests/acceptance/boot_linux_console.py | 25 +
2 files changed, 39 insertions(+)
diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index 7a9459340c..3b8b660a34 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -1070,6 +1070,16 @@ static void
aspeed_machine_rainier_class_init(ObjectClass *oc, void *data)
aspeed_soc_num_cpus(amc->soc_name);
};
+static void aspeed_machine_fuji_class_init(ObjectClass *oc, void *data)
+{
+MachineClass *mc = MACHINE_CLASS(oc);
+AspeedMachineClass *amc = ASPEED_MACHINE_CLASS(oc);
+
+mc->desc = "Facebook Fuji BMC (Cortex-A7)";
+amc->macs_mask = ASPEED_MAC3_ON;
+amc->uart_default = ASPEED_DEV_UART1;
+};
+
static const TypeInfo aspeed_machine_types[] = {
{
.name = MACHINE_TYPE_NAME("palmetto-bmc"),
@@ -1119,6 +1129,10 @@ static const TypeInfo aspeed_machine_types[] = {
.name = MACHINE_TYPE_NAME("rainier-bmc"),
.parent= TYPE_ASPEED_MACHINE,
.class_init= aspeed_machine_rainier_class_init,
+}, {
+.name = MACHINE_TYPE_NAME("fuji-bmc"),
+.parent= MACHINE_TYPE_NAME("ast2600-evb"),
+.class_init= aspeed_machine_fuji_class_init,
}, {
.name = TYPE_ASPEED_MACHINE,
.parent= TYPE_MACHINE,
diff --git a/tests/acceptance/boot_linux_console.py
b/tests/acceptance/boot_linux_console.py
index 5248c8097d..63d32f6743 100644
--- a/tests/acceptance/boot_linux_console.py
+++ b/tests/acceptance/boot_linux_console.py
@@ -1143,6 +1143,31 @@ def test_arm_ast2600_debian(self):
self.wait_for_console_pattern("SMP: Total of 2 processors activated")
self.wait_for_console_pattern("No filesystem could mount root")
+def test_arm_ast2600_fuji_openbmc(self):
+"""
+:avocado: tags=arch:arm
+:avocado: tags=machine:fuji-bmc
+"""
+
+image_url =
('https://github.com/peterdelevoryas/openbmc/releases/download/'
+ 'fuji-v0.1-alpha/fuji.mtd')
+image_hash =
'36dd945a2ee34694684b5f3e7351517598bb39d8b6899c71bbd23791b42e082e'
+image_path = self.fetch_asset(image_url, asset_hash=image_hash,
+ algorithm='sha256')
+
+self.vm.set_console()
+self.vm.add_args('-drive', 'file=' + image_path + ',if=mtd,format=raw',
+ '-net', 'nic')
+self.vm.launch()
+
+self.wait_for_console_pattern("U-Boot 2019.04")
+self.wait_for_console_pattern("## Loading kernel from FIT Image at
2010")
+self.wait_for_console_pattern("Starting kernel ...")
+self.wait_for_console_pattern("Booting Linux on physical CPU 0xf00")
+self.wait_for_console_pattern(
+"aspeed-smc 1e62.spi: read control register: 203b0041")
+self.wait_for_console_pattern("ftgmac100 1e69.ftgmac eth0: irq ")
+
def test_m68k_mcf5208evb(self):
"""
:avocado: tags=arch:m68k
--
2.30.2
[PATCH 0/1] hw/arm/aspeed: Add Fuji machine type
From: Peter Delevoryas Adding a new Aspeed AST2600 machine type, uses MAC3 for ethernet1 and UART1 for the serial console, which is different than the existing ast2600-evb. Otherwise though, my usage so far hasn't required a different set of hardware strap registers or anything, so I just inherited the rest of the configuration from the ast2600-evb. If preferred, I can eliminate the inheritance and just declare Fuji completely independently. I included more info in the commit message, but just for convenience, here's the DTS link and a link to an image I'm providing for the acceptance test. https://github.com/facebook/openbmc-uboot/blob/openbmc/helium/v2019.04/arch/arm/dts/aspeed-bmc-facebook-fuji.dts https://github.com/peterdelevoryas/openbmc/releases/download/fuji-v0.1-alpha/fuji.mtd I'm not sure exactly what the requirements are on the image url provided, or the requirements of the image itself. The existing OpenBMC acceptance tests mostly use images from the Linux Foundation repository's releases, e.g. https://github.com/openbmc/openbmc/releases/download/2.9.0/obmc-phosphor-image-romulus.static.mtd Although, I do see one image from a regular user's repo: https://github.com/hskinnemoen/openbmc/releases/download/20200711-gsj-qemu-0/obmc-phosphor-image-gsj.static.mtd.gz So maybe it's not that unreasonable? I also might be able to organize a release link on the official Facebook OpenBMC Github. As far as the actual requirements of the acceptance test, I didn't use the existing "do_test_arm_aspeed" because Fuji was sufficiently different (different U-Boot version, kernel address, CPU, SPI control register, doesn't use systemd), but I could try unifying them if there's interest or suggestions on how to do so. By the way, Fuji takes a long time to boot on my Macbook: (19/38) tests/acceptance/boot_linux_console.py:BootLinuxConsole.test_arm_ast2600_fuji_openbmc: PASS (85.13 s) The next longest boot time is <20 seconds: 17/38) tests/acceptance/boot_linux_console.py:BootLinuxConsole.test_arm_ast2500_romulus_openbmc_v2_9_0: PASS (17.46 s) Not sure if that's a problem for the CIT infra time or not. Thanks, Peter Peter Delevoryas (1): hw/arm/aspeed: Add Fuji machine type hw/arm/aspeed.c| 14 ++ tests/acceptance/boot_linux_console.py | 25 + 2 files changed, 39 insertions(+) -- 2.30.2
Re: [RFC 05/10] hw/mos6522: Don't clear T1 interrupt flag on latch write
On Wed, 1 Sep 2021, Laurent Vivier wrote:
> Le 26/08/2021 à 07:21, Finn Thain a écrit :
> > On Wed, 25 Aug 2021, Mark Cave-Ayland wrote:
> >
> >> On 24/08/2021 11:09, Finn Thain wrote:
> >>
> >>> The Synertek datasheet says, "A write to T1L-H loads an 8-bit count value
> >>> into the latch. A read of T1L-H transfers the contents of the latch to
> >>> the data bus. Neither operation has an affect [sic] on the interrupt
> >>> flag."
> >>>
> >>> Signed-off-by: Finn Thain
> >>> ---
> >>> hw/misc/mos6522.c | 1 -
> >>> 1 file changed, 1 deletion(-)
> >>>
> >>> diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
> >>> index c0d6bee4cc..8991f4 100644
> >>> --- a/hw/misc/mos6522.c
> >>> +++ b/hw/misc/mos6522.c
> >>> @@ -313,7 +313,6 @@ void mos6522_write(void *opaque, hwaddr addr, uint64_t
> >>> val, unsigned size)
> >>> break;
> >>> case VIA_REG_T1LH:
> >>> s->timers[0].latch = (s->timers[0].latch & 0xff) | (val << 8);
> >>> -s->ifr &= ~T1_INT;
> >>> break;
> >>> case VIA_REG_T2CL:
> >>> s->timers[1].latch = (s->timers[1].latch & 0xff00) | val;
> >>
> >> Hmmm. The reference document I used for QEMU's 6522 device is at
> >> http://archive.6502.org/datasheets/mos_6522_preliminary_nov_1977.pdf and
> >> according to page 6 and the section "Writing the Timer 1 Registers"
> >> writing to
> >> the high byte of the latch does indeed clear the T1 interrupt flag.
> >>
> >> Side note: there is reference in Gary Davidian's excellent CHM video that
> >> 6522s obtained from different manufacturers had different behaviours, and
> >> there are also web pages mentioning that 6522s integrated as part of other
> >> silicon e.g. IOSB/CUDA also had their own bugs... :/
> >>
> >
> > The MOS document you've cited is much older than the Synertek and Rockwell
> > devices. The datasheets for the Synertek and Rockwell parts disagree with
> > MOS about T1LH behaviour. Apple certainly used SY6522 devices in my Mac II
> > and I'd assumed Apple would have used compatible logic cores in the custom
> > ICs found in later models. But I don't really trust assumptions and
> > datasheets so I wrote the Linux patch below and ran it on my Quadra 630.
> >
> > diff --git a/arch/m68k/mac/via.c b/arch/m68k/mac/via.c
> > index 3d11d6219cdd..ed41f6ae2bf2 100644
> > --- a/arch/m68k/mac/via.c
> > +++ b/arch/m68k/mac/via.c
> > @@ -634,3 +634,27 @@ static u64 mac_read_clk(struct clocksource *cs)
> >
> > return ticks;
> > }
> > +
> > +static int baz(void)
> > +{
> > + u8 a, b, c;
> > +
> > + local_irq_disable();
> > +
> > + while (!(via1[vIFR] & VIA_TIMER_1_INT))
> > + continue;
> > + a = via1[vIFR] & VIA_TIMER_1_INT;
> > + via1[vT1LH] = via1[vT1LH];
> > + b = via1[vIFR] & VIA_TIMER_1_INT;
> > + via1[vT1LL] = via1[vT1LL];
> > + c = via1[vIFR] & VIA_TIMER_1_INT;
> > +
> > + printk("a == %2x\n", a);
> > + printk("b == %2x\n", b);
> > + printk("c == %2x\n", c);
> > +
> > + local_irq_enable();
> > +
> > + return 0;
> > +}
> > +late_initcall(baz);
> >
> > Based on the Synertek datasheet* one would expect to see b equal to a but
> > I got this result instead:
> >
> > [ 10.45] a == 40
> > [ 10.45] b == 0
> > [ 10.45] c == 0
> >
> > This amounts to a MOS design flaw and I doubt that this result from my
> > Quadra 630 would apply to other Mac models. So it would be great to see
> > the output from a Quadra 800. But until then, let's disregard this patch.
> >
> > * http://archive.6502.org/datasheets/synertek_sy6522.pdf
> >
>
> Tested on my Quadra 800:
>
> [4.73] a == 40
> [4.73] b == 0
> [4.73] c == 0
>
Much appreciated. I will drop this patch.
USB-MSD non-functional after merging v5.1 to v6.x (seems to be internal USB stack issue?)
Hello! Sending to the list because I was directed here after asking on IRC. Background: I've forked QEMU for a project and am in the process of implementing a more complete STM32F4xx stack as a secondary task. To resolve recent GCC11 build errors, I attempted to merge with upstream QEMU v6 (coming from a late 2020 v5.x.x) and to my surprise, USB mass storage is no longer functional. A few packets are exchanged but at some point I At first I suspected the issue was my own code (fair, my implementations are in varying states of completeness. The STM32F4 USB controller has many similarities to (but is not quite the same as) the DWC2 USB stack). I decided to bisect the merge in order to identify the commit that causes the issue - and much to my surprise, it is this particular commit: https://github.com/qemu/qemu/commit/bbd8323d3196c9979385cba1b8b38859836e63c3 Given this doesn't seem to be anything more than a relocation of declarations (and I don't even use any of these types directly in my code), this would seem to suggest an internal issue in linking or memory initialization. I'm happy to assist in debugging this where I can but I'm hoping someone more knowledgeable about the QEMU USB innards might be able to point me to an area to start digging since the change seems entirely orthogonal to the actual problem and could be just about anywhere. The command line I'm using is as follows: ./qemu-system-buddy -machine prusa-mini -kernel firmware.bin -device usb-storage,drive=usbst -drive id=usbst,file=SDcard.bin where SDCard.bin is a bog-standard image of a FAT32 partition. (using VFAT folder emulation is similarly non-functional). Unfortunately the device does not support anything other than MSD so I cannot check functionality with other USB devices. I've been told this problem is not unique to my own development setup, and a cursory investigation reveals one of the symptoms is a divergence in the size of the incoming USB packets. (I'm hoping to set up a more detailed packet capture when I have more spare time this weekend). For example, the working version I will see some initial setup packets be exchanged, then a few packets of size 1, 36, 13, 13, 14, etc as the ARM firmware talks to the device. (Back when I implemented the USB stack I spent a lot of time debugging and comparing wireshark captures so I'm reasonably confident the USB handling code is correct since the firmware being run is doing full on FAT filesystem support). After merging the offending commit, I see the initial setup and then a series of packets of size 1, 36, 16, 512, and then nothing further. (Internally on the firmware the USB bus gets stuck because this last packet of size 512 is obviously junk and a symptom of the issue.). --enable-sanitizers revealed only a minor bug elsewhere that was unrelated to the issue (and did not resolve it when fixed) The project itself is here: https://github.com/vintagepc/MINI404 with all of my custom implementations temporarily residing in hw/arm/prusa/stm32f407 (see footnote/P.S.) Thanks in advance for any assistance! VintagePC. P.S. - Yes, I recognize it's not organized and formatted entirely in the same style as the rest of QEMU and probably violates a few style guide items. As this is a spare time project (and because I am also leveraging prior work from non-upstream sources) I need to be as efficient as possible in making changes and being able to debug things. Longer term... yes, I am absolutely supportive of getting STM32F4 support upstream because I know there is significant value here. But as it is right now the parts are not functional/polished enough that I feel comfortable doing so - and I just don't have the bandwidth to spend time on that in addition to the project itself. In the long term once the SOC implementation is more complete, I definitely hope to be able to shift focus to refactoring and making what I believe to be sufficiently functional implementations ready for upstream submissions. (And if someone reading this is keen and willing to help, feel free to contact me off-list! to collaborate and/or discuss what needs to happen to make it submittable!)
[PATCH v1] kvm: unsigned datatype in ioctl wrapper
Change the data type of the ioctl _request_ argument from 'int' to
'unsigned long' for the kvm ioctl wrappers.
POSIX defines the request argument as 'int' but the glibc defines the ioctl
call as follows
int ioctl (int fd, unsigned long int request, ...);
Requests with the 0x8000_ bit set will be sign-extended.
Fortunately the Linux Kernel truncates the upper 32bit of the request on
64bit machines [1].
On x86_64 one such example is the KVM_IRQ_LINE_STATUS request.
But requests with the _IOC_READ direction bit set, will have the high
bit set.
[1]: https://sourceware.org/bugzilla/show_bug.cgi?id=14362
Signed-off-by: Johannes Stoelp
---
v1:
- Changed ioctl request from 'unsigned' -> 'unsigned long' in kvm ioctl
wrapper.
v0:
- Changed ioctl request from 'int' -> 'unsigned' in kvm ioctl wrapper.
- L: https://lists.nongnu.org/archive/html/qemu-devel/2021-08/msg00945.html
accel/kvm/kvm-all.c| 10 +-
accel/kvm/trace-events | 8
include/sysemu/kvm.h | 8
3 files changed, 13 insertions(+), 13 deletions(-)
diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 0125c17edb..5f9379f3cc 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -127,7 +127,7 @@ struct KVMState
/* The man page (and posix) say ioctl numbers are signed int, but
* they're not. Linux, glibc and *BSD all treat ioctl numbers as
* unsigned, and treating them as signed here can break things */
-unsigned irq_set_ioctl;
+unsigned long irq_set_ioctl;
unsigned int sigmask_len;
GHashTable *gsimap;
#ifdef KVM_CAP_IRQ_ROUTING
@@ -2967,7 +2967,7 @@ int kvm_cpu_exec(CPUState *cpu)
return ret;
}
-int kvm_ioctl(KVMState *s, int type, ...)
+int kvm_ioctl(KVMState *s, unsigned long type, ...)
{
int ret;
void *arg;
@@ -2985,7 +2985,7 @@ int kvm_ioctl(KVMState *s, int type, ...)
return ret;
}
-int kvm_vm_ioctl(KVMState *s, int type, ...)
+int kvm_vm_ioctl(KVMState *s, unsigned long type, ...)
{
int ret;
void *arg;
@@ -3003,7 +3003,7 @@ int kvm_vm_ioctl(KVMState *s, int type, ...)
return ret;
}
-int kvm_vcpu_ioctl(CPUState *cpu, int type, ...)
+int kvm_vcpu_ioctl(CPUState *cpu, unsigned long type, ...)
{
int ret;
void *arg;
@@ -3021,7 +3021,7 @@ int kvm_vcpu_ioctl(CPUState *cpu, int type, ...)
return ret;
}
-int kvm_device_ioctl(int fd, int type, ...)
+int kvm_device_ioctl(int fd, unsigned long type, ...)
{
int ret;
void *arg;
diff --git a/accel/kvm/trace-events b/accel/kvm/trace-events
index 399aaeb0ec..a1905fe985 100644
--- a/accel/kvm/trace-events
+++ b/accel/kvm/trace-events
@@ -1,11 +1,11 @@
# See docs/devel/tracing.rst for syntax documentation.
# kvm-all.c
-kvm_ioctl(int type, void *arg) "type 0x%x, arg %p"
-kvm_vm_ioctl(int type, void *arg) "type 0x%x, arg %p"
-kvm_vcpu_ioctl(int cpu_index, int type, void *arg) "cpu_index %d, type 0x%x,
arg %p"
+kvm_ioctl(unsigned long type, void *arg) "type 0x%lx, arg %p"
+kvm_vm_ioctl(unsigned long type, void *arg) "type 0x%lx, arg %p"
+kvm_vcpu_ioctl(int cpu_index, unsigned long type, void *arg) "cpu_index %d,
type 0x%lx, arg %p"
kvm_run_exit(int cpu_index, uint32_t reason) "cpu_index %d, reason %d"
-kvm_device_ioctl(int fd, int type, void *arg) "dev fd %d, type 0x%x, arg %p"
+kvm_device_ioctl(int fd, unsigned long type, void *arg) "dev fd %d, type
0x%lx, arg %p"
kvm_failed_reg_get(uint64_t id, const char *msg) "Warning: Unable to retrieve
ONEREG %" PRIu64 " from KVM: %s"
kvm_failed_reg_set(uint64_t id, const char *msg) "Warning: Unable to set
ONEREG %" PRIu64 " to KVM: %s"
kvm_init_vcpu(int cpu_index, unsigned long arch_cpu_id) "index: %d id: %lu"
diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
index a1ab1ee12d..d0f354a897 100644
--- a/include/sysemu/kvm.h
+++ b/include/sysemu/kvm.h
@@ -251,11 +251,11 @@ int kvm_on_sigbus(int code, void *addr);
/* internal API */
-int kvm_ioctl(KVMState *s, int type, ...);
+int kvm_ioctl(KVMState *s, unsigned long type, ...);
-int kvm_vm_ioctl(KVMState *s, int type, ...);
+int kvm_vm_ioctl(KVMState *s, unsigned long type, ...);
-int kvm_vcpu_ioctl(CPUState *cpu, int type, ...);
+int kvm_vcpu_ioctl(CPUState *cpu, unsigned long type, ...);
/**
* kvm_device_ioctl - call an ioctl on a kvm device
@@ -264,7 +264,7 @@ int kvm_vcpu_ioctl(CPUState *cpu, int type, ...);
*
* Returns: -errno on error, nonnegative on success
*/
-int kvm_device_ioctl(int fd, int type, ...);
+int kvm_device_ioctl(int fd, unsigned long type, ...);
/**
* kvm_vm_check_attr - check for existence of a specific vm attribute
--
2.32.0
[PATCH v5 5/5] virtio-gpu: Add gl_flushed callback
Adding this callback provides a way to resume the processing of
cmds in fenceq and cmdq that were not processed because the UI
was waiting on a fence and blocked cmd processing.
Cc: Gerd Hoffmann
Reviewed-by: Gerd Hoffmann
Signed-off-by: Vivek Kasireddy
---
hw/display/virtio-gpu.c | 32 ++--
1 file changed, 30 insertions(+), 2 deletions(-)
diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index 72da5bf500..182e0868b0 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -985,8 +985,10 @@ void virtio_gpu_simple_process_cmd(VirtIOGPU *g,
break;
}
if (!cmd->finished) {
-virtio_gpu_ctrl_response_nodata(g, cmd, cmd->error ? cmd->error :
-VIRTIO_GPU_RESP_OK_NODATA);
+if (!g->parent_obj.renderer_blocked) {
+virtio_gpu_ctrl_response_nodata(g, cmd, cmd->error ? cmd->error :
+VIRTIO_GPU_RESP_OK_NODATA);
+}
}
}
@@ -1042,6 +1044,30 @@ void virtio_gpu_process_cmdq(VirtIOGPU *g)
g->processing_cmdq = false;
}
+static void virtio_gpu_process_fenceq(VirtIOGPU *g)
+{
+struct virtio_gpu_ctrl_command *cmd, *tmp;
+
+QTAILQ_FOREACH_SAFE(cmd, >fenceq, next, tmp) {
+trace_virtio_gpu_fence_resp(cmd->cmd_hdr.fence_id);
+virtio_gpu_ctrl_response_nodata(g, cmd, VIRTIO_GPU_RESP_OK_NODATA);
+QTAILQ_REMOVE(>fenceq, cmd, next);
+g_free(cmd);
+g->inflight--;
+if (virtio_gpu_stats_enabled(g->parent_obj.conf)) {
+fprintf(stderr, "inflight: %3d (-)\r", g->inflight);
+}
+}
+}
+
+static void virtio_gpu_handle_gl_flushed(VirtIOGPUBase *b)
+{
+VirtIOGPU *g = container_of(b, VirtIOGPU, parent_obj);
+
+virtio_gpu_process_fenceq(g);
+virtio_gpu_process_cmdq(g);
+}
+
static void virtio_gpu_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
{
VirtIOGPU *g = VIRTIO_GPU(vdev);
@@ -1400,10 +1426,12 @@ static void virtio_gpu_class_init(ObjectClass *klass,
void *data)
DeviceClass *dc = DEVICE_CLASS(klass);
VirtioDeviceClass *vdc = VIRTIO_DEVICE_CLASS(klass);
VirtIOGPUClass *vgc = VIRTIO_GPU_CLASS(klass);
+VirtIOGPUBaseClass *vgbc = >parent;
vgc->handle_ctrl = virtio_gpu_handle_ctrl;
vgc->process_cmd = virtio_gpu_simple_process_cmd;
vgc->update_cursor_data = virtio_gpu_update_cursor_data;
+vgbc->gl_flushed = virtio_gpu_handle_gl_flushed;
vdc->realize = virtio_gpu_device_realize;
vdc->reset = virtio_gpu_reset;
--
2.30.2
[PATCH v5 4/5] ui/gtk-egl: Wait for the draw signal for dmabuf blobs
Instead of immediately drawing and submitting, queue and wait
for the draw signal if the dmabuf submitted is a blob.
Cc: Gerd Hoffmann
Reviewed-by: Gerd Hoffmann
Signed-off-by: Vivek Kasireddy
---
include/ui/gtk.h | 2 ++
ui/gtk-egl.c | 15 +++
ui/gtk.c | 2 +-
3 files changed, 18 insertions(+), 1 deletion(-)
diff --git a/include/ui/gtk.h b/include/ui/gtk.h
index 43854f3509..7d22affd38 100644
--- a/include/ui/gtk.h
+++ b/include/ui/gtk.h
@@ -182,6 +182,8 @@ void gd_egl_cursor_dmabuf(DisplayChangeListener *dcl,
uint32_t hot_x, uint32_t hot_y);
void gd_egl_cursor_position(DisplayChangeListener *dcl,
uint32_t pos_x, uint32_t pos_y);
+void gd_egl_flush(DisplayChangeListener *dcl,
+ uint32_t x, uint32_t y, uint32_t w, uint32_t h);
void gd_egl_scanout_flush(DisplayChangeListener *dcl,
uint32_t x, uint32_t y, uint32_t w, uint32_t h);
void gtk_egl_init(DisplayGLMode mode);
diff --git a/ui/gtk-egl.c b/ui/gtk-egl.c
index 2c68696d9f..737e7b90d4 100644
--- a/ui/gtk-egl.c
+++ b/ui/gtk-egl.c
@@ -304,6 +304,21 @@ void gd_egl_scanout_flush(DisplayChangeListener *dcl,
eglSwapBuffers(qemu_egl_display, vc->gfx.esurface);
}
+void gd_egl_flush(DisplayChangeListener *dcl,
+ uint32_t x, uint32_t y, uint32_t w, uint32_t h)
+{
+VirtualConsole *vc = container_of(dcl, VirtualConsole, gfx.dcl);
+GtkWidget *area = vc->gfx.drawing_area;
+
+if (vc->gfx.guest_fb.dmabuf) {
+graphic_hw_gl_block(vc->gfx.dcl.con, true);
+gtk_widget_queue_draw_area(area, x, y, w, h);
+return;
+}
+
+gd_egl_scanout_flush(>gfx.dcl, x, y, w, h);
+}
+
void gtk_egl_init(DisplayGLMode mode)
{
GdkDisplay *gdk_display = gdk_display_get_default();
diff --git a/ui/gtk.c b/ui/gtk.c
index 5105c0a33f..b0564d80c1 100644
--- a/ui/gtk.c
+++ b/ui/gtk.c
@@ -637,7 +637,7 @@ static const DisplayChangeListenerOps dcl_egl_ops = {
.dpy_gl_scanout_dmabuf = gd_egl_scanout_dmabuf,
.dpy_gl_cursor_dmabuf= gd_egl_cursor_dmabuf,
.dpy_gl_cursor_position = gd_egl_cursor_position,
-.dpy_gl_update = gd_egl_scanout_flush,
+.dpy_gl_update = gd_egl_flush,
.dpy_gl_release_dmabuf = gd_gl_release_dmabuf,
.dpy_has_dmabuf = gd_has_dmabuf,
};
--
2.30.2
[PATCH v5 2/5] ui/egl: Add egl helpers to help with synchronization
These egl helpers would be used for creating and waiting on
a sync object.
Cc: Gerd Hoffmann
Reviewed-by: Gerd Hoffmann
Signed-off-by: Vivek Kasireddy
---
include/ui/console.h | 2 ++
include/ui/egl-helpers.h | 2 ++
ui/egl-helpers.c | 26 ++
3 files changed, 30 insertions(+)
diff --git a/include/ui/console.h b/include/ui/console.h
index 3be21497a2..45ec129174 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -168,6 +168,8 @@ typedef struct QemuDmaBuf {
uint64_t modifier;
uint32_t texture;
bool y0_top;
+void *sync;
+int fence_fd;
} QemuDmaBuf;
typedef struct DisplayState DisplayState;
diff --git a/include/ui/egl-helpers.h b/include/ui/egl-helpers.h
index f1bf8f97fc..2c3ba92b53 100644
--- a/include/ui/egl-helpers.h
+++ b/include/ui/egl-helpers.h
@@ -45,6 +45,8 @@ int egl_get_fd_for_texture(uint32_t tex_id, EGLint *stride,
EGLint *fourcc,
void egl_dmabuf_import_texture(QemuDmaBuf *dmabuf);
void egl_dmabuf_release_texture(QemuDmaBuf *dmabuf);
+void egl_dmabuf_create_sync(QemuDmaBuf *dmabuf);
+void egl_dmabuf_create_fence(QemuDmaBuf *dmabuf);
#endif
diff --git a/ui/egl-helpers.c b/ui/egl-helpers.c
index 6d0cb2b5cb..d8986b0a7f 100644
--- a/ui/egl-helpers.c
+++ b/ui/egl-helpers.c
@@ -76,6 +76,32 @@ void egl_fb_setup_for_tex(egl_fb *fb, int width, int height,
GL_TEXTURE_2D, fb->texture, 0);
}
+void egl_dmabuf_create_sync(QemuDmaBuf *dmabuf)
+{
+EGLSyncKHR sync;
+
+if (epoxy_has_egl_extension(qemu_egl_display,
+"EGL_KHR_fence_sync") &&
+epoxy_has_egl_extension(qemu_egl_display,
+"EGL_ANDROID_native_fence_sync")) {
+sync = eglCreateSyncKHR(qemu_egl_display,
+EGL_SYNC_NATIVE_FENCE_ANDROID, NULL);
+if (sync != EGL_NO_SYNC_KHR) {
+dmabuf->sync = sync;
+}
+}
+}
+
+void egl_dmabuf_create_fence(QemuDmaBuf *dmabuf)
+{
+if (dmabuf->sync) {
+dmabuf->fence_fd = eglDupNativeFenceFDANDROID(qemu_egl_display,
+ dmabuf->sync);
+eglDestroySyncKHR(qemu_egl_display, dmabuf->sync);
+dmabuf->sync = NULL;
+}
+}
+
void egl_fb_setup_new_tex(egl_fb *fb, int width, int height)
{
GLuint texture;
--
2.30.2
[PATCH v5 3/5] ui: Create sync objects and fences only for blobs
Create sync objects and fences only for dmabufs that are blobs. Once a
fence is created (after glFlush) and is signalled,
graphic_hw_gl_flushed() will be called and virtio-gpu cmd processing
will be resumed.
Cc: Gerd Hoffmann
Signed-off-by: Vivek Kasireddy
---
hw/display/virtio-gpu-udmabuf.c | 1 +
include/ui/console.h| 1 +
include/ui/egl-helpers.h| 1 +
include/ui/gtk.h| 1 +
ui/gtk-egl.c| 20
ui/gtk-gl-area.c| 20
ui/gtk.c| 13 +
7 files changed, 57 insertions(+)
diff --git a/hw/display/virtio-gpu-udmabuf.c b/hw/display/virtio-gpu-udmabuf.c
index 3c01a415e7..c6f7f58784 100644
--- a/hw/display/virtio-gpu-udmabuf.c
+++ b/hw/display/virtio-gpu-udmabuf.c
@@ -185,6 +185,7 @@ static VGPUDMABuf
dmabuf->buf.stride = fb->stride;
dmabuf->buf.fourcc = qemu_pixman_to_drm_format(fb->format);
dmabuf->buf.fd = res->dmabuf_fd;
+dmabuf->buf.allow_fences = true;
dmabuf->scanout_id = scanout_id;
QTAILQ_INSERT_HEAD(>dmabuf.bufs, dmabuf, next);
diff --git a/include/ui/console.h b/include/ui/console.h
index 45ec129174..244664d727 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -170,6 +170,7 @@ typedef struct QemuDmaBuf {
bool y0_top;
void *sync;
int fence_fd;
+bool allow_fences;
} QemuDmaBuf;
typedef struct DisplayState DisplayState;
diff --git a/include/ui/egl-helpers.h b/include/ui/egl-helpers.h
index 2c3ba92b53..2fb6e0dd6b 100644
--- a/include/ui/egl-helpers.h
+++ b/include/ui/egl-helpers.h
@@ -19,6 +19,7 @@ typedef struct egl_fb {
GLuint texture;
GLuint framebuffer;
bool delete_texture;
+QemuDmaBuf *dmabuf;
} egl_fb;
void egl_fb_destroy(egl_fb *fb);
diff --git a/include/ui/gtk.h b/include/ui/gtk.h
index 8e98a79ac8..43854f3509 100644
--- a/include/ui/gtk.h
+++ b/include/ui/gtk.h
@@ -155,6 +155,7 @@ extern bool gtk_use_gl_area;
/* ui/gtk.c */
void gd_update_windowsize(VirtualConsole *vc);
int gd_monitor_update_interval(GtkWidget *widget);
+void gd_hw_gl_flushed(void *vc);
/* ui/gtk-egl.c */
void gd_egl_init(VirtualConsole *vc);
diff --git a/ui/gtk-egl.c b/ui/gtk-egl.c
index b671181272..2c68696d9f 100644
--- a/ui/gtk-egl.c
+++ b/ui/gtk-egl.c
@@ -12,6 +12,7 @@
*/
#include "qemu/osdep.h"
+#include "qemu/main-loop.h"
#include "trace.h"
@@ -63,6 +64,7 @@ void gd_egl_draw(VirtualConsole *vc)
{
GdkWindow *window;
int ww, wh;
+QemuDmaBuf *dmabuf = vc->gfx.guest_fb.dmabuf;
if (!vc->gfx.gls) {
return;
@@ -94,6 +96,14 @@ void gd_egl_draw(VirtualConsole *vc)
}
glFlush();
+if (dmabuf) {
+egl_dmabuf_create_fence(dmabuf);
+if (dmabuf->fence_fd > 0) {
+qemu_set_fd_handler(dmabuf->fence_fd, gd_hw_gl_flushed, NULL, vc);
+return;
+}
+graphic_hw_gl_block(vc->gfx.dcl.con, false);
+}
graphic_hw_gl_flushed(vc->gfx.dcl.con);
}
@@ -209,6 +219,8 @@ void gd_egl_scanout_dmabuf(DisplayChangeListener *dcl,
QemuDmaBuf *dmabuf)
{
#ifdef CONFIG_GBM
+VirtualConsole *vc = container_of(dcl, VirtualConsole, gfx.dcl);
+
egl_dmabuf_import_texture(dmabuf);
if (!dmabuf->texture) {
return;
@@ -217,6 +229,10 @@ void gd_egl_scanout_dmabuf(DisplayChangeListener *dcl,
gd_egl_scanout_texture(dcl, dmabuf->texture,
false, dmabuf->width, dmabuf->height,
0, 0, dmabuf->width, dmabuf->height);
+
+if (dmabuf->allow_fences) {
+vc->gfx.guest_fb.dmabuf = dmabuf;
+}
#endif
}
@@ -281,6 +297,10 @@ void gd_egl_scanout_flush(DisplayChangeListener *dcl,
egl_fb_blit(>gfx.win_fb, >gfx.guest_fb, !vc->gfx.y0_top);
}
+if (vc->gfx.guest_fb.dmabuf) {
+egl_dmabuf_create_sync(vc->gfx.guest_fb.dmabuf);
+}
+
eglSwapBuffers(qemu_egl_display, vc->gfx.esurface);
}
diff --git a/ui/gtk-gl-area.c b/ui/gtk-gl-area.c
index dd5783fec7..1654941dc9 100644
--- a/ui/gtk-gl-area.c
+++ b/ui/gtk-gl-area.c
@@ -8,6 +8,7 @@
*/
#include "qemu/osdep.h"
+#include "qemu/main-loop.h"
#include "trace.h"
@@ -38,6 +39,7 @@ static void gtk_gl_area_set_scanout_mode(VirtualConsole *vc,
bool scanout)
void gd_gl_area_draw(VirtualConsole *vc)
{
int ww, wh, y1, y2;
+QemuDmaBuf *dmabuf = vc->gfx.guest_fb.dmabuf;
if (!vc->gfx.gls) {
return;
@@ -71,7 +73,18 @@ void gd_gl_area_draw(VirtualConsole *vc)
surface_gl_render_texture(vc->gfx.gls, vc->gfx.ds);
}
+if (dmabuf) {
+egl_dmabuf_create_sync(dmabuf);
+}
glFlush();
+if (dmabuf) {
+egl_dmabuf_create_fence(dmabuf);
+if (dmabuf->fence_fd > 0) {
+qemu_set_fd_handler(dmabuf->fence_fd, gd_hw_gl_flushed, NULL, vc);
+return;
+}
+graphic_hw_gl_block(vc->gfx.dcl.con, false);
+}
[PATCH v5 1/5] ui/gtk: Create a common release_dmabuf helper
Since the texture release mechanism is same for both gtk-egl
and gtk-glarea, move the helper from gtk-egl to common gtk
code so that it can be shared by both gtk backends.
Cc: Gerd Hoffmann
Reviewed-by: Gerd Hoffmann
Signed-off-by: Vivek Kasireddy
---
include/ui/gtk.h | 2 --
ui/gtk-egl.c | 8
ui/gtk.c | 11 ++-
3 files changed, 10 insertions(+), 11 deletions(-)
diff --git a/include/ui/gtk.h b/include/ui/gtk.h
index 7835ef1a71..8e98a79ac8 100644
--- a/include/ui/gtk.h
+++ b/include/ui/gtk.h
@@ -181,8 +181,6 @@ void gd_egl_cursor_dmabuf(DisplayChangeListener *dcl,
uint32_t hot_x, uint32_t hot_y);
void gd_egl_cursor_position(DisplayChangeListener *dcl,
uint32_t pos_x, uint32_t pos_y);
-void gd_egl_release_dmabuf(DisplayChangeListener *dcl,
- QemuDmaBuf *dmabuf);
void gd_egl_scanout_flush(DisplayChangeListener *dcl,
uint32_t x, uint32_t y, uint32_t w, uint32_t h);
void gtk_egl_init(DisplayGLMode mode);
diff --git a/ui/gtk-egl.c b/ui/gtk-egl.c
index 2a2e6d3a17..b671181272 100644
--- a/ui/gtk-egl.c
+++ b/ui/gtk-egl.c
@@ -249,14 +249,6 @@ void gd_egl_cursor_position(DisplayChangeListener *dcl,
vc->gfx.cursor_y = pos_y * vc->gfx.scale_y;
}
-void gd_egl_release_dmabuf(DisplayChangeListener *dcl,
- QemuDmaBuf *dmabuf)
-{
-#ifdef CONFIG_GBM
-egl_dmabuf_release_texture(dmabuf);
-#endif
-}
-
void gd_egl_scanout_flush(DisplayChangeListener *dcl,
uint32_t x, uint32_t y, uint32_t w, uint32_t h)
{
diff --git a/ui/gtk.c b/ui/gtk.c
index cfb0728d1f..784a2f6c74 100644
--- a/ui/gtk.c
+++ b/ui/gtk.c
@@ -575,6 +575,14 @@ static bool gd_has_dmabuf(DisplayChangeListener *dcl)
return vc->gfx.has_dmabuf;
}
+static void gd_gl_release_dmabuf(DisplayChangeListener *dcl,
+ QemuDmaBuf *dmabuf)
+{
+#ifdef CONFIG_GBM
+egl_dmabuf_release_texture(dmabuf);
+#endif
+}
+
/** DisplayState Callbacks (opengl version) **/
static const DisplayChangeListenerOps dcl_gl_area_ops = {
@@ -593,6 +601,7 @@ static const DisplayChangeListenerOps dcl_gl_area_ops = {
.dpy_gl_scanout_disable = gd_gl_area_scanout_disable,
.dpy_gl_update = gd_gl_area_scanout_flush,
.dpy_gl_scanout_dmabuf = gd_gl_area_scanout_dmabuf,
+.dpy_gl_release_dmabuf = gd_gl_release_dmabuf,
.dpy_has_dmabuf = gd_has_dmabuf,
};
@@ -615,8 +624,8 @@ static const DisplayChangeListenerOps dcl_egl_ops = {
.dpy_gl_scanout_dmabuf = gd_egl_scanout_dmabuf,
.dpy_gl_cursor_dmabuf= gd_egl_cursor_dmabuf,
.dpy_gl_cursor_position = gd_egl_cursor_position,
-.dpy_gl_release_dmabuf = gd_egl_release_dmabuf,
.dpy_gl_update = gd_egl_scanout_flush,
+.dpy_gl_release_dmabuf = gd_gl_release_dmabuf,
.dpy_has_dmabuf = gd_has_dmabuf,
};
--
2.30.2
[PATCH v5 0/5] virtio-gpu: Add a default synchronization mechanism for blobs
When the Guest and Host are using Blob resources, there is a chance that they may use the underlying storage associated with a Blob at the same time leading to glitches such as flickering or tearing. To prevent these from happening, the Host needs to ensure that it waits until its Blit is completed by the Host GPU before letting the Guest reuse the Blob. This should be the default behavior regardless of the type of Guest that is using Blob resources but would be particularly useful for Guests that are using frontbuffer rendering such as some X compositors, Windows compositors, etc. The way it works is the Guest submits the resource_flush command and waits -- for example over a dma fence -- until virtio-gpu sends an ack. And, the UI will queue a new repaint request and waits until the sync object associated with the Blit is signaled. Once this is done, the UI will trigger virtio-gpu to send an ack for the resource_flush cmd. v2: - Added more description in the cover letter - Removed the wait from resource_flush and included it in a gl_flushed() callback v3: - Instead of explicitly waiting on the sync object and stalling the thread, add the relevant fence fd to Qemu's main loop and wait for it to be signalled. (suggested by Gerd Hoffmann) v4: - Replace the field 'blob' with 'allow_fences' in QemuDmabuf struct. (Gerd) v5: rebase Cc: Gerd Hoffmann Cc: Dongwon Kim Cc: Tina Zhang Vivek Kasireddy (5): ui/gtk: Create a common release_dmabuf helper ui/egl: Add egl helpers to help with synchronization ui: Create sync objects and fences only for blobs ui/gtk-egl: Wait for the draw signal for dmabuf blobs virtio-gpu: Add gl_flushed callback hw/display/virtio-gpu-udmabuf.c | 1 + hw/display/virtio-gpu.c | 32 ++-- include/ui/console.h| 3 +++ include/ui/egl-helpers.h| 3 +++ include/ui/gtk.h| 5 ++-- ui/egl-helpers.c| 26 ui/gtk-egl.c| 43 +++-- ui/gtk-gl-area.c| 20 +++ ui/gtk.c| 26 ++-- 9 files changed, 145 insertions(+), 14 deletions(-) -- 2.30.2
Re: [PATCH v1 1/3] io: Enable write flags for QIOChannel
On Tue, Aug 31, 2021 at 08:02:37AM -0300, Leonardo Bras wrote:
> Some syscalls used for writting, such as sendmsg(), accept flags that
> can modify their behavior, even allowing the usage of features such as
> MSG_ZEROCOPY.
>
> Change qio_channel_write*() interface to allow passing down flags,
> allowing a more flexible use of IOChannel.
>
> At first, it's use is enabled for QIOChannelSocket, but can be easily
> extended to any other QIOChannel implementation.
As a followup to this patch, I wonder if we can also get performance
improvements by implementing MSG_MORE, and using that in preference to
corking/uncorking to better indicate that back-to-back short messages
may behave better when grouped together over the wire. At least the
NBD code could make use of it (going off of my experience with the
libnbd project demonstrating a performance boost when we added
MSG_MORE support there).
>
> Signed-off-by: Leonardo Bras
> ---
> chardev/char-io.c | 2 +-
> hw/remote/mpqemu-link.c | 2 +-
> include/io/channel.h| 56 -
> io/channel-buffer.c | 1 +
> io/channel-command.c| 1 +
> io/channel-file.c | 1 +
> io/channel-socket.c | 4 ++-
> io/channel-tls.c| 1 +
> io/channel-websock.c| 1 +
> io/channel.c| 53 ++-
> migration/rdma.c| 1 +
> scsi/pr-manager-helper.c| 2 +-
> tests/unit/test-io-channel-socket.c | 1 +
> 13 files changed, 81 insertions(+), 45 deletions(-)
>
> diff --git a/chardev/char-io.c b/chardev/char-io.c
> index 8ced184160..4ea7b1ee2a 100644
> --- a/chardev/char-io.c
> +++ b/chardev/char-io.c
> @@ -122,7 +122,7 @@ int io_channel_send_full(QIOChannel *ioc,
>
> ret = qio_channel_writev_full(
> ioc, , 1,
> -fds, nfds, NULL);
> +fds, 0, nfds, NULL);
0 before nfds here...
> if (ret == QIO_CHANNEL_ERR_BLOCK) {
> if (offset) {
> return offset;
> diff --git a/hw/remote/mpqemu-link.c b/hw/remote/mpqemu-link.c
> index 7e841820e5..0d13321ef0 100644
> --- a/hw/remote/mpqemu-link.c
> +++ b/hw/remote/mpqemu-link.c
> @@ -69,7 +69,7 @@ bool mpqemu_msg_send(MPQemuMsg *msg, QIOChannel *ioc, Error
> **errp)
> }
>
> if (!qio_channel_writev_full_all(ioc, send, G_N_ELEMENTS(send),
> -fds, nfds, errp)) {
> + fds, nfds, 0, errp)) {
Thanks for fixing the broken indentation.
...but after nfds here, so one is wrong; up to this point in a linear
review, I can't tell which was intended...
> +++ b/include/io/channel.h
> @@ -104,6 +104,7 @@ struct QIOChannelClass {
> size_t niov,
> int *fds,
> size_t nfds,
> + int flags,
> Error **errp);
...and finally I see that in general, you wanted to add the argument
after. Looks like the change to char-io.c is buggy.
(You can use scripts/git.orderfile as a way to force your patch to
list the .h file first, to make it easier for linear review).
> ssize_t (*io_readv)(QIOChannel *ioc,
> const struct iovec *iov,
> @@ -260,6 +261,7 @@ ssize_t qio_channel_writev_full(QIOChannel *ioc,
> size_t niov,
> int *fds,
> size_t nfds,
> +int flags,
> Error **errp);
>
> /**
> @@ -325,6 +327,7 @@ int qio_channel_readv_all(QIOChannel *ioc,
> * @ioc: the channel object
> * @iov: the array of memory regions to write data from
> * @niov: the length of the @iov array
> + * @flags: optional sending flags
> * @errp: pointer to a NULL-initialized error object
> *
> * Write data to the IO channel, reading it from the
> @@ -339,10 +342,14 @@ int qio_channel_readv_all(QIOChannel *ioc,
> *
> * Returns: 0 if all bytes were written, or -1 on error
> */
> -int qio_channel_writev_all(QIOChannel *ioc,
> - const struct iovec *iov,
> - size_t niov,
> - Error **erp);
> +int qio_channel_writev_all_flags(QIOChannel *ioc,
> + const struct iovec *iov,
> + size_t niov,
> + int flags,
> + Error **errp);
You changed the function name here, but not in the comment beforehand.
> +
> +#define qio_channel_writev_all(ioc, iov, niov, errp) \
> +qio_channel_writev_all_flags(ioc, iov, niov, 0, errp)
In most cases, you were merely adding a new function to minimize churn
to existing callers while making the old name a macro,...
> @@ -853,6 +876,7 @@
Re: [PATCH] tcg/arm: Increase stack alignment for function generation
On Wed, Sep 01, 2021 at 09:17:07PM +0100, Peter Maydell wrote: > On Wed, 1 Sept 2021 at 19:51, Richard W.M. Jones wrote: > > > > On Wed, Sep 01, 2021 at 07:41:21PM +0100, Peter Maydell wrote: > > > Is the failure case short enough to allow -d ... logging to > > > be taken? That's usually the most useful info, but it's so huge > > > it's often not feasible. > > > > I can try -- what exact -d option would be useful? > > Depends what you're after. Personally I'm fairly sure I know > what's going on, I'm just not sure what the right fix is. Another question: We couldn't reproduce this even with the identical ARM guest kernel + initrd + command line using qemu-system-arm compiled for x86-64 host. This was a bit surprising! Was that bad luck or is there some reason why this bug might not be reproducible except on armv7 host? (Both cases use -machine accel=tcg). Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://libguestfs.org
Re: [PATCH v0] kvm: unsigned datatype in ioctl wrapper
On Mon, Aug 30, 2021 at 10:15 PM Peter Maydell wrote: > I think I would vote for following the type used by the ioctl() > function as declared in the headers for both Linux and the BSDs, > and using 'unsigned long'. > (We should change KVMState::irq_set_ioctl too, to match.) I would agree to 'unsigned long', in that case I can generate a new patch. And in theory if all libc implementations specify the ioctl request as 'int' we could go back to 'int'. Johannes
Re: [PATCH] tcg/arm: Increase stack alignment for function generation
On Wed, 1 Sept 2021 at 19:51, Richard W.M. Jones wrote: > > On Wed, Sep 01, 2021 at 07:41:21PM +0100, Peter Maydell wrote: > > Is the failure case short enough to allow -d ... logging to > > be taken? That's usually the most useful info, but it's so huge > > it's often not feasible. > > I can try -- what exact -d option would be useful? Depends what you're after. Personally I'm fairly sure I know what's going on, I'm just not sure what the right fix is. -- PMM
Re: [PATCH v1 0/3] QIOChannel flags + multifd zerocopy
Hello Peter, On Tue, Aug 31, 2021 at 6:24 PM Peter Xu wrote: > > On Tue, Aug 31, 2021 at 08:02:36AM -0300, Leonardo Bras wrote: > > Results: > > So far, the resource usage of __sys_sendmsg() reduced 15 times, and the > > overall migration took 13-18% less time, based in synthetic workload. > > Leo, > > Could you share some of the details of your tests? E.g., what's the > configuration of your VM for testing? What's the migration time before/after > the patchset applied? What is the network you're using? > > Thanks, > > -- > Peter Xu > Sure, - Both receiving and sending hosts have 128GB ram and a 10Gbps network interface - There is a direct connection between the network interfaces. - The guest has 100GB ram, mem-lock=on and enable-kvm. - Before sending, I use a simple application to completely fill all guest pages with unique values, to avoid duplicated pages and zeroed pages. On a single test: Without zerocopy (qemu/master) - Migration took 123355ms, with an average of 6912.58 Mbps With Zerocopy: - Migration took 108514ms, with an average of 7858.39 Mbps This represents a throughput improvement around 13.6%. Comparing perf recorded during default and zerocopy migration: Without zerocopy: - copy_user_generic_string() uses 5.4% of cpu time - __sys_sendmsg() uses 5.19% of cpu time With zerocopy: - copy_user_generic_string() uses 0.02% of cpu time (~1/270 of the original) - __sys_sendmsg() uses 0.34% of cpu time (~1/15 of the original)
Re: [PATCH] tcg/arm: Increase stack alignment for function generation
On Wed, Sep 01, 2021 at 07:41:21PM +0100, Peter Maydell wrote: > Is the failure case short enough to allow -d ... logging to > be taken? That's usually the most useful info, but it's so huge > it's often not feasible. I can try -- what exact -d option would be useful? Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://libguestfs.org
Re: [PATCH] tcg/arm: Increase stack alignment for function generation
On Wed, 1 Sept 2021 at 19:30, Richard W.M. Jones wrote: > > On Wed, Sep 01, 2021 at 07:18:03PM +0100, Peter Maydell wrote: > > On Wed, 1 Sept 2021 at 18:01, Richard W.M. Jones wrote: > > > > > > This avoids the following assertion when the kernel initializes X.509 > > > certificates: > > > > > > [7.315373] Loading compiled-in X.509 certificates > > > qemu-system-arm: ../tcg/tcg.c:3063: temp_allocate_frame: Assertion `align > > > <= TCG_TARGET_STACK_ALIGN' failed. > > > > > > Fixes: commit c1c091948ae > > > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1999878 > > > Cc: qemu-sta...@nongnu.org > > > Tested-by: Richard W.M. Jones > > > Signed-off-by: Richard W.M. Jones > > > --- > > > tcg/arm/tcg-target.h | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/tcg/arm/tcg-target.h b/tcg/arm/tcg-target.h > > > index d113b7f8db..09df3b39a1 100644 > > > --- a/tcg/arm/tcg-target.h > > > +++ b/tcg/arm/tcg-target.h > > > @@ -115,7 +115,7 @@ extern bool use_neon_instructions; > > > #endif > > > > > > /* used for function call generation */ > > > -#define TCG_TARGET_STACK_ALIGN 8 > > > +#define TCG_TARGET_STACK_ALIGN 16 > > > #define TCG_TARGET_CALL_ALIGN_ARGS 1 > > > #define TCG_TARGET_CALL_STACK_OFFSET 0 > > > > The 32-bit Arm procedure call standard only guarantees 8-alignment > > of SP, not 16-alignment, so I suspect this is not the correct fix. > > Wouldn't it be a good idea if asserts in TCG dumped out something > useful about the guest code? Because I can only reproduce this bug in > a very awkward batch environment I need to collect as much information > from log messages as possible. Is the failure case short enough to allow -d ... logging to be taken? That's usually the most useful info, but it's so huge it's often not feasible. Anyway, the problem here is that the arm backend now supports Neon, and it will try to do some operations on 128 bit types, where at least the loads and stores require 16-alignment. But nothing is enforcing that we have 16 alignment. (Without the assert we'd probably blunder on and fault on an unaligned access.) Rereading the TCG code, maybe your patch here is right. It's not clear to me whether TCG_TARGET_STACK_ALIGN is intended to specify "alignment the calling convention requires" (though that's what the comment above it suggests) or "alignment that TCG requires". The prologue does seem to actively align to the specified value, not merely assume-and-preserve that alignment. -- PMM
Re: [PATCH] tcg/arm: Increase stack alignment for function generation
On Wed, Sep 01, 2021 at 07:18:03PM +0100, Peter Maydell wrote: > On Wed, 1 Sept 2021 at 18:01, Richard W.M. Jones wrote: > > > > This avoids the following assertion when the kernel initializes X.509 > > certificates: > > > > [7.315373] Loading compiled-in X.509 certificates > > qemu-system-arm: ../tcg/tcg.c:3063: temp_allocate_frame: Assertion `align > > <= TCG_TARGET_STACK_ALIGN' failed. > > > > Fixes: commit c1c091948ae > > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1999878 > > Cc: qemu-sta...@nongnu.org > > Tested-by: Richard W.M. Jones > > Signed-off-by: Richard W.M. Jones > > --- > > tcg/arm/tcg-target.h | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/tcg/arm/tcg-target.h b/tcg/arm/tcg-target.h > > index d113b7f8db..09df3b39a1 100644 > > --- a/tcg/arm/tcg-target.h > > +++ b/tcg/arm/tcg-target.h > > @@ -115,7 +115,7 @@ extern bool use_neon_instructions; > > #endif > > > > /* used for function call generation */ > > -#define TCG_TARGET_STACK_ALIGN 8 > > +#define TCG_TARGET_STACK_ALIGN 16 > > #define TCG_TARGET_CALL_ALIGN_ARGS 1 > > #define TCG_TARGET_CALL_STACK_OFFSET 0 > > The 32-bit Arm procedure call standard only guarantees 8-alignment > of SP, not 16-alignment, so I suspect this is not the correct fix. Wouldn't it be a good idea if asserts in TCG dumped out something useful about the guest code? Because I can only reproduce this bug in a very awkward batch environment I need to collect as much information from log messages as possible. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://libguestfs.org
Re: [PATCH] tcg/arm: Increase stack alignment for function generation
On Wed, 1 Sept 2021 at 18:01, Richard W.M. Jones wrote: > > This avoids the following assertion when the kernel initializes X.509 > certificates: > > [7.315373] Loading compiled-in X.509 certificates > qemu-system-arm: ../tcg/tcg.c:3063: temp_allocate_frame: Assertion `align <= > TCG_TARGET_STACK_ALIGN' failed. > > Fixes: commit c1c091948ae > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1999878 > Cc: qemu-sta...@nongnu.org > Tested-by: Richard W.M. Jones > Signed-off-by: Richard W.M. Jones > --- > tcg/arm/tcg-target.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tcg/arm/tcg-target.h b/tcg/arm/tcg-target.h > index d113b7f8db..09df3b39a1 100644 > --- a/tcg/arm/tcg-target.h > +++ b/tcg/arm/tcg-target.h > @@ -115,7 +115,7 @@ extern bool use_neon_instructions; > #endif > > /* used for function call generation */ > -#define TCG_TARGET_STACK_ALIGN 8 > +#define TCG_TARGET_STACK_ALIGN 16 > #define TCG_TARGET_CALL_ALIGN_ARGS 1 > #define TCG_TARGET_CALL_STACK_OFFSET 0 The 32-bit Arm procedure call standard only guarantees 8-alignment of SP, not 16-alignment, so I suspect this is not the correct fix. -- PMM
Re: [PATCH] multifd: Implement yank for multifd send side
On Wed, Sep 01, 2021 at 05:58:57PM +0200, Lukas Straub wrote:
> When introducing yank functionality in the migration code I forgot
> to cover the multifd send side.
>
> Signed-off-by: Lukas Straub
> Tested-by: Leonardo Bras
> Reviewed-by: Leonardo Bras
> ---
>
> -v2:
> -add Tested-by and Reviewed-by tags
>
> migration/multifd.c | 6 +-
> migration/multifd.h | 2 ++
> 2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/migration/multifd.c b/migration/multifd.c
> index 377da78f5b..5a4f158f3c 100644
> --- a/migration/multifd.c
> +++ b/migration/multifd.c
> @@ -546,6 +546,9 @@ void multifd_save_cleanup(void)
> MultiFDSendParams *p = _send_state->params[i];
> Error *local_err = NULL;
>
> +if (p->registered_yank) {
> +migration_ioc_unregister_yank(p->c);
> +}
> socket_send_channel_destroy(p->c);
> p->c = NULL;
> qemu_mutex_destroy(>mutex);
> @@ -813,7 +816,8 @@ static bool multifd_channel_connect(MultiFDSendParams *p,
> return false;
> }
> } else {
> -/* update for tls qio channel */
> +migration_ioc_register_yank(ioc);
> +p->registered_yank = true;
> p->c = ioc;
> qemu_thread_create(>thread, p->name, multifd_send_thread, p,
> QEMU_THREAD_JOINABLE);
> diff --git a/migration/multifd.h b/migration/multifd.h
> index 8d6751f5ed..16c4d112d1 100644
> --- a/migration/multifd.h
> +++ b/migration/multifd.h
> @@ -85,6 +85,8 @@ typedef struct {
> bool running;
> /* should this thread finish */
> bool quit;
> +/* is the yank function registered */
> +bool registered_yank;
> /* thread has work to do */
> int pending_job;
> /* array of pages to sent */
> --
> 2.32.0
This is probably yet another case that I'm wondering whether we made unregister
of yank function/instance too strict so we should loose them.
Logically a remove/unregister function doesn't need to assert and crash qemu if
the entry doesn't exist at all. Then it's just something like "makes sure XXX
is removed", and noop if lookup failed. The extra fields do not really help us
from anything else..
Thanks,
--
Peter Xu
Re: [PATCH] 9pfs: fix crash in v9fs_walk()
On Mittwoch, 1. September 2021 18:47:21 CEST Greg Kurz wrote:
> On Wed, 1 Sep 2021 18:15:10 +0200
>
> Christian Schoenebeck wrote:
> > v9fs_walk() utilizes the v9fs_co_run_in_worker({...}) macro to run the
> > supplied fs driver code block on a background worker thread.
> >
> > When either the 'Twalk' client request was interrupted or if the client
> > requested fid for that 'Twalk' request caused a stat error then that
> > fs driver code block was left by 'break' keyword, with the intention to
> >
> > return from worker thread back to main thread as well:
> > v9fs_co_run_in_worker({
> >
> > if (v9fs_request_cancelled(pdu)) {
> >
> > err = -EINTR;
> > break;
> >
> > }
> > err = s->ops->lstat(>ctx, , );
> > if (err < 0) {
> >
> > err = -errno;
> > break;
> >
> > }
> > ...
> >
> > });
> >
> > However that 'break;' statement also skipped the v9fs_co_run_in_worker()
> > macro's final and mandatory
> >
> > /* re-enter back to qemu thread */
> > qemu_coroutine_yield();
> >
> > call and thus caused the rest of v9fs_walk() to be continued being
> > executed on the worker thread instead of main thread, eventually
> > leading to a crash in the transport virtio transport driver.
> >
> > To fix this issue and to prevent the same error from happening again by
> > other users of v9fs_co_run_in_worker() in future, auto wrap the supplied
> > code block into its own
> >
> > do { } while (0);
> >
> > loop inside the 'v9fs_co_run_in_worker' macro definition.
> >
> > Full discussion and backtrace:
> > https://lists.gnu.org/archive/html/qemu-devel/2021-08/msg05209.html
> > https://lists.gnu.org/archive/html/qemu-devel/2021-09/msg00174.html
> >
> > Fixes: 8d6cb100731c4d28535adbf2a3c2d1f29be3fef4
> > Signed-off-by: Christian Schoenebeck
> > Cc: qemu-sta...@nongnu.org
> > ---
>
> Reviewed-by: Greg Kurz
Queued on 9p.next:
https://github.com/cschoenebeck/qemu/commits/9p.next
Thanks!
I'll send out a PR tomorrow.
> > hw/9pfs/coth.h | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/9pfs/coth.h b/hw/9pfs/coth.h
> > index c51289903d..f83c7dda7b 100644
> > --- a/hw/9pfs/coth.h
> > +++ b/hw/9pfs/coth.h
> > @@ -51,7 +51,9 @@
> >
> > */ \
> >
> > qemu_coroutine_yield(); \
> > qemu_bh_delete(co_bh); \
> >
> > -code_block; \
> > +do {\
> > +code_block; \
> > +} while (0);\
> >
> > /* re-enter back to qemu thread */ \
> > qemu_coroutine_yield(); \
> >
> > } while (0)
Best regards,
Christian Schoenebeck
Re: [PATCH v4 3/3] memory: Have 'info mtree' remove duplicated Address Space information
On 01.09.21 18:19, Philippe Mathieu-Daudé wrote: Per Peter Maydell [*]: 'info mtree' monitor command was designed on the assumption that there's really only one or two interesting address spaces, and with more recent developments that's just not the case any more. Similarly about how the FlatView are sorted using a GHashTable, sort the AddressSpace objects to remove the duplications (AS using the same root MemoryRegion). This drastically reduces the output of 'info mtree' on some boards. Before: $ (echo info mtree; echo q) \ | qemu-system-aarch64 -S -monitor stdio -M raspi3b \ | wc -l 423 After: $ (echo info mtree; echo q) \ | qemu-system-aarch64 -S -monitor stdio -M raspi3b \ | wc -l 108 (qemu) info mtree address-space: I/O - (prio 0, i/o): io address-space shared 9 times: - cpu-memory-0 - cpu-memory-1 - cpu-memory-2 - cpu-memory-3 - cpu-secure-memory-0 - cpu-secure-memory-1 - cpu-secure-memory-2 - cpu-secure-memory-3 - memory - (prio 0, i/o): system -3fff (prio 0, ram): ram 3f00-3fff (prio 1, i/o): bcm2835-peripherals 3f003000-3f00301f (prio 0, i/o): bcm2835-sys-timer 3f004000-3f004fff (prio -1000, i/o): bcm2835-txp 3f006000-3f006fff (prio 0, i/o): mphi 3f007000-3f007fff (prio 0, i/o): bcm2835-dma 3f00b200-3f00b3ff (prio 0, i/o): bcm2835-ic 3f00b400-3f00b43f (prio -1000, i/o): bcm2835-sp804 3f00b800-3f00bbff (prio 0, i/o): bcm2835-mbox 3f10-3f1001ff (prio 0, i/o): bcm2835-powermgt 3f101000-3f102fff (prio 0, i/o): bcm2835-cprman 3f104000-3f10400f (prio 0, i/o): bcm2835-rng 3f20-3f200fff (prio 0, i/o): bcm2835_gpio 3f201000-3f201fff (prio 0, i/o): pl011 3f202000-3f202fff (prio 0, i/o): bcm2835-sdhost 3f203000-3f2030ff (prio -1000, i/o): bcm2835-i2s 3f204000-3f20401f (prio -1000, i/o): bcm2835-spi0 3f205000-3f20501f (prio -1000, i/o): bcm2835-i2c0 3f20f000-3f20f07f (prio -1000, i/o): bcm2835-otp 3f212000-3f212007 (prio 0, i/o): bcm2835-thermal 3f214000-3f2140ff (prio -1000, i/o): bcm2835-spis 3f215000-3f2150ff (prio 0, i/o): bcm2835-aux 3f30-3f3000ff (prio 0, i/o): sdhci 3f60-3f6000ff (prio -1000, i/o): bcm2835-smi 3f804000-3f80401f (prio -1000, i/o): bcm2835-i2c1 3f805000-3f80501f (prio -1000, i/o): bcm2835-i2c2 3f90-3f907fff (prio -1000, i/o): bcm2835-dbus 3f91-3f917fff (prio -1000, i/o): bcm2835-ave0 3f98-3f990fff (prio 0, i/o): dwc2 3f98-3f980fff (prio 0, i/o): dwc2-io 3f981000-3f990fff (prio 0, i/o): dwc2-fifo 3fc0-3fc00fff (prio -1000, i/o): bcm2835-v3d 3fe0-3fe000ff (prio -1000, i/o): bcm2835-sdramc 3fe05000-3fe050ff (prio 0, i/o): bcm2835-dma-chan15 4000-40ff (prio 0, i/o): bcm2836-control address-space shared 4 times: - bcm2835-dma-memory - bcm2835-fb-memory - bcm2835-property-memory - dwc2 - (prio 0, i/o): bcm2835-gpu -3fff (prio 0, ram): alias bcm2835-gpu-ram-alias[*] @ram -3fff 4000-7fff (prio 0, ram): alias bcm2835-gpu-ram-alias[*] @ram -3fff 7e00-7eff (prio 1, i/o): alias bcm2835-peripherals @bcm2835-peripherals -00ff 8000-bfff (prio 0, ram): alias bcm2835-gpu-ram-alias[*] @ram -3fff c000- (prio 0, ram): alias bcm2835-gpu-ram-alias[*] @ram -3fff address-space: bcm2835-mbox-memory -008f (prio 0, i/o): bcm2835-mbox 0010-001f (prio 0, i/o): bcm2835-fb 0080-008f (prio 0, i/o): bcm2835-property memory-region: ram -3fff (prio 0, ram): ram memory-region: bcm2835-peripherals 3f00-3fff (prio 1, i/o): bcm2835-peripherals 3f003000-3f00301f (prio 0, i/o):
Re: [PATCH v4 2/3] memory: Extract mtree_info_as() from mtree_info()
On 01.09.21 18:19, Philippe Mathieu-Daudé wrote:
While mtree_info() handles both ASes and flatviews cases,
the two cases share basically no code. Split mtree_info_as()
out of mtree_info() to simplify.
Suggested-by: Peter Maydell
Signed-off-by: Philippe Mathieu-Daudé
---
softmmu/memory.c | 17 ++---
1 file changed, 10 insertions(+), 7 deletions(-)
diff --git a/softmmu/memory.c b/softmmu/memory.c
index 3eb6f52de67..5be7d5e7412 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -3284,18 +3284,12 @@ static void mtree_info_flatview(bool dispatch_tree,
bool owner)
g_hash_table_unref(views);
}
-void mtree_info(bool flatview, bool dispatch_tree, bool owner, bool disabled)
+static void mtree_info_as(bool dispatch_tree, bool owner, bool disabled)
{
MemoryRegionListHead ml_head;
MemoryRegionList *ml, *ml2;
AddressSpace *as;
-if (flatview) {
-mtree_info_flatview(dispatch_tree, owner);
-
-return;
-}
-
QTAILQ_INIT(_head);
QTAILQ_FOREACH(as, _spaces, address_spaces_link) {
@@ -3316,6 +3310,15 @@ void mtree_info(bool flatview, bool dispatch_tree, bool
owner, bool disabled)
}
}
+void mtree_info(bool flatview, bool dispatch_tree, bool owner, bool disabled)
+{
+if (flatview) {
+mtree_info_flatview(dispatch_tree, owner);
+} else {
+mtree_info_as(dispatch_tree, owner, disabled);
+}
+}
+
void memory_region_init_ram(MemoryRegion *mr,
Object *owner,
const char *name,
Reviewed-by: David Hildenbrand
I'd just have squashed that into #1.
--
Thanks,
David / dhildenb
Re: [PATCH v4 1/3] memory: Extract mtree_info_flatview() from mtree_info()
On 01.09.21 18:19, Philippe Mathieu-Daudé wrote: While mtree_info() handles both ASes and flatviews cases, the two cases share basically no code. Split mtree_info_flatview() out of mtree_info() to simplify. Note: Patch easier to review using 'git-diff --color-moved=blocks'. Suggested-by: Peter Maydell Signed-off-by: Philippe Mathieu-Daudé Reviewed-by: David Hildenbrand -- Thanks, David / dhildenb
Re: [PATCH] tests/vhost-user-bridge.c: Fix typo in help message
On Wed, Sep 1, 2021 at 8:13 PM Peter Maydell wrote: > Fix a typo in the help message printed by vhost-user-bridge. > > Signed-off-by: Peter Maydell > Reviewed-by: Marc-André Lureau --- > tests/vhost-user-bridge.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tests/vhost-user-bridge.c b/tests/vhost-user-bridge.c > index cb009545fa5..35088dd67f7 100644 > --- a/tests/vhost-user-bridge.c > +++ b/tests/vhost-user-bridge.c > @@ -831,7 +831,7 @@ main(int argc, char *argv[]) > out: > fprintf(stderr, "Usage: %s ", argv[0]); > fprintf(stderr, "[-c] [-H] [-u ud_socket_path] [-l lhost:lport] [-r > rhost:rport]\n"); > -fprintf(stderr, "\t-u path to unix doman socket. default: %s\n", > +fprintf(stderr, "\t-u path to unix domain socket. default: %s\n", > DEFAULT_UD_SOCKET); > fprintf(stderr, "\t-l local host and port. default: %s:%s\n", > DEFAULT_LHOST, DEFAULT_LPORT); > -- > 2.20.1 > > > -- Marc-André Lureau
Re: [PATCH] tests/vhost-user-bridge.c: Sanity check socket path length
On Wed, Sep 1, 2021 at 8:36 PM Peter Maydell
wrote:
> The vhost-user-bridge binary accepts a UNIX socket path on
> the command line. Sanity check that this is short enough to
> fit into a sockaddr_un before copying it in.
>
> Fixes: Coverity CID 1432866
> Signed-off-by: Peter Maydell
>
Reviewed-by: Marc-André Lureau
---
> tests/vhost-user-bridge.c | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/tests/vhost-user-bridge.c b/tests/vhost-user-bridge.c
> index 24815920b2b..cb009545fa5 100644
> --- a/tests/vhost-user-bridge.c
> +++ b/tests/vhost-user-bridge.c
> @@ -540,6 +540,11 @@ vubr_new(const char *path, bool client)
> CallbackFunc cb;
> size_t len;
>
> +if (strlen(path) >= sizeof(un.sun_path)) {
> +fprintf(stderr, "unix domain socket path '%s' is too long\n",
> path);
> +exit(1);
> +}
> +
> /* Get a UNIX socket. */
> dev->sock = socket(AF_UNIX, SOCK_STREAM, 0);
> if (dev->sock == -1) {
> --
> 2.20.1
>
>
>
--
Marc-André Lureau
Re: [PULL 0/2] Usb 20210901 patches
On Wed, 1 Sept 2021 at 08:01, Gerd Hoffmann wrote: > > The following changes since commit ad22d0583300df420819e6c89b1c022b998fac8a: > > Merge remote-tracking branch 'remotes/dg-gitlab/tags/ppc-for-6.2-20210827' > into staging (2021-08-27 11:34:12 +0100) > > are available in the Git repository at: > > git://git.kraxel.org/qemu tags/usb-20210901-pull-request > > for you to fetch changes up to ae420c957aff2871b8a1af9cf9ee1a7a75b3552b: > > hw/usb: Fix typo in comments and print (2021-09-01 06:37:13 +0200) > > > usb: bugfixes. > Applied, thanks. Please update the changelog at https://wiki.qemu.org/ChangeLog/6.2 for any user-visible changes. -- PMM
Re: [SPAM] [PATCH] 9pfs: fix crash in v9fs_walk()
On Wed, 1 Sep 2021 18:15:10 +0200
Christian Schoenebeck wrote:
> v9fs_walk() utilizes the v9fs_co_run_in_worker({...}) macro to run the
> supplied fs driver code block on a background worker thread.
>
> When either the 'Twalk' client request was interrupted or if the client
> requested fid for that 'Twalk' request caused a stat error then that
> fs driver code block was left by 'break' keyword, with the intention to
> return from worker thread back to main thread as well:
>
> v9fs_co_run_in_worker({
> if (v9fs_request_cancelled(pdu)) {
> err = -EINTR;
> break;
> }
> err = s->ops->lstat(>ctx, , );
> if (err < 0) {
> err = -errno;
> break;
> }
> ...
> });
>
> However that 'break;' statement also skipped the v9fs_co_run_in_worker()
> macro's final and mandatory
>
> /* re-enter back to qemu thread */
> qemu_coroutine_yield();
>
> call and thus caused the rest of v9fs_walk() to be continued being
> executed on the worker thread instead of main thread, eventually
> leading to a crash in the transport virtio transport driver.
>
> To fix this issue and to prevent the same error from happening again by
> other users of v9fs_co_run_in_worker() in future, auto wrap the supplied
> code block into its own
>
> do { } while (0);
>
> loop inside the 'v9fs_co_run_in_worker' macro definition.
>
> Full discussion and backtrace:
> https://lists.gnu.org/archive/html/qemu-devel/2021-08/msg05209.html
> https://lists.gnu.org/archive/html/qemu-devel/2021-09/msg00174.html
>
> Fixes: 8d6cb100731c4d28535adbf2a3c2d1f29be3fef4
> Signed-off-by: Christian Schoenebeck
> Cc: qemu-sta...@nongnu.org
> ---
Reviewed-by: Greg Kurz
> hw/9pfs/coth.h | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/hw/9pfs/coth.h b/hw/9pfs/coth.h
> index c51289903d..f83c7dda7b 100644
> --- a/hw/9pfs/coth.h
> +++ b/hw/9pfs/coth.h
> @@ -51,7 +51,9 @@
> */ \
> qemu_coroutine_yield(); \
> qemu_bh_delete(co_bh); \
> -code_block; \
> +do {\
> +code_block; \
> +} while (0);\
> /* re-enter back to qemu thread */ \
> qemu_coroutine_yield(); \
> } while (0)
[PATCH] tcg/arm: Increase stack alignment for function generation
https://www.mail-archive.com/qemu-devel@nongnu.org/msg833146.html I tested this patch which simply increases the stack alignment to 16 bytes and it fixes the assertion failure and otherwise appears to work (in as far as it boots the libguestfs appliance). However I've no idea if this is the right thing to do. I also had to change the tabs to spaces otherwise checkpatch complains which makes the patch look a bit odd. Rich.
Re: [PATCH v2 1/1] hw/arm/aspeed: Allow machine to set UART default
> On Sep 1, 2021, at 9:19 AM, Cédric Le Goater wrote:
>
> On 9/1/21 5:36 PM, p...@fb.com wrote:
>> From: Peter Delevoryas
>>
>> When you run QEMU with an Aspeed machine and a single serial device
>> using stdio like this:
>>
>>qemu -machine ast2600-evb -drive ... -serial stdio
>>
>> The guest OS can read and write to the UART5 registers at 0x1E784000 and
>> it will receive from stdin and write to stdout. The Aspeed SoC's have a
>> lot more UART's though (AST2500 has 5, AST2600 has 13) and depending on
>> the board design, may be using any of them as the serial console. (See
>> "stdout-path" in a DTS to check which one is chosen).
>>
>> Most boards, including all of those currently defined in
>> hw/arm/aspeed.c, just use UART5, but some use UART1. This change adds
>> some flexibility for different boards without requiring users to change
>> their command-line invocation of QEMU.
>>
>> I tested this doesn't break existing code by booting an AST2500 OpenBMC
>> image and an AST2600 OpenBMC image, each using UART5 as the console.
>>
>> Then I tested switching the default to UART1 and booting an AST2600
>> OpenBMC image that uses UART1, and that worked too.
>>
>> Signed-off-by: Peter Delevoryas
>
> One comment, any how
>
> Reviewed-by: Cédric Le Goater
>
>> ---
>> hw/arm/aspeed.c | 3 +++
>> hw/arm/aspeed_ast2600.c | 8
>> hw/arm/aspeed_soc.c | 9 ++---
>> include/hw/arm/aspeed.h | 1 +
>> include/hw/arm/aspeed_soc.h | 1 +
>> 5 files changed, 15 insertions(+), 7 deletions(-)
>>
>> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
>> index 9d43e26c51..a81e90c539 100644
>> --- a/hw/arm/aspeed.c
>> +++ b/hw/arm/aspeed.c
>> @@ -350,6 +350,8 @@ static void aspeed_machine_init(MachineState *machine)
>> object_property_set_int(OBJECT(>soc), "hw-prot-key",
>> ASPEED_SCU_PROT_KEY, _abort);
>> }
>> +qdev_prop_set_uint32(DEVICE(>soc), "uart-default",
>> + amc->uart_default);
>> qdev_realize(DEVICE(>soc), NULL, _abort);
>>
>> memory_region_add_subregion(get_system_memory(),
>> @@ -804,6 +806,7 @@ static void aspeed_machine_class_init(ObjectClass *oc,
>> void *data)
>> mc->no_parallel = 1;
>> mc->default_ram_id = "ram";
>> amc->macs_mask = ASPEED_MAC0_ON;
>> +amc->uart_default = ASPEED_DEV_UART5;
>>
>> aspeed_machine_class_props_init(oc);
>> }
>> diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
>> index e3013128c6..b07fcf10a0 100644
>> --- a/hw/arm/aspeed_ast2600.c
>> +++ b/hw/arm/aspeed_ast2600.c
>> @@ -322,10 +322,10 @@ static void aspeed_soc_ast2600_realize(DeviceState
>> *dev, Error **errp)
>> sysbus_connect_irq(SYS_BUS_DEVICE(>timerctrl), i, irq);
>> }
>>
>> -/* UART - attach an 8250 to the IO space as our UART5 */
>> -serial_mm_init(get_system_memory(), sc->memmap[ASPEED_DEV_UART5], 2,
>> - aspeed_soc_get_irq(s, ASPEED_DEV_UART5),
>> - 38400, serial_hd(0), DEVICE_LITTLE_ENDIAN);
>> +/* UART - attach an 8250 to the IO space as our UART */
>> +serial_mm_init(get_system_memory(), sc->memmap[s->uart_default], 2,
>> + aspeed_soc_get_irq(s, s->uart_default), 38400,
>> + serial_hd(0), DEVICE_LITTLE_ENDIAN);
>>
>> /* I2C */
>> object_property_set_link(OBJECT(>i2c), "dram", OBJECT(s->dram_mr),
>> diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
>> index 3ad6c56fa9..09c0f83710 100644
>> --- a/hw/arm/aspeed_soc.c
>> +++ b/hw/arm/aspeed_soc.c
>> @@ -287,9 +287,9 @@ static void aspeed_soc_realize(DeviceState *dev, Error
>> **errp)
>> sysbus_connect_irq(SYS_BUS_DEVICE(>timerctrl), i, irq);
>> }
>>
>> -/* UART - attach an 8250 to the IO space as our UART5 */
>> -serial_mm_init(get_system_memory(), sc->memmap[ASPEED_DEV_UART5], 2,
>> - aspeed_soc_get_irq(s, ASPEED_DEV_UART5), 38400,
>> +/* UART - attach an 8250 to the IO space as our UART */
>> +serial_mm_init(get_system_memory(), sc->memmap[s->uart_default], 2,
>> + aspeed_soc_get_irq(s, s->uart_default), 38400,
>>serial_hd(0), DEVICE_LITTLE_ENDIAN);
>>
>> /* I2C */
>> @@ -439,6 +439,8 @@ static void aspeed_soc_realize(DeviceState *dev, Error
>> **errp)
>> static Property aspeed_soc_properties[] = {
>> DEFINE_PROP_LINK("dram", AspeedSoCState, dram_mr, TYPE_MEMORY_REGION,
>> MemoryRegion *),
>> +DEFINE_PROP_UINT32("uart-default", AspeedSoCState, uart_default,
>> + ASPEED_DEV_UART5),
>> DEFINE_PROP_END_OF_LIST(),
>> };
>>
>> @@ -449,6 +451,7 @@ static void aspeed_soc_class_init(ObjectClass *oc, void
>> *data)
>> dc->realize = aspeed_soc_realize;
>> /* Reason: Uses serial_hds and nd_table in realize() directly */
>> dc->user_creatable = false;
>> +
>
> Unneeded change,
>
> Thanks,
>
> C.
Dang it, yeah I noticed that just after I sent the
Re: [PATCH] gitlab: Escape git-describe match pattern on Windows hosts
On 9/1/21 6:47 PM, Daniel P. Berrangé wrote:
> On Wed, Sep 01, 2021 at 06:27:37PM +0200, Cédric Le Goater wrote:
>> On 9/1/21 6:24 PM, Philippe Mathieu-Daudé wrote:
>>> On 9/1/21 5:53 PM, Daniel P. Berrangé wrote:
On Wed, Sep 01, 2021 at 05:35:42PM +0200, Philippe Mathieu-Daudé wrote:
> On 9/1/21 5:21 PM, Daniel P. Berrangé wrote:
>> On Wed, Sep 01, 2021 at 04:17:48PM +0100, Peter Maydell wrote:
>>> On Wed, 1 Sept 2021 at 15:59, Daniel P. Berrangé
>>> wrote:
On Wed, Sep 01, 2021 at 04:52:29PM +0200, Philippe Mathieu-Daudé wrote:
> Properly escape git-describe 'match' pattern to avoid (MinGW):
>
> $ if grep -q "EXESUF=.exe" config-host.mak; then make installer;
> version="$(git describe --match v[0-9]*)";
> mv -v qemu-setup*.exe qemu-setup-${version}.exe; fi
> fatal: No names found, cannot describe anything.
> ERROR: Job failed: exit code 1
>
> Reported-by: Cédric Le Goater
> Fixes: 8619b5ddb56 ("ci: build & store windows installer")
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/591
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> .gitlab-ci.d/crossbuild-template.yml | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/.gitlab-ci.d/crossbuild-template.yml
> b/.gitlab-ci.d/crossbuild-template.yml
> index 10d22dcf6c1..62d33e6661d 100644
> --- a/.gitlab-ci.d/crossbuild-template.yml
> +++ b/.gitlab-ci.d/crossbuild-template.yml
> @@ -14,7 +14,7 @@
> - make -j$(expr $(nproc) + 1) all check-build $MAKE_CHECK_ARGS
> - if grep -q "EXESUF=.exe" config-host.mak;
>then make installer;
> - version="$(git describe --match v[0-9]*)";
> + version="$(git describe --match 'v[0-9]*')";
Do you have a pointer to a pipeline showing this fix works ?
>
> It worked on my fork but I have some versioned tag:
> https://gitlab.com/philmd_rh/qemu/-/jobs/1553450025
I can reproduce the error msg if I do a shallow clone with no history
$ cd qemu
$ git describe --match v[0-9]*
v6.1.0-171-g5e8c1a0c90
$ cd ..
$ git clone --depth 1 https://gitlab.com/qemu-project/qemu/ qemu.new
$ cd qemu.new/
$ git describe --match v[0-9]*
fatal: No names found, cannot describe anything.
but the odd thing is that I think we should have been hitting the
problem frequently if it was related to git depth. This job passes
fine in current CI pipelines and my own fork.
>>>
>>> FYI it didn't work out on Cédric fork:
>>> https://gitlab.com/legoater/qemu/-/jobs/1553492082
>>>
>>
>> Indeed.
>
> I'm curious if you go to
>
> https://gitlab.com/legoater/qemu/-/settings/ci_cd
>
> and expand "General pipelines", what value is set for the
>
> "Git shallow clone"
>
> setting. In my fork it is 0 which means unlimited depth, but in
> gitlab docs I see reference to repos getting this set to 50
> since a particular gitlab release.
>
> Likewise same question for Phil in ?
>
> https://gitlab.com/philmd_rh/qemu/-/settings/ci_cd
0 too.
[PATCH] tcg/arm: Increase stack alignment for function generation
This avoids the following assertion when the kernel initializes X.509 certificates: [7.315373] Loading compiled-in X.509 certificates qemu-system-arm: ../tcg/tcg.c:3063: temp_allocate_frame: Assertion `align <= TCG_TARGET_STACK_ALIGN' failed. Fixes: commit c1c091948ae Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1999878 Cc: qemu-sta...@nongnu.org Tested-by: Richard W.M. Jones Signed-off-by: Richard W.M. Jones --- tcg/arm/tcg-target.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tcg/arm/tcg-target.h b/tcg/arm/tcg-target.h index d113b7f8db..09df3b39a1 100644 --- a/tcg/arm/tcg-target.h +++ b/tcg/arm/tcg-target.h @@ -115,7 +115,7 @@ extern bool use_neon_instructions; #endif /* used for function call generation */ -#define TCG_TARGET_STACK_ALIGN 8 +#define TCG_TARGET_STACK_ALIGN 16 #define TCG_TARGET_CALL_ALIGN_ARGS 1 #define TCG_TARGET_CALL_STACK_OFFSET 0 -- 2.32.0
[PATCH] 9pfs: fix crash in v9fs_walk()
v9fs_walk() utilizes the v9fs_co_run_in_worker({...}) macro to run the
supplied fs driver code block on a background worker thread.
When either the 'Twalk' client request was interrupted or if the client
requested fid for that 'Twalk' request caused a stat error then that
fs driver code block was left by 'break' keyword, with the intention to
return from worker thread back to main thread as well:
v9fs_co_run_in_worker({
if (v9fs_request_cancelled(pdu)) {
err = -EINTR;
break;
}
err = s->ops->lstat(>ctx, , );
if (err < 0) {
err = -errno;
break;
}
...
});
However that 'break;' statement also skipped the v9fs_co_run_in_worker()
macro's final and mandatory
/* re-enter back to qemu thread */
qemu_coroutine_yield();
call and thus caused the rest of v9fs_walk() to be continued being
executed on the worker thread instead of main thread, eventually
leading to a crash in the transport virtio transport driver.
To fix this issue and to prevent the same error from happening again by
other users of v9fs_co_run_in_worker() in future, auto wrap the supplied
code block into its own
do { } while (0);
loop inside the 'v9fs_co_run_in_worker' macro definition.
Full discussion and backtrace:
https://lists.gnu.org/archive/html/qemu-devel/2021-08/msg05209.html
https://lists.gnu.org/archive/html/qemu-devel/2021-09/msg00174.html
Fixes: 8d6cb100731c4d28535adbf2a3c2d1f29be3fef4
Signed-off-by: Christian Schoenebeck
Cc: qemu-sta...@nongnu.org
---
hw/9pfs/coth.h | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/hw/9pfs/coth.h b/hw/9pfs/coth.h
index c51289903d..f83c7dda7b 100644
--- a/hw/9pfs/coth.h
+++ b/hw/9pfs/coth.h
@@ -51,7 +51,9 @@
*/ \
qemu_coroutine_yield(); \
qemu_bh_delete(co_bh); \
-code_block; \
+do {\
+code_block; \
+} while (0);\
/* re-enter back to qemu thread */ \
qemu_coroutine_yield(); \
} while (0)
--
2.20.1
Re: [PATCH] gitlab: Escape git-describe match pattern on Windows hosts
On Wed, Sep 01, 2021 at 06:27:37PM +0200, Cédric Le Goater wrote:
> On 9/1/21 6:24 PM, Philippe Mathieu-Daudé wrote:
> > On 9/1/21 5:53 PM, Daniel P. Berrangé wrote:
> >> On Wed, Sep 01, 2021 at 05:35:42PM +0200, Philippe Mathieu-Daudé wrote:
> >>> On 9/1/21 5:21 PM, Daniel P. Berrangé wrote:
> On Wed, Sep 01, 2021 at 04:17:48PM +0100, Peter Maydell wrote:
> > On Wed, 1 Sept 2021 at 15:59, Daniel P. Berrangé
> > wrote:
> >>
> >> On Wed, Sep 01, 2021 at 04:52:29PM +0200, Philippe Mathieu-Daudé wrote:
> >>> Properly escape git-describe 'match' pattern to avoid (MinGW):
> >>>
> >>> $ if grep -q "EXESUF=.exe" config-host.mak; then make installer;
> >>> version="$(git describe --match v[0-9]*)";
> >>> mv -v qemu-setup*.exe qemu-setup-${version}.exe; fi
> >>> fatal: No names found, cannot describe anything.
> >>> ERROR: Job failed: exit code 1
> >>>
> >>> Reported-by: Cédric Le Goater
> >>> Fixes: 8619b5ddb56 ("ci: build & store windows installer")
> >>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/591
> >>> Signed-off-by: Philippe Mathieu-Daudé
> >>> ---
> >>> .gitlab-ci.d/crossbuild-template.yml | 2 +-
> >>> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/.gitlab-ci.d/crossbuild-template.yml
> >>> b/.gitlab-ci.d/crossbuild-template.yml
> >>> index 10d22dcf6c1..62d33e6661d 100644
> >>> --- a/.gitlab-ci.d/crossbuild-template.yml
> >>> +++ b/.gitlab-ci.d/crossbuild-template.yml
> >>> @@ -14,7 +14,7 @@
> >>> - make -j$(expr $(nproc) + 1) all check-build $MAKE_CHECK_ARGS
> >>> - if grep -q "EXESUF=.exe" config-host.mak;
> >>>then make installer;
> >>> - version="$(git describe --match v[0-9]*)";
> >>> + version="$(git describe --match 'v[0-9]*')";
> >>
> >> Do you have a pointer to a pipeline showing this fix works ?
> >>>
> >>> It worked on my fork but I have some versioned tag:
> >>> https://gitlab.com/philmd_rh/qemu/-/jobs/1553450025
> >>
> >> I can reproduce the error msg if I do a shallow clone with no history
> >>
> >> $ cd qemu
> >> $ git describe --match v[0-9]*
> >> v6.1.0-171-g5e8c1a0c90
> >>
> >> $ cd ..
> >> $ git clone --depth 1 https://gitlab.com/qemu-project/qemu/ qemu.new
> >> $ cd qemu.new/
> >> $ git describe --match v[0-9]*
> >> fatal: No names found, cannot describe anything.
> >>
> >> but the odd thing is that I think we should have been hitting the
> >> problem frequently if it was related to git depth. This job passes
> >> fine in current CI pipelines and my own fork.
> >
> > FYI it didn't work out on Cédric fork:
> > https://gitlab.com/legoater/qemu/-/jobs/1553492082
> >
>
> Indeed.
I'm curious if you go to
https://gitlab.com/legoater/qemu/-/settings/ci_cd
and expand "General pipelines", what value is set for the
"Git shallow clone"
setting. In my fork it is 0 which means unlimited depth, but in
gitlab docs I see reference to repos getting this set to 50
since a particular gitlab release.
Likewise same question for Phil in ?
https://gitlab.com/philmd_rh/qemu/-/settings/ci_cd
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: 9pfs: Twalk crash
On Wed, 01 Sep 2021 18:07:39 +0200
Christian Schoenebeck wrote:
> On Mittwoch, 1. September 2021 17:41:02 CEST Greg Kurz wrote:
> > On Wed, 01 Sep 2021 16:21:06 +0200
> >
> > Christian Schoenebeck wrote:
> > > On Mittwoch, 1. September 2021 14:49:37 CEST Christian Schoenebeck wrote:
> > > > > > And it triggered, however I am not sure if some of those functions I
> > > > > > asserted above are indeed allowed to be executed on a different
> > > > > > thread
> > > > > > than main thread:
> > > > > >
> > > > > > Program terminated with signal SIGABRT, Aborted.
> > > > > > #0 __GI_raise (sig=sig@entry=6) at
> > > > > > ../sysdeps/unix/sysv/linux/raise.c:50
> > > > > > 50 ../sysdeps/unix/sysv/linux/raise.c: No such file or
> > > > > > directory.
> > > > > > [Current thread is 1 (Thread 0x7fd0bcef1700 (LWP 6470))]
> > > > >
> > > > > Based in the thread number, it seems that the signal was raised by
> > > > > the main event thread...
> > > >
> > > > No, it was not main thread actually, gdb's "current thread is 1" output
> > > > is
> > > > misleading.
> > > >
> > > > Following the thread id trace, I extended the thread assertion checks
> > > > over
> > > > to v9fs_walk() as well, like this:
> > > >
> > > > static void coroutine_fn v9fs_walk(void *opaque)
> > > > {
> > > >
> > > > ...
> > > > assert_thread();
> > > > v9fs_co_run_in_worker({
> > > >
> > > > ...
> > > >
> > > > });
> > > > assert_thread();
> > > > ...
> > > >
> > > > }
> > > >
> > > > and made sure the reference thread id to be compared is really the main
> > > > thread.
> > > >
> > > > And what happens here is before v9fs_co_run_in_worker() is entered,
> > > > v9fs_walk() runs on main thread, but after returning from
> > > > v9fs_co_run_in_worker() it runs on a different thread for some reason,
> > > > not
> > > > on main thread as it would be expected at that point.
> > >
> > > Ok, I think I found the root cause: the block is break;-ing out too far.
> > > The
> > That could explain the breakage indeed since the block you've added
> > to v9fs_walk() embeds a bunch of break statements. AFAICT this block
> > breaks on errors... do you know which one ?
>
> Yes, I've verified that. In my case an interrupt of Twalk triggered this bug.
> so it was this path exactly:
>
> v9fs_co_run_in_worker({
> if (v9fs_request_cancelled(pdu)) {
> ...
> break;
> }
> ...
> });
>
> so it was really this break;-ing too far being the root cause of the crash.
>
> > > following patch should fix it:
> > >
> > > diff --git a/hw/9pfs/coth.h b/hw/9pfs/coth.h
> > > index c51289903d..f83c7dda7b 100644
> > > --- a/hw/9pfs/coth.h
> > > +++ b/hw/9pfs/coth.h
> > > @@ -51,7 +51,9 @@
> > >
> > > */ \
> > >
> > > qemu_coroutine_yield(); \
> > > qemu_bh_delete(co_bh); \
> > >
> > > -code_block; \
> > > +do {\
> > > +code_block; \
> > > +} while (0);\
> >
> > Good.
> >
> > > /* re-enter back to qemu thread */ \
> > > qemu_coroutine_yield(); \
> > >
> > > } while (0)
> > >
> > > I haven't triggered a crash with that patch, but due to the occasional
> > > nature of this issue I'll give it some more spins before officially
> > > proclaiming it my bug. :)
> >
> > Well, this is a pre-existing limitation with v9fs_co_run_in_worker().
> > This wasn't documented as such and not really obvious to detect when
> > you optimized TWALK. We've never hit it before because the other
> > v9fs_co_run_in_worker() users don't have break statements.
>
> Yes, I know, this was my bad.
>
No, I mean the opposite actually. You shouldn't feel sorry to have
detected that this macro we're using everywhere is badly broken from
the beginning... even at the cost of a regression we'll fix shortly :)
> > But, indeed, this caused a regression in 6.1 so this will need a Fixes:
> > tag and Cc: qemu-stable.
>
> Yep, I'm preparing a patch now.
>
> Best regards,
> Christian Schoenebeck
>
>
Re: [PATCH] gitlab: Escape git-describe match pattern on Windows hosts
On 9/1/21 6:24 PM, Philippe Mathieu-Daudé wrote:
> On 9/1/21 5:53 PM, Daniel P. Berrangé wrote:
>> On Wed, Sep 01, 2021 at 05:35:42PM +0200, Philippe Mathieu-Daudé wrote:
>>> On 9/1/21 5:21 PM, Daniel P. Berrangé wrote:
On Wed, Sep 01, 2021 at 04:17:48PM +0100, Peter Maydell wrote:
> On Wed, 1 Sept 2021 at 15:59, Daniel P. Berrangé
> wrote:
>>
>> On Wed, Sep 01, 2021 at 04:52:29PM +0200, Philippe Mathieu-Daudé wrote:
>>> Properly escape git-describe 'match' pattern to avoid (MinGW):
>>>
>>> $ if grep -q "EXESUF=.exe" config-host.mak; then make installer;
>>> version="$(git describe --match v[0-9]*)";
>>> mv -v qemu-setup*.exe qemu-setup-${version}.exe; fi
>>> fatal: No names found, cannot describe anything.
>>> ERROR: Job failed: exit code 1
>>>
>>> Reported-by: Cédric Le Goater
>>> Fixes: 8619b5ddb56 ("ci: build & store windows installer")
>>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/591
>>> Signed-off-by: Philippe Mathieu-Daudé
>>> ---
>>> .gitlab-ci.d/crossbuild-template.yml | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/.gitlab-ci.d/crossbuild-template.yml
>>> b/.gitlab-ci.d/crossbuild-template.yml
>>> index 10d22dcf6c1..62d33e6661d 100644
>>> --- a/.gitlab-ci.d/crossbuild-template.yml
>>> +++ b/.gitlab-ci.d/crossbuild-template.yml
>>> @@ -14,7 +14,7 @@
>>> - make -j$(expr $(nproc) + 1) all check-build $MAKE_CHECK_ARGS
>>> - if grep -q "EXESUF=.exe" config-host.mak;
>>>then make installer;
>>> - version="$(git describe --match v[0-9]*)";
>>> + version="$(git describe --match 'v[0-9]*')";
>>
>> Do you have a pointer to a pipeline showing this fix works ?
>>>
>>> It worked on my fork but I have some versioned tag:
>>> https://gitlab.com/philmd_rh/qemu/-/jobs/1553450025
>>
>> I can reproduce the error msg if I do a shallow clone with no history
>>
>> $ cd qemu
>> $ git describe --match v[0-9]*
>> v6.1.0-171-g5e8c1a0c90
>>
>> $ cd ..
>> $ git clone --depth 1 https://gitlab.com/qemu-project/qemu/ qemu.new
>> $ cd qemu.new/
>> $ git describe --match v[0-9]*
>> fatal: No names found, cannot describe anything.
>>
>> but the odd thing is that I think we should have been hitting the
>> problem frequently if it was related to git depth. This job passes
>> fine in current CI pipelines and my own fork.
>
> FYI it didn't work out on Cédric fork:
> https://gitlab.com/legoater/qemu/-/jobs/1553492082
>
Indeed.
Thanks Phil,
C.
Re: [PATCH] tests/vhost-user-bridge.c: Fix typo in help message
On 9/1/21 5:27 PM, Peter Maydell wrote: > Fix a typo in the help message printed by vhost-user-bridge. > > Signed-off-by: Peter Maydell > --- > tests/vhost-user-bridge.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Reviewed-by: Philippe Mathieu-Daudé
Re: [PATCH v2 1/1] hw/arm/aspeed: Allow machine to set UART default
On 9/1/21 6:38 PM, Peter Delevoryas wrote:
>
>> On Sep 1, 2021, at 9:19 AM, Cédric Le Goater wrote:
>>
>> On 9/1/21 5:36 PM, p...@fb.com wrote:
>>> From: Peter Delevoryas
>>>
>>> When you run QEMU with an Aspeed machine and a single serial device
>>> using stdio like this:
>>>
>>>qemu -machine ast2600-evb -drive ... -serial stdio
>>>
>>> The guest OS can read and write to the UART5 registers at 0x1E784000 and
>>> it will receive from stdin and write to stdout. The Aspeed SoC's have a
>>> lot more UART's though (AST2500 has 5, AST2600 has 13) and depending on
>>> the board design, may be using any of them as the serial console. (See
>>> "stdout-path" in a DTS to check which one is chosen).
>>>
>>> Most boards, including all of those currently defined in
>>> hw/arm/aspeed.c, just use UART5, but some use UART1. This change adds
>>> some flexibility for different boards without requiring users to change
>>> their command-line invocation of QEMU.
>>>
>>> I tested this doesn't break existing code by booting an AST2500 OpenBMC
>>> image and an AST2600 OpenBMC image, each using UART5 as the console.
>>>
>>> Then I tested switching the default to UART1 and booting an AST2600
>>> OpenBMC image that uses UART1, and that worked too.
>>>
>>> Signed-off-by: Peter Delevoryas
>>
>> One comment, any how
>>
>> Reviewed-by: Cédric Le Goater
>>
>>> ---
>>> hw/arm/aspeed.c | 3 +++
>>> hw/arm/aspeed_ast2600.c | 8
>>> hw/arm/aspeed_soc.c | 9 ++---
>>> include/hw/arm/aspeed.h | 1 +
>>> include/hw/arm/aspeed_soc.h | 1 +
>>> 5 files changed, 15 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
>>> index 9d43e26c51..a81e90c539 100644
>>> --- a/hw/arm/aspeed.c
>>> +++ b/hw/arm/aspeed.c
>>> @@ -350,6 +350,8 @@ static void aspeed_machine_init(MachineState *machine)
>>> object_property_set_int(OBJECT(>soc), "hw-prot-key",
>>> ASPEED_SCU_PROT_KEY, _abort);
>>> }
>>> +qdev_prop_set_uint32(DEVICE(>soc), "uart-default",
>>> + amc->uart_default);
>>> qdev_realize(DEVICE(>soc), NULL, _abort);
>>>
>>> memory_region_add_subregion(get_system_memory(),
>>> @@ -804,6 +806,7 @@ static void aspeed_machine_class_init(ObjectClass *oc,
>>> void *data)
>>> mc->no_parallel = 1;
>>> mc->default_ram_id = "ram";
>>> amc->macs_mask = ASPEED_MAC0_ON;
>>> +amc->uart_default = ASPEED_DEV_UART5;
>>>
>>> aspeed_machine_class_props_init(oc);
>>> }
>>> diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
>>> index e3013128c6..b07fcf10a0 100644
>>> --- a/hw/arm/aspeed_ast2600.c
>>> +++ b/hw/arm/aspeed_ast2600.c
>>> @@ -322,10 +322,10 @@ static void aspeed_soc_ast2600_realize(DeviceState
>>> *dev, Error **errp)
>>> sysbus_connect_irq(SYS_BUS_DEVICE(>timerctrl), i, irq);
>>> }
>>>
>>> -/* UART - attach an 8250 to the IO space as our UART5 */
>>> -serial_mm_init(get_system_memory(), sc->memmap[ASPEED_DEV_UART5], 2,
>>> - aspeed_soc_get_irq(s, ASPEED_DEV_UART5),
>>> - 38400, serial_hd(0), DEVICE_LITTLE_ENDIAN);
>>> +/* UART - attach an 8250 to the IO space as our UART */
>>> +serial_mm_init(get_system_memory(), sc->memmap[s->uart_default], 2,
>>> + aspeed_soc_get_irq(s, s->uart_default), 38400,
>>> + serial_hd(0), DEVICE_LITTLE_ENDIAN);
>>>
>>> /* I2C */
>>> object_property_set_link(OBJECT(>i2c), "dram", OBJECT(s->dram_mr),
>>> diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
>>> index 3ad6c56fa9..09c0f83710 100644
>>> --- a/hw/arm/aspeed_soc.c
>>> +++ b/hw/arm/aspeed_soc.c
>>> @@ -287,9 +287,9 @@ static void aspeed_soc_realize(DeviceState *dev, Error
>>> **errp)
>>> sysbus_connect_irq(SYS_BUS_DEVICE(>timerctrl), i, irq);
>>> }
>>>
>>> -/* UART - attach an 8250 to the IO space as our UART5 */
>>> -serial_mm_init(get_system_memory(), sc->memmap[ASPEED_DEV_UART5], 2,
>>> - aspeed_soc_get_irq(s, ASPEED_DEV_UART5), 38400,
>>> +/* UART - attach an 8250 to the IO space as our UART */
>>> +serial_mm_init(get_system_memory(), sc->memmap[s->uart_default], 2,
>>> + aspeed_soc_get_irq(s, s->uart_default), 38400,
>>>serial_hd(0), DEVICE_LITTLE_ENDIAN);
>>>
>>> /* I2C */
>>> @@ -439,6 +439,8 @@ static void aspeed_soc_realize(DeviceState *dev, Error
>>> **errp)
>>> static Property aspeed_soc_properties[] = {
>>> DEFINE_PROP_LINK("dram", AspeedSoCState, dram_mr, TYPE_MEMORY_REGION,
>>> MemoryRegion *),
>>> +DEFINE_PROP_UINT32("uart-default", AspeedSoCState, uart_default,
>>> + ASPEED_DEV_UART5),
>>> DEFINE_PROP_END_OF_LIST(),
>>> };
>>>
>>> @@ -449,6 +451,7 @@ static void aspeed_soc_class_init(ObjectClass *oc, void
>>> *data)
>>> dc->realize = aspeed_soc_realize;
>>> /* Reason: Uses serial_hds and nd_table in realize()
[PATCH v4 1/3] memory: Extract mtree_info_flatview() from mtree_info()
While mtree_info() handles both ASes and flatviews cases,
the two cases share basically no code. Split mtree_info_flatview()
out of mtree_info() to simplify.
Note: Patch easier to review using 'git-diff --color-moved=blocks'.
Suggested-by: Peter Maydell
Signed-off-by: Philippe Mathieu-Daudé
---
softmmu/memory.c | 72 ++--
1 file changed, 39 insertions(+), 33 deletions(-)
diff --git a/softmmu/memory.c b/softmmu/memory.c
index bfedaf9c4df..3eb6f52de67 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -3246,6 +3246,44 @@ static gboolean mtree_info_flatview_free(gpointer key,
gpointer value,
return true;
}
+static void mtree_info_flatview(bool dispatch_tree, bool owner)
+{
+struct FlatViewInfo fvi = {
+.counter = 0,
+.dispatch_tree = dispatch_tree,
+.owner = owner,
+};
+AddressSpace *as;
+FlatView *view;
+GArray *fv_address_spaces;
+GHashTable *views = g_hash_table_new(g_direct_hash, g_direct_equal);
+AccelClass *ac = ACCEL_GET_CLASS(current_accel());
+
+if (ac->has_memory) {
+fvi.ac = ac;
+}
+
+/* Gather all FVs in one table */
+QTAILQ_FOREACH(as, _spaces, address_spaces_link) {
+view = address_space_get_flatview(as);
+
+fv_address_spaces = g_hash_table_lookup(views, view);
+if (!fv_address_spaces) {
+fv_address_spaces = g_array_new(false, false, sizeof(as));
+g_hash_table_insert(views, view, fv_address_spaces);
+}
+
+g_array_append_val(fv_address_spaces, as);
+}
+
+/* Print */
+g_hash_table_foreach(views, mtree_print_flatview, );
+
+/* Free */
+g_hash_table_foreach_remove(views, mtree_info_flatview_free, 0);
+g_hash_table_unref(views);
+}
+
void mtree_info(bool flatview, bool dispatch_tree, bool owner, bool disabled)
{
MemoryRegionListHead ml_head;
@@ -3253,39 +3291,7 @@ void mtree_info(bool flatview, bool dispatch_tree, bool
owner, bool disabled)
AddressSpace *as;
if (flatview) {
-FlatView *view;
-struct FlatViewInfo fvi = {
-.counter = 0,
-.dispatch_tree = dispatch_tree,
-.owner = owner,
-};
-GArray *fv_address_spaces;
-GHashTable *views = g_hash_table_new(g_direct_hash, g_direct_equal);
-AccelClass *ac = ACCEL_GET_CLASS(current_accel());
-
-if (ac->has_memory) {
-fvi.ac = ac;
-}
-
-/* Gather all FVs in one table */
-QTAILQ_FOREACH(as, _spaces, address_spaces_link) {
-view = address_space_get_flatview(as);
-
-fv_address_spaces = g_hash_table_lookup(views, view);
-if (!fv_address_spaces) {
-fv_address_spaces = g_array_new(false, false, sizeof(as));
-g_hash_table_insert(views, view, fv_address_spaces);
-}
-
-g_array_append_val(fv_address_spaces, as);
-}
-
-/* Print */
-g_hash_table_foreach(views, mtree_print_flatview, );
-
-/* Free */
-g_hash_table_foreach_remove(views, mtree_info_flatview_free, 0);
-g_hash_table_unref(views);
+mtree_info_flatview(dispatch_tree, owner);
return;
}
--
2.31.1
Re: [PATCH] gitlab: Escape git-describe match pattern on Windows hosts
On 9/1/21 6:27 PM, Cédric Le Goater wrote:
> On 9/1/21 6:24 PM, Philippe Mathieu-Daudé wrote:
>> On 9/1/21 5:53 PM, Daniel P. Berrangé wrote:
>>> On Wed, Sep 01, 2021 at 05:35:42PM +0200, Philippe Mathieu-Daudé wrote:
On 9/1/21 5:21 PM, Daniel P. Berrangé wrote:
> On Wed, Sep 01, 2021 at 04:17:48PM +0100, Peter Maydell wrote:
>> On Wed, 1 Sept 2021 at 15:59, Daniel P. Berrangé
>> wrote:
>>>
>>> On Wed, Sep 01, 2021 at 04:52:29PM +0200, Philippe Mathieu-Daudé wrote:
Properly escape git-describe 'match' pattern to avoid (MinGW):
$ if grep -q "EXESUF=.exe" config-host.mak; then make installer;
version="$(git describe --match v[0-9]*)";
mv -v qemu-setup*.exe qemu-setup-${version}.exe; fi
fatal: No names found, cannot describe anything.
ERROR: Job failed: exit code 1
Reported-by: Cédric Le Goater
Fixes: 8619b5ddb56 ("ci: build & store windows installer")
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/591
Signed-off-by: Philippe Mathieu-Daudé
---
.gitlab-ci.d/crossbuild-template.yml | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/.gitlab-ci.d/crossbuild-template.yml
b/.gitlab-ci.d/crossbuild-template.yml
index 10d22dcf6c1..62d33e6661d 100644
--- a/.gitlab-ci.d/crossbuild-template.yml
+++ b/.gitlab-ci.d/crossbuild-template.yml
@@ -14,7 +14,7 @@
- make -j$(expr $(nproc) + 1) all check-build $MAKE_CHECK_ARGS
- if grep -q "EXESUF=.exe" config-host.mak;
then make installer;
- version="$(git describe --match v[0-9]*)";
+ version="$(git describe --match 'v[0-9]*')";
>>>
>>> Do you have a pointer to a pipeline showing this fix works ?
It worked on my fork but I have some versioned tag:
https://gitlab.com/philmd_rh/qemu/-/jobs/1553450025
>>>
>>> I can reproduce the error msg if I do a shallow clone with no history
>>>
>>> $ cd qemu
>>> $ git describe --match v[0-9]*
>>> v6.1.0-171-g5e8c1a0c90
>>>
>>> $ cd ..
>>> $ git clone --depth 1 https://gitlab.com/qemu-project/qemu/ qemu.new
>>> $ cd qemu.new/
>>> $ git describe --match v[0-9]*
>>> fatal: No names found, cannot describe anything.
>>>
>>> but the odd thing is that I think we should have been hitting the
>>> problem frequently if it was related to git depth. This job passes
>>> fine in current CI pipelines and my own fork.
What about not using the best tag if no versioned tag available?
(no default string in case user wants to download and archive artifacts)
-- >8 --
@@ -14,7 +14,7 @@
- make -j$(expr $(nproc) + 1) all check-build $MAKE_CHECK_ARGS
- if grep -q "EXESUF=.exe" config-host.mak;
then make installer;
- version="$(git describe --match v[0-9]*)";
+ version="$(git describe --tags --match 'v[0-9]*' 2>/dev/null ||
git describe --abbrev)";
mv -v qemu-setup*.exe qemu-setup-${version}.exe;
fi
---
>> FYI it didn't work out on Cédric fork:
>> https://gitlab.com/legoater/qemu/-/jobs/1553492082
>>
>
> Indeed.
>
> Thanks Phil,
>
> C.
>
Re: 9pfs: Twalk crash
On Mittwoch, 1. September 2021 17:41:02 CEST Greg Kurz wrote:
> On Wed, 01 Sep 2021 16:21:06 +0200
>
> Christian Schoenebeck wrote:
> > On Mittwoch, 1. September 2021 14:49:37 CEST Christian Schoenebeck wrote:
> > > > > And it triggered, however I am not sure if some of those functions I
> > > > > asserted above are indeed allowed to be executed on a different
> > > > > thread
> > > > > than main thread:
> > > > >
> > > > > Program terminated with signal SIGABRT, Aborted.
> > > > > #0 __GI_raise (sig=sig@entry=6) at
> > > > > ../sysdeps/unix/sysv/linux/raise.c:50
> > > > > 50 ../sysdeps/unix/sysv/linux/raise.c: No such file or
> > > > > directory.
> > > > > [Current thread is 1 (Thread 0x7fd0bcef1700 (LWP 6470))]
> > > >
> > > > Based in the thread number, it seems that the signal was raised by
> > > > the main event thread...
> > >
> > > No, it was not main thread actually, gdb's "current thread is 1" output
> > > is
> > > misleading.
> > >
> > > Following the thread id trace, I extended the thread assertion checks
> > > over
> > > to v9fs_walk() as well, like this:
> > >
> > > static void coroutine_fn v9fs_walk(void *opaque)
> > > {
> > >
> > > ...
> > > assert_thread();
> > > v9fs_co_run_in_worker({
> > >
> > > ...
> > >
> > > });
> > > assert_thread();
> > > ...
> > >
> > > }
> > >
> > > and made sure the reference thread id to be compared is really the main
> > > thread.
> > >
> > > And what happens here is before v9fs_co_run_in_worker() is entered,
> > > v9fs_walk() runs on main thread, but after returning from
> > > v9fs_co_run_in_worker() it runs on a different thread for some reason,
> > > not
> > > on main thread as it would be expected at that point.
> >
> > Ok, I think I found the root cause: the block is break;-ing out too far.
> > The
> That could explain the breakage indeed since the block you've added
> to v9fs_walk() embeds a bunch of break statements. AFAICT this block
> breaks on errors... do you know which one ?
Yes, I've verified that. In my case an interrupt of Twalk triggered this bug.
so it was this path exactly:
v9fs_co_run_in_worker({
if (v9fs_request_cancelled(pdu)) {
...
break;
}
...
});
so it was really this break;-ing too far being the root cause of the crash.
> > following patch should fix it:
> >
> > diff --git a/hw/9pfs/coth.h b/hw/9pfs/coth.h
> > index c51289903d..f83c7dda7b 100644
> > --- a/hw/9pfs/coth.h
> > +++ b/hw/9pfs/coth.h
> > @@ -51,7 +51,9 @@
> >
> > */ \
> >
> > qemu_coroutine_yield(); \
> > qemu_bh_delete(co_bh); \
> >
> > -code_block; \
> > +do {\
> > +code_block; \
> > +} while (0);\
>
> Good.
>
> > /* re-enter back to qemu thread */ \
> > qemu_coroutine_yield(); \
> >
> > } while (0)
> >
> > I haven't triggered a crash with that patch, but due to the occasional
> > nature of this issue I'll give it some more spins before officially
> > proclaiming it my bug. :)
>
> Well, this is a pre-existing limitation with v9fs_co_run_in_worker().
> This wasn't documented as such and not really obvious to detect when
> you optimized TWALK. We've never hit it before because the other
> v9fs_co_run_in_worker() users don't have break statements.
Yes, I know, this was my bad.
> But, indeed, this caused a regression in 6.1 so this will need a Fixes:
> tag and Cc: qemu-stable.
Yep, I'm preparing a patch now.
Best regards,
Christian Schoenebeck
[PATCH] multifd: Implement yank for multifd send side
When introducing yank functionality in the migration code I forgot
to cover the multifd send side.
Signed-off-by: Lukas Straub
Tested-by: Leonardo Bras
Reviewed-by: Leonardo Bras
---
-v2:
-add Tested-by and Reviewed-by tags
migration/multifd.c | 6 +-
migration/multifd.h | 2 ++
2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/migration/multifd.c b/migration/multifd.c
index 377da78f5b..5a4f158f3c 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -546,6 +546,9 @@ void multifd_save_cleanup(void)
MultiFDSendParams *p = _send_state->params[i];
Error *local_err = NULL;
+if (p->registered_yank) {
+migration_ioc_unregister_yank(p->c);
+}
socket_send_channel_destroy(p->c);
p->c = NULL;
qemu_mutex_destroy(>mutex);
@@ -813,7 +816,8 @@ static bool multifd_channel_connect(MultiFDSendParams *p,
return false;
}
} else {
-/* update for tls qio channel */
+migration_ioc_register_yank(ioc);
+p->registered_yank = true;
p->c = ioc;
qemu_thread_create(>thread, p->name, multifd_send_thread, p,
QEMU_THREAD_JOINABLE);
diff --git a/migration/multifd.h b/migration/multifd.h
index 8d6751f5ed..16c4d112d1 100644
--- a/migration/multifd.h
+++ b/migration/multifd.h
@@ -85,6 +85,8 @@ typedef struct {
bool running;
/* should this thread finish */
bool quit;
+/* is the yank function registered */
+bool registered_yank;
/* thread has work to do */
int pending_job;
/* array of pages to sent */
--
2.32.0
pgpLt11uT5f9F.pgp
Description: OpenPGP digital signature
Re: [PATCH v2 0/1] hw/arm/aspeed: Allow machine to set UART default
On 9/1/21 5:36 PM, p...@fb.com wrote: > From: Peter Delevoryas > > v1: https://lore.kernel.org/qemu-devel/20210831233140.2659116-1-p...@fb.com/ > v2: > - Replaced AspeedMachineClass "serial_hd0" with "uart_default" > - Removed "qdev_get_machine()" usage > - Removed unnecessary aspeed.h (machine class) includes in device files > - Added "uint32_t uart_default" to AspeedSoCState > - Added "uart-default" uint32 property to AspeedSoCState > - Set "uart-default" just before qdev_realize() > > NOTE: Still not totally sure I did this right, especially because I only > initialized the properties in the aspeed_soc.c file (2400 + 2500), but > not aspeed_ast2600.c (2600), but I guess that's because > aspeed_soc_class_init is common to all the SoC's. > > Peter Delevoryas (1): > hw/arm/aspeed: Allow machine to set UART default > > hw/arm/aspeed.c | 3 +++ > hw/arm/aspeed_ast2600.c | 8 > hw/arm/aspeed_soc.c | 9 ++--- > include/hw/arm/aspeed.h | 1 + > include/hw/arm/aspeed_soc.h | 1 + > 5 files changed, 15 insertions(+), 7 deletions(-) > > Interdiff against v1: [...] Not needed because QEMU uses patchew :) https://patchew.org/QEMU/20210831233140.2659116-1-p...@fb.com/diff/20210901153615.2746885-1-p...@fb.com/
[PATCH v4 0/3] memory: Have 'info mtree' remove duplicated Address Space information
Follow Peter Maydell suggestions: - Split mtree_info() as mtree_info_flatview() + mtree_info_as() - Remove duplicated Address Space information Since v3: - Fix typos - Split mtree_info_flatview() + mtree_info_as() first - Rebased last patch keeping Peter's R-b tag Philippe Mathieu-Daudé (3): memory: Extract mtree_info_flatview() from mtree_info() memory: Extract mtree_info_as() from mtree_info() memory: Have 'info mtree' remove duplicated Address Space information softmmu/memory.c | 160 ++- 1 file changed, 118 insertions(+), 42 deletions(-) -- 2.31.1
Re: [PATCH v1 2/3] io: Add zerocopy and errqueue
On Wed, Sep 01, 2021 at 09:50:56AM +0100, Daniel P. Berrangé wrote: > On Tue, Aug 31, 2021 at 04:27:04PM -0400, Peter Xu wrote: > > On Tue, Aug 31, 2021 at 01:57:33PM +0100, Daniel P. Berrangé wrote: > > > On Tue, Aug 31, 2021 at 08:02:38AM -0300, Leonardo Bras wrote: > > > > MSG_ZEROCOPY is a feature that enables copy avoidance in TCP/UDP socket > > > > send calls. It does so by avoiding copying user data into kernel > > > > buffers. > > > > > > > > To make it work, three steps are needed: > > > > 1 - A setsockopt() system call, enabling SO_ZEROCOPY > > > > 2 - Passing down the MSG_ZEROCOPY flag for each send*() syscall > > > > 3 - Process the socket's error queue, dealing with any error > > > > > > AFAICT, this is missing the single most critical aspect of MSG_ZEROCOPY. > > > > > > It is non-obvious, but setting the MSG_ZEROCOPY flag turns sendmsg() > > > from a synchronous call to an asynchronous call. > > > > > > It is forbidden to overwrite/reuse/free the buffer passed to sendmsg > > > until an asynchronous completion notification has been received from > > > the socket error queue. These notifications are not required to > > > arrive in-order, even for a TCP stream, because the kernel hangs on > > > to the buffer if a re-transmit is needed. > > > > > > https://www.kernel.org/doc/html/v5.4/networking/msg_zerocopy.html > > > > > > "Page pinning also changes system call semantics. It temporarily > > >shares the buffer between process and network stack. Unlike with > > >copying, the process cannot immediately overwrite the buffer > > >after system call return without possibly modifying the data in > > >flight. Kernel integrity is not affected, but a buggy program > > >can possibly corrupt its own data stream." > > > > > > AFAICT, the design added in this patch does not provide any way > > > to honour these requirements around buffer lifetime. > > > > > > I can't see how we can introduce MSG_ZEROCOPY in any seemless > > > way. The buffer lifetime requirements imply need for an API > > > design that is fundamentally different for asynchronous usage, > > > with a callback to notify when the write has finished/failed. > > > > Regarding buffer reuse - it indeed has a very deep implication on the buffer > > being available and it's not obvious at all. Just to mention that the > > initial > > user of this work will make sure all zero copy buffers will be guest pages > > only > > (as it's only used in multi-fd), so they should always be there during the > > process. > > That is not the case when migration is using TLS, because the buffers > transmitted are the ciphertext buffer, not the plaintext guest page. Agreed. > > > In short, we may just want to at least having a way to make sure all zero > > copied buffers are finished using and they're sent after some function > > returns > > (e.g., qio_channel_flush()). That may require us to do some accounting on > > when > > we called sendmsg(MSG_ZEROCOPY), meanwhile we should need to read out the > > ee_data field within SO_EE_ORIGIN_ZEROCOPY msg when we do recvmsg() for the > > error queue and keep those information somewhere too. > > > > Some other side notes that reached my mind.. > > > > The qio_channel_writev_full() may not be suitable for async operations, as > > the > > name "full" implies synchronous to me. So maybe we can add a new helper for > > zero copy on the channel? > > All the APIs that exist today are fundamentally only suitable for sync > operations. Supporting async correctly will definitely a brand new APIs > separate from what exists today. Yes. What I wanted to say is maybe we can still keep the io_writev() interface untouched, but just add a new interface at qio_channel_writev_full() level. IOW, we could comment on io_writev() that it can be either sync or async now, just like sendmsg() has that implication too with different flag passed in. When calling io_writev() with different upper helpers, QIO channel could identify what flag to pass over to io_writev(). -- Peter Xu
Re: [PATCH] gitlab: Escape git-describe match pattern on Windows hosts
On 9/1/21 5:21 PM, Daniel P. Berrangé wrote:
> On Wed, Sep 01, 2021 at 04:17:48PM +0100, Peter Maydell wrote:
>> On Wed, 1 Sept 2021 at 15:59, Daniel P. Berrangé wrote:
>>>
>>> On Wed, Sep 01, 2021 at 04:52:29PM +0200, Philippe Mathieu-Daudé wrote:
Properly escape git-describe 'match' pattern to avoid (MinGW):
$ if grep -q "EXESUF=.exe" config-host.mak; then make installer;
version="$(git describe --match v[0-9]*)";
mv -v qemu-setup*.exe qemu-setup-${version}.exe; fi
fatal: No names found, cannot describe anything.
ERROR: Job failed: exit code 1
Reported-by: Cédric Le Goater
Fixes: 8619b5ddb56 ("ci: build & store windows installer")
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/591
Signed-off-by: Philippe Mathieu-Daudé
---
.gitlab-ci.d/crossbuild-template.yml | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/.gitlab-ci.d/crossbuild-template.yml
b/.gitlab-ci.d/crossbuild-template.yml
index 10d22dcf6c1..62d33e6661d 100644
--- a/.gitlab-ci.d/crossbuild-template.yml
+++ b/.gitlab-ci.d/crossbuild-template.yml
@@ -14,7 +14,7 @@
- make -j$(expr $(nproc) + 1) all check-build $MAKE_CHECK_ARGS
- if grep -q "EXESUF=.exe" config-host.mak;
then make installer;
- version="$(git describe --match v[0-9]*)";
+ version="$(git describe --match 'v[0-9]*')";
>>>
>>> Do you have a pointer to a pipeline showing this fix works ?
It worked on my fork but I have some versioned tag:
https://gitlab.com/philmd_rh/qemu/-/jobs/1553450025
Cédric, do you mind testing on your fork?
>>>
>>> It is a bit strange to me. AFAICT, the only difference would
>>> be if the unquoted v[0-9]* matched a filename in the
>>> current directory, but that doesn't seem like it is the
>>> case here.
>>
>> We should quote the glob pattern anyway, to avoid possible
>> really confusing behaviour in the future if such a file ever
>> does turn up...
>
> Sure, I'm happy to see the thing quoted regardless, just want to
> make sure the commit behaviour matches the commit message.
>
>
> Regards,
> Daniel
>
Re: [PATCH v3] memory: Have 'info mtree' remove duplicated Address Space information
On 8/23/21 11:20 AM, David Hildenbrand wrote: > On 23.08.21 10:54, Philippe Mathieu-Daudé wrote: >> Per Peter Maydell [*]: >> >> 'info mtree' monitor command was designed on the assumption that >> there's really only one or two interesting address spaces, and >> with more recent developments that's just not the case any more. >> >> Similarly about how the FlatView are sorted using a GHashTable, >> sort the AddressSpace objects to remove the duplications (AS >> using the same root MemoryRegion). >> >> This drastically reduce 'info mtree' on some boards. > > s/reduce/reduces the output of/ > >> >> Before: >> >> $ (echo info mtree; echo q) \ >> | qemu-system-aarch64 -S -monitor stdio -M raspi3b \ >> | wc -l >> 423 >> >> After: >> >> $ (echo info mtree; echo q) \ >> | qemu-system-aarch64 -S -monitor stdio -M raspi3b \ >> | wc -l >> 106 >> >> (qemu) info mtree >> address-space: I/O >> - (prio 0, i/o): io >> >> address-space: cpu-memory-0 >> address-space: cpu-memory-1 >> address-space: cpu-memory-2 >> address-space: cpu-memory-3 >> address-space: cpu-secure-memory-0 >> address-space: cpu-secure-memory-1 >> address-space: cpu-secure-memory-2 >> address-space: cpu-secure-memory-3 > > We can still distinguish from a completely empty AS, because we don't > have an empty line here, correct? Yes: (qemu) info mtree address-space: I/O - (prio 0, i/o): io address-space shared 4 times: - bcm2835-dma-memory - bcm2835-fb-memory - bcm2835-property-memory - dwc2 - (prio 0, i/o): bcm2835-gpu -3fff (prio 0, ram): alias bcm2835-gpu-ram-alias[*] @ram -3fff 4000-7fff (prio 0, ram): alias bcm2835-gpu-ram-alias[*] @ram -3fff 7e00-7eff (prio 1, i/o): alias bcm2835-peripherals @bcm2835-peripherals -00ff 8000-bfff (prio 0, ram): alias bcm2835-gpu-ram-alias[*] @ram -3fff c000- (prio 0, ram): alias bcm2835-gpu-ram-alias[*] @ram -3fff address-space: bcm2835-mbox-memory -008f (prio 0, i/o): bcm2835-mbox 0010-001f (prio 0, i/o): bcm2835-fb 0080-008f (prio 0, i/o): bcm2835-property [...]
[PATCH] tests/vhost-user-bridge.c: Sanity check socket path length
The vhost-user-bridge binary accepts a UNIX socket path on
the command line. Sanity check that this is short enough to
fit into a sockaddr_un before copying it in.
Fixes: Coverity CID 1432866
Signed-off-by: Peter Maydell
---
tests/vhost-user-bridge.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/tests/vhost-user-bridge.c b/tests/vhost-user-bridge.c
index 24815920b2b..cb009545fa5 100644
--- a/tests/vhost-user-bridge.c
+++ b/tests/vhost-user-bridge.c
@@ -540,6 +540,11 @@ vubr_new(const char *path, bool client)
CallbackFunc cb;
size_t len;
+if (strlen(path) >= sizeof(un.sun_path)) {
+fprintf(stderr, "unix domain socket path '%s' is too long\n", path);
+exit(1);
+}
+
/* Get a UNIX socket. */
dev->sock = socket(AF_UNIX, SOCK_STREAM, 0);
if (dev->sock == -1) {
--
2.20.1
Re: [RFC PATCH 3/3] hw/virtio: Have virtqueue_get_avail_bytes() pass caches arg to callees
On Thu, Aug 26, 2021 at 07:26:58PM +0200, Philippe Mathieu-Daudé wrote:
Both virtqueue_packed_get_avail_bytes() and
virtqueue_split_get_avail_bytes() access the region cache, but
their caller also does. Simplify by having virtqueue_get_avail_bytes
calling both with RCU lock held, and passing the caches as argument.
Signed-off-by: Philippe Mathieu-Daudé
---
RFC because I'm not sure this is safe enough
It seems safe to me.
While reviewing I saw that vring_get_region_caches() has
/* Called within rcu_read_lock(). */ comment, but it seems to me that
we call that function in places where we haven't acquired it, which
shouldn't be a problem, but should we remove that comment?
Thanks,
Stefano
---
hw/virtio/virtio.c | 29 -
1 file changed, 12 insertions(+), 17 deletions(-)
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 3a1f6c520cb..8237693a567 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -984,28 +984,23 @@ static int virtqueue_split_read_next_desc(VirtIODevice
*vdev, VRingDesc *desc,
return VIRTQUEUE_READ_DESC_MORE;
}
+/* Called within rcu_read_lock(). */
static void virtqueue_split_get_avail_bytes(VirtQueue *vq,
unsigned int *in_bytes, unsigned int *out_bytes,
-unsigned max_in_bytes, unsigned max_out_bytes)
+unsigned max_in_bytes, unsigned max_out_bytes,
+VRingMemoryRegionCaches *caches)
{
VirtIODevice *vdev = vq->vdev;
unsigned int max, idx;
unsigned int total_bufs, in_total, out_total;
-VRingMemoryRegionCaches *caches;
MemoryRegionCache indirect_desc_cache = MEMORY_REGION_CACHE_INVALID;
int64_t len = 0;
int rc;
-RCU_READ_LOCK_GUARD();
-
idx = vq->last_avail_idx;
total_bufs = in_total = out_total = 0;
max = vq->vring.num;
-caches = vring_get_region_caches(vq);
-if (!caches) {
-goto err;
-}
while ((rc = virtqueue_num_heads(vq, idx)) > 0) {
MemoryRegionCache *desc_cache = >desc;
@@ -1124,32 +1119,28 @@ static int virtqueue_packed_read_next_desc(VirtQueue
*vq,
return VIRTQUEUE_READ_DESC_MORE;
}
+/* Called within rcu_read_lock(). */
static void virtqueue_packed_get_avail_bytes(VirtQueue *vq,
unsigned int *in_bytes,
unsigned int *out_bytes,
unsigned max_in_bytes,
- unsigned max_out_bytes)
+ unsigned max_out_bytes,
+ VRingMemoryRegionCaches *caches)
{
VirtIODevice *vdev = vq->vdev;
unsigned int max, idx;
unsigned int total_bufs, in_total, out_total;
MemoryRegionCache *desc_cache;
-VRingMemoryRegionCaches *caches;
MemoryRegionCache indirect_desc_cache = MEMORY_REGION_CACHE_INVALID;
int64_t len = 0;
VRingPackedDesc desc;
bool wrap_counter;
-RCU_READ_LOCK_GUARD();
idx = vq->last_avail_idx;
wrap_counter = vq->last_avail_wrap_counter;
total_bufs = in_total = out_total = 0;
max = vq->vring.num;
-caches = vring_get_region_caches(vq);
-if (!caches) {
-goto err;
-}
for (;;) {
unsigned int num_bufs = total_bufs;
@@ -1250,6 +1241,8 @@ void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned
int *in_bytes,
uint16_t desc_size;
VRingMemoryRegionCaches *caches;
+RCU_READ_LOCK_GUARD();
+
if (unlikely(!vq->vring.desc)) {
goto err;
}
@@ -1268,10 +1261,12 @@ void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned
int *in_bytes,
if (virtio_vdev_has_feature(vq->vdev, VIRTIO_F_RING_PACKED)) {
virtqueue_packed_get_avail_bytes(vq, in_bytes, out_bytes,
- max_in_bytes, max_out_bytes);
+ max_in_bytes, max_out_bytes,
+ caches);
} else {
virtqueue_split_get_avail_bytes(vq, in_bytes, out_bytes,
-max_in_bytes, max_out_bytes);
+max_in_bytes, max_out_bytes,
+caches);
}
return;
--
2.31.1
Re: [PATCH v1 3/3] migration: multifd: Enable zerocopy
On Wed, Sep 01, 2021 at 04:44:30PM +0100, Daniel P. Berrangé wrote: > QEMU has mptcp support already: > > commit 8bd1078aebcec5eac196a83ef1a7e74be0ba67b7 > Author: Dr. David Alan Gilbert > Date: Wed Apr 21 12:28:34 2021 +0100 > > sockets: Support multipath TCP > > Multipath TCP allows combining multiple interfaces/routes into a single > socket, with very little work for the user/admin. > > It's enabled by 'mptcp' on most socket addresses: > >./qemu-system-x86_64 -nographic -incoming tcp:0:,mptcp Oops, I totally forgot about that, sorry! > > > KTLS may be implicitly included by a new gnutls, but we need to mark TLS and > > ZEROCOPY mutual exclusive anyway because at least the userspace TLS code of > > gnutls won't has a way to maintain the tls buffers used by zerocopy. So at > > least we need some knob to detect whether kTLS is enabled in gnutls. > > It isn't possible for gnutls to transparently enable KTLS, because > GNUTLS doesn't get to see the actual socket directly - it'll need > some work in QEMU to enable it. We know MPTCP and KTLS are currently > mutually exclusive as they both use the same kernel network hooks > framework. Then we may need to at least figure out whether zerocopy needs to mask out mptcp. -- Peter Xu
Re: [PATCH 1/2] elf2dmp: Check curl_easy_setopt() return value
On 9/1/21 4:39 PM, Peter Maydell wrote:
> Coverity points out that we aren't checking the return value
> from curl_easy_setopt().
>
> Fixes: Coverity CID 1458895
> Signed-off-by: Peter Maydell
> ---
> contrib/elf2dmp/download.c | 28 +---
> 1 file changed, 17 insertions(+), 11 deletions(-)
>
> diff --git a/contrib/elf2dmp/download.c b/contrib/elf2dmp/download.c
> index d09e607431f..01e4a7fc0dc 100644
> --- a/contrib/elf2dmp/download.c
> +++ b/contrib/elf2dmp/download.c
> @@ -21,21 +21,19 @@ int download_url(const char *name, const char *url)
>
> file = fopen(name, "wb");
> if (!file) {
> -err = 1;
> -goto out_curl;
> +goto fail;
> }
>
> -curl_easy_setopt(curl, CURLOPT_URL, url);
> -curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, NULL);
> -curl_easy_setopt(curl, CURLOPT_WRITEDATA, file);
> -curl_easy_setopt(curl, CURLOPT_FOLLOWLOCATION, 1);
> -curl_easy_setopt(curl, CURLOPT_NOPROGRESS, 0);
> +if (curl_easy_setopt(curl, CURLOPT_URL, url) != CURLE_OK ||
> +curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, NULL) != CURLE_OK ||
> +curl_easy_setopt(curl, CURLOPT_WRITEDATA, file) != CURLE_OK ||
> +curl_easy_setopt(curl, CURLOPT_FOLLOWLOCATION, 1) != CURLE_OK ||
> +curl_easy_setopt(curl, CURLOPT_NOPROGRESS, 0) != CURLE_OK) {
> +goto fail;
> +}
>
> if (curl_easy_perform(curl) != CURLE_OK) {
> -err = 1;
> -fclose(file);
> -unlink(name);
> -goto out_curl;
> +goto fail;
> }
>
> err = fclose(file);
> @@ -44,4 +42,12 @@ out_curl:
> curl_easy_cleanup(curl);
>
> return err;
> +
> +fail:
> +err = 1;
> +if (file) {
> +fclose(file);
> +unlink(name);
> +}
> +goto out_curl;
> }
>
Counter proposal without goto and less ifs:
-- >8 --
@@ -25,21 +25,19 @@ int download_url(const char *name, const char *url)
goto out_curl;
}
-curl_easy_setopt(curl, CURLOPT_URL, url);
-curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, NULL);
-curl_easy_setopt(curl, CURLOPT_WRITEDATA, file);
-curl_easy_setopt(curl, CURLOPT_FOLLOWLOCATION, 1);
-curl_easy_setopt(curl, CURLOPT_NOPROGRESS, 0);
-
-if (curl_easy_perform(curl) != CURLE_OK) {
-err = 1;
-fclose(file);
+if (curl_easy_setopt(curl, CURLOPT_URL, url) != CURLE_OK
+|| curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, NULL) !=
CURLE_OK
+|| curl_easy_setopt(curl, CURLOPT_WRITEDATA, file) != CURLE_OK
+|| curl_easy_setopt(curl, CURLOPT_FOLLOWLOCATION, 1) !=
CURLE_OK
+|| curl_easy_setopt(curl, CURLOPT_NOPROGRESS, 0) != CURLE_OK
+|| curl_easy_perform(curl) != CURLE_OK) {
unlink(name);
-goto out_curl;
+fclose(file);
+err = 1;
+} else {
+err = fclose(file);
}
-err = fclose(file);
-
out_curl:
curl_easy_cleanup(curl);
---
Re: [PATCH for 6.1] multifd: Unconditionally unregister yank function
On Wed, 4 Aug 2021 21:26:32 +0200
Lukas Straub wrote:
> Unconditionally unregister yank function in multifd_load_cleanup().
> If it is not unregistered here, it will leak and cause a crash
> in yank_unregister_instance(). Now if the ioc is still in use
> afterwards, it will only lead to qemu not being able to recover
> from a hang related to that ioc.
>
> After checking the code, i am pretty sure that ref is always 1
> when arriving here. So all this currently does is remove the
> unneeded check.
>
> Signed-off-by: Lukas Straub
> ---
>
> This is similar to Peter Xu's
> 39675b3394d44b880d083a214c5e44786170
> "migration: Move the yank unregister of channel_close out"
> in that it removes the "OBJECT(p->c)->ref == 1" hack. So it
> makes sense for 6.1 so these patches are together.
>
> migration/multifd.c | 5 +
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/migration/multifd.c b/migration/multifd.c
> index 377da78f5b..a37805e17e 100644
> --- a/migration/multifd.c
> +++ b/migration/multifd.c
> @@ -987,10 +987,7 @@ int multifd_load_cleanup(Error **errp)
> for (i = 0; i < migrate_multifd_channels(); i++) {
> MultiFDRecvParams *p = _recv_state->params[i];
>
> -if (OBJECT(p->c)->ref == 1) {
> -migration_ioc_unregister_yank(p->c);
> -}
> -
> +migration_ioc_unregister_yank(p->c);
> object_unref(OBJECT(p->c));
> p->c = NULL;
> qemu_mutex_destroy(>mutex);
ping...
--
pgpdfLxCe6G_v.pgp
Description: OpenPGP digital signature
Re: [PATCH] gitlab: Escape git-describe match pattern on Windows hosts
On 9/1/21 5:53 PM, Daniel P. Berrangé wrote:
> On Wed, Sep 01, 2021 at 05:35:42PM +0200, Philippe Mathieu-Daudé wrote:
>> On 9/1/21 5:21 PM, Daniel P. Berrangé wrote:
>>> On Wed, Sep 01, 2021 at 04:17:48PM +0100, Peter Maydell wrote:
On Wed, 1 Sept 2021 at 15:59, Daniel P. Berrangé
wrote:
>
> On Wed, Sep 01, 2021 at 04:52:29PM +0200, Philippe Mathieu-Daudé wrote:
>> Properly escape git-describe 'match' pattern to avoid (MinGW):
>>
>> $ if grep -q "EXESUF=.exe" config-host.mak; then make installer;
>> version="$(git describe --match v[0-9]*)";
>> mv -v qemu-setup*.exe qemu-setup-${version}.exe; fi
>> fatal: No names found, cannot describe anything.
>> ERROR: Job failed: exit code 1
>>
>> Reported-by: Cédric Le Goater
>> Fixes: 8619b5ddb56 ("ci: build & store windows installer")
>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/591
>> Signed-off-by: Philippe Mathieu-Daudé
>> ---
>> .gitlab-ci.d/crossbuild-template.yml | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/.gitlab-ci.d/crossbuild-template.yml
>> b/.gitlab-ci.d/crossbuild-template.yml
>> index 10d22dcf6c1..62d33e6661d 100644
>> --- a/.gitlab-ci.d/crossbuild-template.yml
>> +++ b/.gitlab-ci.d/crossbuild-template.yml
>> @@ -14,7 +14,7 @@
>> - make -j$(expr $(nproc) + 1) all check-build $MAKE_CHECK_ARGS
>> - if grep -q "EXESUF=.exe" config-host.mak;
>>then make installer;
>> - version="$(git describe --match v[0-9]*)";
>> + version="$(git describe --match 'v[0-9]*')";
>
> Do you have a pointer to a pipeline showing this fix works ?
>>
>> It worked on my fork but I have some versioned tag:
>> https://gitlab.com/philmd_rh/qemu/-/jobs/1553450025
>
> I can reproduce the error msg if I do a shallow clone with no history
>
> $ cd qemu
> $ git describe --match v[0-9]*
> v6.1.0-171-g5e8c1a0c90
>
> $ cd ..
> $ git clone --depth 1 https://gitlab.com/qemu-project/qemu/ qemu.new
> $ cd qemu.new/
> $ git describe --match v[0-9]*
> fatal: No names found, cannot describe anything.
>
> but the odd thing is that I think we should have been hitting the
> problem frequently if it was related to git depth. This job passes
> fine in current CI pipelines and my own fork.
FYI it didn't work out on Cédric fork:
https://gitlab.com/legoater/qemu/-/jobs/1553492082
Re: [PATCH v1 3/3] migration: multifd: Enable zerocopy
On Wed, Sep 01, 2021 at 11:35:33AM -0400, Peter Xu wrote:
> On Wed, Sep 01, 2021 at 09:53:07AM +0100, Daniel P. Berrangé wrote:
> > On Tue, Aug 31, 2021 at 04:29:09PM -0400, Peter Xu wrote:
> > > On Tue, Aug 31, 2021 at 02:16:42PM +0100, Daniel P. Berrangé wrote:
> > > > On Tue, Aug 31, 2021 at 08:02:39AM -0300, Leonardo Bras wrote:
> > > > > Call qio_channel_set_zerocopy(true) in the start of every multifd
> > > > > thread.
> > > > >
> > > > > Change the send_write() interface of multifd, allowing it to pass down
> > > > > flags for qio_channel_write*().
> > > > >
> > > > > Pass down MSG_ZEROCOPY flag for sending memory pages, while keeping
> > > > > the
> > > > > other data being sent at the default copying approach.
> > > > >
> > > > > Signed-off-by: Leonardo Bras
> > > > > ---
> > > > > migration/multifd-zlib.c | 7 ---
> > > > > migration/multifd-zstd.c | 7 ---
> > > > > migration/multifd.c | 9 ++---
> > > > > migration/multifd.h | 3 ++-
> > > > > 4 files changed, 16 insertions(+), 10 deletions(-)
> > > >
> > > > > @@ -675,7 +676,8 @@ static void *multifd_send_thread(void *opaque)
> > > > > }
> > > > >
> > > > > if (used) {
> > > > > -ret = multifd_send_state->ops->send_write(p, used,
> > > > > _err);
> > > > > +ret = multifd_send_state->ops->send_write(p, used,
> > > > > MSG_ZEROCOPY,
> > > > > +
> > > > > _err);
> > > >
> > > > I don't think it is valid to unconditionally enable this feature due to
> > > > the
> > > > resource usage implications
> > > >
> > > > https://www.kernel.org/doc/html/v5.4/networking/msg_zerocopy.html
> > > >
> > > > "A zerocopy failure will return -1 with errno ENOBUFS. This happens
> > > >if the socket option was not set, the socket exceeds its optmem
> > > >limit or the user exceeds its ulimit on locked pages."
> > > >
> > > > The limit on locked pages is something that looks very likely to be
> > > > exceeded unless you happen to be running a QEMU config that already
> > > > implies locked memory (eg PCI assignment)
> > >
> > > Yes it would be great to be a migration capability in parallel to
> > > multifd. At
> > > initial phase if it's easy to be implemented on multi-fd only, we can add
> > > a
> > > dependency between the caps. In the future we can remove that dependency
> > > when
> > > the code is ready to go without multifd. Thanks,
> >
> > Also, I'm wondering how zerocopy support interacts with kernel support
> > for kTLS and multipath-TCP, both of which we want to be able to use
> > with migration.
>
> Copying Jason Wang for net implications between these features on kernel side
> and whether they can be enabled together (MSG_ZEROCOPY, mptcp, kTLS).
>
> From the safe side we may want to only enable one of them until we prove
> they'll work together I guess..
MPTCP is good when we're network limited for migration
KTLS will be good when we're CPU limited on AES for migration,
which is essentially always when TLS is used.
ZEROCOPY will be good when we're CPU limited for data copy
on migration, or to reduce the impact on other concurrent
VMs on the same CPUs.
Ultimately we woudld benefit from all of them at the same
time, if it were technically possible todo.
> Not a immediate concern as I don't really think any of them is really
> explicitly supported in qemu.
QEMU has mptcp support already:
commit 8bd1078aebcec5eac196a83ef1a7e74be0ba67b7
Author: Dr. David Alan Gilbert
Date: Wed Apr 21 12:28:34 2021 +0100
sockets: Support multipath TCP
Multipath TCP allows combining multiple interfaces/routes into a single
socket, with very little work for the user/admin.
It's enabled by 'mptcp' on most socket addresses:
./qemu-system-x86_64 -nographic -incoming tcp:0:,mptcp
> KTLS may be implicitly included by a new gnutls, but we need to mark TLS and
> ZEROCOPY mutual exclusive anyway because at least the userspace TLS code of
> gnutls won't has a way to maintain the tls buffers used by zerocopy. So at
> least we need some knob to detect whether kTLS is enabled in gnutls.
It isn't possible for gnutls to transparently enable KTLS, because
GNUTLS doesn't get to see the actual socket directly - it'll need
some work in QEMU to enable it. We know MPTCP and KTLS are currently
mutually exclusive as they both use the same kernel network hooks
framework.
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: [PATCH v4 1/3] memory: Extract mtree_info_flatview() from mtree_info()
On 9/1/21 6:19 PM, Philippe Mathieu-Daudé wrote:
> While mtree_info() handles both ASes and flatviews cases,
> the two cases share basically no code. Split mtree_info_flatview()
> out of mtree_info() to simplify.
>
> Note: Patch easier to review using 'git-diff --color-moved=blocks'.
Surprisingly git-format-patch choose a better algorithm automatically...
> Suggested-by: Peter Maydell
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> softmmu/memory.c | 72 ++--
> 1 file changed, 39 insertions(+), 33 deletions(-)
>
> diff --git a/softmmu/memory.c b/softmmu/memory.c
> index bfedaf9c4df..3eb6f52de67 100644
> --- a/softmmu/memory.c
> +++ b/softmmu/memory.c
> @@ -3246,6 +3246,44 @@ static gboolean mtree_info_flatview_free(gpointer key,
> gpointer value,
> return true;
> }
>
> +static void mtree_info_flatview(bool dispatch_tree, bool owner)
> +{
> +struct FlatViewInfo fvi = {
> +.counter = 0,
> +.dispatch_tree = dispatch_tree,
> +.owner = owner,
> +};
> +AddressSpace *as;
> +FlatView *view;
> +GArray *fv_address_spaces;
> +GHashTable *views = g_hash_table_new(g_direct_hash, g_direct_equal);
> +AccelClass *ac = ACCEL_GET_CLASS(current_accel());
> +
> +if (ac->has_memory) {
> +fvi.ac = ac;
> +}
> +
> +/* Gather all FVs in one table */
> +QTAILQ_FOREACH(as, _spaces, address_spaces_link) {
> +view = address_space_get_flatview(as);
> +
> +fv_address_spaces = g_hash_table_lookup(views, view);
> +if (!fv_address_spaces) {
> +fv_address_spaces = g_array_new(false, false, sizeof(as));
> +g_hash_table_insert(views, view, fv_address_spaces);
> +}
> +
> +g_array_append_val(fv_address_spaces, as);
> +}
> +
> +/* Print */
> +g_hash_table_foreach(views, mtree_print_flatview, );
> +
> +/* Free */
> +g_hash_table_foreach_remove(views, mtree_info_flatview_free, 0);
> +g_hash_table_unref(views);
> +}
> +
> void mtree_info(bool flatview, bool dispatch_tree, bool owner, bool disabled)
> {
> MemoryRegionListHead ml_head;
> @@ -3253,39 +3291,7 @@ void mtree_info(bool flatview, bool dispatch_tree,
> bool owner, bool disabled)
> AddressSpace *as;
>
> if (flatview) {
> -FlatView *view;
> -struct FlatViewInfo fvi = {
> -.counter = 0,
> -.dispatch_tree = dispatch_tree,
> -.owner = owner,
> -};
> -GArray *fv_address_spaces;
> -GHashTable *views = g_hash_table_new(g_direct_hash, g_direct_equal);
> -AccelClass *ac = ACCEL_GET_CLASS(current_accel());
> -
> -if (ac->has_memory) {
> -fvi.ac = ac;
> -}
> -
> -/* Gather all FVs in one table */
> -QTAILQ_FOREACH(as, _spaces, address_spaces_link) {
> -view = address_space_get_flatview(as);
> -
> -fv_address_spaces = g_hash_table_lookup(views, view);
> -if (!fv_address_spaces) {
> -fv_address_spaces = g_array_new(false, false, sizeof(as));
> -g_hash_table_insert(views, view, fv_address_spaces);
> -}
> -
> -g_array_append_val(fv_address_spaces, as);
> -}
> -
> -/* Print */
> -g_hash_table_foreach(views, mtree_print_flatview, );
> -
> -/* Free */
> -g_hash_table_foreach_remove(views, mtree_info_flatview_free, 0);
> -g_hash_table_unref(views);
> +mtree_info_flatview(dispatch_tree, owner);
>
> return;
> }
>
[PATCH v2 0/3] VNC-related HMP/QMP fixes
Since the removal of the generic 'qmp_change' command, one can no longer replace the 'default' VNC display listen address at runtime (AFAIK). For our users who need to set up a secondary VNC access port, this means configuring a second VNC display (in addition to our standard one for web-access), but it turns out one cannot set a password on this second display at the moment, as the 'set_password' call only operates on the 'default' display. Additionally, using secret objects, the password is only read once at startup. This could be considered a bug too, but is not touched in this series and left for a later date. v1 -> v2: * add Marc-André's R-b on patch 1 * use '-d' flag as suggested by Eric Blake and Gerd Hoffmann * I didn't see a way to do this yet, so I added a "flags with values" arg type Stefan Reiter (3): monitor/hmp: correctly invert password argument detection again monitor/hmp: add support for flag argument with value monitor: allow VNC related QMP and HMP commands to take a display ID hmp-commands.hx| 29 +++-- monitor/hmp-cmds.c | 9 ++--- monitor/hmp.c | 17 - monitor/qmp-cmds.c | 9 + qapi/ui.json | 12 ++-- 5 files changed, 52 insertions(+), 24 deletions(-) -- 2.30.2
Re: 9pfs: Twalk crash
On Wed, 01 Sep 2021 16:21:06 +0200
Christian Schoenebeck wrote:
> On Mittwoch, 1. September 2021 14:49:37 CEST Christian Schoenebeck wrote:
> > > > And it triggered, however I am not sure if some of those functions I
> > > > asserted above are indeed allowed to be executed on a different thread
> > > > than main thread:
> > > >
> > > > Program terminated with signal SIGABRT, Aborted.
> > > > #0 __GI_raise (sig=sig@entry=6) at
> > > > ../sysdeps/unix/sysv/linux/raise.c:50
> > > > 50 ../sysdeps/unix/sysv/linux/raise.c: No such file or directory.
> > > > [Current thread is 1 (Thread 0x7fd0bcef1700 (LWP 6470))]
> > >
> > > Based in the thread number, it seems that the signal was raised by
> > > the main event thread...
> >
> > No, it was not main thread actually, gdb's "current thread is 1" output is
> > misleading.
> >
> > Following the thread id trace, I extended the thread assertion checks over
> > to v9fs_walk() as well, like this:
> >
> > static void coroutine_fn v9fs_walk(void *opaque)
> > {
> > ...
> > assert_thread();
> > v9fs_co_run_in_worker({
> > ...
> > });
> > assert_thread();
> > ...
> > }
> >
> > and made sure the reference thread id to be compared is really the main
> > thread.
> >
> > And what happens here is before v9fs_co_run_in_worker() is entered,
> > v9fs_walk() runs on main thread, but after returning from
> > v9fs_co_run_in_worker() it runs on a different thread for some reason, not
> > on main thread as it would be expected at that point.
>
> Ok, I think I found the root cause: the block is break;-ing out too far. The
That could explain the breakage indeed since the block you've added
to v9fs_walk() embeds a bunch of break statements. AFAICT this block
breaks on errors... do you know which one ?
> following patch should fix it:
>
> diff --git a/hw/9pfs/coth.h b/hw/9pfs/coth.h
> index c51289903d..f83c7dda7b 100644
> --- a/hw/9pfs/coth.h
> +++ b/hw/9pfs/coth.h
> @@ -51,7 +51,9 @@
> */ \
> qemu_coroutine_yield(); \
> qemu_bh_delete(co_bh); \
> -code_block; \
> +do {\
> +code_block; \
> +} while (0);\
Good.
> /* re-enter back to qemu thread */ \
> qemu_coroutine_yield(); \
> } while (0)
>
> I haven't triggered a crash with that patch, but due to the occasional nature
> of this issue I'll give it some more spins before officially proclaiming it
> my
> bug. :)
Well, this is a pre-existing limitation with v9fs_co_run_in_worker().
This wasn't documented as such and not really obvious to detect when
you optimized TWALK. We've never hit it before because the other
v9fs_co_run_in_worker() users don't have break statements.
But, indeed, this caused a regression in 6.1 so this will need a Fixes:
tag and Cc: qemu-stable.
>
> Best regards,
> Christian Schoenebeck
>
>
Re: [PATCH v3] qemu-sockets: fix unix socket path copy (again)
On Wed, Sep 1, 2021 at 5:16 PM Michael Tokarev wrote:
> Commit 4cfd970ec188558daa6214f26203fe553fb1e01f added an
> assert which ensures the path within an address of a unix
> socket returned from the kernel is at least one byte and
> does not exceed sun_path buffer. Both of this constraints
> are wrong:
>
> A unix socket can be unnamed, in this case the path is
> completely empty (not even \0)
>
> And some implementations (notable linux) can add extra
> trailing byte (\0) _after_ the sun_path buffer if we
> passed buffer larger than it (and we do).
>
> So remove the assertion (since it causes real-life breakage)
> but at the same time fix the usage of sun_path. Namely,
> we should not access sun_path[0] if kernel did not return
> it at all (this is the case for unnamed sockets),
> and use the returned salen when copyig actual path as an
> upper constraint for the amount of bytes to copy - this
> will ensure we wont exceed the information provided by
> the kernel, regardless whenever there is a trailing \0
> or not. This also helps with unnamed sockets.
>
> Note the case of abstract socket, the sun_path is actually
> a blob and can contain \0 characters, - it should not be
> passed to g_strndup and the like, it should be accessed by
> memcpy-like functions.
>
> Fixes: 4cfd970ec188558daa6214f26203fe553fb1e01f
> Fixes: http://bugs.debian.org/993145
> Signed-off-by: Michael Tokarev
> CC: qemu-sta...@nongnu.org
> ---
> util/qemu-sockets.c | 13 +
> 1 file changed, 5 insertions(+), 8 deletions(-)
>
> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> index f2f3676d1f..c5043999e9 100644
> --- a/util/qemu-sockets.c
> +++ b/util/qemu-sockets.c
> @@ -1345,25 +1345,22 @@ socket_sockaddr_to_address_unix(struct
> sockaddr_storage *sa,
> SocketAddress *addr;
> struct sockaddr_un *su = (struct sockaddr_un *)sa;
>
> -assert(salen >= sizeof(su->sun_family) + 1 &&
> - salen <= sizeof(struct sockaddr_un));
> -
> addr = g_new0(SocketAddress, 1);
> addr->type = SOCKET_ADDRESS_TYPE_UNIX;
> +salen -= offsetof(struct sockaddr_un, sun_path);
> #ifdef CONFIG_LINUX
> -if (!su->sun_path[0]) {
> +if (salen > 0 && !su->sun_path[0]) {
> /* Linux abstract socket */
> -addr->u.q_unix.path = g_strndup(su->sun_path + 1,
> -salen - sizeof(su->sun_family) -
> 1);
> +addr->u.q_unix.path = g_strndup(su->sun_path + 1, salen - 1);
> addr->u.q_unix.has_abstract = true;
> addr->u.q_unix.abstract = true;
> addr->u.q_unix.has_tight = true;
> -addr->u.q_unix.tight = salen < sizeof(*su);
> +addr->u.q_unix.tight = salen < sizeof(su->sun_path);
> return addr;
> }
> #endif
>
> -addr->u.q_unix.path = g_strndup(su->sun_path, sizeof(su->sun_path));
> +addr->u.q_unix.path = g_strndup(su->sun_path, salen);
> return addr;
> }
> #endif /* WIN32 */
> --
> 2.30.2
>
>
Reviewed-by: Marc-André Lureau
[PATCH v4 3/3] memory: Have 'info mtree' remove duplicated Address Space information
Per Peter Maydell [*]: 'info mtree' monitor command was designed on the assumption that there's really only one or two interesting address spaces, and with more recent developments that's just not the case any more. Similarly about how the FlatView are sorted using a GHashTable, sort the AddressSpace objects to remove the duplications (AS using the same root MemoryRegion). This drastically reduces the output of 'info mtree' on some boards. Before: $ (echo info mtree; echo q) \ | qemu-system-aarch64 -S -monitor stdio -M raspi3b \ | wc -l 423 After: $ (echo info mtree; echo q) \ | qemu-system-aarch64 -S -monitor stdio -M raspi3b \ | wc -l 108 (qemu) info mtree address-space: I/O - (prio 0, i/o): io address-space shared 9 times: - cpu-memory-0 - cpu-memory-1 - cpu-memory-2 - cpu-memory-3 - cpu-secure-memory-0 - cpu-secure-memory-1 - cpu-secure-memory-2 - cpu-secure-memory-3 - memory - (prio 0, i/o): system -3fff (prio 0, ram): ram 3f00-3fff (prio 1, i/o): bcm2835-peripherals 3f003000-3f00301f (prio 0, i/o): bcm2835-sys-timer 3f004000-3f004fff (prio -1000, i/o): bcm2835-txp 3f006000-3f006fff (prio 0, i/o): mphi 3f007000-3f007fff (prio 0, i/o): bcm2835-dma 3f00b200-3f00b3ff (prio 0, i/o): bcm2835-ic 3f00b400-3f00b43f (prio -1000, i/o): bcm2835-sp804 3f00b800-3f00bbff (prio 0, i/o): bcm2835-mbox 3f10-3f1001ff (prio 0, i/o): bcm2835-powermgt 3f101000-3f102fff (prio 0, i/o): bcm2835-cprman 3f104000-3f10400f (prio 0, i/o): bcm2835-rng 3f20-3f200fff (prio 0, i/o): bcm2835_gpio 3f201000-3f201fff (prio 0, i/o): pl011 3f202000-3f202fff (prio 0, i/o): bcm2835-sdhost 3f203000-3f2030ff (prio -1000, i/o): bcm2835-i2s 3f204000-3f20401f (prio -1000, i/o): bcm2835-spi0 3f205000-3f20501f (prio -1000, i/o): bcm2835-i2c0 3f20f000-3f20f07f (prio -1000, i/o): bcm2835-otp 3f212000-3f212007 (prio 0, i/o): bcm2835-thermal 3f214000-3f2140ff (prio -1000, i/o): bcm2835-spis 3f215000-3f2150ff (prio 0, i/o): bcm2835-aux 3f30-3f3000ff (prio 0, i/o): sdhci 3f60-3f6000ff (prio -1000, i/o): bcm2835-smi 3f804000-3f80401f (prio -1000, i/o): bcm2835-i2c1 3f805000-3f80501f (prio -1000, i/o): bcm2835-i2c2 3f90-3f907fff (prio -1000, i/o): bcm2835-dbus 3f91-3f917fff (prio -1000, i/o): bcm2835-ave0 3f98-3f990fff (prio 0, i/o): dwc2 3f98-3f980fff (prio 0, i/o): dwc2-io 3f981000-3f990fff (prio 0, i/o): dwc2-fifo 3fc0-3fc00fff (prio -1000, i/o): bcm2835-v3d 3fe0-3fe000ff (prio -1000, i/o): bcm2835-sdramc 3fe05000-3fe050ff (prio 0, i/o): bcm2835-dma-chan15 4000-40ff (prio 0, i/o): bcm2836-control address-space shared 4 times: - bcm2835-dma-memory - bcm2835-fb-memory - bcm2835-property-memory - dwc2 - (prio 0, i/o): bcm2835-gpu -3fff (prio 0, ram): alias bcm2835-gpu-ram-alias[*] @ram -3fff 4000-7fff (prio 0, ram): alias bcm2835-gpu-ram-alias[*] @ram -3fff 7e00-7eff (prio 1, i/o): alias bcm2835-peripherals @bcm2835-peripherals -00ff 8000-bfff (prio 0, ram): alias bcm2835-gpu-ram-alias[*] @ram -3fff c000- (prio 0, ram): alias bcm2835-gpu-ram-alias[*] @ram -3fff address-space: bcm2835-mbox-memory -008f (prio 0, i/o): bcm2835-mbox 0010-001f (prio 0, i/o): bcm2835-fb 0080-008f (prio 0, i/o): bcm2835-property memory-region: ram -3fff (prio 0, ram): ram memory-region: bcm2835-peripherals 3f00-3fff (prio 1, i/o): bcm2835-peripherals 3f003000-3f00301f (prio 0, i/o): bcm2835-sys-timer 3f004000-3f004fff (prio -1000, i/o): bcm2835-txp 3f006000-3f006fff
[PATCH v2 1/1] hw/arm/aspeed: Allow machine to set UART default
From: Peter Delevoryas
When you run QEMU with an Aspeed machine and a single serial device
using stdio like this:
qemu -machine ast2600-evb -drive ... -serial stdio
The guest OS can read and write to the UART5 registers at 0x1E784000 and
it will receive from stdin and write to stdout. The Aspeed SoC's have a
lot more UART's though (AST2500 has 5, AST2600 has 13) and depending on
the board design, may be using any of them as the serial console. (See
"stdout-path" in a DTS to check which one is chosen).
Most boards, including all of those currently defined in
hw/arm/aspeed.c, just use UART5, but some use UART1. This change adds
some flexibility for different boards without requiring users to change
their command-line invocation of QEMU.
I tested this doesn't break existing code by booting an AST2500 OpenBMC
image and an AST2600 OpenBMC image, each using UART5 as the console.
Then I tested switching the default to UART1 and booting an AST2600
OpenBMC image that uses UART1, and that worked too.
Signed-off-by: Peter Delevoryas
---
hw/arm/aspeed.c | 3 +++
hw/arm/aspeed_ast2600.c | 8
hw/arm/aspeed_soc.c | 9 ++---
include/hw/arm/aspeed.h | 1 +
include/hw/arm/aspeed_soc.h | 1 +
5 files changed, 15 insertions(+), 7 deletions(-)
diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index 9d43e26c51..a81e90c539 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -350,6 +350,8 @@ static void aspeed_machine_init(MachineState *machine)
object_property_set_int(OBJECT(>soc), "hw-prot-key",
ASPEED_SCU_PROT_KEY, _abort);
}
+qdev_prop_set_uint32(DEVICE(>soc), "uart-default",
+ amc->uart_default);
qdev_realize(DEVICE(>soc), NULL, _abort);
memory_region_add_subregion(get_system_memory(),
@@ -804,6 +806,7 @@ static void aspeed_machine_class_init(ObjectClass *oc, void
*data)
mc->no_parallel = 1;
mc->default_ram_id = "ram";
amc->macs_mask = ASPEED_MAC0_ON;
+amc->uart_default = ASPEED_DEV_UART5;
aspeed_machine_class_props_init(oc);
}
diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
index e3013128c6..b07fcf10a0 100644
--- a/hw/arm/aspeed_ast2600.c
+++ b/hw/arm/aspeed_ast2600.c
@@ -322,10 +322,10 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev,
Error **errp)
sysbus_connect_irq(SYS_BUS_DEVICE(>timerctrl), i, irq);
}
-/* UART - attach an 8250 to the IO space as our UART5 */
-serial_mm_init(get_system_memory(), sc->memmap[ASPEED_DEV_UART5], 2,
- aspeed_soc_get_irq(s, ASPEED_DEV_UART5),
- 38400, serial_hd(0), DEVICE_LITTLE_ENDIAN);
+/* UART - attach an 8250 to the IO space as our UART */
+serial_mm_init(get_system_memory(), sc->memmap[s->uart_default], 2,
+ aspeed_soc_get_irq(s, s->uart_default), 38400,
+ serial_hd(0), DEVICE_LITTLE_ENDIAN);
/* I2C */
object_property_set_link(OBJECT(>i2c), "dram", OBJECT(s->dram_mr),
diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
index 3ad6c56fa9..09c0f83710 100644
--- a/hw/arm/aspeed_soc.c
+++ b/hw/arm/aspeed_soc.c
@@ -287,9 +287,9 @@ static void aspeed_soc_realize(DeviceState *dev, Error
**errp)
sysbus_connect_irq(SYS_BUS_DEVICE(>timerctrl), i, irq);
}
-/* UART - attach an 8250 to the IO space as our UART5 */
-serial_mm_init(get_system_memory(), sc->memmap[ASPEED_DEV_UART5], 2,
- aspeed_soc_get_irq(s, ASPEED_DEV_UART5), 38400,
+/* UART - attach an 8250 to the IO space as our UART */
+serial_mm_init(get_system_memory(), sc->memmap[s->uart_default], 2,
+ aspeed_soc_get_irq(s, s->uart_default), 38400,
serial_hd(0), DEVICE_LITTLE_ENDIAN);
/* I2C */
@@ -439,6 +439,8 @@ static void aspeed_soc_realize(DeviceState *dev, Error
**errp)
static Property aspeed_soc_properties[] = {
DEFINE_PROP_LINK("dram", AspeedSoCState, dram_mr, TYPE_MEMORY_REGION,
MemoryRegion *),
+DEFINE_PROP_UINT32("uart-default", AspeedSoCState, uart_default,
+ ASPEED_DEV_UART5),
DEFINE_PROP_END_OF_LIST(),
};
@@ -449,6 +451,7 @@ static void aspeed_soc_class_init(ObjectClass *oc, void
*data)
dc->realize = aspeed_soc_realize;
/* Reason: Uses serial_hds and nd_table in realize() directly */
dc->user_creatable = false;
+
device_class_set_props(dc, aspeed_soc_properties);
}
diff --git a/include/hw/arm/aspeed.h b/include/hw/arm/aspeed.h
index c9747b15fc..cbeacb214c 100644
--- a/include/hw/arm/aspeed.h
+++ b/include/hw/arm/aspeed.h
@@ -38,6 +38,7 @@ struct AspeedMachineClass {
uint32_t num_cs;
uint32_t macs_mask;
void (*i2c_init)(AspeedMachineState *bmc);
+uint32_t uart_default;
};
diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h
index d9161d26d6..87d76c9259 100644
--- a/include/hw/arm/aspeed_soc.h
+++
[PULL 55/56] block/export/fuse.c: fix fuse-lseek on uclibc or musl
From: Fabrice Fontaine
Include linux/fs.h to avoid the following build failure on uclibc or
musl raised since version 6.0.0:
../block/export/fuse.c: In function 'fuse_lseek':
../block/export/fuse.c:641:19: error: 'SEEK_HOLE' undeclared (first use in this
function)
641 | if (whence != SEEK_HOLE && whence != SEEK_DATA) {
| ^
../block/export/fuse.c:641:19: note: each undeclared identifier is reported
only once for each function it appears in
../block/export/fuse.c:641:42: error: 'SEEK_DATA' undeclared (first use in this
function); did you mean 'SEEK_SET'?
641 | if (whence != SEEK_HOLE && whence != SEEK_DATA) {
| ^
| SEEK_SET
Fixes:
-
http://autobuild.buildroot.org/results/33c90ebf04997f4d3557cfa66abc9cf9a3076137
Signed-off-by: Fabrice Fontaine
Message-Id: <20210827220301.272887-1-fontaine.fabr...@gmail.com>
Signed-off-by: Hanna Reitz
---
block/export/fuse.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/block/export/fuse.c b/block/export/fuse.c
index fc7b07d2b5..2e3bf8270b 100644
--- a/block/export/fuse.c
+++ b/block/export/fuse.c
@@ -31,6 +31,9 @@
#include
#include
+#ifdef __linux__
+#include
+#endif
/* Prevent overly long bounce buffer allocations */
#define FUSE_MAX_BOUNCE_BYTES (MIN(BDRV_REQUEST_MAX_BYTES, 64 * 1024 * 1024))
--
2.31.1
Re: [PATCH 2/3] hw/virtio: Remove NULL check in virtio_free_region_cache()
On Thu, Aug 26, 2021 at 07:26:57PM +0200, Philippe Mathieu-Daudé wrote:
virtio_free_region_cache() is called within call_rcu(),
always with a non-NULL argument. Ensure new code keep it
that way by replacing the NULL check by an assertion.
Add a comment this function is called within call_rcu().
Signed-off-by: Philippe Mathieu-Daudé
---
hw/virtio/virtio.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index a5214bca612..3a1f6c520cb 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -133,12 +133,10 @@ struct VirtQueue
QLIST_ENTRY(VirtQueue) node;
};
+/* Called within call_rcu(). */
static void virtio_free_region_cache(VRingMemoryRegionCaches *caches)
{
-if (!caches) {
-return;
-}
-
+assert(caches != NULL);
address_space_cache_destroy(>desc);
address_space_cache_destroy(>avail);
address_space_cache_destroy(>used);
--
2.31.1
Reviewed-by: Stefano Garzarella
[PATCH v4 2/3] memory: Extract mtree_info_as() from mtree_info()
While mtree_info() handles both ASes and flatviews cases,
the two cases share basically no code. Split mtree_info_as()
out of mtree_info() to simplify.
Suggested-by: Peter Maydell
Signed-off-by: Philippe Mathieu-Daudé
---
softmmu/memory.c | 17 ++---
1 file changed, 10 insertions(+), 7 deletions(-)
diff --git a/softmmu/memory.c b/softmmu/memory.c
index 3eb6f52de67..5be7d5e7412 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -3284,18 +3284,12 @@ static void mtree_info_flatview(bool dispatch_tree,
bool owner)
g_hash_table_unref(views);
}
-void mtree_info(bool flatview, bool dispatch_tree, bool owner, bool disabled)
+static void mtree_info_as(bool dispatch_tree, bool owner, bool disabled)
{
MemoryRegionListHead ml_head;
MemoryRegionList *ml, *ml2;
AddressSpace *as;
-if (flatview) {
-mtree_info_flatview(dispatch_tree, owner);
-
-return;
-}
-
QTAILQ_INIT(_head);
QTAILQ_FOREACH(as, _spaces, address_spaces_link) {
@@ -3316,6 +3310,15 @@ void mtree_info(bool flatview, bool dispatch_tree, bool
owner, bool disabled)
}
}
+void mtree_info(bool flatview, bool dispatch_tree, bool owner, bool disabled)
+{
+if (flatview) {
+mtree_info_flatview(dispatch_tree, owner);
+} else {
+mtree_info_as(dispatch_tree, owner, disabled);
+}
+}
+
void memory_region_init_ram(MemoryRegion *mr,
Object *owner,
const char *name,
--
2.31.1
Re: [PATCH 1/3] hw/virtio: Document virtio_queue_packed_empty_rcu is called within RCU
On Thu, Aug 26, 2021 at 07:26:56PM +0200, Philippe Mathieu-Daudé wrote:
While virtio_queue_packed_empty_rcu() uses the '_rcu' suffix,
it is not obvious it is called within rcu_read_lock(). All other
functions from this file called with the RCU locked have a comment
describing it. Document this one similarly for consistency.
Signed-off-by: Philippe Mathieu-Daudé
---
hw/virtio/virtio.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 874377f37a7..a5214bca612 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -634,6 +634,7 @@ static int virtio_queue_split_empty(VirtQueue *vq)
return empty;
}
+/* Called within rcu_read_lock(). */
static int virtio_queue_packed_empty_rcu(VirtQueue *vq)
{
struct VRingPackedDesc desc;
--
2.31.1
Reviewed-by: Stefano Garzarella
Re: [PATCH v1 3/3] migration: multifd: Enable zerocopy
On Wed, Sep 01, 2021 at 09:53:07AM +0100, Daniel P. Berrangé wrote:
> On Tue, Aug 31, 2021 at 04:29:09PM -0400, Peter Xu wrote:
> > On Tue, Aug 31, 2021 at 02:16:42PM +0100, Daniel P. Berrangé wrote:
> > > On Tue, Aug 31, 2021 at 08:02:39AM -0300, Leonardo Bras wrote:
> > > > Call qio_channel_set_zerocopy(true) in the start of every multifd
> > > > thread.
> > > >
> > > > Change the send_write() interface of multifd, allowing it to pass down
> > > > flags for qio_channel_write*().
> > > >
> > > > Pass down MSG_ZEROCOPY flag for sending memory pages, while keeping the
> > > > other data being sent at the default copying approach.
> > > >
> > > > Signed-off-by: Leonardo Bras
> > > > ---
> > > > migration/multifd-zlib.c | 7 ---
> > > > migration/multifd-zstd.c | 7 ---
> > > > migration/multifd.c | 9 ++---
> > > > migration/multifd.h | 3 ++-
> > > > 4 files changed, 16 insertions(+), 10 deletions(-)
> > >
> > > > @@ -675,7 +676,8 @@ static void *multifd_send_thread(void *opaque)
> > > > }
> > > >
> > > > if (used) {
> > > > -ret = multifd_send_state->ops->send_write(p, used,
> > > > _err);
> > > > +ret = multifd_send_state->ops->send_write(p, used,
> > > > MSG_ZEROCOPY,
> > > > + _err);
> > >
> > > I don't think it is valid to unconditionally enable this feature due to
> > > the
> > > resource usage implications
> > >
> > > https://www.kernel.org/doc/html/v5.4/networking/msg_zerocopy.html
> > >
> > > "A zerocopy failure will return -1 with errno ENOBUFS. This happens
> > >if the socket option was not set, the socket exceeds its optmem
> > >limit or the user exceeds its ulimit on locked pages."
> > >
> > > The limit on locked pages is something that looks very likely to be
> > > exceeded unless you happen to be running a QEMU config that already
> > > implies locked memory (eg PCI assignment)
> >
> > Yes it would be great to be a migration capability in parallel to multifd.
> > At
> > initial phase if it's easy to be implemented on multi-fd only, we can add a
> > dependency between the caps. In the future we can remove that dependency
> > when
> > the code is ready to go without multifd. Thanks,
>
> Also, I'm wondering how zerocopy support interacts with kernel support
> for kTLS and multipath-TCP, both of which we want to be able to use
> with migration.
Copying Jason Wang for net implications between these features on kernel side
and whether they can be enabled together (MSG_ZEROCOPY, mptcp, kTLS).
>From the safe side we may want to only enable one of them until we prove
they'll work together I guess..
Not a immediate concern as I don't really think any of them is really
explicitly supported in qemu.
KTLS may be implicitly included by a new gnutls, but we need to mark TLS and
ZEROCOPY mutual exclusive anyway because at least the userspace TLS code of
gnutls won't has a way to maintain the tls buffers used by zerocopy. So at
least we need some knob to detect whether kTLS is enabled in gnutls.
--
Peter Xu
[PULL 52/56] iotests/image-fleecing: prepare for adding new test-case
From: Vladimir Sementsov-Ogievskiy
We are going to add a test-case with some behavior modifications. So,
let's prepare a function to be reused.
Signed-off-by: Vladimir Sementsov-Ogievskiy
Reviewed-by: Max Reitz
Message-Id: <20210824083856.17408-33-vsement...@virtuozzo.com>
Signed-off-by: Hanna Reitz
---
tests/qemu-iotests/tests/image-fleecing | 19 +--
1 file changed, 13 insertions(+), 6 deletions(-)
diff --git a/tests/qemu-iotests/tests/image-fleecing
b/tests/qemu-iotests/tests/image-fleecing
index ec4ef5f3f6..e210c00d28 100755
--- a/tests/qemu-iotests/tests/image-fleecing
+++ b/tests/qemu-iotests/tests/image-fleecing
@@ -48,12 +48,7 @@ remainder = [('0xd5', '0x108000', '32k'), # Right-end of
partial-left [1]
('0xdc', '32M', '32k'), # Left-end of partial-right [2]
('0xcd', '0x3ff', '64k')] # patterns[3]
-with iotests.FilePath('base.img') as base_img_path, \
- iotests.FilePath('fleece.img') as fleece_img_path, \
- iotests.FilePath('nbd.sock',
- base_dir=iotests.sock_dir) as nbd_sock_path, \
- iotests.VM() as vm:
-
+def do_test(base_img_path, fleece_img_path, nbd_sock_path, vm):
log('--- Setting up images ---')
log('')
@@ -163,3 +158,15 @@ with iotests.FilePath('base.img') as base_img_path, \
log('')
log('Done')
+
+
+def test():
+with iotests.FilePath('base.img') as base_img_path, \
+ iotests.FilePath('fleece.img') as fleece_img_path, \
+ iotests.FilePath('nbd.sock',
+ base_dir=iotests.sock_dir) as nbd_sock_path, \
+ iotests.VM() as vm:
+do_test(base_img_path, fleece_img_path, nbd_sock_path, vm)
+
+
+test()
--
2.31.1
