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: Wed, 29 Oct 2014 16:20:00 +0800 Message-ID: <5450A330.6020102@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> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <544F7A310200007800042BAC@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/28 18:12, Jan Beulich wrote: >>>> On 28.10.14 at 09:26, wrote: >> On 2014/10/27 18:33, Jan Beulich wrote: >>>>>> On 27.10.14 at 10:05, wrote: >>>> On 2014/10/24 23:11, Jan Beulich wrote: >>>>>>>> On 24.10.14 at 09:34, wrote: >>>>>> + { >>>>>> + /* >>>>>> + * Just set p2m_access_n in case of shared-ept >>>>>> + * or non-shared ept but 1:1 mapping. >>>>>> + */ >>>>>> + if ( iommu_use_hap_pt(d) || >>>>>> + (!iommu_use_hap_pt(d) && mfn == gfn) ) >>>>> >>>>> How would, other than by chance, mfn equal gfn here? Also the >>>>> double use of iommu_use_hap_pt(d) is pointless here. >>>> >>>> There are two scenarios we should concern: >>>> >>>> #1 in case of shared-ept. >>>> >>>> We always need to check so iommu_use_hap_pt(d) is good. >>>> >>>> #2 in case of non-sharepd-ept >>>> >>>> If mfn != gfn I think guest don't access RMRR range, so its allowed. >>> >>> And what if subsequently a device needing a 1:1 mapping at this GFN >>> gets assigned? (I simply don't see why shared vs non-shared would >>> matter here.) >> >> In case of non-shared ept we just create VT-d table, if we assign a >> device with 1:1 RMRR mapping. So as long as mfn != gfn, its not >> necessary to set p2m_access_n. > > I think it was mentioned before (and not just by me) that it's at least > risky to allow the two page tables to get out of sync wrt the > translations they do (as opposed to permissions they set). Okay I will remove all condition check here. > >>>>>> + { >>>>>> + rc = p2m_set_entry(p2m, gfn, _mfn(mfn), page_order, t, >>>>>> + p2m_access_n); >>>>>> + if ( rc ) >>>>>> + gdprintk(XENLOG_WARNING, "set rdm p2m failed: (%#lx)\n", >>>>>> + gfn); >>>>> >>>>> Such messages are (due to acting on a foreign domain) relatively >>>>> useless without also logging the domain that is affected. Conversely, >>>>> logging the current domain and vCPU (due to using gdprintk()) is >>>>> rather pointless. Also please drop either the colon or the >>>>> parentheses in the message. >>>> >>>> Can P2M_DEBUG work here? >>>> >>>> P2M_DEBUG("set rdm p2m failed: %#lx\n", gfn); >>> >>> I don't think this would magically add the missing information. Plus it >> >> Sorry, is it okay? >> >> gdprintk(XENLOG_WARNING, "Domain %hu set rdm p2m failed: %#lx\n", >> d->domain_id, gfn); > > Almost. Use printk() instead of gdprintk(), and %d instead of %hu. > >>> would limit output to the !NDEBUG case, putting the practical >>> usefulness of this under question even more. >>> >>> But anyway, looking at the existing code again, I think you'd be better >>> off falling through to the p2m_set_entry() that's already there, just >> >> Are you saying I should do this in p2m_set_entry()? > > No. I said that I'd prefer you to use the _existing call to_ > p2m_set_entry(). > >>> altering the access permission value you pass. Less code, better >> >> But here, guest_physmap_add_entry() just initiate to set p2m_access_n. >> We will alter the access permission until if we really assign device to >> create a 1:1 mapping. > > And I didn't question that. All I said is to use the existing call by simply > replacing the unconditional use of the default access value with one > conditionally set to p2m_access_n. Okay, @@ -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. */ Thanks Tiejun > > Jan > >