Re: [Xen-devel] [PATCH (V9) 2/2] xen: Add V4V implementation

[Ross Philipson <ross.philipson@xxxxxxxxxx>]

On 05/29/2013 04:34 AM, Jan Beulich wrote:
> > > > On 28.05.13 at 21:43, Ross Philipson<ross.philipson@xxxxxxxxxx>  wrote:
> > @@ -320,6 +328,8 @@ struct domain *domain_create(
> >       xfree(d->mem_event);
> >       if ( init_status&  INIT_arch )
> >           arch_domain_destroy(d);
> > +    if ( init_status&  INIT_v4v )
> > +       v4v_destroy(d);
> Hard tab.
>
> > +static long
> > +v4v_ringbuf_insertv(struct domain *d,
> > +                    struct v4v_ring_info *ring_info,
> > +                    v4v_ring_id_t *src_id, uint32_t message_type,
> > +                    XEN_GUEST_HANDLE_PARAM(v4v_iov_t) iovs,
> > +                    uint32_t niov, size_t len)
> > +{
> > ...
> > +    do {
> > ...
> > +    } while ( 0 );
> > +
> > +    v4v_ring_unmap(ring_info);
> With the unmapping of all pages getting deferred till here - is there
> a sensible upper limit on the number of accumulated mappings? Or
> are you blindly running the host out of mapping space?
It is limited by the maximum size of the ring that was created. In 
theory that could be large. I could look at freeing them in the loop as 
it progresses.
> > +static int
> > +v4v_find_ring_mfns(struct domain *d, struct v4v_ring_info *ring_info,
> > +                   uint32_t npage, XEN_GUEST_HANDLE_PARAM(v4v_pfn_t) pfn_hnd)
> > +{
> > ...
> > +    for ( i = 0; i<  npage; ++i )
> > +    {
> > +        unsigned long pfn;
> If you use scope restricted local variables (which I appreciate),
> please do so consistently at least within functions. I'm particularly
> thinking of "t" here...
I will go through and make this consistent.

> > +
> > +        if ( copy_from_guest_offset(&pfn, pfn_hnd, i, 1) )
> > +        {
> > +            ret = -EFAULT;
> > +            v4v_dprintk("EFAULT\n");
> > +            break;
> > +        }
> > +
> > +        mfn = mfn_x(get_gfn(d, pfn,&t));
> > +        if ( !mfn_valid(mfn) )
> > +        {
> > +            put_gfn(d, pfn);
> > +            printk(KERN_ERR "v4v domain %d passed invalid mfn %"PRI_mfn" ring %p 
> > seq %d\n",
> > +                    d->domain_id, mfn, ring_info, i);
> > +            ret = -EINVAL;
> > +            break;
> No handling of paged out or shared pages here, and not even a
> comment saying this needs further adjustment?
Right, need to adddress this. I guess we should be looking for 
p2m_is_shared/p2m_is_paging and avoid those types. There may be other 
types to avoid too like p2m_is_grant etc.
> Also in code that isn't a Linux clone, please use XENLOG_ instead
> of KERN_ for message levels. And you absolutely have to use
> XENLOG_G_ for messages concerning guest activities.
>
> And then you properly use PRI_mfn here, yet elsewhere I saw MFNs
> getting printed using bogus (void *) casts. This also calls for being
> done consistently.
>
> Finally, also printing the PFN here might aid guest side debugging.
Wilco

> > +static void v4v_ring_remove_mfns(struct domain *d, struct v4v_ring_info 
> > *ring_info)
> > +{
> > ...
> > +    if ( ring_info->mfn_mapping )
> > +        xfree(ring_info->mfn_mapping);
> Pointless if().
>
> > +#define V4V_RING_MAGIC          0xa822f72bb0b9d8ccUL
> > +#define V4V_RING_DATA_MAGIC    0x45fe852220b801d4UL
> Hard tab.
>
> > +#define V4V_MESSAGE_DGRAM       0x3c2c1db8
> > +#define V4V_MESSAGE_STREAM     0x70f6a8e5
> Again. Was this really cleaned up, as the overview patch said?
It was cleaned up. I inherited the patch set and went through it. In the 
diff from version 8 and the inherited set I saw quite a bit of cleanup. 
But that is not an excuse - I will clean the rest up, sorry.
> > --- a/xen/include/xen/sched.h
> > +++ b/xen/include/xen/sched.h
> > @@ -24,6 +24,7 @@
> >   #include<public/sysctl.h>
> >   #include<public/vcpu.h>
> >   #include<public/mem_event.h>
> > +#include<xen/v4v.h>
> Please don't, or if you absolutely have to, not after the public
> headers, but along with the other xen/ ones.
>
> > --- /dev/null
> > +++ b/xen/include/xen/v4v.h
> > ...
> > +struct v4v_domain;
> What is this good for? There's no use in any of the following
> function declarations.
It is a forward declaration of the main internal v4v struct. It is used 
in sched.h in the domain struct - thus the #include you asked about 
earlier. I will of course move that include.
Thanks
Ross

> Jan

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