From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44553) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZZx0r-0002o1-T9 for qemu-devel@nongnu.org; Thu, 10 Sep 2015 04:15:23 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZZx0n-0007Da-L4 for qemu-devel@nongnu.org; Thu, 10 Sep 2015 04:15:21 -0400 Received: from e06smtp07.uk.ibm.com ([195.75.94.103]:34506) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZZx0n-0007DJ-8r for qemu-devel@nongnu.org; Thu, 10 Sep 2015 04:15:17 -0400 Received: from /spool/local by e06smtp07.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 10 Sep 2015 09:05:03 +0100 Received: from b06cxnps4076.portsmouth.uk.ibm.com (d06relay13.portsmouth.uk.ibm.com [9.149.109.198]) by d06dlp01.portsmouth.uk.ibm.com (Postfix) with ESMTP id 5EC2817D8056 for ; Thu, 10 Sep 2015 09:06:15 +0100 (BST) Received: from d06av09.portsmouth.uk.ibm.com (d06av09.portsmouth.uk.ibm.com [9.149.37.250]) by b06cxnps4076.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id t8A84T7O27983970 for ; Thu, 10 Sep 2015 08:04:29 GMT Received: from d06av09.portsmouth.uk.ibm.com (localhost [127.0.0.1]) by d06av09.portsmouth.uk.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id t8A84TgF009695 for ; Thu, 10 Sep 2015 02:04:29 -0600 Date: Thu, 10 Sep 2015 10:04:26 +0200 From: Cornelia Huck Message-ID: <20150910100426.13f52f3c.cornelia.huck@de.ibm.com> In-Reply-To: <20150910103319-mutt-send-email-mst@redhat.com> References: <1441816441-7730-1-git-send-email-pbonzini@redhat.com> <1441816441-7730-3-git-send-email-pbonzini@redhat.com> <20150910103319-mutt-send-email-mst@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 2/2] target-i386: move asm-x86/hyperv.h to standard-headers List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: Paolo Bonzini , qemu-devel@nongnu.org, den@openvz.org On Thu, 10 Sep 2015 10:42:31 +0300 "Michael S. Tsirkin" 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 > > 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/]*\)>/"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/]*\)>/"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 <$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 <$output/linux-headers/linux/virtio_config.h > > #include "standard-headers/linux/virtio_config.h" > > EOF > > @@ -120,7 +122,12 @@ cat <$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 <$output/include/standard-headers/linux/types.h > > #include