[v3] : hw/i386/vmport: Bug fixes and improvements

[PATCH v3 07/16] hw/i386/vmport: Introduce vmport.h

Posted by Liran Alon 5 years, 8 months ago

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

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
-- 
2.20.1

Re: [PATCH v3 07/16] hw/i386/vmport: Introduce vmport.h

Posted by Philippe Mathieu-Daudé 5 years, 8 months ago

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
>

Re: [PATCH v3 07/16] hw/i386/vmport: Introduce vmport.h

Posted by Liran Alon 5 years, 8 months ago

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

Re: [PATCH v3 07/16] hw/i386/vmport: Introduce vmport.h

Posted by Philippe Mathieu-Daudé 5 years, 8 months ago

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?

Re: [PATCH v3 07/16] hw/i386/vmport: Introduce vmport.h

Posted by Liran Alon 5 years, 8 months ago

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?
>

Re: [PATCH v3 07/16] hw/i386/vmport: Introduce vmport.h

Posted by Michael S. Tsirkin 5 years, 8 months ago

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?

Re: [PATCH v3 07/16] hw/i386/vmport: Introduce vmport.h

Posted by Liran Alon 5 years, 8 months ago

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