From mboxrd@z Thu Jan  1 00:00:00 1970
From: Suravee Suthikulanit <suravee.suthikulpanit@amd.com>
Subject: Re: [PATCH 1/1 V3] x86/AMD: Fix nested svm crash due to
 assertion in __virt_to_maddr
Date: Mon, 15 Jul 2013 16:17:04 -0500
Message-ID: <51E466D0.7090504@amd.com>
References: <1373564054-4293-1-git-send-email-suravee.suthikulpanit@amd.com>
	<51DFD3EC02000078000E45BB@nat28.tlf.novell.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"; Format="flowed"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <51DFD3EC02000078000E45BB@nat28.tlf.novell.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Jan Beulich <JBeulich@suse.com>
Cc: chegger@amazon.de, Tim Deegan <tim@xen.org>, xen-devel@lists.xen.org
List-Id: xen-devel@lists.xenproject.org

On 7/12/2013 3:01 AM, Jan Beulich wrote:
>>>> On 11.07.13 at 19:34, <suravee.suthikulpanit@amd.com> wrote:
>> --- a/xen/arch/x86/mm/hap/nested_hap.c
>> +++ b/xen/arch/x86/mm/hap/nested_hap.c
>> @@ -191,6 +191,19 @@ out:
>>       return rc;
>>   }
>>   
>> +int
>> +nestedhvm_walk_L0_p2m(struct vcpu *v, paddr_t L1_gpa, paddr_t *L0_gpa)
>> +{
>> +    p2m_type_t p2mt_10;
>> +    unsigned int page_order_10;
>> +    p2m_access_t p2ma_10 = p2m_access_rwx;
> Pointless initializer?
These are mostly part of the required function prototype.  However, I 
noticed that I don't need to specify page order.
>
>> +
>> +    return nestedhap_walk_L0_p2m ( p2m_get_hostp2m(v->domain),
> Extra spaces around "(".
Ah, thanks.
>
>> +                                 L1_gpa, L0_gpa,
>> +                                 &p2mt_10, &p2ma_10, &page_order_10,
>> +                                 0, 0, 0);
> Wouldn't the caller's use of the GPA imply that you want read and
> write access here?
Actually, access_r and access_x is not used in the 
"nestedhap_walk_L0_p2m" function.
Since we are not writing to this GPA, would we need the write access?
>
>> +}
> I'm not clear about the need for this new wrapper: Is it really
> benign to the caller what type, access, and order get returned
> here? Is it really too much of a burden to have the two call
> sites do the call here directly? The more that (see above) you'd
> really need to give the caller control over the access requested?
Ok, I will just making the nestedhap_walk_L0_p2m not static and add the 
prototype in the svm.h then.

> Finally, considering that now you change a file under
> xen/arch/x86/mm/, you should have Cc'ed Tim on the patch
> submission.
Thanks for pointing out.

Suravee