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

["Kay, Allen M" <allen.m.kay@xxxxxxxxx>]

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@xxxxxxxxxx] 
Sent: Monday, May 16, 2011 1:43 AM
To: Zhang, Yang Z
Cc: Wei Wang; xen-devel@xxxxxxxxxxxxxxxxxxx; 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@xxxxxxxxxxxxxxxxxxx
> > > > [mailto:xen-devel-bounces@xxxxxxxxxxxxxxxxxxx] On Behalf Of Wei Wang
> > > > Sent: Friday, March 25, 2011 6:32 PM
> > > > To: xen-devel@xxxxxxxxxxxxxxxxxxx
> > > > 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@xxxxxxxxxxxxxxxxxxx
> > > > http://lists.xensource.com/xen-devel
> > 
> > --
> > Tim Deegan <Tim.Deegan@xxxxxxxxxx>
> > Principal Software Engineer, Xen Platform Team
> > Citrix Systems UK Ltd.  (Company #02937203, SL9 0BG)

-- 
Tim Deegan <Tim.Deegan@xxxxxxxxxx>
Principal Software Engineer, Xen Platform Team
Citrix Systems UK Ltd.  (Company #02937203, SL9 0BG)

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