Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Create wr-pci-leds example #11

Merged
merged 2 commits into from
Oct 24, 2024
Merged

Create wr-pci-leds example #11

merged 2 commits into from
Oct 24, 2024

Conversation

huitmj
Copy link
Collaborator

@huitmj huitmj commented Oct 23, 2024

add WR PCI Leds device to system build steps
add barebone example PCI LEDs device
added documentation for wr-pci-leds specification

@huitmj @ho28
Create wr-pci-leds example
92a41a5 
add WR PCI Leds device to system build steps
add barebone example PCI LEDs device
added documentation for wr-pci-leds specification
ho28
ho28 reviewed Oct 24, 2024
View reviewed changes
Copy link
Collaborator

@ho28 ho28 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the contribution James! I left several comments, but if you prefer, we can merge this change as-is, and I can address my comments in a follow-up change.

hw/misc/wr-pci-leds.c
#include "hw/pci/pci_device.h"

#define TYPE_MY_DEVICE "wr-pci-leds"
OBJECT_DECLARE_SIMPLE_TYPE(MyDeviceState, MY_DEVICE)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably ok since this is just an example implementation, but since the file name and TYPE_##MODULE_OBJ_NAME are both of wr-pci-leds nomenclature, i think the device state structure should also be named accordingly.

hw/misc/wr-pci-leds.c Outdated
#define TYPE_MY_DEVICE "wr-pci-leds"
OBJECT_DECLARE_SIMPLE_TYPE(MyDeviceState, MY_DEVICE)

typedef struct MyDeviceState
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same comment as above, and throughout the file, the device state structure should probably be named consistently with the device type

hw/misc/wr-pci-leds.c Outdated
}
type_init(my_device_register_types)

/* Notes:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No longer needed notes?

hw/misc/wr-pci-leds.c
};
static void pci_my_device_realize(PCIDevice *dev, Error **errp)
{
printf(">>\t<QEMU> Hello this is my-device PCI device\n");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to prevent these from being interleaved with the driver test output, perhaps these can be qemu log messages that we can redirect to file using "-D " option on the qemu command line, rather than requiring the tab indent

hw/misc/wr-pci-leds.c
{
printf(">>\t<QEMU> Hello this is my-device PCI device\n");
MyDeviceState *s = MY_DEVICE(dev);
memory_region_init_io(&s->bar, OBJECT(s), &my_device_ops, s, "my-device", 32);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/my-device/wr-pci-leds

hw/misc/wr-pci-leds.c
printf(">>\t<QEMU> Write to %lu with %lu %u\n",addr, data, size);
MyDeviceState* s = opaque;
switch (addr) {
case 0: s->led0=data; break;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style nit - case body should have new line per statement and indented

hw/misc/wr-pci-leds.c Outdated
PCIDevice parent_obj;
MemoryRegion bar;

int32_t led0,led1;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should these members be uint64_t to match the assignments in my_device_write()?

@huitmj
Update wr-pci-leds.c
0168ac6 
align led private data type with write data type and remove notes that no longer needed
@ho28 ho28 merged commit ea349f8 into Wind-River:wr-integration Oct 24, 2024
1 check passed
@huitmj huitmj deleted the wr-pci-leds-1 branch October 24, 2024 17:27
ho28 pushed a commit to ho28/wr-qemu that referenced this pull request Nov 15, 2024
@foxmox
block/reqlist: allow adding overlapping requests
7eefbf8 
Allow overlapping request by removing the assert that made it
impossible. There are only two callers:

1. block_copy_task_create()

It already asserts the very same condition before calling
reqlist_init_req().

2. cbw_snapshot_read_lock()

There is no need to have read requests be non-overlapping in
copy-before-write when used for snapshot-access. In fact, there was no
protection against two callers of cbw_snapshot_read_lock() calling
reqlist_init_req() with overlapping ranges and this could lead to an
assertion failure [1].

In particular, with the reproducer script below [0], two
cbw_co_snapshot_block_status() callers could race, with the second
calling reqlist_init_req() before the first one finishes and removes
its conflicting request.

[0]:

> #!/bin/bash -e
> dd if=/dev/urandom of=/tmp/disk.raw bs=1M count=1024
> ./qemu-img create /tmp/fleecing.raw -f raw 1G
> (
> ./qemu-system-x86_64 --qmp stdio \
> --blockdev raw,node-name=node0,file.driver=file,file.filename=/tmp/disk.raw \
> --blockdev raw,node-name=node1,file.driver=file,file.filename=/tmp/fleecing.raw \
> <<EOF
> {"execute": "qmp_capabilities"}
> {"execute": "blockdev-add", "arguments": { "driver": "copy-before-write", "file": "node0", "target": "node1", "node-name": "node3" } }
> {"execute": "blockdev-add", "arguments": { "driver": "snapshot-access", "file": "node3", "node-name": "snap0" } }
> {"execute": "nbd-server-start", "arguments": {"addr": { "type": "unix", "data": { "path": "/tmp/nbd.socket" } } } }
> {"execute": "block-export-add", "arguments": {"id": "exp0", "node-name": "snap0", "type": "nbd", "name": "exp0"}}
> EOF
> ) &
> sleep 5
> while true; do
> ./qemu-nbd -d /dev/nbd0
> ./qemu-nbd -c /dev/nbd0 nbd:unix:/tmp/nbd.socket:exportname=exp0 -f raw -r
> nbdinfo --map 'nbd+unix:///exp0?socket=/tmp/nbd.socket'
> done

[1]:

> Wind-River#5  0x000071e5f0088eb2 in __GI___assert_fail (...) at ./assert/assert.c:101
> Wind-River#6  0x0000615285438017 in reqlist_init_req (...) at ../block/reqlist.c:23
> Wind-River#7  0x00006152853e2d98 in cbw_snapshot_read_lock (...) at ../block/copy-before-write.c:237
> Wind-River#8  0x00006152853e3068 in cbw_co_snapshot_block_status (...) at ../block/copy-before-write.c:304
> Wind-River#9  0x00006152853f4d22 in bdrv_co_snapshot_block_status (...) at ../block/io.c:3726
> Wind-River#10 0x000061528543a63e in snapshot_access_co_block_status (...) at ../block/snapshot-access.c:48
> Wind-River#11 0x00006152853f1a0a in bdrv_co_do_block_status (...) at ../block/io.c:2474
> Wind-River#12 0x00006152853f2016 in bdrv_co_common_block_status_above (...) at ../block/io.c:2652
> Wind-River#13 0x00006152853f22cf in bdrv_co_block_status_above (...) at ../block/io.c:2732
> Wind-River#14 0x00006152853d9a86 in blk_co_block_status_above (...) at ../block/block-backend.c:1473
> Wind-River#15 0x000061528538da6c in blockstatus_to_extents (...) at ../nbd/server.c:2374
> Wind-River#16 0x000061528538deb1 in nbd_co_send_block_status (...) at ../nbd/server.c:2481
> Wind-River#17 0x000061528538f424 in nbd_handle_request (...) at ../nbd/server.c:2978
> Wind-River#18 0x000061528538f906 in nbd_trip (...) at ../nbd/server.c:3121
> Wind-River#19 0x00006152855a7caf in coroutine_trampoline (...) at ../util/coroutine-ucontext.c:175

Cc: [email protected]
Suggested-by: Vladimir Sementsov-Ogievskiy <[email protected]>
Signed-off-by: Fiona Ebner <[email protected]>
Message-Id: <[email protected]>
Reviewed-by: Vladimir Sementsov-Ogievskiy <[email protected]>
Signed-off-by: Vladimir Sementsov-Ogievskiy <[email protected]>
(cherry picked from commit 6475155d519209c80fdda53e05130365aa769838)
Signed-off-by: Michael Tokarev <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ho28 ho28 ho28 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@huitmj @ho28