Re: [PATCH v2.2 07/15] xen: generate hypercall interface related code

[Juergen Gross <jgross@xxxxxxxx>]

On 03.11.21 16:42, Jan Beulich wrote:
> On 03.11.2021 11:20, Juergen Gross wrote:
> > Instead of repeating similar data multiple times use a single source
> > file and a generator script for producing prototypes and call sequences
> > of the hypercalls.
> >
> > As the script already knows the number of parameters used add generating
> > a macro for populating an array with the number of parameters per
> > hypercall.
> >
> > Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> What's missing here now that you have introduced priorities is how you
> did arrive at the chosen values. That's a pretty important aspect,
> especially if someone would later want to adjust these.
Will include the measured data in next version's commit message.

> Just one further remark from looking at the output of the script (thanks
> for making that available):
>
> > +# Generate the output
> > +END {
> > +    # Verbatim generated lines
> > +    for (i = 1; i <= e; i++)
> > +        printf("%s\n", emit[i]);
> > +    printf("\n");
> > +    # Generate prototypes
> > +    for (i = 1; i <= n; i++) {
> > +        for (p = 1; p <= n_pre[i]; p++) {
> > +            printf("%s %s_%s(", rettype[pre[i, p]], pre[i, p], fn[i]);
> > +            if (n_args[i] == 0)
> > +                printf("void");
> > +            else
> > +                for (j = 1; j <= n_args[i]; j++) {
> > +                    if (j > 1)
> > +                        printf(", ");
> > +                    if (ptr[i, j])
> > +                        printf("XEN_GUEST_HANDLE_PARAM(%s)", typ[i, j]);
> > +                    else
> > +                        printf("%s", typ[i, j]);
> > +                    printf(" %s", arg[i, j]);
> > +                }
> > +            printf(");\n");
> > +        }
> > +    }
> > +    # Generate call sequences and args array contents
> > +    for (ca in caller) {
> > +        if (caller[ca] != 1)
> > +            continue;
> > +        need_mask = 0;
> > +        for (pl = 1; pl <= n_prios[ca]; pl++) {
> > +            for (pll = pl; pll > 1; pll--) {
> > +                if (prio_list[ca, pl] > p_list[pll - 1])
> > +                    break;
> > +                else
> > +                    p_list[pll] = p_list[pll - 1];
> > +            }
> > +            p_list[pll] = prio_list[ca, pl];
> > +            # If any prio but the default one has more than 1 entry we need 
> > "mask"
> > +            if (p_list[pll] != 100 && prios[ca, p_list[pll]] > 1)
> > +                need_mask = 1;
> > +        }
> > +        printf("\n");
> > +        printf("#define call_handlers_%s(num, ret, a1, a2, a3, a4, a5) \\\n", 
> > ca);
> > +        printf("{ \\\n");
> > +        if (need_mask)
> > +            printf("    uint64_t mask = 1ULL << num; \\\n");
> > +        printf("    ");
> > +        for (pl = 1; pl <= n_prios[ca]; pl++) {
> > +            if (prios[ca, p_list[pl]] > 1) {
> > +                if (pl < n_prios[ca]) {
> > +                    printf("if ( likely(mask & (%s)) ) \\\n", prio_mask[ca, 
> > p_list[pl]]);
> > +                    printf("    { \\\n");
> > +                }
> > +                if (prios[ca, p_list[pl]] == 2) {
> > +                    fnd = 0;
> > +                    for (i = 1; i <= nc; i++)
> > +                        if (call[i] == ca && call_prio[i] == p_list[pl]) {
> > +                            fnd++;
> > +                            if (fnd == 1)
> > +                                printf("        if ( num == __HYPERVISOR_%s ) 
> > \\\n", fn[call_fn[i]]);
> > +                            else
> > +                                printf("        else \\\n");
> > +                            do_call(call_fn[i], call_p[i]);
> > +                        }
> > +                } else {
> > +                    do_switch(ca, p_list[pl]);
> > +                }
> > +                if (pl < n_prios[ca])
> > +                    printf("    } \\\n");
> > +            } else {
> > +                for (i = 1; i <= nc; i++)
> > +                    if (call[i] == ca && call_prio[i] == p_list[pl]) {
> > +                        printf("if ( likely(num == __HYPERVISOR_%s) ) \\\n", 
> > fn[call_fn[i]]);
> > +                        do_call(call_fn[i], call_p[i]);
> > +                    }
> > +            }
> > +            if (pl < n_prios[ca] || prios[ca, p_list[pl]] <= 2)
> > +                printf("    else ");
> I think there's a line continuation escape + newline missing here.
Yes and no.

This can either continue with another "if" clause not wanting a new
line or with a final ending statement (either a "switch" or a
"ret = -ENOSYS"), which probably would want the new line. Adding
code for handling both cases is possible, but I'm not sure it is
worth it. In case you are preferring an unconditional new line,
this would be easy to arrange, of course.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature