| Message ID | 20200312165431.82118-9-liran.alon@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | : hw/i386/vmport: Bug fixes and improvements | expand |
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 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
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
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);