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

[Wei Wang2 <wei.wang2@amd.com>]

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