Re: [PATCH v3 6/8] admin: Add theory of operation for write recording commands

["Michael S. Tsirkin" <mst@redhat.com>]

On Mon, Nov 13, 2023 at 11:41:07AM +0800, Jason Wang wrote:
> On Fri, Nov 10, 2023 at 2:46âPM Parav Pandit <parav@nvidia.com> wrote:
> >
> > Hi Michael,
> >
> > > From: Michael S. Tsirkin <mst@redhat.com>
> > > Sent: Thursday, November 9, 2023 1:29 PM
> >
> > [..]
> > > > Besides the issue of performance, it's also racy, assuming we are logging
> > > IOVA.
> > > >
> > > > 0) device log IOVA
> > > > 1) hypervisor fetches IOVA from log buffer
> > > > 2) guest map IOVA to a new GPA
> > > > 3) hypervisor traverse guest table to get IOVA to new GPA
> > > >
> > > > Then we lost the old GPA.
> > >
> > > Interesting and a good point. And by the way e.g. vhost has the same issue.  You
> > > need to flush dirty tracking info when changing the mappings somehow.  Parav
> > > what's the plan for this? Should be addressed in the spec too.
> > >
> > As you listed the flush is needed for vhost or device-based DPT.
> 
> What does DPT mean? Device Page Table? Let's not invent terminology
> which is not known by others please.
> 
> We have discussed it many times. You can't just depend on ATS or
> reinventing wheels in virtio.
> 
> What's more, please try not to give me the impression that the
> proposal is optimized for a specific vendor (like device IOMMU
> stuffs).

Devices with IOMMU exist.
So if it's for device IOMMU that's fine, as long as it's well isolated.


> > The necessary plumbing is already covered for this in the query (read and clear) command of this v3 proposal.
> 
> The issue is logging via IOVA ... I don't see how "read and clear" can help.
> 
> > It is listed in Device Write Records Read Command.
> 
> Please explain how your proposal can solve the above race.
> 
> >
> > When the page write record is fully read, it is flushed.
> > How/when to use, I think its hypervisor specific, so we probably better off not documenting those details.
> 
> Well, as the author of this proposal, at least you need to know how a
> hypervisor can work with your proposal, no?
> 
> > May be such read is needed in some other path too depending on how hypervisor implemented.
> 
> What do you mean by "May be ... some other path" here? You're
> inventing a mechanism that you don't know how a hypervisor can use?
> 
> Thanks


It seems like a subtle enough race that it really should be documented.

-- 
MST