From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tamas K Lengyel Subject: Re: [PATCH for-4.5 v7 07/21] xen: Relocate p2m_mem_access_check into common and refactor it. Date: Thu, 18 Sep 2014 12:47:19 +0200 Message-ID: References: <1410987084-11899-1-git-send-email-tklengyel@sec.in.tum.de> <1410987084-11899-8-git-send-email-tklengyel@sec.in.tum.de> <20140918103908.GC65000@deinos.phlegethon.org> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============8235849415332769339==" Return-path: In-Reply-To: <20140918103908.GC65000@deinos.phlegethon.org> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Tim Deegan Cc: Ian Campbell , Julien Grall , Ian Jackson , "xen-devel@lists.xen.org" , Stefano Stabellini , Andres Lagar-Cavilla , Jan Beulich , Daniel De Graaf , Tamas K Lengyel List-Id: xen-devel@lists.xenproject.org --===============8235849415332769339== Content-Type: multipart/alternative; boundary=089e0149d2e8746f50050354b528 --089e0149d2e8746f50050354b528 Content-Type: text/plain; charset=ISO-8859-1 On Thu, Sep 18, 2014 at 12:39 PM, Tim Deegan wrote: > Hi, > > Thanks for the patch. Tidying this up is a great idea but I'm afraid > it can't be done quite this way. > > At 22:51 +0200 on 17 Sep (1410990670), Tamas K Lengyel wrote: > > The p2m_mem_access_check was used only to automatically promote the > > special-case mem_access rights and to allocate the mem_access message. > > In this patch we abstract the violation check into a common > mem_access_check > > function and change its return value to int. > > > > Return with rc < 0 means the fault wasn't triggered by mem_access > permissions. > > Return with rc == 0 it was but rights were not automatically promoted. > > Return with rc > 0 means mem_access rights had been automatically > promoted. > > > > As part of this patch, we relocate the x86/mm/mm-locks.h header into asm > > as to be able to create a generic inlined p2m_gpfn_lock/unlock wrapper > > that each arch can define based on its locking scheme. > > I'm afraid not. mm-locks.h is hidden away in arch/x86/mm > deliberately, to stop p2m internals from leaking out into the rest of > the code and making it harder to reason about. So the fact that you > had to move it suggests that we need to think about using cleaner > interfaces between the code you're extracting and the rest of the p2m > code... > > > +int mem_access_check(paddr_t gpa, unsigned long gla, struct npfec npfec) > > +{ > > + bool_t violation; > > + mfn_t mfn; > > + p2m_access_t p2ma; > > + p2m_type_t p2mt; > > + mem_event_request_t *req; > > + int rc; > > + struct vcpu *v = current; > > + unsigned long gfn = gpa >> PAGE_SHIFT; > > + struct domain *d = v->domain; > > + struct p2m_domain* p2m = p2m_get_hostp2m(d); > > + > > + /* First, handle rx2rw conversion automatically. > > + * These calls to p2m->set_entry() must succeed: we have the gfn > > + * locked and just did a successful get_entry(). */ > > + p2m_gpfn_lock(p2m, gfn); > > + mfn = p2m->get_entry(p2m, gfn, &p2mt, &p2ma, 0, NULL); > > Neither of these (direct access to a p2m lock or direct call of a p2m > function pointer) should happen outside the p2m code. > > From an outside caller you _could_ use get_gfn_type_access(p2m, gfn, > &p2mt, &p2ma, 0, NULL) instead (and put_gfn() instead of > p2m_gpfn_unlock()). But directly calling p2m->set_entry() (or > p2m_set_entry()) is still not OK. > > I think it would be better to leave an arch-specific > "bool_t p2m_mem_access_check(p2m, gfn, npfec, &p2ma)" that does the > lookup and the automatic promotions and returns the access type. It > should also do the check for missing listeners and automatically reset > p2m entries (i.e. the second call to ->set_entry() below). Then this > function won't need to take any locks or get_gfn calls at all -- it > will just have the check for non-vioating faults, the call to > p2m_mem_access_check() that might ajdust entries, and the code to > prepare a req. > > > + > > + /* Check if the access is against the permissions. */ > > + switch ( p2ma ) > > + { > > + case p2m_access_rwx: > > + violation = 0; > > + break; > > + case p2m_access_rw: > > + violation = npfec.insn_fetch; > > + break; > > + case p2m_access_wx: > > + violation = npfec.read_access; > > + break; > > + case p2m_access_rx: > > + case p2m_access_rx2rw: > > + violation = npfec.write_access; > > + break; > > + case p2m_access_x: > > + violation = npfec.read_access || npfec.write_access; > > + break; > > + case p2m_access_w: > > + violation = npfec.read_access || npfec.insn_fetch; > > + break; > > + case p2m_access_r: > > + violation = npfec.write_access || npfec.insn_fetch; > > + break; > > + case p2m_access_n: > > + case p2m_access_n2rwx: > > + default: > > + violation = npfec.read_access || npfec.write_access || > npfec.insn_fetch; > > + break; > > + } > > In any case this block, which doesn't depend on the p2m contents at > all, should happen before any locks or lookups. That way you can > avoid taking the locks at all for non-violating faults (as the > original code does). > > > + /* If no violation is found here, it needs to be reinjected. */ > > + if ( !violation ) > > + { > > + p2m_gpfn_unlock(p2m, gfn); > > + return -EFAULT; > > + } > > + > > + /* Check for automatic setting promotion. */ > > + if ( npfec.write_access && p2ma == p2m_access_rx2rw ) > > + { > > + rc = p2m->set_entry(p2m, gfn, mfn, PAGE_ORDER_4K, p2mt, > p2m_access_rw); > > + ASSERT(rc == 0); > > + p2m_gpfn_unlock(p2m, gfn); > > + return 1; > > + } > > + else if ( p2ma == p2m_access_n2rwx ) > > + { > > + ASSERT(npfec.write_access || npfec.read_access || > npfec.insn_fetch); > > + rc = p2m->set_entry(p2m, gfn, mfn, PAGE_ORDER_4K, > > + p2mt, p2m_access_rwx); > > + ASSERT(rc == 0); > > + } > > + p2m_gpfn_unlock(p2m, gfn); > > + > > + /* Otherwise, check if there is a memory event listener, and send > the message along */ > > + if ( !mem_event_check_ring(&d->mem_event->access) ) > > + { > > + /* No listener */ > > + if ( p2m->access_required ) > > + { > > + gdprintk(XENLOG_INFO, "Memory access permissions failure, " > > + "no mem_event listener VCPU %d, dom > %d\n", > > + v->vcpu_id, d->domain_id); > > + domain_crash(v->domain); > > + return -EFAULT; > > + } > > + else > > + { > > + p2m_gpfn_lock(p2m, gfn); > > + mfn = p2m->get_entry(p2m, gfn, &p2mt, &p2ma, 0, NULL); > > + if ( p2ma != p2m_access_n2rwx ) > > + > > + { > > + /* A listener is not required, so clear the access > > + * restrictions. This set must succeed: we have the > > + * gfn locked and just did a successful get_entry(). */ > > + rc = p2m->set_entry(p2m, gfn, mfn, PAGE_ORDER_4K, > > + p2mt, p2m_access_rwx); > > + ASSERT(rc == 0); > > + } > > + p2m_gpfn_unlock(p2m, gfn); > > + return 1; > > + } > > + } > > + > > + req = xzalloc(mem_event_request_t); > > + if ( req ) > > + { > > + req->reason = MEM_EVENT_REASON_VIOLATION; > > + > > + /* Pause the current VCPU */ > > + if ( p2ma != p2m_access_n2rwx ) > > + req->flags |= MEM_EVENT_FLAG_VCPU_PAUSED; > > + > > + /* Send request to mem event */ > > + req->gfn = gfn; > > + req->offset = gpa & ((1 << PAGE_SHIFT) - 1); > > + req->gla_valid = npfec.gla_valid; > > + req->gla = gla; > > + if ( npfec.kind == npfec_kind_with_gla ) > > + req->fault_with_gla = 1; > > + else if ( npfec.kind == npfec_kind_in_gpt ) > > + req->fault_in_gpt = 1; > > + req->access_r = npfec.read_access; > > + req->access_w = npfec.write_access; > > + req->access_x = npfec.insn_fetch; > > + req->vcpu_id = v->vcpu_id; > > + > > + mem_access_send_req(v->domain, req); > > The original function returned the req for the caller to send, and the > caller delayed sending it until the end of hvm_hap_nested_page_fault(), > when the last locks are dropped. You need to keep this behaviour or > the system can deadlock. > > I wonder whether we could add an ASSERT_NOT_IN_ATOMIC() at the top of > mem_event_wait_slot() to make this kind of thing easier to spot. At > the moment it will only fail if it actually has to wait. > > Cheers, > > Tim. > Ack. I think I'll drop this patch for the time being as the refactoring work required for it to be in common mem_access is quite significant, both in the x86 and the ARM side. And it would only result in a bit of code-deduplication. We could pick-it up later if there is a need for it / value in it. Thanks, Tamas --089e0149d2e8746f50050354b528 Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable


On Thu, Sep 18, 2014 at 12:39 PM, Tim Deegan <tim@xen.org> wro= te:
Hi,

Thanks for the patch.=A0 Tidying this up is a great idea but I'm afraid=
it can't be done quite this way.

At 22:51 +0200 on 17 Sep (1410990670), Tamas K Lengyel wrote:
> The p2m_mem_access_check was used only to automatically promote the > special-case mem_access rights and to allocate the mem_access message.=
> In this patch we abstract the violation check into a common mem_access= _check
> function and change its return value to int.
>
> Return with rc < 0 means the fault wasn't triggered by mem_acce= ss permissions.
> Return with rc =3D=3D 0 it was but rights were not automatically promo= ted.
> Return with rc > 0 means mem_access rights had been automatically p= romoted.
>
> As part of this patch, we relocate the x86/mm/mm-locks.h header into a= sm
> as to be able to create a generic inlined p2m_gpfn_lock/unlock wrapper=
> that each arch can define based on its locking scheme.

I'm afraid not.=A0 mm-locks.h is hidden away in arch/x86/mm
deliberately, to stop p2m internals from leaking out into the rest of
the code and making it harder to reason about.=A0 So the fact that you
had to move it suggests that we need to think about using cleaner
interfaces between the code you're extracting and the rest of the p2m code...

> +int mem_access_check(paddr_t gpa, unsigned long gla, struct npfec npf= ec)
> +{
> +=A0 =A0 bool_t violation;
> +=A0 =A0 mfn_t mfn;
> +=A0 =A0 p2m_access_t p2ma;
> +=A0 =A0 p2m_type_t p2mt;
> +=A0 =A0 mem_event_request_t *req;
> +=A0 =A0 int rc;
> +=A0 =A0 struct vcpu *v =3D current;
> +=A0 =A0 unsigned long gfn =3D gpa >> PAGE_SHIFT;
> +=A0 =A0 struct domain *d =3D v->domain;
> +=A0 =A0 struct p2m_domain* p2m =3D p2m_get_hostp2m(d);
> +
> +=A0 =A0 /* First, handle rx2rw conversion automatically.
> +=A0 =A0 =A0* These calls to p2m->set_entry() must succeed: we have= the gfn
> +=A0 =A0 =A0* locked and just did a successful get_entry(). */
> +=A0 =A0 p2m_gpfn_lock(p2m, gfn);
> +=A0 =A0 mfn =3D p2m->get_entry(p2m, gfn, &p2mt, &p2ma, 0, = NULL);

Neither of these (direct access to a p2m lock or direct call of a p2= m
function pointer) should happen outside the p2m code.

>>From an outside caller you _could_ use get_gfn_type_access(p2m, gfn,
&p2mt, &p2ma, 0, NULL) instead (and put_gfn() instead of
p2m_gpfn_unlock()).=A0 But directly calling p2m->set_entry() (or
p2m_set_entry()) is still not OK.

I think it would be better to leave an arch-specific
"bool_t p2m_mem_access_check(p2m, gfn, npfec, &p2ma)" that do= es the
lookup and the automatic promotions and returns the access type.=A0 It
should also do the check for missing listeners and automatically reset
p2m entries (i.e. the second call to ->set_entry() below).=A0 Then this<= br> function won't need to take any locks or get_gfn calls at all -- it
will just have the check for non-vioating faults, the call to
p2m_mem_access_check() that might ajdust entries, and the code to
prepare a req.

> +
> +=A0 =A0 /* Check if the access is against the permissions. */
> +=A0 =A0 switch ( p2ma )
> +=A0 =A0 {
> +=A0 =A0 case p2m_access_rwx:
> +=A0 =A0 =A0 =A0 violation =3D 0;
> +=A0 =A0 =A0 =A0 break;
> +=A0 =A0 case p2m_access_rw:
> +=A0 =A0 =A0 =A0 violation =3D npfec.insn_fetch;
> +=A0 =A0 =A0 =A0 break;
> +=A0 =A0 case p2m_access_wx:
> +=A0 =A0 =A0 =A0 violation =3D npfec.read_access;
> +=A0 =A0 =A0 =A0 break;
> +=A0 =A0 case p2m_access_rx:
> +=A0 =A0 case p2m_access_rx2rw:
> +=A0 =A0 =A0 =A0 violation =3D npfec.write_access;
> +=A0 =A0 =A0 =A0 break;
> +=A0 =A0 case p2m_access_x:
> +=A0 =A0 =A0 =A0 violation =3D npfec.read_access || npfec.write_access= ;
> +=A0 =A0 =A0 =A0 break;
> +=A0 =A0 case p2m_access_w:
> +=A0 =A0 =A0 =A0 violation =3D npfec.read_access || npfec.insn_fetch;<= br> > +=A0 =A0 =A0 =A0 break;
> +=A0 =A0 case p2m_access_r:
> +=A0 =A0 =A0 =A0 violation =3D npfec.write_access || npfec.insn_fetch;=
> +=A0 =A0 =A0 =A0 break;
> +=A0 =A0 case p2m_access_n:
> +=A0 =A0 case p2m_access_n2rwx:
> +=A0 =A0 default:
> +=A0 =A0 =A0 =A0 violation =3D npfec.read_access || npfec.write_access= || npfec.insn_fetch;
> +=A0 =A0 =A0 =A0 break;
> +=A0 =A0 }

In any case this block, which doesn't depend on the p2m con= tents at
all, should happen before any locks or lookups.=A0 That way you can
avoid taking the locks at all for non-violating faults (as the
original code does).

> +=A0 =A0 /* If no violation is found here, it needs to be reinjected. = */
> +=A0 =A0 if ( !violation )
> +=A0 =A0 {
> +=A0 =A0 =A0 =A0 p2m_gpfn_unlock(p2m, gfn);
> +=A0 =A0 =A0 =A0 return -EFAULT;
> +=A0 =A0 }
> +
> +=A0 =A0 /* Check for automatic setting promotion. */
> +=A0 =A0 if ( npfec.write_access && p2ma =3D=3D p2m_access_rx2= rw )
> +=A0 =A0 {
> +=A0 =A0 =A0 =A0 rc =3D p2m->set_entry(p2m, gfn, mfn, PAGE_ORDER_4K= , p2mt, p2m_access_rw);
> +=A0 =A0 =A0 =A0 ASSERT(rc =3D=3D 0);
> +=A0 =A0 =A0 =A0 p2m_gpfn_unlock(p2m, gfn);
> +=A0 =A0 =A0 =A0 return 1;
> +=A0 =A0 }
> +=A0 =A0 else if ( p2ma =3D=3D p2m_access_n2rwx )
> +=A0 =A0 {
> +=A0 =A0 =A0 =A0 ASSERT(npfec.write_access || npfec.read_access || npf= ec.insn_fetch);
> +=A0 =A0 =A0 =A0 rc =3D p2m->set_entry(p2m, gfn, mfn, PAGE_ORDER_4K= ,
> +=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 p2mt, p2m_acc= ess_rwx);
> +=A0 =A0 =A0 =A0 ASSERT(rc =3D=3D 0);
> +=A0 =A0 }
> +=A0 =A0 p2m_gpfn_unlock(p2m, gfn);
> +
> +=A0 =A0 /* Otherwise, check if there is a memory event listener, and = send the message along */
> +=A0 =A0 if ( !mem_event_check_ring(&d->mem_event->access) )=
> +=A0 =A0 {
> +=A0 =A0 =A0 =A0 /* No listener */
> +=A0 =A0 =A0 =A0 if ( p2m->access_required )
> +=A0 =A0 =A0 =A0 {
> +=A0 =A0 =A0 =A0 =A0 =A0 gdprintk(XENLOG_INFO, "Memory access per= missions failure, "
> +=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 &= quot;no mem_event listener VCPU %d, dom %d\n",
> +=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 v= ->vcpu_id, d->domain_id);
> +=A0 =A0 =A0 =A0 =A0 =A0 domain_crash(v->domain);
> +=A0 =A0 =A0 =A0 =A0 =A0 return -EFAULT;
> +=A0 =A0 =A0 =A0 }
> +=A0 =A0 =A0 =A0 else
> +=A0 =A0 =A0 =A0 {
> +=A0 =A0 =A0 =A0 =A0 =A0 p2m_gpfn_lock(p2m, gfn);
> +=A0 =A0 =A0 =A0 =A0 =A0 mfn =3D p2m->get_entry(p2m, gfn, &p2mt= , &p2ma, 0, NULL);
> +=A0 =A0 =A0 =A0 =A0 =A0 if ( p2ma !=3D p2m_access_n2rwx )
> +
> +=A0 =A0 =A0 =A0 =A0 =A0 {
> +=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* A listener is not required, so cle= ar the access
> +=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0* restrictions.=A0 This set must s= ucceed: we have the
> +=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0* gfn locked and just did a succes= sful get_entry(). */
> +=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 rc =3D p2m->set_entry(p2m, gfn, mf= n, PAGE_ORDER_4K,
> +=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 p2mt, p2m_access_rwx);
> +=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ASSERT(rc =3D=3D 0);
> +=A0 =A0 =A0 =A0 =A0 =A0 }
> +=A0 =A0 =A0 =A0 =A0 =A0 p2m_gpfn_unlock(p2m, gfn);
> +=A0 =A0 =A0 =A0 =A0 =A0 return 1;
> +=A0 =A0 =A0 =A0 }
> +=A0 =A0 }
> +
> +=A0 =A0 req =3D xzalloc(mem_event_request_t);
> +=A0 =A0 if ( req )
> +=A0 =A0 {
> +=A0 =A0 =A0 =A0 req->reason =3D MEM_EVENT_REASON_VIOLATION;
> +
> +=A0 =A0 =A0 =A0 /* Pause the current VCPU */
> +=A0 =A0 =A0 =A0 if ( p2ma !=3D p2m_access_n2rwx )
> +=A0 =A0 =A0 =A0 =A0 =A0 req->flags |=3D MEM_EVENT_FLAG_VCPU_PAUSED= ;
> +
> +=A0 =A0 =A0 =A0 /* Send request to mem event */
> +=A0 =A0 =A0 =A0 req->gfn =3D gfn;
> +=A0 =A0 =A0 =A0 req->offset =3D gpa & ((1 << PAGE_SHIFT)= - 1);
> +=A0 =A0 =A0 =A0 req->gla_valid =3D npfec.gla_valid;
> +=A0 =A0 =A0 =A0 req->gla =3D gla;
> +=A0 =A0 =A0 =A0 if ( npfec.kind =3D=3D npfec_kind_with_gla )
> +=A0 =A0 =A0 =A0 =A0 =A0 req->fault_with_gla =3D 1;
> +=A0 =A0 =A0 =A0 else if ( npfec.kind =3D=3D npfec_kind_in_gpt )
> +=A0 =A0 =A0 =A0 =A0 =A0 req->fault_in_gpt =3D 1;
> +=A0 =A0 =A0 =A0 req->access_r =3D npfec.read_access;
> +=A0 =A0 =A0 =A0 req->access_w =3D npfec.write_access;
> +=A0 =A0 =A0 =A0 req->access_x =3D npfec.insn_fetch;
> +=A0 =A0 =A0 =A0 req->vcpu_id =3D v->vcpu_id;
> +
> +=A0 =A0 =A0 =A0 mem_access_send_req(v->domain, req);

The original function returned the req for the caller to send, = and the
caller delayed sending it until the end of hvm_hap_nested_page_fault(),
when the last locks are dropped.=A0 You need to keep this behaviour or
the system can deadlock.

I wonder whether we could add an ASSERT_NOT_IN_ATOMIC() at the top of
mem_event_wait_slot() to make this kind of thing easier to spot.=A0 At
the moment it will only fail if it actually has to wait.

Cheers,

Tim.

Ack. I think I'll drop this patch for = the time being as the refactoring work required for it to be in common mem_= access is quite significant, both in the x86 and the ARM side. And it would= only result in a bit of code-deduplication. We could pick-it up later if t= here is a need for it / value in it.

Thanks,
Tamas

--089e0149d2e8746f50050354b528-- --===============8235849415332769339== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel --===============8235849415332769339==--