[PATCH svm] svm: fix p2mt type

[PATCH svm] svm: fix p2mt type

[Norbert Manthey]

A pointer mismatch has been reported when compiling with the
compiler goto-gcc of the bounded model checker CBMC.

Fixes: 9a779e4f (Implement SVM specific part for Nested Virtualization)

Signed-off-by: Norbert Manthey <nmanthey@amazon.de>

---
 xen/arch/x86/hvm/svm/svm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
--- 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;
 
         p2m = p2m_get_p2m(v);
-- 
2.7.4




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrer: Christian Schlaeger, Ralf Herbrich
Ust-ID: DE 289 237 879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH svm] svm: fix p2mt type

[Andrew Cooper]

On 05/02/2019 08:29, Norbert Manthey wrote:
> A pointer mismatch has been reported when compiling with the
> compiler goto-gcc of the bounded model checker CBMC.
>
> Fixes: 9a779e4f (Implement SVM specific part for Nested Virtualization)
>
> Signed-off-by: Norbert Manthey <nmanthey@amazon.de>
>
> ---
>  xen/arch/x86/hvm/svm/svm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> --- 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;

You can't change this type, for the same reason that the compiler
complained.  enum has an implementation defined width which may not be
4, and this would break the trace record.

The best course of action is to pass the existing &p2mt into the lookup,
and assign back to _d.p2mt alongside the mfn.  A decent optimising
compiler will be able to simplify this when enum* and uint32_t* are
considered to be compatible.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH svm] svm: fix p2mt type

[Jan Beulich]

>>> On 05.02.19 at 09:29, <nmanthey@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?

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH svm v2] svm: fix xentrace p2mt access

[Norbert Manthey]

A pointer mismatch has been reported when compiling with the
compiler goto-gcc of the bounded model checker CBMC.
To keep trace entry size independent of the compiler implementation,
use the available p2mt variable for the access, and update the
trace record independenly.

Fixes: 9a779e4f (Implement SVM specific part for Nested Virtualization)

Signed-off-by: Norbert Manthey <nmanthey@amazon.de>

---

Notes:
    v2: keep type, use local variable in function call and

 xen/arch/x86/hvm/svm/svm.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1800,8 +1800,9 @@ static void svm_do_nested_pgfault(struct vcpu *v,
         p2m = p2m_get_p2m(v);
         _d.gpa = gpa;
         _d.qualification = 0;
-        mfn = __get_gfn_type_access(p2m, gfn, &_d.p2mt, &p2ma, 0, NULL, 0);
+        mfn = __get_gfn_type_access(p2m, gfn, &p2mt, &p2ma, 0, NULL, 0);
         _d.mfn = mfn_x(mfn);
+        _d.p2mt = p2mt;
         
         __trace_var(TRC_HVM_NPF, 0, sizeof(_d), &_d);
     }
-- 
2.7.4




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrer: Christian Schlaeger, Ralf Herbrich
Ust-ID: DE 289 237 879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH svm] svm: fix p2mt type

[Boris Ostrovsky]

On 2/5/19 4:33 AM, Jan Beulich wrote:
>
> 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?

Yes, I agree.

-boris

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH svm v2] svm: fix xentrace p2mt access

[Boris Ostrovsky]

On 2/5/19 10:09 AM, Norbert Manthey wrote:
> A pointer mismatch has been reported when compiling with the
> compiler goto-gcc of the bounded model checker CBMC.
> To keep trace entry size independent of the compiler implementation,
> use the available p2mt variable for the access, and update the
> trace record independenly.
>
> Fixes: 9a779e4f (Implement SVM specific part for Nested Virtualization)
>
> Signed-off-by: Norbert Manthey <nmanthey@amazon.de>

Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>

>
> ---
>
> Notes:
>     v2: keep type, use local variable in function call and
>
>  xen/arch/x86/hvm/svm/svm.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -1800,8 +1800,9 @@ static void svm_do_nested_pgfault(struct vcpu *v,
>          p2m = p2m_get_p2m(v);
>          _d.gpa = gpa;
>          _d.qualification = 0;
> -        mfn = __get_gfn_type_access(p2m, gfn, &_d.p2mt, &p2ma, 0, NULL, 0);
> +        mfn = __get_gfn_type_access(p2m, gfn, &p2mt, &p2ma, 0, NULL, 0);
>          _d.mfn = mfn_x(mfn);
> +        _d.p2mt = p2mt;
>          
>          __trace_var(TRC_HVM_NPF, 0, sizeof(_d), &_d);
>      }


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH svm] svm: fix p2mt type

[George Dunlap]

On 2/5/19 9:33 AM, Jan Beulich wrote:
>>>> On 05.02.19 at 09:29, <nmanthey@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