Re: [PATCH 0/3] xen/arm: Inject an exception to the guest rather than crashing it

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 30/01/18 19:21, Stefano Stabellini wrote:
> On Tue, 30 Jan 2018, Julien Grall wrote:
>> Hi,
>>
>> On 30/01/18 18:29, Andrew Cooper wrote:
>>> On 30/01/18 17:00, Julien Grall wrote:
>>>>
>>>> On 30/01/18 16:38, Andrew Cooper wrote:
>>>>> On 30/01/18 16:14, Julien Grall wrote:
>>>>>> Hi all,
>>>>>>
>>>>>> This small series replaces all call to domain_crash_synchronous by
>>>>>> injecting
>>>>>> an exception to the guest.
>>>>>>
>>>>>> This will result to a nicer trace from the guest (no need to
>>>>>> manually walk
>>>>>> the stack) and give a chance to the guest to give a bit more
>>>>>> information on
>>>>>> what it was doing.
>>>>>>
>>>>>> Cheers,
>>>>>>
>>>>>> Julien Grall (3):
>>>>>>     xen/arm: io: Distinguish unhandled IO from aborted one
>>>>>>     xen/arm: Don't crash domain on bad MMIO emulation
>>>>>>     xen/arm: Don't crash the domain on invalid HVC immediate
>>>>> Thanks.
>>>>>
>>>>> I don't feel qualified to review these, but some notes.
>>>>>
>>>>> Patch 1.  s/avodi/avoid/ in the commit message
>>>>>
>>>>> Patches 2 and 3.  You probably want to convert the printks to
>>>>> gdprintk()s, otherwise guests can choke up the ratelimited log.  Doing
>>>>> so will also mean that the vcpu will be identified consistently, which
>>>>> it isn't currently.
>>>> We didn't use g*printk because it would be more confusing to print the
>>>> current vCPU in some cases (e.g when accessing the re-distributor of
>>>> another vCPU) or does not matter (e.g for ITS).
>>> In the former case, you'd want to print both current, and the target
>>> vcpu.  The latter still matters what current is if something goes wrong.
>>>
>>> We have plenty of similar cases in x86, but at the point you are
>>> printing an diagnostic message, ignoring current is almost always the
>>> wrong think to do.
>> I will look at it on another series.
>>
>>>> The problem with the debug version is those information are actually
>>>> quite useful in non-debug build. We found quite a few issues thanks to
>>>> them.
>>>>
>>>> I think it would make more sense for Xen to provide per-guest
>>>> ratelimited than hiding those messages in non-debug build.
>>> Per guest is quite a lot more complicated than global, and would still
>>> require a global limit to prevent a concerted attack from multiple
>>> guests to avoid DoSing the system.
>>>
>>> Debug vs unilateral is your prerogative as a maintainer, but as you've
>>> said yourself, the are used for debugging purposes, which proves my point.
>> So on x86, you always request the user to reproduce it with debug build
>> enable?
>>
>> Stefano, what's your opinion here?
> I think it would be great to have per-guest rate limiting.
>
> On ARM, it would be impossible to ask users to repro with debug enabled,
> given that many deployments are embedded (set-top boxes, etc).
>
> I think it is OK to keep the XENLOG_DEBUG, they should be filtered out
> by loglvl. But we need to be careful about the others. We might want to
> convert the XENLOG_WARNING in traps.c to XENLOG_DEBUG: they are warning
> about guests misbehaving, but from the hypervisor point of view, it's not
> a problem, guests can shoot themselves if they want to; it's OK.

This is a valid argument, but to do it consistently, you'll want to
provide an arch-specific version of gdprintk() so that the common code
printk()s don't evaporate into /dev/null.

~Andrew

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