Re: [Xen-devel] [RFC PATCH V3 11/12] xen/vm_event: Decouple vm_event and mem_access.

[Tamas K Lengyel <tamas.lengyel@xxxxxxxxxxxx>]

On Wed, Feb 4, 2015 at 10:47 AM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>> On 29.01.15 at 22:46, <tamas.lengyel@xxxxxxxxxxxx> wrote:
>> --- a/xen/common/Makefile
>> +++ b/xen/common/Makefile
>> @@ -52,9 +52,10 @@ obj-y += tmem_xen.o
>>  obj-y += radix-tree.o
>>  obj-y += rbtree.o
>>  obj-y += lzo.o
>> +obj-y += vm_event.o
>> +obj-y += monitor.o
>
> Please make the (not) sorting situation worse than it already is - the
> original intention was for entries to be alphabetically sorted here.

I'm just going to sort the list in this patch to have everything in
alphabetic order.

>
>> --- /dev/null
>> +++ b/xen/common/monitor.c
>> @@ -0,0 +1,64 @@
>> +/******************************************************************************
>> + * monitor.c
>> + *
>> + * VM event monitor support.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program; if not, write to the Free Software
>> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
>> + */
>> +
>> +#include <xen/sched.h>
>> +#include <xen/vm_event.h>
>> +#include <xen/mem_access.h>
>> +
>> +void monitor_resume(struct domain *d)
>> +{
>> +    vm_event_response_t rsp;
>> +
>> +    /* Pull all responses off the ring. */
>> +    while ( vm_event_get_response(d, &d->vm_event->monitor, &rsp) )
>> +    {
>> +        struct vcpu *v;
>> +
>> +        if ( rsp.version != VM_EVENT_INTERFACE_VERSION )
>> +            continue;
>> +
>> +#ifndef NDEBUG
>> +        if ( rsp.flags & VM_EVENT_FLAG_DUMMY )
>> +            continue;
>> +#endif
>
> This code was unconditional in the original function. Such a change
> needs mentioning in the description.

Ah, yes, the patch that makes this conditional has to be moved up
before this one.

>
>> @@ -49,7 +53,7 @@ int mem_access_send_req(struct domain *d, 
>> vm_event_request_t *req)
>>      return -ENOSYS;
>>  }
>>
>> -static inline void mem_access_resume(struct domain *d) {}
>> +static inline void mem_access_resume(struct vcpu *vcpu, vm_event_response_t 
>> *rsp) {}
>
> Long line.
>
>> --- /dev/null
>> +++ b/xen/include/xen/monitor.h
>> @@ -0,0 +1,38 @@
>> +/******************************************************************************
>> + * monitor.h
>> + *
>> + * Monitor event support.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program; if not, write to the Free Software
>> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
>> + */
>> +
>> +#ifndef _XEN_ASM_MONITOR_H
>> +#define _XEN_ASM_MONITOR_H
>> +
>> +#include <public/memory.h>
>
> This can't be enough (nor can I see why it's needed here), or else ...
>
>> +/* Resumes the running of the VCPU, restarting the last instruction */
>> +void monitor_resume(struct domain *d);
>
> ... struct domain may end up being a forward reference (with scope
> restricted to monitor_resume()).
>
> Jan

I'll revisit the headers needed for this one but it did build fine.

Thanks,
Tamas

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