Re: [Xen-devel] [PATCH 8/9] x86/hvm: Add hvm_copy_{to, from}_guest_virt() helpers

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 26.10.17 at 19:03, <euan.harris@xxxxxxxxxx> wrote:
> hvm_copy_{to,from}_guest_virt() copy data to and from a guest, performing
> segmentatino and paging checks on the provided seg:offset virtual address.

I'm not sure "paging" is worthwhile to mention here, as that's not
done by the new functions, but by the existing one acting on linear
addresses.

> +static int _hvm_copy_guest_virt(
> +    enum x86_segment seg, unsigned long offset, void *buf, unsigned int 
> bytes,
> +    uint32_t pfec, unsigned int flags)
> +{
> +    struct vcpu *curr = current;
> +    struct segment_register sreg, cs;
> +    enum hvm_translation_result res;
> +    pagefault_info_t pfinfo;
> +    unsigned long linear;
> +
> +    ASSERT(is_x86_user_segment(seg));
> +
> +    hvm_get_segment_register(curr, seg, &sreg);
> +    hvm_get_segment_register(curr, x86_seg_cs, &cs);
> +
> +    if ( !hvm_virtual_to_linear_addr(
> +             seg, &sreg, offset, bytes,
> +             flags & HVMCOPY_to_guest ? hvm_access_write : hvm_access_read,
> +             &cs, &linear) )
> +    {
> +        hvm_inject_hw_exception(
> +            (seg == x86_seg_ss) ? TRAP_stack_error : TRAP_gp_fault, 0);
> +        return X86EMUL_EXCEPTION;
> +    }
> +
> +    if ( flags & HVMCOPY_to_guest )
> +        res = hvm_copy_to_guest_linear(linear, buf, bytes, pfec, &pfinfo);
> +    else
> +        res = hvm_copy_from_guest_linear(buf, linear, bytes, pfec, &pfinfo);
> +
> +    if ( res == HVMTRANS_bad_linear_to_gfn )
> +    {
> +        hvm_inject_page_fault(pfinfo.ec, pfinfo.linear);
> +        return X86EMUL_EXCEPTION;
> +    }
> +    else if ( res )
> +        return X86EMUL_RETRY;
> +
> +    return X86EMUL_OKAY;

Please either use switch() here, or at least omit the pointless "else".

> +int hvm_copy_to_guest_virt(
> +    enum x86_segment seg, unsigned long offset, void *buf, unsigned int 
> bytes,
> +    uint32_t pfec)
> +{
> +    return _hvm_copy_guest_virt(seg, offset, buf, bytes, pfec,
> +                                HVMCOPY_to_guest);
> +}
> +
> +int hvm_copy_from_guest_virt(
> +    void *buf, enum x86_segment seg, unsigned long offset, unsigned int 
> bytes,
> +    uint32_t pfec)
> +{
> +    return _hvm_copy_guest_virt(seg, offset, buf, bytes, pfec,
> +                                HVMCOPY_from_guest);
> +}

Is there any reason the parameter order is different between the
two wrappers and the actual worker? That'll cause unnecessary
register shuffling, when really the wrappers could just be a load
of a constant into a register plus a branch.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel