Daniel Verkamp <dverk...@chromium.org> writes:
> On Thu, Feb 13, 2025 at 11:22 AM Daniel Verkamp <dverk...@chromium.org> wrote:
>>
>> On Thu, Feb 13, 2025 at 7:54 AM Sergio Lopez <s...@redhat.com> 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 <s...@redhat.com>
>> > ---
>> > 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?
>>
>> 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.
>
> Alternatively, this could be represented as a single u8 page_shift,
> where page_size = 1 << (page_shift + 12), and then existing devices
> would "just work" (assuming they filled the padding with 0). It is
> probably still a good idea to guard it with a feature bit, though, so
> it doesn't really make that much difference.
I actually considered not using a feature bit and just behave like this,
but then I've noticed that, for the mmio transport, it would mean
reading from a new register. Some VMMs may overreact to the unexpected
read and crash or, even if they don't, it's not guaranteed they aren't
going to return garbage in response. So I think it's better being a bit
conservative and protect the behavior behind a feature bit.
Thanks,
Sergio.
