From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Chen, Tiejun" Subject: Re: [v7][RFC][PATCH 08/13] xen/x86/p2m: set p2m_access_n for reserved device memory mapping Date: Thu, 30 Oct 2014 15:39:52 +0800 Message-ID: <5451EB48.9010103@intel.com> References: <1414136077-18599-1-git-send-email-tiejun.chen@intel.com> <1414136077-18599-9-git-send-email-tiejun.chen@intel.com> <544A88560200007800042056@mail.emea.novell.com> <544E0ACA.8050201@intel.com> <544E2D8002000078000425A9@mail.emea.novell.com> <544F531C.7060401@intel.com> <544F7A310200007800042BAC@mail.emea.novell.com> <5450A330.6020102@intel.com> <5450BF63020000780004305E@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: <5450BF63020000780004305E@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 Cc: yang.z.zhang@intel.com, kevin.tian@intel.com, tim@xen.org, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On 2014/10/29 17:20, Jan Beulich wrote: >>>> On 29.10.14 at 09:20, wrote: >> @@ -686,8 +686,19 @@ guest_physmap_add_entry(struct domain *d, unsigned long gfn, >> /* Now, actually do the two-way mapping */ >> if ( mfn_valid(_mfn(mfn)) ) >> { >> - rc = p2m_set_entry(p2m, gfn, _mfn(mfn), page_order, t, >> - p2m->default_access); >> + rc = 0; >> + if ( !is_hardware_domain(d) ) >> + { >> + rc = iommu_get_reserved_device_memory(p2m_check_reserved_device_memory, >> + &gfn); >> + if ( rc < 0 ) >> + printk("Domain %d can't can't check reserved device memory.\n", >> + d->domain_id); >> + } >> + >> + /* We need to set reserved device memory as p2m_access_n. */ >> + a = ( rc == 1 ) ? p2m_access_n : p2m->default_access; >> + rc = p2m_set_entry(p2m, gfn, _mfn(mfn), page_order, t, a); >> if ( rc ) >> goto out; /* Failed to update p2m, bail without updating m2p. */ > > Getting closer. Just set a to p2m->default_access before the if(), > and overwrite it when rc == 1 inside the if(). And properly handle > the error case (just logging a message - which btw lacks a proper > XENLOG_G_* prefix - doesn't seem enough to me). Please check the follows: @@ -686,8 +686,22 @@ guest_physmap_add_entry(struct domain *d, unsigned long gfn, /* Now, actually do the two-way mapping */ if ( mfn_valid(_mfn(mfn)) ) { - rc = p2m_set_entry(p2m, gfn, _mfn(mfn), page_order, t, - p2m->default_access); + rc = 0; + a = p2m->default_access; + if ( !is_hardware_domain(d) ) + { + rc = iommu_get_reserved_device_memory(p2m_check_reserved_device_memory, + &gfn); + /* We need to set reserved device memory as p2m_access_n. */ + if ( rc == 1 ) + a = p2m_access_n; + else if ( rc < 0 ) + printk(XENLOG_WARNING + "Domain %d can't check reserved device memory.\n", + d->domain_id); + } + + rc = p2m_set_entry(p2m, gfn, _mfn(mfn), page_order, t, a); if ( rc ) goto out; /* Failed to update p2m, bail without updating m2p. */ > > But then again this code may change altogether if you avoid > populating the reserved regions in the first place. Are you saying this scenario? #1 Here we first set these ranges as p2m_access_n #2 We reset them as 1:1 RMRR mapping with p2m_access_rw somewhere #3 Someone may try to populate these ranges again Thanks Tiejun > > Jan > > >