Re: [Xen-devel] [PATCH v2 19/20] xen/privcmd: Add support for Linux 64KB page granularity

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

On 07/13/2015 06:05 PM, Julien Grall wrote:
> Hi Boris,
>
> On 13/07/2015 22:13, Boris Ostrovsky wrote:
> > On 07/09/2015 04:42 PM, Julien Grall wrote:
> > > -
> > >   struct remap_data {
> > >       xen_pfn_t *fgmfn; /* foreign domain's gmfn */
> > > +    xen_pfn_t *efgmfn; /* pointer to the end of the fgmfn array */
> > It might be better to keep size of fgmfn array instead.
> It would means to have an other variable to check that we are at the 
> end the array.

I thought that's what h_iter is. Is it not?


> What about a variable which will be decremented?
>
> > > +static int unmap_gfn(struct page *page, unsigned long pfn, void *data)
> > > +{
> > > +    int *nr = data;
> > > +    struct xen_remove_from_physmap xrp;
> > > +
> > > +    /* The Linux Page may not have been fully mapped to Xen */
> > > +    if (!*nr)
> > > +        return 0;
> > > +
> > > +    xrp.domid = DOMID_SELF;
> > > +    xrp.gpfn = pfn;
> > > +    (void)HYPERVISOR_memory_op(XENMEM_remove_from_physmap, &xrp);
> > > +
> > > +    (*nr)--;
> > > +
> > > +    return 0;
> > > +}
> > > +
> > >   int xen_xlate_unmap_gfn_range(struct vm_area_struct *vma,
> > >                     int nr, struct page **pages)
> > >   {
> > >       int i;
> > > +    int nr_page = round_up(nr, XEN_PFN_PER_PAGE);
> > > -    for (i = 0; i < nr; i++) {
> > > -        struct xen_remove_from_physmap xrp;
> > > -        unsigned long pfn;
> > > +    for (i = 0; i < nr_page; i++) {
> > > +        /* unmap_gfn guarantees ret == 0 */
> > > +        BUG_ON(xen_apply_to_page(pages[i], unmap_gfn, &nr));
> >
> > TBH, I am not sure how useful xen_apply_to_page() routine is. In this
> > patch especially, but also in others.
> It avoids an open loop in each place where it's needed (here, 
> balloon...) which means another indentation layer. You can give a look 
> it's quite ugly.
I didn't notice that it was an inline, in which case it is indeed cleaner.

-boris

> Furthermore, the helper will avoid possible done by developers who are 
> working on PV drivers on x86. If you see code where the MFN 
> translation is done directly via virt_to_mfn or page_to_mfn... it will 
> likely means that the code will be broking when 64KB page granularity 
> will be in used.
> Though, there will still be some place where it's valid to use 
> virt_to_mfn and page_to_mfn.
> Regards,

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