From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Yu, Zhang" Subject: Re: [PATCH v4] x86: add p2m_mmio_write_dm Date: Tue, 02 Dec 2014 15:48:38 +0800 Message-ID: <547D6ED6.2030803@linux.intel.com> References: <1417161552-5155-1-git-send-email-yu.c.zhang@linux.intel.com> <54785525020000780004B588@mail.emea.novell.com> <547C2BAF.6010004@linux.intel.com> <547C43BA020000780004B9BE@mail.emea.novell.com> <20141201103012.GA69236@deinos.phlegethon.org> <547C5C43020000780004BAFD@mail.emea.novell.com> <20141201121300.GB69236@deinos.phlegethon.org> <547C6D98020000780004BB9D@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <547C6D98020000780004BB9D@mail.emea.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich , Tim Deegan Cc: kevin.tian@intel.com, keir@xen.org, ian.campbell@citrix.com, stefano.stabellini@eu.citrix.com, ian.jackson@eu.citrix.com, donald.d.dugger@intel.com, Xen-devel@lists.xen.org, Paul.Durrant@citrix.com, zhiyuan.lv@intel.com, yang.z.zhang@intel.com List-Id: xen-devel@lists.xenproject.org On 12/1/2014 8:31 PM, Jan Beulich wrote: >>>> On 01.12.14 at 13:13, wrote: >> At 11:17 +0000 on 01 Dec (1417429027), Jan Beulich wrote: >>>>>> On 01.12.14 at 11:30, wrote: >>>> At 09:32 +0000 on 01 Dec (1417422746), Jan Beulich wrote: >>>>>>>> On 01.12.14 at 09:49, 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 > > >