On Mon, Oct 09, 2023 at 09:15:31AM +0800, Xuan Zhuo wrote:
> On Sun, 8 Oct 2023 06:42:37 -0400, "Michael S. Tsirkin" <mst@xxxxxxxxxx> wrote:
> > On Sun, Oct 08, 2023 at 05:38:41PM +0800, Xuan Zhuo wrote:
> > > Now, the function vp_modern_map_capability() takes the size parameter,
> > > which corresponds to the size of virtio_pci_common_cfg. As a result,
> > > this indicates the size of memory area to map.
> > >
> > > However, if the _F_RING_RESET is negotiated, the extra items will be
> > > used. Therefore, we need to use the size of virtio_pci_modern_common_cfg
> > > to map more space.
> > >
> > > Meanwhile, this patch removes the feature(_F_RING_ERSET and
> >
> > typo
> >
> > > _F_NOTIFICATION_DATA) when the common cfg size does not match
> > > the corresponding feature.
> > >
> > > Fixes: 0b50cece0b78 ("virtio_pci: introduce helper to get/set queue reset")
> > > Signed-off-by: Xuan Zhuo <xuanzhuo@xxxxxxxxxxxxxxxxx>
> > > ---
> > > drivers/virtio/virtio_pci_modern.c | 20 +++++++++++++++++++-
> > > drivers/virtio/virtio_pci_modern_dev.c | 4 ++--
> > > include/linux/virtio_pci_modern.h | 1 +
> > > 3 files changed, 22 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
> > > index d6bb68ba84e5..c0b9d2363ddb 100644
> > > --- a/drivers/virtio/virtio_pci_modern.c
> > > +++ b/drivers/virtio/virtio_pci_modern.c
> > > @@ -22,8 +22,26 @@
> > > static u64 vp_get_features(struct virtio_device *vdev)
> > > {
> > > struct virtio_pci_device *vp_dev = to_vp_device(vdev);
> > > + u64 features = vp_modern_get_features(&vp_dev->mdev);
> > > +
> > > +#define check_feature(feature, field) \
> > > + do { \
> > > + if (features & BIT_ULL(feature)) { \
> > > + u32 offset = offsetofend(struct virtio_pci_modern_common_cfg, field); \
> > > + if (unlikely(vp_dev->mdev.common_len < offset)) \
> > > + features &= ~BIT_ULL(feature); \
> > > + } \
> > > + } while (0)
> > > +
> > > + /* For buggy devices, if the common len does not match the feature, we
> > > + * remove the feature.
> >
> > I don't like doing this in vp_get_features. userspace won't be able
> > to see the actual device features at all.
> > Also, we should print an info message at least.
> >
> > I am still debating with myself whether clearing feature bits
> > or just failing finalize_features (and thus, probe) is best.
>
> For me, I think failing probe is best.
>
> Then the developer of the device can find that firstly.
> And I think we should print an info message when we detect
> this error.
If you fail probe - maybe even a warning.
> If we clear the feature bits, the developer of the device may
> ignore this error.
>
> >
> >
> > > + */
> > > + check_feature(VIRTIO_F_NOTIFICATION_DATA, queue_notify_data);
> > > + check_feature(VIRTIO_F_RING_RESET, queue_reset);
> > > +
> > > +#undef check_feature
> >
> > this macro's too scary. just pass offset and feature bit as
> > parameters to an inline function.
>
> I should pass the features as a parameter.
>
> Thanks.
>
>
>
> >
> > >
> > > - return vp_modern_get_features(&vp_dev->mdev);
> > > + return features;
> > > }
> > >
> > > static void vp_transport_features(struct virtio_device *vdev, u64 features)
> > > diff --git a/drivers/virtio/virtio_pci_modern_dev.c b/drivers/virtio/virtio_pci_modern_dev.c
> > > index aad7d9296e77..33f319da1558 100644
> > > --- a/drivers/virtio/virtio_pci_modern_dev.c
> > > +++ b/drivers/virtio/virtio_pci_modern_dev.c
> > > @@ -291,8 +291,8 @@ int vp_modern_probe(struct virtio_pci_modern_device *mdev)
> > > err = -EINVAL;
> > > mdev->common = vp_modern_map_capability(mdev, common,
> > > sizeof(struct virtio_pci_common_cfg), 4,
> > > - 0, sizeof(struct virtio_pci_common_cfg),
> > > - NULL, NULL);
> > > + 0, sizeof(struct virtio_pci_modern_common_cfg),
> > > + &mdev->common_len, NULL);
> > > if (!mdev->common)
> > > goto err_map_common;
> > > mdev->isr = vp_modern_map_capability(mdev, isr, sizeof(u8), 1,
> > > diff --git a/include/linux/virtio_pci_modern.h b/include/linux/virtio_pci_modern.h
> > > index 067ac1d789bc..edf62bae0474 100644
> > > --- a/include/linux/virtio_pci_modern.h
> > > +++ b/include/linux/virtio_pci_modern.h
> > > @@ -28,6 +28,7 @@ struct virtio_pci_modern_device {
> > > /* So we can sanity-check accesses. */
> > > size_t notify_len;
> > > size_t device_len;
> > > + size_t common_len;
> > >
> > > /* Capability for when we need to map notifications per-vq. */
> > > int notify_map_cap;
> > > --
> > > 2.32.0.3.g01195cf9f
> >
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
