From mboxrd@z Thu Jan  1 00:00:00 1970
From: "Jan Beulich" <JBeulich@suse.com>
Subject: Re: [PATCH 2/2] altp2m: Implement p2m_get_mem_access
 for altp2m views
Date: Thu, 28 Jan 2016 07:56:09 -0700
Message-ID: <56AA3A1902000078000CC152@prv-mh.provo.novell.com>
References: <1453925201-15926-1-git-send-email-tlengyel@novetta.com>
	<1453925201-15926-2-git-send-email-tlengyel@novetta.com>
	<56AA27D402000078000CC080@prv-mh.provo.novell.com>
	<CAD33N+7=1cRNeJHN-G-DRBiDyzQFtLs9MTEKMEaN6oExV8WcNQ@mail.gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
Received: from mail6.bemta14.messagelabs.com ([193.109.254.103])
	by lists.xen.org with esmtp (Exim 4.72)
	(envelope-from <JBeulich@suse.com>) id 1aOnzY-0005BZ-ED
	for xen-devel@lists.xenproject.org; Thu, 28 Jan 2016 14:56:12 +0000
In-Reply-To: <CAD33N+7=1cRNeJHN-G-DRBiDyzQFtLs9MTEKMEaN6oExV8WcNQ@mail.gmail.com>
Content-Disposition: inline
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: Tamas Lengyel <tlengyel@novetta.com>
Cc: Wei Liu <wei.liu2@citrix.com>, Ian Campbell <ian.campbell@citrix.com>, Razvan Cojocaru <rcojocaru@bitdefender.com>, Stefano Stabellini <stefano.stabellini@eu.citrix.com>, George Dunlap <george.dunlap@eu.citrix.com>, Andrew Cooper <andrew.cooper3@citrix.com>, Ian Jackson <ian.jackson@eu.citrix.com>, Stefano Stabellini <stefano.stabellini@citrix.com>, xen-devel@lists.xenproject.org, Keir Fraser <keir@xen.org>
List-Id: xen-devel@lists.xenproject.org

>>> On 28.01.16 at 15:42, <tlengyel@novetta.com> wrote:
> On Jan 28, 2016 6:38 AM, "Jan Beulich" <JBeulich@suse.com> wrote:
>> >>> On 27.01.16 at 21:06, <tlengyel@novetta.com> wrote:
>> > --- a/xen/arch/x86/mm/p2m.c
>> > +++ b/xen/arch/x86/mm/p2m.c
>> > @@ -1572,7 +1572,9 @@ void p2m_mem_access_emulate_check(struct vcpu *v,
>> >          bool_t violation = 1;
>> >          const struct vm_event_mem_access *data = &rsp->u.mem_access;
>> >
>> > -        if ( p2m_get_mem_access(v->domain, _gfn(data->gfn), &access)
> == 0 )
>> > +        if ( p2m_get_mem_access(v->domain, _gfn(data->gfn),
>> > +                                altp2m_active(v->domain) ?
> vcpu_altp2m(v).p2midx : 0,
>> > +                                &access) == 0 )
>>
>> This looks to be a behavioral change beyond what title and
>> description say, and it's not clear whether that's actually the
>> behavior everyone wants.
> 
> I'm fairly comfident its exactly the expected behavior when one uses
> mem_access in altp2m tables and emulation. Right now because the lack of
> this AFAIK emulation would not work correctly with altp2m. But Razvan
> probably can chime in as he uses this path actively.

But this should then be mentioned in the description.

>> > +    /* altp2m view 0 is treated as the hostp2m */
>> > +    if ( altp2m_idx )
>> > +    {
>> > +        if ( altp2m_idx >= MAX_ALTP2M ||
>> > +             d->arch.altp2m_eptp[altp2m_idx] == INVALID_MFN )
>> > +            return -EINVAL;
>> > +
>> > +        p2m = d->arch.altp2m_p2m[altp2m_idx];
>> > +    }
>> > +    else
>> > +        p2m = hp2m;
>>
>> And I don't see why you need separate p2m and hp2m local
>> variables.
> 
> It was also used above for returning default access, while p2m is the
> actually active one here.

But all that could be done with just the one variable that was already
there.

Jan