From: Alexandru Stefan ISAILA <aisaila@bitdefender.com>
To: "Roger Pau Monné" <roger.pau@citrix.com>
Cc: "kevin.tian@intel.com" <kevin.tian@intel.com>,
"tamas@tklengyel.com" <tamas@tklengyel.com>,
"wei.liu2@citrix.com" <wei.liu2@citrix.com>,
"jun.nakajima@intel.com" <jun.nakajima@intel.com>,
"rcojocaru@bitdefender.com" <rcojocaru@bitdefender.com>,
"george.dunlap@eu.citrix.com" <george.dunlap@eu.citrix.com>,
"andrew.cooper3@citrix.com" <andrew.cooper3@citrix.com>,
"Mihai Donțu" <mdontu@bitdefender.com>,
"Andrei Vlad LUTAS" <vlutas@bitdefender.com>,
"jbeulich@suse.com" <jbeulich@suse.com>,
"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>,
"Anshul Makkar" <anshul.makkar@citrix.com>
Subject: Re: [PATCH v1] x86/hvm: Generic instruction re-execution mechanism for execute faults
Date: Mon, 19 Nov 2018 13:30:09 +0000 [thread overview]
Message-ID: <3d3ae38a-bfae-9908-dac6-163216f900e3@bitdefender.com> (raw)
In-Reply-To: <20181116170412.xrwuun3cqxll22l3@mac>
>> + /* Now transform our RWX values in a XENMEM_access_* constant. */
>> + if ( r == 0 && w == 0 && x == 0 )
>> + new_access = XENMEM_access_n;
>> + else if ( r == 0 && w == 0 && x == 1 )
>> + new_access = XENMEM_access_x;
>> + else if ( r == 0 && w == 1 && x == 0 )
>> + new_access = XENMEM_access_w;
>> + else if ( r == 0 && w == 1 && x == 1 )
>> + new_access = XENMEM_access_wx;
>> + else if ( r == 1 && w == 0 && x == 0 )
>> + new_access = XENMEM_access_r;
>> + else if ( r == 1 && w == 0 && x == 1 )
>> + new_access = XENMEM_access_rx;
>> + else if ( r == 1 && w == 1 && x == 0 )
>> + new_access = XENMEM_access_rw;
>> + else if ( r == 1 && w == 1 && x == 1 )
>> + new_access = XENMEM_access_rwx;
>> + else
>> + new_access = required_access; /* Should never get here. */
>
> There seems to be a lot of translation from xenmem_access_t to bool
> fields and then to xenmem_access_t again. Can't you just avoid the
> booleans?
The translation is done because the rights are cumulative and I think
this is the clear way to do this.
>> if ( vm_event_check_ring(d->vm_event_monitor) &&
>> d->arch.monitor.inguest_pagefault_disabled &&
>> - npfec.kind != npfec_kind_with_gla ) /* don't send a mem_event */
>> + npfec.kind != npfec_kind_with_gla &&
>> + hvm_funcs.start_reexecute_instruction ) /* don't send a mem_event */
>> {
>> - hvm_emulate_one_vm_event(EMUL_KIND_NORMAL, TRAP_invalid_op, X86_EVENT_NO_EC);
>> -
>> + v->arch.vm_event->emulate_flags = 0;
>> + hvm_funcs.start_reexecute_instruction(v, gpa, XENMEM_access_rw);
>> return true;
>> }
>
> Don't you need to fallback to using hvm_emulate_one_vm_event if
> start_reexecute_instruction is not available?
Fallback with hvm_emulate_one_vm_event can result in loosing events.
>> + for_each_vcpu(d, v)
>> + {
>> + if ( !v->arch.rexec_level )
>> + continue;
>> +
>> + for ( i = v->arch.rexec_level - 1; i >= 0; i-- )
>
> Is there any reason this has to be done backwards?
>
> If you do it from 0 to v->arch.rexec_level you could use an unsigned
> int as the index.
This is done backwards because of the corresponding code in
vmx_stop_reexecute_instruction() but here it can be turned the other way
if you insist on i to be unsigned.
>> +#define REEXECUTION_MAX_DEPTH 8
>> + struct rexec_context_t {
>> + unsigned long gpa;
>> + xenmem_access_t old_access;
>> + xenmem_access_t cur_access;
>> + bool_t old_single_step;
>
> bool please
>
>> + } rexec_context[REEXECUTION_MAX_DEPTH];
>
> This is fairly big amount of data that's only used if vm events are
> enabled, could this be allocated on a per-guest basis?
Yes, this can be moved to d->arch.monitor in the next version.
>
>> +
>> + int rexec_level;
>> +
>> + /*
>> + * Will be true when the vcpu is in VMX root,
>> + * false when it is not.
>> + */
>> + bool_t in_host;
>
> bool.
>
>> +
>> struct arch_vm_event *vm_event;
>>
>> struct vcpu_msrs *msrs;
>> diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
>> index 3d3250dff0..1f5d43a98d 100644
>> --- a/xen/include/asm-x86/hvm/hvm.h
>> +++ b/xen/include/asm-x86/hvm/hvm.h
>> @@ -167,6 +167,8 @@ struct hvm_function_table {
>>
>> int (*cpu_up)(void);
>> void (*cpu_down)(void);
>> + int (*start_reexecute_instruction)(struct vcpu *v, unsigned long gpa,
>> + xenmem_access_t required_access);
>
> I would name this reexecute_instruction, I don't think the start_
> prefix adds any value to the handler.
Sure, I will drop the start on the next version
Regards,
Alex
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
next prev parent reply other threads:[~2018-11-19 13:30 UTC|newest]
Thread overview: 52+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-11-16 10:06 [PATCH v1] x86/hvm: Generic instruction re-execution mechanism for execute faults Alexandru Stefan ISAILA
2018-11-16 17:04 ` Roger Pau Monné
2018-11-19 13:30 ` Alexandru Stefan ISAILA [this message]
2018-11-19 14:26 ` Jan Beulich
2018-11-19 15:08 ` Roger Pau Monné
2018-11-19 15:56 ` Alexandru Stefan ISAILA
2018-11-21 9:56 ` Roger Pau Monné
2018-11-21 10:28 ` Alexandru Stefan ISAILA
2018-11-21 11:41 ` Roger Pau Monné
2018-11-21 12:00 ` Alexandru Stefan ISAILA
2018-11-19 13:33 ` Jan Beulich
2018-11-21 18:55 ` Razvan Cojocaru
2018-11-22 9:50 ` Alexandru Stefan ISAILA
2018-11-22 10:00 ` Jan Beulich
2018-11-22 10:07 ` Roger Pau Monné
2018-11-22 10:05 ` Roger Pau Monné
2018-11-22 10:14 ` Razvan Cojocaru
2018-11-22 10:58 ` Roger Pau Monné
2018-11-22 12:48 ` Razvan Cojocaru
2018-11-22 14:49 ` Roger Pau Monné
2018-11-22 15:25 ` Razvan Cojocaru
2018-11-22 15:37 ` Roger Pau Monné
2018-11-22 16:52 ` Razvan Cojocaru
2018-11-22 17:08 ` Roger Pau Monné
2018-11-22 18:24 ` Razvan Cojocaru
2018-11-23 8:54 ` Roger Pau Monné
[not found] ` <59739FBC020000C234861ACF@prv1-mh.provo.novell.com>
[not found] ` <F553A58C020000AB0063616D@prv1-mh.provo.novell.com>
[not found] ` <4D445A680200003E34861ACF@prv1-mh.provo.novell.com>
[not found] ` <DAD49D5A020000780063616D@prv1-mh.provo.novell.com>
[not found] ` <5400A6CB0200003634861ACF@prv1-mh.provo.novell.com>
[not found] ` <203C1A92020000400063616D@prv1-mh.provo.novell.com>
[not found] ` <0DF3BC62020000E934861ACF@prv1-mh.provo.novell.com>
[not found] ` <C6A2E442020000640063616D@prv1-mh.provo.novell.com>
[not found] ` <6EEA58AB020000EA34861ACF@prv1-mh.provo.novell.com>
2018-11-27 10:31 ` Razvan Cojocaru
2018-11-27 11:32 ` Roger Pau Monné
2018-11-27 11:45 ` Razvan Cojocaru
2018-11-27 11:59 ` Andrew Cooper
2018-11-27 12:12 ` Razvan Cojocaru
2018-12-19 16:49 ` Alexandru Stefan ISAILA
2018-12-19 17:40 ` Roger Pau Monné
2018-12-20 14:37 ` Alexandru Stefan ISAILA
[not found] ` <838191050200006B34861ACF@prv1-mh.provo.novell.com>
2018-11-23 9:07 ` Jan Beulich
2018-11-27 10:49 ` Razvan Cojocaru
2018-11-27 11:28 ` Jan Beulich
2018-11-27 11:44 ` Razvan Cojocaru
2019-05-13 13:58 ` Razvan Cojocaru
2019-05-13 13:58 ` [Xen-devel] " Razvan Cojocaru
2019-05-13 14:06 ` Jan Beulich
2019-05-13 14:06 ` [Xen-devel] " Jan Beulich
2019-05-13 14:15 ` Razvan Cojocaru
2019-05-13 14:15 ` [Xen-devel] " Razvan Cojocaru
2019-05-14 13:47 ` Razvan Cojocaru
2019-05-14 13:47 ` [Xen-devel] " Razvan Cojocaru
2019-05-14 14:16 ` Jan Beulich
2019-05-14 14:16 ` [Xen-devel] " Jan Beulich
2019-05-14 14:20 ` Razvan Cojocaru
2019-05-14 14:20 ` [Xen-devel] " Razvan Cojocaru
[not found] ` <A31948D30200007D0063616D@prv1-mh.provo.novell.com>
2018-11-23 9:10 ` Jan Beulich
[not found] ` <9B05ED9E020000C434861ACF@prv1-mh.provo.novell.com>
[not found] ` <626A217B020000C50063616D@prv1-mh.provo.novell.com>
[not found] ` <0D3C56BA0200004834861ACF@prv1-mh.provo.novell.com>
2018-12-20 9:07 ` Jan Beulich
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=3d3ae38a-bfae-9908-dac6-163216f900e3@bitdefender.com \
--to=aisaila@bitdefender.com \
--cc=andrew.cooper3@citrix.com \
--cc=anshul.makkar@citrix.com \
--cc=george.dunlap@eu.citrix.com \
--cc=jbeulich@suse.com \
--cc=jun.nakajima@intel.com \
--cc=kevin.tian@intel.com \
--cc=mdontu@bitdefender.com \
--cc=rcojocaru@bitdefender.com \
--cc=roger.pau@citrix.com \
--cc=tamas@tklengyel.com \
--cc=vlutas@bitdefender.com \
--cc=wei.liu2@citrix.com \
--cc=xen-devel@lists.xenproject.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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).