[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] Possible issue with x86_emulate when writing results back to memory


  • To: Jan Beulich <JBeulich@xxxxxxxx>
  • From: Simon Graham <simon.graham@xxxxxxxxxx>
  • Date: Thu, 9 Jan 2014 16:07:23 +0000
  • Accept-language: en-US
  • Cc: xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Thu, 09 Jan 2014 16:07:31 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xen.org>
  • Thread-index: Ac711YmNMUOUVnlMTTS29ThyibIwTgXqECOAAApgbkA=
  • Thread-topic: [Xen-devel] Possible issue with x86_emulate when writing results back to memory

> > Does this seem a reasonable explanation of the issue?
> 
> Yes - as long as you can also explain why a spin lock operation
> would make it into the emulation code in the first place.
> 

Well, that one is tough and I don't have a good answer... the only thing I 
would say is that in our system we ALWAYS have the shadow memory tracking 
enabled (to track changes to framebuffers)

> > The attached patch is for discussion purposes only - if it is deemed
> > acceptable I'll resubmit a proper patch request against unstable.
> 
> I'd rather not add limited scope special casing like that, but instead
> make the copying much more like real hardware (i.e. not just deal
> with the 16- and 32-bit cases, and especially not rely on memcpy()
> using 64-bit reads/writes when it can). IOW - don't use memcpy()
> here at all (and have a single routine doing The Right Thing (tm)
> rather than having two clones now, and perhaps more later on -
> I'd in particular think that the read side in shadow code would also
> need a similar adjustment).

My concern was that memcpy is (I assume!) highly optimized - it certainly 
should be if it isn't and I would worry that a change to make it atomic for the 
purposes of instruction emulation would result in an across the board perf hit 
when in most cases it isn't necessary that it be atomic.

This would be fine for the writeback code in the shadow module BUT the 
__hvm_copy routine is used generically in situations where atomicity is not 
required...

> 
> On the mechanical side of things: Such a generic routine should
> have proper parameter types: "const void *" for the source pointer
> and "unsigned long" or "size_t" for the count.

Sure - thanks.

> 
> Jan


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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.