[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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.

It's off by default (by virtue of having to explicitly ask to use it in
the first place), and those who've asked for it don't want to be finding
-ENOMEM after the domain has been running for a few seconds (or midway
through the vcpus), when they inveterately want to map the rings.

Those who request buffers in the first place and forget about them are
not semantically different from those who ask for a silly shadow memory
limit, or typo the guest memory and give it too much.  Its a admin
error, not a safety/correctness issue.

~Andrew



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.