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

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

On Thu, 2015-01-15 at 16:51 +0000, Michael S. Tsirkin wrote:
> > +             uint64_t addr = virt_to_phys(info->queue);
> 
> Kernel normally uses u64 for this type.

Sure, well spotted.

> > +
> > +             writel(addr & 0xffffffff,
> > +                             vm_dev->base + VIRTIO_MMIO_QUEUE_DESC_LOW);
> > +             writel((addr >> 32) & 0xffffffff,
> > +                             vm_dev->base + VIRTIO_MMIO_QUEUE_DESC_HIGH);
> > +
> > +             addr += info->num * sizeof(struct vring_desc);
> > +             writel(addr & 0xffffffff,
> > +                             vm_dev->base + VIRTIO_MMIO_QUEUE_AVAIL_LOW);
> > +             writel((addr >> 32) & 0xffffffff,
> > +                             vm_dev->base + VIRTIO_MMIO_QUEUE_AVAIL_HIGH);
> 
> 0xffffffff isn't really needed, is it?

I admit I'm never sure what are the narrowing side effects. You are
probably right that u64 >> 32 will be always 32 bit.

> > +
> > +             addr += sizeof(struct vring_avail) + info->num * sizeof(__u16);
> > +             addr += VIRTIO_MMIO_VRING_ALIGN - 1;
> > +             addr &= ~(VIRTIO_MMIO_VRING_ALIGN - 1);
> 
> 
> Host no longer knows the alignment, so why is it needed?

[skipped the spec reference, it's a separate discussion]

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

> > +             writel(addr & 0xffffffff,
> > +                             vm_dev->base + VIRTIO_MMIO_QUEUE_USED_LOW);
> > +             writel((addr >> 32) & 0xffffffff,
> > +                             vm_dev->base + VIRTIO_MMIO_QUEUE_USED_HIGH);
> > +
> > +             writel(1, vm_dev->base + VIRTIO_MMIO_QUEUE_READY);
> > +     }
> >
> >       /* Create the vring */
> >       vq = vring_new_virtqueue(index, info->num, VIRTIO_MMIO_VRING_ALIGN, vdev,

[...]

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

> > @@ -476,16 +501,26 @@ static int virtio_mmio_probe(struct platform_device *pdev)
> >
> >       /* Check device version */
> >       vm_dev->version = readl(vm_dev->base + VIRTIO_MMIO_VERSION);
> > -     if (vm_dev->version != 1) {
> > +     if (vm_dev->version < 1 || vm_dev->version > 2) {
> >               dev_err(&pdev->dev, "Version %ld not supported!\n",
> >                               vm_dev->version);
> >               return -ENXIO;
> >       }
> >
> >       vm_dev->vdev.id.device = readl(vm_dev->base + VIRTIO_MMIO_DEVICE_ID);
> > +     if (vm_dev->vdev.id.device == 0) {
> > +             /*
> > +              * ID 0 means a dummy (placeholder) device, skip quietly
> > +              * (as in: no error) with no further actions
> > +              */
> > +             return 0;
> 
> Necessary?
> We don't have drivers for this id anyway.

I'm not sure if you are joking or not, after the battle we fought over
it.

The short answer is: yes. Necessary.

"4.2.2 MMIO Device Register Layout

[...]

Virtio Subsystem Device ID
See 5 Device Types for possible values. Value zero (0x0) is used to de-
fine a system memory map with placeholder devices at static, well known
addresses, assigning functions to them depending on user’s needs.

[...]

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

> > +     }
> 
> Need to also
>         1. validate that feature bit VIRTIO_1 is set
>         2. validate that ID is not for a legacy device
> 
> otherwise device specific drivers might get invoked
> on future devices (e.g. when we update balloon for 1.0)
> and they not do the right thing.

I'm not following you, but I admit I haven't though this problem
thoroughly. If you can volunteer an example of things going on, it would
be useful. Either way, I'll think about it again.

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

> > -/* Guest's memory page size in bytes - Write Only */
> > +/* Guest's memory page size in bytes - Write Only
> > + * LEGACY DEVICES ONLY! */
> 
> This is not the preferred style for multi-line comments :)

Fact. Will fix.

> Also - maybe add a flag to selectively disable legacy
> or modern macros?
> Might be clearer than comments that, after all, never compile.

As in, a bunch of #ifdefs disabling the legacy lines of code? Doable,
although I'm not sure how beautiful would that be. Will have a look, but
it probably would only make sense with CONFIG_VIRTIO_MMIO_LEGACY option.

Paweł

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