Re: [RFC] virtio-mmio: Update the device to OASIS spec version

[Pawel Moll <pawel.moll@arm.com>]

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.

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

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

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

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

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization