xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
* [PATCH for-4.15 0/3] tools/libs: Multiple fixes to header handling
@ 2021-03-05 12:49 Andrew Cooper
  2021-03-05 12:49 ` [PATCH 1/3] tools/libxentoolcore: Fill in LIBHEADERS Andrew Cooper
                   ` (3 more replies)
  0 siblings, 4 replies; 18+ messages in thread
From: Andrew Cooper @ 2021-03-05 12:49 UTC (permalink / raw)
  To: Xen-devel
  Cc: Andrew Cooper, Jan Beulich, Roger Pau Monné,
	Wei Liu, Juergen Gross, Ian Jackson

This can of worms is festering.  See patch 1 for yet more issues.

Andrew Cooper (3):
  tools/libxentoolcore: Fill in LIBHEADERS
  xen/dmop: Strip __XEN_TOOLS__ header guard from public API
  tools/libs: Fix headers.chk logic

 tools/libs/devicemodel/private.h | 2 --
 tools/libs/libs.mk               | 2 +-
 tools/libs/toolcore/Makefile     | 2 ++
 xen/include/public/hvm/dm_op.h   | 5 -----
 4 files changed, 3 insertions(+), 8 deletions(-)

-- 
2.11.0



^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH 1/3] tools/libxentoolcore: Fill in LIBHEADERS
  2021-03-05 12:49 [PATCH for-4.15 0/3] tools/libs: Multiple fixes to header handling Andrew Cooper
@ 2021-03-05 12:49 ` Andrew Cooper
  2021-03-05 13:25   ` Ian Jackson
  2021-03-11 13:37   ` Jürgen Groß
  2021-03-05 12:49 ` [PATCH 2/3] xen/dmop: Strip __XEN_TOOLS__ header guard from public API Andrew Cooper
                   ` (2 subsequent siblings)
  3 siblings, 2 replies; 18+ messages in thread
From: Andrew Cooper @ 2021-03-05 12:49 UTC (permalink / raw)
  To: Xen-devel; +Cc: Andrew Cooper, Ian Jackson, Wei Liu, Juergen Gross

c/s 4664034cd replaced a glob over include/*.h with an expectation that
LIBHEADER was suitably set for libraries which didn't have a single,
consistently named, header file.

This wasn't true for xentoolcore, which lost xentoolcore_internal.h as a
consequence, and failed an API/ABI check vs 4.14

Fixes: 4664034cd ("tools/libs: move official headers to common directory")
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Ian Jackson <iwj@xenproject.org>
CC: Wei Liu <wl@xen.org>
CC: Juergen Gross <jgross@suse.com>

For 4.15.  This is a regression from 4.14, even if AFAICT it only impacts the
ABI checking at this point.

I *think* this is the only impacted library, but I would appreciate a second
pair of eyes.

I did experiment with this:

  +ifeq ($(LIBHEADER),)
  +$(warning $(LIBNAME) - No headers)
  +endif
   LIBHEADER ?= $(LIB_FILE_NAME).h
   LIBHEADERS = $(foreach h, $(LIBHEADER), $(XEN_INCLUDE)/$(h))

which yields:

  andrewcoop@andrewcoop:/local/xen.git$ make -j4 -s -C tools/libs/
  /local/xen.git/tools/libs/toollog/../../../tools/libs/libs.mk:50: toollog - No headers
  /local/xen.git/tools/libs/evtchn/../../../tools/libs/libs.mk:50: evtchn - No headers
  /local/xen.git/tools/libs/gnttab/../../../tools/libs/libs.mk:50: gnttab - No headers
  /local/xen.git/tools/libs/call/../../../tools/libs/libs.mk:50: call - No headers
  /local/xen.git/tools/libs/foreignmemory/../../../tools/libs/libs.mk:50: foreignmemory - No headers
  /local/xen.git/tools/libs/devicemodel/../../../tools/libs/libs.mk:50: devicemodel - No headers
  ../libs.mk:50: hypfs - No headers
  /local/xen.git/tools/libs/stat/../../../tools/libs/libs.mk:50: stat - No headers
  /local/xen.git/tools/libs/stat/../../../tools/libs/libs.mk:50: stat - No headers

Headers aside for a moment, there are two bugs here.  hypfs doesn't use the
same include pattern as the others, and stat is entered twice.
---
 tools/libs/toolcore/Makefile | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/libs/toolcore/Makefile b/tools/libs/toolcore/Makefile
index 1cf30733c9..3550786491 100644
--- a/tools/libs/toolcore/Makefile
+++ b/tools/libs/toolcore/Makefile
@@ -5,6 +5,8 @@ MAJOR	= 1
 MINOR	= 0
 AUTOINCS := $(XEN_INCLUDE)/_xentoolcore_list.h
 
+LIBHEADER := xentoolcore.h xentoolcore_internal.h
+
 SRCS-y	+= handlereg.c
 
 include $(XEN_ROOT)/tools/libs/libs.mk
-- 
2.11.0



^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH 2/3] xen/dmop: Strip __XEN_TOOLS__ header guard from public API
  2021-03-05 12:49 [PATCH for-4.15 0/3] tools/libs: Multiple fixes to header handling Andrew Cooper
  2021-03-05 12:49 ` [PATCH 1/3] tools/libxentoolcore: Fill in LIBHEADERS Andrew Cooper
@ 2021-03-05 12:49 ` Andrew Cooper
  2021-03-05 13:26   ` Ian Jackson
  2021-03-05 13:53   ` Jan Beulich
  2021-03-05 12:49 ` [PATCH 3/3] tools/libs: Fix headers.chk logic Andrew Cooper
  2021-03-11 13:28 ` Ping [PATCH for-4.15 0/3] tools/libs: Multiple fixes to header handling Andrew Cooper
  3 siblings, 2 replies; 18+ messages in thread
From: Andrew Cooper @ 2021-03-05 12:49 UTC (permalink / raw)
  To: Xen-devel
  Cc: Andrew Cooper, Jan Beulich, Roger Pau Monné,
	Wei Liu, Paul Durrant, Ian Jackson

Exactly as with c/s f40e1c52e4, this is inappropriate for a stable library.

That change actually broke the build with:

  include/xendevicemodel.h:52:5: error: unknown type name 'ioservid_t'
       ioservid_t *id);
       ^

as libxendevicemodel.h now uses a type it can't see a typedef for.  However,
nothing noticed because the header.chk logic is also broken (fixed
subsequently).

Strip the guard from the public header, and remove compensation from
devicemodel's private.h

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Paul Durrant <paul@xen.org>
CC: Ian Jackson <iwj@xenproject.org>

For 4.15.  This is a build fix, even if current staging can't spot the
breakage.

These two issues highlight that libxendevcemodel.h has never been checked
since its introduction, because the checking logic only saw an empty file.
---
 tools/libs/devicemodel/private.h | 2 --
 xen/include/public/hvm/dm_op.h   | 5 -----
 2 files changed, 7 deletions(-)

diff --git a/tools/libs/devicemodel/private.h b/tools/libs/devicemodel/private.h
index c4a225f8af..c24f3396bb 100644
--- a/tools/libs/devicemodel/private.h
+++ b/tools/libs/devicemodel/private.h
@@ -1,8 +1,6 @@
 #ifndef XENDEVICEMODEL_PRIVATE_H
 #define XENDEVICEMODEL_PRIVATE_H
 
-#define __XEN_TOOLS__ 1
-
 #include <xentoollog.h>
 #include <xendevicemodel.h>
 #include <xencall.h>
diff --git a/xen/include/public/hvm/dm_op.h b/xen/include/public/hvm/dm_op.h
index ef7fbc0d3d..fa3f083fed 100644
--- a/xen/include/public/hvm/dm_op.h
+++ b/xen/include/public/hvm/dm_op.h
@@ -25,9 +25,6 @@
 #define __XEN_PUBLIC_HVM_DM_OP_H__
 
 #include "../xen.h"
-
-#if defined(__XEN__) || defined(__XEN_TOOLS__)
-
 #include "../event_channel.h"
 
 #ifndef uint64_aligned_t
@@ -491,8 +488,6 @@ struct xen_dm_op {
     } u;
 };
 
-#endif /* __XEN__ || __XEN_TOOLS__ */
-
 struct xen_dm_op_buf {
     XEN_GUEST_HANDLE(void) h;
     xen_ulong_t size;
-- 
2.11.0



^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH 3/3] tools/libs: Fix headers.chk logic
  2021-03-05 12:49 [PATCH for-4.15 0/3] tools/libs: Multiple fixes to header handling Andrew Cooper
  2021-03-05 12:49 ` [PATCH 1/3] tools/libxentoolcore: Fill in LIBHEADERS Andrew Cooper
  2021-03-05 12:49 ` [PATCH 2/3] xen/dmop: Strip __XEN_TOOLS__ header guard from public API Andrew Cooper
@ 2021-03-05 12:49 ` Andrew Cooper
  2021-03-05 13:38   ` Andrew Cooper
  2021-03-11 13:38   ` Jürgen Groß
  2021-03-11 13:28 ` Ping [PATCH for-4.15 0/3] tools/libs: Multiple fixes to header handling Andrew Cooper
  3 siblings, 2 replies; 18+ messages in thread
From: Andrew Cooper @ 2021-03-05 12:49 UTC (permalink / raw)
  To: Xen-devel; +Cc: Andrew Cooper, Juergen Gross, Roger Pau Monné, Wei Liu

c/s 4664034cd dropped the $(LIBHEADERSGLOB) dependency for the headers.chk
rule, without replacing it.

As headers.chk uses $^, a typical build looks like:

  andrewcoop@andrewcoop:/local/xen.git$ make -C tools/libs/devicemodel/
  make: Entering directory '/local/xen.git/tools/libs/devicemodel'
  for i in ; do \
      gcc -x c -ansi -Wall -Werror
      -I/local/xen.git/tools/libs/devicemodel/../../../tools/include \
            -S -o /dev/null $i || exit 1; \
      echo $i; \
  done >headers.chk.new
  mv headers.chk.new headers.chk

i.e. with an empty for loop, and checking only the $(AUTOINCS).

Reinsert a $(LIBHEADERS) dependency.

Fixes: 4664034cd ("tools/libs: move official headers to common directory")
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Juergen Gross <jgross@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
---
 tools/libs/libs.mk | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/libs/libs.mk b/tools/libs/libs.mk
index a68cec244c..2d973ccb95 100644
--- a/tools/libs/libs.mk
+++ b/tools/libs/libs.mk
@@ -76,7 +76,7 @@ else
 .PHONY: headers.chk
 endif
 
-headers.chk: $(AUTOINCS)
+headers.chk: $(LIBHEADERS) $(AUTOINCS)
 
 headers.lst: FORCE
 	@{ set -e; $(foreach h,$(LIBHEADERS),echo $(h);) } > $@.tmp
-- 
2.11.0



^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH 1/3] tools/libxentoolcore: Fill in LIBHEADERS
  2021-03-05 12:49 ` [PATCH 1/3] tools/libxentoolcore: Fill in LIBHEADERS Andrew Cooper
@ 2021-03-05 13:25   ` Ian Jackson
  2021-03-11 13:37   ` Jürgen Groß
  1 sibling, 0 replies; 18+ messages in thread
From: Ian Jackson @ 2021-03-05 13:25 UTC (permalink / raw)
  To: Andrew Cooper; +Cc: Xen-devel, Wei Liu, Juergen Gross

Andrew Cooper writes ("[PATCH 1/3] tools/libxentoolcore: Fill in LIBHEADERS"):
> c/s 4664034cd replaced a glob over include/*.h with an expectation that
> LIBHEADER was suitably set for libraries which didn't have a single,
> consistently named, header file.
> 
> This wasn't true for xentoolcore, which lost xentoolcore_internal.h as a
> consequence, and failed an API/ABI check vs 4.14
> 
> Fixes: 4664034cd ("tools/libs: move official headers to common directory")
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Release-Acked-by: Ian Jackson <iwj@xenproject.org>


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/3] xen/dmop: Strip __XEN_TOOLS__ header guard from public API
  2021-03-05 12:49 ` [PATCH 2/3] xen/dmop: Strip __XEN_TOOLS__ header guard from public API Andrew Cooper
@ 2021-03-05 13:26   ` Ian Jackson
  2021-03-05 13:53   ` Jan Beulich
  1 sibling, 0 replies; 18+ messages in thread
From: Ian Jackson @ 2021-03-05 13:26 UTC (permalink / raw)
  To: Andrew Cooper
  Cc: Xen-devel, Jan Beulich, Roger Pau Monné, Wei Liu, Paul Durrant

Andrew Cooper writes ("[PATCH 2/3] xen/dmop: Strip __XEN_TOOLS__ header guard from public API"):
> Exactly as with c/s f40e1c52e4, this is inappropriate for a stable library.
> 
> That change actually broke the build with:
> 
>   include/xendevicemodel.h:52:5: error: unknown type name 'ioservid_t'
>        ioservid_t *id);
>        ^
> 
> as libxendevicemodel.h now uses a type it can't see a typedef for.  However,
> nothing noticed because the header.chk logic is also broken (fixed
> subsequently).
> 
> Strip the guard from the public header, and remove compensation from
> devicemodel's private.h

Release-Acked-by: Ian Jackson <iwj@xenproject.org>


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 3/3] tools/libs: Fix headers.chk logic
  2021-03-05 12:49 ` [PATCH 3/3] tools/libs: Fix headers.chk logic Andrew Cooper
@ 2021-03-05 13:38   ` Andrew Cooper
  2021-03-05 13:43     ` Ian Jackson
  2021-03-11 13:38   ` Jürgen Groß
  1 sibling, 1 reply; 18+ messages in thread
From: Andrew Cooper @ 2021-03-05 13:38 UTC (permalink / raw)
  To: Xen-devel; +Cc: Juergen Gross, Roger Pau Monné, Wei Liu, Ian Jackson

On 05/03/2021 12:49, Andrew Cooper wrote:
> c/s 4664034cd dropped the $(LIBHEADERSGLOB) dependency for the headers.chk
> rule, without replacing it.
>
> As headers.chk uses $^, a typical build looks like:
>
>   andrewcoop@andrewcoop:/local/xen.git$ make -C tools/libs/devicemodel/
>   make: Entering directory '/local/xen.git/tools/libs/devicemodel'
>   for i in ; do \
>       gcc -x c -ansi -Wall -Werror
>       -I/local/xen.git/tools/libs/devicemodel/../../../tools/include \
>             -S -o /dev/null $i || exit 1; \
>       echo $i; \
>   done >headers.chk.new
>   mv headers.chk.new headers.chk
>
> i.e. with an empty for loop, and checking only the $(AUTOINCS).
>
> Reinsert a $(LIBHEADERS) dependency.
>
> Fixes: 4664034cd ("tools/libs: move official headers to common directory")
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Juergen Gross <jgross@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Wei Liu <wl@xen.org>

Apologies - I totally messed up the CC list here.

For 4.15.  Regression from 4.14, in some build-time checking logic.

~Andrew

> ---
>  tools/libs/libs.mk | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/libs/libs.mk b/tools/libs/libs.mk
> index a68cec244c..2d973ccb95 100644
> --- a/tools/libs/libs.mk
> +++ b/tools/libs/libs.mk
> @@ -76,7 +76,7 @@ else
>  .PHONY: headers.chk
>  endif
>  
> -headers.chk: $(AUTOINCS)
> +headers.chk: $(LIBHEADERS) $(AUTOINCS)
>  
>  headers.lst: FORCE
>  	@{ set -e; $(foreach h,$(LIBHEADERS),echo $(h);) } > $@.tmp



^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 3/3] tools/libs: Fix headers.chk logic
  2021-03-05 13:38   ` Andrew Cooper
@ 2021-03-05 13:43     ` Ian Jackson
  0 siblings, 0 replies; 18+ messages in thread
From: Ian Jackson @ 2021-03-05 13:43 UTC (permalink / raw)
  To: Andrew Cooper; +Cc: Xen-devel, Juergen Gross, Roger Pau Monné, Wei Liu

Andrew Cooper writes ("Re: [PATCH 3/3] tools/libs: Fix headers.chk logic"):
> On 05/03/2021 12:49, Andrew Cooper wrote:
> > c/s 4664034cd dropped the $(LIBHEADERSGLOB) dependency for the headers.chk
> > rule, without replacing it.
> >
> > As headers.chk uses $^, a typical build looks like:
> >
> >   andrewcoop@andrewcoop:/local/xen.git$ make -C tools/libs/devicemodel/
> >   make: Entering directory '/local/xen.git/tools/libs/devicemodel'
> >   for i in ; do \
> >       gcc -x c -ansi -Wall -Werror
> >       -I/local/xen.git/tools/libs/devicemodel/../../../tools/include \
> >             -S -o /dev/null $i || exit 1; \
> >       echo $i; \
> >   done >headers.chk.new
> >   mv headers.chk.new headers.chk
> >
> > i.e. with an empty for loop, and checking only the $(AUTOINCS).
> >
> > Reinsert a $(LIBHEADERS) dependency.

Release-Acked-by: Ian Jackson <iwj@xenproject.org>


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/3] xen/dmop: Strip __XEN_TOOLS__ header guard from public API
  2021-03-05 12:49 ` [PATCH 2/3] xen/dmop: Strip __XEN_TOOLS__ header guard from public API Andrew Cooper
  2021-03-05 13:26   ` Ian Jackson
@ 2021-03-05 13:53   ` Jan Beulich
  2021-03-05 14:12     ` Andrew Cooper
  1 sibling, 1 reply; 18+ messages in thread
From: Jan Beulich @ 2021-03-05 13:53 UTC (permalink / raw)
  To: Andrew Cooper
  Cc: Roger Pau Monné, Wei Liu, Paul Durrant, Ian Jackson, Xen-devel

On 05.03.2021 13:49, Andrew Cooper wrote:
> Exactly as with c/s f40e1c52e4, this is inappropriate for a stable library.
> 
> That change actually broke the build with:
> 
>   include/xendevicemodel.h:52:5: error: unknown type name 'ioservid_t'
>        ioservid_t *id);
>        ^
> 
> as libxendevicemodel.h now uses a type it can't see a typedef for.  However,
> nothing noticed because the header.chk logic is also broken (fixed
> subsequently).

While I agree up to here, ...

> Strip the guard from the public header, and remove compensation from
> devicemodel's private.h

... I'm unconvinced that entirely dropping the guard from the
public header is wanted (or needed): We use these to make clear
that in particular kernels aren't supposed to make use of the
enclosed entities. If a type needs exposing, it (and only it)
wants moving ou of the guarded region imo.

Jan


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/3] xen/dmop: Strip __XEN_TOOLS__ header guard from public API
  2021-03-05 13:53   ` Jan Beulich
@ 2021-03-05 14:12     ` Andrew Cooper
  2021-03-05 14:18       ` Jan Beulich
  0 siblings, 1 reply; 18+ messages in thread
From: Andrew Cooper @ 2021-03-05 14:12 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Roger Pau Monné, Wei Liu, Paul Durrant, Ian Jackson, Xen-devel

On 05/03/2021 13:53, Jan Beulich wrote:
> On 05.03.2021 13:49, Andrew Cooper wrote:
>> Exactly as with c/s f40e1c52e4, this is inappropriate for a stable library.
>>
>> That change actually broke the build with:
>>
>>   include/xendevicemodel.h:52:5: error: unknown type name 'ioservid_t'
>>        ioservid_t *id);
>>        ^
>>
>> as libxendevicemodel.h now uses a type it can't see a typedef for.  However,
>> nothing noticed because the header.chk logic is also broken (fixed
>> subsequently).
> While I agree up to here, ...
>
>> Strip the guard from the public header, and remove compensation from
>> devicemodel's private.h
> ... I'm unconvinced that entirely dropping the guard from the
> public header is wanted (or needed): We use these to make clear
> that in particular kernels aren't supposed to make use of the
> enclosed entities. If a type needs exposing, it (and only it)
> wants moving ou of the guarded region imo.

DMOP was invented specifically so a kernel module (i915, for Intel
gVT-g) was independent of the domctl ABI version.

Improving the life of dom0 userspace was an intended consequence, but
not the driving force behind the change.

Exactly the same is true for stubdoms currently, and I am very serious
about purging unstable interfaces eventually.

~Andrew



^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/3] xen/dmop: Strip __XEN_TOOLS__ header guard from public API
  2021-03-05 14:12     ` Andrew Cooper
@ 2021-03-05 14:18       ` Jan Beulich
  2021-03-05 14:21         ` Jan Beulich
  2021-03-05 14:28         ` Ian Jackson
  0 siblings, 2 replies; 18+ messages in thread
From: Jan Beulich @ 2021-03-05 14:18 UTC (permalink / raw)
  To: Andrew Cooper
  Cc: Roger Pau Monné, Wei Liu, Paul Durrant, Ian Jackson, Xen-devel

On 05.03.2021 15:12, Andrew Cooper wrote:
> On 05/03/2021 13:53, Jan Beulich wrote:
>> On 05.03.2021 13:49, Andrew Cooper wrote:
>>> Exactly as with c/s f40e1c52e4, this is inappropriate for a stable library.
>>>
>>> That change actually broke the build with:
>>>
>>>   include/xendevicemodel.h:52:5: error: unknown type name 'ioservid_t'
>>>        ioservid_t *id);
>>>        ^
>>>
>>> as libxendevicemodel.h now uses a type it can't see a typedef for.  However,
>>> nothing noticed because the header.chk logic is also broken (fixed
>>> subsequently).
>> While I agree up to here, ...
>>
>>> Strip the guard from the public header, and remove compensation from
>>> devicemodel's private.h
>> ... I'm unconvinced that entirely dropping the guard from the
>> public header is wanted (or needed): We use these to make clear
>> that in particular kernels aren't supposed to make use of the
>> enclosed entities. If a type needs exposing, it (and only it)
>> wants moving ou of the guarded region imo.
> 
> DMOP was invented specifically so a kernel module (i915, for Intel
> gVT-g) was independent of the domctl ABI version.
> 
> Improving the life of dom0 userspace was an intended consequence, but
> not the driving force behind the change.

This is news to me - so far it had been my understanding that it
was introduced to have a way for the kernel to audit and hand on
requests to the hypervisor without needing to know all the inner
details. I wasn't even aware a kernel module was using any of
these.

Jan


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/3] xen/dmop: Strip __XEN_TOOLS__ header guard from public API
  2021-03-05 14:18       ` Jan Beulich
@ 2021-03-05 14:21         ` Jan Beulich
  2021-03-05 15:13           ` Andrew Cooper
  2021-03-05 14:28         ` Ian Jackson
  1 sibling, 1 reply; 18+ messages in thread
From: Jan Beulich @ 2021-03-05 14:21 UTC (permalink / raw)
  To: Andrew Cooper
  Cc: Roger Pau Monné, Wei Liu, Paul Durrant, Ian Jackson, Xen-devel

On 05.03.2021 15:18, Jan Beulich wrote:
> On 05.03.2021 15:12, Andrew Cooper wrote:
>> On 05/03/2021 13:53, Jan Beulich wrote:
>>> On 05.03.2021 13:49, Andrew Cooper wrote:
>>>> Exactly as with c/s f40e1c52e4, this is inappropriate for a stable library.
>>>>
>>>> That change actually broke the build with:
>>>>
>>>>   include/xendevicemodel.h:52:5: error: unknown type name 'ioservid_t'
>>>>        ioservid_t *id);
>>>>        ^
>>>>
>>>> as libxendevicemodel.h now uses a type it can't see a typedef for.  However,
>>>> nothing noticed because the header.chk logic is also broken (fixed
>>>> subsequently).
>>> While I agree up to here, ...
>>>
>>>> Strip the guard from the public header, and remove compensation from
>>>> devicemodel's private.h
>>> ... I'm unconvinced that entirely dropping the guard from the
>>> public header is wanted (or needed): We use these to make clear
>>> that in particular kernels aren't supposed to make use of the
>>> enclosed entities. If a type needs exposing, it (and only it)
>>> wants moving ou of the guarded region imo.
>>
>> DMOP was invented specifically so a kernel module (i915, for Intel
>> gVT-g) was independent of the domctl ABI version.
>>
>> Improving the life of dom0 userspace was an intended consequence, but
>> not the driving force behind the change.
> 
> This is news to me - so far it had been my understanding that it
> was introduced to have a way for the kernel to audit and hand on
> requests to the hypervisor without needing to know all the inner
> details. I wasn't even aware a kernel module was using any of
> these.

And indeed, quote from docs/designs/dmop.markdown:

"The aim of DMOP is to prevent a compromised device model from
 compromising domains other than the one it is providing emulation
 for (which is therefore likely already compromised)."

And it goes on discussing only the purpose that I've been aware
of.

Jan


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/3] xen/dmop: Strip __XEN_TOOLS__ header guard from public API
  2021-03-05 14:18       ` Jan Beulich
  2021-03-05 14:21         ` Jan Beulich
@ 2021-03-05 14:28         ` Ian Jackson
  1 sibling, 0 replies; 18+ messages in thread
From: Ian Jackson @ 2021-03-05 14:28 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Andrew Cooper, Roger Pau Monné, Wei Liu, Paul Durrant, Xen-devel

Jan Beulich writes ("Re: [PATCH 2/3] xen/dmop: Strip __XEN_TOOLS__ header guard from public API"):
> This is news to me - so far it had been my understanding that it
> was introduced to have a way for the kernel to audit and hand on
> requests to the hypervisor without needing to know all the inner
> details. I wasn't even aware a kernel module was using any of
> these.

Quite so.

Ian.


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/3] xen/dmop: Strip __XEN_TOOLS__ header guard from public API
  2021-03-05 14:21         ` Jan Beulich
@ 2021-03-05 15:13           ` Andrew Cooper
  2021-03-05 15:20             ` Ian Jackson
  0 siblings, 1 reply; 18+ messages in thread
From: Andrew Cooper @ 2021-03-05 15:13 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Roger Pau Monné, Wei Liu, Paul Durrant, Ian Jackson, Xen-devel

On 05/03/2021 14:21, Jan Beulich wrote:
> On 05.03.2021 15:18, Jan Beulich wrote:
>> On 05.03.2021 15:12, Andrew Cooper wrote:
>>> On 05/03/2021 13:53, Jan Beulich wrote:
>>>> On 05.03.2021 13:49, Andrew Cooper wrote:
>>>>> Exactly as with c/s f40e1c52e4, this is inappropriate for a stable library.
>>>>>
>>>>> That change actually broke the build with:
>>>>>
>>>>>   include/xendevicemodel.h:52:5: error: unknown type name 'ioservid_t'
>>>>>        ioservid_t *id);
>>>>>        ^
>>>>>
>>>>> as libxendevicemodel.h now uses a type it can't see a typedef for.  However,
>>>>> nothing noticed because the header.chk logic is also broken (fixed
>>>>> subsequently).
>>>> While I agree up to here, ...
>>>>
>>>>> Strip the guard from the public header, and remove compensation from
>>>>> devicemodel's private.h
>>>> ... I'm unconvinced that entirely dropping the guard from the
>>>> public header is wanted (or needed): We use these to make clear
>>>> that in particular kernels aren't supposed to make use of the
>>>> enclosed entities. If a type needs exposing, it (and only it)
>>>> wants moving ou of the guarded region imo.
>>> DMOP was invented specifically so a kernel module (i915, for Intel
>>> gVT-g) was independent of the domctl ABI version.
>>>
>>> Improving the life of dom0 userspace was an intended consequence, but
>>> not the driving force behind the change.
>> This is news to me - so far it had been my understanding that it
>> was introduced to have a way for the kernel to audit and hand on
>> requests to the hypervisor without needing to know all the inner
>> details. I wasn't even aware a kernel module was using any of
>> these.
> And indeed, quote from docs/designs/dmop.markdown:
>
> "The aim of DMOP is to prevent a compromised device model from
>  compromising domains other than the one it is providing emulation
>  for (which is therefore likely already compromised)."
>
> And it goes on discussing only the purpose that I've been aware
> of.

The use in the dom0 kernel wasn't kept secret in the slightest.  It was
discussed on at the time, and at dev summits.

But upstream tends to only remember/care about the bits which pertain
directly to upstream, and the design particulars of the DMOP ABI were
specifically for userspace.

~Andrew



^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/3] xen/dmop: Strip __XEN_TOOLS__ header guard from public API
  2021-03-05 15:13           ` Andrew Cooper
@ 2021-03-05 15:20             ` Ian Jackson
  0 siblings, 0 replies; 18+ messages in thread
From: Ian Jackson @ 2021-03-05 15:20 UTC (permalink / raw)
  To: Andrew Cooper
  Cc: Jan Beulich, Roger Pau Monné, Wei Liu, Paul Durrant, Xen-devel

Andrew Cooper writes ("Re: [PATCH 2/3] xen/dmop: Strip __XEN_TOOLS__ header guard from public API"):
> The use in the dom0 kernel wasn't kept secret in the slightest.  It was
> discussed on at the time, and at dev summits.

No-one is accusing anyone of keeping anything secret.

> But upstream tends to only remember/care about the bits which pertain
> directly to upstream,

I would prefer to say that upstream only tends to remember things
which are WRITTEN DOWN.

Ian.


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: Ping [PATCH for-4.15 0/3] tools/libs: Multiple fixes to header handling
  2021-03-05 12:49 [PATCH for-4.15 0/3] tools/libs: Multiple fixes to header handling Andrew Cooper
                   ` (2 preceding siblings ...)
  2021-03-05 12:49 ` [PATCH 3/3] tools/libs: Fix headers.chk logic Andrew Cooper
@ 2021-03-11 13:28 ` Andrew Cooper
  3 siblings, 0 replies; 18+ messages in thread
From: Andrew Cooper @ 2021-03-11 13:28 UTC (permalink / raw)
  To: Xen-devel
  Cc: Jan Beulich, Roger Pau Monné, Wei Liu, Juergen Gross, Ian Jackson

On 05/03/2021 12:49, Andrew Cooper wrote:
> This can of worms is festering.  See patch 1 for yet more issues.
>
> Andrew Cooper (3):
>   tools/libxentoolcore: Fill in LIBHEADERS
>   xen/dmop: Strip __XEN_TOOLS__ header guard from public API
>   tools/libs: Fix headers.chk logic

Ping for acks/otherwise on patches 1 and 3?  (They're already R-A'd)

Given their simplicity, as well as being regression fixes for build-time
checks, I'll commit them tomorrow unless any objections are raised.

~Andrew


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 1/3] tools/libxentoolcore: Fill in LIBHEADERS
  2021-03-05 12:49 ` [PATCH 1/3] tools/libxentoolcore: Fill in LIBHEADERS Andrew Cooper
  2021-03-05 13:25   ` Ian Jackson
@ 2021-03-11 13:37   ` Jürgen Groß
  1 sibling, 0 replies; 18+ messages in thread
From: Jürgen Groß @ 2021-03-11 13:37 UTC (permalink / raw)
  To: Andrew Cooper, Xen-devel; +Cc: Ian Jackson, Wei Liu


[-- Attachment #1.1.1: Type: text/plain, Size: 561 bytes --]

On 05.03.21 13:49, Andrew Cooper wrote:
> c/s 4664034cd replaced a glob over include/*.h with an expectation that
> LIBHEADER was suitably set for libraries which didn't have a single,
> consistently named, header file.
> 
> This wasn't true for xentoolcore, which lost xentoolcore_internal.h as a
> consequence, and failed an API/ABI check vs 4.14
> 
> Fixes: 4664034cd ("tools/libs: move official headers to common directory")
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen


[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 3/3] tools/libs: Fix headers.chk logic
  2021-03-05 12:49 ` [PATCH 3/3] tools/libs: Fix headers.chk logic Andrew Cooper
  2021-03-05 13:38   ` Andrew Cooper
@ 2021-03-11 13:38   ` Jürgen Groß
  1 sibling, 0 replies; 18+ messages in thread
From: Jürgen Groß @ 2021-03-11 13:38 UTC (permalink / raw)
  To: Andrew Cooper, Xen-devel; +Cc: Roger Pau Monné, Wei Liu


[-- Attachment #1.1.1: Type: text/plain, Size: 950 bytes --]

On 05.03.21 13:49, Andrew Cooper wrote:
> c/s 4664034cd dropped the $(LIBHEADERSGLOB) dependency for the headers.chk
> rule, without replacing it.
> 
> As headers.chk uses $^, a typical build looks like:
> 
>    andrewcoop@andrewcoop:/local/xen.git$ make -C tools/libs/devicemodel/
>    make: Entering directory '/local/xen.git/tools/libs/devicemodel'
>    for i in ; do \
>        gcc -x c -ansi -Wall -Werror
>        -I/local/xen.git/tools/libs/devicemodel/../../../tools/include \
>              -S -o /dev/null $i || exit 1; \
>        echo $i; \
>    done >headers.chk.new
>    mv headers.chk.new headers.chk
> 
> i.e. with an empty for loop, and checking only the $(AUTOINCS).
> 
> Reinsert a $(LIBHEADERS) dependency.
> 
> Fixes: 4664034cd ("tools/libs: move official headers to common directory")
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen


[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2021-03-11 13:38 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-05 12:49 [PATCH for-4.15 0/3] tools/libs: Multiple fixes to header handling Andrew Cooper
2021-03-05 12:49 ` [PATCH 1/3] tools/libxentoolcore: Fill in LIBHEADERS Andrew Cooper
2021-03-05 13:25   ` Ian Jackson
2021-03-11 13:37   ` Jürgen Groß
2021-03-05 12:49 ` [PATCH 2/3] xen/dmop: Strip __XEN_TOOLS__ header guard from public API Andrew Cooper
2021-03-05 13:26   ` Ian Jackson
2021-03-05 13:53   ` Jan Beulich
2021-03-05 14:12     ` Andrew Cooper
2021-03-05 14:18       ` Jan Beulich
2021-03-05 14:21         ` Jan Beulich
2021-03-05 15:13           ` Andrew Cooper
2021-03-05 15:20             ` Ian Jackson
2021-03-05 14:28         ` Ian Jackson
2021-03-05 12:49 ` [PATCH 3/3] tools/libs: Fix headers.chk logic Andrew Cooper
2021-03-05 13:38   ` Andrew Cooper
2021-03-05 13:43     ` Ian Jackson
2021-03-11 13:38   ` Jürgen Groß
2021-03-11 13:28 ` Ping [PATCH for-4.15 0/3] tools/libs: Multiple fixes to header handling Andrew Cooper

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).