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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

>
> 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.

~Andrew

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