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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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.

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