Re: [Qemu-devel] [PATCH v8 8/8] vmport: Add VMware all ring hack

[Michael S. Tsirkin]

On Tue, Jun 23, 2015 at 07:39:33PM -0400, Don Slutz wrote:
> From: Don Slutz <address@hidden>
> 
> This is done by adding a new machine property vmware-port-ring3 that
> needs to be enabled to have any effect.  It only effects accel=tcg
> mode.  It is needed if you want to use VMware tools in accel=tcg
> mode.
> 
> Signed-off-by: Don Slutz <address@hidden>
> CC: Don Slutz <address@hidden>

IMO this patch isn't ready yet.

> ---
>  hw/i386/pc.c             | 26 +++++++++++++++++++++++++-
>  hw/i386/pc_piix.c        |  2 +-
>  hw/i386/pc_q35.c         |  2 +-
>  include/hw/i386/pc.h     |  6 +++++-
>  target-i386/cpu-qom.h    |  3 +++
>  target-i386/seg_helper.c |  9 +++++++++
>  6 files changed, 44 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 7072930..3ae4476 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1042,7 +1042,9 @@ void pc_hot_add_cpu(const int64_t id, Error **errp)
>      object_unref(OBJECT(cpu));
>  }
>  
> -void pc_cpus_init(const char *cpu_model, DeviceState *icc_bridge)
> +/* vmware_port_ring3 true says enable VMware port access in ring3. */
> +void pc_cpus_init(const char *cpu_model, DeviceState *icc_bridge,
> +                  bool vmware_port_ring3)
>  {
>      int i;
>      X86CPU *cpu = NULL;
> @@ -1073,6 +1075,7 @@ void pc_cpus_init(const char *cpu_model, DeviceState 
> *icc_bridge)
>              error_report_err(error);
>              exit(1);
>          }
> +        cpu->allow_vmport_ring3 = vmware_port_ring3;
>          object_unref(OBJECT(cpu));
>      }
>  
> @@ -1835,6 +1838,21 @@ static bool pc_machine_get_aligned_dimm(Object *obj, 
> Error **errp)
>      return pcms->enforce_aligned_dimm;
>  }
>  
> +static bool pc_machine_get_vmware_port_ring3(Object *obj, Error **errp)
> +{
> +    PCMachineState *pcms = PC_MACHINE(obj);
> +
> +    return pcms->vmware_port_ring3;
> +}
> +
> +static void pc_machine_set_vmware_port_ring3(Object *obj, bool value,
> +                                             Error **errp)
> +{
> +    PCMachineState *pcms = PC_MACHINE(obj);
> +
> +    pcms->vmware_port_ring3 = value;
> +}
> +
>  static void pc_machine_initfn(Object *obj)
>  {
>      PCMachineState *pcms = PC_MACHINE(obj);
> @@ -1865,6 +1883,12 @@ static void pc_machine_initfn(Object *obj)
>      object_property_add_bool(obj, PC_MACHINE_ENFORCE_ALIGNED_DIMM,
>                               pc_machine_get_aligned_dimm,
>                               NULL, NULL);
> +
> +    pcms->vmware_port_ring3 = false;
> +    object_property_add_bool(obj, PC_MACHINE_VMWARE_PORT_RING3,
> +                             pc_machine_get_vmware_port_ring3,
> +                             pc_machine_set_vmware_port_ring3,
> +                             NULL);

New properties should have a description.

>  }
>  
>  static unsigned pc_cpu_index_to_socket_id(unsigned cpu_index)
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index e142f75..890c45e 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -146,7 +146,7 @@ static void pc_init1(MachineState *machine)
>      object_property_add_child(qdev_get_machine(), "icc-bridge",
>                                OBJECT(icc_bridge), NULL);
>  
> -    pc_cpus_init(machine->cpu_model, icc_bridge);
> +    pc_cpus_init(machine->cpu_model, icc_bridge, 
> pc_machine->vmware_port_ring3);
>  
>      if (kvm_enabled() && kvmclock_enabled) {
>          kvmclock_create();
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 082cd93..8288a5f 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -137,7 +137,7 @@ static void pc_q35_init(MachineState *machine)
>      object_property_add_child(qdev_get_machine(), "icc-bridge",
>                                OBJECT(icc_bridge), NULL);
>  
> -    pc_cpus_init(machine->cpu_model, icc_bridge);
> +    pc_cpus_init(machine->cpu_model, icc_bridge, 
> pc_machine->vmware_port_ring3);
>      pc_acpi_init("q35-acpi-dsdt.aml");
>  
>      kvmclock_create();
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 3270a97..f9e74c7 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -40,6 +40,7 @@ struct PCMachineState {
>  
>      uint64_t max_ram_below_4g;
>      OnOffAuto vmport;
> +    bool vmware_port_ring3;
>      bool enforce_aligned_dimm;
>  };
>  
> @@ -48,6 +49,7 @@ struct PCMachineState {
>  #define PC_MACHINE_MAX_RAM_BELOW_4G "max-ram-below-4g"
>  #define PC_MACHINE_VMPORT           "vmport"
>  #define PC_MACHINE_ENFORCE_ALIGNED_DIMM "enforce-aligned-dimm"
> +#define PC_MACHINE_VMWARE_PORT_RING3 "vmware-port-ring3"
>  
>  /**
>   * PCMachineClass:

Creating it at the pc level and propagating makes code
messy but I'd go along with it if it made sense from
user's point of view, but it does not seem to make sense:
to me this looks more like a CPU feature than a machine property.
Or the property of the vmport rpc device that you created.

If it's required for the rpc device to be used -
why not set this unconditionally if the rpc device
is created? Not saying it's a good idea - just asking.


> @@ -161,7 +163,9 @@ extern int fd_bootchk;
>  void pc_register_ferr_irq(qemu_irq irq);
>  void pc_acpi_smi_interrupt(void *opaque, int irq, int level);
>  
> -void pc_cpus_init(const char *cpu_model, DeviceState *icc_bridge);
> +/* vmware_port_ring3 true says enable VMware port access in ring3. */
> +void pc_cpus_init(const char *cpu_model, DeviceState *icc_bridge,
> +                  bool vmware_port_ring3);
>  void pc_hot_add_cpu(const int64_t id, Error **errp);
>  void pc_acpi_init(const char *default_dsdt);
>  
> diff --git a/target-i386/cpu-qom.h b/target-i386/cpu-qom.h
> index 7a4fddd..ccbc5bb 100644
> --- a/target-i386/cpu-qom.h
> +++ b/target-i386/cpu-qom.h
> @@ -109,6 +109,9 @@ typedef struct X86CPU {
>       */
>      bool enable_pmu;
>  
> +    /* allow_vmport_ring3 true says enable VMware port access in ring3 */
> +    bool allow_vmport_ring3;
> +
>      /* in order to simplify APIC support, we leave this pointer to the
>         user */
>      struct DeviceState *apic_state;
> diff --git a/target-i386/seg_helper.c b/target-i386/seg_helper.c
> index 8a4271e..2ddb84f 100644
> --- a/target-i386/seg_helper.c
> +++ b/target-i386/seg_helper.c
> @@ -2566,6 +2566,15 @@ static inline void check_io(CPUX86State *env, int 
> addr, int size)
>  {
>      int io_offset, val, mask;
>  
> +    /* vmport hack: skip iopl checking for VMware port 0x5658 (see
> +     * vmport_realizefn()) */
> +    if (addr == 0x5658) {
> +        X86CPU *cpu = x86_env_get_cpu(env);
> +        if (cpu->allow_vmport_ring3) {
> +            return;
> +        }
> +    }
> +
>      /* TSS must be a valid 32 bit one */
>      if (!(env->tr.flags & DESC_P_MASK) ||
>          ((env->tr.flags >> DESC_TYPE_SHIFT) & 0xf) != 9 ||
> -- 
> 1.8.3.1