[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v2 1/2] altp2m: Merge p2m_set_altp2m_mem_access and p2m_set_mem_access
>>> On 29.01.16 at 17:12, <tlengyel@xxxxxxxxxxx> wrote: > On Fri, Jan 29, 2016 at 4:03 AM, Jan Beulich <JBeulich@xxxxxxxx> wrote: >> >>> On 28.01.16 at 21:58, <tlengyel@xxxxxxxxxxx> 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 _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |