[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 01/11] xen/arm: vpl011: Add pl011 uart emulation in Xen
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, -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx https://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |