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

[Jason Wang <jasowang@redhat.com>]

On Tue, Nov 14, 2023 at 10:04âAM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
>
>
>
> On 11/13/2023 10:30 PM, Michael S. Tsirkin wrote:
> > 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.

It only exists for very few vendors. Whether or not virtio needs a
device IOMMU is a separate topic.

Again, it's far easier to leverage what platform can give us than
re-inventing stuff in virtio.

> > So if it's for device IOMMU that's fine, as long as it's well isolated.
> device side IOMMU is not a must and I hope virito features don't depend
> on it.

Exactly, and I didn't see any description in this series to:

1) claim the feature depends on device IOMMU
2) invent interfaces for device IOMMU

Thanks



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