| Message ID | 1457072152-16128-20-git-send-email-zhaoshenglong@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 04.03.16 at 07:15, <zhaoshenglong@huawei.com> wrote: > From: Shannon Zhao <shannon.zhao@linaro.org> > > Add a new delivery type: > val[63:56] == 3: val[15:8] is flag: val[7:0] is a PPI. > To the flag, bit 8 stands the interrupt mode is edge(1) or level(0) and > bit 9 stands the interrupt polarity is active low(1) or high(0). > > Cc: Jan Beulich <jbeulich@suse.com> > Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org> The set of Cc-s is too narrow - all REST maintainers should be copied. > --- a/xen/include/public/hvm/params.h > +++ b/xen/include/public/hvm/params.h > @@ -55,6 +55,16 @@ > * if this delivery method is available. > */ > > +#define HVM_PARAM_CALLBACK_TYPE_EVENT 3 > +/* > + * val[55:16] need to be zero. > + * val[15:8] is flag of event-channel interrupt: > + * bit 8: interrupt is edge(1) or level(0) triggered > + * bit 9: interrupt is active low(1) or high(0) > + * val[7:0] is PPI number used by event-channel. > + * This is only used by ARM/ARM64. > + */ I think the name of the constant needs improvement. The low 8 bits make this extremely ARM specific, so perhaps HVM_PARAM_CALLBACK_TYPE_PPI? Albeit - wouldn't the vector and/or GSI ones be re-usable for this purpose? (I don't know enough about ARM to be certain.) And then I'm now also wondering about the description of bits 8 and 9 - event channels don't know of edge/level triggering or high/low polarity, so it looks to me as if the comment is at least misleading too. Jan
On Fri, 4 Mar 2016, Jan Beulich wrote: > >>> On 04.03.16 at 07:15, <zhaoshenglong@huawei.com> wrote: > > From: Shannon Zhao <shannon.zhao@linaro.org> > > > > Add a new delivery type: > > val[63:56] == 3: val[15:8] is flag: val[7:0] is a PPI. > > To the flag, bit 8 stands the interrupt mode is edge(1) or level(0) and > > bit 9 stands the interrupt polarity is active low(1) or high(0). > > > > Cc: Jan Beulich <jbeulich@suse.com> > > Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org> > > The set of Cc-s is too narrow - all REST maintainers should be copied. > > > --- a/xen/include/public/hvm/params.h > > +++ b/xen/include/public/hvm/params.h > > @@ -55,6 +55,16 @@ > > * if this delivery method is available. > > */ > > > > +#define HVM_PARAM_CALLBACK_TYPE_EVENT 3 > > +/* > > + * val[55:16] need to be zero. > > + * val[15:8] is flag of event-channel interrupt: > > + * bit 8: interrupt is edge(1) or level(0) triggered > > + * bit 9: interrupt is active low(1) or high(0) > > + * val[7:0] is PPI number used by event-channel. > > + * This is only used by ARM/ARM64. > > + */ > > I think the name of the constant needs improvement. The low 8 > bits make this extremely ARM specific, so perhaps > HVM_PARAM_CALLBACK_TYPE_PPI? That would be OK for me. > Albeit - wouldn't the > vector and/or GSI ones be re-usable for this purpose? (I > don't know enough about ARM to be certain.) Vector is an x86 thing, while GSI is an ACPI term. There is probably a PPI to GSI mapping on ARM ACPI systems, but I wouldn't want this HVM_PARAM to work only with ACPI: it should work with device tree systems too, where GSI has no meaning. > And then I'm now also wondering about the description of bits > 8 and 9 - event channels don't know of edge/level triggering > or high/low polarity, so it looks to me as if the comment is at > least misleading too.
>>> On 04.03.16 at 13:09, <stefano.stabellini@eu.citrix.com> wrote: > On Fri, 4 Mar 2016, Jan Beulich wrote: >> >>> On 04.03.16 at 07:15, <zhaoshenglong@huawei.com> wrote: >> > From: Shannon Zhao <shannon.zhao@linaro.org> >> > >> > Add a new delivery type: >> > val[63:56] == 3: val[15:8] is flag: val[7:0] is a PPI. >> > To the flag, bit 8 stands the interrupt mode is edge(1) or level(0) and >> > bit 9 stands the interrupt polarity is active low(1) or high(0). >> > >> > Cc: Jan Beulich <jbeulich@suse.com> >> > Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org> >> >> The set of Cc-s is too narrow - all REST maintainers should be copied. >> >> > --- a/xen/include/public/hvm/params.h >> > +++ b/xen/include/public/hvm/params.h >> > @@ -55,6 +55,16 @@ >> > * if this delivery method is available. >> > */ >> > >> > +#define HVM_PARAM_CALLBACK_TYPE_EVENT 3 >> > +/* >> > + * val[55:16] need to be zero. >> > + * val[15:8] is flag of event-channel interrupt: >> > + * bit 8: interrupt is edge(1) or level(0) triggered >> > + * bit 9: interrupt is active low(1) or high(0) >> > + * val[7:0] is PPI number used by event-channel. >> > + * This is only used by ARM/ARM64. >> > + */ >> >> I think the name of the constant needs improvement. The low 8 >> bits make this extremely ARM specific, so perhaps >> HVM_PARAM_CALLBACK_TYPE_PPI? > > That would be OK for me. > > >> Albeit - wouldn't the >> vector and/or GSI ones be re-usable for this purpose? (I >> don't know enough about ARM to be certain.) > > Vector is an x86 thing, while GSI is an ACPI term. There is probably a > PPI to GSI mapping on ARM ACPI systems, but I wouldn't want this > HVM_PARAM to work only with ACPI: it should work with device tree > systems too, where GSI has no meaning. Okay. Then how about making _VECTOR x86-specific, and the new one ARM-specific (via #ifdef), at once allowing (if so wanted) both to share the same numeric value? Jan
On Fri, 4 Mar 2016, Jan Beulich wrote: > >>> On 04.03.16 at 13:09, <stefano.stabellini@eu.citrix.com> wrote: > > On Fri, 4 Mar 2016, Jan Beulich wrote: > >> >>> On 04.03.16 at 07:15, <zhaoshenglong@huawei.com> wrote: > >> > From: Shannon Zhao <shannon.zhao@linaro.org> > >> > > >> > Add a new delivery type: > >> > val[63:56] == 3: val[15:8] is flag: val[7:0] is a PPI. > >> > To the flag, bit 8 stands the interrupt mode is edge(1) or level(0) and > >> > bit 9 stands the interrupt polarity is active low(1) or high(0). > >> > > >> > Cc: Jan Beulich <jbeulich@suse.com> > >> > Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org> > >> > >> The set of Cc-s is too narrow - all REST maintainers should be copied. > >> > >> > --- a/xen/include/public/hvm/params.h > >> > +++ b/xen/include/public/hvm/params.h > >> > @@ -55,6 +55,16 @@ > >> > * if this delivery method is available. > >> > */ > >> > > >> > +#define HVM_PARAM_CALLBACK_TYPE_EVENT 3 > >> > +/* > >> > + * val[55:16] need to be zero. > >> > + * val[15:8] is flag of event-channel interrupt: > >> > + * bit 8: interrupt is edge(1) or level(0) triggered > >> > + * bit 9: interrupt is active low(1) or high(0) > >> > + * val[7:0] is PPI number used by event-channel. > >> > + * This is only used by ARM/ARM64. > >> > + */ > >> > >> I think the name of the constant needs improvement. The low 8 > >> bits make this extremely ARM specific, so perhaps > >> HVM_PARAM_CALLBACK_TYPE_PPI? > > > > That would be OK for me. > > > > > >> Albeit - wouldn't the > >> vector and/or GSI ones be re-usable for this purpose? (I > >> don't know enough about ARM to be certain.) > > > > Vector is an x86 thing, while GSI is an ACPI term. There is probably a > > PPI to GSI mapping on ARM ACPI systems, but I wouldn't want this > > HVM_PARAM to work only with ACPI: it should work with device tree > > systems too, where GSI has no meaning. > > Okay. Then how about making _VECTOR x86-specific, and the > new one ARM-specific (via #ifdef), at once allowing (if so > wanted) both to share the same numeric value? Fine by me
On Fri, Mar 04, 2016 at 02:15:49PM +0800, Shannon Zhao wrote: > From: Shannon Zhao <shannon.zhao@linaro.org> > > Add a new delivery type: > val[63:56] == 3: val[15:8] is flag: val[7:0] is a PPI. > To the flag, bit 8 stands the interrupt mode is edge(1) or level(0) and > bit 9 stands the interrupt polarity is active low(1) or high(0). > > Cc: Jan Beulich <jbeulich@suse.com> > Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org> > --- > v5: fix the statement > --- > xen/include/public/hvm/params.h | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/xen/include/public/hvm/params.h b/xen/include/public/hvm/params.h > index 73d4718..2ea8d62 100644 > --- a/xen/include/public/hvm/params.h > +++ b/xen/include/public/hvm/params.h > @@ -55,6 +55,16 @@ > * if this delivery method is available. > */ > > +#define HVM_PARAM_CALLBACK_TYPE_EVENT 3 > +/* > + * val[55:16] need to be zero. > + * val[15:8] is flag of event-channel interrupt: > + * bit 8: interrupt is edge(1) or level(0) triggered > + * bit 9: interrupt is active low(1) or high(0) > + * val[7:0] is PPI number used by event-channel. > + * This is only used by ARM/ARM64. How do you deal with masking and EOI? One of the things that I have realized when looking at vAPIC, HVMOP_set_evtchn_upcall_vector, and this vector callback is that it is a bit ... wild west. On x86 when we inject the vector - the Linux (and FreeBSD) call right in the event channel code. Both of them clear the evtchn_upcall_pending right away but do not set evtchn_upcall_mask. That means if the hypervisor is told to set another event (via EVTCHNOP_SEND) that is say more than 64 bits away from the other prior one (so that the check for evtchn_pending_sel succeeds) it can do so (see evtchn_2l_set_pending and vcpu_mark_events_pending). And then we can go on sending an IPI, causing the guest an VMEXIT, and reinjection of the vector callback. ... I think if the events are spaced just right you can do this injection up to 16 times. Granted the 16th callback will work just fine and work on all the events and then EOI all of them. And the other 15 will just exit out since evtchn_pending_sel will be zero. But it is a nuisance. Anyhow what I am wondering if there are some semantincs when it comes to PPI and it being able to 'mask' an vector until it exits or such? If so you should document that. [Yes, I am going to send a patch for the the above 'interesting' behavior]. > /* > * These are not used by Xen. They are here for convenience of HVM-guest > * xenbus implementations. > -- > 2.0.4 > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel
Hi Jan, On 04/03/2016 10:16, Jan Beulich wrote: >>>> On 04.03.16 at 07:15, <zhaoshenglong@huawei.com> wrote: [...] >> --- a/xen/include/public/hvm/params.h >> +++ b/xen/include/public/hvm/params.h >> @@ -55,6 +55,16 @@ >> * if this delivery method is available. >> */ >> >> +#define HVM_PARAM_CALLBACK_TYPE_EVENT 3 >> +/* >> + * val[55:16] need to be zero. >> + * val[15:8] is flag of event-channel interrupt: >> + * bit 8: interrupt is edge(1) or level(0) triggered >> + * bit 9: interrupt is active low(1) or high(0) >> + * val[7:0] is PPI number used by event-channel. is a PPI >> + * This is only used by ARM/ARM64. >> + */ [...] > And then I'm now also wondering about the description of bits > 8 and 9 - event channels don't know of edge/level triggering > or high/low polarity, so it looks to me as if the comment is at > least misleading too. bit 8 and bit 9 are related to the PPI configuration. We need to know if the interrupt is level/edge trigger active low/high to configure correctly the interrupt controller. What about renaming "interrupt" to "PPI" to make clear is related to the PPI? Regards,
>>> On 16.03.16 at 16:03, <julien.grall@arm.com> wrote: > On 04/03/2016 10:16, Jan Beulich wrote: >>>>> On 04.03.16 at 07:15, <zhaoshenglong@huawei.com> wrote: > > [...] > >>> --- a/xen/include/public/hvm/params.h >>> +++ b/xen/include/public/hvm/params.h >>> @@ -55,6 +55,16 @@ >>> * if this delivery method is available. >>> */ >>> >>> +#define HVM_PARAM_CALLBACK_TYPE_EVENT 3 >>> +/* >>> + * val[55:16] need to be zero. >>> + * val[15:8] is flag of event-channel interrupt: >>> + * bit 8: interrupt is edge(1) or level(0) triggered >>> + * bit 9: interrupt is active low(1) or high(0) >>> + * val[7:0] is PPI number used by event-channel. > > is a PPI > >>> + * This is only used by ARM/ARM64. >>> + */ > > [...] > >> And then I'm now also wondering about the description of bits >> 8 and 9 - event channels don't know of edge/level triggering >> or high/low polarity, so it looks to me as if the comment is at >> least misleading too. > > bit 8 and bit 9 are related to the PPI configuration. We need to know if > the interrupt is level/edge trigger active low/high to configure > correctly the interrupt controller. > > What about renaming "interrupt" to "PPI" to make clear is related to the > PPI? Maybe. What has confused me more was the "event-channel interrupt" part in the comment. Jan
Hi Konrad, On 04/03/2016 21:19, Konrad Rzeszutek Wilk wrote: > Anyhow what I am wondering if there are some semantincs when it comes to PPI > and it being able to 'mask' an vector until it exits or such? If so > you should document that. I'm not sure to understand what you are asking. Xen takes advantage of the virtual interrupt controller to notify the guest of new pending event channels. The life cycle (mask/eoi) of the interrupt associated to the notification is handled by the interrupt controller. The interrupt can't be re-entrant but we may receive spurious notification. AFAIK, there is no specific semantics. Stefano, can you confirm it? Regards,
On Wed, Mar 16, 2016 at 04:34:19PM +0000, Julien Grall wrote: > Hi Konrad, > > On 04/03/2016 21:19, Konrad Rzeszutek Wilk wrote: > >Anyhow what I am wondering if there are some semantincs when it comes to PPI > >and it being able to 'mask' an vector until it exits or such? If so > >you should document that. > > I'm not sure to understand what you are asking. > > Xen takes advantage of the virtual interrupt controller to notify the guest > of new pending event channels. > > The life cycle (mask/eoi) of the interrupt associated to the notification is > handled by the interrupt controller. The interrupt can't be re-entrant but > we may receive spurious notification. Excellent. That is what I wanted to know. It would be good to make sure that is mentioned in the header file. > > AFAIK, there is no specific semantics. Stefano, can you confirm it? Because on x86 the vector callback bypasses the interrupt controller and injects the vector using the mechanism that is usually used for exceptions (or emulating software interrupts). > > Regards, > > -- > Julien Grall
On Wed, 16 Mar 2016, Konrad Rzeszutek Wilk wrote: > On Wed, Mar 16, 2016 at 04:34:19PM +0000, Julien Grall wrote: > > Hi Konrad, > > > > On 04/03/2016 21:19, Konrad Rzeszutek Wilk wrote: > > >Anyhow what I am wondering if there are some semantincs when it comes to PPI > > >and it being able to 'mask' an vector until it exits or such? If so > > >you should document that. > > > > I'm not sure to understand what you are asking. > > > > Xen takes advantage of the virtual interrupt controller to notify the guest > > of new pending event channels. > > > > The life cycle (mask/eoi) of the interrupt associated to the notification is > > handled by the interrupt controller. The interrupt can't be re-entrant but > > we may receive spurious notification. > > Excellent. That is what I wanted to know. It would be good to make > sure that is mentioned in the header file. > > > > > AFAIK, there is no specific semantics. Stefano, can you confirm it? Yes, that's right. > Because on x86 the vector callback bypasses the interrupt controller and > injects the vector using the mechanism that is usually used for exceptions > (or emulating software interrupts). Right, that's the big difference.
diff --git a/xen/include/public/hvm/params.h b/xen/include/public/hvm/params.h index 73d4718..2ea8d62 100644 --- a/xen/include/public/hvm/params.h +++ b/xen/include/public/hvm/params.h @@ -55,6 +55,16 @@ * if this delivery method is available. */ +#define HVM_PARAM_CALLBACK_TYPE_EVENT 3 +/* + * val[55:16] need to be zero. + * val[15:8] is flag of event-channel interrupt: + * bit 8: interrupt is edge(1) or level(0) triggered + * bit 9: interrupt is active low(1) or high(0) + * val[7:0] is PPI number used by event-channel. + * This is only used by ARM/ARM64. + */ + /* * These are not used by Xen. They are here for convenience of HVM-guest * xenbus implementations.