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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 28.09.17 at 11:07, <Paul.Durrant@xxxxxxxxxx> wrote:
>> From: Alexandru Isaila [mailto:aisaila@xxxxxxxxxxxxxxx]
>> Sent: 28 September 2017 09:52
>> +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;
>> +    unsigned long nr_frames = ((linear + bytes) >> PAGE_SHIFT) - (linear >>
>> PAGE_SHIFT) + 1;
> 
> I'm unclear as to why you don't calculate nr_frames first and then calculate 
> final as first + nr_frames - 1? Having completely independent calculations 
> like this seems prone to error.

Nor is the calculation any better than that in v6 for the 64-bit
wrapping case. Easiest here may be to utilize truncation by
making nr_frames unsigned int (but you'll need to check carefully
that this doesn't cause issues elsewhere).

Plus the use of "linear + bytes" (without the "- !!bytes") causes the
result to be one too high when "linear" is exactly "bytes" away from
a page boundary, and "bytes" is not zero.

>> +    int i = 0;

unsigned int

>> +    /*
>> +     * mfn points to the next free slot.  All used slots have a page 
> reference
>> +     * held on them.
>> +     */
>> +    mfn_t *mfn = &hvmemul_ctxt->mfn[0];
>> +
> 
> Extraneous blank line.
> 
>> +
>> +    /*
>> +     * 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 ||
>> +         nr_frames > ARRAY_SIZE(hvmemul_ctxt->mfn) )
>> +    {
>> +        ASSERT_UNREACHABLE();
>> +        goto unhandleable;
>> +    }
>> +
>> +    do {
>> +        enum hvm_translation_result res;
>> +        struct page_info *page;
>> +        pagefault_info_t pfinfo;
>> +        p2m_type_t p2mt;
>> +
> 
> Can't frame now be made local to this loop and calculated up-front using 
> linear and i? It looks like the loop could become a simpler for-loop too.
> 
>> +        /* Error checking.  Confirm that the current slot is clean. */
>> +        ASSERT(mfn_x(*mfn) == 0);
>> +
>> +        res = hvm_translate_get_page(curr, frame, 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(~X86EMUL_EXCEPTION);
>> +            goto out;
>> +
>> +        case HVMTRANS_bad_gfn_to_mfn:
>> +            err = NULL;
>> +            goto out;
>> +
>> +        case HVMTRANS_gfn_paged_out:
>> +        case HVMTRANS_gfn_shared:
>> +            err = ERR_PTR(~X86EMUL_RETRY);
>> +            goto out;
>> +
>> +        default:
>> +            goto unhandleable;
>> +        }
>> +
>> +        *mfn++ = _mfn(page_to_mfn(page));
>> +        i++;
>> +
>> +        if ( p2m_is_discard_write(p2mt) )
>> +        {
>> +            err = ERR_PTR(~X86EMUL_OKAY);
>> +            goto out;
>> +        }
>> +
>> +        frame = (linear + i);

i is the number of pages, not bytes. Also stray parentheses here.

>> +        if ( hvmemul_ctxt->ctxt.addr_size < 64 )
>> +            frame = (uint32_t)frame;
>> +        frame = frame << PAGE_SHIFT;
>> +
>> +    } while ( i < nr_frames );
>> +
>> +    /* Entire access within a single frame? */
>> +    if ( first == final )

nr_frames == 1

>> +        mapping = map_domain_page(hvmemul_ctxt->mfn[0]);
>> +    /* Multiple frames? Need to vmap(). */
>> +    else if ( (mapping = vmap(hvmemul_ctxt->mfn,
>> +                              nr_frames)) == NULL )
>> +        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 + (linear & ~PAGE_MASK);
>> +
>> + unhandleable:
>> +    err = ERR_PTR(~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,
>> +    struct hvm_emulate_ctxt *hvmemul_ctxt)
>> +{
>> +    struct domain *currd = current->domain;
>> +    unsigned long frame = linear >> PAGE_SHIFT;
>> +    unsigned long final = (linear + bytes - !!bytes) >> PAGE_SHIFT;
>> +    mfn_t *mfn = &hvmemul_ctxt->mfn[0];
>> +
> 
> Extraneous blank line.
> 
>> +
>> +    ASSERT(bytes > 0);
>> +
>> +    if ( frame == final )
>> +        unmap_domain_page(mapping);
>> +    else
>> +        vunmap(mapping);
>> +
>> +    do
>> +    {
>> +        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. */
>> +    } while ( ++frame < final );
> 
> Do you not have the same overflow issues here? I would have though the 
> looping construct here should be near identical to that in 
> hvmemul_map_linear_addr().

Indeed, and I _specifically_ said so in reply to v6. Alexandru,
please be quite a bit more careful. Just to emphasize it again -
any comments made to the mapping code need to also be
checked for applicability to the unmapping side.

Jan

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