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

["Michael S. Tsirkin" <mst@redhat.com>]

On Thu, Jan 15, 2015 at 07:51:32PM +0200, Michael S. Tsirkin wrote:
> On Thu, Jan 15, 2015 at 05:32:38PM +0000, Pawel Moll wrote:
> > 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)
> 
> 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.
> And maybe add a TODO item - we can optimize by allocating chunks
> separately.
> 
> > > > +             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.
> 
> 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?
> 
> > > > @@ -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.
> 
> Sorry, I don't remember anymore. Just asking.
> 
> > 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."
> 
> 
> 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?
> 
> > > > +     }
> > > 
> > > 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.
> 
> 1. you need to check ID 0, and assume rev 0. If device also
>    says it needs rev 1, fail. E.g. see my patch for virtio_pci_modern:
>         if (virtio_device_is_legacy_only(vp_dev->vdev.id))
>                 return -ENODEV;
> 
>    you can find the code in my tree, see below.
> 
> 
> 2. it's easy - just get features on probe and validate VIRTIO_1
>    bit is set.
> 
>    s390 does it differently since same device supports version 1 and 0.
>    No example yet - I forgot to code this up for virtio pci.  I'll copy you
>    on patch.

I forgot: s390 does have this code actually:

        if (vcdev->revision >= 1 &&
            !__virtio_test_bit(vdev, VIRTIO_F_VERSION_1)) {
                dev_err(&vdev->dev, "virtio: device uses revision 1 "
                        "but does not have VIRTIO_F_VERSION_1\n");
                return -EINVAL;
        }

I think that's an easier way to do it for PCI as well,
will send patch.


> 
> 
> 
> > > @@ -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.
> 
> > > > -/* 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ł
> 
> 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
> 
> -- 
> MST
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization