Message ID | 1304170225-4859-2-git-send-email-levinsasha928@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
* Sasha Levin <levinsasha928@gmail.com> wrote: > +#define PCI_VENDOR_ID_REDHAT_QUMRANET 0x1af4 > +#define PCI_DEVICE_ID_VIRTIO_RNG 0x1004 > +#define PCI_SUBSYSTEM_VENDOR_ID_REDHAT_QUMRANET 0x1af4 > +#define PCI_SUBSYSTEM_ID_VIRTIO_RNG 0x0004 > +#define PCI_VIRTIO_RNG_DEVNUM 4 > + > +#define VIRTIO_RNG_IRQ 11 > +#define VIRTIO_RNG_PIN 1 > + > +#define NUM_VIRT_QUEUES 1 > + > +#define VIRTIO_RNG_QUEUE_SIZE 128 > + > +struct rng_device { > + uint8_t status; > + uint16_t config_vector; > + int fd_rng; > + > + /* virtio queue */ > + uint16_t queue_selector; > + struct virt_queue vqs[NUM_VIRT_QUEUES]; > + void *jobs[NUM_VIRT_QUEUES]; > +}; Really, have you *looked* at this source code from a distance? Does it look neat and orderly to you?? It does not look readable to me at all: it's full of vertical spacing misalignments even within the *same* syntactic unit. (Not to mention file-scope alignment convention which is all over the place.) > +static struct ioport_operations virtio_rng_io_ops = { > + .io_in = virtio_rng_pci_io_in, > + .io_out = virtio_rng_pci_io_out, > +}; > + > +static struct pci_device_header virtio_rng_pci_device = { > + .vendor_id = PCI_VENDOR_ID_REDHAT_QUMRANET, > + .device_id = PCI_DEVICE_ID_VIRTIO_RNG, > + .header_type = PCI_HEADER_TYPE_NORMAL, > + .revision_id = 0, > + .class = 0x010000, > + .subsys_vendor_id = PCI_SUBSYSTEM_VENDOR_ID_REDHAT_QUMRANET, > + .subsys_id = PCI_SUBSYSTEM_ID_VIRTIO_RNG, > + .bar[0] = IOPORT_VIRTIO_RNG | PCI_BASE_ADDRESS_SPACE_IO, > + .irq_pin = VIRTIO_RNG_PIN, > + .irq_line = VIRTIO_RNG_IRQ, > +}; Same here! It looks like as if random new lines were jumbled within something copy & pasted from elsewhere, with no care taken that they look good together ... There's also other details like: > + int fd_rng; The _rng postfix is superfluous, we already know that this is a rng thing, it's within struct rng_device! The result is suboptimal usage like this: rng_device.fd_rng which says 'rng' twice and clutters the code needlessly. Plus remember the blk_device argument i made yesterday? The *same* issue gets reintroduced here: static struct rng_device rng_device; We generally try to use different names for the structure and the local (or as here, global) variables - and we try to use *short* (while still expressive) names for variables. So the proper and canonical naming, in line with blk_dev and net_dev would be rng_dev, not rng_device. The code looks correct but we really need to try harder to keep the tools/kvm/ code maintainable! Thanks, Ingo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, May 5, 2011 at 9:54 AM, Ingo Molnar <mingo@elte.hu> wrote: > The code looks correct but we really need to try harder to keep the tools/kvm/ > code maintainable! Aww, I must have been on crack when I applied that. Sasha, please send a follow up patch to address Ingo's comments. Pekka -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Pekka Enberg <penberg@kernel.org> wrote: > On Thu, May 5, 2011 at 9:54 AM, Ingo Molnar <mingo@elte.hu> wrote: > > The code looks correct but we really need to try harder to keep the tools/kvm/ > > code maintainable! > > Aww, I must have been on crack when I applied that. Sasha, please send > a follow up patch to address Ingo's comments. another thing i noticed: the patch introduces some new uint_* usages. Once things are fixed can we somehow kill that datatype intelligently, so that if new code uses it the build breaks or so? Thanks, Ingo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, May 5, 2011 at 10:12 AM, Ingo Molnar <mingo@elte.hu> wrote: > another thing i noticed: the patch introduces some new uint_* usages. > > Once things are fixed can we somehow kill that datatype intelligently, so that > if new code uses it the build breaks or so? I guess we could do include/stdint.h that does #error You lose. or something. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
btw., another, wider code structure thought: with the recent (very nice!) amassing of new virtio drivers, isnt it time to put them into their own directory in tools/kvm/virtio/? That way there would be: tools/kvm/virtio/core.c tools/kvm/virtio/net.c tools/kvm/virtio/blk.c tools/kvm/virtio/rng.c etc. Thanks, Ingo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2011-05-05 at 10:14 +0300, Pekka Enberg wrote: > On Thu, May 5, 2011 at 10:12 AM, Ingo Molnar <mingo@elte.hu> wrote: > > another thing i noticed: the patch introduces some new uint_* usages. > > > > Once things are fixed can we somehow kill that datatype intelligently, so that > > if new code uses it the build breaks or so? > > I guess we could do > > include/stdint.h > > that does > > #error You lose. > > or something. I'll make a pass to clean up formatting and datatypes throughout the virtio drivers once I finish with the virtio-blk patch.
On Thu, 2011-05-05 at 10:14 +0300, Pekka Enberg wrote: > On Thu, May 5, 2011 at 10:12 AM, Ingo Molnar <mingo@elte.hu> wrote: > > another thing i noticed: the patch introduces some new uint_* usages. > > > > Once things are fixed can we somehow kill that datatype intelligently, so that > > if new code uses it the build breaks or so? > > I guess we could do > > include/stdint.h > > that does > > #error You lose. > > or something. How would we handle <linux/inet.h> which uses uint*_t types?
* Sasha Levin <levinsasha928@gmail.com> wrote: > On Thu, 2011-05-05 at 10:14 +0300, Pekka Enberg wrote: > > On Thu, May 5, 2011 at 10:12 AM, Ingo Molnar <mingo@elte.hu> wrote: > > > another thing i noticed: the patch introduces some new uint_* usages. > > > > > > Once things are fixed can we somehow kill that datatype intelligently, so that > > > if new code uses it the build breaks or so? > > > > I guess we could do > > > > include/stdint.h > > > > that does > > > > #error You lose. > > > > or something. > > How would we handle <linux/inet.h> which uses uint*_t types? Yeah, it's not realistic to get rid of them like that - also, system include files are using it as well. Thanks, Ingo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/tools/kvm/Makefile b/tools/kvm/Makefile index 659bc35..19e7b9e 100644 --- a/tools/kvm/Makefile +++ b/tools/kvm/Makefile @@ -38,6 +38,7 @@ OBJS += kvm-run.o OBJS += qcow.o OBJS += mptable.o OBJS += threadpool.o +OBJS += virtio-rng.o DEPS := $(patsubst %.o,%.d,$(OBJS)) diff --git a/tools/kvm/include/kvm/ioport.h b/tools/kvm/include/kvm/ioport.h index 2fdcca4..6020124 100644 --- a/tools/kvm/include/kvm/ioport.h +++ b/tools/kvm/include/kvm/ioport.h @@ -12,6 +12,8 @@ #define IOPORT_VIRTIO_CONSOLE_SIZE 256 #define IOPORT_VIRTIO_NET 0xe200 /* Virtio network device */ #define IOPORT_VIRTIO_NET_SIZE 256 +#define IOPORT_VIRTIO_RNG 0xf200 /* Virtio network device */ +#define IOPORT_VIRTIO_RNG_SIZE 256 struct kvm; diff --git a/tools/kvm/include/kvm/virtio-rng.h b/tools/kvm/include/kvm/virtio-rng.h new file mode 100644 index 0000000..7015c1f --- /dev/null +++ b/tools/kvm/include/kvm/virtio-rng.h @@ -0,0 +1,8 @@ +#ifndef KVM__RNG_VIRTIO_H +#define KVM__RNG_VIRTIO_H + +struct kvm; + +void virtio_rng__init(struct kvm *kvm); + +#endif /* KVM__RNG_VIRTIO_H */ diff --git a/tools/kvm/virtio-rng.c b/tools/kvm/virtio-rng.c new file mode 100644 index 0000000..d37fb5e --- /dev/null +++ b/tools/kvm/virtio-rng.c @@ -0,0 +1,184 @@ +#include "kvm/virtio-rng.h" + +#include "kvm/virtio-pci.h" + +#include "kvm/disk-image.h" +#include "kvm/virtio.h" +#include "kvm/ioport.h" +#include "kvm/mutex.h" +#include "kvm/util.h" +#include "kvm/kvm.h" +#include "kvm/pci.h" +#include "kvm/threadpool.h" + +#include <linux/virtio_ring.h> +#include <linux/virtio_rng.h> + +#include <fcntl.h> +#include <sys/types.h> +#include <sys/stat.h> +#include <inttypes.h> +#include <pthread.h> + +#define PCI_VENDOR_ID_REDHAT_QUMRANET 0x1af4 +#define PCI_DEVICE_ID_VIRTIO_RNG 0x1004 +#define PCI_SUBSYSTEM_VENDOR_ID_REDHAT_QUMRANET 0x1af4 +#define PCI_SUBSYSTEM_ID_VIRTIO_RNG 0x0004 +#define PCI_VIRTIO_RNG_DEVNUM 4 + +#define VIRTIO_RNG_IRQ 11 +#define VIRTIO_RNG_PIN 1 + +#define NUM_VIRT_QUEUES 1 + +#define VIRTIO_RNG_QUEUE_SIZE 128 + +struct rng_device { + uint8_t status; + uint16_t config_vector; + int fd_rng; + + /* virtio queue */ + uint16_t queue_selector; + struct virt_queue vqs[NUM_VIRT_QUEUES]; + void *jobs[NUM_VIRT_QUEUES]; +}; + +static struct rng_device rng_device; + +static bool virtio_rng_pci_io_in(struct kvm *kvm, uint16_t port, void *data, int size, uint32_t count) +{ + unsigned long offset; + bool ret = true; + + offset = port - IOPORT_VIRTIO_RNG; + + switch (offset) { + case VIRTIO_PCI_HOST_FEATURES: + case VIRTIO_PCI_GUEST_FEATURES: + case VIRTIO_PCI_QUEUE_SEL: + case VIRTIO_PCI_QUEUE_NOTIFY: + ret = false; + break; + case VIRTIO_PCI_QUEUE_PFN: + ioport__write32(data, rng_device.vqs[rng_device.queue_selector].pfn); + break; + case VIRTIO_PCI_QUEUE_NUM: + ioport__write16(data, VIRTIO_RNG_QUEUE_SIZE); + break; + case VIRTIO_PCI_STATUS: + ioport__write8(data, rng_device.status); + break; + case VIRTIO_PCI_ISR: + ioport__write8(data, 0x1); + kvm__irq_line(kvm, VIRTIO_RNG_IRQ, 0); + break; + case VIRTIO_MSI_CONFIG_VECTOR: + ioport__write16(data, rng_device.config_vector); + break; + default: + ret = false; + }; + + return ret; +} + +static bool virtio_rng_do_io_request(struct kvm *self, struct virt_queue *queue) +{ + struct iovec iov[VIRTIO_RNG_QUEUE_SIZE]; + uint16_t out, in, head; + unsigned int len = 0; + + head = virt_queue__get_iov(queue, iov, &out, &in, self); + len = readv(rng_device.fd_rng, iov, in); + virt_queue__set_used_elem(queue, head, len); + + return true; +} + +static void virtio_rng_do_io(struct kvm *kvm, void *param) +{ + struct virt_queue *vq = param; + + while (virt_queue__available(vq)) { + virtio_rng_do_io_request(kvm, vq); + kvm__irq_line(kvm, VIRTIO_RNG_IRQ, 1); + } +} + +static bool virtio_rng_pci_io_out(struct kvm *kvm, uint16_t port, void *data, int size, uint32_t count) +{ + unsigned long offset; + bool ret = true; + + offset = port - IOPORT_VIRTIO_RNG; + + switch (offset) { + case VIRTIO_MSI_QUEUE_VECTOR: + case VIRTIO_PCI_GUEST_FEATURES: + break; + case VIRTIO_PCI_QUEUE_PFN: { + struct virt_queue *queue; + void *p; + + queue = &rng_device.vqs[rng_device.queue_selector]; + queue->pfn = ioport__read32(data); + p = guest_flat_to_host(kvm, queue->pfn << 12); + + vring_init(&queue->vring, VIRTIO_RNG_QUEUE_SIZE, p, 4096); + + rng_device.jobs[rng_device.queue_selector] = + thread_pool__add_jobtype(kvm, virtio_rng_do_io, queue); + + break; + } + case VIRTIO_PCI_QUEUE_SEL: + rng_device.queue_selector = ioport__read16(data); + break; + case VIRTIO_PCI_QUEUE_NOTIFY: { + uint16_t queue_index; + queue_index = ioport__read16(data); + thread_pool__signal_work(rng_device.jobs[queue_index]); + break; + } + case VIRTIO_PCI_STATUS: + rng_device.status = ioport__read8(data); + break; + case VIRTIO_MSI_CONFIG_VECTOR: + rng_device.config_vector = VIRTIO_MSI_NO_VECTOR; + break; + default: + ret = false; + }; + + return ret; +} + +static struct ioport_operations virtio_rng_io_ops = { + .io_in = virtio_rng_pci_io_in, + .io_out = virtio_rng_pci_io_out, +}; + +static struct pci_device_header virtio_rng_pci_device = { + .vendor_id = PCI_VENDOR_ID_REDHAT_QUMRANET, + .device_id = PCI_DEVICE_ID_VIRTIO_RNG, + .header_type = PCI_HEADER_TYPE_NORMAL, + .revision_id = 0, + .class = 0x010000, + .subsys_vendor_id = PCI_SUBSYSTEM_VENDOR_ID_REDHAT_QUMRANET, + .subsys_id = PCI_SUBSYSTEM_ID_VIRTIO_RNG, + .bar[0] = IOPORT_VIRTIO_RNG | PCI_BASE_ADDRESS_SPACE_IO, + .irq_pin = VIRTIO_RNG_PIN, + .irq_line = VIRTIO_RNG_IRQ, +}; + +void virtio_rng__init(struct kvm *kvm) +{ + rng_device.fd_rng = open("/dev/urandom", O_RDONLY); + if (rng_device.fd_rng < 0) + die("Failed initializing RNG"); + + pci__register(&virtio_rng_pci_device, PCI_VIRTIO_RNG_DEVNUM); + + ioport__register(IOPORT_VIRTIO_RNG, &virtio_rng_io_ops, IOPORT_VIRTIO_RNG_SIZE); +}
Enable virtio-rng, a virtio random number generator. Guest kernel should be compiled with CONFIG_HW_RANDOM_VIRTIO. Once enabled, A RNG device will be located at /dev/hwrng. Signed-off-by: Sasha Levin <levinsasha928@gmail.com> --- tools/kvm/Makefile | 1 + tools/kvm/include/kvm/ioport.h | 2 + tools/kvm/include/kvm/virtio-rng.h | 8 ++ tools/kvm/virtio-rng.c | 184 ++++++++++++++++++++++++++++++++++++ 4 files changed, 195 insertions(+), 0 deletions(-) create mode 100644 tools/kvm/include/kvm/virtio-rng.h create mode 100644 tools/kvm/virtio-rng.c