Re: [PATCH v2 4/7] x86/vmx: add do_vmtrace_op

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.06.2020 19:24, Andrew Cooper wrote:
> On 23/06/2020 09:51, Jan Beulich wrote:
>> On 23.06.2020 03:04, Michał Leszczyński wrote:
>>> ----- 22 cze 2020 o 18:16, Jan Beulich jbeulich@xxxxxxxx napisał(a):
>>>
>>>> On 22.06.2020 18:02, Michał Leszczyński wrote:
>>>>> ----- 22 cze 2020 o 17:22, Jan Beulich jbeulich@xxxxxxxx napisał(a):
>>>>>> On 22.06.2020 16:35, Michał Leszczyński wrote:
>>>>>>> ----- 22 cze 2020 o 15:25, Jan Beulich jbeulich@xxxxxxxx napisał(a):
>>>>>>>> Is any of what you do in this switch() actually legitimate without
>>>>>>>> hvm_set_vmtrace_pt_size() having got called for the guest? From
>>>>>>>> remarks elsewhere I imply you expect the param that you currently
>>>>>>>> use to be set upon domain creation time, but at the very least the
>>>>>>>> potentially big buffer should imo not get allocated up front, but
>>>>>>>> only when tracing is to actually be enabled.
>>>>>>> Wait... so you want to allocate these buffers in runtime?
>>>>>>> Previously we were talking that there is too much runtime logic
>>>>>>> and these enable/disable hypercalls should be stripped to absolute
>>>>>>> minimum.
>>>>>> Basic arrangements can be made at domain creation time. I don't
>>>>>> think though that it would be a good use of memory if you
>>>>>> allocated perhaps many gigabytes of memory just for possibly
>>>>>> wanting to enable tracing on a guest.
>>>>>>
>>>>> From our previous conversations I thought that you want to have
>>>>> as much logic moved to the domain creation as possible.
>>>>>
>>>>> Thus, a parameter "vmtrace_pt_size" was introduced. By default it's
>>>>> zero (= disabled), if you set it to a non-zero value, then trace buffers
>>>>> of given size will be allocated for the domain and you have possibility
>>>>> to use ipt_enable/ipt_disable at any moment.
>>>>>
>>>>> This way the runtime logic is as thin as possible. I assume user knows
>>>>> in advance whether he/she would want to use external monitoring with IPT
>>>>> or not.
>>>> Andrew - I think you requested movement to domain_create(). Could
>>>> you clarify if indeed you mean to also allocate the big buffers
>>>> this early?
>>> I would like to recall what Andrew wrote few days ago:
>>>
>>> ----- 16 cze 2020 o 22:16, Andrew Cooper andrew.cooper3@xxxxxxxxxx wrote:
>>>> Xen has traditionally opted for a "and turn this extra thing on
>>>> dynamically" model, but this has caused no end of security issues and
>>>> broken corner cases.
>>>>
>>>> You can see this still existing in the difference between
>>>> XEN_DOMCTL_createdomain and XEN_DOMCTL_max_vcpus, (the latter being
>>>> required to chose the number of vcpus for the domain) and we're making
>>>> good progress undoing this particular wart (before 4.13, it was
>>>> concerning easy to get Xen to fall over a NULL d->vcpu[] pointer by
>>>> issuing other hypercalls between these two).
>>>>
>>>> There is a lot of settings which should be immutable for the lifetime of
>>>> the domain, and external monitoring looks like another one of these.
>>>> Specifying it at createdomain time allows for far better runtime
>>>> behaviour (you are no longer in a situation where the first time you try
>>>> to turn tracing on, you end up with -ENOMEM because another VM booted in
>>>> the meantime and used the remaining memory), and it makes for rather
>>>> more simple code in Xen itself (at runtime, you can rely on it having
>>>> been set up properly, because a failure setting up will have killed the
>>>> domain already).
>>>>
>>>> ...
>>>>
>>>> ~Andrew
>>> according to this quote I've moved buffer allocation to the create_domain,
>>> the updated version was already sent to the list as patch v3.
>> I'd still like to see an explicit confirmation by him that this
>> use of memory is indeed what he has intended. There are much smaller
>> amounts of memory which we allocate on demand, just to avoid
>> allocating some without then ever using it.
> 
> PT is a debug/diagnostic tool.  Its not something you'd run in
> production against a production VM.

Well, the suggested use with introspection made me assume differently.
If this is (now and forever) truly debug/diag only, so be it then.

Jan