Re: [Xen-devel] [RFC PATCH 8/8]: PVH: privcmd changes

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Thu, 2012-08-16 at 02:07 +0100, Mukesh Rathor wrote:
> ---
>  drivers/xen/privcmd.c |   68 +++++++++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 66 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
> index ccee0f1..0a240ab 100644
> --- a/drivers/xen/privcmd.c
> +++ b/drivers/xen/privcmd.c
> @@ -33,6 +33,7 @@
>  #include <xen/features.h>
>  #include <xen/page.h>
>  #include <xen/xen-ops.h>
> +#include <xen/balloon.h>
>  
>  #include "privcmd.h"
>  
> @@ -199,6 +200,10 @@ static long privcmd_ioctl_mmap(void __user *udata)
>       if (!xen_initial_domain())
>               return -EPERM;
>  
> +     /* PVH: TBD/FIXME. For now we only support privcmd_ioctl_mmap_batch */
> +     if (xen_pvh_domain())
> +             return -ENOSYS;
> +
>       if (copy_from_user(&mmapcmd, udata, sizeof(mmapcmd)))
>               return -EFAULT;
>  
> @@ -251,6 +256,8 @@ struct mmap_batch_state {
>       xen_pfn_t __user *user;
>  };
>  
> +/* PVH dom0: if domU being created is PV, then mfn is mfn(addr on bus). If
> + * it's PVH then mfn is pfn (input to HAP). */
>  static int mmap_batch_fn(void *data, void *state)
>  {
>       xen_pfn_t *mfnp = data;
> @@ -274,6 +281,40 @@ static int mmap_return_errors(void *data, void *state)
>       return put_user(*mfnp, st->user++);
>  }
>  
> +/* Allocate pfns that are then mapped with gmfns from foreign domid. Update
> + * the vma with the page info to use later.
> + * Returns: 0 if success, otherwise -errno
> + */
> +static int pvh_privcmd_resv_pfns(struct vm_area_struct *vma, int numpgs)
> +{
> +     int rc;
> +     struct xen_pvh_sav_pfn_info *savp;
> +
> +     savp = kzalloc(sizeof(struct xen_pvh_sav_pfn_info), GFP_KERNEL);
> +     if (savp == NULL)
> +             return -ENOMEM;
> +
> +     savp->sp_paga = kcalloc(numpgs, sizeof(savp->sp_paga[0]), GFP_KERNEL);
> +     if (savp->sp_paga == NULL) {
> +             kfree(savp);
> +             return -ENOMEM;
> +     }
> +
> +     rc = alloc_xenballooned_pages(numpgs, savp->sp_paga, 0);
> +     if (rc != 0) {
> +             pr_warn("%s Could not alloc %d pfns rc:%d\n", __FUNCTION__,
> +                     numpgs, rc);
> +             kfree(savp->sp_paga);
> +             kfree(savp);
> +             return -ENOMEM;
> +     }

I've just been building on this patch to make proper mmap foreign
support on ARM and I was looking for the place which freed this, both
the pages back to the balloon and then the array itself. There is code
in privcmd_close which unmaps the P2M, but I can't find the code which
frees things back to the balloon. Have I missed something?

I think we also need to think about the layering / abstraction a bit
here too. Currently on setup the caller allocates the array iff pvh and
stores it in the opaque vma->vm_private, then calls
xen_remap_domain_mfn_range which iff pvh calls pvh_add_to_xen_p2m which
assumes that the caller has seeded the vm_private with the correct
struct. xen_remap_domain_mfn_range also sets up the stage 1 PT mappings.

On teardown the iff pvh is in the caller which calls pvh_rem_xen_p2m
directly (this API is unbalanced with the setup side). The stage 1 PT
mappings are torn down implicitly somewhere else (in generic code, I
think you said).

(BTW, the terminology I'm using is stage 1 == guest OS page tables,
stage 2 == HAP)

I think you can't rely on the implicit teardown here since you need to
unmap before you hand the page back to the balloon. The reason this
doesn't look necessary now is that you don't give the page back.

Also not ordering the stage 1 and stage 2 teardown correctly is
dangerous, depending on the eventual ordering you potentially turn an
erroneous access to a virtual address, which should result in a guest OS
level page fault (and e.g. a seg fault to the offending process) into a
hypervisor shoots the guest due to an unexpected stage 2 fault type
failure, which is somewhat undesirable ;-)

With that in mind I think you do in the end need to add
xen_unmap_domain_mfn_range which does the unmap from both stage 1 and
stage 2 -- that balances out the interface (making pvh_rem_xen_p2m
internal) too, which is nice. This function may turn out to be a nop
on !pvh, but that's ok (although maybe there would be no harm in doing
explicit unmaps, for consistency?).

WRT passing data between interfaces in vma->vm_private, which is pretty
subtle, can we push that whole thing down into
xen_{remap,unmap}_domain_mfn_range too? This would make the requirement
on the caller be simple "never use vm_private", as opposed to now where
the requirement is "sometimes you might have to allocate some stuff and
stick it in here". The downside is that it pushes code which could be
generic down into per-arch stuff, although with suitable generic helper
functions this isn't so bad (whatever happened to
{alloc,free}_empty_pages_and_pagevec from the classic kernels? Those did
exactly what we want here, I think)

Ian.


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