[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(®s->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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |