| Message ID | 20240807134819.8987-4-alejandro.vallejo@cloud.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Improve support for EFI multiboot loading | expand |
On 07.08.2024 15:48, Alejandro Vallejo wrote: > No reason to wait, if Xen image is loaded by EFI (not multiboot > EFI path) these are set in efi_arch_load_addr_check, but > not in the multiboot EFI code path. > This change makes the 2 code paths more similar and allows > the usage of these variables if needed. I'm afraid I'm struggling with any "similarity" argument here. Imo it would be better what, if anything, needs (is going to need) either or both of these set earlier. Which isn't to say it's wrong to do early what can be done early, just that ... > --- a/xen/arch/x86/boot/head.S > +++ b/xen/arch/x86/boot/head.S > @@ -259,6 +259,11 @@ __efi64_mb2_start: > jmp x86_32_switch > > .Lefi_multiboot2_proto: > + /* Save Xen image load base address for later use. */ > + lea __image_base__(%rip),%rsi > + movq %rsi, xen_phys_start(%rip) > + movl %esi, trampoline_xen_phys_start(%rip) ... this path is EFI only if I'm not mistaken, while ... > @@ -605,10 +610,6 @@ trampoline_setup: > * Called on legacy BIOS and EFI platforms. > */ > > - /* Save Xen image load base address for later use. */ > - mov %esi, sym_esi(xen_phys_start) > - mov %esi, sym_esi(trampoline_xen_phys_start) ... the comment in context is pretty clear about this code also being used in the non-EFI case. It is, however, the case that %esi is 0 in that case. Yet surely you want to mention this in the description, to clarify the correctness of the change. Also in the code you move please consistently omit insn suffixes when they're not needed. Just like it was in the original code, and just like you already omit the q from "lea". Finally, if you used a register other than %rsi (say %r14) you could replace the "lea" after x86_32_switch by a 2nd "mov", similar to the one that's already there to load %edi. (You'd need to move the new code up by yet a few more lines, to cover the jump to x86_32_switch there, too.) Jan
On Thu, Aug 8, 2024 at 9:25 AM Jan Beulich <jbeulich@suse.com> wrote: > > On 07.08.2024 15:48, Alejandro Vallejo wrote: > > No reason to wait, if Xen image is loaded by EFI (not multiboot > > EFI path) these are set in efi_arch_load_addr_check, but > > not in the multiboot EFI code path. > > This change makes the 2 code paths more similar and allows > > the usage of these variables if needed. > > I'm afraid I'm struggling with any "similarity" argument here. Imo it > would be better what, if anything, needs (is going to need) either or > both of these set earlier. Which isn't to say it's wrong to do early > what can be done early, just that ... > About similarity is that some part of EFI code expect xen_phys_start to be initialized so this change make sure that if in the future these paths are called even for this case they won't break. > > --- a/xen/arch/x86/boot/head.S > > +++ b/xen/arch/x86/boot/head.S > > @@ -259,6 +259,11 @@ __efi64_mb2_start: > > jmp x86_32_switch > > > > .Lefi_multiboot2_proto: > > + /* Save Xen image load base address for later use. */ > > + lea __image_base__(%rip),%rsi > > + movq %rsi, xen_phys_start(%rip) > > + movl %esi, trampoline_xen_phys_start(%rip) > > ... this path is EFI only if I'm not mistaken, while ... > > > @@ -605,10 +610,6 @@ trampoline_setup: > > * Called on legacy BIOS and EFI platforms. > > */ > > > > - /* Save Xen image load base address for later use. */ > > - mov %esi, sym_esi(xen_phys_start) > > - mov %esi, sym_esi(trampoline_xen_phys_start) > > ... the comment in context is pretty clear about this code also being > used in the non-EFI case. It is, however, the case that %esi is 0 in > that case. Yet surely you want to mention this in the description, to > clarify the correctness of the change. Restored this code. > > Also in the code you move please consistently omit insn suffixes when > they're not needed. Just like it was in the original code, and just > like you already omit the q from "lea". > Done > Finally, if you used a register other than %rsi (say %r14) you could > replace the "lea" after x86_32_switch by a 2nd "mov", similar to the > one that's already there to load %edi. (You'd need to move the new > code up by yet a few more lines, to cover the jump to x86_32_switch > there, too.) > IMHO it makes code less readable, it's hard to understand which registers are in use or not, I prefer to compute one more time instead, this code is not in an hard path and it's going to be discarded after initialization. > Jan Frediano
On 09.08.2024 14:48, Frediano Ziglio wrote: > On Thu, Aug 8, 2024 at 9:25 AM Jan Beulich <jbeulich@suse.com> wrote: >> On 07.08.2024 15:48, Alejandro Vallejo wrote: >>> No reason to wait, if Xen image is loaded by EFI (not multiboot >>> EFI path) these are set in efi_arch_load_addr_check, but >>> not in the multiboot EFI code path. >>> This change makes the 2 code paths more similar and allows >>> the usage of these variables if needed. >> >> I'm afraid I'm struggling with any "similarity" argument here. Imo it >> would be better what, if anything, needs (is going to need) either or >> both of these set earlier. Which isn't to say it's wrong to do early >> what can be done early, just that ... >> > > About similarity is that some part of EFI code expect xen_phys_start > to be initialized so this change make sure that if in the future these > paths are called even for this case they won't break. > >>> --- a/xen/arch/x86/boot/head.S >>> +++ b/xen/arch/x86/boot/head.S >>> @@ -259,6 +259,11 @@ __efi64_mb2_start: >>> jmp x86_32_switch >>> >>> .Lefi_multiboot2_proto: >>> + /* Save Xen image load base address for later use. */ >>> + lea __image_base__(%rip),%rsi >>> + movq %rsi, xen_phys_start(%rip) >>> + movl %esi, trampoline_xen_phys_start(%rip) >> >> ... this path is EFI only if I'm not mistaken, while ... >> >>> @@ -605,10 +610,6 @@ trampoline_setup: >>> * Called on legacy BIOS and EFI platforms. >>> */ >>> >>> - /* Save Xen image load base address for later use. */ >>> - mov %esi, sym_esi(xen_phys_start) >>> - mov %esi, sym_esi(trampoline_xen_phys_start) >> >> ... the comment in context is pretty clear about this code also being >> used in the non-EFI case. It is, however, the case that %esi is 0 in >> that case. Yet surely you want to mention this in the description, to >> clarify the correctness of the change. > > Restored this code. Was my analysis wrong then and it's actually needed for some specific case? Jan
On Fri, Aug 9, 2024 at 1:59 PM Jan Beulich <jbeulich@suse.com> wrote: > > On 09.08.2024 14:48, Frediano Ziglio wrote: > > On Thu, Aug 8, 2024 at 9:25 AM Jan Beulich <jbeulich@suse.com> wrote: > >> On 07.08.2024 15:48, Alejandro Vallejo wrote: > >>> No reason to wait, if Xen image is loaded by EFI (not multiboot > >>> EFI path) these are set in efi_arch_load_addr_check, but > >>> not in the multiboot EFI code path. > >>> This change makes the 2 code paths more similar and allows > >>> the usage of these variables if needed. > >> > >> I'm afraid I'm struggling with any "similarity" argument here. Imo it > >> would be better what, if anything, needs (is going to need) either or > >> both of these set earlier. Which isn't to say it's wrong to do early > >> what can be done early, just that ... > >> > > > > About similarity is that some part of EFI code expect xen_phys_start > > to be initialized so this change make sure that if in the future these > > paths are called even for this case they won't break. > > > >>> --- a/xen/arch/x86/boot/head.S > >>> +++ b/xen/arch/x86/boot/head.S > >>> @@ -259,6 +259,11 @@ __efi64_mb2_start: > >>> jmp x86_32_switch > >>> > >>> .Lefi_multiboot2_proto: > >>> + /* Save Xen image load base address for later use. */ > >>> + lea __image_base__(%rip),%rsi > >>> + movq %rsi, xen_phys_start(%rip) > >>> + movl %esi, trampoline_xen_phys_start(%rip) > >> > >> ... this path is EFI only if I'm not mistaken, while ... > >> > >>> @@ -605,10 +610,6 @@ trampoline_setup: > >>> * Called on legacy BIOS and EFI platforms. > >>> */ > >>> > >>> - /* Save Xen image load base address for later use. */ > >>> - mov %esi, sym_esi(xen_phys_start) > >>> - mov %esi, sym_esi(trampoline_xen_phys_start) > >> > >> ... the comment in context is pretty clear about this code also being > >> used in the non-EFI case. It is, however, the case that %esi is 0 in > >> that case. Yet surely you want to mention this in the description, to > >> clarify the correctness of the change. > > > > Restored this code. > > Was my analysis wrong then and it's actually needed for some specific > case? > Not clear to what exactly you are referring. That later part of code (which was removed) is still needed in case of no-EFI. > Jan Frediano
On 09.08.2024 15:50, Frediano Ziglio wrote: > On Fri, Aug 9, 2024 at 1:59 PM Jan Beulich <jbeulich@suse.com> wrote: >> >> On 09.08.2024 14:48, Frediano Ziglio wrote: >>> On Thu, Aug 8, 2024 at 9:25 AM Jan Beulich <jbeulich@suse.com> wrote: >>>> On 07.08.2024 15:48, Alejandro Vallejo wrote: >>>>> No reason to wait, if Xen image is loaded by EFI (not multiboot >>>>> EFI path) these are set in efi_arch_load_addr_check, but >>>>> not in the multiboot EFI code path. >>>>> This change makes the 2 code paths more similar and allows >>>>> the usage of these variables if needed. >>>> >>>> I'm afraid I'm struggling with any "similarity" argument here. Imo it >>>> would be better what, if anything, needs (is going to need) either or >>>> both of these set earlier. Which isn't to say it's wrong to do early >>>> what can be done early, just that ... >>>> >>> >>> About similarity is that some part of EFI code expect xen_phys_start >>> to be initialized so this change make sure that if in the future these >>> paths are called even for this case they won't break. >>> >>>>> --- a/xen/arch/x86/boot/head.S >>>>> +++ b/xen/arch/x86/boot/head.S >>>>> @@ -259,6 +259,11 @@ __efi64_mb2_start: >>>>> jmp x86_32_switch >>>>> >>>>> .Lefi_multiboot2_proto: >>>>> + /* Save Xen image load base address for later use. */ >>>>> + lea __image_base__(%rip),%rsi >>>>> + movq %rsi, xen_phys_start(%rip) >>>>> + movl %esi, trampoline_xen_phys_start(%rip) >>>> >>>> ... this path is EFI only if I'm not mistaken, while ... >>>> >>>>> @@ -605,10 +610,6 @@ trampoline_setup: >>>>> * Called on legacy BIOS and EFI platforms. >>>>> */ >>>>> >>>>> - /* Save Xen image load base address for later use. */ >>>>> - mov %esi, sym_esi(xen_phys_start) >>>>> - mov %esi, sym_esi(trampoline_xen_phys_start) >>>> >>>> ... the comment in context is pretty clear about this code also being >>>> used in the non-EFI case. It is, however, the case that %esi is 0 in >>>> that case. Yet surely you want to mention this in the description, to >>>> clarify the correctness of the change. >>> >>> Restored this code. >> >> Was my analysis wrong then and it's actually needed for some specific >> case? > > Not clear to what exactly you are referring. > That later part of code (which was removed) is still needed in case of no-EFI. Is it? Under what conditions would %esi be non-zero? As indicated by my earlier reply, I think it would never be. In which case the two stores are pointless. Jan
On Fri, Aug 9, 2024 at 3:02 PM Jan Beulich <jbeulich@suse.com> wrote: > > On 09.08.2024 15:50, Frediano Ziglio wrote: > > On Fri, Aug 9, 2024 at 1:59 PM Jan Beulich <jbeulich@suse.com> wrote: > >> > >> On 09.08.2024 14:48, Frediano Ziglio wrote: > >>> On Thu, Aug 8, 2024 at 9:25 AM Jan Beulich <jbeulich@suse.com> wrote: > >>>> On 07.08.2024 15:48, Alejandro Vallejo wrote: > >>>>> No reason to wait, if Xen image is loaded by EFI (not multiboot > >>>>> EFI path) these are set in efi_arch_load_addr_check, but > >>>>> not in the multiboot EFI code path. > >>>>> This change makes the 2 code paths more similar and allows > >>>>> the usage of these variables if needed. > >>>> > >>>> I'm afraid I'm struggling with any "similarity" argument here. Imo it > >>>> would be better what, if anything, needs (is going to need) either or > >>>> both of these set earlier. Which isn't to say it's wrong to do early > >>>> what can be done early, just that ... > >>>> > >>> > >>> About similarity is that some part of EFI code expect xen_phys_start > >>> to be initialized so this change make sure that if in the future these > >>> paths are called even for this case they won't break. > >>> > >>>>> --- a/xen/arch/x86/boot/head.S > >>>>> +++ b/xen/arch/x86/boot/head.S > >>>>> @@ -259,6 +259,11 @@ __efi64_mb2_start: > >>>>> jmp x86_32_switch > >>>>> > >>>>> .Lefi_multiboot2_proto: > >>>>> + /* Save Xen image load base address for later use. */ > >>>>> + lea __image_base__(%rip),%rsi > >>>>> + movq %rsi, xen_phys_start(%rip) > >>>>> + movl %esi, trampoline_xen_phys_start(%rip) > >>>> > >>>> ... this path is EFI only if I'm not mistaken, while ... > >>>> > >>>>> @@ -605,10 +610,6 @@ trampoline_setup: > >>>>> * Called on legacy BIOS and EFI platforms. > >>>>> */ > >>>>> > >>>>> - /* Save Xen image load base address for later use. */ > >>>>> - mov %esi, sym_esi(xen_phys_start) > >>>>> - mov %esi, sym_esi(trampoline_xen_phys_start) > >>>> > >>>> ... the comment in context is pretty clear about this code also being > >>>> used in the non-EFI case. It is, however, the case that %esi is 0 in > >>>> that case. Yet surely you want to mention this in the description, to > >>>> clarify the correctness of the change. > >>> > >>> Restored this code. > >> > >> Was my analysis wrong then and it's actually needed for some specific > >> case? > > > > Not clear to what exactly you are referring. > > That later part of code (which was removed) is still needed in case of no-EFI. > > Is it? Under what conditions would %esi be non-zero? As indicated by my earlier > reply, I think it would never be. In which case the two stores are pointless. > I really don't follow, %esi at that point should be the address where the executable is loader, which should not be zero. > Jan Frediano
On 09.08.2024 16:34, Frediano Ziglio wrote: > On Fri, Aug 9, 2024 at 3:02 PM Jan Beulich <jbeulich@suse.com> wrote: >> >> On 09.08.2024 15:50, Frediano Ziglio wrote: >>> On Fri, Aug 9, 2024 at 1:59 PM Jan Beulich <jbeulich@suse.com> wrote: >>>> >>>> On 09.08.2024 14:48, Frediano Ziglio wrote: >>>>> On Thu, Aug 8, 2024 at 9:25 AM Jan Beulich <jbeulich@suse.com> wrote: >>>>>> On 07.08.2024 15:48, Alejandro Vallejo wrote: >>>>>>> No reason to wait, if Xen image is loaded by EFI (not multiboot >>>>>>> EFI path) these are set in efi_arch_load_addr_check, but >>>>>>> not in the multiboot EFI code path. >>>>>>> This change makes the 2 code paths more similar and allows >>>>>>> the usage of these variables if needed. >>>>>> >>>>>> I'm afraid I'm struggling with any "similarity" argument here. Imo it >>>>>> would be better what, if anything, needs (is going to need) either or >>>>>> both of these set earlier. Which isn't to say it's wrong to do early >>>>>> what can be done early, just that ... >>>>>> >>>>> >>>>> About similarity is that some part of EFI code expect xen_phys_start >>>>> to be initialized so this change make sure that if in the future these >>>>> paths are called even for this case they won't break. >>>>> >>>>>>> --- a/xen/arch/x86/boot/head.S >>>>>>> +++ b/xen/arch/x86/boot/head.S >>>>>>> @@ -259,6 +259,11 @@ __efi64_mb2_start: >>>>>>> jmp x86_32_switch >>>>>>> >>>>>>> .Lefi_multiboot2_proto: >>>>>>> + /* Save Xen image load base address for later use. */ >>>>>>> + lea __image_base__(%rip),%rsi >>>>>>> + movq %rsi, xen_phys_start(%rip) >>>>>>> + movl %esi, trampoline_xen_phys_start(%rip) >>>>>> >>>>>> ... this path is EFI only if I'm not mistaken, while ... >>>>>> >>>>>>> @@ -605,10 +610,6 @@ trampoline_setup: >>>>>>> * Called on legacy BIOS and EFI platforms. >>>>>>> */ >>>>>>> >>>>>>> - /* Save Xen image load base address for later use. */ >>>>>>> - mov %esi, sym_esi(xen_phys_start) >>>>>>> - mov %esi, sym_esi(trampoline_xen_phys_start) >>>>>> >>>>>> ... the comment in context is pretty clear about this code also being >>>>>> used in the non-EFI case. It is, however, the case that %esi is 0 in >>>>>> that case. Yet surely you want to mention this in the description, to >>>>>> clarify the correctness of the change. >>>>> >>>>> Restored this code. >>>> >>>> Was my analysis wrong then and it's actually needed for some specific >>>> case? >>> >>> Not clear to what exactly you are referring. >>> That later part of code (which was removed) is still needed in case of no-EFI. >> >> Is it? Under what conditions would %esi be non-zero? As indicated by my earlier >> reply, I think it would never be. In which case the two stores are pointless. > > I really don't follow, %esi at that point should be the address where > the executable is loader, which should not be zero. In the PVH entry point it'll be, but else? Note this code in setup.c: /* Is the region suitable for relocating Xen? */ if ( !xen_phys_start && e <= limit ) That relocating of Xen wouldn't happen if we stored a non-zero value in the default (xen.gz with grub1/2) case. Also take a look at Xen before the EFI/MB2 path was added. xen_phys_start wasn't even written from head.S at that time. And if it's for the PVH entry point alone, that code then would want moving into the CONFIG_PVH_GUEST section (if at all possible). Or, if the reason for the change really is "just in case", another option of course is to leave these two insn in the one central place they are at right now. Jan
On Mon, Aug 12, 2024 at 9:41 AM Jan Beulich <jbeulich@suse.com> wrote: > > On 09.08.2024 16:34, Frediano Ziglio wrote: > > On Fri, Aug 9, 2024 at 3:02 PM Jan Beulich <jbeulich@suse.com> wrote: > >> > >> On 09.08.2024 15:50, Frediano Ziglio wrote: > >>> On Fri, Aug 9, 2024 at 1:59 PM Jan Beulich <jbeulich@suse.com> wrote: > >>>> > >>>> On 09.08.2024 14:48, Frediano Ziglio wrote: > >>>>> On Thu, Aug 8, 2024 at 9:25 AM Jan Beulich <jbeulich@suse.com> wrote: > >>>>>> On 07.08.2024 15:48, Alejandro Vallejo wrote: > >>>>>>> No reason to wait, if Xen image is loaded by EFI (not multiboot > >>>>>>> EFI path) these are set in efi_arch_load_addr_check, but > >>>>>>> not in the multiboot EFI code path. > >>>>>>> This change makes the 2 code paths more similar and allows > >>>>>>> the usage of these variables if needed. > >>>>>> > >>>>>> I'm afraid I'm struggling with any "similarity" argument here. Imo it > >>>>>> would be better what, if anything, needs (is going to need) either or > >>>>>> both of these set earlier. Which isn't to say it's wrong to do early > >>>>>> what can be done early, just that ... > >>>>>> > >>>>> > >>>>> About similarity is that some part of EFI code expect xen_phys_start > >>>>> to be initialized so this change make sure that if in the future these > >>>>> paths are called even for this case they won't break. > >>>>> > >>>>>>> --- a/xen/arch/x86/boot/head.S > >>>>>>> +++ b/xen/arch/x86/boot/head.S > >>>>>>> @@ -259,6 +259,11 @@ __efi64_mb2_start: > >>>>>>> jmp x86_32_switch > >>>>>>> > >>>>>>> .Lefi_multiboot2_proto: > >>>>>>> + /* Save Xen image load base address for later use. */ > >>>>>>> + lea __image_base__(%rip),%rsi > >>>>>>> + movq %rsi, xen_phys_start(%rip) > >>>>>>> + movl %esi, trampoline_xen_phys_start(%rip) > >>>>>> > >>>>>> ... this path is EFI only if I'm not mistaken, while ... > >>>>>> > >>>>>>> @@ -605,10 +610,6 @@ trampoline_setup: > >>>>>>> * Called on legacy BIOS and EFI platforms. > >>>>>>> */ > >>>>>>> > >>>>>>> - /* Save Xen image load base address for later use. */ > >>>>>>> - mov %esi, sym_esi(xen_phys_start) > >>>>>>> - mov %esi, sym_esi(trampoline_xen_phys_start) > >>>>>> > >>>>>> ... the comment in context is pretty clear about this code also being > >>>>>> used in the non-EFI case. It is, however, the case that %esi is 0 in > >>>>>> that case. Yet surely you want to mention this in the description, to > >>>>>> clarify the correctness of the change. > >>>>> > >>>>> Restored this code. > >>>> > >>>> Was my analysis wrong then and it's actually needed for some specific > >>>> case? > >>> > >>> Not clear to what exactly you are referring. > >>> That later part of code (which was removed) is still needed in case of no-EFI. > >> > >> Is it? Under what conditions would %esi be non-zero? As indicated by my earlier > >> reply, I think it would never be. In which case the two stores are pointless. > > > > I really don't follow, %esi at that point should be the address where > > the executable is loader, which should not be zero. > > In the PVH entry point it'll be, but else? Note this code in setup.c: > > /* Is the region suitable for relocating Xen? */ > if ( !xen_phys_start && e <= limit ) > > That relocating of Xen wouldn't happen if we stored a non-zero value in > the default (xen.gz with grub1/2) case. Also take a look at Xen before > the EFI/MB2 path was added. xen_phys_start wasn't even written from > head.S at that time. And if it's for the PVH entry point alone, that > code then would want moving into the CONFIG_PVH_GUEST section (if at all > possible). Or, if the reason for the change really is "just in case", > another option of course is to leave these two insn in the one central > place they are at right now. > Hi, as I said I added back the lines in the original place too (I didn't still send that update, I want to finish other changes you suggested). The reason I added these lines is the usage in efi-boot.h, it has nothing to do with PVH. Yes, at the moment that part of the code is executed only on direct EFI program so it's not impacting these paths but better safe than sorry. > Jan Frediano
diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S index 296f76146a..5b82221038 100644 --- a/xen/arch/x86/boot/head.S +++ b/xen/arch/x86/boot/head.S @@ -259,6 +259,11 @@ __efi64_mb2_start: jmp x86_32_switch .Lefi_multiboot2_proto: + /* Save Xen image load base address for later use. */ + lea __image_base__(%rip),%rsi + movq %rsi, xen_phys_start(%rip) + movl %esi, trampoline_xen_phys_start(%rip) + /* Zero EFI SystemTable, EFI ImageHandle addresses and cmdline. */ xor %esi,%esi xor %edi,%edi @@ -605,10 +610,6 @@ trampoline_setup: * Called on legacy BIOS and EFI platforms. */ - /* Save Xen image load base address for later use. */ - mov %esi, sym_esi(xen_phys_start) - mov %esi, sym_esi(trampoline_xen_phys_start) - /* Get bottom-most low-memory stack address. */ mov sym_esi(trampoline_phys), %ecx add $TRAMPOLINE_SPACE,%ecx
No reason to wait, if Xen image is loaded by EFI (not multiboot EFI path) these are set in efi_arch_load_addr_check, but not in the multiboot EFI code path. This change makes the 2 code paths more similar and allows the usage of these variables if needed. Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com> --- xen/arch/x86/boot/head.S | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)