diff mbox series

[Xen-devel,01/27,v10] xen/arm: vpl011: Define common ring buffer helper functions in console.h

Message ID 1506068606-17066-2-git-send-email-bhupinder.thakur@linaro.org
State New
Headers show
Series SBSA UART emulation support in Xen | expand

Commit Message

Bhupinder Thakur Sept. 22, 2017, 8:23 a.m. UTC 
DEFINE_XEN_FLEX_RING(xencons) defines common helper functions such as
xencons_queued() to tell the current size of the ring buffer,
xencons_mask() to mask off the index, which are useful helper functions.
pl011 emulation code will use these helper functions.

io/console.h includes io/ring.h which defines DEFINE_XEN_FLEX_RING.

In console/daemon/io.c, string.h had to be included before io/console.h
because ring.h uses string functions.

Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Acked-by: Wei Liu <wei.liu2@citrix.com>
Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
CC: Ian Jackson <ian.jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>

Changes since v4:
- Split this change in a separate patch.

 tools/console/daemon/io.c       | 2 +-
 xen/include/public/io/console.h | 4 ++++
 2 files changed, 5 insertions(+), 1 deletion(-)

Comments

Stefano Stabellini Sept. 22, 2017, 10:44 p.m. UTC | #1 
Adding Jan

On Fri, 22 Sep 2017, Bhupinder Thakur wrote:
> DEFINE_XEN_FLEX_RING(xencons) defines common helper functions such as
> xencons_queued() to tell the current size of the ring buffer,
> xencons_mask() to mask off the index, which are useful helper functions.
> pl011 emulation code will use these helper functions.
> 
> io/console.h includes io/ring.h which defines DEFINE_XEN_FLEX_RING.
> 
> In console/daemon/io.c, string.h had to be included before io/console.h
> because ring.h uses string functions.
> 
> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> Acked-by: Wei Liu <wei.liu2@citrix.com>
> Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Unfortunately this patch breaks the build on x86. The reason is that
DEFINE_XEN_FLEX_RING requires C99, and the current header checks in
xen/include/Makefile use ANSI C.

The only two headers to use DEFINE_XEN_FLEX_RING so far are pvcalls and
9pfs that are both explicitly marked as c99 in xen/include/Makefile, see
PUBLIC_C99_HEADERS.

One way to solve this problem would be to mark console.h as one of the
c99 headers, but I am guessing that Jan will want to keep it ANSI C.

In that case, we could make DEFINE_XEN_FLEX_RING ANSI C, which is ugly
but possible. It requires turning the static inline functions in ring.h
into macros.

Otherwise, we could take DEFINE_XEN_FLEX_RING(xencons) out of
io/console.h. We could move it to a new header file, and the new header
file could be C99.

Jan, do you have other suggestions?


> ---
> CC: Ian Jackson <ian.jackson@eu.citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien.grall@arm.com>
> 
> Changes since v4:
> - Split this change in a separate patch.
> 
>  tools/console/daemon/io.c       | 2 +-
>  xen/include/public/io/console.h | 4 ++++
>  2 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/console/daemon/io.c b/tools/console/daemon/io.c
> index 7e474bb..e8033d2 100644
> --- a/tools/console/daemon/io.c
> +++ b/tools/console/daemon/io.c
> @@ -21,6 +21,7 @@
>  
>  #include "utils.h"
>  #include "io.h"
> +#include <string.h>
>  #include <xenevtchn.h>
>  #include <xengnttab.h>
>  #include <xenstore.h>
> @@ -29,7 +30,6 @@
>  
>  #include <stdlib.h>
>  #include <errno.h>
> -#include <string.h>
>  #include <poll.h>
>  #include <fcntl.h>
>  #include <unistd.h>
> diff --git a/xen/include/public/io/console.h b/xen/include/public/io/console.h
> index e2cd97f..5e45e1c 100644
> --- a/xen/include/public/io/console.h
> +++ b/xen/include/public/io/console.h
> @@ -27,6 +27,8 @@
>  #ifndef __XEN_PUBLIC_IO_CONSOLE_H__
>  #define __XEN_PUBLIC_IO_CONSOLE_H__
>  
> +#include "ring.h"
> +
>  typedef uint32_t XENCONS_RING_IDX;
>  
>  #define MASK_XENCONS_IDX(idx, ring) ((idx) & (sizeof(ring)-1))
> @@ -38,6 +40,8 @@ struct xencons_interface {
>      XENCONS_RING_IDX out_cons, out_prod;
>  };
>  
> +DEFINE_XEN_FLEX_RING(xencons);
> +
>  #endif /* __XEN_PUBLIC_IO_CONSOLE_H__ */
>  
>  /*
> -- 
> 2.7.4
>
Julien Grall Sept. 23, 2017, 1:25 p.m. UTC | #2 
Hi,

On 09/22/2017 11:44 PM, Stefano Stabellini wrote:
> Adding Jan
> 
> On Fri, 22 Sep 2017, Bhupinder Thakur wrote:
>> DEFINE_XEN_FLEX_RING(xencons) defines common helper functions such as
>> xencons_queued() to tell the current size of the ring buffer,
>> xencons_mask() to mask off the index, which are useful helper functions.
>> pl011 emulation code will use these helper functions.
>>
>> io/console.h includes io/ring.h which defines DEFINE_XEN_FLEX_RING.
>>
>> In console/daemon/io.c, string.h had to be included before io/console.h
>> because ring.h uses string functions.
>>
>> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
>> Acked-by: Wei Liu <wei.liu2@citrix.com>
>> Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
> Unfortunately this patch breaks the build on x86.

I am a bit surprised that this breaks only build on x86. I was expecting 
the header checks to be done on all the architecture ... hmmm... looking 
at the Makefile, the checks is only done when building natively. I guess 
you are cross-compiling?

It might be interesting to look at getting headers check in 
cross-compile environment given that this is the main way to build the 
hypervisor today.

> The reason is that
> DEFINE_XEN_FLEX_RING requires C99, and the current header checks in
> xen/include/Makefile use ANSI C.

I was not able to spot why DEFINE_XEN_FLEX_RING would require C99. Can 
you detail it?

Cheers,
Bhupinder Thakur Sept. 23, 2017, 3:09 p.m. UTC | #3 
Hi,

On 23 September 2017 at 18:55, Julien Grall <julien.grall@arm.com> wrote:
> Hi,
>
> On 09/22/2017 11:44 PM, Stefano Stabellini wrote:
>>
>> Adding Jan
>>
>> On Fri, 22 Sep 2017, Bhupinder Thakur wrote:
>>>
>>> DEFINE_XEN_FLEX_RING(xencons) defines common helper functions such as
>>> xencons_queued() to tell the current size of the ring buffer,
>>> xencons_mask() to mask off the index, which are useful helper functions.
>>> pl011 emulation code will use these helper functions.
>>>
>>> io/console.h includes io/ring.h which defines DEFINE_XEN_FLEX_RING.
>>>
>>> In console/daemon/io.c, string.h had to be included before io/console.h
>>> because ring.h uses string functions.
>>>
>>> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
>>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
>>> Acked-by: Wei Liu <wei.liu2@citrix.com>
>>> Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>>
>>
>> Unfortunately this patch breaks the build on x86.
>
>
> I am a bit surprised that this breaks only build on x86. I was expecting the
> header checks to be done on all the architecture ... hmmm... looking at the
> Makefile, the checks is only done when building natively. I guess you are
> cross-compiling?

>
> It might be interesting to look at getting headers check in cross-compile
> environment given that this is the main way to build the hypervisor today.
>
Yes I am doing cross-compilation.

>> The reason is that
>> DEFINE_XEN_FLEX_RING requires C99, and the current header checks in
>> xen/include/Makefile use ANSI C.
>
>
> I was not able to spot why DEFINE_XEN_FLEX_RING would require C99. Can you
> detail it?
This macro expands to generate inline functions, which I believe
require C99 to compile.

Regards,
Bhupinder
Jan Beulich Sept. 25, 2017, 7:49 a.m. UTC | #4 
>>> On 23.09.17 at 00:44, <sstabellini@kernel.org> wrote:
> On Fri, 22 Sep 2017, Bhupinder Thakur wrote:
>> DEFINE_XEN_FLEX_RING(xencons) defines common helper functions such as
>> xencons_queued() to tell the current size of the ring buffer,
>> xencons_mask() to mask off the index, which are useful helper functions.
>> pl011 emulation code will use these helper functions.
>> 
>> io/console.h includes io/ring.h which defines DEFINE_XEN_FLEX_RING.
>> 
>> In console/daemon/io.c, string.h had to be included before io/console.h
>> because ring.h uses string functions.
>> 
>> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
>> Acked-by: Wei Liu <wei.liu2@citrix.com>
>> Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
> Unfortunately this patch breaks the build on x86. The reason is that
> DEFINE_XEN_FLEX_RING requires C99, and the current header checks in
> xen/include/Makefile use ANSI C.
> 
> The only two headers to use DEFINE_XEN_FLEX_RING so far are pvcalls and
> 9pfs that are both explicitly marked as c99 in xen/include/Makefile, see
> PUBLIC_C99_HEADERS.
> 
> One way to solve this problem would be to mark console.h as one of the
> c99 headers, but I am guessing that Jan will want to keep it ANSI C.
> 
> In that case, we could make DEFINE_XEN_FLEX_RING ANSI C, which is ugly
> but possible. It requires turning the static inline functions in ring.h
> into macros.
> 
> Otherwise, we could take DEFINE_XEN_FLEX_RING(xencons) out of
> io/console.h. We could move it to a new header file, and the new header
> file could be C99.
> 
> Jan, do you have other suggestions?

Moving the C99 requiring parts to a separate header is certainly
a reasonable option. Another might be to frame the additions by
a __STRICT_ANSI__ and/or __GNUC__ and/or __STDC_VERSION__
check (we have some examples elsewhere, but they may not be
exact fits for the case here).

Jan
Jan Beulich Sept. 25, 2017, 7:51 a.m. UTC | #5 
>>> On 23.09.17 at 17:09, <bhupinder.thakur@linaro.org> wrote:
> On 23 September 2017 at 18:55, Julien Grall <julien.grall@arm.com> wrote:
>> I was not able to spot why DEFINE_XEN_FLEX_RING would require C99. Can you
>> detail it?
> This macro expands to generate inline functions, which I believe
> require C99 to compile.

Plus there's at least one variable size array member of a structure.

Jan
Bhupinder Thakur Sept. 25, 2017, 11:08 p.m. UTC | #6 
Hi Jan,

Yes, after including the __STRICT_ANSI__ check the headers.chk check
passes. But I had to include string header file (after suggestion from Stefano)
for fixing the headers++.chk.

I have pasted the changes below:

diff --git a/xen/include/Makefile b/xen/include/Makefile
index 1299b19..c90fdee 100644
--- a/xen/include/Makefile
+++ b/xen/include/Makefile
@@ -98,6 +98,7 @@ PUBLIC_C99_HEADERS := public/io/9pfs.h public/io/pvcalls.h
 PUBLIC_ANSI_HEADERS := $(filter-out public/%ctl.h public/xsm/%
public/%hvm/save.h $(PUBLIC_C99_HEADERS), $(PUBLIC_HEADERS))

 public/io/9pfs.h-prereq := string
+public/io/console.h-prereq := string
 public/io/pvcalls.h-prereq := string

 headers.chk: $(PUBLIC_ANSI_HEADERS) Makefile
diff --git a/xen/include/public/io/console.h b/xen/include/public/io/console.h
index 5e45e1c..0f0711f 100644
--- a/xen/include/public/io/console.h
+++ b/xen/include/public/io/console.h
@@ -40,7 +40,9 @@ struct xencons_interface {
     XENCONS_RING_IDX out_cons, out_prod;
 };

+#if defined(__GNUC__) && !defined(__STRICT_ANSI__)
 DEFINE_XEN_FLEX_RING(xencons);
+#endif

 #endif /* __XEN_PUBLIC_IO_CONSOLE_H__ */

Regards,
Bhupinder

On 25 September 2017 at 08:49, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 23.09.17 at 00:44, <sstabellini@kernel.org> wrote:
>> On Fri, 22 Sep 2017, Bhupinder Thakur wrote:
>>> DEFINE_XEN_FLEX_RING(xencons) defines common helper functions such as
>>> xencons_queued() to tell the current size of the ring buffer,
>>> xencons_mask() to mask off the index, which are useful helper functions.
>>> pl011 emulation code will use these helper functions.
>>>
>>> io/console.h includes io/ring.h which defines DEFINE_XEN_FLEX_RING.
>>>
>>> In console/daemon/io.c, string.h had to be included before io/console.h
>>> because ring.h uses string functions.
>>>
>>> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
>>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
>>> Acked-by: Wei Liu <wei.liu2@citrix.com>
>>> Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>>
>> Unfortunately this patch breaks the build on x86. The reason is that
>> DEFINE_XEN_FLEX_RING requires C99, and the current header checks in
>> xen/include/Makefile use ANSI C.
>>
>> The only two headers to use DEFINE_XEN_FLEX_RING so far are pvcalls and
>> 9pfs that are both explicitly marked as c99 in xen/include/Makefile, see
>> PUBLIC_C99_HEADERS.
>>
>> One way to solve this problem would be to mark console.h as one of the
>> c99 headers, but I am guessing that Jan will want to keep it ANSI C.
>>
>> In that case, we could make DEFINE_XEN_FLEX_RING ANSI C, which is ugly
>> but possible. It requires turning the static inline functions in ring.h
>> into macros.
>>
>> Otherwise, we could take DEFINE_XEN_FLEX_RING(xencons) out of
>> io/console.h. We could move it to a new header file, and the new header
>> file could be C99.
>>
>> Jan, do you have other suggestions?
>
> Moving the C99 requiring parts to a separate header is certainly
> a reasonable option. Another might be to frame the additions by
> a __STRICT_ANSI__ and/or __GNUC__ and/or __STDC_VERSION__
> check (we have some examples elsewhere, but they may not be
> exact fits for the case here).
>
> Jan
>
Jan Beulich Sept. 26, 2017, 7:15 a.m. UTC | #7 
>>> On 26.09.17 at 01:08, <bhupinder.thakur@linaro.org> wrote:
> Yes, after including the __STRICT_ANSI__ check the headers.chk check
> passes. But I had to include string header file (after suggestion from 
> Stefano) for fixing the headers++.chk.

I'd like to have a more detailed explanation here - since the header
passed the check without this prereq before, I'd prefer if the
dependency was not added unconditionally.

> --- a/xen/include/public/io/console.h
> +++ b/xen/include/public/io/console.h
> @@ -40,7 +40,9 @@ struct xencons_interface {
>      XENCONS_RING_IDX out_cons, out_prod;
>  };
> 
> +#if defined(__GNUC__) && !defined(__STRICT_ANSI__)
>  DEFINE_XEN_FLEX_RING(xencons);
> +#endif

At the first glance it also looks as if the scope of this conditional
was too narrow.

Jan
Bhupinder Thakur Sept. 26, 2017, 8:16 a.m. UTC | #8 
Hi Jan,

On 26 September 2017 at 12:45, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 26.09.17 at 01:08, <bhupinder.thakur@linaro.org> wrote:
>> Yes, after including the __STRICT_ANSI__ check the headers.chk check
>> passes. But I had to include string header file (after suggestion from
>> Stefano) for fixing the headers++.chk.
>
> I'd like to have a more detailed explanation here - since the header
> passed the check without this prereq before, I'd prefer if the
> dependency was not added unconditionally.

The C header passed the check without the prereq addition. However,
for C++ headers since
__STRICT_ANSI__ is not defined, it tries to expand the
DEFINE_XEN_FLEX_RING macro
and looks for declarations for size_t, memcpy() etc. To satisfy that
requirement, string header
file had to included similar to what was done for pvcalls.

>
>> --- a/xen/include/public/io/console.h
>> +++ b/xen/include/public/io/console.h
>> @@ -40,7 +40,9 @@ struct xencons_interface {
>>      XENCONS_RING_IDX out_cons, out_prod;
>>  };
>>
>> +#if defined(__GNUC__) && !defined(__STRICT_ANSI__)
>>  DEFINE_XEN_FLEX_RING(xencons);
>> +#endif
>
> At the first glance it also looks as if the scope of this conditional
> was too narrow.

Do you mean that xencons_interface should also be under ifdef?

Regards,
Bhupinder
Jan Beulich Sept. 26, 2017, 11:41 a.m. UTC | #9 
>>> On 26.09.17 at 10:16, <bhupinder.thakur@linaro.org> wrote:
> On 26 September 2017 at 12:45, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 26.09.17 at 01:08, <bhupinder.thakur@linaro.org> wrote:
>>> Yes, after including the __STRICT_ANSI__ check the headers.chk check
>>> passes. But I had to include string header file (after suggestion from
>>> Stefano) for fixing the headers++.chk.
>>
>> I'd like to have a more detailed explanation here - since the header
>> passed the check without this prereq before, I'd prefer if the
>> dependency was not added unconditionally.
> 
> The C header passed the check without the prereq addition. However,
> for C++ headers since
> __STRICT_ANSI__ is not defined, it tries to expand the
> DEFINE_XEN_FLEX_RING macro
> and looks for declarations for size_t, memcpy() etc. To satisfy that
> requirement, string header
> file had to included similar to what was done for pvcalls.

Ah, yes. This should equally apply to the C99 check then.

Jan
diff mbox series

Patch

diff --git a/tools/console/daemon/io.c b/tools/console/daemon/io.c
index 7e474bb..e8033d2 100644
--- a/tools/console/daemon/io.c
+++ b/tools/console/daemon/io.c
@@ -21,6 +21,7 @@ 
 
 #include "utils.h"
 #include "io.h"
+#include <string.h>
 #include <xenevtchn.h>
 #include <xengnttab.h>
 #include <xenstore.h>
@@ -29,7 +30,6 @@ 
 
 #include <stdlib.h>
 #include <errno.h>
-#include <string.h>
 #include <poll.h>
 #include <fcntl.h>
 #include <unistd.h>
diff --git a/xen/include/public/io/console.h b/xen/include/public/io/console.h
index e2cd97f..5e45e1c 100644
--- a/xen/include/public/io/console.h
+++ b/xen/include/public/io/console.h
@@ -27,6 +27,8 @@ 
 #ifndef __XEN_PUBLIC_IO_CONSOLE_H__
 #define __XEN_PUBLIC_IO_CONSOLE_H__
 
+#include "ring.h"
+
 typedef uint32_t XENCONS_RING_IDX;
 
 #define MASK_XENCONS_IDX(idx, ring) ((idx) & (sizeof(ring)-1))
@@ -38,6 +40,8 @@  struct xencons_interface {
     XENCONS_RING_IDX out_cons, out_prod;
 };
 
+DEFINE_XEN_FLEX_RING(xencons);
+
 #endif /* __XEN_PUBLIC_IO_CONSOLE_H__ */
 
 /*