On 03/05/2017 12:45 PM, Nadav Har'El wrote:

On Fri, Mar 3, 2017 at 11:45 AM, 'Sergiy Kibrik' via OSv Development 
<osv-dev@googlegroups.com <mailto:osv-dev@googlegroups.com>> wrote:

    Complete driver to properly work with xl console utility, i.e. full 
read/write
    access. Driver is extended to work with shared structure (ring) and event 
channel.

    Signed-off-by: Sergiy Kibrik <sergiy.kib...@globallogic.com 
<mailto:sergiy.kib...@globallogic.com>>
    ---
     drivers/xenconsole.cc | 65 
+++++++++++++++++++++++++++++++++++++++++++++++----
     drivers/xenconsole.hh | 14 +++++++++--
     2 files changed, 72 insertions(+), 7 deletions(-)

    diff --git a/drivers/xenconsole.cc b/drivers/xenconsole.cc
    index fe7c5bf..6ddac85 100644
    --- a/drivers/xenconsole.cc
    +++ b/drivers/xenconsole.cc
    @@ -5,32 +5,87 @@
      * BSD license as described in the LICENSE file in the top-level directory.
      */

    +#define XENHVM //FIXME hack to reveal hvm_get_parameter()
     #include <bsd/porting/netport.h> /* __dead2 defined here */
    +#include <bsd/porting/bus.h>
    +#include <machine/xen/xen-os.h>
     #include <xen/hypervisor.h>
    -
    +#include <xen/interface/hvm/params.h>
    +#include <xen/interface/io/console.h>
    +#include <xen/xen_intr.h>
    +#include <xen/evtchn.h>
    +#include <osv/mmio.hh>
     #include "xenconsole.hh"

     namespace console {

    +XEN_Console::
    +XEN_Console::XEN_Console()
    +    : _interface(0)
    +     , _evtchn(-1)
    +     , _irq(0)
    +     , _pfn(0)
    +{}
    +
    +void XEN_Console::handle_intr()
    +{
    +    _thread->wake();
    +}
    +
     void XEN_Console::write(const char *str, size_t len) {
    -       HYPERVISOR_console_write(str, len);
    +    assert(len > 0);
    +    if (!_interface) {
    +        HYPERVISOR_console_write(str, len);
    +        return;
    +    }
    +
    +    XENCONS_RING_IDX cons, prod;
    +    prod = _interface->out_prod;
    +    cons = _interface->out_cons;
    +    mb();
    +    while (len-- > 0)
    +        _interface->out[MASK_XENCONS_IDX(prod++, _interface->out)] = 
*str++;


It looks like your implementation has no flow control, and if you write too quickly before Xen empties the buffer, you abort instead of waiting for the buffer to be empties. I guess this is fine as a first implementation, but please add a FIXME on this limitation.
I should probably add some flow control right away. BTW, is there any way in 
OSv to check if we're in atomic/interrupt context? I suppose write() may also 
be executed in critical sections..

    +    wmb();
    +    assert(prod - cons <= sizeof(_interface->out));


I guess this assert() is related to my question about. I am curious why test it 
here instead of up-front, using len?

I will fix that.

    +    _interface->out_prod = prod;
     }

     void XEN_Console::dev_start()
     {
    +    _pfn = hvm_get_parameter(HVM_PARAM_CONSOLE_PFN);
    +    _evtchn = hvm_get_parameter(HVM_PARAM_CONSOLE_EVTCHN);
    +
    +    if (!_pfn || !_evtchn)
    +        throw std::runtime_error("fail to get console params");
    +
    +     if (bind_caller_port_to_irqhandler(_evtchn, "xenconsole",
    +            XEN_Console::console_intr,
    +            static_cast<void*>(this),
    +            INTR_TYPE_MISC, &_irq) != 0)
    +        throw std::runtime_error("fail to bind evtchn");
    +
    +    _interface = (xencons_interface*)mmio_map(_pfn << PAGE_SHIFT, 
PAGE_SIZE);
     }

     void XEN_Console::flush()
     {
    +    notify_remote_via_evtchn(_evtchn);
     }

     bool XEN_Console::input_ready()
     {
    -       return false; /*TODO: support input */
    +    mb();


What does this memory barrier, with nothing above it, do?


I double checked console-multiplexer and mb is indeed redundant here.

    +    return _interface->in_cons != _interface->in_prod;
     }

     char XEN_Console::readch() {
    -    return '\0'; /*TODO: support input */
    +    XENCONS_RING_IDX cons;
    +    char c;
    +    assert(_interface);
    +    cons = _interface->in_cons;
    +    c = _interface->in[MASK_XENCONS_IDX(cons, _interface->in)];


I think you should gracefully handle the case where you had a false wakeup and called readch() when nothing is available, and return 0 in this case (as you can see, LineDiscipline::read_poll handles 0 being returned). I guess you need to check if cons is actually beyond prod?


I will do that.

    +    mb();
    +    _interface->in_cons = cons + 1;
    +    return c;
     }
    -
     }
    diff --git a/drivers/xenconsole.hh b/drivers/xenconsole.hh
    index ec43893..ca6d8cf 100644
    --- a/drivers/xenconsole.hh
    +++ b/drivers/xenconsole.hh
    @@ -9,13 +9,14 @@
     #define XEN_CONSOLE_HH

     #include "console-driver.hh"
    -#include "exceptions.hh"
    -#include <osv/interrupt.hh>
    +#include <xen/interface/xen.h>
    +#include <xen/interface/grant_table.h>

     namespace console {

     class XEN_Console : public console_driver {
     public:
    +    XEN_Console();
         virtual void write(const char *str, size_t len);
         virtual void flush();
         virtual bool input_ready();
    @@ -24,6 +25,15 @@ public:
     private:
         virtual void dev_start();
         virtual const char *thread_name() { return "xen-input"; }
    +    void handle_intr();
    +    static void console_intr(void *arg) {
    +        static_cast<XEN_Console*>(arg)->handle_intr();
    +    }
    +
    +    struct xencons_interface *_interface;
    +    int _evtchn;
    +    unsigned int _irq;
    +    unsigned long _pfn;
     };

Thank you for taking a look at this series!

--
regards,
Sergiy

--
You received this message because you are subscribed to the Google Groups "OSv 
Development" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to osv-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Reply via email to