Re: [Xen-devel] [PATCH v2] tools: libxc: flush data cache after loading images into guest memory

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Thu, 2013-12-12 at 17:52 +0000, Ian Jackson wrote:
> Ian Campbell writes ("Re: [PATCH v2] tools: libxc: flush data cache after 
> loading images into guest memory"):
> > Since this is a tool patch more than and ARM one I should have CCd you,
> > sorry.
> 
> NP.  I haven't really any comment on whether your cache flushes are
> good or not, but two things occur to me:
> 
> * Are there other situations where the toolstack (or device model)
>   maps domain memory, which also need to be treated ?

These are all the ones I know of/could find in the domain building case,
which is the main time we access guest memory like this, and the one
which is problematic because the guest starts with its caches turned
off. This is where we have an actual problem in practice today.

The other one would be migration when we get there, but that doesn't
exist yet.

Other than that we would normally expect/require that guests enable
their caches and run with them on. We already require that hypercall
argument buffers are in a cacheable region because doing otherwise adds
a load of complexity to the common case on the hypervisor side.

There is no device model on ARM. If and when there is we would have to
think about what that means wrt caches, especially given that in this
case the guest knows it will have to do cache maintenance to do DMA etc.

> * This:
> 
> > > +static void minios_privcmd_cache_flush(xc_interface *xch,
> > > +                                       const void *ptr, size_t nr)
> > > +{
> > > +#if defined(__i386__) || defined(__x86_64__)
> > > +    /* No need for cache maintenance on x86 */
> > > +#else
> > > +    PERROR("No cache flush operation defined for architecture");
> > > +#endif
> > > +}
> 
> That appears to just print a warning message to a file no-one will
> read.  I think it should crash.

Actually, for minios there is no PERROR defined at all so it won't
compile, I clearly forgot to build test stubdoms.

The rest of xc_minios.c just uses printf, so I will do the same. I find
it hard to believe that whoever is developing a minios based builder on
ARM or some new platform wouldn't be looking at the stubdom console.
Unless you feel strongly that I should stick an abort() in here (not
sure what minios will do with that...).

> You may save some code by having a single unimplemented_cache_flush
> function to put in all these structs.

Only one is built at a time, depending on the platform, so there is no
duplication in the binary. I'm not too worried about the source
duplication in this instance, I think its good to have these platform
files be pretty standalone.

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel