Linux Virtualization - Jan 2015 - [RFC] virtio-mmio: Update the device to OASIS spec version

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?

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.



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

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,
> > > > +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.
> > > > @@ -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?
Not to me, no. There will be a vm_dev registered with an _illegal_ ID.
> > > > +     }
> > > 
> > > 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.
Ok, will have a look.
> > > @@ -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.
> > > > -/* 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
I have no idea what are you talking about here. Why would userspace ever
access the memory mapped control registers? 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...

Pawe?

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