|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v2 18/21] xen/arm: Allow vpl011 to be used by DomU
Thank you Julien. I addressed almost all comments. I had a problem with
the implementation of buffering the chars, and I have an alternative
comment on the function sharing. See below.
On Tue, 24 Jul 2018, Julien Grall wrote:
> Hi Stefano,
>
> On 07/07/18 00:12, Stefano Stabellini 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_date and write_data functions for
> > initial domains: vpl011_write_data_noring is very simple and just writes
> > to the console, while vpl011_read_data_inring is a duplicate of
> > vpl011_read_data. Although textually almost identical, we are forced to
> > duplicate the functions because the struct layout is different.
>
> Looking at the patches applied, I think there need some more comments in the
> code to help a reader differentiate which method is used.
OK
> >
> > Signed-off-by: Stefano Stabellini <stefanos@xxxxxxxxxx>
> > ---
> > 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 | 10 ++-
> > xen/arch/arm/vpl011.c | 185
> > ++++++++++++++++++++++++++++++++++++++-----
> > xen/include/asm-arm/vpl011.h | 10 +++
> > 3 files changed, 182 insertions(+), 23 deletions(-)
> >
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index 718be48..d7e9040 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -2531,7 +2531,15 @@ static int __init construct_domU(struct domain *d,
> > struct dt_device_node *node)
> > if ( rc < 0 )
> > return rc;
> > - return __construct_domain(d, &kinfo);
> > + rc = __construct_domain(d, &kinfo);
> > + if ( rc < 0 )
> > + return rc;
> > +
> > +#ifdef CONFIG_SBSA_VUART_CONSOLE
> > + if ( vpl011 )
> > + rc = domain_vpl011_init(d, NULL);
> > +#endif
> I don't think the #ifdef is necessary here. We want to return an error when
> vpl011 is set but not the emulation compiled in.
OK
> > + return rc;
> > }
> > int __init construct_dom0(struct domain *d)
> > diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
> > index e75957f..d4aab64 100644
> > --- a/xen/arch/arm/vpl011.c
> > +++ b/xen/arch/arm/vpl011.c
> > @@ -83,6 +83,111 @@ static void vpl011_update_interrupt_status(struct domain
> > *d)
> > #endif
> > }
> > +void vpl011_rx_char(struct domain *d, char c)
>
> Please add documentation explain what the use of this function. I would also
> rename it to clarify who can call it (i.e only in the case when the backend is
> in Xen).
OK
> > +{
> > + unsigned long flags;
> > + struct vpl011 *vpl011 = &d->arch.vpl011;
> > + struct xencons_in *intf = vpl011->inring;
> > + XENCONS_RING_IDX in_cons, in_prod, in_fifo_level;
> > +
>
> ASSERT(!vpl011->ring_enable);
OK
> > + VPL011_LOCK(d, flags);
> > +
> > + in_cons = intf->in_cons;
> > + in_prod = intf->in_prod;
> > + 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 + 1;
> > +
> > + in_fifo_level = xencons_queued(in_prod,
> > + in_cons,
> > + sizeof(intf->in));
> > +
> > + vpl011_data_avail(d, in_fifo_level, sizeof(intf->in), 0, 1024);
>
> Where does the 1024 come from? Most likely, you want a define for it.
OK
> > + VPL011_UNLOCK(d, flags);
> > +}
> > +
> > +static void vpl011_write_data_noring(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);
>
> You want to buffer the characters here and only print on newline or when the
> buffer is full. Otherwise characters will get mangled with the Xen console
> output or other domains output.
I did the implementation, but then when I type characters at the domain
prompt, I don't see them anymore one by one. Only after I press
"enter". That makes the domain console not very usable. Should we keep
it as?
> > +
> > + vpl011->uartris |= TXI;
> > + vpl011->uartfr &= ~TXFE;
> > + vpl011_update_interrupt_status(d);
> > +
> > + VPL011_UNLOCK(d, flags);
> > +}
> > +
> > +static uint8_t vpl011_read_data_inring(struct domain *d)
> > +{
>
> I think you can avoid the duplication here by using a macro.
I prefer to avoid MACROS for things like this. I would rather reuse the
existing function for both cases like in v1. Would you be OK to go back
to that?
> > + unsigned long flags;
> > + uint8_t data = 0;
> > + struct vpl011 *vpl011 = &d->arch.vpl011;
> > + struct xencons_in *intf = vpl011->inring;
> > + 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;
> > @@ -246,7 +351,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->ring_enable )
> > + *r = vreg_reg32_extract(vpl011_read_data(d), info);
> > + else
> > + *r = vreg_reg32_extract(vpl011_read_data_inring(d), info);
>
> I think some renaming will improve the reading. This is quite unintuitive to
> see a function with "ring" in it, called when !vpl011->ring_enabled.
OK
> > return 1;
> > case RSR:
> > @@ -331,7 +439,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->ring_enable )
> > + vpl011_write_data(v->domain, data);
> > + else
> > + vpl011_write_data_noring(v->domain, data);
> > return 1;
> > }
> > @@ -476,27 +587,47 @@ int domain_vpl011_init(struct domain *d, struct
> > vpl011_init_info *info)
> > if ( vpl011->ring.ring_buf )
> > return -EINVAL;
> > - /* Map the guest PFN to Xen address space. */
> > - rc = prepare_ring_for_helper(d,
> > - gfn_x(info->gfn),
> > - &vpl011->ring.ring_page,
> > - &vpl011->ring.ring_buf);
> > - if ( rc < 0 )
> > - goto out;
> > + /*
> > + * info is NULL for domUs started by Xen at boot time, with no
> > + * corresponding userspace component in dom0 > + */
>
> I don't think you want to mention domUs at all here. The emulation should not
> care whether it is a guest or the hardware domain. It would theoretically
> possible to have the hardware domain re-using this code.
>
> Furthermore, nothing in the emulation mandates to have the backend in
> the hardware domain. This could be anywhere.
>
> It looks like to me you want to offer 2 solutions:
> 1) Console backend in a domain
> 2) Console backend in the hypervisor
>
> So probably a better naming for ring_enable would be "backend_in_domain" (or
> something similar).
I'll rename ring_enable to backend_in_domain, and improve the comment
above.
> > + if ( info != NULL )
> > + {
> > + vpl011->ring_enable = true;
> > +
> > + /* Map the guest PFN to Xen address space. */
> > + rc = prepare_ring_for_helper(d,
> > + gfn_x(info->gfn),
> > + &vpl011->ring.ring_page,
> > + &vpl011->ring.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->ring_enable = false;
> > +
> > + vpl011->inring = xzalloc(struct xencons_in);
> > + if ( vpl011->inring == 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);
> > @@ -509,7 +640,10 @@ out2:
> > vgic_free_virq(d, GUEST_VPL011_SPI);
> > out1:
> > - destroy_ring_for_helper(&vpl011->ring.ring_buf,
> > vpl011->ring.ring_page);
> > + if ( vpl011->ring_enable )
> > + destroy_ring_for_helper(&vpl011->ring.ring_buf,
> > vpl011->ring.ring_page);
> > + else
> > + xfree(vpl011->inring);
> > out:
> > return rc;
> > @@ -519,11 +653,18 @@ void domain_vpl011_deinit(struct domain *d)
> > {
> > struct vpl011 *vpl011 = &d->arch.vpl011;
> > - if ( !vpl011->ring.ring_buf )
> > - return;
> > + if ( vpl011->ring_enable )
> > + {
> > + if ( !vpl011->ring.ring_buf )
> > + return;
> > - free_xen_event_channel(d, vpl011->evtchn);
> > - destroy_ring_for_helper(&vpl011->ring.ring_buf,
> > vpl011->ring.ring_page);
> > + free_xen_event_channel(d, vpl011->evtchn);
> > + destroy_ring_for_helper(&vpl011->ring.ring_buf,
> > vpl011->ring.ring_page);
> > + }
> > + else
> > + {
> > + xfree(vpl011->inring);
> > + }
> > }
> > /*
> > diff --git a/xen/include/asm-arm/vpl011.h b/xen/include/asm-arm/vpl011.h
> > index c3d375b..be43abf 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>
> > @@ -30,12 +31,19 @@
> > #define SBSA_UART_FIFO_SIZE 32
> > +struct xencons_in {
>
> The name is too close to xencons_interface and I don't think the name is
> correct for the header it lives.
>
> It probably should be vpl011_xen_backend or something similar.
I'll rename xencons_in to vpl011_xen_backend;
> > + char in[1024];
> > + XENCONS_RING_IDX in_cons, in_prod;
> > +};
> > +
> > struct vpl011 {
> > + bool ring_enable;
> > union {
> > struct {
> > void *ring_buf;
> > struct page_info *ring_page;
> > } ring;
> > + struct xencons_in *inring;
>
> The code is quite confusing. You have a field "ring_enabled" to know which
> bits of the union is used. But both name have "ring" in it.
>
> You most likely want to rework the naming. If you follow my suggestion above
> it would be
>
> union
> {
> struct {
> } dom;
> struct vpl011_xen_backend xen;
> } backend;
I'll follow this suggestion.
> The different helpers would then need to be renamed accordingly.
I'll do
> > };
> > uint32_t uartfr; /* Flag register */
> > uint32_t uartcr; /* Control register */
> > @@ -57,6 +65,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(struct domain *d, char c);
> > #else
> > static inline int domain_vpl011_init(struct domain *d,
> > struct vpl011_init_info *info)
> > @@ -65,6 +74,7 @@ static inline int domain_vpl011_init(struct domain *d,
> > }
> > static inline void domain_vpl011_deinit(struct domain *d) { }
> > +static inline void vpl011_rx_char(struct domain *d, char c) { }
>
> I don't think this is necessary. IIRC, you already ifdef the caller.
OK
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |