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

Re: [Xen-devel] [PATCH v14 12/17] pvh: Use PV handlers for cpuid, and IO



On 05/11/13 08:42, Jan Beulich wrote:
On 04.11.13 at 13:15, George Dunlap <george.dunlap@xxxxxxxxxxxxx> wrote:
@@ -140,6 +146,9 @@ static int hvmemul_do_io(
          }
      }
+ if ( is_pvh_vcpu(curr) )
+        ASSERT(vio->io_state == HVMIO_none);
Can we really get here for PVH?

Nope -- sorry I missed that one. :-)


+static int pvhemul_do_pio(
+    unsigned long port, int size, paddr_t ram_gpa, int dir, void *p_data)
+{
+    paddr_t value = ram_gpa;
+    struct vcpu *curr = current;
+    struct cpu_user_regs *regs = guest_cpu_user_regs();
+
+    /*
+     * Weird-sized accesses have undefined behaviour: we discard writes
+     * and read all-ones.
+     */
+    if ( unlikely((size > sizeof(long)) || (size & (size - 1))) )
I think you can safely ASSERT() here - PIO instructions never have
operand sizes not matching the criteria above.

+    {
+        gdprintk(XENLOG_WARNING, "bad mmio size %d\n", size);
+        ASSERT(p_data != NULL); /* cannot happen with a REP prefix */
+        if ( dir == IOREQ_READ )
+            memset(p_data, ~0, size);
+        return X86EMUL_UNHANDLEABLE;
+    }
+
+    if ( dir == IOREQ_WRITE ) {
+        if ( (p_data != NULL) )
Coding style (two instances).

+        {
+            memcpy(&value, p_data, size);
+            p_data = NULL;
+        }
+
+    if ( dir == IOREQ_WRITE )
+        trace_io_assist(0, dir, 1, port, value);
Indentation (or really pointless if()).

Oops...


+
+        guest_io_write(port, size, value, curr, regs);
+    }
+    else
+    {
+        value = guest_io_read(port, size, curr, regs);
+        trace_io_assist(0, dir, 1, port, value);
+        if ( (p_data != NULL) )
Coding style again (sort of at least).

+            memcpy(p_data, &value, size);
+        memcpy(&regs->eax, &value, size);
What is this being matched by in (a) the HVM equivalent and (b)
the write code path? And even if needed, this surely wouldn't
be correct for the size == 4 case (where the upper 32 bits of
any destination register get zeroed).

Hmm, now that I take a second look, I see that this apparently
originates from handle_pio() (which however does the reading
of ->eax as well), so the above comment actually points out a
bug there (which I'm going to prepare a patch for right away).

+    }
+
+    return X86EMUL_OKAY;
+}
+
+
  int hvmemul_do_pio(
      unsigned long port, unsigned long *reps, int size,
      paddr_t ram_gpa, int dir, int df, void *p_data)
  {
-    return hvmemul_do_io(0, port, reps, size, ram_gpa, dir, df, p_data);
+    return is_hvm_vcpu(current) ?
+        hvmemul_do_io(0, port, reps, size, ram_gpa, dir, df, p_data) :
+        pvhemul_do_pio(port, size, ram_gpa, dir, p_data);
You're losing "reps" and "df" here.

Hmm... yes.  Time to do some re-thinking on this one.

 -George

_______________________________________________
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®.