[PATCH] public/hvm: Export the HVM_PARAM_CALLBACK_VIA ABI in the API

[PATCH] public/hvm: Export the HVM_PARAM_CALLBACK_VIA ABI in the API

[Andrew Cooper]

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Tim Deegan <tim@xen.org>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Stefano Stabellini <stefano.stabellini@citrix.com>
CC: Shannon Zhao <zhaoshenglong@huawei.com>

---
This ABI is utterly mad.

Despite appearing to be wrong, it is compatible with its implementation of

    enum {
        HVMIRQ_callback_none,
        HVMIRQ_callback_gsi,
        HVMIRQ_callback_pci_intx,
        HVMIRQ_callback_vector
    } callback_via_type;

Because of a sneaky:

    via_type = (uint8_t)(via >> 56) + 1;
    if ( ((via_type == HVMIRQ_callback_gsi) && (via == 0)) ||
         (via_type > HVMIRQ_callback_vector) )

I also observe:
 * GSI is actually clipped at 8 bits, rather than the documented 55
 * Attempting to use GSI 0 will actually disable callbacks
 * Xen currently ignores DOMAIN and FUNCTION from PCI_INTX.  As this has been
   broken for a very long time, it probably can't be fixed without breaking
   something else.

but lack sufficient time to fix these issues at the moment.
---
 xen/include/public/hvm/params.h | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/xen/include/public/hvm/params.h b/xen/include/public/hvm/params.h
index 356dfd3..fc4cdca 100644
--- a/xen/include/public/hvm/params.h
+++ b/xen/include/public/hvm/params.h
@@ -29,18 +29,21 @@
  * Parameter space for HVMOP_{set,get}_param.
  */
 
+#define HVM_PARAM_CALLBACK_IRQ 0
+
 /*
  * How should CPU0 event-channel notifications be delivered?
- * val[63:56] == 0: val[55:0] is a delivery GSI (Global System Interrupt).
- * val[63:56] == 1: val[55:0] is a delivery PCI INTx line, as follows:
- *                  Domain = val[47:32], Bus  = val[31:16],
- *                  DevFn  = val[15: 8], IntX = val[ 1: 0]
- * val[63:56] == 2: val[7:0] is a vector number, check for
- *                  XENFEAT_hvm_callback_vector to know if this delivery
- *                  method is available.
+ *
  * If val == 0 then CPU0 event-channel notifications are not delivered.
+ * If val != 0, val[63:56] encodes the type, as follows:
  */
-#define HVM_PARAM_CALLBACK_IRQ 0
+#define HVM_PARAM_CALLBACK_TYPE_GSI      0 /* val[55:0] is a delivery GSI */
+#define HVM_PARAM_CALLBACK_TYPE_PCI_INTX 1 /* val[55:0] is a delivery PCI INTx line:
+                                              Domain = val[47:32], Bus  = val[31:16],
+                                              DevFn  = val[15: 8], IntX = val[ 1: 0] */
+#define HVM_PARAM_CALLBACK_TYPE_VECTOR   2 /* val[7:0] is a vector number, check for
+                                              XENFEAT_hvm_callback_vector to know if
+                                              this delivery method is available. */
 
 /*
  * These are not used by Xen. They are here for convenience of HVM-guest
-- 
2.1.4

Re: [PATCH] public/hvm: Export the HVM_PARAM_CALLBACK_VIA ABI in the API

[Jan Beulich]

>>> On 20.11.15 at 19:20, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/include/public/hvm/params.h
> +++ b/xen/include/public/hvm/params.h
> @@ -29,18 +29,21 @@
>   * Parameter space for HVMOP_{set,get}_param.
>   */
>  
> +#define HVM_PARAM_CALLBACK_IRQ 0
> +
>  /*
>   * How should CPU0 event-channel notifications be delivered?
> - * val[63:56] == 0: val[55:0] is a delivery GSI (Global System Interrupt).
> - * val[63:56] == 1: val[55:0] is a delivery PCI INTx line, as follows:
> - *                  Domain = val[47:32], Bus  = val[31:16],
> - *                  DevFn  = val[15: 8], IntX = val[ 1: 0]
> - * val[63:56] == 2: val[7:0] is a vector number, check for
> - *                  XENFEAT_hvm_callback_vector to know if this delivery
> - *                  method is available.
> + *
>   * If val == 0 then CPU0 event-channel notifications are not delivered.
> + * If val != 0, val[63:56] encodes the type, as follows:
>   */
> -#define HVM_PARAM_CALLBACK_IRQ 0
> +#define HVM_PARAM_CALLBACK_TYPE_GSI      0 /* val[55:0] is a delivery GSI */
> +#define HVM_PARAM_CALLBACK_TYPE_PCI_INTX 1 /* val[55:0] is a delivery PCI INTx line:
> +                                              Domain = val[47:32], Bus  = val[31:16],
> +                                              DevFn  = val[15: 8], IntX = val[ 1: 0] */
> +#define HVM_PARAM_CALLBACK_TYPE_VECTOR   2 /* val[7:0] is a vector number, check for
> +                                              XENFEAT_hvm_callback_vector to know if
> +                                              this delivery method is available. */

Would you not think that while cleaning this up, doing some
re-formatting would also help? I.e. Putting the comments on lines of
their own instead of at the end (even more deeply indented than
they were before)? Also the first two of these last three comments
are missing full stops.

Also considering

 * Attempting to use GSI 0 will actually disable callbacks

in your patch comments, I'd suggest extending the GSI comment to
state that it has to be a non-zero one.

Jan

Re: [PATCH] public/hvm: Export the HVM_PARAM_CALLBACK_VIA ABI in the API

[Paul Durrant]

> -----Original Message-----
> From: xen-devel-bounces@lists.xen.org [mailto:xen-devel-
> bounces@lists.xen.org] On Behalf Of Jan Beulich
> Sent: 24 November 2015 07:33
> To: Andrew Cooper
> Cc: Keir (Xen.org); Ian Campbell; Tim (Xen.org); Ian Jackson; Xen-devel;
> Stefano Stabellini; Shannon Zhao
> Subject: Re: [Xen-devel] [PATCH] public/hvm: Export the
> HVM_PARAM_CALLBACK_VIA ABI in the API
> 
> >>> On 20.11.15 at 19:20, <andrew.cooper3@citrix.com> wrote:
> > --- a/xen/include/public/hvm/params.h
> > +++ b/xen/include/public/hvm/params.h
> > @@ -29,18 +29,21 @@
> >   * Parameter space for HVMOP_{set,get}_param.
> >   */
> >
> > +#define HVM_PARAM_CALLBACK_IRQ 0
> > +
> >  /*
> >   * How should CPU0 event-channel notifications be delivered?
> > - * val[63:56] == 0: val[55:0] is a delivery GSI (Global System Interrupt).
> > - * val[63:56] == 1: val[55:0] is a delivery PCI INTx line, as follows:
> > - *                  Domain = val[47:32], Bus  = val[31:16],
> > - *                  DevFn  = val[15: 8], IntX = val[ 1: 0]
> > - * val[63:56] == 2: val[7:0] is a vector number, check for
> > - *                  XENFEAT_hvm_callback_vector to know if this delivery
> > - *                  method is available.
> > + *
> >   * If val == 0 then CPU0 event-channel notifications are not delivered.
> > + * If val != 0, val[63:56] encodes the type, as follows:
> >   */
> > -#define HVM_PARAM_CALLBACK_IRQ 0
> > +#define HVM_PARAM_CALLBACK_TYPE_GSI      0 /* val[55:0] is a delivery
> GSI */
> > +#define HVM_PARAM_CALLBACK_TYPE_PCI_INTX 1 /* val[55:0] is a
> delivery PCI INTx line:
> > +                                              Domain = val[47:32], Bus  = val[31:16],
> > +                                              DevFn  = val[15: 8], IntX = val[ 1: 0] */
> > +#define HVM_PARAM_CALLBACK_TYPE_VECTOR   2 /* val[7:0] is a vector
> number, check for
> > +                                              XENFEAT_hvm_callback_vector to know if
> > +                                              this delivery method is available. */
> 
> Would you not think that while cleaning this up, doing some
> re-formatting would also help? I.e. Putting the comments on lines of
> their own instead of at the end (even more deeply indented than
> they were before)? Also the first two of these last three comments
> are missing full stops.
> 
> Also considering
> 
>  * Attempting to use GSI 0 will actually disable callbacks
> 
> in your patch comments, I'd suggest extending the GSI comment to
> state that it has to be a non-zero one.
> 

Could we also perhaps deprecate this whole mechanism at this point? I added HVMOP_set_evtchn_upcall_vector quite a while ago now.

  Paul

> Jan
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH] public/hvm: Export the HVM_PARAM_CALLBACK_VIA ABI in the API

[Andrew Cooper]

On 24/11/15 09:56, Paul Durrant wrote:
>> -----Original Message-----
>> From: xen-devel-bounces@lists.xen.org [mailto:xen-devel-
>> bounces@lists.xen.org] On Behalf Of Jan Beulich
>> Sent: 24 November 2015 07:33
>> To: Andrew Cooper
>> Cc: Keir (Xen.org); Ian Campbell; Tim (Xen.org); Ian Jackson; Xen-devel;
>> Stefano Stabellini; Shannon Zhao
>> Subject: Re: [Xen-devel] [PATCH] public/hvm: Export the
>> HVM_PARAM_CALLBACK_VIA ABI in the API
>>
>>>>> On 20.11.15 at 19:20, <andrew.cooper3@citrix.com> wrote:
>>> --- a/xen/include/public/hvm/params.h
>>> +++ b/xen/include/public/hvm/params.h
>>> @@ -29,18 +29,21 @@
>>>   * Parameter space for HVMOP_{set,get}_param.
>>>   */
>>>
>>> +#define HVM_PARAM_CALLBACK_IRQ 0
>>> +
>>>  /*
>>>   * How should CPU0 event-channel notifications be delivered?
>>> - * val[63:56] == 0: val[55:0] is a delivery GSI (Global System Interrupt).
>>> - * val[63:56] == 1: val[55:0] is a delivery PCI INTx line, as follows:
>>> - *                  Domain = val[47:32], Bus  = val[31:16],
>>> - *                  DevFn  = val[15: 8], IntX = val[ 1: 0]
>>> - * val[63:56] == 2: val[7:0] is a vector number, check for
>>> - *                  XENFEAT_hvm_callback_vector to know if this delivery
>>> - *                  method is available.
>>> + *
>>>   * If val == 0 then CPU0 event-channel notifications are not delivered.
>>> + * If val != 0, val[63:56] encodes the type, as follows:
>>>   */
>>> -#define HVM_PARAM_CALLBACK_IRQ 0
>>> +#define HVM_PARAM_CALLBACK_TYPE_GSI      0 /* val[55:0] is a delivery
>> GSI */
>>> +#define HVM_PARAM_CALLBACK_TYPE_PCI_INTX 1 /* val[55:0] is a
>> delivery PCI INTx line:
>>> +                                              Domain = val[47:32], Bus  = val[31:16],
>>> +                                              DevFn  = val[15: 8], IntX = val[ 1: 0] */
>>> +#define HVM_PARAM_CALLBACK_TYPE_VECTOR   2 /* val[7:0] is a vector
>> number, check for
>>> +                                              XENFEAT_hvm_callback_vector to know if
>>> +                                              this delivery method is available. */
>> Would you not think that while cleaning this up, doing some
>> re-formatting would also help? I.e. Putting the comments on lines of
>> their own instead of at the end (even more deeply indented than
>> they were before)? Also the first two of these last three comments
>> are missing full stops.
>>
>> Also considering
>>
>>  * Attempting to use GSI 0 will actually disable callbacks
>>
>> in your patch comments, I'd suggest extending the GSI comment to
>> state that it has to be a non-zero one.
>>
> Could we also perhaps deprecate this whole mechanism at this point? I added HVMOP_set_evtchn_upcall_vector quite a while ago now.

January, so Xen 4.6 is the only release with this hvmop in.

The issue with deprecating HVM_PARAM_CALLBACK_IRQ is that DMLite has no
LAPIC, so can't use HVMOP_set_evtchn_upcall_vector.

Much as this ABI is horrible and nasty (and really should be
deprecated), there are not alternatives which cover the common uses.

~Andrew

Re: [PATCH] public/hvm: Export the HVM_PARAM_CALLBACK_VIA ABI in the API

[George Dunlap]

On Tue, Nov 24, 2015 at 10:28 AM, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
>> Could we also perhaps deprecate this whole mechanism at this point? I added HVMOP_set_evtchn_upcall_vector quite a while ago now.
>
> January, so Xen 4.6 is the only release with this hvmop in.

Come on -- if you're not rebuilding from master every week, you're
basically in the dark ages. ;-)

 -George

Re: [PATCH] public/hvm: Export the HVM_PARAM_CALLBACK_VIA ABI in the API

[Paul Durrant]

> -----Original Message-----
> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: 24 November 2015 10:29
> To: Paul Durrant; Jan Beulich
> Cc: Keir (Xen.org); Ian Campbell; Tim (Xen.org); Ian Jackson; Xen-devel;
> Stefano Stabellini; Shannon Zhao
> Subject: Re: [Xen-devel] [PATCH] public/hvm: Export the
> HVM_PARAM_CALLBACK_VIA ABI in the API
> 
> On 24/11/15 09:56, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: xen-devel-bounces@lists.xen.org [mailto:xen-devel-
> >> bounces@lists.xen.org] On Behalf Of Jan Beulich
> >> Sent: 24 November 2015 07:33
> >> To: Andrew Cooper
> >> Cc: Keir (Xen.org); Ian Campbell; Tim (Xen.org); Ian Jackson; Xen-devel;
> >> Stefano Stabellini; Shannon Zhao
> >> Subject: Re: [Xen-devel] [PATCH] public/hvm: Export the
> >> HVM_PARAM_CALLBACK_VIA ABI in the API
> >>
> >>>>> On 20.11.15 at 19:20, <andrew.cooper3@citrix.com> wrote:
> >>> --- a/xen/include/public/hvm/params.h
> >>> +++ b/xen/include/public/hvm/params.h
> >>> @@ -29,18 +29,21 @@
> >>>   * Parameter space for HVMOP_{set,get}_param.
> >>>   */
> >>>
> >>> +#define HVM_PARAM_CALLBACK_IRQ 0
> >>> +
> >>>  /*
> >>>   * How should CPU0 event-channel notifications be delivered?
> >>> - * val[63:56] == 0: val[55:0] is a delivery GSI (Global System Interrupt).
> >>> - * val[63:56] == 1: val[55:0] is a delivery PCI INTx line, as follows:
> >>> - *                  Domain = val[47:32], Bus  = val[31:16],
> >>> - *                  DevFn  = val[15: 8], IntX = val[ 1: 0]
> >>> - * val[63:56] == 2: val[7:0] is a vector number, check for
> >>> - *                  XENFEAT_hvm_callback_vector to know if this delivery
> >>> - *                  method is available.
> >>> + *
> >>>   * If val == 0 then CPU0 event-channel notifications are not delivered.
> >>> + * If val != 0, val[63:56] encodes the type, as follows:
> >>>   */
> >>> -#define HVM_PARAM_CALLBACK_IRQ 0
> >>> +#define HVM_PARAM_CALLBACK_TYPE_GSI      0 /* val[55:0] is a
> delivery
> >> GSI */
> >>> +#define HVM_PARAM_CALLBACK_TYPE_PCI_INTX 1 /* val[55:0] is a
> >> delivery PCI INTx line:
> >>> +                                              Domain = val[47:32], Bus  = val[31:16],
> >>> +                                              DevFn  = val[15: 8], IntX = val[ 1: 0] */
> >>> +#define HVM_PARAM_CALLBACK_TYPE_VECTOR   2 /* val[7:0] is a
> vector
> >> number, check for
> >>> +                                              XENFEAT_hvm_callback_vector to know if
> >>> +                                              this delivery method is available. */
> >> Would you not think that while cleaning this up, doing some
> >> re-formatting would also help? I.e. Putting the comments on lines of
> >> their own instead of at the end (even more deeply indented than
> >> they were before)? Also the first two of these last three comments
> >> are missing full stops.
> >>
> >> Also considering
> >>
> >>  * Attempting to use GSI 0 will actually disable callbacks
> >>
> >> in your patch comments, I'd suggest extending the GSI comment to
> >> state that it has to be a non-zero one.
> >>
> > Could we also perhaps deprecate this whole mechanism at this point? I
> added HVMOP_set_evtchn_upcall_vector quite a while ago now.
> 
> January, so Xen 4.6 is the only release with this hvmop in.
> 
> The issue with deprecating HVM_PARAM_CALLBACK_IRQ is that DMLite has
> no
> LAPIC, so can't use HVMOP_set_evtchn_upcall_vector.
> 
> Much as this ABI is horrible and nasty (and really should be
> deprecated), there are not alternatives which cover the common uses.
>

Yes, I realise the lack of LAPIC means that the HVMOP can't be used in that case, but I really really think that a new hypercall would be preferable to perpetuating the use of the callback_vector type with this param.

  Paul

 
> ~Andrew

[PATCH] public/hvm: Export the HVM_PARAM_CALLBACK_VIA ABI in the API

[Andrew Cooper]

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Tim Deegan <tim@xen.org>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Stefano Stabellini <stefano.stabellini@citrix.com>
CC: Shannon Zhao <zhaoshenglong@huawei.com>

v2: Reformat the comments, and note that GSI 0 cannot be used.
---
This ABI is utterly mad.

Despite appearing to be wrong, it is compatible with its implementation of

    enum {
        HVMIRQ_callback_none,
        HVMIRQ_callback_gsi,
        HVMIRQ_callback_pci_intx,
        HVMIRQ_callback_vector
    } callback_via_type;

Because of a sneaky:

    via_type = (uint8_t)(via >> 56) + 1;
    if ( ((via_type == HVMIRQ_callback_gsi) && (via == 0)) ||
         (via_type > HVMIRQ_callback_vector) )

I also observe:
 * GSI is actually clipped at 8 bits, rather than the documented 55
 * Xen currently ignores DOMAIN and FUNCTION from PCI_INTX.  As this has been
   broken for a very long time, it probably can't be fixed without breaking
   something else.

but lack sufficient time to fix these issues at the moment.
---
 xen/include/public/hvm/params.h | 29 +++++++++++++++++++++--------
 1 file changed, 21 insertions(+), 8 deletions(-)

diff --git a/xen/include/public/hvm/params.h b/xen/include/public/hvm/params.h
index b437444..81f9451 100644
--- a/xen/include/public/hvm/params.h
+++ b/xen/include/public/hvm/params.h
@@ -29,18 +29,31 @@
  * Parameter space for HVMOP_{set,get}_param.
  */
 
+#define HVM_PARAM_CALLBACK_IRQ 0
 /*
  * How should CPU0 event-channel notifications be delivered?
- * val[63:56] == 0: val[55:0] is a delivery GSI (Global System Interrupt).
- * val[63:56] == 1: val[55:0] is a delivery PCI INTx line, as follows:
- *                  Domain = val[47:32], Bus  = val[31:16],
- *                  DevFn  = val[15: 8], IntX = val[ 1: 0]
- * val[63:56] == 2: val[7:0] is a vector number, check for
- *                  XENFEAT_hvm_callback_vector to know if this delivery
- *                  method is available.
+ *
  * If val == 0 then CPU0 event-channel notifications are not delivered.
+ * If val != 0, val[63:56] encodes the type, as follows:
+ */
+
+#define HVM_PARAM_CALLBACK_TYPE_GSI      0
+/*
+ * val[55:0] is a delivery GSI.  GSI 0 cannot be used, as it aliases val == 0,
+ * and disables all notifications.
+ */
+
+#define HVM_PARAM_CALLBACK_TYPE_PCI_INTX 1
+/*
+ * val[55:0] is a delivery PCI INTx line:
+ * Domain = val[47:32], Bus = val[31:16] DevFn = val[15:8], IntX = val[1:0]
+ */
+
+#define HVM_PARAM_CALLBACK_TYPE_VECTOR   2
+/*
+ * val[7:0] is a vector number.  Check for XENFEAT_hvm_callback_vector to know
+ * if this delivery method is available.
  */
-#define HVM_PARAM_CALLBACK_IRQ 0
 
 /*
  * These are not used by Xen. They are here for convenience of HVM-guest
-- 
2.1.4