[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Roger Pau Monne <roger.pau@xxxxxxxxxx>
  • From: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
  • Date: Thu, 26 Sep 2019 10:58:07 +0000
  • Accept-language: en-GB, en-US
  • Authentication-results: esa3.hc3370-68.iphmx.com; dkim=none (message not signed) header.i=none; spf=None smtp.pra=Paul.Durrant@xxxxxxxxxx; spf=Pass smtp.mailfrom=Paul.Durrant@xxxxxxxxxx; spf=None smtp.helo=postmaster@xxxxxxxxxxxxxxx
  • Cc: "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Jan Beulich <jbeulich@xxxxxxxx>, Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>
  • Delivery-date: Thu, 26 Sep 2019 10:58:15 +0000
  • Ironport-sdr: KLX8+v/ROk5VV++c3pgJb+/d9BZFeoEQ6AnkWdv2pF5cloaGrvCdS15FBSQEoPP3N7qx1ctjpY 8cXFGU7MI004FuNGI3rknjiC2NswjHjmLYoF9xCYz+7HUNZKBuaqp2y/2vD2oFwrsBSfUFBuvq A1L5VxU9nJ1JlXaKA5HscggrmmPDQm8waVAWMmjHSdgeaC+xzzy5UDjIejIpdQTUXrjHGGnlZR YFJ/v4fyNxrYKaheXHdHU0V/TdtATGXF3ITXaCRycPGepUx31Tld+mbxesg1uCaYqQtis8eXJD TwY=
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Thread-index: AQHVYnK2oKxcpVD8uUiJJhtdNoUtKqck5f9wgBjjqQCAACLP4A==
  • Thread-topic: [PATCH v2 05/11] ioreq: add internal ioreq initialization support

> -----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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.