* [PATCH v1] vfio/pci: Add support for opregion v2.0+ @ 2020-12-02 17:12 Fred Gao 2020-12-02 18:57 ` Alex Williamson 2021-01-18 12:38 ` [PATCH v2] " Fred Gao 0 siblings, 2 replies; 14+ messages in thread From: Fred Gao @ 2020-12-02 17:12 UTC (permalink / raw) To: kvm, intel-gfx; +Cc: Fred Gao, Zhenyu Wang, Swee Yee Fonn 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 <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 | 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. + * + * 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 + + 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); + 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 relative offset */ + (*(__le64 *)(base + OPREGION_RDVA)) = + cpu_to_le64((rvda - (u64)addr)); + } + size = offset + rvds; + } + } + if (size != OPREGION_SIZE) { memunmap(base); base = memremap(addr, size, MEMREMAP_WB); -- 2.24.1.1.gb6d4d82bd5 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v1] vfio/pci: Add support for opregion v2.0+ 2020-12-02 17:12 [PATCH v1] vfio/pci: Add support for opregion v2.0+ Fred Gao @ 2020-12-02 18:57 ` Alex Williamson 2020-12-03 9:21 ` Gao, Fred 2021-01-18 12:38 ` [PATCH v2] " Fred Gao 1 sibling, 1 reply; 14+ messages in thread From: Alex Williamson @ 2020-12-02 18:57 UTC (permalink / raw) To: Fred Gao; +Cc: kvm, intel-gfx, Zhenyu Wang, Swee Yee Fonn On Thu, 3 Dec 2020 01:12:49 +0800 Fred Gao <fred.gao@intel.com> 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 <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 | 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? > + * > + * 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. > + > + > 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 () > + 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 relative offset */ > + (*(__le64 *)(base + OPREGION_RDVA)) = > + cpu_to_le64((rvda - (u64)addr)); We're writing to the OpRegion and affecting all future use of it, seems dangerous. > + } > + size = offset + rvds; We warn about VBT (whatever that is) not immediately following the OpRegion, but then we go ahead and size the thing that we expose to userspace to allow read access to everything between the OpRegion and VBT?? > + } > + } > + > if (size != OPREGION_SIZE) { > memunmap(base); > base = memremap(addr, size, MEMREMAP_WB); ^ permalink raw reply [flat|nested] 14+ messages in thread
* RE: [PATCH v1] vfio/pci: Add support for opregion v2.0+ 2020-12-02 18:57 ` Alex Williamson @ 2020-12-03 9:21 ` Gao, Fred 2020-12-03 23:38 ` Alex Williamson 0 siblings, 1 reply; 14+ messages in thread From: Gao, Fred @ 2020-12-03 9:21 UTC (permalink / raw) To: Alex Williamson; +Cc: kvm, intel-gfx, Zhenyu Wang, Fonn, Swee Yee Thanks Alex for the timely review. > -----Original Message----- > From: Alex Williamson <alex.williamson@redhat.com> > Sent: Thursday, December 3, 2020 2:57 AM > To: Gao, Fred <fred.gao@intel.com> > Cc: kvm@vger.kernel.org; intel-gfx@lists.freedesktop.org; Zhenyu Wang > <zhenyuw@linux.intel.com>; Fonn, Swee Yee <swee.yee.fonn@intel.com> > Subject: Re: [PATCH v1] vfio/pci: Add support for opregion v2.0+ > > On Thu, 3 Dec 2020 01:12:49 +0800 > Fred Gao <fred.gao@intel.com> 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 <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 | 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 relative offset */ > > + (*(__le64 *)(base + OPREGION_RDVA)) = > > + cpu_to_le64((rvda - (u64)addr)); > > We're writing to the OpRegion and affecting all future use of it, seems > dangerous. from the opregion v2.0+ specification since there is only RVDA difference between opregion v2.0 and v2.1 besides the version It is the simplest solution and should not impact the future use. > > + } > > + size = offset + rvds; > > > We warn about VBT (whatever that is) not immediately following the > OpRegion, but then we go ahead and size the thing that we expose to > userspace to allow read access to everything between the OpRegion and > VBT?? From the specification , there should no hole between opregion and VBT. But I am not sure if some vendor BIOS will make the hole. Can we return the error if this abnormal thing happens ? > > + } > > + } > > + > > if (size != OPREGION_SIZE) { > > memunmap(base); > > base = memremap(addr, size, MEMREMAP_WB); ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v1] vfio/pci: Add support for opregion v2.0+ 2020-12-03 9:21 ` Gao, Fred @ 2020-12-03 23:38 ` Alex Williamson 0 siblings, 0 replies; 14+ messages in thread From: Alex Williamson @ 2020-12-03 23:38 UTC (permalink / raw) To: Gao, Fred; +Cc: kvm, intel-gfx, Zhenyu Wang, Fonn, Swee Yee On Thu, 3 Dec 2020 09:21:03 +0000 "Gao, Fred" <fred.gao@intel.com> wrote: > Thanks Alex for the timely review. > > > -----Original Message----- > > From: Alex Williamson <alex.williamson@redhat.com> > > Sent: Thursday, December 3, 2020 2:57 AM > > To: Gao, Fred <fred.gao@intel.com> > > Cc: kvm@vger.kernel.org; intel-gfx@lists.freedesktop.org; Zhenyu Wang > > <zhenyuw@linux.intel.com>; Fonn, Swee Yee <swee.yee.fonn@intel.com> > > Subject: Re: [PATCH v1] vfio/pci: Add support for opregion v2.0+ > > > > On Thu, 3 Dec 2020 01:12:49 +0800 > > Fred Gao <fred.gao@intel.com> 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 <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 | 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. Are these published anywhere? I can only find revision 1.0 available. > > > + * > > > + * 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 relative offset */ > > > + (*(__le64 *)(base + OPREGION_RDVA)) = > > > + cpu_to_le64((rvda - (u64)addr)); > > > > We're writing to the OpRegion and affecting all future use of it, seems > > dangerous. > > from the opregion v2.0+ specification > since there is only RVDA difference between opregion v2.0 and v2.1 besides the version > It is the simplest solution and should not impact the future use. *Should* not, but I'm not so confident without a spec to reference. > > > + } > > > + size = offset + rvds; > > > > > > We warn about VBT (whatever that is) not immediately following the > > OpRegion, but then we go ahead and size the thing that we expose to > > userspace to allow read access to everything between the OpRegion and > > VBT?? > From the specification , there should no hole between opregion and VBT. > But I am not sure if some vendor BIOS will make the hole. > Can we return the error if this abnormal thing happens ? It seems rather dangerous to allow a user to have read access to an unknown extent of unknown data... right? Thanks, Alex > > > + } > > > + } > > > + > > > if (size != OPREGION_SIZE) { > > > memunmap(base); > > > base = memremap(addr, size, MEMREMAP_WB); > ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2] vfio/pci: Add support for opregion v2.0+ 2020-12-02 17:12 [PATCH v1] vfio/pci: Add support for opregion v2.0+ Fred Gao 2020-12-02 18:57 ` Alex Williamson @ 2021-01-18 12:38 ` Fred Gao 2021-01-21 20:33 ` Alex Williamson 2021-02-08 17:02 ` [PATCH v3] vfio/pci: Add support for opregion v2.1+ Fred Gao 1 sibling, 2 replies; 14+ messages in thread From: Fred Gao @ 2021-01-18 12:38 UTC (permalink / raw) To: kvm, intel-gfx; +Cc: Fred Gao, Zhenyu Wang, Swee Yee Fonn 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. 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 | 59 +++++++++++++++++++++++++++++++++ 1 file changed, 59 insertions(+) diff --git a/drivers/vfio/pci/vfio_pci_igd.c b/drivers/vfio/pci/vfio_pci_igd.c index 53d97f459252..fc470278a492 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,60 @@ static int vfio_pci_igd_opregion_init(struct vfio_pci_device *vdev) size *= 1024; /* In KB */ + /* + * Support opregion v2.0+ + * When VBT data exceeds 6KB size and cannot be within mailbox #4 + * 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. + * + * opregion 2.1+: rvda is unsigned, relative offset from + * opregion base, and should never point within opregion. + */ + 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) { + u32 offset; + + if (version == 0x0200) + offset = rvda - (u64)addr; + else + offset = rvda; + + if (offset != size) { + pci_err(vdev->pdev, + "Extended VBT does not follow opregion !\n" + "opregion version 0x%x:offset 0x%x\n", version, offset); + return -EINVAL; + } + + /* + * the only difference between opregion 2.0 and 2.1 is + * rvda addressing mode. since rvda is physical host + * VBT address and cannot be directly used in guest, + * faked into opregion 2.1's relative offset. + */ + if (version == 0x0200) { + *(__le16 *)(base + OPREGION_VERSION) = + cpu_to_le16(0x0201); + (*(__le64 *)(base + OPREGION_RVDA)) = + cpu_to_le64((rvda - (u64)addr)); + } + + /* region size for opregion v2.0+: opregion and VBT size */ + size = offset + rvds; + } + } + if (size != OPREGION_SIZE) { memunmap(base); base = memremap(addr, size, MEMREMAP_WB); -- 2.24.1.1.gb6d4d82bd5 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v2] vfio/pci: Add support for opregion v2.0+ 2021-01-18 12:38 ` [PATCH v2] " Fred Gao @ 2021-01-21 20:33 ` Alex Williamson 2021-02-02 5:09 ` Zhenyu Wang 2021-02-08 17:02 ` [PATCH v3] vfio/pci: Add support for opregion v2.1+ Fred Gao 1 sibling, 1 reply; 14+ messages in thread From: Alex Williamson @ 2021-01-21 20:33 UTC (permalink / raw) To: Fred Gao; +Cc: kvm, intel-gfx, Zhenyu Wang, Swee Yee Fonn On Mon, 18 Jan 2021 20:38:34 +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. > > 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 | 59 +++++++++++++++++++++++++++++++++ > 1 file changed, 59 insertions(+) > > diff --git a/drivers/vfio/pci/vfio_pci_igd.c b/drivers/vfio/pci/vfio_pci_igd.c > index 53d97f459252..fc470278a492 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,60 @@ static int vfio_pci_igd_opregion_init(struct vfio_pci_device *vdev) > > size *= 1024; /* In KB */ > > + /* > + * Support opregion v2.0+ > + * When VBT data exceeds 6KB size and cannot be within mailbox #4 > + * 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. > + * > + * opregion 2.1+: rvda is unsigned, relative offset from > + * opregion base, and should never point within opregion. > + */ > + 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) { > + u32 offset; > + > + if (version == 0x0200) > + offset = rvda - (u64)addr; > + else > + offset = rvda; > + > + if (offset != size) { > + pci_err(vdev->pdev, > + "Extended VBT does not follow opregion !\n" > + "opregion version 0x%x:offset 0x%x\n", version, offset); > + return -EINVAL; > + } > + > + /* > + * the only difference between opregion 2.0 and 2.1 is > + * rvda addressing mode. since rvda is physical host > + * VBT address and cannot be directly used in guest, > + * faked into opregion 2.1's relative offset. > + */ > + if (version == 0x0200) { > + *(__le16 *)(base + OPREGION_VERSION) = > + cpu_to_le16(0x0201); > + (*(__le64 *)(base + OPREGION_RVDA)) = > + cpu_to_le64((rvda - (u64)addr)); > + } There's a much better description of the fields and logic here, thanks for that. I also see we've closed the gap to require the extended region to immediately follow the opregion table. The code immediately above still makes me nervous as even if this is the only difference between the specs, code might make some differentiation based on the spec version, which we're changing in host memory for all subsequent drivers until the host is rebooted. Could we use a pci_dbg() in this branch to flag that event in dmesg for debugging? Thanks, Alex > + > + /* region size for opregion v2.0+: opregion and VBT size */ > + size = offset + rvds; > + } > + } > + > if (size != OPREGION_SIZE) { > memunmap(base); > base = memremap(addr, size, MEMREMAP_WB); ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] vfio/pci: Add support for opregion v2.0+ 2021-01-21 20:33 ` Alex Williamson @ 2021-02-02 5:09 ` Zhenyu Wang 0 siblings, 0 replies; 14+ messages in thread From: Zhenyu Wang @ 2021-02-02 5:09 UTC (permalink / raw) To: Alex Williamson; +Cc: Fred Gao, kvm, intel-gfx, Swee Yee Fonn [-- Attachment #1: Type: text/plain, Size: 4739 bytes --] On 2021.01.21 13:33:18 -0700, Alex Williamson wrote: > On Mon, 18 Jan 2021 20:38:34 +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. > > > > 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 | 59 +++++++++++++++++++++++++++++++++ > > 1 file changed, 59 insertions(+) > > > > diff --git a/drivers/vfio/pci/vfio_pci_igd.c b/drivers/vfio/pci/vfio_pci_igd.c > > index 53d97f459252..fc470278a492 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,60 @@ static int vfio_pci_igd_opregion_init(struct vfio_pci_device *vdev) > > > > size *= 1024; /* In KB */ > > > > + /* > > + * Support opregion v2.0+ > > + * When VBT data exceeds 6KB size and cannot be within mailbox #4 > > + * 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. > > + * > > + * opregion 2.1+: rvda is unsigned, relative offset from > > + * opregion base, and should never point within opregion. > > + */ > > + 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) { > > + u32 offset; > > + > > + if (version == 0x0200) > > + offset = rvda - (u64)addr; > > + else > > + offset = rvda; > > + > > + if (offset != size) { > > + pci_err(vdev->pdev, > > + "Extended VBT does not follow opregion !\n" > > + "opregion version 0x%x:offset 0x%x\n", version, offset); > > + return -EINVAL; > > + } > > + > > + /* > > + * the only difference between opregion 2.0 and 2.1 is > > + * rvda addressing mode. since rvda is physical host > > + * VBT address and cannot be directly used in guest, > > + * faked into opregion 2.1's relative offset. > > + */ > > + if (version == 0x0200) { > > + *(__le16 *)(base + OPREGION_VERSION) = > > + cpu_to_le16(0x0201); > > + (*(__le64 *)(base + OPREGION_RVDA)) = > > + cpu_to_le64((rvda - (u64)addr)); > > + } > > There's a much better description of the fields and logic here, thanks > for that. I also see we've closed the gap to require the extended > region to immediately follow the opregion table. The code > immediately above still makes me nervous as even if this is the only > difference between the specs, code might make some differentiation > based on the spec version, which we're changing in host memory for all > subsequent drivers until the host is rebooted. Could we use a pci_dbg() > in this branch to flag that event in dmesg for debugging? Thanks, > Alex, that's really valid concern, even we thought it should be no problem, we asked firmware team again, it looks for opregion 2.0 with VBT >6k case (RVDA != 0) is not practically available for end user. So I think we may just not support for that. For opregion 2.1+, just extend the size properly. Thanks > > > + > > + /* region size for opregion v2.0+: opregion and VBT size */ > > + size = offset + rvds; > > + } > > + } > > + > > if (size != OPREGION_SIZE) { > > memunmap(base); > > base = memremap(addr, size, MEMREMAP_WB); > [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 195 bytes --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v3] vfio/pci: Add support for opregion v2.1+ 2021-01-18 12:38 ` [PATCH v2] " Fred Gao 2021-01-21 20:33 ` Alex Williamson @ 2021-02-08 17:02 ` Fred Gao 2021-03-02 13:02 ` [PATCH v4] " Fred Gao 1 sibling, 1 reply; 14+ messages in thread From: Fred Gao @ 2021-02-08 17:02 UTC (permalink / raw) To: kvm, intel-gfx; +Cc: Fred Gao, Zhenyu Wang, Swee Yee Fonn 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..d93a855227e7 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 + * 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. + * + * opregion 2.1+: rvda is unsigned, relative offset from + * opregion base, and should never point within opregion. + */ + 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\n" + "opregion version 0x%x with physical rvda 0x%llx\n", version, rvda); + return -EINVAL; + } + + if ((u32)rvda != size) { + memunmap(base); + pci_err(vdev->pdev, + "Extended VBT does not follow opregion !\n" + "opregion version 0x%x:rvda 0x%llx\n", version, rvda); + return -EINVAL; + } + + /* region size for opregion v2.0+: opregion and VBT size */ + size += rvds; + } + } + if (size != OPREGION_SIZE) { memunmap(base); base = memremap(addr, size, MEMREMAP_WB); -- 2.24.1.1.gb6d4d82bd5 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v4] vfio/pci: Add support for opregion v2.1+ 2021-02-08 17:02 ` [PATCH v3] vfio/pci: Add support for opregion v2.1+ Fred Gao @ 2021-03-02 13:02 ` Fred Gao 2021-03-19 19:26 ` Alex Williamson 2021-03-25 17:09 ` [PATCH v5] " Fred Gao 0 siblings, 2 replies; 14+ messages in thread From: Fred Gao @ 2021-03-02 13:02 UTC (permalink / raw) To: kvm, intel-gfx; +Cc: Fred Gao, Zhenyu Wang, Swee Yee Fonn 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 + * 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. + * + * opregion 2.1+: rvda is unsigned, relative offset from + * opregion base, and should never point within opregion. + */ + 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); + return -EINVAL; + } + + if ((u32)rvda != size) { + memunmap(base); + pci_err(vdev->pdev, + "Extended VBT does not follow opregion !\n" + "opregion version 0x%x:rvda 0x%llx\n", version, rvda); + return -EINVAL; + } + + /* region size for opregion v2.0+: opregion and VBT size */ + size += rvds; + } + } + if (size != OPREGION_SIZE) { memunmap(base); base = memremap(addr, size, MEMREMAP_WB); -- 2.24.1.1.gb6d4d82bd5 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v4] vfio/pci: Add support for opregion v2.1+ 2021-03-02 13:02 ` [PATCH v4] " Fred Gao @ 2021-03-19 19:26 ` Alex Williamson 2021-03-25 8:50 ` Gao, Fred 2021-03-25 17:09 ` [PATCH v5] " Fred Gao 1 sibling, 1 reply; 14+ messages in thread From: Alex Williamson @ 2021-03-19 19:26 UTC (permalink / raw) To: Fred Gao; +Cc: kvm, intel-gfx, Zhenyu Wang, Swee Yee Fonn 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); ^ permalink raw reply [flat|nested] 14+ messages in thread
* RE: [PATCH v4] vfio/pci: Add support for opregion v2.1+ 2021-03-19 19:26 ` Alex Williamson @ 2021-03-25 8:50 ` Gao, Fred 0 siblings, 0 replies; 14+ messages in thread From: Gao, Fred @ 2021-03-25 8:50 UTC (permalink / raw) To: Alex Williamson; +Cc: kvm, intel-gfx, Zhenyu Wang, Fonn, Swee Yee Thank you for offering your valuable advice. Will send the updated version soon. > -----Original Message----- > From: Alex Williamson <alex.williamson@redhat.com> > Sent: Saturday, March 20, 2021 3:27 AM > To: Gao, Fred <fred.gao@intel.com> > Cc: kvm@vger.kernel.org; intel-gfx@lists.freedesktop.org; Zhenyu Wang > <zhenyuw@linux.intel.com>; Fonn, Swee Yee <swee.yee.fonn@intel.com> > Subject: Re: [PATCH v4] vfio/pci: Add support for opregion v2.1+ > > 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); ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v5] vfio/pci: Add support for opregion v2.1+ 2021-03-02 13:02 ` [PATCH v4] " Fred Gao 2021-03-19 19:26 ` Alex Williamson @ 2021-03-25 17:09 ` Fred Gao 2021-03-30 9:08 ` Zhenyu Wang 2021-04-06 19:37 ` Alex Williamson 1 sibling, 2 replies; 14+ messages in thread From: Fred Gao @ 2021-03-25 17:09 UTC (permalink / raw) To: kvm, intel-gfx; +Cc: Fred Gao, Zhenyu Wang, Swee Yee Fonn Before opregion version 2.0 VBT data is stored in opregion mailbox #4, but when VBT data exceeds 6KB size and cannot be within mailbox #4 then 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 would not be practically available for end user and guest can not directly access host physical address, so 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 | 53 +++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/drivers/vfio/pci/vfio_pci_igd.c b/drivers/vfio/pci/vfio_pci_igd.c index e66dfb0178ed..228df565e9bc 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,54 @@ 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, 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 + * (Raw VBT Data Size) from opregion structure member are used to hold the + * address from region base and size of VBT data. RVDA/RVDS are not + * defined before opregion 2.0. + * + * opregion 2.1+: RVDA is unsigned, relative offset from + * opregion base, and should point to the end of opregion. + * otherwise, exposing to userspace to allow read access to everything between + * the OpRegion and VBT is not safe. + * RVDS is defined as size in bytes. + * + * opregion 2.0: rvda is the physical VBT address. + * Since rvda is HPA it cannot be directly used in guest. + * And it should not be practically available for end user,so it is not supported. + */ + 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 assignment does not support opregion v2.0 with an extended VBT region\n"); + return -EINVAL; + } + + if (rvda != size) { + memunmap(base); + pci_err(vdev->pdev, + "Extended VBT does not follow opregion on version 0x%04x\n", + version); + return -EINVAL; + } + + /* region size for opregion v2.0+: opregion and VBT size. */ + size += rvds; + } + } + if (size != OPREGION_SIZE) { memunmap(base); base = memremap(addr, size, MEMREMAP_WB); -- 2.24.1.1.gb6d4d82bd5 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v5] vfio/pci: Add support for opregion v2.1+ 2021-03-25 17:09 ` [PATCH v5] " Fred Gao @ 2021-03-30 9:08 ` Zhenyu Wang 2021-04-06 19:37 ` Alex Williamson 1 sibling, 0 replies; 14+ messages in thread From: Zhenyu Wang @ 2021-03-30 9:08 UTC (permalink / raw) To: Alex Williamson; +Cc: kvm, intel-gfx, Swee Yee Fonn [-- Attachment #1: Type: text/plain, Size: 3910 bytes --] On 2021.03.26 01:09:53 +0800, Fred Gao wrote: > Before opregion version 2.0 VBT data is stored in opregion mailbox #4, > but when VBT data exceeds 6KB size and cannot be within mailbox #4 > then 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 would not be > practically available for end user and guest can not directly access > host physical address, so 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> > --- Reviewed-by: Zhenyu Wang <zhenyuw@linux.intel.com> Hi, Alex, pls let us know if you have other concern to merge this one. Thanks! > drivers/vfio/pci/vfio_pci_igd.c | 53 +++++++++++++++++++++++++++++++++ > 1 file changed, 53 insertions(+) > > diff --git a/drivers/vfio/pci/vfio_pci_igd.c b/drivers/vfio/pci/vfio_pci_igd.c > index e66dfb0178ed..228df565e9bc 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,54 @@ 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, 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 > + * (Raw VBT Data Size) from opregion structure member are used to hold the > + * address from region base and size of VBT data. RVDA/RVDS are not > + * defined before opregion 2.0. > + * > + * opregion 2.1+: RVDA is unsigned, relative offset from > + * opregion base, and should point to the end of opregion. > + * otherwise, exposing to userspace to allow read access to everything between > + * the OpRegion and VBT is not safe. > + * RVDS is defined as size in bytes. > + * > + * opregion 2.0: rvda is the physical VBT address. > + * Since rvda is HPA it cannot be directly used in guest. > + * And it should not be practically available for end user,so it is not supported. > + */ > + 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 assignment does not support opregion v2.0 with an extended VBT region\n"); > + return -EINVAL; > + } > + > + if (rvda != size) { > + memunmap(base); > + pci_err(vdev->pdev, > + "Extended VBT does not follow opregion on version 0x%04x\n", > + version); > + return -EINVAL; > + } > + > + /* region size for opregion v2.0+: opregion and VBT size. */ > + size += rvds; > + } > + } > + > if (size != OPREGION_SIZE) { > memunmap(base); > base = memremap(addr, size, MEMREMAP_WB); > -- > 2.24.1.1.gb6d4d82bd5 > [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 195 bytes --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v5] vfio/pci: Add support for opregion v2.1+ 2021-03-25 17:09 ` [PATCH v5] " Fred Gao 2021-03-30 9:08 ` Zhenyu Wang @ 2021-04-06 19:37 ` Alex Williamson 1 sibling, 0 replies; 14+ messages in thread From: Alex Williamson @ 2021-04-06 19:37 UTC (permalink / raw) To: Fred Gao; +Cc: kvm, intel-gfx, Zhenyu Wang, Swee Yee Fonn On Fri, 26 Mar 2021 01:09:53 +0800 Fred Gao <fred.gao@intel.com> wrote: > Before opregion version 2.0 VBT data is stored in opregion mailbox #4, > but when VBT data exceeds 6KB size and cannot be within mailbox #4 > then 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 would not be > practically available for end user and guest can not directly access > host physical address, so 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 | 53 +++++++++++++++++++++++++++++++++ > 1 file changed, 53 insertions(+) Applied to vfio next branch for v5.13. Thanks, Alex ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2021-04-06 19:37 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-12-02 17:12 [PATCH v1] vfio/pci: Add support for opregion v2.0+ Fred Gao 2020-12-02 18:57 ` Alex Williamson 2020-12-03 9:21 ` Gao, Fred 2020-12-03 23:38 ` Alex Williamson 2021-01-18 12:38 ` [PATCH v2] " Fred Gao 2021-01-21 20:33 ` Alex Williamson 2021-02-02 5:09 ` Zhenyu Wang 2021-02-08 17:02 ` [PATCH v3] vfio/pci: Add support for opregion v2.1+ Fred Gao 2021-03-02 13:02 ` [PATCH v4] " Fred Gao 2021-03-19 19:26 ` Alex Williamson 2021-03-25 8:50 ` Gao, Fred 2021-03-25 17:09 ` [PATCH v5] " Fred Gao 2021-03-30 9:08 ` Zhenyu Wang 2021-04-06 19:37 ` Alex Williamson
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).