Re: [Xen-devel] [PATCH v3 2/5] x86/hyperv: provide Hyper-V hypercall functions

[Jan Beulich <jbeulich@xxxxxxxx>]

On 05.01.2020 17:47, Wei Liu wrote:
> +static inline uint64_t hv_do_fast_hypercall(uint16_t code,
> +                                            uint64_t input1, uint64_t input2)
> +{
> +    uint64_t status;
> +    uint64_t control = (uint64_t)code | HV_HYPERCALL_FAST_BIT;

Unnecessary (afaict) cast.

> +    asm volatile ("mov %[input2], %%r8\n"
> +                  "call hv_hypercall_page"
> +                  : "=a" (status), "+c" (control),
> +                    "+d" (input1) ASM_CALL_CONSTRAINT
> +                  : [input2] "rm" (input2)
> +                  : "cc", "r8", "r9", "r10", "r11");
> +
> +    return status;
> +}
> +
> +static inline uint64_t hv_do_rep_hypercall(uint16_t code, uint16_t rep_count,
> +                                           uint16_t varhead_size,
> +                                           paddr_t input, paddr_t output)
> +{
> +    uint64_t control = code;
> +    uint64_t status;
> +    uint16_t rep_comp;
> +
> +    control |= (uint64_t)varhead_size << HV_HYPERCALL_VARHEAD_OFFSET;
> +    control |= (uint64_t)rep_count << HV_HYPERCALL_REP_COMP_OFFSET;
> +
> +    do {
> +        status = hv_do_hypercall(control, input, output);
> +        if ( (status & HV_HYPERCALL_RESULT_MASK) != HV_STATUS_SUCCESS )
> +            break;
> +
> +        rep_comp = (status & HV_HYPERCALL_REP_COMP_MASK) >>
> +            HV_HYPERCALL_REP_COMP_OFFSET;

MASK_EXTR()? (I then also wonder whether MASK_INSR() would better be
used with some of the other constructs here.)

What's worse though - looking at the definition of
HV_HYPERCALL_REP_COMP_MASK I notice that it and a few others use
GENMASK_ULL(), when it was clearly said during review (perhaps of
another but related patch) that this macro should not be used
outside of Arm-specific code until it gets put into better shape:
https://lists.xenproject.org/archives/html/xen-devel/2019-12/msg00705.html

> +        control &= ~HV_HYPERCALL_REP_START_MASK;
> +        control |= (uint64_t)rep_comp << HV_HYPERCALL_REP_COMP_OFFSET;
> +
> +    } while ( rep_comp < rep_count );

We don't normally have a blank line ahead of the closing brace of a
block.

Jan

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