On Wed, Mar 08, 2023 at 10:59:57AM -0500, Feng Liu wrote:
>
>
> On 2023-03-08 a.m.9:13, Michael S. Tsirkin wrote:
> > External email: Use caution opening links or attachments
> >
> >
> > On Tue, Mar 07, 2023 at 09:17:55PM +0000, Feng Liu wrote:
> > > On 2023-03-07 04:14, David Edmondson wrote:
> > > > External email: Use caution opening links or attachments
> > > >
> > > >
> > > > Feng Liu via Virtualization <virtualization@lists.linux-foundation.org>
> > > > writes:
> > > >
> > > > > Add const to make the read-only pointer parameters clear, similar to
> > > > > many existing functions.
> > > >
> > > > In many of the modified functions the local variable that is a cast of
> > > > the argument could also be const. Is there a reason not to do both at
> > > > the same time?
> > > >
> > >
> > > Hi,David
> > >
> > > In order to prevent the content of a pointer parameter from being
> > > modified and increase the readability of the function, it is recommended
> > > to add the 'const' keyword to the parameter. This is not necessary for
> > > local variables and non-pointer parameters, as they are only stored on
> > > the stack and do not affect the original value or structure member
> > > passed into the function. Therefore, in this case, the 'const' keyword
> > > is only added to pointer parameters.
> >
> > This makes no sense to me. If ytou cast away the const then it is
> > pointless.
> >
>
> Hi, Michael
>
> I really don't quite understand your point of view.
> Is a local variable that needs to be add const? Can you help to point
> out the specific problem/point ?
I just repeated what David said. Basically most of these functions use
to_vvq which uses container_of which in turn loses const qualifier.
So your change is poinless since rest of code accesses vq through
to_vvq.
What to do? I don't like the idea of to_vvq_const.
So I propose a version of container_of using _Generic
which preserves the const qualifier.
#define container_of(ptr, type, member) \
({ \
const void *__mptr = (ptr); \
static_assert(__same_type(*(ptr), ((type *)0)->member) || \
__same_type(*(ptr), void), \
"pointer type mismatch in container_of()"); \
_Generic((ptr), \
typeof(&((const type *)0)->member): \
(const type *)(__mptr - offsetof(type, member)), \
default: \
(type *)(__mptr - offsetof(type, member))); \
})
I'll hack it up in a day or two and post.
> > >
> > > > > Signed-off-by: Feng Liu <fe...@nvidia.com>
> > > > > Reviewed-by: Jiri Pirko <j...@nvidia.com>
> > > > > Reviewed-by: Parav Pandit <pa...@nvidia.com>
> > > > > Reviewed-by: Gavin Li <gav...@nvidia.com>
> > > > > Reviewed-by: Bodong Wang <bod...@nvidia.com>
> > > > > ---
> > > > > drivers/virtio/virtio_ring.c | 25 ++++++++++++-------------
> > > > > include/linux/virtio.h | 12 ++++++------
> > > > > 2 files changed, 18 insertions(+), 19 deletions(-)
> > > > >
> > > > > diff --git a/drivers/virtio/virtio_ring.c
> > > > > b/drivers/virtio/virtio_ring.c
> > > > > index 806cc44eae64..8010fd1d2047 100644
> > > > > --- a/drivers/virtio/virtio_ring.c
> > > > > +++ b/drivers/virtio/virtio_ring.c
> > > > > @@ -233,7 +233,7 @@ static void vring_free(struct virtqueue *_vq);
> > > > >
> > > > > #define to_vvq(_vq) container_of(_vq, struct vring_virtqueue, vq)
> > > > >
> > > > > -static bool virtqueue_use_indirect(struct vring_virtqueue *vq,
> > > > > +static bool virtqueue_use_indirect(const struct vring_virtqueue *vq,
> > > > > unsigned int total_sg)
> > > > > {
> > > > > /*
> > > > > @@ -269,7 +269,7 @@ static bool virtqueue_use_indirect(struct
> > > > > vring_virtqueue *vq,
> > > > > * unconditionally on data path.
> > > > > */
> > > > >
> > > > > -static bool vring_use_dma_api(struct virtio_device *vdev)
> > > > > +static bool vring_use_dma_api(const struct virtio_device *vdev)
> > > > > {
> > > > > if (!virtio_has_dma_quirk(vdev))
> > > > > return true;
> > > > > @@ -289,7 +289,7 @@ static bool vring_use_dma_api(struct
> > > > > virtio_device *vdev)
> > > > > return false;
> > > > > }
> > > > >
> > > > > -size_t virtio_max_dma_size(struct virtio_device *vdev)
> > > > > +size_t virtio_max_dma_size(const struct virtio_device *vdev)
> > > > > {
> > > > > size_t max_segment_size = SIZE_MAX;
> > > > >
> > > > > @@ -423,7 +423,7 @@ static void virtqueue_init(struct vring_virtqueue
> > > > > *vq, u32 num)
> > > > > */
> > > > >
> > > > > static void vring_unmap_one_split_indirect(const struct
> > > > > vring_virtqueue *vq,
> > > > > - struct vring_desc *desc)
> > > > > + const struct vring_desc
> > > > > *desc)
> > > > > {
> > > > > u16 flags;
> > > > >
> > > > > @@ -1183,7 +1183,7 @@ static u16 packed_last_used(u16 last_used_idx)
> > > > > }
> > > > >
> > > > > static void vring_unmap_extra_packed(const struct vring_virtqueue
> > > > > *vq,
> > > > > - struct vring_desc_extra *extra)
> > > > > + const struct vring_desc_extra
> > > > > *extra)
> > > > > {
> > > > > u16 flags;
> > > > >
> > > > > @@ -1206,7 +1206,7 @@ static void vring_unmap_extra_packed(const
> > > > > struct vring_virtqueue *vq,
> > > > > }
> > > > >
> > > > > static void vring_unmap_desc_packed(const struct vring_virtqueue
> > > > > *vq,
> > > > > - struct vring_packed_desc *desc)
> > > > > + const struct vring_packed_desc
> > > > > *desc)
> > > > > {
> > > > > u16 flags;
> > > > >
> > > > > @@ -2786,7 +2786,7 @@ EXPORT_SYMBOL_GPL(vring_transport_features);
> > > > > * Returns the size of the vring. This is mainly used for
> > > > > boasting to
> > > > > * userspace. Unlike other operations, this need not be
> > > > > serialized.
> > > > > */
> > > > > -unsigned int virtqueue_get_vring_size(struct virtqueue *_vq)
> > > > > +unsigned int virtqueue_get_vring_size(const struct virtqueue *_vq)
> > > > > {
> > > > >
> > > > > struct vring_virtqueue *vq = to_vvq(_vq);
> > > > > @@ -2819,7 +2819,7 @@ void __virtqueue_unbreak(struct virtqueue *_vq)
> > > > > }
> > > > > EXPORT_SYMBOL_GPL(__virtqueue_unbreak);
> > > > >
> > > > > -bool virtqueue_is_broken(struct virtqueue *_vq)
> > > > > +bool virtqueue_is_broken(const struct virtqueue *_vq)
> > > > > {
> > > > > struct vring_virtqueue *vq = to_vvq(_vq);
> > > > >
> > > > > @@ -2827,8 +2827,7 @@ bool virtqueue_is_broken(struct virtqueue *_vq)
> > > > > }
> > > > > EXPORT_SYMBOL_GPL(virtqueue_is_broken);
> > > > >
> > > > > -/*
> > > > > - * This should prevent the device from being used, allowing drivers
> > > > > to
> > > > > +/ This should prevent the device from being used, allowing drivers to
> > > > > * recover. You may need to grab appropriate locks to flush.
> > > > > */
> > > > > void virtio_break_device(struct virtio_device *dev)
> > > > > @@ -2881,7 +2880,7 @@ dma_addr_t virtqueue_get_desc_addr(struct
> > > > > virtqueue *_vq)
> > > > > }
> > > > > EXPORT_SYMBOL_GPL(virtqueue_get_desc_addr);
> > > > >
> > > > > -dma_addr_t virtqueue_get_avail_addr(struct virtqueue *_vq)
> > > > > +dma_addr_t virtqueue_get_avail_addr(const struct virtqueue *_vq)
> > > > > {
> > > > > struct vring_virtqueue *vq = to_vvq(_vq);
> > > > >
> > > > > @@ -2895,7 +2894,7 @@ dma_addr_t virtqueue_get_avail_addr(struct
> > > > > virtqueue *_vq)
> > > > > }
> > > > > EXPORT_SYMBOL_GPL(virtqueue_get_avail_addr);
> > > > >
> > > > > -dma_addr_t virtqueue_get_used_addr(struct virtqueue *_vq)
> > > > > +dma_addr_t virtqueue_get_used_addr(const struct virtqueue *_vq)
> > > > > {
> > > > > struct vring_virtqueue *vq = to_vvq(_vq);
> > > > >
> > > > > @@ -2910,7 +2909,7 @@ dma_addr_t virtqueue_get_used_addr(struct
> > > > > virtqueue *_vq)
> > > > > EXPORT_SYMBOL_GPL(virtqueue_get_used_addr);
> > > > >
> > > > > /* Only available for split ring */
> > > > > -const struct vring *virtqueue_get_vring(struct virtqueue *vq)
> > > > > +const struct vring *virtqueue_get_vring(const struct virtqueue *vq)
> > > > > {
> > > > > return &to_vvq(vq)->split.vring;
> > > > > }
> > > > > diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> > > > > index 2b472514c49b..36a79374e735 100644
> > > > > --- a/include/linux/virtio.h
> > > > > +++ b/include/linux/virtio.h
> > > > > @@ -84,14 +84,14 @@ bool virtqueue_enable_cb_delayed(struct virtqueue
> > > > > *vq);
> > > > >
> > > > > void *virtqueue_detach_unused_buf(struct virtqueue *vq);
> > > > >
> > > > > -unsigned int virtqueue_get_vring_size(struct virtqueue *vq);
> > > > > +unsigned int virtqueue_get_vring_size(const struct virtqueue *vq);
> > > > >
> > > > > -bool virtqueue_is_broken(struct virtqueue *vq);
> > > > > +bool virtqueue_is_broken(const struct virtqueue *vq);
> > > > >
> > > > > -const struct vring *virtqueue_get_vring(struct virtqueue *vq);
> > > > > -dma_addr_t virtqueue_get_desc_addr(struct virtqueue *vq);
> > > > > -dma_addr_t virtqueue_get_avail_addr(struct virtqueue *vq);
> > > > > -dma_addr_t virtqueue_get_used_addr(struct virtqueue *vq);
> > > > > +const struct vring *virtqueue_get_vring(const struct virtqueue *vq);
> > > > > +dma_addr_t virtqueue_get_desc_addr(const struct virtqueue *vq);
> > > > > +dma_addr_t virtqueue_get_avail_addr(const struct virtqueue *vq);
> > > > > +dma_addr_t virtqueue_get_used_addr(const struct virtqueue *vq);
> > > > >
> > > > > int virtqueue_resize(struct virtqueue *vq, u32 num,
> > > > > void (*recycle)(struct virtqueue *vq, void
> > > > > *buf));
> > > > > --
> > > > > 2.34.1
> > > > >
> > > > > _______________________________________________
> > > > > Virtualization mailing list
> > > > > Virtualization@lists.linux-foundation.org
> > > > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.linuxfoundation.org%2Fmailman%2Flistinfo%2Fvirtualization&data=05%7C01%7Cfeliu%40nvidia.com%7C0f6803f1797f448823ac08db1fdf67e5%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C638138816610707030%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=FxggnD7U8o%2B%2BqcYnHN6nc%2BemGVRU1ia5sA4k%2FRTDD7U%3D&reserved=0
> > > > --
> > > > Woke up in my clothes again this morning, don't know exactly where I am.
> > >
> >
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
