Marc-André Lureau <marcandre.lur...@gmail.com> writes:
> Hi
>
> On Mon, Feb 29, 2016 at 7:40 PM, Markus Armbruster <arm...@redhat.com> wrote:
>> Signed-off-by: Markus Armbruster <arm...@redhat.com>
>> ---
>> hw/misc/ivshmem.c | 189
>> +++++++++++++++++++++++++++---------------------------
>> 1 file changed, 96 insertions(+), 93 deletions(-)
>>
>> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
>> index 5d33be4..fc46666 100644
>> --- a/hw/misc/ivshmem.c
>> +++ b/hw/misc/ivshmem.c
>> @@ -564,14 +564,106 @@ static void setup_interrupt(IVShmemState *s, int
>> vector)
>> }
>> }
>>
>> +static void process_msg_shmem(IVShmemState *s, int fd)
>> +{
>> + Error *err = NULL;
>> + void *ptr;
>> +
>> + if (memory_region_is_mapped(&s->ivshmem)) {
>> + error_report("shm already initialized");
>> + close(fd);
>> + return;
>> + }
>> +
>> + if (check_shm_size(s, fd, &err) == -1) {
>> + error_report_err(err);
>> + close(fd);
>> + return;
>> + }
>> +
>> + /* mmap the region and map into the BAR2 */
>> + ptr = mmap(0, s->ivshmem_size, PROT_READ | PROT_WRITE, MAP_SHARED, fd,
>> 0);
>> + if (ptr == MAP_FAILED) {
>> + error_report("Failed to mmap shared memory %s", strerror(errno));
>> + close(fd);
>> + return;
>> + }
>> + memory_region_init_ram_ptr(&s->ivshmem, OBJECT(s),
>> + "ivshmem.bar2", s->ivshmem_size, ptr);
>> + qemu_set_ram_fd(s->ivshmem.ram_addr, fd);
>> + vmstate_register_ram(&s->ivshmem, DEVICE(s));
>> + memory_region_add_subregion(&s->bar, 0, &s->ivshmem);
>> +}
>> +
>> +static void process_msg_disconnect(IVShmemState *s, uint16_t posn)
>> +{
>> + IVSHMEM_DPRINTF("posn %d has gone away\n", posn);
>> + close_peer_eventfds(s, posn);
>> +}
>> +
>> +static void process_msg_connect(IVShmemState *s, uint16_t posn, int fd)
>> +{
>> + Peer *peer = &s->peers[posn];
>> + int vector;
>> +
>> + /*
>> + * The N-th connect message for this peer comes with the file
>> + * descriptor for vector N-1. Count messages to find the vector.
>> + */
>> + if (peer->nb_eventfds >= s->vectors) {
>> + error_report("Too many eventfd received, device has %d vectors",
>> + s->vectors);
>> + close(fd);
>> + return;
>> + }
>> + vector = peer->nb_eventfds++;
>> +
>> + IVSHMEM_DPRINTF("eventfds[%d][%d] = %d\n", posn, vector, fd);
>> + event_notifier_init_fd(&peer->eventfds[vector], fd);
>> + fcntl_setfl(fd, O_NONBLOCK); /* msix/irqfd poll non block */
>> +
>> + if (posn == s->vm_id) {
>> + setup_interrupt(s, vector);
>> + }
>> +
>> + if (ivshmem_has_feature(s, IVSHMEM_IOEVENTFD)) {
>> + ivshmem_add_eventfd(s, posn, vector);
>> + }
>> +}
>> +
>> +static void process_msg(IVShmemState *s, int64_t msg, int fd)
>> +{
>> + IVSHMEM_DPRINTF("posn is %" PRId64 ", fd is %d\n", msg, fd);
>> +
>> + if (msg < -1 || msg > IVSHMEM_MAX_PEERS) {
>> + error_report("server sent invalid message %" PRId64, msg);
>> + close(fd);
>> + return;
>> + }
>> +
>> + if (msg == -1) {
>> + process_msg_shmem(s, fd);
>
> the previous code used to close fd if any, it's worth to keep that imho
I'm blind. Where?
>> + return;
>> + }
>> +
>> + if (msg >= s->nb_peers) {
>> + resize_peers(s, msg + 1);
>> + }
>> +
>> + if (fd >= 0) {
>> + process_msg_connect(s, msg, fd);
>> + } else if (s->vm_id == -1) {
>> + s->vm_id = msg;
>> + } else {
>> + process_msg_disconnect(s, msg);
>> + }
>> +}
>> +
>> static void ivshmem_read(void *opaque, const uint8_t *buf, int size)
>> {
>> IVShmemState *s = opaque;
>> int incoming_fd;
>> - int new_eventfd;
>> int64_t incoming_posn;
>> - Error *err = NULL;
>> - Peer *peer;
>>
>> if (!fifo_update_and_get_i64(s, buf, size, &incoming_posn)) {
>> return;
>> @@ -581,96 +673,7 @@ static void ivshmem_read(void *opaque, const uint8_t
>> *buf, int size)
>> IVSHMEM_DPRINTF("posn is %" PRId64 ", fd is %d\n",
>> incoming_posn, incoming_fd);
>>
>> - if (incoming_posn < -1 || incoming_posn > IVSHMEM_MAX_PEERS) {
>> - error_report("server sent invalid message %" PRId64,
>> - incoming_posn);
>> - if (incoming_fd != -1) {
>> - close(incoming_fd);
>> - }
>> - return;
>> - }
>> -
>> - if (incoming_posn >= s->nb_peers) {
>> - resize_peers(s, incoming_posn + 1);
>> - }
>> -
>> - peer = &s->peers[incoming_posn];
>> -
>> - if (incoming_fd == -1) {
>> - /* if posn is positive and unseen before then this is our posn*/
>> - if (incoming_posn >= 0 && s->vm_id == -1) {
>> - /* receive our posn */
>> - s->vm_id = incoming_posn;
>> - } else {
>> - /* otherwise an fd == -1 means an existing peer has gone away */
>> - IVSHMEM_DPRINTF("posn %" PRId64 " has gone away\n",
>> incoming_posn);
>> - close_peer_eventfds(s, incoming_posn);
>> - }
>> - return;
>> - }
>> -
>> - /* if the position is -1, then it's shared memory region fd */
>> - if (incoming_posn == -1) {
>> - void * map_ptr;
>> -
>> - if (memory_region_is_mapped(&s->ivshmem)) {
>> - error_report("shm already initialized");
>> - close(incoming_fd);
>> - return;
>> - }
>> -
>> - if (check_shm_size(s, incoming_fd, &err) == -1) {
>> - error_report_err(err);
>> - close(incoming_fd);
>> - return;
>> - }
>> -
>> - /* mmap the region and map into the BAR2 */
>> - map_ptr = mmap(0, s->ivshmem_size, PROT_READ|PROT_WRITE, MAP_SHARED,
>> - incoming_fd, 0);
>> - if (map_ptr == MAP_FAILED) {
>> - error_report("Failed to mmap shared memory %s",
>> strerror(errno));
>> - close(incoming_fd);
>> - return;
>> - }
>> - memory_region_init_ram_ptr(&s->ivshmem, OBJECT(s),
>> - "ivshmem.bar2", s->ivshmem_size,
>> map_ptr);
>> - qemu_set_ram_fd(s->ivshmem.ram_addr, incoming_fd);
>> - vmstate_register_ram(&s->ivshmem, DEVICE(s));
>> -
>> - IVSHMEM_DPRINTF("guest h/w addr = %p, size = %" PRIu64 "\n",
>> - map_ptr, s->ivshmem_size);
>> -
>> - memory_region_add_subregion(&s->bar, 0, &s->ivshmem);
>> -
>> - return;
>> - }
>> -
>> - /* each peer has an associated array of eventfds, and we keep
>> - * track of how many eventfds received so far */
>> - /* get a new eventfd: */
>> - if (peer->nb_eventfds >= s->vectors) {
>> - error_report("Too many eventfd received, device has %d vectors",
>> - s->vectors);
>> - close(incoming_fd);
>> - return;
>> - }
>> -
>> - new_eventfd = peer->nb_eventfds++;
>> -
>> - /* this is an eventfd for a particular peer VM */
>> - IVSHMEM_DPRINTF("eventfds[%" PRId64 "][%d] = %d\n", incoming_posn,
>> - new_eventfd, incoming_fd);
>> - event_notifier_init_fd(&peer->eventfds[new_eventfd], incoming_fd);
>> - fcntl_setfl(incoming_fd, O_NONBLOCK); /* msix/irqfd poll non block */
>> -
>> - if (incoming_posn == s->vm_id) {
>> - setup_interrupt(s, new_eventfd);
>> - }
>> -
>> - if (ivshmem_has_feature(s, IVSHMEM_IOEVENTFD)) {
>> - ivshmem_add_eventfd(s, incoming_posn, new_eventfd);
>> - }
>> + process_msg(s, incoming_posn, incoming_fd);
>
> while at it, you may want to rename incoming_posn to msg for consistency.
I didn't to minimize churn, but you're probably right. I'll try and see
how the diff comes out.
>> }
>>
>> static void ivshmem_check_version(void *opaque, const uint8_t * buf, int
>> size)
>> --
>> 2.4.3
>>
>>
>
> looks good otherwise
Thanks!
