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

[Keir Fraser <keir.xen@gmail.com>]

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)