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

[Alexandru Stefan ISAILA <aisaila@xxxxxxxxxxxxxxx>]

On Lu, 2017-09-18 at 07:43 -0600, Jan Beulich wrote:
> >
> > >
> > > >
> > > > On 08.09.17 at 18:05, <aisaila@xxxxxxxxxxxxxxx> wrote:
> > Changes since V1:
> > - Moved ASSERT to the begining of the loop
> > - Corrected the decrement on mfn int the while statement
> > - Modified the comment to PAGE_SIZE+1
> While several of my v1 comments were taken care of verbally, some
> haven't been addressed here or during the discussion.
Sorry about that, I must have lost some or some emails have not been
indexed. I'll address all from now on.
>
> >
> > While the maximum size of linear mapping is capped at 1 page, the
> > logic
> > in the helpers is written to work properly as hvmemul_ctxt->mfn[]
> > gets
> > longer,
> > specifically with XSAVE instruction emulation in mind.
> >
> > This has only had light testing so far.
> Has this changed in the meantime?
This has not changed so far.
>
> >
> > +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;
> > +
> > +    /* First and final gfns which need mapping. */
> > +    unsigned long frame = linear >> PAGE_SHIFT, first = frame;
> > +    unsigned long final = (linear + bytes - !!bytes) >>
> > PAGE_SHIFT;
> > +
> > +    /*
> > +     * 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.
> > +     *
> > +     * 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 ||
> > +         final - first > ARRAY_SIZE(hvmemul_ctxt->mfn) - 1 )
> > +    {
> > +        ASSERT_UNREACHABLE();
> > +        goto unhandleable;
> > +    }
> > +
> > +    do {
> > +        enum hvm_translation_result res;
> > +        struct page_info *page;
> > +        pagefault_info_t pfinfo;
> > +        p2m_type_t p2mt;
> > +
> > +        /* Error checking.  Confirm that the current slot is
> > clean. */
> > +        ASSERT(mfn_x(*mfn) == 0);
> > +
> > +        res = hvm_translate_get_page(curr, frame << PAGE_SHIFT,
> > 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);
> > +            err = ERR_PTR(~(long)X86EMUL_EXCEPTION);
> Why the casts to long here and further down?
>
> >
> > +            goto out;
> > +
> > +        case HVMTRANS_bad_gfn_to_mfn:
> > +            err = NULL;
> > +            goto out;
> > +
> > +        case HVMTRANS_gfn_paged_out:
> > +        case HVMTRANS_gfn_shared:
> > +            err = ERR_PTR(~(long)X86EMUL_RETRY);
> > +            goto out;
> > +
> > +        default:
> > +            goto unhandleable;
> > +        }
> > +
> > +        *mfn++ = _mfn(page_to_mfn(page));
> > +        frame++;
> > +
> > +        if ( p2m_is_discard_write(p2mt) )
> > +        {
> > +            err = ERR_PTR(~(long)X86EMUL_OKAY);
> > +            goto out;
> > +        }
> > +
> > +    } while ( frame < final );
> > +
> > +    /* Entire access within a single frame? */
> > +    if ( first == final )
> > +        mapping = map_domain_page(hvmemul_ctxt->mfn[0]) + (linear
> > & ~PAGE_MASK);
> > +    /* Multiple frames? Need to vmap(). */
> > +    else if ( (mapping = vmap(hvmemul_ctxt->mfn,
> > +                              mfn - hvmemul_ctxt->mfn)) == NULL )
> v1 comment was "final - first + 1 would likely yield better code."
will do.
>
> >
> > +        goto unhandleable;
> > +
> > +#ifndef NDEBUG /* Poision unused mfn[]s with INVALID_MFN. */
> > +    while ( mfn < hvmemul_ctxt->mfn + ARRAY_SIZE(hvmemul_ctxt-
> > >mfn) )
> > +    {
> > +        ASSERT(mfn_x(*mfn) == 0);
> > +        *mfn++ = INVALID_MFN;
> > +    }
> > +#endif
> > +
> > +    return mapping;
> > +
> > + unhandleable:
> > +    err = ERR_PTR(~(long)X86EMUL_UNHANDLEABLE);
> > +
> > + out:
> > +    /* Drop all held references. */
> > +    while ( mfn-- > hvmemul_ctxt->mfn )
> > +        put_page(mfn_to_page(mfn_x(*mfn)));
> > +
> > +    return err;
> > +}
> > +
> > +static void hvmemul_unmap_linear_addr(
> > +    void *mapping, unsigned long linear, unsigned int bytes,
> While this was discussed in response to v1, I still think "mapping"
> should be const void *, or a prereq patch (which I would object
> to) should be submitted to drop the const from vunmap() and
> unmap_domain_page().
I'll wait for Andrews opinion on this issue.
>
> >
> > @@ -1007,23 +1160,15 @@ static int hvmemul_write(
> >           (vio->mmio_gla == (addr & PAGE_MASK)) )
> >          return hvmemul_linear_mmio_write(addr, bytes, p_data,
> > pfec, hvmemul_ctxt, 1);
> >
> > -    rc = hvm_copy_to_guest_linear(addr, p_data, bytes, pfec,
> > &pfinfo);
> > -
> > -    switch ( rc )
> > -    {
> > -    case HVMTRANS_okay:
> > -        break;
> > -    case HVMTRANS_bad_linear_to_gfn:
> > -        x86_emul_pagefault(pfinfo.ec, pfinfo.linear,
> > &hvmemul_ctxt->ctxt);
> > -        return X86EMUL_EXCEPTION;
> > -    case HVMTRANS_bad_gfn_to_mfn:
> > +    mapping = hvmemul_map_linear_addr(addr, bytes, pfec,
> > hvmemul_ctxt);
> > +    if ( IS_ERR(mapping) )
> > +        return ~PTR_ERR(mapping);
> > +    else if ( !mapping )
> v1 comment: 'Pointless "else".'
Agreed.

Regards,
Alex

________________________
This email was scanned by Bitdefender
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel