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

[Stephen Hemminger]

On Tue, 6 Dec 2016 00:37:08 +
Long Li <lon...@microsoft.com> wrote:

> > -Original Message-
> > From: Stephen Hemminger [mailto:step...@networkplumber.org]
> > Sent: Monday, December 5, 2016 8:53 AM
> > To: Long Li <lon...@microsoft.com>
> > Cc: KY Srinivasan <k...@microsoft.com>; Haiyang Zhang
> > <haiya...@microsoft.com>; Bjorn Helgaas <bhelg...@google.com>;
> > de...@linuxdriverproject.org; linux-ker...@vger.kernel.org; linux-
> > p...@vger.kernel.org
> > Subject: Re: [PATCH] pci-hyperv: use kmalloc to allocate hypercall params
> > buffer
> > 
> > On Tue,  8 Nov 2016 14:04:38 -0800
> > Long Li <lon...@exchange.microsoft.com> wrote:
> >   
> > > + spin_lock_irqsave(>retarget_msi_interrupt_lock, flags);
> > > +
> > > + params = >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, , NULL);
> > > + spin_unlock_irqrestore(>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.

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Long Li]

> -Original Message-
> From: Stephen Hemminger [mailto:step...@networkplumber.org]
> Sent: Monday, December 5, 2016 8:53 AM
> To: Long Li <lon...@microsoft.com>
> Cc: KY Srinivasan <k...@microsoft.com>; Haiyang Zhang
> <haiya...@microsoft.com>; Bjorn Helgaas <bhelg...@google.com>;
> de...@linuxdriverproject.org; linux-ker...@vger.kernel.org; linux-
> p...@vger.kernel.org
> Subject: Re: [PATCH] pci-hyperv: use kmalloc to allocate hypercall params
> buffer
> 
> On Tue,  8 Nov 2016 14:04:38 -0800
> Long Li <lon...@exchange.microsoft.com> wrote:
> 
> > +   spin_lock_irqsave(>retarget_msi_interrupt_lock, flags);
> > +
> > +   params = >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, , NULL);
> > +   spin_unlock_irqrestore(>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.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Long Li]

> -Original Message-
> From: KY Srinivasan
> Sent: Monday, December 5, 2016 1:23 PM
> To: Cathy Avery <cav...@redhat.com>; Bjorn Helgaas
> <helg...@kernel.org>; Long Li <lon...@microsoft.com>
> Cc: de...@linuxdriverproject.org
> Subject: RE: [PATCH] pci-hyperv: use kmalloc to allocate hypercall params
> buffer
> 
> 
> 
> > -Original Message-
> > From: devel [mailto:driverdev-devel-boun...@linuxdriverproject.org] On
> > Behalf Of Cathy Avery
> > Sent: Monday, December 5, 2016 4:54 AM
> > To: Bjorn Helgaas <helg...@kernel.org>; Long Li <lon...@microsoft.com>
> > Cc: de...@linuxdriverproject.org
> > Subject: Re: [PATCH] pci-hyperv: use kmalloc to allocate hypercall
> > params buffer
> >
> > Hi,
> >
> > Is the double semicolon a typo?
> 
> Yes; it is a typo.

I'll fix this.

> 
> K. Y
> >
> > Thanks,
> >
> > Cathy
> >
> > 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;;
> >   };
> >
> >
> >
> > On 11/29/2016 06:25 PM, Bjorn Helgaas wrote:
> > > On Tue, Nov 08, 2016 at 02:04:38PM -0800, Long Li wrote:
> > >> From: Long Li <lon...@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 <lon...@microsoft.com>
> > >> Reported-by: Haiyang Zhang <haiya...@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(, 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(>retarget_msi_interrupt_lock, flags);
> > >> +
> > >> + 

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

[KY Srinivasan]

> -Original Message-
> From: devel [mailto:driverdev-devel-boun...@linuxdriverproject.org] On
> Behalf Of Cathy Avery
> Sent: Monday, December 5, 2016 4:54 AM
> To: Bjorn Helgaas <helg...@kernel.org>; Long Li <lon...@microsoft.com>
> Cc: de...@linuxdriverproject.org
> Subject: Re: [PATCH] pci-hyperv: use kmalloc to allocate hypercall params
> buffer
> 
> Hi,
> 
> Is the double semicolon a typo?

Yes; it is a typo.

K. Y
> 
> Thanks,
> 
> Cathy
> 
> 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;;
>   };
> 
> 
> 
> On 11/29/2016 06:25 PM, Bjorn Helgaas wrote:
> > On Tue, Nov 08, 2016 at 02:04:38PM -0800, Long Li wrote:
> >> From: Long Li <lon...@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 <lon...@microsoft.com>
> >> Reported-by: Haiyang Zhang <haiya...@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(, 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(>retarget_msi_interrupt_lock, flags);
> >> +
> >> +  params = >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));
> >> +  

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

[Stephen Hemminger]

On Tue,  8 Nov 2016 14:04:38 -0800
Long Li  wrote:

> + spin_lock_irqsave(>retarget_msi_interrupt_lock, flags);
> +
> + params = >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, , NULL);
> + spin_unlock_irqrestore(>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.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Cathy Avery]

Hi,

Is the double semicolon a typo?

Thanks,

Cathy

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;;
 };



On 11/29/2016 06:25 PM, Bjorn Helgaas wrote:

On Tue, Nov 08, 2016 at 02:04:38PM -0800, Long Li wrote:

From: Long Li 

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 
Reported-by: Haiyang Zhang 

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(, 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(>retarget_msi_interrupt_lock, flags);
+
+   params = >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, , NULL);

+   spin_unlock_irqrestore(>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(>resources_for_children);
spin_lock_init(>config_lock);
spin_lock_init(>device_list_lock);
+   spin_lock_init(>retarget_msi_interrupt_lock);
sema_init(>enum_sem, 1);
init_completion(>remove_event);
  
--

2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel



___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Bjorn Helgaas]

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

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(, 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(>retarget_msi_interrupt_lock, flags);
> +
> + params = >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, , NULL);
> + spin_unlock_irqrestore(>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(>resources_for_children);
>   spin_lock_init(>config_lock);
>   spin_lock_init(>device_list_lock);
> + spin_lock_init(>retarget_msi_interrupt_lock);
>   sema_init(>enum_sem, 1);
>   init_completion(>remove_event);
>  
> -- 
> 2.7.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[KY Srinivasan]

> -Original Message-
> From: Bjorn Helgaas [mailto:helg...@kernel.org]
> Sent: Wednesday, November 23, 2016 3:20 PM
> To: Long Li <lon...@microsoft.com>
> Cc: KY Srinivasan <k...@microsoft.com>; Haiyang Zhang
> <haiya...@microsoft.com>; Bjorn Helgaas <bhelg...@google.com>;
> de...@linuxdriverproject.org; linux-...@vger.kernel.org; linux-
> ker...@vger.kernel.org; Long Li <lon...@microsoft.com>
> 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 <lon...@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 <lon...@microsoft.com>
> > Reported-by: Haiyang Zhang <haiya...@microsoft.com>
> 
> Waiting for a maintainer ack for this.

Acked-by: K. Y. Srinivasan <k...@microsoft.com>

> 
> $ ./scripts/get_maintainer.pl -f drivers/pci/host/pci-hyperv.c
> "K. Y. Srinivasan" <k...@microsoft.com> (maintainer:Hyper-V CORE AND
> DRIVERS)
> Haiyang Zhang <haiya...@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(, 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(>retarget_msi_interrupt_lock, flags);
> > +
> > +   params = >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, , NULL);
> > +   spin_unlock_irqrestore(>retarget_msi_interrupt_lock, flags);
> >
> > pci_msi_unmask_irq(data);
> >  }
> > @@ -2186,6 +2194,7 @@ static int hv_pci_probe(struc

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

[Bjorn Helgaas]

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

Waiting for a maintainer ack for this.

$ ./scripts/get_maintainer.pl -f drivers/pci/host/pci-hyperv.c
"K. Y. Srinivasan"  (maintainer:Hyper-V CORE AND DRIVERS)
Haiyang Zhang  (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(, 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(>retarget_msi_interrupt_lock, flags);
> +
> + params = >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, , NULL);
> + spin_unlock_irqrestore(>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(>resources_for_children);
>   spin_lock_init(>config_lock);
>   spin_lock_init(>device_list_lock);
> + spin_lock_init(>retarget_msi_interrupt_lock);
>   sema_init(>enum_sem, 1);
>   init_completion(>remove_event);
>  
> -- 
> 2.7.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Long Li]

> -Original Message-
> From: devel [mailto:driverdev-devel-boun...@linuxdriverproject.org] On
> Behalf Of Long Li
> Sent: Tuesday, November 8, 2016 8:57 AM
> To: Greg KH <gre...@linuxfoundation.org>
> Cc: linux-...@vger.kernel.org; Haiyang Zhang <haiya...@microsoft.com>;
> linux-ker...@vger.kernel.org; Bjorn Helgaas <bhelg...@google.com>;
> de...@linuxdriverproject.org
> Subject: RE: [Resend] [PATCH] pci-hyperv: use kmalloc to allocate hypercall
> params buffer
> 
> This sender failed our fraud detection checks and may not be who they
> appear to be. Learn about spoofing at http://aka.ms/LearnAboutSpoofing
> 
> > -Original Message-
> > From: Greg KH [mailto:gre...@linuxfoundation.org]
> > Sent: Monday, November 7, 2016 11:00 PM
> > To: Long Li <lon...@microsoft.com>
> > Cc: KY Srinivasan <k...@microsoft.com>; Haiyang Zhang
> > <haiya...@microsoft.com>; Bjorn Helgaas <bhelg...@google.com>;
> > de...@linuxdriverproject.org; linux-ker...@vger.kernel.org; linux-
> > p...@vger.kernel.org
> > Subject: Re: [Resend] [PATCH] pci-hyperv: use kmalloc to allocate
> > hypercall params buffer
> >
> > On Tue, Nov 08, 2016 at 12:14:14AM -0800, Long Li wrote:
> > > From: Long Li <lon...@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. Use kmalloc to allocate this buffer.
> >
> > Please wrap your changelog at 72 columns.
> >
> > >
> > > Signed-off-by: Long Li <lon...@microsoft.com>
> > > Reported-by: Haiyang Zhang <haiya...@microsoft.com>
> > > ---
> > >  drivers/pci/host/pci-hyperv.c | 24 +---
> > >  1 file changed, 13 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/drivers/pci/host/pci-hyperv.c
> > > b/drivers/pci/host/pci-hyperv.c index 763ff87..97e6daf 100644
> > > --- a/drivers/pci/host/pci-hyperv.c
> > > +++ b/drivers/pci/host/pci-hyperv.c
> > > @@ -378,6 +378,7 @@ 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;
> >
> > Can you handle potentially unaligned accesses like this?  Is there
> > some lock preventing you from using this structure more than once at the
> same time?
> >
> > >  };
> > >
> > >  /*
> > > @@ -774,7 +775,7 @@ 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;
> > > @@ -785,23 +786,24 @@ void hv_irq_unmask(struct irq_data *data)
> > > pdev = msi_desc_to_pci_dev(msi_desc);
> > > pbus = pdev->bus;
> > > hbus = container_of(pbus->sysdata, struct hv_pcibus_device,
> > > sysdata);
> > > -
> > > -   memset(, 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) |
> > > +   params = >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

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

[Long Li]

From: Long Li 

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 
Reported-by: Haiyang Zhang 
---
 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(, 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(>retarget_msi_interrupt_lock, flags);
+
+   params = >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, , NULL);
+   spin_unlock_irqrestore(>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(>resources_for_children);
spin_lock_init(>config_lock);
spin_lock_init(>device_list_lock);
+   spin_lock_init(>retarget_msi_interrupt_lock);
sema_init(>enum_sem, 1);
init_completion(>remove_event);
 
-- 
2.7.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[KY Srinivasan]

> -Original Message-
> From: Long Li
> Sent: Tuesday, November 8, 2016 8:57 AM
> To: Greg KH <gre...@linuxfoundation.org>
> Cc: KY Srinivasan <k...@microsoft.com>; Haiyang Zhang
> <haiya...@microsoft.com>; Bjorn Helgaas <bhelg...@google.com>;
> de...@linuxdriverproject.org; linux-ker...@vger.kernel.org; linux-
> p...@vger.kernel.org
> Subject: RE: [Resend] [PATCH] pci-hyperv: use kmalloc to allocate hypercall
> params buffer
> 
> 
> 
> > -Original Message-
> > From: Greg KH [mailto:gre...@linuxfoundation.org]
> > Sent: Monday, November 7, 2016 11:00 PM
> > To: Long Li <lon...@microsoft.com>
> > Cc: KY Srinivasan <k...@microsoft.com>; Haiyang Zhang
> > <haiya...@microsoft.com>; Bjorn Helgaas <bhelg...@google.com>;
> > de...@linuxdriverproject.org; linux-ker...@vger.kernel.org; linux-
> > p...@vger.kernel.org
> > Subject: Re: [Resend] [PATCH] pci-hyperv: use kmalloc to allocate hypercall
> > params buffer
> >
> > On Tue, Nov 08, 2016 at 12:14:14AM -0800, Long Li wrote:
> > > From: Long Li <lon...@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. Use kmalloc to allocate this buffer.
> >
> > Please wrap your changelog at 72 columns.

Long, this should not be part of the commit message. Please include changes in
each version below the line ()

Regards,

K. Y 
> >
> > >
> > > Signed-off-by: Long Li <lon...@microsoft.com>
> > > Reported-by: Haiyang Zhang <haiya...@microsoft.com>
> > > ---
> > >  drivers/pci/host/pci-hyperv.c | 24 +---
> > >  1 file changed, 13 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/drivers/pci/host/pci-hyperv.c
> > > b/drivers/pci/host/pci-hyperv.c index 763ff87..97e6daf 100644
> > > --- a/drivers/pci/host/pci-hyperv.c
> > > +++ b/drivers/pci/host/pci-hyperv.c
> > > @@ -378,6 +378,7 @@ 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;
> >
> > Can you handle potentially unaligned accesses like this?  Is there some lock
> > preventing you from using this structure more than once at the same time?
> >
> > >  };
> > >
> > >  /*
> > > @@ -774,7 +775,7 @@ 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;
> > > @@ -785,23 +786,24 @@ void hv_irq_unmask(struct irq_data *data)
> > >   pdev = msi_desc_to_pci_dev(msi_desc);
> > >   pbus = pdev->bus;
> > >   hbus = container_of(pbus->sysdata, struct hv_pcibus_device,
> > > sysdata);
> > > -
> > > - memset(, 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) |
> > > + params = >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, , NULL);
> > > + hv_do_hypercall(HVCALL_RETARGET_INTERRUPT, params, NULL);
> >
> > As you only use this in one spot, why not just allocate it here and then 
> > free
> > it?  Why add it to the pcibus device structure?
> 
> Thanks Greg. I will send a V2.
> 
> >
> > thanks,
> >
> > greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Long Li]

> -Original Message-
> From: Greg KH [mailto:gre...@linuxfoundation.org]
> Sent: Monday, November 7, 2016 11:00 PM
> To: Long Li <lon...@microsoft.com>
> Cc: KY Srinivasan <k...@microsoft.com>; Haiyang Zhang
> <haiya...@microsoft.com>; Bjorn Helgaas <bhelg...@google.com>;
> de...@linuxdriverproject.org; linux-ker...@vger.kernel.org; linux-
> p...@vger.kernel.org
> Subject: Re: [Resend] [PATCH] pci-hyperv: use kmalloc to allocate hypercall
> params buffer
> 
> On Tue, Nov 08, 2016 at 12:14:14AM -0800, Long Li wrote:
> > From: Long Li <lon...@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. Use kmalloc to allocate this buffer.
> 
> Please wrap your changelog at 72 columns.
> 
> >
> > Signed-off-by: Long Li <lon...@microsoft.com>
> > Reported-by: Haiyang Zhang <haiya...@microsoft.com>
> > ---
> >  drivers/pci/host/pci-hyperv.c | 24 +---
> >  1 file changed, 13 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/pci/host/pci-hyperv.c
> > b/drivers/pci/host/pci-hyperv.c index 763ff87..97e6daf 100644
> > --- a/drivers/pci/host/pci-hyperv.c
> > +++ b/drivers/pci/host/pci-hyperv.c
> > @@ -378,6 +378,7 @@ 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;
> 
> Can you handle potentially unaligned accesses like this?  Is there some lock
> preventing you from using this structure more than once at the same time?
> 
> >  };
> >
> >  /*
> > @@ -774,7 +775,7 @@ 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;
> > @@ -785,23 +786,24 @@ void hv_irq_unmask(struct irq_data *data)
> > pdev = msi_desc_to_pci_dev(msi_desc);
> > pbus = pdev->bus;
> > hbus = container_of(pbus->sysdata, struct hv_pcibus_device,
> > sysdata);
> > -
> > -   memset(, 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) |
> > +   params = >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, , NULL);
> > +   hv_do_hypercall(HVCALL_RETARGET_INTERRUPT, params, NULL);
> 
> As you only use this in one spot, why not just allocate it here and then free
> it?  Why add it to the pcibus device structure?

Thanks Greg. I will send a V2.

> 
> thanks,
> 
> greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Greg KH]

On Tue, Nov 08, 2016 at 12:14:14AM -0800, Long Li wrote:
> From: Long Li 
> 
> 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. Use kmalloc to allocate this buffer.

Please wrap your changelog at 72 columns.

> 
> Signed-off-by: Long Li 
> Reported-by: Haiyang Zhang 
> ---
>  drivers/pci/host/pci-hyperv.c | 24 +---
>  1 file changed, 13 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c
> index 763ff87..97e6daf 100644
> --- a/drivers/pci/host/pci-hyperv.c
> +++ b/drivers/pci/host/pci-hyperv.c
> @@ -378,6 +378,7 @@ 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;

Can you handle potentially unaligned accesses like this?  Is there some
lock preventing you from using this structure more than once at the same
time?

>  };
>  
>  /*
> @@ -774,7 +775,7 @@ 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;
> @@ -785,23 +786,24 @@ void hv_irq_unmask(struct irq_data *data)
>   pdev = msi_desc_to_pci_dev(msi_desc);
>   pbus = pdev->bus;
>   hbus = container_of(pbus->sysdata, struct hv_pcibus_device, sysdata);
> -
> - memset(, 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) |
> + params = >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, , NULL);
> + hv_do_hypercall(HVCALL_RETARGET_INTERRUPT, params, NULL);

As you only use this in one spot, why not just allocate it here and then
free it?  Why add it to the pcibus device structure?

thanks,

greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Long Li]

From: Long Li 

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. Use kmalloc to allocate this buffer.

Signed-off-by: Long Li 
Reported-by: Haiyang Zhang 
---
 drivers/pci/host/pci-hyperv.c | 24 +---
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c
index 763ff87..97e6daf 100644
--- a/drivers/pci/host/pci-hyperv.c
+++ b/drivers/pci/host/pci-hyperv.c
@@ -378,6 +378,7 @@ 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;
 };
 
 /*
@@ -774,7 +775,7 @@ 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;
@@ -785,23 +786,24 @@ void hv_irq_unmask(struct irq_data *data)
pdev = msi_desc_to_pci_dev(msi_desc);
pbus = pdev->bus;
hbus = container_of(pbus->sysdata, struct hv_pcibus_device, sysdata);
-
-   memset(, 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) |
+   params = >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, , NULL);
+   hv_do_hypercall(HVCALL_RETARGET_INTERRUPT, params, NULL);
 
pci_msi_unmask_irq(data);
 }
-- 
2.7.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel