1. Patchew
  2. Xen
  3. View series

[PATCH] Arm: constrain {,u}int64_aligned_t in public header

Jan Beulich posted 1 patch 7 months, 4 weeks ago
Failed in applying to current master (apply log)
There is a newer version of this series
[PATCH] Arm: constrain {,u}int64_aligned_t in public header
Posted by Jan Beulich 7 months, 4 weeks ago 
This using a GNU extension, it may not be exposed in general, just like
is done on x86. External consumers need to make this type available up
front (just like we expect {,u}int<N>_t to be supplied) - unlike on x86
the type is actually needed outside of tools-only interfaces, because
guest handle definitions use it.

While there also add underscores around "aligned".

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/include/public/arch-arm.h
+++ b/xen/include/public/arch-arm.h
@@ -152,8 +152,10 @@
 
 #define XEN_HYPERCALL_TAG   0XEA1
 
-#define  int64_aligned_t  int64_t __attribute__((aligned(8)))
-#define uint64_aligned_t uint64_t __attribute__((aligned(8)))
+#if defined(__XEN__) || defined(__XEN_TOOLS__)
+#define  int64_aligned_t  int64_t __attribute__((__aligned__(8)))
+#define uint64_aligned_t uint64_t __attribute__((__aligned__(8)))
+#endif
 
 #ifndef __ASSEMBLY__
 #define ___DEFINE_XEN_GUEST_HANDLE(name, type)                  \
Re: [PATCH] Arm: constrain {,u}int64_aligned_t in public header
Posted by Henry Wang 7 months, 3 weeks ago 
Hi Jan,

> On Sep 1, 2023, at 15:26, Jan Beulich <jbeulich@suse.com> wrote:
> 
> This using a GNU extension, it may not be exposed in general, just like
> is done on x86. External consumers need to make this type available up
> front (just like we expect {,u}int<N>_t to be supplied) - unlike on x86
> the type is actually needed outside of tools-only interfaces, because
> guest handle definitions use it.
> 
> While there also add underscores around "aligned".
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/include/public/arch-arm.h
> +++ b/xen/include/public/arch-arm.h
> @@ -152,8 +152,10 @@
> 
> #define XEN_HYPERCALL_TAG   0XEA1
> 
> -#define  int64_aligned_t  int64_t __attribute__((aligned(8)))
> -#define uint64_aligned_t uint64_t __attribute__((aligned(8)))
> +#if defined(__XEN__) || defined(__XEN_TOOLS__)
> +#define  int64_aligned_t  int64_t __attribute__((__aligned__(8)))
> +#define uint64_aligned_t uint64_t __attribute__((__aligned__(8)))
> +#endif

Today I tested this patch by our CI (on top of today’s staging), and it looks
like below error will happen for both arm32 and arm64 Yocto build:

The arm32 failure:
https://pastebin.com/raw/S7ZqmG6z

The arm64 failure:
https://pastebin.com/raw/HMFh5tuS

Kind regards,
Henry

> 
> #ifndef __ASSEMBLY__
> #define ___DEFINE_XEN_GUEST_HANDLE(name, type)                  \
>
Re: [PATCH] Arm: constrain {,u}int64_aligned_t in public header
Posted by Jan Beulich 7 months, 3 weeks ago 
On 05.09.2023 04:08, Henry Wang wrote:
>> On Sep 1, 2023, at 15:26, Jan Beulich <jbeulich@suse.com> wrote:
>> --- a/xen/include/public/arch-arm.h
>> +++ b/xen/include/public/arch-arm.h
>> @@ -152,8 +152,10 @@
>>
>> #define XEN_HYPERCALL_TAG   0XEA1
>>
>> -#define  int64_aligned_t  int64_t __attribute__((aligned(8)))
>> -#define uint64_aligned_t uint64_t __attribute__((aligned(8)))
>> +#if defined(__XEN__) || defined(__XEN_TOOLS__)
>> +#define  int64_aligned_t  int64_t __attribute__((__aligned__(8)))
>> +#define uint64_aligned_t uint64_t __attribute__((__aligned__(8)))
>> +#endif
> 
> Today I tested this patch by our CI (on top of today’s staging), and it looks
> like below error will happen for both arm32 and arm64 Yocto build:
> 
> The arm32 failure:
> https://pastebin.com/raw/S7ZqmG6z
> 
> The arm64 failure:
> https://pastebin.com/raw/HMFh5tuS

Thanks for pointing this out. Turns out that not even all libraries
get __XEN_TOOLS__ defined (see tools/Rules.mk). In the case of
toolcore, the public xen.h is included solely to get a definition
of domid_t. None of the handles are actually needed. I wonder if such
a dependency couldn't be avoided. Yet doing so would help only a little:
Other libraries (evtchn or gnttab for example) likely need more, yet
don't depend on libxc either.

For the time being I'll extend the #if to also check for __GNUC__, but
I'm not convinced this is a good way of dealing with the issue.

Jan
Re: [PATCH] Arm: constrain {,u}int64_aligned_t in public header
Posted by Bertrand Marquis 7 months, 3 weeks ago 
Hi Jan,

> On 1 Sep 2023, at 09:26, Jan Beulich <jbeulich@suse.com> wrote:
> 
> This using a GNU extension, it may not be exposed in general, just like

Nit: Missing "is"

> is done on x86. External consumers need to make this type available up
> front (just like we expect {,u}int<N>_t to be supplied) - unlike on x86
> the type is actually needed outside of tools-only interfaces, because
> guest handle definitions use it.
> 
> While there also add underscores around "aligned".
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

With the Nit fixed (can be done on commit):

Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

Cheers
Bertrand

> 
> --- a/xen/include/public/arch-arm.h
> +++ b/xen/include/public/arch-arm.h
> @@ -152,8 +152,10 @@
> 
> #define XEN_HYPERCALL_TAG   0XEA1
> 
> -#define  int64_aligned_t  int64_t __attribute__((aligned(8)))
> -#define uint64_aligned_t uint64_t __attribute__((aligned(8)))
> +#if defined(__XEN__) || defined(__XEN_TOOLS__)
> +#define  int64_aligned_t  int64_t __attribute__((__aligned__(8)))
> +#define uint64_aligned_t uint64_t __attribute__((__aligned__(8)))
> +#endif
> 
> #ifndef __ASSEMBLY__
> #define ___DEFINE_XEN_GUEST_HANDLE(name, type)                  \
Re: [PATCH] Arm: constrain {,u}int64_aligned_t in public header
Posted by Jan Beulich 7 months, 3 weeks ago 
On 04.09.2023 15:42, Bertrand Marquis wrote:
>> On 1 Sep 2023, at 09:26, Jan Beulich <jbeulich@suse.com> wrote:
>>
>> This using a GNU extension, it may not be exposed in general, just like
> 
> Nit: Missing "is"

I would guess you would want it added as the 2nd word of the sentence. If
not, please clarify where you think it is missing. If so, then I'm afraid
I can't parse the sentence anymore with it added (i.e. there would need
to be further modifications, e.g. at the very least "so" after the first
comma).

>> is done on x86. External consumers need to make this type available up
>> front (just like we expect {,u}int<N>_t to be supplied) - unlike on x86
>> the type is actually needed outside of tools-only interfaces, because
>> guest handle definitions use it.
>>
>> While there also add underscores around "aligned".
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> With the Nit fixed (can be done on commit):
> 
> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

Thanks, but I'm afraid I can't take it before the above is clarified.

Jan
Re: [PATCH] Arm: constrain {,u}int64_aligned_t in public header
Posted by Bertrand Marquis 7 months, 3 weeks ago 
Hi Jan,

> On 4 Sep 2023, at 16:01, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 04.09.2023 15:42, Bertrand Marquis wrote:
>>> On 1 Sep 2023, at 09:26, Jan Beulich <jbeulich@suse.com> wrote:
>>> 
>>> This using a GNU extension, it may not be exposed in general, just like
>> 
>> Nit: Missing "is"
> 
> I would guess you would want it added as the 2nd word of the sentence. If
> not, please clarify where you think it is missing. If so, then I'm afraid
> I can't parse the sentence anymore with it added (i.e. there would need
> to be further modifications, e.g. at the very least "so" after the first
> comma).

Sorry yes, it should be "This is using a GNU".

> 
>>> is done on x86. External consumers need to make this type available up
>>> front (just like we expect {,u}int<N>_t to be supplied) - unlike on x86
>>> the type is actually needed outside of tools-only interfaces, because
>>> guest handle definitions use it.
>>> 
>>> While there also add underscores around "aligned".
>>> 
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> 
>> With the Nit fixed (can be done on commit):
>> 
>> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
> 
> Thanks, but I'm afraid I can't take it before the above is clarified.

Please see above.

Bertrand

> 
> Jan
Re: [PATCH] Arm: constrain {,u}int64_aligned_t in public header
Posted by Jan Beulich 7 months, 3 weeks ago 
On 04.09.2023 16:05, Bertrand Marquis wrote:
>> On 4 Sep 2023, at 16:01, Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 04.09.2023 15:42, Bertrand Marquis wrote:
>>>> On 1 Sep 2023, at 09:26, Jan Beulich <jbeulich@suse.com> wrote:
>>>>
>>>> This using a GNU extension, it may not be exposed in general, just like
>>>
>>> Nit: Missing "is"
>>
>> I would guess you would want it added as the 2nd word of the sentence. If
>> not, please clarify where you think it is missing. If so, then I'm afraid
>> I can't parse the sentence anymore with it added (i.e. there would need
>> to be further modifications, e.g. at the very least "so" after the first
>> comma).
> 
> Sorry yes, it should be "This is using a GNU".

So as I inferred, yet as said - according to my reading the sentence then
ends up broken. If you continue to think the sentence is wrong as is, would
it help if I replaced "This" by "For"?

Jan

>>>> is done on x86. External consumers need to make this type available up
>>>> front (just like we expect {,u}int<N>_t to be supplied) - unlike on x86
>>>> the type is actually needed outside of tools-only interfaces, because
>>>> guest handle definitions use it.
>>>>
>>>> While there also add underscores around "aligned".
>>>>
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>
>>> With the Nit fixed (can be done on commit):
>>>
>>> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
>>
>> Thanks, but I'm afraid I can't take it before the above is clarified.
> 
> Please see above.
> 
> Bertrand
> 
>>
>> Jan
>
Re: [PATCH] Arm: constrain {,u}int64_aligned_t in public header
Posted by Bertrand Marquis 7 months, 3 weeks ago 
Hi Jan,

> On 4 Sep 2023, at 17:20, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 04.09.2023 16:05, Bertrand Marquis wrote:
>>> On 4 Sep 2023, at 16:01, Jan Beulich <jbeulich@suse.com> wrote:
>>> 
>>> On 04.09.2023 15:42, Bertrand Marquis wrote:
>>>>> On 1 Sep 2023, at 09:26, Jan Beulich <jbeulich@suse.com> wrote:
>>>>> 
>>>>> This using a GNU extension, it may not be exposed in general, just like
>>>> 
>>>> Nit: Missing "is"
>>> 
>>> I would guess you would want it added as the 2nd word of the sentence. If
>>> not, please clarify where you think it is missing. If so, then I'm afraid
>>> I can't parse the sentence anymore with it added (i.e. there would need
>>> to be further modifications, e.g. at the very least "so" after the first
>>> comma).
>> 
>> Sorry yes, it should be "This is using a GNU".
> 
> So as I inferred, yet as said - according to my reading the sentence then
> ends up broken. If you continue to think the sentence is wrong as is, would
> it help if I replaced "This" by "For"?

The sentence looks a bit weird to me but I am not a native english speaker.
Any reformulation coming from me will probably not be good english anyway.
I understand that one as "we don't want to expose this in general because
it is a using a GNU extension and x86 is already not", the sentence here is
just asking me a bit more thinking that is it.

As this was a Nit, feel free to ignore and you can keep my R-b.

Cheers
Bertrand

> 
> Jan
> 
>>>>> is done on x86. External consumers need to make this type available up
>>>>> front (just like we expect {,u}int<N>_t to be supplied) - unlike on x86
>>>>> the type is actually needed outside of tools-only interfaces, because
>>>>> guest handle definitions use it.
>>>>> 
>>>>> While there also add underscores around "aligned".
>>>>> 
>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>> 
>>>> With the Nit fixed (can be done on commit):
>>>> 
>>>> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
>>> 
>>> Thanks, but I'm afraid I can't take it before the above is clarified.
>> 
>> Please see above.
>> 
>> Bertrand
>> 
>>> 
>>> Jan

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.