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@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
@@ -770,14 +770,16 @@ static void del_vq(struct virtio_pci_vq_info *info)
vring_del_virtqueue(vq);
}
-static int virtio_pci_find_shm_cap(struct pci_dev *dev, u8 required_id,
- u8 *bar, u64 *offset, u64 *len)
+static int virtio_pci_find_shm_cap(struct virtio_device *vdev, struct pci_dev *dev,
+ u8 required_id, u8 *bar, u64 *offset, u64 *len,
+ u16 *page_size)
{
int pos;
for (pos = pci_find_capability(dev, PCI_CAP_ID_VNDR); pos > 0;
pos = pci_find_next_capability(dev, pos, PCI_CAP_ID_VNDR)) {
u8 type, cap_len, id, res_bar;
+ u16 res_psize = 0;
u32 tmp32;
u64 res_offset, res_length;
@@ -808,6 +810,23 @@ static int virtio_pci_find_shm_cap(struct pci_dev *dev, u8 required_id,
* Looks good.
*/
+ 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__);
+ res_psize = 4096 >> 12;
+ }
+ }
+
+ /* For backwards compatibility, if the device didn't specify a
+ * page size, assume it to be 4096.
+ */
+ if (!res_psize)
+ res_psize = 4096 >> 12;
+
/* Read the lower 32bit of length and offset */
pci_read_config_dword(dev, pos + offsetof(struct virtio_pci_cap,
offset), &tmp32);
@@ -829,6 +848,7 @@ static int virtio_pci_find_shm_cap(struct pci_dev *dev, u8 required_id,
*bar = res_bar;
*offset = res_offset;
*len = res_length;
+ *page_size = res_psize;
return pos;
}
@@ -841,11 +861,12 @@ static bool vp_get_shm_region(struct virtio_device *vdev,
struct virtio_pci_device *vp_dev = to_vp_device(vdev);
struct pci_dev *pci_dev = vp_dev->pci_dev;
u8 bar;
+ u16 page_size;
u64 offset, len;
phys_addr_t phys_addr;
size_t bar_len;
- if (!virtio_pci_find_shm_cap(pci_dev, id, &bar, &offset, &len))
+ if (!virtio_pci_find_shm_cap(vdev, pci_dev, id, &bar, &offset, &len, &page_size))
return false;
phys_addr = pci_resource_start(pci_dev, bar);
@@ -865,7 +886,7 @@ static bool vp_get_shm_region(struct virtio_device *vdev,
region->len = len;
region->addr = (u64) phys_addr + offset;
- region->page_size = 4096 >> 12;
+ region->page_size = page_size;
return true;
}
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). */
__le32 offset; /* Offset within bar. */
__le32 length; /* Length of the structure, in bytes. */
};
--
2.48.1On Thu, Feb 13, 2025 at 7:54 AM Sergio Lopez <slp@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 <slp@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.
> __le32 offset; /* Offset within bar. */
> __le32 length; /* Length of the structure, in bytes. */
> };
Daniel Verkamp <dverkamp@chromium.org> writes:
> On Thu, Feb 13, 2025 at 7:54 AM Sergio Lopez <slp@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 <slp@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?
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.
On Thu, Feb 13, 2025 at 11:22 AM Daniel Verkamp <dverkamp@chromium.org> wrote:
>
> On Thu, Feb 13, 2025 at 7:54 AM Sergio Lopez <slp@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 <slp@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.
Thanks,
-- Daniel
Daniel Verkamp <dverkamp@chromium.org> writes:
> On Thu, Feb 13, 2025 at 11:22 AM Daniel Verkamp <dverkamp@chromium.org> wrote:
>>
>> On Thu, Feb 13, 2025 at 7:54 AM Sergio Lopez <slp@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 <slp@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.
© 2016 - 2025 Red Hat, Inc.