Re: [PATCH 03/22] virtio_config: make transports implement accessors.

[Rusty Russell <rusty@rustcorp.com.au>]

Sjur Brændeland <sjurbren@gmail.com> writes:
> On Thu, Mar 21, 2013 at 9:29 AM, Rusty Russell <rusty@rustcorp.com.au> wrote:
>> @@ -653,8 +676,14 @@ static int vp_set_vq_affinity(struct virtqueue *vq, int cpu)
>>  }
>>
>>  static const struct virtio_config_ops virtio_pci_config_ops = {
>> -       .get            = vp_get,
>> -       .set            = vp_set,
>> +       .get8           = vp_get8,
>> +       .set8           = vp_set8,
>> +       .get16          = vp_get16,
>> +       .set16          = vp_set16,
>> +       .get32          = vp_get32,
>> +       .set32          = vp_set32,
>> +       .get64          = vp_get64,
>> +       .set64          = vp_set64,
>>         .get_status     = vp_get_status,
>>         .set_status     = vp_set_status,
>>         .reset          = vp_reset,
>
> Would it be possible to make this simpler and less verbose somehow?
> At least three virtio devices: virtio_pci_legacy.c, virtio_mmio.c and
> soon remoteproc_virtio.c will duplicate variants of the code above.
>
> What if set8/get8 was mandatory, and the 16,32,64 variants where optional,
> and then virtio_creadX() virtio_cwriteX did the magic to make things work?

But this is a case where simpler and less verbose are opposites.  These
patches are really straightforward.  Noone can forget to write or assign
an accessor and have still work on x86 and fail for BE machines.

So I like explicit accessors, set by the backend.  But it doesn't have
to be quite this ugly.

How's this (completely untested!)

diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index e342692..6ffa542 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -219,6 +219,75 @@ out:
 }
 EXPORT_SYMBOL_GPL(register_virtio_device);
 
+static void noconv_get(struct virtio_device *vdev, unsigned offset,
+		       size_t len, void *p)
+{
+	u8 *buf = p;
+	while (len) {
+		*buf = vdev->config->get8(vdev, offset);
+		buf++;
+		offset++;
+		len--;
+	}
+}
+
+u16 virtio_config_get_noconv16(struct virtio_device *vdev, unsigned offset)
+{
+	u16 v;
+	noconv_get(vdev, offset, sizeof(v), &v);
+	return v;
+}
+EXPORT_SYMBOL_GPL(virtio_config_get_noconv16);
+
+u32 virtio_config_get_noconv32(struct virtio_device *vdev, unsigned offset)
+{
+	u32 v;
+	noconv_get(vdev, offset, sizeof(v), &v);
+	return v;
+}
+EXPORT_SYMBOL_GPL(virtio_config_get_noconv32);
+
+u64 virtio_config_get_noconv64(struct virtio_device *vdev, unsigned offset)
+{
+	u64 v;
+	noconv_get(vdev, offset, sizeof(v), &v);
+	return v;
+}
+EXPORT_SYMBOL_GPL(virtio_config_get_noconv64);
+
+static void noconv_set(struct virtio_device *vdev, unsigned offset,
+		       size_t len, const void *p)
+{
+	const u8 *buf = p;
+	while (len) {
+		vdev->config->set8(vdev, offset, *buf);
+		buf++;
+		offset++;
+		len--;
+	}
+}
+
+void virtio_config_set_noconv16(struct virtio_device *vdev,
+				unsigned offset, u16 v)
+{
+	noconv_set(vdev, offset, sizeof(v), &v);
+}
+EXPORT_SYMBOL_GPL(virtio_config_set_noconv16);
+
+void virtio_config_set_noconv32(struct virtio_device *vdev,
+				unsigned offset, u32 v)
+{
+	noconv_set(vdev, offset, sizeof(v), &v);
+}
+EXPORT_SYMBOL_GPL(virtio_config_set_noconv32);
+
+void virtio_config_set_noconv64(struct virtio_device *vdev,
+				unsigned offset, u64 v)
+{
+	noconv_set(vdev, offset, sizeof(v), &v);
+}
+EXPORT_SYMBOL_GPL(virtio_config_set_noconv64);
+
 void unregister_virtio_device(struct virtio_device *dev)
 {
 	int index = dev->index; /* save for after device release */
diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
index 9dfe116..9a9aaa0 100644
--- a/include/linux/virtio_config.h
+++ b/include/linux/virtio_config.h
@@ -286,4 +286,23 @@ static inline void virtio_cwrite64(struct virtio_device *vdev,
 		_r;							\
 	})
 
+/* Helpers for non-endian converting transports. */
+u16 virtio_config_get_noconv16(struct virtio_device *vdev, unsigned offset);
+u32 virtio_config_get_noconv32(struct virtio_device *vdev, unsigned offset);
+u64 virtio_config_get_noconv64(struct virtio_device *vdev, unsigned offset);
+void virtio_config_set_noconv16(struct virtio_device *vdev,
+				unsigned offset, u16 v);
+void virtio_config_set_noconv32(struct virtio_device *vdev,
+				unsigned offset, u32 v);
+void virtio_config_set_noconv64(struct virtio_device *vdev,
+				unsigned offset, u64 v);
+
+#define VIRTIO_CONFIG_OPS_NOCONV		\
+	.get16 = virtio_config_get_noconv16,	\
+	.set16 = virtio_config_set_noconv16,	\
+	.get32 = virtio_config_get_noconv32,	\
+	.set32 = virtio_config_set_noconv32,	\
+	.get64 = virtio_config_get_noconv64,	\
+	.set64 = virtio_config_set_noconv64
+
 #endif /* _LINUX_VIRTIO_CONFIG_H */
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization