From mboxrd@z Thu Jan 1 00:00:00 1970 From: George Dunlap Subject: Re: [PATCH v11 1/2] vmx: VT-d posted-interrupt core logic handling Date: Wed, 10 Feb 2016 12:25:17 +0000 Message-ID: <56BB2C2D.20402@citrix.com> References: <1453957951-17062-1-git-send-email-feng.wu@intel.com> <1453957951-17062-2-git-send-email-feng.wu@intel.com> <56AA520702000078000CC1EF@prv-mh.provo.novell.com> <56AB3F8702000078000CC4C7@prv-mh.provo.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: 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" , Jan Beulich Cc: "Tian, Kevin" , Keir Fraser , George Dunlap , Andrew Cooper , Dario Faggioli , "xen-devel@lists.xen.org" List-Id: xen-devel@lists.xenproject.org On 02/02/16 04:48, Wu, Feng wrote: >>>>> +static void vmx_pi_do_resume(struct vcpu *v) >>>>> +{ >>>>> + unsigned long flags; >>>>> + spinlock_t *pi_block_list_lock; >>>>> + struct pi_desc *pi_desc = &v->arch.hvm_vmx.pi_desc; >>>>> + >>>>> + ASSERT(!test_bit(_VPF_blocked, &v->pause_flags)); >>>>> + >>>>> + /* >>>>> + * Set 'NV' field back to posted_intr_vector, so the >>>>> + * Posted-Interrupts can be delivered to the vCPU when >>>>> + * it is running in non-root mode. >>>>> + */ >>>>> + if ( pi_desc->nv != posted_intr_vector ) >>>>> + write_atomic(&pi_desc->nv, posted_intr_vector); >>>> >>>> Perhaps this was discussed before, but I don't recall and now >>>> wonder - why inside an if()? This is a simple memory write on >>>> x86. >>> >>> The initial purpose is that if NV is already equal to 'posted_intr_vector', >>> we can save the following atomically write operation. There are some >>> volatile stuff and barriers in write_atomic(). >> >> But what does the final generated code look like? As I said, I'm >> sure it's just a single MOV. And putting a conditional around it will >> likely make things slower rather than faster. > > Looking at the implementation of wirte_atomic(), it has volatile key > word barrier inside, if you think this is not a big deal, I am fine to > remove the check. Oh, right -- so set_sn and clear_sn use the set/clear bits, which are atomic read-modify-write, which we'd like to avoid on the vmexit/vmentry paths (which is why we have the scheduler hooks); but this is just a straight up write, so it's OK. >>>>> --- a/xen/drivers/passthrough/vtd/iommu.c >>>>> +++ b/xen/drivers/passthrough/vtd/iommu.c >>>>> @@ -2293,6 +2293,8 @@ static int reassign_device_ownership( >>>>> pdev->domain = target; >>>>> } >>>>> >>>>> + vmx_pi_hooks_reassign(source, target); >>>>> + >>>>> return ret; >>>>> } >>>> >>>> I think you need to clear source's hooks here, but target's need to >>>> get set ahead of the actual assignment. >>> >>> I think this is the place where the device is moved from >>> &source->arch.pdev_list to &target->arch.pdev_list, if that is the >>> case, we should clear source and set target here, right? >> >> No - target needs to be ready to deal with events from the device >> _before_ the device actually gets assigned to it. > > I still don't get your point. I still think this is the place where the device > is being got assigned. Or maybe you can share more info about the > place "_before_ the device actually gets assigned to it " ? What happens if a device generates a PI interrupt *immediately* after domain_context_mapping(), but before calling vmx_pi_hooks_reassign()? -George