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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Thu, 8 Aug 2013, Jan Beulich wrote:
> >>> 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.

This is key: I need an hypercall that writes back the mfn of the new
buffer. The old hypercall doesn't do it. This time I specifically wrote
in a comment:

* If return code is zero then @out.extent_list provides the MFNs of the
* newly-allocated memory.

Also it makes sense for the old hypercall not to return the mfn to
autotranslate guests because the p2m mappings are not pinned, so it
can't guarantee that they won't change.


> > @@ -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?

My mistake. I'll remove the warning and propagate the failure instead.


> > +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.

Argh, thanks for catching this.


> > @@ -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?

Because the original hypercall doesn't copy back the mfn if the guest
it's an autotranslate guest.
And because it doesn't pin the p2m mappings.


> 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.

I like this suggestion. I could also keep xen_memory_exchange as
argument for XENMEM_exchange_and_pin, as Ian suggested.


> >      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.

OK.


> > --- 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?

Good point.

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