WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

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

To: "Kay, Allen M" <allen.m.kay@xxxxxxxxx>, Tim Deegan <Tim.Deegan@xxxxxxxxxx>, "Zhang, Yang Z" <yang.z.zhang@xxxxxxxxx>
Subject: Re: [Xen-devel] [RFC PATCH 0/3] AMD IOMMU: Share p2m table with iommu
From: Keir Fraser <keir.xen@xxxxxxxxx>
Date: Tue, 17 May 2011 08:55:11 +0100
Cc: Wei Wang <wei.wang2@xxxxxxx>, "andre.przywara@xxxxxxx" <andre.przywara@xxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxx>
Delivery-date: Tue, 17 May 2011 00:55:57 -0700
Dkim-signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:user-agent:date:subject:from:to:cc:message-id :thread-topic:thread-index:in-reply-to:mime-version:content-type :content-transfer-encoding; bh=7MVp+1Oe7m1pFN3ivbyLrIM/jLlWxqhb1sfGIcQJ8+Q=; b=a8lv7TID17/fFEDjYXTaBViui0XRc3exzNvsVC3GgY8CUi3VzASx6W4OtnL0HYBywB ixvgS95P6LA2qsuLajTooJgFZywdua/7hXZTl9KwLEd6NBWC6XvK1aat+kMDhtLOX78+ e+74Wpv53HNYU5mTZUYD0SSqC4S265KUdobtg=
Domainkey-signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=user-agent:date:subject:from:to:cc:message-id:thread-topic :thread-index:in-reply-to:mime-version:content-type :content-transfer-encoding; b=doLM0Np1qh39hDmIQLHNfOWJMdFZYBTJo5bZYj4d5tT4Pi591mgOrI2Qky6z5+Gphu 7VmPGVE+mhnmvHfoyRIhO2mRlBn+/cPRZ8sFMIFc9bcrK/QZS9Gk4ZgZbVURpoJqzy/5 HdoEzliVlJcMOUyK+rKQDR3C/KPGMRYG9iKO8=
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <987664A83D2D224EAE907B061CE93D5301CD1984EE@xxxxxxxxxxxxxxxxxxxxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
Thread-index: AcwTpU3rsk0H5RGbQIW+ir2rsHcbWwAf8oZgABCp0JM=
Thread-topic: [Xen-devel] [RFC PATCH 0/3] AMD IOMMU: Share p2m table with iommu
User-agent: Microsoft-Entourage/12.29.0.110113
On 17/05/2011 01:13, "Kay, Allen M" <allen.m.kay@xxxxxxxxx> 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@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)



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