[Spice-devel,spice-protocol,v2,0.12.2,1/2] qxl_dev.h: add client monitors configuration notification to guest

Submitted by Alon Levy on Sept. 11, 2012, 2:35 p.m.

Details

Message ID 1347374153-29974-1-git-send-email-alevy@redhat.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Alon Levy Sept. 11, 2012, 2:35 p.m.
So far we have used the agent to notify the guest of a request to change
the monitors configurations (heads) on the qxl device. This patch introduces
a new interrupt and new fields in the qxl rom to notify the guest about
a new request, similarly to how physical hardware notifies the driver.

To avoid overwriting the rom while the guest is reading it there is a
client_monitors_config_updating field in ROM. The update protocol is:

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 that QXLRom::client_monitors_Config_updating is clear
      (a) when set, goto (1)
  (5) check QXL_INTERRUPT_CLIENT_MONITORS_CONFIG bit in irq status
      (a) when set, goto (1)
      (b) when clear we are done

If the interrupt mask is ~0 or 0 we will assume a guest that doesn't support
the new interrupt. If it's mask is neither but doesn't set
QXL_INTERRUPT_CLIENT_MONITORS_CONFIG, we also assume it doesn't support this
interrupt.
---
 spice/qxl_dev.h | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

Patch hide | download patch | download mbox

diff --git a/spice/qxl_dev.h b/spice/qxl_dev.h
index 50784dc..035f86a 100644
--- a/spice/qxl_dev.h
+++ b/spice/qxl_dev.h
@@ -125,6 +125,13 @@  typedef struct SPICE_ATTR_PACKED QXLRect {
     int32_t right;
 } QXLRect;
 
+typedef struct SPICE_ATTR_PACKED QXLURect {
+    uint32_t top;
+    uint32_t left;
+    uint32_t bottom;
+    uint32_t right;
+} QXLURect;
+
 /* qxl-1 compat: append only */
 typedef struct SPICE_ATTR_PACKED QXLRom {
     uint32_t magic;
@@ -151,6 +158,12 @@  typedef struct SPICE_ATTR_PACKED QXLRom {
     /* appended for qxl-4 */
     uint8_t client_present;
     uint8_t client_capabilities[58];
+    uint8_t client_monitors_config_updating;
+    uint8_t padding[1];
+    struct {
+        uint16_t count;
+        QXLURect heads[64];
+    } client_monitors_config;
 } QXLRom;
 
 /* qxl-1 compat: fixed */
@@ -234,6 +247,9 @@  SPICE_RING_DECLARE(QXLReleaseRing, uint64_t, QXL_RELEASE_RING_SIZE);
 #define QXL_INTERRUPT_IO_CMD (1 << 2)
 #define QXL_INTERRUPT_ERROR  (1 << 3)
 #define QXL_INTERRUPT_CLIENT (1 << 4)
+#define QXL_INTERRUPT_CLIENT_MONITORS_CONFIG  (1 << 5)
+
+#define QXL_GUEST_CAP_CLIENT_MONITORS_CONFIG_ISR 0
 
 /* qxl-1 compat: append only */
 typedef struct SPICE_ATTR_PACKED QXLRam {

Comments

Hi,

On 09/11/2012 04:35 PM, Alon Levy wrote:
> So far we have used the agent to notify the guest of a request to change
> the monitors configurations (heads) on the qxl device. This patch introduces
> a new interrupt and new fields in the qxl rom to notify the guest about
> a new request, similarly to how physical hardware notifies the driver.
>
> To avoid overwriting the rom while the guest is reading it there is a
> client_monitors_config_updating field in ROM. The update protocol is:
>
> 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 that QXLRom::client_monitors_Config_updating is clear
>        (a) when set, goto (1)
>    (5) check QXL_INTERRUPT_CLIENT_MONITORS_CONFIG bit in irq status
>        (a) when set, goto (1)
>        (b) when clear we are done
>

This seems very complicated how about:

qemu:
    (1) fill QXLRom::client_monitors_config, including a crc32 of the data
    (2) raise QXL_INTERRUPT_CLIENT_MONITORS_CONFIG

guest on interrupt:
    (1) clear QXL_INTERRUPT_CLIENT_MONITORS_CONFIG bit in irq status
    (2) read QXLRom::client_monitors_config
    (3) (verify-crc)? done : goto 2

That seems more straight-forward to me.

Regards,

Hans
> Hi,
> 
> On 09/11/2012 04:35 PM, Alon Levy wrote:
> > So far we have used the agent to notify the guest of a request to
> > change
> > the monitors configurations (heads) on the qxl device. This patch
> > introduces
> > a new interrupt and new fields in the qxl rom to notify the guest
> > about
> > a new request, similarly to how physical hardware notifies the
> > driver.
> >
> > To avoid overwriting the rom while the guest is reading it there is
> > a
> > client_monitors_config_updating field in ROM. The update protocol
> > is:
> >
> > 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 that QXLRom::client_monitors_Config_updating is clear
> >        (a) when set, goto (1)
> >    (5) check QXL_INTERRUPT_CLIENT_MONITORS_CONFIG bit in irq status
> >        (a) when set, goto (1)
> >        (b) when clear we are done
> >
> 
> This seems very complicated how about:
> 
> qemu:
>     (1) fill QXLRom::client_monitors_config, including a crc32 of the
>     data
>     (2) raise QXL_INTERRUPT_CLIENT_MONITORS_CONFIG
> 
> guest on interrupt:
>     (1) clear QXL_INTERRUPT_CLIENT_MONITORS_CONFIG bit in irq status
>     (2) read QXLRom::client_monitors_config
>     (3) (verify-crc)? done : goto 2
> 
> That seems more straight-forward to me.

Requires crc computing code in qemu and the guest. I know there is such in the kernel. I guess it's fine. It also has the chance of a mistake, I can let that slide I guess..

But more importantly I think I still need to change the protocol. I still need to know if the guest supports the client_monitors_config before issuing it, to take care of the case of a VDAgentMonitorsConfig arriving in multiple chunks, and for that I think one of:

 QXLInterface::client_monit-rs_config_supported()

pro: straightforward
con: add such for each cap? might as well add the guest_capabilities I introduced before, only this time set this cap from qemu instead of via a guest io (and don't introduce that guest io since we won't need it

alternative, reuse client_monitors_config, but have a NULL parameter just check and return without actually sending a message
pro: no extra api
con: abuse of a function

What do you think?

> 
> Regards,
> 
> Hans
>
On 09/11/12 17:32, Alon Levy wrote:
>> Hi,
>>
>> On 09/11/2012 04:35 PM, Alon Levy wrote:
>>> So far we have used the agent to notify the guest of a request to
>>> change
>>> the monitors configurations (heads) on the qxl device. This patch
>>> introduces
>>> a new interrupt and new fields in the qxl rom to notify the guest
>>> about
>>> a new request, similarly to how physical hardware notifies the
>>> driver.
>>>
>>> To avoid overwriting the rom while the guest is reading it there is
>>> a
>>> client_monitors_config_updating field in ROM. The update protocol
>>> is:
>>>
>>> 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 that QXLRom::client_monitors_Config_updating is clear
>>>        (a) when set, goto (1)
>>>    (5) check QXL_INTERRUPT_CLIENT_MONITORS_CONFIG bit in irq status
>>>        (a) when set, goto (1)
>>>        (b) when clear we are done
>>>
>>
>> This seems very complicated how about:
>>
>> qemu:
>>     (1) fill QXLRom::client_monitors_config, including a crc32 of the
>>     data
>>     (2) raise QXL_INTERRUPT_CLIENT_MONITORS_CONFIG
>>
>> guest on interrupt:
>>     (1) clear QXL_INTERRUPT_CLIENT_MONITORS_CONFIG bit in irq status
>>     (2) read QXLRom::client_monitors_config
>>     (3) (verify-crc)? done : goto 2
>>
>> That seems more straight-forward to me.
> 
> Requires crc computing code in qemu and the guest. I know there is
> such in the kernel. I guess it's fine. It also has the chance of a
> mistake, I can let that slide I guess..

I'm not sure it is actually simpler.  Using
client_monitors_config_updating is just two lines of code on the qemu
side (set & clear the bit).  Dunno about crc32, maybe we have such a
function somewhere in the networking code which we can reuse.

On the guest side using client_monitors_config_updating isn't that
complicated too.  The only thing which might add significant complexity
is (2) as this pretty much requires to not run this from irq context.
Otherwise it just adds a loop (needed for crc32 too) and two simple checks.

> But more importantly I think I still need to change the protocol. I
> still need to know if the guest supports the client_monitors_config
> before issuing it, to take care of the case of a VDAgentMonitorsConfig
> arriving in multiple chunks, and for that I think one of:

>  QXLInterface::client_monit-rs_config_supported()
> 
> pro: straightforward
> con: add such for each cap? might as well add the guest_capabilities I introduced before, only this time set this cap from qemu instead of via a guest io (and don't introduce that guest io since we won't need it
> 
> alternative, reuse client_monitors_config, but have a NULL parameter just check and return without actually sending a message
> pro: no extra api
> con: abuse of a function
> 
> What do you think?

I'd tend to go with QXLInterface::client_monitors_config(NULL).

But can't you just queue up the packets in spice-server until
VDAgentMonitorsConfig is complete?  You must do that anyway to assemble
the data for the client_monitors_config call, right?  Then send all
packets in one go after the client_monitors_config call returns (or drop
them, depending on the return value).

cheers,
  Gerd
> On 09/11/12 17:32, Alon Levy wrote:
> >> Hi,
> >>
> >> On 09/11/2012 04:35 PM, Alon Levy wrote:
> >>> So far we have used the agent to notify the guest of a request to
> >>> change
> >>> the monitors configurations (heads) on the qxl device. This patch
> >>> introduces
> >>> a new interrupt and new fields in the qxl rom to notify the guest
> >>> about
> >>> a new request, similarly to how physical hardware notifies the
> >>> driver.
> >>>
> >>> To avoid overwriting the rom while the guest is reading it there
> >>> is
> >>> a
> >>> client_monitors_config_updating field in ROM. The update protocol
> >>> is:
> >>>
> >>> 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 that QXLRom::client_monitors_Config_updating is
> >>>    clear
> >>>        (a) when set, goto (1)
> >>>    (5) check QXL_INTERRUPT_CLIENT_MONITORS_CONFIG bit in irq
> >>>    status
> >>>        (a) when set, goto (1)
> >>>        (b) when clear we are done
> >>>
> >>
> >> This seems very complicated how about:
> >>
> >> qemu:
> >>     (1) fill QXLRom::client_monitors_config, including a crc32 of
> >>     the
> >>     data
> >>     (2) raise QXL_INTERRUPT_CLIENT_MONITORS_CONFIG
> >>
> >> guest on interrupt:
> >>     (1) clear QXL_INTERRUPT_CLIENT_MONITORS_CONFIG bit in irq
> >>     status
> >>     (2) read QXLRom::client_monitors_config
> >>     (3) (verify-crc)? done : goto 2
> >>
> >> That seems more straight-forward to me.
> > 
> > Requires crc computing code in qemu and the guest. I know there is
> > such in the kernel. I guess it's fine. It also has the chance of a
> > mistake, I can let that slide I guess..
> 
> I'm not sure it is actually simpler.  Using
> client_monitors_config_updating is just two lines of code on the qemu
> side (set & clear the bit).  Dunno about crc32, maybe we have such a
> function somewhere in the networking code which we can reuse.

there is crc32 from zlib.h, used elsewhere. There are a few other implementation of crc32/16 in qemu. Linux has linux/crc32.h but surprise it and zlib don't match (well, it's expected - there is no crc32 spec, it depends on the polynomial, if you xor before or after).

I don't really mind which solution we use, I can send patches for the crc32 one already.

> 
> On the guest side using client_monitors_config_updating isn't that
> complicated too.  The only thing which might add significant
> complexity
> is (2) as this pretty much requires to not run this from irq context.
> Otherwise it just adds a loop (needed for crc32 too) and two simple
> checks.
> 
> > But more importantly I think I still need to change the protocol. I
> > still need to know if the guest supports the client_monitors_config
> > before issuing it, to take care of the case of a
> > VDAgentMonitorsConfig
> > arriving in multiple chunks, and for that I think one of:
> 
> >  QXLInterface::client_monit-rs_config_supported()
> > 
> > pro: straightforward
> > con: add such for each cap? might as well add the
> > guest_capabilities I introduced before, only this time set this
> > cap from qemu instead of via a guest io (and don't introduce that
> > guest io since we won't need it
> > 
> > alternative, reuse client_monitors_config, but have a NULL
> > parameter just check and return without actually sending a message
> > pro: no extra api
> > con: abuse of a function
> > 
> > What do you think?
> 
> I'd tend to go with QXLInterface::client_monitors_config(NULL).
> 
> But can't you just queue up the packets in spice-server until
> VDAgentMonitorsConfig is complete?  You must do that anyway to
> assemble
> the data for the client_monitors_config call, right?  Then send all
> packets in one go after the client_monitors_config call returns (or
> drop
> them, depending on the return value).
> 

Yes, I queue the packets in order to assemble them for the client_monitors_config(not NULL) call, but it's still a bit (tiny bit, admitted) simpler not to pass it to the spice-server char device afterwords. I'll send the patches with the client_monitors_config(NULL) implementation.

> cheers,
>   Gerd
> 
>
Hi,

On 09/12/2012 10:24 AM, Alon Levy wrote:
>> On 09/11/12 17:32, Alon Levy wrote:
>>>> Hi,
>>>>
>>>> On 09/11/2012 04:35 PM, Alon Levy wrote:
>>>>> So far we have used the agent to notify the guest of a request to
>>>>> change
>>>>> the monitors configurations (heads) on the qxl device. This patch
>>>>> introduces
>>>>> a new interrupt and new fields in the qxl rom to notify the guest
>>>>> about
>>>>> a new request, similarly to how physical hardware notifies the
>>>>> driver.
>>>>>
>>>>> To avoid overwriting the rom while the guest is reading it there
>>>>> is
>>>>> a
>>>>> client_monitors_config_updating field in ROM. The update protocol
>>>>> is:
>>>>>
>>>>> 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 that QXLRom::client_monitors_Config_updating is
>>>>>     clear
>>>>>         (a) when set, goto (1)
>>>>>     (5) check QXL_INTERRUPT_CLIENT_MONITORS_CONFIG bit in irq
>>>>>     status
>>>>>         (a) when set, goto (1)
>>>>>         (b) when clear we are done
>>>>>
>>>>
>>>> This seems very complicated how about:
>>>>
>>>> qemu:
>>>>      (1) fill QXLRom::client_monitors_config, including a crc32 of
>>>>      the
>>>>      data
>>>>      (2) raise QXL_INTERRUPT_CLIENT_MONITORS_CONFIG
>>>>
>>>> guest on interrupt:
>>>>      (1) clear QXL_INTERRUPT_CLIENT_MONITORS_CONFIG bit in irq
>>>>      status
>>>>      (2) read QXLRom::client_monitors_config
>>>>      (3) (verify-crc)? done : goto 2
>>>>
>>>> That seems more straight-forward to me.
>>>
>>> Requires crc computing code in qemu and the guest. I know there is
>>> such in the kernel. I guess it's fine. It also has the chance of a
>>> mistake, I can let that slide I guess..
>>
>> I'm not sure it is actually simpler.  Using
>> client_monitors_config_updating is just two lines of code on the qemu
>> side (set & clear the bit).  Dunno about crc32, maybe we have such a
>> function somewhere in the networking code which we can reuse.
>
> there is crc32 from zlib.h, used elsewhere. There are a few other implementation of crc32/16 in qemu. Linux has linux/crc32.h but surprise it and zlib don't match (well, it's expected - there is no crc32 spec, it depends on the polynomial, if you xor before or after).
>
> I don't really mind which solution we use, I can send patches for the crc32 one already.

If you've already looked into both, I would go with the one which
is simpler from an implementation pov. I've a feeling that the guest
code with the crc32 will be significantly simpler (not counting the
crc32 implementation itself, you can simply copy the kernels crc.h to
qemu), but it is your call.

>
>>
>> On the guest side using client_monitors_config_updating isn't that
>> complicated too.  The only thing which might add significant
>> complexity
>> is (2) as this pretty much requires to not run this from irq context.
>> Otherwise it just adds a loop (needed for crc32 too) and two simple
>> checks.
>>
>>> But more importantly I think I still need to change the protocol. I
>>> still need to know if the guest supports the client_monitors_config
>>> before issuing it, to take care of the case of a
>>> VDAgentMonitorsConfig
>>> arriving in multiple chunks, and for that I think one of:
>>
>>>   QXLInterface::client_monit-rs_config_supported()
>>>
>>> pro: straightforward
>>> con: add such for each cap? might as well add the
>>> guest_capabilities I introduced before, only this time set this
>>> cap from qemu instead of via a guest io (and don't introduce that
>>> guest io since we won't need it
>>>
>>> alternative, reuse client_monitors_config, but have a NULL
>>> parameter just check and return without actually sending a message
>>> pro: no extra api
>>> con: abuse of a function
>>>
>>> What do you think?
>>
>> I'd tend to go with QXLInterface::client_monitors_config(NULL).
>>
>> But can't you just queue up the packets in spice-server until
>> VDAgentMonitorsConfig is complete?  You must do that anyway to
>> assemble
>> the data for the client_monitors_config call, right?  Then send all
>> packets in one go after the client_monitors_config call returns (or
>> drop
>> them, depending on the return value).
>>
>
> Yes, I queue the packets in order to assemble them for the client_monitors_config(not NULL) call, but it's still a bit (tiny bit, admitted) simpler not to pass it to the spice-server char device afterwords. I'll send the patches with the client_monitors_config(NULL) implementation.

Ok.

Regards,

Hans