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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 02.12.14 at 08:48, <yu.c.zhang@xxxxxxxxxxxxxxx> wrote:

> 
> On 12/1/2014 8:31 PM, 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.
>>
> Thanks, Jan.
> By "inconsistency", do you mean the p2m_ram_ro shall not trigger the 
> handle_mmio_with_translation() in hvm_hap_nested_page_fault()?

Yes, pending Tim's agreement.

> I'm also a bit confused with the "writes are discarded/dropped" comments 
> in the code. Does this mean writes to the p2m_ram_ro pages should be 
> abandoned without going to the dm, or going to the dm and  ignored 
> later? The code seems to be the second one.

And it would seem to me that it should be the former.

Jan


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