| Message ID | 0C2DB64D-B8EB-492B-876F-111BD818101C@suse.de |
|---|---|
| State | New |
| Headers | show |
> > +++ b/scripts/update-linux-headers.sh > > @@ -46,14 +46,26 @@ for arch in $ARCHLIST; do > > > > make -C "$linux" INSTALL_HDR_PATH="$tmpdir" SRCARCH=$arch > > headers_install > > > > + if [ -e "$linux/arch/$arch/include/uapi" ] && > > + ! [ -e "$output/linux-headers/uapi" ] ; then > > + mkdir "$output/linux-headers/uapi" > > mkdir -p > > But looking through this whole thing, it seems like the root cause is actually > different. We don't want any uapi directories exposed to user space. So let's go > back a step: > > Why do we need the uapi include dir? Because some header is using it. > > linux-headers/asm-powerpc/kvm_para.h: The kvm_para.h (also kvm.h) are now defined in include/uapi/asm/ Is not this the correct thing that any header file in include/uapi/asm/ (in this case kvm_para.h) includes another header file (epapr_hcalls.h) in same directory? Also I think now only the uapi/asm/*.h files should be exposed to userspace (QEMU here). Waiting for David's comment for better clarity ... -Bharat > > #include <uapi/asm/epapr_hcalls.h> > > This is the root cause of the problem. We must never manually include any uapi > header paths. We only ever include their normal asm-counterparts which then may > include uapi (in kernel) or actually are uapi (in user space). David, please > correct me if I'm wrong. > > Could you please try and see if this kernel side patch makes things work for you > too? > > > Alex > > > diff --git a/arch/powerpc/include/uapi/asm/kvm_para.h > b/arch/powerpc/include/uapi/asm/kvm_para.h > index ed0e025..e3af328 100644 > --- a/arch/powerpc/include/uapi/asm/kvm_para.h > +++ b/arch/powerpc/include/uapi/asm/kvm_para.h > @@ -78,7 +78,7 @@ struct kvm_vcpu_arch_shared { > > #define KVM_HCALL_TOKEN(num) _EV_HCALL_TOKEN(EV_KVM_VENDOR_ID, num) > > -#include <uapi/asm/epapr_hcalls.h> > +#include <asm/epapr_hcalls.h> > > #define KVM_FEATURE_MAGIC_PAGE 1 >
On 18.12.2012, at 02:14, Bhushan Bharat-R65777 wrote: >>> +++ b/scripts/update-linux-headers.sh >>> @@ -46,14 +46,26 @@ for arch in $ARCHLIST; do >>> >>> make -C "$linux" INSTALL_HDR_PATH="$tmpdir" SRCARCH=$arch >>> headers_install >>> >>> + if [ -e "$linux/arch/$arch/include/uapi" ] && >>> + ! [ -e "$output/linux-headers/uapi" ] ; then >>> + mkdir "$output/linux-headers/uapi" >> >> mkdir -p >> >> But looking through this whole thing, it seems like the root cause is actually >> different. We don't want any uapi directories exposed to user space. So let's go >> back a step: >> >> Why do we need the uapi include dir? Because some header is using it. >> >> linux-headers/asm-powerpc/kvm_para.h: > > The kvm_para.h (also kvm.h) are now defined in include/uapi/asm/ > > Is not this the correct thing that any header file in include/uapi/asm/ (in this case kvm_para.h) includes another header file (epapr_hcalls.h) in same directory? > > Also I think now only the uapi/asm/*.h files should be exposed to userspace (QEMU here). make headers_install should basically remove all the uapi magic and give us normal backwards-compatible asm trees :). Alex > > Waiting for David's comment for better clarity ... > > -Bharat > >> >> #include <uapi/asm/epapr_hcalls.h> >> >> This is the root cause of the problem. We must never manually include any uapi >> header paths. We only ever include their normal asm-counterparts which then may >> include uapi (in kernel) or actually are uapi (in user space). David, please >> correct me if I'm wrong. >> >> Could you please try and see if this kernel side patch makes things work for you >> too? >> >> >> Alex >> >> >> diff --git a/arch/powerpc/include/uapi/asm/kvm_para.h >> b/arch/powerpc/include/uapi/asm/kvm_para.h >> index ed0e025..e3af328 100644 >> --- a/arch/powerpc/include/uapi/asm/kvm_para.h >> +++ b/arch/powerpc/include/uapi/asm/kvm_para.h >> @@ -78,7 +78,7 @@ struct kvm_vcpu_arch_shared { >> >> #define KVM_HCALL_TOKEN(num) _EV_HCALL_TOKEN(EV_KVM_VENDOR_ID, num) >> >> -#include <uapi/asm/epapr_hcalls.h> >> +#include <asm/epapr_hcalls.h> >> >> #define KVM_FEATURE_MAGIC_PAGE 1 >> > > >
> -----Original Message----- > From: Alexander Graf [mailto:agraf@suse.de] > Sent: Tuesday, December 18, 2012 6:51 AM > To: Bhushan Bharat-R65777 > Cc: qemu-devel qemu-devel; Peter Maydell; Jan Kiszka; qemu-ppc@nongnu.org List; > Marcelo Tosatti; David Howells > Subject: Re: [PATCH v2] Added uapi directory into linux-header > > > On 18.12.2012, at 02:14, Bhushan Bharat-R65777 wrote: > > >>> +++ b/scripts/update-linux-headers.sh > >>> @@ -46,14 +46,26 @@ for arch in $ARCHLIST; do > >>> > >>> make -C "$linux" INSTALL_HDR_PATH="$tmpdir" SRCARCH=$arch > >>> headers_install > >>> > >>> + if [ -e "$linux/arch/$arch/include/uapi" ] && > >>> + ! [ -e "$output/linux-headers/uapi" ] ; then > >>> + mkdir "$output/linux-headers/uapi" > >> > >> mkdir -p > >> > >> But looking through this whole thing, it seems like the root cause is > >> actually different. We don't want any uapi directories exposed to > >> user space. So let's go back a step: > >> > >> Why do we need the uapi include dir? Because some header is using it. > >> > >> linux-headers/asm-powerpc/kvm_para.h: > > > > The kvm_para.h (also kvm.h) are now defined in include/uapi/asm/ > > > > Is not this the correct thing that any header file in include/uapi/asm/ (in > this case kvm_para.h) includes another header file (epapr_hcalls.h) in same > directory? > > > > Also I think now only the uapi/asm/*.h files should be exposed to userspace > (QEMU here). > > make headers_install should basically remove all the uapi magic and give us > normal backwards-compatible asm trees :). I am perfectly fine, How we can do this now :) -Bharat > > > Alex > > > > > Waiting for David's comment for better clarity ... > > > > -Bharat > > > >> > >> #include <uapi/asm/epapr_hcalls.h> > >> > >> This is the root cause of the problem. We must never manually include > >> any uapi header paths. We only ever include their normal > >> asm-counterparts which then may include uapi (in kernel) or actually > >> are uapi (in user space). David, please correct me if I'm wrong. > >> > >> Could you please try and see if this kernel side patch makes things > >> work for you too? > >> > >> > >> Alex > >> > >> > >> diff --git a/arch/powerpc/include/uapi/asm/kvm_para.h > >> b/arch/powerpc/include/uapi/asm/kvm_para.h > >> index ed0e025..e3af328 100644 > >> --- a/arch/powerpc/include/uapi/asm/kvm_para.h > >> +++ b/arch/powerpc/include/uapi/asm/kvm_para.h > >> @@ -78,7 +78,7 @@ struct kvm_vcpu_arch_shared { > >> > >> #define KVM_HCALL_TOKEN(num) _EV_HCALL_TOKEN(EV_KVM_VENDOR_ID, num) > >> > >> -#include <uapi/asm/epapr_hcalls.h> > >> +#include <asm/epapr_hcalls.h> > >> > >> #define KVM_FEATURE_MAGIC_PAGE 1 > >> > > > > > > >
On 18.12.2012, at 02:27, Bhushan Bharat-R65777 wrote: > > >> -----Original Message----- >> From: Alexander Graf [mailto:agraf@suse.de] >> Sent: Tuesday, December 18, 2012 6:51 AM >> To: Bhushan Bharat-R65777 >> Cc: qemu-devel qemu-devel; Peter Maydell; Jan Kiszka; qemu-ppc@nongnu.org List; >> Marcelo Tosatti; David Howells >> Subject: Re: [PATCH v2] Added uapi directory into linux-header >> >> >> On 18.12.2012, at 02:14, Bhushan Bharat-R65777 wrote: >> >>>>> +++ b/scripts/update-linux-headers.sh >>>>> @@ -46,14 +46,26 @@ for arch in $ARCHLIST; do >>>>> >>>>> make -C "$linux" INSTALL_HDR_PATH="$tmpdir" SRCARCH=$arch >>>>> headers_install >>>>> >>>>> + if [ -e "$linux/arch/$arch/include/uapi" ] && >>>>> + ! [ -e "$output/linux-headers/uapi" ] ; then >>>>> + mkdir "$output/linux-headers/uapi" >>>> >>>> mkdir -p >>>> >>>> But looking through this whole thing, it seems like the root cause is >>>> actually different. We don't want any uapi directories exposed to >>>> user space. So let's go back a step: >>>> >>>> Why do we need the uapi include dir? Because some header is using it. >>>> >>>> linux-headers/asm-powerpc/kvm_para.h: >>> >>> The kvm_para.h (also kvm.h) are now defined in include/uapi/asm/ >>> >>> Is not this the correct thing that any header file in include/uapi/asm/ (in >> this case kvm_para.h) includes another header file (epapr_hcalls.h) in same >> directory? >>> >>> Also I think now only the uapi/asm/*.h files should be exposed to userspace >> (QEMU here). >> >> make headers_install should basically remove all the uapi magic and give us >> normal backwards-compatible asm trees :). > > I am perfectly fine, How we can do this now :) Well, for starters, do the headers work if you apply the patch I sent in a previous mail plus the epapr_hcall.h copy? If so, then that's the way to go :) Alex
> -----Original Message----- > From: Alexander Graf [mailto:agraf@suse.de] > Sent: Tuesday, December 18, 2012 7:00 AM > To: Bhushan Bharat-R65777 > Cc: qemu-devel qemu-devel; Peter Maydell; Jan Kiszka; qemu-ppc@nongnu.org List; > Marcelo Tosatti; David Howells > Subject: Re: [PATCH v2] Added uapi directory into linux-header > > > On 18.12.2012, at 02:27, Bhushan Bharat-R65777 wrote: > > > > > > >> -----Original Message----- > >> From: Alexander Graf [mailto:agraf@suse.de] > >> Sent: Tuesday, December 18, 2012 6:51 AM > >> To: Bhushan Bharat-R65777 > >> Cc: qemu-devel qemu-devel; Peter Maydell; Jan Kiszka; > >> qemu-ppc@nongnu.org List; Marcelo Tosatti; David Howells > >> Subject: Re: [PATCH v2] Added uapi directory into linux-header > >> > >> > >> On 18.12.2012, at 02:14, Bhushan Bharat-R65777 wrote: > >> > >>>>> +++ b/scripts/update-linux-headers.sh > >>>>> @@ -46,14 +46,26 @@ for arch in $ARCHLIST; do > >>>>> > >>>>> make -C "$linux" INSTALL_HDR_PATH="$tmpdir" SRCARCH=$arch > >>>>> headers_install > >>>>> > >>>>> + if [ -e "$linux/arch/$arch/include/uapi" ] && > >>>>> + ! [ -e "$output/linux-headers/uapi" ] ; then > >>>>> + mkdir "$output/linux-headers/uapi" > >>>> > >>>> mkdir -p > >>>> > >>>> But looking through this whole thing, it seems like the root cause > >>>> is actually different. We don't want any uapi directories exposed > >>>> to user space. So let's go back a step: > >>>> > >>>> Why do we need the uapi include dir? Because some header is using it. > >>>> > >>>> linux-headers/asm-powerpc/kvm_para.h: > >>> > >>> The kvm_para.h (also kvm.h) are now defined in include/uapi/asm/ > >>> > >>> Is not this the correct thing that any header file in > >>> include/uapi/asm/ (in > >> this case kvm_para.h) includes another header file (epapr_hcalls.h) > >> in same directory? > >>> > >>> Also I think now only the uapi/asm/*.h files should be exposed to > >>> userspace > >> (QEMU here). > >> > >> make headers_install should basically remove all the uapi magic and > >> give us normal backwards-compatible asm trees :). > > > > I am perfectly fine, How we can do this now :) > > Well, for starters, do the headers work if you apply the patch I sent in a > previous mail plus the epapr_hcall.h copy? If so, then that's the way to go :) Are you really sure that applying a patch and then syncing (or other way round) is the way you want to go ? To me it does not look good, I think we can go with the script changes to make install_header is updated to do the work. -Bharat > > > Alex >
On 18.12.2012, at 03:07, Bhushan Bharat-R65777 <R65777@freescale.com> wrote: > > >> -----Original Message----- >> From: Alexander Graf [mailto:agraf@suse.de] >> Sent: Tuesday, December 18, 2012 7:00 AM >> To: Bhushan Bharat-R65777 >> Cc: qemu-devel qemu-devel; Peter Maydell; Jan Kiszka; qemu-ppc@nongnu.org List; >> Marcelo Tosatti; David Howells >> Subject: Re: [PATCH v2] Added uapi directory into linux-header >> >> >> On 18.12.2012, at 02:27, Bhushan Bharat-R65777 wrote: >> >>> >>> >>>> -----Original Message----- >>>> From: Alexander Graf [mailto:agraf@suse.de] >>>> Sent: Tuesday, December 18, 2012 6:51 AM >>>> To: Bhushan Bharat-R65777 >>>> Cc: qemu-devel qemu-devel; Peter Maydell; Jan Kiszka; >>>> qemu-ppc@nongnu.org List; Marcelo Tosatti; David Howells >>>> Subject: Re: [PATCH v2] Added uapi directory into linux-header >>>> >>>> >>>> On 18.12.2012, at 02:14, Bhushan Bharat-R65777 wrote: >>>> >>>>>>> +++ b/scripts/update-linux-headers.sh >>>>>>> @@ -46,14 +46,26 @@ for arch in $ARCHLIST; do >>>>>>> >>>>>>> make -C "$linux" INSTALL_HDR_PATH="$tmpdir" SRCARCH=$arch >>>>>>> headers_install >>>>>>> >>>>>>> + if [ -e "$linux/arch/$arch/include/uapi" ] && >>>>>>> + ! [ -e "$output/linux-headers/uapi" ] ; then >>>>>>> + mkdir "$output/linux-headers/uapi" >>>>>> >>>>>> mkdir -p >>>>>> >>>>>> But looking through this whole thing, it seems like the root cause >>>>>> is actually different. We don't want any uapi directories exposed >>>>>> to user space. So let's go back a step: >>>>>> >>>>>> Why do we need the uapi include dir? Because some header is using it. >>>>>> >>>>>> linux-headers/asm-powerpc/kvm_para.h: >>>>> >>>>> The kvm_para.h (also kvm.h) are now defined in include/uapi/asm/ >>>>> >>>>> Is not this the correct thing that any header file in >>>>> include/uapi/asm/ (in >>>> this case kvm_para.h) includes another header file (epapr_hcalls.h) >>>> in same directory? >>>>> >>>>> Also I think now only the uapi/asm/*.h files should be exposed to >>>>> userspace >>>> (QEMU here). >>>> >>>> make headers_install should basically remove all the uapi magic and >>>> give us normal backwards-compatible asm trees :). >>> >>> I am perfectly fine, How we can do this now :) >> >> Well, for starters, do the headers work if you apply the patch I sent in a >> previous mail plus the epapr_hcall.h copy? If so, then that's the way to go :) > > Are you really sure that applying a patch and then syncing (or other way round) is the way you want to go ? Yes, because I'm quite confident we're generating broken headers right now. Alex > > To me it does not look good, I think we can go with the script changes to make install_header is updated to do the work. > > -Bharat > >> >> >> Alex >> > >
> >>>>>>> +++ b/scripts/update-linux-headers.sh > >>>>>>> @@ -46,14 +46,26 @@ for arch in $ARCHLIST; do > >>>>>>> > >>>>>>> make -C "$linux" INSTALL_HDR_PATH="$tmpdir" SRCARCH=$arch > >>>>>>> headers_install > >>>>>>> > >>>>>>> + if [ -e "$linux/arch/$arch/include/uapi" ] && > >>>>>>> + ! [ -e "$output/linux-headers/uapi" ] ; then > >>>>>>> + mkdir "$output/linux-headers/uapi" > >>>>>> > >>>>>> mkdir -p > >>>>>> > >>>>>> But looking through this whole thing, it seems like the root > >>>>>> cause is actually different. We don't want any uapi directories > >>>>>> exposed to user space. So let's go back a step: > >>>>>> > >>>>>> Why do we need the uapi include dir? Because some header is using it. > >>>>>> > >>>>>> linux-headers/asm-powerpc/kvm_para.h: > >>>>> > >>>>> The kvm_para.h (also kvm.h) are now defined in include/uapi/asm/ > >>>>> > >>>>> Is not this the correct thing that any header file in > >>>>> include/uapi/asm/ (in > >>>> this case kvm_para.h) includes another header file (epapr_hcalls.h) > >>>> in same directory? > >>>>> > >>>>> Also I think now only the uapi/asm/*.h files should be exposed to > >>>>> userspace > >>>> (QEMU here). > >>>> > >>>> make headers_install should basically remove all the uapi magic and > >>>> give us normal backwards-compatible asm trees :). > >>> > >>> I am perfectly fine, How we can do this now :) > >> > >> Well, for starters, do the headers work if you apply the patch I sent > >> in a previous mail plus the epapr_hcall.h copy? If so, then that's > >> the way to go :) > > > > Are you really sure that applying a patch and then syncing (or other way > round) is the way you want to go ? > > Yes, because I'm quite confident we're generating broken headers right now. Ok, so every time someone does the sync he/she has to do this? Also do we think that sometime in future this will be taken care by make header_install? Thanks -Bharat > > Alex > > > > > To me it does not look good, I think we can go with the script changes to make > install_header is updated to do the work. > > > > -Bharat > > > >> > >> > >> Alex > >> > > > >
On 18.12.2012, at 11:19, Bhushan Bharat-R65777 <R65777@freescale.com> wrote: >>>>>>>>> +++ b/scripts/update-linux-headers.sh >>>>>>>>> @@ -46,14 +46,26 @@ for arch in $ARCHLIST; do >>>>>>>>> >>>>>>>>> make -C "$linux" INSTALL_HDR_PATH="$tmpdir" SRCARCH=$arch >>>>>>>>> headers_install >>>>>>>>> >>>>>>>>> + if [ -e "$linux/arch/$arch/include/uapi" ] && >>>>>>>>> + ! [ -e "$output/linux-headers/uapi" ] ; then >>>>>>>>> + mkdir "$output/linux-headers/uapi" >>>>>>>> >>>>>>>> mkdir -p >>>>>>>> >>>>>>>> But looking through this whole thing, it seems like the root >>>>>>>> cause is actually different. We don't want any uapi directories >>>>>>>> exposed to user space. So let's go back a step: >>>>>>>> >>>>>>>> Why do we need the uapi include dir? Because some header is using it. >>>>>>>> >>>>>>>> linux-headers/asm-powerpc/kvm_para.h: >>>>>>> >>>>>>> The kvm_para.h (also kvm.h) are now defined in include/uapi/asm/ >>>>>>> >>>>>>> Is not this the correct thing that any header file in >>>>>>> include/uapi/asm/ (in >>>>>> this case kvm_para.h) includes another header file (epapr_hcalls.h) >>>>>> in same directory? >>>>>>> >>>>>>> Also I think now only the uapi/asm/*.h files should be exposed to >>>>>>> userspace >>>>>> (QEMU here). >>>>>> >>>>>> make headers_install should basically remove all the uapi magic and >>>>>> give us normal backwards-compatible asm trees :). >>>>> >>>>> I am perfectly fine, How we can do this now :) >>>> >>>> Well, for starters, do the headers work if you apply the patch I sent >>>> in a previous mail plus the epapr_hcall.h copy? If so, then that's >>>> the way to go :) >>> >>> Are you really sure that applying a patch and then syncing (or other way >> round) is the way you want to go ? >> >> Yes, because I'm quite confident we're generating broken headers right now. > > Ok, so every time someone does the sync he/she has to do this? Also do we think that sometime in future this will be taken care by make header_install? That's the point. The QEMU header sync is a hack that allows us to not use system headers. System headers could be outdated wrt features we want to support. However, system headers get generated using make headers_install. And there we generate a header that refers to a uapi directory that doesn't exist after headers_install. So the next time a distro updates their system headers, they get broken ones. That's what the patch against Linux is trying to fix. Alex
Alexander Graf <agraf@suse.de> wrote: > But looking through this whole thing, it seems like the root cause is > actually different. We don't want any uapi directories exposed to user > space. So let's go back a step: > > Why do we need the uapi include dir? Because some header is using it. > > linux-headers/asm-powerpc/kvm_para.h: > > #include <uapi/asm/epapr_hcalls.h> > > This is the root cause of the problem. We must never manually include any > uapi header paths. We only ever include their normal asm-counterparts which > then may include uapi (in kernel) or actually are uapi (in user > space). David, please correct me if I'm wrong. I think you're correct, if I understand what you're saying. Within the kernel sources, the userspace facing headers are in uapi/ directories whilst the stuff userspace shouldn't see is outside of that. uapi/ headers should _not_ #include headers with uapi/ prefixes; rather they should rely on the -I flags to pull in the UAPI header if the KAPI header of the same name does not exist. However, KAPI headers that shadow UAPI headers (linux/fs.h for example) _must_ manually #include the UAPI header with the uapi/ prefix as there's no other way to reach it. I originally used #include_next for this, but some people refused to countenance it because it's a gcc-ism, but no matter. After doing make headers_install in the kernel, you should find the contents of the appropriate uapi/ directories installed in the target directory without any uapi/ directories present. Further, the _UAPI prefixes on the guards are removed if present. There should be no #includes with "uapi/" in their filenames. David
Alexander Graf <agraf@suse.de> wrote: > Could you please try and see if this kernel side patch makes things work for you too? > > > Alex > > > diff --git a/arch/powerpc/include/uapi/asm/kvm_para.h b/arch/powerpc/include/uapi/asm/kvm_para.h > index ed0e025..e3af328 100644 > --- a/arch/powerpc/include/uapi/asm/kvm_para.h > +++ b/arch/powerpc/include/uapi/asm/kvm_para.h > @@ -78,7 +78,7 @@ struct kvm_vcpu_arch_shared { > > #define KVM_HCALL_TOKEN(num) _EV_HCALL_TOKEN(EV_KVM_VENDOR_ID, num) > > -#include <uapi/asm/epapr_hcalls.h> > +#include <asm/epapr_hcalls.h> > > #define KVM_FEATURE_MAGIC_PAGE 1 This appears to be correct. Acked-by: David Howells <dhowells@redhat.com>
diff --git a/arch/powerpc/include/uapi/asm/kvm_para.h b/arch/powerpc/include/uapi/asm/kvm_para.h index ed0e025..e3af328 100644 --- a/arch/powerpc/include/uapi/asm/kvm_para.h +++ b/arch/powerpc/include/uapi/asm/kvm_para.h @@ -78,7 +78,7 @@ struct kvm_vcpu_arch_shared { #define KVM_HCALL_TOKEN(num) _EV_HCALL_TOKEN(EV_KVM_VENDOR_ID, num) -#include <uapi/asm/epapr_hcalls.h> +#include <asm/epapr_hcalls.h> #define KVM_FEATURE_MAGIC_PAGE 1