Re: [Qemu-devel] [PATCH V10 3/9] include/standard-headers: add pvrdma related headers

2018-02-14 Thread Michael S. Tsirkin
On Wed, Feb 14, 2018 at 06:50:34PM +0200, Marcel Apfelbaum wrote:
> Hi Michael,
> 
> On 14/02/2018 18:15, Michael S. Tsirkin wrote:
> > On Mon, Feb 12, 2018 at 08:08:13PM +0200, Marcel Apfelbaum wrote:
> >> Also modify update-linux-headers.sh script to manage
> >> the headers needed by the pvrdma device.
> >>
> >> Signed-off-by: Marcel Apfelbaum 
> >> Signed-off-by: Yuval Shaia 
> >> ---
> > 
> > Would be best to make script update a separate patch.
> > Automatically generated stuff can come later.
> > 
> 
> I can do that, sure.
> 
> > Overall comments below are minor. if you do not respin
> > you can address them later as a patch on top.
> > 
> >> diff --git a/scripts/update-linux-headers.sh 
> >> b/scripts/update-linux-headers.sh
> >> index 135a10d96a..c1a7c1e99c 100755
> >> --- a/scripts/update-linux-headers.sh
> >> +++ b/scripts/update-linux-headers.sh
> >> @@ -38,6 +38,7 @@ cp_portable() {
> >>   -e 'linux/if_ether' \
> >>   -e 'input-event-codes' \
> >>   -e 'sys/' \
> >> + -e 'pvrdma_verbs' \
> >>   > /dev/null
> >>  then
> >>  echo "Unexpected #include in input file $f".
> >> @@ -46,6 +47,7 @@ cp_portable() {
> >>  
> >>  header=$(basename "$f");
> >>  sed -e 's/__u\([0-9][0-9]*\)/uint\1_t/g' \
> >> +-e 's/u\([0-9][0-9]*\)/uint\1_t/g' \
> >>  -e 's/__s\([0-9][0-9]*\)/int\1_t/g' \
> >>  -e 's/__le\([0-9][0-9]*\)/uint\1_t/g' \
> >>  -e 's/__be\([0-9][0-9]*\)/uint\1_t/g' \
> >> @@ -56,6 +58,7 @@ cp_portable() {
> >>  -e 's/__inline__/inline/' \
> >>  -e '/sys\/ioctl.h/d' \
> >>  -e 's/SW_MAX/SW_MAX_/' \
> >> +-e 's/atomic_t/int/' \
> >>  "$f" > "$to/$header";
> >>  }
> >>  
> >> @@ -147,6 +150,30 @@ for i in "$tmpdir"/include/linux/*virtio*.h 
> >> "$tmpdir/include/linux/input.h" \
> >>  cp_portable "$i" "$output/include/standard-headers/linux"
> >>  done
> >>  
> >> +rm -rf "$output/include/standard-headers/drivers/infiniband/hw/vmw_pvrdma"
> >> +mkdir -p 
> >> "$output/include/standard-headers/drivers/infiniband/hw/vmw_pvrdma"
> >> +
> >> +# Remove the unused functions from pvrdma_verbs.h avoiding the unnecessary
> >> +# import of several infiniband/networking/other headers
> >> +tmp_pvrdma_verbs="$tmpdir/pvrdma_verbs.h"
> >> +sed  -e '1h;2,$H;$!d;g'
> > 
> > what does this do exactly?
> > 
> 
> It parses the whole file instead of line by line.
> It is done in order to match functions that extends
> over multiple lines.
> 
> >>  -e 's/[^};]*pvrdma[^(| ]*([^)]*);//g' \
> > 
> > and this?
> > 
> 
> Looks for function declarations starting with pvrdma prefix.

I would add some documentation to this then.

> >> +"$linux/drivers/infiniband/hw/vmw_pvrdma/pvrdma_verbs.h" > \
> >> +"$tmp_pvrdma_verbs";
> >> +
> > 
> > I suspect you want the enums from these headers but not the
> > rest of stuff there?
> > 
> 
> Right, enum and structs, but not the functions.
> The functions use ib headers we are not interested in.
> Since the enum/structs can be used separately ,it seems it
> would be better if the header is split, but sadly
> is not what's happening today.

It's pretty easy to move code to another header, certainly easier
than playing with sed :)

> >> +for i in "$linux/drivers/infiniband/hw/vmw_pvrdma/pvrdma_ring.h" \
> >> + "$linux/drivers/infiniband/hw/vmw_pvrdma/pvrdma_dev_api.h" \
> >> + "$tmp_pvrdma_verbs"; do \
> >> +cp_portable "$i" \
> >> + 
> >> "$output/include/standard-headers/drivers/infiniband/hw/vmw_pvrdma/"
> >> +done
> > 
> > Is the maintainer aware these are an interface?
> 
> It is an interface, but between the device and the guest driver,
> not between the guest driver and guest user space.

Right - and it's a bit of an abuse of notation, but there's no other
place besides uapi to stick interface files right now.

> This is why I didn't move them to the standard-headers in the first place.

You can move them into some other location and use some other
script if that's preferable.

> We copy once the header with the structs the device receives through the
> command channel and then we are protected by the command channel versioning.
> (Then we can update the headers when we want to move to another version)
> 
> > If yes
> > is there a chance to move at least some of these out to uapi?
> > That will split the code logically, and qemu could import
> > files without hacks.
> > 
> 
> We can ask the maintainers if they agree at least to split the pvrdma_verbs
> header. If/when they agree, we can update the script.

Send a patch that is the best way to ask questions :)

> > 
> >> +
> >> +rm -rf "$output/include/standard-headers/rdma/"
> >> +mkdir -p "$output/include/standard-headers/rdma/"
> >> +for i in "$tmpdir/include/rdma/vmw_pvrdma-abi.h"; 

Re: [Qemu-devel] [PATCH V10 3/9] include/standard-headers: add pvrdma related headers

2018-02-14 Thread Marcel Apfelbaum
Hi Michael,

On 14/02/2018 18:15, Michael S. Tsirkin wrote:
> On Mon, Feb 12, 2018 at 08:08:13PM +0200, Marcel Apfelbaum wrote:
>> Also modify update-linux-headers.sh script to manage
>> the headers needed by the pvrdma device.
>>
>> Signed-off-by: Marcel Apfelbaum 
>> Signed-off-by: Yuval Shaia 
>> ---
> 
> Would be best to make script update a separate patch.
> Automatically generated stuff can come later.
> 

I can do that, sure.

> Overall comments below are minor. if you do not respin
> you can address them later as a patch on top.
> 
>> diff --git a/scripts/update-linux-headers.sh 
>> b/scripts/update-linux-headers.sh
>> index 135a10d96a..c1a7c1e99c 100755
>> --- a/scripts/update-linux-headers.sh
>> +++ b/scripts/update-linux-headers.sh
>> @@ -38,6 +38,7 @@ cp_portable() {
>>   -e 'linux/if_ether' \
>>   -e 'input-event-codes' \
>>   -e 'sys/' \
>> + -e 'pvrdma_verbs' \
>>   > /dev/null
>>  then
>>  echo "Unexpected #include in input file $f".
>> @@ -46,6 +47,7 @@ cp_portable() {
>>  
>>  header=$(basename "$f");
>>  sed -e 's/__u\([0-9][0-9]*\)/uint\1_t/g' \
>> +-e 's/u\([0-9][0-9]*\)/uint\1_t/g' \
>>  -e 's/__s\([0-9][0-9]*\)/int\1_t/g' \
>>  -e 's/__le\([0-9][0-9]*\)/uint\1_t/g' \
>>  -e 's/__be\([0-9][0-9]*\)/uint\1_t/g' \
>> @@ -56,6 +58,7 @@ cp_portable() {
>>  -e 's/__inline__/inline/' \
>>  -e '/sys\/ioctl.h/d' \
>>  -e 's/SW_MAX/SW_MAX_/' \
>> +-e 's/atomic_t/int/' \
>>  "$f" > "$to/$header";
>>  }
>>  
>> @@ -147,6 +150,30 @@ for i in "$tmpdir"/include/linux/*virtio*.h 
>> "$tmpdir/include/linux/input.h" \
>>  cp_portable "$i" "$output/include/standard-headers/linux"
>>  done
>>  
>> +rm -rf "$output/include/standard-headers/drivers/infiniband/hw/vmw_pvrdma"
>> +mkdir -p "$output/include/standard-headers/drivers/infiniband/hw/vmw_pvrdma"
>> +
>> +# Remove the unused functions from pvrdma_verbs.h avoiding the unnecessary
>> +# import of several infiniband/networking/other headers
>> +tmp_pvrdma_verbs="$tmpdir/pvrdma_verbs.h"
>> +sed  -e '1h;2,$H;$!d;g'
> 
> what does this do exactly?
> 

It parses the whole file instead of line by line.
It is done in order to match functions that extends
over multiple lines.

>>  -e 's/[^};]*pvrdma[^(| ]*([^)]*);//g' \
> 
> and this?
> 

Looks for function declarations starting with pvrdma prefix.

>> +"$linux/drivers/infiniband/hw/vmw_pvrdma/pvrdma_verbs.h" > \
>> +"$tmp_pvrdma_verbs";
>> +
> 
> I suspect you want the enums from these headers but not the
> rest of stuff there?
> 

Right, enum and structs, but not the functions.
The functions use ib headers we are not interested in.
Since the enum/structs can be used separately ,it seems it
would be better if the header is split, but sadly
is not what's happening today.

>> +for i in "$linux/drivers/infiniband/hw/vmw_pvrdma/pvrdma_ring.h" \
>> + "$linux/drivers/infiniband/hw/vmw_pvrdma/pvrdma_dev_api.h" \
>> + "$tmp_pvrdma_verbs"; do \
>> +cp_portable "$i" \
>> + 
>> "$output/include/standard-headers/drivers/infiniband/hw/vmw_pvrdma/"
>> +done
> 
> Is the maintainer aware these are an interface?

It is an interface, but between the device and the guest driver,
not between the guest driver and guest user space.
This is why I didn't move them to the standard-headers in the first place.
We copy once the header with the structs the device receives through the
command channel and then we are protected by the command channel versioning.
(Then we can update the headers when we want to move to another version)

> If yes
> is there a chance to move at least some of these out to uapi?
> That will split the code logically, and qemu could import
> files without hacks.
> 

We can ask the maintainers if they agree at least to split the pvrdma_verbs
header. If/when they agree, we can update the script.

> 
>> +
>> +rm -rf "$output/include/standard-headers/rdma/"
>> +mkdir -p "$output/include/standard-headers/rdma/"
>> +for i in "$tmpdir/include/rdma/vmw_pvrdma-abi.h"; do
>> +cp_portable "$i" \
>> + "$output/include/standard-headers/rdma/"
>> +done
>> +
>>  cat <$output/include/standard-headers/linux/types.h
>>  /* For QEMU all types are already defined via osdep.h, so this
>>   * header does not need to do anything.
>> -- 
>> 2.13.5

Other than the split, is it anything else should I modify
before sending the new version?

Thanks,
Marcel



Re: [Qemu-devel] [PATCH V10 3/9] include/standard-headers: add pvrdma related headers

2018-02-14 Thread Michael S. Tsirkin
On Mon, Feb 12, 2018 at 08:08:13PM +0200, Marcel Apfelbaum wrote:
> Also modify update-linux-headers.sh script to manage
> the headers needed by the pvrdma device.
> 
> Signed-off-by: Marcel Apfelbaum 
> Signed-off-by: Yuval Shaia 
> ---

Would be best to make script update a separate patch.
Automatically generated stuff can come later.

Overall comments below are minor. if you do not respin
you can address them later as a patch on top.

> diff --git a/scripts/update-linux-headers.sh b/scripts/update-linux-headers.sh
> index 135a10d96a..c1a7c1e99c 100755
> --- a/scripts/update-linux-headers.sh
> +++ b/scripts/update-linux-headers.sh
> @@ -38,6 +38,7 @@ cp_portable() {
>   -e 'linux/if_ether' \
>   -e 'input-event-codes' \
>   -e 'sys/' \
> + -e 'pvrdma_verbs' \
>   > /dev/null
>  then
>  echo "Unexpected #include in input file $f".
> @@ -46,6 +47,7 @@ cp_portable() {
>  
>  header=$(basename "$f");
>  sed -e 's/__u\([0-9][0-9]*\)/uint\1_t/g' \
> +-e 's/u\([0-9][0-9]*\)/uint\1_t/g' \
>  -e 's/__s\([0-9][0-9]*\)/int\1_t/g' \
>  -e 's/__le\([0-9][0-9]*\)/uint\1_t/g' \
>  -e 's/__be\([0-9][0-9]*\)/uint\1_t/g' \
> @@ -56,6 +58,7 @@ cp_portable() {
>  -e 's/__inline__/inline/' \
>  -e '/sys\/ioctl.h/d' \
>  -e 's/SW_MAX/SW_MAX_/' \
> +-e 's/atomic_t/int/' \
>  "$f" > "$to/$header";
>  }
>  
> @@ -147,6 +150,30 @@ for i in "$tmpdir"/include/linux/*virtio*.h 
> "$tmpdir/include/linux/input.h" \
>  cp_portable "$i" "$output/include/standard-headers/linux"
>  done
>  
> +rm -rf "$output/include/standard-headers/drivers/infiniband/hw/vmw_pvrdma"
> +mkdir -p "$output/include/standard-headers/drivers/infiniband/hw/vmw_pvrdma"
> +
> +# Remove the unused functions from pvrdma_verbs.h avoiding the unnecessary
> +# import of several infiniband/networking/other headers
> +tmp_pvrdma_verbs="$tmpdir/pvrdma_verbs.h"
> +sed  -e '1h;2,$H;$!d;g'

what does this do exactly?

>  -e 's/[^};]*pvrdma[^(| ]*([^)]*);//g' \

and this?

> +"$linux/drivers/infiniband/hw/vmw_pvrdma/pvrdma_verbs.h" > \
> +"$tmp_pvrdma_verbs";
> +

I suspect you want the enums from these headers but not the
rest of stuff there?

> +for i in "$linux/drivers/infiniband/hw/vmw_pvrdma/pvrdma_ring.h" \
> + "$linux/drivers/infiniband/hw/vmw_pvrdma/pvrdma_dev_api.h" \
> + "$tmp_pvrdma_verbs"; do \
> +cp_portable "$i" \
> + "$output/include/standard-headers/drivers/infiniband/hw/vmw_pvrdma/"
> +done

Is the maintainer aware these are an interface? If yes
is there a chance to move at least some of these out to uapi?
That will split the code logically, and qemu could import
files without hacks.


> +
> +rm -rf "$output/include/standard-headers/rdma/"
> +mkdir -p "$output/include/standard-headers/rdma/"
> +for i in "$tmpdir/include/rdma/vmw_pvrdma-abi.h"; do
> +cp_portable "$i" \
> + "$output/include/standard-headers/rdma/"
> +done
> +
>  cat <$output/include/standard-headers/linux/types.h
>  /* For QEMU all types are already defined via osdep.h, so this
>   * header does not need to do anything.
> -- 
> 2.13.5



Re: [Qemu-devel] [PATCH V10 3/9] include/standard-headers: add pvrdma related headers

2018-02-14 Thread Gal Hammer
Hi,

I'm not familiar with the pvrdma device itself, but I reviewed the
changes to the update-linux-headers.sh script.

Reviewed-by: Gal Hammer 

Gal.


On Mon, Feb 12, 2018 at 8:08 PM, Marcel Apfelbaum  wrote:
> Also modify update-linux-headers.sh script to manage
> the headers needed by the pvrdma device.
>
> Signed-off-by: Marcel Apfelbaum 
> Signed-off-by: Yuval Shaia 
> ---
>  .../infiniband/hw/vmw_pvrdma/pvrdma_dev_api.h  | 667 
> +
>  .../drivers/infiniband/hw/vmw_pvrdma/pvrdma_ring.h | 114 
>  .../infiniband/hw/vmw_pvrdma/pvrdma_verbs.h| 383 
>  include/standard-headers/rdma/vmw_pvrdma-abi.h | 293 +
>  scripts/update-linux-headers.sh|  27 +
>  5 files changed, 1484 insertions(+)
>  create mode 100644 
> include/standard-headers/drivers/infiniband/hw/vmw_pvrdma/pvrdma_dev_api.h
>  create mode 100644 
> include/standard-headers/drivers/infiniband/hw/vmw_pvrdma/pvrdma_ring.h
>  create mode 100644 
> include/standard-headers/drivers/infiniband/hw/vmw_pvrdma/pvrdma_verbs.h
>  create mode 100644 include/standard-headers/rdma/vmw_pvrdma-abi.h
>
> diff --git 
> a/include/standard-headers/drivers/infiniband/hw/vmw_pvrdma/pvrdma_dev_api.h 
> b/include/standard-headers/drivers/infiniband/hw/vmw_pvrdma/pvrdma_dev_api.h
> new file mode 100644
> index 00..422eb3f4c1
> --- /dev/null
> +++ 
> b/include/standard-headers/drivers/infiniband/hw/vmw_pvrdma/pvrdma_dev_api.h
> @@ -0,0 +1,667 @@
> +/*
> + * Copyright (c) 2012-2016 VMware, Inc.  All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of EITHER the GNU General Public License
> + * version 2 as published by the Free Software Foundation or the BSD
> + * 2-Clause License. This program is distributed in the hope that it
> + * will be useful, but WITHOUT ANY WARRANTY; WITHOUT EVEN THE IMPLIED
> + * WARRANTY OF MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE.
> + * See the GNU General Public License version 2 for more details at
> + * http://www.gnu.org/licenses/old-licenses/gpl-2.0.en.html.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program available in the file COPYING in the main
> + * directory of this source tree.
> + *
> + * The BSD 2-Clause License
> + *
> + * Redistribution and use in source and binary forms, with or
> + * without modification, are permitted provided that the following
> + * conditions are met:
> + *
> + *  - Redistributions of source code must retain the above
> + *copyright notice, this list of conditions and the following
> + *disclaimer.
> + *
> + *  - Redistributions in binary form must reproduce the above
> + *copyright notice, this list of conditions and the following
> + *disclaimer in the documentation and/or other materials
> + *provided with the distribution.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS
> + * FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE
> + * COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT,
> + * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
> + * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR
> + * SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
> + * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT,
> + * STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
> + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED
> + * OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +#ifndef __PVRDMA_DEV_API_H__
> +#define __PVRDMA_DEV_API_H__
> +
> +#include "standard-headers/linux/types.h"
> +
> +#include "pvrdma_verbs.h"
> +
> +/*
> + * PVRDMA version macros. Some new features require updates to 
> PVRDMA_VERSION.
> + * These macros allow us to check for different features if necessary.
> + */
> +
> +#define PVRDMA_ROCEV1_VERSION  17
> +#define PVRDMA_ROCEV2_VERSION  18
> +#define PVRDMA_VERSION PVRDMA_ROCEV2_VERSION
> +
> +#define PVRDMA_BOARD_ID1
> +#define PVRDMA_REV_ID  1
> +
> +/*
> + * Masks and accessors for page directory, which is a two-level lookup:
> + * page directory -> page table -> page. Only one directory for now, but we
> + * could expand that easily. 9 bits for tables, 9 bits for pages, gives one
> + * gigabyte for memory regions and so forth.
> + */
> +
> +#define PVRDMA_PDIR_SHIFT  18
> +#define PVRDMA_PTABLE_SHIFT9
> +#define PVRDMA_PAGE_DIR_DIR(x) (((x) >> PVRDMA_PDIR_SHIFT) & 0x1)
> +#define PVRDMA_PAGE_DIR_TABLE(x)   (((x) >>