| Message ID | 56bb2024-dea0-79e6-6a51-66e6c35a2733@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Arm: constrain {,u}int64_aligned_t in public header | expand |
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) \
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
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
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 >
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
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) \ >
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
--- 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) \
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>