[PATCH v1] vfio/pci: Add support for opregion v2.0+

[PATCH v1] vfio/pci: Add support for opregion v2.0+

[Fred Gao]

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

Re: [PATCH v1] vfio/pci: Add support for opregion v2.0+

[Alex Williamson]

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);

RE: [PATCH v1] vfio/pci: Add support for opregion v2.0+

[Gao, Fred]

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);

Re: [PATCH v1] vfio/pci: Add support for opregion v2.0+

[Alex Williamson]

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);  
> 

[PATCH v2] vfio/pci: Add support for opregion v2.0+

[Fred Gao]

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

Re: [PATCH v2] vfio/pci: Add support for opregion v2.0+

[Alex Williamson]

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);

Re: [PATCH v2] vfio/pci: Add support for opregion v2.0+

[Zhenyu Wang]

[-- 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 --]

[PATCH v3] vfio/pci: Add support for opregion v2.1+

[Fred Gao]

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

[PATCH v4] vfio/pci: Add support for opregion v2.1+

[Fred Gao]

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

Re: [PATCH v4] vfio/pci: Add support for opregion v2.1+

[Alex Williamson]

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);

RE: [PATCH v4] vfio/pci: Add support for opregion v2.1+

[Gao, Fred]

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);

[PATCH v5] vfio/pci: Add support for opregion v2.1+

[Fred Gao]

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

Re: [PATCH v5] vfio/pci: Add support for opregion v2.1+

[Zhenyu Wang]

[-- 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 --]

Re: [PATCH v5] vfio/pci: Add support for opregion v2.1+

[Alex Williamson]

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