Re: [-next 0/5] Add the pci_is_vga() helper and use it
On Wed, Aug 30, 2023 at 07:15:27PM +0800, Sui Jingfeng wrote:
> From: Sui Jingfeng
>
> The PCI code and ID assignment specification defined four types of
> display controllers for the display base class(03h), and the devices
> with 0x00h sub-class code are VGA devices. VGA devices with programming
> interface 0x00 is VGA-compatible, VGA devices with programming interface
> 0x01 are 8514-compatible controllers. Besides, PCI_CLASS_NOT_DEFINED_VGA
> is defined to provide backward compatibility for devices that were built
> before the class code field was defined. Thus, PCI(e) device with the
> PCI_CLASS_NOT_DEFINED_VGA class code should also be handled as the normal
> VGA-compatible devices.
>
> Compared with the "if ((pdev->class >> 8) != PCI_CLASS_DISPLAY_VGA)" code,
> the newly implemented pci_is_vga() is shorter and straightforward. So it
> is more easy to use. It is designed as a inline function, the more common
> case "if ((pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA))" is put before the
> less common case "if ((pdev->class >> 8) == PCI_CLASS_NOT_DEFINED_VGA)",
> so there should no performance penalty.
>
> Sui Jingfeng (5):
> PCI: Add the pci_is_vga() helper
> PCI/VGA: Deal with VGA devices
> PCI/sysfs: Use pci_is_vga() helper
> drm/virgpu: Switch to pci_is_vga()
> drm/qxl: Switch to pci_is_vga()
>
> drivers/gpu/drm/qxl/qxl_drv.c| 11 +++
> drivers/gpu/drm/virtio/virtgpu_drv.c | 2 +-
> drivers/pci/pci-sysfs.c | 6 +++---
> drivers/pci/vgaarb.c | 19 +--
> include/linux/pci.h | 27 +++
> 5 files changed, 43 insertions(+), 22 deletions(-)
I applied these on pci/vga for v6.7, thanks!
Please take a look at
https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/log/?h=vga
and let me know if I broke anything because I changed a few things;
interdiff below:
diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 522708938563..252ee29fd697 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -1551,9 +1551,8 @@ static umode_t pci_dev_attrs_are_visible(struct kobject
*kobj,
struct device *dev = kobj_to_dev(kobj);
struct pci_dev *pdev = to_pci_dev(dev);
- if (a == _attr_boot_vga.attr)
- if (pci_is_vga(pdev))
- return a->mode;
+ if (a == _attr_boot_vga.attr && pci_is_vga(pdev))
+ return a->mode;
return 0;
}
diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
index ef8fe685de67..feca96fc4255 100644
--- a/drivers/pci/vgaarb.c
+++ b/drivers/pci/vgaarb.c
@@ -1499,6 +1499,7 @@ static int pci_notify(struct notifier_block *nb, unsigned
long action,
vgaarb_dbg(dev, "%s\n", __func__);
+ /* Only deal with VGA class devices */
if (!pci_is_vga(pdev))
return 0;
@@ -1536,8 +1537,8 @@ static struct miscdevice vga_arb_device = {
static int __init vga_arb_device_init(void)
{
- struct pci_dev *pdev = NULL;
int rc;
+ struct pci_dev *pdev;
rc = misc_register(_arb_device);
if (rc < 0)
@@ -1546,11 +1547,13 @@ static int __init vga_arb_device_init(void)
bus_register_notifier(_bus_type, _notifier);
/* Add all VGA class PCI devices by default */
- do {
- pdev = pci_get_subsys(PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID,
PCI_ANY_ID, pdev);
+ pdev = NULL;
+ while ((pdev =
+ pci_get_subsys(PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID,
+ PCI_ANY_ID, pdev)) != NULL) {
if (pci_is_vga(pdev))
vga_arbiter_add_pci_device(pdev);
- } while (pdev);
+ }
pr_info("loaded\n");
return rc;
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 53e24e31c27e..7bab234391cb 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -714,23 +714,20 @@ static inline bool pci_is_bridge(struct pci_dev *dev)
}
/**
- * The PCI code and ID assignment specification defined four types of
- * display controllers for the display base class(03h), and the devices
- * with 0x00h sub-class code are VGA devices. VGA devices with programming
- * interface 0x00 is VGA-compatible, VGA devices with programming interface
- * 0x01 are 8514-compatible controllers. Besides, PCI_CLASS_NOT_DEFINED_VGA
- * is defined to provide backward compatibility for devices that were built
- * before the class code field was defined. This means that it belong to the
- * VGA devices category also.
+ * pci_is_vga - check if the PCI device is a VGA device
*
- * Returns:
- * true if the PCI device is a VGA device, false otherwise.
+ * The PCI Code and ID Assignment spec, r1.15, secs 1.4 and 1.1, define
+ * VGA Base Class and Sub-Classes:
+ *
+ * 03 00 PCI_CLASS_DISPLAY_VGA VGA-compatible or 8514-compatible
+ * 00 01 PCI_CLASS_NOT_DEFINED_VGA VGA-compatible (before Class Code)
+ *
+ * Return true if
Re: [-next 1/5] PCI: Add the pci_is_vga() helper
On Wed, Aug 30, 2023 at 07:15:28PM +0800, Sui Jingfeng wrote:
> From: Sui Jingfeng
>
> The PCI code and ID assignment specification defined four types of
> display controllers for the display base class(03h), and the devices
> with 0x00h sub-class code are VGA devices. VGA devices with programming
I can update this with the spec details (PCI Code and Assignment spec
r1.15, secs 1.1 and 1.4).
> interface 0x00 is VGA-compatible, VGA devices with programming interface
> 0x01 are 8514-compatible controllers. Besides, PCI_CLASS_NOT_DEFINED_VGA
> is defined to provide backward compatibility for devices that were built
> before the class code field was defined. Hence, introduce the pci_is_vga()
> helper, let it handle the details for us. It returns true if the PCI(e)
> device being tested belongs to the VGA devices category.
>
> Cc: "Maciej W. Rozycki"
> Signed-off-by: Sui Jingfeng
> ---
> include/linux/pci.h | 27 +++
> 1 file changed, 27 insertions(+)
>
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index cf6e0b057752..ace727001911 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -713,6 +713,33 @@ static inline bool pci_is_bridge(struct pci_dev *dev)
> dev->hdr_type == PCI_HEADER_TYPE_CARDBUS;
> }
>
> +/**
> + * The PCI code and ID assignment specification defined four types of
> + * display controllers for the display base class(03h), and the devices
> + * with 0x00h sub-class code are VGA devices. VGA devices with programming
> + * interface 0x00 is VGA-compatible, VGA devices with programming interface
> + * 0x01 are 8514-compatible controllers. Besides, PCI_CLASS_NOT_DEFINED_VGA
> + * is defined to provide backward compatibility for devices that were built
> + * before the class code field was defined. This means that it belong to the
> + * VGA devices category also.
> + *
> + * Returns:
> + * true if the PCI device is a VGA device, false otherwise.
> + */
> +static inline bool pci_is_vga(struct pci_dev *pdev)
> +{
> + if (!pdev)
> + return false;
> +
> + if ((pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA)
> + return true;
> +
> + if ((pdev->class >> 8) == PCI_CLASS_NOT_DEFINED_VGA)
> + return true;
Are you seeing a problem that will be fixed by this series, i.e., a
PCI_CLASS_NOT_DEFINED_VGA device that we currently don't handle
correctly?
I think this makes sense per the spec, but there's always a risk of
breaking something, so it's nice if the change actually *fixes*
something to make that risk worthwhile.
> + return false;
> +}
> +
> #define for_each_pci_bridge(dev, bus)\
> list_for_each_entry(dev, >devices, bus_list) \
> if (!pci_is_bridge(dev)) {} else
> --
> 2.34.1
>
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [-next 4/5] drm/virgpu: Switch to pci_is_vga()
On Thu, Oct 05, 2023 at 04:57:14PM -0500, Bjorn Helgaas wrote:
> In subject: "drm/virtio" to match previous history.
>
> On Wed, Aug 30, 2023 at 07:15:31PM +0800, Sui Jingfeng wrote:
> > From: Sui Jingfeng
> >
> > Should be no functional change, just for cleanup purpose.
> >
> > Cc: David Airlie
> > Cc: Gerd Hoffmann
> > Cc: Gurchetan Singh
> > Cc: Chia-I Wu
> > Cc: Daniel Vetter
> > Signed-off-by: Sui Jingfeng
> > ---
> > drivers/gpu/drm/virtio/virtgpu_drv.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.c
> > b/drivers/gpu/drm/virtio/virtgpu_drv.c
> > index add075681e18..3a368304475a 100644
> > --- a/drivers/gpu/drm/virtio/virtgpu_drv.c
> > +++ b/drivers/gpu/drm/virtio/virtgpu_drv.c
> > @@ -51,7 +51,7 @@ static int virtio_gpu_pci_quirk(struct drm_device *dev)
> > {
> > struct pci_dev *pdev = to_pci_dev(dev->dev);
> > const char *pname = dev_name(>dev);
> > - bool vga = (pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA;
> > + bool vga = pci_is_vga(pdev);
>
> This *is* a functional change: Previously "vga" was only true for
> PCI_CLASS_DISPLAY_VGA (0x0300). Now it will be true for both
> PCI_CLASS_DISPLAY_VGA (0x0300) and PCI_CLASS_DISPLAY_OTHER (0x0380).
Oops, sorry, my mistake here. I meant PCI_CLASS_NOT_DEFINED_VGA, not
PCI_CLASS_DISPLAY_OTHER. pci_is_vga() is true for either of:
PCI_CLASS_DISPLAY_VGA 0x0300
PCI_CLASS_NOT_DEFINED_VGA 0x0001
(PCI_CLASS_NOT_DEFINED_VGA is defined in the PCI Code and Assignment
spec r1.15, sec 1.1; PCI_CLASS_DISPLAY_VGA is sec 1.4.)
> Is that desirable? I can't tell. Maybe the GPU folks will chime in.
>
> > int ret;
> >
> > DRM_INFO("pci: %s detected at %s\n",
> > --
> > 2.34.1
> >
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [-next 4/5] drm/virgpu: Switch to pci_is_vga()
In subject: "drm/virtio" to match previous history.
On Wed, Aug 30, 2023 at 07:15:31PM +0800, Sui Jingfeng wrote:
> From: Sui Jingfeng
>
> Should be no functional change, just for cleanup purpose.
>
> Cc: David Airlie
> Cc: Gerd Hoffmann
> Cc: Gurchetan Singh
> Cc: Chia-I Wu
> Cc: Daniel Vetter
> Signed-off-by: Sui Jingfeng
> ---
> drivers/gpu/drm/virtio/virtgpu_drv.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.c
> b/drivers/gpu/drm/virtio/virtgpu_drv.c
> index add075681e18..3a368304475a 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_drv.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_drv.c
> @@ -51,7 +51,7 @@ static int virtio_gpu_pci_quirk(struct drm_device *dev)
> {
> struct pci_dev *pdev = to_pci_dev(dev->dev);
> const char *pname = dev_name(>dev);
> - bool vga = (pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA;
> + bool vga = pci_is_vga(pdev);
This *is* a functional change: Previously "vga" was only true for
PCI_CLASS_DISPLAY_VGA (0x0300). Now it will be true for both
PCI_CLASS_DISPLAY_VGA (0x0300) and PCI_CLASS_DISPLAY_OTHER (0x0380).
Is that desirable? I can't tell. Maybe the GPU folks will chime in.
> int ret;
>
> DRM_INFO("pci: %s detected at %s\n",
> --
> 2.34.1
>
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RFC PATCH v2 1/3] PCI: endpoint: introduce a helper to implement pci ep virtio function
Simple typos, don't repost until there's more substantive feedback.
On Thu, Apr 27, 2023 at 07:44:26PM +0900, Shunsuke Mie wrote:
> The Linux PCIe Endpoint framework supports to implement PCIe endpoint
> functions using a PCIe controller operating in endpoint mode.
> It is possble to realize the behavior of PCIe device, such as virtio PCI
> device. This patch introduces a setof helper functions and data structures
> to implement a PCIe endpoint function that behaves as a virtio device.
s/possble/possible/
s/setof/set of/
> Those functions enable the implementation PCIe endpoint function that
> comply with the virtio lecacy specification. Because modern virtio
> specifications require devices to implement custom PCIe capabilities, which
> are not currently supported by either PCIe controllers/drivers or the PCIe
> endpoint framework.
s/implementation PCIe endpoint function/implementation of PCIe endpoint
functions/
s/lecacy/legacy/ (What does "legacy" mean? Is there a spec for this?)
I guess "legacy virtio" devices need not implement custom PCIe
capabilities, but "modern virtio" devices must implement them?
Capitalize "Endpoint framework" consistently; sometimes it's
"Endpoint", other times it's "endpoint".
> While this patch provides functions for negotiating with host drivers and
> copying data, each PCIe function driver must impl ement operations that
> depend on each specific device, such as network, block, etc.
s/impl ement/implement/
> +#include
> +#include
> +#include
Typically the header includes would be alphabetized if possible.
> + vq_virt = pci_epc_map_addr(epf->epc, epf->func_no, epf->vfunc_no,
> +vq_pci_addr, vq_phys, vq_size);
> + if (IS_ERR(vq_virt)) {
> + pr_err("Failed to map virtuqueue to local");
s/virtuqueue/virtqueue/
I know you probably don't have any way to use dev_err(), but this
message really needs some context, like a driver name and instance or
something.
> +#define VIRTIO_PCI_LEGACY_CFG_BAR 0
What makes this a "legacy cfg BAR"? Is there some spec that covers
virtio BAR usage?
> + * epf_virtio_init - initialize struct epf_virtio and setup BAR for virtio
> + * @evio: struct epf_virtio to initialize.
> + * @hdr: pci configuration space to show remote host.
> + * @bar_size: pci BAR size it depends on the virtio device type.
s/pci/PCI/ (there were also a few more instances above in messages or
comments)
> + * epf_virtio_final - finalize struct epf_virtio. it frees bar and memories
> + * @evio: struct epf_virtio to finalize.
s/bar/BAR/
> +static void epf_virtio_monitor_qnotify(struct epf_virtio *evio)
> +{
> + const u16 qn_default = evio->nvq;
> + u16 tmp;
> +
> + /* Since there is no way to synchronize between the host and EP
> functions,
> + * it is possible to miss multiple notifications.
Multi-line comment style.
> + err = epf_virtio_negotiate_vq(evio);
> + if (err < 0) {
> + pr_err("failed to negoticate configs with driver\n");
s/negoticate/negotiate/
> + * epf_virtio_reset - reset virtio status
Some of the function descriptions end with a period (".") and others
don't. Please figure out what the most common style is and use that
consistently.
> + dst = pci_epc_map_addr(epf->epc, epf->func_no,
> +epf->vfunc_no, dbase, ,
> +slen);
> + if (IS_ERR(dst)) {
> + pr_err("failed to map pci mmoery spact to
> local\n");
s/pci/PCI/
s/mmoery/memory/
s/spact/space/ ?
Also below.
IIRC some previous messages started with a capital letter. Please
make them all consistent.
> + if (dir == DMA_MEM_TO_DEV) {
> + pci_epc_unmap_addr(epf->epc, epf->func_no,
> +epf->vfunc_no, phys, dst, slen);
> + } else {
> + pci_epc_unmap_addr(epf->epc, epf->func_no,
> +epf->vfunc_no, phys, src, slen);
> + }
> + }
> +
> + return 1;
I guess this function returns either a negative error code or the
value 1? That seems sort of weird (I think "negative error code or
*zero* is more typical), but maybe you're following some convention?
> +#include
> +#include
> +#include
> +#include
Alpha order if possible
> + /* Virtual address of pci configuration space */
s/pci/PCI/
> + /* Callback function and parameter for queue notifcation
> + * Note: PCI EP function cannot detect qnotify accurately, therefore
> this
> + * callback function should check all of virtqueue's changes.
> + */
Multi-line comment style.
Bjorn
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RFC PATCH v2 3/3] PCI: endpoint: Add EP function driver to provide virtio-console functionality
Random typos and trivial things. No need to repost until somebody
does a more substantive review.
On Thu, Apr 27, 2023 at 07:44:28PM +0900, Shunsuke Mie wrote:
> Add a new PCIe endpoint function driver that works as a pci virtio-console
> device. The console connect to endpoint side console. It enables to
> communicate PCIe host and endpoint.
s/pci/PCI/
> Architecture is following:
>
> ┌┐ ┌──┬┐
> │virtioe │ │ │virtio │
> │console drv │ ├───┐ │console drv │
> ├┤ │(virtio console│ ├┤
> │ virtio bus │ │ device) │◄►│ virtio bus │
> ├┤ ├---┤ └┤
> ││ │ pci ep virtio │ │
> │ pci bus │ │ console drv │ │
> ││ pcie ├───┤ │
> ││ ◄─► │ pci ep Bus │ │
> └┘ └───┴───┘
>PCIe Root PCIe Endpoint
s/virtioe/virtio/
s/pci/PCI/
s/pcie/PCIe/
s/ep/EP/
> +config PCI_EPF_VCON
> + tristate "PCI Endpoint virito-console driver"
s/virito/virtio/
> + depends on PCI_ENDPOINT
> + select VHOST_RING
> + select PCI_EPF_VIRTIO
> + help
> + PCIe Endpoint virtio-console function implementatino. This module
> + enables to show the virtio-console as pci device to PCIe host side,
> and
> + another virtual virtio-console device registers to endpoint system.
> + Those devices are connected virtually and can communicate each other.
s/implementatino/implementation/
s/pci/PCI/
s/can communicate/can communicate with/
> + * PCI Endpoint function driver to impliment virtio-console device
> + * functionality.
s/impliment/implement/
> +static int virtio_queue_size = 0x100;
> +module_param(virtio_queue_size, int, 0444);
> +MODULE_PARM_DESC(virtio_queue_size, "A length of virtqueue");
When and why would users need this parameter? Where is it documented?
> + /* To access virtqueus of local host driver */
s/virtqueus/virtqueues/
> + /* To show a status whether this driver is ready and the remote is
> connected */
Make this fit in 80 columns.
> + /* This is a minimum implementation. Doesn't support any features of
> + * virtio console. It means driver and device use just 2 virtuque for tx
> + * and rx.
> + */
Use common multi-line comment style:
/*
* This is ...
*/
s/virtuque/virtqueues/
> +static void epf_vcon_raise_irq_handler(struct work_struct *work)
> +{
> + struct epf_vcon *vcon =
> + container_of(work, struct epf_vcon, raise_irq_work);
Rewrap.
> +static int epf_vcon_setup_common(struct epf_vcon *vcon)
> +{
> + vcon->features = 0;
> + vcon->connected = false;
> +
> + vcon->task_wq =
> + alloc_workqueue("pci-epf-vcon/task-wq",
Looks like this would fit on the previous line?
> + WQ_MEM_RECLAIM | WQ_HIGHPRI | WQ_UNBOUND, 0);
> +static void epf_vcon_initialize_complete(void *param)
> +{
> + struct epf_vcon *vcon = param;
> +
> + pr_debug("Remote host has connected\n");
Is there any device info you could include here, e.g., with dev_dbg()?
It's nice if users have a little context.
> +static int epf_vcon_setup_ep_func(struct epf_vcon *vcon, struct pci_epf *epf)
> +{
> + int err;
> + struct epf_virtio *evio = >evio;
> + unsigned int nvq = epf_vcon_get_nvq(vcon);
> +
> + vcon->rdev_iovs =
> + kmalloc_array(nvq, sizeof(vcon->rdev_iovs[0]), GFP_KERNEL);
Move the function name and as many parameters as fit in 80 columns to
the previous line to match prevailing style.
> + /* There is no config for virtio console because this console device
> + * doesn't any support features
> + */
Multi-line comment style.
s/doesn't any support/doesn't support any/? Is that what you mean?
> + /* Do nothing because this console device doesn't any support features
> */
Same.
> +static void epf_vcon_vdev_set_status(struct virtio_device *vdev, u8 status)
> +{
> + if (status & VIRTIO_CONFIG_S_FAILED)
> + pr_debug("driver failed to setup this device\n");
dev_dbg() if possible.
> + err = vringh_init_kern(>vdev_vrhs[i], vcon->features,
> +virtio_queue_size, false, vring->desc,
> +vring->avail, vring->used);
> + if (err) {
> + pr_err("failed to init vringh for vring %d\n", i);
dev_err() if possible.
> +static int epf_vcon_setup_vdev(struct epf_vcon *vcon, struct device *parent)
> +{
> + int err;
> + struct virtio_device *vdev = >vdev;
> + const unsigned int nvq = epf_vcon_get_nvq(vcon);
> +
> + vcon->vdev_vrhs =
> + kmalloc_array(nvq,
Re: [RFC PATCH v2 0/3] Introduce a PCIe endpoint virtio console
On Thu, Apr 27, 2023 at 07:44:25PM +0900, Shunsuke Mie wrote:
> ...
> PCI: endpoint: introduce a helper to implement pci ep virtio function
> virtio_pci: add a definition of queue flag in ISR
> PCI: endpoint: Add EP function driver to provide virtio-console
> functionality
Capitalize the first word consistently to match history ("Introduce",
"Add").
Capitalize "PCI" in English text.
Capitalize "EP" since it's sort of an acronym (you did once, but do it
both places :))
Bjorn
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v9 0/3] virtio: vdpa: new SolidNET DPU driver
On Tue, Jan 10, 2023 at 06:56:35PM +0200, Alvaro Karsz wrote: > This series adds a vDPA driver for SolidNET DPU. > ... > v9: > - Indent PCI id in pci_ids.h with tabs - Patch 1. > - Wrap patch comment log to fill 75 columns - Patch 1 + 2. Beautiful, nice threading, thanks! :) ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v8 1/3] PCI: Add SolidRun vendor ID
On Tue, Jan 10, 2023 at 05:46:37PM +0200, Alvaro Karsz wrote: > > This should be indented with tabs instead of spaces so it matches the > > rest of the file. > > Michael, sorry about all the versions.. > Do you want me to fix it in a new version? > I could fix it with a patch directly to the pci maintainers after your > linux-next is merged, if this is ok with everyone. It's not worth merging this patch and then adding another patch on top just to convert spaces to tabs. > > It's helpful if you can send the patches in a series so the individual > > patches are replies to the cover letter. That makes tools and > > archives work better: > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/5.Posting.rst?id=v6.1#n342 > > Yes, I fixed it in the next version: > https://lists.linuxfoundation.org/pipermail/virtualization/2023-January/064190.html It doesn't look fixed to me. The "lore" archive is better than pipermail, and the cover letter doesn't show any replies: https://lore.kernel.org/linux-pci/20230109080429.1155046-1-alvaro.ka...@solid-run.com/ If you look at https://lore.kernel.org/linux-pci/, you'll see the typical thread structure with patches nested under the cover letter. The patches have "In-Reply-To" headers that reference the cover letter. Bjorn ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v8 1/3] PCI: Add SolidRun vendor ID
On Mon, Jan 09, 2023 at 10:04:53AM +0200, Alvaro Karsz wrote: > Add SolidRun vendor ID to pci_ids.h > > The vendor ID is used in 2 different source files, > the SNET vDPA driver and PCI quirks. Nits: wrap to fill 75 columns. > Signed-off-by: Alvaro Karsz > Acked-by: Bjorn Helgaas > --- > include/linux/pci_ids.h | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h > index b362d90eb9b..9a3102e61db 100644 > --- a/include/linux/pci_ids.h > +++ b/include/linux/pci_ids.h > @@ -3092,6 +3092,8 @@ > > #define PCI_VENDOR_ID_3COM_2 0xa727 > > +#define PCI_VENDOR_ID_SOLIDRUN 0xd063 This should be indented with tabs instead of spaces so it matches the rest of the file. It's helpful if you can send the patches in a series so the individual patches are replies to the cover letter. That makes tools and archives work better: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/5.Posting.rst?id=v6.1#n342 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RESEND PATCH 1/3] Add SolidRun vendor id
On Thu, Dec 29, 2022 at 11:06:02PM +0200, Alvaro Karsz wrote: > > On Mon, Dec 19, 2022 at 10:35:09AM +0200, Alvaro Karsz wrote: > > > The vendor id is used in 2 differrent source files, > > > the SNET vdpa driver and pci quirks. > > > > s/id/ID/ # both in subject and commit log > > s/differrent/different/ > > s/vdpa/vDPA/ # seems to be the conventional style > > s/pci/PCI/ > > > > Make the commit log say what this patch does. > > > > > Signed-off-by: Alvaro Karsz > > > > With the above and the sorting fix below: > > > > Acked-by: Bjorn Helgaas > > > > > --- > > > include/linux/pci_ids.h | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h > > > index b362d90eb9b..33bbe3160b4 100644 > > > --- a/include/linux/pci_ids.h > > > +++ b/include/linux/pci_ids.h > > > @@ -3115,4 +3115,6 @@ > > > > > > #define PCI_VENDOR_ID_NCUBE 0x10ff > > > > > > +#define PCI_VENDOR_ID_SOLIDRUN 0xd063 > > > > Move this to the right spot so the file is sorted by vendor ID. > > PCI_VENDOR_ID_NCUBE, PCI_VENDOR_ID_OCZ, and PCI_VENDOR_ID_XEN got > > added in the wrong place. > > > > > #endif /* _LINUX_PCI_IDS_H */ > > > -- > > Thanks for your comments. > > The patch was taken by another maintainer (CCed) > https://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git/commit/?h=linux-next=afc9dcfb846bf35aa7afb160d5370ab5c75e7a70 > > So, Michael and Bjorn, > Do you want me to create a new version, or fix it in a follow up patch? > > BTW, the same is true for the next patch in the series, New PCI quirk > for SolidRun SNET DPU > https://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git/commit/?h=linux-next=136dd8d8f3a0ac19f75a875e9b27b83d365a5be3 I don't know how Michael runs his tree, so it's up to him, but "New PCI quirk for SolidRun SNET DPU." is completely different from all the history and not very informative, so if it were via my tree I would definitely update both. Bjorn ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RESEND PATCH 1/3] Add SolidRun vendor id
Hi Alvaro, On Mon, Dec 19, 2022 at 10:35:09AM +0200, Alvaro Karsz wrote: > The vendor id is used in 2 differrent source files, > the SNET vdpa driver and pci quirks. s/id/ID/ # both in subject and commit log s/differrent/different/ s/vdpa/vDPA/ # seems to be the conventional style s/pci/PCI/ Make the commit log say what this patch does. > Signed-off-by: Alvaro Karsz With the above and the sorting fix below: Acked-by: Bjorn Helgaas > --- > include/linux/pci_ids.h | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h > index b362d90eb9b..33bbe3160b4 100644 > --- a/include/linux/pci_ids.h > +++ b/include/linux/pci_ids.h > @@ -3115,4 +3115,6 @@ > > #define PCI_VENDOR_ID_NCUBE 0x10ff > > +#define PCI_VENDOR_ID_SOLIDRUN 0xd063 Move this to the right spot so the file is sorted by vendor ID. PCI_VENDOR_ID_NCUBE, PCI_VENDOR_ID_OCZ, and PCI_VENDOR_ID_XEN got added in the wrong place. > #endif /* _LINUX_PCI_IDS_H */ > -- > 2.32.0 > > ___ > Virtualization mailing list > Virtualization@lists.linux-foundation.org > https://lists.linuxfoundation.org/mailman/listinfo/virtualization ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RESEND PATCH 2/3] New PCI quirk for SolidRun SNET DPU.
Hi Alvaro,
Thanks for the patch!
On Mon, Dec 19, 2022 at 10:35:10AM +0200, Alvaro Karsz wrote:
> The DPU advertises FLR, but it may cause the device to hang.
> This only happens with revision 0x1.
Please update the subject line to:
PCI: Avoid FLR for SolidRun SNET DPU rev 1
This makes the subject meaningful by itself and is similar to previous
quirks:
5727043c73fd ("PCI: Avoid FLR for AMD Starship USB 3.0")
0d14f06cd665 ("PCI: Avoid FLR for AMD Matisse HD Audio & USB 3.0")
f65fd1aa4f98 ("PCI: Avoid FLR for Intel 82579 NICs")
Also, update the commit log so it says what this patch does, instead
of simply describing the current situation.
https://chris.beams.io/posts/git-commit/ is a good reference.
With the above changes,
Acked-by: Bjorn Helgaas
> Signed-off-by: Alvaro Karsz
> ---
> drivers/pci/quirks.c | 8
> 1 file changed, 8 insertions(+)
>
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 285acc4aacc..809d03272c2 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -5343,6 +5343,14 @@ DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_AMD, 0x149c,
> quirk_no_flr);
> DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1502, quirk_no_flr);
> DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1503, quirk_no_flr);
>
> +/* FLR may cause the SolidRun SNET DPU (rev 0x1) to hang */
> +static void quirk_no_flr_snet(struct pci_dev *dev)
> +{
> + if (dev->revision == 0x1)
> + quirk_no_flr(dev);
> +}
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_SOLIDRUN, 0x1000, quirk_no_flr_snet);
> +
> static void quirk_no_ext_tags(struct pci_dev *pdev)
> {
> struct pci_host_bridge *bridge = pci_find_host_bridge(pdev->bus);
> --
> 2.32.0
>
> ___
> Virtualization mailing list
> Virtualization@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/1] virtio_pci: use common helper to configure SR-IOV
On Wed, Oct 12, 2022 at 5:01 AM Max Gurtovoy wrote:
>
>
> On 10/12/2022 11:42 AM, Max Gurtovoy wrote:
> >
> > On 10/12/2022 8:02 AM, Michael S. Tsirkin wrote:
> >> On Thu, Sep 29, 2022 at 02:40:08AM +0300, Max Gurtovoy wrote:
> >>> This is instead of re-writing the same logic in virtio driver.
> >>>
> >>> Signed-off-by: Max Gurtovoy
> >> Dropped this as it caused build failures:
> >>
> >> https://lore.kernel.org/r/202210080424.gSmuYfb0-lkp%40intel.com
> >
> > maybe you can re-run it with:
> >
> > diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile
> > index 8e98d24917cc..b383326a20e2 100644
> > --- a/drivers/virtio/Makefile
> > +++ b/drivers/virtio/Makefile
> > @@ -5,10 +5,11 @@ obj-$(CONFIG_VIRTIO_PCI_LIB) += virtio_pci_modern_dev.o
> > obj-$(CONFIG_VIRTIO_PCI_LIB_LEGACY) += virtio_pci_legacy_dev.o
> > obj-$(CONFIG_VIRTIO_MMIO) += virtio_mmio.o
> > obj-$(CONFIG_VIRTIO_PCI) += virtio_pci.o
> > -virtio_pci-y := virtio_pci_modern.o virtio_pci_common.o
> > -virtio_pci-$(CONFIG_VIRTIO_PCI_LEGACY) += virtio_pci_legacy.o
> > obj-$(CONFIG_VIRTIO_BALLOON) += virtio_balloon.o
> > obj-$(CONFIG_VIRTIO_INPUT) += virtio_input.o
> > obj-$(CONFIG_VIRTIO_VDPA) += virtio_vdpa.o
> > obj-$(CONFIG_VIRTIO_MEM) += virtio_mem.o
> > obj-$(CONFIG_VIRTIO_DMA_SHARED_BUFFER) += virtio_dma_buf.o
> > +
> > +virtio_pci-$(CONFIG_VIRTIO_PCI) := virtio_pci_modern.o
> > virtio_pci_common.o
> > +virtio_pci-$(CONFIG_VIRTIO_PCI_LEGACY) += virtio_pci_legacy.o
> >
>
> Now I saw that CONFIG_PCI_IOV is not set in the error log so the bellow
> should fix it:
>
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 060af91bafcd..c519220e8ff8 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -2228,7 +2228,10 @@ static inline int pci_sriov_set_totalvfs(struct
> pci_dev *dev, u16 numvfs)
> { return 0; }
> static inline int pci_sriov_get_totalvfs(struct pci_dev *dev)
> { return 0; }
> -#define pci_sriov_configure_simple NULL
> +static inline int pci_sriov_configure_simple(struct pci_dev *dev, int
> nr_virtfn)
> +{
> + return -ENOSYS;
> +}
> static inline resource_size_t pci_iov_resource_size(struct pci_dev
> *dev, int resno)
> { return 0; }
> static inline void pci_vf_drivers_autoprobe(struct pci_dev *dev, bool
> probe) { }
>
> Bjorn,
>
> WDYT about the above ?
>
> should I send it to the pci subsystem list ?
Yes. I don't apply things that haven't appeared on linux-...@vger.kernel.org.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2] jailhouse: Hold reference returned from of_find_xxx API
On Fri, Sep 16, 2022 at 10:25:31PM -0700, Srivatsa S. Bhat wrote:
> [ Adding author and reviewers of commit 63338a38db95 again ]
>
> On 9/16/22 2:00 AM, Liang He wrote:
> > In jailhouse_paravirt(), we should hold the reference returned from
> > of_find_compatible_node() which has increased the refcount and then
> > call of_node_put() with it when done.
> >
> > Fixes: 63338a38db95 ("jailhouse: Provide detection for non-x86 systems")
> > Signed-off-by: Liang He
> > Co-developed-by: Kelin Wang
> > Signed-off-by: Kelin Wang
>
> Reviewed-by: Srivatsa S. Bhat (VMware)
The message to which you are responding didn't make it to the mailing
list, so it's unlikely that anybody will pick it up. See the archive:
https://lore.kernel.org/all/0069849b-e6c7-5c9b-4b52-5aa6e4a32...@csail.mit.edu/
Maybe it was a multipart message or was HTML, which the mailing lists
reject: http://vger.kernel.org/majordomo-info.html
Bjorn
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH net-next v2 0/2] sfc: Add EF100 BAR config support
On Mon, Jul 11, 2022 at 02:38:54PM +0100, Martin Habets wrote: > On Thu, Jul 07, 2022 at 10:55:00AM -0500, Bjorn Helgaas wrote: > > On Thu, Jul 07, 2022 at 02:07:07PM +0100, Martin Habets wrote: > > > The EF100 NICs allow for different register layouts of a PCI memory BAR. > > > This series provides the framework to switch this layout at runtime. > > > > > > Subsequent patch series will use this to add support for vDPA. > > > > Normally drivers rely on the PCI Vendor and Device ID to learn the > > number of BARs and their layouts. I guess this series implies that > > doesn't work on this device? And the user needs to manually specify > > what kind of device this is? > > When a new PCI device is added (like a VF) it always starts of with > the register layout for an EF100 network device. This is hardcoded, > i.e. it cannot be customised. > The layout can be changed after bootup, and only after the sfc driver has > bound to the device. > The PCI Vendor and Device ID do not change when the layout is changed. > > For vDPA specifically we return the Xilinx PCI Vendor and our device ID > to the vDPA framework via struct vdpa_config_opts. > > > I'm confused about how this is supposed to work. What if the driver > > is built-in and claims a device before the user can specify the > > register layout? > > The bar_config file will only exist once the sfc driver has bound to > the device. So in fact we count on that driver getting loaded. > When a new value is written to bar_config it is the sfc driver that > instructs the NIC to change the register layout. > > > What if the user specifies the wrong layout and the > > driver writes to the wrong registers? > > We have specific hardware and driver requirements for this sort of > situation. For example, the register layouts must have some common > registers (to ensure some compatibility). Obviously we have to deal with the hardware as it exists, but it seems like a hardware design problem that you can change the register layout but the change is not detectable via those common registers. Anyway, it seems weird to me, but doesn't affect the PCI core and I won't stand in your way ;) > A layout that is too different will require a separate device ID. > A driver that writes to the wrong register is a bug. > > Maybe the name "bar_config" is causing most of the confusion here. > Internally we also talk about "function profiles" or "personalities", > but we thought such a name would be too vague. > > Martin > > > > --- > > > > > > Martin Habets (2): > > > sfc: Add EF100 BAR config support > > > sfc: Implement change of BAR configuration > > > > > > > > > drivers/net/ethernet/sfc/ef100_nic.c | 80 > > > ++ > > > drivers/net/ethernet/sfc/ef100_nic.h |6 +++ > > > 2 files changed, 86 insertions(+) > > > > > > -- > > > Martin Habets ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH net-next v2 0/2] sfc: Add EF100 BAR config support
On Thu, Jul 07, 2022 at 02:07:07PM +0100, Martin Habets wrote: > The EF100 NICs allow for different register layouts of a PCI memory BAR. > This series provides the framework to switch this layout at runtime. > > Subsequent patch series will use this to add support for vDPA. Normally drivers rely on the PCI Vendor and Device ID to learn the number of BARs and their layouts. I guess this series implies that doesn't work on this device? And the user needs to manually specify what kind of device this is? I'm confused about how this is supposed to work. What if the driver is built-in and claims a device before the user can specify the register layout? What if the user specifies the wrong layout and the driver writes to the wrong registers? > --- > > Martin Habets (2): > sfc: Add EF100 BAR config support > sfc: Implement change of BAR configuration > > > drivers/net/ethernet/sfc/ef100_nic.c | 80 > ++ > drivers/net/ethernet/sfc/ef100_nic.h |6 +++ > 2 files changed, 86 insertions(+) > > -- > Martin Habets > > ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH RESEND v5 1/4] PCI: Clean up pci_scan_slot()
On Thu, May 12, 2022 at 04:56:42PM +0200, Niklas Schnelle wrote:
> On Thu, 2022-05-05 at 10:38 +0200, Niklas Schnelle wrote:
> > While determining the next PCI function is factored out of
> > pci_scan_slot() into next_fn() the former still handles the first
> > function as a special case. This duplicates the code from the scan loop.
> >
> > Furthermore the non ARI branch of next_fn() is generally hard to
> > understand and especially the check for multifunction devices is hidden
> > in the handling of NULL devices for non-contiguous multifunction. It
> > also signals that no further functions need to be scanned by returning
> > 0 via wraparound and this is a valid function number.
> >
> > Improve upon this by transforming the conditions in next_fn() to be
> > easier to understand.
> >
> > By changing next_fn() to return -ENODEV instead of 0 when there is no
> > next function we can then handle the initial function inside the loop
> > and deduplicate the shared handling. This also makes it more explicit
> > that only function 0 must exist.
> >
> > No functional change is intended.
> >
> > Cc: Jan Kiszka
> > Signed-off-by: Niklas Schnelle
> > ---
>
> Friendly ping :-)
Thanks and sorry for the delay. I'm off today for my daughter's
wedding reception but will get back to it next week. Just to expose
some of my thought process (and not to request more work from you!)
I've been wondering whether b1bd58e448f2 ("PCI: Consolidate
"next-function" functions") is really causing us more trouble than
it's worth. In some ways that makes the single next-function harder
to read. But I guess the hypervisor special case is not exactly a
"next-function" thing -- it's a "keep scanning even if there's no fn
0" thing.
Bjorn
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 1/4] PCI: Clean up pci_scan_slot()
On Thu, Apr 21, 2022 at 11:27:42AM +0200, Niklas Schnelle wrote:
> On Wed, 2022-04-20 at 21:14 -0500, Bjorn Helgaas wrote:
> > On Tue, Apr 19, 2022 at 12:28:00PM +0200, Niklas Schnelle wrote:
> > > While determining the next PCI function is factored out of
> > > pci_scan_slot() into next_fn() the former still handles the first
> > > function as a special case duplicating the code from the scan loop and
> > > splitting the condition that the first function exits from it being
> > > multifunction which is tested in next_fn().
> > >
> > > Furthermore the non ARI branch of next_fn() mixes the case that
> > > multifunction devices may have non-contiguous function ranges and dev
> > > may thus be NULL with the multifunction requirement. It also signals
> > > that no further functions need to be scanned by returning 0 which is
> > > a valid function number.
> > >
> > > Improve upon this by moving all conditions for having to scan for more
> > > functions into next_fn() and make them obvious and commented.
> > >
> > > By changing next_fn() to return -ENODEV instead of 0 when there is no
> > > next function we can then handle the initial function inside the loop
> > > and deduplicate the shared handling.
> > >
> > > No functional change is intended.
> > >
> > > Signed-off-by: Niklas Schnelle
> > > ---
> > > drivers/pci/probe.c | 41 +++--
> > > 1 file changed, 19 insertions(+), 22 deletions(-)
> > >
> > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > > index 17a969942d37..389aa1f9cb2c 100644
> > > --- a/drivers/pci/probe.c
> > > +++ b/drivers/pci/probe.c
> > > @@ -2579,33 +2579,35 @@ struct pci_dev *pci_scan_single_device(struct
> > > pci_bus *bus, int devfn)
> > > }
> > > EXPORT_SYMBOL(pci_scan_single_device);
> > >
> > > -static unsigned int next_fn(struct pci_bus *bus, struct pci_dev *dev,
> > > - unsigned int fn)
> > > +static int next_fn(struct pci_bus *bus, struct pci_dev *dev, int fn)
> > > {
> > > int pos;
> > > u16 cap = 0;
> > > unsigned int next_fn;
> > >
> > > - if (pci_ari_enabled(bus)) {
> > > - if (!dev)
> > > - return 0;
> > > + if (dev && pci_ari_enabled(bus)) {
> >
> > I think this would be easier to verify if we kept the explicit error
> > return, e.g.,
> >
> > if (pci_ari_enabled(bus)) {
> > if (!dev)
> > return -ENODEV;
> > pos = pci_find_ext_capability(...);
> >
> > Otherwise we have to sort through the !dev cases below. I guess
> > -ENODEV would come from either the "!fn && !dev" case or the "fn > 6"
> > case, but it's not obvious to me that those are equivalent to the
> > previous code.
>
> We could keep this the same for this patch but I think for jailhouse
> (patch 2) we need the "!dev" case not to fail here such that we can
> handle the missing function 0 below even if ARI is enabled. For s390
> this doesn't currently matter because pci_ari_enabled(bus) is always
> false but I assumed that this isn't necessarily so for jailhouse. I
> sent a follow up mail on a slight behavior change I can think of for
> this case for v2 but forgot to send it also for v3. Quoted below:
I think it would be good to make the first patch change as little as
possible to make it easier to analyze, then possibly test for
hypervisor when changing this behavior.
> > > - /* dev may be NULL for non-contiguous multifunction devices */
> > > - if (!dev || dev->multifunction)
> > > - return (fn + 1) % 8;
> > > -
> > > - return 0;
> > > + /* only multifunction devices may have more functions */
> > > + if (dev && !dev->multifunction)
> > > + return -ENODEV;
> >
> > I don't understand why the "!dev || dev->multifunction" test needs to
> > change. Isn't that valid even in the hypervisor case? IIUC, you want
> > to return success in some cases that currently return failure, so this
> > case that was already success should be fine as it was.
>
> This isn't a change to the test. It's the negation of the logical
> condition *and* a switch of the branches i.e. keeps the overall
> behavior exactly the same. The equivalence is !(!A || B) == (A && !B).
I see the Boolean equivalence, but it's difficult to verify that the
consequences are equivalent because
Re: [PATCH v3 1/4] PCI: Clean up pci_scan_slot()
Hi Niklas,
I'm sure this makes good sense, but I need a little more hand-holding.
Sorry this is long and rambling.
On Tue, Apr 19, 2022 at 12:28:00PM +0200, Niklas Schnelle wrote:
> While determining the next PCI function is factored out of
> pci_scan_slot() into next_fn() the former still handles the first
> function as a special case duplicating the code from the scan loop and
> splitting the condition that the first function exits from it being
> multifunction which is tested in next_fn().
>
> Furthermore the non ARI branch of next_fn() mixes the case that
> multifunction devices may have non-contiguous function ranges and dev
> may thus be NULL with the multifunction requirement. It also signals
> that no further functions need to be scanned by returning 0 which is
> a valid function number.
>
> Improve upon this by moving all conditions for having to scan for more
> functions into next_fn() and make them obvious and commented.
>
> By changing next_fn() to return -ENODEV instead of 0 when there is no
> next function we can then handle the initial function inside the loop
> and deduplicate the shared handling.
>
> No functional change is intended.
>
> Signed-off-by: Niklas Schnelle
> ---
> drivers/pci/probe.c | 41 +++--
> 1 file changed, 19 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 17a969942d37..389aa1f9cb2c 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -2579,33 +2579,35 @@ struct pci_dev *pci_scan_single_device(struct pci_bus
> *bus, int devfn)
> }
> EXPORT_SYMBOL(pci_scan_single_device);
>
> -static unsigned int next_fn(struct pci_bus *bus, struct pci_dev *dev,
> - unsigned int fn)
> +static int next_fn(struct pci_bus *bus, struct pci_dev *dev, int fn)
> {
> int pos;
> u16 cap = 0;
> unsigned int next_fn;
>
> - if (pci_ari_enabled(bus)) {
> - if (!dev)
> - return 0;
> + if (dev && pci_ari_enabled(bus)) {
I think this would be easier to verify if we kept the explicit error
return, e.g.,
if (pci_ari_enabled(bus)) {
if (!dev)
return -ENODEV;
pos = pci_find_ext_capability(...);
Otherwise we have to sort through the !dev cases below. I guess
-ENODEV would come from either the "!fn && !dev" case or the "fn > 6"
case, but it's not obvious to me that those are equivalent to the
previous code.
> pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ARI);
> if (!pos)
> - return 0;
> + return -ENODEV;
>
> pci_read_config_word(dev, pos + PCI_ARI_CAP, );
> next_fn = PCI_ARI_CAP_NFN(cap);
> if (next_fn <= fn)
> - return 0; /* protect against malformed list */
> + return -ENODEV; /* protect against malformed list */
>
> return next_fn;
> }
>
> - /* dev may be NULL for non-contiguous multifunction devices */
> - if (!dev || dev->multifunction)
> - return (fn + 1) % 8;
> -
> - return 0;
> + /* only multifunction devices may have more functions */
> + if (dev && !dev->multifunction)
> + return -ENODEV;
I don't understand why the "!dev || dev->multifunction" test needs to
change. Isn't that valid even in the hypervisor case? IIUC, you want
to return success in some cases that currently return failure, so this
case that was already success should be fine as it was.
Is this because "(fn + 1) % 8" may be zero, which previously
terminated the loop, but now it doesn't because "fn == 0" is the
*first* execution of the loop?
If so, I wonder if we could avoid that case by adding:
if (fn >= 7)
return -ENODEV;
at the very beginning. Maybe that would allow a more trivial patch
that just changed the error return from 0 to -ENODEV, i.e., leaving
all the logic in next_fn() unchanged?
I'm wondering if this could end up like:
if (fn >= 7)
return -ENODEV;
if (pci_ari_enabled(bus)) {
if (!dev)
return -ENODEV;
...
return next_fn;
}
if (!dev || dev->multifunction)
return (fn + 1) % 8;
+ if (hypervisor_isolated_pci_functions())
+return (fn + 1) % 8;
return -ENODEV;
(The hypervisor part being added in a subsequent patch, and I'm not
sure exactly what logic you need there -- the point being that it's
just an additional success case.)
The "% 8" seems possibly superfluous then, since previously that
caused a zero return that terminated the loop. If we're using -ENODEV
to terminate the loop, we probably don't care about the mod 8.
> + /*
> + * A function 0 is required but multifunction devices may
> + * be non-contiguous so dev can be NULL otherwise.
I understood the original "dev may be NULL ..." comment, but I can't
quite parse this. "dev can be NULL" for non-zero functions? That's
Re: [PATCH v3 05/11] PCI: Use driver_set_override() instead of open-coding
On Sun, Feb 27, 2022 at 02:52:08PM +0100, Krzysztof Kozlowski wrote: > Use a helper for seting driver_override to reduce amount of duplicated > code. Make the driver_override field const char, because it is not > modified by the core and it matches other subsystems. s/seting/setting/ or even better, s/for seting/to set/ > Signed-off-by: Krzysztof Kozlowski Acked-by: Bjorn Helgaas > - char*driver_override; /* Driver name to force a match */ > + /* > + * Driver name to force a match. > + * Do not set directly, because core frees it. > + * Use driver_set_override() to set or clear it. Wrap this comment to fill 78 columns or so. > + */ > + const char *driver_override; > > unsigned long priv_flags; /* Private flags for the PCI driver */ > > -- > 2.32.0 > ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 01/11] driver: platform: Add helper for safer setting of driver_override
On Sun, Feb 27, 2022 at 02:52:04PM +0100, Krzysztof Kozlowski wrote:
> Several core drivers and buses expect that driver_override is a
> dynamically allocated memory thus later they can kfree() it.
> +int driver_set_override(struct device *dev, const char **override,
> + const char *s, size_t len)
> +{
> + const char *new, *old;
> + char *cp;
> +
> + if (!dev || !override || !s)
> + return -EINVAL;
> +
> + /* We need to keep extra room for a newline */
It would help a lot to extend this comment with a hint about where the
room for a newline is needed. It was confusing even before, but it's
much more so now that the check is in a completely different file than
the "show" functions that need the space.
> + if (len >= (PAGE_SIZE - 1))
> + return -EINVAL;
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 05/11] pci: use helper for safer setting of driver_override
On Fri, Feb 25, 2022 at 10:36:20AM +0100, Krzysztof Kozlowski wrote:
> On 25/02/2022 00:52, Bjorn Helgaas wrote:
> > On Thu, Feb 24, 2022 at 08:49:15AM +0100, Krzysztof Kozlowski wrote:
> >> On 23/02/2022 22:51, Bjorn Helgaas wrote:
> >>> In subject, to match drivers/pci/ convention, do something like:
> >>>
> >>> PCI: Use driver_set_override() instead of open-coding
> >>>
> >>> On Wed, Feb 23, 2022 at 08:13:04PM +0100, Krzysztof Kozlowski wrote:
> >>>> Use a helper for seting driver_override to reduce amount of duplicated
> >>>> code.
> >>>> @@ -567,31 +567,15 @@ static ssize_t driver_override_store(struct device
> >>>> *dev,
> >>>> const char *buf, size_t count)
> >>>> {
> >>>> struct pci_dev *pdev = to_pci_dev(dev);
> >>>> -char *driver_override, *old, *cp;
> >>>> +int ret;
> >>>>
> >>>> /* We need to keep extra room for a newline */
> >>>> if (count >= (PAGE_SIZE - 1))
> >>>> return -EINVAL;
> >>>
> >>> This check makes no sense in the new function. Michael alluded to
> >>> this as well.
> >>
> >> I am not sure if I got your comment properly. You mean here:
> >> 1. Move this check to driver_set_override()?
> >> 2. Remove the check entirely?
> >
> > I was mistaken about the purpose of the comment and the check. I
> > thought it had to do with *this* function, and this function doesn't
> > add a newline, and there's no obvious connection with PAGE_SIZE.
> >
> > But looking closer, I think the "extra room for a newline" is really
> > to make sure that *driver_override_show()* can add a newline and have
> > it still fit within the PAGE_SIZE sysfs limit.
> >
> > Most driver_override_*() functions have the same comment, so maybe
> > this was obvious to everybody except me :) I do see that spi.c adds
> > "when displaying value" at the end, which helps a lot.
> >
> > Sorry for the wild goose chase.
>
> I think I will move this check anyway to driver_set_override() helper,
> because there is no particular benefit to have duplicated all over. The
> helper will receive "count" argument so can perform all checks.
Thanks, I think that would be good!
Bjorn
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 05/11] pci: use helper for safer setting of driver_override
On Thu, Feb 24, 2022 at 08:49:15AM +0100, Krzysztof Kozlowski wrote:
> On 23/02/2022 22:51, Bjorn Helgaas wrote:
> > In subject, to match drivers/pci/ convention, do something like:
> >
> > PCI: Use driver_set_override() instead of open-coding
> >
> > On Wed, Feb 23, 2022 at 08:13:04PM +0100, Krzysztof Kozlowski wrote:
> >> Use a helper for seting driver_override to reduce amount of duplicated
> >> code.
> >> @@ -567,31 +567,15 @@ static ssize_t driver_override_store(struct device
> >> *dev,
> >> const char *buf, size_t count)
> >> {
> >>struct pci_dev *pdev = to_pci_dev(dev);
> >> - char *driver_override, *old, *cp;
> >> + int ret;
> >>
> >>/* We need to keep extra room for a newline */
> >>if (count >= (PAGE_SIZE - 1))
> >>return -EINVAL;
> >
> > This check makes no sense in the new function. Michael alluded to
> > this as well.
>
> I am not sure if I got your comment properly. You mean here:
> 1. Move this check to driver_set_override()?
> 2. Remove the check entirely?
I was mistaken about the purpose of the comment and the check. I
thought it had to do with *this* function, and this function doesn't
add a newline, and there's no obvious connection with PAGE_SIZE.
But looking closer, I think the "extra room for a newline" is really
to make sure that *driver_override_show()* can add a newline and have
it still fit within the PAGE_SIZE sysfs limit.
Most driver_override_*() functions have the same comment, so maybe
this was obvious to everybody except me :) I do see that spi.c adds
"when displaying value" at the end, which helps a lot.
Sorry for the wild goose chase.
> >> - driver_override = kstrndup(buf, count, GFP_KERNEL);
> >> - if (!driver_override)
> >> - return -ENOMEM;
> >> -
> >> - cp = strchr(driver_override, '\n');
> >> - if (cp)
> >> - *cp = '\0';
> >> -
> >> - device_lock(dev);
> >> - old = pdev->driver_override;
> >> - if (strlen(driver_override)) {
> >> - pdev->driver_override = driver_override;
> >> - } else {
> >> - kfree(driver_override);
> >> - pdev->driver_override = NULL;
> >> - }
> >> - device_unlock(dev);
> >> -
> >> - kfree(old);
> >> + ret = driver_set_override(dev, >driver_override, buf);
> >> + if (ret)
> >> + return ret;
> >>
> >>return count;
> >> }
> >> --
> >> 2.32.0
> >>
> >>
> >> ___
> >> linux-arm-kernel mailing list
> >> linux-arm-ker...@lists.infradead.org
> >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
>
> Best regards,
> Krzysztof
>
> ___
> linux-arm-kernel mailing list
> linux-arm-ker...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 01/11] driver: platform: add and use helper for safer setting of driver_override
On Wed, Feb 23, 2022 at 08:13:00PM +0100, Krzysztof Kozlowski wrote:
> Several core drivers and buses expect that driver_override is a
> dynamically allocated memory thus later they can kfree() it.
> ...
> + * set_driver_override() - Helper to set or clear driver override.
Doesn't match actual function name.
> + * @dev: Device to change
> + * @override: Address of string to change (e.g. >driver_override);
> + *The contents will be freed and hold newly allocated override.
> + * @s: NULL terminated string, new driver name to force a match, pass empty
> + * string to clear it
> + *
> + * Helper to setr or clear driver override in a device, intended for the
> cases
> + * when the driver_override field is allocated by driver/bus code.
s/setr/set/
> + * Returns: 0 on success or a negative error code on failure.
> + */
> +int driver_set_override(struct device *dev, char **override, const char *s)
> +{
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 05/11] pci: use helper for safer setting of driver_override
In subject, to match drivers/pci/ convention, do something like:
PCI: Use driver_set_override() instead of open-coding
On Wed, Feb 23, 2022 at 08:13:04PM +0100, Krzysztof Kozlowski wrote:
> Use a helper for seting driver_override to reduce amount of duplicated
> code.
s/seting/setting/
> Signed-off-by: Krzysztof Kozlowski
> ---
> drivers/pci/pci-sysfs.c | 24
> 1 file changed, 4 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index 602f0fb0b007..16a163d4623e 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -567,31 +567,15 @@ static ssize_t driver_override_store(struct device *dev,
>const char *buf, size_t count)
> {
> struct pci_dev *pdev = to_pci_dev(dev);
> - char *driver_override, *old, *cp;
> + int ret;
>
> /* We need to keep extra room for a newline */
> if (count >= (PAGE_SIZE - 1))
> return -EINVAL;
This check makes no sense in the new function. Michael alluded to
this as well.
> - driver_override = kstrndup(buf, count, GFP_KERNEL);
> - if (!driver_override)
> - return -ENOMEM;
> -
> - cp = strchr(driver_override, '\n');
> - if (cp)
> - *cp = '\0';
> -
> - device_lock(dev);
> - old = pdev->driver_override;
> - if (strlen(driver_override)) {
> - pdev->driver_override = driver_override;
> - } else {
> - kfree(driver_override);
> - pdev->driver_override = NULL;
> - }
> - device_unlock(dev);
> -
> - kfree(old);
> + ret = driver_set_override(dev, >driver_override, buf);
> + if (ret)
> + return ret;
>
> return count;
> }
> --
> 2.32.0
>
>
> ___
> linux-arm-kernel mailing list
> linux-arm-ker...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v4 11/15] pci: Add pci_iomap_shared{,_range}
On Tue, Aug 24, 2021 at 01:50:00PM -0700, Andi Kleen wrote: > > On 8/24/2021 1:31 PM, Bjorn Helgaas wrote: > > On Tue, Aug 24, 2021 at 01:14:02PM -0700, Andi Kleen wrote: > > > On 8/24/2021 11:55 AM, Bjorn Helgaas wrote: > > > > [+cc Rajat; I still don't know what "shared memory with a hypervisor > > > > in a confidential guest" means, > > > A confidential guest is a guest which uses memory encryption to isolate > > > itself from the host. It doesn't trust the host. But it still needs to > > > communicate with the host for IO, so it has some special memory areas that > > > are explicitly marked shared. These are used to do IO with the host. All > > > their usage needs to be carefully hardened to avoid any security attacks > > > on > > > the guest, that's why we want to limit this interaction only to a small > > > set > > > of hardened drivers. For MMIO, the set is currently only virtio and MSI-X. > > Good material for the commit log next time around. Thanks! > > This is all in the patch intro too, which should make it into the merge > commits. It's good if the cover letter makes into the merge commit log. It's probably just because my git foo is lacking, but merge commit logs don't seem as discoverable as the actual patch commit logs. Five years from now, if I want to learn about pci_iomap_shared() history, I would "git log -p lib/pci_iomap.c" and search for it. But I don't think I would see the merge commit then. Bjorn ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v4 11/15] pci: Add pci_iomap_shared{,_range}
On Tue, Aug 24, 2021 at 01:14:02PM -0700, Andi Kleen wrote: > > On 8/24/2021 11:55 AM, Bjorn Helgaas wrote: > > [+cc Rajat; I still don't know what "shared memory with a hypervisor > > in a confidential guest" means, > > A confidential guest is a guest which uses memory encryption to isolate > itself from the host. It doesn't trust the host. But it still needs to > communicate with the host for IO, so it has some special memory areas that > are explicitly marked shared. These are used to do IO with the host. All > their usage needs to be carefully hardened to avoid any security attacks on > the guest, that's why we want to limit this interaction only to a small set > of hardened drivers. For MMIO, the set is currently only virtio and MSI-X. Good material for the commit log next time around. Thanks! Bjorn ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v4 11/15] pci: Add pci_iomap_shared{,_range}
[+cc Rajat; I still don't know what "shared memory with a hypervisor in a confidential guest" means, but now we're talking about hardened drivers and allow lists, which Rajat is interested in] On Tue, Aug 24, 2021 at 10:20:44AM -0700, Andi Kleen wrote: > > > I see. Hmm. It's a bit of a random thing to do it at the map time > > though. E.g. DMA is all handled transparently behind the DMA API. > > Hardening is much more than just replacing map with map_shared > > and I suspect what you will end up with is basically > > vendors replacing map with map shared to make things work > > for their users and washing their hands. > > That concept exists too. There is a separate allow list for the drivers. So > just adding shared to a driver is not enough, until it's also added to the > allowlist > > Users can of course chose to disable the allowlist, but they need to > understand the security implications. > > > I would say an explicit flag in the driver that says "hardened" > > and refusing to init a non hardened one would be better. > > We have that too (that's the device filtering) > > But the problem is that device filtering just stops the probe functions, not > the initcalls, and lot of legacy drivers do MMIO interactions before going > into probe. In some cases it's unavoidable because of the device doesn't > have a separate enumeration mechanism it needs some kind of probing to even > check for its existence And since we don't want to change all of them it's > far safer to make the ioremap opt-in. > > > -Andi > ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v4 10/15] asm/io.h: Add ioremap_shared fallback
On Wed, Aug 04, 2021 at 05:52:13PM -0700, Kuppuswamy Sathyanarayanan wrote:
> From: Andi Kleen
>
> This function is for declaring memory that should be shared with
> a hypervisor in a confidential guest. If the architecture doesn't
> implement it it's just ioremap.
I would assume ioremap_shared() would "map" something, not "declare"
it.
> Signed-off-by: Andi Kleen
> Signed-off-by: Kuppuswamy Sathyanarayanan
>
> ---
> arch/alpha/include/asm/io.h| 1 +
> arch/mips/include/asm/io.h | 1 +
> arch/parisc/include/asm/io.h | 1 +
> arch/sparc/include/asm/io_64.h | 1 +
> include/asm-generic/io.h | 4
> 5 files changed, 8 insertions(+)
>
> diff --git a/arch/alpha/include/asm/io.h b/arch/alpha/include/asm/io.h
> index 0fab5ac90775..701b44909b94 100644
> --- a/arch/alpha/include/asm/io.h
> +++ b/arch/alpha/include/asm/io.h
> @@ -283,6 +283,7 @@ static inline void __iomem *ioremap(unsigned long port,
> unsigned long size)
> }
>
> #define ioremap_wc ioremap
> +#define ioremap_shared ioremap
> #define ioremap_uc ioremap
>
> static inline void iounmap(volatile void __iomem *addr)
> diff --git a/arch/mips/include/asm/io.h b/arch/mips/include/asm/io.h
> index 6f5c86d2bab4..3713ff624632 100644
> --- a/arch/mips/include/asm/io.h
> +++ b/arch/mips/include/asm/io.h
> @@ -179,6 +179,7 @@ void iounmap(const volatile void __iomem *addr);
> #define ioremap(offset, size)
> \
> ioremap_prot((offset), (size), _CACHE_UNCACHED)
> #define ioremap_uc ioremap
> +#define ioremap_shared ioremap
>
> /*
> * ioremap_cache - map bus memory into CPU space
> diff --git a/arch/parisc/include/asm/io.h b/arch/parisc/include/asm/io.h
> index 0b5259102319..73064e152df7 100644
> --- a/arch/parisc/include/asm/io.h
> +++ b/arch/parisc/include/asm/io.h
> @@ -129,6 +129,7 @@ static inline void gsc_writeq(unsigned long long val,
> unsigned long addr)
> */
> void __iomem *ioremap(unsigned long offset, unsigned long size);
> #define ioremap_wc ioremap
> +#define ioremap_shared ioremap
> #define ioremap_uc ioremap
>
> extern void iounmap(const volatile void __iomem *addr);
> diff --git a/arch/sparc/include/asm/io_64.h b/arch/sparc/include/asm/io_64.h
> index 5ffa820dcd4d..18cc656eb712 100644
> --- a/arch/sparc/include/asm/io_64.h
> +++ b/arch/sparc/include/asm/io_64.h
> @@ -409,6 +409,7 @@ static inline void __iomem *ioremap(unsigned long offset,
> unsigned long size)
> #define ioremap_uc(X,Y) ioremap((X),(Y))
> #define ioremap_wc(X,Y) ioremap((X),(Y))
> #define ioremap_wt(X,Y) ioremap((X),(Y))
> +#define ioremap_shared(X, Y) ioremap((X), (Y))
> static inline void __iomem *ioremap_np(unsigned long offset, unsigned long
> size)
> {
> return NULL;
> diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h
> index e93375c710b9..bfcaee1691c8 100644
> --- a/include/asm-generic/io.h
> +++ b/include/asm-generic/io.h
> @@ -982,6 +982,10 @@ static inline void __iomem *ioremap(phys_addr_t addr,
> size_t size)
> #define ioremap_wt ioremap
> #endif
>
> +#ifndef ioremap_shared
> +#define ioremap_shared ioremap
> +#endif
"ioremap_shared" is a very generic term for a pretty specific thing:
"memory shared with a hypervisor in a confidential guest".
Maybe deserves a comment with at least a hint here. "Hypervisors in a
confidential guest" isn't the first thing that comes to mind when I
read "shared".
> /*
> * ioremap_uc is special in that we do require an explicit architecture
> * implementation. In general you do not want to use this function in a
> --
> 2.25.1
>
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v4 09/15] pci: Consolidate pci_iomap* and pci_iomap*wc
Is there a branch with all of this applied? I was going to apply this
to help take a look at it, but it doesn't apply to v5.14-rc1. I know
you listed some prereqs in the cover letter, but it's a fair amount of
work to sort all that out.
On Wed, Aug 04, 2021 at 05:52:12PM -0700, Kuppuswamy Sathyanarayanan wrote:
> From: Andi Kleen
If I were applying these, I would silently update the subject lines to
match previous commits. Since these will probably be merged via a
different tree, you can update if there's a v5:
PCI: Consolidate pci_iomap_range(), pci_iomap_wc_range()
Also applies to 11/15 and 12/15.
> pci_iomap* and pci_iomap*wc are currently duplicated code, except
> that the _wc variant does not support IO ports. Replace them
> with a common helper and a callback for the mapping. I used
> wrappers for the maps because some architectures implement ioremap
> and friends with macros.
Maybe spell some of this out:
pci_iomap_range() and pci_iomap_wc_range() are currently duplicated
code, ... Implement them using a common helper,
pci_iomap_range_map(), ...
Using "pci_iomap*" obscures the name and doesn't save any space.
Why is it safe to make pci_iomap_wc_range() support IO ports when it
didn't before? That might be desirable, but I think it *is* a
functional change here.
IIUC, pci_iomap_wc_range() on an IO port range previously returned
NULL, and after this patch it will work the same as pci_iomap_range(),
i.e., it will return the result of __pci_ioport_map().
> This will allow to add more variants without excessive code
> duplications. This patch should have no behavior change.
>
> Signed-off-by: Andi Kleen
> Signed-off-by: Kuppuswamy Sathyanarayanan
>
> ---
> lib/pci_iomap.c | 81 +++--
> 1 file changed, 44 insertions(+), 37 deletions(-)
>
> diff --git a/lib/pci_iomap.c b/lib/pci_iomap.c
> index 2d3eb1cb73b8..6251c3f651c6 100644
> --- a/lib/pci_iomap.c
> +++ b/lib/pci_iomap.c
> @@ -10,6 +10,46 @@
> #include
>
> #ifdef CONFIG_PCI
> +
> +/*
> + * Callback wrappers because some architectures define ioremap et.al.
> + * as macros.
> + */
> +static void __iomem *map_ioremap(phys_addr_t addr, size_t size)
> +{
> + return ioremap(addr, size);
> +}
> +
> +static void __iomem *map_ioremap_wc(phys_addr_t addr, size_t size)
> +{
> + return ioremap_wc(addr, size);
> +}
> +
> +static void __iomem *pci_iomap_range_map(struct pci_dev *dev,
> + int bar,
> + unsigned long offset,
> + unsigned long maxlen,
> + void __iomem *(*mapm)(phys_addr_t,
> +size_t))
> +{
> + resource_size_t start = pci_resource_start(dev, bar);
> + resource_size_t len = pci_resource_len(dev, bar);
> + unsigned long flags = pci_resource_flags(dev, bar);
> +
> + if (len <= offset || !start)
> + return NULL;
> + len -= offset;
> + start += offset;
> + if (maxlen && len > maxlen)
> + len = maxlen;
> + if (flags & IORESOURCE_IO)
> + return __pci_ioport_map(dev, start, len);
> + if (flags & IORESOURCE_MEM)
> + return mapm(start, len);
> + /* What? */
> + return NULL;
> +}
> +
> /**
> * pci_iomap_range - create a virtual mapping cookie for a PCI BAR
> * @dev: PCI device that owns the BAR
> @@ -30,22 +70,8 @@ void __iomem *pci_iomap_range(struct pci_dev *dev,
> unsigned long offset,
> unsigned long maxlen)
> {
> - resource_size_t start = pci_resource_start(dev, bar);
> - resource_size_t len = pci_resource_len(dev, bar);
> - unsigned long flags = pci_resource_flags(dev, bar);
> -
> - if (len <= offset || !start)
> - return NULL;
> - len -= offset;
> - start += offset;
> - if (maxlen && len > maxlen)
> - len = maxlen;
> - if (flags & IORESOURCE_IO)
> - return __pci_ioport_map(dev, start, len);
> - if (flags & IORESOURCE_MEM)
> - return ioremap(start, len);
> - /* What? */
> - return NULL;
> + return pci_iomap_range_map(dev, bar, offset, maxlen,
> +map_ioremap);
> }
> EXPORT_SYMBOL(pci_iomap_range);
>
> @@ -70,27 +96,8 @@ void __iomem *pci_iomap_wc_range(struct pci_dev *dev,
>unsigned long offset,
>unsigned long maxlen)
> {
> - resource_size_t start = pci_resource_start(dev, bar);
> - resource_size_t len = pci_resource_len(dev, bar);
> - unsigned long flags = pci_resource_flags(dev, bar);
> -
> -
> - if (flags & IORESOURCE_IO)
> - return NULL;
> -
> - if (len <= offset || !start)
> - return NULL;
> -
> - len -= offset;
> - start += offset;
> - if (maxlen && len > maxlen)
Re: [PATCH] Fix "ordering" comment typos
On Tue, Jan 26, 2021 at 01:50:42PM -0600, Bjorn Helgaas wrote:
> From: Bjorn Helgaas
>
> Fix comment typos in "ordering".
>
> Signed-off-by: Bjorn Helgaas
> ---
> arch/s390/include/asm/facility.h | 2 +-
> drivers/gpu/drm/qxl/qxl_drv.c| 2 +-
> drivers/net/wireless/intel/iwlwifi/fw/file.h | 2 +-
> 3 files changed, 3 insertions(+), 3 deletions(-)
>
>
> Unless somebody objects, I'll just merge these typo fixes via the PCI tree.
Applied to pci/misc for v5.12 with acks from Kalle and Vasily.
> diff --git a/arch/s390/include/asm/facility.h
> b/arch/s390/include/asm/facility.h
> index 68c476b20b57..91b5d714d28f 100644
> --- a/arch/s390/include/asm/facility.h
> +++ b/arch/s390/include/asm/facility.h
> @@ -44,7 +44,7 @@ static inline int __test_facility(unsigned long nr, void
> *facilities)
> }
>
> /*
> - * The test_facility function uses the bit odering where the MSB is bit 0.
> + * The test_facility function uses the bit ordering where the MSB is bit 0.
> * That makes it easier to query facility bits with the bit number as
> * documented in the Principles of Operation.
> */
> diff --git a/drivers/gpu/drm/qxl/qxl_drv.c b/drivers/gpu/drm/qxl/qxl_drv.c
> index 6e7f16f4cec7..dab190a547cc 100644
> --- a/drivers/gpu/drm/qxl/qxl_drv.c
> +++ b/drivers/gpu/drm/qxl/qxl_drv.c
> @@ -141,7 +141,7 @@ static void qxl_drm_release(struct drm_device *dev)
>
> /*
>* TODO: qxl_device_fini() call should be in qxl_pci_remove(),
> - * reodering qxl_modeset_fini() + qxl_device_fini() calls is
> + * reordering qxl_modeset_fini() + qxl_device_fini() calls is
>* non-trivial though.
>*/
> qxl_modeset_fini(qdev);
> diff --git a/drivers/net/wireless/intel/iwlwifi/fw/file.h
> b/drivers/net/wireless/intel/iwlwifi/fw/file.h
> index 597bc88479ba..04fbfe5cbeb0 100644
> --- a/drivers/net/wireless/intel/iwlwifi/fw/file.h
> +++ b/drivers/net/wireless/intel/iwlwifi/fw/file.h
> @@ -866,7 +866,7 @@ struct iwl_fw_dbg_trigger_time_event {
> * tx_bar: tid bitmap to configure on what tid the trigger should occur
> * when a BAR is send (for an Rx BlocAck session).
> * frame_timeout: tid bitmap to configure on what tid the trigger should
> occur
> - * when a frame times out in the reodering buffer.
> + * when a frame times out in the reordering buffer.
> */
> struct iwl_fw_dbg_trigger_ba {
> __le16 rx_ba_start;
> --
> 2.25.1
>
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH] Fix "ordering" comment typos
From: Bjorn Helgaas
Fix comment typos in "ordering".
Signed-off-by: Bjorn Helgaas
---
arch/s390/include/asm/facility.h | 2 +-
drivers/gpu/drm/qxl/qxl_drv.c| 2 +-
drivers/net/wireless/intel/iwlwifi/fw/file.h | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
Unless somebody objects, I'll just merge these typo fixes via the PCI tree.
diff --git a/arch/s390/include/asm/facility.h b/arch/s390/include/asm/facility.h
index 68c476b20b57..91b5d714d28f 100644
--- a/arch/s390/include/asm/facility.h
+++ b/arch/s390/include/asm/facility.h
@@ -44,7 +44,7 @@ static inline int __test_facility(unsigned long nr, void
*facilities)
}
/*
- * The test_facility function uses the bit odering where the MSB is bit 0.
+ * The test_facility function uses the bit ordering where the MSB is bit 0.
* That makes it easier to query facility bits with the bit number as
* documented in the Principles of Operation.
*/
diff --git a/drivers/gpu/drm/qxl/qxl_drv.c b/drivers/gpu/drm/qxl/qxl_drv.c
index 6e7f16f4cec7..dab190a547cc 100644
--- a/drivers/gpu/drm/qxl/qxl_drv.c
+++ b/drivers/gpu/drm/qxl/qxl_drv.c
@@ -141,7 +141,7 @@ static void qxl_drm_release(struct drm_device *dev)
/*
* TODO: qxl_device_fini() call should be in qxl_pci_remove(),
-* reodering qxl_modeset_fini() + qxl_device_fini() calls is
+* reordering qxl_modeset_fini() + qxl_device_fini() calls is
* non-trivial though.
*/
qxl_modeset_fini(qdev);
diff --git a/drivers/net/wireless/intel/iwlwifi/fw/file.h
b/drivers/net/wireless/intel/iwlwifi/fw/file.h
index 597bc88479ba..04fbfe5cbeb0 100644
--- a/drivers/net/wireless/intel/iwlwifi/fw/file.h
+++ b/drivers/net/wireless/intel/iwlwifi/fw/file.h
@@ -866,7 +866,7 @@ struct iwl_fw_dbg_trigger_time_event {
* tx_bar: tid bitmap to configure on what tid the trigger should occur
* when a BAR is send (for an Rx BlocAck session).
* frame_timeout: tid bitmap to configure on what tid the trigger should occur
- * when a frame times out in the reodering buffer.
+ * when a frame times out in the reordering buffer.
*/
struct iwl_fw_dbg_trigger_ba {
__le16 rx_ba_start;
--
2.25.1
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 5/6] iommu/virtio: Support topology description in config space
On Fri, Sep 25, 2020 at 10:12:43AM +0200, Jean-Philippe Brucker wrote: > On Thu, Sep 24, 2020 at 10:22:03AM -0500, Bjorn Helgaas wrote: > > On Fri, Aug 21, 2020 at 03:15:39PM +0200, Jean-Philippe Brucker wrote: > > > + /* Perform the init sequence before we can read the config */ > > > + ret = viommu_pci_reset(common_cfg); > > > > I guess this is some special device-specific reset, not any kind of > > standard PCI reset? > > Yes it's the virtio reset - writing 0 to the status register in the BAR. I wonder if this should be named something like viommu_virtio_reset(), so there's no confusion with PCI resets and all the timing restrictions, config space restoration, etc. associated with them. Bjorn ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 5/6] iommu/virtio: Support topology description in config space
On Fri, Aug 21, 2020 at 03:15:39PM +0200, Jean-Philippe Brucker wrote:
> Platforms without device-tree nor ACPI can provide a topology
> description embedded into the virtio config space. Parse it.
>
> Use PCI FIXUP to probe the config space early, because we need to
> discover the topology before any DMA configuration takes place, and the
> virtio driver may be loaded much later. Since we discover the topology
> description when probing the PCI hierarchy, the virtual IOMMU cannot
> manage other platform devices discovered earlier.
> +struct viommu_cap_config {
> + u8 bar;
> + u32 length; /* structure size */
> + u32 offset; /* structure offset within the bar */
s/the bar/the BAR/ (to match comment below).
> +static void viommu_pci_parse_topology(struct pci_dev *dev)
> +{
> + int ret;
> + u32 features;
> + void __iomem *regs, *common_regs;
> + struct viommu_cap_config cap = {0};
> + struct virtio_pci_common_cfg __iomem *common_cfg;
> +
> + /*
> + * The virtio infrastructure might not be loaded at this point. We need
> + * to access the BARs ourselves.
> + */
> + ret = viommu_pci_find_capability(dev, VIRTIO_PCI_CAP_COMMON_CFG, );
> + if (!ret) {
> + pci_warn(dev, "common capability not found\n");
Is the lack of this capability really an error, i.e., is this
pci_warn() or pci_info()? The "device doesn't have topology
description" below is only pci_dbg(), which suggests that we can live
without this.
Maybe a hint about what "common capability" means?
> + return;
> + }
> +
> + if (pci_enable_device_mem(dev))
> + return;
> +
> + common_regs = pci_iomap(dev, cap.bar, 0);
> + if (!common_regs)
> + return;
> +
> + common_cfg = common_regs + cap.offset;
> +
> + /* Perform the init sequence before we can read the config */
> + ret = viommu_pci_reset(common_cfg);
I guess this is some special device-specific reset, not any kind of
standard PCI reset?
> + if (ret < 0) {
> + pci_warn(dev, "unable to reset device\n");
> + goto out_unmap_common;
> + }
> +
> + iowrite8(VIRTIO_CONFIG_S_ACKNOWLEDGE, _cfg->device_status);
> + iowrite8(VIRTIO_CONFIG_S_ACKNOWLEDGE | VIRTIO_CONFIG_S_DRIVER,
> + _cfg->device_status);
> +
> + /* Find out if the device supports topology description */
> + iowrite32(0, _cfg->device_feature_select);
> + features = ioread32(_cfg->device_feature);
> +
> + if (!(features & BIT(VIRTIO_IOMMU_F_TOPOLOGY))) {
> + pci_dbg(dev, "device doesn't have topology description");
> + goto out_reset;
> + }
> +
> + ret = viommu_pci_find_capability(dev, VIRTIO_PCI_CAP_DEVICE_CFG, );
> + if (!ret) {
> + pci_warn(dev, "device config capability not found\n");
> + goto out_reset;
> + }
> +
> + regs = pci_iomap(dev, cap.bar, 0);
> + if (!regs)
> + goto out_reset;
> +
> + pci_info(dev, "parsing virtio-iommu topology\n");
> + ret = viommu_parse_topology(>dev, regs + cap.offset,
> + pci_resource_len(dev, 0) - cap.offset);
> + if (ret)
> + pci_warn(dev, "failed to parse topology: %d\n", ret);
> +
> + pci_iounmap(dev, regs);
> +out_reset:
> + ret = viommu_pci_reset(common_cfg);
> + if (ret)
> + pci_warn(dev, "unable to reset device\n");
> +out_unmap_common:
> + pci_iounmap(dev, common_regs);
> +}
> +
> +/*
> + * Catch a PCI virtio-iommu implementation early to get the topology
> description
> + * before we start probing other endpoints.
> + */
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_REDHAT_QUMRANET, 0x1040 +
> VIRTIO_ID_IOMMU,
> + viommu_pci_parse_topology);
> --
> 2.28.0
>
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 2/2] virtio-mmio: Reject invalid IRQ 0 command line argument
On Thu, Jul 02, 2020 at 11:06:11AM +0800, Jason Wang wrote:
> On 2020/7/2 上午6:10, Bjorn Helgaas wrote:
> > From: Bjorn Helgaas
> >
> > The "virtio_mmio.device=" command line argument allows a user to specify
> > the size, address, and IRQ of a virtio device. Previously the only
> > requirement for the IRQ was that it be an unsigned integer.
> >
> > Zero is an unsigned integer but an invalid IRQ number, and after
> > a85a6c86c25be ("driver core: platform: Clarify that IRQ 0 is invalid"),
> > attempts to use IRQ 0 cause warnings.
> >
> > If the user specifies IRQ 0, return failure instead of registering a device
> > with IRQ 0.
> >
> > Fixes: a85a6c86c25be ("driver core: platform: Clarify that IRQ 0 is
> > invalid")
> > Signed-off-by: Bjorn Helgaas
> > Cc: Michael S. Tsirkin
> > Cc: Jason Wang
> > Cc: virtualization@lists.linux-foundation.org
> > ---
> > drivers/virtio/virtio_mmio.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
> > index 9d16aaffca9d..627ac0487494 100644
> > --- a/drivers/virtio/virtio_mmio.c
> > +++ b/drivers/virtio/virtio_mmio.c
> > @@ -641,11 +641,11 @@ static int vm_cmdline_set(const char *device,
> > _cmdline_id, );
> > /*
> > -* sscanf() must processes at least 2 chunks; also there
> > +* sscanf() must process at least 2 chunks; also there
> > * must be no extra characters after the last chunk, so
> > * str[consumed] must be '\0'
> > */
> > - if (processed < 2 || str[consumed])
> > + if (processed < 2 || str[consumed] || irq == 0)
> > return -EINVAL;
> > resources[0].flags = IORESOURCE_MEM;
>
> Acked-by: Jason Wang
Thanks, I applied this to my for-linus branch for v5.8.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH 2/2] virtio-mmio: Reject invalid IRQ 0 command line argument
From: Bjorn Helgaas
The "virtio_mmio.device=" command line argument allows a user to specify
the size, address, and IRQ of a virtio device. Previously the only
requirement for the IRQ was that it be an unsigned integer.
Zero is an unsigned integer but an invalid IRQ number, and after
a85a6c86c25be ("driver core: platform: Clarify that IRQ 0 is invalid"),
attempts to use IRQ 0 cause warnings.
If the user specifies IRQ 0, return failure instead of registering a device
with IRQ 0.
Fixes: a85a6c86c25be ("driver core: platform: Clarify that IRQ 0 is invalid")
Signed-off-by: Bjorn Helgaas
Cc: Michael S. Tsirkin
Cc: Jason Wang
Cc: virtualization@lists.linux-foundation.org
---
drivers/virtio/virtio_mmio.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index 9d16aaffca9d..627ac0487494 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -641,11 +641,11 @@ static int vm_cmdline_set(const char *device,
_cmdline_id, );
/*
-* sscanf() must processes at least 2 chunks; also there
+* sscanf() must process at least 2 chunks; also there
* must be no extra characters after the last chunk, so
* str[consumed] must be '\0'
*/
- if (processed < 2 || str[consumed])
+ if (processed < 2 || str[consumed] || irq == 0)
return -EINVAL;
resources[0].flags = IORESOURCE_MEM;
--
2.25.1
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 2/3] PCI: Add DMA configuration for virtual platforms
On Fri, Feb 28, 2020 at 06:25:37PM +0100, Jean-Philippe Brucker wrote: > Hardware platforms usually describe the IOMMU topology using either > device-tree pointers or vendor-specific ACPI tables. For virtual > platforms that don't provide a device-tree, the virtio-iommu device > contains a description of the endpoints it manages. That information > allows us to probe endpoints after the IOMMU is probed (possibly as late > as userspace modprobe), provided it is discovered early enough. > > Add a hook to pci_dma_configure(), which returns -EPROBE_DEFER if the > endpoint is managed by a vIOMMU that will be loaded later, or 0 in any > other case to avoid disturbing the normal DMA configuration methods. > When CONFIG_VIRTIO_IOMMU_TOPOLOGY isn't selected, the call to > virt_dma_configure() is compiled out. > > As long as the information is consistent, platforms can provide both a > device-tree and a built-in topology, and the IOMMU infrastructure is > able to deal with multiple DMA configuration methods. > > Signed-off-by: Jean-Philippe Brucker Acked-by: Bjorn Helgaas > --- > drivers/pci/pci-driver.c | 5 + > 1 file changed, 5 insertions(+) > > diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c > index 0454ca0e4e3f..69303a814f21 100644 > --- a/drivers/pci/pci-driver.c > +++ b/drivers/pci/pci-driver.c > @@ -18,6 +18,7 @@ > #include > #include > #include > +#include > #include "pci.h" > #include "pcie/portdrv.h" > > @@ -1602,6 +1603,10 @@ static int pci_dma_configure(struct device *dev) > struct device *bridge; > int ret = 0; > > + ret = virt_dma_configure(dev); > + if (ret) > + return ret; > + > bridge = pci_get_host_bridge_device(to_pci_dev(dev)); > > if (IS_ENABLED(CONFIG_OF) && bridge->parent && > -- > 2.25.0 > ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v7] pci: quirk to skip msi disable on shutdown
On Tue, Sep 22, 2015 at 02:29:03PM +0300, Michael S. Tsirkin wrote: > On Mon, Sep 21, 2015 at 05:10:43PM -0500, Bjorn Helgaas wrote: > > On Mon, Sep 21, 2015 at 10:42:13PM +0300, Michael S. Tsirkin wrote: > > > On Mon, Sep 21, 2015 at 01:21:47PM -0500, Bjorn Helgaas wrote: > > > > On Sun, Sep 06, 2015 at 06:32:35PM +0300, Michael S. Tsirkin wrote: > > > > > On some hypervisors, virtio devices tend to generate spurious > > > > > interrupts > > > > > when switching between MSI and non-MSI mode. Normally, either MSI or > > > > > non-MSI is used and all is well, but during shutdown, linux disables > > > > > MSI > > > > > which then causes an "irq %d: nobody cared" message, with irq being > > > > > subsequently disabled. > > > > > > > > My understanding is: > > > > > > > > Linux disables MSI/MSI-X during device shutdown. If the device > > > > signals an interrupt after that, it may use INTx. > > > > > > > > This INTx interrupt is not necessarily spurious. Using INTx to signal > > > > an > > > > interrupt that occurs when MSI is disabled seems like reasonable > > > > behavior > > > > for any PCI device. > > > > And it doesn't seem related to switching between MSI and non-MSI mode. > > > > Yes, the INTx happens *after* disabling MSI, but it is not at all > > > > *because* we disabled MSI. So I wouldn't say "they generate spurious > > > > interrupts when switching between MSI and non-MSI." > > > > > > > > Why doesn't virtio-pci just register an INTx handler in addition to an > > > > MSI > > > > handler? > > > > > > The handler causes an expensive exit to the hypervisor, > > > and the INTx lines are shared with other devices. > > > > Do we care? Is this a performance path? I thought we were in a kexec > > shutdown path. > > Yes but the handler would always have to be registered, right? The pci_device_shutdown() path you're modifying calls drv->shutdown() immediately before disabling MSI, so I suppose you could register a handler in a virtio shutdown method. > > > Seems silly to slow them down just so we can do something > > > that triggers the device bug. The bus master is disabled by that time, > > > if linux can just desist from touching MSI enable device won't > > > send either INTx (because MSI is on) or MSI > > > (because bus master is on) and all will be well. > > > > It would also be silly to put special-purpose code in the PCI core > > if there's a reasonable way to handle this in a driver. > > > > Can you describe exactly what the device bug is? Apparently you're > > saying that if we shut down MSI, it triggers the bug? And I guess > > you're talking about a virtio device as implemented in qemu or other > > hypervisors? > > Yes. Basically depending on an internal device state, disabling MSI > sometimes wedges it. The most easy to debug effect is if it starts > sending INTx interrupts, for which there's no handler currently. > Full system reset always gets us out of the bad state. If disabling MSI causes the device to use INTx interrupts, that sounds perfectly normal to me. If disabling MSI causes the device to hang, *that* sounds like a bug. Since this is virtio, we should be able to figure out exactly where that happens. Do you have a pointer to a virtio bug report, or even a QEMU commit that fixes this virtio bug? I understand that even if there is a virtio fix in QEMU, we want a solution that works even with an old QEMU that doesn't contain the fix. But a pointer to a QEMU fix would really help understand and document the Linux fix. Bjorn ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v7] pci: quirk to skip msi disable on shutdown
On Tue, Sep 22, 2015 at 05:07:19PM +0300, Michael S. Tsirkin wrote: > On Tue, Sep 22, 2015 at 07:36:40AM -0500, Bjorn Helgaas wrote: > > On Tue, Sep 22, 2015 at 02:29:03PM +0300, Michael S. Tsirkin wrote: > > > On Mon, Sep 21, 2015 at 05:10:43PM -0500, Bjorn Helgaas wrote: > > > > Can you describe exactly what the device bug is? Apparently you're > > > > saying that if we shut down MSI, it triggers the bug? And I guess > > > > you're talking about a virtio device as implemented in qemu or other > > > > hypervisors? > > > > > > Yes. Basically depending on an internal device state, disabling MSI > > > sometimes wedges it. The most easy to debug effect is if it starts > > > sending INTx interrupts, for which there's no handler currently. > > > Full system reset always gets us out of the bad state. > > > > If disabling MSI causes the device to use INTx interrupts, that sounds > > perfectly normal to me. > > > > If disabling MSI causes the device to hang, *that* sounds like a bug. > > Since this is virtio, we should be able to figure out exactly where > > that happens. Do you have a pointer to a virtio bug report, or even a > > QEMU commit that fixes this virtio bug? > > > > I understand that even if there is a virtio fix in QEMU, we want a > > solution that works even with an old QEMU that doesn't contain the > > fix. But a pointer to a QEMU fix would really help understand and > > document the Linux fix. > > I'm not sure we ever understood it completely. > > I think some of it has to do with the way the whole virtio 0 > device register layout changes when you enable/disable MSI. So should > be ok when using the modern virtio 1 model since we fixed this thing. If you know that: - pci_device_shutdown() disables MSI, - disabling MSI changes the virtio register layout, and - the driver may touch the device after pci_device_shutdown(), it seems like you might want a virtio shutdown method so you can find out when the register layout changes. Changing the register layout sounds completely broken, and dealing with it sounds racy, but it sounds like a mess that should be handled by the driver. > I was hoping that since disabling MSI in pci core is only useful as a > work-around (for devices with a broken bus master enable - even though I > don't think we know what these are exactly), a flag for not disabling it > won't be held to such a high standard. Well, I suppose you see the problem with adding workarounds on top of workarounds, especially when we don't have a clear understanding of why we need them. The "disable MSI on shutdown" code was added here: http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=d52877c7b1af and there is no information in that patch about it being a workaround for devices with broken bus master enable. The cumulative effect of stuff like this is that it becomes impossible to do any meaningful restructuring in the core without breaking some corner case. Bjorn ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v7] pci: quirk to skip msi disable on shutdown
On Mon, Sep 21, 2015 at 10:42:13PM +0300, Michael S. Tsirkin wrote: > On Mon, Sep 21, 2015 at 01:21:47PM -0500, Bjorn Helgaas wrote: > > On Sun, Sep 06, 2015 at 06:32:35PM +0300, Michael S. Tsirkin wrote: > > > On some hypervisors, virtio devices tend to generate spurious interrupts > > > when switching between MSI and non-MSI mode. Normally, either MSI or > > > non-MSI is used and all is well, but during shutdown, linux disables MSI > > > which then causes an "irq %d: nobody cared" message, with irq being > > > subsequently disabled. > > > > My understanding is: > > > > Linux disables MSI/MSI-X during device shutdown. If the device > > signals an interrupt after that, it may use INTx. > > > > This INTx interrupt is not necessarily spurious. Using INTx to signal an > > interrupt that occurs when MSI is disabled seems like reasonable behavior > > for any PCI device. > > And it doesn't seem related to switching between MSI and non-MSI mode. > > Yes, the INTx happens *after* disabling MSI, but it is not at all > > *because* we disabled MSI. So I wouldn't say "they generate spurious > > interrupts when switching between MSI and non-MSI." > > > > Why doesn't virtio-pci just register an INTx handler in addition to an MSI > > handler? > > The handler causes an expensive exit to the hypervisor, > and the INTx lines are shared with other devices. Do we care? Is this a performance path? I thought we were in a kexec shutdown path. > Seems silly to slow them down just so we can do something > that triggers the device bug. The bus master is disabled by that time, > if linux can just desist from touching MSI enable device won't > send either INTx (because MSI is on) or MSI > (because bus master is on) and all will be well. It would also be silly to put special-purpose code in the PCI core if there's a reasonable way to handle this in a driver. Can you describe exactly what the device bug is? Apparently you're saying that if we shut down MSI, it triggers the bug? And I guess you're talking about a virtio device as implemented in qemu or other hypervisors? If we leave MSI enabled (as your patch does), then the device has MSI enabled and Bus Master disabled. I can see these possibilities: 1) the device never recognizes an interrupt condition 2) the device sets the pending bit but doesn't issue the MSI write, so the OS doesn't see the interrupt unless it polls for it 3) the device signals MSI and we still have an MSI handler registered, so we silently handle it 4) the device signals INTx You seem to suggest that if we leave MSI enabled (as your patch does), we're in case 1. But I doubt that disabling MSI causes the device to interrupt. Case 2 seems more likely to me: the device recognized an interrupt condition, e.g., an event occurred, and the OS simply doesn't see the interrupt because the device can't issue the MSI message. Case 3 does seem like it would be a device bug, because the device shouldn't do an MSI write when Bus Master is disabled. I don't see this case mentioned explicitly in the PCI spec, but PCIe r3.0 spec sec 7.5.1.1 does make it clear that disabling Bus Master also disables MSI messages. I don't know whether case 4 would be legal or not. But apparently it doesn't happen with the virtio device anyway, so it's not really a concern here. Bjorn ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v7] pci: quirk to skip msi disable on shutdown
On Sun, Sep 06, 2015 at 06:32:35PM +0300, Michael S. Tsirkin wrote: > On some hypervisors, virtio devices tend to generate spurious interrupts > when switching between MSI and non-MSI mode. Normally, either MSI or > non-MSI is used and all is well, but during shutdown, linux disables MSI > which then causes an "irq %d: nobody cared" message, with irq being > subsequently disabled. My understanding is: Linux disables MSI/MSI-X during device shutdown. If the device signals an interrupt after that, it may use INTx. This INTx interrupt is not necessarily spurious. Using INTx to signal an interrupt that occurs when MSI is disabled seems like reasonable behavior for any PCI device. And it doesn't seem related to switching between MSI and non-MSI mode. Yes, the INTx happens *after* disabling MSI, but it is not at all *because* we disabled MSI. So I wouldn't say "they generate spurious interrupts when switching between MSI and non-MSI." Why doesn't virtio-pci just register an INTx handler in addition to an MSI handler? Bjorn ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v7] pci: quirk to skip msi disable on shutdown
On Sun, Sep 06, 2015 at 06:32:35PM +0300, Michael S. Tsirkin wrote:
> On some hypervisors, virtio devices tend to generate spurious interrupts
> when switching between MSI and non-MSI mode. Normally, either MSI or
> non-MSI is used and all is well, but during shutdown, linux disables MSI
> which then causes an "irq %d: nobody cared" message, with irq being
> subsequently disabled.
>
> Since bus mastering is already disabled at this point, disabling MSI
> isn't actually useful for spec compliant devices: MSI interrupts are
> in-bus memory writes so disabling Bus Master (which is already done)
> disables these as well: after some research, it appears to be there for
> the benefit of devices that ignore the bus master bit.
>
> As it's not clear how common this kind of bug is, this patch simply adds
> a quirk, to be set by devices that wish to skip disabling msi on
> shutdown, relying on bus master instead.
>
> We set this quirk in virtio core.
>
> Reported-by: Fam Zheng <f...@redhat.com>
> Cc: Bjorn Helgaas <bhelg...@google.com>
> Cc: Yinghai Lu <yhlu.kernel.s...@gmail.com>
> Cc: Ulrich Obergfell <uober...@redhat.com>
> Cc: Rusty Russell <ru...@rustcorp.com.au>
> Cc: "Eric W. Biederman" <ebied...@xmission.com>
> Signed-off-by: Michael S. Tsirkin <m...@redhat.com>
Eric, what do you think of this? You had many comments on previous
versions.
Minor comment on a comment below.
> ---
>
>
> changes from v6:
> limit changes to virtio only
> changes from v5:
> rebased on top of pci/msi
> fixed commit log, including comments by Bjorn
> and adding explanation to address comments/questions by Eric
> dropped stable Cc, this patch does not seem to qualify for stable
> changes from v4:
> Yijing Wang <wangyij...@huawei.com> noted that
> early fixups rely on pci_msi_off.
> Split out the functionality and move off the
> required part to run early during pci_device_setup.
> Changes from v3:
> fix a copy-and-paste error in
> pci: drop some duplicate code
> other patches are unchanged
> drop Cc stable for now
> Changes from v2:
> move code from probe to device enumeration
> add patches to unexport pci_msi_off
>
>
> include/linux/pci.h| 2 ++
> drivers/pci/pci-driver.c | 6 --
> drivers/virtio/virtio_pci_common.c | 4
> 3 files changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 860c751..80f3494 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -180,6 +180,8 @@ enum pci_dev_flags {
> PCI_DEV_FLAGS_NO_BUS_RESET = (__force pci_dev_flags_t) (1 << 6),
> /* Do not use PM reset even if device advertises NoSoftRst- */
> PCI_DEV_FLAGS_NO_PM_RESET = (__force pci_dev_flags_t) (1 << 7),
> + /* Do not disable MSI on shutdown, disable bus master instead */
This comment doesn't really match what the patch does. The patch merely
does "Do not disable MSI on shutdown." It doesn't "disable bus master
instead."
Bus master may be disabled elsewhere, but that is independent of the
PCI_DEV_FLAGS_NO_MSI_SHUTDOWN flag.
> + PCI_DEV_FLAGS_NO_MSI_SHUTDOWN = (__force pci_dev_flags_t) (1 << 8),
> };
>
> enum pci_irq_reroute_variant {
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index 3cb2210..59d9e40 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -450,8 +450,10 @@ static void pci_device_shutdown(struct device *dev)
>
> if (drv && drv->shutdown)
> drv->shutdown(pci_dev);
> - pci_msi_shutdown(pci_dev);
> - pci_msix_shutdown(pci_dev);
> + if (!(pci_dev->dev_flags & PCI_DEV_FLAGS_NO_MSI_SHUTDOWN)) {
> + pci_msi_shutdown(pci_dev);
> + pci_msix_shutdown(pci_dev);
> + }
>
> #ifdef CONFIG_KEXEC
> /*
> diff --git a/drivers/virtio/virtio_pci_common.c
> b/drivers/virtio/virtio_pci_common.c
> index 78f804a..26f46c3 100644
> --- a/drivers/virtio/virtio_pci_common.c
> +++ b/drivers/virtio/virtio_pci_common.c
> @@ -528,6 +528,8 @@ static int virtio_pci_probe(struct pci_dev *pci_dev,
> if (rc)
> goto err_register;
>
> + pci_dev->dev_flags |= PCI_DEV_FLAGS_NO_MSI_SHUTDOWN;
> +
> return 0;
>
> err_register:
> @@ -546,6 +548,8 @@ static void virtio_pci_remove(struct pci_dev *pci_dev)
> {
> struct virtio_pci_device *vp_dev = pci_get_drvdata(pci_dev);
>
> + pci_dev->dev_flags &= ~PCI_DEV_FLAGS_NO_MSI_SHUTDOWN;
> +
> unregister_virtio_device(_dev->vdev);
>
> if (vp_dev->ioaddr)
> --
> MST
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 08/16] mn10300: drop dead code
On Wed, Jan 14, 2015 at 07:27:48PM +0200, Michael S. Tsirkin wrote:
pci-iomap.c was (apparently, mistakenly) reintroduced as part of
commit 83c2dc15ce824450e7044b9f90cd529c25747ae0
MN10300: Handle cacheable PCI regions in pci_iomap()
probably as side-effect of forward-porting the patch
from an old kernel.
It's not really needed: the generic pci_iomap does the right thing here.
The new file isn't compiled so it's safe to drop.
Cc: Bjorn Helgaas bhelg...@google.com
Cc: linux-...@vger.kernel.org
Cc: triv...@kernel.org
Cc: David Howells dhowe...@redhat.com
Signed-off-by: Michael S. Tsirkin m...@redhat.com
Acked-by: Bjorn Helgaas bhelg...@google.com
---
Can relevant people please ack this for merging through virtio tree?
arch/mn10300/unit-asb2305/pci-iomap.c | 35
---
1 file changed, 35 deletions(-)
delete mode 100644 arch/mn10300/unit-asb2305/pci-iomap.c
diff --git a/arch/mn10300/unit-asb2305/pci-iomap.c
b/arch/mn10300/unit-asb2305/pci-iomap.c
deleted file mode 100644
index bd65dae..000
--- a/arch/mn10300/unit-asb2305/pci-iomap.c
+++ /dev/null
@@ -1,35 +0,0 @@
-/* ASB2305 PCI I/O mapping handler
- *
- * Copyright (C) 2007 Red Hat, Inc. All Rights Reserved.
- * Written by David Howells (dhowe...@redhat.com)
- *
- * This program is free software; you can redistribute it and/or
- * modify it under the terms of the GNU General Public Licence
- * as published by the Free Software Foundation; either version
- * 2 of the Licence, or (at your option) any later version.
- */
-#include linux/pci.h
-#include linux/module.h
-
-/*
- * Create a virtual mapping cookie for a PCI BAR (memory or IO)
- */
-void __iomem *pci_iomap(struct pci_dev *dev, int bar, unsigned long maxlen)
-{
- resource_size_t start = pci_resource_start(dev, bar);
- resource_size_t len = pci_resource_len(dev, bar);
- unsigned long flags = pci_resource_flags(dev, bar);
-
- if (!len || !start)
- return NULL;
-
- if ((flags IORESOURCE_IO) || (flags IORESOURCE_MEM)) {
- if (flags IORESOURCE_CACHEABLE !(flags IORESOURCE_IO))
- return ioremap(start, len);
- else
- return ioremap_nocache(start, len);
- }
-
- return NULL;
-}
-EXPORT_SYMBOL(pci_iomap);
--
MST
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 09/16] pci: add pci_iomap_range
On Wed, Jan 14, 2015 at 07:27:54PM +0200, Michael S. Tsirkin wrote:
Virtio drivers should map the part of the BAR they need, not necessarily
all of it.
Cc: Bjorn Helgaas bhelg...@google.com
Cc: linux-...@vger.kernel.org
Acked-by: Arnd Bergmann a...@arndb.de
Signed-off-by: Michael S. Tsirkin m...@redhat.com
Signed-off-by: Rusty Russell ru...@rustcorp.com.au
Acked-by: Bjorn Helgaas bhelg...@google.com
Sorry it took so long for me to notice these!
---
Bjorn, can you please ack this for merging through the virtio tree?
include/asm-generic/pci_iomap.h | 10 ++
lib/pci_iomap.c | 35 ++-
2 files changed, 40 insertions(+), 5 deletions(-)
diff --git a/include/asm-generic/pci_iomap.h b/include/asm-generic/pci_iomap.h
index ce37349..7389c87 100644
--- a/include/asm-generic/pci_iomap.h
+++ b/include/asm-generic/pci_iomap.h
@@ -15,6 +15,9 @@ struct pci_dev;
#ifdef CONFIG_PCI
/* Create a virtual mapping cookie for a PCI BAR (memory or IO) */
extern void __iomem *pci_iomap(struct pci_dev *dev, int bar, unsigned long
max);
+extern void __iomem *pci_iomap_range(struct pci_dev *dev, int bar,
+ unsigned long offset,
+ unsigned long maxlen);
/* Create a virtual mapping cookie for a port on a given PCI device.
* Do not call this directly, it exists to make it easier for architectures
* to override */
@@ -30,6 +33,13 @@ static inline void __iomem *pci_iomap(struct pci_dev *dev,
int bar, unsigned lon
{
return NULL;
}
+
+static inline void __iomem *pci_iomap_range(struct pci_dev *dev, int bar,
+ unsigned long offset,
+ unsigned long maxlen)
+{
+ return NULL;
+}
#endif
#endif /* __ASM_GENERIC_IO_H */
diff --git a/lib/pci_iomap.c b/lib/pci_iomap.c
index 0d83ea8..bcce5f1 100644
--- a/lib/pci_iomap.c
+++ b/lib/pci_iomap.c
@@ -10,10 +10,11 @@
#ifdef CONFIG_PCI
/**
- * pci_iomap - create a virtual mapping cookie for a PCI BAR
+ * pci_iomap_range - create a virtual mapping cookie for a PCI BAR
* @dev: PCI device that owns the BAR
* @bar: BAR number
- * @maxlen: length of the memory to map
+ * @offset: map memory at the given offset in BAR
+ * @maxlen: max length of the memory to map
*
* Using this function you will get a __iomem address to your device BAR.
* You can access it using ioread*() and iowrite*(). These functions hide
@@ -21,16 +22,21 @@
* you expect from them in the correct way.
*
* @maxlen specifies the maximum length to map. If you want to get access to
- * the complete BAR without checking for its length first, pass %0 here.
+ * the complete BAR from offset to the end, pass %0 here.
* */
-void __iomem *pci_iomap(struct pci_dev *dev, int bar, unsigned long maxlen)
+void __iomem *pci_iomap_range(struct pci_dev *dev,
+ int bar,
+ unsigned long offset,
+ unsigned long maxlen)
{
resource_size_t start = pci_resource_start(dev, bar);
resource_size_t len = pci_resource_len(dev, bar);
unsigned long flags = pci_resource_flags(dev, bar);
- if (!len || !start)
+ if (len = offset || !start)
return NULL;
+ len -= offset;
+ start += offset;
if (maxlen len maxlen)
len = maxlen;
if (flags IORESOURCE_IO)
@@ -43,6 +49,25 @@ void __iomem *pci_iomap(struct pci_dev *dev, int bar,
unsigned long maxlen)
/* What? */
return NULL;
}
+EXPORT_SYMBOL(pci_iomap_range);
+/**
+ * pci_iomap - create a virtual mapping cookie for a PCI BAR
+ * @dev: PCI device that owns the BAR
+ * @bar: BAR number
+ * @maxlen: length of the memory to map
+ *
+ * Using this function you will get a __iomem address to your device BAR.
+ * You can access it using ioread*() and iowrite*(). These functions hide
+ * the details if this is a MMIO or PIO address space and will just do what
+ * you expect from them in the correct way.
+ *
+ * @maxlen specifies the maximum length to map. If you want to get access to
+ * the complete BAR without checking for its length first, pass %0 here.
+ * */
+void __iomem *pci_iomap(struct pci_dev *dev, int bar, unsigned long maxlen)
+{
+ return pci_iomap_range(dev, bar, 0, maxlen);
+}
EXPORT_SYMBOL(pci_iomap);
#endif /* CONFIG_PCI */
--
MST
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 0/25] Replace DEFINE_PCI_DEVICE_TABLE macro use
[+cc Jingoo] On Fri, Jul 18, 2014 at 12:50 PM, James Bottomley james.bottom...@hansenpartnership.com wrote: On Fri, 2014-07-18 at 11:17 -0700, Greg KH wrote: On Fri, Jul 18, 2014 at 09:54:32AM -0700, James Bottomley wrote: On Fri, 2014-07-18 at 09:43 -0700, Greg KH wrote: On Fri, Jul 18, 2014 at 12:22:13PM -0400, John W. Linville wrote: On Fri, Jul 18, 2014 at 05:26:47PM +0200, Benoit Taine wrote: We should prefer `const struct pci_device_id` over `DEFINE_PCI_DEVICE_TABLE` to meet kernel coding style guidelines. This issue was reported by checkpatch. Honestly, I prefer the macro -- it stands-out more. Maybe the style guidelines and/or checkpatch should change instead? The macro is horrid, no other bus has this type of thing just to save a few characters in typing OK, so this is the macro: #define DEFINE_PCI_DEVICE_TABLE(_table) \ const struct pci_device_id _table[] Could you explain what's so horrible? The reason it's useful today is that people forget the const (and sometimes the [] making it a true table instead of a pointer). If you use the DEFINE_PCI_DEVICE_TABLE macro, the compile breaks if you use it wrongly (good) and you automatically get the correct annotations. We have almost 1000 more uses of the non-macro version than the macro version in the kernel today: $ git grep -w DEFINE_PCI_DEVICE_TABLE | wc -l 262 $ git grep const struct pci_device_id | wc -l 1254 My big complaint is that we need to be consistant, either pick one or the other and stick to it. As the macro is the least used, it's easiest to fix up, and it also is more consistant with all other kernel subsystems which do not have such a macro. I've a weak preference for consistency, but not at the expense of hundreds of patches churning the kernel to remove an innocuous macro. Churn costs time and effort. As there is no need for the __init macro mess anymore, there's no real need for the DEFINE_PCI_DEVICE_TABLE macro either. I think checkpatch will catch the use of non-const users for the id table already today, it catches lots of other uses like this already. , so why should PCI be special in this regard anymore? I think the PCI usage dwarfs most other bus types now, so you could turn the question around. However, I don't think majority voting is a good guide to best practise; lets debate the merits for their own sake. Not really dwarf, USB is close with over 700 such structures: $ git grep const struct usb_device_id | wc -l 725 And i2c is almost just as big as PCI: $ git grep const struct i2c_device_id | wc -l 1223 So again, this macro is not consistent with the majority of PCI drivers, nor with any other type of device id declaration in the kernel, which is why I feel it should be removed. And finally, the PCI documentation itself says to not use this macro, so this isn't a new thing. From Documentation/PCI/pci.txt: The ID table is an array of struct pci_device_id entries ending with an all-zero entry. Definitions with static const are generally preferred. Use of the deprecated macro DEFINE_PCI_DEVICE_TABLE should be avoided. That wording went into the file last December, when we last talked about this and everyone in that discussion agreed to remove the macro for the above reasons. Consistency matters. In this case, I don't think it does that much ... a cut and paste either way (from a macro or non-macro based driver) yields correct code. Since there's no bug here and no apparent way to misuse the macro, why bother? Anyway, it's PCI code ... let the PCI maintainer decide. However, if he does want this do it as one big bang patch via either the PCI or Trivial tree (latter because Jiří has experience doing this, but the former might be useful so the decider feels the pain ...) I don't feel strongly either way, so I guess I'm OK with this, and in the spirit of feeling the pain, I'm willing to handle it. Jingoo proposed similar patches, so it might be nice to give him some credit. Benoit, how about if you wait until about half-way through the merge window after v3.16 releases, generate an up-to-date single patch, and post that? Then we can try to get it in before v3.17-rc1 to minimize merge hassles. Bjorn ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio-blk: make the queue depth configurable
On Wed, Mar 19, 2014 at 12:37 AM, Rusty Russell ru...@rustcorp.com.au wrote:
Joe Perches j...@perches.com writes:
On Sun, 2014-03-16 at 22:00 -0700, Joe Perches wrote:
On Mon, 2014-03-17 at 14:25 +1030, Rusty Russell wrote:
Erk, our tests are insufficient. Testbuilding an allmodconfig with this
now:
Good idea.
diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
[]
@@ -188,6 +188,9 @@ struct kparam_array
/* Default value instead of permissions? */ \
static int __param_perm_check_##name __attribute__((unused)) = \
BUILD_BUG_ON_ZERO((perm) 0 || (perm) 0777 || ((perm) 2)) \
+ /* User perms = group perms = other perms. */ \
+ + BUILD_BUG_ON_ZERO(((perm) 6) (((perm) 3) 7))\
+ + BUILD_BUG_ON_ZEROperm) 3) 7) ((perm) 7)) \
+ BUILD_BUG_ON_ZERO(sizeof(prefix) MAX_PARAM_PREFIX_LEN); \
static const char __param_str_##name[] = prefix #name; \
static struct kernel_param __moduleparam_const __param_##name \
It might make sense to separate this octal permissions
test into a new macro for other checks in macros like
CLASS_ATTR, DEVICE_ATTR, SENSOR_ATTR and SENSOR_ATTR_2.
OK, I took your bikeshed and re-painted it below.
#define VERIFY_OCTAL_PERMISSIONS(perms)
\
({\
if (__builtin_constant_p(perms)) { \
BUILD_BUG_ON((perms) 0); \
BUILD_BUG_ON((perms) 0777); \
/* User perms = group perms = other perms */ \
BUILD_BUG_ON(((perms) 6) (((perms) 3) 7));\
BUILD_BUG_ONperms) 3) 7) ((perms) 7)); \
} \
; \
})
Subject: VERIFY_OCTAL_PERMISSIONS: stricter checking for sysfs perms.
Summary of http://lkml.org/lkml/2014/3/14/363 :
Ted: module_param(queue_depth, int, 444)
Joe: 0444!
Rusty: User perms = group perms = other perms?
Joe: CLASS_ATTR, DEVICE_ATTR, SENSOR_ATTR and SENSOR_ATTR_2?
Side effect of stricter permissions means removing the unnecessary
S_IFREG from drivers/pci/slot.c.
Suggested-by: Joe Perches j...@perches.com
Cc: Bjorn Helgaas bhelg...@google.com
Cc: Greg Kroah-Hartman gre...@linuxfoundation.org
Signed-off-by: Rusty Russell ru...@rustcorp.com.au
Acked-by: Bjorn Helgaas bhelg...@google.com for drivers/pci/slot.c
It looks like fs/ocfs2/cluster/sys.c and fs/ocfs2/stackglue.c also use
S_IFREG in a similar way.
diff --git a/drivers/pci/slot.c b/drivers/pci/slot.c
index 7dd62fa9d0bd..396c200b9ddb 100644
--- a/drivers/pci/slot.c
+++ b/drivers/pci/slot.c
@@ -116,11 +116,11 @@ static void pci_slot_release(struct kobject *kobj)
}
static struct pci_slot_attribute pci_slot_attr_address =
- __ATTR(address, (S_IFREG | S_IRUGO), address_read_file, NULL);
+ __ATTR(address, S_IRUGO, address_read_file, NULL);
static struct pci_slot_attribute pci_slot_attr_max_speed =
- __ATTR(max_bus_speed, (S_IFREG | S_IRUGO), max_speed_read_file, NULL);
+ __ATTR(max_bus_speed, S_IRUGO, max_speed_read_file, NULL);
static struct pci_slot_attribute pci_slot_attr_cur_speed =
- __ATTR(cur_bus_speed, (S_IFREG | S_IRUGO), cur_speed_read_file, NULL);
+ __ATTR(cur_bus_speed, S_IRUGO, cur_speed_read_file, NULL);
static struct attribute *pci_slot_default_attrs[] = {
pci_slot_attr_address.attr,
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 471090093c67..945afeb3058a 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -842,4 +842,13 @@ static inline void ftrace_dump(enum ftrace_dump_mode
oops_dump_mode) { }
# define REBUILD_DUE_TO_FTRACE_MCOUNT_RECORD
#endif
+/* Permissions on a sysfs file: you didn't miss the 0 prefix did you? */
+#define VERIFY_OCTAL_PERMISSIONS(perms)
\
+ ((__builtin_constant_p(perms) ? \
+ BUILD_BUG_ON_ZERO((perms) 0) + \
+ BUILD_BUG_ON_ZERO((perms) 0777) + \
+ /* User perms = group perms = other perms */\
+ BUILD_BUG_ON_ZERO(((perms) 6) (((perms) 3) 7)) +\
+ BUILD_BUG_ON_ZEROperms) 3) 7) ((perms) 7)): 0) + \
+(perms))
#endif
diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
index 175f6995d1af..204a67743804 100644
--- a/include/linux/moduleparam.h
+++ b/include/linux/moduleparam.h
@@ -186,14 +186,12 @@ struct kparam_array
parameters. */
#define __module_param_call(prefix, name, ops, arg, perm, level) \
/* Default value instead
Re: [PATCH 16/16 v6] PCI: document the new PCI boot parameters
On Wednesday 22 October 2008 02:45:31 am Yu Zhao wrote: Document the new PCI[x86] boot parameters. Cc: Alex Chiang [EMAIL PROTECTED] Cc: Grant Grundler [EMAIL PROTECTED] Cc: Greg KH [EMAIL PROTECTED] Cc: Ingo Molnar [EMAIL PROTECTED] Cc: Jesse Barnes [EMAIL PROTECTED] Cc: Matthew Wilcox [EMAIL PROTECTED] Cc: Randy Dunlap [EMAIL PROTECTED] Cc: Roland Dreier [EMAIL PROTECTED] Signed-off-by: Yu Zhao [EMAIL PROTECTED] --- Documentation/kernel-parameters.txt | 10 ++ 1 files changed, 10 insertions(+), 0 deletions(-) diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt index 53ba7c7..5482ae0 100644 --- a/Documentation/kernel-parameters.txt +++ b/Documentation/kernel-parameters.txt @@ -1677,6 +1677,16 @@ and is between 256 and 4096 characters. It is defined in the file cbmemsize=nn[KMG] The fixed amount of bus space which is reserved for the CardBus bridge's memory window. The default value is 64 megabytes. + assign-mmio=[:]bb [X86] reassign memory resources of all + devices under bus [:]bb ( is the domain + number and bb is the bus number). + assign-pio=[:]bb[X86] reassign io port resources of all + devices under bus [:]bb ( is the domain + number and bb is the bus number). + align-mmio=[:]bb:dd.f [X86] relocate memory resources of a + device to minimum PAGE_SIZE alignment ( is + the domain number and bb, dd and f is the bus, + device and function number). pcie_aspm= [PCIE] Forcibly enable or disable PCIe Active State Power Management. I think it's nicer to have the documentation change included in the patch that implements the change. For example, I think this and patch 9/16 add boot option to align ... should be folded into a single patch. And similarly for the other documentation patches. Bjorn ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: [PATCH 8/16 v6] PCI: add boot options to reassign resources
On Wednesday 22 October 2008 02:43:03 am Yu Zhao wrote:
This patch adds boot options so user can reassign device resources
of all devices under a bus.
The boot options can be used as:
pci=assign-mmio=:01,assign-pio=:02
'[:]bb' is the domain and bus number.
I think this example is incorrect because you look for ; to
separate options, not ,.
Bjorn
Cc: Alex Chiang [EMAIL PROTECTED]
Cc: Grant Grundler [EMAIL PROTECTED]
Cc: Greg KH [EMAIL PROTECTED]
Cc: Ingo Molnar [EMAIL PROTECTED]
Cc: Jesse Barnes [EMAIL PROTECTED]
Cc: Matthew Wilcox [EMAIL PROTECTED]
Cc: Randy Dunlap [EMAIL PROTECTED]
Cc: Roland Dreier [EMAIL PROTECTED]
Signed-off-by: Yu Zhao [EMAIL PROTECTED]
---
arch/x86/pci/common.c | 73
+
arch/x86/pci/i386.c | 10 ---
arch/x86/pci/pci.h|3 ++
3 files changed, 82 insertions(+), 4 deletions(-)
diff --git a/arch/x86/pci/common.c b/arch/x86/pci/common.c
index b67732b..06e1ce0 100644
--- a/arch/x86/pci/common.c
+++ b/arch/x86/pci/common.c
@@ -137,6 +137,72 @@ static void __devinit
pcibios_fixup_device_resources(struct pci_dev *dev)
}
}
+static char *pci_assign_pio;
+static char *pci_assign_mmio;
+
+static int pcibios_bus_resource_needs_fixup(struct pci_bus *bus)
+{
+ int i;
+ int type = 0;
+ int domain, busnr;
+
+ if (!bus-self)
+ return 0;
+
+ for (i = 0; i 2; i++) {
+ char *str = i ? pci_assign_pio : pci_assign_mmio;
+
+ while (str *str) {
+ if (sscanf(str, %04x:%02x, domain, busnr) != 2) {
+ if (sscanf(str, %02x, busnr) != 1)
+ break;
+ domain = 0;
+ }
+
+ if (pci_domain_nr(bus) == domain
+ bus-number == busnr) {
+ type |= i ? IORESOURCE_IO : IORESOURCE_MEM;
+ break;
+ }
+
+ str = strchr(str, ';');
+ if (str)
+ str++;
+ }
+ }
+
+ return type;
+}
+
+static void __devinit pcibios_fixup_bus_resources(struct pci_bus *bus)
+{
+ int i;
+ int type = pcibios_bus_resource_needs_fixup(bus);
+
+ if (!type)
+ return;
+
+ for (i = 0; i PCI_BUS_NUM_RESOURCES; i++) {
+ struct resource *res = bus-resource[i];
+
+ if (!res)
+ continue;
+ if (res-flags type)
+ res-flags = 0;
+ }
+}
+
+int pcibios_resource_needs_fixup(struct pci_dev *dev, int resno)
+{
+ struct pci_bus *bus;
+
+ for (bus = dev-bus; bus bus != pci_root_bus; bus = bus-parent)
+ if (pcibios_bus_resource_needs_fixup(bus))
+ return 1;
+
+ return 0;
+}
+
/*
* Called after each bus is probed, but before its children
* are examined.
@@ -147,6 +213,7 @@ void __devinit pcibios_fixup_bus(struct pci_bus *b)
struct pci_dev *dev;
pci_read_bridge_bases(b);
+ pcibios_fixup_bus_resources(b);
list_for_each_entry(dev, b-devices, bus_list)
pcibios_fixup_device_resources(dev);
}
@@ -519,6 +586,12 @@ char * __devinit pcibios_setup(char *str)
} else if (!strcmp(str, skip_isa_align)) {
pci_probe |= PCI_CAN_SKIP_ISA_ALIGN;
return NULL;
+ } else if (!strncmp(str, assign-pio=, 11)) {
+ pci_assign_pio = str + 11;
+ return NULL;
+ } else if (!strncmp(str, assign-mmio=, 12)) {
+ pci_assign_mmio = str + 12;
+ return NULL;
}
return str;
}
diff --git a/arch/x86/pci/i386.c b/arch/x86/pci/i386.c
index 8729bde..ea82a5b 100644
--- a/arch/x86/pci/i386.c
+++ b/arch/x86/pci/i386.c
@@ -169,10 +169,12 @@ static void __init pcibios_allocate_resources(int pass)
(unsigned long long) r-start,
(unsigned long long) r-end,
r-flags, enabled, pass);
- pr = pci_find_parent_resource(dev, r);
- if (pr !request_resource(pr, r))
- continue;
- dev_err(dev-dev, BAR %d: can't allocate resource\n,
idx);
+ if (!pcibios_resource_needs_fixup(dev, idx)) {
+ pr = pci_find_parent_resource(dev, r);
+ if (pr !request_resource(pr, r))
+ continue;
+ dev_err(dev-dev, BAR %d: can't allocate
resource\n, idx);
+ }
/* We'll assign a new address later */
r-end -= r-start;
Re: [PATCH 9/16 v6] PCI: add boot option to align MMIO resources
On Wednesday 22 October 2008 02:43:24 am Yu Zhao wrote:
This patch adds boot option to align MMIO resource for a device.
The alignment is a bigger value between the PAGE_SIZE and the
resource size.
It looks like this forces alignment on PAGE_SIZE, not a bigger
value between the PAGE_SIZE and the resource size. Can you
clarify the changelog to specify exactly what alignment this
option forces?
The boot option can be used as:
pci=align-mmio=:01:02.3
'[:]01:02.3' is the domain, bus, device and function number
of the device.
I think you also support using multiple align-mmio=:BB:dd.f
options separated by ;, but I had to read the code to figure that
out. Can you give an example of this in the changelog and the
kernel-parameters.txt patch?
Bjorn
Cc: Alex Chiang [EMAIL PROTECTED]
Cc: Grant Grundler [EMAIL PROTECTED]
Cc: Greg KH [EMAIL PROTECTED]
Cc: Ingo Molnar [EMAIL PROTECTED]
Cc: Jesse Barnes [EMAIL PROTECTED]
Cc: Matthew Wilcox [EMAIL PROTECTED]
Cc: Randy Dunlap [EMAIL PROTECTED]
Cc: Roland Dreier [EMAIL PROTECTED]
Signed-off-by: Yu Zhao [EMAIL PROTECTED]
---
arch/x86/pci/common.c | 37 +
drivers/pci/pci.c | 20 ++--
include/linux/pci.h |1 +
3 files changed, 56 insertions(+), 2 deletions(-)
diff --git a/arch/x86/pci/common.c b/arch/x86/pci/common.c
index 06e1ce0..3c5d230 100644
--- a/arch/x86/pci/common.c
+++ b/arch/x86/pci/common.c
@@ -139,6 +139,7 @@ static void __devinit
pcibios_fixup_device_resources(struct pci_dev *dev)
static char *pci_assign_pio;
static char *pci_assign_mmio;
+static char *pci_align_mmio;
static int pcibios_bus_resource_needs_fixup(struct pci_bus *bus)
{
@@ -192,6 +193,36 @@ static void __devinit pcibios_fixup_bus_resources(struct
pci_bus *bus)
}
}
+int pcibios_resource_alignment(struct pci_dev *dev, int resno)
+{
+ int domain, busnr, slot, func;
+ char *str = pci_align_mmio;
+
+ if (dev-resource[resno].flags IORESOURCE_IO)
+ return 0;
+
+ while (str *str) {
+ if (sscanf(str, %04x:%02x:%02x.%d,
+ domain, busnr, slot, func) != 4) {
+ if (sscanf(str, %02x:%02x.%d,
+ busnr, slot, func) != 3)
+ break;
+ domain = 0;
+ }
+
+ if (pci_domain_nr(dev-bus) == domain
+ dev-bus-number == busnr
+ dev-devfn == PCI_DEVFN(slot, func))
+ return PAGE_SIZE;
+
+ str = strchr(str, ';');
+ if (str)
+ str++;
+ }
+
+ return 0;
+}
+
int pcibios_resource_needs_fixup(struct pci_dev *dev, int resno)
{
struct pci_bus *bus;
@@ -200,6 +231,9 @@ int pcibios_resource_needs_fixup(struct pci_dev *dev, int
resno)
if (pcibios_bus_resource_needs_fixup(bus))
return 1;
+ if (pcibios_resource_alignment(dev, resno))
+ return 1;
+
return 0;
}
@@ -592,6 +626,9 @@ char * __devinit pcibios_setup(char *str)
} else if (!strncmp(str, assign-mmio=, 12)) {
pci_assign_mmio = str + 12;
return NULL;
+ } else if (!strncmp(str, align-mmio=, 11)) {
+ pci_align_mmio = str + 11;
+ return NULL;
}
return str;
}
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index b02167a..11ecd6f 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1015,6 +1015,20 @@ int __attribute__ ((weak))
pcibios_set_pcie_reset_state(struct pci_dev *dev,
}
/**
+ * pcibios_resource_alignment - get resource alignment requirement
+ * @dev: the PCI device
+ * @resno: resource number
+ *
+ * Queries the resource alignment from PCI low level code. Returns positive
+ * if there is alignment requirement of the resource, or 0 otherwise.
+ */
+int __attribute__ ((weak)) pcibios_resource_alignment(struct pci_dev *dev,
+ int resno)
+{
+ return 0;
+}
+
+/**
* pci_set_pcie_reset_state - set reset state for device dev
* @dev: the PCI-E device reset
* @state: Reset state to enter into
@@ -1913,12 +1927,14 @@ int pci_select_bars(struct pci_dev *dev, unsigned
long flags)
*/
int pci_resource_alignment(struct pci_dev *dev, int resno)
{
- resource_size_t align;
+ resource_size_t align, bios_align;
struct resource *res = dev-resource + resno;
+ bios_align = pcibios_resource_alignment(dev, resno);
+
align = resource_alignment(res);
if (align)
- return align;
+ return align bios_align ? align : bios_align;
dev_err(dev-dev, alignment: invalid resource #%d\n, resno);
return 0;
diff --git a/include/linux/pci.h b/include/linux/pci.h
index
Re: [PATCH 2/16 v6] PCI: define PCI resource names in an 'enum'
On Wednesday 22 October 2008 08:44:24 am Yu Zhao wrote:
Bjorn Helgaas wrote:
On Wednesday 22 October 2008 02:40:41 am Yu Zhao wrote:
This patch moves all definitions of the PCI resource names to an 'enum',
and also replaces some hard-coded resource variables with symbol
names. This change eases introduction of device specific resources.
Thanks for removing a bunch of magic numbers from the code.
static void
pci_restore_bars(struct pci_dev *dev)
{
- int i, numres;
-
- switch (dev-hdr_type) {
- case PCI_HEADER_TYPE_NORMAL:
- numres = 6;
- break;
- case PCI_HEADER_TYPE_BRIDGE:
- numres = 2;
- break;
- case PCI_HEADER_TYPE_CARDBUS:
- numres = 1;
- break;
- default:
- /* Should never get here, but just in case... */
- return;
- }
+ int i;
- for (i = 0; i numres; i++)
+ for (i = 0; i PCI_BRIDGE_RESOURCES; i++)
pci_update_resource(dev, i);
}
The behavior of this function used to depend on dev-hdr_type. Now
we don't look at hdr_type at all, so we do the same thing for all
devices.
For example, for a CardBus device, we used to call pci_update_resource()
only for BAR 0; now we call it for BARs 0-6.
Maybe this is safe, but I can't tell from the patch, so I think you
should explain *why* it's safe in the changelog.
It's safe because pci_update_resource() will ignore unused resources.
E.g., for a Cardbus, only BAR 0 is used and its 'flags' is set, then
pci_update_resource() only updates it. BAR 1-6 are ignored since their
'flags' are 0.
I'll put more explanation in the changelog.
This is a logically separate change from merely substituting enum
names for magic numbers, so you might even consider splitting it
into a separate patch. Better bisection and all that, you know :-)
Bjorn
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
