Re: [PATCH V1 2/3] amd-iommu: Sync IOVA-to-GPA translation during page invalidation

2020-09-30 Thread Wei Huang
On 09/29 01:34, Alex Williamson wrote:
> On Mon, 28 Sep 2020 15:05:05 -0500
> Wei Huang  wrote:
> 
> > Add support to sync the IOVA-to-GPA translation at the time of IOMMU
> > page invalidation. This function is called when two IOMMU commands,
> > AMDVI_CMD_INVAL_AMDVI_PAGES and AMDVI_CMD_INVAL_AMDVI_ALL, are
> > intercepted. Address space notifiers are called accordingly.
> > 
> > Co-developed-by: Wei Huang 
> > Signed-off-by: Suravee Suthikulpanit 
> > ---
> >  hw/i386/amd_iommu.c | 177 
> >  hw/i386/amd_iommu.h |  10 +++
> >  hw/vfio/common.c|   3 +-
> >  3 files changed, 189 insertions(+), 1 deletion(-)
> ...
> > diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> > index 13471ae29436..243216499ce0 100644
> > --- a/hw/vfio/common.c
> > +++ b/hw/vfio/common.c
> > @@ -346,7 +346,8 @@ static int vfio_dma_map(VFIOContainer *container, 
> > hwaddr iova,
> >   * the VGA ROM space.
> >   */
> >  if (ioctl(container->fd, VFIO_IOMMU_MAP_DMA, &map) == 0 ||
> > -(errno == EBUSY && vfio_dma_unmap(container, iova, size) == 0 &&
> > +((errno == EEXIST || errno == EBUSY) &&
> > + vfio_dma_unmap(container, iova, size) == 0 &&
> >   ioctl(container->fd, VFIO_IOMMU_MAP_DMA, &map) == 0)) {
> >  return 0;
> >  }
> 
> 
> This seems like it should be a separate patch.  AFAICT the commit log
> doesn't even hint at why this change is necessary.  I think the -EBUSY
> error pre-dates vIOMMU as well.  Responding the same for an -EEXIST
> almost suggests a coherency issue between QEMU and the kernel, or a
> direct mapping replacement without an invalidation, which doesn't seem
> to be what this patch is implementing.  Thanks,

I went back to check it. Removing this checking code (original code) didn't
trigger any issues with Intel 10G passthru NIC. I think this was from the
residual debugging code when we started to implement it. Sorry for the
confusion. I will remove this code in V2 with more tests.

-Wei

> 
> Alex
> 



Re: [PATCH V1 2/3] amd-iommu: Sync IOVA-to-GPA translation during page invalidation

2020-09-29 Thread Alex Williamson
On Mon, 28 Sep 2020 15:05:05 -0500
Wei Huang  wrote:

> Add support to sync the IOVA-to-GPA translation at the time of IOMMU
> page invalidation. This function is called when two IOMMU commands,
> AMDVI_CMD_INVAL_AMDVI_PAGES and AMDVI_CMD_INVAL_AMDVI_ALL, are
> intercepted. Address space notifiers are called accordingly.
> 
> Co-developed-by: Wei Huang 
> Signed-off-by: Suravee Suthikulpanit 
> ---
>  hw/i386/amd_iommu.c | 177 
>  hw/i386/amd_iommu.h |  10 +++
>  hw/vfio/common.c|   3 +-
>  3 files changed, 189 insertions(+), 1 deletion(-)
...
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 13471ae29436..243216499ce0 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -346,7 +346,8 @@ static int vfio_dma_map(VFIOContainer *container, hwaddr 
> iova,
>   * the VGA ROM space.
>   */
>  if (ioctl(container->fd, VFIO_IOMMU_MAP_DMA, &map) == 0 ||
> -(errno == EBUSY && vfio_dma_unmap(container, iova, size) == 0 &&
> +((errno == EEXIST || errno == EBUSY) &&
> + vfio_dma_unmap(container, iova, size) == 0 &&
>   ioctl(container->fd, VFIO_IOMMU_MAP_DMA, &map) == 0)) {
>  return 0;
>  }


This seems like it should be a separate patch.  AFAICT the commit log
doesn't even hint at why this change is necessary.  I think the -EBUSY
error pre-dates vIOMMU as well.  Responding the same for an -EEXIST
almost suggests a coherency issue between QEMU and the kernel, or a
direct mapping replacement without an invalidation, which doesn't seem
to be what this patch is implementing.  Thanks,

Alex