| Message ID | 20110227190312.GA14445@playa.redhat.com |
|---|---|
| State | New |
| Headers | show |
On 2011-02-27 20:03, Alon Levy wrote: > On Sat, Feb 26, 2011 at 01:29:01PM +0100, Jan Kiszka wrote: >> On 2011-02-26 12:43, xming wrote: >>> When trying to start X (and it loads qxl driver) the kvm process just crashes. > > This is fixed by Gerd's attached patch (taken from rhel repository, don't know > why it wasn't pushed to qemu-kvm upstream). I'll send it to kvm list as well (separate email). Patch looks OK on first glance, but the changelog is misleading: This was broken for _both_ trees, but upstream didn't detect the bug. My concerns regarding other side effects of juggling with global mutex in spice code remain. Jan
On Sun, Feb 27, 2011 at 08:11:26PM +0100, Jan Kiszka wrote: > On 2011-02-27 20:03, Alon Levy wrote: > > On Sat, Feb 26, 2011 at 01:29:01PM +0100, Jan Kiszka wrote: > >> On 2011-02-26 12:43, xming wrote: > >>> When trying to start X (and it loads qxl driver) the kvm process just crashes. > > > > This is fixed by Gerd's attached patch (taken from rhel repository, don't know > > why it wasn't pushed to qemu-kvm upstream). I'll send it to kvm list as well (separate email). > > Patch looks OK on first glance, but the changelog is misleading: This > was broken for _both_ trees, but upstream didn't detect the bug. > The trees the patch commit message refers to are qemu and qemu-kvm. qemu doesn't even have cpu_single_env. It didn't talk about two qemu-kvm trees. > My concerns regarding other side effects of juggling with global mutex > in spice code remain. I know there used to be a mutex in spice code and during the upstreaming process it got ditched in favor of the qemu global io mutex. I would have rather deferred this to Gerd since he wrote this, but he is not available atm. > > Jan >
On 2011-02-27 20:16, Alon Levy wrote: > On Sun, Feb 27, 2011 at 08:11:26PM +0100, Jan Kiszka wrote: >> On 2011-02-27 20:03, Alon Levy wrote: >>> On Sat, Feb 26, 2011 at 01:29:01PM +0100, Jan Kiszka wrote: >>>> On 2011-02-26 12:43, xming wrote: >>>>> When trying to start X (and it loads qxl driver) the kvm process just crashes. >>> >>> This is fixed by Gerd's attached patch (taken from rhel repository, don't know >>> why it wasn't pushed to qemu-kvm upstream). I'll send it to kvm list as well (separate email). >> >> Patch looks OK on first glance, but the changelog is misleading: This >> was broken for _both_ trees, but upstream didn't detect the bug. >> > > The trees the patch commit message refers to are qemu and qemu-kvm. The same did I. > qemu doesn't even have cpu_single_env. Really? Check again. :) > It didn't talk about two qemu-kvm trees. > >> My concerns regarding other side effects of juggling with global mutex >> in spice code remain. > > I know there used to be a mutex in spice code and during the upstreaming process it > got ditched in favor of the qemu global io mutex. I would have rather deferred this > to Gerd since he wrote this, but he is not available atm. It's not necessarily bad to drop the io mutex, but it is more tricky than it may appear on first glance. Jan
On Sun, Feb 27, 2011 at 08:27:01PM +0100, Jan Kiszka wrote: > On 2011-02-27 20:16, Alon Levy wrote: > > On Sun, Feb 27, 2011 at 08:11:26PM +0100, Jan Kiszka wrote: > >> On 2011-02-27 20:03, Alon Levy wrote: > >>> On Sat, Feb 26, 2011 at 01:29:01PM +0100, Jan Kiszka wrote: > >>>> On 2011-02-26 12:43, xming wrote: > >>>>> When trying to start X (and it loads qxl driver) the kvm process just crashes. > >>> > >>> This is fixed by Gerd's attached patch (taken from rhel repository, don't know > >>> why it wasn't pushed to qemu-kvm upstream). I'll send it to kvm list as well (separate email). > >> > >> Patch looks OK on first glance, but the changelog is misleading: This > >> was broken for _both_ trees, but upstream didn't detect the bug. > >> > > > > The trees the patch commit message refers to are qemu and qemu-kvm. > > The same did I. > > > qemu doesn't even have cpu_single_env. > > Really? Check again. :) Sorry, grepped the wrong repo. I'll send this to qemu-devel too then. > > > It didn't talk about two qemu-kvm trees. > > > >> My concerns regarding other side effects of juggling with global mutex > >> in spice code remain. > > > > I know there used to be a mutex in spice code and during the upstreaming process it > > got ditched in favor of the qemu global io mutex. I would have rather deferred this > > to Gerd since he wrote this, but he is not available atm. > > It's not necessarily bad to drop the io mutex, but it is more tricky > than it may appear on first glance. > > Jan >
On Sun, Feb 27, 2011 at 08:27:01PM +0100, Jan Kiszka wrote: > On 2011-02-27 20:16, Alon Levy wrote: > > On Sun, Feb 27, 2011 at 08:11:26PM +0100, Jan Kiszka wrote: > >> On 2011-02-27 20:03, Alon Levy wrote: > >>> On Sat, Feb 26, 2011 at 01:29:01PM +0100, Jan Kiszka wrote: > >>>> On 2011-02-26 12:43, xming wrote: > >>>>> When trying to start X (and it loads qxl driver) the kvm process just crashes. > >>> > >>> This is fixed by Gerd's attached patch (taken from rhel repository, don't know > >>> why it wasn't pushed to qemu-kvm upstream). I'll send it to kvm list as well (separate email). > >> > >> Patch looks OK on first glance, but the changelog is misleading: This > >> was broken for _both_ trees, but upstream didn't detect the bug. > >> > > > > The trees the patch commit message refers to are qemu and qemu-kvm. > > The same did I. > > > qemu doesn't even have cpu_single_env. > > Really? Check again. :) > > > It didn't talk about two qemu-kvm trees. > > > >> My concerns regarding other side effects of juggling with global mutex > >> in spice code remain. > > > > I know there used to be a mutex in spice code and during the upstreaming process it > > got ditched in favor of the qemu global io mutex. I would have rather deferred this > > to Gerd since he wrote this, but he is not available atm. > > It's not necessarily bad to drop the io mutex, but it is more tricky > than it may appear on first glance. The problem with not dropping it is that we may be in vga mode and create updates synthtically (i.e. qemu created and not driver created) that access the framebuffer and need to be locked so the framebuffer isn't updated at the same time. We drop the mutex only when we are about to call the dispatcher, which basically waits on red_worker (a libspice-server thread) to do some work. red_worker may in turn callback into qxl in qemu, which may try to acquire the lock. (the many may's here are just reflections of the codepaths). > > Jan >
On Sun, Feb 27, 2011 at 8:03 PM, Alon Levy <alevy@redhat.com> wrote: > On Sat, Feb 26, 2011 at 01:29:01PM +0100, Jan Kiszka wrote: >> On 2011-02-26 12:43, xming wrote: >> > When trying to start X (and it loads qxl driver) the kvm process just crashes. > > This is fixed by Gerd's attached patch (taken from rhel repository, don't know > why it wasn't pushed to qemu-kvm upstream). I'll send it to kvm list as well (separate email). I can confirm that this patch fixes the issue, thanks a lot cheers
On Sunday 27 February 2011 13:03:14 Alon Levy wrote: > On Sat, Feb 26, 2011 at 01:29:01PM +0100, Jan Kiszka wrote: > > On 2011-02-26 12:43, xming wrote: > > > When trying to start X (and it loads qxl driver) the kvm process just > > > crashes. > > This is fixed by Gerd's attached patch (taken from rhel repository, don't > know why it wasn't pushed to qemu-kvm upstream). I'll send it to kvm list > as well (separate email). > This patch also fixed https://bugs.launchpad.net/bugs/723871 I created the bug report on launchpad, but I suppose it should be left open until the patch hits qemu-kvm? -Rick
On Sun, Feb 27, 2011 at 08:11:26PM +0100, Jan Kiszka wrote: > On 2011-02-27 20:03, Alon Levy wrote: > > On Sat, Feb 26, 2011 at 01:29:01PM +0100, Jan Kiszka wrote: > >> On 2011-02-26 12:43, xming wrote: > >>> When trying to start X (and it loads qxl driver) the kvm process just crashes. > > > > This is fixed by Gerd's attached patch (taken from rhel repository, don't know > > why it wasn't pushed to qemu-kvm upstream). I'll send it to kvm list as well (separate email). > > Patch looks OK on first glance, but the changelog is misleading: This > was broken for _both_ trees, but upstream didn't detect the bug. So I didn't test with qemu not having this patch, but according to the discussion in the launchpad bug the problem only happens with qemu-kvm. This doesn't rule out it being a bug, perhaps it is just triggered much less frequently I guess. > > My concerns regarding other side effects of juggling with global mutex > in spice code remain. > > Jan >
On 2011-03-01 13:58, Alon Levy wrote: > On Sun, Feb 27, 2011 at 08:11:26PM +0100, Jan Kiszka wrote: >> On 2011-02-27 20:03, Alon Levy wrote: >>> On Sat, Feb 26, 2011 at 01:29:01PM +0100, Jan Kiszka wrote: >>>> On 2011-02-26 12:43, xming wrote: >>>>> When trying to start X (and it loads qxl driver) the kvm process just crashes. >>> >>> This is fixed by Gerd's attached patch (taken from rhel repository, don't know >>> why it wasn't pushed to qemu-kvm upstream). I'll send it to kvm list as well (separate email). >> >> Patch looks OK on first glance, but the changelog is misleading: This >> was broken for _both_ trees, but upstream didn't detect the bug. > > So I didn't test with qemu not having this patch, but according to the discussion in the > launchpad bug the problem only happens with qemu-kvm. This doesn't rule out it being a > bug, perhaps it is just triggered much less frequently I guess. Again: qemu-kvm has the instrumentation to detect the bug, qemu is lacking this, but both trees will break subtly if cpu_current_env is not properly restored. Jan
On Wed, Mar 02, 2011 at 09:22:35AM +0100, Jan Kiszka wrote: > On 2011-03-01 13:58, Alon Levy wrote: > > On Sun, Feb 27, 2011 at 08:11:26PM +0100, Jan Kiszka wrote: > >> On 2011-02-27 20:03, Alon Levy wrote: > >>> On Sat, Feb 26, 2011 at 01:29:01PM +0100, Jan Kiszka wrote: > >>>> On 2011-02-26 12:43, xming wrote: > >>>>> When trying to start X (and it loads qxl driver) the kvm process just crashes. > >>> > >>> This is fixed by Gerd's attached patch (taken from rhel repository, don't know > >>> why it wasn't pushed to qemu-kvm upstream). I'll send it to kvm list as well (separate email). > >> > >> Patch looks OK on first glance, but the changelog is misleading: This > >> was broken for _both_ trees, but upstream didn't detect the bug. > > > > So I didn't test with qemu not having this patch, but according to the discussion in the > > launchpad bug the problem only happens with qemu-kvm. This doesn't rule out it being a > > bug, perhaps it is just triggered much less frequently I guess. > > Again: qemu-kvm has the instrumentation to detect the bug, qemu is > lacking this, but both trees will break subtly if cpu_current_env is not > properly restored. ok, so what do you want to be done further before this patch is applied? > > Jan >
On 2011-03-02 11:56, Alon Levy wrote: > On Wed, Mar 02, 2011 at 09:22:35AM +0100, Jan Kiszka wrote: >> On 2011-03-01 13:58, Alon Levy wrote: >>> On Sun, Feb 27, 2011 at 08:11:26PM +0100, Jan Kiszka wrote: >>>> On 2011-02-27 20:03, Alon Levy wrote: >>>>> On Sat, Feb 26, 2011 at 01:29:01PM +0100, Jan Kiszka wrote: >>>>>> On 2011-02-26 12:43, xming wrote: >>>>>>> When trying to start X (and it loads qxl driver) the kvm process just crashes. >>>>> >>>>> This is fixed by Gerd's attached patch (taken from rhel repository, don't know >>>>> why it wasn't pushed to qemu-kvm upstream). I'll send it to kvm list as well (separate email). >>>> >>>> Patch looks OK on first glance, but the changelog is misleading: This >>>> was broken for _both_ trees, but upstream didn't detect the bug. >>> >>> So I didn't test with qemu not having this patch, but according to the discussion in the >>> launchpad bug the problem only happens with qemu-kvm. This doesn't rule out it being a >>> bug, perhaps it is just triggered much less frequently I guess. >> >> Again: qemu-kvm has the instrumentation to detect the bug, qemu is >> lacking this, but both trees will break subtly if cpu_current_env is not >> properly restored. > > ok, so what do you want to be done further before this patch is applied? The patch posted to qemu-devel just requires a changelog that correctly reflects what it addresses (and where). Jan
On Wed, Mar 02, 2011 at 12:34:24PM +0100, Jan Kiszka wrote: > On 2011-03-02 11:56, Alon Levy wrote: > > On Wed, Mar 02, 2011 at 09:22:35AM +0100, Jan Kiszka wrote: > >> On 2011-03-01 13:58, Alon Levy wrote: > >>> On Sun, Feb 27, 2011 at 08:11:26PM +0100, Jan Kiszka wrote: > >>>> On 2011-02-27 20:03, Alon Levy wrote: > >>>>> On Sat, Feb 26, 2011 at 01:29:01PM +0100, Jan Kiszka wrote: > >>>>>> On 2011-02-26 12:43, xming wrote: > >>>>>>> When trying to start X (and it loads qxl driver) the kvm process just crashes. > >>>>> > >>>>> This is fixed by Gerd's attached patch (taken from rhel repository, don't know > >>>>> why it wasn't pushed to qemu-kvm upstream). I'll send it to kvm list as well (separate email). > >>>> > >>>> Patch looks OK on first glance, but the changelog is misleading: This > >>>> was broken for _both_ trees, but upstream didn't detect the bug. > >>> > >>> So I didn't test with qemu not having this patch, but according to the discussion in the > >>> launchpad bug the problem only happens with qemu-kvm. This doesn't rule out it being a > >>> bug, perhaps it is just triggered much less frequently I guess. > >> > >> Again: qemu-kvm has the instrumentation to detect the bug, qemu is > >> lacking this, but both trees will break subtly if cpu_current_env is not > >> properly restored. > > > > ok, so what do you want to be done further before this patch is applied? > > The patch posted to qemu-devel just requires a changelog that correctly > reflects what it addresses (and where). Just sent, Alon > > Jan >
diff --git a/hw/qxl.c b/hw/qxl.c index fe4212b..117f7c8 100644 --- a/hw/qxl.c +++ b/hw/qxl.c @@ -125,6 +125,27 @@ static void qxl_reset_memslots(PCIQXLDevice *d); static void qxl_reset_surfaces(PCIQXLDevice *d); static void qxl_ring_set_dirty(PCIQXLDevice *qxl); +/* qemu-kvm locking ... */ +void qxl_unlock_iothread(SimpleSpiceDisplay *ssd) +{ + if (cpu_single_env) { + assert(ssd->env == NULL); + ssd->env = cpu_single_env; + cpu_single_env = NULL; + } + qemu_mutex_unlock_iothread(); +} + +void qxl_lock_iothread(SimpleSpiceDisplay *ssd) +{ + qemu_mutex_lock_iothread(); + if (ssd->env) { + assert(cpu_single_env == NULL); + cpu_single_env = ssd->env; + ssd->env = NULL; + } +} + static inline uint32_t msb_mask(uint32_t val) { uint32_t mask; @@ -662,10 +683,10 @@ static void qxl_hard_reset(PCIQXLDevice *d, int loadvm) dprint(d, 1, "%s: start%s\n", __FUNCTION__, loadvm ? " (loadvm)" : ""); - qemu_mutex_unlock_iothread(); + qxl_unlock_iothread(&d->ssd); d->ssd.worker->reset_cursor(d->ssd.worker); d->ssd.worker->reset_image_cache(d->ssd.worker); - qemu_mutex_lock_iothread(); + qxl_lock_iothread(&d->ssd); qxl_reset_surfaces(d); qxl_reset_memslots(d); @@ -795,9 +816,9 @@ static void qxl_reset_surfaces(PCIQXLDevice *d) { dprint(d, 1, "%s:\n", __FUNCTION__); d->mode = QXL_MODE_UNDEFINED; - qemu_mutex_unlock_iothread(); + qxl_unlock_iothread(&d->ssd); d->ssd.worker->destroy_surfaces(d->ssd.worker); - qemu_mutex_lock_iothread(); + qxl_lock_iothread(&d->ssd); memset(&d->guest_surfaces.cmds, 0, sizeof(d->guest_surfaces.cmds)); } @@ -866,9 +887,9 @@ static void qxl_destroy_primary(PCIQXLDevice *d) dprint(d, 1, "%s\n", __FUNCTION__); d->mode = QXL_MODE_UNDEFINED; - qemu_mutex_unlock_iothread(); + qxl_unlock_iothread(&d->ssd); d->ssd.worker->destroy_primary_surface(d->ssd.worker, 0); - qemu_mutex_lock_iothread(); + qxl_lock_iothread(&d->ssd); } static void qxl_set_mode(PCIQXLDevice *d, int modenr, int loadvm) @@ -938,10 +959,10 @@ static void ioport_write(void *opaque, uint32_t addr, uint32_t val) case QXL_IO_UPDATE_AREA: { QXLRect update = d->ram->update_area; - qemu_mutex_unlock_iothread(); + qxl_unlock_iothread(&d->ssd); d->ssd.worker->update_area(d->ssd.worker, d->ram->update_surface, &update, NULL, 0, 0); - qemu_mutex_lock_iothread(); + qxl_lock_iothread(&d->ssd); break; } case QXL_IO_NOTIFY_CMD: diff --git a/ui/spice-display.c b/ui/spice-display.c index 020b423..defe652 100644 --- a/ui/spice-display.c +++ b/ui/spice-display.c @@ -186,18 +186,18 @@ void qemu_spice_create_host_primary(SimpleSpiceDisplay *ssd) surface.mem = (intptr_t)ssd->buf; surface.group_id = MEMSLOT_GROUP_HOST; - qemu_mutex_unlock_iothread(); + qxl_unlock_iothread(ssd); ssd->worker->create_primary_surface(ssd->worker, 0, &surface); - qemu_mutex_lock_iothread(); + qxl_lock_iothread(ssd); } void qemu_spice_destroy_host_primary(SimpleSpiceDisplay *ssd) { dprint(1, "%s:\n", __FUNCTION__); - qemu_mutex_unlock_iothread(); + qxl_unlock_iothread(ssd); ssd->worker->destroy_primary_surface(ssd->worker, 0); - qemu_mutex_lock_iothread(); + qxl_lock_iothread(ssd); } void qemu_spice_vm_change_state_handler(void *opaque, int running, int reason) @@ -207,9 +207,9 @@ void qemu_spice_vm_change_state_handler(void *opaque, int running, int reason) if (running) { ssd->worker->start(ssd->worker); } else { - qemu_mutex_unlock_iothread(); + qxl_unlock_iothread(ssd); ssd->worker->stop(ssd->worker); - qemu_mutex_lock_iothread(); + qxl_lock_iothread(ssd); } ssd->running = running; } diff --git a/ui/spice-display.h b/ui/spice-display.h index aef0464..df74828 100644 --- a/ui/spice-display.h +++ b/ui/spice-display.h @@ -43,6 +43,9 @@ typedef struct SimpleSpiceDisplay { QXLRect dirty; int notify; int running; + + /* qemu-kvm locking ... */ + void *env; } SimpleSpiceDisplay; typedef struct SimpleSpiceUpdate { @@ -52,6 +55,9 @@ typedef struct SimpleSpiceUpdate { uint8_t *bitmap; } SimpleSpiceUpdate; +void qxl_unlock_iothread(SimpleSpiceDisplay *ssd); +void qxl_lock_iothread(SimpleSpiceDisplay *ssd); + int qemu_spice_rect_is_empty(const QXLRect* r); void qemu_spice_rect_union(QXLRect *dest, const QXLRect *r);