[PATCH] drm/i915/gvt: Enable GVT vGPU enabled guest S3 and resume

Colin Xu Colin.Xu at intel.com
Thu May 21 04:52:43 UTC 2020


On 2020-05-21 11:56, Zhenyu Wang wrote:
> On 2020.05.21 08:24:23 +0800, Colin Xu wrote:
>> On 2020-05-18 18:32, Zhenyu Wang wrote:
>>> On 2020.05.18 14:19:10 +0800, Colin Xu wrote:
>>>> This patch enables GVT vGPU enabled guest enter and resume from S3.
>>>> During S3, QEMU process is still active so all ppgtt still in memory
>>>> thus no need to invalidate them. However current ppgtt invalidation and
>>>> destroy logic happens in dmlr, this operation happens during both boot
>>>> and resuming. Thus we need differ these different power states for
>>>> proper handling.
>>>> vCPU PCI cfg space is a modified copy from host. If PM cap is supported
>>>> on host, vGPU cfg space will also report the capability and handle read
>>>> or write request on PMCSR register. On receiving PCI_D3hot when enter S3,
>>>> vGPU will track current PM status. During dmlr, gvt will check if current
>>>> dmrl is during normal boot or S3 resume. If S3 resume, skip the ppgtt mm
>>>> invalidation and destroy so that they can be re-used, then clear the PM
>>>> status flag. PCI_D0 is set prior to dmlr so can't depend on it to skip
>>>> the ppgtt invalidation and destroy operation.
>>>>
>>>> To test this feature, make sure S3 is enabled in QEMU parameters:
>>>> i440fx: PIIX4_PM.disable_s3=0
>>>> q35: ICH9-LPC.disable_s3=0
>>>> Also need enable sleep option in guest OS if it's disabled.
>>>>
>>>> With all required options enabled and proper GFX driver installed, user
>>>> can enter S3 from guest OS, and resume from QEMU monitor using
>>>> system_wakeup.
>>>>
>>>> Signed-off-by: Hang Yuan <hang.yuan at linux.intel.com>
>>>> Signed-off-by: Colin Xu <colin.xu at intel.com>
>>>> ---
>>>>    drivers/gpu/drm/i915/gvt/cfg_space.c | 24 ++++++++++++++++++++++++
>>>>    drivers/gpu/drm/i915/gvt/gtt.c       |  2 +-
>>>>    drivers/gpu/drm/i915/gvt/gtt.h       |  2 ++
>>>>    drivers/gpu/drm/i915/gvt/gvt.h       |  3 +++
>>>>    drivers/gpu/drm/i915/gvt/vgpu.c      | 21 ++++++++++++++++++---
>>>>    5 files changed, 48 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/gvt/cfg_space.c b/drivers/gpu/drm/i915/gvt/cfg_space.c
>>>> index 072725a448db..ad86c5eb5bba 100644
>>>> --- a/drivers/gpu/drm/i915/gvt/cfg_space.c
>>>> +++ b/drivers/gpu/drm/i915/gvt/cfg_space.c
>>>> @@ -70,6 +70,7 @@ static void vgpu_pci_cfg_mem_write(struct intel_vgpu *vgpu, unsigned int off,
>>>>    {
>>>>    	u8 *cfg_base = vgpu_cfg_space(vgpu);
>>>>    	u8 mask, new, old;
>>>> +	pci_power_t pwr;
>>>>    	int i = 0;
>>>>    	for (; i < bytes && (off + i < sizeof(pci_cfg_space_rw_bmp)); i++) {
>>>> @@ -91,6 +92,15 @@ static void vgpu_pci_cfg_mem_write(struct intel_vgpu *vgpu, unsigned int off,
>>>>    	/* For other configuration space directly copy as it is. */
>>>>    	if (i < bytes)
>>>>    		memcpy(cfg_base + off + i, src + i, bytes - i);
>>>> +
>>>> +	if (off == vgpu->cfg_space.pmcsr_off && vgpu->cfg_space.pmcsr_off) {
>>>> +		pwr = (pci_power_t __force)(*(u16*)(&vgpu_cfg_space(vgpu)[off])
>>>> +			& PCI_PM_CTRL_STATE_MASK);
>>>> +		if (pwr == PCI_D3hot)
>>>> +			vgpu->d3_entered = true;
>>>> +		gvt_dbg_core("vgpu-%d power status changed to %d\n",
>>>> +			     vgpu->id, pwr);
>>>> +	}
>>> Good for tracking PM state, that's exactly what I expected. But how about
>>> other Dx state? e.g if guest wants to do suspend-to-idle, would it hit same
>>> penalty for ppgtt invalidate?
>> According to www.kernel.org/doc/Documentation/power/states.txt
>> <https://www.kernel.org/doc/Documentation/power/states.txt>
>>
>> S2I is purely software driven while it's ACPI state is S0, unlike S3
>> (Suspend-to-RAM). So If my understanding is correct, vGPU only handles the
>> device ACPI state change D0-D3.
>>
> If guest sets 's2idle' action then set 'mem' for power state, device would also
> be put to D3. So I think current handling should be fine. Have you verified with
> windows guest too?
Yes, both Linux guest (ubuntu) and Windows guest (Win10) are verified 
work as expected.
>
> Would you split release part and re-send?
Sure. Split intel_vgpu_destroy_all_ppgtt_mm() in release_vgpu to another 
patch.
> thanks
>
>>>>    }
>>>>    /**
>>>> @@ -366,6 +376,7 @@ void intel_vgpu_init_cfg_space(struct intel_vgpu *vgpu,
>>>>    	struct intel_gvt *gvt = vgpu->gvt;
>>>>    	const struct intel_gvt_device_info *info = &gvt->device_info;
>>>>    	u16 *gmch_ctl;
>>>> +	u8 next;
>>>>    	memcpy(vgpu_cfg_space(vgpu), gvt->firmware.cfg_space,
>>>>    	       info->cfg_space_size);
>>>> @@ -401,6 +412,19 @@ void intel_vgpu_init_cfg_space(struct intel_vgpu *vgpu,
>>>>    		pci_resource_len(gvt->gt->i915->drm.pdev, 2);
>>>>    	memset(vgpu_cfg_space(vgpu) + PCI_ROM_ADDRESS, 0, 4);
>>>> +
>>>> +	/* PM Support */
>>>> +	vgpu->cfg_space.pmcsr_off = 0;
>>>> +	if (vgpu_cfg_space(vgpu)[PCI_STATUS] & PCI_STATUS_CAP_LIST) {
>>>> +		next = vgpu_cfg_space(vgpu)[PCI_CAPABILITY_LIST];
>>>> +		do {
>>>> +			if (vgpu_cfg_space(vgpu)[next + PCI_CAP_LIST_ID] == PCI_CAP_ID_PM) {
>>>> +				vgpu->cfg_space.pmcsr_off = next + PCI_PM_CTRL;
>>>> +				break;
>>>> +			}
>>>> +			next = vgpu_cfg_space(vgpu)[next + PCI_CAP_LIST_NEXT];
>>>> +		} while (next);
>>>> +	}
>>>>    }
>>>>    /**
>>>> diff --git a/drivers/gpu/drm/i915/gvt/gtt.c b/drivers/gpu/drm/i915/gvt/gtt.c
>>>> index 210016192ce7..a3a4305eda01 100644
>>>> --- a/drivers/gpu/drm/i915/gvt/gtt.c
>>>> +++ b/drivers/gpu/drm/i915/gvt/gtt.c
>>>> @@ -2501,7 +2501,7 @@ int intel_vgpu_init_gtt(struct intel_vgpu *vgpu)
>>>>    	return create_scratch_page_tree(vgpu);
>>>>    }
>>>> -static void intel_vgpu_destroy_all_ppgtt_mm(struct intel_vgpu *vgpu)
>>>> +void intel_vgpu_destroy_all_ppgtt_mm(struct intel_vgpu *vgpu)
>>>>    {
>>>>    	struct list_head *pos, *n;
>>>>    	struct intel_vgpu_mm *mm;
>>>> diff --git a/drivers/gpu/drm/i915/gvt/gtt.h b/drivers/gpu/drm/i915/gvt/gtt.h
>>>> index 320b8d6ad92f..52d0d88abd86 100644
>>>> --- a/drivers/gpu/drm/i915/gvt/gtt.h
>>>> +++ b/drivers/gpu/drm/i915/gvt/gtt.h
>>>> @@ -279,4 +279,6 @@ int intel_vgpu_emulate_ggtt_mmio_read(struct intel_vgpu *vgpu,
>>>>    int intel_vgpu_emulate_ggtt_mmio_write(struct intel_vgpu *vgpu,
>>>>    	unsigned int off, void *p_data, unsigned int bytes);
>>>> +void intel_vgpu_destroy_all_ppgtt_mm(struct intel_vgpu *vgpu);
>>>> +
>>>>    #endif /* _GVT_GTT_H_ */
>>>> diff --git a/drivers/gpu/drm/i915/gvt/gvt.h b/drivers/gpu/drm/i915/gvt/gvt.h
>>>> index a4a6db6b7f90..ff7f2515a6fe 100644
>>>> --- a/drivers/gpu/drm/i915/gvt/gvt.h
>>>> +++ b/drivers/gpu/drm/i915/gvt/gvt.h
>>>> @@ -106,6 +106,7 @@ struct intel_vgpu_pci_bar {
>>>>    struct intel_vgpu_cfg_space {
>>>>    	unsigned char virtual_cfg_space[PCI_CFG_SPACE_EXP_SIZE];
>>>>    	struct intel_vgpu_pci_bar bar[INTEL_GVT_MAX_BAR_NUM];
>>>> +	u32 pmcsr_off;
>>>>    };
>>>>    #define vgpu_cfg_space(vgpu) ((vgpu)->cfg_space.virtual_cfg_space)
>>>> @@ -198,6 +199,8 @@ struct intel_vgpu {
>>>>    	struct intel_vgpu_submission submission;
>>>>    	struct radix_tree_root page_track_tree;
>>>>    	u32 hws_pga[I915_NUM_ENGINES];
>>>> +	/* Set on PCI_D3, reset on DMLR, not reflecting the actual PM state */
>>>> +	bool d3_entered;
>>>>    	struct dentry *debugfs;
>>>> diff --git a/drivers/gpu/drm/i915/gvt/vgpu.c b/drivers/gpu/drm/i915/gvt/vgpu.c
>>>> index 1d5ff88078bd..3779f9f28061 100644
>>>> --- a/drivers/gpu/drm/i915/gvt/vgpu.c
>>>> +++ b/drivers/gpu/drm/i915/gvt/vgpu.c
>>>> @@ -257,7 +257,9 @@ void intel_gvt_release_vgpu(struct intel_vgpu *vgpu)
>>>>    	intel_gvt_deactivate_vgpu(vgpu);
>>>>    	mutex_lock(&vgpu->vgpu_lock);
>>>> +	vgpu->d3_entered = false;
>>>>    	intel_vgpu_clean_workloads(vgpu, ALL_ENGINES);
>>>> +	intel_vgpu_destroy_all_ppgtt_mm(vgpu);
>>> I think this is good to do when release vGPU, but should be in a split patch.
>> Thanks, I'll split this one to another patch.
>>>>    	intel_vgpu_dmabuf_cleanup(vgpu);
>>>>    	mutex_unlock(&vgpu->vgpu_lock);
>>>>    }
>>>> @@ -393,6 +395,7 @@ static struct intel_vgpu *__intel_gvt_create_vgpu(struct intel_gvt *gvt,
>>>>    	INIT_RADIX_TREE(&vgpu->page_track_tree, GFP_KERNEL);
>>>>    	idr_init(&vgpu->object_idr);
>>>>    	intel_vgpu_init_cfg_space(vgpu, param->primary);
>>>> +	vgpu->d3_entered = false;
>>>>    	ret = intel_vgpu_init_mmio(vgpu);
>>>>    	if (ret)
>>>> @@ -557,10 +560,15 @@ void intel_gvt_reset_vgpu_locked(struct intel_vgpu *vgpu, bool dmlr,
>>>>    	/* full GPU reset or device model level reset */
>>>>    	if (engine_mask == ALL_ENGINES || dmlr) {
>>>>    		intel_vgpu_select_submission_ops(vgpu, ALL_ENGINES, 0);
>>>> -		intel_vgpu_invalidate_ppgtt(vgpu);
>>>> +		if (engine_mask == ALL_ENGINES)
>>>> +			intel_vgpu_invalidate_ppgtt(vgpu);
>>>>    		/*fence will not be reset during virtual reset */
>>>>    		if (dmlr) {
>>>> -			intel_vgpu_reset_gtt(vgpu);
>>>> +			if(!vgpu->d3_entered) {
>>>> +				intel_vgpu_invalidate_ppgtt(vgpu);
>>>> +				intel_vgpu_destroy_all_ppgtt_mm(vgpu);
>>>> +			}
>>>> +			intel_vgpu_reset_ggtt(vgpu, true);
>>>>    			intel_vgpu_reset_resource(vgpu);
>>>>    		}
>>>> @@ -572,7 +580,14 @@ void intel_gvt_reset_vgpu_locked(struct intel_vgpu *vgpu, bool dmlr,
>>>>    			intel_vgpu_reset_cfg_space(vgpu);
>>>>    			/* only reset the failsafe mode when dmlr reset */
>>>>    			vgpu->failsafe = false;
>>>> -			vgpu->pv_notified = false;
>>>> +			/*
>>>> +			 * PCI_D0 is set before dmlr, so reset d3_entered here
>>>> +			 * after done using.
>>>> +			 */
>>>> +			if(vgpu->d3_entered)
>>>> +				vgpu->d3_entered = false;
>>>> +			else
>>>> +				vgpu->pv_notified = false;
>>>>    		}
>>>>    	}
>>>> -- 
>>>> 2.26.2
>>>>
>>>> _______________________________________________
>>>> intel-gvt-dev mailing list
>>>> intel-gvt-dev at lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
>> -- 
>> Best Regards,
>> Colin Xu
>>
>> _______________________________________________
>> intel-gvt-dev mailing list
>> intel-gvt-dev at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev

-- 
Best Regards,
Colin Xu



More information about the intel-gvt-dev mailing list