[PATCH v2 3/3] drm/i915/gvt: add VFIO EDID region

[Hang Yuan]

On 1/25/19 1:16 PM, Zhenyu Wang wrote:
> On 2019.01.24 13:46:37 +0800, hang.yuan at linux.intel.com wrote:
>> From: Hang Yuan <hang.yuan at linux.intel.com>
>>
>> Implement VFIO EDID region for vgpu. Support EDID blob update and notify
>> guest on link state change via hotplug event.
>>
>> v2: add EDID sanity check and size update <zhenyu>
>>
>> Tested-by: Gerd Hoffmann <kraxel at redhat.com>
>> Signed-off-by: Hang Yuan <hang.yuan at linux.intel.com>
>> ---
> 
> Looks fine to me, some little comments below.
> 
>>   drivers/gpu/drm/i915/gvt/gvt.h       |   7 ++
>>   drivers/gpu/drm/i915/gvt/hypercall.h |   1 +
>>   drivers/gpu/drm/i915/gvt/kvmgt.c     | 138 +++++++++++++++++++++++++++++++++++
>>   drivers/gpu/drm/i915/gvt/mpt.h       |  17 +++++
>>   drivers/gpu/drm/i915/gvt/vgpu.c      |   6 ++
>>   5 files changed, 169 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/gvt/gvt.h b/drivers/gpu/drm/i915/gvt/gvt.h
>> index 8bce09d..5793619 100644
>> --- a/drivers/gpu/drm/i915/gvt/gvt.h
>> +++ b/drivers/gpu/drm/i915/gvt/gvt.h
>> @@ -127,6 +127,13 @@ struct intel_vgpu_opregion {
>>   
>>   #define vgpu_opregion(vgpu) (&(vgpu->opregion))
>>   
>> +#if IS_ENABLED(CONFIG_DRM_I915_GVT_KVMGT)
>> +	struct intel_vgpu_edid_region {
>> +		struct vfio_region_gfx_edid vfio_edid_regs;
>> +		void *edid_blob;
>> +	};
>> +#endif
> 
> I think better just move this in kvmgt.c itself which is only used there.
Henry: Will address it in next version. Thanks.

>> +
>>   struct intel_vgpu_display {
>>   	struct intel_vgpu_i2c_edid i2c_edid;
>>   	struct intel_vgpu_port ports[I915_MAX_PORTS];
>> diff --git a/drivers/gpu/drm/i915/gvt/hypercall.h b/drivers/gpu/drm/i915/gvt/hypercall.h
>> index 831ab34..4862fb1 100644
>> --- a/drivers/gpu/drm/i915/gvt/hypercall.h
>> +++ b/drivers/gpu/drm/i915/gvt/hypercall.h
>> @@ -67,6 +67,7 @@ struct intel_gvt_mpt {
>>   	int (*set_trap_area)(unsigned long handle, u64 start, u64 end,
>>   			     bool map);
>>   	int (*set_opregion)(void *vgpu);
>> +	int (*set_edid)(void *vgpu, int port_num);
>>   	int (*get_vfio_device)(void *vgpu);
>>   	void (*put_vfio_device)(void *vgpu);
>>   	bool (*is_valid_gfn)(unsigned long handle, unsigned long gfn);
>> diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c
>> index 5bec0b5..ae88f17 100644
>> --- a/drivers/gpu/drm/i915/gvt/kvmgt.c
>> +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
>> @@ -57,6 +57,8 @@ static const struct intel_gvt_ops *intel_gvt_ops;
>>   #define VFIO_PCI_INDEX_TO_OFFSET(index) ((u64)(index) << VFIO_PCI_OFFSET_SHIFT)
>>   #define VFIO_PCI_OFFSET_MASK    (((u64)(1) << VFIO_PCI_OFFSET_SHIFT) - 1)
>>   
>> +#define EDID_BLOB_OFFSET (PAGE_SIZE/2)
>> +
>>   #define OPREGION_SIGNATURE "IntelGraphicsMem"
>>   
>>   struct vfio_region;
>> @@ -427,6 +429,111 @@ static const struct intel_vgpu_regops intel_vgpu_regops_opregion = {
>>   	.release = intel_vgpu_reg_release_opregion,
>>   };
>>   
>> +static int handle_edid_regs(struct intel_vgpu *vgpu,
>> +			struct intel_vgpu_edid_region *region, char *buf,
>> +			size_t count, u16 offset, bool is_write)
>> +{
>> +	struct vfio_region_gfx_edid *regs = &region->vfio_edid_regs;
>> +	unsigned int data;
>> +
>> +	if (offset + count > sizeof(*regs))
>> +		return -EINVAL;
>> +
>> +	if (count != 4)
>> +		return -EINVAL;
>> +
>> +	if (is_write) {
>> +		data = *((unsigned int *)buf);
>> +		switch (offset) {
>> +		case offsetof(struct vfio_region_gfx_edid, link_state):
>> +			if (data == VFIO_DEVICE_GFX_LINK_STATE_UP) {
>> +				if (!drm_edid_block_valid(
>> +					(u8 *)region->edid_blob,
>> +					0,
>> +					true,
>> +					NULL)) {
>> +					gvt_vgpu_err("invalid EDID blob\n");
>> +					return -EINVAL;
>> +				}
> 
> Might just write if (data == ...STATE_UP && !drm_edid_block_valid(...)) return -EINVAL;
Henry: Sounds like it will not reduce the number of 'if'. Then still 
need to check if (data == ...STATE_UP && drm_edid_block_valid(...)) 
emulate_hotplug();

>> +				intel_gvt_ops->emulate_hotplug(vgpu, true);
>> +			} else if (data == VFIO_DEVICE_GFX_LINK_STATE_DOWN)
>> +				intel_gvt_ops->emulate_hotplug(vgpu, false);
>> +			else {
>> +				gvt_vgpu_err("invalid EDID link state %d\n",
>> +					regs->link_state);
>> +				return -EINVAL;
>> +			}
>> +			regs->link_state = data;
>> +			break;
>> +		case offsetof(struct vfio_region_gfx_edid, edid_size):
>> +			if (data > regs->edid_max_size) {
>> +				gvt_vgpu_err("EDID size is bigger than %d!\n",
>> +					regs->edid_max_size);
>> +				return -EINVAL;
>> +			}
>> +			regs->edid_size = data;
>> +			break;
>> +		default:
>> +			/* read-only regs */
>> +			gvt_vgpu_err("write read-only EDID region at offset %d\n",
>> +				offset);
>> +			return -EPERM;
>> +		}
>> +	} else {
>> +		memcpy(buf, (char *)regs + offset, count);
>> +	}
>> +
>> +	return count;
>> +}
>> +