| Message ID | 20250213-virtio-shm-page-size-v1-3-5ee1f9984350@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | virtio: obtain SHM page size from device | expand |
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. > __le32 offset; /* Offset within bar. */ > __le32 length; /* Length of the structure, in bytes. */ > };
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 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.
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.
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. */ };
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(-)