| 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: <20190514102138.13bf7de0@x1.home>
Date: Tue, 14 May 2019 10:21:38 -0600
From: Alex Williamson <alex.williamson@...hat.com>
To: "Liu, Yi L" <yi.l.liu@...el.com>
Cc: kwankhede@...dia.com, kevin.tian@...el.com,
baolu.lu@...ux.intel.com, yi.y.sun@...el.com, joro@...tes.org,
jean-philippe.brucker@....com, peterx@...hat.com,
linux-kernel@...r.kernel.org, kvm@...r.kernel.org,
yamada.masahiro@...ionext.com, iommu@...ts.linux-foundation.org
Subject: Re: [RFC v3 1/3] vfio_pci: split vfio_pci.c into two source files
On Tue, 23 Apr 2019 20:14:38 +0800
"Liu, Yi L" <yi.l.liu@...el.com> wrote:
> This patch splits the non-module specific codes from original
> drivers/vfio/pci/vfio_pci.c into a common.c under drivers/vfio/pci.
> This is for potential code sharing. e.g. vfio-mdev-pci driver
>
> Cc: Kevin Tian <kevin.tian@...el.com>
> Cc: Lu Baolu <baolu.lu@...ux.intel.com>
> Signed-off-by: Liu, Yi L <yi.l.liu@...el.com>
> ---
> drivers/vfio/pci/Makefile | 2 +-
> drivers/vfio/pci/common.c | 1511 +++++++++++++++++++++++++++++++++++
> drivers/vfio/pci/vfio_pci.c | 1476 +---------------------------------
> drivers/vfio/pci/vfio_pci_private.h | 27 +
> 4 files changed, 1551 insertions(+), 1465 deletions(-)
> create mode 100644 drivers/vfio/pci/common.c
>
> diff --git a/drivers/vfio/pci/Makefile b/drivers/vfio/pci/Makefile
> index 9662c06..813f6b3 100644
> --- a/drivers/vfio/pci/Makefile
> +++ b/drivers/vfio/pci/Makefile
> @@ -1,5 +1,5 @@
>
> -vfio-pci-y := vfio_pci.o vfio_pci_intrs.o vfio_pci_rdwr.o vfio_pci_config.o
> +vfio-pci-y := vfio_pci.o common.o vfio_pci_intrs.o vfio_pci_rdwr.o vfio_pci_config.o
> vfio-pci-$(CONFIG_VFIO_PCI_IGD) += vfio_pci_igd.o
> vfio-pci-$(CONFIG_VFIO_PCI_NVLINK2) += vfio_pci_nvlink2.o
>
> diff --git a/drivers/vfio/pci/common.c b/drivers/vfio/pci/common.c
> new file mode 100644
> index 0000000..847e2e4
> --- /dev/null
> +++ b/drivers/vfio/pci/common.c
Nit, I realize that our file naming scheme includes a lot of
redundancy, but better to have redundancy than inconsistency imo.
Perhaps vfio_pci_common.c.
> @@ -0,0 +1,1511 @@
> +/*
> + * Copyright © 2019 Intel Corporation.
> + * Author: Liu, Yi L <yi.l.liu@...el.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * Derived from original vfio_pci.c:
> + * Copyright (C) 2012 Red Hat, Inc. All rights reserved.
> + * Author: Alex Williamson <alex.williamson@...hat.com>
> + *
> + * Derived from original vfio:
> + * Copyright 2010 Cisco Systems, Inc. All rights reserved.
> + * Author: Tom Lyon, pugs@...co.com
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/device.h>
> +#include <linux/eventfd.h>
> +#include <linux/file.h>
> +#include <linux/interrupt.h>
> +#include <linux/iommu.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/notifier.h>
> +#include <linux/pci.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/slab.h>
> +#include <linux/types.h>
> +#include <linux/uaccess.h>
> +#include <linux/vfio.h>
> +#include <linux/vgaarb.h>
> +#include <linux/nospec.h>
> +
> +#include "vfio_pci_private.h"
> +
[snip faithful code moves]
> +void vfio_pci_vga_probe(struct vfio_pci_device *vdev)
> +{
> + vga_client_register(vdev->pdev, vdev, NULL, vfio_pci_set_vga_decode);
> + vga_set_legacy_decoding(vdev->pdev,
> + vfio_pci_set_vga_decode(vdev, false));
> +}
> +
> +void vfio_pci_vga_remove(struct vfio_pci_device *vdev)
> +{
> + vga_client_register(vdev->pdev, NULL, NULL, NULL);
> + vga_set_legacy_decoding(vdev->pdev,
> + VGA_RSRC_NORMAL_IO | VGA_RSRC_NORMAL_MEM |
> + VGA_RSRC_LEGACY_IO | VGA_RSRC_LEGACY_MEM);
> +}
Two new functions, though the names don't really match their purpose.
[snip more faithful code moves]
> +
> +void vfio_pci_probe_idle_d3(struct vfio_pci_device *vdev)
> +{
> +
> + /*
> + * pci-core sets the device power state to an unknown value at
> + * bootup and after being removed from a driver. The only
> + * transition it allows from this unknown state is to D0, which
> + * typically happens when a driver calls pci_enable_device().
> + * We're not ready to enable the device yet, but we do want to
> + * be able to get to D3. Therefore first do a D0 transition
> + * before going to D3.
> + */
> + vfio_pci_set_power_state(vdev, PCI_D0);
> + vfio_pci_set_power_state(vdev, PCI_D3hot);
> +}
Another new function. This also doesn't really match function name to
purpose.
[snip more faithful code moves]
> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> index 3fa20e9..6ce1a81 100644
> --- a/drivers/vfio/pci/vfio_pci.c
> +++ b/drivers/vfio/pci/vfio_pci.c
[en masse code deletes]
> @@ -1324,6 +147,11 @@ static int vfio_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> spin_lock_init(&vdev->irqlock);
> mutex_init(&vdev->ioeventfds_lock);
> INIT_LIST_HEAD(&vdev->ioeventfds_list);
> + vdev->nointxmask = nointxmask;
> +#ifdef CONFIG_VFIO_PCI_VGA
> + vdev->disable_vga = disable_vga;
> +#endif
> + vdev->disable_idle_d3 = disable_idle_d3;
>
> ret = vfio_add_group_dev(&pdev->dev, &vfio_pci_ops, vdev);
> if (ret) {
> @@ -1340,27 +168,13 @@ static int vfio_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> return ret;
> }
>
> - if (vfio_pci_is_vga(pdev)) {
> - vga_client_register(pdev, vdev, NULL, vfio_pci_set_vga_decode);
> - vga_set_legacy_decoding(pdev,
> - vfio_pci_set_vga_decode(vdev, false));
> - }
> + if (vfio_pci_is_vga(pdev))
> + vfio_pci_vga_probe(vdev);
>
> vfio_pci_probe_power_state(vdev);
>
> - if (!disable_idle_d3) {
> - /*
> - * pci-core sets the device power state to an unknown value at
> - * bootup and after being removed from a driver. The only
> - * transition it allows from this unknown state is to D0, which
> - * typically happens when a driver calls pci_enable_device().
> - * We're not ready to enable the device yet, but we do want to
> - * be able to get to D3. Therefore first do a D0 transition
> - * before going to D3.
> - */
> - vfio_pci_set_power_state(vdev, PCI_D0);
> - vfio_pci_set_power_state(vdev, PCI_D3hot);
> - }
> + if (!disable_idle_d3)
> + vfio_pci_probe_idle_d3(vdev);
>
> return ret;
> }
> @@ -1383,48 +197,14 @@ static void vfio_pci_remove(struct pci_dev *pdev)
> vfio_pci_set_power_state(vdev, PCI_D0);
>
> kfree(vdev->pm_save);
> - kfree(vdev);
>
> if (vfio_pci_is_vga(pdev)) {
> - vga_client_register(pdev, NULL, NULL, NULL);
> - vga_set_legacy_decoding(pdev,
> - VGA_RSRC_NORMAL_IO | VGA_RSRC_NORMAL_MEM |
> - VGA_RSRC_LEGACY_IO | VGA_RSRC_LEGACY_MEM);
> - }
> -}
> -
> -static pci_ers_result_t vfio_pci_aer_err_detected(struct pci_dev *pdev,
> - pci_channel_state_t state)
> -{
> - struct vfio_pci_device *vdev;
> - struct vfio_device *device;
> -
> - device = vfio_device_get_from_dev(&pdev->dev);
> - if (device == NULL)
> - return PCI_ERS_RESULT_DISCONNECT;
> -
> - vdev = vfio_device_data(device);
> - if (vdev == NULL) {
> - vfio_device_put(device);
> - return PCI_ERS_RESULT_DISCONNECT;
> + vfio_pci_vga_remove(vdev);
> }
>
> - mutex_lock(&vdev->igate);
> -
> - if (vdev->err_trigger)
> - eventfd_signal(vdev->err_trigger, 1);
> -
> - mutex_unlock(&vdev->igate);
> -
> - vfio_device_put(device);
> -
> - return PCI_ERS_RESULT_CAN_RECOVER;
> + kfree(vdev);
> }
All of the above refactoring should occur in a preceding patch(es).
This patch should be 100% unchanged code moves plus support
includes/defines and comment header in the new file.
> @@ -1685,7 +233,7 @@ static int __init vfio_pci_init(void)
> if (ret)
> goto out_driver;
>
> - vfio_pci_fill_ids();
> + vfio_pci_fill_ids(&ids[0], &vfio_pci_driver);
For instance I missed this change.
>
> return 0;
>
> diff --git a/drivers/vfio/pci/vfio_pci_private.h b/drivers/vfio/pci/vfio_pci_private.h
> index 1812cf2..9bbf22c 100644
> --- a/drivers/vfio/pci/vfio_pci_private.h
> +++ b/drivers/vfio/pci/vfio_pci_private.h
> @@ -125,6 +125,11 @@ struct vfio_pci_device {
> struct list_head dummy_resources_list;
> struct mutex ioeventfds_lock;
> struct list_head ioeventfds_list;
> + bool nointxmask;
> +#ifdef CONFIG_VFIO_PCI_VGA
> + bool disable_vga;
> +#endif
> + bool disable_idle_d3;
More refactoring, do these separately so they can be properly reviewed
without trying to spot changes in 3000 lines of diff. I think what
you've done is sound, it just needs to be refactored separate from the
code moves so that it can be reviewed. Thanks!
Alex
Powered by blists - more mailing lists