From mboxrd@z Thu Jan 1 00:00:00 1970 From: George Dunlap Subject: Re: [PATCH v14 12/17] pvh: Use PV handlers for cpuid, and IO Date: Thu, 7 Nov 2013 16:50:16 +0000 Message-ID: <527BC4C8.7090305@eu.citrix.com> References: <1383567306-6636-1-git-send-email-george.dunlap@eu.citrix.com> <1383567306-6636-13-git-send-email-george.dunlap@eu.citrix.com> <5278BD9A02000078000FF62A@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1VeSmo-0005aQ-J9 for xen-devel@lists.xenproject.org; Thu, 07 Nov 2013 16:50:26 +0000 In-Reply-To: <5278BD9A02000078000FF62A@nat28.tlf.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: xen-devel , Keir Fraser , Tim Deegan List-Id: xen-devel@lists.xenproject.org On 05/11/13 08:42, Jan Beulich wrote: >>>> On 04.11.13 at 13:15, George Dunlap 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