| Message ID | 26cfd90a-91f2-4bf4-9607-8ab6c7823048@suse.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86/ACPI: annotate assembly data with type and size | expand |
On 02/10/2024 8:41 am, Jan Beulich wrote: > Further use the generic framework from xen/linkage.h. While there drop > excess alignment and move to .bss. > > Requested-by: Andrew Cooper <andrew.cooper3@citrix.com> > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > Of course alongside ASM_INT() we could introduce ASM_QUAD() and > ASM_QUAD_LOCAL() (only the latter needed right here) to aid readability. > Thoughts? Honestly, ASM_INT() hiding a .long is confusing enough already. ASM_C_{INT,LONG}() wouldn't be as bad. At least they're clear about being a particular type in another language. > --- a/xen/arch/x86/acpi/wakeup_prot.S > +++ b/xen/arch/x86/acpi/wakeup_prot.S > @@ -1,3 +1,5 @@ > +#define DATA_FILL 0 /* For the .bss contributions at the bottom. */ > + I really feel that here is the wrong place for this to live. Why isn't it in xen/linkage.h? When is data typically padded with anything other than 0's? ~Andrew
On 02.10.2024 11:07, Andrew Cooper wrote: > On 02/10/2024 8:41 am, Jan Beulich wrote: >> Further use the generic framework from xen/linkage.h. While there drop >> excess alignment and move to .bss. >> >> Requested-by: Andrew Cooper <andrew.cooper3@citrix.com> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> --- >> Of course alongside ASM_INT() we could introduce ASM_QUAD() and >> ASM_QUAD_LOCAL() (only the latter needed right here) to aid readability. >> Thoughts? > > Honestly, ASM_INT() hiding a .long is confusing enough already. > > ASM_C_{INT,LONG}() wouldn't be as bad. At least they're clear about > being a particular type in another language. I don't think the _C_ would add much; we all know C is the language Xen is written in. ASM_LONG() / ASM_C_LONG() would not be generalizable, i.e. couldn't be put in xen/linkage.h without arch customization, as that can neither expand uniformly to .long nor to .quad. It's all solvable, but would be getting involved. >> --- a/xen/arch/x86/acpi/wakeup_prot.S >> +++ b/xen/arch/x86/acpi/wakeup_prot.S >> @@ -1,3 +1,5 @@ >> +#define DATA_FILL 0 /* For the .bss contributions at the bottom. */ >> + > > I really feel that here is the wrong place for this to live. > > Why isn't it in xen/linkage.h? When is data typically padded with > anything other than 0's? As per what we currently have, the default data padding is ~0. Personally I consider this marginally better than 0, but it could of course be changed. Jan
--- a/xen/arch/x86/acpi/wakeup_prot.S +++ b/xen/arch/x86/acpi/wakeup_prot.S @@ -1,3 +1,5 @@ +#define DATA_FILL 0 /* For the .bss contributions at the bottom. */ + #include <asm/asm_defns.h> #include <asm/msr-index.h> #include <asm/page.h> @@ -134,13 +136,20 @@ LABEL(s3_resume) ret END(do_suspend_lowlevel) -.data - .align 16 + .bss -saved_rsp: .quad 0 -saved_cr0: .quad 0 +DATA_LOCAL(saved_rsp, 8) + .quad 0 +END(saved_rsp) +DATA_LOCAL(saved_cr0, 8) + .quad 0 +END(saved_cr0) #ifdef CONFIG_XEN_SHSTK -saved_ssp: .quad 0 +DATA_LOCAL(saved_ssp, 8) + .quad 0 +END(saved_ssp) #endif + .data + ASM_INT(saved_magic, 0x9abcdef0)
Further use the generic framework from xen/linkage.h. While there drop excess alignment and move to .bss. Requested-by: Andrew Cooper <andrew.cooper3@citrix.com> Signed-off-by: Jan Beulich <jbeulich@suse.com> --- Of course alongside ASM_INT() we could introduce ASM_QUAD() and ASM_QUAD_LOCAL() (only the latter needed right here) to aid readability. Thoughts?