[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v12 7/8] Add IOREQ_TYPE_VMWARE_PORT



On 07/01/15 16:49, Konrad Rzeszutek Wilk wrote:
On Sat, Jun 27, 2015 at 07:27:44PM -0400, Don Slutz wrote:
From: Don Slutz <dslutz@xxxxxxxxxxx>

This adds synchronization of the 6 vcpu registers (only 32bits of
them) that vmport.c needs between Xen and QEMU.

...
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -169,33 +169,88 @@ static int hvmemul_do_io(
              vio->io_state = HVMIO_handle_mmio_awaiting_completion;
          break;
      case X86EMUL_UNHANDLEABLE:
-    {
-        struct hvm_ioreq_server *s =
-            hvm_select_ioreq_server(curr->domain, &p);
-
-        /* If there is no suitable backing DM, just ignore accesses */
-        if ( !s )
+        if ( vmport_check_port(p.addr, p.size) )
I would have made this !. Thought if Jan or Andrew said to make it that
way - then please ignore me.

I have not checked, but I do not remember any direct statement either way. Last was Jan's "looks better".

That would mean you could also change the function to be 'is_port_vmport' or
such.


Happy to make a change here if needed. Note: This part of the patch conflicts with:


From: Paul Durrant <paul.durrant@xxxxxxxxxx>
To: <xen-devel@xxxxxxxxxxxxxxxxxxxx>
Date: Tue, 30 Jun 2015 14:05:43 +0100
Message-ID: <1435669558-5421-2-git-send-email-paul.durrant@xxxxxxxxxx>
Subject: [Xen-devel] [PATCH v5 01/16] x86/hvm: make sure emulation is
        retried if domain is shutting down


As so is waiting till that is sorted out.

          {
-            hvm_complete_assist_req(&p);
-            rc = X86EMUL_OKAY;
-            vio->io_state = HVMIO_none;
...
+
+                vio->io_state = HVMIO_handle_pio_awaiting_completion;
+                rc = hvm_send_assist_req(s, &p);
+                if ( rc != X86EMUL_RETRY )
+                {
+                    vio->io_state = HVMIO_none;
+                    if ( rc == X86EMUL_OKAY )
+                        rc = X86EMUL_RETRY;
+                }
+            }
          }
          break;
-    }
      default:
          BUG();
      }
if ( rc != X86EMUL_OKAY )
+    {
+        /*
+         * If rc is X86EMUL_RETRY and vio->io_state is
+         * HVMIO_handle_pio_awaiting_completion, then were are of
were are? Perhaps 'were'?
+         * type IOREQ_TYPE_VMWARE_PORT, so completion in
+         * hvm_io_assist() with no re-emulation required
.. is required.


Ok. Plan to change.


With commit 2df1aa01 (and the result of Paul Durrant's change) I was considering just adding a "return X86EMUL_OKAY" as the else of the check on rc above, and therefore drop the need for this check and comment.


+         */
+        if ( rc == X86EMUL_RETRY &&
+             vio->io_state == HVMIO_handle_pio_awaiting_completion )
+            rc = X86EMUL_OKAY;
          return rc;
+    }
finish_access:
      if ( dir == IOREQ_READ )
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index f8ec170..ce8cd09 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -394,6 +394,47 @@ static ioreq_t *get_ioreq(struct hvm_ioreq_server *s, 
struct vcpu *v)
      return &p->vcpu_ioreq[v->vcpu_id];
  }
+static vmware_regs_t *get_vmport_regs_one(struct hvm_ioreq_server *s,
+                                          struct vcpu *v)
+{
+    struct hvm_ioreq_vcpu *sv;
+
+    list_for_each_entry ( sv,
+                          &s->ioreq_vcpu_list,
+                          list_entry )
Could it be just made it one nice line?

I think so, so plan to change.

.. snip..

@@ -2507,6 +2637,13 @@ struct hvm_ioreq_server *hvm_select_ioreq_server(struct 
domain *d,
              }
break;
+        case IOREQ_TYPE_VMWARE_PORT:
+        case IOREQ_TYPE_TIMEOFFSET:
+            /* The 'special' range of [1,1] is checked for being enabled */
Missing full stop.

Will add.

+            if ( rangeset_contains_singleton(r, 1) )
+                return s;
+
+            break;
          }
      }
@@ -2669,6 +2806,7 @@ void hvm_complete_assist_req(ioreq_t *p)
      case IOREQ_TYPE_PCI_CONFIG:
          ASSERT_UNREACHABLE();
          break;
+    case IOREQ_TYPE_VMWARE_PORT:
      case IOREQ_TYPE_COPY:
      case IOREQ_TYPE_PIO:
          if ( p->dir == IOREQ_READ )
diff --git a/xen/arch/x86/hvm/io.c b/xen/arch/x86/hvm/io.c
index c0964ec..c1379bf 100644
--- a/xen/arch/x86/hvm/io.c
+++ b/xen/arch/x86/hvm/io.c
@@ -192,6 +192,22 @@ void hvm_io_assist(ioreq_t *p)
          (void)handle_mmio();
          break;
      case HVMIO_handle_pio_awaiting_completion:
+        if ( p->type == IOREQ_TYPE_VMWARE_PORT )
+        {
+            vmware_regs_t *vr = get_vmport_regs_any(NULL, curr);
+
+            if ( vr )
+            {
+                struct cpu_user_regs *regs = guest_cpu_user_regs();
+
+                /* Only change the 32bit part of the register */
Missing full stop.

Will add.


+                regs->_ebx = vr->ebx;
+                regs->_ecx = vr->ecx;
+                regs->_edx = vr->edx;
+                regs->_esi = vr->esi;
+                regs->_edi = vr->edi;
+            }
+        }
          if ( vio->io_size == 4 ) /* Needs zero extension. */
              guest_cpu_user_regs()->rax = (uint32_t)p->data;
          else
diff --git a/xen/arch/x86/hvm/vmware/vmport.c b/xen/arch/x86/hvm/vmware/vmport.c
index f24d8e3..5e14402 100644
--- a/xen/arch/x86/hvm/vmware/vmport.c
+++ b/xen/arch/x86/hvm/vmware/vmport.c
@@ -137,6 +137,19 @@ void vmport_register(struct domain *d)
      register_portio_handler(d, BDOOR_PORT, 4, vmport_ioport);
  }
+int vmport_check_port(unsigned int port, unsigned int bytes)
+{
+    struct vcpu *curr = current;
+    struct domain *currd = curr->domain;
+
+    if ( port + bytes > BDOOR_PORT && port < BDOOR_PORT + 4 &&
+         is_hvm_domain(currd) &&
+         currd->arch.hvm_domain.is_vmware_port_enabled ) {
The '{' should be on its own line.

Will fix.

+        return 0;
+    }
+    return 1;
+}
+
...
+struct shared_vmport_iopage {
+    struct vmware_regs vcpu_vmport_regs[1];
+};
+typedef struct shared_vmport_iopage shared_vmport_iopage_t;
+
  struct buf_ioreq {
      uint8_t  type;   /* I/O type                    */
      uint8_t  pad:1;
diff --git a/xen/include/public/hvm/params.h b/xen/include/public/hvm/params.h
index 7c73089..130eba9 100644
--- a/xen/include/public/hvm/params.h
+++ b/xen/include/public/hvm/params.h
@@ -50,6 +50,8 @@
  #define HVM_PARAM_PAE_ENABLED  4
#define HVM_PARAM_IOREQ_PFN 5
+/* Extra vmport PFN. */
s/Extra/optional/ ?

I do not think "optional" is good to use here. This PFN is required for QEMU (or any other ioreq server) to handle vmport access. If you have a better name then "extra", please let me know.

   -Don Slutz

+#define HVM_PARAM_VMPORT_REGS_PFN 35
#define HVM_PARAM_BUFIOREQ_PFN 6
  #define HVM_PARAM_BUFIOREQ_EVTCHN 26
@@ -187,6 +189,6 @@
  /* Location of the VM Generation ID in guest physical address space. */
  #define HVM_PARAM_VM_GENERATION_ID_ADDR 34
-#define HVM_NR_PARAMS 35
+#define HVM_NR_PARAMS          36
#endif /* __XEN_PUBLIC_HVM_PARAMS_H__ */
--
1.8.3.1



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.