Daniel Verkamp <dverkamp@xxxxxxxxxxxx> writes:
> On Thu, Feb 13, 2025 at 7:54 AM Sergio Lopez <slp@xxxxxxxxxx> wrote:
>>
>> Turn the 16 bit padding into a page_size field to allow the device to
>> pass its required page size with format PAGE_SIZE >> 12.
>>
>> Signed-off-by: Sergio Lopez <slp@xxxxxxxxxx>
>> ---
>> drivers/virtio/virtio_pci_modern.c | 29 +++++++++++++++++++++++++----
>> include/uapi/linux/virtio_pci.h | 2 +-
>> 2 files changed, 26 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
>> index 79616ce5057bf3b2b88cae7e8fb7729efa9dd632..26e9cd5148c0f10209c34d12e65d64490a855d75 100644
>> --- a/drivers/virtio/virtio_pci_modern.c
>> +++ b/drivers/virtio/virtio_pci_modern.c
> [...]
>> + if (__virtio_test_bit(vdev, VIRTIO_F_SHM_PAGE_SIZE)) {
>> + pci_read_config_word(dev, pos + offsetof(struct virtio_pci_cap,
>> + page_size), &res_psize);
>> + if (!res_psize) {
>> + dev_err(&dev->dev, "%s: shm cap with invalid page size on "
>> + "a device with VIRTIO_F_SHM_PAGE_SIZE feature\n",
>> + __func__);
>
> Maybe this should also constrain the page size to be a power of 2?
>
> [...]
>> diff --git a/include/uapi/linux/virtio_pci.h b/include/uapi/linux/virtio_pci.h
>> index 8549d4571257142ac6c9dad5c01369923791a85a..fb0ccb7a125d8178c1f78333c4d2f43540e1764b 100644
>> --- a/include/uapi/linux/virtio_pci.h
>> +++ b/include/uapi/linux/virtio_pci.h
>> @@ -127,7 +127,7 @@ struct virtio_pci_cap {
>> __u8 cfg_type; /* Identifies the structure. */
>> __u8 bar; /* Where to find it. */
>> __u8 id; /* Multiple capabilities of the same type */
>> - __u8 padding[2]; /* Pad to full dword. */
>> + __u16 page_size; /* Device page size (PAGE_SIZE >> 12). */
>
> This comment should probably clarify that the page_size field is only
> valid when cfg_type is VIRTIO_PCI_CAP_SHARED_MEMORY_CFG, assuming
> that's the case. Or should the device be expected to provide the
> page_size for all capabilities regardless of type?
Only for VIRTIO_PCI_CAP_SHARED_MEMORY_CFG, I'll clarify that in the
comment.
> It seems like the name should also ideally make it clearer that this
> is page_size/4096 rather than the actual page size to avoid confusing
> device implementers.
I like your suggestion in the other email of turning this into
page_shift, which clarifies the field contents and also addresses the
need check the constrains you highlighted above. I'll do that in v2.
Thanks!
Sergio.
