| Message ID | de54075d-2d2f-e4ca-8252-c6f333571027@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | evtchn: recent XSAs follow-on | expand |
Hi Jan, On 28/09/2020 12:00, Jan Beulich wrote: > There's no need to expose them. We are going to need them for LiveUpdate and Non-cooperative Live Migration as the save/restore is happening outside of event_fifo.c. This is because we tried to keep all the save/restore code in a separate directory. Although, I could also see pros for scatter save/restore across the code base. Cheers, > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > I wonder whether we shouldn't do away with event_fifo.h altogether. > > --- a/xen/common/event_fifo.c > +++ b/xen/common/event_fifo.c > @@ -21,6 +21,27 @@ > > #include <public/event_channel.h> > > +struct evtchn_fifo_queue { > + uint32_t *head; /* points into control block */ > + uint32_t tail; > + uint8_t priority; > + spinlock_t lock; > +}; > + > +struct evtchn_fifo_vcpu { > + struct evtchn_fifo_control_block *control_block; > + struct evtchn_fifo_queue queue[EVTCHN_FIFO_MAX_QUEUES]; > +}; > + > +#define EVTCHN_FIFO_EVENT_WORDS_PER_PAGE (PAGE_SIZE / sizeof(event_word_t)) > +#define EVTCHN_FIFO_MAX_EVENT_ARRAY_PAGES \ > + (EVTCHN_FIFO_NR_CHANNELS / EVTCHN_FIFO_EVENT_WORDS_PER_PAGE) > + > +struct evtchn_fifo_domain { > + event_word_t *event_array[EVTCHN_FIFO_MAX_EVENT_ARRAY_PAGES]; > + unsigned int num_evtchns; > +}; > + > static inline event_word_t *evtchn_fifo_word_from_port(const struct domain *d, > unsigned int port) > { > --- a/xen/include/xen/event_fifo.h > +++ b/xen/include/xen/event_fifo.h > @@ -9,27 +9,6 @@ > #ifndef __XEN_EVENT_FIFO_H__ > #define __XEN_EVENT_FIFO_H__ > > -struct evtchn_fifo_queue { > - uint32_t *head; /* points into control block */ > - uint32_t tail; > - uint8_t priority; > - spinlock_t lock; > -}; > - > -struct evtchn_fifo_vcpu { > - struct evtchn_fifo_control_block *control_block; > - struct evtchn_fifo_queue queue[EVTCHN_FIFO_MAX_QUEUES]; > -}; > - > -#define EVTCHN_FIFO_EVENT_WORDS_PER_PAGE (PAGE_SIZE / sizeof(event_word_t)) > -#define EVTCHN_FIFO_MAX_EVENT_ARRAY_PAGES \ > - (EVTCHN_FIFO_NR_CHANNELS / EVTCHN_FIFO_EVENT_WORDS_PER_PAGE) > - > -struct evtchn_fifo_domain { > - event_word_t *event_array[EVTCHN_FIFO_MAX_EVENT_ARRAY_PAGES]; > - unsigned int num_evtchns; > -}; > - > int evtchn_fifo_init_control(struct evtchn_init_control *init_control); > int evtchn_fifo_expand_array(const struct evtchn_expand_array *expand_array); > void evtchn_fifo_destroy(struct domain *domain); >
On 29.09.2020 14:26, Julien Grall wrote: > On 28/09/2020 12:00, Jan Beulich wrote: >> There's no need to expose them. > > We are going to need them for LiveUpdate and Non-cooperative Live > Migration as the save/restore is happening outside of event_fifo.c. > > This is because we tried to keep all the save/restore code in a separate > directory. I'm afraid I don't view this as a reason for the change not to be made right now. If, when, and in what shape LU will hit upstream is unknown at the moment (unless I've missed some indication of a time line). In fact, if we didn't expose things like the ones here to far too wide an "audience", I wonder whether ... > Although, I could also see pros for scatter save/restore across the code > base. ... you wouldn't have chosen this route anyway, just to avoid exposing items widely that are supposed to be (almost) private. Jan
> -----Original Message----- > From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Jan Beulich > Sent: 28 September 2020 12:01 > To: xen-devel@lists.xenproject.org > Cc: Andrew Cooper <andrew.cooper3@citrix.com>; George Dunlap <George.Dunlap@eu.citrix.com>; Ian > Jackson <iwj@xenproject.org>; Julien Grall <julien@xen.org>; Wei Liu <wl@xen.org>; Stefano Stabellini > <sstabellini@kernel.org> > Subject: [PATCH 09/12] evtchn: move FIFO-private struct declarations > > There's no need to expose them. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > I wonder whether we shouldn't do away with event_fifo.h altogether. > +1 I can't see why it needs to exist. 2l doesn't have a header. Paul > --- a/xen/common/event_fifo.c > +++ b/xen/common/event_fifo.c > @@ -21,6 +21,27 @@ > > #include <public/event_channel.h> > > +struct evtchn_fifo_queue { > + uint32_t *head; /* points into control block */ > + uint32_t tail; > + uint8_t priority; > + spinlock_t lock; > +}; > + > +struct evtchn_fifo_vcpu { > + struct evtchn_fifo_control_block *control_block; > + struct evtchn_fifo_queue queue[EVTCHN_FIFO_MAX_QUEUES]; > +}; > + > +#define EVTCHN_FIFO_EVENT_WORDS_PER_PAGE (PAGE_SIZE / sizeof(event_word_t)) > +#define EVTCHN_FIFO_MAX_EVENT_ARRAY_PAGES \ > + (EVTCHN_FIFO_NR_CHANNELS / EVTCHN_FIFO_EVENT_WORDS_PER_PAGE) > + > +struct evtchn_fifo_domain { > + event_word_t *event_array[EVTCHN_FIFO_MAX_EVENT_ARRAY_PAGES]; > + unsigned int num_evtchns; > +}; > + > static inline event_word_t *evtchn_fifo_word_from_port(const struct domain *d, > unsigned int port) > { > --- a/xen/include/xen/event_fifo.h > +++ b/xen/include/xen/event_fifo.h > @@ -9,27 +9,6 @@ > #ifndef __XEN_EVENT_FIFO_H__ > #define __XEN_EVENT_FIFO_H__ > > -struct evtchn_fifo_queue { > - uint32_t *head; /* points into control block */ > - uint32_t tail; > - uint8_t priority; > - spinlock_t lock; > -}; > - > -struct evtchn_fifo_vcpu { > - struct evtchn_fifo_control_block *control_block; > - struct evtchn_fifo_queue queue[EVTCHN_FIFO_MAX_QUEUES]; > -}; > - > -#define EVTCHN_FIFO_EVENT_WORDS_PER_PAGE (PAGE_SIZE / sizeof(event_word_t)) > -#define EVTCHN_FIFO_MAX_EVENT_ARRAY_PAGES \ > - (EVTCHN_FIFO_NR_CHANNELS / EVTCHN_FIFO_EVENT_WORDS_PER_PAGE) > - > -struct evtchn_fifo_domain { > - event_word_t *event_array[EVTCHN_FIFO_MAX_EVENT_ARRAY_PAGES]; > - unsigned int num_evtchns; > -}; > - > int evtchn_fifo_init_control(struct evtchn_init_control *init_control); > int evtchn_fifo_expand_array(const struct evtchn_expand_array *expand_array); > void evtchn_fifo_destroy(struct domain *domain); >
On 30.09.2020 09:37, Paul Durrant wrote: >> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Jan Beulich >> Sent: 28 September 2020 12:01 >> >> There's no need to expose them. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> --- >> I wonder whether we shouldn't do away with event_fifo.h altogether. >> > > +1 > > I can't see why it needs to exist. 2l doesn't have a header. Okay, yet another patch then (I don't think I want to extend this one). Jan
Hi Jan, On 29/09/2020 13:49, Jan Beulich wrote: > On 29.09.2020 14:26, Julien Grall wrote: >> On 28/09/2020 12:00, Jan Beulich wrote: >>> There's no need to expose them. >> >> We are going to need them for LiveUpdate and Non-cooperative Live >> Migration as the save/restore is happening outside of event_fifo.c. >> >> This is because we tried to keep all the save/restore code in a separate >> directory. > > I'm afraid I don't view this as a reason for the change not to be made > right now. If, when, and in what shape LU will hit upstream is unknown > at the moment (unless I've missed some indication of a time line). I was merely pointing out long term use case as I vaguely remember a discussion in the past to avoid short term change. > In > fact, if we didn't expose things like the ones here to far too wide an > "audience", I wonder whether ... > >> Although, I could also see pros for scatter save/restore across the code >> base. > > ... you wouldn't have chosen this route anyway, just to avoid exposing > items widely that are supposed to be (almost) private. I would still chose that route because it helps to keep all the save/restore code in one place. FWIW, this follows what HVM context does today. Anyway, as I said, this I can see pros/cons for each. So at least this discussion clarify the preferred approach. For the patch itself: Acked-by: Julien Grall <jgrall@amazon.com> Cheers,
--- a/xen/common/event_fifo.c +++ b/xen/common/event_fifo.c @@ -21,6 +21,27 @@ #include <public/event_channel.h> +struct evtchn_fifo_queue { + uint32_t *head; /* points into control block */ + uint32_t tail; + uint8_t priority; + spinlock_t lock; +}; + +struct evtchn_fifo_vcpu { + struct evtchn_fifo_control_block *control_block; + struct evtchn_fifo_queue queue[EVTCHN_FIFO_MAX_QUEUES]; +}; + +#define EVTCHN_FIFO_EVENT_WORDS_PER_PAGE (PAGE_SIZE / sizeof(event_word_t)) +#define EVTCHN_FIFO_MAX_EVENT_ARRAY_PAGES \ + (EVTCHN_FIFO_NR_CHANNELS / EVTCHN_FIFO_EVENT_WORDS_PER_PAGE) + +struct evtchn_fifo_domain { + event_word_t *event_array[EVTCHN_FIFO_MAX_EVENT_ARRAY_PAGES]; + unsigned int num_evtchns; +}; + static inline event_word_t *evtchn_fifo_word_from_port(const struct domain *d, unsigned int port) { --- a/xen/include/xen/event_fifo.h +++ b/xen/include/xen/event_fifo.h @@ -9,27 +9,6 @@ #ifndef __XEN_EVENT_FIFO_H__ #define __XEN_EVENT_FIFO_H__ -struct evtchn_fifo_queue { - uint32_t *head; /* points into control block */ - uint32_t tail; - uint8_t priority; - spinlock_t lock; -}; - -struct evtchn_fifo_vcpu { - struct evtchn_fifo_control_block *control_block; - struct evtchn_fifo_queue queue[EVTCHN_FIFO_MAX_QUEUES]; -}; - -#define EVTCHN_FIFO_EVENT_WORDS_PER_PAGE (PAGE_SIZE / sizeof(event_word_t)) -#define EVTCHN_FIFO_MAX_EVENT_ARRAY_PAGES \ - (EVTCHN_FIFO_NR_CHANNELS / EVTCHN_FIFO_EVENT_WORDS_PER_PAGE) - -struct evtchn_fifo_domain { - event_word_t *event_array[EVTCHN_FIFO_MAX_EVENT_ARRAY_PAGES]; - unsigned int num_evtchns; -}; - int evtchn_fifo_init_control(struct evtchn_init_control *init_control); int evtchn_fifo_expand_array(const struct evtchn_expand_array *expand_array); void evtchn_fifo_destroy(struct domain *domain);
There's no need to expose them. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- I wonder whether we shouldn't do away with event_fifo.h altogether.