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

[Paolo Bonzini <pbonzini@redhat.com>]

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.

>>  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

> 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