On 03/05/2017 11:44 AM, 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:

    Xen platform PCI is x86-specific feature, not currently present on aarch64.

    For xenbus to work on both platforms the generic xenbus driver should be 
probed
    directly (under aarch64 in case Xen is detected), but in case of x64 
special PCI
    driver checks for Xen platform PCI device and probes generic xenbus driver
    if such device is found, pretty much like the way it is done in Linux.

    Signed-off-by: Sergiy Kibrik <sergiy.kib...@globallogic.com 
<mailto:sergiy.kib...@globallogic.com>>
    ---
     Makefile                   |  1 +
     arch/x64/arch-setup.cc     |  4 ++--
     drivers/xenfront-xenbus.cc | 33 ++++-----------------------------
     drivers/xenfront-xenbus.hh | 12 +++---------
     drivers/xenfront.hh        |  2 --
     drivers/xenplatform-pci.cc | 40 ++++++++++++++++++++++++++++++++++++++++
     drivers/xenplatform-pci.hh | 33 +++++++++++++++++++++++++++++++++
     7 files changed, 83 insertions(+), 42 deletions(-)
     create mode 100644 drivers/xenplatform-pci.cc
     create mode 100644 drivers/xenplatform-pci.hh

    diff --git a/Makefile b/Makefile
    index 6846c5e..3f011fa 100644
    --- a/Makefile
    +++ b/Makefile
    @@ -818,6 +818,7 @@ drivers += drivers/ahci.o
     drivers += drivers/ide.o
     drivers += drivers/scsi-common.o
     drivers += drivers/vmw-pvscsi.o
    +drivers += drivers/xenplatform-pci.o
     endif # x64

     ifeq ($(arch),aarch64)
    diff --git a/arch/x64/arch-setup.cc b/arch/x64/arch-setup.cc
    index fb5deaa..325c26a 100644
    --- a/arch/x64/arch-setup.cc
    +++ b/arch/x64/arch-setup.cc
    @@ -255,7 +255,7 @@ void arch_init_premain()
     #include "drivers/virtio-net.hh"
     #include "drivers/virtio-assign.hh"
     #include "drivers/virtio-rng.hh"
    -#include "drivers/xenfront-xenbus.hh"
    +#include "drivers/xenplatform-pci.hh"
     #include "drivers/ahci.hh"
     #include "drivers/vmw-pvscsi.hh"
     #include "drivers/vmxnet3.hh"
    @@ -283,7 +283,7 @@ void arch_init_drivers()
             drvman->register_driver(virtio::net::probe);
         }
         drvman->register_driver(virtio::rng::probe);
    -    drvman->register_driver(xenfront::xenbus::probe);
    +    drvman->register_driver(xenfront::xenplatform_pci::probe);
         drvman->register_driver(ahci::hba::probe);
         drvman->register_driver(vmw::pvscsi::probe);
         drvman->register_driver(vmw::vmxnet3::probe);
    diff --git a/drivers/xenfront-xenbus.cc b/drivers/xenfront-xenbus.cc
    index 3f6ab29..ba14384 100644
    --- a/drivers/xenfront-xenbus.cc
    +++ b/drivers/xenfront-xenbus.cc
    @@ -5,8 +5,6 @@
      * BSD license as described in the LICENSE file in the top-level directory.
      */

    -#include "msr.hh"
    -#include "xen.hh"
     #include <osv/types.h>
     #include <osv/mmu.hh>
     #include "string.h"
    @@ -46,27 +44,17 @@ namespace xenfront {

     xenbus *xenbus::_instance = nullptr;

    -xenbus::xenbus(pci::device& pci_dev)
    +xenbus::xenbus()
         : hw_driver()


Does xenbus still need to be a "hw_driver" instance, or can just be utility 
code called from an actual hw_driver?


I intend to enable xenbus driver for aarch64 as soon I have critical bugs 
sorted out. This xenfront::xenbus::probe interface will be used then.

    -    , _dev(pci_dev)
     {
    -    int irqno = pci_dev.get_interrupt_line();
    -
         if (_instance) {
             return;
         } else {
             _instance = this;
         }

    -    parse_pci_config();
    -
    -    _dev.set_bus_master(true);
         _driver_name = std::string("xenfront-xenbus");

    -    if (!processor::features().xen_vector_callback) {
    -        _pgsi.reset(xen::xen_set_callback(irqno));
    -    }
    -
         xs_attach(&_xenstore_device);

         xs_scanf(XST_NIL, "domid", "", NULL, "%d", &_domid);
    @@ -127,27 +115,14 @@ void 
xenbus::for_each_child(std::function<void(xenfront_driver *d)> func)

     hw_driver* xenbus::probe(hw_device* dev)
     {
    -    if (!processor::features().xen_pci) {
    +    if (!is_xen())
             return nullptr;
    -    }
    -
    -    if (auto pci_dev = dynamic_cast<pci::device*>(dev)) {
    -        // dev id is the same for all xen devices?
    -        if (pci_dev->get_subsystem_vid() == XEN_VENDOR_ID) {
    -            return new xenbus(*pci_dev);
    -        }
    -    }
    -    return nullptr;
    +    return new xenbus();
     }


If I understand correctly, this function no longer "probes" anything (it doesn't even use 
the "dev" parameter). So shouldn't you rename it and remove its argument, or even delete 
it altogether (just incorporating its code into the caller)?


As I've written above, this driver will be registered & probed by aarch64 code 
directly — as there's no PCI driver there.


     void xenbus::dump_config()
     {
    -    _dev.dump_config();
    -}
    -
    -bool xenbus::parse_pci_config()
    -{
    -    return true;
    +    /*TODO: print type, name and node path */
     }
     };

    diff --git a/drivers/xenfront-xenbus.hh b/drivers/xenfront-xenbus.hh
    index 808545a..4a587a4 100644
    --- a/drivers/xenfront-xenbus.hh
    +++ b/drivers/xenfront-xenbus.hh
    @@ -9,22 +9,18 @@
     #define XENFRONT_BUS_DRIVER_H

     #include "drivers/xenfront.hh"
    -#include "drivers/pci-device.hh"
    +#include "drivers/device.hh"
     #include <osv/device.h>
     #include <bsd/porting/bus.h>

     namespace xenfront {
    -
         class xenbus : public hw_driver {
         public:

    -        explicit xenbus(pci::device& dev);
    +        explicit xenbus();
             static hw_driver* probe(hw_device* dev);
    -        pci::device& pci_device() { return _dev; }
    -
    -        bool parse_pci_config();
    -        void dump_config();

    +        virtual void dump_config();
             virtual std::string get_name() const { return _driver_name; }
             const std::string &get_node_path() { return _node_path; }

    @@ -40,8 +36,6 @@ namespace xenfront {
         private:
             static struct xenbus *_instance;
             void wait_for_devices();
    -        pci::device& _dev;
    -        std::unique_ptr<gsi_level_interrupt> _pgsi;
             struct device _xenstore_device;

             std::vector<xenfront_driver *> _children;
    diff --git a/drivers/xenfront.hh b/drivers/xenfront.hh
    index eaea39e..8f239dc 100644
    --- a/drivers/xenfront.hh
    +++ b/drivers/xenfront.hh
    @@ -18,8 +18,6 @@
     #include <bsd/porting/bus.h>
     #include <xen/interface/io/xenbus.h>

    -#define XEN_VENDOR_ID 0x5853
    -
     struct xenbus_device_ivars;

     template <typename T>
    diff --git a/drivers/xenplatform-pci.cc b/drivers/xenplatform-pci.cc
    new file mode 100644
    index 0000000..8e5f439
    --- /dev/null
    +++ b/drivers/xenplatform-pci.cc


A silly question from someone who doesn't know much about Xen: What's the difference between 
"xenplatform", "xenfront", and, I guess, "xenbus"?
That is a good question :)
Xen has internal directory-like database (xenstore) with auxiliary per-VM info, 
it stores (among other things) what para-virtualized devices are enabled for a 
VM.

Xenbus driver scans entries in "devices/" directory and probes corresponding 
drivers, e.g. blkfront/netfront etc.
But how we know if xenbus itself must be probed? ARM system has device tree with "xen" node, x86 has special "Xen platform PCI device". ARM Xen world is much more simplified comparing to x86, it doesn't require separate platform PCI-like driver or so, we can simply detect Xen and probe xenbus.
x86 has more cases to handle, plus legacy, so things a bit more complicated 
there.


    @@ -0,0 +1,40 @@
    +/*
    + * Copyright (C) 2017 Sergiy Kibrik <sergiy.kib...@globallogic.com 
<mailto:sergiy.kib...@globallogic.com>>
    + *
    + * This work is open source software, licensed under the terms of the
    + * BSD license as described in the LICENSE file in the top-level directory.
    + */


If you copied code from other places to this file, can you please keep *also* 
the original Cloudius Systems copyright 2013 (?) line? Not that this really 
matters ;-)


yes, sorry for that.

--
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