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/vmport.h | 11 ++++++++++-
3 files changed, 18 insertions(+), 22 deletions(-)
diff --git a/hw/i386/vmmouse.c b/hw/i386/vmmouse.c
index 49a546fd3bb6..afb8e9e09a30 100644
--- a/hw/i386/vmmouse.c
+++ b/hw/i386/vmmouse.c
@@ -34,12 +34,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
@@ -197,10 +191,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);
@@ -219,7 +213,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:
@@ -276,9 +270,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 e9ea5fe7f765..6ab094311d6c 100644
--- a/hw/i386/vmport.c
+++ b/hw/i386/vmport.c
@@ -38,10 +38,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
/* Compatibility flags for migration */
@@ -72,12 +68,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/vmport.h b/include/hw/i386/vmport.h
index f0c1e985ca08..0501ccac6ddf 100644
--- a/include/hw/i386/vmport.h
+++ b/include/hw/i386/vmport.h
@@ -4,12 +4,21 @@
#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 3/12/20 5:54 PM, 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/vmport.h | 11 ++++++++++-
Please setup scripts/git.orderfile to ease reviews.
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> 3 files changed, 18 insertions(+), 22 deletions(-)
>
> diff --git a/hw/i386/vmmouse.c b/hw/i386/vmmouse.c
> index 49a546fd3bb6..afb8e9e09a30 100644
> --- a/hw/i386/vmmouse.c
> +++ b/hw/i386/vmmouse.c
> @@ -34,12 +34,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
> @@ -197,10 +191,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);
> @@ -219,7 +213,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:
> @@ -276,9 +270,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 e9ea5fe7f765..6ab094311d6c 100644
> --- a/hw/i386/vmport.c
> +++ b/hw/i386/vmport.c
> @@ -38,10 +38,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
>
> /* Compatibility flags for migration */
> @@ -72,12 +68,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/vmport.h b/include/hw/i386/vmport.h
> index f0c1e985ca08..0501ccac6ddf 100644
> --- a/include/hw/i386/vmport.h
> +++ b/include/hw/i386/vmport.h
> @@ -4,12 +4,21 @@
> #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);
>
>
On 3/13/20 8:59 PM, Philippe Mathieu-Daudé wrote:
> On 3/12/20 5:54 PM, 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/vmport.h | 11 ++++++++++-
>
> Please setup scripts/git.orderfile to ease reviews.
>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>
>> 3 files changed, 18 insertions(+), 22 deletions(-)
>>
>> diff --git a/hw/i386/vmmouse.c b/hw/i386/vmmouse.c
>> index 49a546fd3bb6..afb8e9e09a30 100644
>> --- a/hw/i386/vmmouse.c
>> +++ b/hw/i386/vmmouse.c
>> @@ -34,12 +34,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
>> @@ -197,10 +191,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);
>> @@ -219,7 +213,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:
>> @@ -276,9 +270,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 e9ea5fe7f765..6ab094311d6c 100644
>> --- a/hw/i386/vmport.c
>> +++ b/hw/i386/vmport.c
>> @@ -38,10 +38,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
>> /* Compatibility flags for migration */
>> @@ -72,12 +68,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/vmport.h b/include/hw/i386/vmport.h
>> index f0c1e985ca08..0501ccac6ddf 100644
>> --- a/include/hw/i386/vmport.h
>> +++ b/include/hw/i386/vmport.h
>> @@ -4,12 +4,21 @@
>> #define TYPE_VMPORT "vmport"
>> typedef uint32_t (VMPortReadFunc)(void *opaque, uint32_t address);
>> +typedef enum {
>> + VMPORT_CMD_GETVERSION = 10,
Can we rename as VMPORT_CMD_GET_VERSION which is easier to read?
>> + VMPORT_CMD_GETRAMSIZE = 20,
Ditto _GET_RAM_SIZE?
>> + 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);
>>
On 13/03/2020 22:05, Philippe Mathieu-Daudé wrote:
> On 3/13/20 8:59 PM, Philippe Mathieu-Daudé wrote:
>> On 3/12/20 5:54 PM, Liran Alon wrote:
>>> --- a/include/hw/i386/vmport.h
>>> +++ b/include/hw/i386/vmport.h
>>> @@ -4,12 +4,21 @@
>>> #define TYPE_VMPORT "vmport"
>>> typedef uint32_t (VMPortReadFunc)(void *opaque, uint32_t address);
>>> +typedef enum {
>>> + VMPORT_CMD_GETVERSION = 10,
>
> Can we rename as VMPORT_CMD_GET_VERSION which is easier to read?
Sure. Will do on v4.
>
>>> + VMPORT_CMD_GETRAMSIZE = 20,
>
> Ditto _GET_RAM_SIZE?
Sure. Will do in v4.
-Liran
On 13/03/2020 21:59, Philippe Mathieu-Daudé wrote: > On 3/12/20 5:54 PM, 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/vmport.h | 11 ++++++++++- > > Please setup scripts/git.orderfile to ease reviews. > > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > Oh ok. Will do for next submissions. Thanks, -Liran
© 2016 - 2025 Red Hat, Inc.