lkml.org 
[lkml]   [2009]   [Aug]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH 4/7] vbus-proxy: add a pci-to-vbus bridge
Date
On Thursday 06 August 2009, you wrote:
> >>> On 8/6/2009 at 10:42 AM, in message <200908061642.40614.arnd@arndb.de>, Arnd
> Bergmann <arnd@arndb.de> wrote:
> > On Monday 03 August 2009, Gregory Haskins wrote:
> >> This patch adds a pci-based driver to interface between the a host VBUS
> >> and the guest's vbus-proxy bus model.
> >>
> >> Signed-off-by: Gregory Haskins <ghaskins@novell.com>
> >
> > This seems to be duplicating parts of virtio-pci that could be kept
> > common by extending the virtio code. Layering on top of virtio
> > would also make it possible to use the same features you add
> > on top of other transports (e.g. the s390 virtio code) without
> > adding yet another backend for each of them.
>
> This doesn't make sense to me, but I suspect we are both looking at what this
> code does differently. I am under the impression that you may believe that
> there is one of these objects per vbus device. Note that this is just a bridge
> to vbus, so there is only one of these per system with potentially many vbus
> devices behind it.

Right, this did not become clear from the posting. For virtio, we discussed
a model like this in the beginning and then rejected it in favour of a
"one PCI device per virtio device" model, which I now think is a better
approach than your pci-to-vbus bridge.

> In essence, this driver's job is to populate the "vbus-proxy" LDM bus with
> objects that it finds across the PCI-OTHER bridge. This would actually sit
> below the virtio components in the stack, so it doesnt make sense (to me) to
> turn around and build this on top of virtio. But perhaps I am missing
> something you are seeing.
>
> Can you elaborate?

Your PCI device does not serve any real purpose as far as I can tell, you
could just as well have a root device as a parent for all the vbus devices
if you do your device probing like this.

However, assuming that you do the IMHO right thing to do probing like
virtio with a PCI device for each slave, the code will be almost the same
as virtio-pci and the two can be the same.

> > This seems to add an artificial abstraction that does not make sense
> > if you stick to the PCI abstraction.
>
> I think there may be confusion about what is going on here. The "device-open"
> pertains to a vbus device *beyond* the bridge, not the PCI device (the bridge)
> itself. Nor is the vbus device a PCI device.
>
> Whats happening here is somewhat analogous to a PCI config-cycle. Its
> a way to open a channel to a device beyond the bridge in _response_ to
> a probe.
>
> We have a way to enumerate devices present beyond the bridge (this yields
> a "device-id") but in order to actually talk to the device, you must first
> call DEVOPEN(id). When a device-id is enumerated, it generates a probe()
> event on vbus-proxy. The responding driver in question would then turn
> around and issue the handle = dev->open(VERSION) to see if it is compatible
> with the device, and to establish a context for further communication.
>
> The reason why DEVOPEN returns a unique handle is to help ensure that the
> driver has established proper context before allowing other calls.

So assuming this kind of bus is the right idea (which I think it's not),
why can't the host assume they are open to start with and you go and
enumerate the devices on the bridge, creating a vbus_device for each
one as you go. Then you just need to match the vbus drivers with the
devices by some string or vendor/device ID tuple.

> >
> > This could be implemented by virtio devices as well, right?
>
> The big difference with dev->shm() is that it is not bound to
> a particular ABI within the shared-memory (as opposed to
> virtio, which assumes a virtio ABI). This just creates a
> n empty shared-memory region (with a bidirectional signaling
> path) which you can overlay a variety of structures (virtio
> included). You can of course also use non-ring based
> structures, such as, say, an array of idempotent state.
>
> The point is that, once this is done, you have a shared-memory
> region and a way (via the shm-signal) to bidirectionally signal
> changes to that memory region. You can then build bigger
> things with it, like virtqueues.

Let me try to rephrase my point: I believe you can implement
the shm/ioq data transport on top of the virtio bus level, by
adding shm and signal functions to struct virtio_config_ops
alongside find_vqs() so that a virtio_device can have either
any combination of virtqueues, shm and ioq.

> > static int
> > vbus_pci_device_call(struct vbus_device_proxy *vdev, u32 func, void *data,
> > size_t len, int flags)
> > {
> > struct vbus_pci_device *dev = to_dev(vdev);
> > struct vbus_pci_hypercall params = {
> > .vector = func,
> > .len = len,
> > .datap = __pa(data),
> > };
> > spin_lock_irqsave(&dev.lock, flags);
> > memcpy_toio(&dev.regs->hypercall.data, &params, sizeof(params));
> > ret = ioread32(&dev.regs->hypercall.result);
> > spin_unlock_irqrestore(&dev.lock, flags);
> >
> > return ret;
> > }
> >
> > This gets rid of your 'handle' and the unwinding through an extra pointer
> > indirection. You just need to make sure that the device specific call
> > numbers
> > don't conflict with any global ones.
>
> Ah, now I see the confusion...
>
> DEVCALL is sending a synchronous call to a specific device beyond the bridge. The MMIO going on here against dev.regs->hypercall.data is sending a synchronous call to the bridge itself. They are distinctly different ;)

well, my point earlier was that they probably should not be different ;-)

Arnd <><


\
 
 \ /
  Last update: 2009-08-06 19:05    [W:0.079 / U:1.840 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site