Re: [Xen-devel] [PATCH 5/6] libxl: support mapping static shared memory areas during domain creation

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Wed, Aug 23, 2017 at 02:08:39AM +0800, Zhongze Liu wrote:
[...]
> diff --git a/tools/libxl/libxl_arch.h b/tools/libxl/libxl_arch.h
> index 5e1fc6060e..1d681d8863 100644
> --- a/tools/libxl/libxl_arch.h
> +++ b/tools/libxl/libxl_arch.h
> @@ -71,6 +71,12 @@ int libxl__arch_extra_memory(libxl__gc *gc,
>                               const libxl_domain_build_info *info,
>                               uint64_t *out);
>  
> +_hidden
> +bool libxl__arch_domain_support_sshm(const libxl_domain_build_info *b_info);

No need to take any argument afaict.

[...]
> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index 1158303e1a..8e5ec486d2 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -501,6 +501,14 @@ int libxl__domain_build(libxl__gc *gc,
>          ret = ERROR_INVAL;
>          goto out;
>      }
> +
> +    /* the p2m has been setup, we could map the static shared memory now. */
> +    ret = libxl__sshm_add(gc, domid, d_config->sshms, d_config->num_sshms);
> +    if (ret != 0) {
> +        LOG(ERROR, "failed to map static shared memory");
> +        goto out;
> +    }
> +
>      ret = libxl__build_post(gc, domid, info, state, vments, localents);
>  out:
>      return ret;
> @@ -918,6 +926,25 @@ static void initiate_domain_create(libxl__egc *egc,
>          goto error_out;
>      }
>  
> +    if (d_config->num_sshms != 0 &&
> +        !libxl__arch_domain_support_sshm(&d_config->b_info)) {
> +        LOGD(ERROR, domid, "static_shm is not supported by this domain 
> type.");
> +        ret = ERROR_INVAL;
> +        goto error_out;
> +    }
> +
> +    ret = libxl__sshm_check_overlap(gc, domid,
> +                                    d_config->sshms, d_config->num_sshms);
> +    if (ret) goto error_out;

Better move this after setting default.

> +
> +    for (i = 0; i < d_config->num_sshms; ++i) {
> +        ret = libxl__sshm_setdefault(gc, domid, &d_config->sshms[i]);
> +        if (ret) {
> +            LOGD(ERROR, domid, "Unable to set defaults for static sshm");
> +            goto error_out;
> +        }
> +    }
> +
>      ret = libxl__domain_make(gc, d_config, &domid, &state->config);
>      if (ret) {
>          LOGD(ERROR, domid, "cannot make domain: %d", ret);
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 724750967c..74bc0acb21 100644
[...]
> diff --git a/tools/libxl/libxl_sshm.c b/tools/libxl/libxl_sshm.c
> new file mode 100644
> index 0000000000..e16c24ccb9
> --- /dev/null
> +++ b/tools/libxl/libxl_sshm.c
> @@ -0,0 +1,336 @@
> +#include "libxl_osdeps.h"
> +#include "libxl_internal.h"
> +#include "libxl_arch.h"
> +
> +#define SSHM_ERROR(domid, sshmid, f, ...)                               \
> +    LOGD(ERROR, domid, "static_shm id = %s: " f, sshmid, ##__VA_ARGS__)
> +
> +
> +/* Set default values for libxl_static_shm */
> +int libxl__sshm_setdefault(libxl__gc *gc, uint32_t domid,
> +                                  libxl_static_shm *sshm)

Indentation.

> +{
> +    int rc;
> +
> +    if (sshm->role == LIBXL_SSHM_ROLE_UNKNOWN) {
> +        sshm->role = LIBXL_SSHM_ROLE_SLAVE;
> +    }
> +    if (sshm->prot == LIBXL_SSHM_PROT_UNKNOWN) {
> +        sshm->prot = LIBXL_SSHM_PROT_RW;
> +    }

You can avoid using {} when there is only one statement.

> +    /* role-specific checks */
> +    if (LIBXL_SSHM_ROLE_SLAVE == sshm->role) {

Style.

> +        if (sshm->offset == LIBXL_SSHM_RANGE_UNKNOWN) {
> +            sshm->offset = 0;
> +        }
> +        if (sshm->cache_policy != LIBXL_SSHM_CACHEPOLICY_UNKNOWN) {
> +            SSHM_ERROR(domid, sshm->id,
> +                       "cache_policy is only applicable to master domains");
> +            return ERROR_INVAL;
> +        }
> +    } else {
> +        if (sshm->offset != LIBXL_SSHM_RANGE_UNKNOWN) {
> +            SSHM_ERROR(domid, sshm->id,
> +                       "offset is only applicable to slave domains");
> +            return ERROR_INVAL;
> +        }
> +        rc = libxl__arch_domain_sshm_cachepolicy_setdefault(sshm);
> +        if (rc) {
> +            SSHM_ERROR(domid, sshm->id,
> +                       "cache policy not supported on this platform");
> +            return ERROR_INVAL;
> +        }
> +    }

Please use goto style error handling.

> +
> +    return 0;
> +}
> +
> +/* Compare function for sorting sshm ranges by sshm->begin */
> +static int sshm_range_cmp(const void *a, const void *b)
> +{
> +    libxl_static_shm *const *sshma = a, *const *sshmb = b;
> +    return (*sshma)->begin > (*sshmb)->begin ? 1 : -1;
> +}
> +
> +/* 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;
> +
> +    slave_sshms = libxl__calloc(gc, len, sizeof(slave_sshms[0]));
> +    num_slaves = 0;
> +    for (i = 0; i < len; ++i) {
> +        if (LIBXL_SSHM_ROLE_SLAVE == sshms[i].role)

Style.

> +/* 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,
> +                              uint64_t mbegin, uint64_t mend)
> +{
> +    int rc;
> +    int i;
> +    unsigned int num_mpages, num_spages, offset;
> +    int *errs;
> +    xen_ulong_t *idxs;
> +    xen_pfn_t *gpfns;
> +
> +    num_mpages = (mend - mbegin) >> 12;
> +    num_spages = (sshm->end - sshm->begin) >> 12;
> +    offset = sshm->offset >> 12;
> +
> +    /* 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] = (mbegin >> 12) + offset + i;
> +        gpfns[i]= (sshm->begin >> 12) + i;
> +    }
> +    rc = xc_domain_add_to_physmap_batch(CTX->xch,
> +                                        sid, mid,
> +                                        XENMAPSPACE_gmfn_foreign,
> +                                        num_spages,
> +                                        idxs, gpfns, errs);
> +
> +    for (i = 0; i< num_spages; i++) {
> +        if (errs[i]) {
> +            SSHM_ERROR(sid, sshm->id,
> +                       "can't map at address 0x%"PRIx64".",
> +                       sshm->begin + (offset << 12) );
> +            rc = ERROR_FAIL;
> +        }
> +    }
> +    if (rc) goto out;
> +

How is partial failure handled?

> +    rc = 0;
> +
> +out:
> +    return rc;
> +}
> +
> +static int libxl__sshm_add_slave(libxl__gc *gc, uint32_t domid,
> +                                 libxl_static_shm *sshm)
> +{
[...]
> +
> +        /* check if the slave is asking too much permission */
> +        if (LIBXL_SSHM_PROT_UNKNOWN == sshm->prot) {
> +            sshm->prot = master_sshm.prot;
> +        }

Style.

> +int libxl__sshm_add(libxl__gc *gc,  uint32_t domid,
> +                    libxl_static_shm *sshms, int len)
> +{
> +    int rc, i;
> +
> +    if (!len) return 0;

Unneeded check.

> +
> +    for (i = 0; i < len; ++i) {
> +        if (LIBXL_SSHM_ROLE_SLAVE == sshms[i].role) {
> +            rc = libxl__sshm_add_slave(gc, domid, sshms+i);
> +        } else {
> +            rc = libxl__sshm_add_master(gc, domid, sshms+i);
> +        }
> +        if (rc)  return rc;
> +    }
> +
> +    return 0;
> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
[...]
> diff --git a/tools/libxl/libxl_xshelp.c b/tools/libxl/libxl_xshelp.c
> index c4a18df353..d91fbf5fda 100644
> --- a/tools/libxl/libxl_xshelp.c
> +++ b/tools/libxl/libxl_xshelp.c
> @@ -193,6 +193,14 @@ char *libxl__xs_libxl_path(libxl__gc *gc, uint32_t domid)
>      return s;
>  }
>  
> +char *libxl__xs_get_sshmpath(libxl__gc *gc, const char *id)
> +{
> +    char *s = GCSPRINTF("/local/static_shm/%s", id);


This can't fail.

> +    if (!s)
> +        LOGE(ERROR, "cannot allocate static shm path");
> +    return s;
> +}
> +
>  int libxl__xs_read_mandatory(libxl__gc *gc, xs_transaction_t t,
>                               const char *path, const char **result_out)
>  {
> -- 
> 2.14.0
> 

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