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

From: Boris Ostrovsky
Date: Fri Oct 14 2016 - 15:35:53 EST


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