From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Jan Beulich" Subject: Re: [PATCH V5 1/3] xen/mem_access: Support for memory-content hiding Date: Tue, 14 Jul 2015 14:37:10 +0100 Message-ID: <55A52CA60200007800090BBB@mail.emea.novell.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> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <55A50E23.5090503@bitdefender.com> Content-Disposition: inline List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Razvan Cojocaru , 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 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. >>> + { >>> + 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. Jan