From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paul Durrant Subject: Re: [PATCH v4 12/17] x86/hvm: split I/O completion handling from state model Date: Thu, 25 Jun 2015 15:59:21 +0000 Message-ID: <9AAE0902D5BC7E449B7C8E4E778ABCD0259715BA@AMSPEX01CL02.citrite.net> References: <1435145089-21999-1-git-send-email-paul.durrant@citrix.com> <1435145089-21999-13-git-send-email-paul.durrant@citrix.com> <558BE8B602000078000896D4@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta3.messagelabs.com ([195.245.230.39]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1Z89Z5-0000CH-8Q for xen-devel@lists.xenproject.org; Thu, 25 Jun 2015 15:59:47 +0000 In-Reply-To: <558BE8B602000078000896D4@mail.emea.novell.com> Content-Language: en-US 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: Andrew Cooper , "Keir (Xen.org)" , "xen-devel@lists.xenproject.org" List-Id: xen-devel@lists.xenproject.org > -----Original Message----- > From: Jan Beulich [mailto:JBeulich@suse.com] > Sent: 25 June 2015 10:41 > To: Paul Durrant > Cc: Andrew Cooper; xen-devel@lists.xenproject.org; Keir (Xen.org) > Subject: Re: [PATCH v4 12/17] x86/hvm: split I/O completion handling from > state model > > >>> On 24.06.15 at 13:24, wrote: > > @@ -428,26 +429,12 @@ static void hvm_io_assist(ioreq_t *p) > > vio->io_state = HVMIO_completed; > > vio->io_data = p->data; > > break; > > - case HVMIO_handle_mmio_awaiting_completion: > > - vio->io_state = HVMIO_completed; > > - vio->io_data = p->data; > > - (void)handle_mmio(); > > - break; > > - case HVMIO_handle_pio_awaiting_completion: > > - if ( vio->io_size == 4 ) /* Needs zero extension. */ > > - guest_cpu_user_regs()->rax = (uint32_t)p->data; > > - else > > - memcpy(&guest_cpu_user_regs()->rax, &p->data, vio->io_size); > > - break; > > default: > > break; > > } > > > > - if ( p->state == STATE_IOREQ_NONE ) > > - { > > - msix_write_completion(curr); > > - vcpu_end_shutdown_deferral(curr); > > - } > > + msix_write_completion(curr); > > + vcpu_end_shutdown_deferral(curr); > > } > > Doesn't this mean that these can now potentially be called more than > once for a single instruction (due to retries)? That's presumably not a > problem for vcpu_end_shutdown_deferral(), but I'm uncertain about > msix_write_completion(). > Actually I think this is the wrong place for this now. It should have been moved in or prior to patch 11. I found the need for a completion function for stdvga so I'll use the same mechanism for msix_write_completion(). As you say, vcpu_end_shutdown_deferral() can stay. > > @@ -508,8 +496,36 @@ void hvm_do_resume(struct vcpu *v) > > } > > } > > > > - if ( vio->mmio_retry ) > > + io_completion = vio->io_completion; > > + vio->io_completion = HVMIO_no_completion; > > + > > + switch ( io_completion ) > > + { > > + case HVMIO_no_completion: > > + break; > > + case HVMIO_mmio_completion: > > (void)handle_mmio(); > > + break; > > + case HVMIO_pio_completion: > > + if ( vio->io_size == 4 ) /* Needs zero extension. */ > > + guest_cpu_user_regs()->rax = (uint32_t)vio->io_data; > > + else > > + memcpy(&guest_cpu_user_regs()->rax, &vio->io_data, vio- > >io_size); > > + vio->io_state = HVMIO_none; > > + break; > > + case HVMIO_realmode_completion: > > + { > > + struct hvm_emulate_ctxt ctxt; > > + > > + hvm_emulate_prepare(&ctxt, guest_cpu_user_regs()); > > + vmx_realmode_emulate_one(&ctxt); > > + hvm_emulate_writeback(&ctxt); > > + > > + break; > > + } > > + default: > > + BUG(); > > I'd prefer such to be ASSERT_UNREACHABLE(), as I don't see > getting here to be a reason to crash the hypervisor (and the > guest would likely crash in such a case anyway). Or maybe > fold in the first case and make this > ASSERT(io_completion == HVMIO_no_completion). > Ok. > > @@ -46,9 +51,10 @@ struct hvm_vcpu_asid { > > > > struct hvm_vcpu_io { > > /* I/O request in flight to device model. */ > > - enum hvm_io_state io_state; > > - unsigned long io_data; > > - int io_size; > > + enum hvm_io_state io_state; > > + unsigned long io_data; > > + int io_size; > > Unless this can validly be negative, please make this unsigned int > if you already touch it. > Sure. Paul > Jan