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

[Julien Grall <julien.grall@xxxxxxx>]

Hi Stefano,

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.
Not really. Some of the XENLOG_WARNING may happen because Xen does not 
handle a trap (see the one on do_trap_guest_sync). That has nothing to 
do with a guest misbehaving and could happen if Xen boots on newer hardware.
Cheers,

--
Julien Grall

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