Re: [Xen-devel] [PATCH v5 4/9] xen/x86: populate PVHv2 Dom0 physical memory map

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 19/01/17 17:29, Roger Pau Monne wrote:
> +static int __init pvh_setup_vmx_realmode_helpers(struct domain *d)
> +{
> +    p2m_type_t p2mt;
> +    uint32_t rc, *ident_pt;
> +    uint8_t *tss;
> +    mfn_t mfn;
> +    paddr_t gaddr;
> +    unsigned int i;
> +
> +    /*
> +     * Steal some space from the last found RAM region. One page will be
> +     * used for the identity page tables, and the remaining space for the
> +     * VM86 TSS. Note that after this not all e820 regions will be aligned
> +     * to PAGE_SIZE.
> +     */
> +    if ( pvh_steal_ram(d, PAGE_SIZE + HVM_VM86_TSS_SIZE, ULONG_MAX, &gaddr) )
> +    {
> +        printk("Unable to find memory to stash the identity map and TSS\n");
> +        return -ENOMEM;
> +    }
> +
> +    /*
> +     * Identity-map page table is required for running with CR0.PG=0
> +     * when using Intel EPT. Create a 32-bit non-PAE page directory of
> +     * superpages.
> +     */
> +    ident_pt = map_domain_gfn(p2m_get_hostp2m(d), _gfn(PFN_DOWN(gaddr)),
> +                              &mfn, &p2mt, 0, &rc);
> +    if ( ident_pt == NULL )
> +    {
> +        printk("Unable to map identity page tables\n");
> +        return -ENOMEM;
> +    }
> +    for ( i = 0; i < PAGE_SIZE / sizeof(*ident_pt); i++ )
> +        ident_pt[i] = ((i << 22) | _PAGE_PRESENT | _PAGE_RW | _PAGE_USER |
> +                       _PAGE_ACCESSED | _PAGE_DIRTY | _PAGE_PSE);

Please can you make helper for this and dedup it with shadow_enable(). 
Something like:

void write_pse_identmap(uint32_t *l2)

rather than duplicating this particular piece of magic.  (It can
probably even be static inline.)

> +    unmap_domain_page(ident_pt);
> +    put_page(mfn_to_page(mfn_x(mfn)));
> +    d->arch.hvm_domain.params[HVM_PARAM_IDENT_PT] = gaddr;
> +    gaddr += PAGE_SIZE;
> +    ASSERT(IS_ALIGNED(gaddr, PAGE_SIZE));
> +
> +    tss = map_domain_gfn(p2m_get_hostp2m(d), _gfn(PFN_DOWN(gaddr)),
> +                         &mfn, &p2mt, 0, &rc);
> +    if ( tss == NULL )
> +    {
> +        printk("Unable to map VM86 TSS area\n");
> +        return 0;
> +    }
> +
> +    memset(tss, 0, HVM_VM86_TSS_SIZE);

Do we actually need to 0 this?  Don't we guarantee to hand out zero'd
pages during construction?  (I can't actually recall.  Perhaps it is
better to explicitly clear it.)

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel