Re: [Qemu-devel] [PATCH 2/3] exec: simplify address_space_get_iotlb_entry

[Peter Xu <peterx@redhat.com>]

On Thu, Jun 08, 2017 at 09:59:50PM +0300, Michael S. Tsirkin wrote:
> On Thu, Jun 08, 2017 at 02:11:50PM +0800, Peter Xu wrote:
> > On Wed, Jun 07, 2017 at 04:07:20PM +0300, Michael S. Tsirkin wrote:
> > > On Wed, Jun 07, 2017 at 11:44:43AM +0800, Peter Xu wrote:
> > > > On Wed, Jun 07, 2017 at 09:47:05AM +1000, David Gibson wrote:
> > > > > On Tue, Jun 06, 2017 at 04:34:30PM +0200, Paolo Bonzini wrote:
> > > > > > 
> > > > > > 
> > > > > > On 05/06/2017 05:07, Peter Xu wrote:
> > > > > > > I don't sure whether it'll be a good interface for IOTLB. AFAIU at
> > > > > > > least for VT-d, the IOMMU translation is page aligned which is defined
> > > > > > > by spec, so it makes sense that (again at least for VT-d) here we'd
> > > > > > > better just use page_mask/addr_mask.
> > > > > > > 
> > > > > > > That's also how I know about IOMMU in general - I assume it do the
> > > > > > > translations always with page masks (never arbitary length), though
> > > > > > > page size can differ from platfrom to platform, that's why here the
> > > > > > > IOTLB interface used addr_mask, then it works for all platforms. I
> > > > > > > don't know whether I'm 100% correct here though.
> > > > > > > 
> > > > > > > Maybe David/Paolo/... would comment as well?
> > > > > > 
> > > > > > I would ask David.  There are PowerPC MMUs that allow fast lookup of
> > > > > > arbitrarily-sized windows (not necessarily power of two),
> > > > > 
> > > > > Uh.. I'm not sure what you mean here.  You might be thinking of the
> > > > > BATs which really old (32-bit) PowerPC MMUs had - those allow
> > > > > arbitrary large block translations, but they do have to be a power of
> > > > > two.
> > > > > 
> > > > > > so maybe the
> > > > > > IOMMUs can do the same.
> > > > > 
> > > > > The only Power IOMMU I know about uses a fixed, power-of-two page size
> > > > > per DMA window.
> > > > 
> > > > If so, I would still be inclined to keep using masks for QEMU IOTLB.
> > > > Then, my first two patches should still stand.
> > > > 
> > > > I am just afraid that not using masks will diverge the emulation from
> > > > real hardware and brings trouble one day.
> > > > 
> > > > For vhost IOTLB interface, it does not need to be strictly aligned to
> > > > QEMU IOMMU IOTLB definition, and that's how it's working now (current
> > > > vhost iotlb allows arbitary length, and I think it's good). So imho we
> > > > don't really need to worry about the performance - after all, we can
> > > > do everything customized for vhost, just like what patch 3 did (yeah,
> > > > it can be better...).
> > > > 
> > > > Thanks,
> > > 
> > > Pre-faults is also something that does not happen on real hardware.
> > > And it's about security so a bigger issue.
> > > 
> > > If I had to choose between that and using non-power-of-2 in
> > > the API, I'd go for non-power-of-2. Let backends that can only
> > > support power of 2 split it up to multiple transactions.
> > 
> > The problem is that when I was fixing the problem that vhost had with
> > PT (a764040, "exec: abstract address_space_do_translate()"), I did
> > broke the IOTLB translation a bit (it was using page masks). IMHO we
> > need to fix it first for correctness (patch 1/2).
> > 
> > For patch 3, if we can have Jason's patch to allow dynamic
> > iommu_platform switching, that'll be the best, then I can rewrite
> > patch 3 with the switching logic rather than caching anything. But
> > IMHO that can be separated from patch 1/2 if you like.
> > 
> > Or do you have better suggestion on how should we fix it?
> > 
> > Thanks,
> 
> Can we drop masks completely and replace with length? I think we
> should do that instead of trying to fix masks.

Do you mean to modify IOMMUTLBEntry.addr_mask into length?

Again, I am not sure this is good... At least we need to get ack from
David since spapr should be the initial user of it, and possibly also
Alex since vfio should be assuming that (IIUC both in QEMU and kernel)
addr_mask is page masks rather than arbirary length.

(CC Alex)

Thanks,

-- 
Peter Xu