linux-kernel - Re: [RFC PATCH] virtio: make sure legacy pci device gain 32bit-pfn vq

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 9 Dec 2021 11:00:55 +0800
From:   王贇 <yun.wang@...ux.alibaba.com>
To:     "Michael S. Tsirkin" <mst@...hat.com>
Cc:     Jason Wang <jasowang@...hat.com>,
        "open list:VIRTIO CORE AND NET DRIVERS" 
        <virtualization@...ts.linux-foundation.org>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH] virtio: make sure legacy pci device gain 32bit-pfn vq



在 2021/12/9 上午8:19, Michael S. Tsirkin 写道:
[snip]
>>>>>>
>>>> Hi, Michael
>>>>
>>>> Thanks for the comment, unfortunately modify device is not an option for us
>>>> :-(
>>>>
>>>> Is there any idea on how to solve this issue properly?
>>>>
>>>> Regards,
>>>> Michael Wang
>>>
>>> By the way, there is a bug in the error message. Want to fix that?
>>
>> Could you please provide more detail about the bug? We'd like to help fixing
>> it :-)
>>
>> Besides, I've checked that patch but it can't address our issue, we actually
>> have this legacy pci device on arm platform, and the memory layout is
>> unfriendly since allocation rarely providing page-address below 44bit, we
>> understand the virtio-iommu case should not do force dma, while we don't
>> have that so it's just working fine.
>>
>> Regards,
>> Michael Wang
> 
> BTW is it just the ring that's at issue?

Yes, the dma address for ring allocated as page can't fit the requirement.

> Figuring out we have this problematic config and then allocating just
> the ring from coherent memory seems more palatable.

Agree, I'm also wondering why can't we force alloc 44bit-pfn page to fit 
the requirement? I mean if there are such pages, we should use them 
firstly as dma address for legacy devices, and only fail when there are 
no such pages at all, but can't find existing API to alloc page with 
such requirement... anyway.

> 
> But please note we still need to detect config with a virtual iommu (can
> be any kind not just virtio-iommu, smmu, vtd are all affected) and
> disable the hacks. This is what the new DMA API I suggested would do.

Fair enough, any more details about the design of new API?

Regards,
Michael Wang


> 
> 
>>>
>>>
>>>>>
>>>>>> ---
>>>>>>     drivers/virtio/virtio_pci_legacy.c | 10 ++++++++++
>>>>>>     drivers/virtio/virtio_ring.c       |  3 +++
>>>>>>     include/linux/virtio.h             |  1 +
>>>>>>     3 files changed, 14 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/virtio/virtio_pci_legacy.c
>>>>>> b/drivers/virtio/virtio_pci_legacy.c
>>>>>> index d62e983..11f2ebf 100644
>>>>>> --- a/drivers/virtio/virtio_pci_legacy.c
>>>>>> +++ b/drivers/virtio/virtio_pci_legacy.c
>>>>>> @@ -263,6 +263,16 @@ int virtio_pci_legacy_probe(struct virtio_pci_device
>>>>>> *vp_dev)
>>>>>>     	vp_dev->setup_vq = setup_vq;
>>>>>>     	vp_dev->del_vq = del_vq;
>>>>>>
>>>>>> +	/*
>>>>>> +	 * The legacy pci device requre 32bit-pfn vq,
>>>>>> +	 * or setup_vq() will failed.
>>>>>> +	 *
>>>>>> +	 * Thus we make sure vring_use_dma_api() will
>>>>>> +	 * return true during the allocation by marking
>>>>>> +	 * force_dma here.
>>>>>> +	 */
>>>>>> +	vp_dev->vdev.force_dma = true;
>>>>>> +
>>>>>>     	return 0;
>>>>>>
>>>>>>     err_iomap:
>>>>>> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
>>>>>> index 3035bb6..6562e01 100644
>>>>>> --- a/drivers/virtio/virtio_ring.c
>>>>>> +++ b/drivers/virtio/virtio_ring.c
>>>>>> @@ -245,6 +245,9 @@ static inline bool virtqueue_use_indirect(struct
>>>>>> virtqueue *_vq,
>>>>>>
>>>>>>     static bool vring_use_dma_api(struct virtio_device *vdev)
>>>>>>     {
>>>>>> +	if (vdev->force_dma)
>>>>>> +		return true;
>>>>>> +
>>>>>>     	if (!virtio_has_dma_quirk(vdev))
>>>>>>     		return true;
>>>>>>
>>>>>> diff --git a/include/linux/virtio.h b/include/linux/virtio.h
>>>>>> index 41edbc0..a4eb29d 100644
>>>>>> --- a/include/linux/virtio.h
>>>>>> +++ b/include/linux/virtio.h
>>>>>> @@ -109,6 +109,7 @@ struct virtio_device {
>>>>>>     	bool failed;
>>>>>>     	bool config_enabled;
>>>>>>     	bool config_change_pending;
>>>>>> +	bool force_dma;
>>>>>>     	spinlock_t config_lock;
>>>>>>     	spinlock_t vqs_list_lock; /* Protects VQs list access */
>>>>>>     	struct device dev;
>>>>>> -- 
>>>>>> 1.8.3.1