* Re: [PATCH] pci-hyperv: use kmalloc to allocate hypercall params buffer
2016-11-08 22:04 [PATCH] pci-hyperv: use kmalloc to allocate hypercall params buffer Long Li
@ 2016-11-23 23:20 ` Bjorn Helgaas
2016-11-24 2:18 ` KY Srinivasan
2016-11-29 23:25 ` Bjorn Helgaas
2016-12-05 16:52 ` Stephen Hemminger
2 siblings, 1 reply; 6+ messages in thread
From: Bjorn Helgaas @ 2016-11-23 23:20 UTC (permalink / raw)
To: Long Li
Cc: K. Y. Srinivasan, Haiyang Zhang, Bjorn Helgaas, devel, linux-pci,
linux-kernel, Long Li
On Tue, Nov 08, 2016 at 02:04:38PM -0800, Long Li wrote:
> From: Long Li <longli@microsoft.com>
>
> hv_do_hypercall assumes that we pass a segment from a physically
> continuous buffer. Buffer allocated on the stack may not work if
> CONFIG_VMAP_STACK=y is set.
>
> Change to use kmalloc to allocate this buffer.
>
> The v2 patch adds locking to access the pre-allocated buffer.
>
> Signed-off-by: Long Li <longli@microsoft.com>
> Reported-by: Haiyang Zhang <haiyangz@microsoft.com>
Waiting for a maintainer ack for this.
$ ./scripts/get_maintainer.pl -f drivers/pci/host/pci-hyperv.c
"K. Y. Srinivasan" <kys@microsoft.com> (maintainer:Hyper-V CORE AND DRIVERS)
Haiyang Zhang <haiyangz@microsoft.com> (maintainer:Hyper-V CORE AND DRIVERS)
...
> ---
> drivers/pci/host/pci-hyperv.c | 29 +++++++++++++++++++----------
> 1 file changed, 19 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c
> index 763ff87..ca553df 100644
> --- a/drivers/pci/host/pci-hyperv.c
> +++ b/drivers/pci/host/pci-hyperv.c
> @@ -378,6 +378,8 @@ struct hv_pcibus_device {
> struct msi_domain_info msi_info;
> struct msi_controller msi_chip;
> struct irq_domain *irq_domain;
> + struct retarget_msi_interrupt retarget_msi_interrupt_params;
> + spinlock_t retarget_msi_interrupt_lock;;
> };
>
> /*
> @@ -774,34 +776,40 @@ void hv_irq_unmask(struct irq_data *data)
> {
> struct msi_desc *msi_desc = irq_data_get_msi_desc(data);
> struct irq_cfg *cfg = irqd_cfg(data);
> - struct retarget_msi_interrupt params;
> + struct retarget_msi_interrupt *params;
> struct hv_pcibus_device *hbus;
> struct cpumask *dest;
> struct pci_bus *pbus;
> struct pci_dev *pdev;
> int cpu;
> + unsigned long flags;
>
> dest = irq_data_get_affinity_mask(data);
> pdev = msi_desc_to_pci_dev(msi_desc);
> pbus = pdev->bus;
> hbus = container_of(pbus->sysdata, struct hv_pcibus_device, sysdata);
>
> - memset(¶ms, 0, sizeof(params));
> - params.partition_id = HV_PARTITION_ID_SELF;
> - params.source = 1; /* MSI(-X) */
> - params.address = msi_desc->msg.address_lo;
> - params.data = msi_desc->msg.data;
> - params.device_id = (hbus->hdev->dev_instance.b[5] << 24) |
> + spin_lock_irqsave(&hbus->retarget_msi_interrupt_lock, flags);
> +
> + params = &hbus->retarget_msi_interrupt_params;
> + memset(params, 0, sizeof(*params));
> + params->partition_id = HV_PARTITION_ID_SELF;
> + params->source = 1; /* MSI(-X) */
> + params->address = msi_desc->msg.address_lo;
> + params->data = msi_desc->msg.data;
> + params->device_id = (hbus->hdev->dev_instance.b[5] << 24) |
> (hbus->hdev->dev_instance.b[4] << 16) |
> (hbus->hdev->dev_instance.b[7] << 8) |
> (hbus->hdev->dev_instance.b[6] & 0xf8) |
> PCI_FUNC(pdev->devfn);
> - params.vector = cfg->vector;
> + params->vector = cfg->vector;
>
> for_each_cpu_and(cpu, dest, cpu_online_mask)
> - params.vp_mask |= (1ULL << vmbus_cpu_number_to_vp_number(cpu));
> + params->vp_mask |= (1ULL << vmbus_cpu_number_to_vp_number(cpu));
> +
> + hv_do_hypercall(HVCALL_RETARGET_INTERRUPT, params, NULL);
>
> - hv_do_hypercall(HVCALL_RETARGET_INTERRUPT, ¶ms, NULL);
> + spin_unlock_irqrestore(&hbus->retarget_msi_interrupt_lock, flags);
>
> pci_msi_unmask_irq(data);
> }
> @@ -2186,6 +2194,7 @@ static int hv_pci_probe(struct hv_device *hdev,
> INIT_LIST_HEAD(&hbus->resources_for_children);
> spin_lock_init(&hbus->config_lock);
> spin_lock_init(&hbus->device_list_lock);
> + spin_lock_init(&hbus->retarget_msi_interrupt_lock);
> sema_init(&hbus->enum_sem, 1);
> init_completion(&hbus->remove_event);
>
> --
> 2.7.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH] pci-hyperv: use kmalloc to allocate hypercall params buffer
2016-11-23 23:20 ` Bjorn Helgaas
@ 2016-11-24 2:18 ` KY Srinivasan
0 siblings, 0 replies; 6+ messages in thread
From: KY Srinivasan @ 2016-11-24 2:18 UTC (permalink / raw)
To: Bjorn Helgaas, Long Li
Cc: Haiyang Zhang, Bjorn Helgaas, devel@linuxdriverproject.org,
linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org, Long Li
> -----Original Message-----
> From: Bjorn Helgaas [mailto:helgaas@kernel.org]
> Sent: Wednesday, November 23, 2016 3:20 PM
> To: Long Li <longli@microsoft.com>
> Cc: KY Srinivasan <kys@microsoft.com>; Haiyang Zhang
> <haiyangz@microsoft.com>; Bjorn Helgaas <bhelgaas@google.com>;
> devel@linuxdriverproject.org; linux-pci@vger.kernel.org; linux-
> kernel@vger.kernel.org; Long Li <longli@microsoft.com>
> Subject: Re: [PATCH] pci-hyperv: use kmalloc to allocate hypercall params
> buffer
>=20
> On Tue, Nov 08, 2016 at 02:04:38PM -0800, Long Li wrote:
> > From: Long Li <longli@microsoft.com>
> >
> > hv_do_hypercall assumes that we pass a segment from a physically
> > continuous buffer. Buffer allocated on the stack may not work if
> > CONFIG_VMAP_STACK=3Dy is set.
> >
> > Change to use kmalloc to allocate this buffer.
> >
> > The v2 patch adds locking to access the pre-allocated buffer.
> >
> > Signed-off-by: Long Li <longli@microsoft.com>
> > Reported-by: Haiyang Zhang <haiyangz@microsoft.com>
>=20
> Waiting for a maintainer ack for this.
Acked-by: K. Y. Srinivasan <kys@microsoft.com>
>=20
> $ ./scripts/get_maintainer.pl -f drivers/pci/host/pci-hyperv.c
> "K. Y. Srinivasan" <kys@microsoft.com> (maintainer:Hyper-V CORE AND
> DRIVERS)
> Haiyang Zhang <haiyangz@microsoft.com> (maintainer:Hyper-V CORE AND
> DRIVERS)
> ...
>=20
> > ---
> > drivers/pci/host/pci-hyperv.c | 29 +++++++++++++++++++----------
> > 1 file changed, 19 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyper=
v.c
> > index 763ff87..ca553df 100644
> > --- a/drivers/pci/host/pci-hyperv.c
> > +++ b/drivers/pci/host/pci-hyperv.c
> > @@ -378,6 +378,8 @@ struct hv_pcibus_device {
> > struct msi_domain_info msi_info;
> > struct msi_controller msi_chip;
> > struct irq_domain *irq_domain;
> > + struct retarget_msi_interrupt retarget_msi_interrupt_params;
> > + spinlock_t retarget_msi_interrupt_lock;;
> > };
> >
> > /*
> > @@ -774,34 +776,40 @@ void hv_irq_unmask(struct irq_data *data)
> > {
> > struct msi_desc *msi_desc =3D irq_data_get_msi_desc(data);
> > struct irq_cfg *cfg =3D irqd_cfg(data);
> > - struct retarget_msi_interrupt params;
> > + struct retarget_msi_interrupt *params;
> > struct hv_pcibus_device *hbus;
> > struct cpumask *dest;
> > struct pci_bus *pbus;
> > struct pci_dev *pdev;
> > int cpu;
> > + unsigned long flags;
> >
> > dest =3D irq_data_get_affinity_mask(data);
> > pdev =3D msi_desc_to_pci_dev(msi_desc);
> > pbus =3D pdev->bus;
> > hbus =3D container_of(pbus->sysdata, struct hv_pcibus_device,
> sysdata);
> >
> > - memset(¶ms, 0, sizeof(params));
> > - params.partition_id =3D HV_PARTITION_ID_SELF;
> > - params.source =3D 1; /* MSI(-X) */
> > - params.address =3D msi_desc->msg.address_lo;
> > - params.data =3D msi_desc->msg.data;
> > - params.device_id =3D (hbus->hdev->dev_instance.b[5] << 24) |
> > + spin_lock_irqsave(&hbus->retarget_msi_interrupt_lock, flags);
> > +
> > + params =3D &hbus->retarget_msi_interrupt_params;
> > + memset(params, 0, sizeof(*params));
> > + params->partition_id =3D HV_PARTITION_ID_SELF;
> > + params->source =3D 1; /* MSI(-X) */
> > + params->address =3D msi_desc->msg.address_lo;
> > + params->data =3D msi_desc->msg.data;
> > + params->device_id =3D (hbus->hdev->dev_instance.b[5] << 24) |
> > (hbus->hdev->dev_instance.b[4] << 16) |
> > (hbus->hdev->dev_instance.b[7] << 8) |
> > (hbus->hdev->dev_instance.b[6] & 0xf8) |
> > PCI_FUNC(pdev->devfn);
> > - params.vector =3D cfg->vector;
> > + params->vector =3D cfg->vector;
> >
> > for_each_cpu_and(cpu, dest, cpu_online_mask)
> > - params.vp_mask |=3D (1ULL <<
> vmbus_cpu_number_to_vp_number(cpu));
> > + params->vp_mask |=3D (1ULL <<
> vmbus_cpu_number_to_vp_number(cpu));
> > +
> > + hv_do_hypercall(HVCALL_RETARGET_INTERRUPT, params, NULL);
> >
> > - hv_do_hypercall(HVCALL_RETARGET_INTERRUPT, ¶ms, NULL);
> > + spin_unlock_irqrestore(&hbus->retarget_msi_interrupt_lock, flags);
> >
> > pci_msi_unmask_irq(data);
> > }
> > @@ -2186,6 +2194,7 @@ static int hv_pci_probe(struct hv_device *hdev,
> > INIT_LIST_HEAD(&hbus->resources_for_children);
> > spin_lock_init(&hbus->config_lock);
> > spin_lock_init(&hbus->device_list_lock);
> > + spin_lock_init(&hbus->retarget_msi_interrupt_lock);
> > sema_init(&hbus->enum_sem, 1);
> > init_completion(&hbus->remove_event);
> >
> > --
> > 2.7.4
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at
> https://na01.safelinks.protection.outlook.com/?url=3Dhttp%3A%2F%2Fvger.ke
> rnel.org%2Fmajordomo-
> info.html&data=3D02%7C01%7Ckys%40microsoft.com%7C95f624c719384a83b0
> e308d413f75282%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C6361
> 55400348714262&sdata=3DiE5j0UF00ExjolDvuilAe9Kxo17TrPScjbkFU%2BA426w
> %3D&reserved=3D0
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] pci-hyperv: use kmalloc to allocate hypercall params buffer
2016-11-08 22:04 [PATCH] pci-hyperv: use kmalloc to allocate hypercall params buffer Long Li
2016-11-23 23:20 ` Bjorn Helgaas
@ 2016-11-29 23:25 ` Bjorn Helgaas
2016-12-05 16:52 ` Stephen Hemminger
2 siblings, 0 replies; 6+ messages in thread
From: Bjorn Helgaas @ 2016-11-29 23:25 UTC (permalink / raw)
To: Long Li
Cc: K. Y. Srinivasan, Haiyang Zhang, Bjorn Helgaas, devel, linux-pci,
linux-kernel, Long Li
On Tue, Nov 08, 2016 at 02:04:38PM -0800, Long Li wrote:
> From: Long Li <longli@microsoft.com>
>
> hv_do_hypercall assumes that we pass a segment from a physically
> continuous buffer. Buffer allocated on the stack may not work if
> CONFIG_VMAP_STACK=y is set.
>
> Change to use kmalloc to allocate this buffer.
>
> The v2 patch adds locking to access the pre-allocated buffer.
>
> Signed-off-by: Long Li <longli@microsoft.com>
> Reported-by: Haiyang Zhang <haiyangz@microsoft.com>
Applied with KY's ack to pci/host-hv, thanks!
> ---
> drivers/pci/host/pci-hyperv.c | 29 +++++++++++++++++++----------
> 1 file changed, 19 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c
> index 763ff87..ca553df 100644
> --- a/drivers/pci/host/pci-hyperv.c
> +++ b/drivers/pci/host/pci-hyperv.c
> @@ -378,6 +378,8 @@ struct hv_pcibus_device {
> struct msi_domain_info msi_info;
> struct msi_controller msi_chip;
> struct irq_domain *irq_domain;
> + struct retarget_msi_interrupt retarget_msi_interrupt_params;
> + spinlock_t retarget_msi_interrupt_lock;;
> };
>
> /*
> @@ -774,34 +776,40 @@ void hv_irq_unmask(struct irq_data *data)
> {
> struct msi_desc *msi_desc = irq_data_get_msi_desc(data);
> struct irq_cfg *cfg = irqd_cfg(data);
> - struct retarget_msi_interrupt params;
> + struct retarget_msi_interrupt *params;
> struct hv_pcibus_device *hbus;
> struct cpumask *dest;
> struct pci_bus *pbus;
> struct pci_dev *pdev;
> int cpu;
> + unsigned long flags;
>
> dest = irq_data_get_affinity_mask(data);
> pdev = msi_desc_to_pci_dev(msi_desc);
> pbus = pdev->bus;
> hbus = container_of(pbus->sysdata, struct hv_pcibus_device, sysdata);
>
> - memset(¶ms, 0, sizeof(params));
> - params.partition_id = HV_PARTITION_ID_SELF;
> - params.source = 1; /* MSI(-X) */
> - params.address = msi_desc->msg.address_lo;
> - params.data = msi_desc->msg.data;
> - params.device_id = (hbus->hdev->dev_instance.b[5] << 24) |
> + spin_lock_irqsave(&hbus->retarget_msi_interrupt_lock, flags);
> +
> + params = &hbus->retarget_msi_interrupt_params;
> + memset(params, 0, sizeof(*params));
> + params->partition_id = HV_PARTITION_ID_SELF;
> + params->source = 1; /* MSI(-X) */
> + params->address = msi_desc->msg.address_lo;
> + params->data = msi_desc->msg.data;
> + params->device_id = (hbus->hdev->dev_instance.b[5] << 24) |
> (hbus->hdev->dev_instance.b[4] << 16) |
> (hbus->hdev->dev_instance.b[7] << 8) |
> (hbus->hdev->dev_instance.b[6] & 0xf8) |
> PCI_FUNC(pdev->devfn);
> - params.vector = cfg->vector;
> + params->vector = cfg->vector;
>
> for_each_cpu_and(cpu, dest, cpu_online_mask)
> - params.vp_mask |= (1ULL << vmbus_cpu_number_to_vp_number(cpu));
> + params->vp_mask |= (1ULL << vmbus_cpu_number_to_vp_number(cpu));
> +
> + hv_do_hypercall(HVCALL_RETARGET_INTERRUPT, params, NULL);
>
> - hv_do_hypercall(HVCALL_RETARGET_INTERRUPT, ¶ms, NULL);
> + spin_unlock_irqrestore(&hbus->retarget_msi_interrupt_lock, flags);
>
> pci_msi_unmask_irq(data);
> }
> @@ -2186,6 +2194,7 @@ static int hv_pci_probe(struct hv_device *hdev,
> INIT_LIST_HEAD(&hbus->resources_for_children);
> spin_lock_init(&hbus->config_lock);
> spin_lock_init(&hbus->device_list_lock);
> + spin_lock_init(&hbus->retarget_msi_interrupt_lock);
> sema_init(&hbus->enum_sem, 1);
> init_completion(&hbus->remove_event);
>
> --
> 2.7.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] pci-hyperv: use kmalloc to allocate hypercall params buffer
2016-11-08 22:04 [PATCH] pci-hyperv: use kmalloc to allocate hypercall params buffer Long Li
2016-11-23 23:20 ` Bjorn Helgaas
2016-11-29 23:25 ` Bjorn Helgaas
@ 2016-12-05 16:52 ` Stephen Hemminger
[not found] ` <BN3PR03MB2227BB61FB49F42EBA2323EBCE820@BN3PR03MB2227.namprd03.prod.outlook.com>
2 siblings, 1 reply; 6+ messages in thread
From: Stephen Hemminger @ 2016-12-05 16:52 UTC (permalink / raw)
To: Long Li
Cc: K. Y. Srinivasan, Haiyang Zhang, Bjorn Helgaas, devel,
linux-kernel, linux-pci
On Tue, 8 Nov 2016 14:04:38 -0800
Long Li <longli@exchange.microsoft.com> wrote:
> + spin_lock_irqsave(&hbus->retarget_msi_interrupt_lock, flags);
> +
> + params = &hbus->retarget_msi_interrupt_params;
> + memset(params, 0, sizeof(*params));
> + params->partition_id = HV_PARTITION_ID_SELF;
> + params->source = 1; /* MSI(-X) */
> + params->address = msi_desc->msg.address_lo;
> + params->data = msi_desc->msg.data;
> + params->device_id = (hbus->hdev->dev_instance.b[5] << 24) |
> (hbus->hdev->dev_instance.b[4] << 16) |
> (hbus->hdev->dev_instance.b[7] << 8) |
> (hbus->hdev->dev_instance.b[6] & 0xf8) |
> PCI_FUNC(pdev->devfn);
> - params.vector = cfg->vector;
> + params->vector = cfg->vector;
>
> for_each_cpu_and(cpu, dest, cpu_online_mask)
> - params.vp_mask |= (1ULL << vmbus_cpu_number_to_vp_number(cpu));
> + params->vp_mask |= (1ULL << vmbus_cpu_number_to_vp_number(cpu));
> +
> + hv_do_hypercall(HVCALL_RETARGET_INTERRUPT, params, NULL);
>
> - hv_do_hypercall(HVCALL_RETARGET_INTERRUPT, ¶ms, NULL);
> + spin_unlock_irqrestore(&hbus->retarget_msi_interrupt_lock, flags);
It looks like the additional locking here is being overly paranoid.
The caller is already holding the irq descriptor lock. Look at fixup_irqs.
^ permalink raw reply [flat|nested] 6+ messages in thread