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

[Cornelia Huck]

On Wed,  9 Sep 2015 15:25:52 +0200
Paolo Bonzini <address@hidden> 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>
> ---
>  .../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, 49 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..a5f8935 100755
> --- a/scripts/update-linux-headers.sh
> +++ b/scripts/update-linux-headers.sh
> @@ -28,39 +28,33 @@ 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
> +    mkdir -p "$to"

I'd probably move the mkdir outside of this function and...

> +    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 +62,8 @@ cp_virtio() {
>  ARCHLIST=$(cd "$linux/arch" && echo *)
> 
>  for arch in $ARCHLIST; do
> +    rm -rf "$output/include/standard-headers/$arch"

...move this line to...

> +
>      # 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
> @@ -86,14 +82,16 @@ 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"

...just before you actually handle the architectures with standard
headers.

> +    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/"
> +    fi
> +    if [ $arch = x86 ]; then
> +        cp_portable "$tmpdir/include/asm/hyperv.h" 
> "$output/include/standard-headers/asm-$arch/"

Either make this "asm-x86" or use "asm-$arch" instead of "asm-s390"
above; else looks a bit inconsistent :)

> +    fi
>  done
> 
>  rm -rf "$output/linux-headers/linux"
> @@ -113,6 +111,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
>  cat <<EOF >$output/linux-headers/linux/virtio_config.h
>  #include "standard-headers/linux/virtio_config.h"
>  EOF
> @@ -120,7 +121,11 @@ 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"
> +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
> 
>  cat <<EOF >$output/include/standard-headers/linux/types.h
>  #include <stdint.h>

On the whole, this looks reasonable.