Re: [Qemu-devel] [PATCH v3 04/12] memory: fix address_space_get_iotlb_entry()

[Peter Xu]

On Fri, May 12, 2017 at 03:25:08PM +1000, David Gibson wrote:
> On Thu, May 11, 2017 at 05:36:03PM +0800, Peter Xu wrote:
> > On Thu, May 11, 2017 at 11:56:38AM +1000, David Gibson wrote:
> > > On Wed, May 10, 2017 at 04:01:47PM +0800, Peter Xu wrote:
> > > > This function has an assumption that we will definitely call translate()
> > > > once (or say, the addr will be located inside one IOMMU memory region),
> > > > otherwise an empty IOTLB will be returned. Nevertheless, this is not
> > > > what we want. When there is no IOMMU memory region, we should build up a
> > > > static mapping for the caller, instead of an invalid IOTLB.
> > > > 
> > > > We won't trigger this path before VT-d passthrough mode. When
> > > > passthrough mode for a vhost device is setup, VT-d is possible to
> > > > disable the IOMMU region for that device. Without current patch, we'll
> > > > get a vhost boot failure, and it'll be failed over to virtio userspace
> > > > mode.
> > > 
> > > This doesn't look right to me.  You're assuming the target is
> > > address_space_memory, which might not be the case - and you should be
> > > able to check from the MR you do hit.  Furthermore it doesn't look
> > > like you're accounting for the trivial translation if the section's
> > > offset in the address space is different from its offset in the MR.
> > 
> > Do you mean this line?
> > 
> >         addr = addr - section->offset_within_address_space
> >                + section->offset_within_region;
> 
> Uh.. where is that line?  But.. wait, yes, I think I was mistaken.  I saw:
>       .translated_addr = iova,
>       
> and thought that meant you were assuming an identify mapping from iova
> to translated addr.  But thinking more carefull, IIRC iova and
> translated_addr are both relative to the MR, not the AS, so I think
> that is correct after all.
> 
> > I thought it was calculating the relative address against that memory
> > region. That should only be useful if we want to do further
> > translate(), right? For the path that this patch tries to handle (when
> > there is no translate() call), then this "addr" is useless here?
> > 
> > Regarding to the address space assignment - do you mean, e.g., I
> > should use section->address_space here instead of
> > &system_address_space? If so, I can do the switch.
> 
> Yes, I think you should.
> 
> > But after all, for
> > now address_space_get_iotlb_entry() is only used by vhost codes, and
> > it only check against iotlb.target_as == NULL, so the address space
> > didn't count too much here...
> 
> > Another reason I used &address_space_memory is that in
> > vfio_iommu_map_notify() we have a check against it:
> > 
> >     if (iotlb->target_as != &address_space_memory) {
> >         error_report("Wrong target AS \"%s\", only system memory is 
> > allowed",
> >                      iotlb->target_as->name ? iotlb->target_as->name : 
> > "none");
> >         return;
> >     }
> > 
> > Or say, we have some assumptions (not only in this patch) that assumes
> > this iotlb.target_as should be system_address_space.
> 
> Right, the vhost code can only handle some IOMMU setups - something
> like nested IOMMUs would break it.  But this way if someone sets up a
> machine with an IOMMU configuration that vhost can't handle, you'll
> get an error message, rather than accesses to unexpected locations,
> which could cause really hard to debug corruption.
> 
> In other words we make assumptions, but we should _test_ those
> assumptions.
> 
> I also think it would make sense to use address_space_translate() if we
> can, since it's an existing interface for a very similar operation.

I did give it a shot to generalize these two functions in this series
(I just posted it):

  [PATCH 0/4] exec: address space translation cleanups

Please see whether that'll be a good replacement of this single patch.

Thanks,

-- 
Peter Xu