On Thu, 3 Dec 2020 09:21:03 +0000
"Gao, Fred" <fred.gao@xxxxxxxxx> wrote:
> Thanks Alex for the timely review.
>
> > -----Original Message-----
> > From: Alex Williamson <alex.williamson@xxxxxxxxxx>
> > Sent: Thursday, December 3, 2020 2:57 AM
> > To: Gao, Fred <fred.gao@xxxxxxxxx>
> > Cc: kvm@xxxxxxxxxxxxxxx; intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Zhenyu Wang
> > <zhenyuw@xxxxxxxxxxxxxxx>; Fonn, Swee Yee <swee.yee.fonn@xxxxxxxxx>
> > 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@xxxxxxxxx> 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@xxxxxxxxxxxxxxx>
> > > Signed-off-by: Swee Yee Fonn <swee.yee.fonn@xxxxxxxxx>
> > > Signed-off-by: Fred Gao <fred.gao@xxxxxxxxx>
> > > ---
> > > 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);
>
