Re: [Xen-devel] [PATCH 1/4] xen/pciback: Check PF instead of VF for PCI_COMMAND_MEMORY

Re: [Xen-devel] [PATCH 1/4] xen/pciback: Check PF instead of VF for PCI_COMMAND_MEMORY

[Jan Beulich]

>>> On 11.02.16 at 22:10, <konrad.wilk@oracle.com> wrote:
> c/s 408fb0e5aa7fda0059db282ff58c3b2a4278baa0
> "xen/pciback: Don't allow MSI-X ops if PCI_COMMAND_MEMORY is not set."
> would check the device for PCI_COMMAND_MEMORY which is great.
> Except that VF devices are unique - for example they have no
> legacy interrupts, and also any writes to PCI_COMMAND_MEMORY
> are silently ignored (by the hardware).
> 
> CC: stable@vger.kernel.org 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

albeit ...

> --- a/drivers/xen/xen-pciback/pciback_ops.c
> +++ b/drivers/xen/xen-pciback/pciback_ops.c
> @@ -213,6 +213,7 @@ int xen_pcibk_enable_msix(struct xen_pcibk_device *pdev,
>  	int i, result;
>  	struct msix_entry *entries;
>  	u16 cmd;
> +	struct pci_dev *phys_dev;
>  
>  	if (unlikely(verbose_request))
>  		printk(KERN_DEBUG DRV_NAME ": %s: enable MSI-X\n",
> @@ -227,8 +228,10 @@ int xen_pcibk_enable_msix(struct xen_pcibk_device *pdev,
>  	/*
>  	 * PCI_COMMAND_MEMORY must be enabled, otherwise we may not be able
>  	 * to access the BARs where the MSI-X entries reside.
> +	 * But VF devices are unique in which the PF needs to be checked.
>  	 */
> -	pci_read_config_word(dev, PCI_COMMAND, &cmd);
> +	phys_dev = pci_physfn(dev);
> +	pci_read_config_word(phys_dev, PCI_COMMAND, &cmd);

... I don't think the use of a local variable here is really needed.

Jan

Re: [Xen-devel] [PATCH 2/4] xen/pciback: Save the number of MSI-X entries to be copied later.

[Jan Beulich]

>>> On 11.02.16 at 22:10, <konrad.wilk@oracle.com> wrote:
> c/s  8135cf8b092723dbfcc611fe6fdcb3a36c9951c5
> "xen/pciback: Save xen_pci_op commands before processing it"
> would copyback the processed values - which was great.
> 
> However it missed the case that xen_pcibk_enable_msix - when
> completing would overwrite op->value (which had the number
> of MSI-X vectors requested) with the return value (which for
> success was zero). Hence the copy-back routine (which would use
> op->value) would copy exactly zero MSI-X vectors back.
> 
> CC: stable@vger.kernel.org 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

but I'd prefer to see ...

> --- a/drivers/xen/xen-pciback/pciback_ops.c
> +++ b/drivers/xen/xen-pciback/pciback_ops.c
> @@ -335,7 +335,9 @@ void xen_pcibk_do_op(struct work_struct *data)
>  	struct xen_pcibk_dev_data *dev_data = NULL;
>  	struct xen_pci_op *op = &pdev->op;
>  	int test_intx = 0;
> -
> +#ifdef CONFIG_PCI_MSI
> +	unsigned int nr = 0;
> +#endif
>  	*op = pdev->sh_info->op;
>  	barrier();
>  	dev = xen_pcibk_get_pci_dev(pdev, op->domain, op->bus, op->devfn);

... the blank line separating declarations from statements to stay.

Jan

Re: [PATCH 4/4] xen/pcifront: Fix mysterious crashes when NUMA locality information was extracted.

[Boris Ostrovsky]

On 02/11/2016 04:10 PM, Konrad Rzeszutek Wilk wrote:
>
> This patch fixes the issue by:
>   1) Use kzalloc to initialize to a well known state.
>   2) Put 'struct pci_sysdata' at the start of 'pcifront_sd'. That
>      way access to the 'node' will access the right offset.
>
> CC: stable@vger.kernel.org
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>

(This, btw, is the second time we got bitten by pcifront_sd bit not 
being pci_sysdata. dc4fdaf0e48 was a workaround for a similar problem 
and we should have fixed it then).

-boris

Re: [PATCH 4/4] xen/pcifront: Fix mysterious crashes when NUMA locality information was extracted.

[Boris Ostrovsky]

On 02/15/2016 09:05 AM, Konrad Rzeszutek Wilk wrote:
> On Sat, Feb 13, 2016 at 08:23:14PM -0500, Boris Ostrovsky wrote:
>> On 02/11/2016 04:10 PM, Konrad Rzeszutek Wilk wrote:
>>> This patch fixes the issue by:
>>>   1) Use kzalloc to initialize to a well known state.
>>>   2) Put 'struct pci_sysdata' at the start of 'pcifront_sd'. That
>>>      way access to the 'node' will access the right offset.
>>>
>>> CC: stable@vger.kernel.org
>>> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>> Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>>
>> (This, btw, is the second time we got bitten by pcifront_sd bit not being
>> pci_sysdata. dc4fdaf0e48 was a workaround for a similar problem and we
>> should have fixed it then).
> I think that the dc4fdaf0e4839109169d8261814813816951c75f commit can be
> reverted then?
>
> Ah no, b/c:
> "    Fixes: 97badf873ab6 (device property: Make it possible to use secondary firmware nodes)"

Right --- the problem which that commit fixed was not specific to Xen 
but could be observed on ia64 as well.

-boris

Re: [Xen-devel] [PATCH 4/4] xen/pcifront: Fix mysterious crashes when NUMA locality information was extracted.

[David Vrabel]

On 11/02/16 21:10, Konrad Rzeszutek Wilk wrote:
> --- a/drivers/pci/xen-pcifront.c
> +++ b/drivers/pci/xen-pcifront.c

> @@ -576,6 +578,7 @@ static void pcifront_free_roots(struct pcifront_device *pdev)
>  		free_root_bus_devs(bus_entry->bus);
>  
>  		kfree(bus_entry->bus->sysdata);
> +		bus_entry->bus->sysdata = NULL;

I dropped this change, as it was unrelated.

I note that we free sysdata before removing the bus which looks racy to me.

David