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 <[email protected]> > >> Signed-off-by: Yuval Shaia <[email protected]> > >> --- > > > > 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"; 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 If you are going to do another version anyway, I'd add comments to document the games you play in place of the script. -- MST
