>>> On 19.01.17 at 18:29, <roger....@citrix.com> wrote:
> @@ -1959,12 +1960,146 @@ static int __init pvh_setup_p2m(struct domain *d)
> #undef MB1_PAGES
> }
>
> +static int __init pvh_load_kernel(struct domain *d, const module_t *image,
> + unsigned long image_headroom,
> + module_t *initrd, char *image_base,
> + char *cmdline, paddr_t *entry,
> + paddr_t *start_info_addr)
> +{
> + char *image_start = image_base + image_headroom;
While for cmdline plain char is certainly fine, I think we should stop
abusing this type for image and image_base, even if this means
some further adjustments elsewhere. This really ought to be u8 or
unsigned char.
> + unsigned long image_len = image->mod_end;
> + struct elf_binary elf;
> + struct elf_dom_parms parms;
> + paddr_t last_addr;
> + struct hvm_start_info start_info;
> + struct hvm_modlist_entry mod = { 0 };
> + struct vcpu *saved_current, *v = d->vcpu[0];
> + int rc;
> +
> + if ( (rc = bzimage_parse(image_base, &image_start, &image_len)) != 0 )
> + {
> + printk("Error trying to detect bz compressed kernel\n");
> + return rc;
> + }
> +
> + if ( (rc = elf_init(&elf, image_start, image_len)) != 0 )
> + {
> + printk("Unable to init ELF\n");
> + return rc;
> + }
> +#ifdef VERBOSE
> + elf_set_verbose(&elf);
> +#endif
> + elf_parse_binary(&elf);
> + if ( (rc = elf_xen_parse(&elf, &parms)) != 0 )
> + {
> + printk("Unable to parse kernel for ELFNOTES\n");
> + return rc;
> + }
> +
> + if ( parms.phys_entry == UNSET_ADDR32 ) {
Please move the brace to its own line.
> + printk("Unable to find XEN_ELFNOTE_PHYS32_ENTRY address\n");
> + return -EINVAL;
> + }
> +
> + printk("OS: %s version: %s loader: %s bitness: %s\n", parms.guest_os,
> + parms.guest_ver, parms.loader,
> + elf_64bit(&elf) ? "64-bit" : "32-bit");
> +
> + /* Copy the OS image and free temporary buffer. */
> + elf.dest_base = (void *)(parms.virt_kstart - parms.virt_base);
> + elf.dest_size = parms.virt_kend - parms.virt_kstart;
> +
> + /*
> + * NB: we need to set vCPU 0 of Dom0 as the current vCPU (instead of the
> + * idle one) because elf_load_binary calls raw_{copy_to/clear}_guest, and
> + * the target domain is not passed anywhere. This is very similar to what
> + * is done during classic PV Dom0 creation, where Xen switches to the
> Dom0
> + * page tables. We also cannot pass a struct domain or vcpu to
> + * elf_load_binary, since the interface is shared with the toolstack, and
> + * it doesn't have any notion of a domain or vcpu struct.
> + */
> + saved_current = current;
> + set_current(v);
> + rc = elf_load_binary(&elf);
> + set_current(saved_current);
While with the comment it's not as bad anymore, I'm still not happy
with this code. The tool stack portion of the explanation in particular
does not really hold: We would add an opaque void * parameter to
the functions involved, which the tool stack could pass NULL for, and
you'd use it to convey the vcpu.
> + if ( rc < 0 )
> + {
> + printk("Failed to load kernel: %d\n", rc);
> + printk("Xen dom0 kernel broken ELF: %s\n", elf_check_broken(&elf));
> + return rc;
> + }
> +
> + last_addr = ROUNDUP(parms.virt_kend - parms.virt_base, PAGE_SIZE);
> +
> + if ( initrd != NULL )
> + {
> + rc = hvm_copy_to_guest_phys_vcpu(last_addr,
> + mfn_to_virt(initrd->mod_start),
> + initrd->mod_end, v);
> + if ( rc )
> + {
> + printk("Unable to copy initrd to guest\n");
> + return rc;
> + }
> +
> + mod.paddr = last_addr;
> + mod.size = initrd->mod_end;
> + last_addr += ROUNDUP(initrd->mod_end, PAGE_SIZE);
> + }
> +
> + /* Free temporary buffers. */
> + discard_initial_images();
> +
> + memset(&start_info, 0, sizeof(start_info));
You clear "mod" with an initializer (which btw could be just the two
braces), but you memset() start_info - please be consistent.
> + if ( cmdline != NULL )
> + {
> + rc = hvm_copy_to_guest_phys_vcpu(last_addr, cmdline,
> + strlen(cmdline) + 1, v);
> + if ( rc )
> + {
> + printk("Unable to copy guest command line\n");
> + return rc;
> + }
> + start_info.cmdline_paddr = last_addr;
> + last_addr += ROUNDUP(strlen(cmdline) + 1, 8);
Where is this 8 coming from?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
