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

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

[Michael S. Tsirkin]

On Tue, Dec 07, 2021 at 03:51:45PM +0800, 王贇 wrote:
> We observed issues like:
>   virtio-pci 0000:14:00.0: platform bug: legacy virtio-mmio must
>   not be used with RAM above 0x4000GB
> 
> when we have a legacy pci device which desired 32bit-pfn vq
> but gain 64bit-pfn instead, lead into the failure of probe.
> 
> vring_use_dma_api() is playing the key role in here, to help the
> allocation process understand which kind of vq it should alloc,
> however, it failed to take care the legacy pci device, which only
> have 32bit feature flag and can never have VIRTIO_F_ACCESS_PLATFORM
> setted.
> 
> This patch introduce force_dma flag to help vring_use_dma_api()
> understanding the requirement better, to avoid the failing.
> 
> Signed-off-by: Michael Wang <yun.wang@linux.alibaba.com>

This will break configs where the device appears behind
a virtual iommu, so this won't fly.
Just make your device support 1.0, eh?

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

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

[Michael S. Tsirkin]

On Tue, Dec 07, 2021 at 05:09:56PM +0800, 王贇 wrote:
> 
> 
> 在 2021/12/7 下午4:13, Michael S. Tsirkin 写道:
> > On Tue, Dec 07, 2021 at 03:51:45PM +0800, 王贇 wrote:
> > > We observed issues like:
> > >    virtio-pci 0000:14:00.0: platform bug: legacy virtio-mmio must
> > >    not be used with RAM above 0x4000GB
> > > 
> > > when we have a legacy pci device which desired 32bit-pfn vq
> > > but gain 64bit-pfn instead, lead into the failure of probe.
> > > 
> > > vring_use_dma_api() is playing the key role in here, to help the
> > > allocation process understand which kind of vq it should alloc,
> > > however, it failed to take care the legacy pci device, which only
> > > have 32bit feature flag and can never have VIRTIO_F_ACCESS_PLATFORM
> > > setted.
> > > 
> > > This patch introduce force_dma flag to help vring_use_dma_api()
> > > understanding the requirement better, to avoid the failing.
> > > 
> > > Signed-off-by: Michael Wang <yun.wang@linux.alibaba.com>
> > 
> > This will break configs where the device appears behind
> > a virtual iommu, so this won't fly.
> > Just make your device support 1.0, eh?
> 
> 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

There's an old idea:
https://www.spinics.net/lists/kernel/msg3248800.html


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

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

[Michael S. Tsirkin]

On Tue, Dec 07, 2021 at 05:09:56PM +0800, 王贇 wrote:
> 
> 
> 在 2021/12/7 下午4:13, Michael S. Tsirkin 写道:
> > On Tue, Dec 07, 2021 at 03:51:45PM +0800, 王贇 wrote:
> > > We observed issues like:
> > >    virtio-pci 0000:14:00.0: platform bug: legacy virtio-mmio must
> > >    not be used with RAM above 0x4000GB
> > > 
> > > when we have a legacy pci device which desired 32bit-pfn vq
> > > but gain 64bit-pfn instead, lead into the failure of probe.
> > > 
> > > vring_use_dma_api() is playing the key role in here, to help the
> > > allocation process understand which kind of vq it should alloc,
> > > however, it failed to take care the legacy pci device, which only
> > > have 32bit feature flag and can never have VIRTIO_F_ACCESS_PLATFORM
> > > setted.
> > > 
> > > This patch introduce force_dma flag to help vring_use_dma_api()
> > > understanding the requirement better, to avoid the failing.
> > > 
> > > Signed-off-by: Michael Wang <yun.wang@linux.alibaba.com>
> > 
> > This will break configs where the device appears behind
> > a virtual iommu, so this won't fly.
> > Just make your device support 1.0, eh?
> 
> 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?


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

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

[Michael S. Tsirkin]

On Wed, Dec 08, 2021 at 04:04:19PM +0800, 王贇 wrote:
> 
> 
> 在 2021/12/8 下午3:23, Michael S. Tsirkin 写道:
> > On Tue, Dec 07, 2021 at 05:09:56PM +0800, 王贇 wrote:
> > > 
> > > 
> > > 在 2021/12/7 下午4:13, Michael S. Tsirkin 写道:
> > > > On Tue, Dec 07, 2021 at 03:51:45PM +0800, 王贇 wrote:
> > > > > We observed issues like:
> > > > >     virtio-pci 0000:14:00.0: platform bug: legacy virtio-mmio must
> > > > >     not be used with RAM above 0x4000GB
> > > > > 
> > > > > when we have a legacy pci device which desired 32bit-pfn vq
> > > > > but gain 64bit-pfn instead, lead into the failure of probe.
> > > > > 
> > > > > vring_use_dma_api() is playing the key role in here, to help the
> > > > > allocation process understand which kind of vq it should alloc,
> > > > > however, it failed to take care the legacy pci device, which only
> > > > > have 32bit feature flag and can never have VIRTIO_F_ACCESS_PLATFORM
> > > > > setted.
> > > > > 
> > > > > This patch introduce force_dma flag to help vring_use_dma_api()
> > > > > understanding the requirement better, to avoid the failing.
> > > > > 
> > > > > Signed-off-by: Michael Wang <yun.wang@linux.alibaba.com>
> > > > 
> > > > This will break configs where the device appears behind
> > > > a virtual iommu, so this won't fly.
> > > > Just make your device support 1.0, eh?
> > > 
> > > 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 :-)

virtio-pci 0000:14:00.0: platform bug: legacy virtio-mmio must ...

should be virtio-pci not virtio-mmio



> 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

Hmm wait a sec is it a physical device or a hypervisor?
If a physical one then doesn't it need VIRTIO_F_ORDER_PLATFORM
on ARM?



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

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

[Michael S. Tsirkin]

On Thu, Dec 09, 2021 at 11:21:36AM +0800, 王贇 wrote:
> 
> 
> 在 2021/12/8 下午7:08, 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 :-)
> > 
> > virtio-pci 0000:14:00.0: platform bug: legacy virtio-mmio must ...
> > 
> > should be virtio-pci not virtio-mmio
> 
> Patch on the way~
> 
> > 
> > 
> > 
> > > 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
> > 
> > Hmm wait a sec is it a physical device or a hypervisor?
> > If a physical one then doesn't it need VIRTIO_F_ORDER_PLATFORM
> > on ARM?
> 
> The PCI device is virtual, I can't see how VIRTIO_F_ORDER_PLATFORM help
> address this issue, legacy pci config is 32bit but it's 36, seems like will
> never be included?
> 
> Regards,
> Michael Wang

Oh, if the device is virtual then I think you should just update it please.
virtio 0.X is architecturally limited to small VMs,
if your hypervisor supports more it should emulate virtio 1.0.



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

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

[Michael S. Tsirkin]

On Thu, Dec 09, 2021 at 04:26:47PM +0800, 王贇 wrote:
> 
> 
> 在 2021/12/9 下午2:40, Michael S. Tsirkin 写道:
> [snip]
> > > > > 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
> > > > 
> > > > Hmm wait a sec is it a physical device or a hypervisor?
> > > > If a physical one then doesn't it need VIRTIO_F_ORDER_PLATFORM
> > > > on ARM?
> > > 
> > > The PCI device is virtual, I can't see how VIRTIO_F_ORDER_PLATFORM help
> > > address this issue, legacy pci config is 32bit but it's 36, seems like will
> > > never be included?
> > > 
> > > Regards,
> > > Michael Wang
> > 
> > Oh, if the device is virtual then I think you should just update it please.
> > virtio 0.X is architecturally limited to small VMs,
> > if your hypervisor supports more it should emulate virtio 1.0.
> 
> I see, nice to confirm the proper approach, although we don't have that
> option on the desk :-P

Don't see why, a stronger justification will be needed before everyone
takes on the maintainance burden of maintaining hacks like this.  If we
make an exception here this opens floodgates for everyone too lazy to
add virtio 1 support to instead push hacks at the linux level.


> So as long as we don't have any iommu enabled, the force dma approach could
> be safe, is this correct?
> 
> Regards,
> Michael Wang

Not unless there's an API at the DMA API level that guarantees DMA
addresses are physical addresses.

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

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

[Michael S. Tsirkin]

On Wed, Dec 08, 2021 at 04:04:19PM +0800, 王贇 wrote:
> 
> 
> 在 2021/12/8 下午3:23, Michael S. Tsirkin 写道:
> > On Tue, Dec 07, 2021 at 05:09:56PM +0800, 王贇 wrote:
> > > 
> > > 
> > > 在 2021/12/7 下午4:13, Michael S. Tsirkin 写道:
> > > > On Tue, Dec 07, 2021 at 03:51:45PM +0800, 王贇 wrote:
> > > > > We observed issues like:
> > > > >     virtio-pci 0000:14:00.0: platform bug: legacy virtio-mmio must
> > > > >     not be used with RAM above 0x4000GB
> > > > > 
> > > > > when we have a legacy pci device which desired 32bit-pfn vq
> > > > > but gain 64bit-pfn instead, lead into the failure of probe.
> > > > > 
> > > > > vring_use_dma_api() is playing the key role in here, to help the
> > > > > allocation process understand which kind of vq it should alloc,
> > > > > however, it failed to take care the legacy pci device, which only
> > > > > have 32bit feature flag and can never have VIRTIO_F_ACCESS_PLATFORM
> > > > > setted.
> > > > > 
> > > > > This patch introduce force_dma flag to help vring_use_dma_api()
> > > > > understanding the requirement better, to avoid the failing.
> > > > > 
> > > > > Signed-off-by: Michael Wang <yun.wang@linux.alibaba.com>
> > > > 
> > > > This will break configs where the device appears behind
> > > > a virtual iommu, so this won't fly.
> > > > Just make your device support 1.0, eh?
> > > 
> > > 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?
Figuring out we have this problematic config and then allocating just
the ring from coherent memory seems more palatable.

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.


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

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

[Michael S. Tsirkin]

On Thu, Dec 09, 2021 at 11:00:55AM +0800, 王贇 wrote:
> 
> 
> 在 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


The idea was that on some systems any DMA address is also a
physical address (as in the case of e.g. bounce buffers) and
then it can be used without VIRTIO_F_ACCESS_PLATFORM.

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

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization