From: "Jan Beulich" <JBeulich@suse.com>
To: Razvan Cojocaru <rcojocaru@bitdefender.com>, Tim Deegan <tim@xen.org>
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
Subject: Re: [PATCH V5 1/3] xen/mem_access: Support for memory-content hiding
Date: Tue, 14 Jul 2015 14:37:10 +0100 [thread overview]
Message-ID: <55A52CA60200007800090BBB@mail.emea.novell.com> (raw)
In-Reply-To: <55A50E23.5090503@bitdefender.com>
>>> On 14.07.15 at 15:26, <rcojocaru@bitdefender.com> wrote:
> On 07/14/2015 03:22 PM, Jan Beulich wrote:
>>>>> On 13.07.15 at 19:14, <rcojocaru@bitdefender.com> 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
next prev parent reply other threads:[~2015-07-14 13:37 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-07-13 17:14 [PATCH V5 0/3] Vm_event memory introspection helpers Razvan Cojocaru
2015-07-13 17:14 ` [PATCH V5 1/3] xen/mem_access: Support for memory-content hiding Razvan Cojocaru
2015-07-13 17:32 ` Lengyel, Tamas
2015-07-13 17:36 ` Razvan Cojocaru
2015-07-14 12:22 ` Jan Beulich
2015-07-14 13:26 ` Razvan Cojocaru
2015-07-14 13:37 ` Jan Beulich [this message]
2015-07-14 13:41 ` Razvan Cojocaru
2015-07-13 17:14 ` [PATCH V5 2/3] xen/vm_event: Support for guest-requested events Razvan Cojocaru
2015-07-13 17:14 ` [PATCH V5 3/3] xen/vm_event: Deny register writes if refused by vm_event reply Razvan Cojocaru
2015-07-14 12:35 ` Jan Beulich
2015-07-14 13:45 ` Razvan Cojocaru
2015-07-14 14:41 ` Jan Beulich
2015-07-14 15:04 ` Razvan Cojocaru
2015-07-14 15:55 ` Jan Beulich
2015-07-14 16:25 ` Razvan Cojocaru
2015-07-14 14:37 ` Razvan Cojocaru
2015-07-14 10:50 ` [PATCH V5 0/3] Vm_event memory introspection helpers Jan Beulich
2015-07-14 11:45 ` Razvan Cojocaru
2015-07-14 11:53 ` Jan Beulich
2015-07-14 13:08 ` Ian Campbell
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=55A52CA60200007800090BBB@mail.emea.novell.com \
--to=jbeulich@suse.com \
--cc=Aravind.Gopalakrishnan@amd.com \
--cc=andrew.cooper3@citrix.com \
--cc=boris.ostrovsky@oracle.com \
--cc=eddie.dong@intel.com \
--cc=george.dunlap@eu.citrix.com \
--cc=ian.campbell@citrix.com \
--cc=ian.jackson@eu.citrix.com \
--cc=jun.nakajima@intel.com \
--cc=keir@xen.org \
--cc=kevin.tian@intel.com \
--cc=rcojocaru@bitdefender.com \
--cc=stefano.stabellini@eu.citrix.com \
--cc=suravee.suthikulpanit@amd.com \
--cc=tim@xen.org \
--cc=tlengyel@novetta.com \
--cc=wei.liu2@citrix.com \
--cc=xen-devel@lists.xen.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.