[Lguest] [PATCH v2 1/3] lguest: move the initial guest page table creation code to the host
Matias Zabaljauregui
zabaljauregui at gmail.com
Tue Oct 21 04:19:58 EST 2008
Hi Rusty, thanks a lot for your reviews and fixes. I really appreciate
it (i know you are having a busy time in RL).
Matias
2008/10/20 Rusty Russell <rusty at rustcorp.com.au>:
> On Monday 29 September 2008 14:39:55 Matias Zabaljauregui wrote:
>> This patch moves the initial guest page table creation code to the host,
>> so the launcher keeps working with PAE enabled configs.
>
> OK, applied with some fixes.
>
>> * the base of Guest "physical" memory, the top physical page to allow, the
>> * top level pagetable and the entry point for the Guest. */
>
> Fixed this comment, and similarly the one from the header.
>
>> +static unsigned long setup_pagetables(unsigned long mem,
>> + unsigned long initrd_size,
>> + unsigned long mem_base)
>
> I found it clearer to pass in lg here, and use lg->mem_base in this function.
>
>> +{
>> + pgd_t *pgdir;
>> + pte_t *linear;
>
> These are actually userspace pointers, so changed to:
>
> + pgd_t __user *pgdir;
> + pte_t __user *linear;
>
>> + /* Linear mapping is easy: put every page's address into the
>> + * mapping in order. */
>> + for (i = 0; i < mapped_pages; i++)
>> + set_pte(&linear[i], pfn_pte(i,
>> + __pgprot(_PAGE_PRESENT | _PAGE_RW | _PAGE_USER)));
>
> This direct access to userspace mem is a no-no. Here's what I have now:
>
> + for (i = 0; i < mapped_pages; i++) {
> + pte_t pte;
> + pte = pfn_pte(i, __pgprot(_PAGE_PRESENT|_PAGE_RW|_PAGE_USER));
> + if (copy_to_user(&linear[i], &pte, sizeof(pte)) != 0)
> + return -EFAULT;
> + }
>
>> + /* The top level points to the linear page table pages above.
>> + * We setup the identity and linear mappings here. */
>> + phys_linear = (unsigned long)linear - mem_base;
>> + for (i = 0; i < mapped_pages; i += PTRS_PER_PTE)
>> + pgdir[i / PTRS_PER_PTE] =
>> + pgdir[pgd_index(PAGE_OFFSET) + i / PTRS_PER_PTE] =
>> + __pgd((phys_linear + i * sizeof(pte_t)) |
>> + (_PAGE_PRESENT | _PAGE_RW | _PAGE_USER));
>
> Same issue here.
>
> + for (i = 0; i < mapped_pages; i += PTRS_PER_PTE) {
> + pgd_t pgd;
> + pgd = __pgd((phys_linear + i * sizeof(pte_t)) |
> + (_PAGE_PRESENT | _PAGE_RW | _PAGE_USER));
> +
> + if (copy_to_user(&pgdir[i / PTRS_PER_PTE], &pgd, sizeof(pgd))
> + || copy_to_user(&pgdir[pgd_index(PAGE_OFFSET)
> + + i / PTRS_PER_PTE],
> + &pgd, sizeof(pgd)))
> + return -EFAULT;
> + }
> +
>
>> + unsigned long initrd_size = 0, mem = 0;
>> + struct boot_params *boot = (struct boot_params *) lg->mem_base;
>> +
>> + /* Get the guest memory size and the ramdisk size
>> + * from the boot header located at lg->mem_base*/
>> + if (copy_from_user(&mem, &boot->e820_map[0].size, 8))
>> + kill_guest(&lg->cpus[0], "Error reading from user space");
>
> This 8 here caused a stack issue (lg got set to NULL in the caller function)!
> I changed mem to a u64.
>
>> + lg->pgdirs[0].gpgdir = setup_pagetables(mem,
>> + initrd_size, (unsigned long) lg->mem_base);
>
> This can now error, so I did:
>
> + lg->pgdirs[0].gpgdir = setup_pagetables(lg, mem, initrd_size);
> + if (IS_ERR_VALUE(lg->pgdirs[0].gpgdir))
> + return lg->pgdirs[0].gpgdir;
>
> Cheers!
> Rusty.
>
More information about the Lguest
mailing list