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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 19.09.17 at 16:39, <Paul.Durrant@xxxxxxxxxx> 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;
>> +
>> +    /* 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 )
> 
>>=, rather than the -1?

Yeah, I had pointed out that one too earlier on. Andrew gave a
reason that didn't really convince me, but which also made me
go silent despite

>> +    {
>> +        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);
> 
> Still the mysterious cast to long here and below that Jan pointed out.

Oh, I've even managed to overlook that. Without an explanation
why this is needed I would withdraw my R-b.

>> +            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++;
> 
> Increment still done here rather than being co-located with test below.

Indeed - if you dislike it going into the while(), at least put it right
ahead of it. Yet then again it sitting next to the mfn increment
doesn't look that bad either, and the mfn increment clearly can't
be moved down.

>> +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];
>> +
>> +    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)));
>> +
>> +        frame++;
> 
> Again, increment should be co-located with test IMO.
> 
>   Paul
> 
>> +        *mfn++ = _mfn(0); /* Clean slot for map()'s error checking. */
>> +
>> +    } while ( frame < final );

Well, here they're at least only spaced apart by another related
operation. It certainly would be nice if the two lines were at least
swapped.

Jan

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