[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