[RFC PATCH 0/3] AMD IOMMU: Share p2m table with iommu

[RFC PATCH 0/3] AMD IOMMU: Share p2m table with iommu

[Wei Wang]

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

RE: [RFC PATCH 0/3] AMD IOMMU: Share p2m table with iommu

[Zhang, Yang Z]

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

Re: [RFC PATCH 0/3] AMD IOMMU: Share p2m table with iommu

[Tim Deegan]

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)

RE: [RFC PATCH 0/3] AMD IOMMU: Share p2m table with iommu

[Zhang, Yang Z]

> 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)

Re: [RFC PATCH 0/3] AMD IOMMU: Share p2m table with iommu

[Tim Deegan]

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)

RE: [RFC PATCH 0/3] AMD IOMMU: Share p2m table with iommu

[Zhang, Yang Z]

> 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)

RE: [RFC PATCH 0/3] AMD IOMMU: Share p2m table with iommu

[Kay, Allen M]

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)

RE: [RFC PATCH 0/3] AMD IOMMU: Share p2m table with iommu

[Kay, Allen M]

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)

Re: [RFC PATCH 0/3] AMD IOMMU: Share p2m table with iommu

[Keir Fraser]

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)

Re: [RFC PATCH 0/3] AMD IOMMU: Share p2m table with iommu

[Keir Fraser]

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)

RE: [RFC PATCH 0/3] AMD IOMMU: Share p2m table with iommu

[Kay, Allen M]

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)

Re: [RFC PATCH 0/3] AMD IOMMU: Share p2m table with iommu

[Tim Deegan]

[-- Attachment #1: Type: text/plain, Size: 1257 bytes --]

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)

[-- Attachment #2: x --]
[-- Type: text/plain, Size: 890 bytes --]

diff -r f531ed84b066 xen/arch/x86/mm/p2m-ept.c
--- a/xen/arch/x86/mm/p2m-ept.c	Tue May 17 17:32:19 2011 +0100
+++ b/xen/arch/x86/mm/p2m-ept.c	Mon May 23 11:52:32 2011 +0100
@@ -586,6 +586,11 @@ static mfn_t ept_get_entry(struct p2m_do
         *t = ept_entry->sa_p2mt;
         *a = ept_entry->access;
 
+        if ( p2m_has_emt(ept_entry->sa_p2mt) 
+             && (!is_epte_present(ept_entry) || ept_entry->mfn == 0) )
+            printk("Bad EPT entry %"PRIx64" for GFN %lx\n", 
+                   ept_entry->epte, gfn)
+
         mfn = _mfn(ept_entry->mfn);
         if ( i )
         {
@@ -736,7 +741,7 @@ void ept_change_entry_emt_with_range(str
         uint64_t trunk = 0;
 
         e = ept_get_entry_content(p2m, gfn, &level);
-        if ( !p2m_has_emt(e.sa_p2mt) )
+        if ( !is_epte_present(&e) || !p2m_has_emt(e.sa_p2mt) )
             continue;
 
         order = 0;

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [RFC PATCH 0/3] AMD IOMMU: Share p2m table with iommu

[Wei Wang2]

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.

Re: [RFC PATCH 0/3] AMD IOMMU: Share p2m table with iommu

[Tim Deegan]

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)

RE: [RFC PATCH 0/3] AMD IOMMU: Share p2m table with iommu

[Zhang, Yang Z]

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)

Re: [RFC PATCH 0/3] AMD IOMMU: Share p2m table with iommu

[Tim Deegan]

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)

RE: [RFC PATCH 0/3] AMD IOMMU: Share p2m table with iommu

[Zhang, Yang Z]

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)

Re: [RFC PATCH 0/3] AMD IOMMU: Share p2m table with iommu

[Tim Deegan]

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)

Re: [RFC PATCH 0/3] AMD IOMMU: Share p2m table with iommu

[Wei Wang2]

> 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.

RE: [RFC PATCH 0/3] AMD IOMMU: Share p2m table with iommu

[Kay, Allen M]

> 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)

Re: [RFC PATCH 0/3] AMD IOMMU: Share p2m table with iommu

[Tim Deegan]

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)