Hi Rusty,
On Sun, Mar 24, 2013 at 5:24 AM, Rusty Russell <rusty@xxxxxxxxxxxxxxx> wrote:
>
> Sjur Brændeland <sjurbren@xxxxxxxxx> writes:
> > On Thu, Mar 21, 2013 at 9:29 AM, Rusty Russell <rusty@xxxxxxxxxxxxxxx> wrote:
> > 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!)
Looks really good to me. This way we avoid copying code around between
virtio devices.
BTW: If you are planning to merge this is for 3.10 we will get some
compile issues in linux-next when merging with my latest remoteproc patches.
But with the changes below it should be easy enough to fix.
Thanks,
Sjur
>
> 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@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
