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 08.08.13 at 16:12, Stefano Stabellini 
> >>> <stefano.stabellini@xxxxxxxxxxxxx> wrote:
> > 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.
> 
> Oh, you're talking about one half of the if() body, and I'm talking
> about the other (the part that was visible from the patch context):
> I agree that copying back the MFNs is useful/necessary here. But
> I don't see why you'd want to call set_gpfn_from_mfn() - that
> should have been taken care of already. So perhaps split the one if
> into two?

Right, good suggestion.


> >> > @@ -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.
> 
> Hmm, I'm confused: Here you reason why you want a new type
> (note that I said "type", not "sub-hypercall"), ...
> 
> >> 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.
> 
> ... yet here you agree that xen_memory_exchange could be
> re-used.
 
I meant only to keep the same parameter struct xen_memory_exchange.

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