[v7,04/10] xen/memory: Add a vmtrace_buf resource type

Commit Message

Andrew Cooper Jan. 21, 2021, 9:27 p.m. UTC

From: Michał Leszczyński <[e-mail]>

Allow to map processor trace buffer using acquire_resource().

Signed-off-by: Michał Leszczyński <[e-mail]>
Signed-off-by: Andrew Cooper <[e-mail]>
---
CC: Jan Beulich <[e-mail]>
CC: Roger Pau Monné <[e-mail]>
CC: Wei Liu <[e-mail]>
CC: Michał Leszczyński <[e-mail]>
CC: Tamas K Lengyel <[e-mail]>

v7:
 * Rebase over changes elsewhere in the series
---
 xen/common/memory.c         | 27 +++++++++++++++++++++++++++
 xen/include/public/memory.h |  1 +
 2 files changed, 28 insertions(+)

Comments

Jan Beulich Jan. 25, 2021, 4:31 p.m. UTC | #1

On 21.01.2021 22:27, Andrew Cooper wrote:
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -1068,11 +1068,35 @@ static unsigned int resource_max_frames(const struct domain *d,
>      case XENMEM_resource_grant_table:
>          return gnttab_resource_max_frames(d, id);
>  
> +    case XENMEM_resource_vmtrace_buf:
> +        return d->vmtrace_frames;
> +
>      default:
>          return arch_resource_max_frames(d, type, id);
>      }
>  }
>  
> +static int acquire_vmtrace_buf(
> +    struct domain *d, unsigned int id, unsigned long frame,
> +    unsigned int nr_frames, xen_pfn_t mfn_list[])
> +{
> +    const struct vcpu *v = domain_vcpu(d, id);
> +    unsigned int i;
> +    mfn_t mfn;
> +
> +    if ( !v || !v->vmtrace.buf ||
> +         nr_frames > d->vmtrace_frames ||
> +         (frame + nr_frames) > d->vmtrace_frames )
> +        return -EINVAL;


I think that for this to guard against overflow, the first nr_frames
needs to be replaced by frame (as having the wider type), or else a
very large value of frame coming in will not yield the intended
-EINVAL. If you agree, with this changed,
Reviewed-by: Jan Beulich <[e-mail]>

Jan

Jan Beulich Jan. 26, 2021, 7:37 a.m. UTC | #2

On 25.01.2021 17:31, Jan Beulich wrote:
> On 21.01.2021 22:27, Andrew Cooper wrote:
>> --- a/xen/common/memory.c
>> +++ b/xen/common/memory.c
>> @@ -1068,11 +1068,35 @@ static unsigned int resource_max_frames(const struct domain *d,
>>      case XENMEM_resource_grant_table:
>>          return gnttab_resource_max_frames(d, id);
>>  
>> +    case XENMEM_resource_vmtrace_buf:
>> +        return d->vmtrace_frames;
>> +
>>      default:
>>          return arch_resource_max_frames(d, type, id);
>>      }
>>  }
>>  
>> +static int acquire_vmtrace_buf(
>> +    struct domain *d, unsigned int id, unsigned long frame,
>> +    unsigned int nr_frames, xen_pfn_t mfn_list[])
>> +{
>> +    const struct vcpu *v = domain_vcpu(d, id);
>> +    unsigned int i;
>> +    mfn_t mfn;
>> +
>> +    if ( !v || !v->vmtrace.buf ||
>> +         nr_frames > d->vmtrace_frames ||
>> +         (frame + nr_frames) > d->vmtrace_frames )
>> +        return -EINVAL;
> 
> 
> I think that for this to guard against overflow, the first nr_frames
> needs to be replaced by frame (as having the wider type), or else a
> very large value of frame coming in will not yield the intended
> -EINVAL.

Actually, besides this then wanting to be >= instead of >, this
wouldn't take care of the 32-bit case (or more generally the
sizeof(long) == sizeof(int) one). So I think you want

    if ( !v || !v->vmtrace.buf ||
         (frame + nr_frames) < frame ||
         (frame + nr_frames) > d->vmtrace_frames )
        return -EINVAL;

> If you agree, with this changed,
> Reviewed-by: Jan Beulich <[e-mail]>

This holds.

Jan

Andrew Cooper Jan. 26, 2021, 9:58 a.m. UTC | #3

On 26/01/2021 07:37, Jan Beulich wrote:
> On 25.01.2021 17:31, Jan Beulich wrote:
>> On 21.01.2021 22:27, Andrew Cooper wrote:
>>> --- a/xen/common/memory.c
>>> +++ b/xen/common/memory.c
>>> @@ -1068,11 +1068,35 @@ static unsigned int resource_max_frames(const struct domain *d,
>>>      case XENMEM_resource_grant_table:
>>>          return gnttab_resource_max_frames(d, id);
>>>  
>>> +    case XENMEM_resource_vmtrace_buf:
>>> +        return d->vmtrace_frames;
>>> +
>>>      default:
>>>          return arch_resource_max_frames(d, type, id);
>>>      }
>>>  }
>>>  
>>> +static int acquire_vmtrace_buf(
>>> +    struct domain *d, unsigned int id, unsigned long frame,
>>> +    unsigned int nr_frames, xen_pfn_t mfn_list[])
>>> +{
>>> +    const struct vcpu *v = domain_vcpu(d, id);
>>> +    unsigned int i;
>>> +    mfn_t mfn;
>>> +
>>> +    if ( !v || !v->vmtrace.buf ||
>>> +         nr_frames > d->vmtrace_frames ||
>>> +         (frame + nr_frames) > d->vmtrace_frames )
>>> +        return -EINVAL;
>>
>> I think that for this to guard against overflow, the first nr_frames
>> needs to be replaced by frame (as having the wider type), or else a
>> very large value of frame coming in will not yield the intended
>> -EINVAL.
> Actually, besides this then wanting to be >= instead of >, this
> wouldn't take care of the 32-bit case (or more generally the
> sizeof(long) == sizeof(int) one). So I think you want
>
>     if ( !v || !v->vmtrace.buf ||
>          (frame + nr_frames) < frame ||
>          (frame + nr_frames) > d->vmtrace_frames )
>         return -EINVAL;
>
>> If you agree, with this changed,
>> Reviewed-by: Jan Beulich <[e-mail]>
> This holds.

I slipped this buggy version in to prove a point.

You're now 3 or 4 attempts into "simplifying" my original version, and
have on at least 2 attempts made your R-b conditional on a buggy version.

This form is clearly too complicated to reason about correctly, and it
is definitely more complicated than I am happy taking.


I am either going to go with my original version, which is trivially and
obviously correct, or I'm considering reducing frame to 32 bits at the
top level to fix this width nonsense throughout Xen.

~Andrew

Jan Beulich Jan. 26, 2021, 10:30 a.m. UTC | #4

On 26.01.2021 10:58, Andrew Cooper wrote:
> On 26/01/2021 07:37, Jan Beulich wrote:
>> On 25.01.2021 17:31, Jan Beulich wrote:
>>> On 21.01.2021 22:27, Andrew Cooper wrote:
>>>> +static int acquire_vmtrace_buf(
>>>> +    struct domain *d, unsigned int id, unsigned long frame,
>>>> +    unsigned int nr_frames, xen_pfn_t mfn_list[])
>>>> +{
>>>> +    const struct vcpu *v = domain_vcpu(d, id);
>>>> +    unsigned int i;
>>>> +    mfn_t mfn;
>>>> +
>>>> +    if ( !v || !v->vmtrace.buf ||
>>>> +         nr_frames > d->vmtrace_frames ||
>>>> +         (frame + nr_frames) > d->vmtrace_frames )
>>>> +        return -EINVAL;
>>>
>>> I think that for this to guard against overflow, the first nr_frames
>>> needs to be replaced by frame (as having the wider type), or else a
>>> very large value of frame coming in will not yield the intended
>>> -EINVAL.
>> Actually, besides this then wanting to be >= instead of >, this
>> wouldn't take care of the 32-bit case (or more generally the
>> sizeof(long) == sizeof(int) one). So I think you want
>>
>>     if ( !v || !v->vmtrace.buf ||
>>          (frame + nr_frames) < frame ||
>>          (frame + nr_frames) > d->vmtrace_frames )
>>         return -EINVAL;
>>
>>> If you agree, with this changed,
>>> Reviewed-by: Jan Beulich <[e-mail]>
>> This holds.
> 
> I slipped this buggy version in to prove a point.

IOW you've been intentionally submitting buggy code. Very
interesting.

> You're now 3 or 4 attempts into "simplifying" my original version, and
> have on at least 2 attempts made your R-b conditional on a buggy version.

In which way is the last proposed version buggy, and in which
way was the intermediate proposal problematic beyond the
aspects I did recognize myself? (I also see no problem with
taking a number of iterations to arrive at the correct result,
and I also wouldn't view this happening as an indication that
an initial comment was wrong then, unless the final result of
this iterative process matches what there was originally.)

> This form is clearly too complicated to reason about correctly, and it
> is definitely more complicated than I am happy taking.
> 
> 
> I am either going to go with my original version, which is trivially and
> obviously correct,

I've just tried to locate your "original version" in my mailbox.
I don't have an earlier patch there with this same title.
Without being able to locate the prior suggestion of mine, I'm
afraid I won't be able to verify if indeed I did suggest the
variant above before; I wouldn't consider it very likely though.
In any event I think it would have helped more if you had
proven to me where I'm wrong; I can be convinced, but calling
something "trivially and obviously correct" is not a technical
statement in such a situation. It instead feels more like a
killer phrase.

By implication, you saying "trivially and obviously correct"
can really mean only one of two things if indeed I had found a
need to comment on this same piece of code (under a different
title) earlier on: I'm trivially and obviously stupid (and
would better go away), or you're wrong with the statement (at
least in assuming what's trivial and obvious to you also
necessarily is to everyone else). I'm sorry to say it this
bluntly, but your reply above feels pretty blunt as well.

> or I'm considering reducing frame to 32 bits at the
> top level to fix this width nonsense throughout Xen.

I wouldn't mind this (and I've been wondering about the
"unsigned long" a number of times), but I'm afraid I don't see
how your construct above would be correctly rejecting all
overflowing cases then.

Jan

Patch

diff --git a/xen/common/memory.c b/xen/common/memory.c
index c89923d909..ec6a55172a 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -1068,11 +1068,35 @@  static unsigned int resource_max_frames(const struct domain *d,
     case XENMEM_resource_grant_table:
         return gnttab_resource_max_frames(d, id);
 
+    case XENMEM_resource_vmtrace_buf:
+        return d->vmtrace_frames;
+
     default:
         return arch_resource_max_frames(d, type, id);
     }
 }
 
+static int acquire_vmtrace_buf(
+    struct domain *d, unsigned int id, unsigned long frame,
+    unsigned int nr_frames, xen_pfn_t mfn_list[])
+{
+    const struct vcpu *v = domain_vcpu(d, id);
+    unsigned int i;
+    mfn_t mfn;
+
+    if ( !v || !v->vmtrace.buf ||
+         nr_frames > d->vmtrace_frames ||
+         (frame + nr_frames) > d->vmtrace_frames )
+        return -EINVAL;
+
+    mfn = page_to_mfn(v->vmtrace.buf);
+
+    for ( i = 0; i < nr_frames; i++ )
+        mfn_list[i] = mfn_x(mfn) + frame + i;
+
+    return nr_frames;
+}
+
 /*
  * Returns -errno on error, or positive in the range [1, nr_frames] on
  * success.  Returning less than nr_frames contitutes a request for a
@@ -1087,6 +1111,9 @@  static int _acquire_resource(
     case XENMEM_resource_grant_table:
         return gnttab_acquire_resource(d, id, frame, nr_frames, mfn_list);
 
+    case XENMEM_resource_vmtrace_buf:
+        return acquire_vmtrace_buf(d, id, frame, nr_frames, mfn_list);
+
     default:
         return arch_acquire_resource(d, type, id, frame, nr_frames, mfn_list);
     }
diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
index c4c47a0b38..b0378bb14b 100644
--- a/xen/include/public/memory.h
+++ b/xen/include/public/memory.h
@@ -625,6 +625,7 @@  struct xen_mem_acquire_resource {
 
 #define XENMEM_resource_ioreq_server 0
 #define XENMEM_resource_grant_table 1
+#define XENMEM_resource_vmtrace_buf 2
 
     /*
      * IN - a type-specific resource identifier, which must be zero