[PATCH 0/4] hw/i386: Restrict vmport/vmmouse devices to x86 targets

[PATCH 0/4] hw/i386: Restrict vmport/vmmouse devices to x86 targets

[Philippe Mathieu-Daudé]

Some x86 patches worth salvaging while doing housekeeping:
Restrict vmport/vmmouse devices to x86 targets.

A step forward having "hw/i386/pc.h" target-specific...

Philippe Mathieu-Daudé (4):
  hw/i386/pc: Create 'vmport' device in place
  hw/i386/vmport: Remove unused 'hw/input/i8042.h' include
  hw/i386: Add 'vmport.h' local header
  hw/i386: Make vmmouse helpers static

 hw/i386/vmport.h     | 34 ++++++++++++++++++++++++++++++++++
 include/hw/i386/pc.h | 13 -------------
 hw/i386/pc.c         |  3 ++-
 hw/i386/vmmouse.c    | 23 ++++++++++++++++++++++-
 hw/i386/vmport.c     | 25 ++-----------------------
 5 files changed, 60 insertions(+), 38 deletions(-)
 create mode 100644 hw/i386/vmport.h

-- 
2.21.3

[PATCH 1/4] hw/i386/pc: Create 'vmport' device in place

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/i386/pc.h | 6 ------
 hw/i386/pc.c         | 2 +-
 2 files changed, 1 insertion(+), 7 deletions(-)

diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 6ab6eda046..26e2a3d92b 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -132,12 +132,6 @@ 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);
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 5143c51653..84669ddc84 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1152,7 +1152,7 @@ static void pc_superio_init(ISABus *isa_bus, bool create_fdctrl, bool no_vmport)
 
     i8042 = isa_create_simple(isa_bus, "i8042");
     if (!no_vmport) {
-        vmport_init(isa_bus);
+        isa_create_simple(isa_bus, TYPE_VMPORT);
         vmmouse = isa_try_create(isa_bus, "vmmouse");
     } else {
         vmmouse = NULL;
-- 
2.21.3

Re: [PATCH 1/4] hw/i386/pc: Create 'vmport' device in place

[Richard Henderson]

On 5/4/20 1:33 AM, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  include/hw/i386/pc.h | 6 ------
>  hw/i386/pc.c         | 2 +-
>  2 files changed, 1 insertion(+), 7 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

[PATCH 2/4] hw/i386/vmport: Remove unused 'hw/input/i8042.h' include

[Philippe Mathieu-Daudé]

Remove unused "hw/input/i8042.h" include.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/i386/vmport.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/i386/vmport.c b/hw/i386/vmport.c
index 1f31e27c8a..114141c6f3 100644
--- a/hw/i386/vmport.c
+++ b/hw/i386/vmport.c
@@ -24,7 +24,6 @@
 #include "qemu/osdep.h"
 #include "hw/isa/isa.h"
 #include "hw/i386/pc.h"
-#include "hw/input/i8042.h"
 #include "sysemu/hw_accel.h"
 #include "qemu/log.h"
 #include "trace.h"
-- 
2.21.3

Re: [PATCH 2/4] hw/i386/vmport: Remove unused 'hw/input/i8042.h' include

[Richard Henderson]

On 5/4/20 1:33 AM, Philippe Mathieu-Daudé wrote:
> Remove unused "hw/input/i8042.h" include.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/i386/vmport.c | 1 -
>  1 file changed, 1 deletion(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

[PATCH 3/4] hw/i386: Add 'vmport.h' local header

[Philippe Mathieu-Daudé]

Move 'vmport' related declarations in a target-specific header.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/i386/vmport.h     | 34 ++++++++++++++++++++++++++++++++++
 include/hw/i386/pc.h |  3 ---
 hw/i386/pc.c         |  1 +
 hw/i386/vmmouse.c    |  1 +
 hw/i386/vmport.c     |  1 +
 5 files changed, 37 insertions(+), 3 deletions(-)
 create mode 100644 hw/i386/vmport.h

diff --git a/hw/i386/vmport.h b/hw/i386/vmport.h
new file mode 100644
index 0000000000..47eda7a22b
--- /dev/null
+++ b/hw/i386/vmport.h
@@ -0,0 +1,34 @@
+/*
+ * QEMU VMPort emulation
+ *
+ * Copyright (C) 2007 Hervé Poussineau
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#ifndef HW_I386_VMPORT_H
+#define HW_I386_VMPORT_H
+
+#define TYPE_VMPORT "vmport"
+
+typedef uint32_t (VMPortReadFunc)(void *opaque, uint32_t address);
+
+void vmport_register(unsigned char command, VMPortReadFunc *func, void *opaque);
+
+#endif
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 26e2a3d92b..de49a57506 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -130,9 +130,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);
-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);
 
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 84669ddc84..f6b8431c8b 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -91,6 +91,7 @@
 #include "qapi/qmp/qerror.h"
 #include "config-devices.h"
 #include "e820_memory_layout.h"
+#include "vmport.h"
 #include "fw_cfg.h"
 #include "trace.h"
 
diff --git a/hw/i386/vmmouse.c b/hw/i386/vmmouse.c
index e8e62bd96b..78b36f6f5d 100644
--- a/hw/i386/vmmouse.c
+++ b/hw/i386/vmmouse.c
@@ -29,6 +29,7 @@
 #include "hw/input/i8042.h"
 #include "hw/qdev-properties.h"
 #include "migration/vmstate.h"
+#include "vmport.h"
 
 /* debug only vmmouse */
 //#define DEBUG_VMMOUSE
diff --git a/hw/i386/vmport.c b/hw/i386/vmport.c
index 114141c6f3..00d47e0c4c 100644
--- a/hw/i386/vmport.c
+++ b/hw/i386/vmport.c
@@ -26,6 +26,7 @@
 #include "hw/i386/pc.h"
 #include "sysemu/hw_accel.h"
 #include "qemu/log.h"
+#include "vmport.h"
 #include "trace.h"
 
 #define VMPORT_CMD_GETVERSION 0x0a
-- 
2.21.3

Re: [PATCH 3/4] hw/i386: Add 'vmport.h' local header

[Richard Henderson]

On 5/4/20 1:33 AM, Philippe Mathieu-Daudé wrote:
> Move 'vmport' related declarations in a target-specific header.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/i386/vmport.h     | 34 ++++++++++++++++++++++++++++++++++
>  include/hw/i386/pc.h |  3 ---
>  hw/i386/pc.c         |  1 +
>  hw/i386/vmmouse.c    |  1 +
>  hw/i386/vmport.c     |  1 +
>  5 files changed, 37 insertions(+), 3 deletions(-)
>  create mode 100644 hw/i386/vmport.h

> +typedef uint32_t (VMPortReadFunc)(void *opaque, uint32_t address);

Maybe get rid of the useless parenthesis here.

Otherwise,

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

[PATCH 4/4] hw/i386: Make vmmouse helpers static

[Philippe Mathieu-Daudé]

The vmmouse helpers are only used in hw/i386/vmmouse.c,
make them static.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/i386/pc.h |  4 ----
 hw/i386/vmmouse.c    | 22 +++++++++++++++++++++-
 hw/i386/vmport.c     | 23 +----------------------
 3 files changed, 22 insertions(+), 27 deletions(-)

diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index de49a57506..05e19455bb 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -129,10 +129,6 @@ typedef struct PCMachineClass {
 
 GSIState *pc_gsi_create(qemu_irq **irqs, bool pci_enabled);
 
-/* vmport.c */
-void vmmouse_get_data(uint32_t *data);
-void vmmouse_set_data(const uint32_t *data);
-
 /* pc.c */
 extern int fd_bootchk;
 
diff --git a/hw/i386/vmmouse.c b/hw/i386/vmmouse.c
index 78b36f6f5d..b3aef41327 100644
--- a/hw/i386/vmmouse.c
+++ b/hw/i386/vmmouse.c
@@ -25,11 +25,11 @@
 #include "qemu/osdep.h"
 #include "qapi/error.h"
 #include "ui/console.h"
-#include "hw/i386/pc.h"
 #include "hw/input/i8042.h"
 #include "hw/qdev-properties.h"
 #include "migration/vmstate.h"
 #include "vmport.h"
+#include "cpu.h"
 
 /* debug only vmmouse */
 //#define DEBUG_VMMOUSE
@@ -71,6 +71,26 @@ typedef struct VMMouseState
     ISAKBDState *i8042;
 } VMMouseState;
 
+static void vmmouse_get_data(uint32_t *data)
+{
+    X86CPU *cpu = X86_CPU(current_cpu);
+    CPUX86State *env = &cpu->env;
+
+    data[0] = env->regs[R_EAX]; data[1] = env->regs[R_EBX];
+    data[2] = env->regs[R_ECX]; data[3] = env->regs[R_EDX];
+    data[4] = env->regs[R_ESI]; data[5] = env->regs[R_EDI];
+}
+
+static void vmmouse_set_data(const uint32_t *data)
+{
+    X86CPU *cpu = X86_CPU(current_cpu);
+    CPUX86State *env = &cpu->env;
+
+    env->regs[R_EAX] = data[0]; env->regs[R_EBX] = data[1];
+    env->regs[R_ECX] = data[2]; env->regs[R_EDX] = data[3];
+    env->regs[R_ESI] = data[4]; env->regs[R_EDI] = data[5];
+}
+
 static uint32_t vmmouse_get_status(VMMouseState *s)
 {
     DPRINTF("vmmouse_get_status()\n");
diff --git a/hw/i386/vmport.c b/hw/i386/vmport.c
index 00d47e0c4c..1aaaab691a 100644
--- a/hw/i386/vmport.c
+++ b/hw/i386/vmport.c
@@ -23,10 +23,10 @@
  */
 #include "qemu/osdep.h"
 #include "hw/isa/isa.h"
-#include "hw/i386/pc.h"
 #include "sysemu/hw_accel.h"
 #include "qemu/log.h"
 #include "vmport.h"
+#include "cpu.h"
 #include "trace.h"
 
 #define VMPORT_CMD_GETVERSION 0x0a
@@ -109,27 +109,6 @@ static uint32_t vmport_cmd_ram_size(void *opaque, uint32_t addr)
     return ram_size;
 }
 
-/* vmmouse helpers */
-void vmmouse_get_data(uint32_t *data)
-{
-    X86CPU *cpu = X86_CPU(current_cpu);
-    CPUX86State *env = &cpu->env;
-
-    data[0] = env->regs[R_EAX]; data[1] = env->regs[R_EBX];
-    data[2] = env->regs[R_ECX]; data[3] = env->regs[R_EDX];
-    data[4] = env->regs[R_ESI]; data[5] = env->regs[R_EDI];
-}
-
-void vmmouse_set_data(const uint32_t *data)
-{
-    X86CPU *cpu = X86_CPU(current_cpu);
-    CPUX86State *env = &cpu->env;
-
-    env->regs[R_EAX] = data[0]; env->regs[R_EBX] = data[1];
-    env->regs[R_ECX] = data[2]; env->regs[R_EDX] = data[3];
-    env->regs[R_ESI] = data[4]; env->regs[R_EDI] = data[5];
-}
-
 static const MemoryRegionOps vmport_ops = {
     .read = vmport_ioport_read,
     .write = vmport_ioport_write,
-- 
2.21.3

Re: [PATCH 4/4] hw/i386: Make vmmouse helpers static

[Richard Henderson]

On 5/4/20 1:33 AM, Philippe Mathieu-Daudé wrote:
> +++ b/hw/i386/vmport.c
> @@ -23,10 +23,10 @@
>   */
>  #include "qemu/osdep.h"
>  #include "hw/isa/isa.h"
> -#include "hw/i386/pc.h"
>  #include "sysemu/hw_accel.h"
>  #include "qemu/log.h"
>  #include "vmport.h"
> +#include "cpu.h"
>  #include "trace.h"
>  
>  #define VMPORT_CMD_GETVERSION 0x0a
> @@ -109,27 +109,6 @@ static uint32_t vmport_cmd_ram_size(void *opaque, uint32_t addr)
>      return ram_size;
>  }
>  
> -/* vmmouse helpers */
> -void vmmouse_get_data(uint32_t *data)
> -{
> -    X86CPU *cpu = X86_CPU(current_cpu);
> -    CPUX86State *env = &cpu->env;
> -
> -    data[0] = env->regs[R_EAX]; data[1] = env->regs[R_EBX];
> -    data[2] = env->regs[R_ECX]; data[3] = env->regs[R_EDX];
> -    data[4] = env->regs[R_ESI]; data[5] = env->regs[R_EDI];
> -}

Why are you adding "cpu.h" when removing code?
Does that mean you don't need to add "cpu.h" to vmmouse.c?


r~

Re: [PATCH 4/4] hw/i386: Make vmmouse helpers static

[Philippe Mathieu-Daudé]

On 5/4/20 7:29 PM, Richard Henderson wrote:
> On 5/4/20 1:33 AM, Philippe Mathieu-Daudé wrote:
>> +++ b/hw/i386/vmport.c
>> @@ -23,10 +23,10 @@
>>    */
>>   #include "qemu/osdep.h"
>>   #include "hw/isa/isa.h"
>> -#include "hw/i386/pc.h"
>>   #include "sysemu/hw_accel.h"
>>   #include "qemu/log.h"
>>   #include "vmport.h"
>> +#include "cpu.h"
>>   #include "trace.h"
>>   
>>   #define VMPORT_CMD_GETVERSION 0x0a
>> @@ -109,27 +109,6 @@ static uint32_t vmport_cmd_ram_size(void *opaque, uint32_t addr)
>>       return ram_size;
>>   }
>>   
>> -/* vmmouse helpers */
>> -void vmmouse_get_data(uint32_t *data)
>> -{
>> -    X86CPU *cpu = X86_CPU(current_cpu);
>> -    CPUX86State *env = &cpu->env;
>> -
>> -    data[0] = env->regs[R_EAX]; data[1] = env->regs[R_EBX];
>> -    data[2] = env->regs[R_ECX]; data[3] = env->regs[R_EDX];
>> -    data[4] = env->regs[R_ESI]; data[5] = env->regs[R_EDI];
>> -}
> 
> Why are you adding "cpu.h" when removing code?

Because this file still use the X86 register definitions:

   static uint32_t vmport_cmd_get_version(void *opaque, uint32_t addr)
   {
       X86CPU *cpu = X86_CPU(current_cpu);

       cpu->env.regs[R_EBX] = VMPORT_MAGIC;
       return 6;
   }

> Does that mean you don't need to add "cpu.h" to vmmouse.c?

Now both files vmmouse/vmport uses the X86 register definitions, but 
they don't use anything declared in "hw/i386/pc.h".

> 
> 
> r~
>