Re: [Intel-gfx] [PATCH V4] drm/i915: Enhanced disable access to stolen memory as a guest

[Jani Nikula <jani.nikula@linux.intel.com>]

On Wed, 05 Apr 2017, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, Apr 05, 2017 at 10:08:26AM +0800, Xiong Zhang wrote:
>> Stolen memory is in RMRR and has identity mapping in iommu, so
>> gt could access stolen memory in host OS. While RMRR isn't supported
>> by kvm, both EPT and guest iommu domain lack of maaping for stolen memory,
>> so both vcpu and gt couldn't access stolen memory in guest.
>> 
>> commit "04a68a3 drm/i915/gvt: Disable access to stolen memory as a guest"
>> isn't enough in IGD passthrough environment where vgt code doesn't run. This
>> patch detects i915 running as a guest through the existence of emulated ISA
>> bridge.
>> 
>> v2:GVT-g may run in non qemu (Zhenyu)
>> v3:Make commit message clear (Daniel)
>> v4:Fix typo	
>> 
>> References: c875d2c iommu/vt-d: Exclude devices using RMRRs from IOMMU API domains

What does it mean to References: a commit? The message text doesn't say
anything about this commit.

Does this patch fix commit 04a68a3 or c875d2c? If so, there should be a
Fixes: tag.

The canonical format for commit references is

04a68a35ce6d ("drm/i915/gvt: Disable access to stolen memory as a guest")
c875d2c1b808 ("iommu/vt-d: Exclude devices using RMRRs from IOMMU API domains")

In particular, note that 12 digits is preferred for sha1 references to
avoid ambiguity in the kernel git repo.

>> Link: https://access.redhat.com/sites/default/files/attachments/rmrr-wp1.pdf

Please use References: for stuff like this. Link: is reserved for the
backlink to the patch and discussion.

>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99028
>> 
>> Signed-off-by: Xiong Zhang <xiong.y.zhang@intel.com>
>> Reviewed-by: Zhenyu Wang <zhenyuw@linux.intel.com>
>> Cc: stable@vger.kernel.org

With a Fixes: tag it would be possible to decide which stable kernels to
backport to.

> Yeah, commit message is now much improved.

I'm sorry, but from the fixes backports POV it still lacks clarity. I
wouldn't know if and where this should be backported.


BR,
Jani.


>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
>> ---
>>  drivers/gpu/drm/i915/i915_drv.c        | 1 +
>>  drivers/gpu/drm/i915/i915_drv.h        | 1 +
>>  drivers/gpu/drm/i915/i915_gem_stolen.c | 4 ++--
>>  3 files changed, 4 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>> index 03d9e45..8b807a9 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.c
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -223,6 +223,7 @@ static void intel_detect_pch(struct drm_i915_private *dev_priv)
>>  					    PCI_SUBVENDOR_ID_REDHAT_QUMRANET &&
>>  				    pch->subsystem_device ==
>>  					    PCI_SUBDEVICE_ID_QEMU)) {
>> +				dev_priv->run_on_qemu = true;
>>  				dev_priv->pch_type =
>>  					intel_virt_detect_pch(dev_priv);
>>  			} else
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index a5947a4..ad95c87 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -2145,6 +2145,7 @@ struct drm_i915_private {
>>  	struct intel_uncore uncore;
>>  
>>  	struct i915_virtual_gpu vgpu;
>> +	bool run_on_qemu;
>>  
>>  	struct intel_gvt *gvt;
>>  
>> diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
>> index f3abdc2..6a011b0 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
>> @@ -409,8 +409,8 @@ int i915_gem_init_stolen(struct drm_i915_private *dev_priv)
>>  
>>  	mutex_init(&dev_priv->mm.stolen_lock);
>>  
>> -	if (intel_vgpu_active(dev_priv)) {
>> -		DRM_INFO("iGVT-g active, disabling use of stolen memory\n");
>> +	if (dev_priv->run_on_qemu || intel_vgpu_active(dev_priv)) {
>> +		DRM_INFO("Running in guest, disabling use of stolen memory\n");
>>  		return 0;
>>  	}
>>  
>> -- 
>> 1.9.1
>> 

-- 
Jani Nikula, Intel Open Source Technology Center