Re: [PATCH v4] x86: add p2m_mmio_write_dm

["Yu, Zhang" <yu.c.zhang@linux.intel.com>]

On 12/1/2014 8:31 PM, Jan Beulich wrote:
>>>> On 01.12.14 at 13:13, <tim@xen.org> wrote:
>> At 11:17 +0000 on 01 Dec (1417429027), Jan Beulich wrote:
>>>>>> On 01.12.14 at 11:30, <tim@xen.org> wrote:
>>>> At 09:32 +0000 on 01 Dec (1417422746), Jan Beulich wrote:
>>>>>>>> On 01.12.14 at 09:49, <yu.c.zhang@linux.intel.com> 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()?
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.

> Jan
>
>
>