linux-kernel - Re: [PATCH RFC 3/5] virtio-pci: extend virtio_pci_cap to hold page

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CABVzXAkFPr-GiALnk2v53CzX0b5Y0yG=KoRoOFR7fy98Xeyb3w@mail.gmail.com>
Date: Thu, 13 Feb 2025 11:31:47 -0800
From: Daniel Verkamp <dverkamp@...omium.org>
To: Sergio Lopez <slp@...hat.com>
Cc: "Michael S. Tsirkin" <mst@...hat.com>, Jason Wang <jasowang@...hat.com>, 
	Xuan Zhuo <xuanzhuo@...ux.alibaba.com>, Eugenio Pérez <eperezma@...hat.com>, 
	David Airlie <airlied@...hat.com>, Gerd Hoffmann <kraxel@...hat.com>, 
	Gurchetan Singh <gurchetansingh@...omium.org>, Chia-I Wu <olvaffe@...il.com>, 
	Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, Maxime Ripard <mripard@...nel.org>, 
	Thomas Zimmermann <tzimmermann@...e.de>, Simona Vetter <simona@...ll.ch>, Rob Clark <robdclark@...il.com>, 
	Dmitry Osipenko <dmitry.osipenko@...labora.com>, fnkl.kernel@...il.com, 
	virtualization@...ts.linux.dev, linux-kernel@...r.kernel.org, 
	dri-devel@...ts.freedesktop.org
Subject: Re: [PATCH RFC 3/5] virtio-pci: extend virtio_pci_cap to hold page_size

On Thu, Feb 13, 2025 at 11:22 AM Daniel Verkamp <dverkamp@...omium.org> wrote:
>
> On Thu, Feb 13, 2025 at 7:54 AM Sergio Lopez <slp@...hat.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@...hat.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