No functional change.

Defining an enum for all VMPort commands have the following advantages:
* It gets rid of the error-prone requirement to update VMPORT_ENTRIES
when new VMPort commands are added to QEMU.
* It makes it clear to know by looking at one place at the source, what
are all the VMPort commands supported by QEMU.

Reviewed-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
 hw/i386/vmmouse.c    | 18 ++++++------------
 hw/i386/vmport.c     | 11 ++---------
 include/hw/i386/pc.h | 11 ++++++++++-
 3 files changed, 18 insertions(+), 22 deletions(-)

diff --git a/hw/i386/vmmouse.c b/hw/i386/vmmouse.c
index e8e62bd96b8c..a61042fc0c5e 100644
--- a/hw/i386/vmmouse.c
+++ b/hw/i386/vmmouse.c
@@ -33,12 +33,6 @@
 /* debug only vmmouse */
 //#define DEBUG_VMMOUSE
 
-/* VMMouse Commands */
-#define VMMOUSE_GETVERSION	10
-#define VMMOUSE_DATA		39
-#define VMMOUSE_STATUS		40
-#define VMMOUSE_COMMAND		41
-
 #define VMMOUSE_READ_ID			0x45414552
 #define VMMOUSE_DISABLE			0x000000f5
 #define VMMOUSE_REQUEST_RELATIVE	0x4c455252
@@ -196,10 +190,10 @@ static uint32_t vmmouse_ioport_read(void *opaque, uint32_t addr)
     command = data[2] & 0xFFFF;
 
     switch (command) {
-    case VMMOUSE_STATUS:
+    case VMPORT_CMD_VMMOUSE_STATUS:
         data[0] = vmmouse_get_status(s);
         break;
-    case VMMOUSE_COMMAND:
+    case VMPORT_CMD_VMMOUSE_COMMAND:
         switch (data[1]) {
         case VMMOUSE_DISABLE:
             vmmouse_disable(s);
@@ -218,7 +212,7 @@ static uint32_t vmmouse_ioport_read(void *opaque, uint32_t addr)
             break;
         }
         break;
-    case VMMOUSE_DATA:
+    case VMPORT_CMD_VMMOUSE_DATA:
         vmmouse_data(s, data, data[1]);
         break;
     default:
@@ -275,9 +269,9 @@ static void vmmouse_realizefn(DeviceState *dev, Error **errp)
         return;
     }
 
-    vmport_register(VMMOUSE_STATUS, vmmouse_ioport_read, s);
-    vmport_register(VMMOUSE_COMMAND, vmmouse_ioport_read, s);
-    vmport_register(VMMOUSE_DATA, vmmouse_ioport_read, s);
+    vmport_register(VMPORT_CMD_VMMOUSE_STATUS, vmmouse_ioport_read, s);
+    vmport_register(VMPORT_CMD_VMMOUSE_COMMAND, vmmouse_ioport_read, s);
+    vmport_register(VMPORT_CMD_VMMOUSE_DATA, vmmouse_ioport_read, s);
 }
 
 static Property vmmouse_properties[] = {
diff --git a/hw/i386/vmport.c b/hw/i386/vmport.c
index c03f57f2f636..2ae5afc42b50 100644
--- a/hw/i386/vmport.c
+++ b/hw/i386/vmport.c
@@ -30,10 +30,6 @@
 #include "qemu/log.h"
 #include "trace.h"
 
-#define VMPORT_CMD_GETVERSION 0x0a
-#define VMPORT_CMD_GETRAMSIZE 0x14
-
-#define VMPORT_ENTRIES 0x2c
 #define VMPORT_MAGIC   0x564D5868
 
 typedef enum {
@@ -60,12 +56,9 @@ typedef struct VMPortState {
 
 static VMPortState *port_state;
 
-void vmport_register(unsigned char command, VMPortReadFunc *func, void *opaque)
+void vmport_register(VMPortCommand command, VMPortReadFunc *func, void *opaque)
 {
-    if (command >= VMPORT_ENTRIES) {
-        return;
-    }
-
+    assert(command < VMPORT_ENTRIES);
     trace_vmport_register(command, func, opaque);
     port_state->func[command] = func;
     port_state->opaque[command] = opaque;
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index d5ac76d54e1f..7f15a01137b1 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -138,12 +138,21 @@ GSIState *pc_gsi_create(qemu_irq **irqs, bool pci_enabled);
 #define TYPE_VMPORT "vmport"
 typedef uint32_t (VMPortReadFunc)(void *opaque, uint32_t address);
 
+typedef enum {
+    VMPORT_CMD_GETVERSION       = 10,
+    VMPORT_CMD_GETRAMSIZE       = 20,
+    VMPORT_CMD_VMMOUSE_DATA     = 39,
+    VMPORT_CMD_VMMOUSE_STATUS   = 40,
+    VMPORT_CMD_VMMOUSE_COMMAND  = 41,
+    VMPORT_ENTRIES
+} VMPortCommand;
+
 static inline void vmport_init(ISABus *bus)
 {
     isa_create_simple(bus, TYPE_VMPORT);
 }
 
-void vmport_register(unsigned char command, VMPortReadFunc *func, void *opaque);
+void vmport_register(VMPortCommand command, VMPortReadFunc *func, void *opaque);
 void vmmouse_get_data(uint32_t *data);
 void vmmouse_set_data(const uint32_t *data);
 
-- 
2.20.1

On Tue, Mar 10, 2020 at 01:54:03AM +0200, Liran Alon wrote:
> No functional change.
> 
> Defining an enum for all VMPort commands have the following advantages:
> * It gets rid of the error-prone requirement to update VMPORT_ENTRIES
> when new VMPort commands are added to QEMU.
> * It makes it clear to know by looking at one place at the source, what
> are all the VMPort commands supported by QEMU.
> 
> Reviewed-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
> Signed-off-by: Liran Alon <liran.alon@oracle.com>
> ---
>  hw/i386/vmmouse.c    | 18 ++++++------------
>  hw/i386/vmport.c     | 11 ++---------
>  include/hw/i386/pc.h | 11 ++++++++++-
>  3 files changed, 18 insertions(+), 22 deletions(-)
> 
> diff --git a/hw/i386/vmmouse.c b/hw/i386/vmmouse.c
> index e8e62bd96b8c..a61042fc0c5e 100644
> --- a/hw/i386/vmmouse.c
> +++ b/hw/i386/vmmouse.c
> @@ -33,12 +33,6 @@
>  /* debug only vmmouse */
>  //#define DEBUG_VMMOUSE
>  
> -/* VMMouse Commands */
> -#define VMMOUSE_GETVERSION	10
> -#define VMMOUSE_DATA		39
> -#define VMMOUSE_STATUS		40
> -#define VMMOUSE_COMMAND		41
> -
>  #define VMMOUSE_READ_ID			0x45414552
>  #define VMMOUSE_DISABLE			0x000000f5
>  #define VMMOUSE_REQUEST_RELATIVE	0x4c455252
> @@ -196,10 +190,10 @@ static uint32_t vmmouse_ioport_read(void *opaque, uint32_t addr)
>      command = data[2] & 0xFFFF;
>  
>      switch (command) {
> -    case VMMOUSE_STATUS:
> +    case VMPORT_CMD_VMMOUSE_STATUS:
>          data[0] = vmmouse_get_status(s);
>          break;
> -    case VMMOUSE_COMMAND:
> +    case VMPORT_CMD_VMMOUSE_COMMAND:
>          switch (data[1]) {
>          case VMMOUSE_DISABLE:
>              vmmouse_disable(s);
> @@ -218,7 +212,7 @@ static uint32_t vmmouse_ioport_read(void *opaque, uint32_t addr)
>              break;
>          }
>          break;
> -    case VMMOUSE_DATA:
> +    case VMPORT_CMD_VMMOUSE_DATA:
>          vmmouse_data(s, data, data[1]);
>          break;
>      default:
> @@ -275,9 +269,9 @@ static void vmmouse_realizefn(DeviceState *dev, Error **errp)
>          return;
>      }
>  
> -    vmport_register(VMMOUSE_STATUS, vmmouse_ioport_read, s);
> -    vmport_register(VMMOUSE_COMMAND, vmmouse_ioport_read, s);
> -    vmport_register(VMMOUSE_DATA, vmmouse_ioport_read, s);
> +    vmport_register(VMPORT_CMD_VMMOUSE_STATUS, vmmouse_ioport_read, s);
> +    vmport_register(VMPORT_CMD_VMMOUSE_COMMAND, vmmouse_ioport_read, s);
> +    vmport_register(VMPORT_CMD_VMMOUSE_DATA, vmmouse_ioport_read, s);
>  }
>  
>  static Property vmmouse_properties[] = {
> diff --git a/hw/i386/vmport.c b/hw/i386/vmport.c
> index c03f57f2f636..2ae5afc42b50 100644
> --- a/hw/i386/vmport.c
> +++ b/hw/i386/vmport.c
> @@ -30,10 +30,6 @@
>  #include "qemu/log.h"
>  #include "trace.h"
>  
> -#define VMPORT_CMD_GETVERSION 0x0a
> -#define VMPORT_CMD_GETRAMSIZE 0x14
> -
> -#define VMPORT_ENTRIES 0x2c
>  #define VMPORT_MAGIC   0x564D5868
>  
>  typedef enum {
> @@ -60,12 +56,9 @@ typedef struct VMPortState {
>  
>  static VMPortState *port_state;
>  
> -void vmport_register(unsigned char command, VMPortReadFunc *func, void *opaque)
> +void vmport_register(VMPortCommand command, VMPortReadFunc *func, void *opaque)
>  {
> -    if (command >= VMPORT_ENTRIES) {
> -        return;
> -    }
> -
> +    assert(command < VMPORT_ENTRIES);
>      trace_vmport_register(command, func, opaque);
>      port_state->func[command] = func;
>      port_state->opaque[command] = opaque;
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index d5ac76d54e1f..7f15a01137b1 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -138,12 +138,21 @@ GSIState *pc_gsi_create(qemu_irq **irqs, bool pci_enabled);
>  #define TYPE_VMPORT "vmport"
>  typedef uint32_t (VMPortReadFunc)(void *opaque, uint32_t address);
>  
> +typedef enum {
> +    VMPORT_CMD_GETVERSION       = 10,
> +    VMPORT_CMD_GETRAMSIZE       = 20,
> +    VMPORT_CMD_VMMOUSE_DATA     = 39,
> +    VMPORT_CMD_VMMOUSE_STATUS   = 40,
> +    VMPORT_CMD_VMMOUSE_COMMAND  = 41,
> +    VMPORT_ENTRIES
> +} VMPortCommand;
> +

Please don't, let's leave pc.h alone. If you must add a new header for
vmport/vmmouse and put this stuff there.

>  static inline void vmport_init(ISABus *bus)
>  {
>      isa_create_simple(bus, TYPE_VMPORT);
>  }
>  
> -void vmport_register(unsigned char command, VMPortReadFunc *func, void *opaque);
> +void vmport_register(VMPortCommand command, VMPortReadFunc *func, void *opaque);
>  void vmmouse_get_data(uint32_t *data);
>  void vmmouse_set_data(const uint32_t *data);
>  
> -- 
> 2.20.1

On 10/03/2020 11:28, Michael S. Tsirkin wrote:
> On Tue, Mar 10, 2020 at 01:54:03AM +0200, Liran Alon wrote:
>> No functional change.
>>
>> Defining an enum for all VMPort commands have the following advantages:
>> * It gets rid of the error-prone requirement to update VMPORT_ENTRIES
>> when new VMPort commands are added to QEMU.
>> * It makes it clear to know by looking at one place at the source, what
>> are all the VMPort commands supported by QEMU.
>>
>> Reviewed-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
>> Signed-off-by: Liran Alon <liran.alon@oracle.com>
>> ---
>>
>> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
>> index d5ac76d54e1f..7f15a01137b1 100644
>> --- a/include/hw/i386/pc.h
>> +++ b/include/hw/i386/pc.h
>> @@ -138,12 +138,21 @@ GSIState *pc_gsi_create(qemu_irq **irqs, bool pci_enabled);
>>   #define TYPE_VMPORT "vmport"
>>   typedef uint32_t (VMPortReadFunc)(void *opaque, uint32_t address);
>>   
>> +typedef enum {
>> +    VMPORT_CMD_GETVERSION       = 10,
>> +    VMPORT_CMD_GETRAMSIZE       = 20,
>> +    VMPORT_CMD_VMMOUSE_DATA     = 39,
>> +    VMPORT_CMD_VMMOUSE_STATUS   = 40,
>> +    VMPORT_CMD_VMMOUSE_COMMAND  = 41,
>> +    VMPORT_ENTRIES
>> +} VMPortCommand;
>> +
> Please don't, let's leave pc.h alone. If you must add a new header for
> vmport/vmmouse and put this stuff there.

As you can see, pc.h already contains definitions which are specific to 
vmport. E.g. TYPE_VMPORT, VMPortReadFunc(), vmport_register(), 
vmmouse_get_data(), vmmouse_set_data(). Adding this enum is not what 
makes the difference.
It is possible to create a new vmport.h header file but it's not really 
related to this patch. It's just general refactoring. I can do that in 
v2 if you think it's appropriate.

-Liran

>
>>   static inline void vmport_init(ISABus *bus)
>>   {
>>       isa_create_simple(bus, TYPE_VMPORT);
>>   }
>>   
>> -void vmport_register(unsigned char command, VMPortReadFunc *func, void *opaque);
>> +void vmport_register(VMPortCommand command, VMPortReadFunc *func, void *opaque);
>>   void vmmouse_get_data(uint32_t *data);
>>   void vmmouse_set_data(const uint32_t *data);
>>   
>> -- 
>> 2.20.1

On Tue, Mar 10, 2020 at 01:16:51PM +0200, Liran Alon wrote:
> 
> On 10/03/2020 11:28, Michael S. Tsirkin wrote:
> > On Tue, Mar 10, 2020 at 01:54:03AM +0200, Liran Alon wrote:
> > > No functional change.
> > > 
> > > Defining an enum for all VMPort commands have the following advantages:
> > > * It gets rid of the error-prone requirement to update VMPORT_ENTRIES
> > > when new VMPort commands are added to QEMU.
> > > * It makes it clear to know by looking at one place at the source, what
> > > are all the VMPort commands supported by QEMU.
> > > 
> > > Reviewed-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
> > > Signed-off-by: Liran Alon <liran.alon@oracle.com>
> > > ---
> > > 
> > > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> > > index d5ac76d54e1f..7f15a01137b1 100644
> > > --- a/include/hw/i386/pc.h
> > > +++ b/include/hw/i386/pc.h
> > > @@ -138,12 +138,21 @@ GSIState *pc_gsi_create(qemu_irq **irqs, bool pci_enabled);
> > >   #define TYPE_VMPORT "vmport"
> > >   typedef uint32_t (VMPortReadFunc)(void *opaque, uint32_t address);
> > > +typedef enum {
> > > +    VMPORT_CMD_GETVERSION       = 10,
> > > +    VMPORT_CMD_GETRAMSIZE       = 20,
> > > +    VMPORT_CMD_VMMOUSE_DATA     = 39,
> > > +    VMPORT_CMD_VMMOUSE_STATUS   = 40,
> > > +    VMPORT_CMD_VMMOUSE_COMMAND  = 41,
> > > +    VMPORT_ENTRIES
> > > +} VMPortCommand;
> > > +
> > Please don't, let's leave pc.h alone. If you must add a new header for
> > vmport/vmmouse and put this stuff there.
> 
> As you can see, pc.h already contains definitions which are specific to
> vmport. E.g. TYPE_VMPORT, VMPortReadFunc(), vmport_register(),
> vmmouse_get_data(), vmmouse_set_data(). Adding this enum is not what makes
> the difference.
> It is possible to create a new vmport.h header file but it's not really
> related to this patch. It's just general refactoring. I can do that in v2 if
> you think it's appropriate.
> 
> -Liran

Well I just don't want lots of enums in pc.h

> > 
> > >   static inline void vmport_init(ISABus *bus)
> > >   {
> > >       isa_create_simple(bus, TYPE_VMPORT);
> > >   }
> > > -void vmport_register(unsigned char command, VMPortReadFunc *func, void *opaque);
> > > +void vmport_register(VMPortCommand command, VMPortReadFunc *func, void *opaque);
> > >   void vmmouse_get_data(uint32_t *data);
> > >   void vmmouse_set_data(const uint32_t *data);
> > > -- 
> > > 2.20.1

On 10/03/2020 13:23, Michael S. Tsirkin wrote:
> On Tue, Mar 10, 2020 at 01:16:51PM +0200, Liran Alon wrote:
>> On 10/03/2020 11:28, Michael S. Tsirkin wrote:
>>> On Tue, Mar 10, 2020 at 01:54:03AM +0200, Liran Alon wrote:
>>>> No functional change.
>>>>
>>>> Defining an enum for all VMPort commands have the following advantages:
>>>> * It gets rid of the error-prone requirement to update VMPORT_ENTRIES
>>>> when new VMPort commands are added to QEMU.
>>>> * It makes it clear to know by looking at one place at the source, what
>>>> are all the VMPort commands supported by QEMU.
>>>>
>>>> Reviewed-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
>>>> Signed-off-by: Liran Alon <liran.alon@oracle.com>
>>>> ---
>>>>
>>>> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
>>>> index d5ac76d54e1f..7f15a01137b1 100644
>>>> --- a/include/hw/i386/pc.h
>>>> +++ b/include/hw/i386/pc.h
>>>> @@ -138,12 +138,21 @@ GSIState *pc_gsi_create(qemu_irq **irqs, bool pci_enabled);
>>>>    #define TYPE_VMPORT "vmport"
>>>>    typedef uint32_t (VMPortReadFunc)(void *opaque, uint32_t address);
>>>> +typedef enum {
>>>> +    VMPORT_CMD_GETVERSION       = 10,
>>>> +    VMPORT_CMD_GETRAMSIZE       = 20,
>>>> +    VMPORT_CMD_VMMOUSE_DATA     = 39,
>>>> +    VMPORT_CMD_VMMOUSE_STATUS   = 40,
>>>> +    VMPORT_CMD_VMMOUSE_COMMAND  = 41,
>>>> +    VMPORT_ENTRIES
>>>> +} VMPortCommand;
>>>> +
>>> Please don't, let's leave pc.h alone. If you must add a new header for
>>> vmport/vmmouse and put this stuff there.
>> As you can see, pc.h already contains definitions which are specific to
>> vmport. E.g. TYPE_VMPORT, VMPortReadFunc(), vmport_register(),
>> vmmouse_get_data(), vmmouse_set_data(). Adding this enum is not what makes
>> the difference.
>> It is possible to create a new vmport.h header file but it's not really
>> related to this patch. It's just general refactoring. I can do that in v2 if
>> you think it's appropriate.
>>
>> -Liran
> Well I just don't want lots of enums in pc.h

This is the only one which is global, and makes sense as it's directly 
related to vmport_register() exposed API.
Similar to how the VMPortReadFunc typedef is put in here.

-Liran

On Tue, Mar 10, 2020 at 01:37:40PM +0200, Liran Alon wrote:
> 
> On 10/03/2020 13:23, Michael S. Tsirkin wrote:
> > On Tue, Mar 10, 2020 at 01:16:51PM +0200, Liran Alon wrote:
> > > On 10/03/2020 11:28, Michael S. Tsirkin wrote:
> > > > On Tue, Mar 10, 2020 at 01:54:03AM +0200, Liran Alon wrote:
> > > > > No functional change.
> > > > > 
> > > > > Defining an enum for all VMPort commands have the following advantages:
> > > > > * It gets rid of the error-prone requirement to update VMPORT_ENTRIES
> > > > > when new VMPort commands are added to QEMU.
> > > > > * It makes it clear to know by looking at one place at the source, what
> > > > > are all the VMPort commands supported by QEMU.
> > > > > 
> > > > > Reviewed-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
> > > > > Signed-off-by: Liran Alon <liran.alon@oracle.com>
> > > > > ---
> > > > > 
> > > > > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> > > > > index d5ac76d54e1f..7f15a01137b1 100644
> > > > > --- a/include/hw/i386/pc.h
> > > > > +++ b/include/hw/i386/pc.h
> > > > > @@ -138,12 +138,21 @@ GSIState *pc_gsi_create(qemu_irq **irqs, bool pci_enabled);
> > > > >    #define TYPE_VMPORT "vmport"
> > > > >    typedef uint32_t (VMPortReadFunc)(void *opaque, uint32_t address);
> > > > > +typedef enum {
> > > > > +    VMPORT_CMD_GETVERSION       = 10,
> > > > > +    VMPORT_CMD_GETRAMSIZE       = 20,
> > > > > +    VMPORT_CMD_VMMOUSE_DATA     = 39,
> > > > > +    VMPORT_CMD_VMMOUSE_STATUS   = 40,
> > > > > +    VMPORT_CMD_VMMOUSE_COMMAND  = 41,
> > > > > +    VMPORT_ENTRIES
> > > > > +} VMPortCommand;
> > > > > +
> > > > Please don't, let's leave pc.h alone. If you must add a new header for
> > > > vmport/vmmouse and put this stuff there.
> > > As you can see, pc.h already contains definitions which are specific to
> > > vmport. E.g. TYPE_VMPORT, VMPortReadFunc(), vmport_register(),
> > > vmmouse_get_data(), vmmouse_set_data(). Adding this enum is not what makes
> > > the difference.
> > > It is possible to create a new vmport.h header file but it's not really
> > > related to this patch. It's just general refactoring. I can do that in v2 if
> > > you think it's appropriate.
> > > 
> > > -Liran
> > Well I just don't want lots of enums in pc.h
> 
> This is the only one which is global, and makes sense as it's directly
> related to vmport_register() exposed API.
> Similar to how the VMPortReadFunc typedef is put in here.
> 
> -Liran

So pls find another home for this stuff. Whoever touches legacy code
gets to clean it up a bit first :) Tough but that's the only stick
maintainers have to make maintainance happen.

-- 
MST

On 10/03/2020 13:46, Michael S. Tsirkin wrote:
> On Tue, Mar 10, 2020 at 01:37:40PM +0200, Liran Alon wrote:
>> On 10/03/2020 13:23, Michael S. Tsirkin wrote:
>>> On Tue, Mar 10, 2020 at 01:16:51PM +0200, Liran Alon wrote:
>>>> On 10/03/2020 11:28, Michael S. Tsirkin wrote:
>>>>> On Tue, Mar 10, 2020 at 01:54:03AM +0200, Liran Alon wrote:
>>>>>> No functional change.
>>>>>>
>>>>>> Defining an enum for all VMPort commands have the following advantages:
>>>>>> * It gets rid of the error-prone requirement to update VMPORT_ENTRIES
>>>>>> when new VMPort commands are added to QEMU.
>>>>>> * It makes it clear to know by looking at one place at the source, what
>>>>>> are all the VMPort commands supported by QEMU.
>>>>>>
>>>>>> Reviewed-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
>>>>>> Signed-off-by: Liran Alon <liran.alon@oracle.com>
>>>>>> ---
>>>>>>
>>>>>> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
>>>>>> index d5ac76d54e1f..7f15a01137b1 100644
>>>>>> --- a/include/hw/i386/pc.h
>>>>>> +++ b/include/hw/i386/pc.h
>>>>>> @@ -138,12 +138,21 @@ GSIState *pc_gsi_create(qemu_irq **irqs, bool pci_enabled);
>>>>>>     #define TYPE_VMPORT "vmport"
>>>>>>     typedef uint32_t (VMPortReadFunc)(void *opaque, uint32_t address);
>>>>>> +typedef enum {
>>>>>> +    VMPORT_CMD_GETVERSION       = 10,
>>>>>> +    VMPORT_CMD_GETRAMSIZE       = 20,
>>>>>> +    VMPORT_CMD_VMMOUSE_DATA     = 39,
>>>>>> +    VMPORT_CMD_VMMOUSE_STATUS   = 40,
>>>>>> +    VMPORT_CMD_VMMOUSE_COMMAND  = 41,
>>>>>> +    VMPORT_ENTRIES
>>>>>> +} VMPortCommand;
>>>>>> +
>>>>> Please don't, let's leave pc.h alone. If you must add a new header for
>>>>> vmport/vmmouse and put this stuff there.
>>>> As you can see, pc.h already contains definitions which are specific to
>>>> vmport. E.g. TYPE_VMPORT, VMPortReadFunc(), vmport_register(),
>>>> vmmouse_get_data(), vmmouse_set_data(). Adding this enum is not what makes
>>>> the difference.
>>>> It is possible to create a new vmport.h header file but it's not really
>>>> related to this patch. It's just general refactoring. I can do that in v2 if
>>>> you think it's appropriate.
>>>>
>>>> -Liran
>>> Well I just don't want lots of enums in pc.h
>> This is the only one which is global, and makes sense as it's directly
>> related to vmport_register() exposed API.
>> Similar to how the VMPortReadFunc typedef is put in here.
>>
>> -Liran
> So pls find another home for this stuff. Whoever touches legacy code
> gets to clean it up a bit first :) Tough but that's the only stick
> maintainers have to make maintainance happen.

Will do then in v2. :)

-Liran