| Message ID | 1454014688-25060-1-git-send-email-tlengyel@novetta.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 28.01.16 at 21:58, <tlengyel@novetta.com> wrote: > --- a/xen/arch/x86/mm/p2m.c > +++ b/xen/arch/x86/mm/p2m.c > @@ -1777,14 +1777,57 @@ bool_t p2m_mem_access_check(paddr_t gpa, unsigned long gla, > return (p2ma == p2m_access_n2rwx); > } > > +static int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m, > + struct p2m_domain *ap2m, p2m_access_t a, > + unsigned long gfn) I think new functions would better not use "unsigned long" for frame numbers. > +{ > + mfn_t mfn; > + p2m_type_t t; > + p2m_access_t old_a; > + unsigned int page_order; > + int rc; > + > + mfn = ap2m->get_entry(ap2m, gfn, &t, &old_a, 0, NULL, NULL); > + > + /* Check host p2m if no valid entry in alternate */ > + if ( !mfn_valid(mfn) ) > + { > + mfn = hp2m->get_entry(hp2m, gfn, &t, &old_a, > + P2M_ALLOC | P2M_UNSHARE, &page_order, NULL); > + > + rc = -ESRCH; > + if ( !mfn_valid(mfn) || t != p2m_ram_rw ) > + goto out; > + > + /* If this is a superpage, copy that first */ > + if ( page_order != PAGE_ORDER_4K ) > + { > + unsigned long mask = ~((1UL << page_order) - 1); > + gfn_t gfn2 = _gfn(gfn & mask); > + mfn_t mfn2 = _mfn(mfn_x(mfn) & mask); > + > + rc = ap2m->set_entry(ap2m, gfn_x(gfn2), mfn2, page_order, t, old_a, 1); > + if ( rc ) > + goto out; > + } > + } > + > + rc = ap2m->set_entry(ap2m, gfn, mfn, PAGE_ORDER_4K, t, a, > + (current->domain != d)); > + > + out: > + return rc; > +} With there not being any involved error handling here, I don't think using a label and goto is warranted here. But I'll leave the ultimate decision to George, of course. > --- 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. Jan
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/arch/x86/mm/p2m.c > > +++ b/xen/arch/x86/mm/p2m.c > > @@ -1777,14 +1777,57 @@ bool_t p2m_mem_access_check(paddr_t gpa, > unsigned long gla, > > return (p2ma == p2m_access_n2rwx); > > } > > > > +static int p2m_set_altp2m_mem_access(struct domain *d, struct > p2m_domain *hp2m, > > + struct p2m_domain *ap2m, > p2m_access_t a, > > + unsigned long gfn) > > I think new functions would better not use "unsigned long" for frame > numbers. > The only place this is called from the gfn is already converted to unsigned long. I don't see much point in converting it back to gfn_t and then back to unsigned long again.. I was thinking this function may even be declared as inline? > > > +{ > > + mfn_t mfn; > > + p2m_type_t t; > > + p2m_access_t old_a; > > + unsigned int page_order; > > + int rc; > > + > > + mfn = ap2m->get_entry(ap2m, gfn, &t, &old_a, 0, NULL, NULL); > > + > > + /* Check host p2m if no valid entry in alternate */ > > + if ( !mfn_valid(mfn) ) > > + { > > + mfn = hp2m->get_entry(hp2m, gfn, &t, &old_a, > > + P2M_ALLOC | P2M_UNSHARE, &page_order, > NULL); > > + > > + rc = -ESRCH; > > + if ( !mfn_valid(mfn) || t != p2m_ram_rw ) > > + goto out; > > + > > + /* If this is a superpage, copy that first */ > > + if ( page_order != PAGE_ORDER_4K ) > > + { > > + unsigned long mask = ~((1UL << page_order) - 1); > > + gfn_t gfn2 = _gfn(gfn & mask); > > + mfn_t mfn2 = _mfn(mfn_x(mfn) & mask); > > + > > + rc = ap2m->set_entry(ap2m, gfn_x(gfn2), mfn2, page_order, > t, old_a, 1); > > + if ( rc ) > > + goto out; > > + } > > + } > > + > > + rc = ap2m->set_entry(ap2m, gfn, mfn, PAGE_ORDER_4K, t, a, > > + (current->domain != d)); > > + > > + out: > > + return rc; > > +} > > With there not being any involved error handling here, I don't think > using a label and goto is warranted here. But I'll leave the ultimate > decision to George, of course. > RIght, this is a remnant from the previous version of this function where out also had the p2m_unlock. Now that it is just a return I could do the return in place of the gotos. Let me know which one is preferred. > > > --- 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. Tamas
>>> 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/arch/x86/mm/p2m.c >> > +++ b/xen/arch/x86/mm/p2m.c >> > @@ -1777,14 +1777,57 @@ bool_t p2m_mem_access_check(paddr_t gpa, >> unsigned long gla, >> > return (p2ma == p2m_access_n2rwx); >> > } >> > >> > +static int p2m_set_altp2m_mem_access(struct domain *d, struct >> p2m_domain *hp2m, >> > + struct p2m_domain *ap2m, >> p2m_access_t a, >> > + unsigned long gfn) >> >> I think new functions would better not use "unsigned long" for frame >> numbers. >> > > The only place this is called from the gfn is already converted to unsigned > long. I don't see much point in converting it back to gfn_t and then back > to unsigned long again.. If you recall, the goal is to have all frame numbers passed around have distinguishable types. > I was thinking this function may even be declared as inline? This is orthogonal (and I really don't care much). >> > +{ >> > + mfn_t mfn; >> > + p2m_type_t t; >> > + p2m_access_t old_a; >> > + unsigned int page_order; >> > + int rc; >> > + >> > + mfn = ap2m->get_entry(ap2m, gfn, &t, &old_a, 0, NULL, NULL); >> > + >> > + /* Check host p2m if no valid entry in alternate */ >> > + if ( !mfn_valid(mfn) ) >> > + { >> > + mfn = hp2m->get_entry(hp2m, gfn, &t, &old_a, >> > + P2M_ALLOC | P2M_UNSHARE, &page_order, >> NULL); >> > + >> > + rc = -ESRCH; >> > + if ( !mfn_valid(mfn) || t != p2m_ram_rw ) >> > + goto out; >> > + >> > + /* If this is a superpage, copy that first */ >> > + if ( page_order != PAGE_ORDER_4K ) >> > + { >> > + unsigned long mask = ~((1UL << page_order) - 1); >> > + gfn_t gfn2 = _gfn(gfn & mask); >> > + mfn_t mfn2 = _mfn(mfn_x(mfn) & mask); >> > + >> > + rc = ap2m->set_entry(ap2m, gfn_x(gfn2), mfn2, page_order, >> t, old_a, 1); >> > + if ( rc ) >> > + goto out; >> > + } >> > + } >> > + >> > + rc = ap2m->set_entry(ap2m, gfn, mfn, PAGE_ORDER_4K, t, a, >> > + (current->domain != d)); >> > + >> > + out: >> > + return rc; >> > +} >> >> With there not being any involved error handling here, I don't think >> using a label and goto is warranted here. But I'll leave the ultimate >> decision to George, of course. > > RIght, this is a remnant from the previous version of this function where > out also had the p2m_unlock. Now that it is just a return I could do the > return in place of the gotos. Let me know which one is preferred. Since you sent this to me - my general request is to avoid goto wherever possible. >> > --- 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. Jan
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/arch/x86/mm/p2m.c > >> > +++ b/xen/arch/x86/mm/p2m.c > >> > @@ -1777,14 +1777,57 @@ bool_t p2m_mem_access_check(paddr_t gpa, > >> unsigned long gla, > >> > return (p2ma == p2m_access_n2rwx); > >> > } > >> > > >> > +static int p2m_set_altp2m_mem_access(struct domain *d, struct > >> p2m_domain *hp2m, > >> > + struct p2m_domain *ap2m, > >> p2m_access_t a, > >> > + unsigned long gfn) > >> > >> I think new functions would better not use "unsigned long" for frame > >> numbers. > >> > > > > The only place this is called from the gfn is already converted to > unsigned > > long. I don't see much point in converting it back to gfn_t and then back > > to unsigned long again.. > > If you recall, the goal is to have all frame numbers passed around > have distinguishable types. > Fine by me, seems a bit of a waste but probably not noticeable. > > > I was thinking this function may even be declared as inline? > > This is orthogonal (and I really don't care much). > Well, AFAIK with an inline it wouldn't technically be passed as the code would be compiled into the other function. But I figure the idea is to have the compiler catch type related problems in general. I'll make it inline and just let the compiler optimization figure out if it really needs the conversion or not while. > > >> > +{ > >> > + mfn_t mfn; > >> > + p2m_type_t t; > >> > + p2m_access_t old_a; > >> > + unsigned int page_order; > >> > + int rc; > >> > + > >> > + mfn = ap2m->get_entry(ap2m, gfn, &t, &old_a, 0, NULL, NULL); > >> > + > >> > + /* Check host p2m if no valid entry in alternate */ > >> > + if ( !mfn_valid(mfn) ) > >> > + { > >> > + mfn = hp2m->get_entry(hp2m, gfn, &t, &old_a, > >> > + P2M_ALLOC | P2M_UNSHARE, &page_order, > >> NULL); > >> > + > >> > + rc = -ESRCH; > >> > + if ( !mfn_valid(mfn) || t != p2m_ram_rw ) > >> > + goto out; > >> > + > >> > + /* If this is a superpage, copy that first */ > >> > + if ( page_order != PAGE_ORDER_4K ) > >> > + { > >> > + unsigned long mask = ~((1UL << page_order) - 1); > >> > + gfn_t gfn2 = _gfn(gfn & mask); > >> > + mfn_t mfn2 = _mfn(mfn_x(mfn) & mask); > >> > + > >> > + rc = ap2m->set_entry(ap2m, gfn_x(gfn2), mfn2, page_order, > >> t, old_a, 1); > >> > + if ( rc ) > >> > + goto out; > >> > + } > >> > + } > >> > + > >> > + rc = ap2m->set_entry(ap2m, gfn, mfn, PAGE_ORDER_4K, t, a, > >> > + (current->domain != d)); > >> > + > >> > + out: > >> > + return rc; > >> > +} > >> > >> With there not being any involved error handling here, I don't think > >> using a label and goto is warranted here. But I'll leave the ultimate > >> decision to George, of course. > > > > RIght, this is a remnant from the previous version of this function where > > out also had the p2m_unlock. Now that it is just a return I could do the > > return in place of the gotos. Let me know which one is preferred. > > Since you sent this to me - my general request is to avoid goto > wherever possible. > Sure. > > >> > --- 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. Tamas
>>> 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_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. Jan
On Thu, Jan 28, 2016 at 01:58:07PM -0700, Tamas K Lengyel wrote: > The altp2m subsystem in its current form uses its own HVMOP hypercall to set > mem_access permissions, duplicating some of the code already present for > setting regular mem_access permissions. In this patch we consolidate the two > and update the corresponding tools. > > Signed-off-by: Tamas K Lengyel <tlengyel@novetta.com> > Cc: Ian Jackson <ian.jackson@eu.citrix.com> > Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > Cc: Ian Campbell <ian.campbell@citrix.com> > Cc: Wei Liu <wei.liu2@citrix.com> > Cc: Razvan Cojocaru <rcojocaru@bitdefender.com> > Cc: Stefano Stabellini <stefano.stabellini@citrix.com> > Cc: Keir Fraser <keir@xen.org> > Cc: Jan Beulich <jbeulich@suse.com> > Cc: Andrew Cooper <andrew.cooper3@citrix.com> > Cc: George Dunlap <george.dunlap@eu.citrix.com> > --- > v2: Don't deprecate the HVMOP hypercall for setting mem_access > Use unsigned int instead of unsigned long > --- > tools/libxc/include/xenctrl.h | 5 +- > tools/libxc/xc_altp2m.c | 25 ------ > tools/libxc/xc_mem_access.c | 14 +-- > tools/tests/xen-access/xen-access.c | 53 ++++------- Acked-by: Wei Liu <wei.liu2@citrix.com>
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_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). Ian.
Ian Campbell writes ("Re: [PATCH v2 1/2] altp2m: Merge p2m_set_altp2m_mem_access and p2m_set_mem_access"):
> 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'm not sure what a separate version number buys us.
> 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).
If we simply want to detect the mismatch that seems like the best
approach.
It's not like we're short of memory op values.
Ian.
>>> 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_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). Jan
On Mon, 2016-02-01 at 09:21 -0700, Jan Beulich wrote: > > > > 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_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? Ian.
On Mon, Feb 1, 2016 at 9:30 AM, Ian Campbell <ian.campbell@citrix.com> wrote: > On Mon, 2016-02-01 at 09:21 -0700, Jan Beulich wrote: > > > > > 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_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
>>> On 01.02.16 at 17:30, <ian.campbell@citrix.com> wrote: > On Mon, 2016-02-01 at 09:21 -0700, Jan Beulich wrote: >> > > > 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_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? Oh, yes, right. I'm sorry for confusing things. Jan
On Mon, Feb 1, 2016 at 9:21 AM, Jan Beulich <JBeulich@suse.com> wrote: > >>> 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_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) > Supporting the two versions in parallel is not much effort, we would just have to add an extra check to the current version to fail if altp2m is enabled. Tamas
>>> On 01.02.16 at 16:22, <Ian.Jackson@eu.citrix.com> wrote: > Ian Campbell writes ("Re: [PATCH v2 1/2] altp2m: Merge > p2m_set_altp2m_mem_access and p2m_set_mem_access"): >> 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'm not sure what a separate version number buys us. > >> 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). > > If we simply want to detect the mismatch that seems like the best > approach. > > It's not like we're short of memory op values. Are we not? They need to fit in 6 bits (unless we want to play tricks), and numbers up to 27 are already in use. Jan
>>> On 01.02.16 at 17:35, <tlengyel@novetta.com> wrote: > On Mon, Feb 1, 2016 at 9:30 AM, Ian Campbell <ian.campbell@citrix.com> > wrote: > >> On Mon, 2016-02-01 at 09:21 -0700, Jan Beulich wrote: >> > > > > 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_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. I think this was intentional, so a guest could control the access rights in the various views of its P2M. Jan
Jan Beulich writes ("Re: [PATCH v2 1/2] altp2m: Merge p2m_set_altp2m_mem_access and p2m_set_mem_access"):
> On 01.02.16 at 16:22, <Ian.Jackson@eu.citrix.com> wrote:
> > It's not like we're short of memory op values.
>
> Are we not? They need to fit in 6 bits (unless we want to play tricks),
> and numbers up to 27 are already in use.
Maybe I am confused. It's hard to make sense of the actual ABI which
doesn't seem to be documented yet.
...
I have just read the docs some more and found this:
/*
* To allow safe resume of do_memory_op() after preemption, we need
* to know at what point in the page list to resume. For this
* purpose I steal the high-order bits of the @cmd parameter, which
* are otherwise unused and zero.
*
* Note that both of these values are effectively part of the ABI,
* even if we don't need to make them a formal part of it: A guest
* suspended for migration in the middle of a continuation would
* fail to work if resumed on a hypervisor using different values.
*/
#define MEMOP_EXTENT_SHIFT 6 /* cmd[:6] == start_extent */
#define MEMOP_CMD_MASK ((1 << MEMOP_EXTENT_SHIFT) - 1)
Urrrrggh!
If we run out of memory_op numbers, can't we just invent a new
hypercall ?
Ian.
>>> On 01.02.16 at 17:58, <Ian.Jackson@eu.citrix.com> wrote: > Jan Beulich writes ("Re: [PATCH v2 1/2] altp2m: Merge > p2m_set_altp2m_mem_access and p2m_set_mem_access"): >> On 01.02.16 at 16:22, <Ian.Jackson@eu.citrix.com> wrote: >> > It's not like we're short of memory op values. >> >> Are we not? They need to fit in 6 bits (unless we want to play tricks), >> and numbers up to 27 are already in use. > > Maybe I am confused. It's hard to make sense of the actual ABI which > doesn't seem to be documented yet. > > ... > > I have just read the docs some more and found this: > > /* > * To allow safe resume of do_memory_op() after preemption, we need > * to know at what point in the page list to resume. For this > * purpose I steal the high-order bits of the @cmd parameter, which > * are otherwise unused and zero. > * > * Note that both of these values are effectively part of the ABI, > * even if we don't need to make them a formal part of it: A guest > * suspended for migration in the middle of a continuation would > * fail to work if resumed on a hypervisor using different values. > */ > #define MEMOP_EXTENT_SHIFT 6 /* cmd[:6] == start_extent */ > #define MEMOP_CMD_MASK ((1 << MEMOP_EXTENT_SHIFT) - 1) > > Urrrrggh! No-one said this is nice. And we had an issue already when this once got widened from (iirc) 4 to 6 bits. > If we run out of memory_op numbers, can't we just invent a new > hypercall ? I suppose we would need to, yes. Jan
diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h index e632b1e..b4e57d8 100644 --- a/tools/libxc/include/xenctrl.h +++ b/tools/libxc/include/xenctrl.h @@ -2027,9 +2027,6 @@ int xc_altp2m_destroy_view(xc_interface *handle, domid_t domid, /* Switch all vCPUs of the domain to the specified altp2m view */ int xc_altp2m_switch_to_view(xc_interface *handle, domid_t domid, uint16_t view_id); -int xc_altp2m_set_mem_access(xc_interface *handle, domid_t domid, - uint16_t view_id, xen_pfn_t gfn, - xenmem_access_t access); int xc_altp2m_change_gfn(xc_interface *handle, domid_t domid, uint16_t view_id, xen_pfn_t old_gfn, xen_pfn_t new_gfn); @@ -2062,7 +2059,7 @@ int xc_mem_paging_load(xc_interface *xch, domid_t domain_id, */ int xc_set_mem_access(xc_interface *xch, domid_t domain_id, xenmem_access_t access, uint64_t first_pfn, - uint32_t nr); + uint32_t nr, uint16_t altp2m_idx); /* * Gets the mem access for the given page (returned in access on success) diff --git a/tools/libxc/xc_altp2m.c b/tools/libxc/xc_altp2m.c index 0639632..4f44a7b 100644 --- a/tools/libxc/xc_altp2m.c +++ b/tools/libxc/xc_altp2m.c @@ -163,31 +163,6 @@ int xc_altp2m_switch_to_view(xc_interface *handle, domid_t domid, return rc; } -int xc_altp2m_set_mem_access(xc_interface *handle, domid_t domid, - uint16_t view_id, xen_pfn_t gfn, - xenmem_access_t access) -{ - int rc; - DECLARE_HYPERCALL_BUFFER(xen_hvm_altp2m_op_t, arg); - - arg = xc_hypercall_buffer_alloc(handle, arg, sizeof(*arg)); - if ( arg == NULL ) - return -1; - - arg->version = HVMOP_ALTP2M_INTERFACE_VERSION; - arg->cmd = HVMOP_altp2m_set_mem_access; - arg->domain = domid; - arg->u.set_mem_access.view = view_id; - arg->u.set_mem_access.hvmmem_access = access; - arg->u.set_mem_access.gfn = gfn; - - rc = xencall2(handle->xcall, __HYPERVISOR_hvm_op, HVMOP_altp2m, - HYPERCALL_BUFFER_AS_ARG(arg)); - - xc_hypercall_buffer_free(handle, arg); - return rc; -} - int xc_altp2m_change_gfn(xc_interface *handle, domid_t domid, uint16_t view_id, xen_pfn_t old_gfn, xen_pfn_t new_gfn) diff --git a/tools/libxc/xc_mem_access.c b/tools/libxc/xc_mem_access.c index 3634c39..d6fb409 100644 --- a/tools/libxc/xc_mem_access.c +++ b/tools/libxc/xc_mem_access.c @@ -27,15 +27,17 @@ int xc_set_mem_access(xc_interface *xch, domid_t domain_id, xenmem_access_t access, uint64_t first_pfn, - uint32_t nr) + uint32_t nr, + uint16_t altp2m_idx) { xen_mem_access_op_t mao = { - .op = XENMEM_access_op_set_access, - .domid = domain_id, - .access = access, - .pfn = first_pfn, - .nr = nr + .op = XENMEM_access_op_set_access, + .domid = domain_id, + .access = access, + .pfn = first_pfn, + .nr = nr, + .altp2m_idx = altp2m_idx }; return do_memory_op(xch, XENMEM_access_op, &mao, sizeof(mao)); diff --git a/tools/tests/xen-access/xen-access.c b/tools/tests/xen-access/xen-access.c index 7993947..2300e9a 100644 --- a/tools/tests/xen-access/xen-access.c +++ b/tools/tests/xen-access/xen-access.c @@ -448,9 +448,6 @@ int main(int argc, char *argv[]) /* With altp2m we just create a new, restricted view of the memory */ if ( altp2m ) { - xen_pfn_t gfn = 0; - unsigned long perm_set = 0; - rc = xc_altp2m_set_domain_state( xch, domain_id, 1 ); if ( rc < 0 ) { @@ -466,17 +463,6 @@ int main(int argc, char *argv[]) } DPRINTF("altp2m view created with id %u\n", altp2m_view_id); - DPRINTF("Setting altp2m mem_access permissions.. "); - - for(; gfn < xenaccess->max_gpfn; ++gfn) - { - rc = xc_altp2m_set_mem_access( xch, domain_id, altp2m_view_id, gfn, - default_access); - if ( !rc ) - perm_set++; - } - - DPRINTF("done! Permissions set on %lu pages.\n", perm_set); rc = xc_altp2m_switch_to_view( xch, domain_id, altp2m_view_id ); if ( rc < 0 ) @@ -493,25 +479,22 @@ int main(int argc, char *argv[]) } } - if ( !altp2m ) + /* Set the default access type and convert all pages to it */ + rc = xc_set_mem_access(xch, domain_id, default_access, ~0ull, 0, 0); + if ( rc < 0 ) { - /* Set the default access type and convert all pages to it */ - rc = xc_set_mem_access(xch, domain_id, default_access, ~0ull, 0); - if ( rc < 0 ) - { - ERROR("Error %d setting default mem access type\n", rc); - goto exit; - } + ERROR("Error %d setting default mem access type\n", rc); + goto exit; + } - rc = xc_set_mem_access(xch, domain_id, default_access, START_PFN, - (xenaccess->max_gpfn - START_PFN) ); + rc = xc_set_mem_access(xch, domain_id, default_access, START_PFN, + (xenaccess->max_gpfn - START_PFN), altp2m_view_id ); - if ( rc < 0 ) - { - ERROR("Error %d setting all memory to access type %d\n", rc, - default_access); - goto exit; - } + if ( rc < 0 ) + { + ERROR("Error %d setting all memory to access type %d\n", rc, + default_access); + goto exit; } if ( breakpoint ) @@ -547,12 +530,12 @@ int main(int argc, char *argv[]) for ( vcpu_id = 0; vcpu_id<XEN_LEGACY_MAX_VCPUS; vcpu_id++) rc = control_singlestep(xch, domain_id, vcpu_id, 0); - } else { - rc = xc_set_mem_access(xch, domain_id, XENMEM_access_rwx, ~0ull, 0); - rc = xc_set_mem_access(xch, domain_id, XENMEM_access_rwx, START_PFN, - (xenaccess->max_gpfn - START_PFN) ); } + rc = xc_set_mem_access(xch, domain_id, XENMEM_access_rwx, ~0ull, 0, 0); + rc = xc_set_mem_access(xch, domain_id, XENMEM_access_rwx, START_PFN, + (xenaccess->max_gpfn - START_PFN), 0 ); + shutting_down = 1; } @@ -623,7 +606,7 @@ int main(int argc, char *argv[]) else if ( default_access != after_first_access ) { rc = xc_set_mem_access(xch, domain_id, after_first_access, - req.u.mem_access.gfn, 1); + req.u.mem_access.gfn, 1, 0); if (rc < 0) { ERROR("Error %d setting gfn to access_type %d\n", rc, diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c index 2190908..8568087 100644 --- a/xen/arch/arm/p2m.c +++ b/xen/arch/arm/p2m.c @@ -1709,13 +1709,13 @@ bool_t p2m_mem_access_check(paddr_t gpa, vaddr_t gla, const struct npfec npfec) if ( npfec.write_access && xma == XENMEM_access_rx2rw ) { rc = p2m_set_mem_access(v->domain, _gfn(paddr_to_pfn(gpa)), 1, - 0, ~0, XENMEM_access_rw); + 0, ~0, XENMEM_access_rw, 0); return false; } else if ( xma == XENMEM_access_n2rwx ) { rc = p2m_set_mem_access(v->domain, _gfn(paddr_to_pfn(gpa)), 1, - 0, ~0, XENMEM_access_rwx); + 0, ~0, XENMEM_access_rwx, 0); } /* Otherwise, check if there is a vm_event monitor subscriber */ @@ -1737,7 +1737,7 @@ bool_t p2m_mem_access_check(paddr_t gpa, vaddr_t gla, const struct npfec npfec) /* A listener is not required, so clear the access * restrictions. */ rc = p2m_set_mem_access(v->domain, _gfn(paddr_to_pfn(gpa)), 1, - 0, ~0, XENMEM_access_rwx); + 0, ~0, XENMEM_access_rwx, 0); } } @@ -1788,7 +1788,8 @@ bool_t p2m_mem_access_check(paddr_t gpa, vaddr_t gla, const struct npfec npfec) * If gfn == INVALID_GFN, sets the default access type. */ long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr, - uint32_t start, uint32_t mask, xenmem_access_t access) + uint32_t start, uint32_t mask, xenmem_access_t access, + unsigned int altp2m_idx) { struct p2m_domain *p2m = p2m_get_hostp2m(d); p2m_access_t a; diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index 674feea..37305fb 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -6398,9 +6398,9 @@ static int do_altp2m_op( if ( a.u.set_mem_access.pad ) rc = -EINVAL; else - rc = p2m_set_altp2m_mem_access(d, a.u.set_mem_access.view, - _gfn(a.u.set_mem_access.gfn), - a.u.set_mem_access.hvmmem_access); + rc = p2m_set_mem_access(d, _gfn(a.u.set_mem_access.gfn), 1, 0, 0, + a.u.set_mem_access.hvmmem_access, + a.u.set_mem_access.view); break; case HVMOP_altp2m_change_gfn: diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c index a45ee35..a3181f3 100644 --- a/xen/arch/x86/mm/p2m.c +++ b/xen/arch/x86/mm/p2m.c @@ -1777,14 +1777,57 @@ bool_t p2m_mem_access_check(paddr_t gpa, unsigned long gla, return (p2ma == p2m_access_n2rwx); } +static int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m, + struct p2m_domain *ap2m, p2m_access_t a, + unsigned long gfn) +{ + mfn_t mfn; + p2m_type_t t; + p2m_access_t old_a; + unsigned int page_order; + int rc; + + mfn = ap2m->get_entry(ap2m, gfn, &t, &old_a, 0, NULL, NULL); + + /* Check host p2m if no valid entry in alternate */ + if ( !mfn_valid(mfn) ) + { + mfn = hp2m->get_entry(hp2m, gfn, &t, &old_a, + P2M_ALLOC | P2M_UNSHARE, &page_order, NULL); + + rc = -ESRCH; + if ( !mfn_valid(mfn) || t != p2m_ram_rw ) + goto out; + + /* If this is a superpage, copy that first */ + if ( page_order != PAGE_ORDER_4K ) + { + unsigned long mask = ~((1UL << page_order) - 1); + gfn_t gfn2 = _gfn(gfn & mask); + mfn_t mfn2 = _mfn(mfn_x(mfn) & mask); + + rc = ap2m->set_entry(ap2m, gfn_x(gfn2), mfn2, page_order, t, old_a, 1); + if ( rc ) + goto out; + } + } + + rc = ap2m->set_entry(ap2m, gfn, mfn, PAGE_ORDER_4K, t, a, + (current->domain != d)); + + out: + return rc; +} + /* * Set access type for a region of gfns. * If gfn == INVALID_GFN, sets the default access type. */ long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr, - uint32_t start, uint32_t mask, xenmem_access_t access) + uint32_t start, uint32_t mask, xenmem_access_t access, + unsigned int altp2m_idx) { - struct p2m_domain *p2m = p2m_get_hostp2m(d); + struct p2m_domain *p2m = p2m_get_hostp2m(d), *ap2m = NULL; p2m_access_t a, _a; p2m_type_t t; mfn_t mfn; @@ -1806,6 +1849,16 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr, #undef 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; + + ap2m = d->arch.altp2m_p2m[altp2m_idx]; + } + switch ( access ) { case 0 ... ARRAY_SIZE(memaccess) - 1: @@ -1826,12 +1879,25 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr, } p2m_lock(p2m); + if ( ap2m ) + p2m_lock(ap2m); + for ( gfn_l = gfn_x(gfn) + start; nr > start; ++gfn_l ) { - mfn = p2m->get_entry(p2m, gfn_l, &t, &_a, 0, NULL, NULL); - rc = p2m->set_entry(p2m, gfn_l, mfn, PAGE_ORDER_4K, t, a, -1); - if ( rc ) - break; + if ( ap2m ) + { + rc = p2m_set_altp2m_mem_access(d, p2m, ap2m, a, gfn_l); + /* If the corresponding mfn is invalid we will just skip it */ + if ( rc && rc != -ESRCH ) + break; + } + else + { + mfn = p2m->get_entry(p2m, gfn_l, &t, &_a, 0, NULL, NULL); + rc = p2m->set_entry(p2m, gfn_l, mfn, PAGE_ORDER_4K, t, a, -1); + if ( rc ) + break; + } /* Check for continuation if it's not the last iteration. */ if ( nr > ++start && !(start & mask) && hypercall_preempt_check() ) @@ -1840,7 +1906,11 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr, break; } } + + if ( ap2m ) + p2m_unlock(ap2m); p2m_unlock(p2m); + return rc; } @@ -2395,93 +2465,6 @@ int p2m_switch_domain_altp2m_by_id(struct domain *d, unsigned int idx) return rc; } -int p2m_set_altp2m_mem_access(struct domain *d, unsigned int idx, - gfn_t gfn, xenmem_access_t access) -{ - struct p2m_domain *hp2m, *ap2m; - p2m_access_t req_a, old_a; - p2m_type_t t; - mfn_t mfn; - unsigned int page_order; - int rc = -EINVAL; - - static const p2m_access_t memaccess[] = { -#define ACCESS(ac) [XENMEM_access_##ac] = p2m_access_##ac - ACCESS(n), - ACCESS(r), - ACCESS(w), - ACCESS(rw), - ACCESS(x), - ACCESS(rx), - ACCESS(wx), - ACCESS(rwx), -#undef ACCESS - }; - - if ( idx >= MAX_ALTP2M || d->arch.altp2m_eptp[idx] == INVALID_MFN ) - return rc; - - ap2m = d->arch.altp2m_p2m[idx]; - - switch ( access ) - { - case 0 ... ARRAY_SIZE(memaccess) - 1: - req_a = memaccess[access]; - break; - case XENMEM_access_default: - req_a = ap2m->default_access; - break; - default: - return rc; - } - - /* If request to set default access */ - if ( gfn_x(gfn) == INVALID_GFN ) - { - ap2m->default_access = req_a; - return 0; - } - - hp2m = p2m_get_hostp2m(d); - - p2m_lock(ap2m); - - mfn = ap2m->get_entry(ap2m, gfn_x(gfn), &t, &old_a, 0, NULL, NULL); - - /* Check host p2m if no valid entry in alternate */ - if ( !mfn_valid(mfn) ) - { - mfn = hp2m->get_entry(hp2m, gfn_x(gfn), &t, &old_a, - P2M_ALLOC | P2M_UNSHARE, &page_order, NULL); - - if ( !mfn_valid(mfn) || t != p2m_ram_rw ) - goto out; - - /* If this is a superpage, copy that first */ - if ( page_order != PAGE_ORDER_4K ) - { - gfn_t gfn2; - unsigned long mask; - mfn_t mfn2; - - mask = ~((1UL << page_order) - 1); - gfn2 = _gfn(gfn_x(gfn) & mask); - mfn2 = _mfn(mfn_x(mfn) & mask); - - if ( ap2m->set_entry(ap2m, gfn_x(gfn2), mfn2, page_order, t, old_a, 1) ) - goto out; - } - } - - if ( !ap2m->set_entry(ap2m, gfn_x(gfn), mfn, PAGE_ORDER_4K, t, req_a, - (current->domain != d)) ) - rc = 0; - - out: - p2m_unlock(ap2m); - return rc; -} - int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx, gfn_t old_gfn, gfn_t new_gfn) { diff --git a/xen/common/mem_access.c b/xen/common/mem_access.c index 159c036..0411443 100644 --- a/xen/common/mem_access.c +++ b/xen/common/mem_access.c @@ -67,7 +67,7 @@ int mem_access_memop(unsigned long cmd, break; rc = p2m_set_mem_access(d, _gfn(mao.pfn), mao.nr, start_iter, - MEMOP_CMD_MASK, mao.access); + MEMOP_CMD_MASK, mao.access, mao.altp2m_idx); if ( rc > 0 ) { ASSERT(!(rc & MEMOP_CMD_MASK)); diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h index fa46dd9..c0df1ea 100644 --- a/xen/include/asm-x86/p2m.h +++ b/xen/include/asm-x86/p2m.h @@ -808,10 +808,6 @@ int p2m_destroy_altp2m_by_id(struct domain *d, unsigned int idx); /* Switch alternate p2m for entire domain */ int p2m_switch_domain_altp2m_by_id(struct domain *d, unsigned int idx); -/* Set access type for a gfn */ -int p2m_set_altp2m_mem_access(struct domain *d, unsigned int idx, - gfn_t gfn, xenmem_access_t access); - /* Change a gfn->mfn mapping */ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx, gfn_t old_gfn, gfn_t new_gfn); diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h index 4df38d6..30ccb41 100644 --- 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; /* * First pfn for set op * pfn for get op diff --git a/xen/include/xen/p2m-common.h b/xen/include/xen/p2m-common.h index 47c40c7..8b70459 100644 --- a/xen/include/xen/p2m-common.h +++ b/xen/include/xen/p2m-common.h @@ -49,7 +49,8 @@ int unmap_mmio_regions(struct domain *d, * If gfn == INVALID_GFN, sets the default access type. */ long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr, - uint32_t start, uint32_t mask, xenmem_access_t access); + uint32_t start, uint32_t mask, xenmem_access_t access, + unsigned int altp2m_idx); /* * Get access type for a gfn.
The altp2m subsystem in its current form uses its own HVMOP hypercall to set mem_access permissions, duplicating some of the code already present for setting regular mem_access permissions. In this patch we consolidate the two and update the corresponding tools. Signed-off-by: Tamas K Lengyel <tlengyel@novetta.com> Cc: Ian Jackson <ian.jackson@eu.citrix.com> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com> Cc: Ian Campbell <ian.campbell@citrix.com> Cc: Wei Liu <wei.liu2@citrix.com> Cc: Razvan Cojocaru <rcojocaru@bitdefender.com> Cc: Stefano Stabellini <stefano.stabellini@citrix.com> Cc: Keir Fraser <keir@xen.org> Cc: Jan Beulich <jbeulich@suse.com> Cc: Andrew Cooper <andrew.cooper3@citrix.com> Cc: George Dunlap <george.dunlap@eu.citrix.com> --- v2: Don't deprecate the HVMOP hypercall for setting mem_access Use unsigned int instead of unsigned long --- tools/libxc/include/xenctrl.h | 5 +- tools/libxc/xc_altp2m.c | 25 ------ tools/libxc/xc_mem_access.c | 14 +-- tools/tests/xen-access/xen-access.c | 53 ++++------- xen/arch/arm/p2m.c | 9 +- xen/arch/x86/hvm/hvm.c | 6 +- xen/arch/x86/mm/p2m.c | 169 ++++++++++++++++-------------------- xen/common/mem_access.c | 2 +- xen/include/asm-x86/p2m.h | 4 - xen/include/public/memory.h | 3 + xen/include/xen/p2m-common.h | 3 +- 11 files changed, 117 insertions(+), 176 deletions(-)