On Jo, 2017-08-24 at 07:24 -0600, Jan Beulich wrote:
> >
> > >
> > > >
> > > > On 24.08.17 at 13:48, <aisa...@bitdefender.com> wrote:
> > The patch splits the vm_event into three structures:vm_event_share,
> > vm_event_paging, vm_event_monitor. The allocation for the
> > structure is moved to vm_event_enable so that it can be
> > allocated/init when needed and freed in vm_event_disable.
> >
> > Signed-off-by: Alexandru Isaila <aisa...@bitdefender.com>
> Missing brief description of changes from v1 here.
>
> >
> > @@ -50,32 +50,37 @@ static int vm_event_enable(
> > int rc;
> > unsigned long ring_gfn = d->arch.hvm_domain.params[param];
> >
> > + if ( !(*ved) )
> > + (*ved) = xzalloc(struct vm_event_domain);
> > + if ( !(*ved) )
> In none of the three cases you really need the parentheses around
> *ved.
>
> >
> > @@ -187,39 +194,45 @@ void vm_event_wake(struct domain *d, struct
> > vm_event_domain *ved)
> > vm_event_wake_blocked(d, ved);
> > }
> >
> > -static int vm_event_disable(struct domain *d, struct
> > vm_event_domain *ved)
> > +static int vm_event_disable(struct domain *d, struct
> > vm_event_domain **ved)
> > {
> > - if ( ved->ring_page )
> > + if ( !*ved )
> > + return 0;
> > +
> > + if ( (*ved)->ring_page )
> > {
> > [...]
> > + xfree(*ved);
> > + *ved = NULL;
> > }
> If both if()-s above are really useful, you are leaking *ved when it
> is non-NULL but ->ring_page is NULL.
>
> >
> > @@ -500,6 +519,9 @@ bool_t vm_event_check_ring(struct
> > vm_event_domain *ved)
> > int __vm_event_claim_slot(struct domain *d, struct vm_event_domain
> > *ved,
> > bool_t allow_sleep)
> > {
> > + if ( !ved )
> > + return -ENOSYS;
> I don't think ENOSYS is correct here.
Can you tell me what is the preferred return value here?
>
> >
> > @@ -510,24 +532,24 @@ int __vm_event_claim_slot(struct domain *d,
> > struct vm_event_domain *ved,
> > /* Registered with Xen-bound event channel for incoming
> > notifications. */
> > static void mem_paging_notification(struct vcpu *v, unsigned int
> > port)
> > {
> > - if ( likely(v->domain->vm_event->paging.ring_page != NULL) )
> > - vm_event_resume(v->domain, &v->domain->vm_event->paging);
> > + if ( likely(v->domain->vm_event_paging->ring_page != NULL) )
> > + vm_event_resume(v->domain, v->domain->vm_event_paging);
> > }
> > #endif
> >
> > /* Registered with Xen-bound event channel for incoming
> > notifications. */
> > static void monitor_notification(struct vcpu *v, unsigned int
> > port)
> > {
> > - if ( likely(v->domain->vm_event->monitor.ring_page != NULL) )
> > - vm_event_resume(v->domain, &v->domain->vm_event->monitor);
> > + if ( likely(v->domain->vm_event_monitor->ring_page != NULL) )
> > + vm_event_resume(v->domain, v->domain->vm_event_monitor);
> > }
> >
> > #ifdef CONFIG_HAS_MEM_SHARING
> > /* Registered with Xen-bound event channel for incoming
> > notifications. */
> > static void mem_sharing_notification(struct vcpu *v, unsigned int
> > port)
> > {
> > - if ( likely(v->domain->vm_event->share.ring_page != NULL) )
> > - vm_event_resume(v->domain, &v->domain->vm_event->share);
> > + if ( likely(v->domain->vm_event_share->ring_page != NULL) )
> > + vm_event_resume(v->domain, v->domain->vm_event_share);
> > }
> > #endif
> For all three a local variable holding v->domain would certain help;
> eventually the functions should even be passed struct domain *
> instead of struct vcpu *, I think.
>
> >
> > @@ -599,7 +621,6 @@ int vm_event_domctl(struct domain *d,
> > xen_domctl_vm_event_op_t *vec,
> > #ifdef CONFIG_HAS_MEM_PAGING
> > case XEN_DOMCTL_VM_EVENT_OP_PAGING:
> > {
> > - struct vm_event_domain *ved = &d->vm_event->paging;
> Dropping this local variable (and similar ones below) pointlessly
> increases the size of this patch.
I have dropped the local var because in case of a XEN_VM_EVENT_ENABLE
d->vm_event_... is allocated in the vm_event_enable function below so
it will allocate mem for the local variable.
>
> >
> > --- a/xen/drivers/passthrough/pci.c
> > +++ b/xen/drivers/passthrough/pci.c
> > @@ -1363,9 +1363,11 @@ static int assign_device(struct domain *d,
> > u16 seg, u8 bus, u8 devfn, u32 flag)
> >
> > /* Prevent device assign if mem paging or mem sharing have
> > been
> > * enabled for this domain */
> > + if( !d->vm_event_paging )
> > + return -EXDEV;
> Is this check the wrong way round? And why can't it be combined
> with ...
>
> >
> > if ( unlikely(!need_iommu(d) &&
> > (d->arch.hvm_domain.mem_sharing_enabled ||
> > - d->vm_event->paging.ring_page ||
> > + d->vm_event_paging->ring_page ||
> > p2m_get_hostp2m(d)->global_logdirty)) )
> > return -EXDEV;
> ... this set?
>
> Jan
Alex
>
> ________________________
> This email was scanned by Bitdefender
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
