From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH v4] x86: properly handle MSI-X unmask operation from guests Date: Sat, 23 Nov 2013 16:19:48 +0000 Message-ID: <5290D5A4.50908@citrix.com> References: <528F91F80200007800105FBB@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" 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 1VkFw0-0004FS-L3 for xen-devel@lists.xenproject.org; Sat, 23 Nov 2013 16:19:52 +0000 In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: "Wu, Feng" Cc: "Nakajima, Jun" , xen-devel , "Auld, Will" , "Zhang, Xiantao" , Jan Beulich List-Id: xen-devel@lists.xenproject.org On 23/11/13 01:40, Wu, Feng wrote: > >> -----Original Message----- >> From: Jan Beulich [mailto:JBeulich@suse.com] >> Sent: Saturday, November 23, 2013 12:19 AM >> To: Wu, Feng >> Cc: Nakajima, Jun; Auld, Will; Zhang, Xiantao; xen-devel >> Subject: Re: [PATCH v4] x86: properly handle MSI-X unmask operation from >> guests >> >>>>> On 22.11.13 at 02:08, "Wu, Feng" wrote: >>> v1: Initial patch to handle this issue involving changing the hypercall interface >>> v2:Totally handled inside hypervisor. >>> v3:Change some logics of handling msi-x pending unmask operations. >>> v4:Some changes related to coding style according to Andrew Cooper's >> comments >> >> So this is _much_ less intrusive than what you did before - good! >> >>> --- a/xen/arch/x86/hvm/io.c >>> +++ b/xen/arch/x86/hvm/io.c >>> @@ -297,6 +297,9 @@ void hvm_io_assist(ioreq_t *p) >>> break; >>> } >>> >>> + if ( msix_post_handler(curr) ) >>> + gdprintk(XENLOG_ERR, "msix_post_handler error\n"); >>> + >>> if ( p->state == STATE_IOREQ_NONE ) >>> vcpu_end_shutdown_deferral(curr); >> I think the addition should be moved into the body of this if(), >> so that it gets executed only upon completion of I/O, not when >> it e.g. need retrying. >> >> Also, XENLOG_ERR seems to heavy a message. XENLOG_WARN >> would be the highest I'd accept. >> >>> +int msix_post_handler(struct vcpu *v) >>> +{ >>> + int rc; >>> + >>> + if ( v->arch.pending_msix_unmask.valid == 0 ) >> Iff you keep this (see below), then boolean checks are >> conventionally done with ! rather than == 0. >> >>> + return 0; >>> + >>> + v->arch.pending_msix_unmask.valid = 0; >>> + >>> + rc = msixtbl_write(v, v->arch.pending_msix_unmask.ctrl_address, 4, 0); >>> + return rc != X86EMUL_OKAY ? -1 : 0; >> Make the function return bool_t, and then simply >> >> return msixtbl_write(v, v->arch.pending_msix_unmask.ctrl_address, 4, 0) == >> X86EMUL_OKAY; >> >>> +struct pending_msix_unmask_info >>> +{ >>> + unsigned long ctrl_address; >>> + bool_t valid; >>> +}; >>> + >>> struct arch_vcpu >>> { >>> /* >>> @@ -439,6 +445,8 @@ struct arch_vcpu >>> >>> /* A secondary copy of the vcpu time info. */ >>> XEN_GUEST_HANDLE(vcpu_time_info_t) time_info_guest; >>> + >>> + struct pending_msix_unmask_info pending_msix_unmask; >> I don't think you need a separate boolean here - for one I don't >> think the address can reasonably be zero, and even if you have >> the bottom two bits available (as it being 4-byte aligned gets >> checked before you consume it). > > The boolean variant "valid", which is set in msixtbl_write(), means whether there is a > msix pending unmask, if there is, just clean this flag and unmask the msix in hardware, > otherwise we just do nothing. If removing "valid", how can I know whether there is a > msix pending unmask operation ? Thanks you! Address can't reasonably be 0, meaning that an address of 0 indicates that an unmask is not pending. ~Andrew