Re: [Xen-devel] [PATCH v12 05/11] x86/mm: add HYPERVISOR_memory_op to acquire guest resources

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Julien Grall [mailto:julien.grall@xxxxxxxxxx]
> Sent: 23 October 2017 20:04
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>; 'Jan Beulich'
> <JBeulich@xxxxxxxx>
> Cc: Julien Grall <julien.grall@xxxxxxx>; Andrew Cooper
> <Andrew.Cooper3@xxxxxxxxxx>; George Dunlap
> <George.Dunlap@xxxxxxxxxx>; Ian Jackson <Ian.Jackson@xxxxxxxxxx>; Roger
> Pau Monne <roger.pau@xxxxxxxxxx>; Wei Liu <wei.liu2@xxxxxxxxxx>; Stefano
> Stabellini <sstabellini@xxxxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx; Konrad
> Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>; Daniel De Graaf
> <dgdegra@xxxxxxxxxxxxx>; Tim (Xen.org) <tim@xxxxxxx>
> Subject: Re: [Xen-devel] [PATCH v12 05/11] x86/mm: add
> HYPERVISOR_memory_op to acquire guest resources
> 
> 
> 
> On 20/10/17 11:10, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: Julien Grall [mailto:julien.grall@xxxxxxxxxx]
> >> Sent: 20 October 2017 11:00
> >> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>; 'Jan Beulich'
> >> <JBeulich@xxxxxxxx>
> >> Cc: Julien Grall <julien.grall@xxxxxxx>; Andrew Cooper
> >> <Andrew.Cooper3@xxxxxxxxxx>; George Dunlap
> >> <George.Dunlap@xxxxxxxxxx>; Ian Jackson <Ian.Jackson@xxxxxxxxxx>;
> Roger
> >> Pau Monne <roger.pau@xxxxxxxxxx>; Wei Liu <wei.liu2@xxxxxxxxxx>;
> Stefano
> >> Stabellini <sstabellini@xxxxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx;
> Konrad
> >> Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>; Daniel De Graaf
> >> <dgdegra@xxxxxxxxxxxxx>; Tim (Xen.org) <tim@xxxxxxx>
> >> Subject: Re: [Xen-devel] [PATCH v12 05/11] x86/mm: add
> >> HYPERVISOR_memory_op to acquire guest resources
> >>
> >> Hi Paul,
> >>
> >> On 20/10/17 09:26, Paul Durrant wrote:
> >>>> -----Original Message-----
> >>>> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> >>>> Sent: 20 October 2017 07:25
> >>>> To: Julien Grall <julien.grall@xxxxxxxxxx>
> >>>> Cc: Julien Grall <julien.grall@xxxxxxx>; Andrew Cooper
> >>>> <Andrew.Cooper3@xxxxxxxxxx>; George Dunlap
> >>>> <George.Dunlap@xxxxxxxxxx>; Ian Jackson <Ian.Jackson@xxxxxxxxxx>;
> Paul
> >>>> Durrant <Paul.Durrant@xxxxxxxxxx>; Roger Pau Monne
> >>>> <roger.pau@xxxxxxxxxx>; Wei Liu <wei.liu2@xxxxxxxxxx>; Stefano
> Stabellini
> >>>> <sstabellini@xxxxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx; Konrad
> >> Rzeszutek
> >>>> Wilk <konrad.wilk@xxxxxxxxxx>; Daniel De Graaf
> >> <dgdegra@xxxxxxxxxxxxx>;
> >>>> Tim (Xen.org) <tim@xxxxxxx>
> >>>> Subject: Re: [Xen-devel] [PATCH v12 05/11] x86/mm: add
> >>>> HYPERVISOR_memory_op to acquire guest resources
> >>>>
> >>>>>>> On 19.10.17 at 18:21, <julien.grall@xxxxxxxxxx> wrote:
> >>>>> Looking a bit more at the resource you can acquire from this hypercall.
> >>>>> Some of them are allocated using alloc_xenheap_page() so not
> assigned
> >> to
> >>>>> a domain.
> >>>>>
> >>>>> So I am not sure how you can expect a function
> set_foreign_p2m_entry
> >> to
> >>>>> take reference in that case.
> >>>>
> >>>> Hmm, with the domain parameter added, DOMID_XEN there (for
> >>>> Xen heap pages) could identify no references to be taken, if that
> >>>> was really the intended behavior in that case. However, even for
> >>>> Xen heap pages life time tracking ought to be done - it is for a
> >>>> reason that share_xen_page_with_guest() assigns the target
> >>>> domain as the owner of such pages, as that allows get_page() to
> >>>> succeed for them.
> >>>>
> >>>
> >
> > Hi Julien,
> >
> >>> So, nothing I'm doing here is making anything worse, right? Grant tables
> are
> >> assigned to the guest, and IOREQ server pages are allocated with
> >> alloc_domheap_page() so nothing is anonymous.
> >>
> >> I don't think grant tables is assigned to the guest today. They are
> >> allocated using xenheap_pages() and I can't find
> >> share_xen_page_with_guest().
> >
> > The guest would not be able to map them if they were not assigned in
> some way!
> 
> Do you mean for PV? For HVM/PVH, we don't check whether the page is
> assigned (see gnttab_map_frame).

Not there, but I though there were checks in guest_physmap_add_page() where the 
mfn passed back from gnttab_map_frame() is actually consumed. It's all quite 
convoluted though so I'm not sure.

> 
> > See the code block at
> http://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=xen/common/grant_tab
> le.c;hb=HEAD#l1716
> > It calls gnttab_create_shared_page() which is what calls through to
> share_xen_page_with_guest().
> 
> Thank you for the link, I will have a look.
> 
> >
> >>
> >> Anyway, I discussed with Stefano about it. set_foreign_p2m_entry is
> >> going to be left unimplemented on Arm until someone as time to
> implement
> >> correctly the function.
> >>
> >
> > That makes sense. Do you still have any issues with this patch apart from
> the cosmetic ones you spotted in the header?
> 
> No. Although, may I request to add a comment in the ARM helpers about
> the reference counting?
> 

Sure. Thanks,

  Paul

> Cheers,
> 
> --
> Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel