Re: [Xen-devel] [PATCH] Add a GNTTABOP to swap the content of two grant references under lock provided that they are not currently active.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 23.01.12 at 12:51, Wei Liu <wei.liu2@xxxxxxxxxx> wrote:
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -2282,6 +2282,78 @@ 
> gnttab_get_version(XEN_GUEST_HANDLE(gnttab_get_version_t uop))
>      return 0;
>  }
>  
> +static s16
> +__gnttab_swap_grant_ref(unsigned long ref_a, unsigned long ref_b)

unsigned long?

> +{
> +    struct domain *d;
> +    struct active_grant_entry *act;
> +    s16 rc = GNTST_okay;
> +
> +    d = rcu_lock_current_domain();
> +
> +    spin_lock(&d->grant_table->lock);
> +
> +    act = &active_entry(d->grant_table, ref_a);
> +    if ( act->pin )
> +        PIN_FAIL(out, GNTST_eagain, "ref %ld busy\n", ref_a);
> +
> +    act = &active_entry(d->grant_table, ref_b);
> +    if ( act->pin )
> +        PIN_FAIL(out, GNTST_eagain, "ref %ld busy\n", ref_b);

As these two messages are (I think) intended to help developers, you
will want to make them distinct (so one knows which one it was that
failed).

> +
> +    if ( d->grant_table->gt_version == 1 ) {

Formatting ({ on the next line).

> +        grant_entry_v1_t shared;
> +
> +        shared = shared_entry_v1(d->grant_table, ref_a);
> +
> +        shared_entry_v1(d->grant_table, ref_a) =
> +            shared_entry_v1(d->grant_table, ref_b);
> +
> +        shared_entry_v1(d->grant_table, ref_b) = shared;
> +    } else {

    }
    else
    {

> +        grant_entry_v2_t shared;
> +        grant_status_t status;
> +
> +        shared = shared_entry_v2(d->grant_table, ref_a);
> +        status = status_entry(d->grant_table, ref_a);
> +
> +        shared_entry_v2(d->grant_table, ref_a) =
> +            shared_entry_v2(d->grant_table, ref_b);
> +        status_entry(d->grant_table, ref_a) =
> +            status_entry(d->grant_table, ref_b);
> +
> +        shared_entry_v2(d->grant_table, ref_b) = shared;
> +        status_entry(d->grant_table, ref_b) = status;
> +    }
> +
> +out:
> +    spin_unlock(&d->grant_table->lock);
> +
> +    rcu_unlock_domain(d);
> +
> +    return rc;
> +}
> +
> +static long
> +gnttab_swap_grant_ref(XEN_GUEST_HANDLE(gnttab_swap_grant_ref_t uop),
> +                      unsigned int count)
> +{
> +    int i;
> +    gnttab_swap_grant_ref_t op;
> +
> +    for ( i = 0; i < count; i++ )
> +    {
> +        if ( i && hypercall_preempt_check() )
> +            return i;
> +        if ( unlikely(__copy_from_guest_offset(&op, uop, i, 1)) )
> +            return -EFAULT;
> +        op.status = __gnttab_swap_grant_ref(op.ref_a, op.ref_b);
> +        if ( unlikely(__copy_to_guest_offset(uop, i, &op, 1)) )
> +            return -EFAULT;
> +    }
> +    return 0;
> +}
> +
>  long
>  do_grant_table_op(
>      unsigned int cmd, XEN_GUEST_HANDLE(void) uop, unsigned int count)
> --- a/xen/include/xlat.lst
> +++ b/xen/include/xlat.lst
> @@ -50,6 +50,7 @@
>  ?    grant_entry_v1                  grant_table.h
>  ?       grant_entry_header              grant_table.h
>  ?    grant_entry_v2                  grant_table.h
> +?    gnttab_swap_grant_ref           grant_table.h

Adding this here isn't enough - you also need to invoke
CHECK_gnttab_swap_grant_ref in xen/common/compat/grant_table.c,
and you should add a stub CASE() at the top of
compat_grant_table_op() (to keep the set complete there).

Jan

>  ?    kexec_exec                      kexec.h
>  !    kexec_image                     kexec.h
>  !    kexec_range                     kexec.h



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel