| Message ID | 20200312165431.82118-8-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. This is mere refactoring. > > Suggested-by: Michael S. Tsirkin <mst@redhat.com> > Signed-off-by: Liran Alon <liran.alon@oracle.com> > --- > hw/i386/pc.c | 1 + > hw/i386/vmmouse.c | 1 + > hw/i386/vmport.c | 1 + > include/hw/i386/pc.h | 13 ------------- > include/hw/i386/vmport.h | 16 ++++++++++++++++ What about moving it to hw/i386/vmport.h (no under include/)? Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > 5 files changed, 19 insertions(+), 13 deletions(-) > create mode 100644 include/hw/i386/vmport.h > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > index 6ab4acb0c62e..6ac71e1af32b 100644 > --- a/hw/i386/pc.c > +++ b/hw/i386/pc.c > @@ -31,6 +31,7 @@ > #include "hw/i386/apic.h" > #include "hw/i386/topology.h" > #include "hw/i386/fw_cfg.h" > +#include "hw/i386/vmport.h" > #include "sysemu/cpus.h" > #include "hw/block/fdc.h" > #include "hw/ide.h" > diff --git a/hw/i386/vmmouse.c b/hw/i386/vmmouse.c > index e8e62bd96b8c..49a546fd3bb6 100644 > --- a/hw/i386/vmmouse.c > +++ b/hw/i386/vmmouse.c > @@ -26,6 +26,7 @@ > #include "qapi/error.h" > #include "ui/console.h" > #include "hw/i386/pc.h" > +#include "hw/i386/vmport.h" > #include "hw/input/i8042.h" > #include "hw/qdev-properties.h" > #include "migration/vmstate.h" > diff --git a/hw/i386/vmport.c b/hw/i386/vmport.c > index ead2f2d5326f..e9ea5fe7f765 100644 > --- a/hw/i386/vmport.c > +++ b/hw/i386/vmport.c > @@ -31,6 +31,7 @@ > #include "qemu/osdep.h" > #include "hw/isa/isa.h" > #include "hw/i386/pc.h" > +#include "hw/i386/vmport.h" > #include "hw/input/i8042.h" > #include "hw/qdev-properties.h" > #include "sysemu/hw_accel.h" > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h > index d5ac76d54e1f..60c988c4a5aa 100644 > --- a/include/hw/i386/pc.h > +++ b/include/hw/i386/pc.h > @@ -134,19 +134,6 @@ typedef struct PCMachineClass { > > GSIState *pc_gsi_create(qemu_irq **irqs, bool pci_enabled); > > -/* vmport.c */ > -#define TYPE_VMPORT "vmport" > -typedef uint32_t (VMPortReadFunc)(void *opaque, uint32_t address); > - > -static inline void vmport_init(ISABus *bus) > -{ > - isa_create_simple(bus, TYPE_VMPORT); > -} > - > -void vmport_register(unsigned char command, VMPortReadFunc *func, void *opaque); > -void vmmouse_get_data(uint32_t *data); > -void vmmouse_set_data(const uint32_t *data); > - > /* pc.c */ > extern int fd_bootchk; > > diff --git a/include/hw/i386/vmport.h b/include/hw/i386/vmport.h > new file mode 100644 > index 000000000000..f0c1e985ca08 > --- /dev/null > +++ b/include/hw/i386/vmport.h > @@ -0,0 +1,16 @@ > +#ifndef HW_VMPORT_H > +#define HW_VMPORT_H > + > +#define TYPE_VMPORT "vmport" > +typedef uint32_t (VMPortReadFunc)(void *opaque, uint32_t address); > + > +static inline void vmport_init(ISABus *bus) > +{ > + isa_create_simple(bus, TYPE_VMPORT); > +} > + > +void vmport_register(unsigned char command, VMPortReadFunc *func, void *opaque); > +void vmmouse_get_data(uint32_t *data); > +void vmmouse_set_data(const uint32_t *data); > + > +#endif >
On 13/03/2020 21:57, Philippe Mathieu-Daudé wrote: > On 3/12/20 5:54 PM, Liran Alon wrote: >> No functional change. This is mere refactoring. >> >> Suggested-by: Michael S. Tsirkin <mst@redhat.com> >> Signed-off-by: Liran Alon <liran.alon@oracle.com> >> --- >> hw/i386/pc.c | 1 + >> hw/i386/vmmouse.c | 1 + >> hw/i386/vmport.c | 1 + >> include/hw/i386/pc.h | 13 ------------- >> include/hw/i386/vmport.h | 16 ++++++++++++++++ > > What about moving it to hw/i386/vmport.h (no under include/)? > > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > > Can you explain the logic that separates between hw/i386/*.h to include/hw/i386/*.h? If it makes sense, sure I will move it. I just don't know what is the convention here. -Liran
On 3/13/20 11:38 PM, Liran Alon wrote: > On 13/03/2020 21:57, Philippe Mathieu-Daudé wrote: >> On 3/12/20 5:54 PM, Liran Alon wrote: >>> No functional change. This is mere refactoring. >>> >>> Suggested-by: Michael S. Tsirkin <mst@redhat.com> >>> Signed-off-by: Liran Alon <liran.alon@oracle.com> >>> --- >>> hw/i386/pc.c | 1 + >>> hw/i386/vmmouse.c | 1 + >>> hw/i386/vmport.c | 1 + >>> include/hw/i386/pc.h | 13 ------------- >>> include/hw/i386/vmport.h | 16 ++++++++++++++++ >> >> What about moving it to hw/i386/vmport.h (no under include/)? >> >> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> >> > Can you explain the logic that separates between hw/i386/*.h to > include/hw/i386/*.h? Headers in the include/hw/ namespace can be consumed by all machine targets. If this is a target-specific device, having it local to the target (hw/i386/) protect generic code (and other targets) of using it. This helps detecting wrong dependencies between components. > If it makes sense, sure I will move it. I just don't know what is the > convention here. Michael/Paolo/Eduardo what do you recommend?
On 14/03/2020 10:31, Philippe Mathieu-Daudé wrote: > On 3/13/20 11:38 PM, Liran Alon wrote: >> On 13/03/2020 21:57, Philippe Mathieu-Daudé wrote: >>> On 3/12/20 5:54 PM, Liran Alon wrote: >>>> No functional change. This is mere refactoring. >>>> >>>> Suggested-by: Michael S. Tsirkin <mst@redhat.com> >>>> Signed-off-by: Liran Alon <liran.alon@oracle.com> >>>> --- >>>> hw/i386/pc.c | 1 + >>>> hw/i386/vmmouse.c | 1 + >>>> hw/i386/vmport.c | 1 + >>>> include/hw/i386/pc.h | 13 ------------- >>>> include/hw/i386/vmport.h | 16 ++++++++++++++++ >>> >>> What about moving it to hw/i386/vmport.h (no under include/)? >>> >>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> >>> >>> >> Can you explain the logic that separates between hw/i386/*.h to >> include/hw/i386/*.h? > > Headers in the include/hw/ namespace can be consumed by all machine > targets. But this doesn't seem true for headers in include/hw/i386/*.h... It contains things that are target-specific. E.g. ioapic.h, x86-iommu.h, intel_iommu.h and etc. I still don't quite understand the separation between these directories. It seems both are i386-specific and one of them shouldn't exists. > If this is a target-specific device, having it local to the target > (hw/i386/) protect generic code (and other targets) of using it. This > helps detecting wrong dependencies between components. > >> If it makes sense, sure I will move it. I just don't know what is the >> convention here. > > Michael/Paolo/Eduardo what do you recommend? >
On Sat, Mar 14, 2020 at 09:31:31AM +0100, Philippe Mathieu-Daudé wrote: > On 3/13/20 11:38 PM, Liran Alon wrote: > > On 13/03/2020 21:57, Philippe Mathieu-Daudé wrote: > > > On 3/12/20 5:54 PM, Liran Alon wrote: > > > > No functional change. This is mere refactoring. > > > > > > > > Suggested-by: Michael S. Tsirkin <mst@redhat.com> > > > > Signed-off-by: Liran Alon <liran.alon@oracle.com> > > > > --- > > > > hw/i386/pc.c | 1 + > > > > hw/i386/vmmouse.c | 1 + > > > > hw/i386/vmport.c | 1 + > > > > include/hw/i386/pc.h | 13 ------------- > > > > include/hw/i386/vmport.h | 16 ++++++++++++++++ > > > > > > What about moving it to hw/i386/vmport.h (no under include/)? > > > > > > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > > > > > > > > Can you explain the logic that separates between hw/i386/*.h to > > include/hw/i386/*.h? > > Headers in the include/hw/ namespace can be consumed by all machine targets. > If this is a target-specific device, having it local to the target > (hw/i386/) protect generic code (and other targets) of using it. This helps > detecting wrong dependencies between components. I think it's true. However when headers were moved to include we weren't always able to do this correctly. So some i386 specific headers are under include. > > If it makes sense, sure I will move it. I just don't know what is the > > convention here. > > Michael/Paolo/Eduardo what do you recommend?
On 14/03/2020 20:25, Michael S. Tsirkin wrote: > On Sat, Mar 14, 2020 at 09:31:31AM +0100, Philippe Mathieu-Daudé wrote: >> On 3/13/20 11:38 PM, Liran Alon wrote: >>> On 13/03/2020 21:57, Philippe Mathieu-Daudé wrote: >>>> On 3/12/20 5:54 PM, Liran Alon wrote: >>>>> No functional change. This is mere refactoring. >>>>> >>>>> Suggested-by: Michael S. Tsirkin <mst@redhat.com> >>>>> Signed-off-by: Liran Alon <liran.alon@oracle.com> >>>>> --- >>>>> hw/i386/pc.c | 1 + >>>>> hw/i386/vmmouse.c | 1 + >>>>> hw/i386/vmport.c | 1 + >>>>> include/hw/i386/pc.h | 13 ------------- >>>>> include/hw/i386/vmport.h | 16 ++++++++++++++++ >>>> What about moving it to hw/i386/vmport.h (no under include/)? >>>> >>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> >>>> >>>> >>> Can you explain the logic that separates between hw/i386/*.h to >>> include/hw/i386/*.h? >> Headers in the include/hw/ namespace can be consumed by all machine targets. >> If this is a target-specific device, having it local to the target >> (hw/i386/) protect generic code (and other targets) of using it. This helps >> detecting wrong dependencies between components. > I think it's true. However when headers were moved to include we > weren't always able to do this correctly. So some i386 > specific headers are under include. > OK. So if I understand correctly, you also support moving this header to hw/i386/ instead of include/hw/i386/. So I will do so in v4. Thanks, -Liran
diff --git a/hw/i386/pc.c b/hw/i386/pc.c index 6ab4acb0c62e..6ac71e1af32b 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -31,6 +31,7 @@ #include "hw/i386/apic.h" #include "hw/i386/topology.h" #include "hw/i386/fw_cfg.h" +#include "hw/i386/vmport.h" #include "sysemu/cpus.h" #include "hw/block/fdc.h" #include "hw/ide.h" diff --git a/hw/i386/vmmouse.c b/hw/i386/vmmouse.c index e8e62bd96b8c..49a546fd3bb6 100644 --- a/hw/i386/vmmouse.c +++ b/hw/i386/vmmouse.c @@ -26,6 +26,7 @@ #include "qapi/error.h" #include "ui/console.h" #include "hw/i386/pc.h" +#include "hw/i386/vmport.h" #include "hw/input/i8042.h" #include "hw/qdev-properties.h" #include "migration/vmstate.h" diff --git a/hw/i386/vmport.c b/hw/i386/vmport.c index ead2f2d5326f..e9ea5fe7f765 100644 --- a/hw/i386/vmport.c +++ b/hw/i386/vmport.c @@ -31,6 +31,7 @@ #include "qemu/osdep.h" #include "hw/isa/isa.h" #include "hw/i386/pc.h" +#include "hw/i386/vmport.h" #include "hw/input/i8042.h" #include "hw/qdev-properties.h" #include "sysemu/hw_accel.h" diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h index d5ac76d54e1f..60c988c4a5aa 100644 --- a/include/hw/i386/pc.h +++ b/include/hw/i386/pc.h @@ -134,19 +134,6 @@ typedef struct PCMachineClass { GSIState *pc_gsi_create(qemu_irq **irqs, bool pci_enabled); -/* vmport.c */ -#define TYPE_VMPORT "vmport" -typedef uint32_t (VMPortReadFunc)(void *opaque, uint32_t address); - -static inline void vmport_init(ISABus *bus) -{ - isa_create_simple(bus, TYPE_VMPORT); -} - -void vmport_register(unsigned char command, VMPortReadFunc *func, void *opaque); -void vmmouse_get_data(uint32_t *data); -void vmmouse_set_data(const uint32_t *data); - /* pc.c */ extern int fd_bootchk; diff --git a/include/hw/i386/vmport.h b/include/hw/i386/vmport.h new file mode 100644 index 000000000000..f0c1e985ca08 --- /dev/null +++ b/include/hw/i386/vmport.h @@ -0,0 +1,16 @@ +#ifndef HW_VMPORT_H +#define HW_VMPORT_H + +#define TYPE_VMPORT "vmport" +typedef uint32_t (VMPortReadFunc)(void *opaque, uint32_t address); + +static inline void vmport_init(ISABus *bus) +{ + isa_create_simple(bus, TYPE_VMPORT); +} + +void vmport_register(unsigned char command, VMPortReadFunc *func, void *opaque); +void vmmouse_get_data(uint32_t *data); +void vmmouse_set_data(const uint32_t *data); + +#endif
No functional change. This is mere refactoring. Suggested-by: Michael S. Tsirkin <mst@redhat.com> Signed-off-by: Liran Alon <liran.alon@oracle.com> --- hw/i386/pc.c | 1 + hw/i386/vmmouse.c | 1 + hw/i386/vmport.c | 1 + include/hw/i386/pc.h | 13 ------------- include/hw/i386/vmport.h | 16 ++++++++++++++++ 5 files changed, 19 insertions(+), 13 deletions(-) create mode 100644 include/hw/i386/vmport.h