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

[Boris Ostrovsky]

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