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

[Tim Deegan <tim@xxxxxxx>]

At 09:32 +0000 on 01 Dec (1417422746), Jan Beulich wrote:
> >>> 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?

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.

During this bit of archaeology I realised that either this new type
should _not_ be made part of P2M_RO_TYPES, or, better, we need a new
class of P2M types (P2M_DISCARD_WRITE_TYPES, say) that should be used
for these paths in emulate_gva_to_mfn() and __hvm_copy(), containing
just p2m_ram_ro and p2m_grant_map_ro.

Cheers,

Tim.

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