|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v8 3/8] libxl: support mapping static shared memory areas during domain creation
Stefano Stabellini writes ("[PATCH v8 3/8] libxl: support mapping static shared
memory areas during domain creation"):
> +_hidden
> +bool libxl__arch_domain_support_sshm(const libxl_domain_build_info *b_info);
> +
> +_hidden
> +int libxl__arch_domain_sshm_cachepolicy_setdefault(libxl_static_shm *sshm);
> +
> #if defined(__i386__) || defined(__x86_64__)
>
> #define LAPIC_BASE_ADDRESS 0xfee00000
> diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
> index 25dc3de..054ad58 100644
> --- a/tools/libxl/libxl_arm.c
> +++ b/tools/libxl/libxl_arm.c
> @@ -1138,6 +1138,21 @@ void
> libxl__arch_domain_build_info_setdefault(libxl__gc *gc,
> libxl_domain_build_info_init_type(b_info, LIBXL_DOMAIN_TYPE_PVH);
> }
>
> +bool libxl__arch_domain_support_sshm(const libxl_domain_build_info *b_info)
> +{
> + return true;
> +}
> +
> +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_ARM_NORMAL;
> + if (sshm->cache_policy >= LIBXL_SSHM_CACHEPOLICY_X86_NORMAL)
> + return ERROR_INVAL;
This cache policy stuff is odd. I couldn't see it being used by the
hypervisor. Why is it even there ?
The same question about prot.
> /*
> * Local variables:
> * mode: C
> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index 320dbed..45ae9e4 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -513,6 +513,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;
> @@ -949,6 +957,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;
> + }
> +
> + 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 shm");
> + goto error_out;
> + }
> + }
> +
> + ret = libxl__sshm_check_overlap(gc, domid,
> + d_config->sshms, d_config->num_sshms);
> + if (ret) goto error_out;
> +
> ret = libxl__domain_make(gc, d_config, &dcs->build_state, &domid);
> 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 43947b1..6f31a3d 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -4434,6 +4434,20 @@ static inline const char
> *libxl__qemu_qmp_path(libxl__gc *gc, int domid)
> #endif
>
> /*
> + * Set up static shared ram pages for HVM domains to communicate
> + *
> + * This function should only be called after the memory map is constructed
> + * and before any further memory access.
> + */
> +_hidden int libxl__sshm_add(libxl__gc *gc, uint32_t domid,
> + libxl_static_shm *sshm, int len);
> +
> +_hidden int libxl__sshm_check_overlap(libxl__gc *gc, uint32_t domid,
> + libxl_static_shm *sshms, int len);
> +_hidden int libxl__sshm_setdefault(libxl__gc *gc, uint32_t domid,
> + libxl_static_shm *sshm);
> +
> +/*
> * Local variables:
> * mode: C
> * c-basic-offset: 4
> diff --git a/tools/libxl/libxl_sshm.c b/tools/libxl/libxl_sshm.c
> new file mode 100644
> index 0000000..f61b80c
> --- /dev/null
> +++ b/tools/libxl/libxl_sshm.c
> @@ -0,0 +1,405 @@
> +#include "libxl_osdeps.h"
> +#include "libxl_internal.h"
> +#include "libxl_arch.h"
> +
> +#define SSHM_PATH(id) GCSPRINTF("/libxl/static_shm/%s", id)
> +
> +#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)
> +{
> + int rc;
> +
> + if (sshm->role != LIBXL_SSHM_ROLE_SLAVE &&
> + sshm->role != LIBXL_SSHM_ROLE_MASTER)
> + return ERROR_INVAL;
> + if (sshm->begin & ~XC_PAGE_MASK ||
> + sshm->size & ~XC_PAGE_MASK ||
> + (sshm->offset != LIBXL_SSHM_RANGE_UNKNOWN &&
> + sshm->offset & ~XC_PAGE_MASK)) {
> + SSHM_ERROR(domid, sshm->id,
> + "begin/size/offset is not a multiple of 4K");
> + return ERROR_INVAL;
> + }
> +
> + /* role-specific checks */
> + if (sshm->role == LIBXL_SSHM_ROLE_SLAVE) {
> + 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");
> + rc = ERROR_INVAL;
> + goto out;
> + }
> + } else {
> + if (sshm->offset != LIBXL_SSHM_RANGE_UNKNOWN) {
> + SSHM_ERROR(domid, sshm->id,
> + "offset is only applicable to slave domains");
> + rc = ERROR_INVAL;
> + goto out;
> + }
> +
> + rc = libxl__arch_domain_sshm_cachepolicy_setdefault(sshm);
> + if (rc) {
> + SSHM_ERROR(domid, sshm->id,
> + "cache policy not supported on this platform");
> + goto out;
> + }
> + }
> +
> + rc = 0;
> +out:
> + return rc;
> +}
> +
> +/* Comparator 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;
> +
> + 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]->begin + slave_sshms[i]->size) {
> + 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
> + * master gfn: [@msshm->begin + @sshm->offset,
> + * @msshm->begin + @msshm->size + @sshm->offset)
> + * into
> + * slave gfn: [@sshm->begin, @sshm->begin + @sshm->size)
> + *
> + * 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 has to guarantee that all the values are page-aligned.
> + */
> +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;
> + xen_pfn_t num_mpages, num_spages, num_success, offset;
> + int *errs;
> + xen_ulong_t *idxs;
> + xen_pfn_t *gpfns;
> +
> + num_mpages = (msshm->size) >> XC_PAGE_SHIFT;
> + num_spages = (sshm->size) >> 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 gfn'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_share,
> + 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;
> +}
> +
> +/* Xenstore ops are protected by a transaction */
> +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/usercnt", sshm_path);
> + rc = libxl__xs_read_checked(gc, xt, count_path, &count_string);
> + if (rc) goto out;
> + count = atoi(count_string);
libxl__xs_read_checked may set count_string to NULL if it was ENOENT.
But see my comments in the docs patch: why is this count here at all ?
> + 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;
> +}
> +
> +static int libxl__sshm_add_slave(libxl__gc *gc, uint32_t domid,
> + libxl_static_shm *sshm)
> +{
> + int rc, i;
> + const char *sshm_path, *slave_path;
> + const char *dom_path, *dom_sshm_path, *dom_role_path;
> + const char *xs_value;
> + char *ents[9];
> + libxl_static_shm master_sshm;
> + uint32_t master_domid;
> + xen_pfn_t *mapped;
> + unsigned int nmapped = 0;
> + xs_transaction_t xt = XBT_NULL;
> + bool isretry;
> +
> + sshm_path = SSHM_PATH(sshm->id);
> + slave_path = GCSPRINTF("%s/slaves/%"PRIu32, sshm_path, domid);
> + dom_path = libxl__xs_get_dompath(gc, domid);
> + /* the domain should be in xenstore by now */
> + assert(dom_path);
> + dom_sshm_path = GCSPRINTF("%s/static_shm/%s", dom_path, sshm->id);
> + dom_role_path = GCSPRINTF("%s/role", dom_sshm_path);
> +
> + /* prepare the slave xenstore entries */
> + ents[0] = "begin";
> + ents[1] = GCSPRINTF("0x%"PRIx64, sshm->begin);
> + ents[2] = "size";
> + ents[3] = GCSPRINTF("0x%"PRIx64, sshm->size);
> + ents[4] = "offset";
> + ents[5] = GCSPRINTF("0x%"PRIx64, sshm->offset);
> + ents[6] = "prot";
> + ents[7] = libxl__strdup(gc, libxl_sshm_prot_to_string(sshm->prot));
> + ents[8] = NULL;
I know this ents[<fixed integer>] approach is very common elsewhere
but it is IMO poor. Please use the flexarray mechanism instead, in
particular flexarray_append_pair. You may want a macro to encapsulate
the formulaic copying of numeric values from sshm to xenstore.
> + mapped = libxl__calloc(gc, sshm->size >> XC_PAGE_SHIFT,
> sizeof(xen_pfn_t));
> +
> + isretry = false;
> + for (;;) {
> + rc = libxl__xs_transaction_start(gc, &xt);
> + if (rc) goto out;
> +
> + if (!libxl__xs_read(gc, xt, sshm_path)) {
Error handling is wrong. Should be libxl__xs_read_checked.
> + /* every ID can appear in each domain at most once */
> + if (libxl__xs_read(gc, xt, dom_sshm_path)) {
> + SSHM_ERROR(domid, sshm->id,
Again.
> + /* look at the master info and see if we could do the mapping */
> + rc = libxl__xs_read_checked(gc, xt,
> + GCSPRINTF("%s/prot", sshm_path),
> + &xs_value);
> + if (rc) goto out;
> + libxl_sshm_prot_from_string(xs_value, &master_sshm.prot);
> +
> + rc = libxl__xs_read_checked(gc, xt,
> + GCSPRINTF("%s/begin", sshm_path),
> + &xs_value);
> + if (rc) goto out;
> + master_sshm.begin = strtoull(xs_value, NULL, 16);
> +
> + rc = libxl__xs_read_checked(gc, xt,
> + GCSPRINTF("%s/size", sshm_path),
> + &xs_value);
> + if (rc) goto out;
> + master_sshm.size = strtoull(xs_value, NULL, 16);
> +
> + rc = libxl__xs_read_checked(gc, xt,
> + GCSPRINTF("%s/master", sshm_path),
> + &xs_value);
> + if (rc) goto out;
> + master_domid = strtoull(xs_value, NULL, 16);
I think this formulaic code needs to be macro-ified or something.
> + /* check if the slave is asking too much permission */
> + if (master_sshm.prot < sshm->prot) {
If you do decide to keep prot, what if it is a bitmap ? Protections
are not necessarily a total order, so I don't think `<' can work.
This kind of future ABI/API issue is one reason why it might be better
to delete the handling of prot, particularly since it isn't wired up
to anything.
> + SSHM_ERROR(domid, sshm->id, "slave is asking too much
> permission.");
> + rc = ERROR_INVAL;
> + goto out;
> + }
> +
> + /* 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;
> + }
Please don't do it like this. I think there is a race here: you are
mapping the memory before you have committed to xenstore; you may
therefore be mapping it after someone else has committed a deletion or
reconfiguration of the sshm region.
You should do the mapping after the xenstore transaction has
successfully committed. That way every existing mapping is
represented in xenstore. (Unmapped mappings may be in xenstore too
but I think that is fine.)
> +static int libxl__sshm_add_master(libxl__gc *gc, uint32_t domid,
> + libxl_static_shm *sshm)
> +{
...
> + ents[9] = libxl__strdup(gc,
> +
> libxl_sshm_cachepolicy_to_string(sshm->cache_policy));
I don't know why you need the strdup here.
> + if (!libxl__xs_read(gc, xt, sshm_path)) {
You need to use libxl__xs_read_checked here too.
> diff --git a/tools/libxl/libxl_x86.c b/tools/libxl/libxl_x86.c
> index 8b6759c..3e70a69 100644
> --- a/tools/libxl/libxl_x86.c
> +++ b/tools/libxl/libxl_x86.c
> @@ -619,6 +619,25 @@ void libxl__arch_domain_build_info_setdefault(libxl__gc
> *gc,
> 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.
> + */
> + return false;
I don't think an unsupported feature warrants a `FIXME'. Also, can
you change the `Mark this as unspported' to `This is unsupported' ?
I think this and the destruction patch need to be combined.
Thanks,
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |