| Message ID | 1487676368-22356-1-git-send-email-bhupinder.thakur@linaro.org |
|---|---|
| Headers | show |
| Series | pl011 emulation support in Xen | expand |
> The following changes were done:
.. snip..
Thank you for this great writeup. I took a stab at it and stopped at patch
#2 b/c Julien said he would look in it deeper. But based on a brief
look I would say:
- Please do remove most of the comments. They really do not add
much context besides describing the code - and we all can
read the code. The idea behind the comments is to describe some
semantics of it, or something that is not obvious at first or
such. Not the code.
- Comments. One line comments are:
/* Comment. */
And please do use proper case and a period.
- Be careful about compiler optimizations and jump tables.
Specifically see https://xenbits.xen.org/xsa/advisory-155.html
The way to make sure you don't introduce an security problem
is to 1) use local variables 2) read once from the ring and
make sure you use a compiler barrier.
- There is also some unrelated changes. Like extra newlines. One
way to avoid this is to send all your patches _just_ to yourself
and review them - but review them in reverse order and from the
bottom of the emails to the top. That way you can catch some of this.
- Think in terms of how one would break this. For example the guest
could change the HVM parameters (or maybe not?) - or find the
console ring and muck with the ring indexes. You need to shield
the code from such changes.
Thanks!
On Fri, Mar 03, 2017 at 03:23:31PM -0500, Konrad Rzeszutek Wilk wrote: > > The following changes were done: > > .. snip.. > > Thank you for this great writeup. I took a stab at it and stopped at patch > #2 b/c Julien said he would look in it deeper. But based on a brief > look I would say: Run this on each patch: $more bin/add_c #!/bin/bash git diff HEAD^.. > /tmp/a echo "---" cat /tmp/a | scripts/get_maintainer.pl --no-l | while read file; do echo "Cc: $file";done rm -f /tmp/a So that you have the right maintainers on the CC line
Hi Bhupinder, On 02/21/2017 11:25 AM, Bhupinder Thakur wrote: > There are still some items which are pending: > > 1. Adding dynamic enable/disable of pl011 emulation for a guest > 2. Add a new console type "pl011" in xenconsoled to allow the user to connect to > either PV/serial/pl011 console. > 3. Add checks to ensure that the new hvm params read/written by the guest A couple of recommendations regarding the series: - All maintainers of the code you touch in a patch should be CCed. You can use scripts/get_maintainters.pl for that. - Providing a git branch with your code will allow people to test your code without handling potential rebasing. Cheers,
Hi, Thanks for your comments. On 3 March 2017 at 21:23, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote: >> The following changes were done: > > .. snip.. > > Thank you for this great writeup. I took a stab at it and stopped at patch > #2 b/c Julien said he would look in it deeper. But based on a brief > look I would say: > - Please do remove most of the comments. They really do not add > much context besides describing the code - and we all can > read the code. The idea behind the comments is to describe some > semantics of it, or something that is not obvious at first or > such. Not the code. I will remove the comments where not required. > > - Comments. One line comments are: > /* Comment. */ > And please do use proper case and a period. > I will correct the comments. > - Be careful about compiler optimizations and jump tables. > Specifically see https://xenbits.xen.org/xsa/advisory-155.html > The way to make sure you don't introduce an security problem > is to 1) use local variables 2) read once from the ring and > make sure you use a compiler barrier. > will check the guidelines. > - There is also some unrelated changes. Like extra newlines. One > way to avoid this is to send all your patches _just_ to yourself > and review them - but review them in reverse order and from the > bottom of the emails to the top. That way you can catch some of this. > > - Think in terms of how one would break this. For example the guest > could change the HVM parameters (or maybe not?) - or find the > console ring and muck with the ring indexes. You need to shield > the code from such changes. > I plan to add the checks that the HVM params can only be accessed from a privileged guest. > Thanks!
Hi, On 5 March 2017 at 12:46, Julien Grall <julien.grall@arm.com> wrote: > Hi Bhupinder, > > On 02/21/2017 11:25 AM, Bhupinder Thakur wrote: >> >> There are still some items which are pending: >> >> 1. Adding dynamic enable/disable of pl011 emulation for a guest >> 2. Add a new console type "pl011" in xenconsoled to allow the user to >> connect to >> either PV/serial/pl011 console. >> 3. Add checks to ensure that the new hvm params read/written by the guest > > > A couple of recommendations regarding the series: > - All maintainers of the code you touch in a patch should be CCed. > You can use scripts/get_maintainters.pl for that. I will run the script before sending the patches. > - Providing a git branch with your code will allow people to test > your code without handling potential rebasing. > I plan to check-in the code in a linaro git repository and will share the branch information in the next patch. > Cheers, > > -- > Julien Grall
PL011 emulation for guests in Xen ===================================== This feature allows the Xen guests to map their console to a SBSA compliant pl011 UART as specified in ARM Service Base System architecture, Appendix B. Currently, Xen supports paravirtualized (aka PV) and an emulated serial consoles. This feature will allow an emulated SBSA pl011 UART console, which a user can access using xenconsoled running on dom0. The device tree passed to the guest VM will contain the pl011 MMIO address range and an irq for receiving rx/tx pl011 interrupts. The device tree format is specified in Documentation/devicetree/bindings/serial/arm_sbsa_uart.txt. The Xen hypervisor will expose two types of interfaces to dom0 and domU. The interface exposed to domU will be an emulated pl011 UART by emulating the access to the following pl011 registers by the guest. - Data register (DR) - RW - Control register (CR) - RW - Raw interrupt status register (RIS) - RO - Masked interrupt status register (MIS)- RO - Interrupt Mask (IMSC) - RW - Interrupt Clear (ICR) - WO It will also inject the pl011 interrupts to the guest in the following conditions: - incoming data in the rx buffer for the guest - there is space in the tx buffer for the guest to write more data The interface exposed to dom0 will be the same PV console interface, which minimizes the changes required in xenconsole to support a new pl011 console. This interface has rx and tx ring buffers and an event channel for sending/receiving events from dom0. So essentially Xen handles the data on behalf of domU and dom0. Any data written by domU is captured by Xen and written to the TX (OUT) ring buffer and a pl011 event is raised to dom0 to read the TX ring buffer. Similarly on reciving a pl011 event, Xen injects an interrupt to guest to indicate there is data available in the RX (IN) ring buffer. Note that the pl011 UART state is completely captured in the set of registers mentioned above and this state is updated everytime there is an event from dom0 or there is register read/write access from domU. For example, if domU has masked the rx interrupt in the IMSC register, then Xen will not inject an interrupt to guest and will just update the RIS register. Once the interrupt is unmasked by guest, the interrupt will be delivered to the guest. The following changes were done: Xen Hypervisor =============== 1. Add emulation code to emulate read/write access to pl011 registers and pl011 interrupts - It emulates DR read/write by reading and writing from/to the IN and OUT ring buffers and raising an event to dom0 when there is data in the OUT ring buffer and injecting an interrupt to the guest when there is data in the IN ring buffer - Other registers are related to interrupt management and essentially control when interrupts are delivered to the guest 2. Add three new HVM param handlers 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 3. Breakup evtchn_send() to allow sending events for a Xen bound channel (Currently, there is a check which disallows generating events for a Xen bound channel) 4. Enable vpl011 emulation for a domain when it is created 5. Ensuring that nr_spis is atleast 1 to allow allocation of vpl011 spi virq Toolstack ========== 1. Reading a VIRQ from Xen using a hvm call and using it for adding new device tree node in the guest DT for SBSA pl011 uart containing the IRQ and the MMIO address range to be used by the guest 2. Allocating a new PFN and passing it on to Xen though a hvm call to be used as the IN/OUT ring buffers 3. Add two new parameters to the xen store - newly allocated PFN to be used as IN/OUT ring buffer by xenconsoled - a new event channel read from Xen using a hvm call to be used by xenconsoled for eventing Xenconsoled ============ 1. Modfication in domain_create_ring() - Bind to the vpl011 event channel obtained from the xen store as a new parameter - Map the PFN to its address space to be used as IN/OUT ring buffers. It obtains the PFN from the xen store as a new parameter 2. Modificaton to handle_ring_read() and buffer_append () to allow reading data from both PV or vpl011 OUT ring buffers and add to the output buffer 3. Modification in handle_tty_read to write the user data to the vpl011 IN ring buffer There are still some items which are pending: 1. Adding dynamic enable/disable of pl011 emulation for a guest 2. Add a new console type "pl011" in xenconsoled to allow the user to connect to either PV/serial/pl011 console. 3. Add checks to ensure that the new hvm params read/written by the guest Bhupinder Thakur (11): xen/arm: vpl011: Add pl011 uart emulation in Xen xen/arm: vpl011: Add new hvm params in Xen for ring buffer/event setup xen/arm: vpl011: Refactor evtchn_send in Xen to allow sending events from a xen bound channel xen/arm: vpl011: Enable vpl011 emulation for a domain in Xen xen/arm: vpl011: Initialize nr_spis in vgic_init in Xen to atleast 1 xen/arm: vpl011: Add a new pl011 uart node in the guest DT in the toolstack xen/arm: vpl011: Add two new vpl011 parameters to xenstore xen/arm: vpl011: Allocate a new PFN in the toolstack and pass to Xen using a hvm call xen/arm: vpl011: Modify domain_create_ring in xenconsole to map the ring buffer and event channel xen/arm: vpl011: Modify handle_ring_read and buffer_append to read/append vpl011 data xen/arm: vpl011: Modify handle_tty_read in xenconsole to redirect user data to vpl011 IN ring buffer tools/console/daemon/io.c | 168 +++++++++++++++--- tools/libxc/include/xc_dom.h | 5 + tools/libxc/xc_dom_arm.c | 7 +- tools/libxc/xc_dom_boot.c | 5 + tools/libxc/xc_domain.c | 3 + tools/libxl/libxl.c | 6 + tools/libxl/libxl_arm.c | 47 +++++- tools/libxl/libxl_dom.c | 18 +- tools/libxl/libxl_internal.h | 4 + xen/arch/arm/Makefile | 1 + xen/arch/arm/domain.c | 8 + xen/arch/arm/hvm.c | 39 +++++ xen/arch/arm/vgic.c | 5 + xen/arch/arm/vpl011.c | 366 ++++++++++++++++++++++++++++++++++++++++ xen/arch/arm/vpl011.h | 208 +++++++++++++++++++++++ xen/common/Kconfig | 6 + xen/common/domctl.c | 2 + xen/common/event_channel.c | 49 ++++-- xen/include/asm-arm/domain.h | 15 ++ xen/include/public/arch-arm.h | 5 + xen/include/public/hvm/params.h | 10 +- xen/include/xen/event.h | 6 + xen/include/xen/sched.h | 4 + 23 files changed, 944 insertions(+), 43 deletions(-) create mode 100644 xen/arch/arm/vpl011.c create mode 100644 xen/arch/arm/vpl011.h