From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Chen, Tiejun" Subject: Re: [v3][PATCH 02/16] xen/x86/p2m: introduce set_identity_p2m_entry Date: Thu, 11 Jun 2015 17:25:42 +0800 Message-ID: <55795416.6050205@intel.com> References: <1433985325-16676-1-git-send-email-tiejun.chen@intel.com> <1433985325-16676-3-git-send-email-tiejun.chen@intel.com> <557955F5020000780008360A@mail.emea.novell.com> <5579459A.7020406@intel.com> <55796FC502000078000837D5@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: <55796FC502000078000837D5@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: tim@xen.org, kevin.tian@intel.com, wei.liu2@citrix.com, ian.campbell@citrix.com, andrew.cooper3@citrix.com, Ian.Jackson@eu.citrix.com, xen-devel@lists.xen.org, stefano.stabellini@citrix.com, yang.z.zhang@intel.com List-Id: xen-devel@lists.xenproject.org On 2015/6/11 17:23, Jan Beulich wrote: >>>> On 11.06.15 at 10:23, wrote: >> On 2015/6/11 15:33, Jan Beulich wrote: >>>>>> On 11.06.15 at 03:15, wrote: >>>> We will create this sort of identity mapping as follows: >>>> >>>> If the gfn space is unoccupied, we just set the mapping. If the space >>>> is already occupied by 1:1 mappings, do nothing. Failed for any >>>> other cases. >>>> >>>> Signed-off-by: Tiejun Chen >>> >>> First of all you continue to be copying each patch to every >>> maintainer involved in some part of the series. Please limit the >> >> I just hope all involved guys can see this series on the whole to >> review. But, >> >>> Cc list of each patch to the actual list of people needing to be >>> Cc-ed on it (or you know explicitly wanting a copy). >> >> Next, I will just send them to each associated maintainer. >> >>> >>>> --- a/xen/arch/x86/mm/p2m.c >>>> +++ b/xen/arch/x86/mm/p2m.c >>>> @@ -898,6 +898,41 @@ int set_mmio_p2m_entry(struct domain *d, unsigned long >> gfn, mfn_t mfn, >>>> return set_typed_p2m_entry(d, gfn, mfn, p2m_mmio_direct, access); >>>> } >>>> >>>> +int set_identity_p2m_entry(struct domain *d, unsigned long gfn, >>>> + p2m_access_t p2ma) >>>> +{ >>>> + p2m_type_t p2mt; >>>> + p2m_access_t a; >>>> + mfn_t mfn; >>>> + struct p2m_domain *p2m = p2m_get_hostp2m(d); >>>> + int ret; >>>> + >>>> + if ( paging_mode_translate(p2m->domain) ) >>>> + { >>>> + gfn_lock(p2m, gfn, 0); >>>> + >>>> + mfn = p2m->get_entry(p2m, gfn, &p2mt, &a, 0, NULL); >>>> + >>>> + if ( p2mt == p2m_invalid || mfn_x(mfn) == INVALID_MFN ) >>> >>> I'm not fundamentally opposed to this extra INVALID_MFN check, but >>> could you please clarify for which P2M type you saw INVALID_MFN >>> coming back here, and for which p2m_invalid cases you didn't also >>> see INVALID_MFN? I.e. I'd really prefer a single check to be used >>> when that can cover all cases. >> >> Actually, I initially adopted "!mfn_valid(mfn)" in our previous version. >> But Tim issued one comment about this, >> >> "I don't think this check is quite right -- for example, this p2m entry >> might be an MMIO mapping or a PoD entry. "if ( p2mt == p2m_invalid )" >> would be better." > > Ah, I right, I now remember. In which case checking against > INVALID_MFN would cover the MMIO case, but not the PoD one. > >> But if I just keep his recommended check, you can see the following when >> I pass through IGD, >> >> (XEN) Cannot identity map d1:ad800, already mapped to ffffffffffffffff >> with p2mt:4. >> >> Looks "4" indicates p2m_mmio_dm, right? > > And it seems to me that this particular combination would need > special treatment, i.e. you'd need > > if ( p2mt == p2m_invalid || > (p2mt == p2m_mmio_dm && mfn_x(mfn) == INVALID_MFN) ) > > as long as p2m_invalid isn't the default type lookups return. But > I'd recommend waiting for Tim to confirm or further adjust that. > Sure. Thanks Tiejun