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

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