On 02/27/2018 01:47 AM, Boris Ostrovsky wrote:
On 02/23/2018 10:35 AM, Oleksandr Andrushchenko wrote:
On 02/23/2018 05:26 PM, Boris Ostrovsky wrote:
On 02/21/2018 03:03 AM, Oleksandr Andrushchenko wrote:
+static struct xen_gem_object *gem_create(struct drm_device *dev, size_t size) +{
- struct xen_drm_front_drm_info *drm_info = dev->dev_private;
- struct xen_gem_object *xen_obj;
- int ret;
- size = round_up(size, PAGE_SIZE);
- xen_obj = gem_create_obj(dev, size);
- if (IS_ERR_OR_NULL(xen_obj))
return xen_obj;- if (drm_info->cfg->be_alloc) {
/** backend will allocate space for this buffer, so* only allocate array of pointers to pages*/xen_obj->be_alloc = true;If be_alloc is a flag (which I am not sure about) --- should it be set to true *after* you've successfully allocated your things?
this is a configuration option telling about the way the buffer gets allocated: either by the frontend or backend (be_alloc -> buffer allocated by the backend)
I can see how drm_info->cfg->be_alloc might be a configuration option but xen_obj->be_alloc is set here and that's not how configuration options typically behave.
you are right, I will put be_alloc down the code and will slightly rework error handling for this function
ret = gem_alloc_pages_array(xen_obj, size);if (ret < 0) {gem_free_pages_array(xen_obj);goto fail;}ret = alloc_xenballooned_pages(xen_obj->num_pages,xen_obj->pages);Why are you allocating balloon pages?
in this use-case we map pages provided by the backend (yes, I know this can be a problem from both security POV and that DomU can die holding pages of Dom0 forever: but still it is a configuration option, so user decides if her use-case needs this and takes responsibility for such a decision).
Perhaps I am missing something here but when you say "I know this can be a problem from both security POV ..." then there is something wrong with your solution.
well, in this scenario there are actually 2 concerns: 1. If DomU dies the pages/grants from Dom0/DomD cannot be reclaimed back 2. Misbehaving guest may send too many requests to the backend exhausting grant references and memory of Dom0/DomD (this is the only concern from security POV). Please see [1]
But, we are focusing on embedded use-cases, so those systems we use are not that "dynamic" with respect to 2). Namely: we have fixed number of domains and their functionality is well known, so we can do rather precise assumption on resource usage. This is why I try to warn on such a use-case and rely on the end user who understands the caveats
I'll probably add more precise description of this use-case clarifying what is that security POV, so there is no confusion
Hope this explanation answers your questions
-boris
Please see description of the buffering modes in xen_drm_front.h specifically for backend allocated buffers:
- Buffers allocated by the backend
- This mode of operation is run-time configured via guest domain
configuration
- through XenStore entries.
- For systems which do not provide IOMMU support, but having specific
- requirements for display buffers it is possible to allocate such
buffers
- at backend side and share those with the frontend.
- For example, if host domain is 1:1 mapped and has DRM/GPU hardware
expecting
- physically contiguous memory, this allows implementing zero-copying
- use-cases.
-boris
if (ret < 0) {DRM_ERROR("Cannot allocate %zu ballooned pages: %d\n",xen_obj->num_pages, ret);goto fail;}return xen_obj;- }
- /*
* need to allocate backing pages now, so we can share those* with the backend*/- xen_obj->num_pages = DIV_ROUND_UP(size, PAGE_SIZE);
- xen_obj->pages = drm_gem_get_pages(&xen_obj->base);
- if (IS_ERR_OR_NULL(xen_obj->pages)) {
ret = PTR_ERR(xen_obj->pages);xen_obj->pages = NULL;goto fail;- }
- return xen_obj;
+fail:
- DRM_ERROR("Failed to allocate buffer with size %zu\n", size);
- return ERR_PTR(ret);
+}
Thank you, Oleksandr
[1] https://lists.xenproject.org/archives/html/xen-devel/2017-07/msg03100.html