[PATCH V6] target/loongarch: Fix tlb huge page loading issue
When we use qemu tcg simulation, the page size of bios is 4KB. When using the level 2 super huge page (page size is 1G) to create the page table, it is found that the content of the corresponding address space is abnormal, resulting in the bios can not start the operating system and graphical interface normally. The lddir and ldpte instruction emulation has a problem with the use of super huge page processing above level 2. The page size is not correctly calculated, resulting in the wrong page size of the table entry found by tlb. Signed-off-by: Xianglai Li --- target/loongarch/cpu-csr.h| 3 + target/loongarch/internals.h | 5 -- target/loongarch/tcg/tlb_helper.c | 113 +- 3 files changed, 82 insertions(+), 39 deletions(-) Changes log: V5->V6: Add some necessary log printing to patch. V4->V5: Modifying the patch Title. Fix incorrect usage of FIELD macro and code logic errors in patch. V3->V4: Optimize the huge page calculation method, use the FIELD macro for bit calculation. V2->V3: Delete the intermediate variable LDDIR_PS, and implement lddir and ldpte huge pages by referring to the latest architecture reference manual. V1->V2: Modified the patch title format and Enrich the commit mesg description diff --git a/target/loongarch/cpu-csr.h b/target/loongarch/cpu-csr.h index c59d7a9fcb..0834e91f30 100644 --- a/target/loongarch/cpu-csr.h +++ b/target/loongarch/cpu-csr.h @@ -67,6 +67,9 @@ FIELD(TLBENTRY, D, 1, 1) FIELD(TLBENTRY, PLV, 2, 2) FIELD(TLBENTRY, MAT, 4, 2) FIELD(TLBENTRY, G, 6, 1) +FIELD(TLBENTRY, HUGE, 6, 1) +FIELD(TLBENTRY, HGLOBAL, 12, 1) +FIELD(TLBENTRY, LEVEL, 13, 2) FIELD(TLBENTRY_32, PPN, 8, 24) FIELD(TLBENTRY_64, PPN, 12, 36) FIELD(TLBENTRY_64, NR, 61, 1) diff --git a/target/loongarch/internals.h b/target/loongarch/internals.h index a2fc54c8a7..944153b180 100644 --- a/target/loongarch/internals.h +++ b/target/loongarch/internals.h @@ -16,11 +16,6 @@ #define TARGET_PHYS_MASK MAKE_64BIT_MASK(0, TARGET_PHYS_ADDR_SPACE_BITS) #define TARGET_VIRT_MASK MAKE_64BIT_MASK(0, TARGET_VIRT_ADDR_SPACE_BITS) -/* Global bit used for lddir/ldpte */ -#define LOONGARCH_PAGE_HUGE_SHIFT 6 -/* Global bit for huge page */ -#define LOONGARCH_HGLOBAL_SHIFT 12 - void loongarch_translate_init(void); void loongarch_cpu_dump_state(CPUState *cpu, FILE *f, int flags); diff --git a/target/loongarch/tcg/tlb_helper.c b/target/loongarch/tcg/tlb_helper.c index 22be031ac7..57f5308632 100644 --- a/target/loongarch/tcg/tlb_helper.c +++ b/target/loongarch/tcg/tlb_helper.c @@ -17,6 +17,34 @@ #include "exec/log.h" #include "cpu-csr.h" +static void get_dir_base_width(CPULoongArchState *env, uint64_t *dir_base, + uint64_t *dir_width, target_ulong level) +{ +switch (level) { +case 1: +*dir_base = FIELD_EX64(env->CSR_PWCL, CSR_PWCL, DIR1_BASE); +*dir_width = FIELD_EX64(env->CSR_PWCL, CSR_PWCL, DIR1_WIDTH); +break; +case 2: +*dir_base = FIELD_EX64(env->CSR_PWCL, CSR_PWCL, DIR2_BASE); +*dir_width = FIELD_EX64(env->CSR_PWCL, CSR_PWCL, DIR2_WIDTH); +break; +case 3: +*dir_base = FIELD_EX64(env->CSR_PWCH, CSR_PWCH, DIR3_BASE); +*dir_width = FIELD_EX64(env->CSR_PWCH, CSR_PWCH, DIR3_WIDTH); +break; +case 4: +*dir_base = FIELD_EX64(env->CSR_PWCH, CSR_PWCH, DIR4_BASE); +*dir_width = FIELD_EX64(env->CSR_PWCH, CSR_PWCH, DIR4_WIDTH); +break; +default: +/* level may be zero for ldpte */ +*dir_base = FIELD_EX64(env->CSR_PWCL, CSR_PWCL, PTBASE); +*dir_width = FIELD_EX64(env->CSR_PWCL, CSR_PWCL, PTWIDTH); +break; +} +} + static void raise_mmu_exception(CPULoongArchState *env, target_ulong address, MMUAccessType access_type, int tlb_error) { @@ -485,7 +513,25 @@ target_ulong helper_lddir(CPULoongArchState *env, target_ulong base, target_ulong badvaddr, index, phys, ret; int shift; uint64_t dir_base, dir_width; -bool huge = (base >> LOONGARCH_PAGE_HUGE_SHIFT) & 0x1; + +if (unlikely((level == 0) || (level > 4))) { +qemu_log_mask(LOG_GUEST_ERROR, + "Attepted LDDIR with level %"PRId64"\n", level); +return base; +} + +if (FIELD_EX64(base, TLBENTRY, HUGE)) { +if (unlikely(level == 4)) { +qemu_log_mask(LOG_GUEST_ERROR, + "Attempted use of level 4 huge page\n"); +} + +if (FIELD_EX64(base, TLBENTRY, LEVEL)) { +return base; +} else { +return FIELD_DP64(base, TLBENTRY, LEVEL, level); +} +} badvaddr = env->CSR_TLBRBADV; base = base & TARGET_PHYS_MASK; @@ -494,30 +540,7 @@ target_ulong helper_lddir(CPULoongArchState *env, target_ulong base, shift = FIELD_EX64(env->CSR_PWCL, CSR_PWCL, PTEWIDTH); shift = (shift + 1) * 3; -if (huge) { -return base; -} -switch
Re: Regression in v7.2.10 - ui-dbus.so requires -fPIC
Hi On Sun, Mar 17, 2024 at 11:10 AM Michael Tokarev wrote: > > 17.03.2024 01:19, Olaf Hering: > > Sat, 16 Mar 2024 22:40:14 +0300 Michael Tokarev : > > > >> meson: ensure dbus-display generated code is built before other units > >> (cherry picked from commit 1222070e772833c6875e0ca63565db12c22df39e) > > > > "static_library" is used often. Some use the 'pic' option, which fixes the > > issue. > > > > I think every usage that ends up in a shared library requires 'pic:true'. > > The prob here seems to be that while fixing other issue (header file isn't > generated > early enough), we all forgot about the fact dbus-display1.o can be used > inside a > shared/loadable object when qemu is built with --enable-modules. > dbus-display1 is also used with static linking for the unit test. It looks like the simplest is to let the actual target decide how it is built, even if it is compiled multiple time then: - dbus_display1_lib = static_library('dbus-display1', dbus_display1, dependencies: gio) - dbus_display1_dep = declare_dependency(link_with: dbus_display1_lib, sources: dbus_display1[0]) + dbus_display1_dep = declare_dependency(sources: dbus_display1, dependencies: gio) This makes commit 186acfbaf7 ("tests/qtest: Depend on dbus_display1_dep") no longer relevant, as dbus-display1.c will be recompiled. Alternatively, we could always build with pic: true (or pic: enable_modules), but that's not ideal either. -- Marc-André Lureau
Re: [QEMU PATCH v6 1/1] virtio-pci: implement No_Soft_Reset bit
Hi Michael S. Tsirkin, Do you have any comments on this patch? On 2024/2/22 11:40, Jiqian Chen wrote: > In current code, when guest does S3, virtio devices are reset due to > the bit No_Soft_Reset is not set. After resetting, the display resources > of virtio-gpu are destroyed, then the display can't come back and only > show blank after resuming. > > Implement No_Soft_Reset bit of PCI_PM_CTRL register, then guest can check > this bit, if this bit is set, the devices resetting will not be done, and > then the display can work after resuming. > > Signed-off-by: Jiqian Chen > --- > hw/virtio/virtio-pci.c | 37 +- > include/hw/virtio/virtio-pci.h | 5 + > 2 files changed, 41 insertions(+), 1 deletion(-) > > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c > index 1a7039fb0c68..da5312010345 100644 > --- a/hw/virtio/virtio-pci.c > +++ b/hw/virtio/virtio-pci.c > @@ -2197,6 +2197,11 @@ static void virtio_pci_realize(PCIDevice *pci_dev, > Error **errp) > pcie_cap_lnkctl_init(pci_dev); > } > > +if (proxy->flags & VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET) { > +pci_set_word(pci_dev->config + pos + PCI_PM_CTRL, > + PCI_PM_CTRL_NO_SOFT_RESET); > +} > + > if (proxy->flags & VIRTIO_PCI_FLAG_INIT_PM) { > /* Init Power Management Control Register */ > pci_set_word(pci_dev->wmask + pos + PCI_PM_CTRL, > @@ -2259,18 +2264,46 @@ static void virtio_pci_reset(DeviceState *qdev) > } > } > > +static bool device_no_need_reset(PCIDevice *dev) > +{ > +if (pci_is_express(dev)) { > +uint16_t pmcsr; > + > +pmcsr = pci_get_word(dev->config + dev->exp.pm_cap + PCI_PM_CTRL); > +/* > + * When No_Soft_Reset bit is set and device > + * is in D3hot state, can't reset device > + */ > +if ((pmcsr & PCI_PM_CTRL_NO_SOFT_RESET) && > +(pmcsr & PCI_PM_CTRL_STATE_MASK) == 3) > +return true; > +} > + > +return false; > +} > + > static void virtio_pci_bus_reset_hold(Object *obj) > { > PCIDevice *dev = PCI_DEVICE(obj); > DeviceState *qdev = DEVICE(obj); > > +if (device_no_need_reset(dev)) > +return; > + > virtio_pci_reset(qdev); > > if (pci_is_express(dev)) { > +uint16_t pmcsr; > +uint16_t val = 0; > + > pcie_cap_deverr_reset(dev); > pcie_cap_lnkctl_reset(dev); > > -pci_set_word(dev->config + dev->exp.pm_cap + PCI_PM_CTRL, 0); > +/* don't reset the RO bits */ > +pmcsr = pci_get_word(dev->config + dev->exp.pm_cap + PCI_PM_CTRL); > +val = val | (pmcsr & PCI_PM_CTRL_NO_SOFT_RESET) | > +(pmcsr & PCI_PM_CTRL_DATA_SCALE_MASK); > +pci_set_word(dev->config + dev->exp.pm_cap + PCI_PM_CTRL, val); > } > } > > @@ -2297,6 +2330,8 @@ static Property virtio_pci_properties[] = { > VIRTIO_PCI_FLAG_INIT_LNKCTL_BIT, true), > DEFINE_PROP_BIT("x-pcie-pm-init", VirtIOPCIProxy, flags, > VIRTIO_PCI_FLAG_INIT_PM_BIT, true), > +DEFINE_PROP_BIT("x-pcie-pm-no-soft-reset", VirtIOPCIProxy, flags, > +VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET_BIT, true), > DEFINE_PROP_BIT("x-pcie-flr-init", VirtIOPCIProxy, flags, > VIRTIO_PCI_FLAG_INIT_FLR_BIT, true), > DEFINE_PROP_BIT("aer", VirtIOPCIProxy, flags, > diff --git a/include/hw/virtio/virtio-pci.h b/include/hw/virtio/virtio-pci.h > index 59d88018c16a..9e67ba38c748 100644 > --- a/include/hw/virtio/virtio-pci.h > +++ b/include/hw/virtio/virtio-pci.h > @@ -43,6 +43,7 @@ enum { > VIRTIO_PCI_FLAG_INIT_FLR_BIT, > VIRTIO_PCI_FLAG_AER_BIT, > VIRTIO_PCI_FLAG_ATS_PAGE_ALIGNED_BIT, > +VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET_BIT, > }; > > /* Need to activate work-arounds for buggy guests at vmstate load. */ > @@ -79,6 +80,10 @@ enum { > /* Init Power Management */ > #define VIRTIO_PCI_FLAG_INIT_PM (1 << VIRTIO_PCI_FLAG_INIT_PM_BIT) > > +/* Init The No_Soft_Reset bit of Power Management */ > +#define VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET \ > + (1 << VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET_BIT) > + > /* Init Function Level Reset capability */ > #define VIRTIO_PCI_FLAG_INIT_FLR (1 << VIRTIO_PCI_FLAG_INIT_FLR_BIT) > -- Best regards, Jiqian Chen.
[PATCH 3/3] ui/cocoa: Use NSTrackingInVisibleRect
I observed [NSTrackingArea rect] becomes de-synchronized with the view frame with some unknown condition. Specify NSTrackingInVisibleRect option to let Cocoa automatically update NSTrackingArea, which also saves code for synchronization. Fixes: 91aa508d0274 ("ui/cocoa: Let the platform toggle fullscreen") Signed-off-by: Akihiko Odaki --- ui/cocoa.m | 48 ++-- 1 file changed, 14 insertions(+), 34 deletions(-) diff --git a/ui/cocoa.m b/ui/cocoa.m index 1324be6d32fe..1e2b7d931905 100644 --- a/ui/cocoa.m +++ b/ui/cocoa.m @@ -306,7 +306,6 @@ static void handleAnyDeviceErrors(Error * err) */ @interface QemuCocoaView : NSView { -NSTrackingArea *trackingArea; QEMUScreen screen; pixman_image_t *pixman_image; QKbdState *kbd; @@ -359,6 +358,19 @@ - (id)initWithFrame:(NSRect)frameRect self = [super initWithFrame:frameRect]; if (self) { +NSTrackingAreaOptions options = NSTrackingActiveInKeyWindow | +NSTrackingMouseEnteredAndExited | +NSTrackingMouseMoved | +NSTrackingInVisibleRect; + +NSTrackingArea *trackingArea = +[[NSTrackingArea alloc] initWithRect:CGRectZero + options:options + owner:self +userInfo:nil]; + +[self addTrackingArea:trackingArea]; +[trackingArea release]; screen.width = frameRect.size.width; screen.height = frameRect.size.height; kbd = qkbd_state_init(dcl.con); @@ -392,41 +404,9 @@ - (BOOL) isOpaque return YES; } -- (void) removeTrackingRect -{ -if (trackingArea) { -[self removeTrackingArea:trackingArea]; -[trackingArea release]; -trackingArea = nil; -} -} - -- (void) frameUpdated -{ -[self removeTrackingRect]; - -if ([self window]) { -NSTrackingAreaOptions options = NSTrackingActiveInKeyWindow | -NSTrackingMouseEnteredAndExited | -NSTrackingMouseMoved; -trackingArea = [[NSTrackingArea alloc] initWithRect:[self frame] -options:options - owner:self - userInfo:nil]; -[self addTrackingArea:trackingArea]; -[self updateUIInfo]; -} -} - - (void) viewDidMoveToWindow { [self resizeWindow]; -[self frameUpdated]; -} - -- (void) viewWillMoveToWindow:(NSWindow *)newWindow -{ -[self removeTrackingRect]; } - (void) hideCursor @@ -1284,7 +1264,7 @@ - (void)windowDidExitFullScreen:(NSNotification *)notification - (void)windowDidResize:(NSNotification *)notification { [cocoaView updateBounds]; -[cocoaView frameUpdated]; +[cocoaView updateUIInfo]; } /* Called when the user clicks on a window's close button */ -- 2.44.0
[PATCH 0/3] Fixes for "ui/cocoa: Let the platform toggle fullscreen"
This series contains patches for regressions caused by commit 91aa508d0274 ("ui/cocoa: Let the platform toggle fullscreen"). Signed-off-by: Akihiko Odaki --- Akihiko Odaki (3): ui/cocoa: Fix aspect ratio ui/cocoa: Resize window after toggling zoom-to-fit ui/cocoa: Use NSTrackingInVisibleRect ui/cocoa.m | 72 -- 1 file changed, 37 insertions(+), 35 deletions(-) --- base-commit: ba49d760eb04630e7b15f423ebecf6c871b8f77b change-id: 20240318-fixes-7b187ec236a0 Best regards, -- Akihiko Odaki
Re: [PATCH 03/14] hw/core/machine-smp: Simplify variables' initialization in machine_parse_smp_config()
On Wed, Mar 13, 2024 at 04:22:30PM +0530, Prasad Pandit wrote: > Date: Wed, 13 Mar 2024 16:22:30 +0530 > From: Prasad Pandit > Subject: Re: [PATCH 03/14] hw/core/machine-smp: Simplify variables' > initialization in machine_parse_smp_config() > > Hello Zhao, > > > (Communicating with you also helped me to understand the QAPI related > > parts.) > > * I'm also visiting QAPI code parts for the first time. Thanks to you. :) > > On Mon, 11 Mar 2024 at 10:36, Zhao Liu wrote: > > SMPConfiguration is created and set in machine_set_smp(). > > Firstly, it is created by g_autoptr(), and then it is initialized by > > visit_type_SMPConfiguration(). > > > > The visit_type_SMPConfiguration() is generated by > > gen_visit_object_members() in scripts/qapi/visit.py. > > > > IIUC, in visit_type_SMPConfiguration() (let's have a look at > > gen_visit_object_members()), there's no explicit initialization of > > SMPConfiguration() (i.e., the parameter named "%(c_name)s *obj" in > > gen_visit_object_members()). For int type, has_* means that the field is > > set. > > > > * Thank you for the above details, appreciate it. I added few > fprintf() calls to visit_type_SMPConfiguration() to see what user > values it receives > === > $ ./qemu-system-x86_64 -smp cores=2,maxcpus=2,cpus=1,sockets=2 > visit_type_SMPConfiguration: name: smp > has_cpus: 1:1 > has_drawvers: 0:0 > has_books: 0:0 > has_sockets: 1:2 > has_dies: 0:0 > has_clusters: 0:0 > has_cores: 1:2 > has_threads: 0:0 > has_maxcpus: 1:2 > qemu-system-x86_64: Invalid CPU topology: product of the hierarchy > must match maxcpus: sockets (2) * dies (1) * cores (2) * threads (0) > != maxcpus (2) > === > * As seen above, undefined -smp fields (both has_ and ) are > set to zero(0). > > === > main > qemu_init > qemu_apply_machine_options >object_set_properties_from_keyval > object_set_properties_from_qdict > object_property_set > machine_set_smp >visit_type_SMPConfiguration > visit_start_struct > (gdb) p v->start_struct > $4 = ... 0x570248e4 > (gdb) > (gdb) > qobject_input_start_struct >if (obj) { > *obj = g_malloc0(size); > } > === > * Stepping through function calls in gdb(1) revealed above call > sequence which leads to 'SMPConfiguration **' objects allocation by > g_malloc0. Thanks! I misunderstood, it turns out that the initialization is done here. > > > This means if user doesn't initialize some field, the the value should > > be considerred as unreliable. And I guess for int, the default > > initialization value is the same as if we had declared a regular integer > > variable. But anyway, fields that are not explicitly initialized should > > not be accessed. > > * g_malloc0() allocating 'SMPConfiguration **' object above assures us > that undefined -smp fields shall always be zero(0). > > * 'obj->has_' field is set only if the user has supplied the > variable value, otherwise it remains set to zero(0). >visit_type_SMPConfiguration_members > ->visit_optional >->v->optional > -> qobject_input_optional Yes, you're right! > > * Overall, I think there is scope to simplify the > 'machine_parse_smp_config()' function by using SMPConfiguration > field(s) ones. Indeed, as you say, these items are initialized to 0 by default. I think, however, that the initialization is so far away from where the smp is currently parsed that one can't easily confirm it (thanks for your confirmation!). >From a code readability view, the fact that we're explicitly initializing to 0 again here brings little overhead, but makes the code more readable as well as easier to maintain. I think the small redundancy here is worth it. Also, in other use cases people always relies on fields marked by has_*, and there is no (or less?) precedent for direct access to places where has_* is not set. I understand that this is also a habit, i.e., fields with a has_* of False by default are unreliable and avoid going directly to them. Regards, Zhao
[PATCH 1/3] ui/cocoa: Fix aspect ratio
[NSWindow setContentAspectRatio:] does not trigger window resize itself, so the wrong aspect ratio will persist if nothing resizes the window. Call [NSWindow setContentSize:] in such a case. Fixes: 91aa508d0274 ("ui/cocoa: Let the platform toggle fullscreen") Signed-off-by: Akihiko Odaki --- ui/cocoa.m | 23 ++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/ui/cocoa.m b/ui/cocoa.m index fa879d7dcd4b..d6a5b462f78b 100644 --- a/ui/cocoa.m +++ b/ui/cocoa.m @@ -508,6 +508,25 @@ - (void) drawRect:(NSRect) rect } } +- (NSSize)fixAspectRatio:(NSSize)original +{ +NSSize scaled; +NSSize fixed; + +scaled.width = screen.width * original.height; +scaled.height = screen.height * original.width; + +if (scaled.width < scaled.height) { +fixed.width = scaled.width / screen.height; +fixed.height = original.height; +} else { +fixed.width = original.width; +fixed.height = scaled.height / screen.width; +} + +return fixed; +} + - (NSSize) screenSafeAreaSize { NSSize size = [[[self window] screen] frame].size; @@ -525,8 +544,10 @@ - (void) resizeWindow [[self window] setContentSize:NSMakeSize(screen.width, screen.height)]; [[self window] center]; } else if ([[self window] styleMask] & NSWindowStyleMaskFullScreen) { -[[self window] setContentSize:[self screenSafeAreaSize]]; +[[self window] setContentSize:[self fixAspectRatio:[self screenSafeAreaSize]]]; [[self window] center]; +} else { +[[self window] setContentSize:[self fixAspectRatio:[self frame].size]]; } } -- 2.44.0
[PATCH 2/3] ui/cocoa: Resize window after toggling zoom-to-fit
Resize the window so that the content will fit without zooming. Fixes: 91aa508d0274 ("ui/cocoa: Let the platform toggle fullscreen") Signed-off-by: Akihiko Odaki --- ui/cocoa.m | 1 + 1 file changed, 1 insertion(+) diff --git a/ui/cocoa.m b/ui/cocoa.m index d6a5b462f78b..1324be6d32fe 100644 --- a/ui/cocoa.m +++ b/ui/cocoa.m @@ -1378,6 +1378,7 @@ - (void)zoomToFit:(id) sender [[cocoaView window] setStyleMask:styleMask]; [sender setState:styleMask & NSWindowStyleMaskResizable ? NSControlStateValueOn : NSControlStateValueOff]; +[cocoaView resizeWindow]; } - (void)toggleZoomInterpolation:(id) sender -- 2.44.0
[PATCH 4/4] ui/curses: Do not use console_select()
ui/curses is the only user of console_select(). Move the implementation to ui/curses. Signed-off-by: Akihiko Odaki --- 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 a703f7466499..3769179e3b2b 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 6bf02a23414c..d77f509046af 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_c
[PATCH 3/4] ui/cocoa: Do not use console_select()
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 --- ui/cocoa.m | 37 ++--- 1 file changed, 26 insertions(+), 11 deletions(-) diff --git a/ui/cocoa.m b/ui/cocoa.m index fa879d7dcd4b..47280c0a93be 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_first_graphic_console(); +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
[PATCH 2/4] ui/vnc: Do not use console_select()
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 --- 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..a703f7466499 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_first_graphic_console(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..6bf02a23414c 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_first_graphic_console(void) +{ +QemuConsole *con; + +QTAILQ_FOREACH(con, &consoles, next) { +if (QEMU_IS_GRAPHIC_CONSOLE(con)) { +return con; +} +} +return NULL; +} + 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..94564b196ba8 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_first_graphic_console(); } if (con != vd->dcl.con) { -- 2.44.0
[PATCH 0/4] ui/console: Remove console_select()
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 (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
[PATCH 1/4] ui/vc: Do not inherit the size of active console
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 --- 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
[PATCH 1/2] ui/cocoa: Add cursor composition
Add accelerated cursor composition to ui/cocoa. This does not only improve performance for display devices that exposes the capability to the guest according to dpy_cursor_define_supported(), but fixes the cursor display for devices that unconditionally expects the availability of the capability (e.g., virtio-gpu). The common pattern to implement accelerated cursor composition is to replace the cursor and warp it so that the replaced cursor is shown at the correct position on the guest display. Unfortunately, ui/cocoa cannot do the same because warping the cursor position interfers with the mouse input so it uses CALayer instead; although it is not specialized for cursor composition, it still can compose images with hardware acceleration. Signed-off-by: Akihiko Odaki --- meson.build | 3 +- ui/cocoa.m | 97 + 2 files changed, 99 insertions(+), 1 deletion(-) diff --git a/meson.build b/meson.build index b375248a7614..b198ca2972ed 100644 --- a/meson.build +++ b/meson.build @@ -1044,7 +1044,8 @@ if get_option('attr').allowed() endif endif -cocoa = dependency('appleframeworks', modules: ['Cocoa', 'CoreVideo'], +cocoa = dependency('appleframeworks', + modules: ['Cocoa', 'CoreVideo', 'QuartzCore'], required: get_option('cocoa')) vmnet = dependency('appleframeworks', modules: 'vmnet', required: get_option('vmnet')) diff --git a/ui/cocoa.m b/ui/cocoa.m index fa879d7dcd4b..c13c2a01e947 100644 --- a/ui/cocoa.m +++ b/ui/cocoa.m @@ -25,6 +25,7 @@ #include "qemu/osdep.h" #import +#import #include #include "qemu/help-texts.h" @@ -92,12 +93,16 @@ static void cocoa_switch(DisplayChangeListener *dcl, DisplaySurface *surface); static void cocoa_refresh(DisplayChangeListener *dcl); +static void cocoa_mouse_set(DisplayChangeListener *dcl, int x, int y, int on); +static void cocoa_cursor_define(DisplayChangeListener *dcl, QEMUCursor *cursor); static const DisplayChangeListenerOps dcl_ops = { .dpy_name = "cocoa", .dpy_gfx_update = cocoa_update, .dpy_gfx_switch = cocoa_switch, .dpy_refresh = cocoa_refresh, +.dpy_mouse_set = cocoa_mouse_set, +.dpy_cursor_define = cocoa_cursor_define, }; static DisplayChangeListener dcl = { .ops = &dcl_ops, @@ -313,6 +318,11 @@ @interface QemuCocoaView : NSView BOOL isMouseGrabbed; BOOL isAbsoluteEnabled; CFMachPortRef eventsTap; +CALayer *cursorLayer; +QEMUCursor *cursor; +int mouseX; +int mouseY; +int mouseOn; } - (void) switchSurface:(pixman_image_t *)image; - (void) grabMouse; @@ -365,6 +375,12 @@ - (id)initWithFrame:(NSRect)frameRect #if MAC_OS_X_VERSION_MAX_ALLOWED >= MAC_OS_VERSION_14_0 [self setClipsToBounds:YES]; #endif +[self setWantsLayer:YES]; +cursorLayer = [[CALayer alloc] init]; +[cursorLayer setAnchorPoint:CGPointMake(0, 1)]; +[cursorLayer setAutoresizingMask:kCALayerMaxXMargin | + kCALayerMinYMargin]; +[[self layer] addSublayer:cursorLayer]; } return self; @@ -445,6 +461,72 @@ - (void) unhideCursor [NSCursor unhide]; } +- (void)setMouseX:(int)x y:(int)y on:(int)on +{ +CGPoint position; + +mouseX = x; +mouseY = y; +mouseOn = on; + +position.x = mouseX; +position.y = screen.height - mouseY; + +[CATransaction begin]; +[CATransaction setDisableActions:YES]; +[cursorLayer setPosition:position]; +[cursorLayer setHidden:!mouseOn]; +[CATransaction commit]; +} + +- (void)setCursor:(QEMUCursor *)given_cursor +{ +CGDataProviderRef provider; +CGImageRef image; +CGRect bounds = CGRectZero; + +cursor_unref(cursor); +cursor = given_cursor; + +if (!cursor) { +return; +} + +cursor_ref(cursor); + +bounds.size.width = cursor->width; +bounds.size.height = cursor->height; + +provider = CGDataProviderCreateWithData( +NULL, +cursor->data, +cursor->width * cursor->height * 4, +NULL +); + +image = CGImageCreate( +cursor->width, //width +cursor->height, //height +8, //bitsPerComponent +32, //bitsPerPixel +cursor->width * 4, //bytesPerRow +CGColorSpaceCreateWithName(kCGColorSpaceSRGB), //colorspace +kCGBitmapByteOrder32Little | kCGImageAlphaFirst, //bitmapInfo +provider, //provider +NULL, //decode +0, //interpolate +kCGRenderingIntentDefault //intent +); + +CGDataProviderRelease(provider); +[CATransaction begin]; +[CATransaction setDisableActions:YES]; +[cursorLayer setBounds:bounds]; +[cursorLayer setContents:(id)image]; +[CATransaction commit]; +CGImageRelease(image); +} + - (void) drawRect:(NSRect) rect { COCOA_DEBUG("QemuCocoaView: drawRect\n"); @@ -1982,6 +2064,21 @@ static void cocoa_refresh(Display
[PATCH 2/2] ui/console: Remove dpy_cursor_define_supported()
Remove dpy_cursor_define_supported() as it brings no benefit today and it has a few inherent problems. All graphical displays except egl-headless support cursor composition without DMA-BUF, and egl-headless is meant to be used in conjunction with another graphical display, so dpy_cursor_define_supported() always returns true and meaningless. Even if we add a new display without cursor composition in the future, dpy_cursor_define_supported() will be problematic as a cursor display fix for it because some display devices like virtio-gpu cannot tell the lack of cursor composition capability to the guest and are unable to utilize the value the function returns. Therefore, all non-headless graphical displays must actually implement cursor composition for correct cursor display. Another problem with dpy_cursor_define_supported() is that it returns true even if only some of the display listeners support cursor composition, which is wrong unless all display listeners that lack cursor composition is headless. Signed-off-by: Akihiko Odaki --- include/ui/console.h| 1 - hw/display/qxl-render.c | 4 hw/display/vmware_vga.c | 6 ++ ui/console.c| 13 - 4 files changed, 2 insertions(+), 22 deletions(-) diff --git a/include/ui/console.h b/include/ui/console.h index a4a49ffc640c..7566282be986 100644 --- a/include/ui/console.h +++ b/include/ui/console.h @@ -342,7 +342,6 @@ void dpy_text_update(QemuConsole *con, int x, int y, int w, int h); void dpy_text_resize(QemuConsole *con, int w, int h); void dpy_mouse_set(QemuConsole *con, int x, int y, int on); void dpy_cursor_define(QemuConsole *con, QEMUCursor *cursor); -bool dpy_cursor_define_supported(QemuConsole *con); bool dpy_gfx_check_format(QemuConsole *con, pixman_format_code_t format); diff --git a/hw/display/qxl-render.c b/hw/display/qxl-render.c index ec99ec887a6e..837d2446cd52 100644 --- a/hw/display/qxl-render.c +++ b/hw/display/qxl-render.c @@ -307,10 +307,6 @@ int qxl_render_cursor(PCIQXLDevice *qxl, QXLCommandExt *ext) return 1; } -if (!dpy_cursor_define_supported(qxl->vga.con)) { -return 0; -} - if (qxl->debug > 1 && cmd->type != QXL_CURSOR_MOVE) { fprintf(stderr, "%s", __func__); qxl_log_cmd_cursor(qxl, cmd, ext->group_id); diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c index 1c0f9d9a991d..fadfefc4c67c 100644 --- a/hw/display/vmware_vga.c +++ b/hw/display/vmware_vga.c @@ -904,10 +904,8 @@ static uint32_t vmsvga_value_read(void *opaque, uint32_t address) caps |= SVGA_CAP_RECT_FILL; #endif #ifdef HW_MOUSE_ACCEL -if (dpy_cursor_define_supported(s->vga.con)) { -caps |= SVGA_CAP_CURSOR | SVGA_CAP_CURSOR_BYPASS_2 | -SVGA_CAP_CURSOR_BYPASS; -} +caps |= SVGA_CAP_CURSOR | SVGA_CAP_CURSOR_BYPASS_2 | +SVGA_CAP_CURSOR_BYPASS; #endif ret = caps; break; diff --git a/ui/console.c b/ui/console.c index 832055675c50..6b6f57441c53 100644 --- a/ui/console.c +++ b/ui/console.c @@ -1058,19 +1058,6 @@ void dpy_cursor_define(QemuConsole *c, QEMUCursor *cursor) } } -bool dpy_cursor_define_supported(QemuConsole *con) -{ -DisplayState *s = con->ds; -DisplayChangeListener *dcl; - -QLIST_FOREACH(dcl, &s->listeners, next) { -if (dcl->ops->dpy_cursor_define) { -return true; -} -} -return false; -} - QEMUGLContext dpy_gl_ctx_create(QemuConsole *con, struct QEMUGLParams *qparams) { -- 2.44.0
[PATCH 0/2] ui/cocoa: Add cursor composition
Add accelerated cursor composition to ui/cocoa. This does not only improve performance for display devices that exposes the capability to the guest according to dpy_cursor_define_supported(), but fixes the cursor display for devices that unconditionally expects the availability of the capability (e.g., virtio-gpu). The common pattern to implement accelerated cursor composition is to replace the cursor and warp it so that the replaced cursor is shown at the correct position on the guest display. Unfortunately, ui/cocoa cannot do the same because warping the cursor position interfers with the mouse input so it uses CALayer instead; although it is not specialized for cursor composition, it still can compose images with hardware acceleration. ui/cocoa was the only non-headless graphical display lacking cursor composition so dpy_cursor_define_supported() is no longer needed and removed. Signed-off-by: Akihiko Odaki --- Akihiko Odaki (2): ui/cocoa: Add cursor composition ui/console: Remove dpy_cursor_define_supported() meson.build | 3 +- include/ui/console.h| 1 - hw/display/qxl-render.c | 4 -- hw/display/vmware_vga.c | 6 +-- ui/console.c| 13 --- ui/cocoa.m | 97 + 6 files changed, 101 insertions(+), 23 deletions(-) --- base-commit: ba49d760eb04630e7b15f423ebecf6c871b8f77b change-id: 20240318-cursor-3491b1806582 Best regards, -- Akihiko Odaki
Re: [PATCH-for-9.1 02/21] target/mips: Declare CPU QOM types using DEFINE_TYPES() macro
On Fri, Mar 15, 2024 at 02:08:50PM +0100, Philippe Mathieu-Daudé wrote: > Date: Fri, 15 Mar 2024 14:08:50 +0100 > From: Philippe Mathieu-Daudé > Subject: [PATCH-for-9.1 02/21] target/mips: Declare CPU QOM types using > DEFINE_TYPES() macro > X-Mailer: git-send-email 2.41.0 > > When multiple QOM types are registered in the same file, > it is simpler to use the the DEFINE_TYPES() macro. In > particular because type array declared with such macro > are easier to review. > > In few commits we are going to add more types, so replace > the type_register_static() to ease further reviews. > > Signed-off-by: Philippe Mathieu-Daudé > Reviewed-by: Richard Henderson > Message-Id: <20231013140116.255-15-phi...@linaro.org> > --- > target/mips/cpu.c | 23 +-- > 1 file changed, 13 insertions(+), 10 deletions(-) Reviewed-by: Zhao Liu
[PATCH 2/5] hw/loongarch: Refine fadt memory table for numa memory
One LoongArch virt machine platform, there is limitation for memory map information. The minimum memory size is 256M and minimum memory size for numa node0 is 256M also. With qemu numa qtest, it is possible that memory size of numa node0 is 128M. Limitations for minimum memory size for both total memory and numa node0 is removed for fadt numa memory table creation. Signed-off-by: Bibo Mao --- hw/loongarch/virt.c | 47 ++--- 1 file changed, 44 insertions(+), 3 deletions(-) diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c index efce112310..ae79b49774 100644 --- a/hw/loongarch/virt.c +++ b/hw/loongarch/virt.c @@ -343,6 +343,48 @@ static void fdt_add_memory_node(MachineState *ms, g_free(nodename); } +static void fdt_add_memory_nodes(MachineState *ms) +{ +hwaddr base, size, ram_size, gap; +int i, nb_numa_nodes, nodes; +NodeInfo *numa_info; + +ram_size = ms->ram_size; +base = VIRT_LOWMEM_BASE; +gap = VIRT_LOWMEM_SIZE; +nodes = nb_numa_nodes = ms->numa_state->num_nodes; +numa_info = ms->numa_state->nodes; +if (!nodes) { +nodes = 1; +} + +for (i = 0; i < nodes; i++) { +if (nb_numa_nodes) { +size = numa_info[i].node_mem; +} else { +size = ram_size; +} + +/* + * memory for the node splited into two part + * lowram: [base, +gap) + * highram: [VIRT_HIGHMEM_BASE, +(len - gap)) + */ +if (size >= gap) { +fdt_add_memory_node(ms, base, gap, i); +size -= gap; +base = VIRT_HIGHMEM_BASE; +gap = ram_size - VIRT_LOWMEM_SIZE; +} + +if (size) { +fdt_add_memory_node(ms, base, size, i); +base += size; +gap -= size; +} +} +} + static void virt_build_smbios(LoongArchMachineState *lams) { MachineState *ms = MACHINE(lams); @@ -864,9 +906,10 @@ static void loongarch_init(MachineState *machine) } fdt_add_cpu_nodes(lams); +fdt_add_memory_nodes(machine); + /* Node0 memory */ memmap_add_entry(VIRT_LOWMEM_BASE, VIRT_LOWMEM_SIZE, 1); -fdt_add_memory_node(machine, VIRT_LOWMEM_BASE, VIRT_LOWMEM_SIZE, 0); memory_region_init_alias(&lams->lowmem, NULL, "loongarch.node0.lowram", machine->ram, offset, VIRT_LOWMEM_SIZE); memory_region_add_subregion(address_space_mem, phyAddr, &lams->lowmem); @@ -880,7 +923,6 @@ static void loongarch_init(MachineState *machine) } phyAddr = VIRT_HIGHMEM_BASE; memmap_add_entry(phyAddr, highram_size, 1); -fdt_add_memory_node(machine, phyAddr, highram_size, 0); memory_region_init_alias(&lams->highmem, NULL, "loongarch.node0.highram", machine->ram, offset, highram_size); memory_region_add_subregion(address_space_mem, phyAddr, &lams->highmem); @@ -896,7 +938,6 @@ static void loongarch_init(MachineState *machine) offset, numa_info[i].node_mem); memory_region_add_subregion(address_space_mem, phyAddr, nodemem); memmap_add_entry(phyAddr, numa_info[i].node_mem, 1); -fdt_add_memory_node(machine, phyAddr, numa_info[i].node_mem, i); offset += numa_info[i].node_mem; phyAddr += numa_info[i].node_mem; } -- 2.39.3
Re: [PATCH-for-9.1 03/21] target/ppc: Declare CPU QOM types using DEFINE_TYPES() macro
On Fri, Mar 15, 2024 at 02:08:51PM +0100, Philippe Mathieu-Daudé wrote: > Date: Fri, 15 Mar 2024 14:08:51 +0100 > From: Philippe Mathieu-Daudé > Subject: [PATCH-for-9.1 03/21] target/ppc: Declare CPU QOM types using > DEFINE_TYPES() macro > X-Mailer: git-send-email 2.41.0 > > When multiple QOM types are registered in the same file, > it is simpler to use the the DEFINE_TYPES() macro. In > particular because type array declared with such macro > are easier to review. > > In few commits we are going to add more types, so replace > the type_register_static() to ease further reviews. > > Signed-off-by: Philippe Mathieu-Daudé > Reviewed-by: Richard Henderson > Message-Id: <20231013140116.255-16-phi...@linaro.org> > --- > target/ppc/cpu_init.c | 52 +++ > 1 file changed, 23 insertions(+), 29 deletions(-) Reviewed-by: Zhao Liu
[PATCH 3/5] hw/loongarch: Refine fwcfg memory map
Memory map table for fwcfg is used for UEFI BIOS, UEFI BIOS uses the first entry from fwcfg memory map as the first memory HOB, the second memory HOB will be used if the first memory HOB is used up. Memory map table for fwcfg does not care about numa node, however in generic the first memory HOB is part of numa node0, so that runtime memory of UEFI which is allocated from the first memory HOB is located at numa node0. Signed-off-by: Bibo Mao --- hw/loongarch/virt.c | 60 ++--- 1 file changed, 57 insertions(+), 3 deletions(-) diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c index ae79b49774..d7e0886c7c 100644 --- a/hw/loongarch/virt.c +++ b/hw/loongarch/virt.c @@ -796,6 +796,62 @@ static void fw_cfg_add_kernel_info(const struct loaderparams *loaderparams, } } +static void fw_cfg_add_memory(MachineState *ms) +{ +hwaddr base, size, ram_size, gap; +int nb_numa_nodes, nodes; +NodeInfo *numa_info; + +ram_size = ms->ram_size; +base = VIRT_LOWMEM_BASE; +gap = VIRT_LOWMEM_SIZE; +nodes = nb_numa_nodes = ms->numa_state->num_nodes; +numa_info = ms->numa_state->nodes; +if (!nodes) { +nodes = 1; +} + +/* add fw_cfg memory map of node0 */ +if (nb_numa_nodes) { +size = numa_info[0].node_mem; +} else { +size = ram_size; +} + +if (size >= gap) { +memmap_add_entry(base, gap, 1); +size -= gap; +base = VIRT_HIGHMEM_BASE; +gap = ram_size - VIRT_LOWMEM_SIZE; +} + +if (size) { +memmap_add_entry(base, size, 1); +base += size; +} + +if (nodes < 2) { +return; +} + +/* add fw_cfg memory map of other nodes */ +size = ram_size - numa_info[0].node_mem; +gap = VIRT_LOWMEM_BASE + VIRT_LOWMEM_SIZE; +if (base < gap && (base + size) > gap) { +/* + * memory map for the maining nodes splited into two part + * lowram: [base, +(gap - base)) + * highram: [VIRT_HIGHMEM_BASE, +(size - (gap - base))) + */ +memmap_add_entry(base, gap - base, 1); +size -= gap - base; +base = VIRT_HIGHMEM_BASE; +} + +if (size) +memmap_add_entry(base, size, 1); +} + static void loongarch_firmware_boot(LoongArchMachineState *lams, const struct loaderparams *loaderparams) { @@ -907,9 +963,9 @@ static void loongarch_init(MachineState *machine) fdt_add_cpu_nodes(lams); fdt_add_memory_nodes(machine); +fw_cfg_add_memory(machine); /* Node0 memory */ -memmap_add_entry(VIRT_LOWMEM_BASE, VIRT_LOWMEM_SIZE, 1); memory_region_init_alias(&lams->lowmem, NULL, "loongarch.node0.lowram", machine->ram, offset, VIRT_LOWMEM_SIZE); memory_region_add_subregion(address_space_mem, phyAddr, &lams->lowmem); @@ -922,7 +978,6 @@ static void loongarch_init(MachineState *machine) highram_size = ram_size - VIRT_LOWMEM_SIZE; } phyAddr = VIRT_HIGHMEM_BASE; -memmap_add_entry(phyAddr, highram_size, 1); memory_region_init_alias(&lams->highmem, NULL, "loongarch.node0.highram", machine->ram, offset, highram_size); memory_region_add_subregion(address_space_mem, phyAddr, &lams->highmem); @@ -937,7 +992,6 @@ static void loongarch_init(MachineState *machine) memory_region_init_alias(nodemem, NULL, ramName, machine->ram, offset, numa_info[i].node_mem); memory_region_add_subregion(address_space_mem, phyAddr, nodemem); -memmap_add_entry(phyAddr, numa_info[i].node_mem, 1); offset += numa_info[i].node_mem; phyAddr += numa_info[i].node_mem; } -- 2.39.3
[PATCH 0/5] hw/loongarch: Refine numa memory map
One LoongArch virt machine platform, there is limitation for memory map information. The minimum memory size is 256M and minimum memory size for numa node0 is 256M also. With qemu numa qtest, it is possible that memory size of numa node0 is 128M. Limitations for minimum memory size for both total memory and numa node0 is removed here, including acpi srat table, fadt memory map table and fw_cfg memory map table. Also remove numa node about memory region, there is only low memory region and how memory region. Bibo Mao (5): hw/loongarch: Refine acpi srat table for numa memory hw/loongarch: Refine fadt memory table for numa memory hw/loongarch: Refine fwcfg memory map hw/loongarch: Refine system dram memory region hw/loongarch: Remove minimum and default memory size hw/loongarch/acpi-build.c | 58 +++-- hw/loongarch/virt.c | 168 ++ 2 files changed, 152 insertions(+), 74 deletions(-) base-commit: ba49d760eb04630e7b15f423ebecf6c871b8f77b -- 2.39.3
Re: [PATCH-for-9.1 04/21] target/sparc: Declare CPU QOM types using DEFINE_TYPES() macro
On Fri, Mar 15, 2024 at 02:08:52PM +0100, Philippe Mathieu-Daudé wrote: > Date: Fri, 15 Mar 2024 14:08:52 +0100 > From: Philippe Mathieu-Daudé > Subject: [PATCH-for-9.1 04/21] target/sparc: Declare CPU QOM types using > DEFINE_TYPES() macro > X-Mailer: git-send-email 2.41.0 > > When multiple QOM types are registered in the same file, > it is simpler to use the the DEFINE_TYPES() macro. In > particular because type array declared with such macro > are easier to review. > > In few commits we are going to add more types, so replace > the type_register_static() to ease further reviews. > > Signed-off-by: Philippe Mathieu-Daudé > Reviewed-by: Richard Henderson > Reviewed-by: Mark Cave-Ayland > Message-Id: <20231013140116.255-17-phi...@linaro.org> > --- > target/sparc/cpu.c | 23 +-- > 1 file changed, 13 insertions(+), 10 deletions(-) Reviewed-by: Zhao Liu
[PATCH 5/5] hw/loongarch: Remove minimum and default memory size
Some qtest test cases such as numa use default memory size of generic machine class, which is 128M by fault. Here generic default memory size is used, and also remove minimum memory size which is 1G originally. Signed-off-by: Bibo Mao --- hw/loongarch/virt.c | 5 - 1 file changed, 5 deletions(-) diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c index a2975c3c5a..deb3750d81 100644 --- a/hw/loongarch/virt.c +++ b/hw/loongarch/virt.c @@ -930,10 +930,6 @@ static void loongarch_init(MachineState *machine) cpu_model = LOONGARCH_CPU_TYPE_NAME("la464"); } -if (ram_size < 1 * GiB) { -error_report("ram_size must be greater than 1G."); -exit(1); -} create_fdt(lams); /* Create IOCSR space */ @@ -1241,7 +1237,6 @@ static void loongarch_class_init(ObjectClass *oc, void *data) mc->desc = "Loongson-3A5000 LS7A1000 machine"; mc->init = loongarch_init; -mc->default_ram_size = 1 * GiB; mc->default_cpu_type = LOONGARCH_CPU_TYPE_NAME("la464"); mc->default_ram_id = "loongarch.ram"; mc->max_cpus = LOONGARCH_MAX_CPUS; -- 2.39.3
[PATCH 4/5] hw/loongarch: Refine system dram memory region
For system dram memory region, it is not necessary to use numa node information. There is only low memory region and high memory region. Remove numa node information for ddr memory region here, it can reduce memory region number about LoongArch virt machine. Signed-off-by: Bibo Mao --- hw/loongarch/virt.c | 56 ++--- 1 file changed, 17 insertions(+), 39 deletions(-) diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c index d7e0886c7c..a2975c3c5a 100644 --- a/hw/loongarch/virt.c +++ b/hw/loongarch/virt.c @@ -917,19 +917,13 @@ static void loongarch_init(MachineState *machine) { LoongArchCPU *lacpu; const char *cpu_model = machine->cpu_type; -ram_addr_t offset = 0; -ram_addr_t ram_size = machine->ram_size; -uint64_t highram_size = 0, phyAddr = 0; MemoryRegion *address_space_mem = get_system_memory(); LoongArchMachineState *lams = LOONGARCH_MACHINE(machine); -int nb_numa_nodes = machine->numa_state->num_nodes; -NodeInfo *numa_info = machine->numa_state->nodes; int i; -hwaddr fdt_base; +hwaddr fdt_base, base, size, ram_size = machine->ram_size; const CPUArchIdList *possible_cpus; MachineClass *mc = MACHINE_GET_CLASS(machine); CPUState *cpu; -char *ramName = NULL; struct loaderparams loaderparams = { }; if (!cpu_model) { @@ -965,41 +959,27 @@ static void loongarch_init(MachineState *machine) fdt_add_memory_nodes(machine); fw_cfg_add_memory(machine); -/* Node0 memory */ -memory_region_init_alias(&lams->lowmem, NULL, "loongarch.node0.lowram", - machine->ram, offset, VIRT_LOWMEM_SIZE); -memory_region_add_subregion(address_space_mem, phyAddr, &lams->lowmem); - -offset += VIRT_LOWMEM_SIZE; -if (nb_numa_nodes > 0) { -assert(numa_info[0].node_mem > VIRT_LOWMEM_SIZE); -highram_size = numa_info[0].node_mem - VIRT_LOWMEM_SIZE; -} else { -highram_size = ram_size - VIRT_LOWMEM_SIZE; +size = ram_size; +base = VIRT_LOWMEM_BASE; +if (size > VIRT_LOWMEM_SIZE) { +size = VIRT_LOWMEM_SIZE; } -phyAddr = VIRT_HIGHMEM_BASE; -memory_region_init_alias(&lams->highmem, NULL, "loongarch.node0.highram", - machine->ram, offset, highram_size); -memory_region_add_subregion(address_space_mem, phyAddr, &lams->highmem); - -/* Node1 - Nodemax memory */ -offset += highram_size; -phyAddr += highram_size; - -for (i = 1; i < nb_numa_nodes; i++) { -MemoryRegion *nodemem = g_new(MemoryRegion, 1); -ramName = g_strdup_printf("loongarch.node%d.ram", i); -memory_region_init_alias(nodemem, NULL, ramName, machine->ram, - offset, numa_info[i].node_mem); -memory_region_add_subregion(address_space_mem, phyAddr, nodemem); -offset += numa_info[i].node_mem; -phyAddr += numa_info[i].node_mem; + +memory_region_init_alias(&lams->lowmem, NULL, "loongarch.lowram", + machine->ram, base, size); +memory_region_add_subregion(address_space_mem, base, &lams->lowmem); +base += size; +if (ram_size - size) { +base = VIRT_HIGHMEM_BASE; +memory_region_init_alias(&lams->highmem, NULL, "loongarch.highram", +machine->ram, VIRT_LOWMEM_BASE + size, ram_size - size); +memory_region_add_subregion(address_space_mem, base, &lams->highmem); +base += ram_size - size; } /* initialize device memory address space */ if (machine->ram_size < machine->maxram_size) { ram_addr_t device_mem_size = machine->maxram_size - machine->ram_size; -hwaddr device_mem_base; if (machine->ram_slots > ACPI_MAX_RAM_SLOTS) { error_report("unsupported amount of memory slots: %"PRIu64, @@ -1013,9 +993,7 @@ static void loongarch_init(MachineState *machine) "%d bytes", TARGET_PAGE_SIZE); exit(EXIT_FAILURE); } -/* device memory base is the top of high memory address. */ -device_mem_base = ROUND_UP(VIRT_HIGHMEM_BASE + highram_size, 1 * GiB); -machine_memory_devices_init(machine, device_mem_base, device_mem_size); +machine_memory_devices_init(machine, base, device_mem_size); } /* load the BIOS image. */ -- 2.39.3
[PATCH 1/5] hw/loongarch: Refine acpi srat table for numa memory
One LoongArch virt machine platform, there is limitation for memory map information. The minimum memory size is 256M and minimum memory size for numa node0 is 256M also. With qemu numa qtest, it is possible that memory size of numa node0 is 128M. Limitations for minimum memory size for both total memory and numa node0 is removed for acpi srat table creation. Signed-off-by: Bibo Mao --- hw/loongarch/acpi-build.c | 58 +++ 1 file changed, 34 insertions(+), 24 deletions(-) diff --git a/hw/loongarch/acpi-build.c b/hw/loongarch/acpi-build.c index e5ab1080af..d0247d93ee 100644 --- a/hw/loongarch/acpi-build.c +++ b/hw/loongarch/acpi-build.c @@ -165,8 +165,9 @@ static void build_srat(GArray *table_data, BIOSLinker *linker, MachineState *machine) { int i, arch_id, node_id; -uint64_t mem_len, mem_base; -int nb_numa_nodes = machine->numa_state->num_nodes; +hwaddr len, base, gap; +NodeInfo *numa_info; +int nodes, nb_numa_nodes = machine->numa_state->num_nodes; LoongArchMachineState *lams = LOONGARCH_MACHINE(machine); MachineClass *mc = MACHINE_GET_CLASS(lams); const CPUArchIdList *arch_ids = mc->possible_cpu_arch_ids(machine); @@ -195,35 +196,44 @@ build_srat(GArray *table_data, BIOSLinker *linker, MachineState *machine) build_append_int_noprefix(table_data, 0, 4); /* Reserved */ } -/* Node0 */ -build_srat_memory(table_data, VIRT_LOWMEM_BASE, VIRT_LOWMEM_SIZE, - 0, MEM_AFFINITY_ENABLED); -mem_base = VIRT_HIGHMEM_BASE; -if (!nb_numa_nodes) { -mem_len = machine->ram_size - VIRT_LOWMEM_SIZE; -} else { -mem_len = machine->numa_state->nodes[0].node_mem - VIRT_LOWMEM_SIZE; +base = VIRT_LOWMEM_BASE; +gap = VIRT_LOWMEM_SIZE; +numa_info = machine->numa_state->nodes; +nodes = nb_numa_nodes; +if (!nodes) { +nodes = 1; } -if (mem_len) -build_srat_memory(table_data, mem_base, mem_len, 0, MEM_AFFINITY_ENABLED); - -/* Node1 - Nodemax */ -if (nb_numa_nodes) { -mem_base += mem_len; -for (i = 1; i < nb_numa_nodes; ++i) { -if (machine->numa_state->nodes[i].node_mem > 0) { -build_srat_memory(table_data, mem_base, - machine->numa_state->nodes[i].node_mem, i, - MEM_AFFINITY_ENABLED); -mem_base += machine->numa_state->nodes[i].node_mem; -} + +for (i = 0; i < nodes; i++) { +if (nb_numa_nodes) { +len = numa_info[i].node_mem; +} else { +len = machine->ram_size; +} + +/* + * memory for the node splited into two part + * lowram: [base, +gap) + * highram: [VIRT_HIGHMEM_BASE, +(len - gap)) + */ +if (len >= gap) { +build_srat_memory(table_data, base, len, i, MEM_AFFINITY_ENABLED); +len -= gap; +base = VIRT_HIGHMEM_BASE; +gap = machine->ram_size - VIRT_LOWMEM_SIZE; +} + +if (len) { +build_srat_memory(table_data, base, len, i, MEM_AFFINITY_ENABLED); +base += len; +gap -= len; } } if (machine->device_memory) { build_srat_memory(table_data, machine->device_memory->base, memory_region_size(&machine->device_memory->mr), - nb_numa_nodes - 1, + nodes - 1, MEM_AFFINITY_HOTPLUGGABLE | MEM_AFFINITY_ENABLED); } -- 2.39.3
Re: [QEMU PATCH v6 1/1] virtio-pci: implement No_Soft_Reset bit
On Thu, Feb 22, 2024 at 11:40:10AM +0800, Jiqian Chen wrote: > In current code, when guest does S3, virtio devices are reset due to > the bit No_Soft_Reset is not set. After resetting, the display resources > of virtio-gpu are destroyed, then the display can't come back and only > show blank after resuming. > > Implement No_Soft_Reset bit of PCI_PM_CTRL register, then guest can check > this bit, if this bit is set, the devices resetting will not be done, and > then the display can work after resuming. > > Signed-off-by: Jiqian Chen > --- > hw/virtio/virtio-pci.c | 37 +- > include/hw/virtio/virtio-pci.h | 5 + > 2 files changed, 41 insertions(+), 1 deletion(-) > > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c > index 1a7039fb0c68..da5312010345 100644 > --- a/hw/virtio/virtio-pci.c > +++ b/hw/virtio/virtio-pci.c > @@ -2197,6 +2197,11 @@ static void virtio_pci_realize(PCIDevice *pci_dev, > Error **errp) > pcie_cap_lnkctl_init(pci_dev); > } > > +if (proxy->flags & VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET) { > +pci_set_word(pci_dev->config + pos + PCI_PM_CTRL, > + PCI_PM_CTRL_NO_SOFT_RESET); > +} > + > if (proxy->flags & VIRTIO_PCI_FLAG_INIT_PM) { > /* Init Power Management Control Register */ > pci_set_word(pci_dev->wmask + pos + PCI_PM_CTRL, Don't we need compat machinery to avoid breaking migration for existing machine types? > @@ -2259,18 +2264,46 @@ static void virtio_pci_reset(DeviceState *qdev) > } > } > > +static bool device_no_need_reset(PCIDevice *dev) > +{ > +if (pci_is_express(dev)) { > +uint16_t pmcsr; > + > +pmcsr = pci_get_word(dev->config + dev->exp.pm_cap + PCI_PM_CTRL); > +/* > + * When No_Soft_Reset bit is set and device the device > + * is in D3hot state, can't reset device can't? or don't? > + */ > +if ((pmcsr & PCI_PM_CTRL_NO_SOFT_RESET) && > +(pmcsr & PCI_PM_CTRL_STATE_MASK) == 3) > +return true; coding style violation > +} > + > +return false; > +} > + > static void virtio_pci_bus_reset_hold(Object *obj) > { > PCIDevice *dev = PCI_DEVICE(obj); > DeviceState *qdev = DEVICE(obj); > > +if (device_no_need_reset(dev)) > +return; > + > virtio_pci_reset(qdev); > > if (pci_is_express(dev)) { > +uint16_t pmcsr; > +uint16_t val = 0; > + > pcie_cap_deverr_reset(dev); > pcie_cap_lnkctl_reset(dev); > > -pci_set_word(dev->config + dev->exp.pm_cap + PCI_PM_CTRL, 0); > +/* don't reset the RO bits */ > +pmcsr = pci_get_word(dev->config + dev->exp.pm_cap + PCI_PM_CTRL); > +val = val | (pmcsr & PCI_PM_CTRL_NO_SOFT_RESET) | > +(pmcsr & PCI_PM_CTRL_DATA_SCALE_MASK); > +pci_set_word(dev->config + dev->exp.pm_cap + PCI_PM_CTRL, val); First, we have test and clear for this. Second, this has to be conditional on the flag, no? Without the flag don't we reset everything? Or you can reuse wmask for this, also an option. Also what's going on with PCI_PM_CTRL_DATA_SCALE_MASK? Where does that come from? > } > } > > @@ -2297,6 +2330,8 @@ static Property virtio_pci_properties[] = { > VIRTIO_PCI_FLAG_INIT_LNKCTL_BIT, true), > DEFINE_PROP_BIT("x-pcie-pm-init", VirtIOPCIProxy, flags, > VIRTIO_PCI_FLAG_INIT_PM_BIT, true), > +DEFINE_PROP_BIT("x-pcie-pm-no-soft-reset", VirtIOPCIProxy, flags, > +VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET_BIT, true), > DEFINE_PROP_BIT("x-pcie-flr-init", VirtIOPCIProxy, flags, > VIRTIO_PCI_FLAG_INIT_FLR_BIT, true), > DEFINE_PROP_BIT("aer", VirtIOPCIProxy, flags, > diff --git a/include/hw/virtio/virtio-pci.h b/include/hw/virtio/virtio-pci.h > index 59d88018c16a..9e67ba38c748 100644 > --- a/include/hw/virtio/virtio-pci.h > +++ b/include/hw/virtio/virtio-pci.h > @@ -43,6 +43,7 @@ enum { > VIRTIO_PCI_FLAG_INIT_FLR_BIT, > VIRTIO_PCI_FLAG_AER_BIT, > VIRTIO_PCI_FLAG_ATS_PAGE_ALIGNED_BIT, > +VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET_BIT, > }; > > /* Need to activate work-arounds for buggy guests at vmstate load. */ > @@ -79,6 +80,10 @@ enum { > /* Init Power Management */ > #define VIRTIO_PCI_FLAG_INIT_PM (1 << VIRTIO_PCI_FLAG_INIT_PM_BIT) > > +/* Init The No_Soft_Reset bit of Power Management */ > +#define VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET \ > + (1 << VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET_BIT) > + > /* Init Function Level Reset capability */ > #define VIRTIO_PCI_FLAG_INIT_FLR (1 << VIRTIO_PCI_FLAG_INIT_FLR_BIT) > > -- > 2.34.1
Re: Regression in v7.2.10 - ui-dbus.so requires -fPIC
Mon, 18 Mar 2024 11:35:54 +0400 Marc-André Lureau : > Alternatively, we could always build with pic: true (or pic: > enable_modules), but that's not ideal either. I'm sure that unconditionally building with -fPIC has no downsides in this context. Olaf pgpGUQPNKGONs.pgp Description: Digitale Signatur von OpenPGP
Re: [PATCH-for-9.1 05/21] cpus: Open code OBJECT_DECLARE_TYPE() in OBJECT_DECLARE_CPU_TYPE()
Hi Philippe, On Fri, Mar 15, 2024 at 02:08:53PM +0100, Philippe Mathieu-Daudé wrote: > Date: Fri, 15 Mar 2024 14:08:53 +0100 > From: Philippe Mathieu-Daudé > Subject: [PATCH-for-9.1 05/21] cpus: Open code OBJECT_DECLARE_TYPE() in > OBJECT_DECLARE_CPU_TYPE() > X-Mailer: git-send-email 2.41.0 > > Since the OBJECT_DECLARE_CPU_TYPE() macro uses the abstract ArchCPU > type, when declaring multiple CPUs of the same ArchCPU type we get > an error related to the indirect G_DEFINE_AUTOPTR_CLEANUP_FUNC() > use within OBJECT_DECLARE_TYPE(): > > target/mips/cpu-qom.h:31:1: error: redefinition of > 'glib_autoptr_clear_ArchCPU' > OBJECT_DECLARE_CPU_TYPE(MIPS64CPU, MIPSCPUClass, MIPS64_CPU) > ^ > include/hw/core/cpu.h:82:5: note: expanded from macro > 'OBJECT_DECLARE_CPU_TYPE' > OBJECT_DECLARE_TYPE(ArchCPU, CpuClassType, CPU_MODULE_OBJ_NAME); > ^ > include/qom/object.h:237:5: note: expanded from macro 'OBJECT_DECLARE_TYPE' > G_DEFINE_AUTOPTR_CLEANUP_FUNC(InstanceType, object_unref) \ > ^ > /usr/include/glib-2.0/glib/gmacros.h:1371:3: note: expanded from macro > 'G_DEFINE_AUTOPTR_CLEANUP_FUNC' > _GLIB_DEFINE_AUTOPTR_CLEANUP_FUNCS(TypeName, TypeName, func) > ^ > /usr/include/glib-2.0/glib/gmacros.h:1354:36: note: expanded from macro > '_GLIB_DEFINE_AUTOPTR_CLEANUP_FUNCS' > static G_GNUC_UNUSED inline void _GLIB_AUTOPTR_CLEAR_FUNC_NAME(TypeName) > (TypeName *_ptr) \ > ^ > /usr/include/glib-2.0/glib/gmacros.h:1338:49: note: expanded from macro > '_GLIB_AUTOPTR_CLEAR_FUNC_NAME' > #define _GLIB_AUTOPTR_CLEAR_FUNC_NAME(TypeName) > glib_autoptr_clear_##TypeName > ^ > :54:1: note: expanded from here > glib_autoptr_clear_ArchCPU > ^ > target/mips/cpu-qom.h:30:1: note: previous definition is here > OBJECT_DECLARE_CPU_TYPE(MIPS32CPU, MIPSCPUClass, MIPS32_CPU) > ^ > > Avoid that problem by expanding the OBJECT_DECLARE_TYPE() macro > within OBJECT_DECLARE_CPU_TYPE(). > > Signed-off-by: Philippe Mathieu-Daudé > Acked-by: Richard Henderson > --- > TODO: check rth comment: > What about adding an OBJECT_DECLARE_CPU_SUBTYPE that omits half the stuff > instead? > We don't need another object typedef at all, for instance. > --- > include/hw/core/cpu.h | 7 ++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h > index ec14f74ce5..4c2e5095bf 100644 > --- a/include/hw/core/cpu.h > +++ b/include/hw/core/cpu.h > @@ -78,7 +78,12 @@ DECLARE_CLASS_CHECKERS(CPUClass, CPU, > */ > #define OBJECT_DECLARE_CPU_TYPE(CpuInstanceType, CpuClassType, > CPU_MODULE_OBJ_NAME) \ > typedef struct ArchCPU CpuInstanceType; \ > -OBJECT_DECLARE_TYPE(ArchCPU, CpuClassType, CPU_MODULE_OBJ_NAME); > +typedef struct CpuClassType CpuClassType; \ > +\ > +G_DEFINE_AUTOPTR_CLEANUP_FUNC(CpuInstanceType, object_unref) \ > +\ > +DECLARE_OBJ_CHECKERS(CpuInstanceType, CpuClassType, \ > + CPU_MODULE_OBJ_NAME, TYPE_##CPU_MODULE_OBJ_NAME) > The OBJECT_DECLARE_TYPE is expaneded as the following: #define OBJECT_DECLARE_TYPE(InstanceType, ClassType, MODULE_OBJ_NAME) \ typedef struct InstanceType InstanceType; \ typedef struct ClassType ClassType; \ \ G_DEFINE_AUTOPTR_CLEANUP_FUNC(InstanceType, object_unref) \ \ DECLARE_OBJ_CHECKERS(InstanceType, ClassType, \ MODULE_OBJ_NAME, TYPE_##MODULE_OBJ_NAME) So the above code change deletes a typedef: typedef struct ArchCPU ArchCPU; Will this deletion break the direct uses of ArchCPU? e.g., in accel/tcg/translator.c: static void set_can_do_io(DisasContextBase *db, bool val) { if (db->saved_can_do_io != val) { ... tcg_gen_st8_i32(tcg_constant_i32(val), tcg_env, offsetof(ArchCPU, parent_obj.neg.can_do_io) - offsetof(ArchCPU, env)); } } Thanks, Zhao
Re: [PATCH 1/4] ui/vc: Do not inherit the size of active console
On Mon, Mar 18, 2024 at 11:59 AM Akihiko Odaki wrote: > > 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 Reviewed-by: Marc-André Lureau > --- > 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 > > -- Marc-André Lureau
Re: [PATCH 2/4] ui/vnc: Do not use console_select()
Hi On Mon, Mar 18, 2024 at 11:58 AM Akihiko Odaki wrote: > > 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 > --- > 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..a703f7466499 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_first_graphic_console(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..6bf02a23414c 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_first_graphic_console(void) > +{ > +QemuConsole *con; > + > +QTAILQ_FOREACH(con, &consoles, next) { > +if (QEMU_IS_GRAPHIC_CONSOLE(con)) { > +return con; > +} > +} > +return NULL; > +} > + > 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..94564b196ba8 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_first_graphic_console(); why this change here? otherwise, lgtm > } > > if (con != vd->dcl.con) { > > -- > 2.44.0 > > -- Marc-André Lureau
Re: [PATCH] vhost-vdpa: check vhost_vdpa_set_vring_ready() return value
On Mon, Mar 18, 2024 at 12:31:59PM +0800, Jason Wang wrote: On Fri, Mar 15, 2024 at 4:23 PM Stefano Garzarella wrote: On Thu, Mar 14, 2024 at 11:17:01AM +0800, Jason Wang wrote: >On Wed, Feb 7, 2024 at 5:27 PM Stefano Garzarella wrote: >> >> vhost_vdpa_set_vring_ready() could already fail, but if Linux's >> patch [1] will be merged, it may fail with more chance if >> userspace does not activate virtqueues before DRIVER_OK when >> VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK is not negotiated. > >I wonder what happens if we just leave it as is. Are you referring to this patch or the kernel patch? This patch. Here I'm just checking the return value of vhost_vdpa_set_vring_ready(). It can return an error also without that kernel patch, so IMHO is better to check the return value here in QEMU. What issue do you see with this patch applied? For the parent which can enable after driver_ok but not advertise it. But this patch is not changing anything in that sense, it just controls the return value of the VHOST_VDPA_SET_VRING_ENABLE ioctl. Why would QEMU ignore an error if it can't activate vrings? If we really want to ignore it we should document it both in QEMU, but also in the kernel, because honestly the way the code is now it shouldn't fail from what I understand. That said, even if we ignore it, IMHO we should at least print a warning in QEMU. (To say the truth, I'm not sure if we need to care about this) I agree on that, but this is related to the patch in the kernel, not this simple patch to fix QEMU error path, right? > >VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK: We do know enabling could be >done after driver_ok. >Without VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK: We don't know whether >enabling could be done after driver_ok or not. I see your point, indeed I didn't send a v2 of that patch. Maybe we should document that, because it could be interpreted that if VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK is not negotiated the enabling should always be done before driver_ok (which is true for example in VDUSE). I see, so I think we probably need the fix. BTW I think we should discuss it in the kernel patch. Thanks, Stefano > >Thanks > >> >> So better check its return value anyway. >> >> [1] https://lore.kernel.org/virtualization/20240206145154.118044-1-sgarz...@redhat.com/T/#u >> >> Signed-off-by: Stefano Garzarella >> --- >> Note: This patch conflicts with [2], but the resolution is simple, >> so for now I sent a patch for the current master, but I'll rebase >> this patch if we merge the other one first. Will go through [2]. Here I meant that the conflict is only in the code touched, because Kevin's patch remove/move some of the code touched by this patch. And rightly he checked the return value of the ioctl as I would like to do in the other places where we call the same ioctl. So honestly I still don't understand what's wrong with this patch... Thanks, Stefano Thanks >> >> [2] >> https://lore.kernel.org/qemu-devel/20240202132521.32714-1-kw...@redhat.com/ >> --- >> hw/virtio/vdpa-dev.c | 8 +++- >> net/vhost-vdpa.c | 15 --- >> 2 files changed, 19 insertions(+), 4 deletions(-) >> >> diff --git a/hw/virtio/vdpa-dev.c b/hw/virtio/vdpa-dev.c >> index eb9ecea83b..d57cd76c18 100644 >> --- a/hw/virtio/vdpa-dev.c >> +++ b/hw/virtio/vdpa-dev.c >> @@ -259,7 +259,11 @@ static int vhost_vdpa_device_start(VirtIODevice *vdev, Error **errp) >> goto err_guest_notifiers; >> } >> for (i = 0; i < s->dev.nvqs; ++i) { >> -vhost_vdpa_set_vring_ready(&s->vdpa, i); >> +ret = vhost_vdpa_set_vring_ready(&s->vdpa, i); >> +if (ret < 0) { >> +error_setg_errno(errp, -ret, "Error starting vring %d", i); >> +goto err_dev_stop; >> +} >> } >> s->started = true; >> >> @@ -274,6 +278,8 @@ static int vhost_vdpa_device_start(VirtIODevice *vdev, Error **errp) >> >> return ret; >> >> +err_dev_stop: >> +vhost_dev_stop(&s->dev, vdev, false); >> err_guest_notifiers: >> k->set_guest_notifiers(qbus->parent, s->dev.nvqs, false); >> err_host_notifiers: >> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c >> index 3726ee5d67..e3d8036479 100644 >> --- a/net/vhost-vdpa.c >> +++ b/net/vhost-vdpa.c >> @@ -381,7 +381,10 @@ static int vhost_vdpa_net_data_load(NetClientState *nc) >> } >> >> for (int i = 0; i < v->dev->nvqs; ++i) { >> -vhost_vdpa_set_vring_ready(v, i + v->dev->vq_index); >> +int ret = vhost_vdpa_set_vring_ready(v, i + v->dev->vq_index); >> +if (ret < 0) { >> +return ret; >> +} >> } >> return 0; >> } >> @@ -1213,7 +1216,10 @@ static int vhost_vdpa_net_cvq_load(NetClientState *nc) >> >> assert(nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA); >> >> -vhost_vdpa_set_vring_ready(v, v->dev->vq_index); >> +r = vhost_vdpa_set_vring_ready(v, v->dev->vq_index); >> +if (unlikely(r < 0)) { >> +return r; >> +
Re: [PATCH for 9.0 v15 02/10] trans_rvv.c.inc: set vstart = 0 in int scalar move insns
On Fri, Mar 15, 2024 at 3:59 AM 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 > Reviewed-by: Richard Henderson Reviewed-by: Alistair Francis Alistair > --- > 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; > -- > 2.44.0 > >
Re: [PATCH-for-9.1 06/21] target/i386: Make X86_CPU common to new I386_CPU / X86_64_CPU types
Hi Philippe, On Fri, Mar 15, 2024 at 02:08:54PM +0100, Philippe Mathieu-Daudé wrote: > Date: Fri, 15 Mar 2024 14:08:54 +0100 > From: Philippe Mathieu-Daudé > Subject: [PATCH-for-9.1 06/21] target/i386: Make X86_CPU common to new > I386_CPU / X86_64_CPU types > X-Mailer: git-send-email 2.41.0 > > "target/foo/cpu-qom.h" can not use any target specific definitions. > > Currently "target/i386/cpu-qom.h" defines TYPE_X86_CPU depending > on the i386/x86_64 build type. This doesn't scale in a heterogeneous > context where we need to access both types concurrently. Does this mean that there would be a TCG case contains both 64-bit and 32-bit i386 core? ;-) > In order to do that, introduce the new I386_CPU / X86_64_CPU > types, both inheriting a common TYPE_X86_CPU base type. > > Keep the current "base" and "max" CPU types as 32 or 64-bit, > depending on the binary built. > > Adapt the cpu-plug-test, since the 'base' architecture is now > common to both 32/64-bit x86 targets. > > Signed-off-by: Philippe Mathieu-Daudé > Acked-by: Richard Henderson > --- > target/i386/cpu-qom.h | 16 ++-- > target/i386/cpu.c | 20 ++-- > tests/qtest/cpu-plug-test.c | 2 +- > 3 files changed, 29 insertions(+), 9 deletions(-) > > diff --git a/target/i386/cpu-qom.h b/target/i386/cpu-qom.h > index d4e216d000..de28d7ea20 100644 > --- a/target/i386/cpu-qom.h > +++ b/target/i386/cpu-qom.h > @@ -1,5 +1,5 @@ > /* > - * QEMU x86 CPU > + * QEMU x86 CPU QOM header (target agnostic) > * > * Copyright (c) 2012 SUSE LINUX Products GmbH > * > @@ -22,14 +22,18 @@ > > #include "hw/core/cpu.h" > > -#ifdef TARGET_X86_64 > -#define TYPE_X86_CPU "x86_64-cpu" > -#else > -#define TYPE_X86_CPU "i386-cpu" > -#endif > +#define TYPE_X86_CPU"x86-cpu" > +#define TYPE_I386_CPU "i386-cpu" > +#define TYPE_X86_64_CPU "x86_64-cpu" > > OBJECT_DECLARE_CPU_TYPE(X86CPU, X86CPUClass, X86_CPU) > > +OBJECT_DECLARE_CPU_TYPE(I386CPU, X86CPUClass, I386_CPU) > +OBJECT_DECLARE_CPU_TYPE(X86_64CPU, X86CPUClass, X86_64_CPU) > + > +#define X86_CPU_TYPE_SUFFIX "-" TYPE_X86_CPU > +#define X86_CPU_TYPE_NAME(name) (name X86_CPU_TYPE_SUFFIX) > + X86_CPU_TYPE_NAME seems to be duplicated because the following line is the existing X86_CPU_TYPE_NAME definition. > #define X86_CPU_TYPE_SUFFIX "-" TYPE_X86_CPU > #define X86_CPU_TYPE_NAME(name) (name X86_CPU_TYPE_SUFFIX) > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > index ebf555f50f..07f64c1ea5 100644 > --- a/target/i386/cpu.c > +++ b/target/i386/cpu.c > @@ -8057,12 +8057,28 @@ static const TypeInfo x86_cpu_types[] = { > .class_size = sizeof(X86CPUClass), > .class_init = x86_cpu_common_class_init, > }, { > -.name = X86_CPU_TYPE_NAME("base"), > +.name = TYPE_I386_CPU, > .parent = TYPE_X86_CPU, > +.abstract = true, > +}, { > +.name = TYPE_X86_64_CPU, > +.parent = TYPE_X86_CPU, > +.abstract = true, > +}, { Should TYPE_I386_CPU/TYPE_X86_64_CPU be also wrapped with TARGET_X86_64? Otherwise, we would keep the 32-bit CPU type definition of TYPE_I386_CPU in the 64-bit case. > +.name = X86_CPU_TYPE_NAME("base"), > +#ifdef TARGET_X86_64 > +.parent = TYPE_X86_64_CPU, > +#else > +.parent = TYPE_I386_CPU, > +#endif > .class_init = x86_cpu_base_class_init, > }, { > .name = X86_CPU_TYPE_NAME("max"), > -.parent = TYPE_X86_CPU, > +#ifdef TARGET_X86_64 > +.parent = TYPE_X86_64_CPU, > +#else > +.parent = TYPE_I386_CPU, > +#endif > .instance_init = max_x86_cpu_initfn, > .class_init = max_x86_cpu_class_init, > }
Re: [PATCH v2 2/3] qom/object_interfaces: Make object_set_properties_from_qdict return bool
On Mon, Mar 18, 2024 at 11:32:09AM +0800, Zhenzhong Duan wrote: > Date: Mon, 18 Mar 2024 11:32:09 +0800 > From: Zhenzhong Duan > Subject: [PATCH v2 2/3] qom/object_interfaces: Make > object_set_properties_from_qdict return bool > X-Mailer: git-send-email 2.34.1 > > Make object_set_properties_from_qdict() return bool, so that > user_creatable_add_type() could check its return value instead > of local_err pointer. > > Opportunistically, do the same change to check return value of > object_property_try_add_child() instead of local_err pointer. > > Suggested-by: Zhao Liu > Signed-off-by: Zhenzhong Duan > --- > qom/object_interfaces.c | 21 ++--- > 1 file changed, 10 insertions(+), 11 deletions(-) Reviewed-by: Zhao Liu
Re: [PATCH v2 3/3] qom/object_interfaces: Remove local_err in user_creatable_add_type
On Mon, Mar 18, 2024 at 11:32:10AM +0800, Zhenzhong Duan wrote: > Date: Mon, 18 Mar 2024 11:32:10 +0800 > From: Zhenzhong Duan > Subject: [PATCH v2 3/3] qom/object_interfaces: Remove local_err in > user_creatable_add_type > X-Mailer: git-send-email 2.34.1 > > In user_creatable_add_type, there is mixed usage of ERRP_GUARD and > local_err. This makes error_abort not taking effect in those callee > functions with &local_err passed. > > Now that we already use ERRP_GUARD, remove local_err and pass errp. > > Signed-off-by: Zhenzhong Duan > --- > qom/object_interfaces.c | 8 +++- > 1 file changed, 3 insertions(+), 5 deletions(-) Reviewed-by: Zhao Liu
Re: [PATCH for 9.0 v15 03/10] target/riscv/vector_helper.c: fix 'vmvr_v' memcpy endianess
On Fri, Mar 15, 2024 at 3:58 AM Daniel Henrique Barboza wrote: > > vmvr_v isn't handling the case where the host might be big endian and > the bytes to be copied aren't sequential. > > Suggested-by: Richard Henderson > Fixes: f714361ed7 ("target/riscv: rvv-1.0: implement vstart CSR") > Signed-off-by: Daniel Henrique Barboza Reviewed-by: Alistair Francis Alistair > --- > target/riscv/vector_helper.c | 10 +- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/target/riscv/vector_helper.c b/target/riscv/vector_helper.c > index ca79571ae2..34ac4aa808 100644 > --- a/target/riscv/vector_helper.c > +++ b/target/riscv/vector_helper.c > @@ -5075,9 +5075,17 @@ void HELPER(vmvr_v)(void *vd, void *vs2, CPURISCVState > *env, uint32_t desc) > uint32_t startb = env->vstart * sewb; > uint32_t i = startb; > > +if (HOST_BIG_ENDIAN && i % 8 != 0) { > +uint32_t j = ROUND_UP(i, 8); > +memcpy((uint8_t *)vd + H1(j - 1), > + (uint8_t *)vs2 + H1(j - 1), > + j - i); > +i = j; > +} > + > memcpy((uint8_t *)vd + H1(i), > (uint8_t *)vs2 + H1(i), > - maxsz - startb); > + maxsz - i); > > env->vstart = 0; > } > -- > 2.44.0 > >
Re: [PATCH for 9.0 v15 04/10] target/riscv: always clear vstart in whole vec move insns
On Fri, Mar 15, 2024 at 3:58 AM Daniel Henrique Barboza wrote: > > These insns have 2 paths: we'll either have vstart already cleared if > vstart_eq_zero or we'll do a brcond to check if vstart >= maxsz to call > the 'vmvr_v' helper. The helper will clear vstart if it executes until > the end, or if vstart >= vl. > > For starters, the check itself is wrong: we're checking vstart >= maxsz, > when in fact we should use vstart in bytes, or 'startb' like 'vmvr_v' is > calling, to do the comparison. But even after fixing the comparison we'll > still need to clear vstart in the end, which isn't happening too. > > We want to make the helpers responsible to manage vstart, including > these corner cases, precisely to avoid these situations: > > - remove the wrong vstart >= maxsz cond from the translation; > - add a 'startb >= maxsz' cond in 'vmvr_v', and clear vstart if that > happens. > > This way we're now sure that vstart is being cleared in the end of the > execution, regardless of the path taken. > > Fixes: f714361ed7 ("target/riscv: rvv-1.0: implement vstart CSR") > Signed-off-by: Daniel Henrique Barboza Reviewed-by: Alistair Francis Alistair > --- > target/riscv/insn_trans/trans_rvv.c.inc | 3 --- > target/riscv/vector_helper.c| 5 + > 2 files changed, 5 insertions(+), 3 deletions(-) > > diff --git a/target/riscv/insn_trans/trans_rvv.c.inc > b/target/riscv/insn_trans/trans_rvv.c.inc > index 8c16a9f5b3..52c26a7834 100644 > --- a/target/riscv/insn_trans/trans_rvv.c.inc > +++ b/target/riscv/insn_trans/trans_rvv.c.inc > @@ -3664,12 +3664,9 @@ static bool trans_##NAME(DisasContext *s, arg_##NAME * > a) \ > vreg_ofs(s, a->rs2), maxsz, maxsz);\ > mark_vs_dirty(s); \ > } else {\ > -TCGLabel *over = gen_new_label(); \ > -tcg_gen_brcondi_tl(TCG_COND_GEU, cpu_vstart, maxsz, over); \ > tcg_gen_gvec_2_ptr(vreg_ofs(s, a->rd), vreg_ofs(s, a->rs2), \ > tcg_env, maxsz, maxsz, 0, gen_helper_vmvr_v); > \ > mark_vs_dirty(s); \ > -gen_set_label(over);\ > } \ > return true;\ > } \ > diff --git a/target/riscv/vector_helper.c b/target/riscv/vector_helper.c > index 34ac4aa808..bcc553c0e2 100644 > --- a/target/riscv/vector_helper.c > +++ b/target/riscv/vector_helper.c > @@ -5075,6 +5075,11 @@ void HELPER(vmvr_v)(void *vd, void *vs2, CPURISCVState > *env, uint32_t desc) > uint32_t startb = env->vstart * sewb; > uint32_t i = startb; > > +if (startb >= maxsz) { > +env->vstart = 0; > +return; > +} > + > if (HOST_BIG_ENDIAN && i % 8 != 0) { > uint32_t j = ROUND_UP(i, 8); > memcpy((uint8_t *)vd + H1(j - 1), > -- > 2.44.0 > >
Re: [QEMU PATCH v6 1/1] virtio-pci: implement No_Soft_Reset bit
On 2024/3/18 16:04, Michael S. Tsirkin wrote: > On Thu, Feb 22, 2024 at 11:40:10AM +0800, Jiqian Chen wrote: >> In current code, when guest does S3, virtio devices are reset due to >> the bit No_Soft_Reset is not set. After resetting, the display resources >> of virtio-gpu are destroyed, then the display can't come back and only >> show blank after resuming. >> >> Implement No_Soft_Reset bit of PCI_PM_CTRL register, then guest can check >> this bit, if this bit is set, the devices resetting will not be done, and >> then the display can work after resuming. >> >> Signed-off-by: Jiqian Chen >> --- >> hw/virtio/virtio-pci.c | 37 +- >> include/hw/virtio/virtio-pci.h | 5 + >> 2 files changed, 41 insertions(+), 1 deletion(-) >> >> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c >> index 1a7039fb0c68..da5312010345 100644 >> --- a/hw/virtio/virtio-pci.c >> +++ b/hw/virtio/virtio-pci.c >> @@ -2197,6 +2197,11 @@ static void virtio_pci_realize(PCIDevice *pci_dev, >> Error **errp) >> pcie_cap_lnkctl_init(pci_dev); >> } >> >> +if (proxy->flags & VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET) { >> +pci_set_word(pci_dev->config + pos + PCI_PM_CTRL, >> + PCI_PM_CTRL_NO_SOFT_RESET); >> +} >> + >> if (proxy->flags & VIRTIO_PCI_FLAG_INIT_PM) { >> /* Init Power Management Control Register */ >> pci_set_word(pci_dev->wmask + pos + PCI_PM_CTRL, > > > Don't we need compat machinery to avoid breaking migration for > existing machine types? Could you elaborate on it? I am sorry I don't know which machine types to be compatible with, and how to be compatible with them. Can I simply set the default value of x-pcie-pm-no-soft-reset to false? If someone need this bit, they can set x-pcie-pm-no-soft-reset=true in the config parameters for Qemu. So that will not affect exiting machine types? > >> @@ -2259,18 +2264,46 @@ static void virtio_pci_reset(DeviceState *qdev) >> } >> } >> >> +static bool device_no_need_reset(PCIDevice *dev) >> +{ >> +if (pci_is_express(dev)) { >> +uint16_t pmcsr; >> + >> +pmcsr = pci_get_word(dev->config + dev->exp.pm_cap + PCI_PM_CTRL); >> +/* >> + * When No_Soft_Reset bit is set and device > > the device Will change in next version. > >> + * is in D3hot state, can't reset device > > can't? or don't? Will change to "don't" in next version. > >> + */ >> +if ((pmcsr & PCI_PM_CTRL_NO_SOFT_RESET) && >> +(pmcsr & PCI_PM_CTRL_STATE_MASK) == 3) >> +return true; > > coding style violation Will add braces {} in next version. > >> +} >> + >> +return false; >> +} >> + >> static void virtio_pci_bus_reset_hold(Object *obj) >> { >> PCIDevice *dev = PCI_DEVICE(obj); >> DeviceState *qdev = DEVICE(obj); >> >> +if (device_no_need_reset(dev)) >> +return; >> + >> virtio_pci_reset(qdev); >> >> if (pci_is_express(dev)) { >> +uint16_t pmcsr; >> +uint16_t val = 0; >> + >> pcie_cap_deverr_reset(dev); >> pcie_cap_lnkctl_reset(dev); >> >> -pci_set_word(dev->config + dev->exp.pm_cap + PCI_PM_CTRL, 0); >> +/* don't reset the RO bits */ >> +pmcsr = pci_get_word(dev->config + dev->exp.pm_cap + PCI_PM_CTRL); >> +val = val | (pmcsr & PCI_PM_CTRL_NO_SOFT_RESET) | >> +(pmcsr & PCI_PM_CTRL_DATA_SCALE_MASK); >> +pci_set_word(dev->config + dev->exp.pm_cap + PCI_PM_CTRL, val); > > First, we have test and clear for this. > > Second, this has to be conditional on the flag, no? Before resetting, I first read the value of config, its function is equivalent to checking the label, right? If the flag is set, the value of No_Soft_Reset is 1, the bit of "val" is also 1. If the flag is not set, the value of No_Soft_Reset is 0, "val" is also 0. > Without the flag don't we reset everything? We need reset everything include the RO bit, right? If so, that is my fault, then I will change to check the flag of proxy in next version. > Or you can reuse wmask for this, also an option. > > Also what's going on with PCI_PM_CTRL_DATA_SCALE_MASK? > Where does that come from? I read from PCI spec, the bit DATA_SCALE and No_Soft_Reset are both RO bit, so I thought we shouldn't reset them. It is something wrong with my understanding, I will remove PCI_PM_CTRL_DATA_SCALE_MASK in next version. Sorry. > >> } >> } >> >> @@ -2297,6 +2330,8 @@ static Property virtio_pci_properties[] = { >> VIRTIO_PCI_FLAG_INIT_LNKCTL_BIT, true), >> DEFINE_PROP_BIT("x-pcie-pm-init", VirtIOPCIProxy, flags, >> VIRTIO_PCI_FLAG_INIT_PM_BIT, true), >> +DEFINE_PROP_BIT("x-pcie-pm-no-soft-reset", VirtIOPCIProxy, flags, >> +VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET_BIT, true), >> DEFINE_PROP_BIT("x-pcie-flr-init", VirtIOPCI
Re: [PATCH 08/10] qapi: Correct error message for 'vcpu_dirty_limit' parameter
Philippe Mathieu-Daudé writes: > On 12/3/24 16:26, Zhao Liu wrote: >> On Tue, Mar 12, 2024 at 03:13:41PM +0100, Markus Armbruster wrote: >>> Date: Tue, 12 Mar 2024 15:13:41 +0100 >>> From: Markus Armbruster >>> Subject: [PATCH 08/10] qapi: Correct error message for 'vcpu_dirty_limit' >>> parameter >>> >>> From: Philippe Mathieu-Daudé >>> >>> QERR_INVALID_PARAMETER_VALUE is defined as: >>> >>>#define QERR_INVALID_PARAMETER_VALUE \ >>>"Parameter '%s' expects %s" >>> >>> The current error is formatted as: >>> >>>"Parameter 'vcpu_dirty_limit' expects is invalid, it must greater then 1 >>> MB/s" >>> >>> Replace by: >>> >>>"Parameter 'vcpu_dirty_limit' is invalid, it must greater than 1 MB/s" >> >> Is there a grammar error here? Maybe >> s/it must greater/it must be greater/ > > Oops indeed! What about dropping "is invalid, "? I.e. go with "Parameter 'vcpu_dirty_limit' must be greater than 1 MB/s" >>> Signed-off-by: Philippe Mathieu-Daudé >>> Reviewed-by: Juan Quintela >>> Signed-off-by: Markus Armbruster >>> --- >>> migration/options.c | 5 ++--- >>> 1 file changed, 2 insertions(+), 3 deletions(-) >>> >>> diff --git a/migration/options.c b/migration/options.c >>> index 40eb930940..c5115f1b5c 100644 >>> --- a/migration/options.c >>> +++ b/migration/options.c >>> @@ -1264,9 +1264,8 @@ bool migrate_params_check(MigrationParameters >>> *params, Error **errp) >>> if (params->has_vcpu_dirty_limit && >>> (params->vcpu_dirty_limit < 1)) { >>> -error_setg(errp, QERR_INVALID_PARAMETER_VALUE, >>> - "vcpu_dirty_limit", >>> - "is invalid, it must greater then 1 MB/s"); >>> +error_setg(errp, "Parameter 'vcpu_dirty_limit' is invalid," >>> + " it must greater than 1 MB/s"); > > So s/it must greater/it must be greater/ :) > >>> return false; >>> } >>> >> Otherwise, >> Reviewed-by: Zhao Liu >>
[PATCH] ui/cocoa: Do not automatically zoom for HiDPI
Cocoa automatically zooms for a HiDPI display like Retina and makes the display blurry. Revert the automatic zooming. Signed-off-by: Akihiko Odaki --- ui/cocoa.m | 9 ++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/ui/cocoa.m b/ui/cocoa.m index fa879d7dcd4b..c5b3c28000ff 100644 --- a/ui/cocoa.m +++ b/ui/cocoa.m @@ -522,7 +522,10 @@ - (void) resizeWindow [[self window] setContentAspectRatio:NSMakeSize(screen.width, screen.height)]; if (!([[self window] styleMask] & NSWindowStyleMaskResizable)) { -[[self window] setContentSize:NSMakeSize(screen.width, screen.height)]; +CGFloat width = screen.width / [[self window] backingScaleFactor]; +CGFloat height = screen.height / [[self window] backingScaleFactor]; + +[[self window] setContentSize:NSMakeSize(width, height)]; [[self window] center]; } else if ([[self window] styleMask] & NSWindowStyleMaskFullScreen) { [[self window] setContentSize:[self screenSafeAreaSize]]; @@ -575,8 +578,8 @@ - (void) updateUIInfoLocked info.xoff = 0; info.yoff = 0; -info.width = frameSize.width; -info.height = frameSize.height; +info.width = frameSize.width * [[self window] backingScaleFactor]; +info.height = frameSize.height * [[self window] backingScaleFactor]; dpy_set_ui_info(dcl.con, &info, TRUE); } --- base-commit: ba49d760eb04630e7b15f423ebecf6c871b8f77b change-id: 20240318-zoom-df4d6834e56b Best regards, -- Akihiko Odaki
Re: [PATCH for-9.0 v3] vdpa-dev: Fix initialisation order to restore VDUSE compatibility
On Mon, Mar 18, 2024 at 12:31:26PM +0800, Jason Wang wrote: > On Fri, Mar 15, 2024 at 11:59 PM Kevin Wolf wrote: > > > > VDUSE requires that virtqueues are first enabled before the DRIVER_OK > > status flag is set; with the current API of the kernel module, it is > > impossible to enable the opposite order in our block export code because > > userspace is not notified when a virtqueue is enabled. > > > > This requirement also mathces the normal initialisation order as done by > > the generic vhost code in QEMU. However, commit 6c482547 accidentally > > changed the order for vdpa-dev and broke access to VDUSE devices with > > this. > > > > This changes vdpa-dev to use the normal order again and use the standard > > vhost callback .vhost_set_vring_enable for this. VDUSE devices can be > > used with vdpa-dev again after this fix. > > > > vhost_net intentionally avoided enabling the vrings for vdpa and does > > this manually later while it does enable them for other vhost backends. > > Reflect this in the vhost_net code and return early for vdpa, so that > > the behaviour doesn't change for this device. > > > > Cc: qemu-sta...@nongnu.org > > Fixes: 6c4825476a4351530bcac17abab72295b75ffe98 > > Signed-off-by: Kevin Wolf > > --- > > v2: > > - Actually make use of the @enable parameter > > - Change vhost_net to preserve the current behaviour > > > > v3: > > - Updated trace point [Stefano] > > - Fixed typo in comment [Stefano] > > > > hw/net/vhost_net.c | 10 ++ > > hw/virtio/vdpa-dev.c | 5 + > > hw/virtio/vhost-vdpa.c | 29 ++--- > > hw/virtio/vhost.c | 8 +++- > > hw/virtio/trace-events | 2 +- > > 5 files changed, 45 insertions(+), 9 deletions(-) > > > > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c > > index e8e1661646..fd1a93701a 100644 > > --- a/hw/net/vhost_net.c > > +++ b/hw/net/vhost_net.c > > @@ -541,6 +541,16 @@ int vhost_set_vring_enable(NetClientState *nc, int > > enable) > > VHostNetState *net = get_vhost_net(nc); > > const VhostOps *vhost_ops = net->dev.vhost_ops; > > > > +/* > > + * vhost-vdpa network devices need to enable dataplane virtqueues after > > + * DRIVER_OK, so they can recover device state before starting > > dataplane. > > + * Because of that, we don't enable virtqueues here and leave it to > > + * net/vhost-vdpa.c. > > + */ > > +if (nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) { > > +return 0; > > +} > > I think we need some inputs from Eugenio, this is only needed for > shadow virtqueue during live migration but not other cases. > > Thanks Yes I think we had a backend flag for this, right? Eugenio can you comment please?
Re: [PATCH for-9.0 v3] vdpa-dev: Fix initialisation order to restore VDUSE compatibility
On Fri, Mar 15, 2024 at 04:59:49PM +0100, Kevin Wolf wrote: > VDUSE requires that virtqueues are first enabled before the DRIVER_OK > status flag is set; with the current API of the kernel module, it is > impossible to enable the opposite order in our block export code because > userspace is not notified when a virtqueue is enabled. > > This requirement also mathces the normal initialisation order as done by > the generic vhost code in QEMU. However, commit 6c482547 accidentally > changed the order for vdpa-dev and broke access to VDUSE devices with > this. > > This changes vdpa-dev to use the normal order again and use the standard > vhost callback .vhost_set_vring_enable for this. VDUSE devices can be > used with vdpa-dev again after this fix. > > vhost_net intentionally avoided enabling the vrings for vdpa and does > this manually later while it does enable them for other vhost backends. > Reflect this in the vhost_net code and return early for vdpa, so that > the behaviour doesn't change for this device. > > Cc: qemu-sta...@nongnu.org > Fixes: 6c4825476a4351530bcac17abab72295b75ffe98 Wrong format for the fixes tag. Fixes: 6c4825476a ("vdpa: move vhost_vdpa_set_vring_ready to the caller") is the right one. > Signed-off-by: Kevin Wolf > --- > v2: > - Actually make use of the @enable parameter > - Change vhost_net to preserve the current behaviour > > v3: > - Updated trace point [Stefano] > - Fixed typo in comment [Stefano] > > hw/net/vhost_net.c | 10 ++ > hw/virtio/vdpa-dev.c | 5 + > hw/virtio/vhost-vdpa.c | 29 ++--- > hw/virtio/vhost.c | 8 +++- > hw/virtio/trace-events | 2 +- > 5 files changed, 45 insertions(+), 9 deletions(-) > > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c > index e8e1661646..fd1a93701a 100644 > --- a/hw/net/vhost_net.c > +++ b/hw/net/vhost_net.c > @@ -541,6 +541,16 @@ int vhost_set_vring_enable(NetClientState *nc, int > enable) > VHostNetState *net = get_vhost_net(nc); > const VhostOps *vhost_ops = net->dev.vhost_ops; > > +/* > + * vhost-vdpa network devices need to enable dataplane virtqueues after > + * DRIVER_OK, so they can recover device state before starting dataplane. > + * Because of that, we don't enable virtqueues here and leave it to > + * net/vhost-vdpa.c. > + */ > +if (nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) { > +return 0; > +} > + > nc->vring_enable = enable; > > if (vhost_ops && vhost_ops->vhost_set_vring_enable) { > diff --git a/hw/virtio/vdpa-dev.c b/hw/virtio/vdpa-dev.c > index eb9ecea83b..13e87f06f6 100644 > --- a/hw/virtio/vdpa-dev.c > +++ b/hw/virtio/vdpa-dev.c > @@ -253,14 +253,11 @@ static int vhost_vdpa_device_start(VirtIODevice *vdev, > Error **errp) > > s->dev.acked_features = vdev->guest_features; > > -ret = vhost_dev_start(&s->dev, vdev, false); > +ret = vhost_dev_start(&s->dev, vdev, true); > if (ret < 0) { > error_setg_errno(errp, -ret, "Error starting vhost"); > goto err_guest_notifiers; > } > -for (i = 0; i < s->dev.nvqs; ++i) { > -vhost_vdpa_set_vring_ready(&s->vdpa, i); > -} > s->started = true; > > /* > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c > index ddae494ca8..31453466af 100644 > --- a/hw/virtio/vhost-vdpa.c > +++ b/hw/virtio/vhost-vdpa.c > @@ -886,19 +886,41 @@ static int vhost_vdpa_get_vq_index(struct vhost_dev > *dev, int idx) > return idx; > } > > -int vhost_vdpa_set_vring_ready(struct vhost_vdpa *v, unsigned idx) > +static int vhost_vdpa_set_vring_enable_one(struct vhost_vdpa *v, unsigned > idx, > + int enable) > { > struct vhost_dev *dev = v->dev; > struct vhost_vring_state state = { > .index = idx, > -.num = 1, > +.num = enable, > }; > int r = vhost_vdpa_call(dev, VHOST_VDPA_SET_VRING_ENABLE, &state); > > -trace_vhost_vdpa_set_vring_ready(dev, idx, r); > +trace_vhost_vdpa_set_vring_enable_one(dev, idx, enable, r); > return r; > } > > +static int vhost_vdpa_set_vring_enable(struct vhost_dev *dev, int enable) > +{ > +struct vhost_vdpa *v = dev->opaque; > +unsigned int i; > +int ret; > + > +for (i = 0; i < dev->nvqs; ++i) { > +ret = vhost_vdpa_set_vring_enable_one(v, i, enable); > +if (ret < 0) { > +return ret; > +} > +} > + > +return 0; > +} > + > +int vhost_vdpa_set_vring_ready(struct vhost_vdpa *v, unsigned idx) > +{ > +return vhost_vdpa_set_vring_enable_one(v, idx, 1); > +} > + > static int vhost_vdpa_set_config_call(struct vhost_dev *dev, > int fd) > { > @@ -1514,6 +1536,7 @@ const VhostOps vdpa_ops = { > .vhost_set_features = vhost_vdpa_set_features, > .vhost_reset_device = vhost_vdpa_reset_device, > .vhost_get_vq_index = vhost_vdpa_get_vq_index, >
Re: [PATCH 2/4] ui/vnc: Do not use console_select()
On 2024/03/18 17:21, Marc-André Lureau wrote: Hi On Mon, Mar 18, 2024 at 11:58 AM Akihiko Odaki wrote: 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 --- 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..a703f7466499 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_first_graphic_console(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..6bf02a23414c 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_first_graphic_console(void) +{ +QemuConsole *con; + +QTAILQ_FOREACH(con, &consoles, next) { +if (QEMU_IS_GRAPHIC_CONSOLE(con)) { +return con; +} +} +return NULL; +} + 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..94564b196ba8 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_first_graphic_console(); why this change here? console_select() is to change the console that is used when DisplayChangeListener::con is NULL. console_select() is no longer called so DisplayChangeListener::con must not be NULL. otherwise, lgtm } if (con != vd->dcl.con) { -- 2.44.0
Re: [PATCH for 9.0 v15 05/10] target/riscv: always clear vstart for ldst_whole insns
On Fri, Mar 15, 2024 at 3:57 AM Daniel Henrique Barboza wrote: > > Commit 8ff8ac6329 added a conditional to guard the vext_ldst_whole() > helper if vstart >= evl. But by skipping the helper we're also not > setting vstart = 0 at the end of the insns, which is incorrect. > > We'll move the conditional to vext_ldst_whole(), following in line with > the removal of all brconds vstart >= vl that the next patch will do. The > idea is to make the helpers responsible for their own vstart management. > > Fix ldst_whole isns by: > > - remove the brcond that skips the helper if vstart is >= evl; > > - vext_ldst_whole() now does an early exit with the same check, where > evl = (vlenb * nf) >> log2_esz, but the early exit will also clear > vstart. > > The 'width' param is now unneeded in ldst_whole_trans() and is also > removed. It was used for the evl calculation for the brcond and has no > other use now. The 'width' is reflected in vext_ldst_whole() via > log2_esz, which is encoded by GEN_VEXT_LD_WHOLE() as > "ctzl(sizeof(ETYPE))". > > Suggested-by: Max Chou > Fixes: 8ff8ac6329 ("target/riscv: rvv: Add missing early exit condition for > whole register load/store") > Signed-off-by: Daniel Henrique Barboza Reviewed-by: Alistair Francis Alistair > --- > target/riscv/insn_trans/trans_rvv.c.inc | 52 +++-- > target/riscv/vector_helper.c| 5 +++ > 2 files changed, 28 insertions(+), 29 deletions(-) > > diff --git a/target/riscv/insn_trans/trans_rvv.c.inc > b/target/riscv/insn_trans/trans_rvv.c.inc > index 52c26a7834..1366445e1f 100644 > --- a/target/riscv/insn_trans/trans_rvv.c.inc > +++ b/target/riscv/insn_trans/trans_rvv.c.inc > @@ -1097,13 +1097,9 @@ GEN_VEXT_TRANS(vle64ff_v, MO_64, r2nfvm, ldff_op, > ld_us_check) > typedef void gen_helper_ldst_whole(TCGv_ptr, TCGv, TCGv_env, TCGv_i32); > > static bool ldst_whole_trans(uint32_t vd, uint32_t rs1, uint32_t nf, > - uint32_t width, gen_helper_ldst_whole *fn, > + gen_helper_ldst_whole *fn, > DisasContext *s) > { > -uint32_t evl = s->cfg_ptr->vlenb * nf / width; > -TCGLabel *over = gen_new_label(); > -tcg_gen_brcondi_tl(TCG_COND_GEU, cpu_vstart, evl, over); > - > TCGv_ptr dest; > TCGv base; > TCGv_i32 desc; > @@ -1120,8 +1116,6 @@ static bool ldst_whole_trans(uint32_t vd, uint32_t rs1, > uint32_t nf, > > fn(dest, base, tcg_env, desc); > > -gen_set_label(over); > - > return true; > } > > @@ -1129,42 +1123,42 @@ static bool ldst_whole_trans(uint32_t vd, uint32_t > rs1, uint32_t nf, > * load and store whole register instructions ignore vtype and vl setting. > * Thus, we don't need to check vill bit. (Section 7.9) > */ > -#define GEN_LDST_WHOLE_TRANS(NAME, ARG_NF, WIDTH) \ > +#define GEN_LDST_WHOLE_TRANS(NAME, ARG_NF)\ > static bool trans_##NAME(DisasContext *s, arg_##NAME * a) \ > { \ > if (require_rvv(s) && \ > QEMU_IS_ALIGNED(a->rd, ARG_NF)) { \ > -return ldst_whole_trans(a->rd, a->rs1, ARG_NF, WIDTH, \ > +return ldst_whole_trans(a->rd, a->rs1, ARG_NF,\ > gen_helper_##NAME, s);\ > } \ > return false; \ > } > > -GEN_LDST_WHOLE_TRANS(vl1re8_v, 1, 1) > -GEN_LDST_WHOLE_TRANS(vl1re16_v, 1, 2) > -GEN_LDST_WHOLE_TRANS(vl1re32_v, 1, 4) > -GEN_LDST_WHOLE_TRANS(vl1re64_v, 1, 8) > -GEN_LDST_WHOLE_TRANS(vl2re8_v, 2, 1) > -GEN_LDST_WHOLE_TRANS(vl2re16_v, 2, 2) > -GEN_LDST_WHOLE_TRANS(vl2re32_v, 2, 4) > -GEN_LDST_WHOLE_TRANS(vl2re64_v, 2, 8) > -GEN_LDST_WHOLE_TRANS(vl4re8_v, 4, 1) > -GEN_LDST_WHOLE_TRANS(vl4re16_v, 4, 2) > -GEN_LDST_WHOLE_TRANS(vl4re32_v, 4, 4) > -GEN_LDST_WHOLE_TRANS(vl4re64_v, 4, 8) > -GEN_LDST_WHOLE_TRANS(vl8re8_v, 8, 1) > -GEN_LDST_WHOLE_TRANS(vl8re16_v, 8, 2) > -GEN_LDST_WHOLE_TRANS(vl8re32_v, 8, 4) > -GEN_LDST_WHOLE_TRANS(vl8re64_v, 8, 8) > +GEN_LDST_WHOLE_TRANS(vl1re8_v, 1) > +GEN_LDST_WHOLE_TRANS(vl1re16_v, 1) > +GEN_LDST_WHOLE_TRANS(vl1re32_v, 1) > +GEN_LDST_WHOLE_TRANS(vl1re64_v, 1) > +GEN_LDST_WHOLE_TRANS(vl2re8_v, 2) > +GEN_LDST_WHOLE_TRANS(vl2re16_v, 2) > +GEN_LDST_WHOLE_TRANS(vl2re32_v, 2) > +GEN_LDST_WHOLE_TRANS(vl2re64_v, 2) > +GEN_LDST_WHOLE_TRANS(vl4re8_v, 4) > +GEN_LDST_WHOLE_TRANS(vl4re16_v, 4) > +GEN_LDST_WHOLE_TRANS(vl4re32_v, 4) > +GEN_LDST_WHOLE_TRANS(vl4re64_v, 4) > +GEN_LDST_WHOLE_TRANS(vl8re8_v, 8) > +GEN_LDST_WHOLE_TRANS(vl8re16_v, 8) > +GEN_LDST_WHOLE_TRANS(vl8re32_v, 8) > +GEN_LDST_WHOLE_TRANS(vl8re64_v, 8) > > /* > * The vector whole register store instructions are encoded similar to > * un
Re: [PATCH v5 25/25] qapi: Dumb down QAPISchema.lookup_entity()
Markus Armbruster writes: > QAPISchema.lookup_entity() takes an optional type argument, a subtype > of QAPISchemaDefinition, and returns that type or None. Callers can > use this to save themselves an isinstance() test. > > The only remaining user of this convenience feature is .lookup_type(). > But we don't actually save anything anymore there: we still the we still need the > isinstance() to help mypy over the hump. > > Drop the .lookup_entity() argument, and adjust .lookup_type(). > > Signed-off-by: Markus Armbruster
Re: [PATCH 2/4] ui/vnc: Do not use console_select()
Hi On Mon, Mar 18, 2024 at 1:05 PM Akihiko Odaki wrote: > > On 2024/03/18 17:21, Marc-André Lureau wrote: > > Hi > > > > On Mon, Mar 18, 2024 at 11:58 AM Akihiko Odaki > > wrote: > >> > >> 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 > >> --- > >> 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..a703f7466499 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_first_graphic_console(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..6bf02a23414c 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_first_graphic_console(void) > >> +{ > >> +QemuConsole *con; > >> + > >> +QTAILQ_FOREACH(con, &consoles, next) { > >> +if (QEMU_IS_GRAPHIC_CONSOLE(con)) { > >> +return con; > >> +} > >> +} > >> +return NULL; > >> +} > >> + > >> 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..94564b196ba8 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_conso
[RFC PATCH v8 01/23] target/arm: Handle HCR_EL2 accesses for bits introduced with FEAT_NMI
FEAT_NMI defines another three new bits in HCRX_EL2: TALLINT, HCRX_VINMI and HCRX_VFNMI. When the feature is enabled, allow these bits to be written in HCRX_EL2. Signed-off-by: Jinjie Ruan Reviewed-by: Richard Henderson --- v4: - Update the comment for FEAT_NMI in hcrx_write(). - Update the commit message, s/thress/three/g. v3: - Add Reviewed-by. - Add HCRX_VINMI and HCRX_VFNMI support in HCRX_EL2. - Upate the commit messsage. --- target/arm/cpu-features.h | 5 + target/arm/helper.c | 5 + 2 files changed, 10 insertions(+) diff --git a/target/arm/cpu-features.h b/target/arm/cpu-features.h index e5758d9fbc..b300d0446d 100644 --- a/target/arm/cpu-features.h +++ b/target/arm/cpu-features.h @@ -681,6 +681,11 @@ static inline bool isar_feature_aa64_sme(const ARMISARegisters *id) return FIELD_EX64(id->id_aa64pfr1, ID_AA64PFR1, SME) != 0; } +static inline bool isar_feature_aa64_nmi(const ARMISARegisters *id) +{ +return FIELD_EX64(id->id_aa64pfr1, ID_AA64PFR1, NMI) != 0; +} + static inline bool isar_feature_aa64_tgran4_lpa2(const ARMISARegisters *id) { return FIELD_SEX64(id->id_aa64mmfr0, ID_AA64MMFR0, TGRAN4) >= 1; diff --git a/target/arm/helper.c b/target/arm/helper.c index 3f3a5b55d4..b19a0178ce 100644 --- a/target/arm/helper.c +++ b/target/arm/helper.c @@ -6190,6 +6190,11 @@ static void hcrx_write(CPUARMState *env, const ARMCPRegInfo *ri, valid_mask |= HCRX_MSCEN | HCRX_MCE2; } +/* FEAT_NMI adds TALLINT, VINMI and VFNMI */ +if (cpu_isar_feature(aa64_nmi, env_archcpu(env))) { +valid_mask |= HCRX_TALLINT | HCRX_VINMI | HCRX_VFNMI; +} + /* Clear RES0 bits. */ env->cp15.hcrx_el2 = value & valid_mask; } -- 2.34.1
[RFC PATCH v8 02/23] target/arm: Add PSTATE.ALLINT
When PSTATE.ALLINT is set, an IRQ or FIQ interrupt that is targeted to ELx, with or without superpriority is masked. As Richard suggested, place ALLINT bit in PSTATE in env->pstate. With the change to pstate_read/write, exception entry and return are automatically handled. Signed-off-by: Jinjie Ruan Reviewed-by: Richard Henderson --- v5: - Remove the ALLINT comment, as it is covered by "all other bits". - Add Reviewed-by. v4: - Keep PSTATE.ALLINT in env->pstate but not env->allint. - Update the commit message. v3: - Remove ALLINT dump in aarch64_cpu_dump_state(). - Update the commit message. --- target/arm/cpu.h | 1 + 1 file changed, 1 insertion(+) diff --git a/target/arm/cpu.h b/target/arm/cpu.h index bc0c84873f..de740d223f 100644 --- a/target/arm/cpu.h +++ b/target/arm/cpu.h @@ -1430,6 +1430,7 @@ void pmu_init(ARMCPU *cpu); #define PSTATE_D (1U << 9) #define PSTATE_BTYPE (3U << 10) #define PSTATE_SSBS (1U << 12) +#define PSTATE_ALLINT (1U << 13) #define PSTATE_IL (1U << 20) #define PSTATE_SS (1U << 21) #define PSTATE_PAN (1U << 22) -- 2.34.1
[RFC PATCH v8 06/23] target/arm: Add support for Non-maskable Interrupt
This only implements the external delivery method via the GICv3. Signed-off-by: Jinjie Ruan Reviewed-by: Richard Henderson --- v8: - Fix the rcu stall after sending a VNMI in qemu VM. v7: - Add Reviewed-by. v6: - env->cp15.hcr_el2 -> arm_hcr_el2_eff(). - env->cp15.hcrx_el2 -> arm_hcrx_el2_eff(). - Not include VF && VFNMI in CPU_INTERRUPT_VNMI. v4: - Accept NMI unconditionally for arm_cpu_has_work() but add comment. - Change from & to && for EXCP_IRQ or EXCP_FIQ. - Refator nmi mask in arm_excp_unmasked(). - Also handle VNMI in arm_cpu_exec_interrupt() and arm_cpu_set_irq(). - Rename virtual to Virtual. v3: - Not include CPU_INTERRUPT_NMI when FEAT_NMI not enabled - Add ARM_CPU_VNMI. - Refator nmi mask in arm_excp_unmasked(). - Test SCTLR_ELx.NMI for ALLINT mask for NMI. --- target/arm/cpu-qom.h | 4 +- target/arm/cpu.c | 85 +++--- target/arm/cpu.h | 4 ++ target/arm/helper.c| 2 + target/arm/internals.h | 9 + 5 files changed, 97 insertions(+), 7 deletions(-) diff --git a/target/arm/cpu-qom.h b/target/arm/cpu-qom.h index 8e032691db..e0c9e18036 100644 --- a/target/arm/cpu-qom.h +++ b/target/arm/cpu-qom.h @@ -36,11 +36,13 @@ DECLARE_CLASS_CHECKERS(AArch64CPUClass, AARCH64_CPU, #define ARM_CPU_TYPE_SUFFIX "-" TYPE_ARM_CPU #define ARM_CPU_TYPE_NAME(name) (name ARM_CPU_TYPE_SUFFIX) -/* Meanings of the ARMCPU object's four inbound GPIO lines */ +/* Meanings of the ARMCPU object's six inbound GPIO lines */ #define ARM_CPU_IRQ 0 #define ARM_CPU_FIQ 1 #define ARM_CPU_VIRQ 2 #define ARM_CPU_VFIQ 3 +#define ARM_CPU_NMI 4 +#define ARM_CPU_VNMI 5 /* For M profile, some registers are banked secure vs non-secure; * these are represented as a 2-element array where the first element diff --git a/target/arm/cpu.c b/target/arm/cpu.c index ab8d007a86..f8d931a917 100644 --- a/target/arm/cpu.c +++ b/target/arm/cpu.c @@ -122,6 +122,13 @@ void arm_restore_state_to_opc(CPUState *cs, } #endif /* CONFIG_TCG */ +/* + * With SCTLR_ELx.NMI == 0, IRQ with Superpriority is masked identically with + * IRQ without Superpriority. Moreover, if the GIC is configured so that + * FEAT_GICv3_NMI is only set if FEAT_NMI is set, then we won't ever see + * CPU_INTERRUPT_*NMI anyway. So we might as well accept NMI here + * unconditionally. + */ static bool arm_cpu_has_work(CPUState *cs) { ARMCPU *cpu = ARM_CPU(cs); @@ -129,6 +136,7 @@ static bool arm_cpu_has_work(CPUState *cs) return (cpu->power_state != PSCI_OFF) && cs->interrupt_request & (CPU_INTERRUPT_FIQ | CPU_INTERRUPT_HARD + | CPU_INTERRUPT_NMI | CPU_INTERRUPT_VNMI | CPU_INTERRUPT_VFIQ | CPU_INTERRUPT_VIRQ | CPU_INTERRUPT_VSERR | CPU_INTERRUPT_EXITTB); } @@ -668,6 +676,7 @@ static inline bool arm_excp_unmasked(CPUState *cs, unsigned int excp_idx, CPUARMState *env = cpu_env(cs); bool pstate_unmasked; bool unmasked = false; +bool allIntMask = false; /* * Don't take exceptions if they target a lower EL. @@ -678,13 +687,31 @@ static inline bool arm_excp_unmasked(CPUState *cs, unsigned int excp_idx, return false; } +if (cpu_isar_feature(aa64_nmi, env_archcpu(env)) && +env->cp15.sctlr_el[target_el] & SCTLR_NMI && cur_el == target_el) { +allIntMask = env->pstate & PSTATE_ALLINT || + ((env->cp15.sctlr_el[target_el] & SCTLR_SPINTMASK) && + (env->pstate & PSTATE_SP)); +} + switch (excp_idx) { +case EXCP_NMI: +pstate_unmasked = !allIntMask; +break; + +case EXCP_VNMI: +if ((!(hcr_el2 & HCR_IMO) && !(hcr_el2 & HCR_FMO)) || + (hcr_el2 & HCR_TGE)) { +/* VNMIs(VIRQs or VFIQs) are only taken when hypervized. */ +return false; +} +return !allIntMask; case EXCP_FIQ: -pstate_unmasked = !(env->daif & PSTATE_F); +pstate_unmasked = (!(env->daif & PSTATE_F)) && (!allIntMask); break; case EXCP_IRQ: -pstate_unmasked = !(env->daif & PSTATE_I); +pstate_unmasked = (!(env->daif & PSTATE_I)) && (!allIntMask); break; case EXCP_VFIQ: @@ -692,13 +719,13 @@ static inline bool arm_excp_unmasked(CPUState *cs, unsigned int excp_idx, /* VFIQs are only taken when hypervized. */ return false; } -return !(env->daif & PSTATE_F); +return !(env->daif & PSTATE_F) && (!allIntMask); case EXCP_VIRQ: if (!(hcr_el2 & HCR_IMO) || (hcr_el2 & HCR_TGE)) { /* VIRQs are only taken when hypervized. */ return false; } -return !(env->daif & PSTATE_I); +return !(env->daif & PSTATE_I) && (!allIntMask); case EXCP_VSERR: if (!(hcr_el2 & HCR_AMO) || (hcr_el2 & HCR_TGE)) { /* VIRQs are only taken when hypervized. */ @@ -804,6 +831,24 @@ static bool arm_cpu_exec_interrupt(CPUState *cs, int interrupt_requ
[RFC PATCH v8 00/23] target/arm: Implement FEAT_NMI and FEAT_GICv3_NMI
This patch set implements FEAT_NMI and FEAT_GICv3_NMI for armv8. These introduce support for a new category of interrupts in the architecture which we can use to provide NMI like functionality. There are two modes for using this FEAT_NMI. When PSTATE.ALLINT or PSTATE.SP & SCTLR_ELx.SCTLR_SPINTMASK is set, any entry to ELx causes all interrupts including those with superpriority to be masked on entry to ELn until the mask is explicitly removed by software or hardware. PSTATE.ALLINT can be managed by software using the new register control ALLINT.ALLINT. Independent controls are provided for this feature at each EL, usage at EL1 should not disrupt EL2 or EL3. I have tested it with the following linux patches which try to support FEAT_NMI in linux kernel: https://lore.kernel.org/linux-arm-kernel/Y4sH5qX5bK9xfEBp@lpieralisi/T/#mb4ba4a2c045bf72c10c2202c1dd1b82d3240dc88 In the test, SGI, PPI and SPI interrupts can all be set to have super priority to be converted to a hardware NMI interrupt. The SGI is tested with kernel IPI as NMI framework, softlockup, hardlockup and kgdb test cases, and the PPI interrupt is tested with "perf top" command with hardware NMI enabled, and the SPI interrupt is tested with a custom test module, in which NMI interrupts can be received and sent normally. And the Virtual NMI(VNMI) SGI, PPI and SPI interrupt has also been tested by accessing the GICD_INMIR, GICR_INMIR0, GICR_ISPENDR0 and GICD_ISPENDR registers with devmem command. +-+ | Distributor | +-+ SPI | NMI | NMI \/\/ ++ +---+ | Redist | | Redist| ++ +---+ SGI | NMI PPI | NMI \/\/ +-+ +---+ |CPU Interface| ... | CPU Interface | +-+ +---+ | NMI | NMI \/\/ +-+ +-+ | PE | | PE | +-+ +-+ Changes in v8: - Fix the rcu stall after sending a VNMI in qemu VM. - Fix an unexpected interrupt bug when sending VNMI by running qemu VM. - Test the VNMI interrupt. - Update the commit message. - Add Reviewed-by. Changes in v7: - env->cp15.hcrx_el2 -> arm_hcrx_el2_eff(). - Reorder the irqbetter() code for clarity. - Eliminate the has_superprio local variable for gicv3_get_priority(). - false -> cs->hpplpi.superprio in gicv3_redist_update_noirqset(). - 0x0 -> false in arm_gicv3_common_reset_hold(). - Clear superprio in several places for hppi, hpplpi and hppvlpi. - Add Reviewed-by. Changes in v6: - Fix DISAS_TOO_MANY to DISAS_UPDATE_EXIT for ALLINT MSR (immediate). - Verify that HCR_EL2.VF is set before checking VFNMI. - env->cp15.hcr_el2 -> arm_hcr_el2_eff(). - env->cp15.hcrx_el2 -> arm_hcrx_el2_eff(). - Not combine VFNMI with CPU_INTERRUPT_VNMI. - Implement icv_nmiar1_read(). - Put the "extract superprio info" code into gicv3_get_priority(). - Update the comment in irqbetter(). - Reset the cs->hppi.superprio to 0x0. - Set hppi.superprio to false for LPI. - Add Reviewed-by. Changes in v5: - Remove the comment for ALLINT in cpu.h. - Merge allint_check() to msr_i_allint to clear the ALLINT MSR (immediate) implementation. - Rename msr_i_allint() to msr_set_allint_el1() to make it clearer. - Drop the & 1 in trans_MSR_i_ALLINT(). - Add Reviewed-by. Changes in v4: - Handle VNMI within the CPU and the GIC. - Keep PSTATE.ALLINT in env->pstate but not env->allint. - Fix the ALLINT MSR (immediate) decodetree implementation. - Accept NMI unconditionally for arm_cpu_has_work() but add comment. - Improve nmi mask in arm_excp_unmasked(). - Make the GICR_INMIR0 and GICD_INMIR implementation more clearer. - Improve ICC_NMIAR1_EL1 implementation - Extract gicv3_get_priority() to avoid priority code repetition. - Add Reviewed-by. Changes in v3: - Remove the FIQ NMI. - Adjust the patches Sequence. - Reomve the patch "Set pstate.ALLINT in arm_cpu_reset_hold". - Check whether support FEAT_NMI and FEAT_GICv3 for FEAT_GICv3_NMI. - With CPU_INTERRUPT_NMI, both CPSR_I and ISR_IS must be set. - Not include NMI logic when FEAT_NMI or SCTLR_ELx.NMI not enabled. - Refator nmi mask in arm_excp_unmasked(). - Add VNMI definitions, add HCRX_VINMI and HCRX_VFNMI support in HCRX_EL2. - Add Reviewed-by and Acked-by. - Update the commit message. Changes in v2: - Break up the patches so that each one does only one thing. - Remove the command line option and just implement it in "max" cpu. Jinjie Ruan (23): target/arm: Hand
[RFC PATCH v8 21/23] hw/intc/arm_gicv3: Report the VNMI interrupt
In vCPU Interface, if the vIRQ has the superpriority property, report vNMI to the corresponding vPE. Signed-off-by: Jinjie Ruan Reviewed-by: Richard Henderson --- v6: - Add Reviewed-by. --- hw/intc/arm_gicv3_cpuif.c | 14 -- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c index 29142fcf8d..0bea6cb020 100644 --- a/hw/intc/arm_gicv3_cpuif.c +++ b/hw/intc/arm_gicv3_cpuif.c @@ -465,6 +465,7 @@ void gicv3_cpuif_virt_irq_fiq_update(GICv3CPUState *cs) int idx; int irqlevel = 0; int fiqlevel = 0; +int nmilevel = 0; idx = hppvi_index(cs); trace_gicv3_cpuif_virt_update(gicv3_redist_affid(cs), idx, @@ -482,9 +483,17 @@ void gicv3_cpuif_virt_irq_fiq_update(GICv3CPUState *cs) uint64_t lr = cs->ich_lr_el2[idx]; if (icv_hppi_can_preempt(cs, lr)) { -/* Virtual interrupts are simple: G0 are always FIQ, and G1 IRQ */ +/* + * Virtual interrupts are simple: G0 are always FIQ, and G1 are + * IRQ or NMI which depends on the ICH_LR_EL2.NMI to have + * non-maskable property. + */ if (lr & ICH_LR_EL2_GROUP) { -irqlevel = 1; +if (cs->gic->nmi_support && (lr & ICH_LR_EL2_NMI)) { +nmilevel = 1; +} else { +irqlevel = 1; +} } else { fiqlevel = 1; } @@ -494,6 +503,7 @@ void gicv3_cpuif_virt_irq_fiq_update(GICv3CPUState *cs) trace_gicv3_cpuif_virt_set_irqs(gicv3_redist_affid(cs), fiqlevel, irqlevel); qemu_set_irq(cs->parent_vfiq, fiqlevel); qemu_set_irq(cs->parent_virq, irqlevel); +qemu_set_irq(cs->parent_vnmi, nmilevel); } static void gicv3_cpuif_virt_update(GICv3CPUState *cs) -- 2.34.1
[RFC PATCH v8 07/23] target/arm: Add support for NMI in arm_phys_excp_target_el()
According to Arm GIC section 4.6.3 Interrupt superpriority, the interrupt with superpriority is always IRQ, never FIQ, so handle NMI same as IRQ in arm_phys_excp_target_el(). Signed-off-by: Jinjie Ruan Reviewed-by: Richard Henderson --- v4: - Add Reviewed-by. v3: - Remove nmi_is_irq flag in CPUARMState. - Handle NMI same as IRQ in arm_phys_excp_target_el(). --- target/arm/helper.c | 1 + 1 file changed, 1 insertion(+) diff --git a/target/arm/helper.c b/target/arm/helper.c index 875a7fa8da..5c637f3413 100644 --- a/target/arm/helper.c +++ b/target/arm/helper.c @@ -10735,6 +10735,7 @@ uint32_t arm_phys_excp_target_el(CPUState *cs, uint32_t excp_idx, hcr_el2 = arm_hcr_el2_eff(env); switch (excp_idx) { case EXCP_IRQ: +case EXCP_NMI: scr = ((env->cp15.scr_el3 & SCR_IRQ) == SCR_IRQ); hcr = hcr_el2 & HCR_IMO; break; -- 2.34.1
[RFC PATCH v8 13/23] hw/intc/arm_gicv3: Add irq superpriority information
A SPI, PPI or SGI interrupt can have a superpriority property. So maintain superpriority information in PendingIrq and GICR/GICD. Signed-off-by: Jinjie Ruan Acked-by: Richard Henderson --- v3: - Place this ahead of implement GICR_INMIR. - Add Acked-by. --- include/hw/intc/arm_gicv3_common.h | 4 1 file changed, 4 insertions(+) diff --git a/include/hw/intc/arm_gicv3_common.h b/include/hw/intc/arm_gicv3_common.h index 7324c7d983..df4380141d 100644 --- a/include/hw/intc/arm_gicv3_common.h +++ b/include/hw/intc/arm_gicv3_common.h @@ -146,6 +146,7 @@ typedef struct { int irq; uint8_t prio; int grp; +bool superprio; } PendingIrq; struct GICv3CPUState { @@ -172,6 +173,7 @@ struct GICv3CPUState { uint32_t gicr_ienabler0; uint32_t gicr_ipendr0; uint32_t gicr_iactiver0; +uint32_t gicr_isuperprio; uint32_t edge_trigger; /* ICFGR0 and ICFGR1 even bits */ uint32_t gicr_igrpmodr0; uint32_t gicr_nsacr; @@ -274,6 +276,7 @@ struct GICv3State { GIC_DECLARE_BITMAP(active); /* GICD_ISACTIVER */ GIC_DECLARE_BITMAP(level);/* Current level */ GIC_DECLARE_BITMAP(edge_trigger); /* GICD_ICFGR even bits */ +GIC_DECLARE_BITMAP(superprio);/* GICD_INMIR */ uint8_t gicd_ipriority[GICV3_MAXIRQ]; uint64_t gicd_irouter[GICV3_MAXIRQ]; /* Cached information: pointer to the cpu i/f for the CPUs specified @@ -313,6 +316,7 @@ GICV3_BITMAP_ACCESSORS(pending) GICV3_BITMAP_ACCESSORS(active) GICV3_BITMAP_ACCESSORS(level) GICV3_BITMAP_ACCESSORS(edge_trigger) +GICV3_BITMAP_ACCESSORS(superprio) #define TYPE_ARM_GICV3_COMMON "arm-gicv3-common" typedef struct ARMGICv3CommonClass ARMGICv3CommonClass; -- 2.34.1
[RFC PATCH v8 17/23] hw/intc/arm_gicv3: Add NMI handling CPU interface registers
Add the NMIAR CPU interface registers which deal with acknowledging NMI. When introduce NMI interrupt, there are some updates to the semantics for the register ICC_IAR1_EL1 and ICC_HPPIR1_EL1. For ICC_IAR1_EL1 register, it should return 1022 if the intid has super priority. And for ICC_NMIAR1_EL1 register, it should return 1023 if the intid do not have super priority. Howerever, these are not necessary for ICC_HPPIR1_EL1 register. Signed-off-by: Jinjie Ruan Reviewed-by: Richard Henderson --- v7: - Add Reviewed-by. v4: - Define ICC_NMIAR1_EL1 only if FEAT_GICv3_NMI is implemented. - Check sctrl_elx.SCTLR_NMI to return 1022 for icc_iar1_read(). - Add gicv3_icc_nmiar1_read() trace event. - Do not check icc_hppi_can_preempt() for icc_nmiar1_read(). - Add icv_nmiar1_read() and call it when EL2Enabled() and HCR_EL2.IMO == '1' --- hw/intc/arm_gicv3_cpuif.c | 59 +-- hw/intc/gicv3_internal.h | 1 + hw/intc/trace-events | 1 + 3 files changed, 58 insertions(+), 3 deletions(-) diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c index e1a60d8c15..df82a413c6 100644 --- a/hw/intc/arm_gicv3_cpuif.c +++ b/hw/intc/arm_gicv3_cpuif.c @@ -795,6 +795,13 @@ static uint64_t icv_iar_read(CPUARMState *env, const ARMCPRegInfo *ri) return intid; } +static uint64_t icv_nmiar1_read(CPUARMState *env, const ARMCPRegInfo *ri) +{ +/* todo */ +uint64_t intid = INTID_SPURIOUS; +return intid; +} + static uint32_t icc_fullprio_mask(GICv3CPUState *cs) { /* @@ -1097,7 +1104,8 @@ static uint64_t icc_hppir0_value(GICv3CPUState *cs, CPUARMState *env) return cs->hppi.irq; } -static uint64_t icc_hppir1_value(GICv3CPUState *cs, CPUARMState *env) +static uint64_t icc_hppir1_value(GICv3CPUState *cs, CPUARMState *env, + bool is_nmi, bool is_hppi) { /* Return the highest priority pending interrupt register value * for group 1. @@ -1108,6 +1116,19 @@ static uint64_t icc_hppir1_value(GICv3CPUState *cs, CPUARMState *env) return INTID_SPURIOUS; } +if (!is_hppi) { +int el = arm_current_el(env); + +if (is_nmi && (!cs->hppi.superprio)) { +return INTID_SPURIOUS; +} + +if ((!is_nmi) && cs->hppi.superprio +&& env->cp15.sctlr_el[el] & SCTLR_NMI) { +return INTID_NMI; +} +} + /* Check whether we can return the interrupt or if we should return * a special identifier, as per the CheckGroup1ForSpecialIdentifiers * pseudocode. (We can simplify a little because for us ICC_SRE_EL1.RM @@ -1168,7 +1189,7 @@ static uint64_t icc_iar1_read(CPUARMState *env, const ARMCPRegInfo *ri) if (!icc_hppi_can_preempt(cs)) { intid = INTID_SPURIOUS; } else { -intid = icc_hppir1_value(cs, env); +intid = icc_hppir1_value(cs, env, false, false); } if (!gicv3_intid_is_special(intid)) { @@ -1179,6 +1200,25 @@ static uint64_t icc_iar1_read(CPUARMState *env, const ARMCPRegInfo *ri) return intid; } +static uint64_t icc_nmiar1_read(CPUARMState *env, const ARMCPRegInfo *ri) +{ +GICv3CPUState *cs = icc_cs_from_env(env); +uint64_t intid; + +if (icv_access(env, HCR_IMO)) { +return icv_nmiar1_read(env, ri); +} + +intid = icc_hppir1_value(cs, env, true, false); + +if (!gicv3_intid_is_special(intid)) { +icc_activate_irq(cs, intid); +} + +trace_gicv3_icc_nmiar1_read(gicv3_redist_affid(cs), intid); +return intid; +} + static void icc_drop_prio(GICv3CPUState *cs, int grp) { /* Drop the priority of the currently active interrupt in @@ -1555,7 +1595,7 @@ static uint64_t icc_hppir1_read(CPUARMState *env, const ARMCPRegInfo *ri) return icv_hppir_read(env, ri); } -value = icc_hppir1_value(cs, env); +value = icc_hppir1_value(cs, env, false, true); trace_gicv3_icc_hppir1_read(gicv3_redist_affid(cs), value); return value; } @@ -2482,6 +2522,15 @@ static const ARMCPRegInfo gicv3_cpuif_icc_apxr23_reginfo[] = { }, }; +static const ARMCPRegInfo gicv3_cpuif_gicv3_nmi_reginfo[] = { +{ .name = "ICC_NMIAR1_EL1", .state = ARM_CP_STATE_BOTH, + .opc0 = 3, .opc1 = 0, .crn = 12, .crm = 9, .opc2 = 5, + .type = ARM_CP_IO | ARM_CP_NO_RAW, + .access = PL1_R, .accessfn = gicv3_irq_access, + .readfn = icc_nmiar1_read, +}, +}; + static uint64_t ich_ap_read(CPUARMState *env, const ARMCPRegInfo *ri) { GICv3CPUState *cs = icc_cs_from_env(env); @@ -2838,6 +2887,10 @@ void gicv3_init_cpuif(GICv3State *s) */ define_arm_cp_regs(cpu, gicv3_cpuif_reginfo); +if (s->nmi_support) { +define_arm_cp_regs(cpu, gicv3_cpuif_gicv3_nmi_reginfo); +} + /* * The CPU implementation specifies the number of supported * bits of physical priority. For backwards compatibility diff --git a/hw/intc/gicv3_internal.h b/hw/intc/gicv3_interna
[RFC PATCH v8 05/23] target/arm: Support MSR access to ALLINT
Support ALLINT msr access as follow: mrs , ALLINT// read allint msr ALLINT, // write allint with imm Signed-off-by: Jinjie Ruan Reviewed-by: Richard Henderson --- v5: - Add Reviewed-by. v4: - Remove arm_is_el2_enabled() check in allint_check(). - Change to env->pstate instead of env->allint. v3: - Remove EL0 check in aa64_allint_access() which alreay checks in .access PL1_RW. - Use arm_hcrx_el2_eff() in aa64_allint_access() instead of env->cp15.hcrx_el2. - Make ALLINT msr access function controlled by aa64_nmi. --- target/arm/helper.c | 34 ++ 1 file changed, 34 insertions(+) diff --git a/target/arm/helper.c b/target/arm/helper.c index b19a0178ce..aa0151c775 100644 --- a/target/arm/helper.c +++ b/target/arm/helper.c @@ -4752,6 +4752,36 @@ static void aa64_daif_write(CPUARMState *env, const ARMCPRegInfo *ri, env->daif = value & PSTATE_DAIF; } +static void aa64_allint_write(CPUARMState *env, const ARMCPRegInfo *ri, + uint64_t value) +{ +env->pstate = (env->pstate & ~PSTATE_ALLINT) | (value & PSTATE_ALLINT); +} + +static uint64_t aa64_allint_read(CPUARMState *env, const ARMCPRegInfo *ri) +{ +return env->pstate & PSTATE_ALLINT; +} + +static CPAccessResult aa64_allint_access(CPUARMState *env, + const ARMCPRegInfo *ri, bool isread) +{ +if (arm_current_el(env) == 1 && (arm_hcrx_el2_eff(env) & HCRX_TALLINT)) { +return CP_ACCESS_TRAP_EL2; +} +return CP_ACCESS_OK; +} + +static const ARMCPRegInfo nmi_reginfo[] = { +{ .name = "ALLINT", .state = ARM_CP_STATE_AA64, + .opc0 = 3, .opc1 = 0, .opc2 = 0, .crn = 4, .crm = 3, + .type = ARM_CP_NO_RAW, + .access = PL1_RW, .accessfn = aa64_allint_access, + .fieldoffset = offsetof(CPUARMState, pstate), + .writefn = aa64_allint_write, .readfn = aa64_allint_read, + .resetfn = arm_cp_reset_ignore }, +}; + static uint64_t aa64_pan_read(CPUARMState *env, const ARMCPRegInfo *ri) { return env->pstate & PSTATE_PAN; @@ -9889,6 +9919,10 @@ void register_cp_regs_for_features(ARMCPU *cpu) if (cpu_isar_feature(aa64_nv2, cpu)) { define_arm_cp_regs(cpu, nv2_reginfo); } + +if (cpu_isar_feature(aa64_nmi, cpu)) { +define_arm_cp_regs(cpu, nmi_reginfo); +} #endif if (cpu_isar_feature(any_predinv, cpu)) { -- 2.34.1
[RFC PATCH v8 14/23] hw/intc/arm_gicv3_redist: Implement GICR_INMIR0
Add GICR_INMIR0 register and support access GICR_INMIR0. Signed-off-by: Jinjie Ruan Reviewed-by: Richard Henderson --- v6: - Add Reviewed-by. v4: - Make the GICR_INMIR0 implementation more clearer. --- hw/intc/arm_gicv3_redist.c | 19 +++ hw/intc/gicv3_internal.h | 1 + 2 files changed, 20 insertions(+) diff --git a/hw/intc/arm_gicv3_redist.c b/hw/intc/arm_gicv3_redist.c index 8153525849..7a16a058b1 100644 --- a/hw/intc/arm_gicv3_redist.c +++ b/hw/intc/arm_gicv3_redist.c @@ -35,6 +35,15 @@ static int gicr_ns_access(GICv3CPUState *cs, int irq) return extract32(cs->gicr_nsacr, irq * 2, 2); } +static void gicr_write_bitmap_reg(GICv3CPUState *cs, MemTxAttrs attrs, + uint32_t *reg, uint32_t val) +{ +/* Helper routine to implement writing to a "set" register */ +val &= mask_group(cs, attrs); +*reg = val; +gicv3_redist_update(cs); +} + static void gicr_write_set_bitmap_reg(GICv3CPUState *cs, MemTxAttrs attrs, uint32_t *reg, uint32_t val) { @@ -406,6 +415,10 @@ static MemTxResult gicr_readl(GICv3CPUState *cs, hwaddr offset, *data = value; return MEMTX_OK; } +case GICR_INMIR0: +*data = cs->gic->nmi_support ? +gicr_read_bitmap_reg(cs, attrs, cs->gicr_isuperprio) : 0; +return MEMTX_OK; case GICR_ICFGR0: case GICR_ICFGR1: { @@ -555,6 +568,12 @@ static MemTxResult gicr_writel(GICv3CPUState *cs, hwaddr offset, gicv3_redist_update(cs); return MEMTX_OK; } +case GICR_INMIR0: +if (cs->gic->nmi_support) { +gicr_write_bitmap_reg(cs, attrs, &cs->gicr_isuperprio, value); +} +return MEMTX_OK; + case GICR_ICFGR0: /* Register is all RAZ/WI or RAO/WI bits */ return MEMTX_OK; diff --git a/hw/intc/gicv3_internal.h b/hw/intc/gicv3_internal.h index 29d5cdc1b6..f35b7d2f03 100644 --- a/hw/intc/gicv3_internal.h +++ b/hw/intc/gicv3_internal.h @@ -109,6 +109,7 @@ #define GICR_ICFGR1 (GICR_SGI_OFFSET + 0x0C04) #define GICR_IGRPMODR0(GICR_SGI_OFFSET + 0x0D00) #define GICR_NSACR(GICR_SGI_OFFSET + 0x0E00) +#define GICR_INMIR0 (GICR_SGI_OFFSET + 0x0F80) /* VLPI redistributor registers, offsets from VLPI_base */ #define GICR_VPROPBASER (GICR_VLPI_OFFSET + 0x70) -- 2.34.1
[RFC PATCH v8 16/23] hw/intc: Enable FEAT_GICv3_NMI Feature
Added properties to enable FEAT_GICv3_NMI feature, setup distributor and redistributor registers to indicate NMI support. Signed-off-by: Jinjie Ruan Reviewed-by: Richard Henderson --- v4: - Add Reviewed-by. --- hw/intc/arm_gicv3_common.c | 1 + hw/intc/arm_gicv3_dist.c | 2 ++ hw/intc/gicv3_internal.h | 1 + include/hw/intc/arm_gicv3_common.h | 1 + 4 files changed, 5 insertions(+) diff --git a/hw/intc/arm_gicv3_common.c b/hw/intc/arm_gicv3_common.c index c52f060026..2d2cea6858 100644 --- a/hw/intc/arm_gicv3_common.c +++ b/hw/intc/arm_gicv3_common.c @@ -569,6 +569,7 @@ static Property arm_gicv3_common_properties[] = { DEFINE_PROP_UINT32("num-irq", GICv3State, num_irq, 32), DEFINE_PROP_UINT32("revision", GICv3State, revision, 3), DEFINE_PROP_BOOL("has-lpi", GICv3State, lpi_enable, 0), +DEFINE_PROP_BOOL("has-nmi", GICv3State, nmi_support, 0), DEFINE_PROP_BOOL("has-security-extensions", GICv3State, security_extn, 0), /* * Compatibility property: force 8 bits of physical priority, even diff --git a/hw/intc/arm_gicv3_dist.c b/hw/intc/arm_gicv3_dist.c index 9739404e35..c4e28d209a 100644 --- a/hw/intc/arm_gicv3_dist.c +++ b/hw/intc/arm_gicv3_dist.c @@ -412,6 +412,7 @@ static bool gicd_readl(GICv3State *s, hwaddr offset, * by GICD_TYPER.IDbits) * MBIS == 0 (message-based SPIs not supported) * SecurityExtn == 1 if security extns supported + * NMI = 1 if Non-maskable interrupt property is supported * CPUNumber == 0 since for us ARE is always 1 * ITLinesNumber == (((max SPI IntID + 1) / 32) - 1) */ @@ -425,6 +426,7 @@ static bool gicd_readl(GICv3State *s, hwaddr offset, bool dvis = s->revision >= 4; *data = (1 << 25) | (1 << 24) | (dvis << 18) | (sec_extn << 10) | +(s->nmi_support << GICD_TYPER_NMI_SHIFT) | (s->lpi_enable << GICD_TYPER_LPIS_SHIFT) | (0xf << 19) | itlinesnumber; return true; diff --git a/hw/intc/gicv3_internal.h b/hw/intc/gicv3_internal.h index a1fc34597e..8d793243f4 100644 --- a/hw/intc/gicv3_internal.h +++ b/hw/intc/gicv3_internal.h @@ -70,6 +70,7 @@ #define GICD_CTLR_E1NWF (1U << 7) #define GICD_CTLR_RWP (1U << 31) +#define GICD_TYPER_NMI_SHIFT 9 #define GICD_TYPER_LPIS_SHIFT 17 /* 16 bits EventId */ diff --git a/include/hw/intc/arm_gicv3_common.h b/include/hw/intc/arm_gicv3_common.h index df4380141d..16c5fa7256 100644 --- a/include/hw/intc/arm_gicv3_common.h +++ b/include/hw/intc/arm_gicv3_common.h @@ -251,6 +251,7 @@ struct GICv3State { uint32_t num_irq; uint32_t revision; bool lpi_enable; +bool nmi_support; bool security_extn; bool force_8bit_prio; bool irq_reset_nonsecure; -- 2.34.1
[RFC PATCH v8 12/23] target/arm: Handle NMI in arm_cpu_do_interrupt_aarch64()
According to Arm GIC section 4.6.3 Interrupt superpriority, the interrupt with superpriority is always IRQ, never FIQ, so the NMI exception trap entry behave like IRQ. And VNMI(vIRQ with Superpriority) can be raised from the GIC or come from the hcrx_el2.HCRX_VINMI bit. Signed-off-by: Jinjie Ruan Reviewed-by: Richard Henderson --- v7: - Add Reviewed-by. v6: - Not combine VFNMI with CPU_INTERRUPT_VNMI. v4: - Also handle VNMI in arm_cpu_do_interrupt_aarch64(). v3: - Remove the FIQ NMI handle. --- target/arm/helper.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/target/arm/helper.c b/target/arm/helper.c index 81f4a8f194..e279b26e9d 100644 --- a/target/arm/helper.c +++ b/target/arm/helper.c @@ -11625,6 +11625,8 @@ static void arm_cpu_do_interrupt_aarch64(CPUState *cs) break; case EXCP_IRQ: case EXCP_VIRQ: +case EXCP_NMI: +case EXCP_VNMI: addr += 0x80; break; case EXCP_FIQ: -- 2.34.1
[RFC PATCH v8 19/23] hw/intc/arm_gicv3: Implement NMI interrupt prioirty
If GICD_CTLR_DS bit is zero and the NMI is non-secure, the NMI prioirty is higher than 0x80, otherwise it is higher than 0x0. And save NMI super prioirty information in hppi.superprio to deliver NMI exception. Since both GICR and GICD can deliver NMI, it is both necessary to check whether the pending irq is NMI in gicv3_redist_update_noirqset and gicv3_update_noirqset. And In irqbetter(), only a non-NMI with the same priority and a smaller interrupt number can be preempted but not NMI. Signed-off-by: Jinjie Ruan Reviewed-by: Richard Henderson --- v8: - Add Reviewed-by. v7: - Reorder the irqbetter() code for clarity. - Eliminate the has_superprio local variable for gicv3_get_priority(). - false -> cs->hpplpi.superprio in gicv3_redist_update_noirqset(). - 0x0 -> false in arm_gicv3_common_reset_hold(). - Clear superprio in several places for hppi, hpplpi and hppvlpi. v6: - Put the "extract superprio info" logic into gicv3_get_priority(). - Update the comment in irqbetter(). - Reset the cs->hppi.superprio to 0x0. - Set hppi.superprio to false for LPI. v4: - Replace is_nmi with has_superprio to not a mix NMI and superpriority. - Update the comment in irqbetter(). - Extract gicv3_get_priority() to avoid code repeat. --- v3: - Add missing brace --- hw/intc/arm_gicv3.c| 69 +- hw/intc/arm_gicv3_common.c | 3 ++ hw/intc/arm_gicv3_redist.c | 3 ++ 3 files changed, 66 insertions(+), 9 deletions(-) diff --git a/hw/intc/arm_gicv3.c b/hw/intc/arm_gicv3.c index 0b8f79a122..9496a28005 100644 --- a/hw/intc/arm_gicv3.c +++ b/hw/intc/arm_gicv3.c @@ -21,7 +21,8 @@ #include "hw/intc/arm_gicv3.h" #include "gicv3_internal.h" -static bool irqbetter(GICv3CPUState *cs, int irq, uint8_t prio) +static bool irqbetter(GICv3CPUState *cs, int irq, uint8_t prio, + bool has_superprio) { /* Return true if this IRQ at this priority should take * precedence over the current recorded highest priority @@ -30,14 +31,23 @@ static bool irqbetter(GICv3CPUState *cs, int irq, uint8_t prio) * is the same as this one (a property which the calling code * relies on). */ -if (prio < cs->hppi.prio) { -return true; +if (prio != cs->hppi.prio) { +return prio < cs->hppi.prio; +} + +/* + * The same priority IRQ with superpriority should signal to the CPU + * as it have the priority higher than the labelled 0x80 or 0x00. + */ +if (has_superprio != cs->hppi.superprio) { +return has_superprio; } + /* If multiple pending interrupts have the same priority then it is an * IMPDEF choice which of them to signal to the CPU. We choose to * signal the one with the lowest interrupt number. */ -if (prio == cs->hppi.prio && irq <= cs->hppi.irq) { +if (irq <= cs->hppi.irq) { return true; } return false; @@ -129,6 +139,40 @@ static uint32_t gicr_int_pending(GICv3CPUState *cs) return pend; } +static bool gicv3_get_priority(GICv3CPUState *cs, bool is_redist, + uint8_t *prio, int irq) +{ +uint32_t superprio = 0x0; + +if (is_redist) { +superprio = extract32(cs->gicr_isuperprio, irq, 1); +} else { +superprio = *gic_bmp_ptr32(cs->gic->superprio, irq); +superprio = superprio & (1 << (irq & 0x1f)); +} + +if (superprio) { +/* DS = 0 & Non-secure NMI */ +if (!(cs->gic->gicd_ctlr & GICD_CTLR_DS) && +((is_redist && extract32(cs->gicr_igroupr0, irq, 1)) || + (!is_redist && gicv3_gicd_group_test(cs->gic, irq { +*prio = 0x80; +} else { +*prio = 0x0; +} + +return true; +} + +if (is_redist) { +*prio = cs->gicr_ipriorityr[irq]; +} else { +*prio = cs->gic->gicd_ipriority[irq]; +} + +return false; +} + /* Update the interrupt status after state in a redistributor * or CPU interface has changed, but don't tell the CPU i/f. */ @@ -141,6 +185,7 @@ static void gicv3_redist_update_noirqset(GICv3CPUState *cs) uint8_t prio; int i; uint32_t pend; +bool has_superprio = false; /* Find out which redistributor interrupts are eligible to be * signaled to the CPU interface. @@ -152,10 +197,11 @@ static void gicv3_redist_update_noirqset(GICv3CPUState *cs) if (!(pend & (1 << i))) { continue; } -prio = cs->gicr_ipriorityr[i]; -if (irqbetter(cs, i, prio)) { +has_superprio = gicv3_get_priority(cs, true, &prio, i); +if (irqbetter(cs, i, prio, has_superprio)) { cs->hppi.irq = i; cs->hppi.prio = prio; +cs->hppi.superprio = has_superprio; seenbetter = true; } } @@ -168,9 +214,11 @@ static void gicv3_redist_update_noirqset(GICv3CPUState *cs) if ((cs->gicr_ctlr & GICR_CTLR_ENABLE_L
[RFC PATCH v8 20/23] hw/intc/arm_gicv3: Report the NMI interrupt in gicv3_cpuif_update()
In CPU Interface, if the IRQ has the superpriority property, report NMI to the corresponding PE. Signed-off-by: Jinjie Ruan Reviewed-by: Richard Henderson --- v6: - Add Reviewed-by. v4: - Swap the ordering of the IFs. v3: - Remove handling nmi_is_irq flag. --- hw/intc/arm_gicv3_cpuif.c | 4 1 file changed, 4 insertions(+) diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c index d67d62359b..29142fcf8d 100644 --- a/hw/intc/arm_gicv3_cpuif.c +++ b/hw/intc/arm_gicv3_cpuif.c @@ -969,6 +969,7 @@ void gicv3_cpuif_update(GICv3CPUState *cs) /* Tell the CPU about its highest priority pending interrupt */ int irqlevel = 0; int fiqlevel = 0; +int nmilevel = 0; ARMCPU *cpu = ARM_CPU(cs->cpu); CPUARMState *env = &cpu->env; @@ -1007,6 +1008,8 @@ void gicv3_cpuif_update(GICv3CPUState *cs) if (isfiq) { fiqlevel = 1; +} else if (cs->hppi.superprio) { +nmilevel = 1; } else { irqlevel = 1; } @@ -1016,6 +1019,7 @@ void gicv3_cpuif_update(GICv3CPUState *cs) qemu_set_irq(cs->parent_fiq, fiqlevel); qemu_set_irq(cs->parent_irq, irqlevel); +qemu_set_irq(cs->parent_nmi, nmilevel); } static uint64_t icc_pmr_read(CPUARMState *env, const ARMCPRegInfo *ri) -- 2.34.1
[RFC PATCH v8 09/23] target/arm: Handle PSTATE.ALLINT on taking an exception
Set or clear PSTATE.ALLINT on taking an exception to ELx according to the SCTLR_ELx.SPINTMASK bit. Signed-off-by: Jinjie Ruan Reviewed-by: Richard Henderson --- v3: - Add Reviewed-by. --- target/arm/helper.c | 9 + 1 file changed, 9 insertions(+) diff --git a/target/arm/helper.c b/target/arm/helper.c index 4bc63bf7ca..81f4a8f194 100644 --- a/target/arm/helper.c +++ b/target/arm/helper.c @@ -11705,6 +11705,15 @@ static void arm_cpu_do_interrupt_aarch64(CPUState *cs) } } +if (cpu_isar_feature(aa64_nmi, cpu) && +(env->cp15.sctlr_el[new_el] & SCTLR_NMI)) { +if (!(env->cp15.sctlr_el[new_el] & SCTLR_SPINTMASK)) { +new_mode |= PSTATE_ALLINT; +} else { +new_mode &= ~PSTATE_ALLINT; +} +} + pstate_write(env, PSTATE_DAIF | new_mode); env->aarch64 = true; aarch64_restore_sp(env, new_el); -- 2.34.1
[RFC PATCH v8 22/23] target/arm: Add FEAT_NMI to max
Enable FEAT_NMI on the 'max' CPU. Signed-off-by: Jinjie Ruan Reviewed-by: Richard Henderson --- v3: - Add Reviewed-by. - Sorted to last. --- docs/system/arm/emulation.rst | 1 + target/arm/tcg/cpu64.c| 1 + 2 files changed, 2 insertions(+) diff --git a/docs/system/arm/emulation.rst b/docs/system/arm/emulation.rst index 2a7bbb82dc..a9ae7ede9f 100644 --- a/docs/system/arm/emulation.rst +++ b/docs/system/arm/emulation.rst @@ -64,6 +64,7 @@ the following architecture extensions: - FEAT_MTE (Memory Tagging Extension) - FEAT_MTE2 (Memory Tagging Extension) - FEAT_MTE3 (MTE Asymmetric Fault Handling) +- FEAT_NMI (Non-maskable Interrupt) - FEAT_NV (Nested Virtualization) - FEAT_NV2 (Enhanced nested virtualization support) - FEAT_PACIMP (Pointer authentication - IMPLEMENTATION DEFINED algorithm) diff --git a/target/arm/tcg/cpu64.c b/target/arm/tcg/cpu64.c index 9f7a9f3d2c..62c4663512 100644 --- a/target/arm/tcg/cpu64.c +++ b/target/arm/tcg/cpu64.c @@ -1175,6 +1175,7 @@ void aarch64_max_tcg_initfn(Object *obj) t = FIELD_DP64(t, ID_AA64PFR1, RAS_FRAC, 0); /* FEAT_RASv1p1 + FEAT_DoubleFault */ t = FIELD_DP64(t, ID_AA64PFR1, SME, 1); /* FEAT_SME */ t = FIELD_DP64(t, ID_AA64PFR1, CSV2_FRAC, 0); /* FEAT_CSV2_2 */ +t = FIELD_DP64(t, ID_AA64PFR1, NMI, 1); /* FEAT_NMI */ cpu->isar.id_aa64pfr1 = t; t = cpu->isar.id_aa64mmfr0; -- 2.34.1
[RFC PATCH v8 03/23] target/arm: Add support for FEAT_NMI, Non-maskable Interrupt
Add support for FEAT_NMI. NMI (FEAT_NMI) is an mandatory feature in ARMv8.8-A and ARM v9.3-A. Signed-off-by: Jinjie Ruan Reviewed-by: Richard Henderson --- v3: - Add Reviewed-by. - Adjust to before the MSR patches. --- target/arm/internals.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/target/arm/internals.h b/target/arm/internals.h index dd3da211a3..516e0584bf 100644 --- a/target/arm/internals.h +++ b/target/arm/internals.h @@ -1229,6 +1229,9 @@ static inline uint32_t aarch64_pstate_valid_mask(const ARMISARegisters *id) if (isar_feature_aa64_mte(id)) { valid |= PSTATE_TCO; } +if (isar_feature_aa64_nmi(id)) { +valid |= PSTATE_ALLINT; +} return valid; } -- 2.34.1
[RFC PATCH v8 04/23] target/arm: Implement ALLINT MSR (immediate)
Add ALLINT MSR (immediate) to decodetree, in which the CRm is 0b000x. The EL0 check is necessary to ALLINT, and the EL1 check is necessary when imm == 1. So implement it inline for EL2/3, or EL1 with imm==0. Avoid the unconditional write to pc and use raise_exception_ra to unwind. Signed-off-by: Jinjie Ruan Reviewed-by: Richard Henderson --- v7: - Add Reviewed-by. v6: - Fix DISAS_TOO_MANY to DISAS_UPDATE_EXIT and add the comment. v5: - Drop the & 1 in trans_MSR_i_ALLINT(). - Simplify and merge msr_i_allint() and allint_check(). - Rename msr_i_allint() to msr_set_allint_el1(). v4: - Fix the ALLINT MSR (immediate) decodetree implementation. - Remove arm_is_el2_enabled() check in allint_check(). - Update env->allint to env->pstate. - Only call allint_check() when imm == 1. - Simplify the allint_check() to not pass "op" and extract. - Implement it inline for EL2/3, or EL1 with imm==0. - Pass (a->imm & 1) * PSTATE_ALLINT (i64) to simplfy the ALLINT set/clear. v3: - Remove EL0 check in allint_check(). - Add TALLINT check for EL1 in allint_check(). - Remove unnecessarily arm_rebuild_hflags() in msr_i_allint helper. --- target/arm/tcg/a64.decode | 1 + target/arm/tcg/helper-a64.c| 12 target/arm/tcg/helper-a64.h| 1 + target/arm/tcg/translate-a64.c | 19 +++ 4 files changed, 33 insertions(+) diff --git a/target/arm/tcg/a64.decode b/target/arm/tcg/a64.decode index 8a20dce3c8..0e7656fd15 100644 --- a/target/arm/tcg/a64.decode +++ b/target/arm/tcg/a64.decode @@ -207,6 +207,7 @@ MSR_i_DIT 1101 0101 0 011 0100 010 1 @msr_i MSR_i_TCO 1101 0101 0 011 0100 100 1 @msr_i MSR_i_DAIFSET 1101 0101 0 011 0100 110 1 @msr_i MSR_i_DAIFCLEAR 1101 0101 0 011 0100 111 1 @msr_i +MSR_i_ALLINT1101 0101 0 001 0100 000 imm:1 000 1 MSR_i_SVCR 1101 0101 0 011 0100 0 mask:2 imm:1 011 1 # MRS, MSR (register), SYS, SYSL. These are all essentially the diff --git a/target/arm/tcg/helper-a64.c b/target/arm/tcg/helper-a64.c index ebaa7f00df..7818537890 100644 --- a/target/arm/tcg/helper-a64.c +++ b/target/arm/tcg/helper-a64.c @@ -66,6 +66,18 @@ void HELPER(msr_i_spsel)(CPUARMState *env, uint32_t imm) update_spsel(env, imm); } +void HELPER(msr_set_allint_el1)(CPUARMState *env) +{ +/* ALLINT update to PSTATE. */ +if (arm_hcrx_el2_eff(env) & HCRX_TALLINT) { +raise_exception_ra(env, EXCP_UDEF, + syn_aa64_sysregtrap(0, 1, 0, 4, 1, 0x1f, 0), + exception_target_el(env), GETPC()); +} + +env->pstate |= PSTATE_ALLINT; +} + static void daif_check(CPUARMState *env, uint32_t op, uint32_t imm, uintptr_t ra) { diff --git a/target/arm/tcg/helper-a64.h b/target/arm/tcg/helper-a64.h index 575a5dab7d..0518165399 100644 --- a/target/arm/tcg/helper-a64.h +++ b/target/arm/tcg/helper-a64.h @@ -22,6 +22,7 @@ DEF_HELPER_FLAGS_1(rbit64, TCG_CALL_NO_RWG_SE, i64, i64) DEF_HELPER_2(msr_i_spsel, void, env, i32) DEF_HELPER_2(msr_i_daifset, void, env, i32) DEF_HELPER_2(msr_i_daifclear, void, env, i32) +DEF_HELPER_1(msr_set_allint_el1, void, env) DEF_HELPER_3(vfp_cmph_a64, i64, f16, f16, ptr) DEF_HELPER_3(vfp_cmpeh_a64, i64, f16, f16, ptr) DEF_HELPER_3(vfp_cmps_a64, i64, f32, f32, ptr) diff --git a/target/arm/tcg/translate-a64.c b/target/arm/tcg/translate-a64.c index 340265beb0..21758b290d 100644 --- a/target/arm/tcg/translate-a64.c +++ b/target/arm/tcg/translate-a64.c @@ -2036,6 +2036,25 @@ static bool trans_MSR_i_DAIFCLEAR(DisasContext *s, arg_i *a) return true; } +static bool trans_MSR_i_ALLINT(DisasContext *s, arg_i *a) +{ +if (!dc_isar_feature(aa64_nmi, s) || s->current_el == 0) { +return false; +} + +if (a->imm == 0) { +clear_pstate_bits(PSTATE_ALLINT); +} else if (s->current_el > 1) { +set_pstate_bits(PSTATE_ALLINT); +} else { +gen_helper_msr_set_allint_el1(tcg_env); +} + +/* Exit the cpu loop to re-evaluate pending IRQs. */ +s->base.is_jmp = DISAS_UPDATE_EXIT; +return true; +} + static bool trans_MSR_i_SVCR(DisasContext *s, arg_MSR_i_SVCR *a) { if (!dc_isar_feature(aa64_sme, s) || a->mask == 0) { -- 2.34.1
[RFC PATCH v8 08/23] target/arm: Handle IS/FS in ISR_EL1 for NMI
Add IS and FS bit in ISR_EL1 and handle the read. With CPU_INTERRUPT_NMI or CPU_INTERRUPT_VNMI, both CPSR_I and ISR_IS must be set. With CPU_INTERRUPT_VFIQ and HCRX_EL2.VFNMI set, both CPSR_F and ISR_FS must be set. Signed-off-by: Jinjie Ruan Reviewed-by: Richard Henderson --- v7: - env->cp15.hcrx_el2 -> arm_hcrx_el2_eff(). - Add Reviewed-by. v6: - Verify that HCR_EL2.VF is set before checking VFNMI. v4; - Also handle VNMI. v3: - CPU_INTERRUPT_NMI do not set FIQ, so remove it. - With CPU_INTERRUPT_NMI, both CPSR_I and ISR_IS must be set. --- target/arm/cpu.h| 2 ++ target/arm/helper.c | 14 ++ 2 files changed, 16 insertions(+) diff --git a/target/arm/cpu.h b/target/arm/cpu.h index 629221e1a9..fba0a364db 100644 --- a/target/arm/cpu.h +++ b/target/arm/cpu.h @@ -1396,6 +1396,8 @@ void pmu_init(ARMCPU *cpu); #define CPSR_N (1U << 31) #define CPSR_NZCV (CPSR_N | CPSR_Z | CPSR_C | CPSR_V) #define CPSR_AIF (CPSR_A | CPSR_I | CPSR_F) +#define ISR_FS (1U << 9) +#define ISR_IS (1U << 10) #define CPSR_IT (CPSR_IT_0_1 | CPSR_IT_2_7) #define CACHED_CPSR_BITS (CPSR_T | CPSR_AIF | CPSR_GE | CPSR_IT | CPSR_Q \ diff --git a/target/arm/helper.c b/target/arm/helper.c index 5c637f3413..4bc63bf7ca 100644 --- a/target/arm/helper.c +++ b/target/arm/helper.c @@ -2021,15 +2021,29 @@ static uint64_t isr_read(CPUARMState *env, const ARMCPRegInfo *ri) if (cs->interrupt_request & CPU_INTERRUPT_VIRQ) { ret |= CPSR_I; } +if (cs->interrupt_request & CPU_INTERRUPT_VNMI) { +ret |= ISR_IS; +ret |= CPSR_I; +} } else { if (cs->interrupt_request & CPU_INTERRUPT_HARD) { ret |= CPSR_I; } + +if (cs->interrupt_request & CPU_INTERRUPT_NMI) { +ret |= ISR_IS; +ret |= CPSR_I; +} } if (hcr_el2 & HCR_FMO) { if (cs->interrupt_request & CPU_INTERRUPT_VFIQ) { ret |= CPSR_F; + +if ((arm_hcr_el2_eff(env) & HCR_VF) && +(arm_hcrx_el2_eff(env) & HCRX_VFNMI)) { +ret |= ISR_FS; +} } } else { if (cs->interrupt_request & CPU_INTERRUPT_FIQ) { -- 2.34.1
[RFC PATCH v8 18/23] hw/intc/arm_gicv3: Handle icv_nmiar1_read() for icc_nmiar1_read()
Implement icv_nmiar1_read() for icc_nmiar1_read(), so add definition for ICH_LR_EL2.NMI and ICH_AP1R_EL2.NMI bit. If FEAT_GICv3_NMI is supported, ich_ap_write() should consider ICH_AP1R_EL2.NMI bit. In icv_activate_irq() and icv_eoir_write(), the ICH_AP1R_EL2.NMI bit should be set or clear according to the Superpriority info. By the way, add gicv3_icv_nmiar1_read trace event. If the hpp irq is a NMI, the icv iar read should return 1022 and trap for NMI again Signed-off-by: Jinjie Ruan Reviewed-by: Richard Henderson --- v8: - Fix an unexpected interrupt bug when sending VNMI by running qemu VM. v7: - Add Reviewed-by. v6: - Implement icv_nmiar1_read(). --- hw/intc/arm_gicv3_cpuif.c | 52 ++- hw/intc/gicv3_internal.h | 3 +++ hw/intc/trace-events | 1 + 3 files changed, 50 insertions(+), 6 deletions(-) diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c index df82a413c6..d67d62359b 100644 --- a/hw/intc/arm_gicv3_cpuif.c +++ b/hw/intc/arm_gicv3_cpuif.c @@ -728,7 +728,7 @@ static uint64_t icv_hppir_read(CPUARMState *env, const ARMCPRegInfo *ri) return value; } -static void icv_activate_irq(GICv3CPUState *cs, int idx, int grp) +static void icv_activate_irq(GICv3CPUState *cs, int idx, int grp, bool nmi) { /* Activate the interrupt in the specified list register * by moving it from Pending to Active state, and update the @@ -742,7 +742,12 @@ static void icv_activate_irq(GICv3CPUState *cs, int idx, int grp) cs->ich_lr_el2[idx] &= ~ICH_LR_EL2_STATE_PENDING_BIT; cs->ich_lr_el2[idx] |= ICH_LR_EL2_STATE_ACTIVE_BIT; -cs->ich_apr[grp][regno] |= (1 << regbit); + +if (cs->gic->nmi_support) { +cs->ich_apr[grp][regno] |= (1 << regbit) | (nmi ? ICH_AP1R_EL2_NMI : 0); +} else { +cs->ich_apr[grp][regno] |= (1 << regbit); +} } static void icv_activate_vlpi(GICv3CPUState *cs) @@ -775,8 +780,10 @@ static uint64_t icv_iar_read(CPUARMState *env, const ARMCPRegInfo *ri) if (thisgrp == grp && icv_hppi_can_preempt(cs, lr)) { intid = ich_lr_vintid(lr); -if (!gicv3_intid_is_special(intid)) { -icv_activate_irq(cs, idx, grp); +if (!gicv3_intid_is_special(intid) && !(lr & ICH_LR_EL2_NMI)) { +icv_activate_irq(cs, idx, grp, false); +} else if (lr & ICH_LR_EL2_NMI) { +intid = INTID_NMI; } else { /* Interrupt goes from Pending to Invalid */ cs->ich_lr_el2[idx] &= ~ICH_LR_EL2_STATE_PENDING_BIT; @@ -797,8 +804,32 @@ static uint64_t icv_iar_read(CPUARMState *env, const ARMCPRegInfo *ri) static uint64_t icv_nmiar1_read(CPUARMState *env, const ARMCPRegInfo *ri) { -/* todo */ +GICv3CPUState *cs = icc_cs_from_env(env); +int idx = hppvi_index(cs); uint64_t intid = INTID_SPURIOUS; + +if (idx >= 0 && idx != HPPVI_INDEX_VLPI) { +uint64_t lr = cs->ich_lr_el2[idx]; +int thisgrp = (lr & ICH_LR_EL2_GROUP) ? GICV3_G1NS : GICV3_G0; + +if ((thisgrp == GICV3_G1NS) && (lr & ICH_LR_EL2_NMI)) { +intid = ich_lr_vintid(lr); +if (!gicv3_intid_is_special(intid)) { +icv_activate_irq(cs, idx, GICV3_G1NS, true); +} else { +/* Interrupt goes from Pending to Invalid */ +cs->ich_lr_el2[idx] &= ~ICH_LR_EL2_STATE_PENDING_BIT; +/* We will now return the (bogus) ID from the list register, + * as per the pseudocode. + */ +} +} +} + +trace_gicv3_icv_nmiar1_read(gicv3_redist_affid(cs), intid); + +gicv3_cpuif_virt_update(cs); + return intid; } @@ -1403,6 +1434,11 @@ static int icv_drop_prio(GICv3CPUState *cs) return (apr0count + i * 32) << (icv_min_vbpr(cs) + 1); } else { *papr1 &= *papr1 - 1; + +if (cs->gic->nmi_support && (*papr1 & ICH_AP1R_EL2_NMI)) { +*papr1 &= ~ICH_AP1R_EL2_NMI; +} + return (apr1count + i * 32) << (icv_min_vbpr(cs) + 1); } } @@ -2552,7 +2588,11 @@ static void ich_ap_write(CPUARMState *env, const ARMCPRegInfo *ri, trace_gicv3_ich_ap_write(ri->crm & 1, regno, gicv3_redist_affid(cs), value); -cs->ich_apr[grp][regno] = value & 0xU; +if (cs->gic->nmi_support) { +cs->ich_apr[grp][regno] = value & (0xU | ICH_AP1R_EL2_NMI); +} else { +cs->ich_apr[grp][regno] = value & 0xU; +} gicv3_cpuif_virt_irq_fiq_update(cs); } diff --git a/hw/intc/gicv3_internal.h b/hw/intc/gicv3_internal.h index 93e56b3726..5e2b32861d 100644 --- a/hw/intc/gicv3_internal.h +++ b/hw/intc/gicv3_internal.h @@ -242,6 +242,7 @@ FIELD(GICR_VPENDBASER, VALID, 63, 1) #define ICH_LR_EL2_PRIORITY_SHIFT 48 #define ICH_LR_EL2_PRIORITY_LENGTH 8 #define ICH_LR_EL2_PRIORITY_MASK (0xffULL << ICH_LR_EL2_PRIORITY_SHIFT) +#define ICH_L
[RFC PATCH v8 15/23] hw/intc/arm_gicv3: Implement GICD_INMIR
Add GICD_INMIR, GICD_INMIRnE register and support access GICD_INMIR0. Signed-off-by: Jinjie Ruan Reviewed-by: Richard Henderson --- v4: - Make the GICD_INMIR implementation more clearer. - Udpate the commit message. v3: - Add Reviewed-by. --- hw/intc/arm_gicv3_dist.c | 34 ++ hw/intc/gicv3_internal.h | 2 ++ 2 files changed, 36 insertions(+) diff --git a/hw/intc/arm_gicv3_dist.c b/hw/intc/arm_gicv3_dist.c index 35e850685c..9739404e35 100644 --- a/hw/intc/arm_gicv3_dist.c +++ b/hw/intc/arm_gicv3_dist.c @@ -89,6 +89,29 @@ static int gicd_ns_access(GICv3State *s, int irq) return extract32(s->gicd_nsacr[irq / 16], (irq % 16) * 2, 2); } +static void gicd_write_bitmap_reg(GICv3State *s, MemTxAttrs attrs, + uint32_t *bmp, maskfn *maskfn, + int offset, uint32_t val) +{ +/* + * Helper routine to implement writing to a "set" register + * (GICD_INMIR, etc). + * Semantics implemented here: + * RAZ/WI for SGIs, PPIs, unimplemented IRQs + * Bits corresponding to Group 0 or Secure Group 1 interrupts RAZ/WI. + * offset should be the offset in bytes of the register from the start + * of its group. + */ +int irq = offset * 8; + +if (irq < GIC_INTERNAL || irq >= s->num_irq) { +return; +} +val &= mask_group_and_nsacr(s, attrs, maskfn, irq); +*gic_bmp_ptr32(bmp, irq) = val; +gicv3_update(s, irq, 32); +} + static void gicd_write_set_bitmap_reg(GICv3State *s, MemTxAttrs attrs, uint32_t *bmp, maskfn *maskfn, @@ -543,6 +566,11 @@ static bool gicd_readl(GICv3State *s, hwaddr offset, /* RAZ/WI since affinity routing is always enabled */ *data = 0; return true; +case GICD_INMIR ... GICD_INMIR + 0x7f: +*data = (!s->nmi_support) ? 0 : +gicd_read_bitmap_reg(s, attrs, s->superprio, NULL, + offset - GICD_INMIR); +return true; case GICD_IROUTER ... GICD_IROUTER + 0x1fdf: { uint64_t r; @@ -752,6 +780,12 @@ static bool gicd_writel(GICv3State *s, hwaddr offset, case GICD_SPENDSGIR ... GICD_SPENDSGIR + 0xf: /* RAZ/WI since affinity routing is always enabled */ return true; +case GICD_INMIR ... GICD_INMIR + 0x7f: +if (s->nmi_support) { +gicd_write_bitmap_reg(s, attrs, s->superprio, NULL, + offset - GICD_INMIR, value); +} +return true; case GICD_IROUTER ... GICD_IROUTER + 0x1fdf: { uint64_t r; diff --git a/hw/intc/gicv3_internal.h b/hw/intc/gicv3_internal.h index f35b7d2f03..a1fc34597e 100644 --- a/hw/intc/gicv3_internal.h +++ b/hw/intc/gicv3_internal.h @@ -52,6 +52,8 @@ #define GICD_SGIR0x0F00 #define GICD_CPENDSGIR 0x0F10 #define GICD_SPENDSGIR 0x0F20 +#define GICD_INMIR 0x0F80 +#define GICD_INMIRnE 0x3B00 #define GICD_IROUTER 0x6000 #define GICD_IDREGS 0xFFD0 -- 2.34.1
[RFC PATCH v8 23/23] hw/arm/virt: Add FEAT_GICv3_NMI feature support in virt GIC
A PE that implements FEAT_NMI and FEAT_GICv3 also implements FEAT_GICv3_NMI. A PE that does not implement FEAT_NMI, does not implement FEAT_GICv3_NMI So included support FEAT_GICv3_NMI feature as part of virt platform GIC initialization if FEAT_NMI and FEAT_GICv3 supported. Signed-off-by: Jinjie Ruan Reviewed-by: Richard Henderson --- v4: - Add Reviewed-by. v3: - Adjust to be the last after add FEAT_NMI to max. - Check whether support FEAT_NMI and FEAT_GICv3 for FEAT_GICv3_NMI. --- hw/arm/virt.c | 18 ++ 1 file changed, 18 insertions(+) diff --git a/hw/arm/virt.c b/hw/arm/virt.c index dc7959e164..680a8a3eb6 100644 --- a/hw/arm/virt.c +++ b/hw/arm/virt.c @@ -729,6 +729,19 @@ static void create_v2m(VirtMachineState *vms) vms->msi_controller = VIRT_MSI_CTRL_GICV2M; } +/* + * A PE that implements FEAT_NMI and FEAT_GICv3 also implements + * FEAT_GICv3_NMI. A PE that does not implement FEAT_NMI, does not implement + * FEAT_GICv3_NMI. + */ +static bool gicv3_nmi_present(VirtMachineState *vms) +{ +ARMCPU *cpu = ARM_CPU(qemu_get_cpu(0)); + +return cpu_isar_feature(aa64_nmi, cpu) && + (vms->gic_version != VIRT_GIC_VERSION_2); +} + static void create_gic(VirtMachineState *vms, MemoryRegion *mem) { MachineState *ms = MACHINE(vms); @@ -802,6 +815,11 @@ static void create_gic(VirtMachineState *vms, MemoryRegion *mem) vms->virt); } } + +if (gicv3_nmi_present(vms)) { +qdev_prop_set_bit(vms->gic, "has-nmi", true); +} + gicbusdev = SYS_BUS_DEVICE(vms->gic); sysbus_realize_and_unref(gicbusdev, &error_fatal); sysbus_mmio_map(gicbusdev, 0, vms->memmap[VIRT_GIC_DIST].base); -- 2.34.1
[RFC PATCH v8 10/23] hw/arm/virt: Wire NMI and VNMI irq lines from GIC to CPU
Wire the new NMI and VNMI interrupt line from the GIC to each CPU. Signed-off-by: Jinjie Ruan Reviewed-by: Richard Henderson --- v4: - Add Reviewed-by. v3: - Also add VNMI wire. --- hw/arm/virt.c | 7 ++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/hw/arm/virt.c b/hw/arm/virt.c index e5cd935232..dc7959e164 100644 --- a/hw/arm/virt.c +++ b/hw/arm/virt.c @@ -821,7 +821,8 @@ static void create_gic(VirtMachineState *vms, MemoryRegion *mem) /* Wire the outputs from each CPU's generic timer and the GICv3 * maintenance interrupt signal to the appropriate GIC PPI inputs, - * and the GIC's IRQ/FIQ/VIRQ/VFIQ interrupt outputs to the CPU's inputs. + * and the GIC's IRQ/FIQ/VIRQ/VFIQ/NMI/VNMI interrupt outputs to the + * CPU's inputs. */ for (i = 0; i < smp_cpus; i++) { DeviceState *cpudev = DEVICE(qemu_get_cpu(i)); @@ -865,6 +866,10 @@ static void create_gic(VirtMachineState *vms, MemoryRegion *mem) qdev_get_gpio_in(cpudev, ARM_CPU_VIRQ)); sysbus_connect_irq(gicbusdev, i + 3 * smp_cpus, qdev_get_gpio_in(cpudev, ARM_CPU_VFIQ)); +sysbus_connect_irq(gicbusdev, i + 4 * smp_cpus, + qdev_get_gpio_in(cpudev, ARM_CPU_NMI)); +sysbus_connect_irq(gicbusdev, i + 5 * smp_cpus, + qdev_get_gpio_in(cpudev, ARM_CPU_VNMI)); } fdt_add_gic_node(vms); -- 2.34.1
[RFC PATCH v8 11/23] hw/intc/arm_gicv3: Add external IRQ lines for NMI
Augment the GICv3's QOM device interface by adding one new set of sysbus IRQ line, to signal NMI to each CPU. Signed-off-by: Jinjie Ruan Reviewed-by: Richard Henderson --- v4: - Add Reviewed-by. v3: - Add support for VNMI. --- hw/intc/arm_gicv3_common.c | 6 ++ include/hw/intc/arm_gic_common.h | 2 ++ include/hw/intc/arm_gicv3_common.h | 2 ++ 3 files changed, 10 insertions(+) diff --git a/hw/intc/arm_gicv3_common.c b/hw/intc/arm_gicv3_common.c index cb55c72681..c52f060026 100644 --- a/hw/intc/arm_gicv3_common.c +++ b/hw/intc/arm_gicv3_common.c @@ -299,6 +299,12 @@ void gicv3_init_irqs_and_mmio(GICv3State *s, qemu_irq_handler handler, for (i = 0; i < s->num_cpu; i++) { sysbus_init_irq(sbd, &s->cpu[i].parent_vfiq); } +for (i = 0; i < s->num_cpu; i++) { +sysbus_init_irq(sbd, &s->cpu[i].parent_nmi); +} +for (i = 0; i < s->num_cpu; i++) { +sysbus_init_irq(sbd, &s->cpu[i].parent_vnmi); +} memory_region_init_io(&s->iomem_dist, OBJECT(s), ops, s, "gicv3_dist", 0x1); diff --git a/include/hw/intc/arm_gic_common.h b/include/hw/intc/arm_gic_common.h index 7080375008..97fea4102d 100644 --- a/include/hw/intc/arm_gic_common.h +++ b/include/hw/intc/arm_gic_common.h @@ -71,6 +71,8 @@ struct GICState { qemu_irq parent_fiq[GIC_NCPU]; qemu_irq parent_virq[GIC_NCPU]; qemu_irq parent_vfiq[GIC_NCPU]; +qemu_irq parent_nmi[GIC_NCPU]; +qemu_irq parent_vnmi[GIC_NCPU]; qemu_irq maintenance_irq[GIC_NCPU]; /* GICD_CTLR; for a GIC with the security extensions the NS banked version diff --git a/include/hw/intc/arm_gicv3_common.h b/include/hw/intc/arm_gicv3_common.h index 4e2fb518e7..7324c7d983 100644 --- a/include/hw/intc/arm_gicv3_common.h +++ b/include/hw/intc/arm_gicv3_common.h @@ -155,6 +155,8 @@ struct GICv3CPUState { qemu_irq parent_fiq; qemu_irq parent_virq; qemu_irq parent_vfiq; +qemu_irq parent_nmi; +qemu_irq parent_vnmi; /* Redistributor */ uint32_t level; /* Current IRQ level */ -- 2.34.1
Re: [PATCH 02/22] tcg: Make tcg/helper-info.h self-contained
Richard Henderson writes: > Move MAX_CALL_IARGS from tcg.h and include for > the define of TCG_TARGET_REG_BITS. > > Signed-off-by: Richard Henderson Reviewed-by: Alex Bennée -- Alex Bennée Virtualisation Tech Lead @ Linaro
Re: [PATCH 03/22] tcg: Pass function pointer to tcg_gen_call*
Richard Henderson writes: > For normal helpers, read the function pointer from the > structure earlier. For plugins, this will allow the > function pointer to come from elsewhere. > > Signed-off-by: Richard Henderson Reviewed-by: Alex Bennée -- Alex Bennée Virtualisation Tech Lead @ Linaro
Re: [PATCH 02/22] tcg: Make tcg/helper-info.h self-contained
Richard Henderson writes: > Move MAX_CALL_IARGS from tcg.h and include for > the define of TCG_TARGET_REG_BITS. > > Signed-off-by: Richard Henderson This may have broken TCI: ../tcg/tci.c: In function 'tcg_qemu_tb_exec': ../tcg/tci.c:391:34: error: 'MAX_CALL_IARGS' undeclared (first use in this function) 391 | void *call_slots[MAX_CALL_IARGS]; | ^~ ../tcg/tci.c:391:34: note: each undeclared identifier is reported only once for each function it appears in ../tcg/tci.c:391:23: error: unused variable 'call_slots' [-Werror=unused-variable] 391 | void *call_slots[MAX_CALL_IARGS]; | ^~ cc1: all warnings being treated as errors the gift that keeps on giving ;-) -- Alex Bennée Virtualisation Tech Lead @ Linaro
Re: [PATCH 04/22] plugins: Zero new qemu_plugin_dyn_cb entries
Richard Henderson writes: > Signed-off-by: Richard Henderson Reviewed-by: Alex Bennée -- Alex Bennée Virtualisation Tech Lead @ Linaro
Re: [PATCH 05/22] plugins: Move function pointer in qemu_plugin_dyn_cb
Richard Henderson writes: > The out-of-line function pointer is mutually exclusive > with inline expansion, so move it into the union. > Wrap the pointer in a structure named 'regular' to match > PLUGIN_CB_REGULAR. > > Signed-off-by: Richard Henderson Reviewed-by: Alex Bennée -- Alex Bennée Virtualisation Tech Lead @ Linaro
[PATCH-for-9.1 3/3] ui/console: Add 'rotate_arcdegree' field to allow per-console rotation
Add the 'rotate_arcdegree' field to QemuConsole and remove the use of the 'graphic_rotate' global. Signed-off-by: Philippe Mathieu-Daudé --- ui/console-priv.h | 1 + ui/console.c | 7 +++ 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/ui/console-priv.h b/ui/console-priv.h index 88569ed2cc..6e54b476d9 100644 --- a/ui/console-priv.h +++ b/ui/console-priv.h @@ -31,6 +31,7 @@ struct QemuConsole { const GraphicHwOps *hw_ops; void *hw; CoQueue dump_queue; +unsigned rotate_arcdegree; QTAILQ_ENTRY(QemuConsole) next; }; diff --git a/ui/console.c b/ui/console.c index 84aee76846..a36674bacf 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" @@ -210,17 +209,17 @@ void qemu_console_set_window_id(QemuConsole *con, int window_id) void qemu_console_set_rotate(QemuConsole *con, unsigned arcdegree) { -graphic_rotate = arcdegree; +con->rotate_arcdegree = arcdegree; } bool qemu_console_is_rotated(QemuConsole *con) { -return graphic_rotate != 0; +return con->rotate_arcdegree != 0; } unsigned qemu_console_get_rotate_arcdegree(QemuConsole *con) { -return graphic_rotate; +return con->rotate_arcdegree; } void graphic_hw_invalidate(QemuConsole *con) -- 2.41.0
[PATCH-for-9.1 0/3] ui/display: Introduce API to change console orientation
Hi, The idea behind this series is to reduce the use of the 'graphic_rotate' global. It is only used by the Spitz machine, so we could convert the '-rotate' argument to a sugar property on the PXA2XX_LCD_TYPE model, but since the Spitz machine has been deprecated recently (commit a2531bb855 "Deprecate various old Arm machine types") it doesn't seem worthwhile. So just extract the API to change console orientation. Regards, Phil. Philippe Mathieu-Daudé (3): ui/console: Introduce API to change console orientation hw/display/pxa2xx_lcd: Set rotation angle using qemu_console_set_rotate ui/console: Add 'rotate_arcdegree' field to allow per-console rotation include/ui/console.h| 3 +++ ui/console-priv.h | 1 + hw/display/pxa2xx_lcd.c | 1 + ui/console.c| 15 +++ ui/input.c | 9 - 5 files changed, 24 insertions(+), 5 deletions(-) -- 2.41.0
[PATCH-for-9.1 1/3] ui/console: Introduce API to change console orientation
Extract the following methods: - qemu_console_set_rotate() - qemu_console_is_rotated() - qemu_console_get_rotate_arcdegree() Signed-off-by: Philippe Mathieu-Daudé --- 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 */ -- 2.41.0
[PATCH-for-9.1 2/3] hw/display/pxa2xx_lcd: Set rotation angle using qemu_console_set_rotate
Signed-off-by: Philippe Mathieu-Daudé --- 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); -- 2.41.0
Re: [PATCH v1 1/2] system/cpus: Fix pause_all_vcpus() under concurrent environment
On 17.03.24 09:37, Keqian Zhu via wrote: Both main loop thread and vCPU thread are allowed to call pause_all_vcpus(), and in general resume_all_vcpus() is called after it. Two issues live in pause_all_vcpus(): In general, calling pause_all_vcpus() from VCPU threads is quite dangerous. Do we have reproducers for the cases below? 1. There is possibility that during thread T1 waits on qemu_pause_cond with bql unlocked, other thread has called pause_all_vcpus() and resume_all_vcpus(), then thread T1 will stuck, because the condition all_vcpus_paused() is always false. How can this happen? Two threads calling pause_all_vcpus() is borderline broken, as you note. IIRC, we should call pause_all_vcpus() only if some other mechanism prevents these races. For example, based on runstate changes. Just imagine one thread calling pause_all_vcpus() while another one calls resume_all_vcpus(). It cannot possibly work. 2. After all_vcpus_paused() has been checked as true, we will unlock bql to relock replay_mutex. During the bql was unlocked, the vcpu's state may has been changed by other thread, so we must retry. Signed-off-by: Keqian Zhu --- system/cpus.c | 29 - 1 file changed, 24 insertions(+), 5 deletions(-) diff --git a/system/cpus.c b/system/cpus.c index 68d161d96b..4e41abe23e 100644 --- a/system/cpus.c +++ b/system/cpus.c @@ -571,12 +571,14 @@ static bool all_vcpus_paused(void) return true; } -void pause_all_vcpus(void) +static void request_pause_all_vcpus(void) { CPUState *cpu; -qemu_clock_enable(QEMU_CLOCK_VIRTUAL, false); CPU_FOREACH(cpu) { +if (cpu->stopped) { +continue; +} if (qemu_cpu_is_self(cpu)) { qemu_cpu_stop(cpu, true); } else { @@ -584,6 +586,14 @@ void pause_all_vcpus(void) qemu_cpu_kick(cpu); } } +} + +void pause_all_vcpus(void) +{ +qemu_clock_enable(QEMU_CLOCK_VIRTUAL, false); + +retry: +request_pause_all_vcpus(); /* We need to drop the replay_lock so any vCPU threads woken up * can finish their replay tasks @@ -592,14 +602,23 @@ void pause_all_vcpus(void) while (!all_vcpus_paused()) { qemu_cond_wait(&qemu_pause_cond, &bql); -CPU_FOREACH(cpu) { -qemu_cpu_kick(cpu); -} +/* During we waited on qemu_pause_cond the bql was unlocked, + * the vcpu's state may has been changed by other thread, so + * we must request the pause state on all vcpus again. + */ +request_pause_all_vcpus(); } bql_unlock(); replay_mutex_lock(); bql_lock(); + +/* During the bql was unlocked, the vcpu's state may has been + * changed by other thread, so we must retry. + */ +if (!all_vcpus_paused()) { +goto retry; +} } void cpu_resume(CPUState *cpu) -- Cheers, David / dhildenb
Re: [PATCH-for-9.1 0/3] ui/display: Introduce API to change console orientation
(Forgot to Cc Akihiko) On 18/3/24 11:05, Philippe Mathieu-Daudé wrote: Hi, The idea behind this series is to reduce the use of the 'graphic_rotate' global. It is only used by the Spitz machine, so we could convert the '-rotate' argument to a sugar property on the PXA2XX_LCD_TYPE model, but since the Spitz machine has been deprecated recently (commit a2531bb855 "Deprecate various old Arm machine types") it doesn't seem worthwhile. So just extract the API to change console orientation. Regards, Phil. Philippe Mathieu-Daudé (3): ui/console: Introduce API to change console orientation hw/display/pxa2xx_lcd: Set rotation angle using qemu_console_set_rotate ui/console: Add 'rotate_arcdegree' field to allow per-console rotation
Re: [PATCH v1 2/2] system/cpus: Fix resume_all_vcpus() under vCPU hotplug condition
On 17.03.24 09:37, Keqian Zhu via wrote: For vCPU being hotplugged, qemu_init_vcpu() is called. In this function, we set vcpu state as stopped, and then wait vcpu thread to be created. As the vcpu state is stopped, it will inform us it has been created and then wait on halt_cond. After we has realized vcpu object, we will resume the vcpu thread. However, during we wait vcpu thread to be created, the bql is unlocked, and other thread is allowed to call resume_all_vcpus(), which will resume the un-realized vcpu. This fixes the issue by filter out un-realized vcpu during resume_all_vcpus(). Similar question: is there a reproducer? How could we currently hotplug a VCPU, and while it is being created, see pause_all_vcpus()/resume_all_vcpus() getting claled. If I am not getting this wrong, there seems to be some other mechanism missing that makes sure that this cannot happen. Dropping the BQL half-way through creating a VCPU might be the problem. -- Cheers, David / dhildenb
Re: [PATCH 08/10] qapi: Correct error message for 'vcpu_dirty_limit' parameter
On 18/3/24 09:58, Markus Armbruster wrote: Philippe Mathieu-Daudé writes: On 12/3/24 16:26, Zhao Liu wrote: On Tue, Mar 12, 2024 at 03:13:41PM +0100, Markus Armbruster wrote: Date: Tue, 12 Mar 2024 15:13:41 +0100 From: Markus Armbruster Subject: [PATCH 08/10] qapi: Correct error message for 'vcpu_dirty_limit' parameter From: Philippe Mathieu-Daudé QERR_INVALID_PARAMETER_VALUE is defined as: #define QERR_INVALID_PARAMETER_VALUE \ "Parameter '%s' expects %s" The current error is formatted as: "Parameter 'vcpu_dirty_limit' expects is invalid, it must greater then 1 MB/s" Replace by: "Parameter 'vcpu_dirty_limit' is invalid, it must greater than 1 MB/s" Is there a grammar error here? Maybe s/it must greater/it must be greater/ Oops indeed! What about dropping "is invalid, "? I.e. go with "Parameter 'vcpu_dirty_limit' must be greater than 1 MB/s" Yes. Signed-off-by: Philippe Mathieu-Daudé Reviewed-by: Juan Quintela Signed-off-by: Markus Armbruster --- migration/options.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
Re: [PATCH-for-9.1 3/3] ui/console: Add 'rotate_arcdegree' field to allow per-console rotation
On 2024/03/18 19:05, Philippe Mathieu-Daudé wrote: Add the 'rotate_arcdegree' field to QemuConsole and remove the use of the 'graphic_rotate' global. I think QemuGraphicConsole is a better place to put the field. Regards, Akihiko Odaki Signed-off-by: Philippe Mathieu-Daudé --- ui/console-priv.h | 1 + ui/console.c | 7 +++ 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/ui/console-priv.h b/ui/console-priv.h index 88569ed2cc..6e54b476d9 100644 --- a/ui/console-priv.h +++ b/ui/console-priv.h @@ -31,6 +31,7 @@ struct QemuConsole { const GraphicHwOps *hw_ops; void *hw; CoQueue dump_queue; +unsigned rotate_arcdegree; QTAILQ_ENTRY(QemuConsole) next; }; diff --git a/ui/console.c b/ui/console.c index 84aee76846..a36674bacf 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" @@ -210,17 +209,17 @@ void qemu_console_set_window_id(QemuConsole *con, int window_id) void qemu_console_set_rotate(QemuConsole *con, unsigned arcdegree) { -graphic_rotate = arcdegree; +con->rotate_arcdegree = arcdegree; } bool qemu_console_is_rotated(QemuConsole *con) { -return graphic_rotate != 0; +return con->rotate_arcdegree != 0; } unsigned qemu_console_get_rotate_arcdegree(QemuConsole *con) { -return graphic_rotate; +return con->rotate_arcdegree; } void graphic_hw_invalidate(QemuConsole *con)
Re: [PATCH v4 14/25] memory: Add Error** argument to the global_dirty_log routines
--- a/migration/ram.c +++ b/migration/ram.c @@ -2836,18 +2836,31 @@ static void migration_bitmap_clear_discarded_pages(RAMState *rs) static void ram_init_bitmaps(RAMState *rs) { +Error *local_err = NULL; +bool ret = true; + qemu_mutex_lock_ramlist(); WITH_RCU_READ_LOCK_GUARD() { ram_list_init_bitmaps(); /* We don't use dirty log with background snapshots */ if (!migrate_background_snapshot()) { -memory_global_dirty_log_start(GLOBAL_DIRTY_MIGRATION); +ret = memory_global_dirty_log_start(GLOBAL_DIRTY_MIGRATION, +&local_err); +if (!ret) { +error_report_err(local_err); +goto out_unlock; Here we may need to free the bitmaps created in ram_list_init_bitmaps(). We can have a helper ram_bitmaps_destroy() for that. One thing be careful is the new file_bmap can be created but missing in the ram_save_cleanup(), it's because it's freed earlier. IMHO if we will have a new ram_bitmaps_destroy() we can unconditionally free file_bmap there too, as if it's freed early g_free() is noop. OK. Let's do that in a new prereq patch. I will change ram_state_init() and xbzrle_init() to take an Error ** argument while at it. Thanks, C.
Re: [PATCH 00/10] Reduce usage of QERR_ macros further
Markus Armbruster writes: > Philippe posted "[PATCH v2 00/22] qapi: Kill 'qapi/qmp/qerror.h' for > good" a couple of months ago. I cherry-picked just its simplest parts > for now. > > Markus Armbruster (1): > error: Drop superfluous #include "qapi/qmp/qerror.h" Queued for 9.1 with PATCH 08's new error message fixed. Thanks!
Re: [PATCH v2 1/1] cxl/mem: Fix for the index of Clear Event Record Handle
On Mon, 18 Mar 2024 10:29:28 +0800 Yuquan Wang 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 > --- > 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 > > if (i == max_handles) { > payload->nr_recs = i;
Re: [PATCH 03/14] hw/core/machine-smp: Simplify variables' initialization in machine_parse_smp_config()
Hello, On Mon, 18 Mar 2024 at 13:23, Zhao Liu wrote: > Indeed, as you say, these items are initialized to 0 by default. > > I think, however, that the initialization is so far away from where the > smp is currently parsed that one can't easily confirm it (thanks for > your confirmation!). > > From a code readability view, the fact that we're explicitly > initializing to 0 again here brings little overhead, but makes the code > more readable as well as easier to maintain. I think the small redundancy > here is worth it. > > Also, in other use cases people always relies on fields marked by has_*, > and there is no (or less?) precedent for direct access to places where > has_* is not set. I understand that this is also a habit, i.e., fields > with a has_* of False by default are unreliable and avoid going directly > to them. * Ummn...okay. (I'm not fully convinced, but that's fine, I'm okay to go with you on this.) Thank you. --- - Prasad
Re: [PATCH v3 for 9.1 0/6] virtio, vhost: Add VIRTIO_F_NOTIFICATION_DATA support
On 3/16/24 11:45 AM, Jiri Pirko wrote: Fri, Mar 15, 2024 at 05:55:51PM CET, jonah.pal...@oracle.com wrote: The goal of these patches are to add support to a variety of virtio and vhost devices for the VIRTIO_F_NOTIFICATION_DATA transport feature. This feature indicates that a driver will pass extra data (instead of just a virtqueue's index) when notifying the corresponding device. The data passed in by the driver when this feature is enabled varies in format depending on if the device is using a split or packed virtqueue layout: Split VQ - Upper 16 bits: shadow_avail_idx - Lower 16 bits: virtqueue index Packed VQ - Upper 16 bits: 1-bit wrap counter & 15-bit shadow_avail_idx - Lower 16 bits: virtqueue index Also, due to the limitations of ioeventfd not being able to carry the extra provided by the driver, having both VIRTIO_F_NOTIFICATION_DATA feature and ioeventfd enabled is a functional mismatch. The user must explicitly disable ioeventfd for the device in the Qemu arguments when using this feature, else the device will fail to complete realization. For example, a device must explicitly enable notification_data as well as disable ioeventfd: -device virtio-scsi-pci,...,ioeventfd=off,notification_data=on A significant aspect of this effort has been to maintain compatibility across different backends. As such, the feature is offered by backend devices only when supported, with fallback mechanisms where backend support is absent. v3: Validate VQ idx via. virtio_queue_get_num() (pci, mmio, ccw) Rename virtio_queue_set_shadow_avail_data Only pass in upper 16 bits of 32-bit extra data (was redundant) Make notification compatibility check function static Drop tags on patches 1/6, 3/6, and 4/6 v2: Don't disable ioeventfd by default, user must disable it Drop tags on patch 2/6 Jonah Palmer (6): virtio/virtio-pci: Handle extra notification data virtio: Prevent creation of device using notification-data with ioeventfd virtio-mmio: Handle extra notification data virtio-ccw: Handle extra notification data vhost/vhost-user: Add VIRTIO_F_NOTIFICATION_DATA to vhost feature bits virtio: Add VIRTIO_F_NOTIFICATION_DATA property definition Jonah, do you have kernel patches to add this feature as well? Thanks! Hi Jiri! I think there are already kernel patches for VIRTIO_F_NOTIFICATION_DATA, unless you're referring to something more specific that wasn't included in these patches: [1]: virtio: add VIRTIO_F_NOTIFICATION_DATA feature support https://lore.kernel.org/lkml/20230324195029.2410503-1-vik...@daynix.com/ [2]: virtio-vdpa: add VIRTIO_F_NOTIFICATION_DATA feature support https://lore.kernel.org/lkml/20230413081855.36643-3-alvaro.ka...@solid-run.com/ Jonah
[PATCH-for-9.1 v2 2/3] hw/display/pxa2xx_lcd: Set rotation angle using qemu_console_set_rotate
Signed-off-by: Philippe Mathieu-Daudé --- 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); -- 2.41.0
Re: [PATCH-for-9.1 3/3] ui/console: Add 'rotate_arcdegree' field to allow per-console rotation
On 18/3/24 11:36, Akihiko Odaki wrote: On 2024/03/18 19:05, Philippe Mathieu-Daudé wrote: Add the 'rotate_arcdegree' field to QemuConsole and remove the use of the 'graphic_rotate' global. I think QemuGraphicConsole is a better place to put the field. Good point. Regards, Akihiko Odaki
[PATCH-for-9.1 v2 3/3] ui/console: Add 'rotate_arcdegree' field to allow per-console rotation
Add the 'rotate_arcdegree' field to QemuGraphicConsole and remove the use of the 'graphic_rotate' global. Signed-off-by: Philippe Mathieu-Daudé --- 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) -- 2.41.0
[PATCH-for-9.1 v2 1/3] ui/console: Introduce API to change console orientation
Extract the following methods: - qemu_console_set_rotate() - qemu_console_is_rotated() - qemu_console_get_rotate_arcdegree() Signed-off-by: Philippe Mathieu-Daudé --- 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 */ -- 2.41.0
[PATCH-for-9.1 v2 0/3] ui/display: Introduce API to change console orientation
Since v1: - Move rotate_arcdegree to QemuGraphicConsole (Akihiko) Hi, The idea behind this series is to reduce the use of the 'graphic_rotate' global. It is only used by the Spitz machine, so we could convert the '-rotate' argument to a sugar property on the PXA2XX_LCD_TYPE model, but since the Spitz machine has been deprecated recently (commit a2531bb855 "Deprecate various old Arm machine types") it doesn't seem worthwhile. So just extract the API to change console orientation. Regards, Phil. Philippe Mathieu-Daudé (3): ui/console: Introduce API to change console orientation hw/display/pxa2xx_lcd: Set rotation angle using qemu_console_set_rotate ui/console: Add 'rotate_arcdegree' field to allow per-console rotation include/ui/console.h| 3 +++ hw/display/pxa2xx_lcd.c | 1 + ui/console.c| 23 +++ ui/input.c | 9 - 4 files changed, 31 insertions(+), 5 deletions(-) -- 2.41.0
Re: [PATCH for-9.0] mirror: Don't call job_pause_point() under graph lock
Am 14.03.2024 um 15:29 hat Stefan Hajnoczi geschrieben: > On Wed, Mar 13, 2024 at 04:30:00PM +0100, Kevin Wolf wrote: > > Calling job_pause_point() while holding the graph reader lock > > potentially results in a deadlock: bdrv_graph_wrlock() first drains > > everything, including the mirror job, which pauses it. The job is only > > unpaused at the end of the drain section, which is when the graph writer > > lock has been successfully taken. However, if the job happens to be > > paused at a pause point where it still holds the reader lock, the writer > > lock can't be taken as long as the job is still paused. > > > > Mark job_pause_point() as GRAPH_UNLOCKED and fix mirror accordingly. > > > > Cc: qemu-sta...@nongnu.org > > Buglink: https://issues.redhat.com/browse/RHEL-28125 > > Fixes: 004915a96a7a40e942ac85e6d22518cbcd283506 > > Signed-off-by: Kevin Wolf > > --- > > include/qemu/job.h | 2 +- > > block/mirror.c | 10 ++ > > 2 files changed, 7 insertions(+), 5 deletions(-) > > > > diff --git a/include/qemu/job.h b/include/qemu/job.h > > index 9ea98b5927..2b873f2576 100644 > > --- a/include/qemu/job.h > > +++ b/include/qemu/job.h > > @@ -483,7 +483,7 @@ void job_enter(Job *job); > > * > > * Called with job_mutex *not* held. > > */ > > -void coroutine_fn job_pause_point(Job *job); > > +void coroutine_fn GRAPH_UNLOCKED job_pause_point(Job *job); > > > > /** > > * @job: The job that calls the function. > > diff --git a/block/mirror.c b/block/mirror.c > > index 5145eb53e1..1bdce3b657 100644 > > --- a/block/mirror.c > > +++ b/block/mirror.c > > @@ -479,9 +479,9 @@ static unsigned mirror_perform(MirrorBlockJob *s, > > int64_t offset, > > return bytes_handled; > > } > > > > -static void coroutine_fn GRAPH_RDLOCK mirror_iteration(MirrorBlockJob *s) > > +static void coroutine_fn GRAPH_UNLOCKED mirror_iteration(MirrorBlockJob *s) > > { > > -BlockDriverState *source = s->mirror_top_bs->backing->bs; > > +BlockDriverState *source; > > MirrorOp *pseudo_op; > > int64_t offset; > > /* At least the first dirty chunk is mirrored in one iteration. */ > > @@ -489,6 +489,10 @@ static void coroutine_fn GRAPH_RDLOCK > > mirror_iteration(MirrorBlockJob *s) > > bool write_zeroes_ok = > > bdrv_can_write_zeroes_with_unmap(blk_bs(s->target)); > > int max_io_bytes = MAX(s->buf_size / MAX_IN_FLIGHT, MAX_IO_BYTES); > > > > +bdrv_graph_co_rdlock(); > > +source = s->mirror_top_bs->backing->bs; > > Is bdrv_ref(source) needed here so that source cannot go away if someone > else write locks the graph and removes it? Or maybe something else > protects against that. Either way, please add a comment that explains > why this is safe. We didn't even get to looking at this level of detail with the graph locking work. We probably should, but this is not the only place in mirror we need to look at then. Commit 004915a9 just took the lazy path of taking the lock for the whole function, and it turns out that this was wrong and causes deadlocks, so I'm reverting it and replacing it with what other parts of the code do - the minimal thing to let it compile. I think we already own a reference, we do a block_job_add_bdrv() in mirror_start_job(). But once it changes, we have a reference to the wrong node. So it looks to me that mirror has a problem with a changing source node that is more fundamental than graph locking in one specific function because it stores BDS pointers in its state. Active commit already freezes the backing chain between mirror_top_bs and target, maybe other mirror jobs need to freeze the link between mirror_top_bs and source at least. So I agree that it might be worth looking into this more, but I consider it unrelated to this patch. We just go back to the state in which it has always been before 8.2 (which might contain a latent bug that apparently never triggered in practice) to fix a regression that we do see in practice. Kevin signature.asc Description: PGP signature
Re: [PATCH] blockdev: Fix blockdev-snapshot-sync error reporting for no medium
Am 06.03.2024 um 15:28 hat Markus Armbruster geschrieben: > When external_snapshot_abort() rejects a BlockDriverState without a > medium, it creates an error like this: > > error_setg(errp, "Device '%s' has no medium", device); > > Trouble is @device can be null. My system formats null as "(null)", > but other systems might crash. Reproducer: > > 1. Create a block device without a medium > > -> {"execute": "blockdev-add", "arguments": {"driver": "host_cdrom", > "node-name": "blk0", "filename": "/dev/sr0"}} > <- {"return": {}} > > 3. Attempt to snapshot it > > -> {"execute":"blockdev-snapshot-sync", "arguments": { "node-name": > "blk0", "snapshot-file":"/tmp/foo.qcow2","format":"qcow2"}} > <- {"error": {"class": "GenericError", "desc": "Device '(null)' has no > medium"}} > > Broken when commit 0901f67ecdb made @device optional. > > Use bdrv_get_device_or_node_name() instead. Now it fails as it > should: > > <- {"error": {"class": "GenericError", "desc": "Device 'blk0' has no > medium"}} > > Fixes: 0901f67ecdb7 (qmp: Allow to take external snapshots on bs graphs node.) > Signed-off-by: Markus Armbruster Thanks, applied to the block branch. Kevin
Re: [PATCH 7/9] i386/sev: Refactor setting of reset vector and initial CPU state
On Tue, 2024-03-12 at 16:12 +, Daniel P. Berrangé wrote: > On Tue, Mar 12, 2024 at 03:45:20PM +, Roy Hopkins wrote: > > On Fri, 2024-03-01 at 17:01 +, Daniel P. Berrangé wrote: > > > On Tue, Feb 27, 2024 at 02:50:13PM +, Roy Hopkins wrote: > > > > + /* > > > > + * Ideally we would provide the VMSA directly to kvm which > > > > would > > > > + * ensure that the resulting initial VMSA measurement which > > > > is > > > > + * calculated during KVM_SEV_LAUNCH_UPDATE_VMSA is > > > > calculated > > > > from > > > > + * exactly what we provide here. Currently this is not > > > > possible > > > > so > > > > + * we need to copy the parts of the VMSA structure that we > > > > currently > > > > + * support into the CPU state. > > > > + */ > > > > > > This sounds like it is saying that the code is not honouring > > > everything in the VMSA defiend by the IGVM file ? > > > > > > If so, that is pretty awkward. The VMSA is effectively an external > > > ABI between QEMU and the guest owner (or whatever is validating > > > guest attestation reports for them), and thus predictability and > > > stability of this over time is critical. > > > > > > We don't want the attestation process to be dependent/variable on > > > the particular QEMU/KVM version, because any upgrade to QEMU/KVM > > > could then alter the effective VMSA that the guest owner sees. > > > > > > We've already suffered pain in this respect not long ago when the > > > kernel arbitrarily changed a default setting which altered the > > > VMSA it exposed, breaking existing apps that validate attestation. > > > > > > What will it take to provide the full VMSA to KVM, so that we can > > > guarantee to the guest owner than the VMSA for the guest is going > > > to perfectly match what their IGVM defined ? > > > > > > > Yes, the fact that we have to copy the individual fields from the VMSA to > > "CPUX86State" is less than ideal - a problem made worse by the fact that the > > kernel does not allow direct control over some of the fields from userspace, > > "sev_features" being a good example here where "SVM_SEV_FEAT_DEBUG_SWAP" is > > unconditionally added by the kernel. > > Ah yes, the SVM_SEV_FEAT_DEBUG_SWAP feature is the one I couldn't remember > the name of in my quoted text above, that break our apps when the kernel > suddenly set it by default (thankfully now reverted in Linux with > 5abf6dceb066f2b02b225fd561440c98a8062681). > > > The kernel VMSA is at least predictable. So, although we cannot yet allow > > full > > flexibility in providing a complete VMSA from QEMU and guarantee it will be > > honoured, we could check to see if any settings conflict with those imposed > > by > > the kernel and exit with an error if this is the case. I chose not to > > implement > > for this first series but could easily add a patch to support this. The > > problem > > here is that it ties the version of QEMU to VMSA handling functionality in > > the > > kernel. Any change to the VMSA handling in the kernel would potentially > > invalidate the checks in QEMU. The one upside here is that this will easily > > be > > detectable by the attestation measurement not matching the expected > > measurement > > of the IGVM file. But it will be difficult for the user to determine what > > the > > discrepancy is. > > Yes, the difficulty in diagnosis is the big thing I'm worried about from > a distro supportability POV. The DEBUG_SWAP issue caused us a bunch of > pain and that's before CVMs are even widely used. > > I agree that hardcoding checks in QEMU is pretty unpleasant, and probably > not something that I'd want us to do. I'd want QEMU to be able to live > query the kernel's default initial VMSA, if it were to be reporting > differences vs the IGVM provided VMSA. I dn't think there's a way to > do that nicely though - i only know of ftrace probes to dump it informally. > > I guess if we know & document what subset of the VMSA QEMU /can/ directly > control, that at least narrows down where to look if something does change > or go wrong. > Yes, it makes sense to document the subset that can be reliably set by QEMU, along with any modifications made byt the kernel. Perhaps I should go one step further and check that the VMSA does not contain any entries beyond what is copied in "sev_apply_cpu_context()"? If any field other than those explicitly copied by this function contain a non-zero value then an error is generated. As you suggest this will limit the scope of any measurement differences to the documented subset. > > The ideal solution is to add or modify a KVM ioctl to allow the VMSA to be > > set > > directly, overriding the state in "CPUX86State". The current > > KVM_SEV_LAUNCH_UPDATE_VMSA ioctl triggers the synchronisation of the VMSA > > but > > does not allow it to be specified directly. This could be modified for what > > we > > need. The SEV-SNP kernel patc
Re: [PATCH v4] qemu-img: Fix Column Width and Improve Formatting in snapshot list
Am 23.01.2024 um 06:03 hat Abhiram Tilak geschrieben: > When running the command `qemu-img snapshot -l SNAPSHOT` the output of > VM_CLOCK (measures the offset between host and VM clock) cannot to > accommodate values in the order of thousands (4-digit). > > This line [1] hints on the problem. Additionally, the column width for > the VM_CLOCK field was reduced from 15 to 13 spaces in commit b39847a5 > in line [2], resulting in a shortage of space. > > [1]: > https://gitlab.com/qemu-project/qemu/-/blob/master/block/qapi.c?ref_type=heads#L753 > [2]: > https://gitlab.com/qemu-project/qemu/-/blob/master/block/qapi.c?ref_type=heads#L763 > > This patch restores the column width to 15 spaces and makes adjustments > to the affected iotests accordingly. Furthermore, addresses a potential > source > of confusion by removing whitespace in column headers. Example, VM CLOCK > is modified to VM_CLOCK. Additionally a '--' symbol is introduced when > ICOUNT returns no output for clarity. > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2062 > Fixes: b39847a50553 (migration: introduce icount field for snapshots ) > Signed-off-by: Abhiram Tilak Thanks, applied to the block branch. Kevin
Re: [PATCH v3 for 9.1 0/6] virtio, vhost: Add VIRTIO_F_NOTIFICATION_DATA support
Mon, Mar 18, 2024 at 12:22:02PM CET, jonah.pal...@oracle.com wrote: > > >On 3/16/24 11:45 AM, Jiri Pirko wrote: >> Fri, Mar 15, 2024 at 05:55:51PM CET, jonah.pal...@oracle.com wrote: >> > The goal of these patches are to add support to a variety of virtio and >> > vhost devices for the VIRTIO_F_NOTIFICATION_DATA transport feature. This >> > feature indicates that a driver will pass extra data (instead of just a >> > virtqueue's index) when notifying the corresponding device. >> > >> > The data passed in by the driver when this feature is enabled varies in >> > format depending on if the device is using a split or packed virtqueue >> > layout: >> > >> > Split VQ >> > - Upper 16 bits: shadow_avail_idx >> > - Lower 16 bits: virtqueue index >> > >> > Packed VQ >> > - Upper 16 bits: 1-bit wrap counter & 15-bit shadow_avail_idx >> > - Lower 16 bits: virtqueue index >> > >> > Also, due to the limitations of ioeventfd not being able to carry the >> > extra provided by the driver, having both VIRTIO_F_NOTIFICATION_DATA >> > feature and ioeventfd enabled is a functional mismatch. The user must >> > explicitly disable ioeventfd for the device in the Qemu arguments when >> > using this feature, else the device will fail to complete realization. >> > >> > For example, a device must explicitly enable notification_data as well >> > as disable ioeventfd: >> > >> > -device virtio-scsi-pci,...,ioeventfd=off,notification_data=on >> > >> > A significant aspect of this effort has been to maintain compatibility >> > across different backends. As such, the feature is offered by backend >> > devices only when supported, with fallback mechanisms where backend >> > support is absent. >> > >> > v3: Validate VQ idx via. virtio_queue_get_num() (pci, mmio, ccw) >> > Rename virtio_queue_set_shadow_avail_data >> > Only pass in upper 16 bits of 32-bit extra data (was redundant) >> > Make notification compatibility check function static >> > Drop tags on patches 1/6, 3/6, and 4/6 >> > >> > v2: Don't disable ioeventfd by default, user must disable it >> > Drop tags on patch 2/6 >> > >> > Jonah Palmer (6): >> > virtio/virtio-pci: Handle extra notification data >> > virtio: Prevent creation of device using notification-data with ioeventfd >> > virtio-mmio: Handle extra notification data >> > virtio-ccw: Handle extra notification data >> > vhost/vhost-user: Add VIRTIO_F_NOTIFICATION_DATA to vhost feature bits >> > virtio: Add VIRTIO_F_NOTIFICATION_DATA property definition >> >> Jonah, do you have kernel patches to add this feature as well? >> >> Thanks! > >Hi Jiri! I think there are already kernel patches for >VIRTIO_F_NOTIFICATION_DATA, unless you're referring to something more >specific that wasn't included in these patches: > >[1]: virtio: add VIRTIO_F_NOTIFICATION_DATA feature support >https://lore.kernel.org/lkml/20230324195029.2410503-1-vik...@daynix.com/ > >[2]: virtio-vdpa: add VIRTIO_F_NOTIFICATION_DATA feature support >https://lore.kernel.org/lkml/20230413081855.36643-3-alvaro.ka...@solid-run.com/ I missed this. Thx! > >Jonah
Re: [PATCH 0/9] tests/qemu-iotests: Fix running with "check -ssh -qcow2"
Am 15.03.2024 um 12:10 hat Thomas Huth geschrieben: > I recently wanted to check for some changes that I did to the URI handling > in the block layer code, but I had to discover that a lot of iotests only > work with the raw file format when using a protocol that is not "file", > i.e. "./check -ssh -qcow2" shows a lot of failures. > While some tests could be fixed to work with the "ssh" protocol, too, > many other tests seem to be written for the "file" protocol only and > thus have to be marked accordingly. > > After applying these patches, there is still one failure left in test 181 > where I'm unsure whether it's a real bug or whether this test should also > simply be marked to work with the "file" protocol only. Suggestions are > welcome! > > Thomas Huth (9): > tests/qemu-iotests: Fix test 033 for running with non-file protocols > tests/qemu-iotests: Restrict test 066 to the 'file' protocol > tests/qemu-iotests: Restrict test 114 to the 'file' protocol > tests/qemu-iotests: Restrict test 130 to the 'file' protocol > tests/qemu-iotests: Restrict test 134 and 158 to the 'file' protocol > tests/qemu-iotests: Restrict test 156 to the 'file' protocol > tests/qemu-iotests: Restrict tests that use --image-opts to the 'file' > protocol > tests/qemu-iotests: Fix some tests that use --image-opts for other > protocols > tests/qemu-iotests: Restrict tests using "--blockdev file" to the file > protocol Thanks, applied to the block branch. Kevin