linux-kernel - Re: [RFC 1/5] virtio-pci: flexible configuration layout

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <878vnab85w.fsf@rustcorp.com.au>
Date:	Mon, 21 Nov 2011 11:31:31 +1030
From:	Rusty Russell <rusty@...tcorp.com.au>
To:	Sasha Levin <levinsasha928@...il.com>, linux-kernel@...r.kernel.org
Cc:	mst@...hat.com, virtualization@...ts.linux-foundation.org,
	kvm@...r.kernel.org, penberg@...nel.org, mingo@...e.hu
Subject: Re: [RFC 1/5] virtio-pci: flexible configuration layout

On Tue, 15 Nov 2011 01:43:13 +0200, Sasha Levin <levinsasha928@...il.com> wrote:
> From: "Michael S. Tsirkin" <mst@...hat.com>
> 
> Add a flexible mechanism to specify virtio configuration layout, using
> pci vendor-specific capability.  A separate capability is used for each
> of common, device specific and data-path accesses.

OK, a couple of minor suggestions:

> +static int virtio_pci_iomap(struct virtio_pci_device *vp_dev)
> +{
> +	vp_dev->isr_map = virtio_pci_map_cfg(vp_dev,
> +					     VIRTIO_PCI_CAP_ISR_CFG,
> +					     sizeof(u8));
> +	vp_dev->notify_map = virtio_pci_map_cfg(vp_dev,
> +						VIRTIO_PCI_CAP_NOTIFY_CFG,
> +						sizeof(u16));
> +	vp_dev->common_map = virtio_pci_map_cfg(vp_dev,
> +						VIRTIO_PCI_CAP_COMMON_CFG,
> +						sizeof(u32));
> +	vp_dev->device_map = virtio_pci_map_cfg(vp_dev,
> +						VIRTIO_PCI_CAP_DEVICE_CFG,
> +						sizeof(u8));
> +
> +	if (!vp_dev->notify_map || !vp_dev->common_map ||
> +	    !vp_dev->device_map) {
> +		/*
> +		 * If not all capabilities present, map legacy PIO.
> +		 * Legacy access is at BAR 0. We never need to map
> +		 * more than 256 bytes there, since legacy config space
> +		 * used PIO which has this size limit.
> +		 * */
> +		vp_dev->legacy_map = pci_iomap(vp_dev->pci_dev, 0, 256);
> +		if (!vp_dev->legacy_map) {
> +			dev_err(&vp_dev->vdev.dev, "Unable to map legacy PIO");
> +			goto err;
> +		}
> +	}

Please don't mix the two.  Ever, under any circumstances.  If it helps,
put CONFIG_VIRTIO_PCI_LEGACY #ifdefs in there:

	vp_dev->common_map = virtio_pci_map_cfg(vp_dev,
						VIRTIO_PCI_CAP_COMMON_CFG,
						sizeof(u32));
        /* Something horribly wrong? */
        if (IS_ERR(vp_dev->common_map)) {
                err = PTR_ERR(vp_dev->common_map);
                goto fail;
        }

        /* Not found? */
        if (!vp_dev->common_map)
                return legacy_setup(vp_dev, ...);

        ...

Practice has shown that if we allow something, it'll be done.  We should
break horribly on malformed devices, and really really only fall back
when we have a well-formed, but old, device.

And various other legacy paths can happily use:

#ifdef CONFIG_VIRTIO_PCI_LEGACY
#define is_legacy(vp_dev) ((vp_dev)->legacy_map != NULL)
#else
#define is_legacy(vp_dev) false
#endif

Here's suggested Kconfig entry:

config VIRTIO_PCI_LEGACY
	bool
        default y
	depends on VIRTIO_PCI
	---help---
	  Look out into your driveway.  Do you have a flying car?  If
          so, you can happily disable this option and virtio will not
          break.  Otherwise, leave it set.  Unless you're testing what
          life will be like in The Future.

Cheers,
Rusty.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/