| Message ID | 1487676368-22356-2-git-send-email-bhupinder.thakur@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | pl011 emulation support in Xen | expand |
Hi Bhupinder, On 02/21/2017 11:25 AM, Bhupinder Thakur wrote: > 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 A link to the documentation would have been helpful. > Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org> > --- > xen/arch/arm/Makefile | 1 + > xen/arch/arm/vpl011.c | 366 ++++++++++++++++++++++++++++++++++++++++++ > xen/arch/arm/vpl011.h | 208 ++++++++++++++++++++++++ > xen/common/Kconfig | 6 + > xen/include/asm-arm/domain.h | 15 ++ > xen/include/public/arch-arm.h | 5 + > 6 files changed, 601 insertions(+) > create mode 100644 xen/arch/arm/vpl011.c > create mode 100644 xen/arch/arm/vpl011.h > > diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile > index 7afb8a3..a94bdab 100644 > --- a/xen/arch/arm/Makefile > +++ b/xen/arch/arm/Makefile > @@ -49,6 +49,7 @@ obj-y += vm_event.o > obj-y += vtimer.o > obj-y += vpsci.o > obj-y += vuart.o > +obj-$(CONFIG_VPL011_CONSOLE) += vpl011.o > > #obj-bin-y += ....o > > diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c > new file mode 100644 > index 0000000..88ba968 > --- /dev/null > +++ b/xen/arch/arm/vpl011.c > @@ -0,0 +1,366 @@ > +/* > + * arch/arm/vpl011.c > + * > + * Virtual PL011 UART > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms and conditions of the GNU General Public License, > + * version 2, as published by the Free Software Foundation. > + * > + * This program is distributed in the hope it will be useful, but WITHOUT > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or > + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for > + * more details. > + * > + * You should have received a copy of the GNU General Public License along with > + * this program; If not, see <http://www.gnu.org/licenses/>. > + */ > + > +#include <xen/config.h> xen/config.h is included by default. Please drop it. > +#include <xen/init.h> > +#include <xen/lib.h> > +#include <xen/errno.h> > +#include <xen/guest_access.h> > +#include <xen/sched.h> > +#include <xen/monitor.h> Why do you need to include monitor.h? > +#include <xen/event.h> > +#include <xen/vmap.h> > + > +#include <xsm/xsm.h> Ditto. > + > +#include <public/xen.h> > +#include <public/hvm/params.h> > +#include <public/hvm/hvm_op.h> > + > +#include <asm/hypercall.h> Ditto. > +#include "vpl011.h" > + > +static int vpl011_mmio_read(struct vcpu *v, mmio_info_t *info, register_t *r, void *priv) > +{ > + unsigned char ch; > + > + switch (info->gpa - GUEST_PL011_BASE) Coding style: switch ( ... ) > + { > + case VPL011_UARTCR_OFFSET: Coding style: the case should be aligned to {. E.g { case ... Also, I would prefer if you don't include _OFFSET in all the name. Furthermore, can you please order the case by offset in the MMIO region. This would help to find if a register has been emulated or not. Lastly, the user may have requested to read 8-bit, 16-bit, 32-bit. But you always return a 32-bit. Is a guest allowed to access using any size and in the middle of a register? Give a look on what is done for vgic-v{2,3}.c to check the size and read register. You may want to pull some code out to re-use here. > + *r = v->domain->arch.vpl011.control; Similarly, I would prefer if the name of the field match the register name. > + break; > + case VPL011_UARTDR_OFFSET: > + vpl011_read_data(v->domain, &ch); Should not you check the return value of vpl011_read_data? Also, what if there is no data? > + *r = ch; > + break; > + case VPL011_UARTFR_OFFSET: > + *r = v->domain->arch.vpl011.flag; I am fairly surprised that none of this code is actually protected by lock. For instance the update of flag is not atomic. So is it safe? > + break; > + case VPL011_UARTIMSC_OFFSET: > + *r = v->domain->arch.vpl011.intr_mask; > + break; > + case VPL011_UARTICR_OFFSET: > + *r = 0; Looking at the spec, this register is write-only. So why do you implement RAZ? > + break; > + case VPL011_UARTRIS_OFFSET: > + *r = v->domain->arch.vpl011.raw_intr_status; > + break; > + case VPL011_UARTMIS_OFFSET: > + *r = v->domain->arch.vpl011.raw_intr_status & > + v->domain->arch.vpl011.intr_mask; > + break; > + case VPL011_UARTDMACR_OFFSET: > + *r = 0; /* uart DMA is not supported. Here it always returns 0 */ My understanding of the spec is DMA is not optional. So what would happen if the guest tries to enable it? > + break; > + case VPL011_UARTRSR_OFFSET: > + *r = 0; /* it always returns 0 as there are no physical errors */ This register contains contains the bit OE that tells whether the FIFO is full or not. The FIFO here is the PV ring, so maybe we should set this bit if the ring is full. > + break; > + default: > + printk ("vpl011_mmio_read: invalid switch case %d\n", (int)(info->gpa - GUEST_PL011_BASE)); Coding style: printk(...). Also, printk is not ratelimited by default. Please use gprintk(...) which will be ratelimited and print the domain information. This is useful when you have multiple guest. > + break; > + } > + > + return VPL011_EMUL_OK; Please use plain value as the return is not pl011 specific. Also, I am a bit surprised that you return "ok" even when a register as not been emulated. IHMO, a data abort should be sent to the guest. Furthermore, looking at the overall function, I think it should be better if you re-use the template used by the other emulation (see vgic-v2.c) for instance. They have labels that helps to know what's going on. I will stop here in the review for today and will comment the rest tomorrow. Cheers,
Hi Bhupinder, On 26/02/17 21:37, Julien Grall wrote: > On 02/21/2017 11:25 AM, Bhupinder Thakur wrote: [...] >> + { >> + case VPL011_UARTCR_OFFSET: > > Coding style: the case should be aligned to {. E.g > > { > case ... > > Also, I would prefer if you don't include _OFFSET in all the name. > > Furthermore, can you please order the case by offset in the MMIO region. > This would help to find if a register has been emulated or not. > > Lastly, the user may have requested to read 8-bit, 16-bit, 32-bit. But > you always return a 32-bit. Is a guest allowed to access using any size > and in the middle of a register? Give a look on what is done for > vgic-v{2,3}.c to check the size and read register. You may want to pull > some code out to re-use here. Answering to myself here. Looking at the SBSA UART spec, register could be accessed with different size. SO please handle that in the next version. [...] >> + case VPL011_UARTDMACR_OFFSET: >> + *r = 0; /* uart DMA is not supported. Here it always >> returns 0 */ > > My understanding of the spec is DMA is not optional. So what would > happen if the guest tries to enable it? Answering to myself here. As discussed on Wednesday, we will emulate a subset of PL011 registers to be compliant with SBSA UART as exposed in the guest DT (see patch #6). This would avoid for us to handle unnecessary register (such as DMA, line control register...). But I would keep the file name vpl011.c so someone can extend to support a full PL011 if necessary. You would also need to explain in the commit message what we are emulating and give a link to the SBSA UART spec. Cheers,
.snip.. > + case VPL011_UARTCR_OFFSET: > + *r = v->domain->arch.vpl011.control; > + break; Pls add a new newline after each break. > + case VPL011_UARTDR_OFFSET: > + vpl011_read_data(v->domain, &ch); > + *r = ch; > + break; > + case VPL011_UARTFR_OFFSET: > + *r = v->domain->arch.vpl011.flag; > + break; > + case VPL011_UARTIMSC_OFFSET: > + *r = v->domain->arch.vpl011.intr_mask; > + break; > + case VPL011_UARTICR_OFFSET: > + *r = 0; > + break; > + case VPL011_UARTRIS_OFFSET: > + *r = v->domain->arch.vpl011.raw_intr_status; > + break; > + case VPL011_UARTMIS_OFFSET: > + *r = v->domain->arch.vpl011.raw_intr_status & > + v->domain->arch.vpl011.intr_mask; > + break; > + case VPL011_UARTDMACR_OFFSET: > + *r = 0; /* uart DMA is not supported. Here it always returns 0 */ > + break; > + case VPL011_UARTRSR_OFFSET: > + *r = 0; /* it always returns 0 as there are no physical errors */ > + break; > + default: > + printk ("vpl011_mmio_read: invalid switch case %d\n", (int)(info->gpa - GUEST_PL011_BASE)); .. and you still return EMULOK? Also is this printk really neccessary? > + break; > + } > + > + return VPL011_EMUL_OK; > +} > + > +static int vpl011_mmio_write(struct vcpu *v, mmio_info_t *info, register_t r, void *priv) > +{ > + unsigned char ch = r; > + > + switch (info->gpa - GUEST_PL011_BASE) > + { > + case VPL011_UARTCR_OFFSET: > + v->domain->arch.vpl011.control = r; > + break; > + case VPL011_UARTDR_OFFSET: > + vpl011_write_data(v->domain, ch); > + break; > + case VPL011_UARTIMSC_OFFSET: > + v->domain->arch.vpl011.intr_mask = r; > + if ( (v->domain->arch.vpl011.raw_intr_status & v->domain->arch.vpl011.intr_mask) ) > + vgic_vcpu_inject_spi(v->domain, (int)v->domain->arch.hvm_domain.params[HVM_PARAM_VPL011_VIRQ]); > + break; > + case VPL011_UARTICR_OFFSET: > + /* > + * clear all bits which are set in the input > + */ > + v->domain->arch.vpl011.raw_intr_status &= ~r; > + if ( (v->domain->arch.vpl011.raw_intr_status & v->domain->arch.vpl011.intr_mask) ) > + { > + vgic_vcpu_inject_spi(v->domain, (int)v->domain->arch.hvm_domain.params[HVM_PARAM_VPL011_VIRQ]); > + } > + break; > + case VPL011_UARTRSR_OFFSET: // nothing to clear > + break; > + case VPL011_UARTFR_OFFSET: // these are all RO registers > + case VPL011_UARTRIS_OFFSET: > + case VPL011_UARTMIS_OFFSET: > + case VPL011_UARTDMACR_OFFSET: > + break; > + default: > + printk ("vpl011_mmio_write: switch case not handled %d\n", (int)(info->gpa - GUEST_PL011_BASE)); > + break; > + } > + > + return VPL011_EMUL_OK; > +} > + > +static const struct mmio_handler_ops vpl011_mmio_handler = { > + .read = vpl011_mmio_read, > + .write = vpl011_mmio_write, > +}; > + > + why the extra newline. > + > +int vpl011_map_guest_page(struct domain *d) > +{ > + int rc=0; No need for = 0; > + > + /* > + * map the guest PFN to Xen address space > + */ > + rc = prepare_ring_for_helper(d, > + d->arch.hvm_domain.params[HVM_PARAM_VPL011_CONSOLE_PFN], > + &d->arch.vpl011.ring_page, > + (void **)&d->arch.vpl011.ring_buf); > + if ( rc < 0 ) > + { > + printk("Failed to map vpl011 guest PFN\n"); > + } > + > + return rc; Could you just make this whole routine be: return prepare_ring_for_helper(..) That printk is not needed I think? I would add it to the caller of this function. > +} > + > +static int vpl011_data_avail(struct domain *d) > +{ > + int rc=0; > + unsigned long flags; > + > + struct console_interface *intf=(struct console_interface *)d->arch.vpl011.ring_buf; Can you have an macro for this? > + > + VPL011_LOCK(d, flags); Please don't. Just use normal spin_lock invocation. > + > + /*` > + * check IN ring buffer > + */ Please remove the comment. It does not add much context to the code. > + if ( !VPL011_IN_RING_EMPTY(intf) ) > + { > + /* > + * clear the RX FIFO empty flag as the ring is not empty > + */ Please remove this comment. > + d->arch.vpl011.flag &= ~(VPL011_UARTFR_RXFE); > + > + /* > + * if the buffer is full then set the RX FIFO FULL flag Also please remove this comment. > + */ > + if ( VPL011_IN_RING_FULL(intf) ) > + d->arch.vpl011.flag |= (VPL011_UARTFR_RXFF); > + > + /* > + * set the RX interrupt status > + */ And this one too. What I would recommend is that you write one nice comment at the start of the 'if' saying: "Have to set RX state regardless whether it is full or has some entries." > + d->arch.vpl011.raw_intr_status |= (VPL011_UARTRIS_RXRIS); > + } > + > + /* > + * check OUT ring buffer Please remove this comment. > + */ > + if ( !VPL011_OUT_RING_FULL(intf) ) > + { > + /* > + * if the buffer is not full then clear the TX FIFO full flag > + */ Please remove this comment. > + d->arch.vpl011.flag &= ~(VPL011_UARTFR_TXFF); > + > + /* > + * set the TX interrupt status > + */ > + d->arch.vpl011.raw_intr_status |= (VPL011_UARTRIS_TXRIS); > + > + if ( VPL011_OUT_RING_EMPTY(intf) ) > + { > + /* > + * clear the uart busy flag and set the TX FIFO empty flag > + */ > + d->arch.vpl011.flag &= ~(VPL011_UARTFR_BUSY); > + d->arch.vpl011.flag |= (VPL011_UARTFR_TXFE); > + } > + } > + > + VPL011_UNLOCK(d, flags); > + > + /* > + * send an interrupt if it is not masked So does that mean we would send an interrupt even if the in/out rings are full? > + */ > + if ( (d->arch.vpl011.raw_intr_status & d->arch.vpl011.intr_mask) ) > + vgic_vcpu_inject_spi(d, (int)d->arch.hvm_domain.params[HVM_PARAM_VPL011_VIRQ]); > + > + if ( !VPL011_OUT_RING_EMPTY(intf) ) This is asking for a race. So it may have changed between the first time you read it and this one. Is that OK? Would it still be OK to send an interrupt even if say the ring was emtpry at start of the function but became empty now? Would it be better if you read all the values from the ring at the start of the function in local variables and made sure to use 'barrier()' so there are no compiler "optimizations" done? > + { > + /* > + * raise an interrupt to dom0 > + */ Please remove this. > + rc = raw_evtchn_send(d, > + d->arch.hvm_domain.params[HVM_PARAM_VPL011_CONSOLE_EVTCHN], NULL); What if HVM_PARAM_VPL011_CONSOLE_EVTCHN is zero? Should you check that first? Or is it given (in which case please add an ASSERT) that the HVM_PARAM_VPL011_CONSOLE_EVTCHN is _always_ set? > + > + if ( rc < 0 ) > + printk("Failed to send vpl011 interrupt to dom0\n"); gdprintk. But is this useful? How will this help in debugging the problem? > + } > + > + return rc; > +} > + > +int vpl011_read_data(struct domain *d, unsigned char *data) > +{ > + int rc=0; No point for this. Just do 'return 0' > + unsigned long flags; > + struct console_interface *intf=(struct console_interface *)d->arch.vpl011.ring_buf; > + > + *data = 0; > + > + VPL011_LOCK(d, flags); > + > + /* > + * if there is data in the ring buffer then copy it to the output buffer > + */ > + if ( !VPL011_IN_RING_EMPTY(intf) ) > + { > + *data = intf->in[MASK_VPL011CONS_IDX(intf->in_cons++, intf->in)]; > + } > + > + /* > + * if the ring buffer is empty then set the RX FIFO empty flag > + */ > + if ( VPL011_IN_RING_EMPTY(intf) ) So there is a nice race here. Why don't you just use an 'else' ? > + { > + d->arch.vpl011.flag |= (VPL011_UARTFR_RXFE); > + d->arch.vpl011.raw_intr_status &= ~(VPL011_UARTRIS_RXRIS); > + } > + > + /* > + * clear the RX FIFO full flag > + */ > + d->arch.vpl011.flag &= ~(VPL011_UARTFR_RXFF); > + > + VPL011_UNLOCK(d, flags); > + > + return rc; > +} > + > +int vpl011_write_data(struct domain *d, unsigned char data) > +{ > + int rc=0; Pls remove the '=0' > + unsigned long flags; > + struct console_interface *intf=(struct console_interface *)d->arch.vpl011.ring_buf; > + > + VPL011_LOCK(d, flags); > + > + /* > + * if there is space in the ring buffer then write the data Please moreve this comment. > + */ > + if ( !VPL011_OUT_RING_FULL(intf) ) > + { > + intf->out[MASK_VPL011CONS_IDX(intf->out_prod++, intf->out)] = data; > + smp_wmb(); > + } > > + /* > + * if there is no space in the ring buffer then set the > + * TX FIFO FULL flag Again, why not just an 'else'? > + */ > + if ( VPL011_OUT_RING_FULL(intf) ) > + { > + d->arch.vpl011.flag |= (VPL011_UARTFR_TXFF); > + d->arch.vpl011.raw_intr_status &= ~(VPL011_UARTRIS_TXRIS); > + } > + > + /* > + * set the uart busy status > + */ > + d->arch.vpl011.flag |= (VPL011_UARTFR_BUSY); > + > + /* > + * clear the TX FIFO empty flag > + */ Please remove these comments. They are distracting. Also a single line comment should be: /* <comment> */ > + d->arch.vpl011.flag &= ~(VPL011_UARTFR_TXFE); > + > + VPL011_UNLOCK(d, flags); > + > + /* > + * raise an event to dom0 only if it is the first character in the buffer Why? Why only the first charcter? > + */ > + if ( VPL011_RING_DEPTH(intf, out) == 1 ) > + { > + rc = raw_evtchn_send(d, > + d->arch.hvm_domain.params[HVM_PARAM_VPL011_CONSOLE_EVTCHN], NULL); And again, should we have an assert for HVM_PARAM_VPL011_CONSOLE_EVTCHN or a check for it? > + > + if ( rc < 0 ) > + printk("Failed to send vpl011 interrupt to dom0\n"); gdprintk. > + } > + > + return rc; > +} > + > +static void vpl011_notification(struct vcpu *v, unsigned int port) > +{ > + vpl011_data_avail(v->domain); > +} > + > +int domain_vpl011_init(struct domain *d) > +{ > + int rc=0; You can remove the =0; > + > + /* > + * register xen event channel > + */ Please remove this comment. > + rc = alloc_unbound_xen_event_channel(d, 0, current->domain->domain_id, > + vpl011_notification); > + if (rc < 0) Spaces. > + { > + printk ("Failed to allocate vpl011 event channel\n"); gdprintk And also not sure if this is needed. Won't the rc be propagated to the caller who can print this? > + return rc; > + } > + d->arch.hvm_domain.params[HVM_PARAM_VPL011_CONSOLE_EVTCHN] = rc; > + > + /* > + * allocate an SPI VIRQ for the guest > + */ > + d->arch.hvm_domain.params[HVM_PARAM_VPL011_VIRQ] = vgic_allocate_spi(d); You are not checking vgic_allocate_spi for errors? > + > + /* > + * register mmio handler > + */ Please remove these comments. They are just saying what the code is doing. Not adding anything extra. > + register_mmio_handler (d, &vpl011_mmio_handler, GUEST_PL011_BASE, GUEST_PL011_SIZE, NULL); Please kill that space before (. And what if register_mmio_handler returns an error? > + > + /* > + * initialize the lock > + */ That is a comment that is really not needed. > + spin_lock_init(&d->arch.vpl011.lock); > + > + /* > + * clear the flag, control and interrupt status registers > + */ Why not just do memset on the structure? > + d->arch.vpl011.control = 0; > + d->arch.vpl011.flag = 0; > + d->arch.vpl011.intr_mask = 0; > + d->arch.vpl011.intr_clear = 0; > + d->arch.vpl011.raw_intr_status = 0; > + d->arch.vpl011.masked_intr_status = 0; > + > + return 0; > +} > diff --git a/xen/arch/arm/vpl011.h b/xen/arch/arm/vpl011.h > new file mode 100644 > index 0000000..f2c577f > --- /dev/null > +++ b/xen/arch/arm/vpl011.h > @@ -0,0 +1,208 @@ > +/* > + * include/xen/vpl011.h > + * > + * Virtual PL011 UART > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms and conditions of the GNU General Public License, > + * version 2, as published by the Free Software Foundation. > + * > + * This program is distributed in the hope it will be useful, but WITHOUT > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or > + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for > + * more details. > + * > + * You should have received a copy of the GNU General Public License along with > + * this program; If not, see <http://www.gnu.org/licenses/>. > + */ > + > +#ifndef _VPL011_H_ > + > +#define _VPL011_H_ > + > +/* > + * register offsets > + */ > +#define VPL011_UARTDR_OFFSET 0x0 // data register (RW) Wrong type of comments. Pls use /* */ variant > +#define VPL011_UARTRSR_OFFSET 0x4 // receive status and error clear register (RW) > +#define VPL011_UARTFR_OFFSET 0x18 // flag register (RO) > +#define VPL011_UARTRIS_OFFSET 0x3c // raw interrupt status register (RO) > +#define VPL011_UARTMIS_OFFSET 0x40 // masked interrupt status register (RO) > +#define VPL011_UARTIMSC_OFFSET 0x38 // interrupt mask set/clear register (RW) > +#define VPL011_UARTICR_OFFSET 0x44 // interrupt clear register (WO) > +#define VPL011_UARTCR_OFFSET 0x30 // uart control register > +#define VPL011_UARTDMACR_OFFSET 0x48 // uart dma control register > + > +/* > + * control register bits - RW > + */ > +#define VPL011_UARTCR_UARTEN_BIT 0 > +#define VPL011_UARTCR_UARTEN (1<<VPL011_UARTCR_UARTEN_BIT) > +#define VPL011_UARTCR_TXE_BIT 8 > +#define VPL011_UARTCR_TXE (1<<VPL011_UARTCR_TXE_BIT) > +#define VPL011_UARTCR_RXE_BIT 9 > +#define VPL011_UARTCR_RXE (1<<VPL011_UARTCR_RXE_BIT) > + > +/* > + * Flag register bits - RO > + */ > +#define VPL011_UARTFR_CTS_BIT 0 // clear to send > +#define VPL011_UARTFR_CTS (1<<VPL011_UARTFR_CTS_BIT) > +#define VPL011_UARTFR_DSR_BIT 1 // data set ready > +#define VPL011_UARTFR_DSR (1<<VPL011_UARTFR_DSR_BIT) > +#define VPL011_UARTFR_DCD_BIT 2 // data carrier detect > +#define VPL011_UARTFR_DCD (1<<VPL011_UARTFR_DCD_BIT) > +#define VPL011_UARTFR_BUSY_BIT 3 // uart busy > +#define VPL011_UARTFR_BUSY (1<<VPL011_UARTFR_BUSY_BIT) > +#define VPL011_UARTFR_RXFE_BIT 4 // receive fifo empty > +#define VPL011_UARTFR_RXFE (1<<VPL011_UARTFR_RXFE_BIT) > +#define VPL011_UARTFR_TXFF_BIT 5 // transmit fifo full > +#define VPL011_UARTFR_TXFF (1<<VPL011_UARTFR_TXFF_BIT) > +#define VPL011_UARTFR_RXFF_BIT 6 // receive fifo full > +#define VPL011_UARTFR_RXFF (1<<VPL011_UARTFR_RXFF_BIT) > +#define VPL011_UARTFR_TXFE_BIT 7 // transmit fifo empty > +#define VPL011_UARTFR_TXFE (1<<VPL011_UARTFR_TXFE_BIT) > +#define VPL011_UARTFR_RI_BIT 8 // ring indicator > +#define VPL011_UARTFR_RI (1<<VPL011_UARTFR_RI_BIT) > + > +/* > + * UART raw interrupt status bits - RO > + */ > +#define VPL011_UARTRIS_RIRMIS_BIT 0 > +#define VPL011_UARTRIS_RIRMIS (1<<VPL011_UARTRIS_RIRMIS_BIT) > +#define VPL011_UARTRIS_CTSRMIS_BIT 1 > +#define VPL011_UARTRIS_CTSRMIS (1<<VPL011_UARTRIS_CTSRMIS_BIT) > +#define VPL011_UARTRIS_DCDRMIS_BIT 2 > +#define VPL011_UARTRIS_DCDRMIS (1<<VPL011_UARTRIS_DCDRMIS_BIT) > +#define VPL011_UARTRIS_DSRRMIS_BIT 3 > +#define VPL011_UARTRIS_DSRRMIS (1<<VPL011_UARTRIS_DSRRMIS_BIT) > +#define VPL011_UARTRIS_RXRIS_BIT 4 > +#define VPL011_UARTRIS_RXRIS (1<<VPL011_UARTRIS_RXRIS_BIT) > +#define VPL011_UARTRIS_TXRIS_BIT 5 > +#define VPL011_UARTRIS_TXRIS (1<<VPL011_UARTRIS_TXRIS_BIT) > +#define VPL011_UARTRIS_RTRIS_BIT 6 > +#define VPL011_UARTRIS_RTRIS (1<<VPL011_UARTRIS_RTRIS_BIT) > +#define VPL011_UARTRIS_FERIS_BIT 7 > +#define VPL011_UARTRIS_FERIS (1<<VPL011_UARTRIS_FERIS_BIT) > +#define VPL011_UARTRIS_PERIS_BIT 8 > +#define VPL011_UARTRIS_PERIS (1<<VPL011_UARTRIS_PERIS_BIT) > +#define VPL011_UARTRIS_BERIS_BIT 9 > +#define VPL011_UARTRIS_BERIS (1<<VPL011_UARTRIS_BERIS_BIT) > +#define VPL011_UARTRIS_OERIS_BIT 10 > +#define VPL011_UARTRIS_OERIS (1<<VPL011_UARTRIS_OERIS_BIT) > + > +/* > + * UART masked interrupt status bits - RO > + */ > +#define VPL011_UARTMIS_RIMMIS_BIT 0 > +#define VPL011_UARTMIS_RIMMIS (1<<VPL011_UARTMIS_RIMMIS_BIT) > +#define VPL011_UARTMIS_CTSMMIS_BIT 1 > +#define VPL011_UARTMIS_CTSMMIS (1<<VPL011_UARTMIS_CTSMMIS_BIT) > +#define VPL011_UARTMIS_DCDMMIS_BIT 2 > +#define VPL011_UARTMIS_DCDMMIS (1<<VPL011_UARTMIS_DCDMMIS_BIT) > +#define VPL011_UARTMIS_DSRMMIS_BIT 3 > +#define VPL011_UARTMIS_DSRMMIS (1<<VPL011_UARTMIS_DSRMMIS_BIT) > +#define VPL011_UARTMIS_RXMIS_BIT 4 > +#define VPL011_UARTMIS_RXMIS (1<<VPL011_UARTMIS_RXMIS_BIT) > +#define VPL011_UARTMIS_TXMIS_BIT 5 > +#define VPL011_UARTMIS_TXMIS (1<<VPL011_UARTMIS_TXMIS_BIT) > +#define VPL011_UARTMIS_RTMIS_BIT 6 > +#define VPL011_UARTMIS_RTMIS (1<<VPL011_UARTMIS_RTMIS_BIT) > +#define VPL011_UARTMIS_FEMIS_BIT 7 > +#define VPL011_UARTMIS_FEMIS (1<<VPL011_UARTMIS_FEMIS_BIT) > +#define VPL011_UARTMIS_PEMIS_BIT 8 > +#define VPL011_UARTMIS_PEMIS (1<<VPL011_UARTMIS_PEMIS_BIT) > +#define VPL011_UARTMIS_BEMIS_BIT 9 > +#define VPL011_UARTMIS_BEMIS (1<<VPL011_UARTMIS_BEMIS_BIT) > +#define VPL011_UARTMIS_OEMIS_BIT 10 > +#define VPL011_UARTMIS_OEMIS (1<<VPL011_UARTMIS_OEMIS_BIT) > + > +/* > + * UART interrupt clear bits - WO > + */ > +#define VPL011_UARTICR_RIMIC_BIT 0 > +#define VPL011_UARTICR_RIMIC (1<<VPL011_UARTICR_RIMIC_BIT) > +#define VPL011_UARTICR_CTSMIC_BIT 1 > +#define VPL011_UARTICR_CTSMIC (1<<VPL011_UARTICR_CTSMIC_BIT) > +#define VPL011_UARTICR_DCDMIC_BIT 2 > +#define VPL011_UARTICR_DCDMIC (1<<VPL011_UARTICR_DCDMIC_BIT) > +#define VPL011_UARTICR_DSRMIC_BIT 3 > +#define VPL011_UARTICR_DSRMIC (1<<VPL011_UARTICR_DSRMIC_BIT) > +#define VPL011_UARTICR_RXIC_BIT 4 > +#define VPL011_UARTICR_RXIC (1<<VPL011_UARTICR_RXIC_BIT) > +#define VPL011_UARTICR_TXIC_BIT 5 > +#define VPL011_UARTICR_TXIC (1<<VPL011_UARTICR_TXIC_BIT) > +#define VPL011_UARTICR_RTIC_BIT 6 > +#define VPL011_UARTICR_RTIC (1<<VPL011_UARTICR_RTIC_BIT) > +#define VPL011_UARTICR_FEIC_BIT 7 > +#define VPL011_UARTICR_FEIC (1<<VPL011_UARTICR_FEIC_BIT) > +#define VPL011_UARTICR_PEIC_BIT 8 > +#define VPL011_UARTICR_PEIC (1<<VPL011_UARTICR_PEIC_BIT) > +#define VPL011_UARTICR_BEIC_BIT 9 > +#define VPL011_UARTICR_BEIC (1<<VPL011_UARTICR_BEIC_BIT) > +#define VPL011_UARTICR_OEIC_BIT 10 > +#define VPL011_UARTICR_OEIC (1<<VPL011_UARTICR_OEIC_BIT) > + > +/* > + * UART interrupt mask set/clear bits - RW > + */ > +#define VPL011_UARTIMSC_RIMIM_BIT 0 > +#define VPL011_UARTIMSC_RIMIM (1<<VPL011_UARTIMSC_RIMIM_BIT) > +#define VPL011_UARTIMSC_CTSMIM_BIT 1 > +#define VPL011_UARTIMSC_CTSMIM (1<<VPL011_UARTIMSC_CTSMIM_BIT) > +#define VPL011_UARTIMSC_DCDMIM_BIT 2 > +#define VPL011_UARTIMSC_DCDMIM (1<<VPL011_UARTIMSC_DCDMIM_BIT) > +#define VPL011_UARTIMSC_DSRMIM_BIT 3 > +#define VPL011_UARTIMSC_DSRMIM (1<<VPL011_UARTIMSC_DSRMIM_BIT) > +#define VPL011_UARTIMSC_RXIM_BIT 4 > +#define VPL011_UARTIMSC_RXIM (1<<VPL011_UARTIMSC_RXIM_BIT) > +#define VPL011_UARTIMSC_TXIM_BIT 5 > +#define VPL011_UARTIMSC_TXIM (1<<VPL011_UARTIMSC_TXIM_BIT) > +#define VPL011_UARTIMSC_RTIM_BIT 6 > +#define VPL011_UARTIMSC_RTIM (1<<VPL011_UARTIMSC_RTIM_BIT) > +#define VPL011_UARTIMSC_FEIM_BIT 7 > +#define VPL011_UARTIMSC_FEIM (1<<VPL011_UARTIMSC_FEIM_BIT) > +#define VPL011_UARTIMSC_PEIM_BIT 8 > +#define VPL011_UARTIMSC_PEIM (1<<VPL011_UARTIMSC_PEIM_BIT) > +#define VPL011_UARTIMSC_BEIM_BIT 9 > +#define VPL011_UARTIMSC_BEIM (1<<VPL011_UARTIMSC_BEIM_BIT) > +#define VPL011_UARTIMSC_OEIM_BIT 10 > +#define VPL011_UARTIMSC_OEIM (1<<VPL011_UARTIMSC_OEIM_BIT) > + > + > +/* > + * helper macros > + */ > +#define VPL011_RING_DEPTH(intf,dir) (((intf)->dir ## _prod - (intf)->dir ## _cons)) > +#define VPL011_RING_MAX_DEPTH(intf,dir) (sizeof((intf)->dir)-1) > + > +#define VPL011_IN_RING_EMPTY(intf) (VPL011_RING_DEPTH(intf, in) == 0) > + > +#define VPL011_OUT_RING_EMPTY(intf) (VPL011_RING_DEPTH(intf, out) == 0) > + > +#define VPL011_IN_RING_FULL(intf) (VPL011_RING_DEPTH(intf, in) == VPL011_RING_MAX_DEPTH(intf, in)) > + > +#define VPL011_OUT_RING_FULL(intf) (VPL011_RING_DEPTH(intf, out) == VPL011_RING_MAX_DEPTH(intf,out)) > + > +#define VPL011_LOCK(d,flags) spin_lock_irqsave(&(d)->arch.vpl011.lock, flags) > +#define VPL011_UNLOCK(d,flags) spin_unlock_irqrestore(&(d)->arch.vpl011.lock, flags) > + > +/* > + * MMIO return values > + */ > +#define VPL011_EMUL_OK 1 > +#define VPL011_EMUL_FAIL 0 > + > +int domain_vpl011_init(struct domain *d); > +int vpl011_map_guest_page(struct domain *d); > +int vpl011_read_data(struct domain *d, unsigned char *data); > +int vpl011_write_data(struct domain *d, unsigned char data); > + > +#define MASK_VPL011CONS_IDX(idx, ring) ((idx) & (sizeof(ring)-1)) > +struct console_interface { > + char in[1024]; > + char out[2048]; > + uint32_t in_cons, in_prod; > + uint32_t out_cons, out_prod; > +}; > +#endif > diff --git a/xen/common/Kconfig b/xen/common/Kconfig > index f2ecbc4..7e2feac 100644 > --- a/xen/common/Kconfig > +++ b/xen/common/Kconfig > @@ -237,4 +237,10 @@ config FAST_SYMBOL_LOOKUP > The only user of this is Live patching. > > If unsure, say Y. > + > +config VPL011_CONSOLE > + bool "Emulated pl011 console support" > + default y I thought by default it would be 'n'? Or perhaps defauly y if CONFIG_ARM or such? > + ---help--- > + Allows a guest to use pl011 UART as a console > endmenu > diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h > index 2d6fbb1..ff2403a 100644 > --- a/xen/include/asm-arm/domain.h > +++ b/xen/include/asm-arm/domain.h > @@ -40,6 +40,7 @@ struct vtimer { > uint64_t cval; > }; > > + ?? > struct arch_domain > { > #ifdef CONFIG_ARM_64 > @@ -131,6 +132,20 @@ struct arch_domain > struct { > uint8_t privileged_call_enabled : 1; > } monitor; > + > +#ifdef CONFIG_VPL011_CONSOLE > + struct vpl011 { > + void *ring_buf; > + struct page_info *ring_page; > + uint32_t flag; /* flag register */ > + uint32_t control; /* control register */ > + uint32_t intr_mask; /* interrupt mask register*/ > + uint32_t intr_clear; /* interrupt clear register */ > + uint32_t raw_intr_status; /* raw interrupt status register */ > + uint32_t masked_intr_status; /* masked interrupt register */ > + spinlock_t lock; > + } vpl011; > +#endif > } __cacheline_aligned; > > struct arch_vcpu > diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h > index bd974fb..1d4548f 100644 > --- a/xen/include/public/arch-arm.h > +++ b/xen/include/public/arch-arm.h > @@ -410,6 +410,10 @@ typedef uint64_t xen_callback_t; > #define GUEST_ACPI_BASE 0x20000000ULL > #define GUEST_ACPI_SIZE 0x02000000ULL > > +/* PL011 mappings */ > +#define GUEST_PL011_BASE 0x22000000ULL > +#define GUEST_PL011_SIZE 0x00001000ULL > + > /* > * 16MB == 4096 pages reserved for guest to use as a region to map its > * grant table in. > @@ -420,6 +424,7 @@ typedef uint64_t xen_callback_t; > #define GUEST_MAGIC_BASE xen_mk_ullong(0x39000000) > #define GUEST_MAGIC_SIZE xen_mk_ullong(0x01000000) > > + ??? > #define GUEST_RAM_BANKS 2 > > #define GUEST_RAM0_BASE xen_mk_ullong(0x40000000) /* 3GB of low RAM @ 1GB */ > -- > 2.7.4 > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > https://lists.xen.org/xen-devel
Hi Konrad, On 03/03/2017 07:59 PM, Konrad Rzeszutek Wilk wrote: >> +} >> + >> +static int vpl011_data_avail(struct domain *d) >> +{ >> + int rc=0; >> + unsigned long flags; >> + >> + struct console_interface *intf=(struct console_interface *)d->arch.vpl011.ring_buf; > > Can you have an macro for this? >> + >> + VPL011_LOCK(d, flags); > > Please don't. Just use normal spin_lock invocation. This is really a matter of taste. We've been using macro on the vgic emulation for the lock and I find it clearer and less error-prone. At least you are sure that all critical paths protected by the local will have IRQ disable. Cheers,
Hi, On 03/03/2017 07:59 PM, Konrad Rzeszutek Wilk wrote: >> diff --git a/xen/common/Kconfig b/xen/common/Kconfig >> index f2ecbc4..7e2feac 100644 >> --- a/xen/common/Kconfig >> +++ b/xen/common/Kconfig >> @@ -237,4 +237,10 @@ config FAST_SYMBOL_LOOKUP >> The only user of this is Live patching. >> >> If unsure, say Y. >> + >> +config VPL011_CONSOLE >> + bool "Emulated pl011 console support" >> + default y > > I thought by default it would be 'n'? > > Or perhaps defauly y if CONFIG_ARM or such? I forgot to answer to this. The new config should go in arch/arm/Kconfig because the emulation is living there. Cheers,
Hi Konrad, On 03/03/2017 07:59 PM, Konrad Rzeszutek Wilk wrote: >> + rc = alloc_unbound_xen_event_channel(d, 0, current->domain->domain_id, >> + vpl011_notification); >> + if (rc < 0) > > Spaces. >> + { >> + printk ("Failed to allocate vpl011 event channel\n"); > > gdprintk gdprintk will print the wrong domain as we are allocating a new domain. In general g*printk should be limited to the code we are sure the domain in parameter is the current one. Cheers,
Hi Bhupinder, On 21/02/17 11:25, Bhupinder Thakur wrote: > diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c > new file mode 100644 > index 0000000..88ba968 > --- /dev/null > +++ b/xen/arch/arm/vpl011.c [...] > +static int vpl011_mmio_write(struct vcpu *v, mmio_info_t *info, register_t r, void *priv) See my comment on the vpl011_mmio_read about the structure of the function. > +{ > + unsigned char ch = r; > + > + switch (info->gpa - GUEST_PL011_BASE) > + { > + case VPL011_UARTCR_OFFSET: This register is not required in the SBSA UART. > + v->domain->arch.vpl011.control = r; > + break; > + case VPL011_UARTDR_OFFSET: > + vpl011_write_data(v->domain, ch); The implicit casting of register_t to ch is a bit confusing. Also I would prefer to use uint8_t as it reflects the size of the field. Lastly, what if vpl011_write_data is returning an error? > + break; > + case VPL011_UARTIMSC_OFFSET: > + v->domain->arch.vpl011.intr_mask = r; You need to sanitize the value written and make sure reserved field and Write As Ones register and handle correctly (see the spec). > + if ( (v->domain->arch.vpl011.raw_intr_status & v->domain->arch.vpl011.intr_mask) ) This line and the line below are over 80 columns. Also the code in general would benefits if you define a local variable to point to v->domain->arch.vpl011. > + vgic_vcpu_inject_spi(v->domain, (int)v->domain->arch.hvm_domain.params[HVM_PARAM_VPL011_VIRQ]); I don't think we need an HVM_PARAM for the pl011 SPI. We cannot hardcode this in the guest layout (see include/public/arch-arm.h). > + break; > + case VPL011_UARTICR_OFFSET: > + /* > + * clear all bits which are set in the input > + */ > + v->domain->arch.vpl011.raw_intr_status &= ~r; > + if ( (v->domain->arch.vpl011.raw_intr_status & v->domain->arch.vpl011.intr_mask) ) > + { { } are not necessary. > + vgic_vcpu_inject_spi(v->domain, (int)v->domain->arch.hvm_domain.params[HVM_PARAM_VPL011_VIRQ]); > + } The if is exactly the same as the on in IMSC. This is the call of an helper to check the interrupt state and inject one if necessary. > + break; > + case VPL011_UARTRSR_OFFSET: // nothing to clear The coding style for single line comment in Xen is /* ... */ Also, I think we may want to handle Overrun error as the ring may be full. > + break; > + case VPL011_UARTFR_OFFSET: // these are all RO registers > + case VPL011_UARTRIS_OFFSET: > + case VPL011_UARTMIS_OFFSET: > + case VPL011_UARTDMACR_OFFSET: Please have a look at the vGICv2/vGICv3 emulation see how we implemented write ignore register. > + break; > + default: > + printk ("vpl011_mmio_write: switch case not handled %d\n", (int)(info->gpa - GUEST_PL011_BASE)); See my comment about the prinkt in the read emulation. > + break; > + } > + > + return VPL011_EMUL_OK; > +} > + > +static const struct mmio_handler_ops vpl011_mmio_handler = { > + .read = vpl011_mmio_read, > + .write = vpl011_mmio_write, > +}; > + > + > + Only newline is sufficient. This was mention by Konrad and I will try to avoid repeating his comments. > +int vpl011_map_guest_page(struct domain *d) > +{ > + int rc=0; > + > + /* > + * map the guest PFN to Xen address space > + */ > + rc = prepare_ring_for_helper(d, > + d->arch.hvm_domain.params[HVM_PARAM_VPL011_CONSOLE_PFN], > + &d->arch.vpl011.ring_page, > + (void **)&d->arch.vpl011.ring_buf); Why do you need the cast? Also I cannot find any code in this series which destroy the ring. Please have a helper called vpl011_unmap_guest_page to do this job and call when the domain is destroyed. > + if ( rc < 0 ) > + { > + printk("Failed to map vpl011 guest PFN\n"); > + } > + > + return rc; > +} > + > +static int vpl011_data_avail(struct domain *d) > +{ > + int rc=0; > + unsigned long flags; > + > + struct console_interface *intf=(struct console_interface *)d->arch.vpl011.ring_buf; > + > + VPL011_LOCK(d, flags); > + > + /*` > + * check IN ring buffer > + */ > + if ( !VPL011_IN_RING_EMPTY(intf) ) > + { > + /* > + * clear the RX FIFO empty flag as the ring is not empty > + */ > + d->arch.vpl011.flag &= ~(VPL011_UARTFR_RXFE); > + > + /* > + * if the buffer is full then set the RX FIFO FULL flag > + */ > + if ( VPL011_IN_RING_FULL(intf) ) > + d->arch.vpl011.flag |= (VPL011_UARTFR_RXFF); > + > + /* > + * set the RX interrupt status > + */ > + d->arch.vpl011.raw_intr_status |= (VPL011_UARTRIS_RXRIS); > + } > + > + /* > + * check OUT ring buffer > + */ > + if ( !VPL011_OUT_RING_FULL(intf) ) > + { > + /* > + * if the buffer is not full then clear the TX FIFO full flag > + */ > + d->arch.vpl011.flag &= ~(VPL011_UARTFR_TXFF); > + > + /* > + * set the TX interrupt status > + */ > + d->arch.vpl011.raw_intr_status |= (VPL011_UARTRIS_TXRIS); > + > + if ( VPL011_OUT_RING_EMPTY(intf) ) > + { > + /* > + * clear the uart busy flag and set the TX FIFO empty flag > + */ > + d->arch.vpl011.flag &= ~(VPL011_UARTFR_BUSY); > + d->arch.vpl011.flag |= (VPL011_UARTFR_TXFE); > + } > + } > + > + VPL011_UNLOCK(d, flags); > + > + /* > + * send an interrupt if it is not masked > + */ > + if ( (d->arch.vpl011.raw_intr_status & d->arch.vpl011.intr_mask) ) > + vgic_vcpu_inject_spi(d, (int)d->arch.hvm_domain.params[HVM_PARAM_VPL011_VIRQ]); See my comment above about having an helper for this code. > + > + if ( !VPL011_OUT_RING_EMPTY(intf) ) > + { > + /* > + * raise an interrupt to dom0 The backend console does not necessarily live in DOM0. Anyway, Konrad requested to drop the comment and I am happy with that. > + */ > + rc = raw_evtchn_send(d, > + d->arch.hvm_domain.params[HVM_PARAM_VPL011_CONSOLE_EVTCHN], NULL); You are using a function that is been defined in a later patch (i.w #3). All the patch should be able to compile without applying upcoming patch to allow bisection. Ideally, Xen should still be functional for every patch. But it is a best effort. > + > + if ( rc < 0 ) > + printk("Failed to send vpl011 interrupt to dom0\n"); > + } > + > + return rc; > +} > + > +int vpl011_read_data(struct domain *d, unsigned char *data) This function does not seem to be used outside of this file. So why do you export it? > +{ > + int rc=0; > + unsigned long flags; > + struct console_interface *intf=(struct console_interface *)d->arch.vpl011.ring_buf; > + > + *data = 0; > + > + VPL011_LOCK(d, flags); > + > + /* > + * if there is data in the ring buffer then copy it to the output buffer > + */ > + if ( !VPL011_IN_RING_EMPTY(intf) ) > + { > + *data = intf->in[MASK_VPL011CONS_IDX(intf->in_cons++, intf->in)]; > + } > + > + /* > + * if the ring buffer is empty then set the RX FIFO empty flag > + */ > + if ( VPL011_IN_RING_EMPTY(intf) ) > + { > + d->arch.vpl011.flag |= (VPL011_UARTFR_RXFE); > + d->arch.vpl011.raw_intr_status &= ~(VPL011_UARTRIS_RXRIS); > + } > + > + /* > + * clear the RX FIFO full flag > + */ > + d->arch.vpl011.flag &= ~(VPL011_UARTFR_RXFF); > + > + VPL011_UNLOCK(d, flags); > + > + return rc; > +} > + > +int vpl011_write_data(struct domain *d, unsigned char data) Same has for vpl011_read_data. > +{ > + int rc=0; > + unsigned long flags; > + struct console_interface *intf=(struct console_interface *)d->arch.vpl011.ring_buf; > + > + VPL011_LOCK(d, flags); > + > + /* > + * if there is space in the ring buffer then write the data > + */ > + if ( !VPL011_OUT_RING_FULL(intf) ) > + { > + intf->out[MASK_VPL011CONS_IDX(intf->out_prod++, intf->out)] = data; > + smp_wmb(); > + } > + > + /* > + * if there is no space in the ring buffer then set the > + * TX FIFO FULL flag > + */ > + if ( VPL011_OUT_RING_FULL(intf) ) > + { > + d->arch.vpl011.flag |= (VPL011_UARTFR_TXFF); > + d->arch.vpl011.raw_intr_status &= ~(VPL011_UARTRIS_TXRIS); > + } > + > + /* > + * set the uart busy status > + */ > + d->arch.vpl011.flag |= (VPL011_UARTFR_BUSY); > + > + /* > + * clear the TX FIFO empty flag > + */ > + d->arch.vpl011.flag &= ~(VPL011_UARTFR_TXFE); > + > + VPL011_UNLOCK(d, flags); > + > + /* > + * raise an event to dom0 only if it is the first character in the buffer > + */ > + if ( VPL011_RING_DEPTH(intf, out) == 1 ) > + { > + rc = raw_evtchn_send(d, > + d->arch.hvm_domain.params[HVM_PARAM_VPL011_CONSOLE_EVTCHN], NULL); > + > + if ( rc < 0 ) > + printk("Failed to send vpl011 interrupt to dom0\n"); > + } > + > + return rc; > +} > + > +static void vpl011_notification(struct vcpu *v, unsigned int port) > +{ > + vpl011_data_avail(v->domain); vpl011_data_avail is returning an error but you don't check. What is the point of it then? > +} > + > +int domain_vpl011_init(struct domain *d) I was expected a destroy function to clean-up the vpl011 emulation. > +{ > + int rc=0; > + > + /* > + * register xen event channel > + */ > + rc = alloc_unbound_xen_event_channel(d, 0, current->domain->domain_id, > + vpl011_notification); This line looks wrong to me. The console backend may not live in the same domain as the toolstack. > + if (rc < 0) > + { > + printk ("Failed to allocate vpl011 event channel\n"); > + return rc; > + } > + d->arch.hvm_domain.params[HVM_PARAM_VPL011_CONSOLE_EVTCHN] = rc; > + > + /* > + * allocate an SPI VIRQ for the guest > + */ > + d->arch.hvm_domain.params[HVM_PARAM_VPL011_VIRQ] = vgic_allocate_spi(d); It makes little sense to hardcode the MMIO region and not the SPI. Also, I would rather avoid to introduce new HVM_PARAM when not necessary. So hardcoding the value looks more sensible to me. > + > + /* > + * register mmio handler > + */ > + register_mmio_handler (d, &vpl011_mmio_handler, GUEST_PL011_BASE, GUEST_PL011_SIZE, NULL); Coding style. No space between the function name and (. > + > + /* > + * initialize the lock > + */ > + spin_lock_init(&d->arch.vpl011.lock); > + > + /* > + * clear the flag, control and interrupt status registers > + */ > + d->arch.vpl011.control = 0; > + d->arch.vpl011.flag = 0; > + d->arch.vpl011.intr_mask = 0; > + d->arch.vpl011.intr_clear = 0; > + d->arch.vpl011.raw_intr_status = 0; > + d->arch.vpl011.masked_intr_status = 0; The domain structure is already zeroed. So no need to 0 it again. > + > + return 0; > +} Missing e-macs magic here. > diff --git a/xen/arch/arm/vpl011.h b/xen/arch/arm/vpl011.h > new file mode 100644 > index 0000000..f2c577f > --- /dev/null > +++ b/xen/arch/arm/vpl011.h Header should live in include. [...] > +#ifndef _VPL011_H_ > + > +#define _VPL011_H_ > + > +/* > + * register offsets > + */ We already define the PL011 register in include/asm-arm/pl011-uart.h. Please re-use them rather than re-inventing the wheel. > +#define VPL011_UARTDR_OFFSET 0x0 // data register (RW) > +#define VPL011_UARTRSR_OFFSET 0x4 // receive status and error clear register (RW) > +#define VPL011_UARTFR_OFFSET 0x18 // flag register (RO) > +#define VPL011_UARTRIS_OFFSET 0x3c // raw interrupt status register (RO) > +#define VPL011_UARTMIS_OFFSET 0x40 // masked interrupt status register (RO) > +#define VPL011_UARTIMSC_OFFSET 0x38 // interrupt mask set/clear register (RW) > +#define VPL011_UARTICR_OFFSET 0x44 // interrupt clear register (WO) > +#define VPL011_UARTCR_OFFSET 0x30 // uart control register > +#define VPL011_UARTDMACR_OFFSET 0x48 // uart dma control register [...] > +#define MASK_VPL011CONS_IDX(idx, ring) ((idx) & (sizeof(ring)-1)) > +struct console_interface { > + char in[1024]; > + char out[2048]; > + uint32_t in_cons, in_prod; > + uint32_t out_cons, out_prod; > +}; > +#endif The communication between Xen and the console backend is based on the PV console ring. It would be easier to re-use it rather than recreating one. > diff --git a/xen/common/Kconfig b/xen/common/Kconfig > index f2ecbc4..7e2feac 100644 > --- a/xen/common/Kconfig > +++ b/xen/common/Kconfig > @@ -237,4 +237,10 @@ config FAST_SYMBOL_LOOKUP > The only user of this is Live patching. > > If unsure, say Y. > + > +config VPL011_CONSOLE > + bool "Emulated pl011 console support" > + default y > + ---help--- > + Allows a guest to use pl011 UART as a console > endmenu This should go in arch/arm/Kconfig > diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h > index 2d6fbb1..ff2403a 100644 > --- a/xen/include/asm-arm/domain.h > +++ b/xen/include/asm-arm/domain.h > @@ -40,6 +40,7 @@ struct vtimer { > uint64_t cval; > }; > > + Spurious line. > struct arch_domain > { > #ifdef CONFIG_ARM_64 > @@ -131,6 +132,20 @@ struct arch_domain > struct { > uint8_t privileged_call_enabled : 1; > } monitor; > + > +#ifdef CONFIG_VPL011_CONSOLE > + struct vpl011 { > + void *ring_buf; > + struct page_info *ring_page; > + uint32_t flag; /* flag register */ > + uint32_t control; /* control register */ > + uint32_t intr_mask; /* interrupt mask register*/ > + uint32_t intr_clear; /* interrupt clear register */ > + uint32_t raw_intr_status; /* raw interrupt status register */ > + uint32_t masked_intr_status; /* masked interrupt register */ > + spinlock_t lock; > + } vpl011; > +#endif I think the structure should be defined in vpl011.h. > } __cacheline_aligned; > > struct arch_vcpu > diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h > index bd974fb..1d4548f 100644 > --- a/xen/include/public/arch-arm.h > +++ b/xen/include/public/arch-arm.h > @@ -410,6 +410,10 @@ typedef uint64_t xen_callback_t; > #define GUEST_ACPI_BASE 0x20000000ULL > #define GUEST_ACPI_SIZE 0x02000000ULL > > +/* PL011 mappings */ > +#define GUEST_PL011_BASE 0x22000000ULL > +#define GUEST_PL011_SIZE 0x00001000ULL > + > /* > * 16MB == 4096 pages reserved for guest to use as a region to map its > * grant table in. > @@ -420,6 +424,7 @@ typedef uint64_t xen_callback_t; > #define GUEST_MAGIC_BASE xen_mk_ullong(0x39000000) > #define GUEST_MAGIC_SIZE xen_mk_ullong(0x01000000) > > + Spurious line. > #define GUEST_RAM_BANKS 2 > > #define GUEST_RAM0_BASE xen_mk_ullong(0x40000000) /* 3GB of low RAM @ 1GB */ > Cheers,
On Sun, Mar 05, 2017 at 01:04:54AM +0000, Julien Grall wrote: > Hi Konrad, > > On 03/03/2017 07:59 PM, Konrad Rzeszutek Wilk wrote: > > > +} > > > + > > > +static int vpl011_data_avail(struct domain *d) > > > +{ > > > + int rc=0; > > > + unsigned long flags; > > > + > > > + struct console_interface *intf=(struct console_interface *)d->arch.vpl011.ring_buf; > > > > Can you have an macro for this? > > > + > > > + VPL011_LOCK(d, flags); > > > > Please don't. Just use normal spin_lock invocation. > > This is really a matter of taste. We've been using macro on the vgic > emulation for the lock and I find it clearer and less error-prone. At least > you are sure that all critical paths protected by the local will have IRQ > disable. OK. I am more of the other way, but then I can see the beaty of having an macro that requires two parameters so there is no way to mess this up. Bhupinder, please ignore my request.
Hi Julien, On 26 February 2017 at 22:37, Julien Grall <julien.grall@arm.com> wrote: >> + >> +#include <xen/config.h> > > > xen/config.h is included by default. Please drop it. Removed. > >> +#include <xen/init.h> >> +#include <xen/lib.h> >> +#include <xen/errno.h> >> +#include <xen/guest_access.h> >> +#include <xen/sched.h> >> +#include <xen/monitor.h> > > > Why do you need to include monitor.h? > >> +#include <xen/event.h> >> +#include <xen/vmap.h> >> + >> +#include <xsm/xsm.h> > > > Ditto. > >> + >> +#include <public/xen.h> >> +#include <public/hvm/params.h> >> +#include <public/hvm/hvm_op.h> >> + >> +#include <asm/hypercall.h> > > > Ditto. > I have removed the unncessary includes. I probably added them when I initially created the file. >> +#include "vpl011.h" >> + >> +static int vpl011_mmio_read(struct vcpu *v, mmio_info_t *info, register_t >> *r, void *priv) >> +{ >> + unsigned char ch; >> + >> + switch (info->gpa - GUEST_PL011_BASE) > > > Coding style: > > switch ( ... ) > >> + { >> + case VPL011_UARTCR_OFFSET: > > > Coding style: the case should be aligned to {. E.g > > { > case ... > Switch/case style corrected. > Also, I would prefer if you don't include _OFFSET in all the name. > Removed the _OFFSET. > Furthermore, can you please order the case by offset in the MMIO region. > This would help to find if a register has been emulated or not. > Ordered the offsets in increasing order. > Lastly, the user may have requested to read 8-bit, 16-bit, 32-bit. But you > always return a 32-bit. Is a guest allowed to access using any size and in > the middle of a register? Give a look on what is done for vgic-v{2,3}.c to > check the size and read register. You may want to pull some code out to > re-use here. > >> + *r = v->domain->arch.vpl011.control; > > > Similarly, I would prefer if the name of the field match the register name. > Renamed the vpl011 fields match the corresponding register names. >> + break; >> + case VPL011_UARTDR_OFFSET: >> + vpl011_read_data(v->domain, &ch); > > > Should not you check the return value of vpl011_read_data? Also, what if > there is no data? This condition should not happen because the RX FIFO empty bit would be set in the UARTFR register when the last data is read from the ring buffer and the the guest is not supposed to issue next read until the RX FIFO empty bit is cleared indicating there is more data now. > >> + *r = ch; >> + break; >> + case VPL011_UARTFR_OFFSET: >> + *r = v->domain->arch.vpl011.flag; > > > I am fairly surprised that none of this code is actually protected by lock. > For instance the update of flag is not atomic. So is it safe? For reading, I thought no locking was required, as I believe a 32-bit value read/write on ARM should be atomic. So if the value is modified in some other context while it is being read in another context, the reader should see either the old value or the new value. For register writes, yes I need to take a lock where I am updating certain bits in the register. I will add the locking there. > >> + break; >> + case VPL011_UARTIMSC_OFFSET: >> + *r = v->domain->arch.vpl011.intr_mask; >> + break; >> + case VPL011_UARTICR_OFFSET: >> + *r = 0; > > > Looking at the spec, this register is write-only. So why do you implement > RAZ? In such cases, where the guest tries to write to RO register or tries to read a WO register, should I send a abort to the guest? > >> + break; >> + case VPL011_UARTRIS_OFFSET: >> + *r = v->domain->arch.vpl011.raw_intr_status; >> + break; >> + case VPL011_UARTMIS_OFFSET: >> + *r = v->domain->arch.vpl011.raw_intr_status & >> + v->domain->arch.vpl011.intr_mask; >> + break; >> + case VPL011_UARTDMACR_OFFSET: >> + *r = 0; /* uart DMA is not supported. Here it always returns >> 0 */ > > > My understanding of the spec is DMA is not optional. So what would happen if > the guest tries to enable it? > >> + break; >> + case VPL011_UARTRSR_OFFSET: >> + *r = 0; /* it always returns 0 as there are no physical >> errors */ > > > This register contains contains the bit OE that tells whether the FIFO is > full or not. The FIFO here is the PV ring, so maybe we should set this bit > if the ring is full. The OE condition will not happen in this case since xenconsole will not write more data to the guest if the ring buffer is full. There is a separate UARTFR status bit which indicates whether the ring buffer is full. > >> + break; >> + default: >> + printk ("vpl011_mmio_read: invalid switch case %d\n", >> (int)(info->gpa - GUEST_PL011_BASE)); > > > Coding style: printk(...). > > Also, printk is not ratelimited by default. Please use gprintk(...) which > will be ratelimited and print the domain information. This is useful when > you have multiple guest. > Replaced printk with gprintk. >> + break; >> + } >> + >> + return VPL011_EMUL_OK; > > > Please use plain value as the return is not pl011 specific. Also, I am a bit > surprised that you return "ok" even when a register as not been emulated. > IHMO, a data abort should be sent to the guest. Corrected this. Now it returns an error incase the register is not emulated . For the return values, I see that typically values 1/0 are returned like in vgic-v2/3.c. Are there some common macros which I can use? > > Furthermore, looking at the overall function, I think it should be better if > you re-use the template used by the other emulation (see vgic-v2.c) for > instance. They have labels that helps to know what's going on. > > I will stop here in the review for today and will comment the rest tomorrow. > > Cheers, Regards, Bhupinder
On 03/21/2017 01:27 PM, Bhupinder Thakur wrote: > Hi Julien, Hi Bhupinder, > On 26 February 2017 at 22:37, Julien Grall <julien.grall@arm.com> wrote: >>> + break; >>> + case VPL011_UARTDR_OFFSET: >>> + vpl011_read_data(v->domain, &ch); >> >> >> Should not you check the return value of vpl011_read_data? Also, what if >> there is no data? > This condition should not happen because the RX FIFO empty bit would > be set in the UARTFR register when the last data is read from the ring > buffer and the the guest is not supposed to issue next read until the > RX FIFO empty bit is cleared indicating there is more data now. What you describe is how a well-behave guest will interact with the pl011 emulation. It does not describe what would happen if a misbehaved guest will read continuously the register. I cannot find any things in the spec about what should be the state of the register if no data is available. So I guess we just need to ensure that we don't leak in information from the stack. This seems to be addressed by *data = 0 in "vpl011_read_data". Please document it to avoid removing it by mistake in the future. > >> >>> + *r = ch; >>> + break; >>> + case VPL011_UARTFR_OFFSET: >>> + *r = v->domain->arch.vpl011.flag; >> >> >> I am fairly surprised that none of this code is actually protected by lock. >> For instance the update of flag is not atomic. So is it safe? > For reading, I thought no locking was required, as I believe a 32-bit > value read/write on ARM should be atomic. So if the value is modified > in some other context while it is being read in another context, the > reader should see either the old value or the new value. > For register writes, yes I need to take a lock where I am updating > certain bits in the register. I will add the locking there. Fair point. I was more worry about ordering between read/write between multiple vCPU. But I guess we don't care if the read does not return an update to date value. > >> >>> + break; >>> + case VPL011_UARTIMSC_OFFSET: >>> + *r = v->domain->arch.vpl011.intr_mask; >>> + break; >>> + case VPL011_UARTICR_OFFSET: >>> + *r = 0; >> >> >> Looking at the spec, this register is write-only. So why do you implement >> RAZ? > In such cases, where the guest tries to write to RO register or tries > to read a WO register, should I send a abort to the guest? I don't see any behavior requirement in the spec. So I would send an abort to the guest (e.g return 0 in the function). >> >>> + break; >>> + case VPL011_UARTRIS_OFFSET: >>> + *r = v->domain->arch.vpl011.raw_intr_status; >>> + break; >>> + case VPL011_UARTMIS_OFFSET: >>> + *r = v->domain->arch.vpl011.raw_intr_status & >>> + v->domain->arch.vpl011.intr_mask; >>> + break; >>> + case VPL011_UARTDMACR_OFFSET: >>> + *r = 0; /* uart DMA is not supported. Here it always returns >>> 0 */ >> >> >> My understanding of the spec is DMA is not optional. So what would happen if >> the guest tries to enable it? >> >>> + break; >>> + case VPL011_UARTRSR_OFFSET: >>> + *r = 0; /* it always returns 0 as there are no physical >>> errors */ >> >> >> This register contains contains the bit OE that tells whether the FIFO is >> full or not. The FIFO here is the PV ring, so maybe we should set this bit >> if the ring is full. > The OE condition will not happen in this case since xenconsole will > not write more data to the guest if the ring buffer is full. There is > a separate UARTFR status bit which indicates whether the ring buffer > is full. Sorry I still don't get it. Xenconsole will likely have to hold characters that are not written in the PV console. I would have expected that this would be used to set the OE bit. Did I miss anything? >> >>> + break; >>> + default: >>> + printk ("vpl011_mmio_read: invalid switch case %d\n", >>> (int)(info->gpa - GUEST_PL011_BASE)); >> >> >> Coding style: printk(...). >> >> Also, printk is not ratelimited by default. Please use gprintk(...) which >> will be ratelimited and print the domain information. This is useful when >> you have multiple guest. >> > Replaced printk with gprintk. > >>> + break; >>> + } >>> + >>> + return VPL011_EMUL_OK; >> >> >> Please use plain value as the return is not pl011 specific. Also, I am a bit >> surprised that you return "ok" even when a register as not been emulated. >> IHMO, a data abort should be sent to the guest. > > Corrected this. Now it returns an error incase the register is not > emulated . For the return values, I see that typically values 1/0 are > returned like in vgic-v2/3.c. Are there some common macros which I can > use? No. I want to replace 0/1 by false/true but I never had the time to do the work. Cheers,
Hi Konrad, On 4 March 2017 at 01:29, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote: > .snip.. >> + case VPL011_UARTCR_OFFSET: >> + *r = v->domain->arch.vpl011.control; >> + break; > > Pls add a new newline after each break. Ok. > >> + case VPL011_UARTDR_OFFSET: >> + vpl011_read_data(v->domain, &ch); >> + *r = ch; >> + break; >> + case VPL011_UARTFR_OFFSET: >> + *r = v->domain->arch.vpl011.flag; >> + break; >> + case VPL011_UARTIMSC_OFFSET: >> + *r = v->domain->arch.vpl011.intr_mask; >> + break; >> + case VPL011_UARTICR_OFFSET: >> + *r = 0; >> + break; >> + case VPL011_UARTRIS_OFFSET: >> + *r = v->domain->arch.vpl011.raw_intr_status; >> + break; >> + case VPL011_UARTMIS_OFFSET: >> + *r = v->domain->arch.vpl011.raw_intr_status & >> + v->domain->arch.vpl011.intr_mask; >> + break; >> + case VPL011_UARTDMACR_OFFSET: >> + *r = 0; /* uart DMA is not supported. Here it always returns 0 */ >> + break; >> + case VPL011_UARTRSR_OFFSET: >> + *r = 0; /* it always returns 0 as there are no physical errors */ >> + break; >> + default: >> + printk ("vpl011_mmio_read: invalid switch case %d\n", (int)(info->gpa - GUEST_PL011_BASE)); > > .. and you still return EMULOK? > Now I am returning error for the default case. > Also is this printk really neccessary? > >> + break; >> + } >> + >> + return VPL011_EMUL_OK; >> +} >> + >> +static int vpl011_mmio_write(struct vcpu *v, mmio_info_t *info, register_t r, void *priv) >> +{ >> + unsigned char ch = r; >> + >> + switch (info->gpa - GUEST_PL011_BASE) >> + { >> + case VPL011_UARTCR_OFFSET: >> + v->domain->arch.vpl011.control = r; >> + break; >> + case VPL011_UARTDR_OFFSET: >> + vpl011_write_data(v->domain, ch); >> + break; >> + case VPL011_UARTIMSC_OFFSET: >> + v->domain->arch.vpl011.intr_mask = r; >> + if ( (v->domain->arch.vpl011.raw_intr_status & v->domain->arch.vpl011.intr_mask) ) >> + vgic_vcpu_inject_spi(v->domain, (int)v->domain->arch.hvm_domain.params[HVM_PARAM_VPL011_VIRQ]); >> + break; >> + case VPL011_UARTICR_OFFSET: >> + /* >> + * clear all bits which are set in the input >> + */ >> + v->domain->arch.vpl011.raw_intr_status &= ~r; >> + if ( (v->domain->arch.vpl011.raw_intr_status & v->domain->arch.vpl011.intr_mask) ) >> + { >> + vgic_vcpu_inject_spi(v->domain, (int)v->domain->arch.hvm_domain.params[HVM_PARAM_VPL011_VIRQ]); >> + } >> + break; >> + case VPL011_UARTRSR_OFFSET: // nothing to clear >> + break; >> + case VPL011_UARTFR_OFFSET: // these are all RO registers >> + case VPL011_UARTRIS_OFFSET: >> + case VPL011_UARTMIS_OFFSET: >> + case VPL011_UARTDMACR_OFFSET: >> + break; >> + default: >> + printk ("vpl011_mmio_write: switch case not handled %d\n", (int)(info->gpa - GUEST_PL011_BASE)); >> + break; >> + } >> + >> + return VPL011_EMUL_OK; >> +} >> + >> +static const struct mmio_handler_ops vpl011_mmio_handler = { >> + .read = vpl011_mmio_read, >> + .write = vpl011_mmio_write, >> +}; >> + >> + > > why the extra newline. > >> + >> +int vpl011_map_guest_page(struct domain *d) >> +{ >> + int rc=0; > > No need for = 0; Ok. >> + >> + /* >> + * map the guest PFN to Xen address space >> + */ >> + rc = prepare_ring_for_helper(d, >> + d->arch.hvm_domain.params[HVM_PARAM_VPL011_CONSOLE_PFN], >> + &d->arch.vpl011.ring_page, >> + (void **)&d->arch.vpl011.ring_buf); >> + if ( rc < 0 ) >> + { >> + printk("Failed to map vpl011 guest PFN\n"); >> + } >> + >> + return rc; > > Could you just make this whole routine be: > > return prepare_ring_for_helper(..) > > That printk is not needed I think? I would add it to the caller of this function. > Ok. >> +} >> + >> +static int vpl011_data_avail(struct domain *d) >> +{ >> + int rc=0; >> + unsigned long flags; >> + >> + struct console_interface *intf=(struct console_interface *)d->arch.vpl011.ring_buf; > > Can you have an macro for this? >> + >> + VPL011_LOCK(d, flags); > > Please don't. Just use normal spin_lock invocation. > >> + >> + /*` >> + * check IN ring buffer >> + */ > > Please remove the comment. It does not add much context to the code. > >> + if ( !VPL011_IN_RING_EMPTY(intf) ) >> + { >> + /* >> + * clear the RX FIFO empty flag as the ring is not empty >> + */ > > Please remove this comment. > >> + d->arch.vpl011.flag &= ~(VPL011_UARTFR_RXFE); >> + >> + /* >> + * if the buffer is full then set the RX FIFO FULL flag > > Also please remove this comment. > >> + */ >> + if ( VPL011_IN_RING_FULL(intf) ) >> + d->arch.vpl011.flag |= (VPL011_UARTFR_RXFF); >> + >> + /* >> + * set the RX interrupt status >> + */ > > And this one too. > > What I would recommend is that you write one nice comment at the start > of the 'if' saying: > > "Have to set RX state regardless whether it is full or has some entries." > >> + d->arch.vpl011.raw_intr_status |= (VPL011_UARTRIS_RXRIS); >> + } >> + >> + /* >> + * check OUT ring buffer > > Please remove this comment. >> + */ >> + if ( !VPL011_OUT_RING_FULL(intf) ) >> + { >> + /* >> + * if the buffer is not full then clear the TX FIFO full flag >> + */ > > > Please remove this comment. >> + d->arch.vpl011.flag &= ~(VPL011_UARTFR_TXFF); >> + >> + /* >> + * set the TX interrupt status >> + */ >> + d->arch.vpl011.raw_intr_status |= (VPL011_UARTRIS_TXRIS); >> + >> + if ( VPL011_OUT_RING_EMPTY(intf) ) >> + { >> + /* >> + * clear the uart busy flag and set the TX FIFO empty flag >> + */ >> + d->arch.vpl011.flag &= ~(VPL011_UARTFR_BUSY); >> + d->arch.vpl011.flag |= (VPL011_UARTFR_TXFE); >> + } >> + } > >> + >> + VPL011_UNLOCK(d, flags); >> + >> + /* >> + * send an interrupt if it is not masked > > So does that mean we would send an interrupt even if the in/out rings > are full? If the out ring is full then the interrupt would not be raised because the Tx interrupt status bit would be cleared in UARTRIS register in vpl011_write_data(). For the in ring, the Rx interrupt would be raised if it is not masked. > >> + */ >> + if ( (d->arch.vpl011.raw_intr_status & d->arch.vpl011.intr_mask) ) >> + vgic_vcpu_inject_spi(d, (int)d->arch.hvm_domain.params[HVM_PARAM_VPL011_VIRQ]); >> + >> + if ( !VPL011_OUT_RING_EMPTY(intf) ) > > This is asking for a race. > > So it may have changed between the first time you read it and this > one. > > Is that OK? Would it still be OK to send an interrupt even if say > the ring was emtpry at start of the function but became empty now? > Yes the out ring state can change between the two times it is checked. So finally it will be either be empty or non-empty irrespective of what it was the first time. If it is non-empty then an event is raised to dom0 to read the data from the out ring. > Would it be better if you read all the values from > the ring at the start of the function in local variables > and made sure to use 'barrier()' so there are no compiler > "optimizations" done? > >> + { >> + /* >> + * raise an interrupt to dom0 >> + */ > > Please remove this. > >> + rc = raw_evtchn_send(d, >> + d->arch.hvm_domain.params[HVM_PARAM_VPL011_CONSOLE_EVTCHN], NULL); > > What if HVM_PARAM_VPL011_CONSOLE_EVTCHN is zero? Should you check > that first? > > Or is it given (in which case please add an ASSERT) that the > HVM_PARAM_VPL011_CONSOLE_EVTCHN is _always_ set? > Yes. During vpl011 initialization for this domain, this event channel will be allocated. I will add an assert. >> + >> + if ( rc < 0 ) >> + printk("Failed to send vpl011 interrupt to dom0\n"); > > gdprintk. But is this useful? How will this help in debugging the > problem? > >> + } >> + >> + return rc; >> +} >> + >> +int vpl011_read_data(struct domain *d, unsigned char *data) >> +{ >> + int rc=0; > > No point for this. Just do 'return 0' >> + unsigned long flags; >> + struct console_interface *intf=(struct console_interface *)d->arch.vpl011.ring_buf; >> + >> + *data = 0; >> + >> + VPL011_LOCK(d, flags); >> + >> + /* >> + * if there is data in the ring buffer then copy it to the output buffer >> + */ >> + if ( !VPL011_IN_RING_EMPTY(intf) ) >> + { >> + *data = intf->in[MASK_VPL011CONS_IDX(intf->in_cons++, intf->in)]; >> + } >> + >> + /* >> + * if the ring buffer is empty then set the RX FIFO empty flag >> + */ >> + if ( VPL011_IN_RING_EMPTY(intf) ) > > So there is a nice race here. Why don't you just use an 'else' ? > Since we are reading the data from the ring buffer in the first step, the ring buffer may go empty after the first step. If I use else then I will not enter the 2nd if statement. > >> + { >> + d->arch.vpl011.flag |= (VPL011_UARTFR_RXFE); >> + d->arch.vpl011.raw_intr_status &= ~(VPL011_UARTRIS_RXRIS); >> + } >> + >> + /* >> + * clear the RX FIFO full flag >> + */ >> + d->arch.vpl011.flag &= ~(VPL011_UARTFR_RXFF); >> + >> + VPL011_UNLOCK(d, flags); >> + >> + return rc; >> +} >> + >> +int vpl011_write_data(struct domain *d, unsigned char data) >> +{ >> + int rc=0; > > Pls remove the '=0' >> + unsigned long flags; >> + struct console_interface *intf=(struct console_interface *)d->arch.vpl011.ring_buf; >> + >> + VPL011_LOCK(d, flags); >> + >> + /* >> + * if there is space in the ring buffer then write the data > > Please moreve this comment. >> + */ >> + if ( !VPL011_OUT_RING_FULL(intf) ) >> + { >> + intf->out[MASK_VPL011CONS_IDX(intf->out_prod++, intf->out)] = data; >> + smp_wmb(); >> + } >> >> + /* >> + * if there is no space in the ring buffer then set the >> + * TX FIFO FULL flag > > Again, why not just an 'else'? > >> + */ >> + if ( VPL011_OUT_RING_FULL(intf) ) >> + { >> + d->arch.vpl011.flag |= (VPL011_UARTFR_TXFF); >> + d->arch.vpl011.raw_intr_status &= ~(VPL011_UARTRIS_TXRIS); >> + } >> + >> + /* >> + * set the uart busy status >> + */ >> + d->arch.vpl011.flag |= (VPL011_UARTFR_BUSY); >> + >> + /* >> + * clear the TX FIFO empty flag >> + */ > > Please remove these comments. They are distracting. > > Also a single line comment should be: > > /* <comment> */ > > >> + d->arch.vpl011.flag &= ~(VPL011_UARTFR_TXFE); >> + >> + VPL011_UNLOCK(d, flags); >> + >> + /* >> + * raise an event to dom0 only if it is the first character in the buffer > > Why? Why only the first charcter? > I wanted to avoid raising too many events to dom0. Since dom0 is going to process the data in the ring buffer until it goes empty, it should be ok to raise an event initially only when the first data is written to the ring buffer so that dom0 can start reading the data from the ring buffer. >> + */ >> + if ( VPL011_RING_DEPTH(intf, out) == 1 ) >> + { >> + rc = raw_evtchn_send(d, >> + d->arch.hvm_domain.params[HVM_PARAM_VPL011_CONSOLE_EVTCHN], NULL); > > And again, should we have an assert for HVM_PARAM_VPL011_CONSOLE_EVTCHN or a check > for it? I will add an assert. > >> + >> + if ( rc < 0 ) >> + printk("Failed to send vpl011 interrupt to dom0\n"); > > gdprintk. > >> + } >> + >> + return rc; >> +} >> + >> +static void vpl011_notification(struct vcpu *v, unsigned int port) >> +{ >> + vpl011_data_avail(v->domain); >> +} >> + >> +int domain_vpl011_init(struct domain *d) >> +{ >> + int rc=0; > > You can remove the =0; >> + >> + /* >> + * register xen event channel >> + */ > > Please remove this comment. > >> + rc = alloc_unbound_xen_event_channel(d, 0, current->domain->domain_id, >> + vpl011_notification); >> + if (rc < 0) > > Spaces. >> + { >> + printk ("Failed to allocate vpl011 event channel\n"); > > gdprintk > > And also not sure if this is needed. Won't the rc be propagated to the > caller who can print this? > >> + return rc; >> + } >> + d->arch.hvm_domain.params[HVM_PARAM_VPL011_CONSOLE_EVTCHN] = rc; >> + >> + /* >> + * allocate an SPI VIRQ for the guest >> + */ >> + d->arch.hvm_domain.params[HVM_PARAM_VPL011_VIRQ] = vgic_allocate_spi(d); > > You are not checking vgic_allocate_spi for errors? > I have added a check. >> + >> + /* >> + * register mmio handler >> + */ > > Please remove these comments. They are just saying what the code > is doing. Not adding anything extra. > >> + register_mmio_handler (d, &vpl011_mmio_handler, GUEST_PL011_BASE, GUEST_PL011_SIZE, NULL); > > Please kill that space before (. > > And what if register_mmio_handler returns an error? > register_mmio_handler() does not return any value. >> + >> + /* >> + * initialize the lock >> + */ > > That is a comment that is really not needed. > >> + spin_lock_init(&d->arch.vpl011.lock); >> + >> + /* >> + * clear the flag, control and interrupt status registers >> + */ > > Why not just do memset on the structure? > >> + d->arch.vpl011.control = 0; >> + d->arch.vpl011.flag = 0; >> + d->arch.vpl011.intr_mask = 0; >> + d->arch.vpl011.intr_clear = 0; >> + d->arch.vpl011.raw_intr_status = 0; >> + d->arch.vpl011.masked_intr_status = 0; >> + >> + return 0; >> +} I will use the memset. >> diff --git a/xen/arch/arm/vpl011.h b/xen/arch/arm/vpl011.h >> new file mode 100644 >> index 0000000..f2c577f >> --- /dev/null >> +++ b/xen/arch/arm/vpl011.h >> @@ -0,0 +1,208 @@ >> +/* >> + * include/xen/vpl011.h >> + * >> + * Virtual PL011 UART >> + * >> + * This program is free software; you can redistribute it and/or modify it >> + * under the terms and conditions of the GNU General Public License, >> + * version 2, as published by the Free Software Foundation. >> + * >> + * This program is distributed in the hope it will be useful, but WITHOUT >> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or >> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for >> + * more details. >> + * >> + * You should have received a copy of the GNU General Public License along with >> + * this program; If not, see <http://www.gnu.org/licenses/>. >> + */ >> + >> +#ifndef _VPL011_H_ >> + >> +#define _VPL011_H_ >> + >> +/* >> + * register offsets >> + */ >> +#define VPL011_UARTDR_OFFSET 0x0 // data register (RW) > > Wrong type of comments. Pls use /* */ variant Ok. > >> +#define VPL011_UARTRSR_OFFSET 0x4 // receive status and error clear register (RW) >> +#define VPL011_UARTFR_OFFSET 0x18 // flag register (RO) >> +#define VPL011_UARTRIS_OFFSET 0x3c // raw interrupt status register (RO) >> +#define VPL011_UARTMIS_OFFSET 0x40 // masked interrupt status register (RO) >> +#define VPL011_UARTIMSC_OFFSET 0x38 // interrupt mask set/clear register (RW) >> +#define VPL011_UARTICR_OFFSET 0x44 // interrupt clear register (WO) >> +#define VPL011_UARTCR_OFFSET 0x30 // uart control register >> +#define VPL011_UARTDMACR_OFFSET 0x48 // uart dma control register >> + >> +/* >> + * control register bits - RW >> + */ >> +#define VPL011_UARTCR_UARTEN_BIT 0 >> +#define VPL011_UARTCR_UARTEN (1<<VPL011_UARTCR_UARTEN_BIT) >> +#define VPL011_UARTCR_TXE_BIT 8 >> +#define VPL011_UARTCR_TXE (1<<VPL011_UARTCR_TXE_BIT) >> +#define VPL011_UARTCR_RXE_BIT 9 >> +#define VPL011_UARTCR_RXE (1<<VPL011_UARTCR_RXE_BIT) >> + >> +/* >> + * Flag register bits - RO >> + */ >> +#define VPL011_UARTFR_CTS_BIT 0 // clear to send >> +#define VPL011_UARTFR_CTS (1<<VPL011_UARTFR_CTS_BIT) >> +#define VPL011_UARTFR_DSR_BIT 1 // data set ready >> +#define VPL011_UARTFR_DSR (1<<VPL011_UARTFR_DSR_BIT) >> +#define VPL011_UARTFR_DCD_BIT 2 // data carrier detect >> +#define VPL011_UARTFR_DCD (1<<VPL011_UARTFR_DCD_BIT) >> +#define VPL011_UARTFR_BUSY_BIT 3 // uart busy >> +#define VPL011_UARTFR_BUSY (1<<VPL011_UARTFR_BUSY_BIT) >> +#define VPL011_UARTFR_RXFE_BIT 4 // receive fifo empty >> +#define VPL011_UARTFR_RXFE (1<<VPL011_UARTFR_RXFE_BIT) >> +#define VPL011_UARTFR_TXFF_BIT 5 // transmit fifo full >> +#define VPL011_UARTFR_TXFF (1<<VPL011_UARTFR_TXFF_BIT) >> +#define VPL011_UARTFR_RXFF_BIT 6 // receive fifo full >> +#define VPL011_UARTFR_RXFF (1<<VPL011_UARTFR_RXFF_BIT) >> +#define VPL011_UARTFR_TXFE_BIT 7 // transmit fifo empty >> +#define VPL011_UARTFR_TXFE (1<<VPL011_UARTFR_TXFE_BIT) >> +#define VPL011_UARTFR_RI_BIT 8 // ring indicator >> +#define VPL011_UARTFR_RI (1<<VPL011_UARTFR_RI_BIT) >> + >> +/* >> + * UART raw interrupt status bits - RO >> + */ >> +#define VPL011_UARTRIS_RIRMIS_BIT 0 >> +#define VPL011_UARTRIS_RIRMIS (1<<VPL011_UARTRIS_RIRMIS_BIT) >> +#define VPL011_UARTRIS_CTSRMIS_BIT 1 >> +#define VPL011_UARTRIS_CTSRMIS (1<<VPL011_UARTRIS_CTSRMIS_BIT) >> +#define VPL011_UARTRIS_DCDRMIS_BIT 2 >> +#define VPL011_UARTRIS_DCDRMIS (1<<VPL011_UARTRIS_DCDRMIS_BIT) >> +#define VPL011_UARTRIS_DSRRMIS_BIT 3 >> +#define VPL011_UARTRIS_DSRRMIS (1<<VPL011_UARTRIS_DSRRMIS_BIT) >> +#define VPL011_UARTRIS_RXRIS_BIT 4 >> +#define VPL011_UARTRIS_RXRIS (1<<VPL011_UARTRIS_RXRIS_BIT) >> +#define VPL011_UARTRIS_TXRIS_BIT 5 >> +#define VPL011_UARTRIS_TXRIS (1<<VPL011_UARTRIS_TXRIS_BIT) >> +#define VPL011_UARTRIS_RTRIS_BIT 6 >> +#define VPL011_UARTRIS_RTRIS (1<<VPL011_UARTRIS_RTRIS_BIT) >> +#define VPL011_UARTRIS_FERIS_BIT 7 >> +#define VPL011_UARTRIS_FERIS (1<<VPL011_UARTRIS_FERIS_BIT) >> +#define VPL011_UARTRIS_PERIS_BIT 8 >> +#define VPL011_UARTRIS_PERIS (1<<VPL011_UARTRIS_PERIS_BIT) >> +#define VPL011_UARTRIS_BERIS_BIT 9 >> +#define VPL011_UARTRIS_BERIS (1<<VPL011_UARTRIS_BERIS_BIT) >> +#define VPL011_UARTRIS_OERIS_BIT 10 >> +#define VPL011_UARTRIS_OERIS (1<<VPL011_UARTRIS_OERIS_BIT) >> + >> +/* >> + * UART masked interrupt status bits - RO >> + */ >> +#define VPL011_UARTMIS_RIMMIS_BIT 0 >> +#define VPL011_UARTMIS_RIMMIS (1<<VPL011_UARTMIS_RIMMIS_BIT) >> +#define VPL011_UARTMIS_CTSMMIS_BIT 1 >> +#define VPL011_UARTMIS_CTSMMIS (1<<VPL011_UARTMIS_CTSMMIS_BIT) >> +#define VPL011_UARTMIS_DCDMMIS_BIT 2 >> +#define VPL011_UARTMIS_DCDMMIS (1<<VPL011_UARTMIS_DCDMMIS_BIT) >> +#define VPL011_UARTMIS_DSRMMIS_BIT 3 >> +#define VPL011_UARTMIS_DSRMMIS (1<<VPL011_UARTMIS_DSRMMIS_BIT) >> +#define VPL011_UARTMIS_RXMIS_BIT 4 >> +#define VPL011_UARTMIS_RXMIS (1<<VPL011_UARTMIS_RXMIS_BIT) >> +#define VPL011_UARTMIS_TXMIS_BIT 5 >> +#define VPL011_UARTMIS_TXMIS (1<<VPL011_UARTMIS_TXMIS_BIT) >> +#define VPL011_UARTMIS_RTMIS_BIT 6 >> +#define VPL011_UARTMIS_RTMIS (1<<VPL011_UARTMIS_RTMIS_BIT) >> +#define VPL011_UARTMIS_FEMIS_BIT 7 >> +#define VPL011_UARTMIS_FEMIS (1<<VPL011_UARTMIS_FEMIS_BIT) >> +#define VPL011_UARTMIS_PEMIS_BIT 8 >> +#define VPL011_UARTMIS_PEMIS (1<<VPL011_UARTMIS_PEMIS_BIT) >> +#define VPL011_UARTMIS_BEMIS_BIT 9 >> +#define VPL011_UARTMIS_BEMIS (1<<VPL011_UARTMIS_BEMIS_BIT) >> +#define VPL011_UARTMIS_OEMIS_BIT 10 >> +#define VPL011_UARTMIS_OEMIS (1<<VPL011_UARTMIS_OEMIS_BIT) >> + >> +/* >> + * UART interrupt clear bits - WO >> + */ >> +#define VPL011_UARTICR_RIMIC_BIT 0 >> +#define VPL011_UARTICR_RIMIC (1<<VPL011_UARTICR_RIMIC_BIT) >> +#define VPL011_UARTICR_CTSMIC_BIT 1 >> +#define VPL011_UARTICR_CTSMIC (1<<VPL011_UARTICR_CTSMIC_BIT) >> +#define VPL011_UARTICR_DCDMIC_BIT 2 >> +#define VPL011_UARTICR_DCDMIC (1<<VPL011_UARTICR_DCDMIC_BIT) >> +#define VPL011_UARTICR_DSRMIC_BIT 3 >> +#define VPL011_UARTICR_DSRMIC (1<<VPL011_UARTICR_DSRMIC_BIT) >> +#define VPL011_UARTICR_RXIC_BIT 4 >> +#define VPL011_UARTICR_RXIC (1<<VPL011_UARTICR_RXIC_BIT) >> +#define VPL011_UARTICR_TXIC_BIT 5 >> +#define VPL011_UARTICR_TXIC (1<<VPL011_UARTICR_TXIC_BIT) >> +#define VPL011_UARTICR_RTIC_BIT 6 >> +#define VPL011_UARTICR_RTIC (1<<VPL011_UARTICR_RTIC_BIT) >> +#define VPL011_UARTICR_FEIC_BIT 7 >> +#define VPL011_UARTICR_FEIC (1<<VPL011_UARTICR_FEIC_BIT) >> +#define VPL011_UARTICR_PEIC_BIT 8 >> +#define VPL011_UARTICR_PEIC (1<<VPL011_UARTICR_PEIC_BIT) >> +#define VPL011_UARTICR_BEIC_BIT 9 >> +#define VPL011_UARTICR_BEIC (1<<VPL011_UARTICR_BEIC_BIT) >> +#define VPL011_UARTICR_OEIC_BIT 10 >> +#define VPL011_UARTICR_OEIC (1<<VPL011_UARTICR_OEIC_BIT) >> + >> +/* >> + * UART interrupt mask set/clear bits - RW >> + */ >> +#define VPL011_UARTIMSC_RIMIM_BIT 0 >> +#define VPL011_UARTIMSC_RIMIM (1<<VPL011_UARTIMSC_RIMIM_BIT) >> +#define VPL011_UARTIMSC_CTSMIM_BIT 1 >> +#define VPL011_UARTIMSC_CTSMIM (1<<VPL011_UARTIMSC_CTSMIM_BIT) >> +#define VPL011_UARTIMSC_DCDMIM_BIT 2 >> +#define VPL011_UARTIMSC_DCDMIM (1<<VPL011_UARTIMSC_DCDMIM_BIT) >> +#define VPL011_UARTIMSC_DSRMIM_BIT 3 >> +#define VPL011_UARTIMSC_DSRMIM (1<<VPL011_UARTIMSC_DSRMIM_BIT) >> +#define VPL011_UARTIMSC_RXIM_BIT 4 >> +#define VPL011_UARTIMSC_RXIM (1<<VPL011_UARTIMSC_RXIM_BIT) >> +#define VPL011_UARTIMSC_TXIM_BIT 5 >> +#define VPL011_UARTIMSC_TXIM (1<<VPL011_UARTIMSC_TXIM_BIT) >> +#define VPL011_UARTIMSC_RTIM_BIT 6 >> +#define VPL011_UARTIMSC_RTIM (1<<VPL011_UARTIMSC_RTIM_BIT) >> +#define VPL011_UARTIMSC_FEIM_BIT 7 >> +#define VPL011_UARTIMSC_FEIM (1<<VPL011_UARTIMSC_FEIM_BIT) >> +#define VPL011_UARTIMSC_PEIM_BIT 8 >> +#define VPL011_UARTIMSC_PEIM (1<<VPL011_UARTIMSC_PEIM_BIT) >> +#define VPL011_UARTIMSC_BEIM_BIT 9 >> +#define VPL011_UARTIMSC_BEIM (1<<VPL011_UARTIMSC_BEIM_BIT) >> +#define VPL011_UARTIMSC_OEIM_BIT 10 >> +#define VPL011_UARTIMSC_OEIM (1<<VPL011_UARTIMSC_OEIM_BIT) >> + >> + >> +/* >> + * helper macros >> + */ >> +#define VPL011_RING_DEPTH(intf,dir) (((intf)->dir ## _prod - (intf)->dir ## _cons)) >> +#define VPL011_RING_MAX_DEPTH(intf,dir) (sizeof((intf)->dir)-1) >> + >> +#define VPL011_IN_RING_EMPTY(intf) (VPL011_RING_DEPTH(intf, in) == 0) >> + >> +#define VPL011_OUT_RING_EMPTY(intf) (VPL011_RING_DEPTH(intf, out) == 0) >> + >> +#define VPL011_IN_RING_FULL(intf) (VPL011_RING_DEPTH(intf, in) == VPL011_RING_MAX_DEPTH(intf, in)) >> + >> +#define VPL011_OUT_RING_FULL(intf) (VPL011_RING_DEPTH(intf, out) == VPL011_RING_MAX_DEPTH(intf,out)) >> + >> +#define VPL011_LOCK(d,flags) spin_lock_irqsave(&(d)->arch.vpl011.lock, flags) >> +#define VPL011_UNLOCK(d,flags) spin_unlock_irqrestore(&(d)->arch.vpl011.lock, flags) >> + >> +/* >> + * MMIO return values >> + */ >> +#define VPL011_EMUL_OK 1 >> +#define VPL011_EMUL_FAIL 0 >> + >> +int domain_vpl011_init(struct domain *d); >> +int vpl011_map_guest_page(struct domain *d); >> +int vpl011_read_data(struct domain *d, unsigned char *data); >> +int vpl011_write_data(struct domain *d, unsigned char data); >> + >> +#define MASK_VPL011CONS_IDX(idx, ring) ((idx) & (sizeof(ring)-1)) >> +struct console_interface { >> + char in[1024]; >> + char out[2048]; >> + uint32_t in_cons, in_prod; >> + uint32_t out_cons, out_prod; >> +}; >> +#endif >> diff --git a/xen/common/Kconfig b/xen/common/Kconfig >> index f2ecbc4..7e2feac 100644 >> --- a/xen/common/Kconfig >> +++ b/xen/common/Kconfig >> @@ -237,4 +237,10 @@ config FAST_SYMBOL_LOOKUP >> The only user of this is Live patching. >> >> If unsure, say Y. >> + >> +config VPL011_CONSOLE >> + bool "Emulated pl011 console support" >> + default y > > I thought by default it would be 'n'? > > Or perhaps defauly y if CONFIG_ARM or such? I moved this config option to xen/arch/arm/Kconfig. > >> + ---help--- >> + Allows a guest to use pl011 UART as a console >> endmenu >> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h >> index 2d6fbb1..ff2403a 100644 >> --- a/xen/include/asm-arm/domain.h >> +++ b/xen/include/asm-arm/domain.h >> @@ -40,6 +40,7 @@ struct vtimer { >> uint64_t cval; >> }; >> >> + > > ?? >> struct arch_domain >> { >> #ifdef CONFIG_ARM_64 >> @@ -131,6 +132,20 @@ struct arch_domain >> struct { >> uint8_t privileged_call_enabled : 1; >> } monitor; >> + >> +#ifdef CONFIG_VPL011_CONSOLE >> + struct vpl011 { >> + void *ring_buf; >> + struct page_info *ring_page; >> + uint32_t flag; /* flag register */ >> + uint32_t control; /* control register */ >> + uint32_t intr_mask; /* interrupt mask register*/ >> + uint32_t intr_clear; /* interrupt clear register */ >> + uint32_t raw_intr_status; /* raw interrupt status register */ >> + uint32_t masked_intr_status; /* masked interrupt register */ >> + spinlock_t lock; >> + } vpl011; >> +#endif >> } __cacheline_aligned; >> >> struct arch_vcpu >> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h >> index bd974fb..1d4548f 100644 >> --- a/xen/include/public/arch-arm.h >> +++ b/xen/include/public/arch-arm.h >> @@ -410,6 +410,10 @@ typedef uint64_t xen_callback_t; >> #define GUEST_ACPI_BASE 0x20000000ULL >> #define GUEST_ACPI_SIZE 0x02000000ULL >> >> +/* PL011 mappings */ >> +#define GUEST_PL011_BASE 0x22000000ULL >> +#define GUEST_PL011_SIZE 0x00001000ULL >> + >> /* >> * 16MB == 4096 pages reserved for guest to use as a region to map its >> * grant table in. >> @@ -420,6 +424,7 @@ typedef uint64_t xen_callback_t; >> #define GUEST_MAGIC_BASE xen_mk_ullong(0x39000000) >> #define GUEST_MAGIC_SIZE xen_mk_ullong(0x01000000) >> >> + > > ??? I will remove unnecessary newlines. >> #define GUEST_RAM_BANKS 2 >> >> #define GUEST_RAM0_BASE xen_mk_ullong(0x40000000) /* 3GB of low RAM @ 1GB */ >> -- >> 2.7.4 >> >> >> _______________________________________________ >> Xen-devel mailing list >> Xen-devel@lists.xen.org >> https://lists.xen.org/xen-devel Regards, Bhupinder
.Hi Julien, On 5 March 2017 at 17:42, Julien Grall <julien.grall@arm.com> wrote: > Hi Bhupinder, > > On 21/02/17 11:25, Bhupinder Thakur wrote: >> >> diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c >> new file mode 100644 >> index 0000000..88ba968 >> --- /dev/null >> +++ b/xen/arch/arm/vpl011.c > > > [...] > >> +static int vpl011_mmio_write(struct vcpu *v, mmio_info_t *info, >> register_t r, void *priv) > > > See my comment on the vpl011_mmio_read about the structure of the function. > >> +{ >> + unsigned char ch = r; >> + >> + switch (info->gpa - GUEST_PL011_BASE) >> + { >> + case VPL011_UARTCR_OFFSET: > > > This register is not required in the SBSA UART. Yes, I checked that. I will remove it. > >> + v->domain->arch.vpl011.control = r; >> + break; >> + case VPL011_UARTDR_OFFSET: >> + vpl011_write_data(v->domain, ch); > > > The implicit casting of register_t to ch is a bit confusing. Also I would > prefer to use uint8_t as it reflects the size of the field. I have removed implicit casing with explicit casting. > > Lastly, what if vpl011_write_data is returning an error? > Normally vpl011_write_data() should not fail since the guest should stop writing more data once the ring buffer goes full and TXFF bit is set in UARTFR. So there should always be space in the ring buffer for the next data when a mmio write happens. If the guest still writes when ring buffer is already full then data would be lost. >> + break; >> + case VPL011_UARTIMSC_OFFSET: >> + v->domain->arch.vpl011.intr_mask = r; > > > You need to sanitize the value written and make sure reserved field and > Write As Ones register and handle correctly (see the spec). > >> + if ( (v->domain->arch.vpl011.raw_intr_status & >> v->domain->arch.vpl011.intr_mask) ) > > > This line and the line below are over 80 columns. Also the code in general > would benefits if you define a local variable to point to > v->domain->arch.vpl011. I will use a local variable for v->domain->arch.vpl011 > >> + vgic_vcpu_inject_spi(v->domain, >> (int)v->domain->arch.hvm_domain.params[HVM_PARAM_VPL011_VIRQ]); > > > I don't think we need an HVM_PARAM for the pl011 SPI. We cannot hardcode > this in the guest layout (see include/public/arch-arm.h). Please see my comment below on how to reserve 1 SPI for vpl011. > >> + break; >> + case VPL011_UARTICR_OFFSET: >> + /* >> + * clear all bits which are set in the input >> + */ >> + v->domain->arch.vpl011.raw_intr_status &= ~r; >> + if ( (v->domain->arch.vpl011.raw_intr_status & >> v->domain->arch.vpl011.intr_mask) ) >> + { > > > { } are not necessary. > >> + vgic_vcpu_inject_spi(v->domain, >> (int)v->domain->arch.hvm_domain.params[HVM_PARAM_VPL011_VIRQ]); >> + } > > > The if is exactly the same as the on in IMSC. This is the call of an helper > to check the interrupt state and inject one if necessary. > >> + break; >> + case VPL011_UARTRSR_OFFSET: // nothing to clear > > > The coding style for single line comment in Xen is /* ... */ > > Also, I think we may want to handle Overrun error as the ring may be full. > >> + break; >> + case VPL011_UARTFR_OFFSET: // these are all RO registers >> + case VPL011_UARTRIS_OFFSET: >> + case VPL011_UARTMIS_OFFSET: >> + case VPL011_UARTDMACR_OFFSET: > > > Please have a look at the vGICv2/vGICv3 emulation see how we implemented > write ignore register. > Ok. >> + break; >> + default: >> + printk ("vpl011_mmio_write: switch case not handled %d\n", >> (int)(info->gpa - GUEST_PL011_BASE)); > > > See my comment about the prinkt in the read emulation. > >> + break; >> + } >> + >> + return VPL011_EMUL_OK; >> +} >> + >> +static const struct mmio_handler_ops vpl011_mmio_handler = { >> + .read = vpl011_mmio_read, >> + .write = vpl011_mmio_write, >> +}; >> + >> + >> + > > > Only newline is sufficient. This was mention by Konrad and I will try to > avoid repeating his comments. > >> +int vpl011_map_guest_page(struct domain *d) >> +{ >> + int rc=0; >> + >> + /* >> + * map the guest PFN to Xen address space >> + */ >> + rc = prepare_ring_for_helper(d, >> + >> d->arch.hvm_domain.params[HVM_PARAM_VPL011_CONSOLE_PFN], >> + &d->arch.vpl011.ring_page, >> + (void **)&d->arch.vpl011.ring_buf); > > > Why do you need the cast? > I will remove the casting. > Also I cannot find any code in this series which destroy the ring. Please > have a helper called vpl011_unmap_guest_page to do this job and call when > the domain is destroyed. > Ok. > >> + if ( rc < 0 ) >> + { >> + printk("Failed to map vpl011 guest PFN\n"); >> + } >> + >> + return rc; >> +} >> + >> +static int vpl011_data_avail(struct domain *d) >> +{ >> + int rc=0; >> + unsigned long flags; >> + >> + struct console_interface *intf=(struct console_interface >> *)d->arch.vpl011.ring_buf; >> + >> + VPL011_LOCK(d, flags); >> + >> + /*` >> + * check IN ring buffer >> + */ >> + if ( !VPL011_IN_RING_EMPTY(intf) ) >> + { >> + /* >> + * clear the RX FIFO empty flag as the ring is not empty >> + */ >> + d->arch.vpl011.flag &= ~(VPL011_UARTFR_RXFE); >> + >> + /* >> + * if the buffer is full then set the RX FIFO FULL flag >> + */ >> + if ( VPL011_IN_RING_FULL(intf) ) >> + d->arch.vpl011.flag |= (VPL011_UARTFR_RXFF); >> + >> + /* >> + * set the RX interrupt status >> + */ >> + d->arch.vpl011.raw_intr_status |= (VPL011_UARTRIS_RXRIS); >> + } >> + >> + /* >> + * check OUT ring buffer >> + */ >> + if ( !VPL011_OUT_RING_FULL(intf) ) >> + { >> + /* >> + * if the buffer is not full then clear the TX FIFO full flag >> + */ >> + d->arch.vpl011.flag &= ~(VPL011_UARTFR_TXFF); >> + >> + /* >> + * set the TX interrupt status >> + */ >> + d->arch.vpl011.raw_intr_status |= (VPL011_UARTRIS_TXRIS); >> + >> + if ( VPL011_OUT_RING_EMPTY(intf) ) >> + { >> + /* >> + * clear the uart busy flag and set the TX FIFO empty flag >> + */ >> + d->arch.vpl011.flag &= ~(VPL011_UARTFR_BUSY); >> + d->arch.vpl011.flag |= (VPL011_UARTFR_TXFE); >> + } >> + } >> + >> + VPL011_UNLOCK(d, flags); >> + >> + /* >> + * send an interrupt if it is not masked >> + */ >> + if ( (d->arch.vpl011.raw_intr_status & d->arch.vpl011.intr_mask) ) >> + vgic_vcpu_inject_spi(d, >> (int)d->arch.hvm_domain.params[HVM_PARAM_VPL011_VIRQ]); > > > See my comment above about having an helper for this code. Ok. I will add a helper function. > >> + >> + if ( !VPL011_OUT_RING_EMPTY(intf) ) >> + { >> + /* >> + * raise an interrupt to dom0 > > > The backend console does not necessarily live in DOM0. Anyway, Konrad > requested to drop the comment and I am happy with that. > >> + */ >> + rc = raw_evtchn_send(d, >> + >> d->arch.hvm_domain.params[HVM_PARAM_VPL011_CONSOLE_EVTCHN], NULL); > > > You are using a function that is been defined in a later patch (i.w #3). All > the patch should be able to compile without applying upcoming patch to allow > bisection. > > Ideally, Xen should still be functional for every patch. But it is a best > effort. > I will reorder the patches accordingly. >> + >> + if ( rc < 0 ) >> + printk("Failed to send vpl011 interrupt to dom0\n"); >> + } >> + >> + return rc; >> +} >> + >> +int vpl011_read_data(struct domain *d, unsigned char *data) > > > This function does not seem to be used outside of this file. So why do you > export it? > Made it a static function. > >> +{ >> + int rc=0; >> + unsigned long flags; >> + struct console_interface *intf=(struct console_interface >> *)d->arch.vpl011.ring_buf; >> + >> + *data = 0; >> + >> + VPL011_LOCK(d, flags); >> + >> + /* >> + * if there is data in the ring buffer then copy it to the output >> buffer >> + */ >> + if ( !VPL011_IN_RING_EMPTY(intf) ) >> + { >> + *data = intf->in[MASK_VPL011CONS_IDX(intf->in_cons++, intf->in)]; >> + } >> + >> + /* >> + * if the ring buffer is empty then set the RX FIFO empty flag >> + */ >> + if ( VPL011_IN_RING_EMPTY(intf) ) >> + { >> + d->arch.vpl011.flag |= (VPL011_UARTFR_RXFE); >> + d->arch.vpl011.raw_intr_status &= ~(VPL011_UARTRIS_RXRIS); >> + } >> + >> + /* >> + * clear the RX FIFO full flag >> + */ >> + d->arch.vpl011.flag &= ~(VPL011_UARTFR_RXFF); >> + >> + VPL011_UNLOCK(d, flags); >> + >> + return rc; >> +} >> + >> +int vpl011_write_data(struct domain *d, unsigned char data) > > > Same has for vpl011_read_data. > > >> +{ >> + int rc=0; >> + unsigned long flags; >> + struct console_interface *intf=(struct console_interface >> *)d->arch.vpl011.ring_buf; >> + >> + VPL011_LOCK(d, flags); >> + >> + /* >> + * if there is space in the ring buffer then write the data >> + */ >> + if ( !VPL011_OUT_RING_FULL(intf) ) >> + { >> + intf->out[MASK_VPL011CONS_IDX(intf->out_prod++, intf->out)] = >> data; >> + smp_wmb(); >> + } >> + >> + /* >> + * if there is no space in the ring buffer then set the >> + * TX FIFO FULL flag >> + */ >> + if ( VPL011_OUT_RING_FULL(intf) ) >> + { >> + d->arch.vpl011.flag |= (VPL011_UARTFR_TXFF); >> + d->arch.vpl011.raw_intr_status &= ~(VPL011_UARTRIS_TXRIS); >> + } >> + >> + /* >> + * set the uart busy status >> + */ >> + d->arch.vpl011.flag |= (VPL011_UARTFR_BUSY); >> + >> + /* >> + * clear the TX FIFO empty flag >> + */ >> + d->arch.vpl011.flag &= ~(VPL011_UARTFR_TXFE); >> + >> + VPL011_UNLOCK(d, flags); >> + >> + /* >> + * raise an event to dom0 only if it is the first character in the >> buffer >> + */ >> + if ( VPL011_RING_DEPTH(intf, out) == 1 ) >> + { >> + rc = raw_evtchn_send(d, >> + >> d->arch.hvm_domain.params[HVM_PARAM_VPL011_CONSOLE_EVTCHN], NULL); >> + >> + if ( rc < 0 ) >> + printk("Failed to send vpl011 interrupt to dom0\n"); >> + } >> + >> + return rc; >> +} >> + >> +static void vpl011_notification(struct vcpu *v, unsigned int port) >> +{ >> + vpl011_data_avail(v->domain); > > > vpl011_data_avail is returning an error but you don't check. What is the > point of it then? > >> +} >> + >> +int domain_vpl011_init(struct domain *d) > > > I was expected a destroy function to clean-up the vpl011 emulation. > I will add a deinit function. >> +{ >> + int rc=0; >> + >> + /* >> + * register xen event channel >> + */ >> + rc = alloc_unbound_xen_event_channel(d, 0, >> current->domain->domain_id, >> + >> vpl011_notification); > > > This line looks wrong to me. The console backend may not live in the same > domain as the toolstack. How should I figure out the correct domain where xenconsoled is running? > >> + if (rc < 0) >> + { >> + printk ("Failed to allocate vpl011 event channel\n"); >> + return rc; >> + } >> + d->arch.hvm_domain.params[HVM_PARAM_VPL011_CONSOLE_EVTCHN] = rc; >> + >> + /* >> + * allocate an SPI VIRQ for the guest >> + */ >> + d->arch.hvm_domain.params[HVM_PARAM_VPL011_VIRQ] = >> vgic_allocate_spi(d); > > > It makes little sense to hardcode the MMIO region and not the SPI. Also, I > would rather avoid to introduce new HVM_PARAM when not necessary. So > hardcoding the value looks more sensible to me. > So for reserving a SPI for vpl011, should I reserve one bit in the vgic.allocated_irqs bitmap in domain_vgic_init(), say 988, for vpl011? I think if I am going to reserve 1 SPI for vpl011 then I need not bump up nr_spis by 1. >> + >> + /* >> + * register mmio handler >> + */ >> + register_mmio_handler (d, &vpl011_mmio_handler, GUEST_PL011_BASE, >> GUEST_PL011_SIZE, NULL); > > > Coding style. No space between the function name and (. > Ok. I will remove the space. >> + >> + /* >> + * initialize the lock >> + */ >> + spin_lock_init(&d->arch.vpl011.lock); >> + >> + /* >> + * clear the flag, control and interrupt status registers >> + */ >> + d->arch.vpl011.control = 0; >> + d->arch.vpl011.flag = 0; >> + d->arch.vpl011.intr_mask = 0; >> + d->arch.vpl011.intr_clear = 0; >> + d->arch.vpl011.raw_intr_status = 0; >> + d->arch.vpl011.masked_intr_status = 0; > > > > The domain structure is already zeroed. So no need to 0 it again. > Ok. >> + >> + return 0; >> +} > > > Missing e-macs magic here. > >> diff --git a/xen/arch/arm/vpl011.h b/xen/arch/arm/vpl011.h >> new file mode 100644 >> index 0000000..f2c577f >> --- /dev/null >> +++ b/xen/arch/arm/vpl011.h > > > Header should live in include. > I will move vpl011.h to xen/include/xen. > [...] > >> +#ifndef _VPL011_H_ >> + >> +#define _VPL011_H_ >> + >> +/* >> + * register offsets >> + */ > > > We already define the PL011 register in include/asm-arm/pl011-uart.h. Please > re-use them rather than re-inventing the wheel. > >> +#define VPL011_UARTDR_OFFSET 0x0 // data register (RW) >> +#define VPL011_UARTRSR_OFFSET 0x4 // receive status and error clear >> register (RW) >> +#define VPL011_UARTFR_OFFSET 0x18 // flag register (RO) >> +#define VPL011_UARTRIS_OFFSET 0x3c // raw interrupt status register >> (RO) >> +#define VPL011_UARTMIS_OFFSET 0x40 // masked interrupt status register >> (RO) >> +#define VPL011_UARTIMSC_OFFSET 0x38 // interrupt mask set/clear register >> (RW) >> +#define VPL011_UARTICR_OFFSET 0x44 // interrupt clear register (WO) >> +#define VPL011_UARTCR_OFFSET 0x30 // uart control register >> +#define VPL011_UARTDMACR_OFFSET 0x48 // uart dma control register > > I will reuse the pl011-uart definitions. > > [...] > >> +#define MASK_VPL011CONS_IDX(idx, ring) ((idx) & (sizeof(ring)-1)) >> +struct console_interface { >> + char in[1024]; >> + char out[2048]; >> + uint32_t in_cons, in_prod; >> + uint32_t out_cons, out_prod; >> +}; >> +#endif > > > The communication between Xen and the console backend is based on the PV > console ring. It would be easier to re-use it rather than recreating one. > I will reuse the ring definition. >> diff --git a/xen/common/Kconfig b/xen/common/Kconfig >> index f2ecbc4..7e2feac 100644 >> --- a/xen/common/Kconfig >> +++ b/xen/common/Kconfig >> @@ -237,4 +237,10 @@ config FAST_SYMBOL_LOOKUP >> The only user of this is Live patching. >> >> If unsure, say Y. >> + >> +config VPL011_CONSOLE >> + bool "Emulated pl011 console support" >> + default y >> + ---help--- >> + Allows a guest to use pl011 UART as a console >> endmenu > > > This should go in arch/arm/Kconfig > Ok. >> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h >> index 2d6fbb1..ff2403a 100644 >> --- a/xen/include/asm-arm/domain.h >> +++ b/xen/include/asm-arm/domain.h >> @@ -40,6 +40,7 @@ struct vtimer { >> uint64_t cval; >> }; >> >> + > > > Spurious line. > >> struct arch_domain >> { >> #ifdef CONFIG_ARM_64 >> @@ -131,6 +132,20 @@ struct arch_domain >> struct { >> uint8_t privileged_call_enabled : 1; >> } monitor; >> + >> +#ifdef CONFIG_VPL011_CONSOLE >> + struct vpl011 { >> + void *ring_buf; >> + struct page_info *ring_page; >> + uint32_t flag; /* flag register */ >> + uint32_t control; /* control register */ >> + uint32_t intr_mask; /* interrupt mask register*/ >> + uint32_t intr_clear; /* interrupt clear register */ >> + uint32_t raw_intr_status; /* raw interrupt status register >> */ >> + uint32_t masked_intr_status; /* masked interrupt register */ >> + spinlock_t lock; >> + } vpl011; >> +#endif > > > I think the structure should be defined in vpl011.h. > I will move the definition to vpl011.h. >> } __cacheline_aligned; >> >> struct arch_vcpu >> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h >> index bd974fb..1d4548f 100644 >> --- a/xen/include/public/arch-arm.h >> +++ b/xen/include/public/arch-arm.h >> @@ -410,6 +410,10 @@ typedef uint64_t xen_callback_t; >> #define GUEST_ACPI_BASE 0x20000000ULL >> #define GUEST_ACPI_SIZE 0x02000000ULL >> >> +/* PL011 mappings */ >> +#define GUEST_PL011_BASE 0x22000000ULL >> +#define GUEST_PL011_SIZE 0x00001000ULL >> + >> /* >> * 16MB == 4096 pages reserved for guest to use as a region to map its >> * grant table in. >> @@ -420,6 +424,7 @@ typedef uint64_t xen_callback_t; >> #define GUEST_MAGIC_BASE xen_mk_ullong(0x39000000) >> #define GUEST_MAGIC_SIZE xen_mk_ullong(0x01000000) >> >> + > > > Spurious line. > >> #define GUEST_RAM_BANKS 2 >> >> #define GUEST_RAM0_BASE xen_mk_ullong(0x40000000) /* 3GB of low RAM @ >> 1GB */ >> > > Cheers, > > -- > Julien Grall
Hi Julien, >>>> + break; >>>> + case VPL011_UARTDR_OFFSET: >>>> + vpl011_read_data(v->domain, &ch); >>> >>> >>> >>> Should not you check the return value of vpl011_read_data? Also, what if >>> there is no data? >> >> This condition should not happen because the RX FIFO empty bit would >> be set in the UARTFR register when the last data is read from the ring >> buffer and the the guest is not supposed to issue next read until the >> RX FIFO empty bit is cleared indicating there is more data now. > > > What you describe is how a well-behave guest will interact with the pl011 > emulation. It does not describe what would happen if a misbehaved guest will > read continuously the register. > > I cannot find any things in the spec about what should be the state of the > register if no data is available. So I guess we just need to ensure that we > don't leak in information from the stack. This seems to be addressed by > *data = 0 in "vpl011_read_data". > > Please document it to avoid removing it by mistake in the future. > Ok. I will add a comment. >> >>> >>>> + *r = ch; >>>> + break; >>>> + case VPL011_UARTFR_OFFSET: >>>> + *r = v->domain->arch.vpl011.flag; >>> >>> >>> >>> I am fairly surprised that none of this code is actually protected by >>> lock. >>> For instance the update of flag is not atomic. So is it safe? >> >> For reading, I thought no locking was required, as I believe a 32-bit >> value read/write on ARM should be atomic. So if the value is modified >> in some other context while it is being read in another context, the >> reader should see either the old value or the new value. >> For register writes, yes I need to take a lock where I am updating >> certain bits in the register. I will add the locking there. > > > Fair point. I was more worry about ordering between read/write between > multiple vCPU. But I guess we don't care if the read does not return an > update to date value. > >> >>> >>>> + break; >>>> + case VPL011_UARTIMSC_OFFSET: >>>> + *r = v->domain->arch.vpl011.intr_mask; >>>> + break; >>>> + case VPL011_UARTICR_OFFSET: >>>> + *r = 0; >>> >>> >>> >>> Looking at the spec, this register is write-only. So why do you implement >>> RAZ? >> >> In such cases, where the guest tries to write to RO register or tries >> to read a WO register, should I send a abort to the guest? > > > I don't see any behavior requirement in the spec. So I would send an abort > to the guest (e.g return 0 in the function). > Ok. >>> >>>> + break; >>>> + case VPL011_UARTRIS_OFFSET: >>>> + *r = v->domain->arch.vpl011.raw_intr_status; >>>> + break; >>>> + case VPL011_UARTMIS_OFFSET: >>>> + *r = v->domain->arch.vpl011.raw_intr_status & >>>> + v->domain->arch.vpl011.intr_mask; >>>> + break; >>>> + case VPL011_UARTDMACR_OFFSET: >>>> + *r = 0; /* uart DMA is not supported. Here it always >>>> returns >>>> 0 */ >>> >>> >>> >>> My understanding of the spec is DMA is not optional. So what would happen >>> if >>> the guest tries to enable it? >>> >>>> + break; >>>> + case VPL011_UARTRSR_OFFSET: >>>> + *r = 0; /* it always returns 0 as there are no physical >>>> errors */ >>> >>> >>> >>> This register contains contains the bit OE that tells whether the FIFO is >>> full or not. The FIFO here is the PV ring, so maybe we should set this >>> bit >>> if the ring is full. >> >> The OE condition will not happen in this case since xenconsole will >> not write more data to the guest if the ring buffer is full. There is >> a separate UARTFR status bit which indicates whether the ring buffer >> is full. > > > Sorry I still don't get it. Xenconsole will likely have to hold characters > that are not written in the PV console. I would have expected that this > would be used to set the OE bit. Did I miss anything? I think OE bit is more applicable to physical UART, where data may arrive while the hardware RX FIFO is already full. In that case, it has to drop the incoming data but it also sets the OE bit to indicate that overflow condition happened. In our case, since the ring buffer acts as a flow control mechanism (i.e. if ring buffer is full then xenconsole stops writing more data and hold the data) this condition may not happen. The pl011 emulation code does not know if xenconsole is holding the data when the ring buffer is full. One thing we could do is to treat the ring buffer full condition as an indication that overflow is likely to happen and set this bit. Typically, the data rate from xenconsole to the guest would be very low for this condition to ever happen. > >>> >>>> + break; >>>> + default: >>>> + printk ("vpl011_mmio_read: invalid switch case %d\n", >>>> (int)(info->gpa - GUEST_PL011_BASE)); >>> >>> >>> >>> Coding style: printk(...). >>> >>> Also, printk is not ratelimited by default. Please use gprintk(...) which >>> will be ratelimited and print the domain information. This is useful when >>> you have multiple guest. >>> >> Replaced printk with gprintk. >> >>>> + break; >>>> + } >>>> + >>>> + return VPL011_EMUL_OK; >>> >>> >>> >>> Please use plain value as the return is not pl011 specific. Also, I am a >>> bit >>> surprised that you return "ok" even when a register as not been emulated. >>> IHMO, a data abort should be sent to the guest. >> >> >> Corrected this. Now it returns an error incase the register is not >> emulated . For the return values, I see that typically values 1/0 are >> returned like in vgic-v2/3.c. Are there some common macros which I can >> use? > > > No. I want to replace 0/1 by false/true but I never had the time to do the > work. > I will use true/false. Regards, Bhupinder
Hi Bhupinder, On 23/03/17 09:44, Bhupinder Thakur wrote: >>>> >>>>> + break; >>>>> + case VPL011_UARTRIS_OFFSET: >>>>> + *r = v->domain->arch.vpl011.raw_intr_status; >>>>> + break; >>>>> + case VPL011_UARTMIS_OFFSET: >>>>> + *r = v->domain->arch.vpl011.raw_intr_status & >>>>> + v->domain->arch.vpl011.intr_mask; >>>>> + break; >>>>> + case VPL011_UARTDMACR_OFFSET: >>>>> + *r = 0; /* uart DMA is not supported. Here it always >>>>> returns >>>>> 0 */ >>>> >>>> >>>> >>>> My understanding of the spec is DMA is not optional. So what would happen >>>> if >>>> the guest tries to enable it? >>>> >>>>> + break; >>>>> + case VPL011_UARTRSR_OFFSET: >>>>> + *r = 0; /* it always returns 0 as there are no physical >>>>> errors */ >>>> >>>> >>>> >>>> This register contains contains the bit OE that tells whether the FIFO is >>>> full or not. The FIFO here is the PV ring, so maybe we should set this >>>> bit >>>> if the ring is full. >>> >>> The OE condition will not happen in this case since xenconsole will >>> not write more data to the guest if the ring buffer is full. There is >>> a separate UARTFR status bit which indicates whether the ring buffer >>> is full. >> >> >> Sorry I still don't get it. Xenconsole will likely have to hold characters >> that are not written in the PV console. I would have expected that this >> would be used to set the OE bit. Did I miss anything? > I think OE bit is more applicable to physical UART, where data may > arrive while the hardware RX FIFO is already full. From my understanding, the ring is the RX FIFO and xenconsole will act of the "user" of the hardware. Hence why I though OE should be set if the ring is full. Anyway, both point of views are valid, you should just document what has been chosen. >> >>>> >>>>> + break; >>>>> + default: >>>>> + printk ("vpl011_mmio_read: invalid switch case %d\n", >>>>> (int)(info->gpa - GUEST_PL011_BASE)); >>>> >>>> >>>> >>>> Coding style: printk(...). >>>> >>>> Also, printk is not ratelimited by default. Please use gprintk(...) which >>>> will be ratelimited and print the domain information. This is useful when >>>> you have multiple guest. >>>> >>> Replaced printk with gprintk. >>> >>>>> + break; >>>>> + } >>>>> + >>>>> + return VPL011_EMUL_OK; >>>> >>>> >>>> >>>> Please use plain value as the return is not pl011 specific. Also, I am a >>>> bit >>>> surprised that you return "ok" even when a register as not been emulated. >>>> IHMO, a data abort should be sent to the guest. >>> >>> >>> Corrected this. Now it returns an error incase the register is not >>> emulated . For the return values, I see that typically values 1/0 are >>> returned like in vgic-v2/3.c. Are there some common macros which I can >>> use? >> >> >> No. I want to replace 0/1 by false/true but I never had the time to do the >> work. >> > I will use true/false. It is not what I said. We should not mix true/false with int. My point was that I have an action to switch the return type for int to bool. For the time being we should use 0/1. Cheers,
On 23/03/17 09:14, Bhupinder Thakur wrote: > .Hi Julien, Hi Bhupinder, > On 5 March 2017 at 17:42, Julien Grall <julien.grall@arm.com> wrote: >> Hi Bhupinder, >> >> On 21/02/17 11:25, Bhupinder Thakur wrote: >>> >>> diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c >> >> Lastly, what if vpl011_write_data is returning an error? >> > Normally vpl011_write_data() should not fail since the guest should > stop writing more data once the ring buffer goes full and TXFF bit is > set in UARTFR. > So there should always be space in the ring buffer for the next data > when a mmio write happens. > > If the guest still writes when ring buffer is already full then data > would be lost. If the return code is meaningless, then the function should be void. Also, the behavior should be documented. [...] >>> +{ >>> + int rc=0; >>> + >>> + /* >>> + * register xen event channel >>> + */ >>> + rc = alloc_unbound_xen_event_channel(d, 0, >>> current->domain->domain_id, >>> + >>> vpl011_notification); >> >> >> This line looks wrong to me. The console backend may not live in the same >> domain as the toolstack. > > How should I figure out the correct domain where xenconsoled is > running? I think you would to pass the console domain id from the toolstack (see console->backend_domid). >> >>> + if (rc < 0) >>> + { >>> + printk ("Failed to allocate vpl011 event channel\n"); >>> + return rc; >>> + } >>> + d->arch.hvm_domain.params[HVM_PARAM_VPL011_CONSOLE_EVTCHN] = rc; >>> + >>> + /* >>> + * allocate an SPI VIRQ for the guest >>> + */ >>> + d->arch.hvm_domain.params[HVM_PARAM_VPL011_VIRQ] = >>> vgic_allocate_spi(d); >> >> >> It makes little sense to hardcode the MMIO region and not the SPI. Also, I >> would rather avoid to introduce new HVM_PARAM when not necessary. So >> hardcoding the value looks more sensible to me. >> > So for reserving a SPI for vpl011, should I reserve one bit in the > vgic.allocated_irqs bitmap in domain_vgic_init(), say 988, for vpl011? > I think if I am going to reserve 1 SPI for vpl011 then I need not bump > up nr_spis by 1. Regardless the solution solution, nr_spis still need to be incremented. This field is used to know the number of SPIs exposed to the guest. Cheers,
Hi Julien, On 23 March 2017 at 19:46, Julien Grall <julien.grall@arm.com> wrote: >>> It makes little sense to hardcode the MMIO region and not the SPI. Also, >>> I >>> would rather avoid to introduce new HVM_PARAM when not necessary. So >>> hardcoding the value looks more sensible to me. >>> >> So for reserving a SPI for vpl011, should I reserve one bit in the >> vgic.allocated_irqs bitmap in domain_vgic_init(), say 988, for vpl011? >> I think if I am going to reserve 1 SPI for vpl011 then I need not bump >> up nr_spis by 1. > > > Regardless the solution solution, nr_spis still need to be incremented. This > field is used to know the number of SPIs exposed to the guest. The d->arch.vgic.allocated_irqs size is allocated based on the nr_spis and the nr_spis is set to highest numbered irq requested by the user+1 by the toolstack (see libxl__arch_domain_prepare_config). There are two possible values which we can reserve for vpl011 SPI: 1. Reserve one SPI for vpl011 at the end of the SPI range to 988 (1020-32), then nr_spis will have to be always set to 989. 2. Reserve one at the beginning of the range say 0 (i.e. virq number 32). In this case nr_spis will be set to 1. I think 2nd option is better as it wastes less memory for maintaining the irq bitmap data structures. Let me know. Regards, Bhupinder
diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile index 7afb8a3..a94bdab 100644 --- a/xen/arch/arm/Makefile +++ b/xen/arch/arm/Makefile @@ -49,6 +49,7 @@ obj-y += vm_event.o obj-y += vtimer.o obj-y += vpsci.o obj-y += vuart.o +obj-$(CONFIG_VPL011_CONSOLE) += vpl011.o #obj-bin-y += ....o diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c new file mode 100644 index 0000000..88ba968 --- /dev/null +++ b/xen/arch/arm/vpl011.c @@ -0,0 +1,366 @@ +/* + * arch/arm/vpl011.c + * + * Virtual PL011 UART + * + * This program is free software; you can redistribute it and/or modify it + * under the terms and conditions of the GNU General Public License, + * version 2, as published by the Free Software Foundation. + * + * This program is distributed in the hope it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for + * more details. + * + * You should have received a copy of the GNU General Public License along with + * this program; If not, see <http://www.gnu.org/licenses/>. + */ + +#include <xen/config.h> +#include <xen/init.h> +#include <xen/lib.h> +#include <xen/errno.h> +#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> + +#include <public/xen.h> +#include <public/hvm/params.h> +#include <public/hvm/hvm_op.h> + +#include <asm/hypercall.h> +#include "vpl011.h" + +static int vpl011_mmio_read(struct vcpu *v, mmio_info_t *info, register_t *r, void *priv) +{ + unsigned char ch; + + switch (info->gpa - GUEST_PL011_BASE) + { + case VPL011_UARTCR_OFFSET: + *r = v->domain->arch.vpl011.control; + break; + case VPL011_UARTDR_OFFSET: + vpl011_read_data(v->domain, &ch); + *r = ch; + break; + case VPL011_UARTFR_OFFSET: + *r = v->domain->arch.vpl011.flag; + break; + case VPL011_UARTIMSC_OFFSET: + *r = v->domain->arch.vpl011.intr_mask; + break; + case VPL011_UARTICR_OFFSET: + *r = 0; + break; + case VPL011_UARTRIS_OFFSET: + *r = v->domain->arch.vpl011.raw_intr_status; + break; + case VPL011_UARTMIS_OFFSET: + *r = v->domain->arch.vpl011.raw_intr_status & + v->domain->arch.vpl011.intr_mask; + break; + case VPL011_UARTDMACR_OFFSET: + *r = 0; /* uart DMA is not supported. Here it always returns 0 */ + break; + case VPL011_UARTRSR_OFFSET: + *r = 0; /* it always returns 0 as there are no physical errors */ + break; + default: + printk ("vpl011_mmio_read: invalid switch case %d\n", (int)(info->gpa - GUEST_PL011_BASE)); + break; + } + + return VPL011_EMUL_OK; +} + +static int vpl011_mmio_write(struct vcpu *v, mmio_info_t *info, register_t r, void *priv) +{ + unsigned char ch = r; + + switch (info->gpa - GUEST_PL011_BASE) + { + case VPL011_UARTCR_OFFSET: + v->domain->arch.vpl011.control = r; + break; + case VPL011_UARTDR_OFFSET: + vpl011_write_data(v->domain, ch); + break; + case VPL011_UARTIMSC_OFFSET: + v->domain->arch.vpl011.intr_mask = r; + if ( (v->domain->arch.vpl011.raw_intr_status & v->domain->arch.vpl011.intr_mask) ) + vgic_vcpu_inject_spi(v->domain, (int)v->domain->arch.hvm_domain.params[HVM_PARAM_VPL011_VIRQ]); + break; + case VPL011_UARTICR_OFFSET: + /* + * clear all bits which are set in the input + */ + v->domain->arch.vpl011.raw_intr_status &= ~r; + if ( (v->domain->arch.vpl011.raw_intr_status & v->domain->arch.vpl011.intr_mask) ) + { + vgic_vcpu_inject_spi(v->domain, (int)v->domain->arch.hvm_domain.params[HVM_PARAM_VPL011_VIRQ]); + } + break; + case VPL011_UARTRSR_OFFSET: // nothing to clear + break; + case VPL011_UARTFR_OFFSET: // these are all RO registers + case VPL011_UARTRIS_OFFSET: + case VPL011_UARTMIS_OFFSET: + case VPL011_UARTDMACR_OFFSET: + break; + default: + printk ("vpl011_mmio_write: switch case not handled %d\n", (int)(info->gpa - GUEST_PL011_BASE)); + break; + } + + return VPL011_EMUL_OK; +} + +static const struct mmio_handler_ops vpl011_mmio_handler = { + .read = vpl011_mmio_read, + .write = vpl011_mmio_write, +}; + + + +int vpl011_map_guest_page(struct domain *d) +{ + int rc=0; + + /* + * map the guest PFN to Xen address space + */ + rc = prepare_ring_for_helper(d, + d->arch.hvm_domain.params[HVM_PARAM_VPL011_CONSOLE_PFN], + &d->arch.vpl011.ring_page, + (void **)&d->arch.vpl011.ring_buf); + if ( rc < 0 ) + { + printk("Failed to map vpl011 guest PFN\n"); + } + + return rc; +} + +static int vpl011_data_avail(struct domain *d) +{ + int rc=0; + unsigned long flags; + + struct console_interface *intf=(struct console_interface *)d->arch.vpl011.ring_buf; + + VPL011_LOCK(d, flags); + + /*` + * check IN ring buffer + */ + if ( !VPL011_IN_RING_EMPTY(intf) ) + { + /* + * clear the RX FIFO empty flag as the ring is not empty + */ + d->arch.vpl011.flag &= ~(VPL011_UARTFR_RXFE); + + /* + * if the buffer is full then set the RX FIFO FULL flag + */ + if ( VPL011_IN_RING_FULL(intf) ) + d->arch.vpl011.flag |= (VPL011_UARTFR_RXFF); + + /* + * set the RX interrupt status + */ + d->arch.vpl011.raw_intr_status |= (VPL011_UARTRIS_RXRIS); + } + + /* + * check OUT ring buffer + */ + if ( !VPL011_OUT_RING_FULL(intf) ) + { + /* + * if the buffer is not full then clear the TX FIFO full flag + */ + d->arch.vpl011.flag &= ~(VPL011_UARTFR_TXFF); + + /* + * set the TX interrupt status + */ + d->arch.vpl011.raw_intr_status |= (VPL011_UARTRIS_TXRIS); + + if ( VPL011_OUT_RING_EMPTY(intf) ) + { + /* + * clear the uart busy flag and set the TX FIFO empty flag + */ + d->arch.vpl011.flag &= ~(VPL011_UARTFR_BUSY); + d->arch.vpl011.flag |= (VPL011_UARTFR_TXFE); + } + } + + VPL011_UNLOCK(d, flags); + + /* + * send an interrupt if it is not masked + */ + if ( (d->arch.vpl011.raw_intr_status & d->arch.vpl011.intr_mask) ) + vgic_vcpu_inject_spi(d, (int)d->arch.hvm_domain.params[HVM_PARAM_VPL011_VIRQ]); + + if ( !VPL011_OUT_RING_EMPTY(intf) ) + { + /* + * raise an interrupt to dom0 + */ + rc = raw_evtchn_send(d, + d->arch.hvm_domain.params[HVM_PARAM_VPL011_CONSOLE_EVTCHN], NULL); + + if ( rc < 0 ) + printk("Failed to send vpl011 interrupt to dom0\n"); + } + + return rc; +} + +int vpl011_read_data(struct domain *d, unsigned char *data) +{ + int rc=0; + unsigned long flags; + struct console_interface *intf=(struct console_interface *)d->arch.vpl011.ring_buf; + + *data = 0; + + VPL011_LOCK(d, flags); + + /* + * if there is data in the ring buffer then copy it to the output buffer + */ + if ( !VPL011_IN_RING_EMPTY(intf) ) + { + *data = intf->in[MASK_VPL011CONS_IDX(intf->in_cons++, intf->in)]; + } + + /* + * if the ring buffer is empty then set the RX FIFO empty flag + */ + if ( VPL011_IN_RING_EMPTY(intf) ) + { + d->arch.vpl011.flag |= (VPL011_UARTFR_RXFE); + d->arch.vpl011.raw_intr_status &= ~(VPL011_UARTRIS_RXRIS); + } + + /* + * clear the RX FIFO full flag + */ + d->arch.vpl011.flag &= ~(VPL011_UARTFR_RXFF); + + VPL011_UNLOCK(d, flags); + + return rc; +} + +int vpl011_write_data(struct domain *d, unsigned char data) +{ + int rc=0; + unsigned long flags; + struct console_interface *intf=(struct console_interface *)d->arch.vpl011.ring_buf; + + VPL011_LOCK(d, flags); + + /* + * if there is space in the ring buffer then write the data + */ + if ( !VPL011_OUT_RING_FULL(intf) ) + { + intf->out[MASK_VPL011CONS_IDX(intf->out_prod++, intf->out)] = data; + smp_wmb(); + } + + /* + * if there is no space in the ring buffer then set the + * TX FIFO FULL flag + */ + if ( VPL011_OUT_RING_FULL(intf) ) + { + d->arch.vpl011.flag |= (VPL011_UARTFR_TXFF); + d->arch.vpl011.raw_intr_status &= ~(VPL011_UARTRIS_TXRIS); + } + + /* + * set the uart busy status + */ + d->arch.vpl011.flag |= (VPL011_UARTFR_BUSY); + + /* + * clear the TX FIFO empty flag + */ + d->arch.vpl011.flag &= ~(VPL011_UARTFR_TXFE); + + VPL011_UNLOCK(d, flags); + + /* + * raise an event to dom0 only if it is the first character in the buffer + */ + if ( VPL011_RING_DEPTH(intf, out) == 1 ) + { + rc = raw_evtchn_send(d, + d->arch.hvm_domain.params[HVM_PARAM_VPL011_CONSOLE_EVTCHN], NULL); + + if ( rc < 0 ) + printk("Failed to send vpl011 interrupt to dom0\n"); + } + + return rc; +} + +static void vpl011_notification(struct vcpu *v, unsigned int port) +{ + vpl011_data_avail(v->domain); +} + +int domain_vpl011_init(struct domain *d) +{ + int rc=0; + + /* + * register xen event channel + */ + rc = alloc_unbound_xen_event_channel(d, 0, current->domain->domain_id, + vpl011_notification); + if (rc < 0) + { + printk ("Failed to allocate vpl011 event channel\n"); + return rc; + } + d->arch.hvm_domain.params[HVM_PARAM_VPL011_CONSOLE_EVTCHN] = rc; + + /* + * allocate an SPI VIRQ for the guest + */ + d->arch.hvm_domain.params[HVM_PARAM_VPL011_VIRQ] = vgic_allocate_spi(d); + + /* + * register mmio handler + */ + register_mmio_handler (d, &vpl011_mmio_handler, GUEST_PL011_BASE, GUEST_PL011_SIZE, NULL); + + /* + * initialize the lock + */ + spin_lock_init(&d->arch.vpl011.lock); + + /* + * clear the flag, control and interrupt status registers + */ + d->arch.vpl011.control = 0; + d->arch.vpl011.flag = 0; + d->arch.vpl011.intr_mask = 0; + d->arch.vpl011.intr_clear = 0; + d->arch.vpl011.raw_intr_status = 0; + d->arch.vpl011.masked_intr_status = 0; + + return 0; +} diff --git a/xen/arch/arm/vpl011.h b/xen/arch/arm/vpl011.h new file mode 100644 index 0000000..f2c577f --- /dev/null +++ b/xen/arch/arm/vpl011.h @@ -0,0 +1,208 @@ +/* + * include/xen/vpl011.h + * + * Virtual PL011 UART + * + * This program is free software; you can redistribute it and/or modify it + * under the terms and conditions of the GNU General Public License, + * version 2, as published by the Free Software Foundation. + * + * This program is distributed in the hope it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for + * more details. + * + * You should have received a copy of the GNU General Public License along with + * this program; If not, see <http://www.gnu.org/licenses/>. + */ + +#ifndef _VPL011_H_ + +#define _VPL011_H_ + +/* + * register offsets + */ +#define VPL011_UARTDR_OFFSET 0x0 // data register (RW) +#define VPL011_UARTRSR_OFFSET 0x4 // receive status and error clear register (RW) +#define VPL011_UARTFR_OFFSET 0x18 // flag register (RO) +#define VPL011_UARTRIS_OFFSET 0x3c // raw interrupt status register (RO) +#define VPL011_UARTMIS_OFFSET 0x40 // masked interrupt status register (RO) +#define VPL011_UARTIMSC_OFFSET 0x38 // interrupt mask set/clear register (RW) +#define VPL011_UARTICR_OFFSET 0x44 // interrupt clear register (WO) +#define VPL011_UARTCR_OFFSET 0x30 // uart control register +#define VPL011_UARTDMACR_OFFSET 0x48 // uart dma control register + +/* + * control register bits - RW + */ +#define VPL011_UARTCR_UARTEN_BIT 0 +#define VPL011_UARTCR_UARTEN (1<<VPL011_UARTCR_UARTEN_BIT) +#define VPL011_UARTCR_TXE_BIT 8 +#define VPL011_UARTCR_TXE (1<<VPL011_UARTCR_TXE_BIT) +#define VPL011_UARTCR_RXE_BIT 9 +#define VPL011_UARTCR_RXE (1<<VPL011_UARTCR_RXE_BIT) + +/* + * Flag register bits - RO + */ +#define VPL011_UARTFR_CTS_BIT 0 // clear to send +#define VPL011_UARTFR_CTS (1<<VPL011_UARTFR_CTS_BIT) +#define VPL011_UARTFR_DSR_BIT 1 // data set ready +#define VPL011_UARTFR_DSR (1<<VPL011_UARTFR_DSR_BIT) +#define VPL011_UARTFR_DCD_BIT 2 // data carrier detect +#define VPL011_UARTFR_DCD (1<<VPL011_UARTFR_DCD_BIT) +#define VPL011_UARTFR_BUSY_BIT 3 // uart busy +#define VPL011_UARTFR_BUSY (1<<VPL011_UARTFR_BUSY_BIT) +#define VPL011_UARTFR_RXFE_BIT 4 // receive fifo empty +#define VPL011_UARTFR_RXFE (1<<VPL011_UARTFR_RXFE_BIT) +#define VPL011_UARTFR_TXFF_BIT 5 // transmit fifo full +#define VPL011_UARTFR_TXFF (1<<VPL011_UARTFR_TXFF_BIT) +#define VPL011_UARTFR_RXFF_BIT 6 // receive fifo full +#define VPL011_UARTFR_RXFF (1<<VPL011_UARTFR_RXFF_BIT) +#define VPL011_UARTFR_TXFE_BIT 7 // transmit fifo empty +#define VPL011_UARTFR_TXFE (1<<VPL011_UARTFR_TXFE_BIT) +#define VPL011_UARTFR_RI_BIT 8 // ring indicator +#define VPL011_UARTFR_RI (1<<VPL011_UARTFR_RI_BIT) + +/* + * UART raw interrupt status bits - RO + */ +#define VPL011_UARTRIS_RIRMIS_BIT 0 +#define VPL011_UARTRIS_RIRMIS (1<<VPL011_UARTRIS_RIRMIS_BIT) +#define VPL011_UARTRIS_CTSRMIS_BIT 1 +#define VPL011_UARTRIS_CTSRMIS (1<<VPL011_UARTRIS_CTSRMIS_BIT) +#define VPL011_UARTRIS_DCDRMIS_BIT 2 +#define VPL011_UARTRIS_DCDRMIS (1<<VPL011_UARTRIS_DCDRMIS_BIT) +#define VPL011_UARTRIS_DSRRMIS_BIT 3 +#define VPL011_UARTRIS_DSRRMIS (1<<VPL011_UARTRIS_DSRRMIS_BIT) +#define VPL011_UARTRIS_RXRIS_BIT 4 +#define VPL011_UARTRIS_RXRIS (1<<VPL011_UARTRIS_RXRIS_BIT) +#define VPL011_UARTRIS_TXRIS_BIT 5 +#define VPL011_UARTRIS_TXRIS (1<<VPL011_UARTRIS_TXRIS_BIT) +#define VPL011_UARTRIS_RTRIS_BIT 6 +#define VPL011_UARTRIS_RTRIS (1<<VPL011_UARTRIS_RTRIS_BIT) +#define VPL011_UARTRIS_FERIS_BIT 7 +#define VPL011_UARTRIS_FERIS (1<<VPL011_UARTRIS_FERIS_BIT) +#define VPL011_UARTRIS_PERIS_BIT 8 +#define VPL011_UARTRIS_PERIS (1<<VPL011_UARTRIS_PERIS_BIT) +#define VPL011_UARTRIS_BERIS_BIT 9 +#define VPL011_UARTRIS_BERIS (1<<VPL011_UARTRIS_BERIS_BIT) +#define VPL011_UARTRIS_OERIS_BIT 10 +#define VPL011_UARTRIS_OERIS (1<<VPL011_UARTRIS_OERIS_BIT) + +/* + * UART masked interrupt status bits - RO + */ +#define VPL011_UARTMIS_RIMMIS_BIT 0 +#define VPL011_UARTMIS_RIMMIS (1<<VPL011_UARTMIS_RIMMIS_BIT) +#define VPL011_UARTMIS_CTSMMIS_BIT 1 +#define VPL011_UARTMIS_CTSMMIS (1<<VPL011_UARTMIS_CTSMMIS_BIT) +#define VPL011_UARTMIS_DCDMMIS_BIT 2 +#define VPL011_UARTMIS_DCDMMIS (1<<VPL011_UARTMIS_DCDMMIS_BIT) +#define VPL011_UARTMIS_DSRMMIS_BIT 3 +#define VPL011_UARTMIS_DSRMMIS (1<<VPL011_UARTMIS_DSRMMIS_BIT) +#define VPL011_UARTMIS_RXMIS_BIT 4 +#define VPL011_UARTMIS_RXMIS (1<<VPL011_UARTMIS_RXMIS_BIT) +#define VPL011_UARTMIS_TXMIS_BIT 5 +#define VPL011_UARTMIS_TXMIS (1<<VPL011_UARTMIS_TXMIS_BIT) +#define VPL011_UARTMIS_RTMIS_BIT 6 +#define VPL011_UARTMIS_RTMIS (1<<VPL011_UARTMIS_RTMIS_BIT) +#define VPL011_UARTMIS_FEMIS_BIT 7 +#define VPL011_UARTMIS_FEMIS (1<<VPL011_UARTMIS_FEMIS_BIT) +#define VPL011_UARTMIS_PEMIS_BIT 8 +#define VPL011_UARTMIS_PEMIS (1<<VPL011_UARTMIS_PEMIS_BIT) +#define VPL011_UARTMIS_BEMIS_BIT 9 +#define VPL011_UARTMIS_BEMIS (1<<VPL011_UARTMIS_BEMIS_BIT) +#define VPL011_UARTMIS_OEMIS_BIT 10 +#define VPL011_UARTMIS_OEMIS (1<<VPL011_UARTMIS_OEMIS_BIT) + +/* + * UART interrupt clear bits - WO + */ +#define VPL011_UARTICR_RIMIC_BIT 0 +#define VPL011_UARTICR_RIMIC (1<<VPL011_UARTICR_RIMIC_BIT) +#define VPL011_UARTICR_CTSMIC_BIT 1 +#define VPL011_UARTICR_CTSMIC (1<<VPL011_UARTICR_CTSMIC_BIT) +#define VPL011_UARTICR_DCDMIC_BIT 2 +#define VPL011_UARTICR_DCDMIC (1<<VPL011_UARTICR_DCDMIC_BIT) +#define VPL011_UARTICR_DSRMIC_BIT 3 +#define VPL011_UARTICR_DSRMIC (1<<VPL011_UARTICR_DSRMIC_BIT) +#define VPL011_UARTICR_RXIC_BIT 4 +#define VPL011_UARTICR_RXIC (1<<VPL011_UARTICR_RXIC_BIT) +#define VPL011_UARTICR_TXIC_BIT 5 +#define VPL011_UARTICR_TXIC (1<<VPL011_UARTICR_TXIC_BIT) +#define VPL011_UARTICR_RTIC_BIT 6 +#define VPL011_UARTICR_RTIC (1<<VPL011_UARTICR_RTIC_BIT) +#define VPL011_UARTICR_FEIC_BIT 7 +#define VPL011_UARTICR_FEIC (1<<VPL011_UARTICR_FEIC_BIT) +#define VPL011_UARTICR_PEIC_BIT 8 +#define VPL011_UARTICR_PEIC (1<<VPL011_UARTICR_PEIC_BIT) +#define VPL011_UARTICR_BEIC_BIT 9 +#define VPL011_UARTICR_BEIC (1<<VPL011_UARTICR_BEIC_BIT) +#define VPL011_UARTICR_OEIC_BIT 10 +#define VPL011_UARTICR_OEIC (1<<VPL011_UARTICR_OEIC_BIT) + +/* + * UART interrupt mask set/clear bits - RW + */ +#define VPL011_UARTIMSC_RIMIM_BIT 0 +#define VPL011_UARTIMSC_RIMIM (1<<VPL011_UARTIMSC_RIMIM_BIT) +#define VPL011_UARTIMSC_CTSMIM_BIT 1 +#define VPL011_UARTIMSC_CTSMIM (1<<VPL011_UARTIMSC_CTSMIM_BIT) +#define VPL011_UARTIMSC_DCDMIM_BIT 2 +#define VPL011_UARTIMSC_DCDMIM (1<<VPL011_UARTIMSC_DCDMIM_BIT) +#define VPL011_UARTIMSC_DSRMIM_BIT 3 +#define VPL011_UARTIMSC_DSRMIM (1<<VPL011_UARTIMSC_DSRMIM_BIT) +#define VPL011_UARTIMSC_RXIM_BIT 4 +#define VPL011_UARTIMSC_RXIM (1<<VPL011_UARTIMSC_RXIM_BIT) +#define VPL011_UARTIMSC_TXIM_BIT 5 +#define VPL011_UARTIMSC_TXIM (1<<VPL011_UARTIMSC_TXIM_BIT) +#define VPL011_UARTIMSC_RTIM_BIT 6 +#define VPL011_UARTIMSC_RTIM (1<<VPL011_UARTIMSC_RTIM_BIT) +#define VPL011_UARTIMSC_FEIM_BIT 7 +#define VPL011_UARTIMSC_FEIM (1<<VPL011_UARTIMSC_FEIM_BIT) +#define VPL011_UARTIMSC_PEIM_BIT 8 +#define VPL011_UARTIMSC_PEIM (1<<VPL011_UARTIMSC_PEIM_BIT) +#define VPL011_UARTIMSC_BEIM_BIT 9 +#define VPL011_UARTIMSC_BEIM (1<<VPL011_UARTIMSC_BEIM_BIT) +#define VPL011_UARTIMSC_OEIM_BIT 10 +#define VPL011_UARTIMSC_OEIM (1<<VPL011_UARTIMSC_OEIM_BIT) + + +/* + * helper macros + */ +#define VPL011_RING_DEPTH(intf,dir) (((intf)->dir ## _prod - (intf)->dir ## _cons)) +#define VPL011_RING_MAX_DEPTH(intf,dir) (sizeof((intf)->dir)-1) + +#define VPL011_IN_RING_EMPTY(intf) (VPL011_RING_DEPTH(intf, in) == 0) + +#define VPL011_OUT_RING_EMPTY(intf) (VPL011_RING_DEPTH(intf, out) == 0) + +#define VPL011_IN_RING_FULL(intf) (VPL011_RING_DEPTH(intf, in) == VPL011_RING_MAX_DEPTH(intf, in)) + +#define VPL011_OUT_RING_FULL(intf) (VPL011_RING_DEPTH(intf, out) == VPL011_RING_MAX_DEPTH(intf,out)) + +#define VPL011_LOCK(d,flags) spin_lock_irqsave(&(d)->arch.vpl011.lock, flags) +#define VPL011_UNLOCK(d,flags) spin_unlock_irqrestore(&(d)->arch.vpl011.lock, flags) + +/* + * MMIO return values + */ +#define VPL011_EMUL_OK 1 +#define VPL011_EMUL_FAIL 0 + +int domain_vpl011_init(struct domain *d); +int vpl011_map_guest_page(struct domain *d); +int vpl011_read_data(struct domain *d, unsigned char *data); +int vpl011_write_data(struct domain *d, unsigned char data); + +#define MASK_VPL011CONS_IDX(idx, ring) ((idx) & (sizeof(ring)-1)) +struct console_interface { + char in[1024]; + char out[2048]; + uint32_t in_cons, in_prod; + uint32_t out_cons, out_prod; +}; +#endif diff --git a/xen/common/Kconfig b/xen/common/Kconfig index f2ecbc4..7e2feac 100644 --- a/xen/common/Kconfig +++ b/xen/common/Kconfig @@ -237,4 +237,10 @@ config FAST_SYMBOL_LOOKUP The only user of this is Live patching. If unsure, say Y. + +config VPL011_CONSOLE + bool "Emulated pl011 console support" + default y + ---help--- + Allows a guest to use pl011 UART as a console endmenu diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h index 2d6fbb1..ff2403a 100644 --- a/xen/include/asm-arm/domain.h +++ b/xen/include/asm-arm/domain.h @@ -40,6 +40,7 @@ struct vtimer { uint64_t cval; }; + struct arch_domain { #ifdef CONFIG_ARM_64 @@ -131,6 +132,20 @@ struct arch_domain struct { uint8_t privileged_call_enabled : 1; } monitor; + +#ifdef CONFIG_VPL011_CONSOLE + struct vpl011 { + void *ring_buf; + struct page_info *ring_page; + uint32_t flag; /* flag register */ + uint32_t control; /* control register */ + uint32_t intr_mask; /* interrupt mask register*/ + uint32_t intr_clear; /* interrupt clear register */ + uint32_t raw_intr_status; /* raw interrupt status register */ + uint32_t masked_intr_status; /* masked interrupt register */ + spinlock_t lock; + } vpl011; +#endif } __cacheline_aligned; struct arch_vcpu diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h index bd974fb..1d4548f 100644 --- a/xen/include/public/arch-arm.h +++ b/xen/include/public/arch-arm.h @@ -410,6 +410,10 @@ typedef uint64_t xen_callback_t; #define GUEST_ACPI_BASE 0x20000000ULL #define GUEST_ACPI_SIZE 0x02000000ULL +/* PL011 mappings */ +#define GUEST_PL011_BASE 0x22000000ULL +#define GUEST_PL011_SIZE 0x00001000ULL + /* * 16MB == 4096 pages reserved for guest to use as a region to map its * grant table in. @@ -420,6 +424,7 @@ typedef uint64_t xen_callback_t; #define GUEST_MAGIC_BASE xen_mk_ullong(0x39000000) #define GUEST_MAGIC_SIZE xen_mk_ullong(0x01000000) + #define GUEST_RAM_BANKS 2 #define GUEST_RAM0_BASE xen_mk_ullong(0x40000000) /* 3GB of low RAM @ 1GB */
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 Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org> --- xen/arch/arm/Makefile | 1 + xen/arch/arm/vpl011.c | 366 ++++++++++++++++++++++++++++++++++++++++++ xen/arch/arm/vpl011.h | 208 ++++++++++++++++++++++++ xen/common/Kconfig | 6 + xen/include/asm-arm/domain.h | 15 ++ xen/include/public/arch-arm.h | 5 + 6 files changed, 601 insertions(+) create mode 100644 xen/arch/arm/vpl011.c create mode 100644 xen/arch/arm/vpl011.h