From: Razvan Cojocaru <rcojocaru@bitdefender.com>
To: Jan Beulich <JBeulich@suse.com>
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 16:26:59 +0300 [thread overview]
Message-ID: <55A50E23.5090503@bitdefender.com> (raw)
In-Reply-To: <55A51B140200007800090AE1@mail.emea.novell.com>
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.".
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.
That is also why I'm setting v->arch.vm_event.emul_read_data to NULL in
the vm_event domain cleanup function, so that this xfree() does not
double-free.
But this xfree() is probably not needed, so if confirmed I'll remove it.
>> --- a/xen/arch/x86/hvm/emulate.c
>> +++ b/xen/arch/x86/hvm/emulate.c
>> @@ -67,6 +67,27 @@ static int null_write(const struct hvm_io_handler *handler,
>> return X86EMUL_OKAY;
>> }
>>
>> +static int set_context_data(void *buffer, unsigned int bytes)
>
> I think in the context of this function alone, "size" would be better
> than "bytes".
Ack.
>> +{
>> + struct vcpu *curr = current;
>> +
>> + if ( !curr->arch.vm_event.emul_read_data )
>> + return X86EMUL_UNHANDLEABLE;
>> + else
>
> Else after the respective if() unconditionally returning is odd.
> Perhaps better to invert the if() condition...
I agree, I only used the else so that I wouldn't need to put safe_bytes
on the stack if I didn't have to. I'll invert the condition.
>> + {
>> + 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.
Since nobody else seems to think it's worth it, I'll remove it.
>> @@ -1005,6 +1043,36 @@ static int hvmemul_rep_ins(
>> !!(ctxt->regs->eflags & X86_EFLAGS_DF), gpa);
>> }
>>
>> +static int hvmemul_rep_outs_set_context(
>> + enum x86_segment src_seg,
>> + unsigned long src_offset,
>> + uint16_t dst_port,
>> + unsigned int bytes_per_rep,
>> + unsigned long *reps,
>> + struct x86_emulate_ctxt *ctxt)
>> +{
>> + unsigned int bytes = *reps * bytes_per_rep;
>> + char *buf;
>> + int rc;
>> +
>> + buf = xmalloc_array(char, bytes);
>> +
>> + if ( buf == NULL )
>> + return X86EMUL_UNHANDLEABLE;
>> +
>> + rc = set_context_data(buf, bytes);
>> +
>> + if ( rc != X86EMUL_OKAY )
>> + goto out;
>> +
>> + rc = hvmemul_do_pio_buffer(dst_port, bytes, IOREQ_WRITE, buf);
>> +
>> +out:
>
> Labels should be indented by at least one space. But realistically
> the code wouldn't look worse without any goto...
Understood, will remove the label completely.
>> @@ -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().
>> --- a/xen/arch/x86/vm_event.c
>> +++ b/xen/arch/x86/vm_event.c
>> @@ -23,6 +23,36 @@
>> #include <xen/sched.h>
>> #include <asm/hvm/hvm.h>
>>
>> +int vm_event_init_domain(struct domain *d)
>> +{
>> + struct vcpu *v;
>> +
>> + for_each_vcpu( d, v )
>
> Either you consider for_each_vcpu a keyword-like thing, then this
> is missing a blank before the opening parenthesis, or you don't, in
> which case the blanks immediately inside the parentheses should
> be dropped.
Understood, will fix it.
>> + {
>> + if ( v->arch.vm_event.emul_read_data )
>> + continue;
>> +
>> + v->arch.vm_event.emul_read_data =
>> + xzalloc(struct vm_event_emul_read_data);
>> +
>> + if ( !v->arch.vm_event.emul_read_data )
>> + return -ENOMEM;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +void vm_event_cleanup_domain(struct domain *d)
>> +{
>> + struct vcpu *v;
>> +
>> + for_each_vcpu( d, v )
>> + {
>> + xfree(v->arch.vm_event.emul_read_data);
>> + v->arch.vm_event.emul_read_data = NULL;
>> + }
>> +}
>
> There not being a race here is attributed to vm_event_enable()
> being implicitly serialized by the domctl lock, and
> vm_event_disable(), beyond its domctl use, only being on domain
> cleanup paths? Would be nice to have a comment to that effect.
Indeed, that's how I understood it to happen. I'll add a comment.
Thanks,
Razvan
next prev parent reply other threads:[~2015-07-14 13:26 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 [this message]
2015-07-14 13:37 ` Jan Beulich
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=55A50E23.5090503@bitdefender.com \
--to=rcojocaru@bitdefender.com \
--cc=Aravind.Gopalakrishnan@amd.com \
--cc=JBeulich@suse.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=stefano.stabellini@eu.citrix.com \
--cc=suravee.suthikulpanit@amd.com \
--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.