From mboxrd@z Thu Jan 1 00:00:00 1970 From: Keir Fraser Subject: Re: [RFC PATCH 0/3] AMD IOMMU: Share p2m table with iommu Date: Tue, 17 May 2011 08:51:39 +0100 Message-ID: References: <987664A83D2D224EAE907B061CE93D5301CD1985D0@orsmsx505.amr.corp.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="ISO-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: In-Reply-To: <987664A83D2D224EAE907B061CE93D5301CD1985D0@orsmsx505.amr.corp.intel.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: "Kay, Allen M" , Tim Deegan , "Zhang, Yang Z" Cc: Wei Wang , "andre.przywara@amd.com" , "xen-devel@lists.xensource.com" List-Id: xen-devel@lists.xenproject.org On 17/05/2011 03:21, "Kay, Allen M" wrote: > Looking closer, I have found there is indeed a check in hvm_get_insn_byte= s(). > However, it should return 1 instead of 0 for hvm_funcs.get_insn_bytes > undefined case so that original code gets called. >=20 > return (hvm_funcs.get_insn_bytes ? hvm_funcs.get_insn_bytes(v, buf) := 0); >=20 > Should be: >=20 > return (hvm_funcs.get_insn_bytes ? hvm_funcs.get_insn_bytes(v, buf) := 1); Ummm, no, it's correct as it is. -- Keir >=20 > -----Original Message----- > From: Kay, Allen M > Sent: Monday, May 16, 2011 5:13 PM > To: 'Tim Deegan'; Zhang, Yang Z; Keir Fraser > Cc: Wei Wang; xen-devel@lists.xensource.com; 'andre.przywara@amd.com' > Subject: RE: [Xen-devel] [RFC PATCH 0/3] AMD IOMMU: Share p2m table with = iommu >=20 > The problem appear to in part caused by unconditional call of get_insn_by= tes() > in hvm_function_table (cs# 23238). This function interface is defined fo= r svm > but not for vmx. However, it is call unconditionally from hvm_emulate_on= e(). > For some reason it fails silently without any indication that it is > dereferencing a null function pointer. >=20 > I see there are also other unconditional for nested functions nhvm* such = as > nhvm_vcpu_vmext_trap() and nhvm_intr_blocked() - which not defined in > hvm_function_table for vmx. >=20 > What is the appropriate way to handle asymmetric function table definitio= n > between svm and vmx? Should the caller always check for whether the func= tion > is defined or not before calling it in generic code? >=20 > Allen >=20 > -----Original Message----- > From: Tim Deegan [mailto:Tim.Deegan@citrix.com] > Sent: Monday, May 16, 2011 1:43 AM > To: Zhang, Yang Z > Cc: Wei Wang; xen-devel@lists.xensource.com; Kay, Allen M > Subject: Re: [Xen-devel] [RFC PATCH 0/3] AMD IOMMU: Share p2m table with = iommu >=20 > At 09:36 +0100 on 16 May (1305538573), Zhang, Yang Z wrote: >>> What's the failure mode? Or better, what change in the common code are >>> you objecting to? >=20 >> The following patch cause the vt-d fail to work. I suspect that the >> change is not appropriate for intel platform. >=20 > Thank you. In what way does it fail? >=20 > Have you tested with 23300:4b0692880dfa applied? It's a fix on top of > this change.=20 >=20 > Thanks, >=20 > Tim. >=20 >> Add allen to CC list. Maybe he can give a more authoritative answer. >>=20 >> diff -r 51d89366c859 -r 78145a98915c xen/arch/x86/mm/p2m.c >> --- a/xen/arch/x86/mm/p2m.c Mon Apr 18 15:12:04 2011 +0100 >> +++ b/xen/arch/x86/mm/p2m.c Mon Apr 18 17:24:21 2011 +0100 >> @@ -80,7 +80,12 @@ unsigned long p2m_type_to_flags(p2m_type { >> unsigned long flags; >> #ifdef __x86_64__ >> - flags =3D (unsigned long)(t & 0x3fff) << 9; >> + /* >> + * AMD IOMMU: When we share p2m table with iommu, bit 9 - bit 11 wi= ll be >> + * used for iommu hardware to encode next io page level. Bit 59 - b= it 62 >> + * are used for iommu flags, We could not use these bits to store p= 2m >> types. >> + */ >> + flags =3D (unsigned long)(t & 0x7f) << 12; >> #else >> flags =3D (t & 0x7UL) << 9; >> #endif >> @@ -1826,6 +1831,9 @@ static mfn_t p2m_gfn_to_mfn_current(stru >> p2mt =3D p2m_flags_to_type(l1e_get_flags(l1e)); >> ASSERT(l1e_get_pfn(l1e) !=3D INVALID_MFN || !p2m_is_ram(p2mt)= ); >>=20 >> + if ( l1e.l1 =3D=3D 0 ) >> + p2mt =3D p2m_invalid; >> + >> if ( p2m_flags_to_type(l1e_get_flags(l1e)) >> =3D=3D p2m_populate_on_demand ) >> { >> diff -r 51d89366c859 -r 78145a98915c xen/include/asm-x86/p2m.h >> --- a/xen/include/asm-x86/p2m.h Mon Apr 18 15:12:04 2011 +0100 >> +++ b/xen/include/asm-x86/p2m.h Mon Apr 18 17:24:21 2011 +0100 >> @@ -63,9 +63,15 @@ >> * Further expansions of the type system will only be supported on >> * 64-bit Xen. >> */ >> + >> +/* >> + * AMD IOMMU: When we share p2m table with iommu, bit 52 -bit 58 in pte >> + * cannot be non-zero, otherwise, hardware generates io page faults >> +when >> + * device access those pages. Therefore, p2m_ram_rw has to be defined a= s 0. >> + */ >> typedef enum { >> - p2m_invalid =3D 0, /* Nothing mapped here */ >> - p2m_ram_rw =3D 1, /* Normal read/write guest RAM */ >> + p2m_ram_rw =3D 0, /* Normal read/write guest RAM */ >> + p2m_invalid =3D 1, /* Nothing mapped here */ >> p2m_ram_logdirty =3D 2, /* Temporarily read-only for log-dirty = */ >> p2m_ram_ro =3D 3, /* Read-only; writes are silently dropp= ed */ >> p2m_mmio_dm =3D 4, /* Reads and write go to the device mod= el */ >> @@ -375,7 +381,13 @@ static inline p2m_type_t p2m_flags_to_ty { >> /* Type is stored in the "available" bits */ #ifdef __x86_64__ >> - return (flags >> 9) & 0x3fff; >> + /* >> + * AMD IOMMU: When we share p2m table with iommu, bit 9 - bit 11 wi= ll be >> + * used for iommu hardware to encode next io page level. Bit 59 - b= it 62 >> + * are used for iommu flags, We could not use these bits to store p= 2m >> types. >> + */ >> + >> + return (flags >> 12) & 0x7f; >> #else >> return (flags >> 9) & 0x7; >> #endif >>=20 >>>=20 >>> BTW, this is not the patch set that was applied; when I applied the >>> revised patches (about a month ago) I CC'd the Intel IOMMU maintainer. >>>=20 >>> Tim. >>>=20 >>>> best regards >>>> yang >>>>=20 >>>>> -----Original Message----- >>>>> From: xen-devel-bounces@lists.xensource.com >>>>> [mailto:xen-devel-bounces@lists.xensource.com] On Behalf Of Wei Wang >>>>> Sent: Friday, March 25, 2011 6:32 PM >>>>> To: xen-devel@lists.xensource.com >>>>> Subject: [Xen-devel] [RFC PATCH 0/3] AMD IOMMU: Share p2m table with >>>>> iommu >>>>>=20 >>>>> Hi, >>>>> This patch set implements p2m table sharing for amd iommu. Please >>>>> comment it. >>>>> Thanks, >>>>> Wei >>>>>=20 >>>>> -- >>>>> Advanced Micro Devices GmbH >>>>> Sitz: Dornach, Gemeinde Aschheim, >>>>> Landkreis M=FCnchen Registergericht M=FCnchen, >>>>> HRB Nr. 43632 >>>>> WEEE-Reg-Nr: DE 12919551 >>>>> Gesch=E4ftsf=FChrer: >>>>> Alberto Bozzo, Andrew Bowd >>>>>=20 >>>>>=20 >>>>>=20 >>>>>=20 >>>>> _______________________________________________ >>>>> Xen-devel mailing list >>>>> Xen-devel@lists.xensource.com >>>>> http://lists.xensource.com/xen-devel >>>=20 >>> -- >>> Tim Deegan >>> Principal Software Engineer, Xen Platform Team >>> Citrix Systems UK Ltd. (Company #02937203, SL9 0BG)