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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 01.12.14 at 09:49, <yu.c.zhang@xxxxxxxxxxxxxxx> wrote:
> On 11/28/2014 5:57 PM, Jan Beulich wrote:
>>>>> On 28.11.14 at 08:59, <yu.c.zhang@xxxxxxxxxxxxxxx> wrote:
>>> --- a/xen/arch/x86/hvm/hvm.c
>>> +++ b/xen/arch/x86/hvm/hvm.c
>>> @@ -2838,7 +2838,8 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned 
>>> long gla,
>>>        * to the mmio handler.
>>>        */
>>>       if ( (p2mt == p2m_mmio_dm) ||
>>> -         (npfec.write_access && (p2mt == p2m_ram_ro)) )
>>> +         (npfec.write_access &&
>>> +           ((p2mt == p2m_ram_ro) || (p2mt == p2m_mmio_write_dm))) )
>>
>> Why are you corrupting indentation here?
> Thanks for your comments, Jan.
> The indentation here is to make sure the
> ((p2mt == p2m_ram_ro) || (p2mt == p2m_mmio_write_dm)) are grouped 
> together. But I am not sure if this is correct according to xen coding 
> style. I may have misunderstood your previous comments on Sep 3, which 
> said "the indentation would need adjustment" in reply to "[Xen-devel] 
> [PATCH v3 1/2] x86: add p2m_mmio_write_dm"

Getting the alignment right is needed, but no via using hard tabs.

>> Furthermore the code you modify here suggests that p2m_ram_ro
>> already has the needed semantics - writes get passed to the DM.
>> None of the other changes you make, and none of the other uses
>> of p2m_ram_ro appear to be in conflict with your intentions, so
>> you'd really need to explain better why you need the new type.
>>
> Thanks Jan.
> 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?

> Maybe I need to update my commit message to explain this more clearly. :)

At the very least, yes.

Jan


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