Thanks Farhan,
On 10/23/25 1:31 PM, Farhan Ali wrote:
[snip]
+
+int pci_write(uint32_t fhandle, uint64_t offset, uint64_t data,
uint8_t len)
+{
+
+ uint64_t req = ZPCI_CREATE_REQ(fhandle, 4, len);
This assumes that we will only read to BAR 4? I think we should pass
the PCIAS here as well if we want to generalize this function?
I was thinking about this too and I agree that it should be
generalized. Also when reading the capabilities from the configuration
space, I did not check that the location is, actually, BAR 4. I will
generalize the functions so that we do not make any assumptions about BAR 4.
+ uint8_t status;
+ int rc;
+
+ rc = pcistg(data, req, offset, &status);
+ if (rc == 1) {
+ return status;
+ } else if (rc) {
+ return rc;
+ }
+
+ return 0;
+}
+
+int pci_read(uint32_t fhandle, uint64_t offset, uint8_t picas, void
*buf, uint8_t len)
+{
+ uint64_t req;
+ uint64_t data;
+ uint8_t status;
+ int readlen;
+ int i = 0;
+ int rc = 0;
+
+ while (len > 0 && !rc) {
+ data = 0;
+ readlen = len > 8 ? 8 : len;
+ req = ZPCI_CREATE_REQ(fhandle, picas, readlen);
+ rc = pcilg(&data, req, offset + (i * 8), &status);
Shouldn't this be offset + (i * readlen)? but I guess this works
because we will only increment i on reads greater than 8 bytes. Maybe
we could try to simplify this and have a single pci_read function and
several other helper functions that uses pci_read to read sizes of
1/2/4/8 bytes. For reads greater than 8 bytes we can have another
function (similar to zpci_memcpy_from_io, in the kernel). From what I
can tell most of the pci_read calls reads are 8 bytes in the rest of
the patches, except maybe for one case which reads greater than 8?
Yes I agree. Thomas mentioned something similar. I think using some
helper functions will make the code easier to read also.
+ ((uint64_t *)buf)[i] = data;
+ len -= readlen;
+ i++;
+ }
+
+ if (rc == 1) {
+ return status;
+ } else if (rc) {
+ return rc;
+ }
+
+ return 0;
+}
+
+/*
+ * Find the position of the capability config within PCI configuration
+ * space for a given cfg type. Return the position if found,
otherwise 0.
+ */
+uint8_t find_cap_pos(uint32_t fhandle, uint64_t cfg_type) {
+ uint64_t req, next, cfg;
+ uint8_t status;
+ int rc;
+
+ req = ZPCI_CREATE_REQ(fhandle, 0xf, 1);
+ rc = pcilg(&next, req, PCI_CAPABILITY_LIST, &status);
+ rc = pcilg(&cfg, req, next + 3, &status);
Why are we reading next + 3 into cfg? If I understand this correctly
next will be the address of the first capability in the linked list,
and so we should just read the first byte from next to get the
capability id? I think we should have helper function like
qpci_find_capability to find the capabilities?
+
+ while (!rc && (cfg != cfg_type) && next) {
+ rc = pcilg(&next, req, next + 1, &status);
+ rc = pcilg(&cfg, req, next + 3, &status);
Same question here?
I see you already answered this yourself in a follow-up mail, but you
are correct that this is for the vendor-specific capabilities. I will
move it to virtio-pci.c and add a comment for clarity.
+ }
+
+ return rc ? 0 : next;
+}
+
[..snip..]
Regards,
Jared Rossi
