[Qemu-devel] [PATCH RFC] scripts/update-linux-headers.sh: pull virtio hdrs

[Qemu-devel] [PATCH RFC] scripts/update-linux-headers.sh: pull virtio hdrs

[Michael S. Tsirkin]

It doesn't make sense to copy values manually:
the only issue with getting headers from linux
seems to be dealing with linux/types, we
can easily fix that automatically while importing.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---

FYI this is what I propose instead of the recently
suggested
    virtio: uniform virtio device IDs
we can then rework existing code to include these headers.

Will automatically bring in goodies as they arrive in linux.

This doesn't yet import virtio ccw header,
that won't be hard to add later.

 scripts/update-linux-headers.sh | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/scripts/update-linux-headers.sh b/scripts/update-linux-headers.sh
index c8e026d..0bd8437 100755
--- a/scripts/update-linux-headers.sh
+++ b/scripts/update-linux-headers.sh
@@ -76,4 +76,14 @@ else
     cp "$linux/COPYING" "$output/linux-headers"
 fi
 
+rm -rf "$output/standard-headers/linux"
+mkdir -p "$output/standard-headers/linux"
+for f in $tmpdir/include/linux/virtio*h; do
+    header=$(expr "$f" : '.*/\(.*\)');
+    sed -e 's/__u\([0-9][0-9]*\)/uint\1_t/g' \
+        -e 's/linux\/types/inttypes/' \
+        -e 's/__bitwise__//' \
+        "$tmpdir/include/linux/$header" > \
+        "$output/standard-headers/linux/$header";
+done
 rm -rf "$tmpdir"
-- 
MST

Re: [Qemu-devel] [PATCH RFC] scripts/update-linux-headers.sh: pull virtio hdrs

[Chen, Tiejun]

On 2015/2/10 3:56, Michael S. Tsirkin wrote:
> It doesn't make sense to copy values manually:
> the only issue with getting headers from linux
> seems to be dealing with linux/types, we
> can easily fix that automatically while importing.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>
> FYI this is what I propose instead of the recently
> suggested
>      virtio: uniform virtio device IDs
> we can then rework existing code to include these headers.
>
> Will automatically bring in goodies as they arrive in linux.
>
> This doesn't yet import virtio ccw header,
> that won't be hard to add later.
>
>   scripts/update-linux-headers.sh | 10 ++++++++++
>   1 file changed, 10 insertions(+)
>
> diff --git a/scripts/update-linux-headers.sh b/scripts/update-linux-headers.sh
> index c8e026d..0bd8437 100755
> --- a/scripts/update-linux-headers.sh
> +++ b/scripts/update-linux-headers.sh
> @@ -76,4 +76,14 @@ else
>       cp "$linux/COPYING" "$output/linux-headers"
>   fi
>
> +rm -rf "$output/standard-headers/linux"
> +mkdir -p "$output/standard-headers/linux"

Shouldn't we add something in configure file to execute this automatically?

Or instead of creating 'standard-headers/, why can't we go that existing 
linux-headers/?

Thanks
Tiejun

> +for f in $tmpdir/include/linux/virtio*h; do
> +    header=$(expr "$f" : '.*/\(.*\)');
> +    sed -e 's/__u\([0-9][0-9]*\)/uint\1_t/g' \
> +        -e 's/linux\/types/inttypes/' \
> +        -e 's/__bitwise__//' \
> +        "$tmpdir/include/linux/$header" > \
> +        "$output/standard-headers/linux/$header";
> +done
>   rm -rf "$tmpdir"
>

Re: [Qemu-devel] [PATCH RFC] scripts/update-linux-headers.sh: pull virtio hdrs

[Peter Maydell]

On 11 February 2015 at 01:36, Chen, Tiejun <tiejun.chen@intel.com> wrote:
> On 2015/2/10 3:56, Michael S. Tsirkin wrote:
>>
>> It doesn't make sense to copy values manually:
>> the only issue with getting headers from linux
>> seems to be dealing with linux/types, we
>> can easily fix that automatically while importing.
>>
>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>> ---
>>
>> FYI this is what I propose instead of the recently
>> suggested
>>      virtio: uniform virtio device IDs
>> we can then rework existing code to include these headers.
>>
>> Will automatically bring in goodies as they arrive in linux.
>>
>> This doesn't yet import virtio ccw header,
>> that won't be hard to add later.
>>
>>   scripts/update-linux-headers.sh | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> diff --git a/scripts/update-linux-headers.sh
>> b/scripts/update-linux-headers.sh
>> index c8e026d..0bd8437 100755
>> --- a/scripts/update-linux-headers.sh
>> +++ b/scripts/update-linux-headers.sh
>> @@ -76,4 +76,14 @@ else
>>       cp "$linux/COPYING" "$output/linux-headers"
>>   fi
>>
>> +rm -rf "$output/standard-headers/linux"
>> +mkdir -p "$output/standard-headers/linux"
>
>
> Shouldn't we add something in configure file to execute this automatically?

No. We want to run this script only when we're updating the
header files, which only happens when we have a valid
kernel source tree available and you're a developer doing
it as a specific action. configure is run by everybody and
should definitely not be doing header updates.

> Or instead of creating 'standard-headers/, why can't we go that existing
> linux-headers/?

The linux-headers/ directory contains header files which can only
validly be included if the host we're compiling on is Linux. Some
of them will cause compile failures on OSX or Windows if they
are in the include path. The idea of this patch is that the
standard-headers/ directory has "sanitized" header files which
have had the linux-specific types and includes stripped out.
So if we take the route this patch proposes we do need two
directories.

-- PMM

Re: [Qemu-devel] [PATCH RFC] scripts/update-linux-headers.sh: pull virtio hdrs

[Peter Maydell]

On 9 February 2015 at 19:56, Michael S. Tsirkin <mst@redhat.com> wrote:
> +rm -rf "$output/standard-headers/linux"
> +mkdir -p "$output/standard-headers/linux"
> +for f in $tmpdir/include/linux/virtio*h; do
> +    header=$(expr "$f" : '.*/\(.*\)');
> +    sed -e 's/__u\([0-9][0-9]*\)/uint\1_t/g' \
> +        -e 's/linux\/types/inttypes/' \
> +        -e 's/__bitwise__//' \
> +        "$tmpdir/include/linux/$header" > \
> +        "$output/standard-headers/linux/$header";
> +done

This doesn't seem to be doing anything to fix up
the '__attribute__((packed))' annotations.
Presumably you're intending to put standard-headers/
on the include path? It would probably be better to
not make the headers be in "linux/" in that case,
since it would mean confusion/clashes for what
"linux/virtio_net.h" etc mean -- are they the QEMU
sanitized versions or the host OS's? (Having them
in linux/ also makes code review harder since it breaks
the current rule of thumb that is "no include of
linux/anything in code that's not Linux-host-specific".)

You need to strip out all the #include <linux/something.h>
from these headers, otherwise this won't build on non
Linux hosts. Then you need to add in whatever the
equivalent is to get the defines/types those includes
were providing (for instance our virtio-net.h does a
simple #define of ETH_ALEN). You probably want to make
the update script fail if there's an include it's not
expecting to deal with, otherwise you're likely to
end up with a set of headers that seem OK on Linux
but fail when tested on other OSes -- better to fail
early and for the person trying to do the header update
than to end up with a change that won't pass my build
tests and gets bounced.

All that makes it seem to me like it's more trouble
than it's worth compared to doing a one-time manual
import.

-- PMM

Re: [Qemu-devel] [PATCH RFC] scripts/update-linux-headers.sh: pull virtio hdrs

[Chen, Tiejun]

On 2015/2/11 10:03, Peter Maydell wrote:
> On 11 February 2015 at 01:36, Chen, Tiejun <tiejun.chen@intel.com> wrote:
>> On 2015/2/10 3:56, Michael S. Tsirkin wrote:
>>>
>>> It doesn't make sense to copy values manually:
>>> the only issue with getting headers from linux
>>> seems to be dealing with linux/types, we
>>> can easily fix that automatically while importing.
>>>
>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>> ---
>>>
>>> FYI this is what I propose instead of the recently
>>> suggested
>>>       virtio: uniform virtio device IDs
>>> we can then rework existing code to include these headers.
>>>
>>> Will automatically bring in goodies as they arrive in linux.
>>>
>>> This doesn't yet import virtio ccw header,
>>> that won't be hard to add later.
>>>
>>>    scripts/update-linux-headers.sh | 10 ++++++++++
>>>    1 file changed, 10 insertions(+)
>>>
>>> diff --git a/scripts/update-linux-headers.sh
>>> b/scripts/update-linux-headers.sh
>>> index c8e026d..0bd8437 100755
>>> --- a/scripts/update-linux-headers.sh
>>> +++ b/scripts/update-linux-headers.sh
>>> @@ -76,4 +76,14 @@ else
>>>        cp "$linux/COPYING" "$output/linux-headers"
>>>    fi
>>>
>>> +rm -rf "$output/standard-headers/linux"
>>> +mkdir -p "$output/standard-headers/linux"
>>
>>
>> Shouldn't we add something in configure file to execute this automatically?
>
> No. We want to run this script only when we're updating the
> header files, which only happens when we have a valid
> kernel source tree available and you're a developer doing
> it as a specific action. configure is run by everybody and
> should definitely not be doing header updates.
>
>> Or instead of creating 'standard-headers/, why can't we go that existing
>> linux-headers/?
>
> The linux-headers/ directory contains header files which can only
> validly be included if the host we're compiling on is Linux. Some
> of them will cause compile failures on OSX or Windows if they
> are in the include path. The idea of this patch is that the
> standard-headers/ directory has "sanitized" header files which
> have had the linux-specific types and includes stripped out.
> So if we take the route this patch proposes we do need two
> directories.
>

This confounds me since for instance, one of goals based on this patch 
is, it exposes those Virtio devices ID definition to hw/virtio, instead 
of my original patch, right? So without this sort of standard-hearders, 
how can we compile virtio? Or you mean we still keep those original 
stuff in include/hw/virtio*, but somehow update them once we execute 
that script manually.

Thanks
Tiejun

Re: [Qemu-devel] [PATCH RFC] scripts/update-linux-headers.sh: pull virtio hdrs

[Peter Maydell]

On 11 February 2015 at 02:50, Chen, Tiejun <tiejun.chen@intel.com> wrote:
> On 2015/2/11 10:03, Peter Maydell wrote:
>> The linux-headers/ directory contains header files which can only
>> validly be included if the host we're compiling on is Linux. Some
>> of them will cause compile failures on OSX or Windows if they
>> are in the include path. The idea of this patch is that the
>> standard-headers/ directory has "sanitized" header files which
>> have had the linux-specific types and includes stripped out.
>> So if we take the route this patch proposes we do need two
>> directories.
>>
>
> This confounds me since for instance, one of goals based on this patch is,
> it exposes those Virtio devices ID definition to hw/virtio, instead of my
> original patch, right? So without this sort of standard-hearders, how can we
> compile virtio? Or you mean we still keep those original stuff in
> include/hw/virtio*, but somehow update them once we execute that script
> manually.

I'm confused about why you're confused. We have two basic
approaches we can take:

(1) What we do at the moment. There are headers defining the virtio
interface in include/hw/virtio, and these are basically manually
created and updated as necessary.

(2) What this patch is proposing. The headers defining virtio are
automatically copied into standard-headers/ and fixed up to make
them work with QEMU on all the hosts we support. This happens when
this script is run by a developer to update QEMU's headers based
on some new upstream kernel.

Personally I think that option 1 is more reliable and overall
less effort, since automatiing the fixups is hard and virtio
doesn't change very much.

-- PMM

Re: [Qemu-devel] [PATCH RFC] scripts/update-linux-headers.sh: pull virtio hdrs

[Chen, Tiejun]

On 2015/2/11 11:46, Peter Maydell wrote:
> On 11 February 2015 at 02:50, Chen, Tiejun <tiejun.chen@intel.com> wrote:
>> On 2015/2/11 10:03, Peter Maydell wrote:
>>> The linux-headers/ directory contains header files which can only
>>> validly be included if the host we're compiling on is Linux. Some
>>> of them will cause compile failures on OSX or Windows if they
>>> are in the include path. The idea of this patch is that the
>>> standard-headers/ directory has "sanitized" header files which
>>> have had the linux-specific types and includes stripped out.
>>> So if we take the route this patch proposes we do need two
>>> directories.
>>>
>>
>> This confounds me since for instance, one of goals based on this patch is,
>> it exposes those Virtio devices ID definition to hw/virtio, instead of my
>> original patch, right? So without this sort of standard-hearders, how can we
>> compile virtio? Or you mean we still keep those original stuff in
>> include/hw/virtio*, but somehow update them once we execute that script
>> manually.
>
> I'm confused about why you're confused. We have two basic
> approaches we can take:
>
> (1) What we do at the moment. There are headers defining the virtio
> interface in include/hw/virtio, and these are basically manually
> created and updated as necessary.
>
> (2) What this patch is proposing. The headers defining virtio are
> automatically copied into standard-headers/ and fixed up to make
> them work with QEMU on all the hosts we support. This happens when
> this script is run by a developer to update QEMU's headers based
> on some new upstream kernel.

I guess this mean this patch should be extended to smooth something in 
include/hw/virtio* in some ways.

>
> Personally I think that option 1 is more reliable and overall

Agreed.

> less effort, since automatiing the fixups is hard and virtio
> doesn't change very much.
>

So sounds my original patch is fine to you.

Thanks
Tiejun

Re: [Qemu-devel] [PATCH RFC] scripts/update-linux-headers.sh: pull virtio hdrs

[Michael S. Tsirkin]

On Wed, Feb 11, 2015 at 03:46:20AM +0000, Peter Maydell wrote:
> On 11 February 2015 at 02:50, Chen, Tiejun <tiejun.chen@intel.com> wrote:
> > On 2015/2/11 10:03, Peter Maydell wrote:
> >> The linux-headers/ directory contains header files which can only
> >> validly be included if the host we're compiling on is Linux. Some
> >> of them will cause compile failures on OSX or Windows if they
> >> are in the include path. The idea of this patch is that the
> >> standard-headers/ directory has "sanitized" header files which
> >> have had the linux-specific types and includes stripped out.
> >> So if we take the route this patch proposes we do need two
> >> directories.
> >>
> >
> > This confounds me since for instance, one of goals based on this patch is,
> > it exposes those Virtio devices ID definition to hw/virtio, instead of my
> > original patch, right? So without this sort of standard-hearders, how can we
> > compile virtio? Or you mean we still keep those original stuff in
> > include/hw/virtio*, but somehow update them once we execute that script
> > manually.
> 
> I'm confused about why you're confused. We have two basic
> approaches we can take:
> 
> (1) What we do at the moment. There are headers defining the virtio
> interface in include/hw/virtio, and these are basically manually
> created and updated as necessary.
> 
> (2) What this patch is proposing. The headers defining virtio are
> automatically copied into standard-headers/ and fixed up to make
> them work with QEMU on all the hosts we support. This happens when
> this script is run by a developer to update QEMU's headers based
> on some new upstream kernel.
> 
> Personally I think that option 1 is more reliable and overall
> less effort, since automatiing the fixups is hard and virtio
> doesn't change very much.
> 
> -- PMM

It picked up speed recently, so it's too much effort I think.
My script seems to have automated fixups - didn't seem hard.

-- 
MST

Re: [Qemu-devel] [PATCH RFC] scripts/update-linux-headers.sh: pull virtio hdrs

[Michael S. Tsirkin]

On Wed, Feb 11, 2015 at 02:12:35AM +0000, Peter Maydell wrote:
> On 9 February 2015 at 19:56, Michael S. Tsirkin <mst@redhat.com> wrote:
> > +rm -rf "$output/standard-headers/linux"
> > +mkdir -p "$output/standard-headers/linux"
> > +for f in $tmpdir/include/linux/virtio*h; do
> > +    header=$(expr "$f" : '.*/\(.*\)');
> > +    sed -e 's/__u\([0-9][0-9]*\)/uint\1_t/g' \
> > +        -e 's/linux\/types/inttypes/' \
> > +        -e 's/__bitwise__//' \
> > +        "$tmpdir/include/linux/$header" > \
> > +        "$output/standard-headers/linux/$header";
> > +done
> 
> This doesn't seem to be doing anything to fix up
> the '__attribute__((packed))' annotations.

I don't know - what needs to be fixed up?

> Presumably you're intending to put standard-headers/
> on the include path? It would probably be better to
> not make the headers be in "linux/" in that case,
> since it would mean confusion/clashes for what
> "linux/virtio_net.h" etc mean -- are they the QEMU
> sanitized versions or the host OS's? (Having them
> in linux/ also makes code review harder since it breaks
> the current rule of thumb that is "no include of
> linux/anything in code that's not Linux-host-specific".)

Agreed, I'll change this.

> You need to strip out all the #include <linux/something.h>
> from these headers, otherwise this won't build on non
> Linux hosts.
> Then you need to add in whatever the
> equivalent is to get the defines/types those includes
> were providing (for instance our virtio-net.h does a
> simple #define of ETH_ALEN). You probably want to make
> the update script fail if there's an include it's not
> expecting to deal with, otherwise you're likely to
> end up with a set of headers that seem OK on Linux
> but fail when tested on other OSes -- better to fail
> early and for the person trying to do the header update
> than to end up with a change that won't pass my build
> tests and gets bounced.

OK, seems easy.
Will do.

> All that makes it seem to me like it's more trouble
> than it's worth compared to doing a one-time manual
> import.
> 
> -- PMM

Only true if we are perfect and don't make mistakes.
If we do make mistakes, I want to fix them in one place
and propagate the fix automatically.
And since we are working on virtio 1.0 which is a huge change,
introducing mistakes seems more likely.

-- 
MST

Re: [Qemu-devel] [PATCH RFC] scripts/update-linux-headers.sh: pull virtio hdrs

[Peter Maydell]

On 11 February 2015 at 12:33, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Wed, Feb 11, 2015 at 02:12:35AM +0000, Peter Maydell wrote:
>> On 9 February 2015 at 19:56, Michael S. Tsirkin <mst@redhat.com> wrote:
>> > +rm -rf "$output/standard-headers/linux"
>> > +mkdir -p "$output/standard-headers/linux"
>> > +for f in $tmpdir/include/linux/virtio*h; do
>> > +    header=$(expr "$f" : '.*/\(.*\)');
>> > +    sed -e 's/__u\([0-9][0-9]*\)/uint\1_t/g' \
>> > +        -e 's/linux\/types/inttypes/' \
>> > +        -e 's/__bitwise__//' \
>> > +        "$tmpdir/include/linux/$header" > \
>> > +        "$output/standard-headers/linux/$header";
>> > +done
>>
>> This doesn't seem to be doing anything to fix up
>> the '__attribute__((packed))' annotations.
>
> I don't know - what needs to be fixed up?

Needs to use the QEMU_PACKED macro.

-- PMM

Re: [Qemu-devel] [PATCH RFC] scripts/update-linux-headers.sh: pull virtio hdrs

[Michael S. Tsirkin]

On Wed, Feb 11, 2015 at 01:29:53PM +0000, Peter Maydell wrote:
> On 11 February 2015 at 12:33, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Wed, Feb 11, 2015 at 02:12:35AM +0000, Peter Maydell wrote:
> >> On 9 February 2015 at 19:56, Michael S. Tsirkin <mst@redhat.com> wrote:
> >> > +rm -rf "$output/standard-headers/linux"
> >> > +mkdir -p "$output/standard-headers/linux"
> >> > +for f in $tmpdir/include/linux/virtio*h; do
> >> > +    header=$(expr "$f" : '.*/\(.*\)');
> >> > +    sed -e 's/__u\([0-9][0-9]*\)/uint\1_t/g' \
> >> > +        -e 's/linux\/types/inttypes/' \
> >> > +        -e 's/__bitwise__//' \
> >> > +        "$tmpdir/include/linux/$header" > \
> >> > +        "$output/standard-headers/linux/$header";
> >> > +done
> >>
> >> This doesn't seem to be doing anything to fix up
> >> the '__attribute__((packed))' annotations.
> >
> > I don't know - what needs to be fixed up?
> 
> Needs to use the QEMU_PACKED macro.
> 
> -- PMM

Thanks.