Re: [PATCH 2/2] virtio/virtio_ring: Fix the dma_max_mapping_size call
Hi Christoph, Michael,
On 7/25/19 3:04 PM, Christoph Hellwig wrote:
> On Thu, Jul 25, 2019 at 01:53:49PM +0200, Auger Eric wrote:
>> I am confused: if vring_use_dma_api() returns false if the dma_mask is
>> unset (ie. vring_use_dma_api() returns false), the virtio-blk-pci device
>> will not be able to get translated addresses and won't work properly.
>>
>> The patch above allows the dma api to be used and only influences the
>> max_segment_size and it works properly.
>>
>> So is it normal the dma_mask is unset in my case?
>
> Its not normal. I assume you use virtio-nmio? Due to the mess with
> the dma_mask being a pointer a lot of subsystems forgot to set a dma
> mask up, and oddly enough seem to mostly get away with it.
>
No the issue is encountered with virtio-blk-pci
I think the problem is virtio_max_dma_size() is called from
virtblk_probe (virtio_blk.c) on the virtio device and not the actual
virtio_pci_device which has a dma_mask set. I don't think the virtio
device ever has a dma_mask set.
We do not hit the guest crash on the virtio-net-pci device as the
virtio-net driver does not call virtio_max_dma_size() on the virtio
device.
Does fd1068e1860e ("virtio-blk: Consider virtio_max_dma_size() for
maximum segment size") call virtio_max_dma_size() on the right device?
Thanks
Eric
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 2/2] virtio/virtio_ring: Fix the dma_max_mapping_size call
On Thu, Jul 25, 2019 at 01:53:49PM +0200, Auger Eric wrote: > I am confused: if vring_use_dma_api() returns false if the dma_mask is > unset (ie. vring_use_dma_api() returns false), the virtio-blk-pci device > will not be able to get translated addresses and won't work properly. > > The patch above allows the dma api to be used and only influences the > max_segment_size and it works properly. > > So is it normal the dma_mask is unset in my case? Its not normal. I assume you use virtio-nmio? Due to the mess with the dma_mask being a pointer a lot of subsystems forgot to set a dma mask up, and oddly enough seem to mostly get away with it. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 2/2] virtio/virtio_ring: Fix the dma_max_mapping_size call
Hi,
On 7/23/19 5:38 PM, Christoph Hellwig wrote:
> On Mon, Jul 22, 2019 at 04:36:09PM +0100, Robin Murphy wrote:
>>> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
>>> index c8be1c4f5b55..37c143971211 100644
>>> --- a/drivers/virtio/virtio_ring.c
>>> +++ b/drivers/virtio/virtio_ring.c
>>> @@ -262,7 +262,7 @@ size_t virtio_max_dma_size(struct virtio_device *vdev)
>>> {
>>> size_t max_segment_size = SIZE_MAX;
>>> - if (vring_use_dma_api(vdev))
>>> + if (vring_use_dma_api(vdev) && vdev->dev.dma_mask)
>>
>> Hmm, might it make sense to roll that check up into vring_use_dma_api()
>> itself? After all, if the device has no mask then it's likely that other
>> DMA API ops wouldn't really work as expected either.
>
> Makes sense to me.
>
I am confused: if vring_use_dma_api() returns false if the dma_mask is
unset (ie. vring_use_dma_api() returns false), the virtio-blk-pci device
will not be able to get translated addresses and won't work properly.
The patch above allows the dma api to be used and only influences the
max_segment_size and it works properly.
So is it normal the dma_mask is unset in my case?
Thanks
Eric
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 2/2] virtio/virtio_ring: Fix the dma_max_mapping_size call
On Wed, Jul 24, 2019 at 06:10:53PM -0400, Michael S. Tsirkin wrote: > Christoph - would a documented API wrapping dma_mask make sense? > With the documentation explaining how users must > desist from using DMA APIs if that returns false ... We have some bigger changes in this are planned, including turning dma_mask into a scalar instead of a pointer, please stay tuned. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 2/2] virtio/virtio_ring: Fix the dma_max_mapping_size call
On Tue, Jul 23, 2019 at 05:38:51PM +0200, Christoph Hellwig wrote:
> On Mon, Jul 22, 2019 at 04:36:09PM +0100, Robin Murphy wrote:
> >> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> >> index c8be1c4f5b55..37c143971211 100644
> >> --- a/drivers/virtio/virtio_ring.c
> >> +++ b/drivers/virtio/virtio_ring.c
> >> @@ -262,7 +262,7 @@ size_t virtio_max_dma_size(struct virtio_device *vdev)
> >> {
> >>size_t max_segment_size = SIZE_MAX;
> >> -if (vring_use_dma_api(vdev))
> >> + if (vring_use_dma_api(vdev) && vdev->dev.dma_mask)
> >
> > Hmm, might it make sense to roll that check up into vring_use_dma_api()
> > itself? After all, if the device has no mask then it's likely that other
> > DMA API ops wouldn't really work as expected either.
>
> Makes sense to me.
Christoph - would a documented API wrapping dma_mask make sense?
With the documentation explaining how users must
desist from using DMA APIs if that returns false ...
--
MST
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 2/2] virtio/virtio_ring: Fix the dma_max_mapping_size call
On Tue, Jul 23, 2019 at 05:38:30PM +0200, Christoph Hellwig wrote:
> On Mon, Jul 22, 2019 at 11:33:35AM -0400, Michael S. Tsirkin wrote:
> > On Mon, Jul 22, 2019 at 04:55:09PM +0200, Eric Auger wrote:
> > > Do not call dma_max_mapping_size for devices that have no DMA
> > > mask set, otherwise we can hit a NULL pointer dereference.
> > >
> > > This occurs when a virtio-blk-pci device is protected with
> > > a virtual IOMMU.
> > >
> > > Fixes: e6d6dd6c875e ("virtio: Introduce virtio_max_dma_size()")
> > > Signed-off-by: Eric Auger
> > > Suggested-by: Christoph Hellwig
> >
> > Christoph, I wonder why did you suggest this?
> > The connection between dma_mask and dma_max_mapping_size
> > is far from obvious. The documentation doesn't exist.
> > Do we really have to teach all users about this hack?
> > Why not just make dma_max_mapping_size DTRT?
>
> Because we should not call into dma API functions for devices that
> are not DMA capable.
I'd rather call is_device_dma_capable then, better than poking
at DMA internals.
--
MST
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 2/2] virtio/virtio_ring: Fix the dma_max_mapping_size call
On Mon, Jul 22, 2019 at 04:36:09PM +0100, Robin Murphy wrote:
>> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
>> index c8be1c4f5b55..37c143971211 100644
>> --- a/drivers/virtio/virtio_ring.c
>> +++ b/drivers/virtio/virtio_ring.c
>> @@ -262,7 +262,7 @@ size_t virtio_max_dma_size(struct virtio_device *vdev)
>> {
>> size_t max_segment_size = SIZE_MAX;
>> - if (vring_use_dma_api(vdev))
>> +if (vring_use_dma_api(vdev) && vdev->dev.dma_mask)
>
> Hmm, might it make sense to roll that check up into vring_use_dma_api()
> itself? After all, if the device has no mask then it's likely that other
> DMA API ops wouldn't really work as expected either.
Makes sense to me.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 2/2] virtio/virtio_ring: Fix the dma_max_mapping_size call
On Mon, Jul 22, 2019 at 11:33:35AM -0400, Michael S. Tsirkin wrote:
> On Mon, Jul 22, 2019 at 04:55:09PM +0200, Eric Auger wrote:
> > Do not call dma_max_mapping_size for devices that have no DMA
> > mask set, otherwise we can hit a NULL pointer dereference.
> >
> > This occurs when a virtio-blk-pci device is protected with
> > a virtual IOMMU.
> >
> > Fixes: e6d6dd6c875e ("virtio: Introduce virtio_max_dma_size()")
> > Signed-off-by: Eric Auger
> > Suggested-by: Christoph Hellwig
>
> Christoph, I wonder why did you suggest this?
> The connection between dma_mask and dma_max_mapping_size
> is far from obvious. The documentation doesn't exist.
> Do we really have to teach all users about this hack?
> Why not just make dma_max_mapping_size DTRT?
Because we should not call into dma API functions for devices that
are not DMA capable.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 2/2] virtio/virtio_ring: Fix the dma_max_mapping_size call
On Mon, Jul 22, 2019 at 04:36:09PM +0100, Robin Murphy wrote:
> On 22/07/2019 15:55, Eric Auger wrote:
> > Do not call dma_max_mapping_size for devices that have no DMA
> > mask set, otherwise we can hit a NULL pointer dereference.
> >
> > This occurs when a virtio-blk-pci device is protected with
> > a virtual IOMMU.
> >
> > Fixes: e6d6dd6c875e ("virtio: Introduce virtio_max_dma_size()")
> > Signed-off-by: Eric Auger
> > Suggested-by: Christoph Hellwig
> > ---
> > drivers/virtio/virtio_ring.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > index c8be1c4f5b55..37c143971211 100644
> > --- a/drivers/virtio/virtio_ring.c
> > +++ b/drivers/virtio/virtio_ring.c
> > @@ -262,7 +262,7 @@ size_t virtio_max_dma_size(struct virtio_device *vdev)
> > {
> > size_t max_segment_size = SIZE_MAX;
> > - if (vring_use_dma_api(vdev))
> > + if (vring_use_dma_api(vdev) && vdev->dev.dma_mask)
>
> Hmm, might it make sense to roll that check up into vring_use_dma_api()
> itself? After all, if the device has no mask then it's likely that other DMA
> API ops wouldn't really work as expected either.
>
> Robin.
Nope, Eric pointed out it's just dma_addressing_limited that is broken.
Other APIs call dma_get_mask which handles the NULL mask case fine.
> > max_segment_size = dma_max_mapping_size(>dev);
> > return max_segment_size;
> >
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 2/2] virtio/virtio_ring: Fix the dma_max_mapping_size call
On Mon, Jul 22, 2019 at 05:27:10PM +0200, Christoph Hellwig wrote:
> On Mon, Jul 22, 2019 at 04:55:09PM +0200, Eric Auger wrote:
> > Do not call dma_max_mapping_size for devices that have no DMA
> > mask set, otherwise we can hit a NULL pointer dereference.
> >
> > This occurs when a virtio-blk-pci device is protected with
> > a virtual IOMMU.
> >
> > Fixes: e6d6dd6c875e ("virtio: Introduce virtio_max_dma_size()")
> > Signed-off-by: Eric Auger
> > Suggested-by: Christoph Hellwig
>
> Looks good. virtio maintainers, let me know if you want to queue
> it up or if I should pick the patch up through the dma-mapping tree.
Personally I dislike this API because I feel presence of dma mask does
not strictly have to reflect max size. And generally the requirement to
check presence of mask feels like an undocumented hack to me. Even
reading code will not help you avoid the warning, everyone will get it
wrong and get the warning splat in their logs. So I would prefer just
v1 of the patch that makes dma API do the right thing for us.
However, if that's not going to be the case, let's fix it up in virtio.
In any case, for both v1 and v2 of the patches, you can merge them
through your tree:
Acked-by: Michael S. Tsirkin
--
MST
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 2/2] virtio/virtio_ring: Fix the dma_max_mapping_size call
On 22/07/2019 15:55, Eric Auger wrote:
Do not call dma_max_mapping_size for devices that have no DMA
mask set, otherwise we can hit a NULL pointer dereference.
This occurs when a virtio-blk-pci device is protected with
a virtual IOMMU.
Fixes: e6d6dd6c875e ("virtio: Introduce virtio_max_dma_size()")
Signed-off-by: Eric Auger
Suggested-by: Christoph Hellwig
---
drivers/virtio/virtio_ring.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index c8be1c4f5b55..37c143971211 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -262,7 +262,7 @@ size_t virtio_max_dma_size(struct virtio_device *vdev)
{
size_t max_segment_size = SIZE_MAX;
- if (vring_use_dma_api(vdev))
+ if (vring_use_dma_api(vdev) && vdev->dev.dma_mask)
Hmm, might it make sense to roll that check up into vring_use_dma_api()
itself? After all, if the device has no mask then it's likely that other
DMA API ops wouldn't really work as expected either.
Robin.
max_segment_size = dma_max_mapping_size(>dev);
return max_segment_size;
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 2/2] virtio/virtio_ring: Fix the dma_max_mapping_size call
On Mon, Jul 22, 2019 at 04:55:09PM +0200, Eric Auger wrote:
> Do not call dma_max_mapping_size for devices that have no DMA
> mask set, otherwise we can hit a NULL pointer dereference.
>
> This occurs when a virtio-blk-pci device is protected with
> a virtual IOMMU.
>
> Fixes: e6d6dd6c875e ("virtio: Introduce virtio_max_dma_size()")
> Signed-off-by: Eric Auger
> Suggested-by: Christoph Hellwig
Christoph, I wonder why did you suggest this?
The connection between dma_mask and dma_max_mapping_size
is far from obvious. The documentation doesn't exist.
Do we really have to teach all users about this hack?
Why not just make dma_max_mapping_size DTRT?
> ---
> drivers/virtio/virtio_ring.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index c8be1c4f5b55..37c143971211 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -262,7 +262,7 @@ size_t virtio_max_dma_size(struct virtio_device *vdev)
> {
> size_t max_segment_size = SIZE_MAX;
>
> - if (vring_use_dma_api(vdev))
> + if (vring_use_dma_api(vdev) && vdev->dev.dma_mask)
> max_segment_size = dma_max_mapping_size(>dev);
>
> return max_segment_size;
> --
> 2.20.1
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 2/2] virtio/virtio_ring: Fix the dma_max_mapping_size call
On Mon, Jul 22, 2019 at 04:55:09PM +0200, Eric Auger wrote:
> Do not call dma_max_mapping_size for devices that have no DMA
> mask set, otherwise we can hit a NULL pointer dereference.
>
> This occurs when a virtio-blk-pci device is protected with
> a virtual IOMMU.
>
> Fixes: e6d6dd6c875e ("virtio: Introduce virtio_max_dma_size()")
> Signed-off-by: Eric Auger
> Suggested-by: Christoph Hellwig
Looks good. virtio maintainers, let me know if you want to queue
it up or if I should pick the patch up through the dma-mapping tree.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH 2/2] virtio/virtio_ring: Fix the dma_max_mapping_size call
Do not call dma_max_mapping_size for devices that have no DMA
mask set, otherwise we can hit a NULL pointer dereference.
This occurs when a virtio-blk-pci device is protected with
a virtual IOMMU.
Fixes: e6d6dd6c875e ("virtio: Introduce virtio_max_dma_size()")
Signed-off-by: Eric Auger
Suggested-by: Christoph Hellwig
---
drivers/virtio/virtio_ring.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index c8be1c4f5b55..37c143971211 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -262,7 +262,7 @@ size_t virtio_max_dma_size(struct virtio_device *vdev)
{
size_t max_segment_size = SIZE_MAX;
- if (vring_use_dma_api(vdev))
+ if (vring_use_dma_api(vdev) && vdev->dev.dma_mask)
max_segment_size = dma_max_mapping_size(>dev);
return max_segment_size;
--
2.20.1
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
