From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Yu, Zhang" Subject: Re: [PATCH v5 2/2] add a new p2m type - p2m_mmio_write_dm Date: Fri, 05 Dec 2014 10:00:44 +0800 Message-ID: <548111CC.1070300@linux.intel.com> References: <1417698806-26807-1-git-send-email-yu.c.zhang@linux.intel.com> <1417698806-26807-3-git-send-email-yu.c.zhang@linux.intel.com> <20141204160445.GD43116@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: <20141204160445.GD43116@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 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, JBeulich@suse.com, yang.z.zhang@intel.com List-Id: xen-devel@lists.xenproject.org On 12/5/2014 12:04 AM, Tim Deegan wrote: > Hi, > > At 21:13 +0800 on 04 Dec (1417724006), Yu Zhang wrote: >> A new p2m type, p2m_mmio_write_dm, is added to trap and emulate >> the write operations on GPU's page tables. Handling of this new >> p2m type are similar with existing p2m_ram_ro in most condition >> checks, with only difference on final policy of emulation vs. drop. >> For p2m_ram_ro types, write operations will not trigger the device >> model, and will be discarded later in __hvm_copy(); while for the >> p2m_mmio_write_dm type pages, writes will go to the device model >> via ioreq-server. >> >> Signed-off-by: Yu Zhang >> Signed-off-by: Wei Ye > > Thanks for this -- only two comments: > >> @@ -5978,7 +5982,8 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg) >> goto param_fail4; >> } >> if ( !p2m_is_ram(t) && >> - (!p2m_is_hole(t) || a.hvmmem_type != HVMMEM_mmio_dm) ) >> + (!p2m_is_hole(t) || a.hvmmem_type != HVMMEM_mmio_dm) && >> + t != p2m_mmio_write_dm ) > > I think that Jan already brough this up, and maybe I missed your > answer: this realaxation looks wrong to me. I would have thought that > transition between p2m_mmio_write_dm and p2m_ram_rw/p2m_ram_logdirty > would be the only ones you would want to allow. Ha. Sorry, my negligence, and thanks for pointing out. :) The transition we use now is only between p2m_mmio_write_dm and p2m_ram_rw. So how about this: if ( !p2m_is_ram(t) && (!p2m_is_hole(t) || a.hvmmem_type != HVMMEM_mmio_dm) && (t != p2m_mmio_write_dm || a.hvmmem_type != HVMMEM_ram_rw) ) > >> @@ -111,7 +112,8 @@ typedef unsigned int p2m_query_t; >> #define P2M_RO_TYPES (p2m_to_mask(p2m_ram_logdirty) \ >> | p2m_to_mask(p2m_ram_ro) \ >> | p2m_to_mask(p2m_grant_map_ro) \ >> - | p2m_to_mask(p2m_ram_shared) ) >> + | p2m_to_mask(p2m_ram_shared) \ >> + | p2m_to_mask(p2m_mmio_write_dm)) > > Nit: please align the '\' with the others above it. Got it, and thanks. B.R. Yu > > Cheers, > > Tim. > >