Re: [PATCH v3 2/2] qdisk - hw/block/xen_disk: grant copy implementation

[Paulina Szubarczyk <paulinaszubarczyk@gmail.com>]

On 07/19/2016 11:12 AM, Roger Pau Monné wrote:
> On Wed, Jun 22, 2016 at 10:38:53AM +0200, Paulina Szubarczyk wrote:
>> Copy data operated on during request from/to local buffers to/from
>> the grant references.
>>
>> Before grant copy operation local buffers must be allocated what is
>> done by calling ioreq_init_copy_buffers. For the 'read' operation,
>> first, the qemu device invokes the read operation on local buffers
>> and on the completion grant copy is called and buffers are freed.
>> For the 'write' operation grant copy is performed before invoking
>> write by qemu device.
>>
>> A new value 'feature_grant_copy' is added to recognize when the
>> grant copy operation is supported by a guest.
>> The body of the function 'ioreq_runio_qemu_aio' is moved to
>> 'ioreq_runio_qemu_aio_blk' and in the 'ioreq_runio_qemu_aio' depending
>> on the support for grant copy according checks, initialization, grant
>> operation are made, then the 'ioreq_runio_qemu_aio_blk' function is
>> called.
>>
>> Signed-off-by: Paulina Szubarczyk <paulinaszubarczyk@gmail.com>
>> ---
>> Changes since v2:
>> - to use the xengnttab_* function directly added -lxengnttab to configure
>>    and include <xengnttab.h> in include/hw/xen/xen_common.h
>> - in ioreq_copy removed an out path, changed a log level, made explicit
>>    assignement to 'xengnttab_copy_grant_segment'
>> * I did not change the way of testing if grant_copy operation is implemented.
>>    As far as I understand if the code from gnttab_unimp.c is used then the gnttab
>>    device is unavailable and the handler to gntdev would be invalid. But
>>    if the handler is valid then the ioctl should return operation unimplemented
>>    if the gntdev does not implement the operation.
>>
>>   configure                   |   2 +-
>>   hw/block/xen_disk.c         | 171 ++++++++++++++++++++++++++++++++++++++++----
>>   include/hw/xen/xen_common.h |   2 +
>>   3 files changed, 162 insertions(+), 13 deletions(-)
>
> [...]
>
>> @@ -1020,10 +1160,17 @@ static int blk_connect(struct XenDevice *xendev)
>>
>>       xen_be_bind_evtchn(&blkdev->xendev);
>>
>> +    blkdev->feature_grant_copy =
>> +                (xengnttab_grant_copy(blkdev->xendev.gnttabdev, 0, NULL) == 0);
>
> Isn't this going to trigger an abort on OSes that don't implement
> xengnttab_grant_copy? AFAICT the 'unimplemented' handler in libgnttab for
> this is just an abort.

So is the xengnttab_map_grant_refs and the pointer to 
blkdev->xendev.gnttabdev would be invalid so the sring would not be 
initialized a few lines earlier in that function leading to the fail of 
the initialization. In case the gntdev does not implement the ioctl then 
only an error code will be returned.

Paulina
>
> Roger.
>

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