lkml.org 
[lkml]   [2021]   [Aug]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] vfio/pci: Add OpRegion 2.0 Extended VBT support.
Hi Alex,

In addition to the background that devices on market may still need
OpRegion 2.0 support in vfio-pci, do you have other comments to the patch
body?

On Tue, 17 Aug 2021, Colin Xu wrote:

> On Mon, 16 Aug 2021, Alex Williamson wrote:
>
>> On Fri, 13 Aug 2021 10:13:29 +0800
>> Colin Xu <colin.xu@intel.com> wrote:
>>
>>> Due to historical reason, some legacy shipped system doesn't follow
>>> OpRegion 2.1 spec but still stick to OpRegion 2.0, in which the extended
>>> VBT is not contigious after OpRegion in physical address, but any
>>> location pointed by RVDA via absolute address. Thus it's impossible
>>> to map a contigious range to hold both OpRegion and extended VBT as 2.1.
>>>
>>> Since the only difference between OpRegion 2.0 and 2.1 is where extended
>>> VBT is stored: For 2.0, RVDA is the absolute address of extended VBT
>>> while for 2.1, RVDA is the relative address of extended VBT to OpRegion
>>> baes, and there is no other difference between OpRegion 2.0 and 2.1,
>>> it's feasible to amend OpRegion support for these legacy system (before
>>> upgrading the system firmware), by kazlloc a range to shadown OpRegion
>>> from the beginning and stitch VBT after closely, patch the shadow
>>> OpRegion version from 2.0 to 2.1, and patch the shadow RVDA to relative
>>> address. So that from the vfio igd OpRegion r/w ops view, only OpRegion
>>> 2.1 is exposed regardless the underneath host OpRegion is 2.0 or 2.1
>>> if the extended VBT exists. vfio igd OpRegion r/w ops will return either
>>> shadowed data (OpRegion 2.0) or directly from physical address
>>> (OpRegion 2.1+) based on host OpRegion version and RVDA/RVDS. The shadow
>>> mechanism makes it possible to support legacy systems on the market.
>>
>> Which systems does this enable? There's a suggestion above that these
>> systems could update firmware to get OpRegion v2.1 support, why
>> shouldn't we ask users to do that instead? When we added OpRegion v2.1
>> support we were told that v2.0 support was essentially non-existent,
>> why should we add code to support and old spec with few users for such
>> a niche use case?
> Hi Alex, there was some mis-alignment with the BIOS owner that we were told
> the 2.0 system doesn't for retail but only for internal development. However
> in other projects we DO see the retail market has such systems, including NUC
> NUC6CAYB, some APL industrial PC used in RT system, and some customized APL
> motherboard by commercial virtualization solution. We immediately contact the
> BIOS owner to ask for a clarification and they admit it. These system won't
> get updated BIOS for OpRegion update but still under warranty. That's why the
> OpRegion 2.0 support is still needed.
>
>>
>>> Cc: Zhenyu Wang <zhenyuw@linux.intel.com>
>>> Cc: Hang Yuan <hang.yuan@linux.intel.com>
>>> Cc: Swee Yee Fonn <swee.yee.fonn@intel.com>
>>> Cc: Fred Gao <fred.gao@intel.com>
>>> Signed-off-by: Colin Xu <colin.xu@intel.com>
>>> ---
>>> drivers/vfio/pci/vfio_pci_igd.c | 117 ++++++++++++++++++++------------
>>> 1 file changed, 75 insertions(+), 42 deletions(-)
>>>
>>> diff --git a/drivers/vfio/pci/vfio_pci_igd.c
>>> b/drivers/vfio/pci/vfio_pci_igd.c
>>> index 228df565e9bc..22b9436a3044 100644
>>> --- a/drivers/vfio/pci/vfio_pci_igd.c
>>> +++ b/drivers/vfio/pci/vfio_pci_igd.c
>>> @@ -48,7 +48,10 @@ static size_t vfio_pci_igd_rw(struct vfio_pci_device
>>> *vdev, char __user *buf,
>>> static void vfio_pci_igd_release(struct vfio_pci_device *vdev,
>>> struct vfio_pci_region *region)
>>> {
>>> - memunmap(region->data);
>>> + if (is_ioremap_addr(region->data))
>>> + memunmap(region->data);
>>> + else
>>> + kfree(region->data);
>>> }
>>>
>>> static const struct vfio_pci_regops vfio_pci_igd_regops = {
>>> @@ -59,10 +62,11 @@ static const struct vfio_pci_regops
>>> vfio_pci_igd_regops = {
>>> static int vfio_pci_igd_opregion_init(struct vfio_pci_device *vdev)
>>> {
>>> __le32 *dwordp = (__le32 *)(vdev->vconfig + OPREGION_PCI_ADDR);
>>> - u32 addr, size;
>>> - void *base;
>>> + u32 addr, size, rvds = 0;
>>> + void *base, *opregionvbt;
>>> int ret;
>>> u16 version;
>>> + u64 rvda = 0;
>>>
>>> ret = pci_read_config_dword(vdev->pdev, OPREGION_PCI_ADDR, &addr);
>>> if (ret)
>>> @@ -89,66 +93,95 @@ 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 and VBT:
>>> + * When VBT data doesn't exceed 6KB, it's stored in Mailbox #4.
>>> + * When VBT data exceeds 6KB size, Mailbox #4 is no longer large
>>> enough
>>> + * to hold the VBT data, the Extended VBT region is introduced since
>>> + * OpRegion 2.0 to hold the VBT data. Since OpRegion 2.0, RVDA/RVDS
>>> are
>>> + * introduced to define the extended VBT data location and size.
>>> + * OpRegion 2.0: RVDA defines the absolute physical address of the
>>> + * extended VBT data, RVDS defines the VBT data size.
>>> + * OpRegion 2.1 and above: RVDA defines the relative address of the
>>> + * extended VBT data to OpRegion base, RVDS defines the VBT data
>>> size.
>>> *
>>> - * 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.
>>> + * Due to the RVDA difference in OpRegion VBT (also the only diff
>>> between
>>> + * 2.0 and 2.1), while for OpRegion 2.1 and above it's possible to
>>> map
>>> + * a contigious memory to expose OpRegion and VBT r/w via the vfio
>>> + * region, for OpRegion 2.0 shadow and amendment mechanism is used to
>>> + * expose OpRegion and VBT r/w properly. So that from r/w ops view,
>>> only
>>> + * OpRegion 2.1 is exposed regardless underneath Region is 2.0 or
>>> 2.1.
>>> */
>>> version = le16_to_cpu(*(__le16 *)(base + OPREGION_VERSION));
>>> - if (version >= 0x0200) {
>>> - u64 rvda;
>>> - u32 rvds;
>>>
>>> + if (version >= 0x0200) {
>>> rvda = le64_to_cpu(*(__le64 *)(base + OPREGION_RVDA));
>>> rvds = le32_to_cpu(*(__le32 *)(base + OPREGION_RVDS));
>>> +
>>> + /* The extended VBT is valid only when RVDA/RVDS are
>>> non-zero. */
>>> if (rvda && rvds) {
>>> - /* no support for opregion v2.0 with physical VBT
>>> address */
>>> - if (version == 0x0200) {
>>> + size += rvds;
>>> + }
>>> +
>>> + /* The extended VBT must follows OpRegion for OpRegion 2.1+
>>> */
>>> + if (rvda != size && version > 0x0200) {
>>
>> But we already added rvds to size, this is not compatible with the
>> previous code that required rvda == size BEFORE adding rvds.
>>
>>> + memunmap(base);
>>> + pci_err(vdev->pdev,
>>> + "Extended VBT does not follow opregion on
>>> version 0x%04x\n",
>>> + version);
>>> + return -EINVAL;
>>> + }
>>> + }
>>> +
>>> + if (size != OPREGION_SIZE) {
>>> + /* Allocate memory for OpRegion and extended VBT for 2.0 */
>>> + if (rvda && rvds && version == 0x0200) {
>>> + void *vbt_base;
>>> +
>>> + vbt_base = memremap(rvda, rvds, MEMREMAP_WB);
>>> + if (!vbt_base) {
>>> memunmap(base);
>>> - pci_err(vdev->pdev,
>>> - "IGD assignment does not support
>>> opregion v2.0 with an extended VBT region\n");
>>> - return -EINVAL;
>>> + return -ENOMEM;
>>> }
>>>
>>> - if (rvda != size) {
>>> + opregionvbt = kzalloc(size, GFP_KERNEL);
>>> + if (!opregionvbt) {
>>> memunmap(base);
>>> - pci_err(vdev->pdev,
>>> - "Extended VBT does not follow
>>> opregion on version 0x%04x\n",
>>> - version);
>>> - return -EINVAL;
>>> + memunmap(vbt_base);
>>> + return -ENOMEM;
>>> }
>>>
>>> - /* region size for opregion v2.0+: opregion and VBT
>>> size. */
>>> - size += rvds;
>>> + /* Stitch VBT after OpRegion noncontigious */
>>> + memcpy(opregionvbt, base, OPREGION_SIZE);
>>> + memcpy(opregionvbt + OPREGION_SIZE, vbt_base, rvds);
>>> +
>>> + /* Patch OpRegion 2.0 to 2.1 */
>>> + *(__le16 *)(opregionvbt + OPREGION_VERSION) = 0x0201;
>>> + /* Patch RVDA to relative address after OpRegion */
>>> + *(__le64 *)(opregionvbt + OPREGION_RVDA) =
>>> OPREGION_SIZE;
>>
>> AIUI, the OpRegion is a two-way channel between the IGD device/system
>> BIOS and the driver, numerous fields are writable by the driver. Now
>> the driver writes to a shadow copy of the OpRegion table. What
>> completes the write to the real OpRegion table for consumption by the
>> device/BIOS? Likewise, what updates the fields that are written by the
>> device/BIOS for consumption by the driver?
>>
>> If a shadow copy of the OpRegion detached from the physical table is
>> sufficient here, why wouldn't we always shadow the OpRegion and prevent
>> all userspace writes from touching the real version? Thanks,
>>
>> Alex
>>
>>> +
>>> + memunmap(vbt_base);
>>> + memunmap(base);
>>> +
>>> + /* Register shadow instead of map as vfio_region */
>>> + base = opregionvbt;
>>> + /* Remap OpRegion + extended VBT for 2.1+ */
>>> + } else {
>>> + memunmap(base);
>>> + base = memremap(addr, size, MEMREMAP_WB);
>>> + if (!base)
>>> + return -ENOMEM;
>>> }
>>> }
>>>
>>> - if (size != OPREGION_SIZE) {
>>> - memunmap(base);
>>> - base = memremap(addr, size, MEMREMAP_WB);
>>> - if (!base)
>>> - return -ENOMEM;
>>> - }
>>> -
>>> ret = vfio_pci_register_dev_region(vdev,
>>> PCI_VENDOR_ID_INTEL | VFIO_REGION_TYPE_PCI_VENDOR_TYPE,
>>> VFIO_REGION_SUBTYPE_INTEL_IGD_OPREGION,
>>> &vfio_pci_igd_regops, size, VFIO_REGION_INFO_FLAG_READ, base);
>>> if (ret) {
>>> - memunmap(base);
>>> + if (is_ioremap_addr(base))
>>> + memunmap(base);
>>> + else
>>> + kfree(base);
>>> return ret;
>>> }
>>>
>>
>>
>
> --
> Best Regards,
> Colin Xu
>
>

--
Best Regards,
Colin Xu

\
 
 \ /
  Last update: 2021-08-27 03:37    [W:0.368 / U:0.032 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site