Re: [Xen-devel] [PATCH v2 05/11] ioreq: add internal ioreq initialization support

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
[snip]
> > >
> > >      return 0;
> > >
> > >   fail_add:
> > > +    ASSERT(!hvm_ioreq_is_internal(id));
> > >      hvm_ioreq_server_remove_all_vcpus(s);
> > >      hvm_ioreq_server_unmap_pages(s);
> > >
> >
> > I think it would be worthwhile having that ASSERT repeated in the called 
> > functions, and other
> functions that only operate on external ioreq servers e.g. 
> hvm_ioreq_server_add_vcpu(),
> hvm_ioreq_server_map_pages(), etc.
> 
> That's fine, but then I would also need to pass the ioreq server id to
> those functions just to perform the ASSERT. I will leave those as-is
> because I think passing the id just for that ASSERT is kind of
> pointless.

Oh, I was misremembering the id being recorded in the struct. I guess that was 
when ioreq servers were in a list rather than an array. Indeed there's no point 
in passing an id just to ASSERT on it.

> 
> > > @@ -864,20 +897,21 @@ int hvm_destroy_ioreq_server(struct domain *d, 
> > > ioservid_t id)
> > >          goto out;
> > >
> > >      rc = -EPERM;
> > > -    if ( s->emulator != current->domain )
> > > +    /* NB: internal servers cannot be destroyed. */
> > > +    if ( hvm_ioreq_is_internal(id) || s->emulator != current->domain )
> >
> > Shouldn't the test of hvm_ioreq_is_internal(id) simply be an ASSERT? This 
> > function should only be
> called from a dm_op(), right?
> 
> Right, I think I've wrongly assumed this was also called when
> destroying a domain, but domain destruction uses
> hvm_destroy_all_ioreq_servers instead.
> 

That's right.

> > >          goto out;
> > >
> > >      domain_pause(d);
> > >
> > >      p2m_set_ioreq_server(d, 0, id);
> > >
> > > -    hvm_ioreq_server_disable(s);
> > > +    hvm_ioreq_server_disable(s, hvm_ioreq_is_internal(id));
> > >
> > >      /*
> > >       * It is safe to call hvm_ioreq_server_deinit() prior to
> > >       * set_ioreq_server() since the target domain is paused.
> > >       */
> > > -    hvm_ioreq_server_deinit(s);
> > > +    hvm_ioreq_server_deinit(s, hvm_ioreq_is_internal(id));
> > >      set_ioreq_server(d, id, NULL);
> > >
> > >      domain_unpause(d);
> > > @@ -909,7 +943,8 @@ int hvm_get_ioreq_server_info(struct domain *d, 
> > > ioservid_t id,
> > >          goto out;
> > >
> > >      rc = -EPERM;
> > > -    if ( s->emulator != current->domain )
> > > +    /* NB: don't allow fetching information from internal ioreq servers. 
> > > */
> > > +    if ( hvm_ioreq_is_internal(id) || s->emulator != current->domain )
> >
> > Again here, and several places below.
> 
> I've fixed the calls to hvm_get_ioreq_server_info,
> hvm_get_ioreq_server_frame and hvm_map_mem_type_to_ioreq_server to
> include an ASSERT that the passed ioreq is not internal.
> 

Cool. Thanks,

  Paul

> Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel