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: Mon, 27 Oct 2014 17:05:14 +0800 Message-ID: <544E0ACA.8050201@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> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <544A88560200007800042056@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/24 23:11, Jan Beulich wrote: >>>> On 24.10.14 at 09:34, wrote: >> --- a/xen/arch/x86/mm/p2m.c >> +++ b/xen/arch/x86/mm/p2m.c >> @@ -686,6 +686,30 @@ guest_physmap_add_entry(struct domain *d, unsigned long gfn, >> /* Now, actually do the two-way mapping */ >> if ( mfn_valid(_mfn(mfn)) ) >> { >> + >> + if ( !is_hardware_domain(d) ) >> + { >> + rc = iommu_get_reserved_device_memory(p2m_check_reserved_device_memory, >> + &gfn); > > Okay, no I see what that function is needed for. It being an inline > function is of course very questionable looking at this use site. --- a/xen/arch/x86/mm/p2m.c +++ b/xen/arch/x86/mm/p2m.c @@ -556,6 +556,17 @@ guest_physmap_remove_page(struct domain *d, unsigned long gfn, gfn_unlock(p2m, gfn, page_order); } +/* Check if we are accessing rdm. */ +int p2m_check_reserved_device_memory(xen_pfn_t start, + xen_ulong_t nr, + void *d) +{ + unsigned long *gfn = d; + xen_pfn_t end = start + nr; + + return ( *gfn >= start && *gfn <= end ) ? 1 : 0; +} + int guest_physmap_add_entry(struct domain *d, unsigned long gfn, unsigned long mfn, unsigned int page_order, > >> + if ( rc ) > > And the return value from the called function is of type int - > non-zero may not just mean "true" but (when negative) also > "error". You need to distinguish these cases. But in our case its impossible to get a negative value. > >> + { >> + /* >> + * 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. > >> + { >> + 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); Thanks Tiejun