[Intel-gfx] [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 at redhat.com>
> Sent: Thursday, December 3, 2020 2:57 AM
> To: Gao, Fred <fred.gao at intel.com>
> Cc: kvm at vger.kernel.org; intel-gfx at lists.freedesktop.org; Zhenyu Wang
> <zhenyuw at linux.intel.com>; Fonn, Swee Yee <swee.yee.fonn at 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 at 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 at linux.intel.com>
> > Signed-off-by: Swee Yee Fonn <swee.yee.fonn at intel.com>
> > Signed-off-by: Fred Gao <fred.gao at 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);