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

["Jan Beulich" <JBeulich@suse.com>]

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