RE: [PATCH v6 1/8] xen: reuse x86 EFI stub functions for Arm

[Wei Chen <Wei.Chen@xxxxxxx>]

Hi Jan,

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: 2022年6月30日 20:36
> To: Wei Chen <Wei.Chen@xxxxxxx>
> Cc: nd <nd@xxxxxxx>; Stefano Stabellini <sstabellini@xxxxxxxxxx>; Bertrand
> Marquis <Bertrand.Marquis@xxxxxxx>; Volodymyr Babchuk
> <Volodymyr_Babchuk@xxxxxxxx>; Andrew Cooper <andrew.cooper3@xxxxxxxxxx>;
> Roger Pau Monné <roger.pau@xxxxxxxxxx>; Wei Liu <wl@xxxxxxx>; Jiamei Xie
> <Jiamei.Xie@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx; Julien Grall
> <julien@xxxxxxx>
> Subject: Re: [PATCH v6 1/8] xen: reuse x86 EFI stub functions for Arm
> 
> On 30.06.2022 13:25, Wei Chen wrote:
> >> From: Jan Beulich <jbeulich@xxxxxxxx>
> >> Sent: 2022年6月24日 18:09
> >>
> >> On 24.06.2022 12:05, Jan Beulich wrote:
> >>> On 24.06.2022 11:49, Julien Grall wrote:
> >>>>>>>>> --- a/xen/arch/arm/efi/Makefile
> >>>>>>>>> +++ b/xen/arch/arm/efi/Makefile
> >>>>>>>>> @@ -1,4 +1,12 @@
> >>>>>>>>>    include $(srctree)/common/efi/efi-common.mk
> >>>>>>>>>
> >>>>>>>>> +ifeq ($(CONFIG_ARM_EFI),y)
> >>>>>>>>>    obj-y += $(EFIOBJ-y)
> >>>>>>>>>    obj-$(CONFIG_ACPI) +=  efi-dom0.init.o
> >>>>>>>>> +else
> >>>>>>>>> +# Add stub.o to EFIOBJ-y to re-use the clean-files in
> >>>>>>>>> +# efi-common.mk. Otherwise the link of stub.c in arm/efi
> >>>>>>>>> +# will not be cleaned in "make clean".
> >>>>>>>>> +EFIOBJ-y += stub.o
> >>>>>>>>> +obj-y += stub.o
> >>>>>>>>> +endif
> >>>>>>>>
> >>>>>>>> This has caused
> >>>>>>>>
> >>>>>>>> ld: warning: arch/arm/efi/built_in.o uses 2-byte wchar_t yet the
> >> output is
> >>>>>>>> to use 4-byte wchar_t; use of wchar_t values across objects may
> >> fail
> >>>>>>>>
> >>>>>>>> for the 32-bit Arm build that I keep doing every once in a while,
> >> with
> >>>>>>>> (if it matters) GNU ld 2.38. I guess you will want to consider
> >> building
> >>>>>>>> all of Xen with -fshort-wchar, or to avoid building stub.c with
> >> that
> >>>>>>>> option.
> >>>>>>>>
> >>>>>>>
> >>>>>>> Thanks for pointing this out. I will try to use -fshort-wchar for
> >> Arm32,
> >>>>>>> if Arm maintainers agree.
> >>>>>>
> >>>>>> Looking at the code we don't seem to build Xen arm64 with -fshort-
> >> wchar
> >>>>>> (aside the EFI files). So it is not entirely clear why we would
> want
> >> to
> >>>>>> use -fshort-wchar for arm32.
> >>>>>
> >>>>> We don't use wchar_t outside of EFI code afaict. Hence to all other
> >> code
> >>>>> it should be benign whether -fshort-wchar is in use. So the
> suggestion
> >>>>> to use the flag unilaterally on Arm32 is really just to silence the
> ld
> >>>>> warning;
> >>>>
> >>>> Ok. This is odd. Why would ld warn on arm32 but not other arch?
> >>>
> >>> Arm32 embeds ABI information in a note section in each object file.
> >>
> >> Or a note-like one (just to avoid possible confusion); I think it's
> >> ".ARM.attributes".
> >>
> >> Jan
> >>
> >>> The mismatch of the wchar_t part of this information is what causes
> >>> ld to emit the warning.
> >>>
> >>>>> off the top of my head I can't see anything wrong with using
> >>>>> the option also for Arm64 or even globally. Yet otoh we typically
> try
> >> to
> >>>>> not make changes for environments where they aren't really needed.
> >>>>
> >>>> I agree. If we need a workaround, then my preference would be to not
> >>>> build stub.c with -fshort-wchar.
> >>>
> >>> This would need to be an Arm-special then, as on x86 it needs to be
> >> built
> >>> this way.
> >
> > I have taken a look into this warning:
> > This is because the "-fshort-wchar" flag causes GCC to generate
> > code that is not binary compatible with code generated without
> > that flag. Why this warning hasn't been triggered in Arm64 is
> > because we don't use any wchar in Arm64 codes.
> 
> I don't think that's quite right - you actually say below that we
> do use it there when interacting with UEFI. There's no warning
> there solely because the information isn't embedded in the object
> files there, from all I can tell.
> 

Maybe I should describe it this way: Arm64 does not use wchar type
directly in any code for parameters, variables and return values.
So Arm64 object files are exactly the same with "-fshort-wchar" and
without "-fshort-wchar".

Although Xen's EFI code interacts with UEFI firmware, similar to RPC
function calls, these code also do not explicitly use wchar. But it
is reasonable to keep -fshort-wchar for Xen's EFI code, but as long
as we use wchar in EFI code in the future, we will definitely encounter
this warning like Arm32.

> > We are also not
> > using wchar in Arm32 codes, but Arm32 will embed ABI information
> > in ".ARM.attributes" section. This section stores some object
> > file attributes, like ABI version, CPU arch and etc. And wchar
> > size is described in this section by "Tag_ABI_PCS_wchar_t" too.
> > Tag_ABI_PCS_wchar_t is 2 for object files with "-fshort-wchar",
> > but for object files without "-fshort-wchar" is 4. Arm32 GCC
> > ld will check this tag, and throw above warning when it finds
> > the object files have different Tag_ABI_PCS_wchar_t values.
> >
> > As gnu-efi-3.0 use the GCC option "-fshort-wchar" to force wchar
> > to use short integers (2 bytes) instead of integers (4 bytes).
> > We can't remove this option from x86 and Arm64, because they need
> > to interact with EFI firmware. So I have to options:
> > 1. Remove "-fshort-wchar" from efi-common.mk and add it back by
> >    x86 and arm64's EFI Makefile
> > 2. Add "-no-wchar-size-warning" to Arm32's linker flags
> >
> > I personally prefer option#1, because Arm32 doesn't need to interact
> > with EFI firmware, all it requires are some stub functions. And
> > "-no-wchar-size-warning" may hide some warnings we should aware in
> > future.
> 
> I don't mind #1, but I think your subsequently proposed #3 would be
> the first thing to try. There may be caveats, so if that doesn't work
> out I'd suggest falling back to #1. Albeit ideally the flag setting
> wouldn't be moved back (it _is_ a common EFI thing, after all), but
> rather Arm32 arranging for its addition to be suppressed.
> 

I am OK with option#3 to set "-fshort-wchar" as a global CFLAGS. This
flag will affect wchar performance (non-4bytes-alignment), but Xen
doesn't use wchar out of EFI. So setting it as a global flag should
be harmless. It can also avoid similar warnings from appearing again.

Cheers,
Wei Chen

> Jan