2016-11-08 20:11:20

by Long Li

Subject: [PATCH] pci-hyperv: use kmalloc to allocate hypercall params buffer

From: Long Li <[email protected]>

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 <[email protected]>
Reported-by: Haiyang Zhang <[email protected]>
---
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(&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) |
+ 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, &params, 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

2016-11-23 23:21:05

by Bjorn Helgaas

Subject: Re: [PATCH] pci-hyperv: use kmalloc to allocate hypercall params buffer

On Tue, Nov 08, 2016 at 02:04:38PM -0800, Long Li wrote:
> From: Long Li <[email protected]>
>
> 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 <[email protected]>
> Reported-by: Haiyang Zhang <[email protected]>

Waiting for a maintainer ack for this.

$ ./scripts/get_maintainer.pl -f drivers/pci/host/pci-hyperv.c
"K. Y. Srinivasan" <[email protected]> (maintainer:Hyper-V CORE AND DRIVERS)
Haiyang Zhang <[email protected]> (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(&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) |
> + 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, &params, 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

2016-11-24 02:18:52

by KY Srinivasan

Subject: RE: [PATCH] pci-hyperv: use kmalloc to allocate hypercall params buffer

> -----Original Message-----
> From: Bjorn Helgaas [mailto:[email protected]]
> Sent: Wednesday, November 23, 2016 3:20 PM
> To: Long Li <[email protected]>
> Cc: KY Srinivasan <[email protected]>; Haiyang Zhang
> <[email protected]>; Bjorn Helgaas <[email protected]>;
> [email protected]; [email protected]; linux-
> [email protected]; Long Li <[email protected]>
> Subject: Re: [PATCH] pci-hyperv: use kmalloc to allocate hypercall params
> buffer
>
> On Tue, Nov 08, 2016 at 02:04:38PM -0800, Long Li wrote:
> > From: Long Li <[email protected]>
> >
> > 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 <[email protected]>
> > Reported-by: Haiyang Zhang <[email protected]>
>
> Waiting for a maintainer ack for this.

Acked-by: K. Y. Srinivasan <[email protected]>

>
> $ ./scripts/get_maintainer.pl -f drivers/pci/host/pci-hyperv.c
> "K. Y. Srinivasan" <[email protected]> (maintainer:Hyper-V CORE AND
> DRIVERS)
> Haiyang Zhang <[email protected]> (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(&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) |
> > + 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, &params, 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 [email protected]
> > More majordomo info at
> https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fvger.ke
> rnel.org%2Fmajordomo-
> info.html&data=02%7C01%7Ckys%40microsoft.com%7C95f624c719384a83b0
> e308d413f75282%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C6361
> 55400348714262&sdata=iE5j0UF00ExjolDvuilAe9Kxo17TrPScjbkFU%2BA426w
> %3D&reserved=0

2016-11-29 23:25:18

by Bjorn Helgaas

Subject: Re: [PATCH] pci-hyperv: use kmalloc to allocate hypercall params buffer

On Tue, Nov 08, 2016 at 02:04:38PM -0800, Long Li wrote:
> From: Long Li <[email protected]>
>
> 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 <[email protected]>
> Reported-by: Haiyang Zhang <[email protected]>

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(&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) |
> + 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, &params, 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

2016-12-05 16:53:29

by Stephen Hemminger

Subject: Re: [PATCH] pci-hyperv: use kmalloc to allocate hypercall params buffer

On Tue, 8 Nov 2016 14:04:38 -0800
Long Li <[email protected]> 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, &params, 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.

2016-12-06 03:09:51

by Long Li

Subject: RE: [PATCH] pci-hyperv: use kmalloc to allocate hypercall params buffer

> -----Original Message-----
> From: Stephen Hemminger [mailto:[email protected]]
> Sent: Monday, December 5, 2016 8:53 AM
> To: Long Li <[email protected]>
> Cc: KY Srinivasan <[email protected]>; Haiyang Zhang
> <[email protected]>; Bjorn Helgaas <[email protected]>;
> [email protected]; [email protected]; linux-
> [email protected]
> Subject: Re: [PATCH] pci-hyperv: use kmalloc to allocate hypercall params
> buffer
>
> On Tue, 8 Nov 2016 14:04:38 -0800
> Long Li <[email protected]> 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, &params, 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.

You are right. On my test machine, there are two possible places calling hv_irq_unmask(): request _irq() and handle_edge_irq(). They both have desc->lock held when calling .irq_unmask on the chip. A review of the IRQ code shows that desc->lock is always held while calling chip->irq_unmask().

Since the lock doesn't do any harm and it is not on performance code path, we can remove the lock in the upcoming patches.

2016-12-06 16:21:55

by Stephen Hemminger

Subject: Re: [PATCH] pci-hyperv: use kmalloc to allocate hypercall params buffer

On Tue, 6 Dec 2016 00:37:08 +0000
Long Li <[email protected]> wrote:

> > -----Original Message-----
> > From: Stephen Hemminger [mailto:[email protected]]
> > Sent: Monday, December 5, 2016 8:53 AM
> > To: Long Li <[email protected]>
> > Cc: KY Srinivasan <[email protected]>; Haiyang Zhang
> > <[email protected]>; Bjorn Helgaas <[email protected]>;
> > [email protected]; [email protected]; linux-
> > [email protected]
> > Subject: Re: [PATCH] pci-hyperv: use kmalloc to allocate hypercall params
> > buffer
> >
> > On Tue, 8 Nov 2016 14:04:38 -0800
> > Long Li <[email protected]> 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, &params, 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.
>
> You are right. On my test machine, there are two possible places calling hv_irq_unmask(): request _irq() and handle_edge_irq(). They both have desc->lock held when calling .irq_unmask on the chip. A review of the IRQ code shows that desc->lock is always held while calling chip->irq_unmask().
>
> Since the lock doesn't do any harm and it is not on performance code path, we can remove the lock in the upcoming patches.

Why add it then remove it, your patch hasn't been accepted. Please revise it and remove it.
Don't add additional unnecessary code.