Re: [Xen-devel] [PATCH v9] x86/hvm: Implement hvmemul_write() using real mappings

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 09.10.17 at 12:56, <aisaila@xxxxxxxxxxxxxxx> wrote:
> +static void *hvmemul_map_linear_addr(
> +    unsigned long linear, unsigned int bytes, uint32_t pfec,
> +    struct hvm_emulate_ctxt *hvmemul_ctxt)
> +{
> +    struct vcpu *curr = current;
> +    void *err, *mapping;
> +    unsigned int nr_frames = ((linear + bytes - !!bytes) >> PAGE_SHIFT) -
> +        (linear >> PAGE_SHIFT) + 1;
> +    unsigned int i;
> +
> +    /*
> +     * mfn points to the next free slot.  All used slots have a page 
> reference
> +     * held on them.
> +     */
> +    mfn_t *mfn = &hvmemul_ctxt->mfn[0];
> +
> +    /*
> +     * The caller has no legitimate reason for trying a zero-byte write, but
> +     * final is calculate to fail safe in release builds.

This comment has become stale too. There's no "final" anymore,
and I'm also unclear what the referral to release builds is connected
with.

> +     * The maximum write size depends on the number of adjacent mfns[] which
> +     * can be vmap()'d, accouting for possible misalignment within the 
> region.
> +     * The higher level emulation callers are responsible for ensuring that
> +     * mfns[] is large enough for the requested write size.
> +     */
> +    if ( bytes == 0 ||
> +         nr_frames > ARRAY_SIZE(hvmemul_ctxt->mfn) )
> +    {
> +        ASSERT_UNREACHABLE();
> +        printk("goto unhandle ERROR~!~~\n");

Stray debugging leftover?

> +        goto unhandleable;
> +    }
> +
> +    for ( i = 0; i < nr_frames; i++ )
> +    {
> +        enum hvm_translation_result res;
> +        struct page_info *page;
> +        pagefault_info_t pfinfo;
> +        p2m_type_t p2mt;
> +        unsigned long addr = i ? (linear + (i << PAGE_SHIFT)) & PAGE_MASK : 
> linear;
> +
> +        if ( hvmemul_ctxt->ctxt.addr_size < 64 )
> +            addr = (uint32_t)addr;
> +
> +        /* Error checking.  Confirm that the current slot is clean. */
> +        ASSERT(mfn_x(*mfn) == 0);
> +
> +        res = hvm_translate_get_page(curr, addr, true, pfec,
> +                                     &pfinfo, &page, NULL, &p2mt);
> +
> +        switch ( res )
> +        {
> +        case HVMTRANS_okay:
> +            break;
> +
> +        case HVMTRANS_bad_linear_to_gfn:
> +            x86_emul_pagefault(pfinfo.ec, pfinfo.linear, 
> &hvmemul_ctxt->ctxt);

Considering the earlier problems with the reported CR2, perhaps
worth adding ASSERT(pfinfo.linear == addr) here?

> +static void hvmemul_unmap_linear_addr(
> +    void *mapping, unsigned long linear, unsigned int bytes,
> +    struct hvm_emulate_ctxt *hvmemul_ctxt)
> +{
> +    struct domain *currd = current->domain;
> +    unsigned int nr_frames = ((linear + bytes - !!bytes) >> PAGE_SHIFT) -
> +        (linear >> PAGE_SHIFT) + 1;
> +    unsigned int i;
> +    mfn_t *mfn = &hvmemul_ctxt->mfn[0];
> +
> +    ASSERT(bytes > 0);
> +
> +    if ( nr_frames == 1 )
> +        unmap_domain_page(mapping);
> +    else
> +        vunmap(mapping);
> +
> +    for ( i = 0; i < nr_frames; i++ )
> +    {
> +        ASSERT(mfn_valid(*mfn));
> +        paging_mark_dirty(currd, *mfn);
> +        put_page(mfn_to_page(mfn_x(*mfn)));
> +
> +        *mfn++ = _mfn(0); /* Clean slot for map()'s error checking. */
> +    }
> +
> +
> +#ifndef NDEBUG /* Check (and clean) all unused mfns. */

Stray double blank lines above here.

I'd be fine taking care of all the comments while committing (and
then adding my R-b), provided you (and ideally also Andrew)
agree, and of course assuming Paul would ack the patch, plus
no-one else finds yet another problem which once again I may
have overlooked.

Jan


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