linux-kernel - Re: [Xen-devel] [PATCH 4/8] xen/pvh: Bootstrap PVH guest

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <fc448a0b-bb35-7383-9e44-ed7a71b3c2d1@oracle.com>
Date:   Fri, 14 Oct 2016 15:34:45 -0400
From:   Boris Ostrovsky <boris.ostrovsky@...cle.com>
To:     Konrad Rzeszutek Wilk <konrad.wilk@...cle.com>
Cc:     david.vrabel@...rix.com, JGross@...e.com,
        Matt Fleming <matt@...eblueprint.co.uk>,
        xen-devel@...ts.xenproject.org, linux-kernel@...r.kernel.org,
        roger.pau@...rix.com
Subject: Re: [Xen-devel] [PATCH 4/8] xen/pvh: Bootstrap PVH guest

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