Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756213AbcJNTeJ convert rfc822-to-8bit (ORCPT ); Fri, 14 Oct 2016 15:34:09 -0400 Received: from userp1040.oracle.com ([156.151.31.81]:51969 "EHLO userp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753709AbcJNTeI (ORCPT ); Fri, 14 Oct 2016 15:34:08 -0400 Subject: Re: [Xen-devel] [PATCH 4/8] xen/pvh: Bootstrap PVH guest To: Konrad Rzeszutek Wilk References: <1476468318-24422-1-git-send-email-boris.ostrovsky@oracle.com> <1476468318-24422-5-git-send-email-boris.ostrovsky@oracle.com> <20161014191403.GA16777@localhost.localdomain> Cc: david.vrabel@citrix.com, JGross@suse.com, Matt Fleming , xen-devel@lists.xenproject.org, linux-kernel@vger.kernel.org, roger.pau@citrix.com From: Boris Ostrovsky Message-ID: Date: Fri, 14 Oct 2016 15:34:45 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.3.0 MIME-Version: 1.0 In-Reply-To: <20161014191403.GA16777@localhost.localdomain> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8BIT X-Source-IP: userv0021.oracle.com [156.151.31.71] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2155 Lines: 97 On 10/14/2016 03:14 PM, Konrad Rzeszutek Wilk wrote: > >> + >> + memset(&pvh_bootparams, 0, sizeof(pvh_bootparams)); >> + >> + memmap.nr_entries = ARRAY_SIZE(pvh_bootparams.e820_map); >> + set_xen_guest_handle(memmap.buffer, pvh_bootparams.e820_map); >> + if (HYPERVISOR_memory_op(XENMEM_memory_map, &memmap)) { >> + xen_raw_console_write("XENMEM_memory_map failed\n"); > Should we print the error value at least? I will have to check again but IIRC there was something about not being able to format strings properly this early. But if we can --- sure. >> + BUG(); >> + } >> + >> + pvh_bootparams.e820_map[memmap.nr_entries].addr = >> + ISA_START_ADDRESS; > What if nr_entries is 128? Should we double-check for that? > OK. >> + */ >> +void __init xen_prepare_pvh(void) >> +{ >> + u32 eax, ecx, edx, msr; > msr = 0 ? Won't cpuid() (or cpuid_ebx()) overwrite it anyway? >> + u64 pfn; >> + >> + xen_pvh = 1; >> + >> + cpuid(xen_cpuid_base() + 2, &eax, &msr, &ecx, &edx); > cpuid_ebx ? And that way you don't have have ecx and edx? >> + cli >> + cld >> + >> + mov $_pa(gdt), %eax >> + lgdt (%eax) >> + >> + movl $(__BOOT_DS),%eax >> + movl %eax,%ds >> + movl %eax,%es >> + movl %eax,%ss >> + >> + /* Stash hvm_start_info */ >> + mov $_pa(pvh_start_info), %edi >> + mov %ebx, %esi > Should we derference the first byte or such to check for the magic > string? Actually I am not even seeing the check in the C code? Yes, good idea. >> + .code64 >> +1: >> + call xen_prepare_pvh >> + >> + /* startup_64 expects boot_params in %rsi */ > .. >> + mov $_pa(pvh_bootparams), %rsi >> + movq $_pa(startup_64), %rax >> + jmp *%rax >> + >> +#else /* CONFIG_X86_64 */ >> + >> + call setup_pgtable_32 >> + >> + mov $_pa(initial_page_table), %eax >> + movl %eax, %cr3 >> + >> + movl %cr0, %eax >> + orl $(X86_CR0_PG | X86_CR0_PE), %eax >> + movl %eax, %cr0 >> + >> + ljmp $__BOOT_CS,$1f >> +1: >> + call xen_prepare_pvh >> + mov $_pa(pvh_bootparams), %esi >> + >> + /* startup_32 doesn't expect paging and PAE to be on */ > Should 'startup_32' be documented with this? It is documented in Documentation/x86/boot.txt and in the startup_64 code. -boris