Re: [Xen-devel] [PATCH v3 3/3] xen: introduce XENMEM_get_dma_buf and XENMEM_put_dma_buf

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 05.08.13 at 18:40, Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx> 
>>> wrote:
> @@ -496,7 +496,7 @@ static long 
> memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg)
>              mfn = page_to_mfn(page);
>              guest_physmap_add_page(d, gpfn, mfn, exch.out.extent_order);
>  
> -            if ( !paging_mode_translate(d) )
> +            if ( op == XENMEM_get_dma_buf || !paging_mode_translate(d) )

This is bogus: Whether the translation tables need updating
here shouldn't depend on the operation.

> @@ -505,6 +505,18 @@ static long 
> memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg)
>                                              &mfn, 1) )
>                      rc = -EFAULT;
>              }
> +
> +            if ( op == XENMEM_get_dma_buf )
> +            {
> +                static int warning;

bool_t. Missing blank line. And this should go into the next scope,
or the two if-s should be combined.

> +                if ( guest_physmap_pin_range(d, gpfn, exch.out.extent_order) 
> )
> +                {
> +                    if (!warning) {

Coding style.

> +                        gdprintk(XENLOG_WARNING, "guest_physmap_pin_range 
> not implemented\n");

Is "not implemented" really the only failure mode?

> +static long put_dma_buf(XEN_GUEST_HANDLE_PARAM(xen_put_dma_buf_t) arg)
> +{
> +    int rc, i;

Please no signed variables for loops having an unsigned upper
bound. Even more, the bound is xen_ulong_t, i.e. the loop below
may end up running forever on 64-bit at least.

> @@ -630,8 +690,15 @@ long do_memory_op(unsigned long cmd, 
> XEN_GUEST_HANDLE_PARAM(void) arg)
>  
>          break;
>  
> +    case XENMEM_get_dma_buf:
> +        /* xen_get_dma_buf_t is identical to xen_memory_exchange_t, so
> +         * just cast it and reuse memory_exchange */

Then why do you define a new type for it in the first place?

And anyway, the naming of the new sub-op isn't really
representing what the operation is capable of.
XENMEM_exchange_and_pin would seem to come much closer.

>      case XENMEM_exchange:
> -        rc = memory_exchange(guest_handle_cast(arg, xen_memory_exchange_t));
> +        rc = memory_exchange(op, guest_handle_cast(arg, 
> xen_memory_exchange_t));
> +        break;
> +
> +    case XENMEM_put_dma_buf:
> +        rc = put_dma_buf(guest_handle_cast(arg, xen_put_dma_buf_t));

And similarly this should be XENMEM_unpin or some such.

> --- a/xen/include/public/memory.h
> +++ b/xen/include/public/memory.h
> @@ -459,6 +459,70 @@ DEFINE_XEN_GUEST_HANDLE(xen_mem_sharing_op_t);
>   * The zero value is appropiate.
>   */
>  
> +#define XENMEM_get_dma_buf             26
> +/*
> + * This hypercall is similar to XENMEM_exchange: it exchanges the pages
> + * passed in with a new set of pages, contiguous and under 4G if so

The mentioning of 4G here is pointless - the hypercall can do all
sorts of address range restriction, so why mention this special
case?

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel