Re: [PATCH v18 25/26] drm/virtio: Support shmem shrinking
On Sun, Oct 29, 2023 at 4:03 PM Dmitry Osipenko <
dmitry.osipe...@collabora.com> wrote:
> Support generic drm-shmem memory shrinker and add new madvise IOCTL to
> the VirtIO-GPU driver. BO cache manager of Mesa driver will mark BOs as
> "don't need" using the new IOCTL to let shrinker purge the marked BOs on
> OOM, the shrinker will also evict unpurgeable shmem BOs from memory if
> guest supports SWAP file or partition.
>
> Acked-by: Gerd Hoffmann
> Signed-off-by: Daniel Almeida
> Signed-off-by: Dmitry Osipenko
> ---
> drivers/gpu/drm/virtio/virtgpu_drv.h| 13 +-
> drivers/gpu/drm/virtio/virtgpu_gem.c| 35 ++
> drivers/gpu/drm/virtio/virtgpu_ioctl.c | 25 ++
> drivers/gpu/drm/virtio/virtgpu_kms.c| 8
> drivers/gpu/drm/virtio/virtgpu_object.c | 61 +
> drivers/gpu/drm/virtio/virtgpu_vq.c | 40
> include/uapi/drm/virtgpu_drm.h | 14 ++
> 7 files changed, 195 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h
> b/drivers/gpu/drm/virtio/virtgpu_drv.h
> index 421f524ae1de..33a78b24c272 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_drv.h
> +++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
> @@ -278,7 +278,7 @@ struct virtio_gpu_fpriv {
> };
>
> /* virtgpu_ioctl.c */
> -#define DRM_VIRTIO_NUM_IOCTLS 12
> +#define DRM_VIRTIO_NUM_IOCTLS 13
> extern struct drm_ioctl_desc virtio_gpu_ioctls[DRM_VIRTIO_NUM_IOCTLS];
> void virtio_gpu_create_context(struct drm_device *dev, struct drm_file
> *file);
>
> @@ -316,6 +316,8 @@ void virtio_gpu_array_put_free_delayed(struct
> virtio_gpu_device *vgdev,
> void virtio_gpu_array_put_free_work(struct work_struct *work);
> int virtio_gpu_array_prepare(struct virtio_gpu_device *vgdev,
> struct virtio_gpu_object_array *objs);
> +int virtio_gpu_gem_host_mem_release(struct virtio_gpu_object *bo);
> +int virtio_gpu_gem_madvise(struct virtio_gpu_object *obj, int madv);
> int virtio_gpu_gem_pin(struct virtio_gpu_object *bo);
> void virtio_gpu_gem_unpin(struct virtio_gpu_object *bo);
>
> @@ -329,6 +331,8 @@ void virtio_gpu_cmd_create_resource(struct
> virtio_gpu_device *vgdev,
> struct virtio_gpu_fence *fence);
> void virtio_gpu_cmd_unref_resource(struct virtio_gpu_device *vgdev,
>struct virtio_gpu_object *bo);
> +int virtio_gpu_cmd_release_resource(struct virtio_gpu_device *vgdev,
> + struct virtio_gpu_object *bo);
> void virtio_gpu_cmd_transfer_to_host_2d(struct virtio_gpu_device *vgdev,
> uint64_t offset,
> uint32_t width, uint32_t height,
> @@ -349,6 +353,9 @@ void virtio_gpu_object_attach(struct virtio_gpu_device
> *vgdev,
> struct virtio_gpu_object *obj,
> struct virtio_gpu_mem_entry *ents,
> unsigned int nents);
> +void virtio_gpu_object_detach(struct virtio_gpu_device *vgdev,
> + struct virtio_gpu_object *obj,
> + struct virtio_gpu_fence *fence);
> void virtio_gpu_cursor_ping(struct virtio_gpu_device *vgdev,
> struct virtio_gpu_output *output);
> int virtio_gpu_cmd_get_display_info(struct virtio_gpu_device *vgdev);
> @@ -492,4 +499,8 @@ void virtio_gpu_vram_unmap_dma_buf(struct device *dev,
> int virtio_gpu_execbuffer_ioctl(struct drm_device *dev, void *data,
> struct drm_file *file);
>
> +/* virtgpu_gem_shrinker.c */
> +int virtio_gpu_gem_shrinker_init(struct virtio_gpu_device *vgdev);
> +void virtio_gpu_gem_shrinker_fini(struct virtio_gpu_device *vgdev);
> +
> #endif
> diff --git a/drivers/gpu/drm/virtio/virtgpu_gem.c
> b/drivers/gpu/drm/virtio/virtgpu_gem.c
> index 97e67064c97e..748f7bbb0e6d 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_gem.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_gem.c
> @@ -147,10 +147,20 @@ void virtio_gpu_gem_object_close(struct
> drm_gem_object *obj,
> struct virtio_gpu_device *vgdev = obj->dev->dev_private;
> struct virtio_gpu_fpriv *vfpriv = file->driver_priv;
> struct virtio_gpu_object_array *objs;
> + struct virtio_gpu_object *bo;
>
> if (!vgdev->has_virgl_3d)
> return;
>
> + bo = gem_to_virtio_gpu_obj(obj);
> +
> + /*
> +* Purged BO was already detached and released, the resource ID
> +* is invalid by now.
> +*/
> + if (!virtio_gpu_gem_madvise(bo, VIRTGPU_MADV_WILLNEED))
> + return;
> +
> objs = virtio_gpu_array_alloc(1);
> if (!objs)
> return;
> @@ -315,6 +325,31 @@ int virtio_gpu_array_prepare(struct virtio_gpu_device
> *vgdev,
> return ret;
> }
>
> +int virtio_gpu_gem_madvise(struct virtio_gpu_object *bo, int madv)
> +{
> + if (virtio_gpu_is_
Re: [PATCH V3] vdpa/mlx5: preserve CVQ vringh index
In V3 I added the fixes tag and the acks. No other changes.
- Steve
On 11/3/2023 8:26 AM, Steve Sistare wrote:
> mlx5_vdpa does not preserve userland's view of vring base for the control
> queue in the following sequence:
>
> ioctl VHOST_SET_VRING_BASE
> ioctl VHOST_VDPA_SET_STATUS VIRTIO_CONFIG_S_DRIVER_OK
> mlx5_vdpa_set_status()
> setup_cvq_vring()
> vringh_init_iotlb()
> vringh_init_kern()
> vrh->last_avail_idx = 0;
> ioctl VHOST_GET_VRING_BASE
>
> To fix, restore the value of cvq->vring.last_avail_idx after calling
> vringh_init_iotlb.
>
> Fixes: 5262912ef3cf ("vdpa/mlx5: Add support for control VQ and MAC setting")
>
> Signed-off-by: Steve Sistare
> Acked-by: Eugenio Pérez
> Acked-by: Jason Wang
> ---
> drivers/vdpa/mlx5/net/mlx5_vnet.c | 7 ++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> index 946488b8989f..ca972af3c89a 100644
> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> @@ -2795,13 +2795,18 @@ static int setup_cvq_vring(struct mlx5_vdpa_dev
> *mvdev)
> struct mlx5_control_vq *cvq = &mvdev->cvq;
> int err = 0;
>
> - if (mvdev->actual_features & BIT_ULL(VIRTIO_NET_F_CTRL_VQ))
> + if (mvdev->actual_features & BIT_ULL(VIRTIO_NET_F_CTRL_VQ)) {
> + u16 idx = cvq->vring.last_avail_idx;
> +
> err = vringh_init_iotlb(&cvq->vring, mvdev->actual_features,
> MLX5_CVQ_MAX_ENT, false,
> (struct vring_desc
> *)(uintptr_t)cvq->desc_addr,
> (struct vring_avail
> *)(uintptr_t)cvq->driver_addr,
> (struct vring_used
> *)(uintptr_t)cvq->device_addr);
>
> + if (!err)
> + cvq->vring.last_avail_idx = cvq->vring.last_used_idx =
> idx;
> + }
> return err;
> }
>
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH V3] vdpa/mlx5: preserve CVQ vringh index
mlx5_vdpa does not preserve userland's view of vring base for the control
queue in the following sequence:
ioctl VHOST_SET_VRING_BASE
ioctl VHOST_VDPA_SET_STATUS VIRTIO_CONFIG_S_DRIVER_OK
mlx5_vdpa_set_status()
setup_cvq_vring()
vringh_init_iotlb()
vringh_init_kern()
vrh->last_avail_idx = 0;
ioctl VHOST_GET_VRING_BASE
To fix, restore the value of cvq->vring.last_avail_idx after calling
vringh_init_iotlb.
Fixes: 5262912ef3cf ("vdpa/mlx5: Add support for control VQ and MAC setting")
Signed-off-by: Steve Sistare
Acked-by: Eugenio Pérez
Acked-by: Jason Wang
---
drivers/vdpa/mlx5/net/mlx5_vnet.c | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c
b/drivers/vdpa/mlx5/net/mlx5_vnet.c
index 946488b8989f..ca972af3c89a 100644
--- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
+++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
@@ -2795,13 +2795,18 @@ static int setup_cvq_vring(struct mlx5_vdpa_dev *mvdev)
struct mlx5_control_vq *cvq = &mvdev->cvq;
int err = 0;
- if (mvdev->actual_features & BIT_ULL(VIRTIO_NET_F_CTRL_VQ))
+ if (mvdev->actual_features & BIT_ULL(VIRTIO_NET_F_CTRL_VQ)) {
+ u16 idx = cvq->vring.last_avail_idx;
+
err = vringh_init_iotlb(&cvq->vring, mvdev->actual_features,
MLX5_CVQ_MAX_ENT, false,
(struct vring_desc
*)(uintptr_t)cvq->desc_addr,
(struct vring_avail
*)(uintptr_t)cvq->driver_addr,
(struct vring_used
*)(uintptr_t)cvq->device_addr);
+ if (!err)
+ cvq->vring.last_avail_idx = cvq->vring.last_used_idx =
idx;
+ }
return err;
}
--
2.39.3
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v4 4/4] vduse: Add LSM hooks to check Virtio device type
On Fri, Nov 03, 2023 at 08:55:19AM +0100, Maxime Coquelin wrote: > > > On 11/2/23 19:59, Michael S. Tsirkin wrote: > > On Thu, Nov 02, 2023 at 06:56:59PM +0100, Maxime Coquelin wrote: > > > > > > > > > On 10/24/23 17:30, Casey Schaufler wrote: > > > > On 10/24/2023 2:49 AM, Maxime Coquelin wrote: > > > > > > > > > > > > > > > On 10/23/23 17:13, Casey Schaufler wrote: > > > > > > On 10/23/2023 12:28 AM, Maxime Coquelin wrote: > > > > > > > > > > > > > > > > > > > > > On 10/21/23 00:20, Casey Schaufler wrote: > > > > > > > > On 10/20/2023 8:58 AM, Maxime Coquelin wrote: > > > > > > > > > This patch introduces LSM hooks for devices creation, > > > > > > > > > destruction and opening operations, checking the > > > > > > > > > application is allowed to perform these operations for > > > > > > > > > the Virtio device type. > > > > > > > > > > > > > > > > Why do you think that there needs to be a special LSM check for > > > > > > > > virtio > > > > > > > > devices? What can't existing device attributes be used? > > > > > > > > > > > > > > Michael asked for a way for SELinux to allow/prevent the creation > > > > > > > of > > > > > > > some types of devices [0]. > > > > > > > > > > > > > > A device is created using ioctl() on VDUSE control chardev. Its > > > > > > > type is > > > > > > > specified via a field in the structure passed in argument. > > > > > > > > > > > > > > I didn't see other way than adding dedicated LSM hooks to achieve > > > > > > > this, > > > > > > > but it is possible that their is a better way to do it? > > > > > > > > > > > > At the very least the hook should be made more general, and I'd > > > > > > have to > > > > > > see a proposal before commenting on that. security_dev_destroy(dev) > > > > > > might > > > > > > be a better approach. If there's reason to control destruction of > > > > > > vduse > > > > > > devices it's reasonable to assume that there are other devices with > > > > > > the > > > > > > same or similar properties. > > > > > > > > > > VDUSE is different from other devices as the device is actually > > > > > implemented by the user-space application, so this is very specific in > > > > > my opinion. > > > > > > > > This is hardly unique. If you're implementing the device > > > > in user-space you may well be able to implement the desired > > > > controls there. > > > > > > > > > > > > > > > > > > > > > Since SELinux is your target use case, can you explain why you can't > > > > > > create SELinux policy to enforce the restrictions you're after? I > > > > > > believe > > > > > > (but can be proven wrong, of course) that SELinux has mechanism for > > > > > > dealing > > > > > > with controls on ioctls. > > > > > > > > > > > > > > > > I am not aware of such mechanism to deal with ioctl(), if you have a > > > > > pointer that would be welcome. > > > > > > > > security/selinux/hooks.c > > > > > > We might be able to extend selinux_file_ioctl(), but that will only > > > covers the ioctl for the control file, this patch also adds hook for the > > > device file opening that would need dedicated hook as the device type > > > information is stored in the device's private data. > > > > > > Michael, before going further, I would be interested in your feedback. > > > Was this patch what you had in mind when requesting for a way to > > > allow/deny devices types for a given application? > > > > > > Regards, > > > Maxime > > > > > > Yes, this is more or less what I had in mind. > > Great. > > Do you think we need to cover both ioctl() on the control file and > open() on the device file, or only ioctl() is enough? > > If the former, we will need VDUSE-specific hooks. I may be able to > improve my patch to have a single hook instead of 3 by passing the type > of operation as an extra argument (create/destroy/open). > > If the latter, we may be able to extend the generic ioctl hook. > > Personally, I think it would make sense to also ensure a given > application can only open existing VDUSE devices it supports. For > example, openvswitch should only be allowed to open networking VDUSE > devices. > > Thanks, > Maxime I agree here. I think an open hook is important. Make sure to document the need in the cover letter and commit log. > > > > > > > > > > > > > > > > Thanks, > > > > > Maxime > > > > > > > > > > > > > > > > > > > > > > > > > Thanks, > > > > > > > Maxime > > > > > > > > > > > > > > [0]: > > > > > > > https://lore.kernel.org/all/20230829130430-mutt-send-email-...@kernel.org/ > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v4 4/4] vduse: Add LSM hooks to check Virtio device type
On 11/2/23 19:59, Michael S. Tsirkin wrote: On Thu, Nov 02, 2023 at 06:56:59PM +0100, Maxime Coquelin wrote: On 10/24/23 17:30, Casey Schaufler wrote: On 10/24/2023 2:49 AM, Maxime Coquelin wrote: On 10/23/23 17:13, Casey Schaufler wrote: On 10/23/2023 12:28 AM, Maxime Coquelin wrote: On 10/21/23 00:20, Casey Schaufler wrote: On 10/20/2023 8:58 AM, Maxime Coquelin wrote: This patch introduces LSM hooks for devices creation, destruction and opening operations, checking the application is allowed to perform these operations for the Virtio device type. Why do you think that there needs to be a special LSM check for virtio devices? What can't existing device attributes be used? Michael asked for a way for SELinux to allow/prevent the creation of some types of devices [0]. A device is created using ioctl() on VDUSE control chardev. Its type is specified via a field in the structure passed in argument. I didn't see other way than adding dedicated LSM hooks to achieve this, but it is possible that their is a better way to do it? At the very least the hook should be made more general, and I'd have to see a proposal before commenting on that. security_dev_destroy(dev) might be a better approach. If there's reason to control destruction of vduse devices it's reasonable to assume that there are other devices with the same or similar properties. VDUSE is different from other devices as the device is actually implemented by the user-space application, so this is very specific in my opinion. This is hardly unique. If you're implementing the device in user-space you may well be able to implement the desired controls there. Since SELinux is your target use case, can you explain why you can't create SELinux policy to enforce the restrictions you're after? I believe (but can be proven wrong, of course) that SELinux has mechanism for dealing with controls on ioctls. I am not aware of such mechanism to deal with ioctl(), if you have a pointer that would be welcome. security/selinux/hooks.c We might be able to extend selinux_file_ioctl(), but that will only covers the ioctl for the control file, this patch also adds hook for the device file opening that would need dedicated hook as the device type information is stored in the device's private data. Michael, before going further, I would be interested in your feedback. Was this patch what you had in mind when requesting for a way to allow/deny devices types for a given application? Regards, Maxime Yes, this is more or less what I had in mind. Great. Do you think we need to cover both ioctl() on the control file and open() on the device file, or only ioctl() is enough? If the former, we will need VDUSE-specific hooks. I may be able to improve my patch to have a single hook instead of 3 by passing the type of operation as an extra argument (create/destroy/open). If the latter, we may be able to extend the generic ioctl hook. Personally, I think it would make sense to also ensure a given application can only open existing VDUSE devices it supports. For example, openvswitch should only be allowed to open networking VDUSE devices. Thanks, Maxime Thanks, Maxime Thanks, Maxime [0]: https://lore.kernel.org/all/20230829130430-mutt-send-email-...@kernel.org/ ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH V2 vfio 5/9] virtio-pci: Initialize the supported admin commands
On Fri, Nov 03, 2023 at 08:33:06AM +0800, kernel test robot wrote:
> Hi Yishai,
>
> kernel test robot noticed the following build warnings:
>
> [auto build test WARNING on awilliam-vfio/for-linus]
> [also build test WARNING on linus/master v6.6]
> [cannot apply to awilliam-vfio/next mst-vhost/linux-next next-20231102]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
>
> url:
> https://github.com/intel-lab-lkp/linux/commits/Yishai-Hadas/virtio-Define-feature-bit-for-administration-virtqueue/20231030-000414
> base: https://github.com/awilliam/linux-vfio.git for-linus
> patch link:
> https://lore.kernel.org/r/20231029155952.67686-6-yishaih%40nvidia.com
> patch subject: [PATCH V2 vfio 5/9] virtio-pci: Initialize the supported admin
> commands
> config: i386-randconfig-061-20231102
> (https://download.01.org/0day-ci/archive/20231103/202311030838.gjyabtjm-...@intel.com/config)
> compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0
> reproduce (this is a W=1 build):
> (https://download.01.org/0day-ci/archive/20231103/202311030838.gjyabtjm-...@intel.com/reproduce)
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version
> of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot
> | Closes:
> https://lore.kernel.org/oe-kbuild-all/202311030838.gjyabtjm-...@intel.com/
>
> sparse warnings: (new ones prefixed by >>)
> >> drivers/virtio/virtio_pci_modern.c:726:16: sparse: sparse: restricted
> >> __le16 degrades to integer
>
> vim +726 drivers/virtio/virtio_pci_modern.c
>
>673
>674static int vp_modern_admin_cmd_exec(struct virtio_device *vdev,
>675struct virtio_admin_cmd
> *cmd)
>676{
>677struct scatterlist *sgs[VIRTIO_AVQ_SGS_MAX], hdr, stat;
>678struct virtio_pci_device *vp_dev = to_vp_device(vdev);
>679struct virtio_admin_cmd_status *va_status;
>680unsigned int out_num = 0, in_num = 0;
>681struct virtio_admin_cmd_hdr *va_hdr;
>682struct virtqueue *avq;
>683u16 status;
>684int ret;
>685
>686avq = virtio_has_feature(vdev, VIRTIO_F_ADMIN_VQ) ?
>687vp_dev->admin_vq.info.vq : NULL;
>688if (!avq)
>689return -EOPNOTSUPP;
>690
>691va_status = kzalloc(sizeof(*va_status), GFP_KERNEL);
>692if (!va_status)
>693return -ENOMEM;
>694
>695va_hdr = kzalloc(sizeof(*va_hdr), GFP_KERNEL);
>696if (!va_hdr) {
>697ret = -ENOMEM;
>698goto err_alloc;
>699}
>700
>701va_hdr->opcode = cmd->opcode;
>702va_hdr->group_type = cmd->group_type;
>703va_hdr->group_member_id = cmd->group_member_id;
>704
>705/* Add header */
>706sg_init_one(&hdr, va_hdr, sizeof(*va_hdr));
>707sgs[out_num] = &hdr;
>708out_num++;
>709
>710if (cmd->data_sg) {
>711sgs[out_num] = cmd->data_sg;
>712out_num++;
>713}
>714
>715/* Add return status */
>716sg_init_one(&stat, va_status, sizeof(*va_status));
>717sgs[out_num + in_num] = &stat;
>718in_num++;
>719
>720if (cmd->result_sg) {
>721sgs[out_num + in_num] = cmd->result_sg;
>722in_num++;
>723}
>724
>725if (cmd->opcode == VIRTIO_ADMIN_CMD_LIST_QUERY ||
> > 726cmd->opcode == VIRTIO_ADMIN_CMD_LIST_USE)
yes, this is broken on BE. You need to convert enums to LE before you
compare.
>727ret =
> __virtqueue_exec_admin_cmd(&vp_dev->admin_vq, sgs,
>728 out_num, in_num,
>729 sgs, GFP_KERNEL);
>730else
>
