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

["Michael S. Tsirkin" <mst@redhat.com>]

On Thu, Sep 10, 2015 at 11:19:48AM +0200, Paolo Bonzini wrote:
> 
> 
> On 10/09/2015 09:42, 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 <pbonzini@redhat.com>
> > 
> > I'd rather see a script update, then result of running it
> > in a separate patch.
> 
> Okay, I'll split the patch in two.
> 
> >> @@ -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.
> 
> Right.
> 
> >> +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.
> 
> It is needed because kvm headers include <asm/hyperv.h> and you don't
> want to pick the system version.

I see. Makes sense. Then maybe don't bother with kvm.c either?
Keeps it consistent ...

> >>  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?
> 
> I disagree, because input.h and pci_regs.h make no sense for s390.
> 
> Paolo

And Cornelia thinks s390 won't need anything new ever, either.
OK then.

> > Then we can reuse it for asm like we did.
> > hyperv can use cp_portable if it wants to.
> > 
> >>  
> >>  cat <<EOF >$output/include/standard-headers/linux/types.h
> >>  #include <stdint.h>
> >> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> >> index 066d03d..d8a11be 100644
> >> --- a/target-i386/kvm.c
> >> +++ b/target-i386/kvm.c
> >> @@ -33,7 +33,7 @@
> >>  #include "hw/i386/apic_internal.h"
> >>  #include "hw/i386/apic-msidef.h"
> >>  #include "exec/ioport.h"
> >> -#include <asm/hyperv.h>
> >> +#include "standard-headers/asm-x86/hyperv.h"
> >>  #include "hw/pci/pci.h"
> >>  #include "migration/migration.h"
> >>  #include "exec/memattrs.h"
> >> -- 
> >> 2.4.3