Re: [Xen-devel] [PATCH v4] x86: add p2m_mmio_write_dm

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 02.12.14 at 11:37, <tim@xxxxxxx> wrote:
> At 12:31 +0000 on 01 Dec (1417433464), Jan Beulich wrote:
>> >>> On 01.12.14 at 13:13, <tim@xxxxxxx> wrote:
>> > At 11:17 +0000 on 01 Dec (1417429027), Jan Beulich wrote:
>> >> >>> On 01.12.14 at 11:30, <tim@xxxxxxx> wrote:
>> >> > At 09:32 +0000 on 01 Dec (1417422746), Jan Beulich wrote:
>> >> >> >>> On 01.12.14 at 09:49, <yu.c.zhang@xxxxxxxxxxxxxxx> wrote:
>> >> >> > To my understanding, pages with p2m_ram_ro are not supposed to be 
>> >> >> > modified by guest. So in __hvm_copy(), when p2m type of a page is 
>> >> >> > p2m_ram_rom, no copy will occur.
>> >> >> > However, to our usage, we just wanna this page to be write 
>> >> >> > protected, so 
>> >> >> > that our device model can be triggered to do some emulation. The 
>> >> >> > content 
>> >> >> > written to this page is supposed not to be dropped. This way, if 
>> >> >> > sequentially a read operation is performed by guest to this page, 
>> >> >> > the 
>> >> >> > guest will still see its anticipated value.
>> >> >> 
>> >> >> __hvm_copy() is only a helper function, and doesn't write to
>> >> >> mmio_dm space either; instead its (indirect) callers would invoke
>> >> >> hvmemul_do_mmio() upon seeing HVMCOPY_bad_gfn_to_mfn
>> >> >> returns. The question hence is about the apparent inconsistency
>> >> >> resulting from writes to ram_ro being dropped here but getting
>> >> >> passed to the DM by hvm_hap_nested_page_fault(). Tim - is
>> >> >> that really intentional?
>> >> > 
>> >> > No - and AFAICT it shouldn't be happening.  It _is_ how it was
>> >> > implemented originally, because it involved fewer moving parts and
>> >> > didn't need to be efficient (and after all, writes to entirely missing
>> >> > addresses go to the device model too).
>> >> > 
>> >> > But the code was later updated to log and discard writes to read-only
>> >> > memory (see 4d8aa29 from Trolle Selander).
>> >> > 
>> >> > Early version of p2m_ram_ro were documented in the internal headers as
>> >> > sending the writes to the DM, but the public interface (HVMMEM_ram_ro)
>> >> > has always said that writes are discarded.
>> >> 
>> >> Hmm, so which way do you recommend resolving the inconsistency
>> >> then - match what the public interface says or what the apparent
>> >> original intention for the internal type was? Presumably we need to
>> >> follow the public interface mandated model, and hence require the
>> >> new type to be introduced.
>> > 
>> > Sorry, I was unclear -- there isn't an inconsistency; both internal
>> > and public headers currently say that writes are discarded and AFAICT
>> > that is what the code does.
>> 
>> Not for hvm_hap_nested_page_fault() afaict - the forwarding to
>> DM there contradicts the "writes are discarded" model that other
>> code paths follow.
> 
> It calls handle_mmio() to emulate the instruction for it, but
> handle_mmio() ought not to send anything to the DM.  hvmemul_write()
> will call hvm_copy(), which will drop the write and report success.

Oh, of course - it's really just the wording of the comment there
that mislead me to assume the operation gets passed on for
actual handling. Thanks for clarifying!

Jan


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