Re: [PATCH 05/10] kexec: extend hypercall with improved load/unload ops

[David Vrabel <david.vrabel@citrix.com>]

On 25/06/13 09:31, Jan Beulich wrote:
>>>> On 24.06.13 at 19:42, David Vrabel <david.vrabel@citrix.com> wrote:
>> 
>> +static int init_transition_pgtable(struct kexec_image *image, l4_pgentry_t *l4)
[...]
>> +    l3 = __map_domain_page(l3_page);
>> +    l3 += l3_table_offset(vaddr);
>> +    if ( !(l3e_get_flags(*l3) & _PAGE_PRESENT) )
>> +    {
>> +        l2_page = kimage_alloc_control_page(image, 0);
>> +        if ( !l2_page )
>> +            goto out;
>> +        l3e_write(l3, l3e_from_page(l2_page, __PAGE_HYPERVISOR));
>> +    }
>> +    else
>> +        l2_page = l3e_get_page(*l3);
> 
> Afaict you're done using "l3" here, so you should unmap it in order
> to reduce the pressure on the domain page mapping resources.

The unmaps are grouped at the end to make the error paths simpler and I
would prefer to keep it like this.  This is only using 4 entries.  Are
we really that short?

>> +static int build_reloc_page_table(struct kexec_image *image)
>> +{
>> +    struct page_info *l4_page;
>> +    l4_pgentry_t *l4;
>> +    int result;
>> +
>> +    l4_page = kimage_alloc_control_page(image, 0);
>> +    if ( !l4_page )
>> +        return -ENOMEM;
>> +    l4 = __map_domain_page(l4_page);
>> +
>> +    result = init_level4_page(image, l4, 0, max_page << PAGE_SHIFT);
> 
> What about holes in the physical address space - not just the
> MMIO hole below 4Gb is a problem here, but also discontiguous
> physical memory.

I don't see a problem with creating mappings for non-RAM regions.

The discontiguous physical memory is a problem though.  I think I'll
solve this by specifying that images are only executed with the first 4
GiB of physical address space linearly mapped.

If this turns out not to be enough then the mappings can be extended
without breaking existing tools or images.

>> --- /dev/null
>> +++ b/xen/arch/x86/x86_64/kexec_reloc.S
[...]
> And just 8 here.

I seem to recall reading that some processors needed 16 byte alignment
for the GDT.  I may be misremembering or this was for an older processor
that Xen no longer supports.

>> +compat_mode_gdt:
>> +        .quad 0x0000000000000000     /* null                              */
>> +        .quad 0x00cf92000000ffff     /* 0x0008 ring 0 data                */
>> +        .quad 0x00cf9a000000ffff     /* 0x0010 ring 0 code, compatibility */
[...]
>> +        /*
>> +         * 16 words of stack are more than enough.
>> +         */
>> +        .fill 16,8,0
>> +reloc_stack:
> 
> And now you don't care for the stack being mis-aligned?

I do find the way you make some review comments as a question like this
rather ambiguous.  I guess I don't care? But now I'm not sure if I should.

David