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

[Roger Pau Monne <roger.pau@xxxxxxxxxx>]

On Fri, Jan 27, 2017 at 08:11:56AM -0700, Jan Beulich wrote:
> >>> On 27.01.17 at 13:23, <roger.pau@xxxxxxxxxx> wrote:
> > On Thu, Jan 26, 2017 at 05:41:58AM -0700, Jan Beulich wrote:
> >> >>> On 19.01.17 at 18:29, <roger.pau@xxxxxxxxxx> wrote:
> >> > @@ -43,6 +44,9 @@ static long __initdata dom0_nrpages;
> >> >  static long __initdata dom0_min_nrpages;
> >> >  static long __initdata dom0_max_nrpages = LONG_MAX;
> >> >  
> >> > +/* Size of the VM86 TSS for virtual 8086 mode to use. */
> >> > +#define HVM_VM86_TSS_SIZE   128
> >> 
> >> I continue to be puzzled by this value. Why 128? I think this really
> >> needs to be clarified in the comment.
> > 
> > Given the recent comments by Tim, and that this is starting to look like a 
> > can
> > of worms, I would like to leave this as-is for the moment, on the grounds 
> > that
> > it's what hvmloader does (I'm not saying it's right), and that this issue
> > should be treated independently from this patch series.
> 
> Well, for the purpose of this patch it would be sufficient if the
> comment referred to hvmloader. But then I think I saw you set the
> TSS limit to 0x67, which is neither in line with the value above nor

Hm, no, I'm not setting the limit anywhere here, this is done in
vmx_set_segment_register, and it's indeed set to 0xff which is wrong for
hvmloader too according to the conversation that's going on related to this
HVM_VM86_TSS_SIZE param.

> - according to what Tim said (but I didn't check myself yet) - the
> 255 used in hvmloader. I.e. if you clone hvmloader code, all
> aspects of it should match.
> 
> > Alternatively, I can just remove setting HVM_PARAM_VM86_TSS for a PVHv2 
> > Dom0.
> > IIRC I've tried that before (without unrestricted mode support) and it was
> > working fine.
> 
> Now if that's the case, then why bother with the TSS?

It seems like it working was just luck, but I don't know all the details. Maybe
the emulator is somehow fixing this up when the TSS is corrupted/incorrect?

> >> > @@ -333,7 +338,9 @@ static unsigned long __init compute_dom0_nr_pages(
> >> >              avail -= max_pdx >> s;
> >> >      }
> >> >  
> >> > -    need_paging = opt_dom0_shadow || (is_pvh_domain(d) && 
> >> > !iommu_hap_pt_share);
> >> > +    need_paging = opt_dom0_shadow ||
> >> > +                  (has_hvm_container_domain(d) && (!iommu_hap_pt_share 
> >> > ||
> >> > +                                                   
> >> > !paging_mode_hap(d)));
> >> 
> >> What is the !paging_mode_hap() part good for? It's being taken care
> >> of by checking opt_dom0_shadow already, isn't it? Alternatively, to
> >> make the distinction more obvious, I'd suggest
> >> 
> >>     need_paging = has_hvm_container_domain(d)
> >>                   ? !iommu_hap_pt_share || !paging_mode_hap(d)
> >>                   : opt_dom0_shadow;
> > 
> > AFAICT it *might* be possible to run a PVHv2 Dom0 on a box with no EPT, but
> > with an IOMMU? Does that exist? In that case opt_dom0_shadow won't be set, 
> > but
> > paging_mode_hap would be false. Maybe that's just an impossible combination 
> > in
> > any case...
> 
> At least when running Xen itself virtualized, I wouldn't dare to assume
> this is an impossible combination. However, I can't see how that case
> would be handled any different by the original or the suggested
> replacement expressions: need_paging would get set either way afaict.

Oh yes, sorry, my reply was to the "What is the !paging_mode_hap() part good
for?" question. I've changed setting need_paging as you suggested.

> > Given the change that you requested in pvh_steal_ram, now the start of the
> > memory area returned by it it's not going to be page-aligned, so I will 
> > have to
> > perform the TSS setup first, and then the identity page tables.
> 
> Or simply pass the required alignment.

Passing an alignment here would mean that pvh_steal_ram would have to return 2
pages in order to meet this alignment, and we would end up wasting memory.
Also, this is the only caller of pvh_steal_ram that requires alignment. This is
what I have after changing pvh_steal_ram to remove RAM from the end of the
region:

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 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, GB(4), &gaddr) )
    {
        printk("Unable to find memory to stash the identity map and TSS\n");
        return -ENOMEM;
    }

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

    gaddr += HVM_VM86_TSS_SIZE;
    ASSERT(IS_ALIGNED(gaddr, PAGE_SIZE));

    /*
     * 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;
    }
    write_32bit_pse_identmap(ident_pt);
    unmap_domain_page(ident_pt);
    put_page(mfn_to_page(mfn_x(mfn)));
    d->arch.hvm_domain.params[HVM_PARAM_IDENT_PT] = gaddr;

    return 0;
}

Roger.

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