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:38:46 +0800 Message-ID: <547D6C86.4090400@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> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20141201121300.GB69236@deinos.phlegethon.org> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Tim Deegan , Jan Beulich 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:13 PM, Tim Deegan 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. > > But yes, we ought to follow the established hypercall interface, and > so we need the new type. > >>> 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. >> >> And I suppose in that latter case the new type could be made part >> of P2M_RO_TYPES()? > > Yes indeed, as P2M_RO_TYPES is defined as "must have the _PAGE_RW bit > clear in their PTEs". > Thanks Tim. Following are my understanding of the P2M_RO_TYPES and your comments. Not sure if I get it right. Please correct me if anything wrong: 1> The P2M_RO_TYPES now bears 2 meanings: one is "w bit is clear in the pte"; and another being to discard the write operations; 2> We better define another class to bear the second meaning. Also some questions for the new p2m class, say P2M_DISCARD_WRITE_TYPES, and the new predicates, say p2m_is_discard_write: 1> You mentioned emulate_gva_to_mfn() and __hvm_copy() should discard the write ops, yet I also noticed many other places using the p2m_is_readonly, or only the "p2mt == p2m_ram_ro" judgement(in __hvm_copy/__hvm_clear). Among all these other places, is there any ones also supposed to use the p2m_is_discard_write? 2> p2m_grant_map_ro is also supposed to be discarded? Will handling of this type of pages goes into __hvm_copy()/__hvm_clear(), or should? I'm a new guy of this area, and sorry for my messed questions. :) Yu > Cheers, > > Tim. > >