| Message ID | 1487676368-22356-3-git-send-email-bhupinder.thakur@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | pl011 emulation support in Xen | expand |
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
(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,
>>> 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
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,
>>> 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
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
>>> 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
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
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,
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,
>>> 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
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.
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
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
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.
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. >
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
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.
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__ */
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(-)