On 10/11/2018 04:04 PM, Chris Wilson wrote:
> Quoting Xiaolin Zhang (2018-10-11 07:24:19)
>> To enable pvmmio feature, we need to prepare one 4K shared page
>> which will be accessed by both guest and backend i915 driver.
>>
>> guest i915 allocate one page memory and then the guest physical address is
>> passed to backend i915 driver through PVINFO register so that backend i915
>> driver can access this shared page without hypeviser trap cost for shared
>> data exchagne via hyperviser read_gpa functionality.
>>
>> v1: addressed RFC comment to move both shared_page_lock and shared_page
>> to i915_virtual_gpu structure
>> v0: RFC
>>
>> Signed-off-by: Xiaolin Zhang <xiaolin.zh...@intel.com>
>> ---
>> drivers/gpu/drm/i915/i915_drv.c | 8 ++++++++
>> drivers/gpu/drm/i915/i915_drv.h | 2 ++
>> drivers/gpu/drm/i915/i915_pvinfo.h | 24 +++++++++++++++++++++++-
>> drivers/gpu/drm/i915/i915_vgpu.c | 19 ++++++++++++++++++-
>> 4 files changed, 51 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c
>> b/drivers/gpu/drm/i915/i915_drv.c
>> index 19302342..9b25bb7 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.c
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -987,6 +987,10 @@ static void i915_mmio_cleanup(struct drm_i915_private
>> *dev_priv)
>>
>> intel_teardown_mchbar(dev_priv);
>> pci_iounmap(pdev, dev_priv->regs);
>> + if (intel_vgpu_active(dev_priv) && dev_priv->vgpu.shared_page) {
>> + free_page((unsigned long)dev_priv->vgpu.shared_page);
>> + dev_priv->vgpu.shared_page = NULL;
>> + }
> intel_vgpu_(teardown,cleanup,fini)
>
> setting to NULL? fetch_and_zero() if you must, but here it is a little
> pointless.
agree with you it is a little pointless to set value to NULL. I will
move it to intel_gvt_cleanup() which do kind of vgpu cleanup gatekeeper.
BRs, Xiaolin
>> }
>>
>> /**
>> @@ -1029,6 +1033,10 @@ static int i915_driver_init_mmio(struct
>> drm_i915_private *dev_priv)
>> return 0;
>>
>> err_uncore:
>> + if (intel_vgpu_active(dev_priv) && dev_priv->vgpu.shared_page) {
>> + free_page((unsigned long)dev_priv->vgpu.shared_page);
>> + dev_priv->vgpu.shared_page = NULL;
> Same again. Remember that shared_page will never be !0 unless active
> anyway.
>
>> + }
>> intel_uncore_fini(dev_priv);
>> err_bridge:
>> pci_dev_put(dev_priv->bridge_dev);
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h
>> b/drivers/gpu/drm/i915/i915_drv.h
>> index d22154a..2c131d4 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -1345,6 +1345,8 @@ struct i915_virtual_gpu {
>> bool active;
>> u32 caps;
>> u32 pv_caps;
>> + spinlock_t shared_page_lock;
>> + struct gvt_shared_page *shared_page;
> Ugh, be considerate of struct packing and/or cachelines.
Thanks your comment, will consider this in next version.
>> };
>>
>> /* used in computing the new watermarks state */
>> diff --git a/drivers/gpu/drm/i915/i915_pvinfo.h
>> b/drivers/gpu/drm/i915/i915_pvinfo.h
>> index 26709e8..179d558 100644
>> --- a/drivers/gpu/drm/i915/i915_pvinfo.h
>> +++ b/drivers/gpu/drm/i915/i915_pvinfo.h
>> @@ -49,6 +49,24 @@ enum vgt_g2v_type {
>> VGT_G2V_MAX,
>> };
>>
>> +struct pv_ppgtt_update {
>> + u64 pdp;
>> + u64 start;
>> + u64 length;
>> + u32 cache_level;
>> +};
> Not used. Only introduce interfaces as they are being used, or just
> before.
it is used in gvt_shared_page structure.
>
>> +/*
>> + * shared page(4KB) between gvt and VM, could be allocated by guest driver
>> + * or a fixed location in PCI bar 0 region
>> + */
>> +struct gvt_shared_page {
>> + u32 elsp_data[4];
>> + u32 reg_addr;
>> + u32 disable_irq;
>> + struct pv_ppgtt_update pv_ppgtt;
>> +};
>> +
>> #define VGPU_PVMMIO(vgpu) vgpu_vreg_t(vgpu, vgtif_reg(enable_pvmmio))
>>
>> /*
>> @@ -121,8 +139,12 @@ struct vgt_if {
>> u32 execlist_context_descriptor_lo;
>> u32 execlist_context_descriptor_hi;
>> u32 enable_pvmmio;
>> + struct {
>> + u32 lo;
>> + u32 hi;
>> + } shared_page_gpa;
>>
>> - u32 rsv7[0x200 - 25]; /* pad to one page */
>> + u32 rsv7[0x200 - 27]; /* pad to one page */
>> } __packed;
>>
>> #define vgtif_reg(x) \
>> diff --git a/drivers/gpu/drm/i915/i915_vgpu.c
>> b/drivers/gpu/drm/i915/i915_vgpu.c
>> index 907bbd2..609eefe 100644
>> --- a/drivers/gpu/drm/i915/i915_vgpu.c
>> +++ b/drivers/gpu/drm/i915/i915_vgpu.c
>> @@ -62,6 +62,7 @@ void i915_check_vgpu(struct drm_i915_private *dev_priv)
>> {
>> u64 magic;
>> u16 version_major;
>> + u64 shared_page_gpa;
> Scope.
can't understand your meaning here, could you explain a little bit more?
BRs, Xiaolin
>
>> BUILD_BUG_ON(sizeof(struct vgt_if) != VGT_PVINFO_SIZE);
>>
>> @@ -91,7 +92,23 @@ void i915_check_vgpu(struct drm_i915_private *dev_priv)
>> dev_priv->vgpu.pv_caps);
>> dev_priv->vgpu.pv_caps = __raw_i915_read32(dev_priv,
>> vgtif_reg(enable_pvmmio));
>> -
>> + if (intel_vgpu_active(dev_priv) && dev_priv->vgpu.pv_caps) {
>> + dev_priv->vgpu.shared_page = (struct gvt_shared_page *)
>> + get_zeroed_page(GFP_KERNEL);
>> + if (!dev_priv->vgpu.shared_page) {
>> + DRM_ERROR("out of memory for shared page memory\n");
>> + return;
>> + }
>> + shared_page_gpa = __pa(dev_priv->vgpu.shared_page);
>> + __raw_i915_write32(dev_priv, vgtif_reg(shared_page_gpa.lo),
>> + lower_32_bits(shared_page_gpa));
>> + __raw_i915_write32(dev_priv, vgtif_reg(shared_page_gpa.hi),
>> + upper_32_bits(shared_page_gpa));
> Shouldn't there be an ack? 12bits there, at least one should be used for
> a valid flag.
> -Chris
>
thanks your comment. will be addressed in next version.
BRs, Xaiolin
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
