| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <0dd961ae-78a7-0b67-af51-008ecbcdbbef@loongson.cn>
Date: Thu, 22 Jun 2023 13:08:15 +0800
From: Sui Jingfeng <suijingfeng@...ngson.cn>
To: Sui Jingfeng <15330273260@....cn>,
Bjorn Helgaas <bhelgaas@...gle.com>
Cc: amd-gfx@...ts.freedesktop.org, dri-devel@...ts.freedesktop.org,
linux-kernel@...r.kernel.org, intel-gfx@...ts.freedesktop.org,
nouveau@...ts.freedesktop.org, linux-pci@...r.kernel.org,
kvm@...r.kernel.org, linux-fbdev@...r.kernel.org,
Alex Deucher <alexander.deucher@....com>,
Christian Konig <christian.koenig@....com>,
Pan Xinhui <Xinhui.Pan@....com>,
David Airlie <airlied@...il.com>,
Daniel Vetter <daniel@...ll.ch>,
Jani Nikula <jani.nikula@...ux.intel.com>,
Joonas Lahtinen <joonas.lahtinen@...ux.intel.com>,
Rodrigo Vivi <rodrigo.vivi@...el.com>,
Tvrtko Ursulin <tvrtko.ursulin@...ux.intel.com>,
Ben Skeggs <bskeggs@...hat.com>,
Karol Herbst <kherbst@...hat.com>,
Lyude Paul <lyude@...hat.com>,
Alex Williamson <alex.williamson@...hat.com>,
Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
Maxime Ripard <mripard@...nel.org>,
Thomas Zimmermann <tzimmermann@...e.de>,
Hawking Zhang <Hawking.Zhang@....com>,
Mario Limonciello <mario.limonciello@....com>,
Lijo Lazar <lijo.lazar@....com>,
YiPeng Chai <YiPeng.Chai@....com>,
Bokun Zhang <Bokun.Zhang@....com>,
Likun Gao <Likun.Gao@....com>,
Ville Syrjala <ville.syrjala@...ux.intel.com>,
Jason Gunthorpe <jgg@...pe.ca>,
Kevin Tian <kevin.tian@...el.com>,
Cornelia Huck <cohuck@...hat.com>,
Yishai Hadas <yishaih@...dia.com>,
Abhishek Sahu <abhsahu@...dia.com>, Yi Liu <yi.l.liu@...el.com>
Subject: Re: [PATCH v7 6/8] PCI/VGA: Introduce is_boot_device function
callback to vga_client_register
Hi,
A nouveau developer(Lyude) from redhat send me a R-B,
Thanks for the developers of nouveau project.
Please allow me add a link[1] here.
[1]
https://lore.kernel.org/all/0afadc69f99a36bc9d03ecf54ff25859dbc10e28.camel@redhat.com/
On 2023/6/13 11:01, Sui Jingfeng wrote:
> From: Sui Jingfeng <suijingfeng@...ngson.cn>
>
> The vga_is_firmware_default() function is arch-dependent, it's probably
> wrong if we simply remove the arch guard. As the VRAM BAR which contains
> firmware framebuffer may move, while the lfb_base and lfb_size members of
> the screen_info does not change accordingly. In short, it should take the
> re-allocation of the PCI BAR into consideration.
>
> With the observation that device drivers or video aperture helpers may
> have better knowledge about which PCI bar contains the firmware fb,
> which could avoid the need to iterate all of the PCI BARs. But as a PCI
> function at pci/vgaarb.c, vga_is_firmware_default() is not suitable to
> make such an optimization since it is loaded too early.
>
> There are PCI display controllers that don't have a dedicated VRAM bar,
> this function will lose its effectiveness in such a case. Luckily, the
> device driver can provide an accurate workaround.
>
> Therefore, this patch introduces a callback that allows the device driver
> to tell the VGAARB if the device is the default boot device. This patch
> only intends to introduce the mechanism, while the implementation is left
> to the device driver authors. Also honor the comment: "Clients have two
> callback mechanisms they can use"
>
> Cc: Alex Deucher <alexander.deucher@....com>
> Cc: Christian Konig <christian.koenig@....com>
> Cc: Pan Xinhui <Xinhui.Pan@....com>
> Cc: David Airlie <airlied@...il.com>
> Cc: Daniel Vetter <daniel@...ll.ch>
> Cc: Jani Nikula <jani.nikula@...ux.intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@...ux.intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@...el.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@...ux.intel.com>
> Cc: Ben Skeggs <bskeggs@...hat.com>
> Cc: Karol Herbst <kherbst@...hat.com>
> Cc: Lyude Paul <lyude@...hat.com>
> Cc: Bjorn Helgaas <bhelgaas@...gle.com>
> Cc: Alex Williamson <alex.williamson@...hat.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>
> Cc: Maxime Ripard <mripard@...nel.org>
> Cc: Thomas Zimmermann <tzimmermann@...e.de>
> Cc: Hawking Zhang <Hawking.Zhang@....com>
> Cc: Mario Limonciello <mario.limonciello@....com>
> Cc: Lijo Lazar <lijo.lazar@....com>
> Cc: YiPeng Chai <YiPeng.Chai@....com>
> Cc: Bokun Zhang <Bokun.Zhang@....com>
> Cc: Likun Gao <Likun.Gao@....com>
> Cc: Ville Syrjala <ville.syrjala@...ux.intel.com>
> Cc: Jason Gunthorpe <jgg@...pe.ca>
> CC: Kevin Tian <kevin.tian@...el.com>
> Cc: Cornelia Huck <cohuck@...hat.com>
> Cc: Yishai Hadas <yishaih@...dia.com>
> Cc: Abhishek Sahu <abhsahu@...dia.com>
> Cc: Yi Liu <yi.l.liu@...el.com>
> Signed-off-by: Sui Jingfeng <suijingfeng@...ngson.cn>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 2 +-
> drivers/gpu/drm/i915/display/intel_vga.c | 3 +--
> drivers/gpu/drm/nouveau/nouveau_vga.c | 2 +-
> drivers/gpu/drm/radeon/radeon_device.c | 2 +-
> drivers/pci/vgaarb.c | 21 ++++++++++++++++++++-
> drivers/vfio/pci/vfio_pci_core.c | 2 +-
> include/linux/vgaarb.h | 8 +++++---
> 7 files changed, 30 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 5c7d40873ee2..7a096f2d5c16 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -3960,7 +3960,7 @@ int amdgpu_device_init(struct amdgpu_device *adev,
> /* this will fail for cards that aren't VGA class devices, just
> * ignore it */
> if ((adev->pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA)
> - vga_client_register(adev->pdev, amdgpu_device_vga_set_decode);
> + vga_client_register(adev->pdev, amdgpu_device_vga_set_decode, NULL);
>
> px = amdgpu_device_supports_px(ddev);
>
> diff --git a/drivers/gpu/drm/i915/display/intel_vga.c b/drivers/gpu/drm/i915/display/intel_vga.c
> index 286a0bdd28c6..98d7d4dffe9f 100644
> --- a/drivers/gpu/drm/i915/display/intel_vga.c
> +++ b/drivers/gpu/drm/i915/display/intel_vga.c
> @@ -115,7 +115,6 @@ intel_vga_set_decode(struct pci_dev *pdev, bool enable_decode)
>
> int intel_vga_register(struct drm_i915_private *i915)
> {
> -
> struct pci_dev *pdev = to_pci_dev(i915->drm.dev);
> int ret;
>
> @@ -127,7 +126,7 @@ int intel_vga_register(struct drm_i915_private *i915)
> * then we do not take part in VGA arbitration and the
> * vga_client_register() fails with -ENODEV.
> */
> - ret = vga_client_register(pdev, intel_vga_set_decode);
> + ret = vga_client_register(pdev, intel_vga_set_decode, NULL);
> if (ret && ret != -ENODEV)
> return ret;
>
> diff --git a/drivers/gpu/drm/nouveau/nouveau_vga.c b/drivers/gpu/drm/nouveau/nouveau_vga.c
> index f8bf0ec26844..162b4f4676c7 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_vga.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_vga.c
> @@ -92,7 +92,7 @@ nouveau_vga_init(struct nouveau_drm *drm)
> return;
> pdev = to_pci_dev(dev->dev);
>
> - vga_client_register(pdev, nouveau_vga_set_decode);
> + vga_client_register(pdev, nouveau_vga_set_decode, NULL);
>
> /* don't register Thunderbolt eGPU with vga_switcheroo */
> if (pci_is_thunderbolt_attached(pdev))
> diff --git a/drivers/gpu/drm/radeon/radeon_device.c b/drivers/gpu/drm/radeon/radeon_device.c
> index afbb3a80c0c6..71f2ff39d6a1 100644
> --- a/drivers/gpu/drm/radeon/radeon_device.c
> +++ b/drivers/gpu/drm/radeon/radeon_device.c
> @@ -1425,7 +1425,7 @@ int radeon_device_init(struct radeon_device *rdev,
> /* if we have > 1 VGA cards, then disable the radeon VGA resources */
> /* this will fail for cards that aren't VGA class devices, just
> * ignore it */
> - vga_client_register(rdev->pdev, radeon_vga_set_decode);
> + vga_client_register(rdev->pdev, radeon_vga_set_decode, NULL);
>
> if (rdev->flags & RADEON_IS_PX)
> runtime = true;
> diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
> index ceb914245383..c574898380f0 100644
> --- a/drivers/pci/vgaarb.c
> +++ b/drivers/pci/vgaarb.c
> @@ -53,6 +53,7 @@ struct vga_device {
> bool bridge_has_one_vga;
> bool is_firmware_default; /* device selected by firmware */
> unsigned int (*set_decode)(struct pci_dev *pdev, bool decode);
> + bool (*is_boot_device)(struct pci_dev *pdev);
> };
>
> static LIST_HEAD(vga_list);
> @@ -969,6 +970,10 @@ EXPORT_SYMBOL(vga_set_legacy_decoding);
> * @set_decode callback: If a client can disable its GPU VGA resource, it
> * will get a callback from this to set the encode/decode state.
> *
> + * @is_boot_device: callback to the device driver, query if a client is the
> + * default boot device, as the device driver typically has better knowledge
> + * if specific device is the boot device. But this callback is optional.
> + *
> * Rationale: we cannot disable VGA decode resources unconditionally, some
> * single GPU laptops seem to require ACPI or BIOS access to the VGA registers
> * to control things like backlights etc. Hopefully newer multi-GPU laptops do
> @@ -984,7 +989,8 @@ EXPORT_SYMBOL(vga_set_legacy_decoding);
> * Returns: 0 on success, -1 on failure
> */
> int vga_client_register(struct pci_dev *pdev,
> - unsigned int (*set_decode)(struct pci_dev *pdev, bool decode))
> + unsigned int (*set_decode)(struct pci_dev *pdev, bool decode),
> + bool (*is_boot_device)(struct pci_dev *pdev))
> {
> int ret = -ENODEV;
> struct vga_device *vgadev;
> @@ -996,6 +1002,7 @@ int vga_client_register(struct pci_dev *pdev,
> goto bail;
>
> vgadev->set_decode = set_decode;
> + vgadev->is_boot_device = is_boot_device;
> ret = 0;
>
> bail:
> @@ -1523,6 +1530,18 @@ static int pci_notify(struct notifier_block *nb, unsigned long action,
> notify = vga_arbiter_add_pci_device(pdev);
> else if (action == BUS_NOTIFY_DEL_DEVICE)
> notify = vga_arbiter_del_pci_device(pdev);
> + else if (action == BUS_NOTIFY_BOUND_DRIVER) {
> + struct vga_device *vgadev = vgadev_find(pdev);
> + bool boot_dev = false;
> +
> + if (vgadev && vgadev->is_boot_device)
> + boot_dev = vgadev->is_boot_device(pdev);
> +
> + if (boot_dev) {
> + vgaarb_info(&pdev->dev, "Set as boot device (dictated by driver)\n");
> + vga_set_default_device(pdev);
> + }
> + }
>
> vgaarb_dbg(dev, "%s: action = %lu\n", __func__, action);
>
> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> index a5ab416cf476..2a8873a330ba 100644
> --- a/drivers/vfio/pci/vfio_pci_core.c
> +++ b/drivers/vfio/pci/vfio_pci_core.c
> @@ -2067,7 +2067,7 @@ static int vfio_pci_vga_init(struct vfio_pci_core_device *vdev)
> if (ret)
> return ret;
>
> - ret = vga_client_register(pdev, vfio_pci_set_decode);
> + ret = vga_client_register(pdev, vfio_pci_set_decode, NULL);
> if (ret)
> return ret;
> vga_set_legacy_decoding(pdev, vfio_pci_set_decode(pdev, false));
> diff --git a/include/linux/vgaarb.h b/include/linux/vgaarb.h
> index 97129a1bbb7d..dfde5a6ba55a 100644
> --- a/include/linux/vgaarb.h
> +++ b/include/linux/vgaarb.h
> @@ -33,7 +33,8 @@ struct pci_dev *vga_default_device(void);
> void vga_set_default_device(struct pci_dev *pdev);
> int vga_remove_vgacon(struct pci_dev *pdev);
> int vga_client_register(struct pci_dev *pdev,
> - unsigned int (*set_decode)(struct pci_dev *pdev, bool state));
> + unsigned int (*set_decode)(struct pci_dev *pdev, bool state),
> + bool (*is_boot_device)(struct pci_dev *pdev));
> #else /* CONFIG_VGA_ARB */
> static inline void vga_set_legacy_decoding(struct pci_dev *pdev,
> unsigned int decodes)
> @@ -59,7 +60,8 @@ static inline int vga_remove_vgacon(struct pci_dev *pdev)
> return 0;
> }
> static inline int vga_client_register(struct pci_dev *pdev,
> - unsigned int (*set_decode)(struct pci_dev *pdev, bool state))
> + unsigned int (*set_decode)(struct pci_dev *pdev, bool state),
> + bool (*is_boot_device)(struct pci_dev *pdev))
> {
> return 0;
> }
> @@ -97,7 +99,7 @@ static inline int vga_get_uninterruptible(struct pci_dev *pdev,
>
> static inline void vga_client_unregister(struct pci_dev *pdev)
> {
> - vga_client_register(pdev, NULL);
> + vga_client_register(pdev, NULL, NULL);
> }
>
> #endif /* LINUX_VGA_H */
--
Jingfeng
Powered by blists - more mailing lists