Re: [PATCH] irqchip/gic-v3-its: Lock its device list during find and create its device

[Zheng Xiang <zhengxiang9@huawei.com>]

Hi Marc,

On 2019/1/29 13:42, Zheng Xiang wrote:
> On 2019/1/28 21:51, Marc Zyngier wrote:
>> On 28/01/2019 07:13, Zheng Xiang wrote:
>>> Hi Marc,
>>>
>>> Thanks for your review.
>>>
>>> On 2019/1/26 19:38, Marc Zyngier wrote:
>>>> Hi Zheng,
>>>>
>>>> On Sat, 26 Jan 2019 06:16:24 +0000,
>>>> Zheng Xiang <zhengxiang9@huawei.com> wrote:
>>>>>
>>>>> Currently each PCI device under a PCI Bridge shares the same device id
>>>>> and ITS device. Assume there are two PCI devices call its_msi_prepare
>>>>> concurrently and they are both going to find and create their ITS
>>>>> device. There is a chance that the later one couldn't find ITS device
>>>>> before the other one creating the ITS device. It will cause the later
>>>>> one to create a different ITS device even if they have the same
>>>>> device_id.
>>>>
>>>> Interesting finding. Is this something you've actually seen in practice
>>>> with two devices being probed in parallel? Or something that you found
>>>> by inspection?
>>>
>>> Yes, I find this problem after analyzing the reason of VM hung. At last, I
>>> find that the virtio-gpu cannot receive the MSI interrupts due to sharing
>>> a same event_id as virtio-serial.
>>>
>>> See https://lkml.org/lkml/2019/1/10/299 for the bug report.
>>>
>>> This problem can be reproducted with high probability by booting a Qemu/KVM
>>> VM with a virtio-serial controller and a virtio-gpu adding to a PCI Bridge
>>> and also adding some delay before creating ITS device.
>>
>> Fair enough. Do you mind sharing your QEMU command line? It'd be useful
>> if I could reproduce it here (and would give me a way to check that it
>> doesn't regress).
> 

Have you reproduced it with my QEMU command line?

If so, should I send a V2 patch with your suggestion?

> Yes of course, my QEMU command line is below:
> 
> qemu-system-aarch64 \
>     -name guest=arm64 \
>     -machine virt,accel=kvm,usb=off,gic-version=3 \
>     -cpu host \
>     -bios /usr/share/edk2/aarch64/QEMU_EFI.fd \
>     -nodefaults \
>     -m 2048 \
>     -smp 1 \
>     -device ioh3420,port=0x8,chassis=1,id=pci.1,bus=pcie.0,multifunction=on,addr=0x1 \
>     -device i82801b11-bridge,id=pci.2,bus=pcie.0,addr=0x2 \
>     -device pci-bridge,chassis_nr=3,id=pci.3,bus=pci.2,addr=0x0 \
>     -device ioh3420,port=0x9,chassis=4,id=pci.4,bus=pcie.0,addr=0x1.0x1 \
>     -device virtio-scsi-pci,id=scsi0,bus=pci.4,addr=0x0 \
>     -drive file=/home/zhengxiang/tmp.raw,format=raw,if=none,id=drive-scsi0-0-0-0,cache=none,aio=threads \
>     -device scsi-hd,bus=scsi0.0,channel=0,scsi-id=0,lun=0,drive=drive-scsi0-0-0-0,id=scsi0-0-0-0,bootindex=1 \
>     -drive file=/dev/mapper/VolGroup00-lvol_7,format=raw,if=none,id=drive-virtio-disk0,cache=none,aio=threads \
>     -device virtio-blk-pci,scsi=off,bus=pci.1,addr=0x0,drive=drive-virtio-disk0,id=virtio-disk0 \
>     -device virtio-gpu-pci,id=video0,bus=pci.3,addr=0x2 \
>     -device virtio-serial-pci,id=virtio-serial0,bus=pci.3,addr=0x3 \
>     -device usb-ehci,id=usb,bus=pci.3,addr=0x1 \
>     -device usb-kbd,id=input1,bus=usb.0,port=2 \
>     -monitor telnet:0.0.0.0:22222,server,nowait \
>     -vnc 0.0.0.0:8 \
>     -msg timestamp=on \
>     -serial stdio \
> 
> Add *msleep* between *its_find_device* and *its_create_device* to increase the
> rate of probability, .
> 
>>
>>>
>>>>
>>>> The whole RID aliasing is such a mess, I wish we never supported
>>>> it. Anyway, comments below.
>>>>
>>>>>
>>>>> Signed-off-by: Zheng Xiang <zhengxiang9@huawei.com>
>>>>> ---
>>>>>  drivers/irqchip/irq-gic-v3-its.c | 52 +++++++++++++++-------------------------
>>>>>  1 file changed, 19 insertions(+), 33 deletions(-)
>>>>>
>>>>> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
>>>>> index db20e99..397edc8 100644
>>>>> --- a/drivers/irqchip/irq-gic-v3-its.c
>>>>> +++ b/drivers/irqchip/irq-gic-v3-its.c
>>>>> @@ -2205,25 +2205,6 @@ static void its_cpu_init_collections(void)
>>>>>  	raw_spin_unlock(&its_lock);
>>>>>  }
>>>>>  
>>>>> -static struct its_device *its_find_device(struct its_node *its, u32 dev_id)
>>>>> -{
>>>>> -	struct its_device *its_dev = NULL, *tmp;
>>>>> -	unsigned long flags;
>>>>> -
>>>>> -	raw_spin_lock_irqsave(&its->lock, flags);
>>>>> -
>>>>> -	list_for_each_entry(tmp, &its->its_device_list, entry) {
>>>>> -		if (tmp->device_id == dev_id) {
>>>>> -			its_dev = tmp;
>>>>> -			break;
>>>>> -		}
>>>>> -	}
>>>>> -
>>>>> -	raw_spin_unlock_irqrestore(&its->lock, flags);
>>>>> -
>>>>> -	return its_dev;
>>>>> -}
>>>>> -
>>>>>  static struct its_baser *its_get_baser(struct its_node *its, u32 type)
>>>>>  {
>>>>>  	int i;
>>>>> @@ -2321,7 +2302,7 @@ static bool its_alloc_vpe_table(u32 vpe_id)
>>>>>  static struct its_device *its_create_device(struct its_node *its, u32 dev_id,
>>>>>  					    int nvecs, bool alloc_lpis)
>>>>>  {
>>>>> -	struct its_device *dev;
>>>>> +	struct its_device *dev = NULL, *tmp;
>>>>>  	unsigned long *lpi_map = NULL;
>>>>>  	unsigned long flags;
>>>>>  	u16 *col_map = NULL;
>>>>> @@ -2331,6 +2312,24 @@ static struct its_device *its_create_device(struct its_node *its, u32 dev_id,
>>>>>  	int nr_ites;
>>>>>  	int sz;
>>>>>  
>>>>> +	raw_spin_lock_irqsave(&its->lock, flags);
>>>>> +	list_for_each_entry(tmp, &its->its_device_list, entry) {
>>>>> +		if (tmp->device_id == dev_id) {
>>>>> +			dev = tmp;
>>>>> +			break;
>>>>> +		}
>>>>> +	}
>>>>> +	if (dev) {
>>>>> +		/*
>>>>> +		 * We already have seen this ID, probably through
>>>>> +		 * another alias (PCI bridge of some sort). No need to
>>>>> +		 * create the device.
>>>>> +		 */
>>>>> +		pr_debug("Reusing ITT for devID %x\n", dev_id);
>>>>> +		raw_spin_unlock_irqrestore(&its->lock, flags);
>>>>> +		return dev;
>>>>> +	}
>>>>> +
>>>>>  	if (!its_alloc_device_table(its, dev_id))
>>>>
>>>> You're now performing all sort of allocations in an atomic context,
>>>> which is pretty horrible (and the kernel will shout at you for doing
>>>> so).
>>>>
>>>> We could probably keep the current logic and wrap it around a mutex
>>>> instead, which would give us the appropriate guarantees WRT allocations.
>>>> Something along those lines (untested):>
>>>> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
>>>> index db20e992a40f..99feb62e63ba 100644
>>>> --- a/drivers/irqchip/irq-gic-v3-its.c
>>>> +++ b/drivers/irqchip/irq-gic-v3-its.c
>>>> @@ -97,9 +97,14 @@ struct its_device;
>>>>   * The ITS structure - contains most of the infrastructure, with the
>>>>   * top-level MSI domain, the command queue, the collections, and the
>>>>   * list of devices writing to it.
>>>> + *
>>>> + * alloc_lock has to be taken for any allocation that can happen at
>>>> + * run time, while the spinlock must be taken to parse data structures
>>>> + * such as the device list.
>>>>   */
>>>>  struct its_node {
>>>>  	raw_spinlock_t		lock;
>>>> +	struct mutex		alloc_lock;
>>>>  	struct list_head	entry;
>>>>  	void __iomem		*base;
>>>>  	phys_addr_t		phys_base;
>>>> @@ -2421,6 +2426,7 @@ static int its_msi_prepare(struct irq_domain *domain, struct device *dev,
>>>>  	struct its_device *its_dev;
>>>>  	struct msi_domain_info *msi_info;
>>>>  	u32 dev_id;
>>>> +	int err = 0;
>>>>  
>>>>  	/*
>>>>  	 * We ignore "dev" entierely, and rely on the dev_id that has
>>>> @@ -2443,6 +2449,7 @@ static int its_msi_prepare(struct irq_domain *domain, struct device *dev,
>>>>  		return -EINVAL;
>>>>  	}
>>>>  
>>>> +	mutex_lock(&its->alloc_lock);
>>>>  	its_dev = its_find_device(its, dev_id);
>>>>  	if (its_dev) {
>>>>  		/*
>>>> @@ -2455,11 +2462,14 @@ static int its_msi_prepare(struct irq_domain *domain, struct device *dev,
>>>>  	}
>>>>  
>>>>  	its_dev = its_create_device(its, dev_id, nvec, true);
>>>> -	if (!its_dev)
>>>> -		return -ENOMEM;
>>>> +	if (!its_dev) {
>>>> +		err = -ENOMEM;
>>>> +		goto out;
>>>> +	}
>>>>  
>>>>  	pr_debug("ITT %d entries, %d bits\n", nvec, ilog2(nvec));
>>>>  out:
>>>> +	mutex_unlock(&its->alloc_lock);
>>>>  	info->scratchpad[0].ptr = its_dev;>>>  	return 0;
>>>
>>> Should it return *err* here?
>>
>> Absolutely. Does it fix the problem for you?
> 
> Yes, VM doesn't get hung anymore after thousands of times of boot/reboot.
> 
>>
>> Thanks,
>>
>> 	M.
>>
-- 

Thanks,
Xiang