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

[Marcel Apfelbaum <marcel@redhat.com>]

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 <marcel@redhat.com>
>> Signed-off-by: Yuval Shaia <yuval.shaia@oracle.com>
>> ---
> 
> 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 <<EOF >$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