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

[Ross Philipson <ross.philipson@citrix.com>]

On 05/29/2013 04:34 AM, Jan Beulich wrote:
>>>> On 28.05.13 at 21:43, Ross Philipson<ross.philipson@citrix.com>  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
>