On Thu, Jan 15, 2015 at 06:42:02PM +0000, Pawel Moll wrote:
> On Thu, 2015-01-15 at 18:29 +0000, Michael S. Tsirkin wrote:
> > On Thu, Jan 15, 2015 at 06:11:17PM +0000, Pawel Moll wrote:
> > > On Thu, 2015-01-15 at 17:51 +0000, Michael S. Tsirkin wrote:
> > > > > > I think you shouldn't use VIRTIO_MMIO_VRING_ALIGN in non-legacy code:
> > > > > > it's a legacy thing.
> > > > >
> > > > > But I still need to pass something to vring_new_virtqueue() below, don't
> > > > > I? And it will allocate the queue based on some alignment value. I can't
> > > > > see anything that would create the layout for me, neither in mainline
> > > > > nor in next. Have I missed something? (wouldn't be surprised if I have)
> > > >
> > > > No, but it's no longer a virtio thing - just an optimization
> > > > choice by a specific driver. So please just stick e.g. PAGE_SIZE there.
> > >
> > > #define VIRTIO_MMIO_VRING_ALIGN PAGE_SIZE
> > >
> > > > And maybe add a TODO item - we can optimize by allocating chunks
> > > > separately.
> > >
> > > I'll wait and see how do deal with
> > >
> > > vq = vring_new_virtqueue(index, info->num,
> > > VIRTIO_PCI_VRING_ALIGN, &vp_dev->vdev,
> >
> > Check out the code in my tree.
> > It really does, fundamentally:
> >
> > /* TODO: don't allocate contigiously */
> > vq = vring_new_virtqueue(index, info->num,
> > SMP_CACHE_BYTES, &vp_dev->vdev,
>
> Will have a look.
>
> > > > > > > +static struct device_attribute vm_dev_attr_version =
> > > > > > > + __ATTR(version, S_IRUGO, vm_dev_attr_version_show, NULL);
> > > > > > > +
> > > > > > > static int virtio_mmio_probe(struct platform_device *pdev)
> > > > > > > {
> > > > > > > struct virtio_mmio_device *vm_dev;
> > > > > >
> > > > > > We already expose feature bits - this one really necessary?
> > > > >
> > > > > Necessary? Of course not, just a debugging feature, really, to see what
> > > > > version of control registers are available. Useful - I strongly believe
> > > > > so.
> > > >
> > > > Yes but the point is the same info is already available
> > > > in core: just look at feature bit 31.
> > > > If you think it's important enough to expose in a decoded
> > > > way, let's add this in core?
> > >
> > > How do you mean "in core"? It's a mmio-specific value. Content of the
> > > VIRTIO_MMIO_VERSION control register.
> >
> > Yes but if driver loaded, then revision is always in sync
> > with the feature bit.
>
> Well, not quite: as of now I've got legacy block device driver happily
> working on top of compliant (so v2 in MMIO speech) MMIO device - the
> transport if completely transparent here.
Spec says explicitly it's an illegal configuration.
Once driver code is upstream, we won't be able to drop it
because someone, somewhere will use it.
So *if* there's some reason to support a legacy device on top of
a modern transport, then let's document that,
but unless we do let's not create more configurations
that we later need to support.
> > If driver failed to attach, attribute is not there
> > so can't be used for debugging.
>
> Interesting point on its own, haven't thought of that. This is an issue
> with platform devices, no standard set of attributes, always available.
> Will have a look at this.
>
> > > > Absolutely. So what happens if you drop these code lines?
> > > > There's no driver registered for this ID, so it's just ignored.
> > > > Seems like what spec is asking for, no?
> > >
> > > Not to me, no. There will be a vm_dev registered with an _illegal_ ID.
> >
> > Yes - there will be a device, but no driver will drive it.
>
> A device with an *illegal* ID.
So? The "device" is just a bunch of allocated memory. There's no driver
and no one touches is, which is what the spec requires.
> > > > > > @@ -496,7 +531,8 @@ static int virtio_mmio_remove(struct platform_device *pdev)
> > > > > > > {
> > > > > > > struct virtio_mmio_device *vm_dev = platform_get_drvdata(pdev);
> > > > > > >
> > > > > > > - unregister_virtio_device(&vm_dev->vdev);
> > > > > > > + if (vm_dev)
> > > > > > > + unregister_virtio_device(&vm_dev->vdev);
> > > > > > >
> > > > > >
> > > > > > Will remove ever be called if probe fails?
> > > > >
> > > > > No.
> > > >
> > > > Then this if isn't necessary: vm_dev is always set.
> > >
> > > Not (in the current code) if ID is 0.
> >
> > So just return -ENODEV, then probe will be considered
> > failed and remove won't be called.
>
> "4.2.2.2 Driver Requirements: MMIO Device Register Layout
>
> The driver MUST ignore a device with DeviceID 0x0, but MUST NOT report any error."
>
> Returning -ENODEV *reports* an error.
Reports to whom? Do you refer to http://xkcd.com/838/ ?
ENODEV is not "reported". It's just a function return value,
it tells kernel not to call remove.
Users don't know: module probe succeeds, core will not print any errors, user is not
queried.
What happens with your patch? Driver is attached to
device (check where does driver attribute points to!),
but doesn't do anything.
Looks like if you just keep going, you'll achieve
the same result.
> > Or - better - just register the device, it's harmless
> > as no driver will try to attach to it, and there
> > won't be any need to special-case it.
>
> Really, you may want to refresh your memory. We've been there. This *is*
> a special case. Intentionally.
Hmm I found https://issues.oasis-open.org/browse/VIRTIO-7
but the resolution there is merely asking that no driver
matches ID 0. Seems OK.
The MMIO changes were all made as part of
https://issues.oasis-open.org/browse/VIRTIO-44
Anyway, my memory is irrelevant, we need to document motivation
for kernel code changes for future readers. It seems that dropping this
chunk satisfies the spec, so if not true, let's add a comment in code
that explains why.
Assume you stick in device with ID 0.
kernel probes the device, and then sees there is no driver
and leaves it alone.
Seems perfect, matches the spec.
Do you have a config that's not handled well here?
> > > > Not necessarily - the point is for userspace to be able to
> > > > avoid getting useless legacy macros by means of a simple #define.
> > > > Again, take a look at my tree:
> > > >
> > > > git://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git vhost-next
> > >
> > > I have no idea what are you talking about here. Why would userspace ever
> > > access the memory mapped control registers?
> >
> > Userspace being qemu which implements these.
>
> Right, qemu is a valid userspace case.
>
> > > That's all what this header
> > > describes. Actually, if I was typing the driver today, I'd define the
> > > offsets in virtio_mmio.c file, not in a separate header. If fact, I may
> > > move them there...
> >
> > And then you will have to copy and paste them in qemu.
> >
> > Which we do ATM for silly reasons like
> > we include linux/types.h, but I fully intend to get rid of that,
> > just tweak the linux ones slightly for portability.
>
> Ok, more than happy to do whatever will make qemu happy. An example?
>
> Paweł
See include/uapi/linux/virtio_pci.h in my tree above.
--
MST
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
