[RFC PATCH 1/3] AMD IOMMU: p2m table changes

[RFC PATCH 1/3] AMD IOMMU: p2m table changes

[Wei Wang]

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

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





[-- Attachment #2: p2m_1.patch --]
[-- Type: text/plain, Size: 2221 bytes --]

# HG changeset patch
# User Wei Wang <wei.wang2@amd.com>
# Date 1300979908 -3600
# Node ID 6827fd35cf583353a7cb05946cdc5c0f13930f39
# Parent  12f7c7ac7f19e122fa83c16c8c6d9a6700ddc409
AMD IOMMU hardware uses bit 9 - bit 11 to encode lower page levels. Therefore, 
p2m type bits has to be shifted from bit 9 to bit 12 in p2m flags. Also, bit 52
to bit 60 cannot be non-zero for iommu pte. So, I have to swap the definition of 
p2m_ram_rw with p2m_invalid. This patch is tested OK with both SPT and NPT
guests.

Signed-off-by: Wei Wang <wei.wang2@amd.com>

diff -r 12f7c7ac7f19 -r 6827fd35cf58 xen/arch/x86/mm/p2m.c
--- a/xen/arch/x86/mm/p2m.c	Fri Mar 18 17:15:52 2011 +0000
+++ b/xen/arch/x86/mm/p2m.c	Thu Mar 24 16:18:28 2011 +0100
@@ -79,7 +79,7 @@
 {
     unsigned long flags;
 #ifdef __x86_64__
-    flags = (unsigned long)(t & 0x3fff) << 9;
+    flags = (unsigned long)(t & 0x3fff) << 12;
 #else
     flags = (t & 0x7UL) << 9;
 #endif
@@ -1760,6 +1760,9 @@
             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 12f7c7ac7f19 -r 6827fd35cf58 xen/include/asm-x86/p2m.h
--- a/xen/include/asm-x86/p2m.h	Fri Mar 18 17:15:52 2011 +0000
+++ b/xen/include/asm-x86/p2m.h	Thu Mar 24 16:18:28 2011 +0100
@@ -64,8 +64,8 @@
  * 64-bit Xen.
  */
 typedef enum {
-    p2m_invalid = 0,            /* Nothing mapped here */
-    p2m_ram_rw = 1,             /* Normal read/write guest RAM */
+    p2m_invalid = 1,            /* Nothing mapped here */
+    p2m_ram_rw = 0,             /* Normal read/write guest RAM */
     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 */
@@ -312,7 +312,7 @@
 {
     /* Type is stored in the "available" bits */
 #ifdef __x86_64__
-    return (flags >> 9) & 0x3fff;
+    return (flags >> 12) & 0x3fff;
 #else
     return (flags >> 9) & 0x7;
 #endif

[-- 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 1/3] AMD IOMMU: p2m table changes

[Tim Deegan]

> AMD IOMMU hardware uses bit 9 - bit 11 to encode lower page levels. Therefore, 
> p2m type bits has to be shifted from bit 9 to bit 12 in p2m flags.

OK, but you need to add a comment explaining it, so the next person
doesn't try to reuse those three bits. 

> Also, bit 52
> to bit 60 cannot be non-zero for iommu pte. So, I have to swap the definition of 
> p2m_ram_rw with p2m_invalid.

That _definitely_ needs a comment explianing why it's so. 

Also, what's the failure mode if the guest tries to access other types
of memory via the IOMMU?  In the current code the entries will not be
present in the IOMMU table; after this change they'll be present but
malformed.  Is that equivalent?

Cheers,

Tim.

> This patch is tested OK with both SPT and NPT
> guests.
> 
> Signed-off-by: Wei Wang <wei.wang2@amd.com>
> 
> diff -r 12f7c7ac7f19 -r 6827fd35cf58 xen/arch/x86/mm/p2m.c
> --- a/xen/arch/x86/mm/p2m.c	Fri Mar 18 17:15:52 2011 +0000
> +++ b/xen/arch/x86/mm/p2m.c	Thu Mar 24 16:18:28 2011 +0100
> @@ -79,7 +79,7 @@
>  {
>      unsigned long flags;
>  #ifdef __x86_64__
> -    flags = (unsigned long)(t & 0x3fff) << 9;
> +    flags = (unsigned long)(t & 0x3fff) << 12;
>  #else
>      flags = (t & 0x7UL) << 9;
>  #endif
> @@ -1760,6 +1760,9 @@
>              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 12f7c7ac7f19 -r 6827fd35cf58 xen/include/asm-x86/p2m.h
> --- a/xen/include/asm-x86/p2m.h	Fri Mar 18 17:15:52 2011 +0000
> +++ b/xen/include/asm-x86/p2m.h	Thu Mar 24 16:18:28 2011 +0100
> @@ -64,8 +64,8 @@
>   * 64-bit Xen.
>   */
>  typedef enum {
> -    p2m_invalid = 0,            /* Nothing mapped here */
> -    p2m_ram_rw = 1,             /* Normal read/write guest RAM */
> +    p2m_invalid = 1,            /* Nothing mapped here */
> +    p2m_ram_rw = 0,             /* Normal read/write guest RAM */
>      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 */
> @@ -312,7 +312,7 @@
>  {
>      /* Type is stored in the "available" bits */
>  #ifdef __x86_64__
> -    return (flags >> 9) & 0x3fff;
> +    return (flags >> 12) & 0x3fff;
>  #else
>      return (flags >> 9) & 0x7;
>  #endif

> _______________________________________________
> 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 1/3] AMD IOMMU: p2m table changes

[Wei Wang2]

Tim,
On Monday 04 April 2011 12:59:40 Tim Deegan wrote:
> > AMD IOMMU hardware uses bit 9 - bit 11 to encode lower page levels.
> > Therefore, p2m type bits has to be shifted from bit 9 to bit 12 in p2m
> > flags.
>
> OK, but you need to add a comment explaining it, so the next person
> doesn't try to reuse those three bits.
>
> > Also, bit 52
> > to bit 60 cannot be non-zero for iommu pte. So, I have to swap the
> > definition of p2m_ram_rw with p2m_invalid.
>
> That _definitely_ needs a comment explianing why it's so.
OK, I will add that. Thanks for point this out.

> Also, what's the failure mode if the guest tries to access other types
> of memory via the IOMMU?  In the current code the entries will not be
> present in the IOMMU table; after this change they'll be present but
> malformed.  Is that equivalent?
AMD IOMMU hardware will generate IO page faults when guest try to access these 
pages with bit 52 - bit 58 in pte are non-zore. They are reserved bits for 
now. I could not clear present bits for the ptes since this might break other 
p2m functionalities but I should be no problem to clear read/write bits for 
those entries.
Thanks,
Wei
> Cheers,
>
> Tim.
>
> > This patch is tested OK with both SPT and NPT
> > guests.
> >
> > Signed-off-by: Wei Wang <wei.wang2@amd.com>
> >
> > diff -r 12f7c7ac7f19 -r 6827fd35cf58 xen/arch/x86/mm/p2m.c
> > --- a/xen/arch/x86/mm/p2m.c	Fri Mar 18 17:15:52 2011 +0000
> > +++ b/xen/arch/x86/mm/p2m.c	Thu Mar 24 16:18:28 2011 +0100
> > @@ -79,7 +79,7 @@
> >  {
> >      unsigned long flags;
> >  #ifdef __x86_64__
> > -    flags = (unsigned long)(t & 0x3fff) << 9;
> > +    flags = (unsigned long)(t & 0x3fff) << 12;
> >  #else
> >      flags = (t & 0x7UL) << 9;
> >  #endif
> > @@ -1760,6 +1760,9 @@
> >              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 12f7c7ac7f19 -r 6827fd35cf58 xen/include/asm-x86/p2m.h
> > --- a/xen/include/asm-x86/p2m.h	Fri Mar 18 17:15:52 2011 +0000
> > +++ b/xen/include/asm-x86/p2m.h	Thu Mar 24 16:18:28 2011 +0100
> > @@ -64,8 +64,8 @@
> >   * 64-bit Xen.
> >   */
> >  typedef enum {
> > -    p2m_invalid = 0,            /* Nothing mapped here */
> > -    p2m_ram_rw = 1,             /* Normal read/write guest RAM */
> > +    p2m_invalid = 1,            /* Nothing mapped here */
> > +    p2m_ram_rw = 0,             /* Normal read/write guest RAM */
> >      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
> > */ @@ -312,7 +312,7 @@
> >  {
> >      /* Type is stored in the "available" bits */
> >  #ifdef __x86_64__
> > -    return (flags >> 9) & 0x3fff;
> > +    return (flags >> 12) & 0x3fff;
> >  #else
> >      return (flags >> 9) & 0x7;
> >  #endif
> >
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xensource.com
> > http://lists.xensource.com/xen-devel