| Message ID | ca2674739cfa71cae0bf084a7b471ad4518026d3.1673362493.git.oleksii.kurochko@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Basic early_printk and smoke test implementation | expand |
On 10.01.2023 16:17, Oleksii Kurochko wrote: > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> > --- > Changes in V3: > - Nothing changed > --- > Changes in V2: > - Remove unneeded now types from <asm/types.h> I guess you went a little too far: Types used in common code, even if you do not build that yet, will want declaring right away imo. Of course I should finally try and get rid of at least some of the being-phased-out ones (s8 and s16 look to be relatively low hanging fruit, for example, and of these only s16 looks to be used in common code) ... Jan
On Tue, 2023-01-10 at 17:58 +0100, Jan Beulich wrote: > On 10.01.2023 16:17, Oleksii Kurochko wrote: > > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> > > --- > > Changes in V3: > > - Nothing changed > > --- > > Changes in V2: > > - Remove unneeded now types from <asm/types.h> > > I guess you went a little too far: Types used in common code, even if > you It looks then I didn't understand which one of types are needed. In "[PATCH v1 2/8] xen/riscv: introduce asm/types.h header file" all types were introduced as most of them are used in <xen/types.h>: __{u|s}{8|16|32|64}. Thereby it looks like the following types in <asm/types.h> should be present from the start: typedef __signed__ char __s8; typedef unsigned char __u8; typedef __signed__ short __s16; typedef unsigned short __u16; typedef __signed__ int __s32; typedef unsigned int __u32; #if defined(__GNUC__) && !defined(__STRICT_ANSI__) #if defined(CONFIG_RISCV_32) typedef __signed__ long long __s64; typedef unsigned long long __u64; #elif defined (CONFIG_RISCV_64) typedef __signed__ long __s64; typedef unsigned long __u64; #endif #endif Then it turns out that there is no any sense in: typedef signed char s8; typedef unsigned char u8; typedef signed short s16; typedef unsigned short u16; typedef signed int s32; typedef unsigned int u32; typedef signed long long s64; typedef unsigned long long u64; OR typedef signed long s64; typedef unsigned long u64; As I understand instead of them should be used: {u|s}int<N>_t. All other types such as {v,p}addr_t, register_t and definitions PRIvaddr, INVALID_PADDR, PRIpaadr, PRIregister should be present in <asm/types.h> from the start. Am I right? > do not build that yet, will want declaring right away imo. Of course > I > should finally try and get rid of at least some of the being-phased- > out > ones (s8 and s16 look to be relatively low hanging fruit, for > example, > and of these only s16 looks to be used in common code) ... > > Jan ~Oleksii
On 11.01.2023 09:47, Oleksii wrote: > On Tue, 2023-01-10 at 17:58 +0100, Jan Beulich wrote: >> On 10.01.2023 16:17, Oleksii Kurochko wrote: >>> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> >>> --- >>> Changes in V3: >>> - Nothing changed >>> --- >>> Changes in V2: >>> - Remove unneeded now types from <asm/types.h> >> >> I guess you went a little too far: Types used in common code, even if >> you > It looks then I didn't understand which one of types are needed. > > In "[PATCH v1 2/8] xen/riscv: introduce asm/types.h header file" all > types were introduced as most of them are used in <xen/types.h>: > __{u|s}{8|16|32|64}. Thereby it looks like the following types in > <asm/types.h> should be present from the start: > typedef __signed__ char __s8; > typedef unsigned char __u8; > > typedef __signed__ short __s16; > typedef unsigned short __u16; > > typedef __signed__ int __s32; > typedef unsigned int __u32; > > #if defined(__GNUC__) && !defined(__STRICT_ANSI__) > #if defined(CONFIG_RISCV_32) > typedef __signed__ long long __s64; > typedef unsigned long long __u64; > #elif defined (CONFIG_RISCV_64) > typedef __signed__ long __s64; > typedef unsigned long __u64; > #endif > #endif > > Then it turns out that there is no any sense in: > typedef signed char s8; > typedef unsigned char u8; > > typedef signed short s16; > typedef unsigned short u16; > > typedef signed int s32; > typedef unsigned int u32; > > typedef signed long long s64; > typedef unsigned long long u64; > OR > typedef signed long s64; > typedef unsigned long u64; > As I understand instead of them should be used: {u|s}int<N>_t. Hmm, the situation is worse than I thought (recalled) it was: You're right, xen/types.h actually uses __{u,s}<N>. So I'm sorry for mis- guiding you; we'll need to do more cleanup first for asm/types.h to become smaller. Jan
On 1/11/23 11:08, Jan Beulich wrote: > On 11.01.2023 09:47, Oleksii wrote: >> On Tue, 2023-01-10 at 17:58 +0100, Jan Beulich wrote: >>> On 10.01.2023 16:17, Oleksii Kurochko wrote: >>>> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> >>>> --- >>>> Changes in V3: >>>> - Nothing changed >>>> --- >>>> Changes in V2: >>>> - Remove unneeded now types from <asm/types.h> >>> >>> I guess you went a little too far: Types used in common code, even if >>> you >> It looks then I didn't understand which one of types are needed. >> >> In "[PATCH v1 2/8] xen/riscv: introduce asm/types.h header file" all >> types were introduced as most of them are used in <xen/types.h>: >> __{u|s}{8|16|32|64}. Thereby it looks like the following types in >> <asm/types.h> should be present from the start: >> typedef __signed__ char __s8; >> typedef unsigned char __u8; >> >> typedef __signed__ short __s16; >> typedef unsigned short __u16; >> >> typedef __signed__ int __s32; >> typedef unsigned int __u32; >> >> #if defined(__GNUC__) && !defined(__STRICT_ANSI__) >> #if defined(CONFIG_RISCV_32) >> typedef __signed__ long long __s64; >> typedef unsigned long long __u64; >> #elif defined (CONFIG_RISCV_64) >> typedef __signed__ long __s64; >> typedef unsigned long __u64; >> #endif >> #endif >> >> Then it turns out that there is no any sense in: >> typedef signed char s8; >> typedef unsigned char u8; >> >> typedef signed short s16; >> typedef unsigned short u16; >> >> typedef signed int s32; >> typedef unsigned int u32; >> >> typedef signed long long s64; >> typedef unsigned long long u64; >> OR >> typedef signed long s64; >> typedef unsigned long u64; >> As I understand instead of them should be used: {u|s}int<N>_t. > > Hmm, the situation is worse than I thought (recalled) it was: You're > right, xen/types.h actually uses __{u,s}<N>. So I'm sorry for mis- > guiding you; we'll need to do more cleanup first for asm/types.h to > become smaller. What is the reason for not declaring __{u,s}<N> directly in xen/types.h as type alias to {u,s}<N>? IIUC __{u,s}<N> and {u,s}<N> are needed by code ported from Linux while Xen code should use {u|s}int<N>_t instead, right?
On 11.01.2023 11:22, Xenia Ragiadakou wrote: > > On 1/11/23 11:08, Jan Beulich wrote: >> On 11.01.2023 09:47, Oleksii wrote: >>> On Tue, 2023-01-10 at 17:58 +0100, Jan Beulich wrote: >>>> On 10.01.2023 16:17, Oleksii Kurochko wrote: >>>>> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> >>>>> --- >>>>> Changes in V3: >>>>> - Nothing changed >>>>> --- >>>>> Changes in V2: >>>>> - Remove unneeded now types from <asm/types.h> >>>> >>>> I guess you went a little too far: Types used in common code, even if >>>> you >>> It looks then I didn't understand which one of types are needed. >>> >>> In "[PATCH v1 2/8] xen/riscv: introduce asm/types.h header file" all >>> types were introduced as most of them are used in <xen/types.h>: >>> __{u|s}{8|16|32|64}. Thereby it looks like the following types in >>> <asm/types.h> should be present from the start: >>> typedef __signed__ char __s8; >>> typedef unsigned char __u8; >>> >>> typedef __signed__ short __s16; >>> typedef unsigned short __u16; >>> >>> typedef __signed__ int __s32; >>> typedef unsigned int __u32; >>> >>> #if defined(__GNUC__) && !defined(__STRICT_ANSI__) >>> #if defined(CONFIG_RISCV_32) >>> typedef __signed__ long long __s64; >>> typedef unsigned long long __u64; >>> #elif defined (CONFIG_RISCV_64) >>> typedef __signed__ long __s64; >>> typedef unsigned long __u64; >>> #endif >>> #endif >>> >>> Then it turns out that there is no any sense in: >>> typedef signed char s8; >>> typedef unsigned char u8; >>> >>> typedef signed short s16; >>> typedef unsigned short u16; >>> >>> typedef signed int s32; >>> typedef unsigned int u32; >>> >>> typedef signed long long s64; >>> typedef unsigned long long u64; >>> OR >>> typedef signed long s64; >>> typedef unsigned long u64; >>> As I understand instead of them should be used: {u|s}int<N>_t. >> >> Hmm, the situation is worse than I thought (recalled) it was: You're >> right, xen/types.h actually uses __{u,s}<N>. So I'm sorry for mis- >> guiding you; we'll need to do more cleanup first for asm/types.h to >> become smaller. > > What is the reason for not declaring __{u,s}<N> directly in xen/types.h > as type alias to {u,s}<N>? > > IIUC __{u,s}<N> and {u,s}<N> are needed by code ported from Linux while > Xen code should use {u|s}int<N>_t instead, right? Yes. Hence in the long run only Linux files should get to see __{u,s}<N> and {u,s}<N>, perhaps from a new linux-types.h. Jan
On 1/11/23 13:38, Jan Beulich wrote: > On 11.01.2023 11:22, Xenia Ragiadakou wrote: >> >> On 1/11/23 11:08, Jan Beulich wrote: >>> On 11.01.2023 09:47, Oleksii wrote: >>>> On Tue, 2023-01-10 at 17:58 +0100, Jan Beulich wrote: >>>>> On 10.01.2023 16:17, Oleksii Kurochko wrote: >>>>>> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> >>>>>> --- >>>>>> Changes in V3: >>>>>> - Nothing changed >>>>>> --- >>>>>> Changes in V2: >>>>>> - Remove unneeded now types from <asm/types.h> >>>>> >>>>> I guess you went a little too far: Types used in common code, even if >>>>> you >>>> It looks then I didn't understand which one of types are needed. >>>> >>>> In "[PATCH v1 2/8] xen/riscv: introduce asm/types.h header file" all >>>> types were introduced as most of them are used in <xen/types.h>: >>>> __{u|s}{8|16|32|64}. Thereby it looks like the following types in >>>> <asm/types.h> should be present from the start: >>>> typedef __signed__ char __s8; >>>> typedef unsigned char __u8; >>>> >>>> typedef __signed__ short __s16; >>>> typedef unsigned short __u16; >>>> >>>> typedef __signed__ int __s32; >>>> typedef unsigned int __u32; >>>> >>>> #if defined(__GNUC__) && !defined(__STRICT_ANSI__) >>>> #if defined(CONFIG_RISCV_32) >>>> typedef __signed__ long long __s64; >>>> typedef unsigned long long __u64; >>>> #elif defined (CONFIG_RISCV_64) >>>> typedef __signed__ long __s64; >>>> typedef unsigned long __u64; >>>> #endif >>>> #endif >>>> >>>> Then it turns out that there is no any sense in: >>>> typedef signed char s8; >>>> typedef unsigned char u8; >>>> >>>> typedef signed short s16; >>>> typedef unsigned short u16; >>>> >>>> typedef signed int s32; >>>> typedef unsigned int u32; >>>> >>>> typedef signed long long s64; >>>> typedef unsigned long long u64; >>>> OR >>>> typedef signed long s64; >>>> typedef unsigned long u64; >>>> As I understand instead of them should be used: {u|s}int<N>_t. >>> >>> Hmm, the situation is worse than I thought (recalled) it was: You're >>> right, xen/types.h actually uses __{u,s}<N>. So I'm sorry for mis- >>> guiding you; we'll need to do more cleanup first for asm/types.h to >>> become smaller. >> >> What is the reason for not declaring __{u,s}<N> directly in xen/types.h >> as type alias to {u,s}<N>? >> >> IIUC __{u,s}<N> and {u,s}<N> are needed by code ported from Linux while >> Xen code should use {u|s}int<N>_t instead, right? > > Yes. Hence in the long run only Linux files should get to see __{u,s}<N> > and {u,s}<N>, perhaps from a new linux-types.h. Thanks for the clarification. Could you please help me understand also why __signed__ keyword is required when declaring __{u,s}<N>? I mean why __{u,s}<N> cannot be declared using the signed type specifier, just like {u,s}<N>?
On 11.01.2023 13:30, Xenia Ragiadakou wrote: > Could you please help me understand also > why __signed__ keyword is required when declaring __{u,s}<N>? > I mean why __{u,s}<N> cannot be declared using the signed type > specifier, just like {u,s}<N>? I'm afraid I can't, as this looks to have been (blindly?) imported from Linux very, very long ago. Having put time in going through our own history, I'm afraid I don't want to go further and see whether I could spot the reason for you by going through Linux'es. Jan
On 1/11/23 15:17, Jan Beulich wrote: > On 11.01.2023 13:30, Xenia Ragiadakou wrote: >> Could you please help me understand also >> why __signed__ keyword is required when declaring __{u,s}<N>? >> I mean why __{u,s}<N> cannot be declared using the signed type >> specifier, just like {u,s}<N>? > > I'm afraid I can't, as this looks to have been (blindly?) imported > from Linux very, very long ago. Having put time in going through > our own history, I'm afraid I don't want to go further and see > whether I could spot the reason for you by going through Linux'es. Sorry, I was not aiming to drag you (or anyone) into spotting why Linux uses __signed__ when declaring __s<N>. AfAIU these types are exported and used in userspace and maybe the reason is to support building with pre-standard C compilers. I am just wondering why Xen, that is compiled with std=c99, uses __signed__. If there is no reason, I think this difference between the declarations of __{u,s}<N> and {u,s}<N> is misleading and confusing (to me at least).
diff --git a/xen/arch/riscv/include/asm/types.h b/xen/arch/riscv/include/asm/types.h new file mode 100644 index 0000000000..fbe352ef20 --- /dev/null +++ b/xen/arch/riscv/include/asm/types.h @@ -0,0 +1,22 @@ +#ifndef __RISCV_TYPES_H__ +#define __RISCV_TYPES_H__ + +#ifndef __ASSEMBLY__ + +#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: + */
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> --- Changes in V3: - Nothing changed --- Changes in V2: - Remove unneeded now types from <asm/types.h> --- xen/arch/riscv/include/asm/types.h | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) create mode 100644 xen/arch/riscv/include/asm/types.h