Re: [PATCH 4/7] pc-bios/s390-ccw: Introduce PCI device IPL format

[Jared Rossi <jrossi@linux.ibm.com>]

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