From: "Roger Pau Monné" <roger.pau@citrix.com>
To: Paul Durrant <Paul.Durrant@citrix.com>
Cc: "xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>,
Wei Liu <wl@xen.org>, Jan Beulich <jbeulich@suse.com>,
Andrew Cooper <Andrew.Cooper3@citrix.com>
Subject: Re: [Xen-devel] [PATCH v2 05/11] ioreq: add internal ioreq initialization support
Date: Thu, 26 Sep 2019 12:49:16 +0200 [thread overview]
Message-ID: <20190926104916.7pho64cfjmfakjsu@Air-de-Roger> (raw)
In-Reply-To: <133e6d6e83f846638ebdd9f66cef279b@AMSPEX02CL03.citrite.net>
On Tue, Sep 10, 2019 at 02:59:57PM +0200, Paul Durrant wrote:
> > -----Original Message-----
> > From: Roger Pau Monne <roger.pau@citrix.com>
> > Sent: 03 September 2019 17:14
> > To: xen-devel@lists.xenproject.org
> > Cc: Roger Pau Monne <roger.pau@citrix.com>; Jan Beulich <jbeulich@suse.com>; Andrew Cooper
> > <Andrew.Cooper3@citrix.com>; Wei Liu <wl@xen.org>; Paul Durrant <Paul.Durrant@citrix.com>
> > Subject: [PATCH v2 05/11] ioreq: add internal ioreq initialization support
> > @@ -736,33 +754,39 @@ static int hvm_ioreq_server_init(struct hvm_ioreq_server *s,
> > int rc;
> >
> > s->target = d;
> > -
> > - get_knownalive_domain(currd);
> > - s->emulator = currd;
> > -
> > spin_lock_init(&s->lock);
> > - INIT_LIST_HEAD(&s->ioreq_vcpu_list);
> > - spin_lock_init(&s->bufioreq_lock);
> > -
> > - s->ioreq.gfn = INVALID_GFN;
> > - s->bufioreq.gfn = INVALID_GFN;
> >
> > rc = hvm_ioreq_server_alloc_rangesets(s, id);
> > if ( rc )
> > return rc;
> >
> > - s->bufioreq_handling = bufioreq_handling;
> > -
> > - for_each_vcpu ( d, v )
> > + if ( !hvm_ioreq_is_internal(id) )
> > {
> > - rc = hvm_ioreq_server_add_vcpu(s, v);
> > - if ( rc )
> > - goto fail_add;
> > + get_knownalive_domain(currd);
> > +
> > + s->emulator = currd;
> > + INIT_LIST_HEAD(&s->ioreq_vcpu_list);
> > + spin_lock_init(&s->bufioreq_lock);
> > +
> > + s->ioreq.gfn = INVALID_GFN;
> > + s->bufioreq.gfn = INVALID_GFN;
> > +
> > + s->bufioreq_handling = bufioreq_handling;
> > +
> > + for_each_vcpu ( d, v )
> > + {
> > + rc = hvm_ioreq_server_add_vcpu(s, v);
> > + if ( rc )
> > + goto fail_add;
> > + }
> > }
> > + else
> > + s->handler = NULL;
>
> The struct is zeroed out so initializing the handler is not necessary.
>
> >
> > 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.
> > @@ -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.
> > 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.
Thanks, Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
next prev parent reply other threads:[~2019-09-26 10:49 UTC|newest]
Thread overview: 56+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-09-03 16:14 [Xen-devel] [PATCH v2 00/11] ioreq: add support for internal servers Roger Pau Monne
2019-09-03 16:14 ` [Xen-devel] [PATCH v2 01/11] ioreq: fix hvm_all_ioreq_servers_add_vcpu fail path cleanup Roger Pau Monne
2019-09-10 10:44 ` Paul Durrant
2019-09-10 13:28 ` Jan Beulich
2019-09-10 13:33 ` Roger Pau Monné
2019-09-10 13:35 ` Jan Beulich
2019-09-10 13:42 ` Roger Pau Monné
2019-09-10 13:53 ` Paul Durrant
2019-09-03 16:14 ` [Xen-devel] [PATCH v2 02/11] ioreq: terminate cf8 handling at hypervisor level Roger Pau Monne
2019-09-03 17:13 ` Andrew Cooper
2019-09-04 7:49 ` Roger Pau Monné
2019-09-04 8:00 ` Roger Pau Monné
2019-09-04 8:04 ` Jan Beulich
2019-09-04 9:46 ` Paul Durrant
2019-09-04 13:39 ` Roger Pau Monné
2019-09-04 13:56 ` Paul Durrant
2019-09-03 16:14 ` [Xen-devel] [PATCH v2 03/11] ioreq: switch selection and forwarding to use ioservid_t Roger Pau Monne
2019-09-10 12:31 ` Paul Durrant
2019-09-20 10:47 ` Jan Beulich
2019-09-03 16:14 ` [Xen-devel] [PATCH v2 04/11] ioreq: add fields to allow internal ioreq servers Roger Pau Monne
2019-09-10 12:34 ` Paul Durrant
2019-09-20 10:53 ` Jan Beulich
2019-09-03 16:14 ` [Xen-devel] [PATCH v2 05/11] ioreq: add internal ioreq initialization support Roger Pau Monne
2019-09-10 12:59 ` Paul Durrant
2019-09-26 10:49 ` Roger Pau Monné [this message]
2019-09-26 10:58 ` Paul Durrant
2019-09-20 11:15 ` Jan Beulich
2019-09-26 10:51 ` Roger Pau Monné
2019-09-03 16:14 ` [Xen-devel] [PATCH v2 06/11] ioreq: allow dispatching ioreqs to internal servers Roger Pau Monne
2019-09-10 13:06 ` Paul Durrant
2019-09-20 11:35 ` Jan Beulich
2019-09-26 11:14 ` Roger Pau Monné
2019-09-26 13:17 ` Jan Beulich
2019-09-26 13:46 ` Roger Pau Monné
2019-09-26 15:13 ` Jan Beulich
2019-09-26 15:59 ` Roger Pau Monné
2019-09-27 8:17 ` Paul Durrant
2019-09-26 16:36 ` Roger Pau Monné
2019-09-03 16:14 ` [Xen-devel] [PATCH v2 07/11] ioreq: allow registering internal ioreq server handler Roger Pau Monne
2019-09-10 13:12 ` Paul Durrant
2019-09-03 16:14 ` [Xen-devel] [PATCH v2 08/11] ioreq: allow decoding accesses to MMCFG regions Roger Pau Monne
2019-09-10 13:37 ` Paul Durrant
2019-09-03 16:14 ` [Xen-devel] [PATCH v2 09/11] vpci: register as an internal ioreq server Roger Pau Monne
2019-09-10 13:49 ` Paul Durrant
2019-09-26 15:07 ` Roger Pau Monné
2019-09-27 8:29 ` Paul Durrant
2019-09-27 8:45 ` Roger Pau Monné
2019-09-27 9:01 ` Paul Durrant
2019-09-27 10:46 ` Roger Pau Monné
2019-09-03 16:14 ` [Xen-devel] [PATCH v2 10/11] ioreq: split the code to detect PCI config space accesses Roger Pau Monne
2019-09-10 14:06 ` Paul Durrant
2019-09-26 16:05 ` Roger Pau Monné
2019-09-03 16:14 ` [Xen-devel] [PATCH v2 11/11] ioreq: provide support for long-running operations Roger Pau Monne
2019-09-10 14:14 ` Paul Durrant
2019-09-10 14:28 ` Roger Pau Monné
2019-09-10 14:40 ` Paul Durrant
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190926104916.7pho64cfjmfakjsu@Air-de-Roger \
--to=roger.pau@citrix.com \
--cc=Andrew.Cooper3@citrix.com \
--cc=Paul.Durrant@citrix.com \
--cc=jbeulich@suse.com \
--cc=wl@xen.org \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).