Re: [PATCH 3/3] virtio_ring: Use const to annotate read-only pointer params

[Feng Liu via Virtualization <virtualization@lists.linux-foundation.org>]

On 2023-03-08 a.m.11:25, Michael S. Tsirkin wrote:
> External email: Use caution opening links or attachments
> 
> 
> 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.
> 
> 
Hi, Michael
Ok, I see. Thanks for your explaination. I will push the next version of 
this patch after you post

> 
> 
>>>>
>>>>>> Signed-off-by: Feng Liu <feliu@nvidia.com>
>>>>>> Reviewed-by: Jiri Pirko <jiri@nvidia.com>
>>>>>> Reviewed-by: Parav Pandit <parav@nvidia.com>
>>>>>> Reviewed-by: Gavin Li <gavinl@nvidia.com>
>>>>>> Reviewed-by: Bodong Wang <bodong@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%7C7dc87cb48ac44bead13808db1ff1d29a%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C638138895717817297%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=ZT5o%2Fs2TWQT6yP7tUUDnV4ojKgKtUmEzSVpnDTs5PtU%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