Re: [PATCH v4 08/15] xen: arm: don't pretend to handle cache maintenance by set/way

[Ian Campbell <ian.campbell@citrix.com>]

On Fri, 2015-03-27 at 16:36 +0000, Julien Grall wrote:
> Hi Ian,
> 
> On 27/03/15 14:33, Ian Campbell wrote:
> > We set HCR_EL2.TSW but only (sort of) handle 32-bit access to DCCISW
> > but not the other two registers, nor any 64-bit access. Add handlers
> > for all of these.
> 
> We don't set HCR_EL2.TSW so DCCISW is not trapped.

I was completely sure we did, but I was wrong.

> > diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
> > index c2dcb66..cf3d6cc 100644
> > --- a/xen/include/public/arch-arm.h
> > +++ b/xen/include/public/arch-arm.h
> > @@ -161,6 +161,11 @@
> >   *
> >   * - The device tree Xen compatible node is fully described under Linux
> >   *   at Documentation/devicetree/bindings/arm/xen.txt.
> > + *
> > + * - Cache maintenaince operations by set/way ("dc isw|cisw|csw" and

I've just spotted a typo here, which I have fixed in my tree.

> > + *   the equivalent cp15 registers) are not available when running
> > + *   under Xen and will result in an undefined instruction exception
> > + *   delivered to the guest.
> >   */
> 
> set/way operations is used by Linux ARM32 in order to flush all the
> cache. Injecting an undefined instruction would make guest unusable.

Yes, that's a shame. AIUI operations by set/way aren't really very
useful under virt. See ARM v7 B1.14.4.

AIUI2 the reasons Linux does those flushes are due to bootloader's
dirtying of cachelines, which we take great care to avoid in our domain
builder, so I _think_ they probably don't need this under Xen, but have
no easy way to know that at the point they do them. Perhaps it would be
needed for a bootloader run under Xen to do this, I think that would
come under "things to fix when paravirtualaising a bootloader"

So I think we have a few options:

     1. Continue to not trap set/way operations. Guests will be able to
        flush the entire host cache by set/way. I don't think this is a
        good idea to keep allowing as a general principal.
     2. Trap set/way operations and do one of:
             1. Inject #undef
             2. Silently ignore
             3. Ignore with a debug level print of some sort
             4. Try to do some sort of useful operation.
                
I don't think #1 is a very good idea, and we've essentially ruled out
#2.1 (essentially this patch + enable the trap) here I think.

I've absolutely no idea what #2.4 might be.

So I think we are down to trap and ignore either with or without
logging.

Given the caveats with s/w under a hypervisor knowing about it would be
nice. The logging would be a few dozen (nr_sets*nr_ways) on each guest
boot, so would have to be a debug level log, but it wouldn't be terribly
spammy.

On the other hand, there is no way for a kernel to know it can not
bother with these, so those log messages will always be there and any
problematic uses won't be noticeable anyway.

So I am probably leaning towards #2.2

The KVM approach appears to be to flush the entire guest RAM space on
the first s/w operation and set HCR_EL2.TVM and then ignore all
subsequent s/w ops until caches are enabled, at which point they disable
HCR_EL2.TVM and go back to normal until the next s/w op. This catches
the actual expected use of s/w which is when enabling/disabling caches.

Something similar might work for us too actually. Maybe we could go with
#2.2 in the short term and plan to do the more complex thing later?

Ian.