[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v14 4/9] x86: add multiboot2 protocol support for EFI platforms
On Mon, Feb 06, 2017 at 03:18:53AM -0700, Jan Beulich wrote: > >>> On 02.02.17 at 23:01, <daniel.kiper@xxxxxxxxxx> wrote: > > This way Xen can be loaded on EFI platforms using GRUB2 and > > other boot loaders which support multiboot2 protocol. > > > > Signed-off-by: Daniel Kiper <daniel.kiper@xxxxxxxxxx> > > --- > > v14 - suggestions/fixes: > > - mark .init.data section as writable; by the way we must change > > similar definition in xen/arch/x86/boot/x86_64.S because otherwise > > compiler complains about section types conflicts > > Did you observe a problem here, or was this just based on memory > from having seen such in other cases? I ask because I doubt there It happened here. Though I am not able to reproduce it now... Hmmm... > would be any problem here, as the compiler isn't involved: This is > assembly code, and iirc you validly talk about a compiler diagnostic. > Hence the wrong attributes want correcting, but in a separate > patch. OK. > > .section .init.text, "ax", @progbits > > > > bad_cpu: > > mov $(sym_phys(.Lbad_cpu_msg)),%esi # Error message > > - jmp print_err > > + jmp .Lget_vtb > > not_multiboot: > > mov $(sym_phys(.Lbad_ldr_msg)),%esi # Error message > > -print_err: > > - mov $0xB8000,%edi # VGA framebuffer > > -1: mov (%esi),%bl > > + jmp .Lget_vtb > > +.Lmb2_no_st: > > + mov $(sym_phys(.Lbad_ldr_nst)),%esi # Error message > > + jmp .Lget_vtb > > +.Lmb2_no_ih: > > + mov $(sym_phys(.Lbad_ldr_nih)),%esi # Error message > > + jmp .Lget_vtb > > +.Lmb2_no_bs: > > + mov $(sym_phys(.Lbad_ldr_nbs)),%esi # Error message > > + xor %edi,%edi # No VGA text buffer > > + jmp .Lsend_chr > > +.Lmb2_efi_ia_32: > > + mov $(sym_phys(.Lbad_efi_msg)),%esi # Error message > > + xor %edi,%edi # No VGA text buffer > > + jmp .Lsend_chr > > I continue to have problems to understand when you use / don't use > the VGA buffer here: I think at the very least we want a comment as > to when which of the two models applies. The situation isn't being > helped by .Lmb2_no_bs reachable via two paths (one neighboring a > use of .Lmb2_efi_ia_32, and another neighboring uses of > .Lmb2_no_st and .Lmb2_no_ih). Since you zap vga_text_buffer in > the EFI case, couldn't you simply use that one instead of clearing > %edi here? Here it is much simpler to do. Otherwise we must move jumps to .Lmb2_no_bs and .Lmb2_efi_ia_32 behind Multiboot2 scanning loop. Earlier we could not have MB2_load_base_addr value which is needed to calculate real vga_text_buffer address in 32-bit mode. Ahhh... This is not obvious when you look at this patch alone because MB2_load_base_addr is added by "x86: add multiboot2 protocol support for relocatable images" patch. > > +void __init noreturn efi_multiboot2(EFI_HANDLE ImageHandle, > > + EFI_SYSTEM_TABLE *SystemTable) > > +{ > > + static const CHAR16 __initconst err[] = > > + L"Xen does not have EFI code build in!\r\nSystem halted!\r\n"; > > + SIMPLE_TEXT_OUTPUT_INTERFACE *StdErr; > > + > > + StdErr = SystemTable->StdErr ? SystemTable->StdErr : > > SystemTable->ConOut; > > + > > + /* > > + * Print error message and halt the system. > > + * > > + * We have to open code MS x64 calling convention > > + * in assembly because here this convention may > > + * not be directly supported by C compiler. > > + */ > > + asm volatile( > > + " call *%2 \n" > > + "0: hlt \n" > > + " jmp 0b \n" > > + : "+c" (StdErr) : "d" (err), "rm" (StdErr->OutputString) > > + : "rax", "r8", "r9", "r10", "r11", "memory"); > > Regardless of the hlt I think this would better be correct: As done > in various other places, you simply need to indicate "=d" as a > (dummy) output; since you clobber StdErr anyway, you could > simply use that one: > > asm volatile( > " call *%2 \n" > "0: hlt \n" > " jmp 0b \n" > : "+c" (StdErr), "=d" (StdErr) : "1" (err), "rm" (StdErr->OutputString) > : "rax", "r8", "r9", "r10", "r11", "memory"); > > Otherwise I'd ask for consistency, i.e. for %rcx to be an input only, > too. OK, I will try. Daniel _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx https://lists.xen.org/xen-devel
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |