[v4,1/4] xen/riscv: introduce asm/types.h header file

Commit Message

Oleksii Kurochko Jan. 16, 2023, 2:39 p.m. UTC

Signed-off-by: Oleksii Kurochko <[e-mail]>
---
Changes in V4:
    - Clean up types in <asm/types.h> and remain only necessary.
      The following types was removed as they are defined in <xen/types.h>:
      {__|}{u|s}{8|16|32|64}
---
Changes in V3:
    - Nothing changed
---
Changes in V2:
    - Remove unneeded now types from <asm/types.h>
---
 xen/arch/riscv/include/asm/types.h | 43 ++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)
 create mode 100644 xen/arch/riscv/include/asm/types.h

Comments

Jan Beulich Jan. 16, 2023, 2:59 p.m. UTC | #1

On 16.01.2023 15:39, Oleksii Kurochko wrote:
> Signed-off-by: Oleksii Kurochko <[e-mail]>
> ---
> Changes in V4:
>     - Clean up types in <asm/types.h> and remain only necessary.
>       The following types was removed as they are defined in <xen/types.h>:
>       {__|}{u|s}{8|16|32|64}

For one you still typedef u32 and u64. And imo correctly so, until we
get around to move the definition basic types into xen/types.h. Plus
I can't see how things build for you: xen/types.h expects __{u,s}<N>
to be defined in order to then derive {u,}int<N>_t from them.

> --- /dev/null
> +++ b/xen/arch/riscv/include/asm/types.h
> @@ -0,0 +1,43 @@
> +#ifndef __RISCV_TYPES_H__
> +#define __RISCV_TYPES_H__
> +
> +#ifndef __ASSEMBLY__
> +
> +#if defined(CONFIG_RISCV_32)
> +typedef unsigned long long u64;
> +typedef unsigned int u32;
> +typedef u32 vaddr_t;
> +#define PRIvaddr PRIx32
> +typedef u64 paddr_t;
> +#define INVALID_PADDR (~0ULL)
> +#define PRIpaddr "016llx"
> +typedef u32 register_t;
> +#define PRIregister "x"
> +#elif defined (CONFIG_RISCV_64)
> +typedef unsigned long u64;
> +typedef u64 vaddr_t;
> +#define PRIvaddr PRIx64
> +typedef u64 paddr_t;
> +#define INVALID_PADDR (~0UL)
> +#define PRIpaddr "016lx"
> +typedef u64 register_t;
> +#define PRIregister "lx"
> +#endif

Any chance you could insert blank lines after #if, around #elif, and
before #endif?

> +#if defined(__SIZE_TYPE__)
> +typedef __SIZE_TYPE__ size_t;
> +#else
> +typedef unsigned long size_t;
> +#endif

I'd appreciate if this part was dropped by re-basing on top of my
"include/types: move stddef.h-kind types to common header" [1], to
avoid that (re-based) patch then needing to drop this from here
again. I would have committed this already, if osstest wasn't
completely broken right now.

Jan

[1] https://lists.xen.org/archives/html/xen-devel/2023-01/msg00720.html
(since you would not be able to find a patch of the quoted title,
as in the submission I mistakenly referenced stdlib.h)

Oleksii Kurochko Jan. 17, 2023, 9:29 a.m. UTC | #2

On Mon, 2023-01-16 at 15:59 +0100, Jan Beulich wrote:
> On 16.01.2023 15:39, Oleksii Kurochko wrote:
> > Signed-off-by: Oleksii Kurochko <[e-mail]>
> > ---
> > Changes in V4:
> >     - Clean up types in <asm/types.h> and remain only necessary.
> >       The following types was removed as they are defined in
> > <xen/types.h>:
> >       {__|}{u|s}{8|16|32|64}
> 
> For one you still typedef u32 and u64. And imo correctly so, until we
> get around to move the definition basic types into xen/types.h. Plus
> I can't see how things build for you: xen/types.h expects __{u,s}<N>
It builds because nothing is used <xen/types.h> now so that's why I
missed them but you are right __{u,s}<N> should be backed.
It looks like {__,}{u,s}{8,16,32} are the same for all available in Xen
architectures so probably can I move them to <xen/types.h> instead of
remain them in <asm/types.h>?
> to be defined in order to then derive {u,}int<N>_t from them.
> 
> > --- /dev/null
> > +++ b/xen/arch/riscv/include/asm/types.h
> > @@ -0,0 +1,43 @@
> > +#ifndef __RISCV_TYPES_H__
> > +#define __RISCV_TYPES_H__
> > +
> > +#ifndef __ASSEMBLY__
> > +
> > +#if defined(CONFIG_RISCV_32)
> > +typedef unsigned long long u64;
> > +typedef unsigned int u32;
> > +typedef u32 vaddr_t;
> > +#define PRIvaddr PRIx32
> > +typedef u64 paddr_t;
> > +#define INVALID_PADDR (~0ULL)
> > +#define PRIpaddr "016llx"
> > +typedef u32 register_t;
> > +#define PRIregister "x"
> > +#elif defined (CONFIG_RISCV_64)
> > +typedef unsigned long u64;
> > +typedef u64 vaddr_t;
> > +#define PRIvaddr PRIx64
> > +typedef u64 paddr_t;
> > +#define INVALID_PADDR (~0UL)
> > +#define PRIpaddr "016lx"
> > +typedef u64 register_t;
> > +#define PRIregister "lx"
> > +#endif
> 
> Any chance you could insert blank lines after #if, around #elif, and
> before #endif?
> 
Sure, I will fix that.
> > +#if defined(__SIZE_TYPE__)
> > +typedef __SIZE_TYPE__ size_t;
> > +#else
> > +typedef unsigned long size_t;
> > +#endif
> 
> I'd appreciate if this part was dropped by re-basing on top of my
> "include/types: move stddef.h-kind types to common header" [1], to
> avoid that (re-based) patch then needing to drop this from here
> again. I would have committed this already, if osstest wasn't
> completely broken right now.
> 
I'll take it into account for the next version of the patch series.
> Jan
> 
> [1]
> https://lists.xen.org/archives/html/xen-devel/2023-01/msg00720.html
> (since you would not be able to find a patch of the quoted title,
> as in the submission I mistakenly referenced stdlib.h)
Thanks for the link.

~Oleksii

Jan Beulich Jan. 17, 2023, 10:04 a.m. UTC | #3

On 17.01.2023 10:29, Oleksii wrote:
> On Mon, 2023-01-16 at 15:59 +0100, Jan Beulich wrote:
>> On 16.01.2023 15:39, Oleksii Kurochko wrote:
>>> Signed-off-by: Oleksii Kurochko <[e-mail]>
>>> ---
>>> Changes in V4:
>>>     - Clean up types in <asm/types.h> and remain only necessary.
>>>       The following types was removed as they are defined in
>>> <xen/types.h>:
>>>       {__|}{u|s}{8|16|32|64}
>>
>> For one you still typedef u32 and u64. And imo correctly so, until we
>> get around to move the definition basic types into xen/types.h. Plus
>> I can't see how things build for you: xen/types.h expects __{u,s}<N>
> It builds because nothing is used <xen/types.h> now so that's why I
> missed them but you are right __{u,s}<N> should be backed.
> It looks like {__,}{u,s}{8,16,32} are the same for all available in Xen
> architectures so probably can I move them to <xen/types.h> instead of
> remain them in <asm/types.h>?

This next step isn't quite as obvious, i.e. has room for being
contentious. In particular deriving fixed width types from C basic
types is setting us up for future problems (especially in the
context of RISC-V think of RV128). Therefore, if we touch and
generalize this, I'd like to sanitize things at the same time.

I'd then prefer to typedef {u,}int<N>_t by using either the "mode"
attribute (requiring us to settle on a prereq of there always being
8 bits per char) or the compiler supplied __{U,}INT<N>_TYPE__
(taking gcc 4.7 as a prereq; didn't check clang yet). Both would
allow {u,}int64_t to also be put in the common header. Yet if e.g.
a prereq assumption faced opposition, some other approach would
need to be found. Plus using either of the named approaches has
issues with the printf() format specifiers, for which I'm yet to
figure out a solution (or maybe someone else knows a good way to
deal with that; using compiler provided headers isn't an option
afaict, as gcc provides stdint.h but not inttypes.h, but maybe
glibc's simplistic approach is good enough - they're targeting
far more architectures than we do and get away with that).

Jan

Oleksii Kurochko Jan. 18, 2023, 11:23 a.m. UTC | #4

On Tue, 2023-01-17 at 11:04 +0100, Jan Beulich wrote:
> On 17.01.2023 10:29, Oleksii wrote:
> > On Mon, 2023-01-16 at 15:59 +0100, Jan Beulich wrote:
> > > On 16.01.2023 15:39, Oleksii Kurochko wrote:
> > > > Signed-off-by: Oleksii Kurochko <[e-mail]>
> > > > ---
> > > > Changes in V4:
> > > >     - Clean up types in <asm/types.h> and remain only
> > > > necessary.
> > > >       The following types was removed as they are defined in
> > > > <xen/types.h>:
> > > >       {__|}{u|s}{8|16|32|64}
> > > 
> > > For one you still typedef u32 and u64. And imo correctly so,
> > > until we
> > > get around to move the definition basic types into xen/types.h.
> > > Plus
> > > I can't see how things build for you: xen/types.h expects
> > > __{u,s}<N>
> > It builds because nothing is used <xen/types.h> now so that's why I
> > missed them but you are right __{u,s}<N> should be backed.
> > It looks like {__,}{u,s}{8,16,32} are the same for all available in
> > Xen
> > architectures so probably can I move them to <xen/types.h> instead
> > of
> > remain them in <asm/types.h>?
> 
> This next step isn't quite as obvious, i.e. has room for being
> contentious. In particular deriving fixed width types from C basic
> types is setting us up for future problems (especially in the
> context of RISC-V think of RV128). Therefore, if we touch and
> generalize this, I'd like to sanitize things at the same time.
> 
> I'd then prefer to typedef {u,}int<N>_t by using either the "mode"
> attribute (requiring us to settle on a prereq of there always being
> 8 bits per char) or the compiler supplied __{U,}INT<N>_TYPE__
> (taking gcc 4.7 as a prereq; didn't check clang yet). Both would
> allow {u,}int64_t to also be put in the common header. Yet if e.g.
> a prereq assumption faced opposition, some other approach would
> need to be found. Plus using either of the named approaches has
> issues with the printf() format specifiers, for which I'm yet to
> figure out a solution (or maybe someone else knows a good way to
> deal with that; using compiler provided headers isn't an option
> afaict, as gcc provides stdint.h but not inttypes.h, but maybe
> glibc's simplistic approach is good enough - they're targeting
> far more architectures than we do and get away with that).
> 
Thanks for the explanation.

If back to RISCV's <asm/types.h> it looks that the v2 of the patch
(https://lore.kernel.org/xen-devel/ca267473[e-mail]/)
is the best one option now because as I can see some work is going on
around <xen/types.h> and keeping the minimal set of types now will
allow us to not remove unneeded types for RISCV's port from asm/types.h
in the future.

Even more based on your patch [
https://lists.xen.org/archives/html/xen-devel/2023-01/msg00720.html ]
RISCV's <asm/types.h> can be empty for this patch series.

> Jan
~Oleksii

Jan Beulich Jan. 18, 2023, 1:03 p.m. UTC | #5

On 18.01.2023 12:23, Oleksii wrote:
> On Tue, 2023-01-17 at 11:04 +0100, Jan Beulich wrote:
>> On 17.01.2023 10:29, Oleksii wrote:
>>> On Mon, 2023-01-16 at 15:59 +0100, Jan Beulich wrote:
>>>> On 16.01.2023 15:39, Oleksii Kurochko wrote:
>>>>> Signed-off-by: Oleksii Kurochko <[e-mail]>
>>>>> ---
>>>>> Changes in V4:
>>>>>     - Clean up types in <asm/types.h> and remain only
>>>>> necessary.
>>>>>       The following types was removed as they are defined in
>>>>> <xen/types.h>:
>>>>>       {__|}{u|s}{8|16|32|64}
>>>>
>>>> For one you still typedef u32 and u64. And imo correctly so,
>>>> until we
>>>> get around to move the definition basic types into xen/types.h.
>>>> Plus
>>>> I can't see how things build for you: xen/types.h expects
>>>> __{u,s}<N>
>>> It builds because nothing is used <xen/types.h> now so that's why I
>>> missed them but you are right __{u,s}<N> should be backed.
>>> It looks like {__,}{u,s}{8,16,32} are the same for all available in
>>> Xen
>>> architectures so probably can I move them to <xen/types.h> instead
>>> of
>>> remain them in <asm/types.h>?
>>
>> This next step isn't quite as obvious, i.e. has room for being
>> contentious. In particular deriving fixed width types from C basic
>> types is setting us up for future problems (especially in the
>> context of RISC-V think of RV128). Therefore, if we touch and
>> generalize this, I'd like to sanitize things at the same time.
>>
>> I'd then prefer to typedef {u,}int<N>_t by using either the "mode"
>> attribute (requiring us to settle on a prereq of there always being
>> 8 bits per char) or the compiler supplied __{U,}INT<N>_TYPE__
>> (taking gcc 4.7 as a prereq; didn't check clang yet). Both would
>> allow {u,}int64_t to also be put in the common header. Yet if e.g.
>> a prereq assumption faced opposition, some other approach would
>> need to be found. Plus using either of the named approaches has
>> issues with the printf() format specifiers, for which I'm yet to
>> figure out a solution (or maybe someone else knows a good way to
>> deal with that; using compiler provided headers isn't an option
>> afaict, as gcc provides stdint.h but not inttypes.h, but maybe
>> glibc's simplistic approach is good enough - they're targeting
>> far more architectures than we do and get away with that).
>>
> Thanks for the explanation.
> 
> If back to RISCV's <asm/types.h> it looks that the v2 of the patch
> (https://lore.kernel.org/xen-devel/ca267473[e-mail]/)
> is the best one option now because as I can see some work is going on
> around <xen/types.h> and keeping the minimal set of types now will
> allow us to not remove unneeded types for RISCV's port from asm/types.h
> in the future.

Well, as said before, I'd prefer if that header wasn't populated piecemeal,
but ...

> Even more based on your patch [
> https://lists.xen.org/archives/html/xen-devel/2023-01/msg00720.html ]
> RISCV's <asm/types.h> can be empty for this patch series.

... leaving it empty for now would of course be fine. Once the basic
fixed width types are needed, imo they ought to be populated all in one
got. But maybe by then we've managed to have that stuff in xen/types.h.

Jan

Patch

diff --git a/xen/arch/riscv/include/asm/types.h b/xen/arch/riscv/include/asm/types.h
new file mode 100644
index 0000000000..9e55bcf776
--- /dev/null
+++ b/xen/arch/riscv/include/asm/types.h
@@ -0,0 +1,43 @@ 
+#ifndef __RISCV_TYPES_H__
+#define __RISCV_TYPES_H__
+
+#ifndef __ASSEMBLY__
+
+#if defined(CONFIG_RISCV_32)
+typedef unsigned long long u64;
+typedef unsigned int u32;
+typedef u32 vaddr_t;
+#define PRIvaddr PRIx32
+typedef u64 paddr_t;
+#define INVALID_PADDR (~0ULL)
+#define PRIpaddr "016llx"
+typedef u32 register_t;
+#define PRIregister "x"
+#elif defined (CONFIG_RISCV_64)
+typedef unsigned long u64;
+typedef u64 vaddr_t;
+#define PRIvaddr PRIx64
+typedef u64 paddr_t;
+#define INVALID_PADDR (~0UL)
+#define PRIpaddr "016lx"
+typedef u64 register_t;
+#define PRIregister "lx"
+#endif
+
+#if defined(__SIZE_TYPE__)
+typedef __SIZE_TYPE__ size_t;
+#else
+typedef unsigned long size_t;
+#endif
+
+#endif /* __ASSEMBLY__ */
+
+#endif /* __RISCV_TYPES_H__ */
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */