From mboxrd@z Thu Jan 1 00:00:00 1970 From: Razvan Cojocaru Subject: Re: [PATCH V5 1/3] xen/mem_access: Support for memory-content hiding Date: Tue, 14 Jul 2015 16:41:04 +0300 Message-ID: <55A51170.7030109@bitdefender.com> References: <1436807687-9826-1-git-send-email-rcojocaru@bitdefender.com> <1436807687-9826-2-git-send-email-rcojocaru@bitdefender.com> <55A51B140200007800090AE1@mail.emea.novell.com> <55A50E23.5090503@bitdefender.com> <55A52CA60200007800090BBB@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <55A52CA60200007800090BBB@mail.emea.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 , Tim Deegan Cc: jun.nakajima@intel.com, kevin.tian@intel.com, wei.liu2@citrix.com, ian.campbell@citrix.com, stefano.stabellini@eu.citrix.com, george.dunlap@eu.citrix.com, andrew.cooper3@citrix.com, ian.jackson@eu.citrix.com, xen-devel@lists.xen.org, eddie.dong@intel.com, Aravind.Gopalakrishnan@amd.com, suravee.suthikulpanit@amd.com, tlengyel@novetta.com, keir@xen.org, boris.ostrovsky@oracle.com List-Id: xen-devel@lists.xenproject.org On 07/14/2015 04:37 PM, Jan Beulich wrote: >>>> On 14.07.15 at 15:26, wrote: >> On 07/14/2015 03:22 PM, Jan Beulich wrote: >>>>>> On 13.07.15 at 19:14, wrote: >>>> --- a/xen/arch/x86/domain.c >>>> +++ b/xen/arch/x86/domain.c >>>> @@ -511,6 +511,8 @@ int vcpu_initialise(struct vcpu *v) >>>> >>>> void vcpu_destroy(struct vcpu *v) >>>> { >>>> + xfree(v->arch.vm_event.emul_read_data); >>> >>> Is this still needed now that you have >>> vm_event_cleanup_domain()? >> >> I had thought that there might be a code path where >> vm_event_cleanup_domain() doesn't get called and yet the domain is being >> destroyed, but I can't find anything obvious in the code except a >> comment in arch/x86/mm/shadow/common.c - shadow_final_teardown() - >> stating that "It is possible for a domain that never got domain_kill()ed >> to get here with its shadow allocation intact.". > > Tim? > >> Since common/domain.c's domain_kill() seems to be the only caller of >> vm_event_cleanup(), I took that comment to mean that it could be >> possible to end up in vcpu_destroy() without vm_event_cleanup_domain() >> having been called, so I took the better-safe-than-sorry approach. > > Better-safe-than-sorry would imply you'd also have to clear the > pointer in vcpu_destroy(), covering the (hypothetical?) case of > vm_event_cleanup_domain() being called afterwards. True. If that can happen I'll clear the pointer there too. >>>> + { >>>> + unsigned int safe_bytes = >>>> + min(bytes, curr->arch.vm_event.emul_read_data->size); >>>> + >>>> + if ( safe_bytes ) >>>> + memcpy(buffer, curr->arch.vm_event.emul_read_data->data, >>>> + safe_bytes); >>> >>> So why did you still keep this conditional? >> >> I thought a memcpy() call that ends up doing nothing (copying 0 bytes) >> would be expensive and I've tried to optimize the code by not doing the >> call if safe_bytes == 0. > > That argumentation would then also apply to the subsequent > memset(). > >> Since nobody else seems to think it's worth it, I'll remove it. > > Thanks. > >>>> @@ -1133,7 +1205,20 @@ static int hvmemul_rep_movs( >>>> */ >>>> rc = hvm_copy_from_guest_phys(buf, sgpa, bytes); >>>> if ( rc == HVMCOPY_okay ) >>>> + { >>>> + if ( unlikely(hvmemul_ctxt->set_context) ) >>>> + { >>>> + rc = set_context_data(buf, bytes); >>>> + >>>> + if ( rc != X86EMUL_OKAY) >>>> + { >>>> + xfree(buf); >>>> + return rc; >>>> + } >>>> + } >>>> + >>>> rc = hvm_copy_to_guest_phys(dgpa, buf, bytes); >>>> + } >>> >>> Why do you not bypass hvm_copy_from_guest_phys() here? This >>> way it would - afaict - become consistent with the other cases. >> >> You're right, it's unnecessary. Will remove the hvm_copy_from_guest_phys(). > > s/remove/bypass/ hopefully. Yes, sorry for the typo. Thanks, Razvan