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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Mon, 2012-09-24 at 15:24 +0100, Stefano Stabellini wrote:
> On Fri, 21 Sep 2012, Mukesh Rathor wrote:
> > ---
> >  drivers/xen/privcmd.c |   77 
> > ++++++++++++++++++++++++++++++++++++++++++++----
> >  1 files changed, 70 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
> > index ccee0f1..195d89f 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"
> >  
> > @@ -178,7 +179,7 @@ static int mmap_mfn_range(void *data, void *state)
> >                                     msg->va & PAGE_MASK,
> >                                     msg->mfn, msg->npages,
> >                                     vma->vm_page_prot,
> > -                                   st->domain);
> > +                                   st->domain, NULL);
> >     if (rc < 0)
> >             return rc;
> >  
> > @@ -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_pv_domain() && xen_feature(XENFEAT_auto_translated_physmap))
> > +           return -ENOSYS;
> > +
> >     if (copy_from_user(&mmapcmd, udata, sizeof(mmapcmd)))
> >             return -EFAULT;
> >  
> > @@ -251,13 +256,18 @@ struct mmap_batch_state {
> >     xen_pfn_t __user *user;
> >  };
> >  
> > +/* PVH dom0 fyi: 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;
> >     struct mmap_batch_state *st = state;
> > +   struct vm_area_struct *vma = st->vma;
> > +   struct xen_pvh_pfn_info *pvhp = vma ? vma->vm_private_data : NULL;
> >  
> > -   if (xen_remap_domain_mfn_range(st->vma, st->va & PAGE_MASK, *mfnp, 1,
> > -                                  st->vma->vm_page_prot, st->domain) < 0) {
> > +   if (xen_remap_domain_mfn_range(vma, st->va & PAGE_MASK, *mfnp, 1,
> > +                                  vma->vm_page_prot, st->domain, 
> > +                                  pvhp) < 0) {
> >             *mfnp |= 0xf0000000U;
> >             st->err++;
> >     }
> 
> I don't like that a parameter like "xen_pvh_pfn_info" has been added to
> a generic arch-agnostic function like xen_remap_domain_mfn_range.

This might have been my suggestion but actually I was thinking more
along the lines of what you suggest :

> If you need to pass more parameters to xen_remap_domain_mfn_range, it
> should be done in a cross-architecture way. In fact, keep in mind that
> privcmd.c compiles on ARM (and IA64?) as well.
> 
> I think that in this particular case you are using pvh to actually
> specify auto_translate_physmap, am I correct?
> Maybe we just need to rename xen_pvh_pfn_info to xen_xlat_pfn_info.

Or perhaps passing pvhp->pages as the new argument.

> > @@ -315,6 +359,12 @@ static long privcmd_ioctl_mmap_batch(void __user 
> > *udata)
> >             goto out;
> >     }
> >  
> > +   if (xen_pv_domain() && xen_feature(XENFEAT_auto_translated_physmap)) {
> > +           if ((ret=pvh_privcmd_resv_pfns(vma, m.num))) {
> 
> I would change this into:
> 
>     if ((ret = pvh_privcmd_resv_pfns(vma, m.num)) < 0) {

I thought assignment in if statements was frowned upon by CodingStyle,
although having looked the only bit which backs that up is "Kernel
coding style is super simple.  Avoid tricky expressions." which isn't
exactly explicit.

Ian.


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