Re: [PATCH v8] x86/hvm: Implement hvmemul_write() using real mappings

["Jan Beulich" <JBeulich@suse.com>]

>>> On 02.10.17 at 14:23, <aisaila@bitdefender.com> 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 int nr_frames = ((linear + bytes - !!bytes) >> PAGE_SHIFT) -
> +        (linear >> PAGE_SHIFT) + 1;

The comment is now stale. Instead you may want to state that by
truncating long to int the 64-bit wrapping case is being taken care
of. Or if you drop the comment altogether, please also drop the
preceding blank line.

> +    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.
> +     *
> +     * 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");
> +        goto unhandleable;
> +    }
> +
> +    for( i = 0; i < nr_frames; i++ )

Style (missing blank; also in the unmap code below).

> +    {
> +        enum hvm_translation_result res;
> +        struct page_info *page;
> +        pagefault_info_t pfinfo;
> +        p2m_type_t p2mt;
> +        unsigned long frame = (linear >> PAGE_SHIFT) + i;
> +        unsigned long addr = frame << PAGE_SHIFT;

This is wrong on the first iteration (it'll lead to guest CR2 pointing
at the beginning of the page, should a #PF need raising).

        unsigned long addr = i ? (linear + (i << PAGE_SHIFT)) & PAGE_MASK : linear;

or some thing similar, dropping frame at once (which eliminates
the risk of someone later adding another use of it).

Jan


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