Skip to content

pci: implement RW1C handling for PCI status register#865

Open
zp78256pxd-ux wants to merge 5 commits into
nutanix:masterfrom
zp78256pxd-ux:fix-pci-status-rw1c
Open

pci: implement RW1C handling for PCI status register#865
zp78256pxd-ux wants to merge 5 commits into
nutanix:masterfrom
zp78256pxd-ux:fix-pci-status-rw1c

Conversation

@zp78256pxd-ux

Copy link
Copy Markdown

pci: implement RW1C handling for PCI status register

First of all, really impressed by libvfio-user — the codebase is clean and well-structured, and the Python test framework is a great approach for this kind of low-level emulation testing. Happy to be contributing.

Summary

Implement RW1C (Read/Write 1 to Clear) handling for PCI Status register writes.

Previously, writes to PCI_STATUS were silently ignored, even though several status bits are defined as RW1C by the PCI 3.0 spec. A FIXME in the code acknowledged this:

/* FIXME ignoring write completely is wrong as some bits are RW1C */
vfu_log(vfu_ctx, LOG_INFO, "write to status ignored");

Changes

  • Add handle_status_write() implementing correct RW1C semantics for all six RW1C bits
    (dpd, sta, rta, rma, sse,dpe)
  • Replace the ignored write path in pci_hdr_write() with a call to handle_status_write()
  • Add regression test coverage in test/py/test_pci_status.py
  • Wire new test into test/py/meson.build

Validation

Executed:

meson test -C build --print-errorlogs
pytest test/py/test_pci_status.py -v

All unit and Python tests passed. The test-lspci failure appears unrelated to this change (environment/output formatting difference in the test environment).

References

  • PCI Local Bus Specification Revision 3.0, 6.2.3 (Device Status Register).

Comment thread test/py/test_pci_status.py Outdated
Comment thread lib/pci.c Outdated
vfu_pci_config_space_t *pci,
const char *buf)
{
vfu_pci_hdr_sts_t w;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit, rename to device_status or something more descriptive?

Comment thread test/py/test_pci_status.py Outdated
Comment thread test/py/test_pci_status.py

@tmakatos tmakatos left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

BTW do you mind sharing what specific problem you've encountered that motivated you to fix this?

Comment thread test/py/test_pci_status.py Outdated
@@ -1,17 +1,37 @@
#
# Copyright (c) 2026 Nutanix Inc.
# Copyright (c) 2021 Nutanix Inc. All rights reserved.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I should have been more explicit, you don't have to add something, but if you it has to be correct. If in doubt, don't add anything.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thanks for clarifying — I copied the copyright header from an existing test file when creating this test. Could you clarify what would be the correct header in this case? Should I remove it entirely or follow a different convention for new tests?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I was wrong about this, we do need the Copyright (year, company, author name) as well as the license.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

You still have Nutanix as the company and John as the author.

Comment thread test/py/test_pci_status.py Outdated
@tmakatos

Copy link
Copy Markdown
Collaborator

BTW do you mind sharing what specific problem you've encountered that motivated you to fix this?

Just to clear we don't hear that much of how libvfio-user is used, feel free to decline if you don't want to discuss this further.

@zp78256pxd-ux

Copy link
Copy Markdown
Author

I’m currently exploring ideas around reducing cloud infrastructure cost and building an eBPF-based agent, and while reading through virtualization-related components I came across libvfio-user.

This wasn’t triggered by a production issue — I noticed the PCI status handling and thought it would be a good area to contribute and learn more about the stack.

@tmakatos tmakatos left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM apart from the copyright header.

@zp78256pxd-ux

Copy link
Copy Markdown
Author

Okay brother will do.One idea I’ve been exploring is whether eBPF could complement traditional observability by collecting more direct kernel-level signals instead of relying only on exported metrics.

I’m curious whether you’ve seen similar approaches used for things like OOM root-cause analysis, VM efficiency, or infrastructure cost reduction.ur thought brother !!

@tmakatos tmakatos left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We also need SOB in the commit message.

Comment thread test/py/test_pci_status.py Outdated
@@ -1,17 +1,37 @@
#
# Copyright (c) 2026 Nutanix Inc.
# Copyright (c) 2021 Nutanix Inc. All rights reserved.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I was wrong about this, we do need the Copyright (year, company, author name) as well as the license.

@jlevon

jlevon commented Jun 17, 2026

Copy link
Copy Markdown
Collaborator

thanks for contribution, as per above, we need SOB (you can make it a single commit), plus you have some python lint :)

Implement Read/Write-1-to-Clear (RW1C) semantics for PCI status register writes.

Handle clearing of supported RW1C bits and extend tests to verify:
- writing 1 clears the target bit
- writing 0 preserves status bits
- read-only bits remain unchanged

Signed-off-by: zp78256pxd-ux <zp78256pxd@privaterelay.appleid.com>
@zp78256pxd-ux

Copy link
Copy Markdown
Author

Thanks for the review and feedback.

I updated the commit with Signed-off-by, cleaned up the commit message, and force-pushed the changes. I also reran the PCI status test locally and verified the RW1C coverage still passes.
Please take another look when convenient.

@tmakatos

Copy link
Copy Markdown
Collaborator

Thanks for the review and feedback.

I updated the commit with Signed-off-by, cleaned up the commit message, and force-pushed the changes. I also reran the PCI status test locally and verified the RW1C coverage still passes. Please take another look when convenient.

As @jlevon said there's a lint error and you still have John and Nutanix on the header. Also, don't force push or squash commits, we'll do that when merging, makes it easier to ensure that the content of the commit stays the same.

billzypop and others added 3 commits June 25, 2026 02:43
Signed-off-by: Siddharth C <siddharthcibi@icloud.com>
Signed-off-by: Siddharth C <siddharthcibi@icloud.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants