Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH 8/9] drm/xen-front: Implement GEM operations
To:     Boris Ostrovsky <boris.ostrovsky@oracle.com>,
        Oleksandr Andrushchenko <andr2000@gmail.com>,
        xen-devel@lists.xenproject.org, linux-kernel@vger.kernel.org,
        dri-devel@lists.freedesktop.org, airlied@linux.ie,
        daniel.vetter@intel.com, seanpaul@chromium.org,
        gustavo@padovan.org, jgross@suse.com, konrad.wilk@oracle.com
References: <1519200222-20623-1-git-send-email-andr2000@gmail.com>
 <1519200222-20623-9-git-send-email-andr2000@gmail.com>
 <2f2c6fea-c0cb-e244-41f3-269db07986fc@oracle.com>
From:   Oleksandr Andrushchenko <Oleksandr_Andrushchenko@epam.com>
Message-ID: <56c4a78b-356a-fb35-a97e-187581ae45ad@epam.com>
Date:   Fri, 23 Feb 2018 17:35:40 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.6.0
MIME-Version: 1.0
In-Reply-To: <2f2c6fea-c0cb-e244-41f3-269db07986fc@oracle.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Received-SPF: None (protection.outlook.com: epam.com does not designate
 permitted sender hosts)
SpamDiagnosticOutput: 1:99
SpamDiagnosticMetadata: NSPM
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 23 Feb 2018 15:35:44.4791 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 0a1be7ed-976e-4190-1b20-08d57ad31b21
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: b41b72d0-4e9f-4c26-8a69-f949f367c91d
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM4PR0301MB1940
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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

Please see description of the buffering modes in xen_drm_front.h
specifically for backend allocated buffers:
  *******************************************************************************
  * 2. 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);
>> +}
>> +
>>