From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Chen, Tiejun" Subject: Re: [PATCH 1/1] xen:vtd: missing RMRR mapping while share EPT Date: Thu, 24 Jul 2014 15:00:10 +0800 Message-ID: <53D0AEFA.7080303@intel.com> References: <1406108103-11981-1-git-send-email-tiejun.chen@intel.com> <53CFF40A020000780002539F@mail.emea.novell.com> <53D06011.5080106@intel.com> <53D0C0590200007800025556@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: <53D0C0590200007800025556@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, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On 2014/7/24 14:14, Jan Beulich wrote: >>>> On 24.07.14 at 03:23, wrote: >> On 2014/7/23 23:42, Jan Beulich wrote: >>>>>> On 23.07.14 at 11:35, wrote: >>>> @@ -1867,7 +1869,19 @@ static int rmrr_identity_mapping(struct domain *d, >>>> >>>> while ( base_pfn < end_pfn ) >>>> { >>>> - if ( intel_iommu_map_page(d, base_pfn, base_pfn, >>>> + if ( iommu_use_hap_pt(d) ) { >>>> + dprintk(XENLOG_DEBUG VTDPREFIX, >>>> + "Set RMRR mapping: pfn:0x%lx mfn:0x%lx.\n", >>>> + base_pfn, mfn_x(_mfn(base_pfn))); >>> >>> Do we really need this message, even more so not at guest level? >> >> Its useful to debug as I think, but if you insist on this point, I'm >> fine to remove this as well. > > The main question is how frequently this may get printed vs how > useful the message is. > >>> Apart from the above there are several Indentation issues here. >> >> Are you saying this thing? >> >> if () >> { >> } > > Yes, among other things. > >> So what about this? > > Almost: > >> @@ -1867,7 +1869,21 @@ static int rmrr_identity_mapping(struct domain *d, >> >> while ( base_pfn < end_pfn ) >> { >> - if ( intel_iommu_map_page(d, base_pfn, base_pfn, >> + if ( iommu_use_hap_pt(d) ) > > Don't you, btw, need to extend this condition by > && (!iommu_passthrough || !is_hardware_domain(d))? Why do we need these checks here? Current problem I met is issued when do GFX passthrough for Windows Guest. > >> + { >> + dprintk(XENLOG_DEBUG VTDPREFIX, > > This still (if you absolutely want to retain the message) needs I will remove this simply since this is not a big deal :) > changing to XENLOG_G_DEBUG, and you want to include the domain > ID in what gets printed for the message to be of any practical use. > >> + "Set RMRR mapping: pfn:%#lx mfn:%#lx.\n", > > Additionally please omit the stop at the end. Also, with VTDPREFIX > not ending with a space, you want the message to be starting > with one. > >> + base_pfn, mfn_x(_mfn(base_pfn))); >> + p2m_lock(p2m); >> + if ( p2m_set_entry(p2m, base_pfn, _mfn(base_pfn), PAGE_ORDER_4K, >> + p2m_mmio_direct, p2m_access_rw) ) > > Indentation. > >> + { >> + p2m_unlock(p2m); >> + return -1; >> + } >> + p2m_unlock(p2m); >> + } >> + else if ( intel_iommu_map_page(d, base_pfn, base_pfn, >> IOMMUF_readable|IOMMUF_writable) ) > > Again (here you need you also adjust the second line for indentation > to match up again). I'm not familiar with our xen coding style so I'm wondering if we have such a similar .pl like checkpatch.pl. Thanks Tiejun