Re: [PATCH 3/4] xen/arm: introduce XENMEM_cache_flush

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

>>> On 02.10.14 at 12:02, <stefano.stabellini@eu.citrix.com> wrote:
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -1134,6 +1134,98 @@ long arch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg)
>      case XENMEM_get_sharing_shared_pages:
>      case XENMEM_get_sharing_freed_pages:
>          return 0;
> +    case XENMEM_cache_flush:

Blank line missing above this one.

> +    {
> +        struct xen_cache_flush cflush;
> +        struct domain *d, *owner;
> +        struct page_info *page;
> +        uint64_t mfn, end;
> +        uint64_t offset, size;
> +        void *v;
> +        int ret = 0;
> +
> +        if ( copy_from_guest(&cflush, arg, 1) )
> +            return -EFAULT;
> +
> +        d = rcu_lock_current_domain();
> +        if ( d == NULL )
> +            return -ESRCH;

I think this should be the last check prior to the loop below,
simplifying error handling (break instead of goto).

> +
> +        if ( (cflush.size >> PAGE_SHIFT) > (1U<<MAX_ORDER) )

The why is size a uint64_t in the first place?

> +        {
> +            printk(XENLOG_G_ERR "invalid size %llx\n", cflush.size);
> +            ret = -EINVAL;
> +            goto out;
> +        }
> +
> +        if ( cflush.size == 0 || cflush.op == 0 )
> +        {
> +            ret = 0;
> +            goto out;
> +        }
> +
> +        if ( cflush.op & ~(XENMEM_CACHE_INVAL|XENMEM_CACHE_CLEAN) )
> +        {
> +            printk(XENLOG_G_ERR "invalid op %x\n", cflush.op);
> +            ret = -EINVAL;
> +            goto out;
> +        }
> +
> +        end = cflush.addr + cflush.size;

As said in various contexts before: Such an addition can overflow,
and (even if this doesn't matter here due to the earlier check) you
can't express "the entire address space" with a (base,size) pair
either. Representing arbitrary ranges is better done using inclusive
pairs of addresses/MFNs/whatever.

> +        while ( cflush.addr < end )
> +        {
> +            mfn = cflush.addr >> PAGE_SHIFT;
> +            offset = cflush.addr & ~PAGE_MASK;
> +
> +            if ( !mfn_valid(mfn) )
> +            {
> +                printk(XENLOG_G_ERR "mfn=%llx is not valid\n", mfn);
> +                ret = -EINVAL;
> +                goto out;
> +            }
> +
> +            page = mfn_to_page(mfn);
> +            if ( !page )
> +            {
> +                printk(XENLOG_G_ERR "couldn't get page for mfn %llx\n", mfn);
> +                ret =  -EFAULT;
> +                goto out;
> +            }
> +
> +            owner = page_get_owner(page);
> +            if ( !owner )
> +            {
> +                printk(XENLOG_G_ERR "couldn't get owner for mfn %llx\n", mfn);
> +                ret = -EFAULT;
> +                goto out;
> +            }
> +
> +            if ( owner != d && !grant_map_exists(d, owner->grant_table, mfn) )
> +            {
> +                printk(XENLOG_G_ERR "mfn %llx hasn't been granted by %d to %d\n", mfn, owner->domain_id, d->domain_id);

Long line. But - do you really need all these printk()s?

> +                ret = -EINVAL;
> +                goto out;
> +            }
> +
> +            v = map_domain_page(mfn);
> +            v += offset;
> +            size = cflush.size - cflush.addr;
> +            if ( size > PAGE_SIZE - offset )
> +                size = PAGE_SIZE - offset;
> +
> +            if ( cflush.op & XENMEM_CACHE_INVAL )
> +                invalidate_xen_dcache_va_range(v, size);
> +            if ( cflush.op & XENMEM_CACHE_CLEAN )
> +                clean_xen_dcache_va_range(v, size);

Is this really the right sequence when both flags are set? Of course
I can only guess a what "clean" and "invalidate" here mean (nothing
more specific is being said alongside their definition), but I'd expect
you to want to clean (flush) cache contents _before_ invalidating.

> +            unmap_domain_page(v);
> +
> +            cflush.addr += PAGE_SIZE - offset;
> +        }
> +
> +out:

Labels should be indented by at least one space. But you don't
really need the label anyway if you follow the suggestion above.

Jan