| Message ID | 20200309235411.76587-7-liran.alon@oracle.com |
|---|---|
| State | New |
| Headers | show |
| Series | : hw/i386/vmport: Bug fixes and improvements | expand |
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.
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
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);