[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH v3 2/8] x86/vm-event/monitor: relocate code-motion more appropriately
On 7/8/2016 1:37 PM, Jan Beulich wrote:
On 08.07.16 at 12:22, <czuzu@xxxxxxxxxxxxxxx> wrote:
On 7/8/2016 10:21 AM, Jan Beulich wrote:
On 06.07.16 at 17:50, <czuzu@xxxxxxxxxxxxxxx> wrote:
The title of this patch keeps confusing me - which code motion is
being relocated here?
As the commit message clearly states, the code motions that are being
relocated are:
Again this sentence makes no sense to me: I can't see how
"code motions" can be "relocated", just like I don't see how you
could move a move. But maybe it's just me...
Hah, sorry, I'm not very good expressivity-wise, a weakness I'm aware of
and which makes me pick up expressions I notice other people use (in
this case those of maintainers).
I think you were the one I noticed using the expression back in an older
patch-series I've sent and I thought by "code-motion" you meant simply
"that which some code does, tries to accomplish and the code itself".
1) handling of monitor_write_data @ hvm_do_resume
2) the code in vmx_update_guest_cr (when cr = 0) that deals with setting
CR3 load-exiting for cr-write monitor vm-events, i.e. the comment:
/* Trap CR3 updates if CR3 memory events are enabled. */
and what's removed from under it.
By 'relocation' I meant making that code vm-event specific (moving it to
vm-event specific files).
Yes, that what I've guessed.
+{
+ struct vcpu *v;
+ struct arch_vmx_struct *avmx;
+ unsigned int cr3_bitmask;
+ bool_t cr3_vmevent, cr3_ldexit;
+
+ /* domain must be paused */
+ ASSERT(atomic_read(&d->pause_count));
Comment style.
As in change to "/* Domain must be paused. */"?
Yes, as mandated by ./CODING_STYLE.
+ /* non-hap domains trap CR3 writes unconditionally */
+ if ( !paging_mode_hap(d) )
+ {
+#ifndef NDEBUG
+ for_each_vcpu ( d, v )
+ ASSERT(v->arch.hvm_vmx.exec_control & CPU_BASED_CR3_LOAD_EXITING);
+#endif
+ return;
+ }
+
+ cr3_bitmask = monitor_ctrlreg_bitmask(VM_EVENT_X86_CR3);
+ cr3_vmevent = !!(d->arch.monitor.write_ctrlreg_enabled & cr3_bitmask);
+
+ for_each_vcpu ( d, v )
+ {
+ avmx = &v->arch.hvm_vmx;
+ cr3_ldexit = !!(avmx->exec_control & CPU_BASED_CR3_LOAD_EXITING);
+
+ if ( cr3_vmevent == cr3_ldexit )
+ continue;
+
+ /*
+ * If CR0.PE=0, CR3 load exiting must remain enabled.
+ * See vmx_update_guest_cr code motion for cr = 0.
+ */
Same as for the title - what code motion is this referring to? In a
code comment you clearly shouldn't be referring to anything the
patch effects, only to its result.
The "vmx_update_guest_cr code motion for cr = 0", that's what's
referring to.
So I guess my problem really is that I don't understand what a
"code motion" is (other than the act of moving code from one
place to another).
Again, sorry, will try to rephrase all of this properly :-).
'vmx_update_guest_cr()' is a function, 'cr' is one of its parameters.
In other words, see what's happening in the function
'vmx_update_guest_cr() when you pass it cr = 0' and you'll understand
why CR3 load-exiting must remain enabled when CR0.PE=0.
+ if ( cr3_ldexit && !hvm_paging_enabled(v) &&
!vmx_unrestricted_guest(v) )
+ continue;
The first sentence of the comment should be brought in line with
this condition.
Would this do (aligned with the above observation):
"
/*
* If CR3 load-exiting was enabled and CR0.PE=0, then it must remain
* enabled (see vmx_update_guest_cr(v, cr) function when cr = 0).
*/
"
?
Not really: The condition checks whether paging is enabled and
whether it's an unrestricted guest. The comment talks about
protected mode being enabled.
Hah you're right, I only now notice, that comment has actually been
adopted (although I don't remember from where, I wonder if it was
meantime removed and I only now see), I always thought it said "CR0.PG =
0"...
So...
"
/*
* If domain paging is disabled (CR0.PG=0) and
* the domain is not in real-mode, then CR3 load-exiting
* must remain enabled (see vmx_update_guest_cr(v, cr) when cr = 0).
*/
"
?
+static inline void write_ctrlreg_adjust_traps(struct domain *d, uint8_t
index)
Unless there is a particular reason for this uint8_t, please convert to
unsigned int.
The particular reason is cr-indexes being uint8_t typed (see
typeof(xen_domctl_monitor_op.mov_to_cr.index)).
But I will change it to unsigned int if you prefer (maybe you could
explain the preference though).
No use of fixed width types when fixed width types aren't really
required. Generally generated code is less efficient when having
to deal with fixed width types.
Strange, I would have thought the compiler would properly (and easily)
deal with such efficiency issues.
+{
+ /* vmx only */
+ ASSERT(cpu_has_vmx);
Comment style (more below). Should perhaps also get "for now" or
some such added.
As in "/* For now, VMX only. */"?
For example, yes.
+static inline void write_ctrlreg_disable_traps(struct domain *d)
+{
+ unsigned int old = d->arch.monitor.write_ctrlreg_enabled;
+ d->arch.monitor.write_ctrlreg_enabled = 0;
+
+ if ( old )
+ {
+ /* vmx only */
+ ASSERT(cpu_has_vmx);
Wouldn't this better move ahead of the if()?
+ /* was CR3 load-exiting enabled due to monitoring? */
+ if ( old & monitor_ctrlreg_bitmask(VM_EVENT_X86_CR3) )
And then this if() alone would suffice.
No, it would be wrong because that ASSERT may not hold if "old == 0",
i.e. we only ASSERT the implication "CR-write vm-events can be enabled
-> vmx domain", but since the function is called by
arch_monitor_cleanup_domain, putting the ASSERT before the if() would
change that implication to "(any) monitor vm-events available -> vmx
domain", assertion which wouldn't be proper TBD here.
Ah, okay - I was under the impression that no VM events were
allowed under SVM.
Jan
Thanks,
Corneliu.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
