Virtualization - Jul 2021 - [RFC PATCH V2 0/7] Do not read from descripto ring

? 2021/5/14 ??7:13, Michael S. Tsirkin ??:
> On Thu, May 06, 2021 at 01:38:29PM +0100, Christoph Hellwig wrote:
>> On Thu, May 06, 2021 at 04:12:17AM -0400, Michael S. Tsirkin wrote:
>>> Let's try for just a bit, won't make this window anyway:
>>>
>>> I have an old idea. Add a way to find out that unmap is a nop
>>> (or more exactly does not use the address/length).
>>> Then in that case even with DMA API we do not need
>>> the extra data. Hmm?
>> So we actually do have a check for that from the early days of the DMA
>> API, but it only works at compile time: CONFIG_NEED_DMA_MAP_STATE.
>>
>> But given how rare configs without an iommu or swiotlb are these days
>> it has stopped to be very useful.  Unfortunately a runtime-version is
>> not entirely trivial, but maybe if we allow for false positives we
>> could do something like this
>>
>> bool dma_direct_need_state(struct device *dev)
>> {
>> 	/* some areas could not be covered by any map at all */
>> 	if (dev->dma_range_map)
>> 		return false;
>> 	if (force_dma_unencrypted(dev))
>> 		return false;
>> 	if (dma_direct_need_sync(dev))
>> 		return false;
>> 	return *dev->dma_mask == DMA_BIT_MASK(64);
>> }
>>
>> bool dma_need_state(struct device *dev)
>> {
>> 	const struct dma_map_ops *ops = get_dma_ops(dev);
>>
>> 	if (dma_map_direct(dev, ops))
>> 		return dma_direct_need_state(dev);
>> 	return ops->unmap_page ||
>> 		ops->sync_single_for_cpu || ops->sync_single_for_device;
>> }
> Yea that sounds like a good idea. We will need to document that.
>
>
> Something like:
>
> /*
>   * dma_need_state - report whether unmap calls use the address and length
>   * @dev: device to guery
>   *
>   * This is a runtime version of CONFIG_NEED_DMA_MAP_STATE.
>   *
>   * Return the value indicating whether dma_unmap_* and dma_sync_* calls
for the device
>   * use the DMA state parameters passed to them.
>   * The DMA state parameters are: scatter/gather list/table, address and
>   * length.
>   *
>   * If dma_need_state returns false then DMA state parameters are
>   * ignored by all dma_unmap_* and dma_sync_* calls, so it is safe to pass
0 for
>   * address and length, and DMA_UNMAP_SG_TABLE_INVALID and
>   * DMA_UNMAP_SG_LIST_INVALID for s/g table and length respectively.
>   * If dma_need_state returns true then DMA state might
>   * be used and so the actual values are required.
>   */
>
> And we will need DMA_UNMAP_SG_TABLE_INVALID and
> DMA_UNMAP_SG_LIST_INVALID as pointers to an empty global table and list
> for calls such as dma_unmap_sgtable that dereference pointers before
checking
> they are used.
>
>
> Does this look good?
>
> The table/length variants are for consistency, virtio specifically does
> not use s/g at the moment, but it seems nicer than leaving
> users wonder what to do about these.
>
> Thoughts? Jason want to try implementing?

I can add it in my todo list other if other people are interested in 
this, please let us know.

But this is just about saving the efforts of unmap and it doesn't 
eliminate the necessary of using private memory (addr, length) for the 
metadata for validating the device inputs.

And just to clarify, the slight regression we see is testing without 
VIRTIO_F_ACCESS_PLATFORM which means DMA API is not used.

So I will go to post a formal version of this series and we can start 
from there.

Thanks

>

On Fri, Jun 04, 2021 at 01:38:01PM +0800, Jason Wang
wrote:
> 
> ? 2021/5/14 ??7:13, Michael S. Tsirkin ??:
> > On Thu, May 06, 2021 at 01:38:29PM +0100, Christoph Hellwig wrote:
> > > On Thu, May 06, 2021 at 04:12:17AM -0400, Michael S. Tsirkin
wrote:
> > > > Let's try for just a bit, won't make this window
anyway:
> > > > 
> > > > I have an old idea. Add a way to find out that unmap is a
nop
> > > > (or more exactly does not use the address/length).
> > > > Then in that case even with DMA API we do not need
> > > > the extra data. Hmm?
> > > So we actually do have a check for that from the early days of
the DMA
> > > API, but it only works at compile time:
CONFIG_NEED_DMA_MAP_STATE.
> > > 
> > > But given how rare configs without an iommu or swiotlb are these
days
> > > it has stopped to be very useful.  Unfortunately a
runtime-version is
> > > not entirely trivial, but maybe if we allow for false positives
we
> > > could do something like this
> > > 
> > > bool dma_direct_need_state(struct device *dev)
> > > {
> > > 	/* some areas could not be covered by any map at all */
> > > 	if (dev->dma_range_map)
> > > 		return false;
> > > 	if (force_dma_unencrypted(dev))
> > > 		return false;
> > > 	if (dma_direct_need_sync(dev))
> > > 		return false;
> > > 	return *dev->dma_mask == DMA_BIT_MASK(64);
> > > }
> > > 
> > > bool dma_need_state(struct device *dev)
> > > {
> > > 	const struct dma_map_ops *ops = get_dma_ops(dev);
> > > 
> > > 	if (dma_map_direct(dev, ops))
> > > 		return dma_direct_need_state(dev);
> > > 	return ops->unmap_page ||
> > > 		ops->sync_single_for_cpu || ops->sync_single_for_device;
> > > }
> > Yea that sounds like a good idea. We will need to document that.
> > 
> > 
> > Something like:
> > 
> > /*
> >   * dma_need_state - report whether unmap calls use the address and
length
> >   * @dev: device to guery
> >   *
> >   * This is a runtime version of CONFIG_NEED_DMA_MAP_STATE.
> >   *
> >   * Return the value indicating whether dma_unmap_* and dma_sync_*
calls for the device
> >   * use the DMA state parameters passed to them.
> >   * The DMA state parameters are: scatter/gather list/table, address
and
> >   * length.
> >   *
> >   * If dma_need_state returns false then DMA state parameters are
> >   * ignored by all dma_unmap_* and dma_sync_* calls, so it is safe to
pass 0 for
> >   * address and length, and DMA_UNMAP_SG_TABLE_INVALID and
> >   * DMA_UNMAP_SG_LIST_INVALID for s/g table and length respectively.
> >   * If dma_need_state returns true then DMA state might
> >   * be used and so the actual values are required.
> >   */
> > 
> > And we will need DMA_UNMAP_SG_TABLE_INVALID and
> > DMA_UNMAP_SG_LIST_INVALID as pointers to an empty global table and
list
> > for calls such as dma_unmap_sgtable that dereference pointers before
checking
> > they are used.
> > 
> > 
> > Does this look good?
> > 
> > The table/length variants are for consistency, virtio specifically
does
> > not use s/g at the moment, but it seems nicer than leaving
> > users wonder what to do about these.
> > 
> > Thoughts? Jason want to try implementing?
> 
> 
> I can add it in my todo list other if other people are interested in this,
> please let us know.
> 
> But this is just about saving the efforts of unmap and it doesn't
eliminate
> the necessary of using private memory (addr, length) for the metadata for
> validating the device inputs.

Besides unmap, why do we need to validate address? length can be
typically validated by specific drivers - not all of them even use it ..
> And just to clarify, the slight regression we see is testing without
> VIRTIO_F_ACCESS_PLATFORM which means DMA API is not used.
I guess this is due to extra cache pressure? Maybe create yet another
array just for DMA state ...
> So I will go to post a formal version of this series and we can start from
> there.
> 
> Thanks
> 
> 
> >