Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation

On Wed, Sep 05, 2012 at 08:15:36PM +0200, Sjur Brændeland wrote:

Hi Michael.

quoted

quoted

If the device then asks for VIRTIO_CONSOLE_F_DMA_MEM
when DMA is not supported, virtio will do BUG_ON() from
virtio_check_driver_offered_feature().

Is this acceptable or should we add a check in virtcons_probe()
and let the probing fail instead?

E.g:
      /* Refuse to bind if F_DMA_MEM request cannot be met */
      if (!VIRTIO_CONSOLE_HAS_DMA &&
          (vdev->config->get_features(vdev) & (1 << VIRTIO_CONSOLE_F_DMA_MEM))){
              dev_err(&vdev->dev,
                      "DMA_MEM requested, but arch does not support DMA\n");
              err = -EINVAL;
              goto fail;
      }

Regards,
Sjur

Failing probe would be cleaner. But there is still a problem:
old driver will happily bind to that device and then
fail to work, right?

Not just fail to work, the kernel will panic on the BUG_ON().
Remoteproc gets the virtio configuration from firmware loaded
from user space. So this type of problem might be triggered
for other virtio drivers as well.

how?

quoted

virtio pci has revision id for this, but remoteproc doesn't
seem to have anything similar. Or did I miss it?

No there are currently no sanity check of
virtio type and feature bits in remoteproc.
One option may be to add this...

you can not fix the past.

quoted

If not -
we probably need to use a different
device id, and not a feature bit.

But if I create a new virtio console type, remoteproc
could still call the existing virtio_console with random
bad feature bits, causing kernel panic.

cirtio core checks device id - this should not happen.

Even if we fix this particular problem, the general problem
still exists: bogus virtio declarations in remoteproc's firmware
may cause BUG_ON().

which BUG_ON exactly?

(Note the fundamental difference
between visualizations and remoteproc. For remoteproc
the virtio configuration comes from binaries loaded from
user space).

So maybe we should look for a more generic solution, e.g.
changing virtio probe functionality so that devices with
bad feature bits will not trigger BUG_ON(), but rather refuse
to bind the driver.

Regards,
Sjur