Re: [Xen-devel] [PATCH v3 4/7] libxl: support mapping static shared memory areas during domain creation

[Zhongze Liu <blackskygg@xxxxxxxxx>]

Hi Wei,

2017-11-01 23:55 GMT+08:00 Wei Liu <wei.liu2@xxxxxxxxxx>:
> On Thu, Oct 19, 2017 at 10:36:32AM +0800, Zhongze Liu wrote:
>> Add libxl__sshm_add to map shared pages from one DomU to another, The mapping
>> process involves the follwing steps:
>>
>>   * Set defaults and check for further errors in the static_shm configs:
>>     overlapping areas, invalid ranges, duplicated master domain,
>>     no master domain etc.
>>   * Write infomation of static shared memory areas into the appropriate
>>     xenstore paths.
>>   * Use xc_domain_add_to_physmap_batch to do the page sharing.
>>   * Set the refcount of the shared region accordingly
>>
>> Temporarily mark this as unsupported on x86 because calling p2m_add_foregin 
>> on
>> two domU's is currently not allowd on x86 (see the comments in
>> x86/mm/p2m.c:p2m_add_foregin for more details).
>>
>> This is for the proposal "Allow setting up shared memory areas between VMs
>> from xl config file" (see [1]).
>>
>> [1] https://lists.xen.org/archives/html/xen-devel/2017-08/msg03242.html
>>
>> Signed-off-by: Zhongze Liu <blackskygg@xxxxxxxxx>
>>
>> Cc: Wei Liu <wei.liu2@xxxxxxxxxx>
>> Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
>> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>> Cc: Julien Grall <julien.grall@xxxxxxx>
>> Cc: xen-devel@xxxxxxxxxxxxx
>> ---
>>   V3:
>>   * unmap the successfully mapped pages whenever rc != 0
>
>
> A general note: please properly capitalise the comments.
>
>> ---
>>  tools/libxl/Makefile         |   2 +-
>>  tools/libxl/libxl_arch.h     |   6 +
>>  tools/libxl/libxl_arm.c      |  15 ++
>>  tools/libxl/libxl_create.c   |  27 +++
>>  tools/libxl/libxl_internal.h |  13 ++
>>  tools/libxl/libxl_sshm.c     | 395 
>> +++++++++++++++++++++++++++++++++++++++++++
>>  tools/libxl/libxl_x86.c      |  18 ++
>>  7 files changed, 475 insertions(+), 1 deletion(-)
>>  create mode 100644 tools/libxl/libxl_sshm.c
>>
>> diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
>> index 5a861f72cb..91bc70cda2 100644
>> --- a/tools/libxl/Makefile
>> +++ b/tools/libxl/Makefile
> [...]
>> +
>> +/* check if the sshm slave configs in @sshm overlap */
>> +int libxl__sshm_check_overlap(libxl__gc *gc, uint32_t domid,
>> +                                     libxl_static_shm *sshms, int len)
>> +{
>> +
>> +    const libxl_static_shm **slave_sshms = NULL;
>> +    int num_slaves;
>> +    int i;
>> +
>> +    if (!len) return 0;
>> +
>> +    slave_sshms = libxl__calloc(gc, len, sizeof(slave_sshms[0]));
>> +    num_slaves = 0;
>> +    for (i = 0; i < len; ++i) {
>> +        if (sshms[i].role == LIBXL_SSHM_ROLE_SLAVE)
>> +            slave_sshms[num_slaves++] = sshms + i;
>> +    }
>> +    qsort(slave_sshms, num_slaves, sizeof(slave_sshms[0]), sshm_range_cmp);
>> +
>> +    for (i = 0; i < num_slaves - 1; ++i) {
>> +        if (slave_sshms[i+1]->begin < slave_sshms[i]->end) {
>> +            SSHM_ERROR(domid, slave_sshms[i+1]->id, "slave ranges 
>> overlap.");
>> +            return ERROR_INVAL;
>> +        }
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +/*   libxl__sshm_do_map -- map pages into slave's physmap
>> + *
>> + *   This functions maps
>> + *     mater gfn: [@msshm->begin + @sshm->offset, @msshm->end + 
>> @sshm->offset)
>
> "master"
>
> This is confusing. What if offset is not page aligned?

No, it will always be page-aligned. This was documented in the man
page and also checked
in the corresponding libxlu_* function.

>
>> + *   into
>> + *     slave gfn: [@sshm->begin, @sshm->end)
>> + *
>> + *   The gfns of the pages that are successfully mapped will be stored
>> + *   in @mapped, and the number of the gfns will be stored in @nmapped.
>> + *
>> + *   The caller have to guarentee that sshm->begin < sshm->end */
>> +static int libxl__sshm_do_map(libxl__gc *gc, uint32_t mid, uint32_t sid,
>> +                              libxl_static_shm *sshm, libxl_static_shm 
>> *msshm,
>> +                              xen_pfn_t *mapped, unsigned int *nmapped)
>> +{
>> +    int rc;
>> +    int i;
>> +    unsigned int num_mpages, num_spages, num_success, offset;
>> +    int *errs;
>> +    xen_ulong_t *idxs;
>> +    xen_pfn_t *gpfns;
>> +
>> +    num_mpages = (msshm->end - msshm->begin) >> XC_PAGE_SHIFT;
>> +    num_spages = (sshm->end - sshm->begin) >> XC_PAGE_SHIFT;
>> +    offset = sshm->offset >> XC_PAGE_SHIFT;
>> +
>> +    /* Check range. Test offset < mpages first to avoid overflow */
>> +    if ((offset >= num_mpages) || (num_mpages - offset < num_spages)) {
>> +        SSHM_ERROR(sid, sshm->id, "exceeds master's address space.");
>> +        rc = ERROR_INVAL;
>> +        goto out;
>> +    }
>> +
>> +    /* fill out the pfn's and do the mapping */
>> +    errs = libxl__calloc(gc, num_spages, sizeof(int));
>> +    idxs = libxl__calloc(gc, num_spages, sizeof(xen_ulong_t));
>> +    gpfns = libxl__calloc(gc, num_spages, sizeof(xen_pfn_t));
>> +    for (i = 0; i < num_spages; i++) {
>> +        idxs[i] = (msshm->begin >> XC_PAGE_SHIFT) + offset + i;
>> +        gpfns[i]= (sshm->begin >> XC_PAGE_SHIFT) + i;
>> +    }
>> +    rc = xc_domain_add_to_physmap_batch(CTX->xch,
>> +                                        sid, mid,
>> +                                        XENMAPSPACE_gmfn_foreign,
>> +                                        num_spages,
>> +                                        idxs, gpfns, errs);
>> +
>> +    num_success = 0;
>> +    for (i = 0; i < num_spages; i++) {
>> +        if (errs[i]) {
>> +            SSHM_ERROR(sid, sshm->id,
>> +                       "can't map at address 0x%"PRIx64".",
>> +                       gpfns[i] << XC_PAGE_SHIFT);
>> +            rc = ERROR_FAIL;
>> +        } else {
>> +            mapped[num_success++] = gpfns[i];
>> +        }
>> +    }
>> +    *nmapped = num_success;
>> +    if (rc) goto out;
>> +
>> +    rc = 0;
>> +out:
>> +    return rc;
>> +}
>> +
>> +static int libxl__sshm_incref(libxl__gc *gc, xs_transaction_t xt,
>> +                              const char *sshm_path)
>> +{
>> +    int rc, count;
>> +    const char *count_path, *count_string;
>> +
>> +    count_path = GCSPRINTF("%s/users", sshm_path);
>> +    rc = libxl__xs_read_checked(gc, xt, count_path, &count_string);
>> +    if (rc) goto out;
>> +    count = atoi(count_string);
>> +
>> +    count_string = GCSPRINTF("%d", count+1);
>> +    rc = libxl__xs_write_checked(gc, xt, count_path, count_string);
>> +    if (rc) goto out;
>> +
>> +    rc = 0;
>> +out:
>> +    return rc;
>> +}
>> +
> [...]
>> +
>> +        /* all checks passed, do the job */
>> +        if (!isretry) {
>> +            rc = libxl__sshm_do_map(gc, master_domid, domid,
>> +                                    sshm, &master_sshm,
>> +                                    mapped, &nmapped);
>> +            if (rc) goto out;
>> +        }
>> +
>> +        /* write the result to xenstore and commit */
>> +        rc = libxl__xs_write_checked(gc, xt, dom_role_path, "slave");
>> +        if (rc) goto out;
>> +        rc = libxl__xs_writev(gc, xt, slave_path, ents);
>> +        if (rc) goto out;
>> +        rc = libxl__sshm_incref(gc, xt, sshm_path);
>> +        if (rc) goto out;
>> +
>> +        rc = libxl__xs_transaction_commit(gc, &xt);
>> +        if (!rc) break;
>> +        if (rc < 0) goto out;
>> +        isretry = true;
>> +    }
>> +
>> +    rc = 0;
>> +out:
>> +    if (rc) {
>> +        /* role back successfully mapped pages */
>> +        SSHM_ERROR(domid, sshm->id, "failed to map some pages, 
>> cancelling.");
>> +        for (i = 0; i < nmapped; i++) {
>> +            xc_domain_remove_from_physmap(CTX->xch, domid, mapped[i]);
>> +        }
>
> It is possible to get here when the first attempt of mapping has failed.
> The do_map function can return early before it modifies nmapped. At
> first glance this code snippet is buggy.
>
> I would suggest you change isretry to mapped and adjust the predicate
> for rolling back.

nmapped was initialized to 0, so even if do_map fails early (and the
only possible case
where do_map doesn't modify nmapped is when no page was actually mapped), the
value of nmapped stay correct and no pages will be rolled back.

>
>> +    }
>> +
>> +    libxl__xs_transaction_abort(gc, &xt);
>> +
>> +    return rc;
>> +}
>> +
>> +static int libxl__sshm_add_master(libxl__gc *gc, uint32_t domid,
>> +                                  libxl_static_shm *sshm)
>> +{
>> +    int rc;
>> +    const char *sshm_path, *dom_path, *dom_role_path;
>> +    char *ents[13];
>> +    struct xs_permissions noperm;
>> +    xs_transaction_t xt = XBT_NULL;
>> +
>> +    sshm_path = SSHM_PATH(sshm->id);
>> +    dom_path = libxl__xs_get_dompath(gc, domid);
>> +    /* the domain should be in xenstore by now */
>> +    assert(dom_path);
>> +    dom_role_path = GCSPRINTF("%s/static_shm/%s/role", dom_path, sshm->id);
>> +
>> +    /* prepare the xenstore entries */
>> +    ents[0] = "master";
>> +    ents[1] = GCSPRINTF("%"PRIu32, domid);
>> +    ents[2] = "begin";
>> +    ents[3] = GCSPRINTF("0x%"PRIx64, sshm->begin);
>> +    ents[4] = "end";
>> +    ents[5] = GCSPRINTF("0x%"PRIx64, sshm->end);
>> +    ents[6] = "prot";
>> +    ents[7] = libxl__strdup(gc, libxl_sshm_prot_to_string(sshm->prot));
>> +    ents[8] = "cache_policy";
>> +    ents[9] = libxl__strdup(gc,
>> +                            
>> libxl_sshm_cachepolicy_to_string(sshm->cache_policy));
>> +    ents[10] = "users";
>> +    ents[11] = "1";
>> +    ents[12] = NULL;
>> +
>> +    /* could only be accessed by Dom0 */
>> +    noperm.id = 0;
>> +    noperm.perms = XS_PERM_NONE;
>> +
>
> The owner of the path is going to be Dom0 and all other domains have no
> permission, which matches the comment.
>
> Do you not want domid to be able to read? If you don't want it to read
> from the path, why not put in under some other paths like /libxl/?
>

I think this is to be discussed. But in my opinion, the target user won't need
this information because the address is often statically setup and hardcoded
in the simple bare metal apps running inside the DomU's, so I will currently
mark it as noperm and move it to /libxl as you suggested (secure by default).
If, the future, there are needs to read this information from within the DomU's,
we could easily move it back and set the permissions accordingly.

>> +    for (;;) {
>> +        rc = libxl__xs_transaction_start(gc, &xt);
>> +        if (rc) goto out;
>> +
>> +        if (!libxl__xs_read(gc, xt, sshm_path)) {
>> +            /* every ID can appear in each domain at most once */
>> +            if (libxl__xs_read(gc, xt, dom_role_path)) {
>> +                SSHM_ERROR(domid, sshm->id,
>> +                           "domain tried to map the same ID twice.");
>> +                rc = ERROR_FAIL;
>> +                goto out;
>> +            }
>> +            rc = libxl__xs_write_checked(gc, xt, dom_role_path, "master");
>> +            if (rc) goto out;;
>> +
>> +            libxl__xs_mknod(gc, xt, sshm_path, &noperm, 1);
>> +            libxl__xs_writev(gc, xt, sshm_path, ents);
>> +        } else {
>> +            SSHM_ERROR(domid, sshm->id, "can only have one master.");
>> +            rc = ERROR_FAIL;
>> +            goto out;
>> +        }
>> +
>> +        rc = libxl__xs_transaction_commit(gc, &xt);
>> +        if (!rc) break;
>> +        if (rc < 0) goto out;
>> +    }
>> +
>> +    rc = 0;
>> +out:
>> +    libxl__xs_transaction_abort(gc, &xt);
>> +    return rc;
>> +}
>> +
> [...]
>> diff --git a/tools/libxl/libxl_x86.c b/tools/libxl/libxl_x86.c
>> index 5f91fe4f92..450fb3d354 100644
>> --- a/tools/libxl/libxl_x86.c
>> +++ b/tools/libxl/libxl_x86.c
>> @@ -596,6 +596,24 @@ void libxl__arch_domain_build_info_acpi_setdefault(
>>      libxl_defbool_setdefault(&b_info->acpi, true);
>>  }
>>
>> +bool libxl__arch_domain_support_sshm(const libxl_domain_build_info *b_info)
>> +{
>> +    /* FIXME: Mark this as unsupported for calling p2m_add_foreign on two
>> +     * DomU's is currently not allowd on x86, see the comments in
>> +     * x86/mm/p2m.c: p2m_add_foreign */
>
> Move the */ to a new line please. And add "." after "p2m_add_foreign".
>
>> +     return false;
>> +}
>> +
>> +int libxl__arch_domain_sshm_cachepolicy_setdefault(libxl_static_shm *sshm)
>> +{
>> +    if (sshm->cache_policy == LIBXL_SSHM_CACHEPOLICY_UNKNOWN)
>> +        sshm->cache_policy = LIBXL_SSHM_CACHEPOLICY_X86_NORMAL;
>> +    if (sshm->cache_policy < LIBXL_SSHM_CACHEPOLICY_X86_NORMAL)
>> +        return ERROR_INVAL;
>> +
>> +    return 0;
>> +}
>> +
>>  /*
>>   * Local variables:
>>   * mode: C
>> --
>> 2.14.2
>>

Cheers,

Zhongze Liu

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