Re: [Intel-gfx] ✗ Fi.CI.BAT: failure for vfio/pci: Add support for opregion v2.0+ (rev4)
Hi, Lakshmi: 1. The test failed on version 4 while passed on version 3. The only difference between v3/v4 lies in the format indent issue :“Alignment should match open parenthesis ” 2. My patch only works in VFIO during a guest virtual machine is launched on VT-d gpu passthru. i,e my patch should not be called in common i915 tests. Can u help on this failure ? Thanks in advance. BR/Fred From: Patchwork Sent: Tuesday, March 2, 2021 2:50 PM To: Gao, Fred Cc: intel-gfx@lists.freedesktop.org Subject: ✗ Fi.CI.BAT: failure for vfio/pci: Add support for opregion v2.0+ (rev4) Patch Details Series: vfio/pci: Add support for opregion v2.0+ (rev4) URL: https://patchwork.freedesktop.org/series/84494/ State: failure Details: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19740/index.html CI Bug Log - changes from CI_DRM_9819 -> Patchwork_19740 Summary FAILURE Serious unknown changes coming with Patchwork_19740 absolutely need to be verified manually. If you think the reported changes have nothing to do with the changes introduced in Patchwork_19740, please notify your bug team to allow them to document this new failure mode, which will reduce false positives in CI. External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19740/index.html Possible new issues Here are the unknown changes that may have been introduced in Patchwork_19740: IGT changes Possible regressions * igt@gem_exec_gttfill@basic: * fi-kbl-8809g: PASS<https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9819/fi-kbl-8809g/igt@gem_exec_gttf...@basic.html> -> TIMEOUT<https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19740/fi-kbl-8809g/igt@gem_exec_gttf...@basic.html> +1 similar issue Known issues Here are the changes found in Patchwork_19740 that come from known issues: IGT changes Issues hit * igt@amdgpu/amd_basic@memory-alloc: * fi-cml-u2: NOTRUN -> SKIP<https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19740/fi-cml-u2/igt@amdgpu/amd_ba...@memory-alloc.html> (fdo#109315<https://bugs.freedesktop.org/show_bug.cgi?id=109315>) +17 similar issues * igt@debugfs_test@read_all_entries: * fi-tgl-y: PASS<https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9819/fi-tgl-y/igt@debugfs_test@read_all_entries.html> -> DMESG-WARN<https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19740/fi-tgl-y/igt@debugfs_test@read_all_entries.html> (i915#402<https://gitlab.freedesktop.org/drm/intel/issues/402>) +2 similar issues * igt@gem_exec_fence@basic-busy@bcs0: * fi-cml-u2: NOTRUN -> SKIP<https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19740/fi-cml-u2/igt@gem_exec_fence@basic-b...@bcs0.html> (i915#1208<https://gitlab.freedesktop.org/drm/intel/issues/1208>) +1 similar issue * igt@gem_huc_copy@huc-copy: * fi-cml-u2: NOTRUN -> SKIP<https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19740/fi-cml-u2/igt@gem_huc_c...@huc-copy.html> (i915#2190<https://gitlab.freedesktop.org/drm/intel/issues/2190>) * igt@kms_chamelium@hdmi-hpd-fast: * fi-cml-u2: NOTRUN -> SKIP<https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19740/fi-cml-u2/igt@kms_chamel...@hdmi-hpd-fast.html> (i915#1004<https://gitlab.freedesktop.org/drm/intel/issues/1004>) +2 similar issues * igt@kms_chamelium@vga-edid-read: * fi-cml-u2: NOTRUN -> SKIP<https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19740/fi-cml-u2/igt@kms_chamel...@vga-edid-read.html> (fdo#109309<https://bugs.freedesktop.org/show_bug.cgi?id=109309>) +1 similar issue * igt@kms_force_connector_basic@force-load-detect: * fi-cml-u2: NOTRUN -> SKIP<https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19740/fi-cml-u2/igt@kms_force_connector_ba...@force-load-detect.html> (fdo#109285<https://bugs.freedesktop.org/show_bug.cgi?id=109285>) * igt@kms_pipe_crc_basic@compare-crc-sanitycheck-pipe-d: * fi-cml-u2: NOTRUN -> SKIP<https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19740/fi-cml-u2/igt@kms_pipe_crc_ba...@compare-crc-sanitycheck-pipe-d.html> (fdo#109278<https://bugs.freedesktop.org/show_bug.cgi?id=109278> / i915#533<https://gitlab.freedesktop.org/drm/intel/issues/533>) Possible fixes * igt@gem_linear_blits@basic: * fi-kbl-8809g: TIMEOUT<https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9819/fi-kbl-8809g/igt@gem_linear_bl...@basic.html> (i915#2502<https://gitlab.freedesktop.org/drm/intel/issues/2502>) -> PASS<https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19740/fi-kbl-8809g/igt@gem_linear_bl...@basic.html> * igt@prime_self_import@basic-with_one_bo_two_files: * fi-tgl-y: DMESG-WARN<https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9819/fi-tgl-y/igt@prime_self_import@basic-with_one_bo_two_files.html> (i915#402<https://gitlab.freedesktop.org/drm/intel/issues/402>) -> PASS&l
Re: [Intel-gfx] [PATCH v4] vfio/pci: Add support for opregion v2.1+
Thank you for offering your valuable advice.
Will send the updated version soon.
> -Original Message-
> From: Alex Williamson
> Sent: Saturday, March 20, 2021 3:27 AM
> To: Gao, Fred
> Cc: k...@vger.kernel.org; intel-gfx@lists.freedesktop.org; Zhenyu Wang
> ; Fonn, Swee Yee
> Subject: Re: [PATCH v4] vfio/pci: Add support for opregion v2.1+
>
> On Tue, 2 Mar 2021 21:02:20 +0800
> Fred Gao wrote:
>
> > Before opregion version 2.0 VBT data is stored in opregion mailbox #4,
> > However, When VBT data exceeds 6KB size and cannot be within mailbox
> > #4 starting from opregion v2.0+, Extended VBT region, next to
> > opregion, is used to hold the VBT data, so the total size will be
> > opregion size plus extended VBT region size.
> >
> > since opregion v2.0 with physical host VBT address should not be
> > practically available for end user, it is not supported.
> >
> > Cc: Zhenyu Wang
> > Signed-off-by: Swee Yee Fonn
> > Signed-off-by: Fred Gao
> > ---
> > drivers/vfio/pci/vfio_pci_igd.c | 49
> > +
> > 1 file changed, 49 insertions(+)
> >
> > diff --git a/drivers/vfio/pci/vfio_pci_igd.c
> > b/drivers/vfio/pci/vfio_pci_igd.c index 53d97f459252..4edb8afcdbfc
> > 100644
> > --- a/drivers/vfio/pci/vfio_pci_igd.c
> > +++ b/drivers/vfio/pci/vfio_pci_igd.c
> > @@ -21,6 +21,10 @@
> > #define OPREGION_SIZE (8 * 1024)
> > #define OPREGION_PCI_ADDR 0xfc
> >
> > +#define OPREGION_RVDA 0x3ba
> > +#define OPREGION_RVDS 0x3c2
> > +#define OPREGION_VERSION 0x16
> > +
> > static size_t vfio_pci_igd_rw(struct vfio_pci_device *vdev, char __user
> *buf,
> > size_t count, loff_t *ppos, bool iswrite) { @@ -
> 58,6 +62,7
> > @@ static int vfio_pci_igd_opregion_init(struct vfio_pci_device *vdev)
> > u32 addr, size;
> > void *base;
> > int ret;
> > + u16 version;
> >
> > ret = pci_read_config_dword(vdev->pdev, OPREGION_PCI_ADDR,
> &addr);
> > if (ret)
> > @@ -83,6 +88,50 @@ static int vfio_pci_igd_opregion_init(struct
> > vfio_pci_device *vdev)
> >
> > size *= 1024; /* In KB */
> >
> > + /*
> > +* Support opregion v2.1+
> > +* When VBT data exceeds 6KB size and cannot be within mailbox #4
>
> s/#4/#4, then the/
>
> > +* Extended VBT region, next to opregion, is used to hold the VBT
> data.
> > +* RVDA (Relative Address of VBT Data from Opregion Base) and
> RVDS
> > +* (VBT Data Size) from opregion structure member are used to hold
> the
> > +* address from region base and size of VBT data while RVDA/RVDS
> > +* are not defined before opregion 2.0.
> > +*
> > +* opregion 2.0: rvda is the physical VBT address.
>
> Let's expand the comment to include why this is a problem to support
> (virtualization of this register would be required in userspace) and why we're
> choosing not to manipulate this into a 2.1+ table, which I think is both the
> practical lack of v2.0 tables in use and any implicit dependencies software
> may have on the OpRegion version.
>
> > +*
> > +* opregion 2.1+: rvda is unsigned, relative offset from
> > +* opregion base, and should never point within opregion.
>
> And for our purposes must exactly follow the base opregion to avoid
> exposing unknown host memory to userspace, ie. provide a more descriptive
> justification for the 2nd error condition below.
>
> > +*/
> > + version = le16_to_cpu(*(__le16 *)(base + OPREGION_VERSION));
> > + if (version >= 0x0200) {
> > + u64 rvda;
> > + u32 rvds;
> > +
> > + rvda = le64_to_cpu(*(__le64 *)(base + OPREGION_RVDA));
> > + rvds = le32_to_cpu(*(__le32 *)(base + OPREGION_RVDS));
> > + if (rvda && rvds) {
> > + /* no support for opregion v2.0 with physical VBT
> address */
> > + if (version == 0x0200) {
> > + memunmap(base);
> > + pci_err(vdev->pdev,
> > + "IGD passthrough does not support
> opregion\n"
> > + "version 0x%x with physical rvda
> 0x%llx\n", version, rvda);
>
>
> Why do we need a new line midway through this log message?
>
> s/passthrough/assignment/
>
> In testing the version you include the leading zero, do you also want that
> leading zer
Re: [Intel-gfx] ✗ Fi.CI.DOCS: warning for vfio/pci: Add support for opregion v2.0+ (rev5)
Hi, Lakshmi: Can u help on this failure again, the only difference between version 5& 4 is the comments. Thanks in advance. > -Original Message- > From: Patchwork > Sent: Friday, March 26, 2021 5:05 AM > To: Gao, Fred > Cc: intel-gfx@lists.freedesktop.org > Subject: ✗ Fi.CI.DOCS: warning for vfio/pci: Add support for opregion v2.0+ > (rev5) > > == Series Details == > > Series: vfio/pci: Add support for opregion v2.0+ (rev5) > URL : https://patchwork.freedesktop.org/series/84494/ > State : warning > > == Summary == > > $ make htmldocs 2>&1 > /dev/null | grep i915 > ./drivers/gpu/drm/i915/gem/i915_gem_shrinker.c:102: warning: Function > parameter or member 'ww' not described in 'i915_gem_shrink' > ./drivers/gpu/drm/i915/i915_cmd_parser.c:1420: warning: Excess function > parameter 'trampoline' description in 'intel_engine_cmd_parser' > ./drivers/gpu/drm/i915/i915_cmd_parser.c:1420: warning: Function > parameter or member 'jump_whitelist' not described in > 'intel_engine_cmd_parser' > ./drivers/gpu/drm/i915/i915_cmd_parser.c:1420: warning: Function > parameter or member 'shadow_map' not described in > 'intel_engine_cmd_parser' > ./drivers/gpu/drm/i915/i915_cmd_parser.c:1420: warning: Function > parameter or member 'batch_map' not described in > 'intel_engine_cmd_parser' > ./drivers/gpu/drm/i915/i915_cmd_parser.c:1420: warning: Excess function > parameter 'trampoline' description in 'intel_engine_cmd_parser' > /home/cidrm/kernel/Documentation/gpu/i915:22: ./drivers/gpu/drm/i915/i > ntel_runtime_pm.c:423: WARNING: Inline strong start-string without end- > string. > ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] ✗ Fi.CI.DOCS: warning for vfio/pci: Add support for opregion v2.0+ (rev5)
Thanks for the quick help. Test of Fi.CI.DOCS is still failed although we do not change any code in i915. > -Original Message- > From: Vudum, Lakshminarayana > Sent: Tuesday, March 30, 2021 12:20 AM > To: Gao, Fred ; intel-gfx@lists.freedesktop.org > Cc: Wang, Zhenyu Z > Subject: RE: ✗ Fi.CI.DOCS: warning for vfio/pci: Add support for opregion > v2.0+ (rev5) > > Failure is reported. It's now showing as success. > > -----Original Message- > From: Gao, Fred > Sent: Monday, March 29, 2021 12:12 AM > To: intel-gfx@lists.freedesktop.org; Vudum, Lakshminarayana > > Cc: Wang, Zhenyu Z > Subject: RE: ✗ Fi.CI.DOCS: warning for vfio/pci: Add support for opregion > v2.0+ (rev5) > > Hi, Lakshmi: > > Can u help on this failure again, > the only difference between version 5& 4 is the comments. > Thanks in advance. > > > -Original Message- > > From: Patchwork > > Sent: Friday, March 26, 2021 5:05 AM > > To: Gao, Fred > > Cc: intel-gfx@lists.freedesktop.org > > Subject: ✗ Fi.CI.DOCS: warning for vfio/pci: Add support for opregion > > v2.0+ > > (rev5) > > > > == Series Details == > > > > Series: vfio/pci: Add support for opregion v2.0+ (rev5) > > URL : https://patchwork.freedesktop.org/series/84494/ > > State : warning > > > > == Summary == > > > > $ make htmldocs 2>&1 > /dev/null | grep i915 > > ./drivers/gpu/drm/i915/gem/i915_gem_shrinker.c:102: warning: Function > > parameter or member 'ww' not described in 'i915_gem_shrink' > > ./drivers/gpu/drm/i915/i915_cmd_parser.c:1420: warning: Excess > > function parameter 'trampoline' description in 'intel_engine_cmd_parser' > > ./drivers/gpu/drm/i915/i915_cmd_parser.c:1420: warning: Function > > parameter or member 'jump_whitelist' not described in > > 'intel_engine_cmd_parser' > > ./drivers/gpu/drm/i915/i915_cmd_parser.c:1420: warning: Function > > parameter or member 'shadow_map' not described in > > 'intel_engine_cmd_parser' > > ./drivers/gpu/drm/i915/i915_cmd_parser.c:1420: warning: Function > > parameter or member 'batch_map' not described in > > 'intel_engine_cmd_parser' > > ./drivers/gpu/drm/i915/i915_cmd_parser.c:1420: warning: Excess > > function parameter 'trampoline' description in 'intel_engine_cmd_parser' > > > /home/cidrm/kernel/Documentation/gpu/i915:22: ./drivers/gpu/drm/i915/i > > ntel_runtime_pm.c:423: WARNING: Inline strong start-string without > > end- string. > > > > ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v1] vfio/pci: Add support for opregion v2.0+
Thanks Alex for the timely review.
> -Original Message-
> From: Alex Williamson
> Sent: Thursday, December 3, 2020 2:57 AM
> To: Gao, Fred
> Cc: k...@vger.kernel.org; intel-gfx@lists.freedesktop.org; Zhenyu Wang
> ; Fonn, Swee Yee
> Subject: Re: [PATCH v1] vfio/pci: Add support for opregion v2.0+
>
> On Thu, 3 Dec 2020 01:12:49 +0800
> Fred Gao wrote:
>
> > When VBT data exceeds 6KB size and cannot be within mailbox #4
> > starting from opregion v2.0+, Extended VBT region, next to opregion,
> > is used to hold the VBT data, so the total size will be opregion size
> > plus extended VBT region size.
> >
> > For opregion 2.1+: since rvda is relative offset from opregion base,
> > rvda as extended VBT start offset should be same as opregion size.
> >
> > For opregion 2.0: the only difference between opregion 2.0 and 2.1 is
> > rvda addressing mode besides the version. since rvda is physical host
> > VBT address and cannot be directly used in guest, it is faked into
> > opregion 2.1's relative offset.
> >
> > Cc: Zhenyu Wang
> > Signed-off-by: Swee Yee Fonn
> > Signed-off-by: Fred Gao
> > ---
> > drivers/vfio/pci/vfio_pci_igd.c | 44
> > +
> > 1 file changed, 44 insertions(+)
> >
> > diff --git a/drivers/vfio/pci/vfio_pci_igd.c
> > b/drivers/vfio/pci/vfio_pci_igd.c index 53d97f459252..78919a289914
> > 100644
> > --- a/drivers/vfio/pci/vfio_pci_igd.c
> > +++ b/drivers/vfio/pci/vfio_pci_igd.c
> > @@ -21,6 +21,17 @@
> > #define OPREGION_SIZE (8 * 1024)
> > #define OPREGION_PCI_ADDR 0xfc
> >
> > +/*
> > + * opregion 2.0: rvda is the physical VBT address.
>
> What's rvda? What's VBT?
Rvda is a struct member in opregion mailbox 3 ,
same definition in i915's struct opregion_asle.
I,e Physical address of raw VBT data (v2.0) or
Relative address from opregion (v2.1).
VBT: video bios table ,
the data is stored in opregion mailbox 4 before opregion v2.0.
After opregion v2.0+ , VBT data is larger than mailbox 4,
so Extended VBT region, next to opregion is used to hold the data.
> > + *
> > + * opregion 2.1+: rvda is unsigned, relative offset from
> > + * opregion base, and should never point within opregion.
> > + */
> > +#define OPREGION_RDVA 0x3ba
> > +#define OPREGION_RDVS 0x3c2
> > +#define OPREGION_VERSION 22
>
> Why is this specified as decimal and the others in hex? This makes it seem
> like the actual version rather than the offset of a version register.
Yes, it is an offset, will redefine the opregion version offset in hex.
> > +
> > +
> > static size_t vfio_pci_igd_rw(struct vfio_pci_device *vdev, char __user
> *buf,
> > size_t count, loff_t *ppos, bool iswrite) { @@ -
> 58,6 +69,7
> > @@ static int vfio_pci_igd_opregion_init(struct vfio_pci_device *vdev)
> > u32 addr, size;
> > void *base;
> > int ret;
> > + u16 version;
> >
> > ret = pci_read_config_dword(vdev->pdev, OPREGION_PCI_ADDR,
> &addr);
> > if (ret)
> > @@ -83,6 +95,38 @@ static int vfio_pci_igd_opregion_init(struct
> > vfio_pci_device *vdev)
> >
> > size *= 1024; /* In KB */
> >
> > + /* Support opregion v2.0+ */
> > + version = le16_to_cpu(*(__le16 *)(base + OPREGION_VERSION));
> > + if (version >= 0x0200) {
> > + u64 rvda;
> > + u32 rvds;
> > +
> > + rvda = le64_to_cpu(*(__le64 *)(base + OPREGION_RDVA));
> > + rvds = le32_to_cpu(*(__le32 *)(base + OPREGION_RDVS));
> > + if (rvda && rvds) {
> > + u32 offset;
> > +
> > + if (version == 0x0200)
> > + offset = (rvda - (u64)addr);
>
> Unnecessary outer ()
Thx, will remove in new patch.
> > + else
> > + offset = rvda;
> > +
> > + pci_WARN(vdev->pdev, offset != size,
> > + "Extended VBT does not follow opregion !\n"
> > + "opregion version 0x%x:offset 0x%x\n",
> version, offset);
> > +
> > + if (version == 0x0200) {
> > + /* opregion version v2.0 faked to v2.1 */
> > + *(__le16 *)(base + OPREGION_VERSION) =
> > + cpu_to_le16(0x0201);
> > + /* rvda faked to rela
