Re: [Xen-devel] [PATCH v2 1/2] altp2m: Merge p2m_set_altp2m_mem_access and p2m_set_mem_access

["Lengyel, Tamas" <tlengyel@xxxxxxxxxxx>]

On Fri, Jan 29, 2016 at 9:19 AM, Jan Beulich <JBeulich@xxxxxxxx> wrote:

>>> 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.

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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel