From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tamas K Lengyel Subject: Re: [PATCH V3] x86/monitor: Disallow setting mem_access_emulate_each_rep when vm_event is NULL Date: Tue, 3 May 2016 12:28:22 -0600 Message-ID: References: <1460181249-13651-1-git-send-email-rcojocaru@bitdefender.com> <3a451eea-47b8-42f0-fa73-132c0fe4e39a@bitdefender.com> <572879ED02000078000E7E19@prv-mh.provo.novell.com> <54db3ade-df12-e7d6-3f30-87417ae0784a@bitdefender.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============8071915682435143359==" Return-path: In-Reply-To: <54db3ade-df12-e7d6-3f30-87417ae0784a@bitdefender.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xen.org Sender: "Xen-devel" To: Razvan Cojocaru Cc: Andrew Cooper , Keir Fraser , "wei.liu2@citrix.com" , Jan Beulich , Xen-devel List-Id: xen-devel@lists.xenproject.org --===============8071915682435143359== Content-Type: multipart/alternative; boundary=001a114422943108600531f446c3 --001a114422943108600531f446c3 Content-Type: text/plain; charset=UTF-8 On Tue, May 3, 2016 at 2:18 AM, Razvan Cojocaru wrote: > On 05/03/2016 11:14 AM, Jan Beulich wrote: > >>>> On 29.04.16 at 18:12, wrote: > >> On 04/09/16 08:54, Razvan Cojocaru wrote: > >>> It is meaningless (and potentially dangerous - see > hvmemul_virtual_to_linear()) > >>> to set mem_access_emulate_each_rep before xc_monitor_enable() (which > allocates > >>> vcpu->arch.vm_event) has been called, so return an error from the > >>> XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP hypercall when that is the case. > >>> > >>> Signed-off-by: Razvan Cojocaru > >>> Reviewed-by: Andrew Cooper > >>> > >>> --- > >>> Changes since V2: > >>> - Updated the if() condition as recommended by Andrew Cooper. > >>> - Added Andrew Cooper's Reviewed-by. > >>> --- > >>> xen/include/asm-x86/monitor.h | 16 +++++++++++++--- > >>> 1 file changed, 13 insertions(+), 3 deletions(-) > >>> > >>> diff --git a/xen/include/asm-x86/monitor.h > b/xen/include/asm-x86/monitor.h > >>> index 0954b59..d367099 100644 > >>> --- a/xen/include/asm-x86/monitor.h > >>> +++ b/xen/include/asm-x86/monitor.h > >>> @@ -32,19 +32,29 @@ > >>> static inline > >>> int arch_monitor_domctl_op(struct domain *d, struct > xen_domctl_monitor_op *mop) > >>> { > >>> + int rc = 0; > >>> + > >>> switch ( mop->op ) > >>> { > >>> case XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP: > >>> domain_pause(d); > >>> - d->arch.mem_access_emulate_each_rep = !!mop->event; > >>> + /* > >>> + * Enabling mem_access_emulate_each_rep without a vm_event > subscriber > >>> + * is meaningless. > >>> + */ > >>> + if ( d->max_vcpus && d->vcpu[0] && d->vcpu[0]->arch.vm_event ) > >>> + d->arch.mem_access_emulate_each_rep = !!mop->event; > >>> + else > >>> + rc = -EINVAL; > >>> + > >>> domain_unpause(d); > >>> break; > >>> > >>> default: > >>> - return -EOPNOTSUPP; > >>> + rc = -EOPNOTSUPP; > >>> } > >>> > >>> - return 0; > >>> + return rc; > >>> } > >>> > >>> int arch_monitor_domctl_event(struct domain *d, > >> > >> According to the previous list discussion with Andrew Cooper, this fix > >> might be considered for the 4.7 release, so CC-ing Wei for a release > >> ack, as suggested. > > > > Even if - without the pending ./MAINTAINERS adjustment - not > > formally required, I don't understand why you didn't Cc Tamas on > > this patch. I don't think this should go in without his ack. > > Of course, I was under the impression that he was in the recipients list > (I let scripts/maintaners.pl do the work and didn't pay much attention > to its output). > > By all means. > The maintainers file wasn't covering this header properly. Fixed in my other patch-set. Acked-by: Tamas K Lengyel --001a114422943108600531f446c3 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable


On Tue, May 3, 2016 at 2:18 AM, Razvan Cojocaru <<= a href=3D"mailto:rcojocaru@bitdefender.com" target=3D"_blank">rcojocaru@bit= defender.com> wrote:
On 05/03/2016 11:14 AM, Jan Beulich wrote= :
>>>> On 29.04.16 at 18:12, <rcojocaru@bitdefender.com> wrote:
>> On 04/09/16 08:54, Razvan Cojocaru wrote:
>>> It is meaningless (and potentially dangerous - see hvmemul_vir= tual_to_linear())
>>> to set mem_access_emulate_each_rep before xc_monitor_enable() = (which allocates
>>> vcpu->arch.vm_event) has been called, so return an error fr= om the
>>> XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP hypercall when that is = the case.
>>>
>>> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
>>> Reviewed-by: Andrew Cooper <andrew.cooper3@citirx.com>
>>>
>>> ---
>>> Changes since V2:
>>>=C2=A0 - Updated the if() condition as recommended by Andrew Co= oper.
>>>=C2=A0 - Added Andrew Cooper's Reviewed-by.
>>> ---
>>>=C2=A0 xen/include/asm-x86/monitor.h | 16 +++++++++++++---
>>>=C2=A0 1 file changed, 13 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/xen/include/asm-x86/monitor.h b/xen/include/asm-x= 86/monitor.h
>>> index 0954b59..d367099 100644
>>> --- a/xen/include/asm-x86/monitor.h
>>> +++ b/xen/include/asm-x86/monitor.h
>>> @@ -32,19 +32,29 @@
>>>=C2=A0 static inline
>>>=C2=A0 int arch_monitor_domctl_op(struct domain *d, struct xen_= domctl_monitor_op *mop)
>>>=C2=A0 {
>>> +=C2=A0 =C2=A0 int rc =3D 0;
>>> +
>>>=C2=A0 =C2=A0 =C2=A0 switch ( mop->op )
>>>=C2=A0 =C2=A0 =C2=A0 {
>>>=C2=A0 =C2=A0 =C2=A0 case XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_RE= P:
>>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 domain_pause(d);
>>> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 d->arch.mem_access_emulate_eac= h_rep =3D !!mop->event;
>>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 /*
>>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* Enabling mem_access_emula= te_each_rep without a vm_event subscriber
>>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* is meaningless.
>>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0*/
>>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 if ( d->max_vcpus && d= ->vcpu[0] && d->vcpu[0]->arch.vm_event )
>>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 d->arch.mem_acce= ss_emulate_each_rep =3D !!mop->event;
>>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 else
>>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 rc =3D -EINVAL;
>>> +
>>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 domain_unpause(d);
>>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 break;
>>>
>>>=C2=A0 =C2=A0 =C2=A0 default:
>>> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 return -EOPNOTSUPP;
>>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 rc =3D -EOPNOTSUPP;
>>>=C2=A0 =C2=A0 =C2=A0 }
>>>
>>> -=C2=A0 =C2=A0 return 0;
>>> +=C2=A0 =C2=A0 return rc;
>>>=C2=A0 }
>>>
>>>=C2=A0 int arch_monitor_domctl_event(struct domain *d,
>>
>> According to the previous list discussion with Andrew Cooper, this= fix
>> might be considered for the 4.7 release, so CC-ing Wei for a relea= se
>> ack, as suggested.
>
> Even if - without the pending ./MAINTAINERS adjustment - not
> formally required, I don't understand why you didn't Cc Tamas = on
> this patch. I don't think this should go in without his ack.

Of course, I was under the impression that he was in the recipi= ents list
(I let scripts/maintaners.pl do the work and didn't pay much attention=
to its output).

By all means.

The maintainers file wasn= 't covering this header properly. Fixed in my other patch-set.

<= /div>
Acked-by: Tamas K Lengyel <tamas@tklengyel.com>
--001a114422943108600531f446c3-- --===============8071915682435143359== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KWGVuLWRldmVs IG1haWxpbmcgbGlzdApYZW4tZGV2ZWxAbGlzdHMueGVuLm9yZwpodHRwOi8vbGlzdHMueGVuLm9y Zy94ZW4tZGV2ZWwK --===============8071915682435143359==--