From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Jan Beulich" Subject: Re: [PATCH 2/2] altp2m: Implement p2m_get_mem_access for altp2m views Date: Thu, 28 Jan 2016 06:38:12 -0700 Message-ID: <56AA27D402000078000CC080@prv-mh.provo.novell.com> References: <1453925201-15926-1-git-send-email-tlengyel@novetta.com> <1453925201-15926-2-git-send-email-tlengyel@novetta.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta4.messagelabs.com ([85.158.143.247]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1aOmmC-0006dz-HK for xen-devel@lists.xenproject.org; Thu, 28 Jan 2016 13:38:20 +0000 In-Reply-To: <1453925201-15926-2-git-send-email-tlengyel@novetta.com> Content-Disposition: inline List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Tamas K Lengyel Cc: Wei Liu , Ian Campbell , Razvan Cojocaru , Stefano Stabellini , George Dunlap , Andrew Cooper , Ian Jackson , Stefano Stabellini , xen-devel@lists.xenproject.org, Keir Fraser List-Id: xen-devel@lists.xenproject.org >>> On 27.01.16 at 21:06, 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. > @@ -1918,9 +1920,10 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr, > * Get access type for a gfn. > * If gfn == INVALID_GFN, gets the default access type. > */ > -int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t *access) > +int p2m_get_mem_access(struct domain *d, gfn_t gfn, unsigned long altp2m_idx, > + xenmem_access_t *access) > { > - struct p2m_domain *p2m = p2m_get_hostp2m(d); > + struct p2m_domain *hp2m = p2m_get_hostp2m(d), *p2m = NULL; > p2m_type_t t; > p2m_access_t a; > mfn_t mfn; > @@ -1943,10 +1946,22 @@ int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t *access) > /* If request to get default access. */ > if ( gfn_x(gfn) == INVALID_GFN ) > { > - *access = memaccess[p2m->default_access]; > + *access = memaccess[hp2m->default_access]; > return 0; > } And following the above - why would this not use the altp2m's default access? > + /* 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. > --- a/xen/include/xen/p2m-common.h > +++ b/xen/include/xen/p2m-common.h > @@ -56,6 +56,7 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr, > * Get access type for a gfn. > * If gfn == INVALID_GFN, gets the default access type. > */ > -int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t *access); > +int p2m_get_mem_access(struct domain *d, gfn_t gfn, unsigned long altp2m_idx, > + xenmem_access_t *access); Same question as for patch 1 regarding the "unsigned long" here. Jan