|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v5 22/25] xen/arm: Allow vpl011 to be used by DomU
On Mon, Oct 29, 2018 at 10:03 PM Stefano Stabellini
<sstabellini@xxxxxxxxxx> wrote:
>
> On Wed, 24 Oct 2018, Oleksandr Tyshchenko wrote:
> > Hi, Stefano
> >
> > On Tue, Oct 23, 2018 at 5:04 AM Stefano Stabellini
> > <sstabellini@xxxxxxxxxx> wrote:
> > >
> > > Make vpl011 being able to be used without a userspace component in Dom0.
> > > In that case, output is printed to the Xen serial and input is received
> > > from the Xen serial one character at a time.
> > >
> > > Call domain_vpl011_init during construct_domU if vpl011 is enabled.
> > >
> > > Introduce a new ring struct with only the ring array to avoid a waste of
> > > memory. Introduce separate read_data and write_data functions for
> > > initial domains: vpl011_write_data_xen is very simple and just writes
> > > to the console, while vpl011_read_data_xen is a duplicate of
> > > vpl011_read_data. Although textually almost identical, we are forced to
> > > duplicate the functions because the struct layout is different.
> > >
> > > Output characters are printed one by one, potentially leading to
> > > intermixed output of different domains on the console. A follow-up patch
> > > will solve the issue by introducing buffering.
> > >
> > > Signed-off-by: Stefano Stabellini <stefanos@xxxxxxxxxx>
> > > Acked-by: Julien Grall <julien.grall@xxxxxxx>
> > > ---
> > > Changes in v5:
> > > - renable call to vpl011_rx_char_xen from console.c
> > >
> > > Changes in v4:
> > > - code style
> > >
> > > Changes in v3:
> > > - add in-code comments
> > > - improve existing comments
> > > - remove ifdef around domain_vpl011_init in construct_domU
> > > - add ASSERT
> > > - use SBSA_UART_FIFO_SIZE for in buffer size
> > > - rename ring_enable to backend_in_domain
> > > - rename struct xencons_in to struct vpl011_xen_backend
> > > - rename inring field to xen
> > > - rename helper functions accordingly
> > > - remove unnecessary stub implementation of vpl011_rx_char
> > > - move vpl011_rx_char_xen within the file to avoid the need of a forward
> > > declaration of vpl011_data_avail
> > > - fix small bug in vpl011_rx_char_xen: increment in_prod before using it
> > > to check xencons_queued.
> > >
> > > Changes in v2:
> > > - only init if vpl011
> > > - rename vpl011_read_char to vpl011_rx_char
> > > - remove spurious change
> > > - fix coding style
> > > - use different ring struct
> > > - move the write_data changes to their own function
> > > (vpl011_write_data_noring)
> > > - duplicate vpl011_read_data
> > > ---
> > > xen/arch/arm/domain_build.c | 9 +-
> > > xen/arch/arm/vpl011.c | 200
> > > +++++++++++++++++++++++++++++++++++++------
> > > xen/drivers/char/console.c | 2 +-
> > > xen/include/asm-arm/vpl011.h | 8 ++
> > > 4 files changed, 193 insertions(+), 26 deletions(-)
> > >
> > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > > index 6026b77..9ffd919 100644
> > > --- a/xen/arch/arm/domain_build.c
> > > +++ b/xen/arch/arm/domain_build.c
> > > @@ -2629,7 +2629,14 @@ static int __init construct_domU(struct domain *d,
> > > if ( rc < 0 )
> > > return rc;
> > >
> > > - return construct_domain(d, &kinfo);
> > > + rc = construct_domain(d, &kinfo);
> > > + if ( rc < 0 )
> > > + return rc;
> > > +
> > > + if ( kinfo.vpl011 )
> > > + rc = domain_vpl011_init(d, NULL);
> > > +
> > > + return rc;
> > > }
> > >
> > > void __init create_domUs(void)
> > > diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
> > > index 68452a8..131507e 100644
> > > --- a/xen/arch/arm/vpl011.c
> > > +++ b/xen/arch/arm/vpl011.c
> > > @@ -77,6 +77,91 @@ static void vpl011_update_interrupt_status(struct
> > > domain *d)
> > > #endif
> > > }
> > >
> > > +/*
> > > + * vpl011_write_data_xen writes chars from the vpl011 out buffer to the
> > > + * console. Only to be used when the backend is Xen.
> > > + */
> > > +static void vpl011_write_data_xen(struct domain *d, uint8_t data)
> > > +{
> > > + unsigned long flags;
> > > + struct vpl011 *vpl011 = &d->arch.vpl011;
> > > +
> > > + VPL011_LOCK(d, flags);
> > > +
> > > + printk("%c", data);
> > > + if (data == '\n')
> > > + printk("DOM%u: ", d->domain_id);
> > > +
> > > + vpl011->uartris |= TXI;
> > > + vpl011->uartfr &= ~TXFE;
> > > + vpl011_update_interrupt_status(d);
> > > +
> > > + VPL011_UNLOCK(d, flags);
> > > +}
> > > +
> > > +/*
> > > + * vpl011_read_data_xen reads data when the backend is xen. Characters
> > > + * are added to the vpl011 receive buffer by vpl011_rx_char_xen.
> > > + */
> > > +static uint8_t vpl011_read_data_xen(struct domain *d)
> > > +{
> > > + unsigned long flags;
> > > + uint8_t data = 0;
> > > + struct vpl011 *vpl011 = &d->arch.vpl011;
> > > + struct vpl011_xen_backend *intf = vpl011->backend.xen;
> > > + XENCONS_RING_IDX in_cons, in_prod;
> > > +
> > > + VPL011_LOCK(d, flags);
> > > +
> > > + in_cons = intf->in_cons;
> > > + in_prod = intf->in_prod;
> > > +
> > > + smp_rmb();
> > > +
> > > + /*
> > > + * It is expected that there will be data in the ring buffer when
> > > this
> > > + * function is called since the guest is expected to read the data
> > > register
> > > + * only if the TXFE flag is not set.
> > > + * If the guest still does read when TXFE bit is set then 0 will be
> > > returned.
> > > + */
> > > + if ( xencons_queued(in_prod, in_cons, sizeof(intf->in)) > 0 )
> > > + {
> > > + unsigned int fifo_level;
> > > +
> > > + data = intf->in[xencons_mask(in_cons, sizeof(intf->in))];
> > > + in_cons += 1;
> > > + smp_mb();
> > > + intf->in_cons = in_cons;
> > > +
> > > + fifo_level = xencons_queued(in_prod, in_cons, sizeof(intf->in));
> > > +
> > > + /* If the FIFO is now empty, we clear the receive timeout
> > > interrupt. */
> > > + if ( fifo_level == 0 )
> > > + {
> > > + vpl011->uartfr |= RXFE;
> > > + vpl011->uartris &= ~RTI;
> > > + }
> > > +
> > > + /* If the FIFO is more than half empty, we clear the RX
> > > interrupt. */
> > > + if ( fifo_level < sizeof(intf->in) - SBSA_UART_FIFO_LEVEL )
> > > + vpl011->uartris &= ~RXI;
> > > +
> > > + vpl011_update_interrupt_status(d);
> > > + }
> > > + else
> > > + gprintk(XENLOG_ERR, "vpl011: Unexpected IN ring buffer empty\n");
> > > +
> > > + /*
> > > + * We have consumed a character or the FIFO was empty, so clear the
> > > + * "FIFO full" bit.
> > > + */
> > > + vpl011->uartfr &= ~RXFF;
> > > +
> > > + VPL011_UNLOCK(d, flags);
> > > +
> > > + return data;
> > > +}
> > > +
> > > static uint8_t vpl011_read_data(struct domain *d)
> > > {
> > > unsigned long flags;
> > > @@ -240,7 +325,10 @@ static int vpl011_mmio_read(struct vcpu *v,
> > > case DR:
> > > if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;
> > >
> > > - *r = vreg_reg32_extract(vpl011_read_data(d), info);
> > > + if ( vpl011->backend_in_domain )
> > > + *r = vreg_reg32_extract(vpl011_read_data(d), info);
> > > + else
> > > + *r = vreg_reg32_extract(vpl011_read_data_xen(d), info);
> > > return 1;
> > >
> > > case RSR:
> > > @@ -325,7 +413,10 @@ static int vpl011_mmio_write(struct vcpu *v,
> > >
> > > vreg_reg32_update(&data, r, info);
> > > data &= 0xFF;
> > > - vpl011_write_data(v->domain, data);
> > > + if ( vpl011->backend_in_domain )
> > > + vpl011_write_data(v->domain, data);
> > > + else
> > > + vpl011_write_data_xen(v->domain, data);
> > > return 1;
> > > }
> > >
> > > @@ -430,6 +521,39 @@ static void vpl011_data_avail(struct domain *d,
> > > vpl011->uartfr |= TXFE;
> > > }
> > >
> > > +/*
> > > + * vpl011_rx_char_xen adds a char to a domain's vpl011 receive buffer.
> > > + * It is only used when the vpl011 backend is in Xen.
> > > + */
> > > +void vpl011_rx_char_xen(struct domain *d, char c)
> > > +{
> > > + unsigned long flags;
> > > + struct vpl011 *vpl011 = &d->arch.vpl011;
> > > + struct vpl011_xen_backend *intf = vpl011->backend.xen;
> > > + XENCONS_RING_IDX in_cons, in_prod, in_fifo_level;
> > > +
> > > + ASSERT(!vpl011->backend_in_domain);
> > > + VPL011_LOCK(d, flags);
> > > +
> > > + in_cons = intf->in_cons;
> > > + in_prod = intf->in_prod;
> > Do we need barriers here and after writing the ring?
>
> We do not need barriers because both frontend and backend are in Xen for
> this use-case: both in_cons and in_prod are modified by Xen with the
> VPL011_LOCK held. There are no simultaneous reads/writes and the lock
> includes a barrier.
I got it. Thank you for the explanation.
>
>
> > > + if ( xencons_queued(in_prod, in_cons, sizeof(intf->in)) ==
> > > sizeof(intf->in) )
> > > + {
> > > + VPL011_UNLOCK(d, flags);
> > > + return;
> > > + }
> > > +
> > > + intf->in[xencons_mask(in_prod, sizeof(intf->in))] = c;
> > > + intf->in_prod = ++in_prod;
> > > +
> > > + in_fifo_level = xencons_queued(in_prod,
> > > + in_cons,
> > > + sizeof(intf->in));
> > > +
> > > + vpl011_data_avail(d, in_fifo_level, sizeof(intf->in), 0,
> > > SBSA_UART_FIFO_SIZE);
> > > + VPL011_UNLOCK(d, flags);
> > > +}
> > > +
> > > static void vpl011_notification(struct vcpu *v, unsigned int port)
> > > {
> > > unsigned long flags;
> > > @@ -470,27 +594,47 @@ int domain_vpl011_init(struct domain *d, struct
> > > vpl011_init_info *info)
> > > if ( vpl011->backend.dom.ring_buf )
> > > return -EINVAL;
> > >
> > > - /* Map the guest PFN to Xen address space. */
> > > - rc = prepare_ring_for_helper(d,
> > > - gfn_x(info->gfn),
> > > - &vpl011->backend.dom.ring_page,
> > > - &vpl011->backend.dom.ring_buf);
> > > - if ( rc < 0 )
> > > - goto out;
> > > + /*
> > > + * info is NULL when the backend is in Xen.
> > > + * info is != NULL when the backend is in a domain.
> > > + */
> > > + if ( info != NULL )
> > > + {
> > > + vpl011->backend_in_domain = true;
> > > +
> > > + /* Map the guest PFN to Xen address space. */
> > > + rc = prepare_ring_for_helper(d,
> > > + gfn_x(info->gfn),
> > > + &vpl011->backend.dom.ring_page,
> > > + &vpl011->backend.dom.ring_buf);
> > > + if ( rc < 0 )
> > > + goto out;
> > > +
> > > + rc = alloc_unbound_xen_event_channel(d, 0, info->console_domid,
> > > + vpl011_notification);
> > > + if ( rc < 0 )
> > > + goto out1;
> > > +
> > > + vpl011->evtchn = info->evtchn = rc;
> > > + }
> > > + else
> > > + {
> > > + vpl011->backend_in_domain = false;
> > > +
> > > + vpl011->backend.xen = xzalloc(struct vpl011_xen_backend);
> > > + if ( vpl011->backend.xen == NULL )
> > > + {
> > > + rc = -EINVAL;
> > > + goto out1;
> > > + }
> > > + }
> > >
> > > rc = vgic_reserve_virq(d, GUEST_VPL011_SPI);
> > > if ( !rc )
> > > {
> > > rc = -EINVAL;
> > > - goto out1;
> > > - }
> > > -
> > > - rc = alloc_unbound_xen_event_channel(d, 0, info->console_domid,
> > > - vpl011_notification);
> > > - if ( rc < 0 )
> > > goto out2;
> > > -
> > > - vpl011->evtchn = info->evtchn = rc;
> > > + }
> > >
> > > spin_lock_init(&vpl011->lock);
> > >
> > > @@ -503,8 +647,11 @@ out2:
> > > vgic_free_virq(d, GUEST_VPL011_SPI);
> > >
> > > out1:
> > > - destroy_ring_for_helper(&vpl011->backend.dom.ring_buf,
> > > - vpl011->backend.dom.ring_page);
> > > + if ( vpl011->backend_in_domain )
> > > + destroy_ring_for_helper(&vpl011->backend.dom.ring_buf,
> > > + vpl011->backend.dom.ring_page);
> > > + else
> > > + xfree(vpl011->backend.xen);
> > >
> > > out:
> > > return rc;
> > > @@ -514,12 +661,17 @@ void domain_vpl011_deinit(struct domain *d)
> > > {
> > > struct vpl011 *vpl011 = &d->arch.vpl011;
> > >
> > > - if ( !vpl011->backend.dom.ring_buf )
> > > - return;
> > > + if ( vpl011->backend_in_domain )
> > > + {
> > > + if ( !vpl011->backend.dom.ring_buf )
> > > + return;
> > >
> > > - free_xen_event_channel(d, vpl011->evtchn);
> > > - destroy_ring_for_helper(&vpl011->backend.dom.ring_buf,
> > > - vpl011->backend.dom.ring_page);
> > > + free_xen_event_channel(d, vpl011->evtchn);
> > > + destroy_ring_for_helper(&vpl011->backend.dom.ring_buf,
> > > + vpl011->backend.dom.ring_page);
> > > + }
> > > + else
> > > + xfree(vpl011->backend.xen);
> > > }
> > >
> > > /*
> > > diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
> > > index 75172e7..990c51c 100644
> > > --- a/xen/drivers/char/console.c
> > > +++ b/xen/drivers/char/console.c
> > > @@ -444,7 +444,7 @@ static void __serial_rx(char c, struct cpu_user_regs
> > > *regs)
> > > send_global_virq(VIRQ_CONSOLE);
> > > break;
> > > }
> > > -#if 0
> > > +#ifdef CONFIG_SBSA_VUART_CONSOLE
> > > default:
> > > {
> > > struct domain *d = rcu_lock_domain_by_any_id(console_rx - 1);
> > > diff --git a/xen/include/asm-arm/vpl011.h b/xen/include/asm-arm/vpl011.h
> > > index 42d7a24..5eb6d25 100644
> > > --- a/xen/include/asm-arm/vpl011.h
> > > +++ b/xen/include/asm-arm/vpl011.h
> > > @@ -21,6 +21,7 @@
> > >
> > > #include <public/domctl.h>
> > > #include <public/io/ring.h>
> > > +#include <public/io/console.h>
> > > #include <asm/vreg.h>
> > > #include <xen/mm.h>
> > >
> > > @@ -29,13 +30,19 @@
> > > #define VPL011_UNLOCK(d,flags)
> > > spin_unlock_irqrestore(&(d)->arch.vpl011.lock, flags)
> > >
> > > #define SBSA_UART_FIFO_SIZE 32
> > > +struct vpl011_xen_backend {
> > > + char in[SBSA_UART_FIFO_SIZE];
> > > + XENCONS_RING_IDX in_cons, in_prod;
> > > +};
> > >
> > > struct vpl011 {
> > > + bool backend_in_domain;
> > > union {
> > > struct {
> > > void *ring_buf;
> > > struct page_info *ring_page;
> > > } dom;
> > > + struct vpl011_xen_backend *xen;
> > > } backend;
> > > uint32_t uartfr; /* Flag register */
> > > uint32_t uartcr; /* Control register */
> > > @@ -57,6 +64,7 @@ struct vpl011_init_info {
> > > int domain_vpl011_init(struct domain *d,
> > > struct vpl011_init_info *info);
> > > void domain_vpl011_deinit(struct domain *d);
> > > +void vpl011_rx_char_xen(struct domain *d, char c);
> > > #else
> > > static inline int domain_vpl011_init(struct domain *d,
> > > struct vpl011_init_info *info)
--
Regards,
Oleksandr Tyshchenko
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |