>-----Original Message-----
>From: Eric Auger <eric.auger@redhat.com>
>Subject: Re: [PATCH v1 05/11] vfio: Introduce host_iommu_device_create
>callback
>
>
>
>On 3/18/24 14:52, Eric Auger wrote:
>> Hi ZHenzhong,
>>
>> On 2/28/24 04:58, Zhenzhong Duan wrote:
>>> Introduce host_iommu_device_create callback and a wrapper for it.
>>>
>>> This callback is used to allocate a host iommu device instance and
>>> initialize it based on type.
>>>
>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>>> ---
>>> include/hw/vfio/vfio-common.h | 1 +
>>> include/hw/vfio/vfio-container-base.h | 1 +
>>> hw/vfio/common.c | 8 ++++++++
>>> 3 files changed, 10 insertions(+)
>>>
>>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-
>common.h
>>> index b6676c9f79..9fefea4b89 100644
>>> --- a/include/hw/vfio/vfio-common.h
>>> +++ b/include/hw/vfio/vfio-common.h
>>> @@ -208,6 +208,7 @@ struct vfio_device_info *vfio_get_device_info(int
>fd);
>>> int vfio_attach_device(char *name, VFIODevice *vbasedev,
>>> AddressSpace *as, Error **errp);
>>> void vfio_detach_device(VFIODevice *vbasedev);
>>> +void host_iommu_device_create(VFIODevice *vbasedev);
>>>
>>> int vfio_kvm_device_add_fd(int fd, Error **errp);
>>> int vfio_kvm_device_del_fd(int fd, Error **errp);
>>> diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-
>container-base.h
>>> index b2813b0c11..dc003f6eb2 100644
>>> --- a/include/hw/vfio/vfio-container-base.h
>>> +++ b/include/hw/vfio/vfio-container-base.h
>>> @@ -120,6 +120,7 @@ struct VFIOIOMMUClass {
>>> int (*attach_device)(const char *name, VFIODevice *vbasedev,
>>> AddressSpace *as, Error **errp);
>>> void (*detach_device)(VFIODevice *vbasedev);
>>> + void (*host_iommu_device_create)(VFIODevice *vbasedev);
>>> /* migration feature */
>>> int (*set_dirty_page_tracking)(const VFIOContainerBase *bcontainer,
>>> bool start);
>>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>>> index 059bfdc07a..41e9031c59 100644
>>> --- a/hw/vfio/common.c
>>> +++ b/hw/vfio/common.c
>>> @@ -1521,3 +1521,11 @@ void vfio_detach_device(VFIODevice
>*vbasedev)
>>> }
>>> vbasedev->bcontainer->ops->detach_device(vbasedev);
>>> }
>>> +
>>> +void host_iommu_device_create(VFIODevice *vbasedev)
>>> +{
>>> + const VFIOIOMMUClass *ops = vbasedev->bcontainer->ops;
>>> +
>>> + assert(ops->host_iommu_device_create);
>> at this stage ops actual implementation do not exist yet so this will
>> break the bisection
>
>Sorry it is OK at the function only is called in
>[PATCH v1 08/11] vfio/pci: Allocate and initialize HostIOMMUDevice after
>attachment
>
>Sorry for the noise
Ah, send too quickly. No problem.
Thanks
Zhenzhong
Hi Eric, >-----Original Message----- >From: Eric Auger <eric.auger@redhat.com> >Subject: Re: [PATCH v1 05/11] vfio: Introduce host_iommu_device_create >callback > >Hi ZHenzhong, > >On 2/28/24 04:58, Zhenzhong Duan wrote: >> Introduce host_iommu_device_create callback and a wrapper for it. >> >> This callback is used to allocate a host iommu device instance and >> initialize it based on type. >> >> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >> --- >> include/hw/vfio/vfio-common.h | 1 + >> include/hw/vfio/vfio-container-base.h | 1 + >> hw/vfio/common.c | 8 ++++++++ >> 3 files changed, 10 insertions(+) >> >> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio- >common.h >> index b6676c9f79..9fefea4b89 100644 >> --- a/include/hw/vfio/vfio-common.h >> +++ b/include/hw/vfio/vfio-common.h >> @@ -208,6 +208,7 @@ struct vfio_device_info *vfio_get_device_info(int >fd); >> int vfio_attach_device(char *name, VFIODevice *vbasedev, >> AddressSpace *as, Error **errp); >> void vfio_detach_device(VFIODevice *vbasedev); >> +void host_iommu_device_create(VFIODevice *vbasedev); >> >> int vfio_kvm_device_add_fd(int fd, Error **errp); >> int vfio_kvm_device_del_fd(int fd, Error **errp); >> diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio- >container-base.h >> index b2813b0c11..dc003f6eb2 100644 >> --- a/include/hw/vfio/vfio-container-base.h >> +++ b/include/hw/vfio/vfio-container-base.h >> @@ -120,6 +120,7 @@ struct VFIOIOMMUClass { >> int (*attach_device)(const char *name, VFIODevice *vbasedev, >> AddressSpace *as, Error **errp); >> void (*detach_device)(VFIODevice *vbasedev); >> + void (*host_iommu_device_create)(VFIODevice *vbasedev); >> /* migration feature */ >> int (*set_dirty_page_tracking)(const VFIOContainerBase *bcontainer, >> bool start); >> diff --git a/hw/vfio/common.c b/hw/vfio/common.c >> index 059bfdc07a..41e9031c59 100644 >> --- a/hw/vfio/common.c >> +++ b/hw/vfio/common.c >> @@ -1521,3 +1521,11 @@ void vfio_detach_device(VFIODevice >*vbasedev) >> } >> vbasedev->bcontainer->ops->detach_device(vbasedev); >> } >> + >> +void host_iommu_device_create(VFIODevice *vbasedev) >> +{ >> + const VFIOIOMMUClass *ops = vbasedev->bcontainer->ops; >> + >> + assert(ops->host_iommu_device_create); >at this stage ops actual implementation do not exist yet so this will >break the bisection This patch only introcudes host_iommu_device_create but no one call into it. Patch6-7 implement callback for different backend, patch8 call host_iommu_device_create(), so I think the order is ok. Let me know if I missed your points. Thanks Zhenzhong > >Eric >> + ops->host_iommu_device_create(vbasedev); >> +}
ui/curses is the only user of console_select(). Move the implementation to ui/curses. Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> --- include/ui/console.h | 1 - ui/console-priv.h | 2 +- ui/console-vc-stubs.c | 2 +- ui/console-vc.c | 3 +- ui/console.c | 121 +++++++++----------------------------------------- ui/curses.c | 48 +++++++++++--------- 6 files changed, 51 insertions(+), 126 deletions(-) diff --git a/include/ui/console.h b/include/ui/console.h index 3729d2db29c7..0bc7a00ac0bb 100644 --- a/include/ui/console.h +++ b/include/ui/console.h @@ -433,7 +433,6 @@ int qemu_console_get_window_id(QemuConsole *con); /* Set the low-level window id for the console */ void qemu_console_set_window_id(QemuConsole *con, int window_id); -void console_select(unsigned int index); void qemu_console_resize(QemuConsole *con, int width, int height); DisplaySurface *qemu_console_surface(QemuConsole *con); void coroutine_fn qemu_console_co_wait_update(QemuConsole *con); diff --git a/ui/console-priv.h b/ui/console-priv.h index 88569ed2cc41..43ceb8122f13 100644 --- a/ui/console-priv.h +++ b/ui/console-priv.h @@ -35,7 +35,7 @@ struct QemuConsole { QTAILQ_ENTRY(QemuConsole) next; }; -void qemu_text_console_select(QemuTextConsole *c); +void qemu_text_console_update_size(QemuTextConsole *c); const char * qemu_text_console_get_label(QemuTextConsole *c); void qemu_text_console_update_cursor(void); void qemu_text_console_handle_keysym(QemuTextConsole *s, int keysym); diff --git a/ui/console-vc-stubs.c b/ui/console-vc-stubs.c index 2afc52329f0c..b63e2fb2345f 100644 --- a/ui/console-vc-stubs.c +++ b/ui/console-vc-stubs.c @@ -10,7 +10,7 @@ #include "chardev/char.h" #include "ui/console-priv.h" -void qemu_text_console_select(QemuTextConsole *c) +void qemu_text_console_update_size(QemuTextConsole *c) { } diff --git a/ui/console-vc.c b/ui/console-vc.c index f22c8e23c2ed..899fa11c9485 100644 --- a/ui/console-vc.c +++ b/ui/console-vc.c @@ -958,10 +958,9 @@ static void vc_chr_set_echo(Chardev *chr, bool echo) drv->console->echo = echo; } -void qemu_text_console_select(QemuTextConsole *c) +void qemu_text_console_update_size(QemuTextConsole *c) { dpy_text_resize(QEMU_CONSOLE(c), c->width, c->height); - qemu_text_console_update_cursor(); } static void vc_chr_open(Chardev *chr, diff --git a/ui/console.c b/ui/console.c index fbc1b9b8b57c..43226c5c1454 100644 --- a/ui/console.c +++ b/ui/console.c @@ -66,7 +66,6 @@ struct DisplayState { }; static DisplayState *display_state; -static QemuConsole *active_console; static QTAILQ_HEAD(, QemuConsole) consoles = QTAILQ_HEAD_INITIALIZER(consoles); @@ -135,7 +134,6 @@ void graphic_hw_update_done(QemuConsole *con) void graphic_hw_update(QemuConsole *con) { bool async = false; - con = con ? con : active_console; if (!con) { return; } @@ -209,9 +207,6 @@ void qemu_console_set_window_id(QemuConsole *con, int window_id) void graphic_hw_invalidate(QemuConsole *con) { - if (!con) { - con = active_console; - } if (con && con->hw_ops->invalidate) { con->hw_ops->invalidate(con->hw); } @@ -219,9 +214,6 @@ void graphic_hw_invalidate(QemuConsole *con) void graphic_hw_text_update(QemuConsole *con, console_ch_t *chardata) { - if (!con) { - con = active_console; - } if (con && con->hw_ops->text_update) { con->hw_ops->text_update(con->hw, chardata); } @@ -265,12 +257,12 @@ static void dpy_gfx_update_texture(QemuConsole *con, DisplaySurface *surface, } static void displaychangelistener_display_console(DisplayChangeListener *dcl, - QemuConsole *con, Error **errp) { static const char nodev[] = "This VM has no graphic display device."; static DisplaySurface *dummy; + QemuConsole *con = dcl->con; if (!con || !console_compatible_with(con, dcl, errp)) { if (!dummy) { @@ -305,39 +297,8 @@ static void displaychangelistener_display_console(DisplayChangeListener *dcl, } } -void console_select(unsigned int index) -{ - DisplayChangeListener *dcl; - QemuConsole *s; - - trace_console_select(index); - s = qemu_console_lookup_by_index(index); - if (s) { - DisplayState *ds = s->ds; - - active_console = s; - QLIST_FOREACH (dcl, &ds->listeners, next) { - if (dcl->con != NULL) { - continue; - } - displaychangelistener_display_console(dcl, s, NULL); - } - - if (QEMU_IS_TEXT_CONSOLE(s)) { - qemu_text_console_select(QEMU_TEXT_CONSOLE(s)); - } - } -} - void qemu_text_console_put_keysym(QemuTextConsole *s, int keysym) { - if (!s) { - if (!QEMU_IS_TEXT_CONSOLE(active_console)) { - return; - } - s = QEMU_TEXT_CONSOLE(active_console); - } - qemu_text_console_handle_keysym(s, keysym); } @@ -392,11 +353,6 @@ qemu_console_register(QemuConsole *c) { int i; - if (!active_console || (!QEMU_IS_GRAPHIC_CONSOLE(active_console) && - QEMU_IS_GRAPHIC_CONSOLE(c))) { - active_console = c; - } - if (QTAILQ_EMPTY(&consoles)) { c->index = 0; QTAILQ_INSERT_TAIL(&consoles, c, next); @@ -751,8 +707,6 @@ dcl_set_graphic_cursor(DisplayChangeListener *dcl, QemuGraphicConsole *con) void register_displaychangelistener(DisplayChangeListener *dcl) { - QemuConsole *con; - assert(!dcl->ds); trace_displaychangelistener_register(dcl, dcl->ops->dpy_name); @@ -761,13 +715,12 @@ void register_displaychangelistener(DisplayChangeListener *dcl) gui_setup_refresh(dcl->ds); if (dcl->con) { dcl->con->dcls++; - con = dcl->con; - } else { - con = active_console; } - displaychangelistener_display_console(dcl, con, dcl->con ? &error_fatal : NULL); - if (QEMU_IS_GRAPHIC_CONSOLE(con)) { - dcl_set_graphic_cursor(dcl, QEMU_GRAPHIC_CONSOLE(con)); + displaychangelistener_display_console(dcl, &error_fatal); + if (QEMU_IS_GRAPHIC_CONSOLE(dcl->con)) { + dcl_set_graphic_cursor(dcl, QEMU_GRAPHIC_CONSOLE(dcl->con)); + } else if (QEMU_IS_TEXT_CONSOLE(dcl->con)) { + qemu_text_console_update_size(QEMU_TEXT_CONSOLE(dcl->con)); } qemu_text_console_update_cursor(); } @@ -805,9 +758,6 @@ static void dpy_set_ui_info_timer(void *opaque) bool dpy_ui_info_supported(const QemuConsole *con) { - if (con == NULL) { - con = active_console; - } if (con == NULL) { return false; } @@ -819,19 +769,11 @@ const QemuUIInfo *dpy_get_ui_info(const QemuConsole *con) { assert(dpy_ui_info_supported(con)); - if (con == NULL) { - con = active_console; - } - return &con->ui_info; } int dpy_set_ui_info(QemuConsole *con, QemuUIInfo *info, bool delay) { - if (con == NULL) { - con = active_console; - } - if (!dpy_ui_info_supported(con)) { return -1; } @@ -870,7 +812,7 @@ void dpy_gfx_update(QemuConsole *con, int x, int y, int w, int h) } dpy_gfx_update_texture(con, con->surface, x, y, w, h); QLIST_FOREACH(dcl, &s->listeners, next) { - if (con != (dcl->con ? dcl->con : active_console)) { + if (con != dcl->con) { continue; } if (dcl->ops->dpy_gfx_update) { @@ -916,7 +858,7 @@ void dpy_gfx_replace_surface(QemuConsole *con, con->surface = new_surface; dpy_gfx_create_texture(con, new_surface); QLIST_FOREACH(dcl, &s->listeners, next) { - if (con != (dcl->con ? dcl->con : active_console)) { + if (con != dcl->con) { continue; } displaychangelistener_gfx_switch(dcl, new_surface, surface ? FALSE : TRUE); @@ -970,7 +912,7 @@ void dpy_text_cursor(QemuConsole *con, int x, int y) return; } QLIST_FOREACH(dcl, &s->listeners, next) { - if (con != (dcl->con ? dcl->con : active_console)) { + if (con != dcl->con) { continue; } if (dcl->ops->dpy_text_cursor) { @@ -988,7 +930,7 @@ void dpy_text_update(QemuConsole *con, int x, int y, int w, int h) return; } QLIST_FOREACH(dcl, &s->listeners, next) { - if (con != (dcl->con ? dcl->con : active_console)) { + if (con != dcl->con) { continue; } if (dcl->ops->dpy_text_update) { @@ -1006,7 +948,7 @@ void dpy_text_resize(QemuConsole *con, int w, int h) return; } QLIST_FOREACH(dcl, &s->listeners, next) { - if (con != (dcl->con ? dcl->con : active_console)) { + if (con != dcl->con) { continue; } if (dcl->ops->dpy_text_resize) { @@ -1028,7 +970,7 @@ void dpy_mouse_set(QemuConsole *c, int x, int y, int on) return; } QLIST_FOREACH(dcl, &s->listeners, next) { - if (c != (dcl->con ? dcl->con : active_console)) { + if (c != dcl->con) { continue; } if (dcl->ops->dpy_mouse_set) { @@ -1049,7 +991,7 @@ void dpy_cursor_define(QemuConsole *c, QEMUCursor *cursor) return; } QLIST_FOREACH(dcl, &s->listeners, next) { - if (c != (dcl->con ? dcl->con : active_console)) { + if (c != dcl->con) { continue; } if (dcl->ops->dpy_cursor_define) { @@ -1099,7 +1041,7 @@ void dpy_gl_scanout_disable(QemuConsole *con) con->scanout.kind = SCANOUT_NONE; } QLIST_FOREACH(dcl, &s->listeners, next) { - if (con != (dcl->con ? dcl->con : active_console)) { + if (con != dcl->con) { continue; } if (dcl->ops->dpy_gl_scanout_disable) { @@ -1126,7 +1068,7 @@ void dpy_gl_scanout_texture(QemuConsole *con, x, y, width, height, d3d_tex2d, }; QLIST_FOREACH(dcl, &s->listeners, next) { - if (con != (dcl->con ? dcl->con : active_console)) { + if (con != dcl->con) { continue; } if (dcl->ops->dpy_gl_scanout_texture) { @@ -1148,7 +1090,7 @@ void dpy_gl_scanout_dmabuf(QemuConsole *con, con->scanout.kind = SCANOUT_DMABUF; con->scanout.dmabuf = dmabuf; QLIST_FOREACH(dcl, &s->listeners, next) { - if (con != (dcl->con ? dcl->con : active_console)) { + if (con != dcl->con) { continue; } if (dcl->ops->dpy_gl_scanout_dmabuf) { @@ -1164,7 +1106,7 @@ void dpy_gl_cursor_dmabuf(QemuConsole *con, QemuDmaBuf *dmabuf, DisplayChangeListener *dcl; QLIST_FOREACH(dcl, &s->listeners, next) { - if (con != (dcl->con ? dcl->con : active_console)) { + if (con != dcl->con) { continue; } if (dcl->ops->dpy_gl_cursor_dmabuf) { @@ -1181,7 +1123,7 @@ void dpy_gl_cursor_position(QemuConsole *con, DisplayChangeListener *dcl; QLIST_FOREACH(dcl, &s->listeners, next) { - if (con != (dcl->con ? dcl->con : active_console)) { + if (con != dcl->con) { continue; } if (dcl->ops->dpy_gl_cursor_position) { @@ -1197,7 +1139,7 @@ void dpy_gl_release_dmabuf(QemuConsole *con, DisplayChangeListener *dcl; QLIST_FOREACH(dcl, &s->listeners, next) { - if (con != (dcl->con ? dcl->con : active_console)) { + if (con != dcl->con) { continue; } if (dcl->ops->dpy_gl_release_dmabuf) { @@ -1216,7 +1158,7 @@ void dpy_gl_update(QemuConsole *con, graphic_hw_gl_block(con, true); QLIST_FOREACH(dcl, &s->listeners, next) { - if (con != (dcl->con ? dcl->con : active_console)) { + if (con != dcl->con) { continue; } if (dcl->ops->dpy_gl_update) { @@ -1415,30 +1357,21 @@ static QemuConsole *qemu_graphic_console_lookup_unused(void) QEMUCursor *qemu_console_get_cursor(QemuConsole *con) { - if (con == NULL) { - con = active_console; - } return QEMU_IS_GRAPHIC_CONSOLE(con) ? QEMU_GRAPHIC_CONSOLE(con)->cursor : NULL; } bool qemu_console_is_visible(QemuConsole *con) { - return (con == active_console) || (con->dcls > 0); + return con->dcls > 0; } bool qemu_console_is_graphic(QemuConsole *con) { - if (con == NULL) { - con = active_console; - } return con && QEMU_IS_GRAPHIC_CONSOLE(con); } bool qemu_console_is_fixedsize(QemuConsole *con) { - if (con == NULL) { - con = active_console; - } return con && (QEMU_IS_GRAPHIC_CONSOLE(con) || QEMU_IS_FIXED_TEXT_CONSOLE(con)); } @@ -1505,17 +1438,11 @@ char *qemu_console_get_label(QemuConsole *con) int qemu_console_get_index(QemuConsole *con) { - if (con == NULL) { - con = active_console; - } return con ? con->index : -1; } uint32_t qemu_console_get_head(QemuConsole *con) { - if (con == NULL) { - con = active_console; - } if (con == NULL) { return -1; } @@ -1527,9 +1454,6 @@ uint32_t qemu_console_get_head(QemuConsole *con) int qemu_console_get_width(QemuConsole *con, int fallback) { - if (con == NULL) { - con = active_console; - } if (con == NULL) { return fallback; } @@ -1547,9 +1471,6 @@ int qemu_console_get_width(QemuConsole *con, int fallback) int qemu_console_get_height(QemuConsole *con, int fallback) { - if (con == NULL) { - con = active_console; - } if (con == NULL) { return fallback; } diff --git a/ui/curses.c b/ui/curses.c index 8bde8c5cf7c3..ec61615f7c1f 100644 --- a/ui/curses.c +++ b/ui/curses.c @@ -98,7 +98,7 @@ static void curses_update(DisplayChangeListener *dcl, static void curses_calc_pad(void) { - if (qemu_console_is_fixedsize(NULL)) { + if (qemu_console_is_fixedsize(dcl->con)) { width = gwidth; height = gheight; } else { @@ -201,7 +201,7 @@ static void curses_cursor_position(DisplayChangeListener *dcl, curs_set(1); /* it seems that curs_set(1) must always be called before * curs_set(2) for the latter to have effect */ - if (!qemu_console_is_graphic(NULL)) { + if (!qemu_console_is_graphic(dcl->con)) { curs_set(2); } return; @@ -274,11 +274,11 @@ static void curses_refresh(DisplayChangeListener *dcl) clear(); refresh(); curses_calc_pad(); - graphic_hw_invalidate(NULL); + graphic_hw_invalidate(dcl->con); invalidate = 0; } - graphic_hw_text_update(NULL, screen); + graphic_hw_text_update(dcl->con, screen); while (1) { /* while there are any pending key strokes to process */ @@ -318,11 +318,16 @@ static void curses_refresh(DisplayChangeListener *dcl) /* process keys reserved for qemu */ if (keycode >= QEMU_KEY_CONSOLE0 && keycode < QEMU_KEY_CONSOLE0 + 9) { - erase(); - wnoutrefresh(stdscr); - console_select(keycode - QEMU_KEY_CONSOLE0); - - invalidate = 1; + QemuConsole *con = qemu_console_lookup_by_index(keycode - QEMU_KEY_CONSOLE0); + if (con) { + erase(); + wnoutrefresh(stdscr); + unregister_displaychangelistener(dcl); + dcl->con = con; + register_displaychangelistener(dcl); + + invalidate = 1; + } continue; } } @@ -354,45 +359,45 @@ static void curses_refresh(DisplayChangeListener *dcl) if (keycode == -1) continue; - if (qemu_console_is_graphic(NULL)) { + if (qemu_console_is_graphic(dcl->con)) { /* since terminals don't know about key press and release * events, we need to emit both for each key received */ if (keycode & SHIFT) { - qemu_input_event_send_key_number(NULL, SHIFT_CODE, true); + qemu_input_event_send_key_number(dcl->con, SHIFT_CODE, true); qemu_input_event_send_key_delay(0); } if (keycode & CNTRL) { - qemu_input_event_send_key_number(NULL, CNTRL_CODE, true); + qemu_input_event_send_key_number(dcl->con, CNTRL_CODE, true); qemu_input_event_send_key_delay(0); } if (keycode & ALT) { - qemu_input_event_send_key_number(NULL, ALT_CODE, true); + qemu_input_event_send_key_number(dcl->con, ALT_CODE, true); qemu_input_event_send_key_delay(0); } if (keycode & ALTGR) { - qemu_input_event_send_key_number(NULL, GREY | ALT_CODE, true); + qemu_input_event_send_key_number(dcl->con, GREY | ALT_CODE, true); qemu_input_event_send_key_delay(0); } - qemu_input_event_send_key_number(NULL, keycode & KEY_MASK, true); + qemu_input_event_send_key_number(dcl->con, keycode & KEY_MASK, true); qemu_input_event_send_key_delay(0); - qemu_input_event_send_key_number(NULL, keycode & KEY_MASK, false); + qemu_input_event_send_key_number(dcl->con, keycode & KEY_MASK, false); qemu_input_event_send_key_delay(0); if (keycode & ALTGR) { - qemu_input_event_send_key_number(NULL, GREY | ALT_CODE, false); + qemu_input_event_send_key_number(dcl->con, GREY | ALT_CODE, false); qemu_input_event_send_key_delay(0); } if (keycode & ALT) { - qemu_input_event_send_key_number(NULL, ALT_CODE, false); + qemu_input_event_send_key_number(dcl->con, ALT_CODE, false); qemu_input_event_send_key_delay(0); } if (keycode & CNTRL) { - qemu_input_event_send_key_number(NULL, CNTRL_CODE, false); + qemu_input_event_send_key_number(dcl->con, CNTRL_CODE, false); qemu_input_event_send_key_delay(0); } if (keycode & SHIFT) { - qemu_input_event_send_key_number(NULL, SHIFT_CODE, false); + qemu_input_event_send_key_number(dcl->con, SHIFT_CODE, false); qemu_input_event_send_key_delay(0); } } else { @@ -400,7 +405,7 @@ static void curses_refresh(DisplayChangeListener *dcl) if (keysym == -1) keysym = chr; - qemu_text_console_put_keysym(NULL, keysym); + qemu_text_console_put_keysym(QEMU_TEXT_CONSOLE(dcl->con), keysym); } } } @@ -798,6 +803,7 @@ static void curses_display_init(DisplayState *ds, DisplayOptions *opts) curses_winch_init(); dcl = g_new0(DisplayChangeListener, 1); + dcl->con = qemu_console_lookup_default(); dcl->ops = &dcl_ops; register_displaychangelistener(dcl); -- 2.44.0
console_select() is shared by other displays and a console_select() call from one of them triggers console switching also in ui/curses, circumventing key state reinitialization that needs to be performed in preparation and resulting in stuck keys. Use its internal state to track the current active console to prevent such a surprise console switch. Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> --- include/ui/console.h | 1 + include/ui/kbd-state.h | 11 +++++++++++ ui/console.c | 12 ++++++++++++ ui/kbd-state.c | 6 ++++++ ui/vnc.c | 14 +++++++++----- 5 files changed, 39 insertions(+), 5 deletions(-) diff --git a/include/ui/console.h b/include/ui/console.h index a4a49ffc640c..3729d2db29c7 100644 --- a/include/ui/console.h +++ b/include/ui/console.h @@ -413,6 +413,7 @@ void qemu_console_early_init(void); void qemu_console_set_display_gl_ctx(QemuConsole *con, DisplayGLCtx *ctx); +QemuConsole *qemu_console_lookup_default(void); QemuConsole *qemu_console_lookup_by_index(unsigned int index); QemuConsole *qemu_console_lookup_by_device(DeviceState *dev, uint32_t head); QemuConsole *qemu_console_lookup_by_device_name(const char *device_id, diff --git a/include/ui/kbd-state.h b/include/ui/kbd-state.h index fb79776128cf..1f37b932eb62 100644 --- a/include/ui/kbd-state.h +++ b/include/ui/kbd-state.h @@ -99,4 +99,15 @@ bool qkbd_state_modifier_get(QKbdState *kbd, QKbdModifier mod); */ void qkbd_state_lift_all_keys(QKbdState *kbd); +/** + * qkbd_state_switch_console: Switch console. + * + * This sends key up events to the previous console for all keys which are in + * down state to prevent keys being stuck, and remembers the new console. + * + * @kbd: state tracker state. + * @con: new QemuConsole for this state tracker. + */ +void qkbd_state_switch_console(QKbdState *kbd, QemuConsole *con); + #endif /* QEMU_UI_KBD_STATE_H */ diff --git a/ui/console.c b/ui/console.c index 832055675c50..fbc1b9b8b57c 100644 --- a/ui/console.c +++ b/ui/console.c @@ -1325,6 +1325,18 @@ void graphic_console_close(QemuConsole *con) dpy_gfx_replace_surface(con, surface); } +QemuConsole *qemu_console_lookup_default(void) +{ + QemuConsole *con; + + QTAILQ_FOREACH(con, &consoles, next) { + if (QEMU_IS_GRAPHIC_CONSOLE(con)) { + return con; + } + } + return QTAILQ_FIRST(&consoles); +} + QemuConsole *qemu_console_lookup_by_index(unsigned int index) { QemuConsole *con; diff --git a/ui/kbd-state.c b/ui/kbd-state.c index 62d42a7a22e1..52ed28b8a89b 100644 --- a/ui/kbd-state.c +++ b/ui/kbd-state.c @@ -117,6 +117,12 @@ void qkbd_state_lift_all_keys(QKbdState *kbd) } } +void qkbd_state_switch_console(QKbdState *kbd, QemuConsole *con) +{ + qkbd_state_lift_all_keys(kbd); + kbd->con = con; +} + void qkbd_state_set_delay(QKbdState *kbd, int delay_ms) { kbd->key_delay_ms = delay_ms; diff --git a/ui/vnc.c b/ui/vnc.c index fc12b343e254..b3fd78022b19 100644 --- a/ui/vnc.c +++ b/ui/vnc.c @@ -1872,12 +1872,16 @@ static void do_key_event(VncState *vs, int down, int keycode, int sym) /* QEMU console switch */ switch (qcode) { case Q_KEY_CODE_1 ... Q_KEY_CODE_9: /* '1' to '9' keys */ - if (vs->vd->dcl.con == NULL && down && + if (down && qkbd_state_modifier_get(vs->vd->kbd, QKBD_MOD_CTRL) && qkbd_state_modifier_get(vs->vd->kbd, QKBD_MOD_ALT)) { - /* Reset the modifiers sent to the current console */ - qkbd_state_lift_all_keys(vs->vd->kbd); - console_select(qcode - Q_KEY_CODE_1); + QemuConsole *con = qemu_console_lookup_by_index(qcode - Q_KEY_CODE_1); + if (con) { + unregister_displaychangelistener(&vs->vd->dcl); + qkbd_state_switch_console(vs->vd->kbd, con); + vs->vd->dcl.con = con; + register_displaychangelistener(&vs->vd->dcl); + } return; } default: @@ -4206,7 +4210,7 @@ void vnc_display_open(const char *id, Error **errp) goto fail; } } else { - con = NULL; + con = qemu_console_lookup_default(); } if (con != vd->dcl.con) { -- 2.44.0
A chardev-vc used to inherit the size of a graphic console when its size not explicitly specified, but it often did not make sense. If a chardev-vc is instantiated during the startup, the active graphic console has no content at the time, so it will have the size of graphic console placeholder, which contains no useful information. It's better to have the standard size of text console instead. Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- ui/console-vc.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ui/console-vc.c b/ui/console-vc.c index 9c13cc2981b0..f22c8e23c2ed 100644 --- a/ui/console-vc.c +++ b/ui/console-vc.c @@ -990,8 +990,8 @@ static void vc_chr_open(Chardev *chr, trace_console_txt_new(width, height); if (width == 0 || height == 0) { s = QEMU_TEXT_CONSOLE(object_new(TYPE_QEMU_TEXT_CONSOLE)); - width = qemu_console_get_width(NULL, 80 * FONT_WIDTH); - height = qemu_console_get_height(NULL, 24 * FONT_HEIGHT); + width = 80 * FONT_WIDTH; + height = 24 * FONT_HEIGHT; } else { s = QEMU_TEXT_CONSOLE(object_new(TYPE_QEMU_FIXED_TEXT_CONSOLE)); } -- 2.44.0
ui/cocoa needs to update the UI info and reset the keyboard state tracker when switching the console, or the new console will see the stale UI info or keyboard state. Previously, updating the UI info was done with cocoa_switch(), but it is meant to be called when the surface is being replaced, and may be called even when not switching the console. ui/cocoa never reset the keyboard state, which resulted in stuck keys. Add ui/cocoa's own implementation of console_select(), which updates the UI info and resets the keyboard state tracker. Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> --- ui/cocoa.m | 37 ++++++++++++++++++++++++++----------- 1 file changed, 26 insertions(+), 11 deletions(-) diff --git a/ui/cocoa.m b/ui/cocoa.m index fa879d7dcd4b..810751cf2644 100644 --- a/ui/cocoa.m +++ b/ui/cocoa.m @@ -102,6 +102,7 @@ static void cocoa_switch(DisplayChangeListener *dcl, static DisplayChangeListener dcl = { .ops = &dcl_ops, }; +static QKbdState *kbd; static int cursor_hide = 1; static int left_command_key_enabled = 1; static bool swap_opt_cmd; @@ -309,7 +310,6 @@ @interface QemuCocoaView : NSView NSTrackingArea *trackingArea; QEMUScreen screen; pixman_image_t *pixman_image; - QKbdState *kbd; BOOL isMouseGrabbed; BOOL isAbsoluteEnabled; CFMachPortRef eventsTap; @@ -361,7 +361,6 @@ - (id)initWithFrame:(NSRect)frameRect screen.width = frameRect.size.width; screen.height = frameRect.size.height; - kbd = qkbd_state_init(dcl.con); #if MAC_OS_X_VERSION_MAX_ALLOWED >= MAC_OS_VERSION_14_0 [self setClipsToBounds:YES]; #endif @@ -378,8 +377,6 @@ - (void) dealloc pixman_image_unref(pixman_image); } - qkbd_state_free(kbd); - if (eventsTap) { CFRelease(eventsTap); } @@ -429,6 +426,20 @@ - (void) viewWillMoveToWindow:(NSWindow *)newWindow [self removeTrackingRect]; } +- (void) selectConsoleLocked:(unsigned int)index +{ + QemuConsole *con = qemu_console_lookup_by_index(index); + if (!con) { + return; + } + + unregister_displaychangelistener(&dcl); + qkbd_state_switch_console(kbd, con); + dcl.con = con; + register_displaychangelistener(&dcl); + [self updateUIInfo]; +} + - (void) hideCursor { if (!cursor_hide) { @@ -718,7 +729,8 @@ - (void) handleMonitorInput:(NSEvent *)event } if (keysym) { - qemu_text_console_put_keysym(NULL, keysym); + QemuTextConsole *con = QEMU_TEXT_CONSOLE(dcl.con); + qemu_text_console_put_keysym(con, keysym); } } @@ -898,7 +910,7 @@ - (bool) handleEventLocked:(NSEvent *)event // enable graphic console case '1' ... '9': - console_select(key - '0' - 1); /* ascii math */ + [self selectConsoleLocked:key - '0' - 1]; /* ascii math */ return true; // release the mouse grab @@ -909,7 +921,7 @@ - (bool) handleEventLocked:(NSEvent *)event } } - if (qemu_console_is_graphic(NULL)) { + if (qemu_console_is_graphic(dcl.con)) { qkbd_state_key_event(kbd, keycode, true); } else { [self handleMonitorInput: event]; @@ -924,7 +936,7 @@ - (bool) handleEventLocked:(NSEvent *)event return true; } - if (qemu_console_is_graphic(NULL)) { + if (qemu_console_is_graphic(dcl.con)) { qkbd_state_key_event(kbd, keycode, false); } return true; @@ -1374,7 +1386,7 @@ - (void)toggleZoomInterpolation:(id) sender - (void)displayConsole:(id)sender { with_bql(^{ - console_select([sender tag]); + [cocoaView selectConsoleLocked:[sender tag]]; }); } @@ -1945,7 +1957,6 @@ static void cocoa_switch(DisplayChangeListener *dcl, pixman_image_ref(image); dispatch_async(dispatch_get_main_queue(), ^{ - [cocoaView updateUIInfo]; [cocoaView switchSurface:image]; }); } @@ -1955,7 +1966,7 @@ static void cocoa_refresh(DisplayChangeListener *dcl) NSAutoreleasePool * pool = [[NSAutoreleasePool alloc] init]; COCOA_DEBUG("qemu_cocoa: cocoa_refresh\n"); - graphic_hw_update(NULL); + graphic_hw_update(dcl->con); if (qemu_input_is_absolute(dcl->con)) { dispatch_async(dispatch_get_main_queue(), ^{ @@ -2039,8 +2050,12 @@ static void cocoa_display_init(DisplayState *ds, DisplayOptions *opts) add_console_menu_entries(); addRemovableDevicesMenuItems(); + dcl.con = qemu_console_lookup_default(); + kbd = qkbd_state_init(dcl.con); + // register vga output callbacks register_displaychangelistener(&dcl); + [cocoaView updateUIInfo]; qemu_event_init(&cbevent, false); cbowner = [[QemuCocoaPasteboardTypeOwner alloc] init]; -- 2.44.0
ui/console has a concept of "active" console; the active console is used when NULL is set for DisplayListener::con, and console_select() updates the active console state. However, the global nature of the state cause odd behaviors, and replacing NULL with the active console also resulted in extra code. Remove it to solve these problems. The active console state is shared, so if there are two displays referring to the active console, switching the console for one will also affect the other. All displays that use the active console state, namely cocoa, curses, and vnc, need to reset some of its state before switching the console, and such a reset operation cannot be performed if the console is switched by another display. This can result in stuck keys, for example. While the active console state is shared, displays other than cocoa, curses, and vnc don't update the state. A chardev-vc inherits the size of the active console, but it does not make sense for such a display. This series removes the shared "active" console state from ui/console. curses, cocoa, and vnc will hold the reference to the console currently shown with DisplayListener::con. This also eliminates the need to replace NULL with the active console and save code. Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> --- Changes in v2: - Changed to fall back to a text console if there is no graphical console as previously done. - Link to v1: https://lore.kernel.org/r/20240318-console-v1-0-f4efbfa71253@daynix.com --- Akihiko Odaki (4): ui/vc: Do not inherit the size of active console ui/vnc: Do not use console_select() ui/cocoa: Do not use console_select() ui/curses: Do not use console_select() include/ui/console.h | 2 +- include/ui/kbd-state.h | 11 ++++ ui/console-priv.h | 2 +- ui/console-vc-stubs.c | 2 +- ui/console-vc.c | 7 ++- ui/console.c | 133 ++++++++++++------------------------------------- ui/curses.c | 48 ++++++++++-------- ui/kbd-state.c | 6 +++ ui/vnc.c | 14 ++++-- ui/cocoa.m | 37 ++++++++++---- 10 files changed, 118 insertions(+), 144 deletions(-) --- base-commit: ba49d760eb04630e7b15f423ebecf6c871b8f77b change-id: 20240317-console-6744d4ab8ba6 Best regards, -- Akihiko Odaki <akihiko.odaki@daynix.com>
>-----Original Message----- >From: Eric Auger <eric.auger@redhat.com> >Subject: Re: [PATCH v1 04/11] vfio: Add HostIOMMUDevice handle into >VFIODevice > > > >On 2/28/24 04:58, Zhenzhong Duan wrote: >> This handle points to either IOMMULegacyDevice or IOMMUFDDevice >variant, >> neither both. >I would reword into: >store an handle to the HostIOMMUDevice the VFIODevice is associated with >. Its actual nature depends on the backend in use (VFIO or IOMMUFD). More clear, thanks Eric, will use it. Zhenzhong > >Thanks > >Eric >> >> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >> --- >> include/hw/vfio/vfio-common.h | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio- >common.h >> index 8bfb9cbe94..b6676c9f79 100644 >> --- a/include/hw/vfio/vfio-common.h >> +++ b/include/hw/vfio/vfio-common.h >> @@ -130,6 +130,7 @@ typedef struct VFIODevice { >> OnOffAuto pre_copy_dirty_page_tracking; >> bool dirty_pages_supported; >> bool dirty_tracking; >> + HostIOMMUDevice *base_hdev; >> int devid; >> IOMMUFDBackend *iommufd; >> } VFIODevice;
On 2024/03/18 20:31, Philippe Mathieu-Daudé wrote: > Extract the following methods: > > - qemu_console_set_rotate() > - qemu_console_is_rotated() > - qemu_console_get_rotate_arcdegree() > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> Reviewed-by: Akihiko Odaki <akihiko.odaki@daynix.com> > --- > include/ui/console.h | 3 +++ > ui/console.c | 16 ++++++++++++++++ > ui/input.c | 9 ++++----- > 3 files changed, 23 insertions(+), 5 deletions(-) > > diff --git a/include/ui/console.h b/include/ui/console.h > index a4a49ffc64..86ba36e391 100644 > --- a/include/ui/console.h > +++ b/include/ui/console.h > @@ -422,15 +422,18 @@ bool qemu_console_is_visible(QemuConsole *con); > bool qemu_console_is_graphic(QemuConsole *con); > bool qemu_console_is_fixedsize(QemuConsole *con); > bool qemu_console_is_gl_blocked(QemuConsole *con); > +bool qemu_console_is_rotated(QemuConsole *con); > char *qemu_console_get_label(QemuConsole *con); > int qemu_console_get_index(QemuConsole *con); > uint32_t qemu_console_get_head(QemuConsole *con); > int qemu_console_get_width(QemuConsole *con, int fallback); > int qemu_console_get_height(QemuConsole *con, int fallback); > +unsigned qemu_console_get_rotate_arcdegree(QemuConsole *con); > /* Return the low-level window id for the console */ > int qemu_console_get_window_id(QemuConsole *con); > /* Set the low-level window id for the console */ > void qemu_console_set_window_id(QemuConsole *con, int window_id); > +void qemu_console_set_rotate(QemuConsole *con, unsigned arcdegree); > > void console_select(unsigned int index); > void qemu_console_resize(QemuConsole *con, int width, int height); > diff --git a/ui/console.c b/ui/console.c > index 832055675c..84aee76846 100644 > --- a/ui/console.c > +++ b/ui/console.c > @@ -37,6 +37,7 @@ > #include "trace.h" > #include "exec/memory.h" > #include "qom/object.h" > +#include "sysemu/sysemu.h" > > #include "console-priv.h" > > @@ -207,6 +208,21 @@ void qemu_console_set_window_id(QemuConsole *con, int window_id) > con->window_id = window_id; > } > > +void qemu_console_set_rotate(QemuConsole *con, unsigned arcdegree) > +{ > + graphic_rotate = arcdegree; > +} > + > +bool qemu_console_is_rotated(QemuConsole *con) > +{ > + return graphic_rotate != 0; > +} > + > +unsigned qemu_console_get_rotate_arcdegree(QemuConsole *con) > +{ > + return graphic_rotate; > +} > + > void graphic_hw_invalidate(QemuConsole *con) > { > if (!con) { > diff --git a/ui/input.c b/ui/input.c > index dc745860f4..951806bf05 100644 > --- a/ui/input.c > +++ b/ui/input.c > @@ -1,5 +1,4 @@ > #include "qemu/osdep.h" > -#include "sysemu/sysemu.h" > #include "qapi/error.h" > #include "qapi/qapi-commands-ui.h" > #include "trace.h" > @@ -179,10 +178,10 @@ static int qemu_input_transform_invert_abs_value(int value) > return (int64_t)INPUT_EVENT_ABS_MAX - value + INPUT_EVENT_ABS_MIN; > } > > -static void qemu_input_transform_abs_rotate(InputEvent *evt) > +static void qemu_input_transform_abs_rotate(QemuConsole *src, InputEvent *evt) > { > InputMoveEvent *move = evt->u.abs.data; > - switch (graphic_rotate) { > + switch (qemu_console_get_rotate_arcdegree(src)) { > case 90: > if (move->axis == INPUT_AXIS_X) { > move->axis = INPUT_AXIS_Y; > @@ -341,8 +340,8 @@ void qemu_input_event_send_impl(QemuConsole *src, InputEvent *evt) > qemu_input_event_trace(src, evt); > > /* pre processing */ > - if (graphic_rotate && (evt->type == INPUT_EVENT_KIND_ABS)) { > - qemu_input_transform_abs_rotate(evt); > + if (qemu_console_is_rotated(src) && (evt->type == INPUT_EVENT_KIND_ABS)) { > + qemu_input_transform_abs_rotate(src, evt); > } > > /* send event */
On 2024/03/18 20:31, Philippe Mathieu-Daudé wrote: > Add the 'rotate_arcdegree' field to QemuGraphicConsole and > remove the use of the 'graphic_rotate' global. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> Reviewed-by: Akihiko Odaki <akihiko.odaki@daynix.com> > --- > ui/console.c | 15 +++++++++++---- > 1 file changed, 11 insertions(+), 4 deletions(-) > > diff --git a/ui/console.c b/ui/console.c > index 84aee76846..94624c37ae 100644 > --- a/ui/console.c > +++ b/ui/console.c > @@ -37,7 +37,6 @@ > #include "trace.h" > #include "exec/memory.h" > #include "qom/object.h" > -#include "sysemu/sysemu.h" > > #include "console-priv.h" > > @@ -51,6 +50,8 @@ typedef struct QemuGraphicConsole { > > QEMUCursor *cursor; > int cursor_x, cursor_y, cursor_on; > + > + unsigned rotate_arcdegree; > } QemuGraphicConsole; > > typedef QemuConsoleClass QemuGraphicConsoleClass; > @@ -210,17 +211,23 @@ void qemu_console_set_window_id(QemuConsole *con, int window_id) > > void qemu_console_set_rotate(QemuConsole *con, unsigned arcdegree) > { > - graphic_rotate = arcdegree; > + QemuGraphicConsole *gc = QEMU_GRAPHIC_CONSOLE(con); > + > + gc->rotate_arcdegree = arcdegree; > } > > bool qemu_console_is_rotated(QemuConsole *con) > { > - return graphic_rotate != 0; > + QemuGraphicConsole *gc = QEMU_GRAPHIC_CONSOLE(con); > + > + return gc->rotate_arcdegree != 0; > } > > unsigned qemu_console_get_rotate_arcdegree(QemuConsole *con) > { > - return graphic_rotate; > + QemuGraphicConsole *gc = QEMU_GRAPHIC_CONSOLE(con); > + > + return gc->rotate_arcdegree; > } > > void graphic_hw_invalidate(QemuConsole *con)
On 2024/03/18 20:31, Philippe Mathieu-Daudé wrote: > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> Reviewed-by: Akihiko Odaki <akihiko.odaki@daynix.com> > --- > hw/display/pxa2xx_lcd.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/hw/display/pxa2xx_lcd.c b/hw/display/pxa2xx_lcd.c > index a9d0d981a0..7d03fa57d0 100644 > --- a/hw/display/pxa2xx_lcd.c > +++ b/hw/display/pxa2xx_lcd.c > @@ -1439,6 +1439,7 @@ PXA2xxLCDState *pxa2xx_lcdc_init(MemoryRegion *sysmem, > memory_region_add_subregion(sysmem, base, &s->iomem); > > s->con = graphic_console_init(NULL, 0, &pxa2xx_ops, s); > + qemu_console_set_rotate(s->con, graphic_rotate); > > vmstate_register(NULL, 0, &vmstate_pxa2xx_lcdc, s); >
On 2/29/2024 14:36, Xiaoyao Li wrote:> KVM provides TDX capabilities via sub command KVM_TDX_CAPABILITIES of > IOCTL(KVM_MEMORY_ENCRYPT_OP). Get the capabilities when initializing > TDX context. It will be used to validate user's setting later. > > Since there is no interface reporting how many cpuid configs contains in > KVM_TDX_CAPABILITIES, QEMU chooses to try starting with a known number > and abort when it exceeds KVM_MAX_CPUID_ENTRIES. > > Besides, introduce the interfaces to invoke TDX "ioctls" at different > scope (KVM, VM and VCPU) in preparation. tdx_platform_ioctl() is dropped because no user so suggest rephrasing this statement because no KVM scope ioctl interface is introduced in this patch. > > Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com> > --- > Changes in v4: > - use {} to initialize struct kvm_tdx_cmd, to avoid memset(); > - remove tdx_platform_ioctl() because no user; > > Changes in v3: > - rename __tdx_ioctl() to tdx_ioctl_internal() > - Pass errp in get_tdx_capabilities(); > > changes in v2: > - Make the error message more clear; > > changes in v1: > - start from nr_cpuid_configs = 6 for the loop; > - stop the loop when nr_cpuid_configs exceeds KVM_MAX_CPUID_ENTRIES; > --- > target/i386/kvm/kvm.c | 2 - > target/i386/kvm/kvm_i386.h | 2 + > target/i386/kvm/tdx.c | 91 +++++++++++++++++++++++++++++++++++++- > 3 files changed, 92 insertions(+), 3 deletions(-) > > diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c > index 52d99d30bdc8..0e68e80f4291 100644 > --- a/target/i386/kvm/kvm.c > +++ b/target/i386/kvm/kvm.c > @@ -1685,8 +1685,6 @@ static int hyperv_init_vcpu(X86CPU *cpu) > > static Error *invtsc_mig_blocker; > > -#define KVM_MAX_CPUID_ENTRIES 100 > - > static void kvm_init_xsave(CPUX86State *env) > { > if (has_xsave2) { > diff --git a/target/i386/kvm/kvm_i386.h b/target/i386/kvm/kvm_i386.h > index 55fb25fa8e2e..c3ef46a97a7b 100644 > --- a/target/i386/kvm/kvm_i386.h > +++ b/target/i386/kvm/kvm_i386.h > @@ -13,6 +13,8 @@ > > #include "sysemu/kvm.h" > > +#define KVM_MAX_CPUID_ENTRIES 100 > + > #ifdef CONFIG_KVM > > #define kvm_pit_in_kernel() \ > diff --git a/target/i386/kvm/tdx.c b/target/i386/kvm/tdx.c > index d9a1dd46dc69..2b956450a083 100644 > --- a/target/i386/kvm/tdx.c > +++ b/target/i386/kvm/tdx.c > @@ -12,18 +12,107 @@ > */ > > #include "qemu/osdep.h" > +#include "qemu/error-report.h" > +#include "qapi/error.h" > #include "qom/object_interfaces.h" > +#include "sysemu/kvm.h" > > #include "hw/i386/x86.h" > +#include "kvm_i386.h" > #include "tdx.h" > > +static struct kvm_tdx_capabilities *tdx_caps; > + > +enum tdx_ioctl_level{ > + TDX_VM_IOCTL, > + TDX_VCPU_IOCTL, > +}; > + > +static int tdx_ioctl_internal(void *state, enum tdx_ioctl_level level, int cmd_id, > + __u32 flags, void *data) > +{ > + struct kvm_tdx_cmd tdx_cmd = {}; > + int r; > + > + tdx_cmd.id = cmd_id; > + tdx_cmd.flags = flags; > + tdx_cmd.data = (__u64)(unsigned long)data; > + > + switch (level) { > + case TDX_VM_IOCTL: > + r = kvm_vm_ioctl(kvm_state, KVM_MEMORY_ENCRYPT_OP, &tdx_cmd); > + break; > + case TDX_VCPU_IOCTL: > + r = kvm_vcpu_ioctl(state, KVM_MEMORY_ENCRYPT_OP, &tdx_cmd); > + break; > + default: > + error_report("Invalid tdx_ioctl_level %d", level); > + exit(1); > + } > + > + return r; > +} > + > +static inline int tdx_vm_ioctl(int cmd_id, __u32 flags, void *data) > +{ > + return tdx_ioctl_internal(NULL, TDX_VM_IOCTL, cmd_id, flags, data); > +} > + > +static inline int tdx_vcpu_ioctl(void *vcpu_fd, int cmd_id, __u32 flags, > + void *data) > +{ > + return tdx_ioctl_internal(vcpu_fd, TDX_VCPU_IOCTL, cmd_id, flags, data); > +} > + > +static int get_tdx_capabilities(Error **errp) > +{ > + struct kvm_tdx_capabilities *caps; > + /* 1st generation of TDX reports 6 cpuid configs */ > + int nr_cpuid_configs = 6; > + size_t size; > + int r; > + > + do { > + size = sizeof(struct kvm_tdx_capabilities) + > + nr_cpuid_configs * sizeof(struct kvm_tdx_cpuid_config); > + caps = g_malloc0(size); > + caps->nr_cpuid_configs = nr_cpuid_configs; > + > + r = tdx_vm_ioctl(KVM_TDX_CAPABILITIES, 0, caps); > + if (r == -E2BIG) { > + g_free(caps); > + nr_cpuid_configs *= 2; > + if (nr_cpuid_configs > KVM_MAX_CPUID_ENTRIES) { > + error_setg(errp, "%s: KVM TDX seems broken that number of CPUID " > + "entries in kvm_tdx_capabilities exceeds limit %d", > + __func__, KVM_MAX_CPUID_ENTRIES); > + return r; > + } > + } else if (r < 0) { > + g_free(caps); > + error_setg_errno(errp, -r, "%s: KVM_TDX_CAPABILITIES failed", __func__); > + return r; > + } > + } > + while (r == -E2BIG); > + > + tdx_caps = caps; > + > + return 0; > +} > + > static int tdx_kvm_init(ConfidentialGuestSupport *cgs, Error **errp) > { > MachineState *ms = MACHINE(qdev_get_machine()); > + int r = 0; > > ms->require_guest_memfd = true; > > - return 0; > + if (!tdx_caps) { > + r = get_tdx_capabilities(errp); > + } > + > + return r; > } > > /* tdx guest */
With numa_test test case, there is subcase named test_def_cpu_split(), there are 8 sockets and 2 numa nodes. Here is command line: "-machine smp.cpus=8,smp.sockets=8 -numa node,memdev=ram -numa node" The required result is: node 0 cpus: 0 2 4 6 node 1 cpus: 1 3 5 7 Test case numa_test fails on LoongArch, since the actual result is: node 0 cpus: 0 1 2 3 node 1 cpus: 4 5 6 7 It will be better if all the cpus in one socket share the same numa node. Here socket id is used to calculate numa id in function virt_get_default_cpu_node_id(). Signed-off-by: Bibo Mao <maobibo@loongson.cn> --- hw/loongarch/virt.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c index deb3750d81..29885f6777 100644 --- a/hw/loongarch/virt.c +++ b/hw/loongarch/virt.c @@ -1219,15 +1219,14 @@ virt_cpu_index_to_props(MachineState *ms, unsigned cpu_index) static int64_t virt_get_default_cpu_node_id(const MachineState *ms, int idx) { - int64_t nidx = 0; + int64_t socket_id; if (ms->numa_state->num_nodes) { - nidx = idx / (ms->smp.cpus / ms->numa_state->num_nodes); - if (ms->numa_state->num_nodes <= nidx) { - nidx = ms->numa_state->num_nodes - 1; - } + socket_id = ms->possible_cpus->cpus[idx].props.socket_id; + return socket_id % ms->numa_state->num_nodes; + } else { + return 0; } - return nidx; } static void loongarch_class_init(ObjectClass *oc, void *data) -- 2.39.3
From: Yao Xingtao <yaoxt.fnst@fujitsu.com> In qemu monitor mode, when we use gpa2hva command to print the host virtual address corresponding to a guest physical address, if the gpa is not in RAM, the error message is below: (qemu) gpa2hva 0x750000000 Memory at address 0x750000000is not RAM a space is missed between '0x750000000' and 'is'. Signed-off-by: Yao Xingtao <yaoxt.fnst@fujitsu.com> --- monitor/hmp-cmds-target.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/monitor/hmp-cmds-target.c b/monitor/hmp-cmds-target.c index 9338ae8440..ff01cf9d8d 100644 --- a/monitor/hmp-cmds-target.c +++ b/monitor/hmp-cmds-target.c @@ -261,7 +261,7 @@ void *gpa2hva(MemoryRegion **p_mr, hwaddr addr, uint64_t size, Error **errp) } if (!memory_region_is_ram(mrs.mr) && !memory_region_is_romd(mrs.mr)) { - error_setg(errp, "Memory at address 0x%" HWADDR_PRIx "is not RAM", addr); + error_setg(errp, "Memory at address 0x%" HWADDR_PRIx " is not RAM", addr); memory_region_unref(mrs.mr); return NULL; } -- 2.27.0
On 2/29/2024 14:36, Xiaoyao Li wrote: > From: Chao Peng <chao.p.peng@linux.intel.com> > > When geeting KVM_EXIT_MEMORY_FAULT exit, it indicates userspace needs to > do the memory conversion on the RAMBlock to turn the memory into desired > attribute, i.e., private/shared. > > Currently only KVM_MEMORY_EXIT_FLAG_PRIVATE in flags is valid when > KVM_EXIT_MEMORY_FAULT happens. > > Note, KVM_EXIT_MEMORY_FAULT makes sense only when the RAMBlock has > guest_memfd memory backend. > > Note, KVM_EXIT_MEMORY_FAULT returns with -EFAULT, so special handling is > added. > > When page is converted from shared to private, the original shared > memory can be discarded via ram_block_discard_range(). Note, shared > memory can be discarded only when it's not back'ed by hugetlb because > hugetlb is supposed to be pre-allocated and no need for discarding. > > Signed-off-by: Chao Peng <chao.p.peng@linux.intel.com> > Co-developed-by: Xiaoyao Li <xiaoyao.li@intel.com> > Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com> > > --- > Changes in v4: > - open-coded ram_block_discard logic; > - change warn_report() to error_report(); (Daniel) > --- > accel/kvm/kvm-all.c | 94 ++++++++++++++++++++++++++++++++++++++++----- > 1 file changed, 84 insertions(+), 10 deletions(-) > > diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c > index 70d482a2c936..87e4275932a7 100644 > --- a/accel/kvm/kvm-all.c > +++ b/accel/kvm/kvm-all.c > @@ -2903,6 +2903,68 @@ static void kvm_eat_signals(CPUState *cpu) > } while (sigismember(&chkset, SIG_IPI)); > } > > +static int kvm_convert_memory(hwaddr start, hwaddr size, bool to_private) > +{ > + MemoryRegionSection section; > + ram_addr_t offset; > + MemoryRegion *mr; > + RAMBlock *rb; > + void *addr; > + int ret = -1; > + > + if (!QEMU_PTR_IS_ALIGNED(start, qemu_host_page_size) || > + !QEMU_PTR_IS_ALIGNED(size, qemu_host_page_size)) { > + return -1; > + } > + > + if (!size) { > + return -1; > + } > + > + section = memory_region_find(get_system_memory(), start, size); > + mr = section.mr; > + if (!mr) { > + return -1; > + } > + > + if (memory_region_has_guest_memfd(mr)) { > + if (to_private) { > + ret = kvm_set_memory_attributes_private(start, size); > + } else { > + ret = kvm_set_memory_attributes_shared(start, size); > + } > + > + if (ret) { > + memory_region_unref(section.mr); > + return ret; > + } > + > + addr = memory_region_get_ram_ptr(mr) + section.offset_within_region; > + rb = qemu_ram_block_from_host(addr, false, &offset); > + > + if (to_private) { > + if (rb->page_size != qemu_host_page_size) { > + /* > + * shared memory is back'ed by hugetlb, which is supposed to be > + * pre-allocated and doesn't need to be discarded > + */ Nit: comment indentation is broken here. > + return 0; > + } else { > + ret = ram_block_discard_range(rb, offset, size); > + } > + } else { > + ret = ram_block_discard_guest_memfd_range(rb, offset, size); > + } > + } else { > + error_report("Convert non guest_memfd backed memory region " > + "(0x%"HWADDR_PRIx" ,+ 0x%"HWADDR_PRIx") to %s", Same as above. > + start, size, to_private ? "private" : "shared"); > + } > + > + memory_region_unref(section.mr); > + return ret; > +} > + > int kvm_cpu_exec(CPUState *cpu) > { > struct kvm_run *run = cpu->kvm_run; > @@ -2970,18 +3032,20 @@ int kvm_cpu_exec(CPUState *cpu) > ret = EXCP_INTERRUPT; > break; > } > - fprintf(stderr, "error: kvm run failed %s\n", > - strerror(-run_ret)); > + if (!(run_ret == -EFAULT && run->exit_reason == KVM_EXIT_MEMORY_FAULT)) { > + fprintf(stderr, "error: kvm run failed %s\n", > + strerror(-run_ret)); > #ifdef TARGET_PPC > - if (run_ret == -EBUSY) { > - fprintf(stderr, > - "This is probably because your SMT is enabled.\n" > - "VCPU can only run on primary threads with all " > - "secondary threads offline.\n"); > - } > + if (run_ret == -EBUSY) { > + fprintf(stderr, > + "This is probably because your SMT is enabled.\n" > + "VCPU can only run on primary threads with all " > + "secondary threads offline.\n"); > + } > #endif > - ret = -1; > - break; > + ret = -1; > + break; > + } > } > > trace_kvm_run_exit(cpu->cpu_index, run->exit_reason); > @@ -3064,6 +3128,16 @@ int kvm_cpu_exec(CPUState *cpu) > break; > } > break; > + case KVM_EXIT_MEMORY_FAULT: > + if (run->memory_fault.flags & ~KVM_MEMORY_EXIT_FLAG_PRIVATE) { > + error_report("KVM_EXIT_MEMORY_FAULT: Unknown flag 0x%" PRIx64, > + (uint64_t)run->memory_fault.flags); > + ret = -1; > + break; > + } > + ret = kvm_convert_memory(run->memory_fault.gpa, run->memory_fault.size, > + run->memory_fault.flags & KVM_MEMORY_EXIT_FLAG_PRIVATE); > + break; > default: > ret = kvm_arch_handle_exit(cpu, run); > break;
On 3/19/2024 5:51 AM, Paolo Bonzini wrote: > On Thu, Feb 29, 2024 at 7:01 AM Xiaoyao Li <xiaoyao.li@intel.com> wrote: >> >> Use confidential_guest_kvm_init() instead of calling SEV specific >> sev_kvm_init(). As a bouns, it fits to future TDX when TDX implements >> its own confidential_guest_support and .kvm_init(). >> >> Move the "TypeInfo sev_guest_info" definition and related functions to >> the end of the file, to avoid declaring the sev_kvm_init() ahead. >> >> Delete the sve-stub.c since it's not needed anymore. >> >> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com> >> --- >> Changes from rfc v1: >> - check ms->cgs not NULL before calling confidential_guest_kvm_init(); >> - delete the sev-stub.c; > > Queued, with just one small simplification that can be done on top: thank you, Paolo! > diff --git a/target/i386/sev.c b/target/i386/sev.c > index e89d64fa52..b8f79d34d1 100644 > --- a/target/i386/sev.c > +++ b/target/i386/sev.c > @@ -851,18 +851,13 @@ sev_vm_state_change(void *opaque, bool running, > RunState state) > > static int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp) > { > - SevGuestState *sev > - = (SevGuestState *)object_dynamic_cast(OBJECT(cgs), TYPE_SEV_GUEST); > + SevGuestState *sev = SEV_GUEST(cgs); > char *devname; > int ret, fw_error, cmd; > uint32_t ebx; > uint32_t host_cbitpos; > struct sev_user_data_status status = {}; > > - if (!sev) { > - return 0; > - } > - > ret = ram_block_discard_disable(true); > if (ret) { > error_report("%s: cannot disable RAM discard", __func__); It looks good. > Thanks! > > Paolo
On 2/29/2024 14:36, Xiaoyao Li wrote:> Introduce the helper functions to set the attributes of a range of > memory to private or shared. > > This is necessary to notify KVM the private/shared attribute of each gpa > range. KVM needs the information to decide the GPA needs to be mapped at > hva-based shared memory or guest_memfd based private memory. > > Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com> > --- > Changes in v4: > - move the check of kvm_supported_memory_attributes to the common > kvm_set_memory_attributes(); (Wang Wei) > - change warn_report() to error_report() in kvm_set_memory_attributes() > and drop the __func__; (Daniel) > --- > accel/kvm/kvm-all.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ > include/sysemu/kvm.h | 3 +++ > 2 files changed, 47 insertions(+) > > diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c > index cd0aa7545a1f..70d482a2c936 100644 > --- a/accel/kvm/kvm-all.c > +++ b/accel/kvm/kvm-all.c > @@ -92,6 +92,7 @@ static bool kvm_has_guest_debug; > static int kvm_sstep_flags; > static bool kvm_immediate_exit; > static bool kvm_guest_memfd_supported; > +static uint64_t kvm_supported_memory_attributes; > static hwaddr kvm_max_slot_size = ~0; > > static const KVMCapabilityInfo kvm_required_capabilites[] = { > @@ -1304,6 +1305,46 @@ void kvm_set_max_memslot_size(hwaddr max_slot_size) > kvm_max_slot_size = max_slot_size; > } > > +static int kvm_set_memory_attributes(hwaddr start, hwaddr size, uint64_t attr) > +{ > + struct kvm_memory_attributes attrs; > + int r; > + > + if (kvm_supported_memory_attributes == 0) { > + error_report("No memory attribute supported by KVM\n"); > + return -EINVAL; > + } > + > + if ((attr & kvm_supported_memory_attributes) != attr) { > + error_report("memory attribute 0x%lx not supported by KVM," > + " supported bits are 0x%lx\n", > + attr, kvm_supported_memory_attributes); > + return -EINVAL; > + } > + > + attrs.attributes = attr; > + attrs.address = start; > + attrs.size = size; > + attrs.flags = 0; > + > + r = kvm_vm_ioctl(kvm_state, KVM_SET_MEMORY_ATTRIBUTES, &attrs); > + if (r) { > + error_report("failed to set memory (0x%lx+%#zx) with attr 0x%lx error '%s'", > + start, size, attr, strerror(errno)); > + } > + return r; > +} > + > +int kvm_set_memory_attributes_private(hwaddr start, hwaddr size) > +{ > + return kvm_set_memory_attributes(start, size, KVM_MEMORY_ATTRIBUTE_PRIVATE); > +} > + > +int kvm_set_memory_attributes_shared(hwaddr start, hwaddr size) > +{ > + return kvm_set_memory_attributes(start, size, 0); > +} > + > /* Called with KVMMemoryListener.slots_lock held */ > static void kvm_set_phys_mem(KVMMemoryListener *kml, > MemoryRegionSection *section, bool add) > @@ -2439,6 +2480,9 @@ static int kvm_init(MachineState *ms) > > kvm_guest_memfd_supported = kvm_check_extension(s, KVM_CAP_GUEST_MEMFD); > > + ret = kvm_check_extension(s, KVM_CAP_MEMORY_ATTRIBUTES); > + kvm_supported_memory_attributes = ret > 0 ? ret : 0; kvm_check_extension() only returns non-negative value, so we can just kvm_supported_memory_attributes = kvm_check_extension(s, KVM_CAP_MEMORY_ATTRIBUTES); > + > if (object_property_find(OBJECT(current_machine), "kvm-type")) { > g_autofree char *kvm_type = object_property_get_str(OBJECT(current_machine), > "kvm-type", > diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h > index 6cdf82de8372..8e83adfbbd19 100644 > --- a/include/sysemu/kvm.h > +++ b/include/sysemu/kvm.h > @@ -546,4 +546,7 @@ uint32_t kvm_dirty_ring_size(void); > bool kvm_hwpoisoned_mem(void); > > int kvm_create_guest_memfd(uint64_t size, uint64_t flags, Error **errp); > + > +int kvm_set_memory_attributes_private(hwaddr start, hwaddr size); > +int kvm_set_memory_attributes_shared(hwaddr start, hwaddr size); > #endif
On 2024/3/15 1:56, Daniel Henrique Barboza wrote: > trans_vmv_x_s, trans_vmv_s_x, trans_vfmv_f_s and trans_vfmv_s_f aren't > setting vstart = 0 after execution. This is usually done by a helper in > vector_helper.c but these functions don't use helpers. > > We'll set vstart after any potential 'over' brconds, and that will also > mandate a mark_vs_dirty() too. > > Fixes: dedc53cbc9 ("target/riscv: rvv-1.0: integer scalar move instructions") > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> > Reviewed-by: Richard Henderson <richard.henderson@linaro.org> Reviewd-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com> Zhiwei > --- > target/riscv/insn_trans/trans_rvv.c.inc | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/target/riscv/insn_trans/trans_rvv.c.inc b/target/riscv/insn_trans/trans_rvv.c.inc > index e42728990e..8c16a9f5b3 100644 > --- a/target/riscv/insn_trans/trans_rvv.c.inc > +++ b/target/riscv/insn_trans/trans_rvv.c.inc > @@ -3373,6 +3373,8 @@ static bool trans_vmv_x_s(DisasContext *s, arg_vmv_x_s *a) > vec_element_loadi(s, t1, a->rs2, 0, true); > tcg_gen_trunc_i64_tl(dest, t1); > gen_set_gpr(s, a->rd, dest); > + tcg_gen_movi_tl(cpu_vstart, 0); > + mark_vs_dirty(s); > return true; > } > return false; > @@ -3399,8 +3401,9 @@ static bool trans_vmv_s_x(DisasContext *s, arg_vmv_s_x *a) > s1 = get_gpr(s, a->rs1, EXT_NONE); > tcg_gen_ext_tl_i64(t1, s1); > vec_element_storei(s, a->rd, 0, t1); > - mark_vs_dirty(s); > gen_set_label(over); > + tcg_gen_movi_tl(cpu_vstart, 0); > + mark_vs_dirty(s); > return true; > } > return false; > @@ -3427,6 +3430,8 @@ static bool trans_vfmv_f_s(DisasContext *s, arg_vfmv_f_s *a) > } > > mark_fs_dirty(s); > + tcg_gen_movi_tl(cpu_vstart, 0); > + mark_vs_dirty(s); > return true; > } > return false; > @@ -3452,8 +3457,9 @@ static bool trans_vfmv_s_f(DisasContext *s, arg_vfmv_s_f *a) > do_nanbox(s, t1, cpu_fpr[a->rs1]); > > vec_element_storei(s, a->rd, 0, t1); > - mark_vs_dirty(s); > gen_set_label(over); > + tcg_gen_movi_tl(cpu_vstart, 0); > + mark_vs_dirty(s); > return true; > } > return false;
On 2024/3/15 1:56, Daniel Henrique Barboza wrote: > The helper isn't setting env->vstart = 0 after its execution, as it is > expected from every vector instruction that completes successfully. > > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> > Reviewed-by: Richard Henderson <richard.henderson@linaro.org> > Reviewed-by: Alistair Francis <alistair.francis@wdc.com> Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com> Zhiwei > --- > target/riscv/vector_helper.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/target/riscv/vector_helper.c b/target/riscv/vector_helper.c > index fe56c007d5..ca79571ae2 100644 > --- a/target/riscv/vector_helper.c > +++ b/target/riscv/vector_helper.c > @@ -4781,6 +4781,7 @@ void HELPER(NAME)(void *vd, void *v0, target_ulong s1, void *vs2, \ > } \ > *((ETYPE *)vd + H(i)) = *((ETYPE *)vs2 + H(i - offset)); \ > } \ > + env->vstart = 0; \ > /* set tail elements to 1s */ \ > vext_set_elems_1s(vd, vta, vl * esz, total_elems * esz); \ > }
Jonathan Cameron wrote:
> On Mon, 18 Mar 2024 10:29:28 +0800
> Yuquan Wang <wangyuquan1236@phytium.com.cn> wrote:
>
> > The dev_dbg info for Clear Event Records mailbox command would report
> > the handle of the next record to clear not the current one.
> >
> > This was because the index 'i' had incremented before printing the
> > current handle value.
> >
> > Signed-off-by: Yuquan Wang <wangyuquan1236@phytium.com.cn>
> > ---
> > drivers/cxl/core/mbox.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> > index 9adda4795eb7..b810a6aa3010 100644
> > --- a/drivers/cxl/core/mbox.c
> > +++ b/drivers/cxl/core/mbox.c
> > @@ -915,7 +915,7 @@ static int cxl_clear_event_record(struct cxl_memdev_state *mds,
> >
> > payload->handles[i++] = gen->hdr.handle;
> > dev_dbg(mds->cxlds.dev, "Event log '%d': Clearing %u\n", log,
> > - le16_to_cpu(payload->handles[i]));
> > + le16_to_cpu(payload->handles[i-1]));
> Trivial but needs spaces around the -. e.g. [i - 1]
>
> Maybe Dan can fix up whilst applying.
>
> Otherwise
>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
I have enlisted Dave to start wrangling CXL kernel patches upstream, and
I will fall back to just reviewing.
Dave, you can add my:
Reviewed-by: Dan Williams <dan.j.williams@intel.com>
...with the same caveat as above.
On Mon, Mar 18, 2024 at 08:08:29PM +0300, Vladimir Sementsov-Ogievskiy wrote: > On 08.03.24 06:47, Peter Xu wrote: > > On Thu, Mar 07, 2024 at 12:06:59PM +0300, Maksim Davydov wrote: > > > > > > On 3/6/24 04:57, Peter Xu wrote: > > > > On Tue, Mar 05, 2024 at 03:43:41PM +0100, Markus Armbruster wrote: > > > > > Peter Maydell<peter.maydell@linaro.org> writes: > > > > > > > > > > > On Mon, 4 Mar 2024 at 13:52, Maksim Davydov<davydov-max@yandex-team.ru> wrote: > > > > > > > The following changes since commit e1007b6bab5cf97705bf4f2aaec1f607787355b8: > > > > > > > > > > > > > > Merge tag 'pull-request-2024-03-01' ofhttps://gitlab.com/thuth/qemu into staging (2024-03-01 10:14:32 +0000) > > > > > > > > > > > > > > are available in the Git repository at: > > > > > > > > > > > > > > https://gitlab.com/davydov-max/qemu.git tags/pull-compare-mt-2024-03-04 > > > > > > > > > > > > > > for you to fetch changes up to 7693a2e8518811a907d73a85807ee71dac8fabcb: > > > > > > > > > > > > > > scripts: add script to compare compatibility properties (2024-03-04 14:10:53 +0300) > > > > > > > > > > > > > > ---------------------------------------------------------------- > > > > > > > Please note. This is the first pull request from me. > > > > > > > My public GPG key is available here > > > > > > > https://keys.openpgp.org/vks/v1/by-fingerprint/CDB5BEEF8837142579F5CDFE8E927E10F72F78D4 > > > > > > > > > > > > > > ---------------------------------------------------------------- > > > > > > > scripts: add a new script for machine development > > > > > > > > > > > > > > ---------------------------------------------------------------- > > > > > > Hi; I would prefer this to go through some existing submaintainer > > > > > > tree if possible, please. > > > > > Migration? QOM? Not sure. Cc'ing the maintainers anyway. > > > > Yeah this seems like migration relevant.. however now I'm slightly confused > > > > on when this script should be useful. > > > > > > > > According to: > > > > > > > > https://lore.kernel.org/qemu-devel/20240222153912.646053-5-davydov-max@yandex-team.ru/ > > > > > > > > This script runs QEMU to obtain compat_props of machines and > > > > default values of different types of drivers to produce comparison > > > > table. This table can be used to compare machine types to choose > > > > the most suitable machine or compare binaries to be sure that > > > > migration to the newer version will save all device > > > > properties. Also the json or csv format of this table can be used > > > > to check does a new machine affect the previous ones by comparing > > > > tables with and without the new machine. > > > > > > > > In regards to "choose the most suitable machine": why do you need to choose > > > > a machine? > > > > > > > > If it's pretty standalone setup, shouldn't we always try to use the latest > > > > machine type if possible (as normally compat props are only used to keep > > > > compatible with old machine types, and the default should always be > > > > preferred). If it's a cluster setup, IMHO it should depend on the oldest > > > > QEMU version that plans to be supported. I don't see how such comparison > > > > helps yet in either of the cases.. > > > > > > > > In regards to "compare binaries to be sure that migration to the newer > > > > version will save all device properties": do we even support migrating > > > > _between_ machine types?? > > > > > > > > Sololy relying on compat properties to detect device compatibility is also > > > > not reliable. For example, see VMStateField.field_exists() or similarly, > > > > VMStateDescription.needed(), which are hooks that device can provide to > > > > dynamically decide what device state to be saved/loaded. Such things are > > > > not reflected in compat properties, so even if compat properties of all > > > > devices are the same between two machine types, it may not mean that the > > > > migration stream will always be compatible. > > > > > > > > Thanks, > > > > > > In fact, the last commit describes the meaning of this series best. Perhaps > > > it should have been moved to the cover letter: > > > Often, many teams have their own "machines" inherited from "upstream" to > > > manage default values of devices. This is done because of the limitations > > > imposed by the compatibility requirements or the desire to help their > > > customers better configure their devices. And since machine type has > > > a hard-to-read structure, it is very easy to make a mistake when > > > transferring > > > default values from one machine to another. For example, when some property > > > is set for the entire abstract class x86_64-cpu (which will be applied to > > > all > > > models), and then rolled back for a specific model. The situation is about > > > the same with changing the default values of devices: if the value changes > > > in the description of the device itself, then you need to make sure that > > > nothing changes when using the current machine. > > > Therefore, there was a desire to make a dev tool that will help quickly > > > expand > > > the definition of a machine or compare several machines from different > > > binary > > > files. It can be used when rebasing to a new version of qemu and/or for > > > automatic tests. > > > > OK, thanks. > > > > So is it a migration compatibility issue that you care (migrating VMs from > > your old downstream binary to new downstream binary should always succeed), > > or perhaps you care more on making sure the features you wanted, i.e., you > > want to make sure some specific devices that you care will have the > > properties that you expect? > > Actually both things. > > 1. We need a tool to analyze, what exactly changes between MT-s. Do we want to move on new upstream MT or not, how much it is different from our downstream MT and so on. > 2. It also could be used to check, that new MT is correctly defined (not breaking old MT's) > > > > > I think compat properties are mostly used for migration purposes, but > > indeed it can also be used to keep old behaviors of devices, even if the > > migration could succed with/without such a compat property entry. > > > > If it's about migration, I'd like to know whether vmstate-static-checker.py > > could also help your case (under scripts/), perhaps in a better way, > > because it directly observes the VMSD structures (which is the ultimate > > form on wire, after all these compat properties applied to the devices). > > Hmm, vmstate-static-checker.py checks a concrete device configuration. So it's a different thing. I don't think so - 'qemu -dump-vmstate' should dump all device states that it ever supports. Feel free to have a look at dump_vmstate_json_to_file(), or just try give it a dump. > > > > > If it's not about migration, then maybe it's more QOM-relevant, and if so I > > don't have a strong opinion. It seems still make some sense to have a tool > > simply dump the QOM tree for a machine type with all properties and compare > > them between machines with some binaries. For that I'll leave that to > > Markus to decide. > > Markus ACKed :) I didn't see Markus acked all the patches yet, but if so that's okay then. Even if so, I think what Peter Maydell suggested is then this series should go through the QOM tree, rather than a separate pull. Thanks, -- Peter Xu
On 3/17/2024 8:22 PM, Jason Wang wrote: > On Sat, Mar 16, 2024 at 2:45 AM Si-Wei Liu <si-wei.liu@oracle.com> wrote: >> >> >> On 3/14/2024 9:03 PM, Jason Wang wrote: >>> On Fri, Mar 15, 2024 at 5:39 AM Si-Wei Liu <si-wei.liu@oracle.com> wrote: >>>> On setups with one or more virtio-net devices with vhost on, >>>> dirty tracking iteration increases cost the bigger the number >>>> amount of queues are set up e.g. on idle guests migration the >>>> following is observed with virtio-net with vhost=on: >>>> >>>> 48 queues -> 78.11% [.] vhost_dev_sync_region.isra.13 >>>> 8 queues -> 40.50% [.] vhost_dev_sync_region.isra.13 >>>> 1 queue -> 6.89% [.] vhost_dev_sync_region.isra.13 >>>> 2 devices, 1 queue -> 18.60% [.] vhost_dev_sync_region.isra.14 >>>> >>>> With high memory rates the symptom is lack of convergence as soon >>>> as it has a vhost device with a sufficiently high number of queues, >>>> the sufficient number of vhost devices. >>>> >>>> On every migration iteration (every 100msecs) it will redundantly >>>> query the *shared log* the number of queues configured with vhost >>>> that exist in the guest. For the virtqueue data, this is necessary, >>>> but not for the memory sections which are the same. So essentially >>>> we end up scanning the dirty log too often. >>>> >>>> To fix that, select a vhost device responsible for scanning the >>>> log with regards to memory sections dirty tracking. It is selected >>>> when we enable the logger (during migration) and cleared when we >>>> disable the logger. If the vhost logger device goes away for some >>>> reason, the logger will be re-selected from the rest of vhost >>>> devices. >>>> >>>> After making mem-section logger a singleton instance, constant cost >>>> of 7%-9% (like the 1 queue report) will be seen, no matter how many >>>> queues or how many vhost devices are configured: >>>> >>>> 48 queues -> 8.71% [.] vhost_dev_sync_region.isra.13 >>>> 2 devices, 8 queues -> 7.97% [.] vhost_dev_sync_region.isra.14 >>>> >>>> Co-developed-by: Joao Martins <joao.m.martins@oracle.com> >>>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com> >>>> Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com> >>>> >>>> --- >>>> v3 -> v4: >>>> - add comment to clarify effect on cache locality and >>>> performance >>>> >>>> v2 -> v3: >>>> - add after-fix benchmark to commit log >>>> - rename vhost_log_dev_enabled to vhost_dev_should_log >>>> - remove unneeded comparisons for backend_type >>>> - use QLIST array instead of single flat list to store vhost >>>> logger devices >>>> - simplify logger election logic >>>> --- >>>> hw/virtio/vhost.c | 67 ++++++++++++++++++++++++++++++++++++++++++----- >>>> include/hw/virtio/vhost.h | 1 + >>>> 2 files changed, 62 insertions(+), 6 deletions(-) >>>> >>>> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c >>>> index 612f4db..58522f1 100644 >>>> --- a/hw/virtio/vhost.c >>>> +++ b/hw/virtio/vhost.c >>>> @@ -45,6 +45,7 @@ >>>> >>>> static struct vhost_log *vhost_log[VHOST_BACKEND_TYPE_MAX]; >>>> static struct vhost_log *vhost_log_shm[VHOST_BACKEND_TYPE_MAX]; >>>> +static QLIST_HEAD(, vhost_dev) vhost_log_devs[VHOST_BACKEND_TYPE_MAX]; >>>> >>>> /* Memslots used by backends that support private memslots (without an fd). */ >>>> static unsigned int used_memslots; >>>> @@ -149,6 +150,47 @@ bool vhost_dev_has_iommu(struct vhost_dev *dev) >>>> } >>>> } >>>> >>>> +static inline bool vhost_dev_should_log(struct vhost_dev *dev) >>>> +{ >>>> + assert(dev->vhost_ops); >>>> + assert(dev->vhost_ops->backend_type > VHOST_BACKEND_TYPE_NONE); >>>> + assert(dev->vhost_ops->backend_type < VHOST_BACKEND_TYPE_MAX); >>>> + >>>> + return dev == QLIST_FIRST(&vhost_log_devs[dev->vhost_ops->backend_type]); >>> A dumb question, why not simple check >>> >>> dev->log == vhost_log_shm[dev->vhost_ops->backend_type] >> Because we are not sure if the logger comes from vhost_log_shm[] or >> vhost_log[]. Don't want to complicate the check here by calling into >> vhost_dev_log_is_shared() everytime when the .log_sync() is called. > It has very low overhead, isn't it? Whether this has low overhead will have to depend on the specific backend's implementation for .vhost_requires_shm_log(), which the common vhost layer should not assume upon or rely on the current implementation. > > static bool vhost_dev_log_is_shared(struct vhost_dev *dev) > { > return dev->vhost_ops->vhost_requires_shm_log && > dev->vhost_ops->vhost_requires_shm_log(dev); > } > > And it helps to simplify the logic. Generally yes, but when it comes to hot path operations the performance consideration could override this principle. I think there's no harm to check against logger device cached in vhost layer itself, and the current patch does not create a lot of complexity or performance side effect (actually I think the conditional should be very straightforward to turn into just a couple of assembly compare and branch instructions rather than indirection through another jmp call). -Siwei > > Thanks > >> -Siwei >>> ? >>> >>> Thanks >>>
On 3/17/2024 8:20 PM, Jason Wang wrote: > On Sat, Mar 16, 2024 at 2:33 AM Si-Wei Liu <si-wei.liu@oracle.com> wrote: >> >> >> On 3/14/2024 8:50 PM, Jason Wang wrote: >>> On Fri, Mar 15, 2024 at 5:39 AM Si-Wei Liu <si-wei.liu@oracle.com> wrote: >>>> There could be a mix of both vhost-user and vhost-kernel clients >>>> in the same QEMU process, where separate vhost loggers for the >>>> specific vhost type have to be used. Make the vhost logger per >>>> backend type, and have them properly reference counted. >>> It's better to describe what's the advantage of doing this. >> Yes, I can add that to the log. Although it's a niche use case, it was >> actually a long standing limitation / bug that vhost-user and >> vhost-kernel loggers can't co-exist per QEMU process, but today it's >> just silent failure that may be ended up with. This bug fix removes that >> implicit limitation in the code. > Ok. > >>>> Suggested-by: Michael S. Tsirkin <mst@redhat.com> >>>> Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com> >>>> >>>> --- >>>> v3->v4: >>>> - remove checking NULL return value from vhost_log_get >>>> >>>> v2->v3: >>>> - remove non-effective assertion that never be reached >>>> - do not return NULL from vhost_log_get() >>>> - add neccessary assertions to vhost_log_get() >>>> --- >>>> hw/virtio/vhost.c | 45 +++++++++++++++++++++++++++++++++------------ >>>> 1 file changed, 33 insertions(+), 12 deletions(-) >>>> >>>> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c >>>> index 2c9ac79..612f4db 100644 >>>> --- a/hw/virtio/vhost.c >>>> +++ b/hw/virtio/vhost.c >>>> @@ -43,8 +43,8 @@ >>>> do { } while (0) >>>> #endif >>>> >>>> -static struct vhost_log *vhost_log; >>>> -static struct vhost_log *vhost_log_shm; >>>> +static struct vhost_log *vhost_log[VHOST_BACKEND_TYPE_MAX]; >>>> +static struct vhost_log *vhost_log_shm[VHOST_BACKEND_TYPE_MAX]; >>>> >>>> /* Memslots used by backends that support private memslots (without an fd). */ >>>> static unsigned int used_memslots; >>>> @@ -287,6 +287,10 @@ static int vhost_set_backend_type(struct vhost_dev *dev, >>>> r = -1; >>>> } >>>> >>>> + if (r == 0) { >>>> + assert(dev->vhost_ops->backend_type == backend_type); >>>> + } >>>> + >>> Under which condition could we hit this? >> Just in case some other function inadvertently corrupted this earlier, >> we have to capture discrepancy in the first place... On the other hand, >> it will be helpful for other vhost backend writers to diagnose day-one >> bug in the code. I feel just code comment here will not be >> sufficient/helpful. > See below. > >>> It seems not good to assert a local logic. >> It seems to me quite a few local asserts are in the same file already, >> vhost_save_backend_state, > For example it has assert for > > assert(!dev->started); > > which is not the logic of the function itself but require > vhost_dev_start() not to be called before. > > But it looks like this patch you assert the code just a few lines > above the assert itself? Yes, that was the intent - for e.g. xxx_ops may contain corrupted xxx_ops.backend_type already before coming to this vhost_set_backend_type() function. And we may capture this corrupted state by asserting the expected xxx_ops.backend_type (to be consistent with the backend_type passed in), which needs be done in the first place when this discrepancy is detected. In practice I think there should be no harm to add this assert, but this will add warranted guarantee to the current code. Regards, -Siwei > > dev->vhost_ops = &xxx_ops; > > ... > > assert(dev->vhost_ops->backend_type == backend_type) > > ? > > Thanks > >> vhost_load_backend_state, >> vhost_virtqueue_mask, vhost_config_mask, just to name a few. Why local >> assert a problem? >> >> Thanks, >> -Siwei >> >>> Thanks >>>
On 18.03.24 21:27, Ilya Leoshkevich wrote:
> From: Ido Plat <ido.plat@ibm.com>
>
> Otherwise TCG would assume the register that holds t1 would be constant
> and reuse whenever it needs the value within it.
>
> Cc: qemu-stable@nongnu.org
> Fixes: f1ea739bd598 ("target/s390x: Use tcg_constant_* in local contexts")
> Reviewed-by: Ilya Leoshkevich <iii@linux.ibm.com>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> [iii: Adjust a newline and capitalization, add tags]
> Signed-off-by: Ido Plat <ido.plat@ibm.com>
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
Reviewed-by: David Hildenbrand <david@redhat.com>
--
Cheers,
David / dhildenb
cgs->ready can be false if the accelerator does not look at current_machine->cgs altogether. Assume that the lack of initialization is due to this, and report a nicer error instead of an assertion failure: $ qemu-system-x86_64 -object sev-guest,id=sev0,policy=0x5,id=sev0,cbitpos=51,reduced-phys-bits=1 -M confidential-guest-support=sev0 qemu-system-x86_64: ../softmmu/vl.c:2619: qemu_machine_creation_done: Assertion `machine->cgs->ready' failed. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- system/vl.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/system/vl.c b/system/vl.c index 0c970cf0203..c6442229824 100644 --- a/system/vl.c +++ b/system/vl.c @@ -2676,11 +2676,10 @@ static bool qemu_machine_creation_done(Error **errp) qdev_machine_creation_done(); - if (machine->cgs) { - /* - * Verify that Confidential Guest Support has actually been initialized - */ - assert(machine->cgs->ready); + if (machine->cgs && !machine->cgs->ready) { + error_setg(errp, "accelerator does not support confidential guest %s", + object_get_typename(OBJECT(machine->cgs))); + exit(1); } if (foreach_device_config(DEV_GDB, gdbserver_start) < 0) { -- 2.44.0