Re: [Qemu-devel] [PATCH v2 2/2] target-i386: move asm-x86/hyperv.h to standard-headers

[Cornelia Huck]

On Thu, 10 Sep 2015 10:42:31 +0300
"Michael S. Tsirkin" <address@hidden> wrote:

> On Wed, Sep 09, 2015 at 06:34:01PM +0200, Paolo Bonzini wrote:
> > The Hyper-V definitions are an industry standard and can be used
> > from code that is not KVM-specific.
> > 
> > The changes to scripts/update-linux-headers.sh are required because there
> > is both an asm-x86/hyperv.h and a linux/hyperv.h file.  linux/hyperv.h
> > introduces dependencies on additional Linux uapi headers, so we only
> > want the former.
> > 
> > The solution is to make cp_virtio (now renamed to cp_portable) copy
> > one file only, instead of using the "find" command, and call it multiple
> > times.  The new function is really just a reindentation of the old one.
> > 
> > Signed-off-by: Paolo Bonzini <address@hidden>
> 
> I'd rather see a script update, then result of running it
> in a separate patch.
> 
> > ---
> >  .../standard-headers}/asm-x86/hyperv.h    |  10 +-
> >  linux-headers/asm-x86/hyperv.h            | 253 
> > +-----------------------------
> >  scripts/update-linux-headers.sh           |  79 +++++-----
> >  target-i386/kvm.c                         |   2 +-
> >  4 files changed, 52 insertions(+), 295 deletions(-)
> >  copy {linux-headers => include/standard-headers}/asm-x86/hyperv.h (98%)

> > diff --git a/scripts/update-linux-headers.sh 
> > b/scripts/update-linux-headers.sh
> > index 7f7b592..2f25e84 100755
> > --- a/scripts/update-linux-headers.sh
> > +++ b/scripts/update-linux-headers.sh
> > @@ -28,39 +28,32 @@ if [ -z "$output" ]; then
> >      output="$PWD"
> >  fi
> >  
> > -cp_virtio() {
> > -    from=$1
> > +cp_portable() {
> > +    f=$1
> >      to=$2
> > -    virtio=$(find "$from" -name '*virtio*h' -o -name "input.h" -o -name 
> > "pci_regs.h")
> > -    if [ "$virtio" ]; then
> > -        rm -rf "$to"
> > -        mkdir -p "$to"
> > -        for f in $virtio; do
> > -            if
> > -                grep '#include' "$f" | grep -v -e 'linux/virtio' \
> > -                                             -e 'linux/types' \
> > -                                             -e 'stdint' \
> > -                                             -e 'linux/if_ether' \
> > -                                             -e 'sys/' \
> > -                                             > /dev/null
> > -            then
> > -                echo "Unexpected #include in input file $f".
> > -                exit 2
> > -            fi
> > -
> > -            header=$(basename "$f");
> > -            sed -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' \
> > -                -e 's/<linux\/\([^>]*\)>/"standard-headers\/linux\/\1"/' \
> > -                -e 's/__bitwise__//' \
> > -                -e 's/__attribute__((packed))/QEMU_PACKED/' \
> > -                -e 's/__inline__/inline/' \
> > -                -e '/sys\/ioctl.h/d' \
> > -                "$f" > "$to/$header";
> > -        done
> > +    if
> > +        grep '#include' "$f" | grep -v -e 'linux/virtio' \
> > +                                     -e 'linux/types' \
> > +                                     -e 'stdint' \
> > +                                     -e 'linux/if_ether' \
> > +                                     -e 'sys/' \
> > +                                     > /dev/null
> > +    then
> > +        echo "Unexpected #include in input file $f".
> > +        exit 2
> >      fi
> > +
> > +    header=$(basename "$f");
> > +    sed -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' \
> > +        -e 's/<linux\/\([^>]*\)>/"standard-headers\/linux\/\1"/' \
> > +        -e 's/__bitwise__//' \
> > +        -e 's/__attribute__((packed))/QEMU_PACKED/' \
> > +        -e 's/__inline__/inline/' \
> > +        -e '/sys\/ioctl.h/d' \
> > +        "$f" > "$to/$header";
> >  }
> >  
> >  # This will pick up non-directories too (eg "Kconfig") but we will
> > @@ -68,6 +61,7 @@ cp_virtio() {
> >  ARCHLIST=$(cd "$linux/arch" && echo *)
> >  
> >  for arch in $ARCHLIST; do
> > +
> >      # Discard anything which isn't a KVM-supporting architecture
> >      if ! [ -e "$linux/arch/$arch/include/asm/kvm.h" ] &&
> >          ! [ -e "$linux/arch/$arch/include/uapi/asm/kvm.h" ] ; then
> 
> This empty line looks ugly imho.
> 
> > @@ -86,14 +80,19 @@ for arch in $ARCHLIST; do
> >      for header in kvm.h kvm_para.h; do
> >          cp "$tmpdir/include/asm/$header" "$output/linux-headers/asm-$arch"
> >      done
> > -    if [ $arch = x86 ]; then
> > -        cp "$tmpdir/include/asm/hyperv.h" "$output/linux-headers/asm-x86"
> > -    fi
> >      if [ $arch = powerpc ]; then
> >          cp "$tmpdir/include/asm/epapr_hcalls.h" 
> > "$output/linux-headers/asm-powerpc/"
> >      fi
> >  
> > -    cp_virtio "$tmpdir/include/asm" 
> > "$output/include/standard-headers/asm-$arch"
> > +    rm -rf "$output/include/standard-headers/asm-$arch"
> > +    mkdir -p "$output/include/standard-headers/asm-$arch"
> > +    if [ $arch = s390 ]; then
> > +        cp_portable "$tmpdir/include/asm/kvm_virtio.h" 
> > "$output/include/standard-headers/asm-s390/"
> > +        cp_portable "$tmpdir/include/asm/virtio-ccw.h" 
> > "$output/include/standard-headers/asm-s390/"
> 
> I think it's possible that s390 will split its virtio files up in
> the future, 

I frankly don't see how we'd want to split those up any further.

> or that more architectures will add their own.

Probably unlikely for virtio files: s390 is the only one with two
unique transports.

For other portable headers: possible; but I'd think they're more likely
to appear in architecture-independent code.

> See below for a suggestion.
> 
> 
> > +    fi
> > +    if [ $arch = x86 ]; then
> > +        cp_portable "$tmpdir/include/asm/hyperv.h" 
> > "$output/include/standard-headers/asm-x86/"
> > +    fi
> >  done
> >  
> >  rm -rf "$output/linux-headers/linux"
> > @@ -113,6 +112,9 @@ else
> >      cp "$linux/COPYING" "$output/linux-headers"
> >  fi
> >  
> > +cat <<EOF >$output/linux-headers/asm-x86/hyperv.h
> > +#include "standard-headers/asm-x86/hyperv.h"
> > +EOF
> 
> I don't think this is needed. We only did this for
> virtio_config to avoid the code churn. Hyperv has
> a single user, so no issue.
> 
> >  cat <<EOF >$output/linux-headers/linux/virtio_config.h
> >  #include "standard-headers/linux/virtio_config.h"
> >  EOF
> > @@ -120,7 +122,12 @@ cat <<EOF >$output/linux-headers/linux/virtio_ring.h
> >  #include "standard-headers/linux/virtio_ring.h"
> >  EOF
> >  
> > -cp_virtio "$tmpdir/include/linux/" "$output/include/standard-headers/linux"
> > +rm -rf "$output/include/standard-headers/linux"
> > +mkdir -p "$output/include/standard-headers/linux"
> > +for i in "$tmpdir"/include/linux/*virtio*.h 
> > "$tmpdir/include/linux/input.h" \
> > +         "$tmpdir/include/linux/pci_regs.h"; do
> > +    cp_portable "$i" "$output/include/standard-headers/linux"
> > +done
> 
> How about we move the above loop into cp_virtio?
> Then we can reuse it for asm like we did.
> hyperv can use cp_portable if it wants to.

I prefer specifying the files to copy outside of the copying function:
it's much more obvious what's going on. Otherwise, you get "if there's
a file that happens to match this pattern, copy it" - and it's unclear
to the casual reader which of those actually exist, as linux/ and asm/
have different sets of files.

> 
> >  
> >  cat <<EOF >$output/include/standard-headers/linux/types.h
> >  #include <stdint.h>