[Xen-devel,02/11] xen/arm: vpl011: Add new hvm params in Xen for ring buffer/event setup

Message ID 1487676368-22356-3-git-send-email-bhupinder.thakur@linaro.org
State New
Headers show
Series
  • pl011 emulation support in Xen
Related show

Commit Message

Bhupinder Thakur Feb. 21, 2017, 11:25 a.m.
Three new HVM param handlers added for:
    - allocating a new VIRQ and return to the toolstack
    - allocating a new event channel for sending/receiving events from Xen and return
      to the toolstack
    - mapping the PFN allocted by the toolstack to be used as IN/OUT ring buffers

Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
---
 xen/arch/arm/hvm.c              | 39 +++++++++++++++++++++++++++++++++++++++
 xen/include/public/hvm/params.h | 10 +++++++++-
 2 files changed, 48 insertions(+), 1 deletion(-)

Comments

Konrad Rzeszutek Wilk March 3, 2017, 8:02 p.m. | #1
On Tue, Feb 21, 2017 at 04:55:59PM +0530, Bhupinder Thakur wrote:
> Three new HVM param handlers added for:
>     - allocating a new VIRQ and return to the toolstack
>     - allocating a new event channel for sending/receiving events from Xen and return
>       to the toolstack
>     - mapping the PFN allocted by the toolstack to be used as IN/OUT ring buffers
> 
> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
> ---
>  xen/arch/arm/hvm.c              | 39 +++++++++++++++++++++++++++++++++++++++
>  xen/include/public/hvm/params.h | 10 +++++++++-
>  2 files changed, 48 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c
> index d999bde..f3b9eb1 100644
> --- a/xen/arch/arm/hvm.c
> +++ b/xen/arch/arm/hvm.c
> @@ -23,6 +23,8 @@
>  #include <xen/guest_access.h>
>  #include <xen/sched.h>
>  #include <xen/monitor.h>
> +#include <xen/event.h>
> +#include <xen/vmap.h>
>  
>  #include <xsm/xsm.h>
>  
> @@ -31,6 +33,7 @@
>  #include <public/hvm/hvm_op.h>
>  
>  #include <asm/hypercall.h>
> +#include "vpl011.h"
>  
>  long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
>  {
> @@ -61,9 +64,45 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
>          if ( op == HVMOP_set_param )
>          {
>              d->arch.hvm_domain.params[a.index] = a.value;
> +
> +#ifdef CONFIG_VPL011_CONSOLE
> +            /*
> +             * if it is a vpl011 console pfn then map it to its

s/if/If/
> +             * own address space

And you can also add an . here.

> +             */
> +            if ( a.index == HVM_PARAM_VPL011_CONSOLE_PFN )
> +            {
> +                vpl011_map_guest_page(d);
> +            }
> +#else
> +            /* 
> +             * if VPL011 is not compiled in then disallow setting of any 
> +             * related HVM params
> +             */
> +            if ( a.index == HVM_PARAM_VPL011_CONSOLE_PFN ||
> +                 a.index == HVM_PARAM_VPL011_CONSOLE_EVTCHN ||
> +                 a.index == HVM_PARAM_VPL011_VIRQ )
> +            {
> +                rc = -1;
> +                goto param_fail;
> +            }
> +#endif
>          }
>          else
>          {
> +#ifndef CONFIG_VPL011_CONSOLE
> +            /* 
> +             * if VPL011 is not compiled in then disallow setting of any 
> +             * related HVM params
> +             */
> +            if ( a.index == HVM_PARAM_VPL011_CONSOLE_PFN ||
> +                 a.index == HVM_PARAM_VPL011_CONSOLE_EVTCHN ||
> +                 a.index == HVM_PARAM_VPL011_VIRQ )
> +            {
> +                rc = -1;
> +                goto param_fail;

This and the above look like it could be nicely folded in a function?

Say:

static bool vpl011_built(unsigned int idx)
{
#ifdef CONFIG_VPL011_CONSOLE
	if ( idx == ..
		return true;
#else
	return false;
}

And here you can just do:
	if (vpl011_built())
	{
		rc = -1;
	.. and so on.
		
> +            }
> +#endif
>              a.value = d->arch.hvm_domain.params[a.index];
>              rc = copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
>          }
> diff --git a/xen/include/public/hvm/params.h b/xen/include/public/hvm/params.h
> index 3f54a49..13bf719 100644
> --- a/xen/include/public/hvm/params.h
> +++ b/xen/include/public/hvm/params.h
> @@ -203,10 +203,17 @@
>   */
>  #define HVM_PARAM_ACPI_IOPORTS_LOCATION 19
>  
> -/* Deprecated */
> +#if defined(__arm__) || defined(__aarch64__)
> +#define HVM_PARAM_VPL011_CONSOLE_PFN    20
> +#define HVM_PARAM_VPL011_CONSOLE_EVTCHN 21
> +#define HVM_PARAM_VPL011_VIRQ           22
> +#else
>  #define HVM_PARAM_MEMORY_EVENT_CR0          20
>  #define HVM_PARAM_MEMORY_EVENT_CR3          21
>  #define HVM_PARAM_MEMORY_EVENT_CR4          22
> +#endif
> +
> +/* Deprecated */
>  #define HVM_PARAM_MEMORY_EVENT_INT3         23
>  #define HVM_PARAM_MEMORY_EVENT_SINGLE_STEP  25
>  #define HVM_PARAM_MEMORY_EVENT_MSR          30
> @@ -253,6 +260,7 @@
>   */
>  #define HVM_PARAM_X87_FIP_WIDTH 36
>  
> +


???
>  #define HVM_NR_PARAMS 37
>  
>  #endif /* __XEN_PUBLIC_HVM_PARAMS_H__ */
> -- 
> 2.7.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel
Julien Grall March 5, 2017, 12:35 p.m. | #2
(CC "The REST" maintainers)

Hi Bhupinder,

On 02/21/2017 11:25 AM, Bhupinder Thakur wrote:
> Three new HVM param handlers added for:
>     - allocating a new VIRQ and return to the toolstack

This is not necessary. We could hardcode it.

>     - allocating a new event channel for sending/receiving events from Xen and return
>       to the toolstack
>     - mapping the PFN allocted by the toolstack to be used as IN/OUT ring buffers

s/allocted/allocated/

>
> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
> ---
>  xen/arch/arm/hvm.c              | 39 +++++++++++++++++++++++++++++++++++++++
>  xen/include/public/hvm/params.h | 10 +++++++++-
>  2 files changed, 48 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c
> index d999bde..f3b9eb1 100644
> --- a/xen/arch/arm/hvm.c
> +++ b/xen/arch/arm/hvm.c
> @@ -23,6 +23,8 @@
>  #include <xen/guest_access.h>
>  #include <xen/sched.h>
>  #include <xen/monitor.h>
> +#include <xen/event.h>
> +#include <xen/vmap.h>
>
>  #include <xsm/xsm.h>
>
> @@ -31,6 +33,7 @@
>  #include <public/hvm/hvm_op.h>
>
>  #include <asm/hypercall.h>
> +#include "vpl011.h"
>
>  long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
>  {
> @@ -61,9 +64,45 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
>          if ( op == HVMOP_set_param )
>          {
>              d->arch.hvm_domain.params[a.index] = a.value;
> +
> +#ifdef CONFIG_VPL011_CONSOLE
> +            /*
> +             * if it is a vpl011 console pfn then map it to its
> +             * own address space
> +             */
> +            if ( a.index == HVM_PARAM_VPL011_CONSOLE_PFN )
> +            {
> +                vpl011_map_guest_page(d);

vpl011_map_guest_page(...) is will return an error if it fails to map 
the page. Shouldn't you propagate the value.

Also, this code does not prevent the new HVM param to be set twice or 
even set by the guest. We probably want to introduce an helper to check 
if the domain is allowed to set it. See hvm_allow_set_param on x86.

> +            }
> +#else
> +            /*
> +             * if VPL011 is not compiled in then disallow setting of any
> +             * related HVM params
> +             */

We really don't care if the toolstack set unused HVM param as they 
should not be used.

> +            if ( a.index == HVM_PARAM_VPL011_CONSOLE_PFN ||
> +                 a.index == HVM_PARAM_VPL011_CONSOLE_EVTCHN ||
> +                 a.index == HVM_PARAM_VPL011_VIRQ )
> +            {
> +                rc = -1;
> +                goto param_fail;
> +            }
> +#endif
>          }
>          else
>          {
> +#ifndef CONFIG_VPL011_CONSOLE
> +            /*
> +             * if VPL011 is not compiled in then disallow setting of any
> +             * related HVM params
> +             */
> +            if ( a.index == HVM_PARAM_VPL011_CONSOLE_PFN ||
> +                 a.index == HVM_PARAM_VPL011_CONSOLE_EVTCHN ||
> +                 a.index == HVM_PARAM_VPL011_VIRQ )
> +            {
> +                rc = -1;
> +                goto param_fail;
> +            }
> +#endif

Ditto.

>              a.value = d->arch.hvm_domain.params[a.index];

We probably don't want the guest to be able to read the console pfn 
page. See hvm_allow_get_param.

>              rc = copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
>          }
> diff --git a/xen/include/public/hvm/params.h b/xen/include/public/hvm/params.h
> index 3f54a49..13bf719 100644
> --- a/xen/include/public/hvm/params.h
> +++ b/xen/include/public/hvm/params.h
> @@ -203,10 +203,17 @@
>   */
>  #define HVM_PARAM_ACPI_IOPORTS_LOCATION 19
>
> -/* Deprecated */
> +#if defined(__arm__) || defined(__aarch64__)
> +#define HVM_PARAM_VPL011_CONSOLE_PFN    20
> +#define HVM_PARAM_VPL011_CONSOLE_EVTCHN 21
> +#define HVM_PARAM_VPL011_VIRQ           22
> +#else
>  #define HVM_PARAM_MEMORY_EVENT_CR0          20
>  #define HVM_PARAM_MEMORY_EVENT_CR3          21
>  #define HVM_PARAM_MEMORY_EVENT_CR4          22

Those parameters are still deprecated but you drop the comment stating that.

> +#endif
> +

Those params are x86 specific so should have never been set on ARM. But 
I am not sure if it is fine to re-purpose deprecated number.

I have CCed "The REST" maintainers to have their input here.

> +/* Deprecated */
>  #define HVM_PARAM_MEMORY_EVENT_INT3         23
>  #define HVM_PARAM_MEMORY_EVENT_SINGLE_STEP  25
>  #define HVM_PARAM_MEMORY_EVENT_MSR          30
> @@ -253,6 +260,7 @@
>   */
>  #define HVM_PARAM_X87_FIP_WIDTH 36
>
> +

Spurious change.

>  #define HVM_NR_PARAMS 37
>
>  #endif /* __XEN_PUBLIC_HVM_PARAMS_H__ */
>

Cheers,
Jan Beulich March 6, 2017, 8:06 a.m. | #3
>>> On 05.03.17 at 13:35, <julien.grall@arm.com> wrote:
> On 02/21/2017 11:25 AM, Bhupinder Thakur wrote:
>> --- a/xen/include/public/hvm/params.h
>> +++ b/xen/include/public/hvm/params.h
>> @@ -203,10 +203,17 @@
>>   */
>>  #define HVM_PARAM_ACPI_IOPORTS_LOCATION 19
>>
>> -/* Deprecated */
>> +#if defined(__arm__) || defined(__aarch64__)
>> +#define HVM_PARAM_VPL011_CONSOLE_PFN    20
>> +#define HVM_PARAM_VPL011_CONSOLE_EVTCHN 21
>> +#define HVM_PARAM_VPL011_VIRQ           22
>> +#else
>>  #define HVM_PARAM_MEMORY_EVENT_CR0          20
>>  #define HVM_PARAM_MEMORY_EVENT_CR3          21
>>  #define HVM_PARAM_MEMORY_EVENT_CR4          22
> 
> Those parameters are still deprecated but you drop the comment stating that.
> 
>> +#endif
>> +
> 
> Those params are x86 specific so should have never been set on ARM. But 
> I am not sure if it is fine to re-purpose deprecated number.
> 
> I have CCed "The REST" maintainers to have their input here.

I think re-purposing something that was never (meant to be) used is
fine in a case like this. However, the question is moot with your
suggestion to not use params here in the first place.

Jan
Julien Grall March 6, 2017, 11:42 a.m. | #4
Hi Jan,

On 06/03/17 08:06, Jan Beulich wrote:
>>>> On 05.03.17 at 13:35, <julien.grall@arm.com> wrote:
>> On 02/21/2017 11:25 AM, Bhupinder Thakur wrote:
>>> --- a/xen/include/public/hvm/params.h
>>> +++ b/xen/include/public/hvm/params.h
>>> @@ -203,10 +203,17 @@
>>>   */
>>>  #define HVM_PARAM_ACPI_IOPORTS_LOCATION 19
>>>
>>> -/* Deprecated */
>>> +#if defined(__arm__) || defined(__aarch64__)
>>> +#define HVM_PARAM_VPL011_CONSOLE_PFN    20
>>> +#define HVM_PARAM_VPL011_CONSOLE_EVTCHN 21
>>> +#define HVM_PARAM_VPL011_VIRQ           22
>>> +#else
>>>  #define HVM_PARAM_MEMORY_EVENT_CR0          20
>>>  #define HVM_PARAM_MEMORY_EVENT_CR3          21
>>>  #define HVM_PARAM_MEMORY_EVENT_CR4          22
>>
>> Those parameters are still deprecated but you drop the comment stating that.
>>
>>> +#endif
>>> +
>>
>> Those params are x86 specific so should have never been set on ARM. But
>> I am not sure if it is fine to re-purpose deprecated number.
>>
>> I have CCed "The REST" maintainers to have their input here.
>
> I think re-purposing something that was never (meant to be) used is
> fine in a case like this. However, the question is moot with your
> suggestion to not use params here in the first place.

I suggested to drop HVM_PARAM_VPL011_VIRQ because we can hardcode the 
guest interrupt number for the UART as we already do for the MMIO 
region. The 2 other HVM_PARAM looks sensible to me.

I thought a bit more about those params. I think the name should be 
generic and not tie to pl011 because we may want to emulate different 
UART for the guest in the future.

Also, by re-using deprecated encoding it means that it will not be 
possible to use those parameters on x86 if you ever decide to emulate 
UART in Xen. I am not sure whether if you are happy with that?

Cheers,
Jan Beulich March 6, 2017, 12:41 p.m. | #5
>>> On 06.03.17 at 12:42, <julien.grall@arm.com> wrote:
> I thought a bit more about those params. I think the name should be 
> generic and not tie to pl011 because we may want to emulate different 
> UART for the guest in the future.

That's reasonable, but I continue to have reservations against the
underlying approach here, namely ...

> Also, by re-using deprecated encoding it means that it will not be 
> possible to use those parameters on x86 if you ever decide to emulate 
> UART in Xen. I am not sure whether if you are happy with that?

... with this in mind: If we wanted to do this for HVM guests, we'd
indeed want the parameters. If we wanted this for PV guests, the
model wouldn't fit at all.

Plus what I'm not understanding (perhaps because most of the
discussion around this seemed to be ARM-specific, and hence I've
skipped reading through it) is - why is this UART emulation needed
in the first place? We've never had a need for such on x86, afaia.

Furthermore - how does this scale? I.e. what if other devices pop
up wanting to be emulated? Or multiple UARTs?

Jan
Julien Grall March 6, 2017, 1:21 p.m. | #6
Hi Jan,

On 06/03/17 12:41, Jan Beulich wrote:
>>>> On 06.03.17 at 12:42, <julien.grall@arm.com> wrote:
>> I thought a bit more about those params. I think the name should be
>> generic and not tie to pl011 because we may want to emulate different
>> UART for the guest in the future.
>
> That's reasonable, but I continue to have reservations against the
> underlying approach here, namely ...
>
>> Also, by re-using deprecated encoding it means that it will not be
>> possible to use those parameters on x86 if you ever decide to emulate
>> UART in Xen. I am not sure whether if you are happy with that?
>
> ... with this in mind: If we wanted to do this for HVM guests, we'd
> indeed want the parameters. If we wanted this for PV guests, the
> model wouldn't fit at all.
>
> Plus what I'm not understanding (perhaps because most of the
> discussion around this seemed to be ARM-specific, and hence I've
> skipped reading through it) is - why is this UART emulation needed
> in the first place? We've never had a need for such on x86, afaia.

Linaro has published a set of guidelines to guarantee a same guest image 
can run on multiple hypervisors (see [1]). This specification mandates 
the presence of a SBSA-compliant UART.

I just realized the cover letter does not explain why we need to emulate 
a PL011 on ARM. Bhupinder can you detail it on the next version?

>
> Furthermore - how does this scale? I.e. what if other devices pop
> up wanting to be emulated? Or multiple UARTs?

I think you can appreciate that using QEMU for emulating an UART is 
quite overkill.

Also, we are expecting more people to look at providing mediation (such 
as Firmware access) within Xen as they are concerned about performance 
and certification.

An idea to ensure the integrity of Xen would be to run this code at a 
lower level.

Cheers,

[1] 
https://www.linaro.org/app/resources/WhitePaper/VMSystemSpecificationForARM-v2.0.pdf
Jan Beulich March 6, 2017, 1:48 p.m. | #7
>>> On 06.03.17 at 14:21, <julien.grall@arm.com> wrote:
> On 06/03/17 12:41, Jan Beulich wrote:
>> Furthermore - how does this scale? I.e. what if other devices pop
>> up wanting to be emulated? Or multiple UARTs?
> 
> I think you can appreciate that using QEMU for emulating an UART is 
> quite overkill.

Sure, but this doesn't answer my questions.

Jan
George Dunlap March 6, 2017, 2:48 p.m. | #8
On 06/03/17 13:21, Julien Grall wrote:
> Hi Jan,
> 
> On 06/03/17 12:41, Jan Beulich wrote:
>>>>> On 06.03.17 at 12:42, <julien.grall@arm.com> wrote:
>>> I thought a bit more about those params. I think the name should be
>>> generic and not tie to pl011 because we may want to emulate different
>>> UART for the guest in the future.
>>
>> That's reasonable, but I continue to have reservations against the
>> underlying approach here, namely ...
>>
>>> Also, by re-using deprecated encoding it means that it will not be
>>> possible to use those parameters on x86 if you ever decide to emulate
>>> UART in Xen. I am not sure whether if you are happy with that?
>>
>> ... with this in mind: If we wanted to do this for HVM guests, we'd
>> indeed want the parameters. If we wanted this for PV guests, the
>> model wouldn't fit at all.
>>
>> Plus what I'm not understanding (perhaps because most of the
>> discussion around this seemed to be ARM-specific, and hence I've
>> skipped reading through it) is - why is this UART emulation needed
>> in the first place? We've never had a need for such on x86, afaia.
> 
> Linaro has published a set of guidelines to guarantee a same guest image
> can run on multiple hypervisors (see [1]). This specification mandates
> the presence of a SBSA-compliant UART.
> 
> I just realized the cover letter does not explain why we need to emulate
> a PL011 on ARM. Bhupinder can you detail it on the next version?

Right, but in order to evaluate your question about whether we're
"happy" with not being able to use these parameters if we ever decide to
emulate UART on x86, we need to know a reason that one might decide to
add a UART *on x86*, not on ARM.

I mean, I don't think we're running out of bits, so I don't think it's a
problem allocating new HVM_PARAM numbers so that we can re-use them if
we ever decide to implement UARTs on x86.  On the other hand, given that
nobody has ever suggested doing so or knows why it might be useful,
maybe it makes more sense to just re-use some x86 params and allocate
extra numbers if / when we decide we want to implement UART on x86.

 -George
Julien Grall March 8, 2017, 1:52 p.m. | #9
Hi George,

On 06/03/17 14:48, George Dunlap wrote:
> On 06/03/17 13:21, Julien Grall wrote:
>> Hi Jan,
>>
>> On 06/03/17 12:41, Jan Beulich wrote:
>>>>>> On 06.03.17 at 12:42, <julien.grall@arm.com> wrote:
>>>> I thought a bit more about those params. I think the name should be
>>>> generic and not tie to pl011 because we may want to emulate different
>>>> UART for the guest in the future.
>>>
>>> That's reasonable, but I continue to have reservations against the
>>> underlying approach here, namely ...
>>>
>>>> Also, by re-using deprecated encoding it means that it will not be
>>>> possible to use those parameters on x86 if you ever decide to emulate
>>>> UART in Xen. I am not sure whether if you are happy with that?
>>>
>>> ... with this in mind: If we wanted to do this for HVM guests, we'd
>>> indeed want the parameters. If we wanted this for PV guests, the
>>> model wouldn't fit at all.
>>>
>>> Plus what I'm not understanding (perhaps because most of the
>>> discussion around this seemed to be ARM-specific, and hence I've
>>> skipped reading through it) is - why is this UART emulation needed
>>> in the first place? We've never had a need for such on x86, afaia.
>>
>> Linaro has published a set of guidelines to guarantee a same guest image
>> can run on multiple hypervisors (see [1]). This specification mandates
>> the presence of a SBSA-compliant UART.
>>
>> I just realized the cover letter does not explain why we need to emulate
>> a PL011 on ARM. Bhupinder can you detail it on the next version?
>
> Right, but in order to evaluate your question about whether we're
> "happy" with not being able to use these parameters if we ever decide to
> emulate UART on x86, we need to know a reason that one might decide to
> add a UART *on x86*, not on ARM.
>
> I mean, I don't think we're running out of bits, so I don't think it's a
> problem allocating new HVM_PARAM numbers so that we can re-use them if
> we ever decide to implement UARTs on x86.  On the other hand, given that
> nobody has ever suggested doing so or knows why it might be useful,
> maybe it makes more sense to just re-use some x86 params and allocate
> extra numbers if / when we decide we want to implement UART on x86.

I agree that the reason I gave is very ARM focused. I don't know what is 
the future on Xen for x86, but on ARM we have another potential use case 
for the UART emulation which I think could also apply for x86.

On ARM, the guest is booting through the same path as on native. All the 
devices are discovered through the firmware tables. So it would be 
possible to run a guest OS without any knowledge of Xen if you assign 
all the necessary physical devices (e.g disk, network, serial...). Often 
you want to log the console of all your guests and keep them safely in 
the controller domain. This is where an emulated UART can be useful, you 
don't need to add Xen drivers in the guest and still can use the guest 
seamlessly via xl.

Cheers,
Julien Grall March 8, 2017, 2:45 p.m. | #10
Hi Jan,

On 06/03/17 13:48, Jan Beulich wrote:
>>>> On 06.03.17 at 14:21, <julien.grall@arm.com> wrote:
>> On 06/03/17 12:41, Jan Beulich wrote:
>>> Furthermore - how does this scale? I.e. what if other devices pop
>>> up wanting to be emulated? Or multiple UARTs?
>>
>> I think you can appreciate that using QEMU for emulating an UART is
>> quite overkill.
>
> Sure, but this doesn't answer my questions.

Devices we are planning to emulate are in order to be compliant with the 
SBSA. Looking at the spec, it requires:
	* SBSA UART
	* RTC: It is expected to drive through EFI API but we may want to also 
support in non-UEFI case.
	* Persistent environment storage: This is only required for the UEFI.

Another device we are expecting to emulate in Xen is the PCI root 
complex in order to avoid as much as PV drivers for the guest.

So I think we would need to emulate 3 devices: SBSA UART, RTC and root 
complex.

I cannot predict what will happen in the future, but I would expect the 
number of devices we require to emulate very limited.

Regarding the multiple UARTs, you have a point here. The code in itself 
could handle any number of UARTs easily, but we thought that guest will 
usually use only one console.

I was looking at the backend code and see it is using DOMCTL command. I 
guess it is considered that the console backend will be tied to a 
specific Xen version. Am I correct?

so maybe we can introduce new domctl command for handling vUART. This 
would avoid us to commit on an ABI and allow us to extend it if 
necessary in the future to support multiple UARTs.

Any opinions?

Cheers,
Jan Beulich March 8, 2017, 3:21 p.m. | #11
>>> On 08.03.17 at 15:45, <julien.grall@arm.com> wrote:
> I was looking at the backend code and see it is using DOMCTL command. I 
> guess it is considered that the console backend will be tied to a 
> specific Xen version. Am I correct?

I don't think I'm qualified to talk about the console backend
implementation (and possible quirks it has). Generally I'd expect
backends not to use domctl-s, as that would tie them to the tool
stack domain.

> so maybe we can introduce new domctl command for handling vUART. This 
> would avoid us to commit on an ABI and allow us to extend it if 
> necessary in the future to support multiple UARTs.

Well, without having the context of who it would be to use such a
domctl (and for what purpose) I don#t think I can comment here.

Jan
Stefano Stabellini March 8, 2017, 6:22 p.m. | #12
On Wed, 8 Mar 2017, Jan Beulich wrote:
> >>> On 08.03.17 at 15:45, <julien.grall@arm.com> wrote:
> > I was looking at the backend code and see it is using DOMCTL command. I 
> > guess it is considered that the console backend will be tied to a 
> > specific Xen version. Am I correct?
> 
> I don't think I'm qualified to talk about the console backend
> implementation (and possible quirks it has). Generally I'd expect
> backends not to use domctl-s, as that would tie them to the tool
> stack domain.
> 
> > so maybe we can introduce new domctl command for handling vUART. This 
> > would avoid us to commit on an ABI and allow us to extend it if 
> > necessary in the future to support multiple UARTs.
> 
> Well, without having the context of who it would be to use such a
> domctl (and for what purpose) I don#t think I can comment here.

I guess the assumption was that xenconsoled was part of the Xen tools.
Indeed, it is part of the tools and is installed as such.

I don't have an opinion on this. If introducing a new DOMCTL makes the
code nicer in xen and xenconsoled, taking away some edges, like the
changes to evtchn_send, then we should probably just do it.
Bhupinder Thakur March 24, 2017, 6:58 a.m. | #13
Hi Konrad,

On 4 March 2017 at 01:32, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> On Tue, Feb 21, 2017 at 04:55:59PM +0530, Bhupinder Thakur wrote:
>> Three new HVM param handlers added for:
>>     - allocating a new VIRQ and return to the toolstack
>>     - allocating a new event channel for sending/receiving events from Xen and return
>>       to the toolstack
>>     - mapping the PFN allocted by the toolstack to be used as IN/OUT ring buffers
>>
>> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
>> ---
>>  xen/arch/arm/hvm.c              | 39 +++++++++++++++++++++++++++++++++++++++
>>  xen/include/public/hvm/params.h | 10 +++++++++-
>>  2 files changed, 48 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c
>> index d999bde..f3b9eb1 100644
>> --- a/xen/arch/arm/hvm.c
>> +++ b/xen/arch/arm/hvm.c
>> @@ -23,6 +23,8 @@
>>  #include <xen/guest_access.h>
>>  #include <xen/sched.h>
>>  #include <xen/monitor.h>
>> +#include <xen/event.h>
>> +#include <xen/vmap.h>
>>
>>  #include <xsm/xsm.h>
>>
>> @@ -31,6 +33,7 @@
>>  #include <public/hvm/hvm_op.h>
>>
>>  #include <asm/hypercall.h>
>> +#include "vpl011.h"
>>
>>  long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
>>  {
>> @@ -61,9 +64,45 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
>>          if ( op == HVMOP_set_param )
>>          {
>>              d->arch.hvm_domain.params[a.index] = a.value;
>> +
>> +#ifdef CONFIG_VPL011_CONSOLE
>> +            /*
>> +             * if it is a vpl011 console pfn then map it to its
>
> s/if/If/
>> +             * own address space
>
> And you can also add an . here.
>
>> +             */
>> +            if ( a.index == HVM_PARAM_VPL011_CONSOLE_PFN )
>> +            {
>> +                vpl011_map_guest_page(d);
>> +            }
>> +#else
>> +            /*
>> +             * if VPL011 is not compiled in then disallow setting of any
>> +             * related HVM params
>> +             */
>> +            if ( a.index == HVM_PARAM_VPL011_CONSOLE_PFN ||
>> +                 a.index == HVM_PARAM_VPL011_CONSOLE_EVTCHN ||
>> +                 a.index == HVM_PARAM_VPL011_VIRQ )
>> +            {
>> +                rc = -1;
>> +                goto param_fail;
>> +            }
>> +#endif
>>          }
>>          else
>>          {
>> +#ifndef CONFIG_VPL011_CONSOLE
>> +            /*
>> +             * if VPL011 is not compiled in then disallow setting of any
>> +             * related HVM params
>> +             */
>> +            if ( a.index == HVM_PARAM_VPL011_CONSOLE_PFN ||
>> +                 a.index == HVM_PARAM_VPL011_CONSOLE_EVTCHN ||
>> +                 a.index == HVM_PARAM_VPL011_VIRQ )
>> +            {
>> +                rc = -1;
>> +                goto param_fail;
>
> This and the above look like it could be nicely folded in a function?
>
> Say:
>
> static bool vpl011_built(unsigned int idx)
> {
> #ifdef CONFIG_VPL011_CONSOLE
>         if ( idx == ..
>                 return true;
> #else
>         return false;
> }
>
> And here you can just do:
>         if (vpl011_built())
>         {
>                 rc = -1;
>         .. and so on.
>
>> +            }
>> +#endif
Using vpl011_built() removes the #ifdef from the code. But I still
need to check that the param is a vpl011 param before calling
vpl011_built() to decide whether to handle it or
flag an error.
So I have modified the code like this:

static bool vpl011_built()
{
#ifdef CONFIG_VPL011_CONSOLE
         return true;
#else
         return false;
#endif
}

if ( a.index == vpl011_param1 || a.index == vpl011_param2 || ... )
{
      if ( vpl011_built() )
      {
            /* handle vpl011 params */
           ...
      }
      else
      {
             rc = -1;
             ...
      }
}
... continue as usual for other parameters

>>              a.value = d->arch.hvm_domain.params[a.index];
>>              rc = copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
>>          }
>> diff --git a/xen/include/public/hvm/params.h b/xen/include/public/hvm/params.h
>> index 3f54a49..13bf719 100644
>> --- a/xen/include/public/hvm/params.h
>> +++ b/xen/include/public/hvm/params.h
>> @@ -203,10 +203,17 @@
>>   */
>>  #define HVM_PARAM_ACPI_IOPORTS_LOCATION 19
>>
>> -/* Deprecated */
>> +#if defined(__arm__) || defined(__aarch64__)
>> +#define HVM_PARAM_VPL011_CONSOLE_PFN    20
>> +#define HVM_PARAM_VPL011_CONSOLE_EVTCHN 21
>> +#define HVM_PARAM_VPL011_VIRQ           22
>> +#else
>>  #define HVM_PARAM_MEMORY_EVENT_CR0          20
>>  #define HVM_PARAM_MEMORY_EVENT_CR3          21
>>  #define HVM_PARAM_MEMORY_EVENT_CR4          22
>> +#endif
>> +
>> +/* Deprecated */
>>  #define HVM_PARAM_MEMORY_EVENT_INT3         23
>>  #define HVM_PARAM_MEMORY_EVENT_SINGLE_STEP  25
>>  #define HVM_PARAM_MEMORY_EVENT_MSR          30
>> @@ -253,6 +260,7 @@
>>   */
>>  #define HVM_PARAM_X87_FIP_WIDTH 36
>>
>> +
Removed extra line.
>
>
> ???
>>  #define HVM_NR_PARAMS 37
>>
>>  #endif /* __XEN_PUBLIC_HVM_PARAMS_H__ */
>> --
>> 2.7.4
>>
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> https://lists.xen.org/xen-devel
Bhupinder Thakur March 24, 2017, 7:31 a.m. | #14
Hi Julien,


>> Three new HVM param handlers added for:
>>     - allocating a new VIRQ and return to the toolstack
>
>
> This is not necessary. We could hardcode it.
I will modify the code to use a fixed SPI for vpl011.

>
>>     - allocating a new event channel for sending/receiving events from Xen
>> and return
>>       to the toolstack
>>     - mapping the PFN allocted by the toolstack to be used as IN/OUT ring
>> buffers
>
>
> s/allocted/allocated/
>
>
>>
>> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
>> ---
>>  xen/arch/arm/hvm.c              | 39
>> +++++++++++++++++++++++++++++++++++++++
>>  xen/include/public/hvm/params.h | 10 +++++++++-
>>  2 files changed, 48 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c
>> index d999bde..f3b9eb1 100644
>> --- a/xen/arch/arm/hvm.c
>> +++ b/xen/arch/arm/hvm.c
>> @@ -23,6 +23,8 @@
>>  #include <xen/guest_access.h>
>>  #include <xen/sched.h>
>>  #include <xen/monitor.h>
>> +#include <xen/event.h>
>> +#include <xen/vmap.h>
>>
>>  #include <xsm/xsm.h>
>>
>> @@ -31,6 +33,7 @@
>>  #include <public/hvm/hvm_op.h>
>>
>>  #include <asm/hypercall.h>
>> +#include "vpl011.h"
>>
>>  long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
>>  {
>> @@ -61,9 +64,45 @@ long do_hvm_op(unsigned long op,
>> XEN_GUEST_HANDLE_PARAM(void) arg)
>>          if ( op == HVMOP_set_param )
>>          {
>>              d->arch.hvm_domain.params[a.index] = a.value;
>> +
>> +#ifdef CONFIG_VPL011_CONSOLE
>> +            /*
>> +             * if it is a vpl011 console pfn then map it to its
>> +             * own address space
>> +             */
>> +            if ( a.index == HVM_PARAM_VPL011_CONSOLE_PFN )
>> +            {
>> +                vpl011_map_guest_page(d);
>
>
> vpl011_map_guest_page(...) is will return an error if it fails to map the
> page. Shouldn't you propagate the value.
I will propagate the value.

>
> Also, this code does not prevent the new HVM param to be set twice or even
> set by the guest. We probably want to introduce an helper to check if the
> domain is allowed to set it. See hvm_allow_set_param on x86.
How are these checks done on ARM currently?

>
>> +            }
>> +#else
>> +            /*
>> +             * if VPL011 is not compiled in then disallow setting of any
>> +             * related HVM params
>> +             */
>
>
> We really don't care if the toolstack set unused HVM param as they should
> not be used.
If the toolstack tries to set/get the vpl011 parameters (because user
has enabled pl011 emulation in libxl) while vpl011 support is compiled
out in Xen,
I wanted the toolstack to handle the failure case correctly.

>
>> +            if ( a.index == HVM_PARAM_VPL011_CONSOLE_PFN ||
>> +                 a.index == HVM_PARAM_VPL011_CONSOLE_EVTCHN ||
>> +                 a.index == HVM_PARAM_VPL011_VIRQ )
>> +            {
>> +                rc = -1;
>> +                goto param_fail;
>> +            }
>> +#endif
>>          }
>>          else
>>          {
>> +#ifndef CONFIG_VPL011_CONSOLE
>> +            /*
>> +             * if VPL011 is not compiled in then disallow setting of any
>> +             * related HVM params
>> +             */
>> +            if ( a.index == HVM_PARAM_VPL011_CONSOLE_PFN ||
>> +                 a.index == HVM_PARAM_VPL011_CONSOLE_EVTCHN ||
>> +                 a.index == HVM_PARAM_VPL011_VIRQ )
>> +            {
>> +                rc = -1;
>> +                goto param_fail;
>> +            }
>> +#endif
>
>
> Ditto.
>
>>              a.value = d->arch.hvm_domain.params[a.index];
>
>
> We probably don't want the guest to be able to read the console pfn page.
> See hvm_allow_get_param.
>
>>              rc = copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
>>          }
>> diff --git a/xen/include/public/hvm/params.h
>> b/xen/include/public/hvm/params.h
>> index 3f54a49..13bf719 100644
>> --- a/xen/include/public/hvm/params.h
>> +++ b/xen/include/public/hvm/params.h
>> @@ -203,10 +203,17 @@
>>   */
>>  #define HVM_PARAM_ACPI_IOPORTS_LOCATION 19
>>
>> -/* Deprecated */
>> +#if defined(__arm__) || defined(__aarch64__)
>> +#define HVM_PARAM_VPL011_CONSOLE_PFN    20
>> +#define HVM_PARAM_VPL011_CONSOLE_EVTCHN 21
>> +#define HVM_PARAM_VPL011_VIRQ           22
>> +#else
>>  #define HVM_PARAM_MEMORY_EVENT_CR0          20
>>  #define HVM_PARAM_MEMORY_EVENT_CR3          21
>>  #define HVM_PARAM_MEMORY_EVENT_CR4          22
>
>
> Those parameters are still deprecated but you drop the comment stating that.
Added the deprecated comment for x86.
>
>> +#endif
>> +
>
>
> Those params are x86 specific so should have never been set on ARM. But I am
> not sure if it is fine to re-purpose deprecated number.
>
> I have CCed "The REST" maintainers to have their input here.
>
>> +/* Deprecated */
>>  #define HVM_PARAM_MEMORY_EVENT_INT3         23
>>  #define HVM_PARAM_MEMORY_EVENT_SINGLE_STEP  25
>>  #define HVM_PARAM_MEMORY_EVENT_MSR          30
>> @@ -253,6 +260,7 @@
>>   */
>>  #define HVM_PARAM_X87_FIP_WIDTH 36
>>
>> +
>
>
> Spurious change.
>
>>  #define HVM_NR_PARAMS 37
>>
>>  #endif /* __XEN_PUBLIC_HVM_PARAMS_H__ */
>>
>
> Cheers,
>
> --
> Julien Grall
Bhupinder Thakur April 11, 2017, 2:38 p.m. | #15
Hi,

Kindly let me know if my understanding is correct.

Using a domctl API will allow us to keep the vUART configuration
flexible. Currently, we can operate on one ring-buf PFN and an event
channel port only for a single vUART but if we use DOMCTL interface,
then we can effectively get/set multiple event channel ports/multiple
PFNs from/to Xen in a single domctl call.

I am not clear who will be the caller of this domctl API. Is it
xenconsoled or the toolstack? Currently, xenconsoled reads the
ring-buf PFN and event channel from the xenstore, which is populated
by the toolstack.

Regards,
Bhupinder

On 8 March 2017 at 23:52, Stefano Stabellini <sstabellini@kernel.org> wrote:
> On Wed, 8 Mar 2017, Jan Beulich wrote:
>> >>> On 08.03.17 at 15:45, <julien.grall@arm.com> wrote:
>> > I was looking at the backend code and see it is using DOMCTL command. I
>> > guess it is considered that the console backend will be tied to a
>> > specific Xen version. Am I correct?
>>
>> I don't think I'm qualified to talk about the console backend
>> implementation (and possible quirks it has). Generally I'd expect
>> backends not to use domctl-s, as that would tie them to the tool
>> stack domain.
>>
>> > so maybe we can introduce new domctl command for handling vUART. This
>> > would avoid us to commit on an ABI and allow us to extend it if
>> > necessary in the future to support multiple UARTs.
>>
>> Well, without having the context of who it would be to use such a
>> domctl (and for what purpose) I don#t think I can comment here.
>
> I guess the assumption was that xenconsoled was part of the Xen tools.
> Indeed, it is part of the tools and is installed as such.
>
> I don't have an opinion on this. If introducing a new DOMCTL makes the
> code nicer in xen and xenconsoled, taking away some edges, like the
> changes to evtchn_send, then we should probably just do it.
Stefano Stabellini April 11, 2017, 10:07 p.m. | #16
On Tue, 11 Apr 2017, Bhupinder Thakur wrote:
> Hi,
> 
> Kindly let me know if my understanding is correct.
> 
> Using a domctl API will allow us to keep the vUART configuration
> flexible. Currently, we can operate on one ring-buf PFN and an event
> channel port only for a single vUART but if we use DOMCTL interface,
> then we can effectively get/set multiple event channel ports/multiple
> PFNs from/to Xen in a single domctl call.
> 
> I am not clear who will be the caller of this domctl API. Is it
> xenconsoled or the toolstack? Currently, xenconsoled reads the
> ring-buf PFN and event channel from the xenstore, which is populated
> by the toolstack.

The caller could be either, but I think it would make sense for it to be
xenconsoled to cut the middle-man (xl).


> On 8 March 2017 at 23:52, Stefano Stabellini <sstabellini@kernel.org> wrote:
> > On Wed, 8 Mar 2017, Jan Beulich wrote:
> >> >>> On 08.03.17 at 15:45, <julien.grall@arm.com> wrote:
> >> > I was looking at the backend code and see it is using DOMCTL command. I
> >> > guess it is considered that the console backend will be tied to a
> >> > specific Xen version. Am I correct?
> >>
> >> I don't think I'm qualified to talk about the console backend
> >> implementation (and possible quirks it has). Generally I'd expect
> >> backends not to use domctl-s, as that would tie them to the tool
> >> stack domain.
> >>
> >> > so maybe we can introduce new domctl command for handling vUART. This
> >> > would avoid us to commit on an ABI and allow us to extend it if
> >> > necessary in the future to support multiple UARTs.
> >>
> >> Well, without having the context of who it would be to use such a
> >> domctl (and for what purpose) I don#t think I can comment here.
> >
> > I guess the assumption was that xenconsoled was part of the Xen tools.
> > Indeed, it is part of the tools and is installed as such.
> >
> > I don't have an opinion on this. If introducing a new DOMCTL makes the
> > code nicer in xen and xenconsoled, taking away some edges, like the
> > changes to evtchn_send, then we should probably just do it.
>
Bhupinder Thakur April 14, 2017, 7:12 a.m. | #17
Hi Stefano,

On 12 April 2017 at 03:37, Stefano Stabellini <sstabellini@kernel.org> wrote:
> On Tue, 11 Apr 2017, Bhupinder Thakur wrote:
>> Hi,
>>
>> Kindly let me know if my understanding is correct.
>>
>> Using a domctl API will allow us to keep the vUART configuration
>> flexible. Currently, we can operate on one ring-buf PFN and an event
>> channel port only for a single vUART but if we use DOMCTL interface,
>> then we can effectively get/set multiple event channel ports/multiple
>> PFNs from/to Xen in a single domctl call.
>>
>> I am not clear who will be the caller of this domctl API. Is it
>> xenconsoled or the toolstack? Currently, xenconsoled reads the
>> ring-buf PFN and event channel from the xenstore, which is populated
>> by the toolstack.
>
> The caller could be either, but I think it would make sense for it to be
> xenconsoled to cut the middle-man (xl).
>
I see the issue with Xenconsoled getting the PFN using a DOMCTL API.

PFN is allocated in alloc_magic_pages() which is part of
libxenguest.so and the DOMCTL API is part of libxenctrl.so. As per my
understanding, libxenguest.so has dependency on libxenctrl.so but not
the other way round.So I am not sure how libxenctrl can call into
libxenguest to get the PFN.

If the DOMCTL API is called from libxenguest then I can get the PFN easily.

Regards,
Bhupinder
Stefano Stabellini April 19, 2017, 6:43 p.m. | #18
On Fri, 14 Apr 2017, Bhupinder Thakur wrote:
> Hi Stefano,
> 
> On 12 April 2017 at 03:37, Stefano Stabellini <sstabellini@kernel.org> wrote:
> > On Tue, 11 Apr 2017, Bhupinder Thakur wrote:
> >> Hi,
> >>
> >> Kindly let me know if my understanding is correct.
> >>
> >> Using a domctl API will allow us to keep the vUART configuration
> >> flexible. Currently, we can operate on one ring-buf PFN and an event
> >> channel port only for a single vUART but if we use DOMCTL interface,
> >> then we can effectively get/set multiple event channel ports/multiple
> >> PFNs from/to Xen in a single domctl call.
> >>
> >> I am not clear who will be the caller of this domctl API. Is it
> >> xenconsoled or the toolstack? Currently, xenconsoled reads the
> >> ring-buf PFN and event channel from the xenstore, which is populated
> >> by the toolstack.
> >
> > The caller could be either, but I think it would make sense for it to be
> > xenconsoled to cut the middle-man (xl).
> >
> I see the issue with Xenconsoled getting the PFN using a DOMCTL API.
> 
> PFN is allocated in alloc_magic_pages() which is part of
> libxenguest.so and the DOMCTL API is part of libxenctrl.so. As per my
> understanding, libxenguest.so has dependency on libxenctrl.so but not
> the other way round.So I am not sure how libxenctrl can call into
> libxenguest to get the PFN.
> 
> If the DOMCTL API is called from libxenguest then I can get the PFN easily.

Yes, that is the case. libxenguest would call libxenctrl to issue
domctls. You would simply replace hvm_param reads/writes with a new pair
of domctl, conceptually very similar.

Patch

diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c
index d999bde..f3b9eb1 100644
--- a/xen/arch/arm/hvm.c
+++ b/xen/arch/arm/hvm.c
@@ -23,6 +23,8 @@ 
 #include <xen/guest_access.h>
 #include <xen/sched.h>
 #include <xen/monitor.h>
+#include <xen/event.h>
+#include <xen/vmap.h>
 
 #include <xsm/xsm.h>
 
@@ -31,6 +33,7 @@ 
 #include <public/hvm/hvm_op.h>
 
 #include <asm/hypercall.h>
+#include "vpl011.h"
 
 long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
 {
@@ -61,9 +64,45 @@  long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
         if ( op == HVMOP_set_param )
         {
             d->arch.hvm_domain.params[a.index] = a.value;
+
+#ifdef CONFIG_VPL011_CONSOLE
+            /*
+             * if it is a vpl011 console pfn then map it to its
+             * own address space
+             */
+            if ( a.index == HVM_PARAM_VPL011_CONSOLE_PFN )
+            {
+                vpl011_map_guest_page(d);
+            }
+#else
+            /* 
+             * if VPL011 is not compiled in then disallow setting of any 
+             * related HVM params
+             */
+            if ( a.index == HVM_PARAM_VPL011_CONSOLE_PFN ||
+                 a.index == HVM_PARAM_VPL011_CONSOLE_EVTCHN ||
+                 a.index == HVM_PARAM_VPL011_VIRQ )
+            {
+                rc = -1;
+                goto param_fail;
+            }
+#endif
         }
         else
         {
+#ifndef CONFIG_VPL011_CONSOLE
+            /* 
+             * if VPL011 is not compiled in then disallow setting of any 
+             * related HVM params
+             */
+            if ( a.index == HVM_PARAM_VPL011_CONSOLE_PFN ||
+                 a.index == HVM_PARAM_VPL011_CONSOLE_EVTCHN ||
+                 a.index == HVM_PARAM_VPL011_VIRQ )
+            {
+                rc = -1;
+                goto param_fail;
+            }
+#endif
             a.value = d->arch.hvm_domain.params[a.index];
             rc = copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
         }
diff --git a/xen/include/public/hvm/params.h b/xen/include/public/hvm/params.h
index 3f54a49..13bf719 100644
--- a/xen/include/public/hvm/params.h
+++ b/xen/include/public/hvm/params.h
@@ -203,10 +203,17 @@ 
  */
 #define HVM_PARAM_ACPI_IOPORTS_LOCATION 19
 
-/* Deprecated */
+#if defined(__arm__) || defined(__aarch64__)
+#define HVM_PARAM_VPL011_CONSOLE_PFN    20
+#define HVM_PARAM_VPL011_CONSOLE_EVTCHN 21
+#define HVM_PARAM_VPL011_VIRQ           22
+#else
 #define HVM_PARAM_MEMORY_EVENT_CR0          20
 #define HVM_PARAM_MEMORY_EVENT_CR3          21
 #define HVM_PARAM_MEMORY_EVENT_CR4          22
+#endif
+
+/* Deprecated */
 #define HVM_PARAM_MEMORY_EVENT_INT3         23
 #define HVM_PARAM_MEMORY_EVENT_SINGLE_STEP  25
 #define HVM_PARAM_MEMORY_EVENT_MSR          30
@@ -253,6 +260,7 @@ 
  */
 #define HVM_PARAM_X87_FIP_WIDTH 36
 
+
 #define HVM_NR_PARAMS 37
 
 #endif /* __XEN_PUBLIC_HVM_PARAMS_H__ */