Wei Wang
2011-Mar-25 10:31 UTC
[Xen-devel] [RFC PATCH 0/3] AMD IOMMU: Share p2m table with iommu
Hi, This patch set implements p2m table sharing for amd iommu. Please comment it. Thanks, Wei -- Advanced Micro Devices GmbH Sitz: Dornach, Gemeinde Aschheim, Landkreis München Registergericht München, HRB Nr. 43632 WEEE-Reg-Nr: DE 12919551 Geschäftsführer: Alberto Bozzo, Andrew Bowd _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Zhang, Yang Z
2011-May-16 01:42 UTC
RE: [Xen-devel] [RFC PATCH 0/3] AMD IOMMU: Share p2m table with iommu
Did you test your patch in Intel platform? I see that your patch change some common iommu logic code and it should not appropriate for intel platform. And this cause our device pass-through fail to work. best regards yang> -----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 > > Hi, > This patch set implements p2m table sharing for amd iommu. Please > comment it. > Thanks, > Wei > > -- > Advanced Micro Devices GmbH > Sitz: Dornach, Gemeinde Aschheim, > Landkreis München Registergericht München, > HRB Nr. 43632 > WEEE-Reg-Nr: DE 12919551 > Geschäftsführer: > Alberto Bozzo, Andrew Bowd > > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Tim Deegan
2011-May-16 08:27 UTC
Re: [Xen-devel] [RFC PATCH 0/3] AMD IOMMU: Share p2m table with iommu
At 02:42 +0100 on 16 May (1305513766), Zhang, Yang Z wrote:> Did you test your patch in Intel platform? I see that your patch > change some common iommu logic code and it should not appropriate for > intel platform. And this cause our device pass-through fail to work.What''s the failure mode? Or better, what change in the common code are you objecting to? 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. Tim.> best regards > yang > > > -----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 > > > > Hi, > > This patch set implements p2m table sharing for amd iommu. Please > > comment it. > > Thanks, > > Wei > > > > -- > > Advanced Micro Devices GmbH > > Sitz: Dornach, Gemeinde Aschheim, > > Landkreis München Registergericht München, > > HRB Nr. 43632 > > WEEE-Reg-Nr: DE 12919551 > > Geschäftsführer: > > Alberto Bozzo, Andrew Bowd > > > > > > > > > > _______________________________________________ > > Xen-devel mailing list > > Xen-devel@lists.xensource.com > > http://lists.xensource.com/xen-devel-- Tim Deegan <Tim.Deegan@citrix.com> Principal Software Engineer, Xen Platform Team Citrix Systems UK Ltd. (Company #02937203, SL9 0BG) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Zhang, Yang Z
2011-May-16 08:36 UTC
RE: [Xen-devel] [RFC PATCH 0/3] AMD IOMMU: Share p2m table with iommu
> What''s the failure mode? Or better, what change in the common code are > you objecting to?The following patch cause the vt-d fail to work. I suspect that the change is not appropriate for intel platform. Add allen to CC list. Maybe he can give a more authoritative answer. 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 = (unsigned long)(t & 0x3fff) << 9; + /* + * AMD IOMMU: When we share p2m table with iommu, bit 9 - bit 11 will be + * used for iommu hardware to encode next io page level. Bit 59 - bit 62 + * are used for iommu flags, We could not use these bits to store p2m types. + */ + flags = (unsigned long)(t & 0x7f) << 12; #else flags = (t & 0x7UL) << 9; #endif @@ -1826,6 +1831,9 @@ static mfn_t p2m_gfn_to_mfn_current(stru p2mt = p2m_flags_to_type(l1e_get_flags(l1e)); ASSERT(l1e_get_pfn(l1e) != INVALID_MFN || !p2m_is_ram(p2mt)); + if ( l1e.l1 == 0 ) + p2mt = p2m_invalid; + if ( p2m_flags_to_type(l1e_get_flags(l1e)) == 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 as 0. + */ typedef enum { - p2m_invalid = 0, /* Nothing mapped here */ - p2m_ram_rw = 1, /* Normal read/write guest RAM */ + p2m_ram_rw = 0, /* Normal read/write guest RAM */ + p2m_invalid = 1, /* Nothing mapped here */ p2m_ram_logdirty = 2, /* Temporarily read-only for log-dirty */ p2m_ram_ro = 3, /* Read-only; writes are silently dropped */ p2m_mmio_dm = 4, /* Reads and write go to the device model */ @@ -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 will be + * used for iommu hardware to encode next io page level. Bit 59 - bit 62 + * are used for iommu flags, We could not use these bits to store p2m types. + */ + + return (flags >> 12) & 0x7f; #else return (flags >> 9) & 0x7; #endif> > 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. > > Tim. > > > best regards > > yang > > > > > -----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 > > > > > > Hi, > > > This patch set implements p2m table sharing for amd iommu. Please > > > comment it. > > > Thanks, > > > Wei > > > > > > -- > > > Advanced Micro Devices GmbH > > > Sitz: Dornach, Gemeinde Aschheim, > > > Landkreis München Registergericht München, > > > HRB Nr. 43632 > > > WEEE-Reg-Nr: DE 12919551 > > > Geschäftsführer: > > > Alberto Bozzo, Andrew Bowd > > > > > > > > > > > > > > > _______________________________________________ > > > Xen-devel mailing list > > > Xen-devel@lists.xensource.com > > > http://lists.xensource.com/xen-devel > > -- > Tim Deegan <Tim.Deegan@citrix.com> > Principal Software Engineer, Xen Platform Team > Citrix Systems UK Ltd. (Company #02937203, SL9 0BG)_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Tim Deegan
2011-May-16 08:43 UTC
Re: [Xen-devel] [RFC PATCH 0/3] AMD IOMMU: Share p2m table with iommu
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?> The following patch cause the vt-d fail to work. I suspect that the > change is not appropriate for intel platform.Thank you. In what way does it fail? Have you tested with 23300:4b0692880dfa applied? It''s a fix on top of this change. Thanks, Tim.> Add allen to CC list. Maybe he can give a more authoritative answer. > > 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 = (unsigned long)(t & 0x3fff) << 9; > + /* > + * AMD IOMMU: When we share p2m table with iommu, bit 9 - bit 11 will be > + * used for iommu hardware to encode next io page level. Bit 59 - bit 62 > + * are used for iommu flags, We could not use these bits to store p2m types. > + */ > + flags = (unsigned long)(t & 0x7f) << 12; > #else > flags = (t & 0x7UL) << 9; > #endif > @@ -1826,6 +1831,9 @@ static mfn_t p2m_gfn_to_mfn_current(stru > p2mt = p2m_flags_to_type(l1e_get_flags(l1e)); > ASSERT(l1e_get_pfn(l1e) != INVALID_MFN || !p2m_is_ram(p2mt)); > > + if ( l1e.l1 == 0 ) > + p2mt = p2m_invalid; > + > if ( p2m_flags_to_type(l1e_get_flags(l1e)) > == 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 as 0. > + */ > typedef enum { > - p2m_invalid = 0, /* Nothing mapped here */ > - p2m_ram_rw = 1, /* Normal read/write guest RAM */ > + p2m_ram_rw = 0, /* Normal read/write guest RAM */ > + p2m_invalid = 1, /* Nothing mapped here */ > p2m_ram_logdirty = 2, /* Temporarily read-only for log-dirty */ > p2m_ram_ro = 3, /* Read-only; writes are silently dropped */ > p2m_mmio_dm = 4, /* Reads and write go to the device model */ > @@ -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 will be > + * used for iommu hardware to encode next io page level. Bit 59 - bit 62 > + * are used for iommu flags, We could not use these bits to store p2m types. > + */ > + > + return (flags >> 12) & 0x7f; > #else > return (flags >> 9) & 0x7; > #endif > > > > > 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. > > > > Tim. > > > > > best regards > > > yang > > > > > > > -----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 > > > > > > > > Hi, > > > > This patch set implements p2m table sharing for amd iommu. Please > > > > comment it. > > > > Thanks, > > > > Wei > > > > > > > > -- > > > > Advanced Micro Devices GmbH > > > > Sitz: Dornach, Gemeinde Aschheim, > > > > Landkreis München Registergericht München, > > > > HRB Nr. 43632 > > > > WEEE-Reg-Nr: DE 12919551 > > > > Geschäftsführer: > > > > Alberto Bozzo, Andrew Bowd > > > > > > > > > > > > > > > > > > > > _______________________________________________ > > > > Xen-devel mailing list > > > > Xen-devel@lists.xensource.com > > > > http://lists.xensource.com/xen-devel > > > > -- > > Tim Deegan <Tim.Deegan@citrix.com> > > Principal Software Engineer, Xen Platform Team > > Citrix Systems UK Ltd. (Company #02937203, SL9 0BG)-- Tim Deegan <Tim.Deegan@citrix.com> Principal Software Engineer, Xen Platform Team Citrix Systems UK Ltd. (Company #02937203, SL9 0BG) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Zhang, Yang Z
2011-May-16 08:50 UTC
RE: [Xen-devel] [RFC PATCH 0/3] AMD IOMMU: Share p2m table with iommu
> Thank you. In what way does it fail?Guest with the device assigned cannot boot up. The guest disappeared when I run "xl create guest.config". And there have no error output in console, serial and qemu log.> Have you tested with 23300:4b0692880dfa applied? It''s a fix on top of > this change.Yes, I also hit this issue with 23339. Best regards Yang> Thanks, > > Tim. > > > Add allen to CC list. Maybe he can give a more authoritative answer. > > > > 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 = (unsigned long)(t & 0x3fff) << 9; > > + /* > > + * AMD IOMMU: When we share p2m table with iommu, bit 9 - bit 11 > will be > > + * used for iommu hardware to encode next io page level. Bit 59 - bit > 62 > > + * are used for iommu flags, We could not use these bits to store p2m > types. > > + */ > > + flags = (unsigned long)(t & 0x7f) << 12; > > #else > > flags = (t & 0x7UL) << 9; > > #endif > > @@ -1826,6 +1831,9 @@ static mfn_t p2m_gfn_to_mfn_current(stru > > p2mt = p2m_flags_to_type(l1e_get_flags(l1e)); > > ASSERT(l1e_get_pfn(l1e) != INVALID_MFN > || !p2m_is_ram(p2mt)); > > > > + if ( l1e.l1 == 0 ) > > + p2mt = p2m_invalid; > > + > > if ( p2m_flags_to_type(l1e_get_flags(l1e)) > > == 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 as > 0. > > + */ > > typedef enum { > > - p2m_invalid = 0, /* Nothing mapped here */ > > - p2m_ram_rw = 1, /* Normal read/write guest RAM */ > > + p2m_ram_rw = 0, /* Normal read/write guest RAM */ > > + p2m_invalid = 1, /* Nothing mapped here */ > > p2m_ram_logdirty = 2, /* Temporarily read-only for log-dirty */ > > p2m_ram_ro = 3, /* Read-only; writes are silently > dropped */ > > p2m_mmio_dm = 4, /* Reads and write go to the device > model */ > > @@ -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 > will be > > + * used for iommu hardware to encode next io page level. Bit 59 - bit > 62 > > + * are used for iommu flags, We could not use these bits to store p2m > types. > > + */ > > + > > + return (flags >> 12) & 0x7f; > > #else > > return (flags >> 9) & 0x7; > > #endif > > > > > > > > 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. > > > > > > Tim. > > > > > > > best regards > > > > yang > > > > > > > > > -----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 > > > > > > > > > > Hi, > > > > > This patch set implements p2m table sharing for amd iommu. Please > > > > > comment it. > > > > > Thanks, > > > > > Wei > > > > > > > > > > -- > > > > > Advanced Micro Devices GmbH > > > > > Sitz: Dornach, Gemeinde Aschheim, > > > > > Landkreis München Registergericht München, > > > > > HRB Nr. 43632 > > > > > WEEE-Reg-Nr: DE 12919551 > > > > > Geschäftsführer: > > > > > Alberto Bozzo, Andrew Bowd > > > > > > > > > > > > > > > > > > > > > > > > > _______________________________________________ > > > > > Xen-devel mailing list > > > > > Xen-devel@lists.xensource.com > > > > > http://lists.xensource.com/xen-devel > > > > > > -- > > > Tim Deegan <Tim.Deegan@citrix.com> > > > Principal Software Engineer, Xen Platform Team > > > Citrix Systems UK Ltd. (Company #02937203, SL9 0BG) > > -- > Tim Deegan <Tim.Deegan@citrix.com> > Principal Software Engineer, Xen Platform Team > Citrix Systems UK Ltd. (Company #02937203, SL9 0BG)_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Kay, Allen M
2011-May-17 00:13 UTC
RE: [Xen-devel] [RFC PATCH 0/3] AMD IOMMU: Share p2m table with iommu
The problem appear to in part caused by unconditional call of get_insn_bytes() in hvm_function_table (cs# 23238). This function interface is defined for svm but not for vmx. However, it is call unconditionally from hvm_emulate_one(). For some reason it fails silently without any indication that it is dereferencing a null function pointer. 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. What is the appropriate way to handle asymmetric function table definition between svm and vmx? Should the caller always check for whether the function is defined or not before calling it in generic code? Allen -----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 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?> The following patch cause the vt-d fail to work. I suspect that the > change is not appropriate for intel platform.Thank you. In what way does it fail? Have you tested with 23300:4b0692880dfa applied? It''s a fix on top of this change. Thanks, Tim.> Add allen to CC list. Maybe he can give a more authoritative answer. > > 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 = (unsigned long)(t & 0x3fff) << 9; > + /* > + * AMD IOMMU: When we share p2m table with iommu, bit 9 - bit 11 will be > + * used for iommu hardware to encode next io page level. Bit 59 - bit 62 > + * are used for iommu flags, We could not use these bits to store p2m types. > + */ > + flags = (unsigned long)(t & 0x7f) << 12; > #else > flags = (t & 0x7UL) << 9; > #endif > @@ -1826,6 +1831,9 @@ static mfn_t p2m_gfn_to_mfn_current(stru > p2mt = p2m_flags_to_type(l1e_get_flags(l1e)); > ASSERT(l1e_get_pfn(l1e) != INVALID_MFN || !p2m_is_ram(p2mt)); > > + if ( l1e.l1 == 0 ) > + p2mt = p2m_invalid; > + > if ( p2m_flags_to_type(l1e_get_flags(l1e)) > == 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 as 0. > + */ > typedef enum { > - p2m_invalid = 0, /* Nothing mapped here */ > - p2m_ram_rw = 1, /* Normal read/write guest RAM */ > + p2m_ram_rw = 0, /* Normal read/write guest RAM */ > + p2m_invalid = 1, /* Nothing mapped here */ > p2m_ram_logdirty = 2, /* Temporarily read-only for log-dirty */ > p2m_ram_ro = 3, /* Read-only; writes are silently dropped */ > p2m_mmio_dm = 4, /* Reads and write go to the device model */ > @@ -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 will be > + * used for iommu hardware to encode next io page level. Bit 59 - bit 62 > + * are used for iommu flags, We could not use these bits to store p2m types. > + */ > + > + return (flags >> 12) & 0x7f; > #else > return (flags >> 9) & 0x7; > #endif > > > > > 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. > > > > Tim. > > > > > best regards > > > yang > > > > > > > -----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 > > > > > > > > Hi, > > > > This patch set implements p2m table sharing for amd iommu. Please > > > > comment it. > > > > Thanks, > > > > Wei > > > > > > > > -- > > > > Advanced Micro Devices GmbH > > > > Sitz: Dornach, Gemeinde Aschheim, > > > > Landkreis München Registergericht München, > > > > HRB Nr. 43632 > > > > WEEE-Reg-Nr: DE 12919551 > > > > Geschäftsführer: > > > > Alberto Bozzo, Andrew Bowd > > > > > > > > > > > > > > > > > > > > _______________________________________________ > > > > Xen-devel mailing list > > > > Xen-devel@lists.xensource.com > > > > http://lists.xensource.com/xen-devel > > > > -- > > Tim Deegan <Tim.Deegan@citrix.com> > > Principal Software Engineer, Xen Platform Team > > Citrix Systems UK Ltd. (Company #02937203, SL9 0BG)-- Tim Deegan <Tim.Deegan@citrix.com> Principal Software Engineer, Xen Platform Team Citrix Systems UK Ltd. (Company #02937203, SL9 0BG) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Kay, Allen M
2011-May-17 02:21 UTC
RE: [Xen-devel] [RFC PATCH 0/3] AMD IOMMU: Share p2m table with iommu
Looking closer, I have found there is indeed a check in hvm_get_insn_bytes().
However, it should return 1 instead of 0 for hvm_funcs.get_insn_bytes undefined
case so that original code gets called.
return (hvm_funcs.get_insn_bytes ? hvm_funcs.get_insn_bytes(v, buf) : 0);
Should be:
return (hvm_funcs.get_insn_bytes ? hvm_funcs.get_insn_bytes(v, buf) : 1);
-----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
The problem appear to in part caused by unconditional call of get_insn_bytes()
in hvm_function_table (cs# 23238). This function interface is defined for svm
but not for vmx. However, it is call unconditionally from hvm_emulate_one().
For some reason it fails silently without any indication that it is
dereferencing a null function pointer.
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.
What is the appropriate way to handle asymmetric function table definition
between svm and vmx? Should the caller always check for whether the function is
defined or not before calling it in generic code?
Allen
-----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
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?
> The following patch cause the vt-d fail to work. I suspect that the
> change is not appropriate for intel platform.
Thank you. In what way does it fail?
Have you tested with 23300:4b0692880dfa applied? It''s a fix on top of
this change.
Thanks,
Tim.
> Add allen to CC list. Maybe he can give a more authoritative answer.
>
> 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 = (unsigned long)(t & 0x3fff) << 9;
> + /*
> + * AMD IOMMU: When we share p2m table with iommu, bit 9 - bit 11 will
be
> + * used for iommu hardware to encode next io page level. Bit 59 - bit
62
> + * are used for iommu flags, We could not use these bits to store p2m
types.
> + */
> + flags = (unsigned long)(t & 0x7f) << 12;
> #else
> flags = (t & 0x7UL) << 9;
> #endif
> @@ -1826,6 +1831,9 @@ static mfn_t p2m_gfn_to_mfn_current(stru
> p2mt = p2m_flags_to_type(l1e_get_flags(l1e));
> ASSERT(l1e_get_pfn(l1e) != INVALID_MFN || !p2m_is_ram(p2mt));
>
> + if ( l1e.l1 == 0 )
> + p2mt = p2m_invalid;
> +
> if ( p2m_flags_to_type(l1e_get_flags(l1e))
> == 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 as
0.
> + */
> typedef enum {
> - p2m_invalid = 0, /* Nothing mapped here */
> - p2m_ram_rw = 1, /* Normal read/write guest RAM */
> + p2m_ram_rw = 0, /* Normal read/write guest RAM */
> + p2m_invalid = 1, /* Nothing mapped here */
> p2m_ram_logdirty = 2, /* Temporarily read-only for log-dirty */
> p2m_ram_ro = 3, /* Read-only; writes are silently dropped
*/
> p2m_mmio_dm = 4, /* Reads and write go to the device model
*/
> @@ -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 will
be
> + * used for iommu hardware to encode next io page level. Bit 59 - bit
62
> + * are used for iommu flags, We could not use these bits to store p2m
types.
> + */
> +
> + return (flags >> 12) & 0x7f;
> #else
> return (flags >> 9) & 0x7;
> #endif
>
> >
> > 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.
> >
> > Tim.
> >
> > > best regards
> > > yang
> > >
> > > > -----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
> > > >
> > > > Hi,
> > > > This patch set implements p2m table sharing for amd iommu.
Please
> > > > comment it.
> > > > Thanks,
> > > > Wei
> > > >
> > > > --
> > > > Advanced Micro Devices GmbH
> > > > Sitz: Dornach, Gemeinde Aschheim,
> > > > Landkreis München Registergericht München,
> > > > HRB Nr. 43632
> > > > WEEE-Reg-Nr: DE 12919551
> > > > Geschäftsführer:
> > > > Alberto Bozzo, Andrew Bowd
> > > >
> > > >
> > > >
> > > >
> > > > _______________________________________________
> > > > Xen-devel mailing list
> > > > Xen-devel@lists.xensource.com
> > > > http://lists.xensource.com/xen-devel
> >
> > --
> > Tim Deegan <Tim.Deegan@citrix.com>
> > Principal Software Engineer, Xen Platform Team
> > Citrix Systems UK Ltd. (Company #02937203, SL9 0BG)
--
Tim Deegan <Tim.Deegan@citrix.com>
Principal Software Engineer, Xen Platform Team
Citrix Systems UK Ltd. (Company #02937203, SL9 0BG)
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Keir Fraser
2011-May-17 07:51 UTC
Re: [Xen-devel] [RFC PATCH 0/3] AMD IOMMU: Share p2m table with iommu
On 17/05/2011 03:21, "Kay, Allen M" <allen.m.kay@intel.com> wrote:> Looking closer, I have found there is indeed a check in hvm_get_insn_bytes(). > However, it should return 1 instead of 0 for hvm_funcs.get_insn_bytes > undefined case so that original code gets called. > > return (hvm_funcs.get_insn_bytes ? hvm_funcs.get_insn_bytes(v, buf) : 0); > > Should be: > > return (hvm_funcs.get_insn_bytes ? hvm_funcs.get_insn_bytes(v, buf) : 1);Ummm, no, it''s correct as it is. -- Keir> > -----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 > > The problem appear to in part caused by unconditional call of get_insn_bytes() > in hvm_function_table (cs# 23238). This function interface is defined for svm > but not for vmx. However, it is call unconditionally from hvm_emulate_one(). > For some reason it fails silently without any indication that it is > dereferencing a null function pointer. > > 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. > > What is the appropriate way to handle asymmetric function table definition > between svm and vmx? Should the caller always check for whether the function > is defined or not before calling it in generic code? > > Allen > > -----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 > > 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? > >> The following patch cause the vt-d fail to work. I suspect that the >> change is not appropriate for intel platform. > > Thank you. In what way does it fail? > > Have you tested with 23300:4b0692880dfa applied? It''s a fix on top of > this change. > > Thanks, > > Tim. > >> Add allen to CC list. Maybe he can give a more authoritative answer. >> >> 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 = (unsigned long)(t & 0x3fff) << 9; >> + /* >> + * AMD IOMMU: When we share p2m table with iommu, bit 9 - bit 11 will be >> + * used for iommu hardware to encode next io page level. Bit 59 - bit 62 >> + * are used for iommu flags, We could not use these bits to store p2m >> types. >> + */ >> + flags = (unsigned long)(t & 0x7f) << 12; >> #else >> flags = (t & 0x7UL) << 9; >> #endif >> @@ -1826,6 +1831,9 @@ static mfn_t p2m_gfn_to_mfn_current(stru >> p2mt = p2m_flags_to_type(l1e_get_flags(l1e)); >> ASSERT(l1e_get_pfn(l1e) != INVALID_MFN || !p2m_is_ram(p2mt)); >> >> + if ( l1e.l1 == 0 ) >> + p2mt = p2m_invalid; >> + >> if ( p2m_flags_to_type(l1e_get_flags(l1e)) >> == 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 as 0. >> + */ >> typedef enum { >> - p2m_invalid = 0, /* Nothing mapped here */ >> - p2m_ram_rw = 1, /* Normal read/write guest RAM */ >> + p2m_ram_rw = 0, /* Normal read/write guest RAM */ >> + p2m_invalid = 1, /* Nothing mapped here */ >> p2m_ram_logdirty = 2, /* Temporarily read-only for log-dirty */ >> p2m_ram_ro = 3, /* Read-only; writes are silently dropped */ >> p2m_mmio_dm = 4, /* Reads and write go to the device model */ >> @@ -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 will be >> + * used for iommu hardware to encode next io page level. Bit 59 - bit 62 >> + * are used for iommu flags, We could not use these bits to store p2m >> types. >> + */ >> + >> + return (flags >> 12) & 0x7f; >> #else >> return (flags >> 9) & 0x7; >> #endif >> >>> >>> 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. >>> >>> Tim. >>> >>>> best regards >>>> yang >>>> >>>>> -----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 >>>>> >>>>> Hi, >>>>> This patch set implements p2m table sharing for amd iommu. Please >>>>> comment it. >>>>> Thanks, >>>>> Wei >>>>> >>>>> -- >>>>> Advanced Micro Devices GmbH >>>>> Sitz: Dornach, Gemeinde Aschheim, >>>>> Landkreis München Registergericht München, >>>>> HRB Nr. 43632 >>>>> WEEE-Reg-Nr: DE 12919551 >>>>> Geschäftsführer: >>>>> Alberto Bozzo, Andrew Bowd >>>>> >>>>> >>>>> >>>>> >>>>> _______________________________________________ >>>>> Xen-devel mailing list >>>>> Xen-devel@lists.xensource.com >>>>> http://lists.xensource.com/xen-devel >>> >>> -- >>> Tim Deegan <Tim.Deegan@citrix.com> >>> Principal Software Engineer, Xen Platform Team >>> Citrix Systems UK Ltd. (Company #02937203, SL9 0BG)_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2011-May-17 07:55 UTC
Re: [Xen-devel] [RFC PATCH 0/3] AMD IOMMU: Share p2m table with iommu
On 17/05/2011 01:13, "Kay, Allen M" <allen.m.kay@intel.com> wrote:> The problem appear to in part caused by unconditional call of get_insn_bytes() > in hvm_function_table (cs# 23238). This function interface is defined for svm > but not for vmx. However, it is call unconditionally from hvm_emulate_one(). > For some reason it fails silently without any indication that it is > dereferencing a null function pointer.As you subsequently noted, this is not true.> 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. > > What is the appropriate way to handle asymmetric function table definition > between svm and vmx? Should the caller always check for whether the function > is defined or not before calling it in generic code?If needed, a NULL check in the calling wrapper would be appropriate. But be aware that every apparently naked call is almost certainly protected by a nestedhvm_enabled(d) check, which would never be TRUE on Intel. IOW You haven''t actually found any bugs yet. :-) -- Keir> Allen > > -----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 > > 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? > >> The following patch cause the vt-d fail to work. I suspect that the >> change is not appropriate for intel platform. > > Thank you. In what way does it fail? > > Have you tested with 23300:4b0692880dfa applied? It''s a fix on top of > this change. > > Thanks, > > Tim. > >> Add allen to CC list. Maybe he can give a more authoritative answer. >> >> 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 = (unsigned long)(t & 0x3fff) << 9; >> + /* >> + * AMD IOMMU: When we share p2m table with iommu, bit 9 - bit 11 will be >> + * used for iommu hardware to encode next io page level. Bit 59 - bit 62 >> + * are used for iommu flags, We could not use these bits to store p2m >> types. >> + */ >> + flags = (unsigned long)(t & 0x7f) << 12; >> #else >> flags = (t & 0x7UL) << 9; >> #endif >> @@ -1826,6 +1831,9 @@ static mfn_t p2m_gfn_to_mfn_current(stru >> p2mt = p2m_flags_to_type(l1e_get_flags(l1e)); >> ASSERT(l1e_get_pfn(l1e) != INVALID_MFN || !p2m_is_ram(p2mt)); >> >> + if ( l1e.l1 == 0 ) >> + p2mt = p2m_invalid; >> + >> if ( p2m_flags_to_type(l1e_get_flags(l1e)) >> == 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 as 0. >> + */ >> typedef enum { >> - p2m_invalid = 0, /* Nothing mapped here */ >> - p2m_ram_rw = 1, /* Normal read/write guest RAM */ >> + p2m_ram_rw = 0, /* Normal read/write guest RAM */ >> + p2m_invalid = 1, /* Nothing mapped here */ >> p2m_ram_logdirty = 2, /* Temporarily read-only for log-dirty */ >> p2m_ram_ro = 3, /* Read-only; writes are silently dropped */ >> p2m_mmio_dm = 4, /* Reads and write go to the device model */ >> @@ -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 will be >> + * used for iommu hardware to encode next io page level. Bit 59 - bit 62 >> + * are used for iommu flags, We could not use these bits to store p2m >> types. >> + */ >> + >> + return (flags >> 12) & 0x7f; >> #else >> return (flags >> 9) & 0x7; >> #endif >> >>> >>> 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. >>> >>> Tim. >>> >>>> best regards >>>> yang >>>> >>>>> -----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 >>>>> >>>>> Hi, >>>>> This patch set implements p2m table sharing for amd iommu. Please >>>>> comment it. >>>>> Thanks, >>>>> Wei >>>>> >>>>> -- >>>>> Advanced Micro Devices GmbH >>>>> Sitz: Dornach, Gemeinde Aschheim, >>>>> Landkreis München Registergericht München, >>>>> HRB Nr. 43632 >>>>> WEEE-Reg-Nr: DE 12919551 >>>>> Geschäftsführer: >>>>> Alberto Bozzo, Andrew Bowd >>>>> >>>>> >>>>> >>>>> >>>>> _______________________________________________ >>>>> Xen-devel mailing list >>>>> Xen-devel@lists.xensource.com >>>>> http://lists.xensource.com/xen-devel >>> >>> -- >>> Tim Deegan <Tim.Deegan@citrix.com> >>> Principal Software Engineer, Xen Platform Team >>> Citrix Systems UK Ltd. (Company #02937203, SL9 0BG)_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Kay, Allen M
2011-May-21 00:51 UTC
RE: [Xen-devel] [RFC PATCH 0/3] AMD IOMMU: Share p2m table with iommu
Yang is correct. I was confused by what appears to be QEMU VGA or xl related
issues in older changesets.
The common code that caused problem is the following.
typedef enum {
- p2m_invalid = 0, /* Nothing mapped here */
- p2m_ram_rw = 1, /* Normal read/write guest RAM */
+ p2m_ram_rw = 0, /* Normal read/write guest RAM */
+ p2m_invalid = 1, /* Nothing mapped here */
With the above change, guest with device direct assignment fails to boot. QEMU
VGA displays some weird color patterns. We also see the following error
messages during first guest boot:
....
(XEN) memory.c:196:d0 Bad page free for domain 1
(XEN) mm.c:2137:d0 Error pfn 0: rd=ffff8301c94c6000, od=ffff8301d8039000, caf=80
00000000000001, taf=7400000000000001
(XEN) memory.c:196:d0 Bad page free for domain 1
(XEN) mm.c:2137:d0 Error pfn 0: rd=ffff8301c94c6000, od=ffff8301d8039000, caf=80
00000000000001, taf=7400000000000001
(XEN) memory.c:196:d0 Bad page free for domain 1
(XEN) mm.c:2137:d0 Error pfn 0: rd=ffff8301c94c6000, od=ffff8301d8039000, caf=80
00000000000001, taf=7400000000000001
...
If I back out the above change, the latest changeset works again so this is the
only change that causes the problem. I remember we had to do a lot of special
treatment of p2m_invlid quite a bit during initial enabling of vt-d device
passthrough.
Allen
-----Original Message-----
From: Zhang, Yang Z
Sent: Monday, May 16, 2011 1:36 AM
To: Tim Deegan
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
> What''s the failure mode? Or better, what change in the common
code are
> you objecting to?
The following patch cause the vt-d fail to work. I suspect that the change is
not appropriate for intel platform.
Add allen to CC list. Maybe he can give a more authoritative answer.
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 = (unsigned long)(t & 0x3fff) << 9;
+ /*
+ * AMD IOMMU: When we share p2m table with iommu, bit 9 - bit 11 will be
+ * used for iommu hardware to encode next io page level. Bit 59 - bit 62
+ * are used for iommu flags, We could not use these bits to store p2m
types.
+ */
+ flags = (unsigned long)(t & 0x7f) << 12;
#else
flags = (t & 0x7UL) << 9;
#endif
@@ -1826,6 +1831,9 @@ static mfn_t p2m_gfn_to_mfn_current(stru
p2mt = p2m_flags_to_type(l1e_get_flags(l1e));
ASSERT(l1e_get_pfn(l1e) != INVALID_MFN || !p2m_is_ram(p2mt));
+ if ( l1e.l1 == 0 )
+ p2mt = p2m_invalid;
+
if ( p2m_flags_to_type(l1e_get_flags(l1e))
== 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 as 0.
+ */
typedef enum {
- p2m_invalid = 0, /* Nothing mapped here */
- p2m_ram_rw = 1, /* Normal read/write guest RAM */
+ p2m_ram_rw = 0, /* Normal read/write guest RAM */
+ p2m_invalid = 1, /* Nothing mapped here */
p2m_ram_logdirty = 2, /* Temporarily read-only for log-dirty */
p2m_ram_ro = 3, /* Read-only; writes are silently dropped */
p2m_mmio_dm = 4, /* Reads and write go to the device model */
@@ -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 will be
+ * used for iommu hardware to encode next io page level. Bit 59 - bit 62
+ * are used for iommu flags, We could not use these bits to store p2m
types.
+ */
+
+ return (flags >> 12) & 0x7f;
#else
return (flags >> 9) & 0x7;
#endif
>
> 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.
>
> Tim.
>
> > best regards
> > yang
> >
> > > -----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
> > >
> > > Hi,
> > > This patch set implements p2m table sharing for amd iommu. Please
> > > comment it.
> > > Thanks,
> > > Wei
> > >
> > > --
> > > Advanced Micro Devices GmbH
> > > Sitz: Dornach, Gemeinde Aschheim,
> > > Landkreis München Registergericht München,
> > > HRB Nr. 43632
> > > WEEE-Reg-Nr: DE 12919551
> > > Geschäftsführer:
> > > Alberto Bozzo, Andrew Bowd
> > >
> > >
> > >
> > >
> > > _______________________________________________
> > > Xen-devel mailing list
> > > Xen-devel@lists.xensource.com
> > > http://lists.xensource.com/xen-devel
>
> --
> Tim Deegan <Tim.Deegan@citrix.com>
> Principal Software Engineer, Xen Platform Team
> Citrix Systems UK Ltd. (Company #02937203, SL9 0BG)
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Tim Deegan
2011-May-23 10:58 UTC
Re: [Xen-devel] [RFC PATCH 0/3] AMD IOMMU: Share p2m table with iommu
Hi, At 01:51 +0100 on 21 May (1305942710), Kay, Allen M wrote:> The common code that caused problem is the following. > > typedef enum { > - p2m_invalid = 0, /* Nothing mapped here */ > - p2m_ram_rw = 1, /* Normal read/write guest RAM */ > + p2m_ram_rw = 0, /* Normal read/write guest RAM */ > + p2m_invalid = 1, /* Nothing mapped here */ > > With the above change, guest with device direct assignment fails to > boot. QEMU VGA displays some weird color patterns.Unfortunately this change seems to be necessary for AMD IOMMU to share pagetables with the p2m. I''d rather we didn''t have it, because it means empty ptes look like RAM mappings of frame 0. :( Wei, is there any way we can reorganise the AMD IOMMU pagetables so we can store the p2m type somewhere that''s not required to be zero? If not, I''m inclined to revert the p2m-sharing for AMD IOMMUs, since at the very least we''d like to be able to handle types other than ram_rw (e.g. ram_ro). In the meantime, Allen, does the attached patch make things any better for you? Cheers, Tim. -- Tim Deegan <Tim.Deegan@citrix.com> Principal Software Engineer, Xen Platform Team Citrix Systems UK Ltd. (Company #02937203, SL9 0BG) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Wei Wang2
2011-May-23 12:08 UTC
Re: [Xen-devel] [RFC PATCH 0/3] AMD IOMMU: Share p2m table with iommu
On Monday 23 May 2011 12:58:00 Tim Deegan wrote:> Hi, > > At 01:51 +0100 on 21 May (1305942710), Kay, Allen M wrote: > > The common code that caused problem is the following. > > > > typedef enum { > > - p2m_invalid = 0, /* Nothing mapped here */ > > - p2m_ram_rw = 1, /* Normal read/write guest RAM */ > > + p2m_ram_rw = 0, /* Normal read/write guest RAM */ > > + p2m_invalid = 1, /* Nothing mapped here */ > > > > With the above change, guest with device direct assignment fails to > > boot. QEMU VGA displays some weird color patterns. > > Unfortunately this change seems to be necessary for AMD IOMMU to share > pagetables with the p2m. I''d rather we didn''t have it, because it means > empty ptes look like RAM mappings of frame 0. :( > > Wei, is there any way we can reorganise the AMD IOMMU pagetables so we > can store the p2m type somewhere that''s not required to be zero? If > not, I''m inclined to revert the p2m-sharing for AMD IOMMUs, since at the > very least we''d like to be able to handle types other than ram_rw > (e.g. ram_ro).Tim, Theoretically, we just need to keep bit 52 - bit 58 all zero for valid dma translation entry. Probably we could define ram_rw as 11000000000b, which is the valid r/w permission for iommu and leaves bit 52 - 58 zero? Thanks, Wei> In the meantime, Allen, does the attached patch make things any better > for you? > > Cheers, > > Tim._______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Tim Deegan
2011-May-23 13:19 UTC
Re: [Xen-devel] [RFC PATCH 0/3] AMD IOMMU: Share p2m table with iommu
Hi, At 13:08 +0100 on 23 May (1306156129), Wei Wang2 wrote:> > Unfortunately this change seems to be necessary for AMD IOMMU to share > > pagetables with the p2m. I''d rather we didn''t have it, because it means > > empty ptes look like RAM mappings of frame 0. :( > > > > Wei, is there any way we can reorganise the AMD IOMMU pagetables so we > > can store the p2m type somewhere that''s not required to be zero? If > > not, I''m inclined to revert the p2m-sharing for AMD IOMMUs, since at the > > very least we''d like to be able to handle types other than ram_rw > > (e.g. ram_ro). > > Theoretically, we just need to keep bit 52 - bit 58 all zero for valid dma > translation entry. Probably we could define ram_rw as 11000000000b, > which is the valid r/w permission for iommu and leaves bit 52 - 58 zero?Ugh; no, that will break EPT as well, and restricts us to only one accessible type. It looks like there are no bits that are available in both normal pagetable and IOMMU pagetables. How inconvenient. So our only options are to harden the rest of the p2m code against blank entries looking like RAM, or to avoid sharing pagetables between p2m and AMD IOMMU. :( I guess that depends on how much of a PITA it''ll be to track down the rest of the places where EPT code trips over itself. Maybe we should replace the clear_page() in allocating p2m pages with a loop that explicitly makes everything p2m_invalid. It''s not a terribly hot path, after all. But even if we do that, don''t you want read-only and grant-mapped memory to work with the IOMMU? Tim. -- Tim Deegan <Tim.Deegan@citrix.com> Principal Software Engineer, Xen Platform Team Citrix Systems UK Ltd. (Company #02937203, SL9 0BG) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Zhang, Yang Z
2011-May-23 13:33 UTC
RE: [Xen-devel] [RFC PATCH 0/3] AMD IOMMU: Share p2m table with iommu
Another question? Does this change ok? How to covert the p2m_type whose value
great than 7 to flags, like the type p2m_ram_shared which equal to 13?
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 flags;
#ifdef __x86_64__
- flags = (unsigned long)(t & 0x3fff) << 9;
+ /*
+ * AMD IOMMU: When we share p2m table with iommu, bit 9 - bit 11 will be
+ * used for iommu hardware to encode next io page level. Bit 59 - bit 62
+ * are used for iommu flags, We could not use these bits to store p2m
types.
+ */
+ flags = (unsigned long)(t & 0x7f) << 12;
best regards
yang
> -----Original Message-----
> From: Tim Deegan [mailto:Tim.Deegan@citrix.com]
> Sent: Monday, May 23, 2011 6:58 PM
> To: Kay, Allen M
> Cc: Zhang, Yang Z; Wei Wang; xen-devel@lists.xensource.com
> Subject: Re: [Xen-devel] [RFC PATCH 0/3] AMD IOMMU: Share p2m table with
> iommu
>
> Hi,
>
> At 01:51 +0100 on 21 May (1305942710), Kay, Allen M wrote:
> > The common code that caused problem is the following.
> >
> > typedef enum {
> > - p2m_invalid = 0, /* Nothing mapped here */
> > - p2m_ram_rw = 1, /* Normal read/write guest RAM */
> > + p2m_ram_rw = 0, /* Normal read/write guest RAM */
> > + p2m_invalid = 1, /* Nothing mapped here */
> >
> > With the above change, guest with device direct assignment fails to
> > boot. QEMU VGA displays some weird color patterns.
>
> Unfortunately this change seems to be necessary for AMD IOMMU to share
> pagetables with the p2m. I''d rather we didn''t have it,
because it means
> empty ptes look like RAM mappings of frame 0. :(
>
> Wei, is there any way we can reorganise the AMD IOMMU pagetables so we
> can store the p2m type somewhere that''s not required to be zero?
If not, I''m
> inclined to revert the p2m-sharing for AMD IOMMUs, since at the very least
> we''d like to be able to handle types other than ram_rw (e.g.
ram_ro).
>
> In the meantime, Allen, does the attached patch make things any better for
> you?
>
> Cheers,
>
> Tim.
>
> --
> Tim Deegan <Tim.Deegan@citrix.com>
> Principal Software Engineer, Xen Platform Team Citrix Systems UK Ltd.
> (Company #02937203, SL9 0BG)
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Tim Deegan
2011-May-23 13:40 UTC
Re: [Xen-devel] [RFC PATCH 0/3] AMD IOMMU: Share p2m table with iommu
At 14:33 +0100 on 23 May (1306161213), Zhang, Yang Z wrote:> Another question? Does this change ok? How to covert the p2m_type > whose value great than 7 to flags, like the type p2m_ram_shared which > equal to 13?The mask is 7F, not 7. Tim.> 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 flags; > #ifdef __x86_64__ > - flags = (unsigned long)(t & 0x3fff) << 9; > + /* > + * AMD IOMMU: When we share p2m table with iommu, bit 9 - bit 11 will be > + * used for iommu hardware to encode next io page level. Bit 59 - bit 62 > + * are used for iommu flags, We could not use these bits to store p2m types. > + */ > + flags = (unsigned long)(t & 0x7f) << 12; > > best regards > yang > > > > -----Original Message----- > > From: Tim Deegan [mailto:Tim.Deegan@citrix.com] > > Sent: Monday, May 23, 2011 6:58 PM > > To: Kay, Allen M > > Cc: Zhang, Yang Z; Wei Wang; xen-devel@lists.xensource.com > > Subject: Re: [Xen-devel] [RFC PATCH 0/3] AMD IOMMU: Share p2m table with > > iommu > > > > Hi, > > > > At 01:51 +0100 on 21 May (1305942710), Kay, Allen M wrote: > > > The common code that caused problem is the following. > > > > > > typedef enum { > > > - p2m_invalid = 0, /* Nothing mapped here */ > > > - p2m_ram_rw = 1, /* Normal read/write guest RAM */ > > > + p2m_ram_rw = 0, /* Normal read/write guest RAM */ > > > + p2m_invalid = 1, /* Nothing mapped here */ > > > > > > With the above change, guest with device direct assignment fails to > > > boot. QEMU VGA displays some weird color patterns. > > > > Unfortunately this change seems to be necessary for AMD IOMMU to share > > pagetables with the p2m. I''d rather we didn''t have it, because it means > > empty ptes look like RAM mappings of frame 0. :( > > > > Wei, is there any way we can reorganise the AMD IOMMU pagetables so we > > can store the p2m type somewhere that''s not required to be zero? If not, I''m > > inclined to revert the p2m-sharing for AMD IOMMUs, since at the very least > > we''d like to be able to handle types other than ram_rw (e.g. ram_ro). > > > > In the meantime, Allen, does the attached patch make things any better for > > you? > > > > Cheers, > > > > Tim. > > > > -- > > Tim Deegan <Tim.Deegan@citrix.com> > > Principal Software Engineer, Xen Platform Team Citrix Systems UK Ltd. > > (Company #02937203, SL9 0BG)-- Tim Deegan <Tim.Deegan@citrix.com> Principal Software Engineer, Xen Platform Team Citrix Systems UK Ltd. (Company #02937203, SL9 0BG) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Zhang, Yang Z
2011-May-23 13:46 UTC
RE: [Xen-devel] [RFC PATCH 0/3] AMD IOMMU: Share p2m table with iommu
No, I mean 0x3fff doesn''t mask the low 14 bits but 0x7f only leave the low 7 bits. How I get the p2m_type which between 8 to 14 bit? best regards yang> -----Original Message----- > From: Tim Deegan [mailto:Tim.Deegan@citrix.com] > Sent: Monday, May 23, 2011 9:41 PM > To: Zhang, Yang Z > Cc: Kay, Allen M; Wei Wang; xen-devel@lists.xensource.com > Subject: Re: [Xen-devel] [RFC PATCH 0/3] AMD IOMMU: Share p2m table with > iommu > > At 14:33 +0100 on 23 May (1306161213), Zhang, Yang Z wrote: > > Another question? Does this change ok? How to covert the p2m_type > > whose value great than 7 to flags, like the type p2m_ram_shared which > > equal to 13? > > The mask is 7F, not 7. > > Tim. > > > 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 flags; > > #ifdef __x86_64__ > > - flags = (unsigned long)(t & 0x3fff) << 9; > > + /* > > + * AMD IOMMU: When we share p2m table with iommu, bit 9 - bit 11 > will be > > + * used for iommu hardware to encode next io page level. Bit 59 - bit > 62 > > + * are used for iommu flags, We could not use these bits to store p2m > types. > > + */ > > + flags = (unsigned long)(t & 0x7f) << 12; > > > > best regards > > yang > > > > > > > -----Original Message----- > > > From: Tim Deegan [mailto:Tim.Deegan@citrix.com] > > > Sent: Monday, May 23, 2011 6:58 PM > > > To: Kay, Allen M > > > Cc: Zhang, Yang Z; Wei Wang; xen-devel@lists.xensource.com > > > Subject: Re: [Xen-devel] [RFC PATCH 0/3] AMD IOMMU: Share p2m table > with > > > iommu > > > > > > Hi, > > > > > > At 01:51 +0100 on 21 May (1305942710), Kay, Allen M wrote: > > > > The common code that caused problem is the following. > > > > > > > > typedef enum { > > > > - p2m_invalid = 0, /* Nothing mapped here */ > > > > - p2m_ram_rw = 1, /* Normal read/write guest RAM > */ > > > > + p2m_ram_rw = 0, /* Normal read/write guest RAM > */ > > > > + p2m_invalid = 1, /* Nothing mapped here */ > > > > > > > > With the above change, guest with device direct assignment fails to > > > > boot. QEMU VGA displays some weird color patterns. > > > > > > Unfortunately this change seems to be necessary for AMD IOMMU to share > > > pagetables with the p2m. I''d rather we didn''t have it, because it means > > > empty ptes look like RAM mappings of frame 0. :( > > > > > > Wei, is there any way we can reorganise the AMD IOMMU pagetables so > we > > > can store the p2m type somewhere that''s not required to be zero? If not, > I''m > > > inclined to revert the p2m-sharing for AMD IOMMUs, since at the very least > > > we''d like to be able to handle types other than ram_rw (e.g. ram_ro). > > > > > > In the meantime, Allen, does the attached patch make things any better for > > > you? > > > > > > Cheers, > > > > > > Tim. > > > > > > -- > > > Tim Deegan <Tim.Deegan@citrix.com> > > > Principal Software Engineer, Xen Platform Team Citrix Systems UK Ltd. > > > (Company #02937203, SL9 0BG) > > -- > Tim Deegan <Tim.Deegan@citrix.com> > Principal Software Engineer, Xen Platform Team > Citrix Systems UK Ltd. (Company #02937203, SL9 0BG)_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Tim Deegan
2011-May-23 14:27 UTC
Re: [Xen-devel] [RFC PATCH 0/3] AMD IOMMU: Share p2m table with iommu
At 14:46 +0100 on 23 May (1306162019), Zhang, Yang Z wrote:> No, I mean 0x3fff doesn''t mask the low 14 bits but 0x7f only leave the > low 7 bits. How I get the p2m_type which between 8 to 14 bit?p2m_types lie between 0 and 14, i.e. four bits. Cutting from 14 bits to 7 bits still leaves space for another 113 types. Tim.> best regards > yang > > > > -----Original Message----- > > From: Tim Deegan [mailto:Tim.Deegan@citrix.com] > > Sent: Monday, May 23, 2011 9:41 PM > > To: Zhang, Yang Z > > Cc: Kay, Allen M; Wei Wang; xen-devel@lists.xensource.com > > Subject: Re: [Xen-devel] [RFC PATCH 0/3] AMD IOMMU: Share p2m table with > > iommu > > > > At 14:33 +0100 on 23 May (1306161213), Zhang, Yang Z wrote: > > > Another question? Does this change ok? How to covert the p2m_type > > > whose value great than 7 to flags, like the type p2m_ram_shared which > > > equal to 13? > > > > The mask is 7F, not 7. > > > > Tim. > > > > > 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 flags; > > > #ifdef __x86_64__ > > > - flags = (unsigned long)(t & 0x3fff) << 9; > > > + /* > > > + * AMD IOMMU: When we share p2m table with iommu, bit 9 - bit 11 > > will be > > > + * used for iommu hardware to encode next io page level. Bit 59 - bit > > 62 > > > + * are used for iommu flags, We could not use these bits to store p2m > > types. > > > + */ > > > + flags = (unsigned long)(t & 0x7f) << 12; > > > > > > best regards > > > yang > > > > > > > > > > -----Original Message----- > > > > From: Tim Deegan [mailto:Tim.Deegan@citrix.com] > > > > Sent: Monday, May 23, 2011 6:58 PM > > > > To: Kay, Allen M > > > > Cc: Zhang, Yang Z; Wei Wang; xen-devel@lists.xensource.com > > > > Subject: Re: [Xen-devel] [RFC PATCH 0/3] AMD IOMMU: Share p2m table > > with > > > > iommu > > > > > > > > Hi, > > > > > > > > At 01:51 +0100 on 21 May (1305942710), Kay, Allen M wrote: > > > > > The common code that caused problem is the following. > > > > > > > > > > typedef enum { > > > > > - p2m_invalid = 0, /* Nothing mapped here */ > > > > > - p2m_ram_rw = 1, /* Normal read/write guest RAM > > */ > > > > > + p2m_ram_rw = 0, /* Normal read/write guest RAM > > */ > > > > > + p2m_invalid = 1, /* Nothing mapped here */ > > > > > > > > > > With the above change, guest with device direct assignment fails to > > > > > boot. QEMU VGA displays some weird color patterns. > > > > > > > > Unfortunately this change seems to be necessary for AMD IOMMU to share > > > > pagetables with the p2m. I''d rather we didn''t have it, because it means > > > > empty ptes look like RAM mappings of frame 0. :( > > > > > > > > Wei, is there any way we can reorganise the AMD IOMMU pagetables so > > we > > > > can store the p2m type somewhere that''s not required to be zero? If not, > > I''m > > > > inclined to revert the p2m-sharing for AMD IOMMUs, since at the very least > > > > we''d like to be able to handle types other than ram_rw (e.g. ram_ro). > > > > > > > > In the meantime, Allen, does the attached patch make things any better for > > > > you? > > > > > > > > Cheers, > > > > > > > > Tim. > > > > > > > > -- > > > > Tim Deegan <Tim.Deegan@citrix.com> > > > > Principal Software Engineer, Xen Platform Team Citrix Systems UK Ltd. > > > > (Company #02937203, SL9 0BG) > > > > -- > > Tim Deegan <Tim.Deegan@citrix.com> > > Principal Software Engineer, Xen Platform Team > > Citrix Systems UK Ltd. (Company #02937203, SL9 0BG)-- Tim Deegan <Tim.Deegan@citrix.com> Principal Software Engineer, Xen Platform Team Citrix Systems UK Ltd. (Company #02937203, SL9 0BG) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Wei Wang2
2011-May-23 16:13 UTC
Re: [Xen-devel] [RFC PATCH 0/3] AMD IOMMU: Share p2m table with iommu
> Hi, > > At 13:08 +0100 on 23 May (1306156129), Wei Wang2 wrote: > > > Unfortunately this change seems to be necessary for AMD IOMMU to share > > > pagetables with the p2m. I''d rather we didn''t have it, because it > > > means empty ptes look like RAM mappings of frame 0. :( > > > > > > Wei, is there any way we can reorganise the AMD IOMMU pagetables so we > > > can store the p2m type somewhere that''s not required to be zero? If > > > not, I''m inclined to revert the p2m-sharing for AMD IOMMUs, since at > > > the very least we''d like to be able to handle types other than ram_rw > > > (e.g. ram_ro). > > > > Theoretically, we just need to keep bit 52 - bit 58 all zero for valid > > dma translation entry. Probably we could define ram_rw as 11000000000b, > > which is the valid r/w permission for iommu and leaves bit 52 - 58 zero? > > Ugh; no, that will break EPT as well, and restricts us to only one > accessible type. It looks like there are no bits that are available in > both normal pagetable and IOMMU pagetables. How inconvenient.OK, understand. Indeed there are no bits available to use. I am not strictly against reversing this patch, if this causes too much changes for vtd.> So our only options are to harden the rest of the p2m code against > blank entries looking like RAM, or to avoid sharing pagetables between > p2m and AMD IOMMU. :( I guess that depends on how much of a PITA it''ll > be to track down the rest of the places where EPT code trips over > itself. Maybe we should replace the clear_page() in allocating p2m > pages with a loop that explicitly makes everything p2m_invalid. It''s > not a terribly hot path, after all.> But even if we do that, don''t you want read-only and grant-mapped memory > to work with the IOMMU?Ture, we lose grant mapping and RO here. But what is the use case of iommu using RO and grant-mapping in hvm? For hvm, since we could not know which parts of memory are actually used by dma transaction, should it be more safe that only r/w pages are accessed by iommu through p2m? Thanks, Wei> Tim._______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Kay, Allen M
2011-May-24 00:21 UTC
RE: [Xen-devel] [RFC PATCH 0/3] AMD IOMMU: Share p2m table with iommu
> In the meantime, Allen, does the attached patch make things any better for you?Yes, your patch fixed the issue ... just missing a ";". Allen -----Original Message----- From: Tim Deegan [mailto:Tim.Deegan@citrix.com] Sent: Monday, May 23, 2011 3:58 AM To: Kay, Allen M Cc: Zhang, Yang Z; Wei Wang; xen-devel@lists.xensource.com Subject: Re: [Xen-devel] [RFC PATCH 0/3] AMD IOMMU: Share p2m table with iommu Hi, At 01:51 +0100 on 21 May (1305942710), Kay, Allen M wrote:> The common code that caused problem is the following. > > typedef enum { > - p2m_invalid = 0, /* Nothing mapped here */ > - p2m_ram_rw = 1, /* Normal read/write guest RAM */ > + p2m_ram_rw = 0, /* Normal read/write guest RAM */ > + p2m_invalid = 1, /* Nothing mapped here */ > > With the above change, guest with device direct assignment fails to > boot. QEMU VGA displays some weird color patterns.Unfortunately this change seems to be necessary for AMD IOMMU to share pagetables with the p2m. I''d rather we didn''t have it, because it means empty ptes look like RAM mappings of frame 0. :( Wei, is there any way we can reorganise the AMD IOMMU pagetables so we can store the p2m type somewhere that''s not required to be zero? If not, I''m inclined to revert the p2m-sharing for AMD IOMMUs, since at the very least we''d like to be able to handle types other than ram_rw (e.g. ram_ro). In the meantime, Allen, does the attached patch make things any better for you? Cheers, Tim. -- Tim Deegan <Tim.Deegan@citrix.com> Principal Software Engineer, Xen Platform Team Citrix Systems UK Ltd. (Company #02937203, SL9 0BG) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Tim Deegan
2011-May-24 09:13 UTC
Re: [Xen-devel] [RFC PATCH 0/3] AMD IOMMU: Share p2m table with iommu
At 01:21 +0100 on 24 May (1306200100), Kay, Allen M wrote:> > In the meantime, Allen, does the attached patch make things any better for you? > > Yes, your patch fixed the issue ... just missing a ";".Great. I''ve applied the useful part; I think that grants a stay of execution to the AMD p2m-sharing patch. Cheers, Tim. -- Tim Deegan <Tim.Deegan@citrix.com> Principal Software Engineer, Xen Platform Team Citrix Systems UK Ltd. (Company #02937203, SL9 0BG) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel