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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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

Jan


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