Re: [Qemu-devel] [PATCH V6 4/5] pvrdma: initial implementation

[Cornelia Huck]

On Wed, 10 Jan 2018 11:28:59 +0200
Marcel Apfelbaum <address@hidden> wrote:

> On 09/01/2018 14:51, Cornelia Huck wrote:
> > On Tue, 9 Jan 2018 13:08:33 +0200
> > Yuval Shaia <address@hidden> wrote:
> >   
> >> On Tue, Jan 09, 2018 at 11:39:11AM +0100, Cornelia Huck wrote:  
> >>> On Sun,  7 Jan 2018 14:32:23 +0200
> >>> Marcel Apfelbaum <address@hidden> wrote:  
> >   
> >>>> diff --git a/hw/rdma/vmw/pvrdma_dev_api.h b/hw/rdma/vmw/pvrdma_dev_api.h
> >>>> new file mode 100644
> >>>> index 0000000000..bf1986a976
> >>>> --- /dev/null
> >>>> +++ b/hw/rdma/vmw/pvrdma_dev_api.h
> >>>> @@ -0,0 +1,602 @@
> >>>> +/*
> >>>> + * QEMU VMWARE paravirtual RDMA device definitions
> >>>> + *
> >>>> + * Copyright (C) 2018 Oracle
> >>>> + * Copyright (C) 2018 Red Hat Inc
> >>>> + *
> >>>> + * Authors:
> >>>> + *     Yuval Shaia <address@hidden>
> >>>> + *     Marcel Apfelbaum <address@hidden>
> >>>> + *
> >>>> + * This work is licensed under the terms of the GNU GPL, version 2.
> >>>> + * See the COPYING file in the top-level directory.
> >>>> + *
> >>>> + */
> >>>> +
> >>>> +#ifndef PVRDMA_DEV_API_H
> >>>> +#define PVRDMA_DEV_API_H
> >>>> +
> >>>> +/*
> >>>> + * Following is an interface definition for PVRDMA device as provided by
> >>>> + * VMWARE.
> >>>> + * See original copyright from Linux kernel v4.14.5 header file
> >>>> + * drivers/infiniband/hw/vmw_pvrdma/pvrdma_dev_api.h  
> >>>
> >>> Could that file be exported as UAPI in the kernel and added to the
> >>> linux-headers script?  
> >>
> >> We took this approach as apposed to kernel-headers with the following on
> >> our mind:
> >> (1) This is the convention used in vmxnet3.
> >> (2) vmw_pvrdma was introduced only lately, taking the kernel-headers
> >> approach will force specific kernel on a host in order to compile QEMU.  
> > 
> > qemu will get the kernel headers once from the upstream kernel and then
> > will be able to be built everywhere.
> >   
> >> (3) To support VMWare's pvrdma device we took a snapshot of existing
> >> driver/device settings and breezed there. This is driver/device API and we
> >> can't allow our self to chase VMWare's tail whenever they are changing the
> >> API. Just consider a case where they will change for example the ARM bit.  
> > 
> > But as want to enable the existing device driver, you'll want to be
> > able to produce a compatible device anyway, don't you? Also, wouldn't
> > VMWare break older kernels if they suddenly changed the api?
> >   
> 
> I think it was a missunderstanding here :)
> 
> We don't actually need the VMware headers here in order to compile the device.
> Requesting the pvrdma headers to be present really limits the hosts we could
> compile QEMU without an actual benefit.
> 
> The headers describe the data structures passed by the Guest driver to the 
> PVRDMA device.
> Having them on the host should not be a requirement, we could simply define 
> our own
> structs but it would be harder to maintain. (diffing with linux headers to 
> see what's new)
> 
> Note we don't care about ABI changes, we emulate a PCI device, not
> a para-virt one, the driver has the contract to pass the same data 
> definitions always.
> If the data changes, there are 2 options:
> 1. Pass another "version nr" on the command channel (DSR) and we declare as 
> not supported
>     by this device and fail gracefully.
> 2. Use another device id.

OK, thanks for the clarification, that makes sense.

> 
> We looked for what are the options for *not requiring* the headers to be 
> present
> and we saw two in QEMU:
> 1.  Adding it to include/standard-headers/linux/
> But it seemed to much, only our device need them.
> 2. Adding it to the hw/rdma/vwm directory
> Please look at: (hw/net/vmxnet3.h)
> 
> The second option seemed better, but we could go with "1."
> if I got it right and you think is cleaner.
> But again, the first option kind of requires updating the
> linux headers always, but we want quite the opposite, to not
> be connected to the changes.
> I hope is more clear now.

OK, that makes sense as well, if you intend this to be a one-time
effort. "Copy a header from the Linux kernel" just raised kind of a
flag for me.

> 
> 
> > [Also, is there a canonical reference for this API?]
> >   
> 
> Linux Kernel PVRDMA driver implementation...

:)

> 
> >>
> >> Just IMHO.
> >>  
> 
> Thanks a lot for your inputs, much appreciated!

NP. I'd like to do more, as this is interesting stuff, but you know how
it goes...