From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Vrabel Subject: Re: [PATCH 05/10] kexec: extend hypercall with improved load/unload ops Date: Tue, 25 Jun 2013 15:30:21 +0100 Message-ID: <51C9A97D.8050606@citrix.com> References: <1372095741-27012-1-git-send-email-david.vrabel@citrix.com> <1372095741-27012-6-git-send-email-david.vrabel@citrix.com> <51C9718D02000078000E03A0@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <51C9718D02000078000E03A0@nat28.tlf.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: Daniel Kiper , Keir Fraser , David Vrabel , xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On 25/06/13 09:31, Jan Beulich wrote: >>>> On 24.06.13 at 19:42, David Vrabel 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