Re: [Xen-devel] [PATCH v6 3/7] xen/x86: populate PVHv2 Dom0 physical memory map

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 10.02.17 at 13:33, <roger.pau@xxxxxxxxxx> wrote:
> ---
> Changes since v5:
>  - Adjust the logic to set need_paging.
>  - Remove the usage of the _AC macro.
>  - Subtract memory from the end of regions instead of the start.
>  - Create the VM86_TSS before the identity page table, so that the page table
>    is aligned to a page boundary.
>  - Use MB1_PAGES in modify_identity_mmio.
>  - Move and simply the ASSERT in pvh_setup_p2m.
>  - Move the creation of the PSE page tables to a separate function, and use it
>    in shadow_enable also.
>  - Make the map modify_identiy_mmio parameter a constant.
>  - Add a comment to HVM_VM86_TSS_SIZE, although it seems this might need
>    further fixing.

Indeed, I think this wants to be re-based on top of the patch I've
just sent (with you Cc-ed).

> +static int __init pvh_steal_ram(struct domain *d, unsigned long size,
> +                                unsigned long align, paddr_t limit,
> +                                paddr_t *addr)
> +{
> +    unsigned int i = d->arch.nr_e820;
> +
> +    /*
> +     * Alignment 0 should be set to 1, so it doesn't wrap around in the
> +     * calculations below.
> +     */
> +    align = align ? : 1;
> +    while ( i-- )
> +    {
> +        struct e820entry *entry = &d->arch.e820[i];
> +
> +        if ( entry->type != E820_RAM || entry->addr + entry->size > limit ||
> +             entry->addr < MB(1) )
> +            continue;
> +
> +        *addr = (entry->addr + entry->size - size) & ~(align - 1);

Without taking the present callers into account (who don't request
huge alignment) this (theoretically) risks running into the low 1Mb.
I see two options - either add a comment clarifying that an entry
will never cross the 1Mb boundary (and hence the issue can't
occur), or limit the alignment (by way of ASSERT()) to PAGE_SIZE
(in which case no significant harm would result from crossing the
boundary).

> +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;
> +
> +    /*
> +     * Steal some space from the last RAM region below 4GB and use it to
> +     * store the real-mode TSS.
> +     */
> +    if ( !pvh_steal_ram(d, HVM_VM86_TSS_SIZE, 0, GB(4), &gaddr) &&

Please request at least 128-byte alignment here, to avoid the
base TSS structure crossing a page boundary.

> +         (tss = map_domain_gfn(p2m_get_hostp2m(d), _gfn(PFN_DOWN(gaddr)),
> +                               &mfn, &p2mt, 0, &rc)) )
> +    {
> +        memset(tss, 0, HVM_VM86_TSS_SIZE);

What makes you assume that you've mapped all the space you've
allocated?

> --- a/xen/include/asm-x86/page.h
> +++ b/xen/include/asm-x86/page.h
> @@ -374,6 +374,18 @@ perms_strictly_increased(uint32_t old_flags, uint32_t 
> new_flags)
>      return ((of | (of ^ nf)) == nf);
>  }
>  
> +/* Build a 32bit PSE page table using 4MB pages. */
> +static inline void
> +write_32bit_pse_identmap(uint32_t *l2)

Why does this need to be an inline function?

Jan


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