On 2/5/19 9:33 AM, Jan Beulich wrote: >>>> On 05.02.19 at 09:29, <nmant...@amazon.de> wrote: >> --- a/xen/arch/x86/hvm/svm/svm.c >> +++ b/xen/arch/x86/hvm/svm/svm.c >> @@ -1794,7 +1794,7 @@ static void svm_do_nested_pgfault(struct vcpu *v, >> uint64_t gpa; >> uint64_t mfn; >> uint32_t qualification; >> - uint32_t p2mt; >> + p2m_type_t p2mt; >> } _d; > > Practically speaking this should work in all cases. But there's the > theoretical risk of p2m_type_t being a different width than > uint32_t. Trace records use fixed width types so that producer > and consumer can be in sync with respect to layout. Therefore > I think you want to go through an intermediate variable instead, > the more that there already is a suitable one. > > Mentioning the word "trace" or "xentrace" in the subject may > also help easily seeing what the issue is with. > > SVM maintainers / George: I find it odd that there are two calls > to __get_gfn_type_access() here. Doesn't this bear the risk of > the trace record not reflecting what has actually happened (i.e. > what has lead to the domain crash)? Perhaps the better fix here > is to remove the second, tracing specific call altogether?
I'm seeing one call related to xentrace, which happens first; and then a second one for the gdprintk. By "tracing specific call", do you mean the gdprintk one? It's worth pointing out that in the common case, *neither* call will happen, because 1) tracing is disabled, and 2) the vast majority of the time no SVM violations will happen. Duplicating the call is trading space for efficiency. Probably the best thing to do would be to move the __get_gfn_type_access() call up under the if() statement there -- i.e., it will only be called if the tracing one didn't happen. -George _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
