| Message ID | 1456778376-2983-1-git-send-email-konrad.wilk@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Feb 29, 2016 at 03:39:36PM -0500, Konrad Rzeszutek Wilk wrote: > Couple of updates: > - Add an macro to make it easier to use the vector callback. > > - Fix the odditity in Xen internal usage of an enum which offset > by one compared to the #defines. Make it the same. > > - This also clears up the printing of the Callback in the > irq_dump() to match up with the #defines. > > Reported-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> CC-ing Stefano. > --- > xen/arch/x86/hvm/irq.c | 2 +- > xen/include/asm-x86/hvm/irq.h | 12 ++++++++---- > xen/include/public/hvm/hvm_op.h | 3 ++- > xen/include/public/hvm/params.h | 15 +++++++++++++++ > 4 files changed, 26 insertions(+), 6 deletions(-) > > diff --git a/xen/arch/x86/hvm/irq.c b/xen/arch/x86/hvm/irq.c > index 5323d7c..0c6ead4 100644 > --- a/xen/arch/x86/hvm/irq.c > +++ b/xen/arch/x86/hvm/irq.c > @@ -325,7 +325,7 @@ void hvm_set_callback_via(struct domain *d, uint64_t via) > unsigned int gsi=0, pdev=0, pintx=0; > uint8_t via_type; > > - via_type = (uint8_t)(via >> 56) + 1; > + via_type = (uint8_t)(via >> HVM_PARAM_CALLBACK_TYPE_SHIFT); > if ( ((via_type == HVMIRQ_callback_gsi) && (via == 0)) || > (via_type > HVMIRQ_callback_vector) ) > via_type = HVMIRQ_callback_none; > diff --git a/xen/include/asm-x86/hvm/irq.h b/xen/include/asm-x86/hvm/irq.h > index 73b8fb0..2a301db 100644 > --- a/xen/include/asm-x86/hvm/irq.h > +++ b/xen/include/asm-x86/hvm/irq.h > @@ -26,6 +26,8 @@ > #include <asm/hvm/vpic.h> > #include <asm/hvm/vioapic.h> > > +#include <public/hvm/params.h> > + > struct hvm_irq { > /* > * Virtual interrupt wires for a single PCI bus. > @@ -50,11 +52,13 @@ struct hvm_irq { > /* Virtual interrupt and via-link for paravirtual platform driver. */ > uint32_t callback_via_asserted; > union { > + /* These MUST match with HVM_PARAM_CALLBACK_IRQ types. */ > enum { > - HVMIRQ_callback_none, > - HVMIRQ_callback_gsi, > - HVMIRQ_callback_pci_intx, > - HVMIRQ_callback_vector > + HVMIRQ_callback_gsi = HVM_PARAM_CALLBACK_TYPE_GSI, > + HVMIRQ_callback_pci_intx = HVM_PARAM_CALLBACK_TYPE_PCI_INTX, > + HVMIRQ_callback_vector = HVM_PARAM_CALLBACK_TYPE_VECTOR, > + /* Will change if we add more types. */ > + HVMIRQ_callback_none = HVM_PARAM_CALLBACK_TYPE_NUM, > } callback_via_type; > }; > union { > diff --git a/xen/include/public/hvm/hvm_op.h b/xen/include/public/hvm/hvm_op.h > index 1606185..5908f39 100644 > --- a/xen/include/public/hvm/hvm_op.h > +++ b/xen/include/public/hvm/hvm_op.h > @@ -386,7 +386,8 @@ DEFINE_XEN_GUEST_HANDLE(xen_hvm_set_ioreq_server_state_t); > * channel upcalls on the specified <vcpu>. If set, > * this vector will be used in preference to the > * domain global callback via (see > - * HVM_PARAM_CALLBACK_IRQ). > + * HVM_PARAM_CALLBACK_IRQ and > + * HVM_PARAM_CALLBACK_VECTOR). > */ > #define HVMOP_set_evtchn_upcall_vector 23 > struct xen_hvm_evtchn_upcall_vector { > diff --git a/xen/include/public/hvm/params.h b/xen/include/public/hvm/params.h > index 73d4718..5c7fbc5 100644 > --- a/xen/include/public/hvm/params.h > +++ b/xen/include/public/hvm/params.h > @@ -56,6 +56,21 @@ > */ > > /* > + * In the future this may change. > + */ > +#define HVM_PARAM_CALLBACK_TYPE_NUM HVM_PARAM_CALLBACK_TYPE_VECTOR + 1 > +/* > + * The val[63:56] convenience shift. > + */ > +#define HVM_PARAM_CALLBACK_TYPE_SHIFT 56 > + > +/* > + * Wrapper around for HVM_PARAM_CALLBACK_TYPE_VECTOR. > + */ > +#define HVM_PARAM_CALLBACK_VECTOR(x) \ > + (((uint64_t)HVM_PARAM_CALLBACK_TYPE_VECTOR) << \ > + HVM_PARAM_CALLBACK_TYPE_SHIFT | (x)) > +/* > * These are not used by Xen. They are here for convenience of HVM-guest > * xenbus implementations. > */ > -- > 2.4.3 >
>>> On 29.02.16 at 21:39, <konrad.wilk@oracle.com> wrote: > @@ -50,11 +52,13 @@ struct hvm_irq { > /* Virtual interrupt and via-link for paravirtual platform driver. */ > uint32_t callback_via_asserted; > union { > + /* These MUST match with HVM_PARAM_CALLBACK_IRQ types. */ > enum { > - HVMIRQ_callback_none, > - HVMIRQ_callback_gsi, > - HVMIRQ_callback_pci_intx, > - HVMIRQ_callback_vector > + HVMIRQ_callback_gsi = HVM_PARAM_CALLBACK_TYPE_GSI, > + HVMIRQ_callback_pci_intx = HVM_PARAM_CALLBACK_TYPE_PCI_INTX, > + HVMIRQ_callback_vector = HVM_PARAM_CALLBACK_TYPE_VECTOR, > + /* Will change if we add more types. */ > + HVMIRQ_callback_none = HVM_PARAM_CALLBACK_TYPE_NUM, > } callback_via_type; > }; I.e. a domain will now start in HVMIRQ_callback_gsi mode (due to HVM_PARAM_CALLBACK_TYPE_GSI = 0) instead of HVMIRQ_callback_none? That can't be right. > --- a/xen/include/public/hvm/params.h > +++ b/xen/include/public/hvm/params.h > @@ -56,6 +56,21 @@ > */ > > /* > + * In the future this may change. > + */ > +#define HVM_PARAM_CALLBACK_TYPE_NUM HVM_PARAM_CALLBACK_TYPE_VECTOR + 1 Missing parentheses. > +/* > + * The val[63:56] convenience shift. > + */ > +#define HVM_PARAM_CALLBACK_TYPE_SHIFT 56 > + > +/* > + * Wrapper around for HVM_PARAM_CALLBACK_TYPE_VECTOR. Looks like either "around" or "for" wants to be dropped. I also think this would better live right next to that constant's definition. Also note that all comments you add are single line ones, and hence don't conform to our coding style (yes, there are other [bad] examples of such in this file). Jan
On Tue, Mar 01, 2016 at 03:25:55AM -0700, Jan Beulich wrote: > >>> On 29.02.16 at 21:39, <konrad.wilk@oracle.com> wrote: > > @@ -50,11 +52,13 @@ struct hvm_irq { > > /* Virtual interrupt and via-link for paravirtual platform driver. */ > > uint32_t callback_via_asserted; > > union { > > + /* These MUST match with HVM_PARAM_CALLBACK_IRQ types. */ > > enum { > > - HVMIRQ_callback_none, > > - HVMIRQ_callback_gsi, > > - HVMIRQ_callback_pci_intx, > > - HVMIRQ_callback_vector > > + HVMIRQ_callback_gsi = HVM_PARAM_CALLBACK_TYPE_GSI, > > + HVMIRQ_callback_pci_intx = HVM_PARAM_CALLBACK_TYPE_PCI_INTX, > > + HVMIRQ_callback_vector = HVM_PARAM_CALLBACK_TYPE_VECTOR, > > + /* Will change if we add more types. */ > > + HVMIRQ_callback_none = HVM_PARAM_CALLBACK_TYPE_NUM, > > } callback_via_type; > > }; > > I.e. a domain will now start in HVMIRQ_callback_gsi mode (due to > HVM_PARAM_CALLBACK_TYPE_GSI = 0) instead of > HVMIRQ_callback_none? That can't be right. Argh. I was looking for all the cases for callback_via_type being set but of course missed the most obvious one! > > > --- a/xen/include/public/hvm/params.h > > +++ b/xen/include/public/hvm/params.h > > @@ -56,6 +56,21 @@ > > */ > > > > /* > > + * In the future this may change. > > + */ > > +#define HVM_PARAM_CALLBACK_TYPE_NUM HVM_PARAM_CALLBACK_TYPE_VECTOR + 1 > > Missing parentheses. > > > +/* > > + * The val[63:56] convenience shift. > > + */ > > +#define HVM_PARAM_CALLBACK_TYPE_SHIFT 56 > > + > > +/* > > + * Wrapper around for HVM_PARAM_CALLBACK_TYPE_VECTOR. > > Looks like either "around" or "for" wants to be dropped. I also > think this would better live right next to that constant's definition. OK. > > Also note that all comments you add are single line ones, and > hence don't conform to our coding style (yes, there are other > [bad] examples of such in this file). /me nods. Will make it conform to the single line one type comment.
diff --git a/xen/arch/x86/hvm/irq.c b/xen/arch/x86/hvm/irq.c index 5323d7c..0c6ead4 100644 --- a/xen/arch/x86/hvm/irq.c +++ b/xen/arch/x86/hvm/irq.c @@ -325,7 +325,7 @@ void hvm_set_callback_via(struct domain *d, uint64_t via) unsigned int gsi=0, pdev=0, pintx=0; uint8_t via_type; - via_type = (uint8_t)(via >> 56) + 1; + via_type = (uint8_t)(via >> HVM_PARAM_CALLBACK_TYPE_SHIFT); if ( ((via_type == HVMIRQ_callback_gsi) && (via == 0)) || (via_type > HVMIRQ_callback_vector) ) via_type = HVMIRQ_callback_none; diff --git a/xen/include/asm-x86/hvm/irq.h b/xen/include/asm-x86/hvm/irq.h index 73b8fb0..2a301db 100644 --- a/xen/include/asm-x86/hvm/irq.h +++ b/xen/include/asm-x86/hvm/irq.h @@ -26,6 +26,8 @@ #include <asm/hvm/vpic.h> #include <asm/hvm/vioapic.h> +#include <public/hvm/params.h> + struct hvm_irq { /* * Virtual interrupt wires for a single PCI bus. @@ -50,11 +52,13 @@ struct hvm_irq { /* Virtual interrupt and via-link for paravirtual platform driver. */ uint32_t callback_via_asserted; union { + /* These MUST match with HVM_PARAM_CALLBACK_IRQ types. */ enum { - HVMIRQ_callback_none, - HVMIRQ_callback_gsi, - HVMIRQ_callback_pci_intx, - HVMIRQ_callback_vector + HVMIRQ_callback_gsi = HVM_PARAM_CALLBACK_TYPE_GSI, + HVMIRQ_callback_pci_intx = HVM_PARAM_CALLBACK_TYPE_PCI_INTX, + HVMIRQ_callback_vector = HVM_PARAM_CALLBACK_TYPE_VECTOR, + /* Will change if we add more types. */ + HVMIRQ_callback_none = HVM_PARAM_CALLBACK_TYPE_NUM, } callback_via_type; }; union { diff --git a/xen/include/public/hvm/hvm_op.h b/xen/include/public/hvm/hvm_op.h index 1606185..5908f39 100644 --- a/xen/include/public/hvm/hvm_op.h +++ b/xen/include/public/hvm/hvm_op.h @@ -386,7 +386,8 @@ DEFINE_XEN_GUEST_HANDLE(xen_hvm_set_ioreq_server_state_t); * channel upcalls on the specified <vcpu>. If set, * this vector will be used in preference to the * domain global callback via (see - * HVM_PARAM_CALLBACK_IRQ). + * HVM_PARAM_CALLBACK_IRQ and + * HVM_PARAM_CALLBACK_VECTOR). */ #define HVMOP_set_evtchn_upcall_vector 23 struct xen_hvm_evtchn_upcall_vector { diff --git a/xen/include/public/hvm/params.h b/xen/include/public/hvm/params.h index 73d4718..5c7fbc5 100644 --- a/xen/include/public/hvm/params.h +++ b/xen/include/public/hvm/params.h @@ -56,6 +56,21 @@ */ /* + * In the future this may change. + */ +#define HVM_PARAM_CALLBACK_TYPE_NUM HVM_PARAM_CALLBACK_TYPE_VECTOR + 1 +/* + * The val[63:56] convenience shift. + */ +#define HVM_PARAM_CALLBACK_TYPE_SHIFT 56 + +/* + * Wrapper around for HVM_PARAM_CALLBACK_TYPE_VECTOR. + */ +#define HVM_PARAM_CALLBACK_VECTOR(x) \ + (((uint64_t)HVM_PARAM_CALLBACK_TYPE_VECTOR) << \ + HVM_PARAM_CALLBACK_TYPE_SHIFT | (x)) +/* * These are not used by Xen. They are here for convenience of HVM-guest * xenbus implementations. */
Couple of updates: - Add an macro to make it easier to use the vector callback. - Fix the odditity in Xen internal usage of an enum which offset by one compared to the #defines. Make it the same. - This also clears up the printing of the Callback in the irq_dump() to match up with the #defines. Reported-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> --- xen/arch/x86/hvm/irq.c | 2 +- xen/include/asm-x86/hvm/irq.h | 12 ++++++++---- xen/include/public/hvm/hvm_op.h | 3 ++- xen/include/public/hvm/params.h | 15 +++++++++++++++ 4 files changed, 26 insertions(+), 6 deletions(-)