Re: [PATCH 1/6] virtio: wrap find_vqs

[Cornelia Huck]

On Wed, 29 Mar 2017 23:48:44 +0300
"Michael S. Tsirkin"  wrote:

> We are going to add more parameters to find_vqs, let's wrap the call so
> we don't need to tweak all drivers every time.
> 
> Signed-off-by: Michael S. Tsirkin 
> ---
>  drivers/block/virtio_blk.c | 3 +--
>  drivers/char/virtio_console.c  | 6 +++---
>  drivers/crypto/virtio/virtio_crypto_core.c | 3 +--
>  drivers/gpu/drm/virtio/virtgpu_kms.c   | 3 +--
>  drivers/net/caif/caif_virtio.c | 3 +--
>  drivers/net/virtio_net.c   | 3 +--
>  drivers/rpmsg/virtio_rpmsg_bus.c   | 2 +-
>  drivers/scsi/virtio_scsi.c | 3 +--
>  drivers/virtio/virtio_balloon.c| 3 +--
>  drivers/virtio/virtio_input.c  | 3 +--
>  include/linux/virtio_config.h  | 9 +
>  net/vmw_vsock/virtio_transport.c   | 6 +++---
>  12 files changed, 24 insertions(+), 23 deletions(-)

Regardless whether that context thing is the right thing to do, this
looks like a sensible cleanup.

Re: [PATCH] linux/types.h: enable endian checks for all sparse builds

[Cornelia Huck]

On Thu, 8 Dec 2016 04:29:39 +0200
"Michael S. Tsirkin"  wrote:

> By now, linux is mostly endian-clean. Enabling endian-ness
> checks for everyone produces about 200 new sparse warnings for me -
> less than 10% over the 2000 sparse warnings already there.

Out of curiousity: Where do most of those warnings show up?

> 
> Not a big deal, OTOH enabling this helps people notice
> they are introducing new bugs.
> 
> So let's just drop __CHECK_ENDIAN__. Follow-up patches
> can drop distinction between __bitwise and __bitwise__.
> 
> Cc: Linus Torvalds 
> Suggested-by: Christoph Hellwig 
> Signed-off-by: Michael S. Tsirkin 
> ---
> 
> Linus, could you ack this for upstream? If yes I'll
> merge through my tree as a replacement for enabling
> this just for virtio.
> 
>  include/uapi/linux/types.h | 4 
>  1 file changed, 4 deletions(-)
> 
> diff --git a/include/uapi/linux/types.h b/include/uapi/linux/types.h
> index acf0979..41e5914 100644
> --- a/include/uapi/linux/types.h
> +++ b/include/uapi/linux/types.h
> @@ -23,11 +23,7 @@
>  #else
>  #define __bitwise__
>  #endif
> -#ifdef __CHECK_ENDIAN__
>  #define __bitwise __bitwise__
> -#else
> -#define __bitwise
> -#endif
> 
>  typedef __u16 __bitwise __le16;
>  typedef __u16 __bitwise __be16;

FWIW, I like this better than just enabling it for the virtio code.

--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [virtio-dev] Re: [PATCH v3] crypto: add virtio-crypto driver

[Cornelia Huck]

On Tue, 29 Nov 2016 01:37:44 +
"Gonglei (Arei)"  wrote:

> > On Mon, 28 Nov 2016 20:08:23 +0800
> > Gonglei  wrote:
> > 
> > > +static int virtcrypto_update_status(struct virtio_crypto *vcrypto)
> > > +{
> > > + u32 status;
> > > + int err;
> > > +
> > > + virtio_cread(vcrypto->vdev,
> > > + struct virtio_crypto_config, status, );
> > > +
> > > + /* Ignore unknown (future) status bits */
> > > + status &= VIRTIO_CRYPTO_S_HW_READY;
> > 
> > I'm wondering what the driver really should do if it encounters unknown
> > status bits.
> > 
> > I'd expect that new status bits are guarded by a feature bit and that
> > the device should not set status bits if the respective feature bit has
> > not been negotiated. Therefore, unknown status bits would be a host
> > error and the driver should consider the device to be broken.
> > 
> > Thoughts?
> > 
> I agree with you. 
> 
> The reasonable way is reset the device if the driver
> receive an unknown status IMO.

What about setting FAILED in the generic virtio status? This indicates
to the host that the driver 'has given up on the device', as the spec
puts it. If the driver simply resets it, chances are that we will end
up in the same situation again (after all, that's a host bug).

Or/additionally use virtio_break_device(), as a quick grep revealed
that qemu, for one, does not do anything with FAILED. That way at least
the driver will stop mucking with the device.

--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3] crypto: add virtio-crypto driver

[Cornelia Huck]

On Tue, 29 Nov 2016 09:25:49 +
Stefan Hajnoczi  wrote:

> On Tue, Nov 29, 2016 at 08:22:58AM +, Gonglei (Arei) wrote:
> > Hi,
> > 
> > > > > > +source "drivers/crypto/virtio/Kconfig"
> > > > > > +
> > > > > >  endif # CRYPTO_HW
> > > > > > diff --git a/drivers/crypto/Makefile b/drivers/crypto/Makefile
> > > > > > index ad7250f..bc53cb8 100644
> > > > > > --- a/drivers/crypto/Makefile
> > > > > > +++ b/drivers/crypto/Makefile
> > > > > > @@ -32,3 +32,4 @@ obj-$(CONFIG_CRYPTO_DEV_VMX) += vmx/
> > > > > >  obj-$(CONFIG_CRYPTO_DEV_SUN4I_SS) += sunxi-ss/
> > > > > >  obj-$(CONFIG_CRYPTO_DEV_ROCKCHIP) += rockchip/
> > > > > >  obj-$(CONFIG_CRYPTO_DEV_CHELSIO) += chelsio/
> > > > > > +obj-$(CONFIG_CRYPTO_DEV_VIRTIO) += virtio/
> > > > > > diff --git a/drivers/crypto/virtio/Kconfig 
> > > > > > b/drivers/crypto/virtio/Kconfig
> > > > > > new file mode 100644
> > > > > > index 000..ceae88c
> > > > > > --- /dev/null
> > > > > > +++ b/drivers/crypto/virtio/Kconfig
> > > > > > @@ -0,0 +1,10 @@
> > > > > > +config CRYPTO_DEV_VIRTIO
> > > > > > +   tristate "VirtIO crypto driver"
> > > > > > +   depends on VIRTIO
> > > > > > +select CRYPTO_AEAD
> > > > > > +select CRYPTO_AUTHENC
> > > > > > +select CRYPTO_BLKCIPHER
> > > > >
> > > > > Inconsistent tab vs space whitespace usage.
> > > > >
> > > Will fix.
> > > 
> > > > > > +   default m
> > > > > > +   help
> > > > > > + This driver provides support for virtio crypto device. If you
> > > > > > + choose 'M' here, this module will be called virtio-crypto.
> > > > >
> > > > > All the other virtio drivers use underscore ('_') instead of hyphen
> > > > > ('-').
> > > >
> > > > Except virtio-rng.
> > > >
> > > > >  I suggest calling it virtio_crypto for consistency.
> > > > >
> > > OK, I will change the Makefile to fix it.
> > > 
> > 
> > I tried to do this, but I failed. Because virtio_crypto.ko
> > consists of more than one source file which include tree files currently,
> > and virtio_crypto.ko matchs the name of virtio_crypto.c.
> > That's different with all other virtio drivers.
> 
> Can you rename virtio_crypto.c to virtio_crypto_core.c?

+1

I think that's the way to go.

--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3] crypto: add virtio-crypto driver

[Cornelia Huck]

On Mon, 28 Nov 2016 20:08:23 +0800
Gonglei  wrote:

> +static int virtcrypto_update_status(struct virtio_crypto *vcrypto)
> +{
> + u32 status;
> + int err;
> +
> + virtio_cread(vcrypto->vdev,
> + struct virtio_crypto_config, status, );
> +
> + /* Ignore unknown (future) status bits */
> + status &= VIRTIO_CRYPTO_S_HW_READY;

I'm wondering what the driver really should do if it encounters unknown
status bits.

I'd expect that new status bits are guarded by a feature bit and that
the device should not set status bits if the respective feature bit has
not been negotiated. Therefore, unknown status bits would be a host
error and the driver should consider the device to be broken.

Thoughts?

> +
> + if (vcrypto->status == status)
> + return 0;
> +
> + vcrypto->status = status;
> +
> + if (vcrypto->status & VIRTIO_CRYPTO_S_HW_READY) {
> + err = virtcrypto_dev_start(vcrypto);
> + if (err) {
> + dev_err(>vdev->dev,
> + "Failed to start virtio crypto device.\n");
> + virtcrypto_dev_stop(vcrypto);
> + return -EPERM;
> + }
> + dev_info(>vdev->dev, "Accelerator is ready\n");
> + } else {
> + virtcrypto_dev_stop(vcrypto);
> + dev_info(>vdev->dev, "Accelerator is not ready\n");
> + }
> +
> + return 0;
> +}
> +

--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html