Re: [Xen-devel] [PATCH RFC v1 34/74] x86/guest: add PV console code

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Fri, Jan 05, 2018 at 08:22:46AM -0700, Jan Beulich wrote:
> >>> On 04.01.18 at 14:05, <wei.liu2@xxxxxxxxxx> wrote:
> > +void __init pv_console_set_rx_handler(serial_rx_fn fn)
> > +{
> > +    cons_rx_handler = fn;
> > +}
> 
> Especially this and ...
> 
> > +size_t pv_console_rx(struct cpu_user_regs *regs)
> > +{
> > +    char c;
> > +    XENCONS_RING_IDX cons, prod;
> > +    size_t recv = 0;
> > +
> > +    if ( !cons_ring )
> > +        return 0;
> > +
> > +    /* TODO: move this somewhere */
> > +    if ( !test_bit(cons_evtchn, XEN_shared_info->evtchn_pending) )
> > +        return 0;
> 
> ... the need for this and ...
> 
> > +    prod = ACCESS_ONCE(cons_ring->in_prod);
> > +    cons = cons_ring->in_cons;
> > +    /* Get pointers before reading the ring */
> > +    smp_rmb();
> > +
> > +    ASSERT((prod - cons) <= sizeof(cons_ring->in));
> > +
> > +    while ( cons != prod )
> > +    {
> > +        c = cons_ring->in[MASK_XENCONS_IDX(cons++, cons_ring->in)];
> > +        if ( cons_rx_handler )
> > +            cons_rx_handler(c, regs);
> > +        recv++;
> > +    }
> > +
> > +    /* No need for a mem barrier because every character was already 
> > consumed */
> > +    barrier();
> > +    ACCESS_ONCE(cons_ring->in_cons) = cons;
> > +    notify_daemon();
> > +
> > +    clear_bit(cons_evtchn, XEN_shared_info->evtchn_pending);
> 
> ... this at this layer are very hard to judge about with all the code
> here being dead for the moment. Can't this driver be modeled like
> any other of the UART drivers, surfacing the accessors through
> struct uart_driver (and making the ad-hoc call sites in the next
> patch [mostly] unnecessary)?

I've spoken to Sergey and he agrees that this should be solved and
that using uart_driver seems like the right approach.

However given that we would like to merge this ASAP, do you consider
this a blocker? I haven't really looked at how much effort it would
take to model this code as a proper uart driver.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel