| Message ID | 1347346570-2469-3-git-send-email-alevy@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 09/11/12 08:56, Alon Levy wrote: > Until now we used only the agent to change the monitor count and each > monitor resolution. This patch introduces the qemu part of using the > device as the mediator instead of the agent via virtio-serial. > > Spice (>=0.11.5) calls the new QXLInterface::client_monitors_config, > generating an interrupt QXL_INTERRUPT_CLIENT_MONITORS_CONFIG which the > client indicates handling of (after reading from > QXLRom::client_monitors_config) by QXL_IO_CLIENT_MONITORS_CONFIG_DONE. I don't think an explicit handshake via QXL_IO_CLIENT_MONITORS_CONFIG_DONE is a good idea. How about this update protocol: qemu: (1) set QXLRom::client_monitors_config_updating (2) fill QXLRom::client_monitors_config (3) raise QXL_INTERRUPT_CLIENT_MONITORS_CONFIG (4) clear QXLRom::client_monitors_config_updating guest: (1) clear QXL_INTERRUPT_CLIENT_MONITORS_CONFIG bit in irq status (2) wait until QXLRom::client_monitors_config_updating is clear (3) parse QXLRom::client_monitors_config (4) check QXL_INTERRUPT_CLIENT_MONITORS_CONFIG bit in irq status (a) when set, goto (1). (b) when clear we are done. While thinking about it: I think we also don't need the guest capabilities. With the handshake gone we can update QXLRom::client_monitors_config unconditionally. We might want to notify spice-server when the guest flips the QXL_INTERRUPT_CLIENT_MONITORS_CONFIG bit in the irq mask, so we can route the event accordingly. Or we just route it unconditionally both ways and let the guest sort it (i.e. make vdagent ignore monitors config when the qxl kms driver is active). cheers, Gerd
> On 09/11/12 08:56, Alon Levy wrote: > > Until now we used only the agent to change the monitor count and > > each > > monitor resolution. This patch introduces the qemu part of using > > the > > device as the mediator instead of the agent via virtio-serial. > > > > Spice (>=0.11.5) calls the new > > QXLInterface::client_monitors_config, > > generating an interrupt QXL_INTERRUPT_CLIENT_MONITORS_CONFIG which > > the > > client indicates handling of (after reading from > > QXLRom::client_monitors_config) by > > QXL_IO_CLIENT_MONITORS_CONFIG_DONE. > > I don't think an explicit handshake via > QXL_IO_CLIENT_MONITORS_CONFIG_DONE is a good idea. Why? I don't see the below as being better - it just moves the checking to the guest, and racily. > > How about this update protocol: > > qemu: > (1) set QXLRom::client_monitors_config_updating > (2) fill QXLRom::client_monitors_config > (3) raise QXL_INTERRUPT_CLIENT_MONITORS_CONFIG > (4) clear QXLRom::client_monitors_config_updating > > guest: > (1) clear QXL_INTERRUPT_CLIENT_MONITORS_CONFIG bit in irq status > (2) wait until QXLRom::client_monitors_config_updating is clear > (3) parse QXLRom::client_monitors_config > (4) check QXL_INTERRUPT_CLIENT_MONITORS_CONFIG bit in irq status > (a) when set, goto (1). > (b) when clear we are done. > Hmm, you are making the guest more complicated instead of the host side, don't know if that's better. Point (2) is a busy wait, no? Also, guest will have to handle half old / half new configurations: qemu(1) qemu(2) qemu(3) qemu(4) qemu(1) qemu(2) start guest(1) guest(2) guest(3) reads half old half new qemu(2) complete qemu(3) qemu(4) guest(1) guest(2) guest(3) reads full new Thinking about it some more, I can complete the handling of outstanding client_monitors_config requests by having an additional struct (we don't need to keep the whole bunch, just the current and pending). That would make the logic similar to what you outlined (but reverse - qemu does the waiting and re-interrupt-ing). > While thinking about it: I think we also don't need the guest > capabilities. With the handshake gone we can update > QXLRom::client_monitors_config unconditionally. The capability is there for spice, not for qemu, I used it in qemu since it was already there. Spice needs it to know if to pass the message (VDAgentMonitorsConfig) to the guest agent or not. But if you just meant we don't need to check the capability in qemu code, I understand. > > We might want to notify spice-server when the guest flips the > QXL_INTERRUPT_CLIENT_MONITORS_CONFIG bit in the irq mask, so we can > route the event accordingly. That would mean even more complex logic, to delay spice-server from sending the monitors command to the guest while more data could be incoming (which can be multiple chunks comprising one message, so we must wait for it to complete before pushing the VDAgentMonitorsConfig message). > Or we just route it unconditionally > both > ways and let the guest sort it (i.e. make vdagent ignore monitors > config > when the qxl kms driver is active). Routing it only one way is simpler in spice code. In other words, I had a buggy version doing that and decided that I should just do it one way or the other to not bang my head on it. But it's also simpler to handle - what order are the events going to happen in the guest? Also, not only spice-vdagent plus our driver, but with, for instance, gnome-settings-daemon listening to the uevent from the kernel it will do modesetting by itself, racing with spice-vdagent. > > cheers, > Gerd > >
Hi, >> I don't think an explicit handshake via >> QXL_IO_CLIENT_MONITORS_CONFIG_DONE is a good idea. > > Why? I don't see the below as being better - it just moves the checking to the guest, and racily. It is more robust. We don't have to keep state in qxl for the handshake, one less opportunity for a buggy guest driver to screw up things. It is also closer to how real hardware handles this. Yes, there is a race, but we detect and handle that case, so it is no problem. >> >> How about this update protocol: >> >> qemu: >> (1) set QXLRom::client_monitors_config_updating >> (2) fill QXLRom::client_monitors_config >> (3) raise QXL_INTERRUPT_CLIENT_MONITORS_CONFIG >> (4) clear QXLRom::client_monitors_config_updating >> >> guest: >> (1) clear QXL_INTERRUPT_CLIENT_MONITORS_CONFIG bit in irq status >> (2) wait until QXLRom::client_monitors_config_updating is clear >> (3) parse QXLRom::client_monitors_config >> (4) check QXL_INTERRUPT_CLIENT_MONITORS_CONFIG bit in irq status >> (a) when set, goto (1). >> (b) when clear we are done. >> > > Hmm, you are making the guest more complicated instead of the host > side, don't know if that's better. > > Point (2) is a busy wait, no? The guest doesn't have to spin, it can yield(). But yes, better don't do that in a IRQ handler. > Also, guest will have to handle half old / half new configurations: It should not. > qemu(1) > qemu(2) start > guest(1) > guest(2) > guest(3) reads half old half new No, guest will wait at (2). But I just see it can happen the other way around though: guest starts reading and qemu starts updating in parallel. We need one additional step (3a): The guest needs to check QXLRom::client_monitors_config_updating is clear after parsing the data. When qemu updated the data while the guest was reading it one of the two conditions will be true: either (3a) if qemu is still busy updating or (4) if qemu finished updating. >> We might want to notify spice-server when the guest flips the >> QXL_INTERRUPT_CLIENT_MONITORS_CONFIG bit in the irq mask, so we can >> route the event accordingly. > > That would mean even more complex logic, to delay spice-server from > sending the monitors command to the guest while more data could be > incoming (which can be multiple chunks comprising one message, so we > must wait for it to complete before pushing the VDAgentMonitorsConfig > message). I don't understand. Why is the client capability bit fundamentally different from the irq mask? I'd expect a guest driver which supports it would set the irq mask bit and the capability bit at the same time, no? >> Or we just route it unconditionally both ways and let the guest >> sort it (i.e. make vdagent ignore monitors config when the qxl kms >> driver is active). > Routing it only one way is simpler in spice code. In other words, I > had a buggy version doing that and decided that I should just do it > one way or the other to not bang my head on it. But it's also simpler > to handle - what order are the events going to happen in the guest? > Also, not only spice-vdagent plus our driver, but with, for instance, > gnome-settings-daemon listening to the uevent from the kernel it will > do modesetting by itself, racing with spice-vdagent. I was thinking about spice-vdagent detecting the qxl kms driver is active, then just ignore all VDAgentMonitorsConfig messages unconditionally if this happens to be the case, so there will be no race. But if you think it is too complicated, no problem. Was just an idea. Notifying spice-server which way to route is fine with me too. cheers, Gerd
> Hi, > > >> I don't think an explicit handshake via > >> QXL_IO_CLIENT_MONITORS_CONFIG_DONE is a good idea. > > > > Why? I don't see the below as being better - it just moves the > > checking to the guest, and racily. > > It is more robust. I suggested a way for it to be as robust - I take robust to mean "no lost messages except for intermediate ones", which both solutions suffer from anyway. > We don't have to keep state in qxl for the > handshake You mean it's preferable to have state on the device (QXLRom::client_monitors_config_updating) rather then private qxl device state? > one less opportunity for a buggy guest driver to screw up > things. The logic you outline can be screwed up as well, the logic for writing this io is basically: qxl_display_copy_rom_client_monitors_config(qdev); qxl_crtc_set_from_monitors_config(qdev); outb(0, qdev->io_base + QXL_IO_CLIENT_MONITORS_CONFIG_DONE); > It is also closer to how real hardware handles this. I don't know how it is in real hardware. > > Yes, there is a race, but we detect and handle that case, so it is no > problem. > > >> > >> How about this update protocol: > >> > >> qemu: > >> (1) set QXLRom::client_monitors_config_updating > >> (2) fill QXLRom::client_monitors_config > >> (3) raise QXL_INTERRUPT_CLIENT_MONITORS_CONFIG > >> (4) clear QXLRom::client_monitors_config_updating > >> > >> guest: > >> (1) clear QXL_INTERRUPT_CLIENT_MONITORS_CONFIG bit in irq status > >> (2) wait until QXLRom::client_monitors_config_updating is clear > >> (3) parse QXLRom::client_monitors_config > >> (4) check QXL_INTERRUPT_CLIENT_MONITORS_CONFIG bit in irq status > >> (a) when set, goto (1). > >> (b) when clear we are done. > >> > > > > Hmm, you are making the guest more complicated instead of the host > > side, don't know if that's better. > > > > Point (2) is a busy wait, no? > > The guest doesn't have to spin, it can yield(). But yes, better > don't > do that in a IRQ handler. > > > Also, guest will have to handle half old / half new configurations: > > It should not. > > > qemu(1) > > qemu(2) start > > guest(1) > > guest(2) > > guest(3) reads half old half new > > No, guest will wait at (2). > > But I just see it can happen the other way around though: guest > starts > reading and qemu starts updating in parallel. We need one additional > step (3a): The guest needs to check > QXLRom::client_monitors_config_updating is clear after parsing the > data. > > When qemu updated the data while the guest was reading it one of the > two > conditions will be true: either (3a) if qemu is still busy updating > or > (4) if qemu finished updating. Making things more complicated in the host, qemu, means making the kernel driver in the guest simpler, so even though you have a good solution for the race you discovered I don't see why the alternative is worse. (answered point to point above). > > >> We might want to notify spice-server when the guest flips the > >> QXL_INTERRUPT_CLIENT_MONITORS_CONFIG bit in the irq mask, so we > >> can > >> route the event accordingly. > > > > That would mean even more complex logic, to delay spice-server from > > sending the monitors command to the guest while more data could be > > incoming (which can be multiple chunks comprising one message, so > > we > > must wait for it to complete before pushing the > > VDAgentMonitorsConfig > > message). > > I don't understand. Why is the client capability bit fundamentally > different from the irq mask? I'd expect a guest driver which > supports > it would set the irq mask bit and the capability bit at the same > time, no? You are right, so we can do away with the guest capabilities field, but remain with the guest capabilities api in spice, because spice depends on it for the routing decision. > > >> Or we just route it unconditionally both ways and let the guest > >> sort it (i.e. make vdagent ignore monitors config when the qxl kms > >> driver is active). > > > Routing it only one way is simpler in spice code. In other words, I > > had a buggy version doing that and decided that I should just do it > > one way or the other to not bang my head on it. But it's also > > simpler > > to handle - what order are the events going to happen in the guest? > > Also, not only spice-vdagent plus our driver, but with, for > > instance, > > gnome-settings-daemon listening to the uevent from the kernel it > > will > > do modesetting by itself, racing with spice-vdagent. > > I was thinking about spice-vdagent detecting the qxl kms driver is > active, then just ignore all VDAgentMonitorsConfig messages > unconditionally if this happens to be the case, so there will be no > race. But if you think it is too complicated, no problem. This sounds like a good idea, didn't think of it. But that leaves me fixing the vdagent code now :) Also, I guess I still think doing just one message looks simpler. > Was just an idea. Notifying spice-server which way to route is fine with me too. OK, if it's all the same to you I'll stick with spice-server routing the message. Overall, if you find this tedious I will switch to your suggestion since it isn't such a big deal. > > cheers, > Gerd > >
Hi,
Sorry for top posting, but trying to summarize this thread here.
I must say I like Gerd's approach, as it unifies code paths mostly,
instead of having yet another interface where we do 2 way capabilities
negotiation, with all the extra test matrix entries that would entice
for full testing, we keep things simple.
So we would have:
1) monitor config in rom space
2) QXL_INTERRUPT_CLIENT_MONITORS_CONFIG to tell the guest it is updated
3) Some way to avoid a new monitor config arriving and the guest being
busy reading the previous race.
4) The server will always update the monitor config in rom space
5) If the guest has not requested QXL_INTERRUPT_CLIENT_MONITORS_CONFIG
and there is an agent the server will send the monitor info to the
agent
Note an alternative to the handshake suggested is simply adding a crc
to the monitor config block. If that fails we hit the the (rare) race and
the guest re-reads it.
Regards,
Hans
> Hi, > > Sorry for top posting, but trying to summarize this thread here. > > I must say I like Gerd's approach, as it unifies code paths mostly, > instead of having yet another interface where we do 2 way > capabilities > negotiation, with all the extra test matrix entries that would entice > for full testing, we keep things simple. So you are suggesting to send the message to both parties, and ignore it in the guest agent if it sees a qxl device. That's the only way this works, since otherwise you need a handshake between spice and qxl: server (1) receive VDAgentMonitorsConfig config (2) call qxl->client_monitors_config (3) wait for qxl->client_monitors_config_not_acked <-- after timeout? when do we decide interrupt wasn't cleared due to guest not supporting it, or due to not enough time having passed? (4a) if timeout, send VDAgentMonitorsConfig to agent (4b) else done > > So we would have: > 1) monitor config in rom space > 2) QXL_INTERRUPT_CLIENT_MONITORS_CONFIG to tell the guest it is > updated > 3) Some way to avoid a new monitor config arriving and the guest > being > busy reading the previous race. > 4) The server will always update the monitor config in rom space > 5) If the guest has not requested > QXL_INTERRUPT_CLIENT_MONITORS_CONFIG > and there is an agent the server will send the monitor info to > the > agent > > Note an alternative to the handshake suggested is simply adding a crc > to the monitor config block. If that fails we hit the the (rare) race > and > the guest re-reads it. > > Regards, > > Hans > >
On 09/11/12 13:29, Alon Levy wrote: >> Hi, >> >>>> I don't think an explicit handshake via >>>> QXL_IO_CLIENT_MONITORS_CONFIG_DONE is a good idea. >>> >>> Why? I don't see the below as being better - it just moves the >>> checking to the guest, and racily. >> >> It is more robust. > > I suggested a way for it to be as robust - I take robust to mean "no > lost messages except for intermediate ones", which both solutions > suffer from anyway. > >> We don't have to keep state in qxl for the handshake > You mean it's preferable to have state on the device > (QXLRom::client_monitors_config_updating) rather then private qxl > device state? The difference is that QXLRom::client_monitors_config_updating is one-way, it informs the guest what qemu is doing. qxl->client_monitors_config_isr_in_progress depends on the guest doing the handshake correctly. >> It is also closer to how real hardware handles this. > > I don't know how it is in real hardware. Modern hardware would stuff this into an event queue, simliar to a virtqueue. Raise a ring-full IRQ in case it can't. Two monitor config updates in a row would simply result in two event queue entries. Setting up a new spice ring just for that is probably overkill though. But (maybe you've seen it) for a virtio-based qxl design I would clearly pass that kind of data as virtqueue payload in a event queue, so a new update wouldn't overwrite the previous one. >> When qemu updated the data while the guest was reading it one of >> the two conditions will be true: either (3a) if qemu is still busy >> updating or (4) if qemu finished updating. > > Making things more complicated in the host, qemu, means making the > kernel driver in the guest simpler, so even though you have a good > solution for the race you discovered I don't see why the alternative > is worse. (answered point to point above). Real hardware tends to be the other way around. >> I was thinking about spice-vdagent detecting the qxl kms driver is >> active, then just ignore all VDAgentMonitorsConfig messages >> unconditionally if this happens to be the case, so there will be >> no race. But if you think it is too complicated, no problem. > > This sounds like a good idea, didn't think of it. But that leaves me > fixing the vdagent code now :) Also, I guess I still think doing just > one message looks simpler. > >> Was just an idea. Notifying spice-server which way to route is >> fine with me too. > > OK, if it's all the same to you I'll stick with spice-server routing > the message. > > Overall, if you find this tedious I will switch to your suggestion > since it isn't such a big deal. I think sending both ways unconditionally could make things easier. For starters you'll have valid data in QXLRom unconditionally, even in case the spice client has sent it before the qxl kms driver was loaded and signaled support (this will only work in case the update is handshake-free). You have a better view on the big picture though, how this all interacts vdagent starting and vdagent capability negotiation. cheers, Gerd
> > Hi, > > > > Sorry for top posting, but trying to summarize this thread here. > > > > I must say I like Gerd's approach, as it unifies code paths mostly, > > instead of having yet another interface where we do 2 way > > capabilities > > negotiation, with all the extra test matrix entries that would > > entice > > for full testing, we keep things simple. > > So you are suggesting to send the message to both parties, and ignore > it in the guest agent if it sees a qxl device. s/device/drm device file/ - this is linux specific right now, for windows guests we would check for something else presumably (not interesting right now). > That's the only way > this works, since otherwise you need a handshake between spice and > qxl: > > server > (1) receive VDAgentMonitorsConfig config > (2) call qxl->client_monitors_config > (3) wait for qxl->client_monitors_config_not_acked <-- after timeout? > when do we decide interrupt wasn't cleared due to guest not > supporting it, or due to not enough time having passed? > (4a) if timeout, send VDAgentMonitorsConfig to agent > (4b) else done > > > > > So we would have: > > 1) monitor config in rom space > > 2) QXL_INTERRUPT_CLIENT_MONITORS_CONFIG to tell the guest it is > > updated > > 3) Some way to avoid a new monitor config arriving and the guest > > being > > busy reading the previous race. > > 4) The server will always update the monitor config in rom space > > 5) If the guest has not requested > > QXL_INTERRUPT_CLIENT_MONITORS_CONFIG > > and there is an agent the server will send the monitor info to > > the > > agent > > > > Note an alternative to the handshake suggested is simply adding a > > crc > > to the monitor config block. If that fails we hit the the (rare) > > race > > and > > the guest re-reads it. > > > > Regards, > > > > Hans > > > > > >
Hi, On 09/11/2012 02:03 PM, Alon Levy wrote: >> Hi, >> >> Sorry for top posting, but trying to summarize this thread here. >> >> I must say I like Gerd's approach, as it unifies code paths mostly, >> instead of having yet another interface where we do 2 way >> capabilities >> negotiation, with all the extra test matrix entries that would entice >> for full testing, we keep things simple. > > So you are suggesting to send the message to both parties, and ignore it in the guest agent if it sees a qxl device. No, slightly more sophisticated, what I'm suggesting is that the server will not forward the VDAgentMonitorsConfig to the agent, if the qxl-dev's int_mask contains QXL_INTERRUPT_CLIENT_MONITORS_CONFIG. This should work because either we have a QXL_INTERRUPT_CLIENT_MONITORS_CONFIG aware driver, ie the new kms driver and it sets QXL_INTERRUPT_CLIENT_MONITORS_CONFIG in int_mask, or we don't and then the right thing to do is send the message to the agent to deal with it. This will need spice-server <-> qemu/hw/qxl.c coordination, but I assume we will need some changes there anyway to set QXL_INTERRUPT_CLIENT_MONITORS_CONFIG, the qemu/hw/qxl.c function patching the MonitorsConfig into the romspace, and setting the irq in int_pending, could return whether or not the flag was set in int_mask for example. Regards, Hans
Hi, > This will need spice-server <-> qemu/hw/qxl.c coordination, but I assume we > will need some changes there anyway to set > QXL_INTERRUPT_CLIENT_MONITORS_CONFIG, > the qemu/hw/qxl.c function patching the MonitorsConfig into the romspace, > and setting the irq in int_pending, could return whether or not the flag > was > set in int_mask for example. Yes, interface_client_monitors_config() can return whenever the guest has masked the interrupt or not, and based on that spice-server can send the message to the agent or not. cheers, Gerd
> Hi, > > > This will need spice-server <-> qemu/hw/qxl.c coordination, but I > > assume we > > will need some changes there anyway to set > > QXL_INTERRUPT_CLIENT_MONITORS_CONFIG, > > the qemu/hw/qxl.c function patching the MonitorsConfig into the > > romspace, > > and setting the irq in int_pending, could return whether or not the > > flag > > was > > set in int_mask for example. > > Yes, interface_client_monitors_config() can return whenever the guest > has masked the interrupt or not, and based on that spice-server can > send > the message to the agent or not. ok, I'm missing something here. (and trying to catch up via Vol 3A is taking too long). I thought the order is: (1) qemu raises interrupt (2) qemu calls kvm ioctl (3) guest interrupt handler (4) guest clears interrupt by writing ~0 to qxl ram_header->int_mask. (5) qemu detects this next time it raises interrupt. so where does qemu/hw/qxl.c get a chance to see this masking *immediately* after it raises the interrupt, i.e. before (2) above, since otherwise there is a timeout here, you need to add a callback, it gets complicated, and then the unconditional two way sending looks much better. (I'm already on the same page with you on not needing guest capabilities at this point, even though for the future it did look like a good thing to have). > > cheers, > Gerd > > >
> ok, I'm missing something here. (and trying to catch up via Vol 3A is taking too long). > I thought the order is: > (1) qemu raises interrupt > (2) qemu calls kvm ioctl > (3) guest interrupt handler > (4) guest clears interrupt by writing ~0 to qxl ram_header->int_mask. > (5) qemu detects this next time it raises interrupt. > so where does qemu/hw/qxl.c get a chance to see this masking > *immediately* after it raises the interrupt, i.e. before (2) above, > since otherwise there is a timeout here, you need to add a callback, > it gets complicated, and then the unconditional two way sending looks > much better. (I'm already on the same page with you on not needing > guest capabilities at this point, even though for the future it did > look like a good thing to have). There are two registers: (1) the interrupt enable register (aka ram->int_mask) (2) the interrupt status register (aka ram->int_pending) qemu sets the irq bit in the status register each time the irq condition is meet. qemu actually raises an irq in case the guest has the irq bit set in the enable register. guest acks the irq by clearing the irq bit in the status register (then issue QXL_IO_UPDATE_IRQ to notify qemu that it touched interrupt registers, which we need because our registers in memory not mmio space). So qxl can simply look at the enable register bit to figure whenever the guest is interested in specific interrupts or not. cheers, Gerd
> > ok, I'm missing something here. (and trying to catch up via Vol 3A > > is taking too long). > > I thought the order is: > > (1) qemu raises interrupt > > (2) qemu calls kvm ioctl > > (3) guest interrupt handler > > (4) guest clears interrupt by writing ~0 to qxl > > ram_header->int_mask. > > (5) qemu detects this next time it raises interrupt. > > > so where does qemu/hw/qxl.c get a chance to see this masking > > *immediately* after it raises the interrupt, i.e. before (2) above, > > since otherwise there is a timeout here, you need to add a > > callback, > > it gets complicated, and then the unconditional two way sending > > looks > > much better. (I'm already on the same page with you on not needing > > guest capabilities at this point, even though for the future it did > > look like a good thing to have). > > There are two registers: > > (1) the interrupt enable register (aka ram->int_mask) > (2) the interrupt status register (aka ram->int_pending) > > qemu sets the irq bit in the status register each time the irq > condition > is meet. qemu actually raises an irq in case the guest has the irq > bit > set in the enable register. guest acks the irq by clearing the irq > bit > in the status register (then issue QXL_IO_UPDATE_IRQ to notify qemu > that > it touched interrupt registers, which we need because our registers > in > memory not mmio space). > > So qxl can simply look at the enable register bit to figure whenever > the > guest is interested in specific interrupts or not. Hans and myself discussed offline the current windows driver implementation. In short, it sets ram->int_mask to ~0, thereby claiming to support all 32 interrupts (including those we haven't thought of yet..). > > cheers, > Gerd > >
Hi, On 09/11/2012 03:05 PM, Alon Levy wrote: >>> ok, I'm missing something here. (and trying to catch up via Vol 3A >>> is taking too long). >>> I thought the order is: >>> (1) qemu raises interrupt >>> (2) qemu calls kvm ioctl >>> (3) guest interrupt handler >>> (4) guest clears interrupt by writing ~0 to qxl >>> ram_header->int_mask. >>> (5) qemu detects this next time it raises interrupt. >> >>> so where does qemu/hw/qxl.c get a chance to see this masking >>> *immediately* after it raises the interrupt, i.e. before (2) above, >>> since otherwise there is a timeout here, you need to add a >>> callback, >>> it gets complicated, and then the unconditional two way sending >>> looks >>> much better. (I'm already on the same page with you on not needing >>> guest capabilities at this point, even though for the future it did >>> look like a good thing to have). >> >> There are two registers: >> >> (1) the interrupt enable register (aka ram->int_mask) >> (2) the interrupt status register (aka ram->int_pending) >> >> qemu sets the irq bit in the status register each time the irq >> condition >> is meet. qemu actually raises an irq in case the guest has the irq >> bit >> set in the enable register. guest acks the irq by clearing the irq >> bit >> in the status register (then issue QXL_IO_UPDATE_IRQ to notify qemu >> that >> it touched interrupt registers, which we need because our registers >> in >> memory not mmio space). >> >> So qxl can simply look at the enable register bit to figure whenever >> the >> guest is interested in specific interrupts or not. > > Hans and myself discussed offline the current windows driver implementation. In short, it sets ram->int_mask to ~0, thereby claiming to support all 32 interrupts (including those we haven't thought of yet..). Right, thinking more about this, this means that the don't send it to the agent when QXL_INTERRUPT_CLIENT_MONITORS_CONFIG is set in mask trick won't work, for windows with an older driver. I suggest rather then doing the whole capabilities dance, we simply detect the (older) windows driver (mask == ~0), and then treat that as QXL_INTERRUPT_CLIENT_MONITORS_CONFIG not being set in mask, a bit of hack but still much simpler then adding a full capabilities interface. If windows ever wants to actually support CLIENT_MONITORS_CONFIG through the driver rather then trough the agent, the driver will need updating anyways and we can then drop the ~0 replacing it with the proper mask. Regards, Hans
> Hi, > > On 09/11/2012 03:05 PM, Alon Levy wrote: > >>> ok, I'm missing something here. (and trying to catch up via Vol > >>> 3A > >>> is taking too long). > >>> I thought the order is: > >>> (1) qemu raises interrupt > >>> (2) qemu calls kvm ioctl > >>> (3) guest interrupt handler > >>> (4) guest clears interrupt by writing ~0 to qxl > >>> ram_header->int_mask. > >>> (5) qemu detects this next time it raises interrupt. > >> > >>> so where does qemu/hw/qxl.c get a chance to see this masking > >>> *immediately* after it raises the interrupt, i.e. before (2) > >>> above, > >>> since otherwise there is a timeout here, you need to add a > >>> callback, > >>> it gets complicated, and then the unconditional two way sending > >>> looks > >>> much better. (I'm already on the same page with you on not > >>> needing > >>> guest capabilities at this point, even though for the future it > >>> did > >>> look like a good thing to have). > >> > >> There are two registers: > >> > >> (1) the interrupt enable register (aka ram->int_mask) > >> (2) the interrupt status register (aka ram->int_pending) > >> > >> qemu sets the irq bit in the status register each time the irq > >> condition > >> is meet. qemu actually raises an irq in case the guest has the > >> irq > >> bit > >> set in the enable register. guest acks the irq by clearing the > >> irq > >> bit > >> in the status register (then issue QXL_IO_UPDATE_IRQ to notify > >> qemu > >> that > >> it touched interrupt registers, which we need because our > >> registers > >> in > >> memory not mmio space). > >> > >> So qxl can simply look at the enable register bit to figure > >> whenever > >> the > >> guest is interested in specific interrupts or not. > > > > Hans and myself discussed offline the current windows driver > > implementation. In short, it sets ram->int_mask to ~0, thereby > > claiming to support all 32 interrupts (including those we haven't > > thought of yet..). > > Right, thinking more about this, this means that the don't send it to > the agent when QXL_INTERRUPT_CLIENT_MONITORS_CONFIG is set in mask > trick > won't work, for windows with an older driver. > > I suggest rather then doing the whole capabilities dance, we simply > detect > the (older) windows driver (mask == ~0), and then treat that as > QXL_INTERRUPT_CLIENT_MONITORS_CONFIG not being set in mask, a bit of > hack > but still much simpler then adding a full capabilities interface. > > If windows ever wants to actually support CLIENT_MONITORS_CONFIG > through > the driver rather then trough the agent, the driver will need > updating > anyways and we can then drop the ~0 replacing it with the proper > mask. That sounds good, so I'll make sure the kms driver doesn't have this bug. Updated patches for spice-protocol, spice & qemu coming up. > > Regards, > > Hans > > > > > > >
diff --git a/hw/qxl.c b/hw/qxl.c index 583a2d9..18c63ff 100644 --- a/hw/qxl.c +++ b/hw/qxl.c @@ -34,6 +34,7 @@ #ifndef CONFIG_QXL_IO_CLIENT_MONITORS_CONFIG #define QXL_IO_CAPABILITIES_SET (QXL_IO_FLUSH_RELEASE + 2) +#define QXL_IO_CLIENT_MONITORS_CONFIG_DONE (QXL_IO_FLUSH_RELEASE + 3) #endif /* @@ -596,6 +597,8 @@ static const char *io_port_to_string(uint32_t io_port) [QXL_IO_FLUSH_RELEASE] = "QXL_IO_FLUSH_RELEASE", [QXL_IO_MONITORS_CONFIG_ASYNC] = "QXL_IO_MONITORS_CONFIG_ASYNC", [QXL_IO_CAPABILITIES_SET] = "QXL_IO_CAPABILITIES_SET", + [QXL_IO_CLIENT_MONITORS_CONFIG_DONE] = + "QXL_IO_CLIENT_MONITORS_CONFIG_DONE", }; if (io_port >= ARRAY_SIZE(io_port_to_string)) { @@ -986,6 +989,64 @@ static void interface_set_client_capabilities(QXLInstance *sin, #endif +#if defined(CONFIG_QXL_IO_CLIENT_MONITORS_CONFIG) \ + && SPICE_SERVER_VERSION >= 0x000b05 +static int qxl_test_guest_cap(PCIQXLDevice *qxl, unsigned cap) +{ + int ind = cap / 8; + uint8_t mask = 1 << (cap & 7); + + if (ind >= sizeof(qxl->ram->guest_capabilities)) { + return 0; + } + return qxl->ram->guest_capabilities[ind] & mask; +} + +/* called from main context only */ +static void interface_client_monitors_config(QXLInstance *sin, + VDAgentMonitorsConfig *monitors_config) +{ + PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl); + QXLRom *rom = memory_region_get_ram_ptr(&qxl->rom_bar); + int i; + + if (!qxl_test_guest_cap(qxl, QXL_GUEST_CAP_CLIENT_MONITORS_CONFIG_ISR)) { + trace_qxl_client_monitors_config_unsupported_by_guest(qxl->id, + monitors_config); + return; + } + if (qxl->client_monitors_config_isr_in_progress) { + trace_qxl_client_monitors_config_ignored_guest_busy(qxl->id, + monitors_config); + return; + } + rom->client_monitors_config.count = monitors_config->num_of_monitors; + /* monitors_config->flags ignored */ + if (rom->client_monitors_config.count >= + ARRAY_SIZE(rom->client_monitors_config.heads)) { + trace_qxl_client_monitors_config_capped(qxl->id, + monitors_config->num_of_monitors, + ARRAY_SIZE(rom->client_monitors_config.heads)); + rom->client_monitors_config.count = + ARRAY_SIZE(rom->client_monitors_config.heads); + } + for (i = 0 ; i < rom->client_monitors_config.count ; ++i) { + VDAgentMonConfig *monitor = &monitors_config->monitors[i]; + QXLURect *rect = &rom->client_monitors_config.heads[i]; + /* monitor->depth ignored */ + rect->left = monitor->x; + rect->top = monitor->y; + rect->right = monitor->x + monitor->width; + rect->bottom = monitor->y + monitor->height; + } + trace_qxl_interrupt_client_monitors_config(qxl->id, + rom->client_monitors_config.count, + rom->client_monitors_config.heads); + qxl->client_monitors_config_isr_in_progress = 1; + qxl_send_events(qxl, QXL_INTERRUPT_CLIENT_MONITORS_CONFIG); +} +#endif + static const QXLInterface qxl_interface = { .base.type = SPICE_INTERFACE_QXL, .base.description = "qxl gpu", @@ -1010,6 +1071,10 @@ static const QXLInterface qxl_interface = { #if SPICE_SERVER_VERSION >= 0x000b04 .set_client_capabilities = interface_set_client_capabilities, #endif +#if SPICE_SERVER_VERSION >= 0x000b05 && \ + defined(CONFIG_QXL_IO_CLIENT_MONITORS_CONFIG) + .client_monitors_config = interface_client_monitors_config, +#endif }; static void qxl_enter_vga_mode(PCIQXLDevice *d) @@ -1604,6 +1669,16 @@ async_common: case QXL_IO_CAPABILITIES_SET: qxl_spice_guest_capabilities_set(d); break; + case QXL_IO_CLIENT_MONITORS_CONFIG_DONE: + trace_qxl_io_client_monitors_config_done(d->id); + if (!d->client_monitors_config_isr_in_progress) { + qxl_set_guest_bug(d, + "%s: unexpected QXL_IO_CLIENT_MONITORS_CONFIG_DONE\n", + __func__); + } else { + d->client_monitors_config_isr_in_progress = 0; + } + break; default: qxl_set_guest_bug(d, "%s: unexpected ioport=0x%x\n", __func__, io_port); } diff --git a/hw/qxl.h b/hw/qxl.h index 5553824..a1ede3c 100644 --- a/hw/qxl.h +++ b/hw/qxl.h @@ -72,6 +72,8 @@ typedef struct PCIQXLDevice { QXLPHYSICAL guest_monitors_config; + int client_monitors_config_isr_in_progress; + QemuMutex track_lock; /* thread signaling */ diff --git a/trace-events b/trace-events index 7a35f62..4ea7cdd 100644 --- a/trace-events +++ b/trace-events @@ -928,6 +928,7 @@ qxl_io_read_unexpected(int qid) "%d" qxl_io_unexpected_vga_mode(int qid, uint64_t addr, uint64_t val, const char *desc) "%d 0x%"PRIx64"=%"PRIu64" (%s)" qxl_io_write(int qid, const char *mode, uint64_t addr, uint64_t val, unsigned size, int async) "%d %s addr=%"PRIu64 " val=%"PRIu64" size=%u async=%d" qxl_io_capabilities_set(int qid, int caps_size, uint8_t *caps) "%d %d %p" +qxl_io_client_monitors_config_done(int qid) "%d" qxl_memslot_add_guest(int qid, uint32_t slot_id, uint64_t guest_start, uint64_t guest_end) "%d %u: guest phys 0x%"PRIx64 " - 0x%" PRIx64 qxl_post_load(int qid, const char *mode) "%d %s" qxl_pre_load(int qid) "%d" @@ -968,6 +969,10 @@ qxl_spice_update_area_rest(int qid, uint32_t num_dirty_rects, uint32_t clear_dir qxl_surfaces_dirty(int qid, int surface, int offset, int size) "%d surface=%d offset=%d size=%d" qxl_send_events(int qid, uint32_t events) "%d %d" qxl_set_guest_bug(int qid) "%d" +qxl_interrupt_client_monitors_config(int qid, int num_heads, void *heads) "%d %d %p" +qxl_client_monitors_config_unsupported_by_guest(int qid, void *client_monitors_config) "%d %p" +qxl_client_monitors_config_ignored_guest_busy(int qid, void *client_monitors_config) "%d %p" +qxl_client_monitors_config_capped(int qid, int requested, int limit) "%d %d %d" # hw/qxl-render.c qxl_render_blit_guest_primary_initialized(void) ""
Until now we used only the agent to change the monitor count and each monitor resolution. This patch introduces the qemu part of using the device as the mediator instead of the agent via virtio-serial. Spice (>=0.11.5) calls the new QXLInterface::client_monitors_config, generating an interrupt QXL_INTERRUPT_CLIENT_MONITORS_CONFIG which the client indicates handling of (after reading from QXLRom::client_monitors_config) by QXL_IO_CLIENT_MONITORS_CONFIG_DONE. The maximal number of monitors is limited on the QXLRom to 63. QXL checks the existance of a QXL_GUEST_CAP_CLIENT_MONITORS_CONFIG_ISR before issuing the interrupt. (this is in addition to spice checking for the same capability). Signed-off-by: Alon Levy <alevy@redhat.com> --- hw/qxl.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ hw/qxl.h | 2 ++ trace-events | 5 ++++ 3 files changed, 82 insertions(+)