[PATCH] drm/i915/gvt: clean up vGPU resource r/w length
Du, Changbin
changbin.du at intel.com
Mon Aug 7 05:50:47 UTC 2017
On Wed, Aug 02, 2017 at 01:27:25PM +0800, Zhenyu Wang wrote:
> We can do max of 64bit for vGPU resource e.g MMIO r/w access, but
> original KVMGT code just always force to do max of 32bit access.
> This cleans up for old duplicated code block and allows for 64bit
> access in one call. Also remove unnecessary resource length check
> for both MMIO and config space access.
>
> Cc: Xiong Zhang <xiong.y.zhang at intel.com>
> Signed-off-by: Zhenyu Wang <zhenyuw at linux.intel.com>
> ---
> drivers/gpu/drm/i915/gvt/cfg_space.c | 6 ---
> drivers/gpu/drm/i915/gvt/handlers.c | 3 --
> drivers/gpu/drm/i915/gvt/kvmgt.c | 86 +++++++++---------------------------
> 3 files changed, 22 insertions(+), 73 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gvt/cfg_space.c b/drivers/gpu/drm/i915/gvt/cfg_space.c
> index 40af17ec6312..7cf90db5ec00 100644
> --- a/drivers/gpu/drm/i915/gvt/cfg_space.c
> +++ b/drivers/gpu/drm/i915/gvt/cfg_space.c
> @@ -98,9 +98,6 @@ static void vgpu_pci_cfg_mem_write(struct intel_vgpu *vgpu, unsigned int off,
> int intel_vgpu_emulate_cfg_read(struct intel_vgpu *vgpu, unsigned int offset,
> void *p_data, unsigned int bytes)
> {
> - if (WARN_ON(bytes > 4))
> - return -EINVAL;
> -
> if (WARN_ON(offset + bytes > INTEL_GVT_MAX_CFG_SPACE_SZ))
> return -EINVAL;
>
> @@ -285,9 +282,6 @@ int intel_vgpu_emulate_cfg_write(struct intel_vgpu *vgpu, unsigned int offset,
> {
> int ret;
>
> - if (WARN_ON(bytes > 4))
> - return -EINVAL;
> -
> if (WARN_ON(offset + bytes > INTEL_GVT_MAX_CFG_SPACE_SZ))
> return -EINVAL;
>
> diff --git a/drivers/gpu/drm/i915/gvt/handlers.c b/drivers/gpu/drm/i915/gvt/handlers.c
> index d85264d48585..b8efda3374fa 100644
> --- a/drivers/gpu/drm/i915/gvt/handlers.c
> +++ b/drivers/gpu/drm/i915/gvt/handlers.c
> @@ -3034,9 +3034,6 @@ int intel_vgpu_mmio_reg_rw(struct intel_vgpu *vgpu, unsigned int offset,
> gvt_mmio_func func;
> int ret;
>
> - if (WARN_ON(bytes > 4))
> - return -EINVAL;
> -
> /*
> * Handle special MMIO blocks.
> */
> diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c
> index fd0c85f9ef3c..9e8e3562bdef 100644
> --- a/drivers/gpu/drm/i915/gvt/kvmgt.c
> +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
> @@ -692,47 +692,26 @@ static ssize_t intel_vgpu_read(struct mdev_device *mdev, char __user *buf,
> {
> unsigned int done = 0;
> int ret;
> + char tmp[8];
>
> while (count) {
> size_t filled;
>
> - if (count >= 4 && !(*ppos % 4)) {
> - u32 val;
> -
> - ret = intel_vgpu_rw(mdev, (char *)&val, sizeof(val),
> - ppos, false);
> - if (ret <= 0)
> - goto read_err;
> -
> - if (copy_to_user(buf, &val, sizeof(val)))
> - goto read_err;
> -
> + if (count >= 8 && !(*ppos % 8)) {
> + filled = 8;
> + } else if (count >= 4 && !(*ppos % 4)) {
> filled = 4;
> } else if (count >= 2 && !(*ppos % 2)) {
> - u16 val;
> -
> - ret = intel_vgpu_rw(mdev, (char *)&val, sizeof(val),
> - ppos, false);
> - if (ret <= 0)
> - goto read_err;
> -
> - if (copy_to_user(buf, &val, sizeof(val)))
> - goto read_err;
> -
> filled = 2;
> - } else {
> - u8 val;
> -
> - ret = intel_vgpu_rw(mdev, &val, sizeof(val), ppos,
> - false);
> - if (ret <= 0)
> - goto read_err;
> + } else
> + filled = 1;
>
> - if (copy_to_user(buf, &val, sizeof(val)))
> - goto read_err;
> + ret = intel_vgpu_rw(mdev, tmp, filled, ppos, false);
> + if (ret <= 0)
> + goto read_err;
>
> - filled = 1;
> - }
> + if (copy_to_user(buf, tmp, filled))
> + goto read_err;
>
> count -= filled;
> done += filled;
zhenyu/xiong, how about move such function to device module so xengt and kvmgt can
share this snippet?
And I have a concern for opening larger (> 4 bytes) MMIO access - The tracked
registers are all handled by one DWORD, so access larger MMIO (8bytes for example)
only the handler of first DWORD executed. What's more, there's a securtity issue
that acess the last MMIO with >4 bytes can lead to memroy overflow. The first
concern is not important since BSpec has required for 32bit access, but the
second one should take account.
> @@ -752,47 +731,26 @@ static ssize_t intel_vgpu_write(struct mdev_device *mdev,
> {
> unsigned int done = 0;
> int ret;
> + char val[8];
>
> while (count) {
> size_t filled;
>
> - if (count >= 4 && !(*ppos % 4)) {
> - u32 val;
> -
> - if (copy_from_user(&val, buf, sizeof(val)))
> - goto write_err;
> -
> - ret = intel_vgpu_rw(mdev, (char *)&val, sizeof(val),
> - ppos, true);
> - if (ret <= 0)
> - goto write_err;
> -
> + if (count >= 8 && !(*ppos % 8)) {
> + filled = 8;
> + } else if (count >= 4 && !(*ppos % 4)) {
> filled = 4;
> } else if (count >= 2 && !(*ppos % 2)) {
> - u16 val;
> -
> - if (copy_from_user(&val, buf, sizeof(val)))
> - goto write_err;
> -
> - ret = intel_vgpu_rw(mdev, (char *)&val,
> - sizeof(val), ppos, true);
> - if (ret <= 0)
> - goto write_err;
> -
> filled = 2;
> - } else {
> - u8 val;
> -
> - if (copy_from_user(&val, buf, sizeof(val)))
> - goto write_err;
> + } else
> + filled = 1;
>
> - ret = intel_vgpu_rw(mdev, &val, sizeof(val),
> - ppos, true);
> - if (ret <= 0)
> - goto write_err;
> + if (copy_from_user(val, buf, filled))
> + goto write_err;
>
> - filled = 1;
> - }
> + ret = intel_vgpu_rw(mdev, val, filled, ppos, true);
> + if (ret <= 0)
> + goto write_err;
>
> count -= filled;
> done += filled;
> --
> 2.13.2
>
> _______________________________________________
> intel-gvt-dev mailing list
> intel-gvt-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
--
Thanks,
Changbin Du
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/intel-gvt-dev/attachments/20170807/f1c0dde4/attachment.sig>
More information about the intel-gvt-dev
mailing list