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

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