|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH RFC 13/15] xen/arm: Allow vpl011 to be used by DomU
On Fri, 15 Jun 2018, Julien Grall wrote:
> Hi Stefano,
>
> On 06/13/2018 11:15 PM, 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.
> >
> > Signed-off-by: Stefano Stabellini <stefanos@xxxxxxxxxx>
> > ---
> > xen/arch/arm/domain_build.c | 9 +++-
> > xen/arch/arm/vpl011.c | 98
> > +++++++++++++++++++++++++++++++++-----------
> > xen/include/asm-arm/vpl011.h | 2 +
> > 3 files changed, 84 insertions(+), 25 deletions(-)
> >
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index ff65057..97f14ca 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -2482,7 +2482,14 @@ 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
> > + rc = domain_vpl011_init(d, NULL);
>
> See my remark on the previous patch about exposing vpl011 by default.
Done
> > +#endif
> > + return rc;
> > }
> > int __init construct_dom0(struct domain *d)
> > diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
> > index a281eab..5f1dc7a 100644
> > --- a/xen/arch/arm/vpl011.c
> > +++ b/xen/arch/arm/vpl011.c
> > @@ -34,6 +34,8 @@
> > #include <asm/vgic-emul.h>
> > #include <asm/vpl011.h>
> > +static void vpl011_data_avail(struct domain *d);
> > +
> > /*
> > * Since pl011 registers are 32-bit registers, all registers
> > * are handled similarly allowing 8-bit, 16-bit and 32-bit
> > @@ -77,6 +79,29 @@ static void vpl011_update_interrupt_status(struct domain
> > *d)
> > #endif
> > }
> > +void vpl011_read_char(struct domain *d, char c)
>
> The name is slightly odd. From the name, I would expect that a character is
> returned. But in fact, you write a character you received in the ring. So a
> better name would be vpl011_rx_char.
OK
> > +{
> > + unsigned long flags;
> > + XENCONS_RING_IDX in_cons, in_prod;
> > + struct xencons_interface *intf = d->arch.vpl011.ring_buf;
> > +
> > + 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;
> > +
> > + VPL011_UNLOCK(d, flags);
> > + vpl011_data_avail(d);
> > +}
> > +
> > static uint8_t vpl011_read_data(struct domain *d)
> > {
> > unsigned long flags;
> > @@ -166,9 +191,18 @@ static void vpl011_write_data(struct domain *d, uint8_t
> > data)
> > struct vpl011 *vpl011 = &d->arch.vpl011;
> > struct xencons_interface *intf = vpl011->ring_buf;
> > XENCONS_RING_IDX out_cons, out_prod;
> > + unsigned int fifo_level = 0;
> > VPL011_LOCK(d, flags);
> > + if ( vpl011->ring_page == NULL )
> > + {
> > + printk("%c", data);
> > + if (data == '\n')
> > + printk("DOM%u: ", d->domain_id);
> > + goto done;
> > + }
> > +
>
> I would rather introduce separate function to read/write data for the case
> without PV console. And use it where appropriate. This would make the code
> slightly easier to understand because "ring_page == NULL" is slightly
> untuitive.
>
> An idea would be introduce callback and set them during the initialization of
> the vpl011 for the domain.
>
> > out_cons = intf->out_cons;
> > out_prod = intf->out_prod;
> > @@ -184,13 +218,10 @@ static void vpl011_write_data(struct domain *d,
> > uint8_t data)
> > if ( xencons_queued(out_prod, out_cons, sizeof(intf->out)) !=
> > sizeof (intf->out) )
> > {
> > - unsigned int fifo_level;
> > -
> > intf->out[xencons_mask(out_prod, sizeof(intf->out))] = data;
> > out_prod += 1;
> > smp_wmb();
> > intf->out_prod = out_prod;
> > -
>
> Spurious change.
removed
> > fifo_level = xencons_queued(out_prod, out_cons,
> > sizeof(intf->out));
> > if ( fifo_level == sizeof(intf->out) )
> > @@ -205,14 +236,15 @@ static void vpl011_write_data(struct domain *d,
> > uint8_t data)
> > */
> > vpl011->uartfr |= BUSY;
> > }
> > -
> > - vpl011_update_tx_fifo_status(vpl011, fifo_level);
> > -
> > - vpl011_update_interrupt_status(d);
> > }
> > else
> > gprintk(XENLOG_ERR, "vpl011: Unexpected OUT ring buffer full\n");
> > +done:
> > + vpl011_update_tx_fifo_status(vpl011, fifo_level);
> > +
> > + vpl011_update_interrupt_status(d);
>
> Hmmm, now you will call vpl011_update_* in the error case when writing to the
> case. If you want to keep that, this should at least be explained in the
> commit message or probably be a separate patch.
I removed this changed and opted for separate read/write functions and a
smaller ring struct.
> > +
> > vpl011->uartfr &= ~TXFE;
> > VPL011_UNLOCK(d, flags);
> > @@ -462,13 +494,30 @@ int domain_vpl011_init(struct domain *d, struct
> > vpl011_init_info *info)
> > if ( vpl011->ring_buf )
> > return -EINVAL;
> > - /* Map the guest PFN to Xen address space. */
> > - rc = prepare_ring_for_helper(d,
> > - gfn_x(info->gfn),
> > - &vpl011->ring_page,
> > - &vpl011->ring_buf);
> > - if ( rc < 0 )
> > - goto out;
> > + if ( info != NULL )
>
> Please document how info could be NULL here.
OK
> > + {
> > + /* Map the guest PFN to Xen address space. */
> > + rc = prepare_ring_for_helper(d,
> > + gfn_x(info->gfn),
> > + &vpl011->ring_page,
> > + &vpl011->ring_buf);
> > + if ( rc < 0 )
> > + goto out;
> > +
> > + rc = alloc_unbound_xen_event_channel(d, 0, info->console_domid,
> > + vpl011_notification);
> > + if ( rc < 0 )
> > + goto out2;
>
> When you move code around, you should also look at the error path. In that
> case, you are going to free a virq that does not exist (because not yet
> allocated), and if vgic_reserve_virq below fails, you will not free the event
> channel allocated.
I'll fix
> > +
> > + vpl011->evtchn = info->evtchn = rc;
> > + } else {
> > + vpl011->ring_buf = xzalloc(struct xencons_interface);
>
> Using ring_buf is such waste of memory. You basically only allow 1024
> character but still using 4K.
>
> Furthermore, the way you use ring_page is really confusing. A bit more
> documentation might help. Although, this new code does not feel integrated
> with the rest of the vpl011.
>
> It looks like you want to rework the vpl011 code to have move anything related
> to ring in separate function. Once it is done, we could then introduce new
> callback for the guest started in Xen.
I have done that now, I introduced a smaller ring struct with only the
in parts and introduced separate read_data and write_data functions.
Overall, I think is better. The price to pay is that the new read_data
is almost an exact duplicate of the existing function. They cannot be
reused because the pointer format is different, although the field names
are the same. You'll see in the next version.
> > + if ( vpl011->ring_buf == NULL )
> > + {
> > + rc = -EINVAL;
> > + goto out;
> > + }
> > + }
> > rc = vgic_reserve_virq(d, GUEST_VPL011_SPI);
> > if ( !rc )
> > @@ -477,13 +526,6 @@ int domain_vpl011_init(struct domain *d, struct
> > vpl011_init_info *info)
> > 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);
> > register_mmio_handler(d, &vpl011_mmio_handler,
> > @@ -495,7 +537,10 @@ out2:
> > vgic_free_virq(d, GUEST_VPL011_SPI);
> > out1:
> > - destroy_ring_for_helper(&vpl011->ring_buf, vpl011->ring_page);
> > + if ( vpl011->ring_page == NULL && vpl011->ring_buf != NULL )
> > + xfree(vpl011->ring_buf);
> > + else
> > + destroy_ring_for_helper(&vpl011->ring_buf, vpl011->ring_page);
> > out:
> > return rc;
> > @@ -508,8 +553,13 @@ void domain_vpl011_deinit(struct domain *d)
> > if ( !vpl011->ring_buf )
> > return;
> > - free_xen_event_channel(d, vpl011->evtchn);
> > - destroy_ring_for_helper(&vpl011->ring_buf, vpl011->ring_page);
> > + if ( vpl011->ring_page == NULL && vpl011->ring_buf != NULL )
> > + {
> > + xfree(vpl011->ring_buf);
> > + } else {
>
> Coding style
>
> }
> else
> {
OK
> > + free_xen_event_channel(d, vpl011->evtchn);
> > + destroy_ring_for_helper(&vpl011->ring_buf, vpl011->ring_page);
> > + }
> > }
> > /*
> > diff --git a/xen/include/asm-arm/vpl011.h b/xen/include/asm-arm/vpl011.h
> > index db95ff8..8d9b0da 100644
> > --- a/xen/include/asm-arm/vpl011.h
> > +++ b/xen/include/asm-arm/vpl011.h
> > @@ -53,6 +53,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_read_char(struct domain *d, char c);
> > #else
> > static inline int domain_vpl011_init(struct domain *d,
> > struct vpl011_init_info *info)
> > @@ -61,6 +62,7 @@ static inline int domain_vpl011_init(struct domain *d,
> > }
> > static inline void domain_vpl011_deinit(struct domain *d) { }
> > +static inline void vpl011_read_char(struct domain *d, char c) { }
> > #endif
> > #endif /* _VPL011_H_ */
> >
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |