From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Lengyel, Tamas" Subject: Re: [PATCH v2 1/2] altp2m: Merge p2m_set_altp2m_mem_access and p2m_set_mem_access Date: Mon, 1 Feb 2016 09:35:50 -0700 Message-ID: References: <1454014688-25060-1-git-send-email-tlengyel@novetta.com> <56AB552202000078000CC5E5@prv-mh.provo.novell.com> <56AB9F3102000078000CC78F@prv-mh.provo.novell.com> <56ABA5CA02000078000CC820@prv-mh.provo.novell.com> <1454337922.28781.102.camel@citrix.com> <56AF93FD02000078000CD176@prv-mh.provo.novell.com> <1454344256.28781.113.camel@citrix.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============8115166800646586487==" Return-path: Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1aQHSX-0000zQ-Do for xen-devel@lists.xenproject.org; Mon, 01 Feb 2016 16:36:13 +0000 Received: by mail-vk0-f42.google.com with SMTP id e6so80014494vkh.2 for ; Mon, 01 Feb 2016 08:36:11 -0800 (PST) In-Reply-To: <1454344256.28781.113.camel@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell Cc: Wei Liu , Razvan Cojocaru , Stefano Stabellini , GeorgeDunlap , Andrew Cooper , Ian Jackson , Stefano Stabellini , Jan Beulich , xen-devel@lists.xenproject.org, KeirFraser List-Id: xen-devel@lists.xenproject.org --===============8115166800646586487== Content-Type: multipart/alternative; boundary=001a11430efe7f2898052ab7fb01 --001a11430efe7f2898052ab7fb01 Content-Type: text/plain; charset=UTF-8 On Mon, Feb 1, 2016 at 9:30 AM, Ian Campbell wrote: > On Mon, 2016-02-01 at 09:21 -0700, Jan Beulich wrote: > > > > > On 01.02.16 at 15:45, wrote: > > > On Fri, 2016-01-29 at 09:47 -0700, Jan Beulich wrote: > > > > > > > On 29.01.16 at 17:32, wrote: > > > > > On Fri, Jan 29, 2016 at 9:19 AM, Jan Beulich > > > > > wrote: > > > > > > > > > On 29.01.16 at 17:12, wrote: > > > > > > > On Fri, Jan 29, 2016 at 4:03 AM, Jan Beulich < > JBeulich@suse.com > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > On 28.01.16 at 21:58, wrote: > > > > > > > > > --- a/xen/include/public/memory.h > > > > > > > > > +++ b/xen/include/public/memory.h > > > > > > > > > @@ -423,11 +423,14 @@ struct xen_mem_access_op { > > > > > > > > > /* xenmem_access_t */ > > > > > > > > > uint8_t access; > > > > > > > > > domid_t domid; > > > > > > > > > + uint16_t altp2m_idx; > > > > > > > > > + uint16_t _pad; > > > > > > > > > /* > > > > > > > > > * Number of pages for set op > > > > > > > > > * Ignored on setting default access and other ops > > > > > > > > > */ > > > > > > > > > uint32_t nr; > > > > > > > > > + uint32_t _pad2; > > > > > > > > > > > > > > > > Repeating what I had said on v1: So this is a tools only > > > > > > > > interface, > > > > > > > > yes. But it's not versioned (other than e.g. domctl and > > > > > > > > sysctl), > > > > > > > > so > > > > > > > > altering the interface structure is at least fragile. > > > > > > > > > > > > > > Not sure what I can do to address this. > > > > > > > > > > > > Deprecate the old interface and introduce a new one. But other > > > > > > maintainers' opinions would be welcome. > > > > > > > > > > That seems like a very heavy handed solution to me. > > > > > > > > I understand that - hence the request for others' opinions. > > > > > > It's unfortunate that we've found ourselves here, but I think rather > > > than > > > deprecating the current and adding a new op alongside we should just > > > accept > > > the one-time fragility this time around, add the version field as part > > > of > > > this set of changes and try and remember to include a version number > > > for > > > next time we add a tools only interface. I don't think xenaccess is yet > > > widely used outside of Tamas and the Bitdfender folks, who I would > > > assume > > > can cope with such a change. > > > > > > I could accept changing the op number would make sense, but I don't > > > think > > > we should deprecate the old one (which implies continuing to support it > > > in > > > parallel), if we go this route we should just retire the old number to > > > straight away to return -ENOSYS (or maybe -EACCESS, which is what a > > > version > > > mismatch would have resulted in). > > > > That actually looks like a reasonable compromise, until we finally > > manage to get around to morph the tools-only HVM-ops into a > > new hvmctl hypercall (leaving only guest accessible ones in the > > current interface). > > Aren't the ones being discussed here xenmem subops rather than hvmops? > Yes, in v2 I'm not touching the guest-visible altp2m_set_mem_access hvmop, just how it is handled internal to Xen. I do wonder if it was intended to be guest-visible operation though or if that was an overlook of not putting it into tools-only. Tamas --001a11430efe7f2898052ab7fb01 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable


On Mon, Feb 1, 2016 at 9:30 AM, Ian Campbell <ian.campbell@citri= x.com> wrote:
On Mon, 2016-02-01 at 09:21 -0700, Jan Beulich w= rote:
> > > > On 01.02.16 at 15:45, <ian.campbell@citrix.com> wrote:
> > On Fri, 2016-01-29 at 09:47 -0700, Jan Beulich wrote:
> > > > > > On 29.01.16 at 17:32, <tlengyel@novetta.com> wrote:
> > > > On Fri, Jan 29, 2016 at 9:19 AM, Jan Beulich <JBeulich@suse.com>
> > > > wrote:
> > > > > > > > On 29.01.16 at 17:12, <tlengyel@novetta.com> wrote:
> > > > > > On Fri, Jan 29, 2016 at 4:03 AM, Jan Beulich = <JBeulich@suse.com
> > > > > > >
> > > > > > wrote:
> > > > > > > > > > On 28.01.16 at 21:58, <= ;tlengyel@novetta.com> wrote= :
> > > > > > > > --- a/xen/include/public/memory.h > > > > > > > > +++ b/xen/include/public/memory.h > > > > > > > > @@ -423,11 +423,14 @@ struct xen_me= m_access_op {
> > > > > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0/* xe= nmem_access_t */
> > > > > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0uint8= _t access;
> > > > > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0domid= _t domid;
> > > > > > > > +=C2=A0=C2=A0=C2=A0=C2=A0uint16_t a= ltp2m_idx;
> > > > > > > > +=C2=A0=C2=A0=C2=A0=C2=A0uint16_t _= pad;
> > > > > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0/* > > > > > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0* Number of pages for set op
> > > > > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0* Ignored on setting default access and other ops
> > > > > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0*/
> > > > > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0uint3= 2_t nr;
> > > > > > > > +=C2=A0=C2=A0=C2=A0=C2=A0uint32_t _= pad2;
> > > > > > >
> > > > > > > Repeating what I had said on v1: So this= is a tools only
> > > > > > > interface,
> > > > > > > yes. But it's not versioned (other t= han e.g. domctl and
> > > > > > > sysctl),
> > > > > > > so
> > > > > > > altering the interface structure is at l= east fragile.
> > > > > >
> > > > > > Not sure what I can do to address this.
> > > > >
> > > > > Deprecate the old interface and introduce a new on= e. But other
> > > > > maintainers' opinions would be welcome.
> > > >
> > > > That seems like a very heavy handed solution to me.
> > >
> > > I understand that - hence the request for others' opinio= ns.
> >
> > It's unfortunate that we've found ourselves here, but I t= hink rather
> > than
> > deprecating the current and adding a new op alongside we should j= ust
> > accept
> > the one-time fragility this time around, add the version field as= part
> > of
> > this set of changes and try and remember to include a version num= ber
> > for
> > next time we add a tools only interface. I don't think xenacc= ess is yet
> > widely used outside of Tamas and the Bitdfender folks, who I woul= d
> > assume
> > can cope with such a change.
> >
> > I could accept changing the op number would make sense, but I don= 't
> > think
> > we should deprecate the old one (which implies continuing to supp= ort it
> > in
> > parallel), if we go this route we should just retire the old numb= er to
> > straight away to return -ENOSYS (or maybe -EACCESS, which is what= a
> > version
> > mismatch would have resulted in).
>
> That actually looks like a reasonable compromise, until we finally
> manage to get around to morph the tools-only HVM-ops into a
> new hvmctl hypercall (leaving only guest accessible ones in the
> current interface).

Aren't the ones being discussed here xenmem subops rather t= han hvmops?

Yes, in v2 I'm not touching the guest-visible al= tp2m_set_mem_access hvmop, just how it is handled internal to Xen. I do won= der if it was intended to be guest-visible operation though or if that was = an overlook of not putting it into tools-only.

Tamas
=

--001a11430efe7f2898052ab7fb01-- --===============8115166800646586487== 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 --===============8115166800646586487==--