From: Alex Williamson <alex.williamson@redhat.com>
To: Fred Gao <fred.gao@intel.com>
Cc: Swee Yee Fonn <swee.yee.fonn@intel.com>,
intel-gfx@lists.freedesktop.org, kvm@vger.kernel.org
Subject: Re: [Intel-gfx] [PATCH v4] vfio/pci: Add support for opregion v2.1+
Date: Fri, 19 Mar 2021 13:26:34 -0600 [thread overview]
Message-ID: <20210319132634.5af398b9@omen.home.shazbot.org> (raw)
In-Reply-To: <20210302130220.9349-1-fred.gao@intel.com>
On Tue, 2 Mar 2021 21:02:20 +0800
Fred Gao <fred.gao@intel.com> 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 <zhenyuw@linux.intel.com>
> Signed-off-by: Swee Yee Fonn <swee.yee.fonn@intel.com>
> Signed-off-by: Fred Gao <fred.gao@intel.com>
> ---
> 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 zero in the printed version, ie. %04x?
If we get to this code, we already know that both rvda and rvds are
non-zero, why is it useful to print the rvda value in this error
message? For example, we could print:
"IGD assignment does not support opregion version 0x%04x with an extended VBT region"
> + return -EINVAL;
> + }
> +
> + if ((u32)rvda != size) {
What allows us to assume rvda is a 32bit value given that it's a 64bit
register? It seems safer not to include this cast.
> + memunmap(base);
> + pci_err(vdev->pdev,
> + "Extended VBT does not follow opregion !\n"
> + "opregion version 0x%x:rvda 0x%llx\n", version, rvda);
Again I'm not sure about the usefulness of printing the rvda value on
its own. Without knowing the size value it seems meaningless. Like
above, get rid of the mid-error new line and random space if you keep
the exclamation point.
> + return -EINVAL;
> + }
> +
> + /* region size for opregion v2.0+: opregion and VBT size */
> + size += rvds;
RVDS is defined as size in bytes, not in kilobytes like the base
opregion size, right? Let's include that clarification in the comment
since the spec is private. Thanks,
Alex
> + }
> + }
> +
> if (size != OPREGION_SIZE) {
> memunmap(base);
> base = memremap(addr, size, MEMREMAP_WB);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2021-03-19 19:26 UTC|newest]
Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-12-02 17:12 [Intel-gfx] [PATCH v1] vfio/pci: Add support for opregion v2.0+ Fred Gao
2020-12-02 10:04 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for " Patchwork
2020-12-02 10:35 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork
2020-12-02 12:06 ` [Intel-gfx] ✓ Fi.CI.IGT: " Patchwork
2020-12-02 18:57 ` [Intel-gfx] [PATCH v1] " Alex Williamson
2020-12-03 9:21 ` Gao, Fred
2020-12-03 23:38 ` Alex Williamson
2021-01-18 5:15 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for vfio/pci: Add support for opregion v2.0+ (rev2) Patchwork
2021-01-18 5:46 ` [Intel-gfx] ✗ Fi.CI.BAT: failure " Patchwork
2021-01-18 12:38 ` [Intel-gfx] [PATCH v2] vfio/pci: Add support for opregion v2.0+ Fred Gao
2021-01-21 20:33 ` Alex Williamson
2021-02-02 5:09 ` Zhenyu Wang
2021-02-08 17:02 ` [Intel-gfx] [PATCH v3] vfio/pci: Add support for opregion v2.1+ Fred Gao
2021-03-02 13:02 ` [Intel-gfx] [PATCH v4] " Fred Gao
2021-03-19 19:26 ` Alex Williamson [this message]
2021-03-25 8:50 ` Gao, Fred
2021-03-25 17:09 ` [Intel-gfx] [PATCH v5] " Fred Gao
2021-03-30 9:08 ` Zhenyu Wang
2021-04-06 19:37 ` Alex Williamson
2021-02-08 9:22 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for vfio/pci: Add support for opregion v2.0+ (rev3) Patchwork
2021-02-08 9:52 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork
2021-03-02 6:49 ` [Intel-gfx] ✗ Fi.CI.BAT: failure for vfio/pci: Add support for opregion v2.0+ (rev4) Patchwork
2021-03-02 12:47 ` Gao, Fred
2021-03-02 6:55 ` [Intel-gfx] ✓ Fi.CI.IGT: success " Patchwork
2021-03-02 17:09 ` [Intel-gfx] ✓ Fi.CI.BAT: " Patchwork
2021-03-25 21:05 ` [Intel-gfx] ✗ Fi.CI.DOCS: warning for vfio/pci: Add support for opregion v2.0+ (rev5) Patchwork
2021-03-29 7:12 ` Gao, Fred
2021-03-29 7:12 ` Zhenyu Wang
2021-03-29 16:19 ` Vudum, Lakshminarayana
2021-03-29 16:22 ` Gao, Fred
2021-03-29 16:35 ` Vudum, Lakshminarayana
2021-03-25 21:31 ` [Intel-gfx] ✗ Fi.CI.BAT: failure " Patchwork
2021-03-29 15:51 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork
2021-03-29 18:35 ` [Intel-gfx] ✓ Fi.CI.IGT: " Patchwork
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210319132634.5af398b9@omen.home.shazbot.org \
--to=alex.williamson@redhat.com \
--cc=fred.gao@intel.com \
--cc=intel-gfx@lists.freedesktop.org \
--cc=kvm@vger.kernel.org \
--cc=swee.yee.fonn@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).