Re: [Xen-devel] about the funtion call memory_type_changed()

["Tian, Kevin" <kevin.tian@xxxxxxxxx>]

> From: Li, Liang Z
> Sent: Friday, January 23, 2015 1:55 PM
> 
> > >>> On 22.01.15 at 08:44, <yang.z.zhang@xxxxxxxxx> wrote:
> > > Tian, Kevin wrote on 2015-01-22:
> > >>> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> > >>> Sent: Wednesday, January 21, 2015 6:31 PM
> > >>>
> > >>>>
> > >>>> Yes, it's true. But I still don't understand why to do the
> > >>>> flush_all just when iommu_enable is true. Could you  explain why ?
> > >>>
> > >>> The question you raise doesn't reflect what the function does: It
> > >>> doesn't flush when iommu_enabled, in calls
> > >>> p2m_memory_type_changed() in that case. And that operation is what
> > >>> requires a flush afterwards (unless we know that nothing can be
> > >>> cached yet), as there may have been a cachable -> non- cachable
> > >>> transition for some of the pages assigned to the guest.
> > >
> > > I find in vmx_wbinvd_intercept(), it will check whether iommu_snoop is
> > > enabled before flushing. And this check exists in
> > > memory_type_changes() before, but it is removed by you. I think either
> > > both are removed or both are there. Is there any difference between the
> > two code pieces?
> >
> > I had raised the question on the various uses of iommu_snoop quite some
> > time ago - afair without ever getting a satisfying answer. Some (if not all)
> > instances look suspicious, but without knowing all the details I can't 
> > really
> > propose a patch removing some/all of them.
> >
> > >>> The fact that the call is made dependent on iommu_enabled is simply
> > >>> because when that flag is not set, all memory of the guest is
> > >>> treated WB (as no physical device can be assigned to the guest in
> > >>> that case), and hence to type changes can occur.
> > >
> > > Even the flush is required, the flush_all is too heavy. Just use the
> > > vcpu_dirty_cpumask is enough.
> >
> > No, that mask isn't sufficient - when a CPU gets cleared from it, its cache
> > doesn't get flushed (only the TLB would).
> >
> 
> One thing I want to confirm:
> There is no need to call memory_type_changes() during the restore process, is
> that right?
> 

I think it's required if restoring memory happens before loading mtrr. the
key is some dirty cache lines which may not be flushed to memory due to
cache attribute change. dirty cache lines can be caused by guest itself
(at runtime), or by toostack (when restoring the guest).

How about my proposal to check upon need_iommu(d) plus a possible
force flush in assign_device if an earlier flush is not conducted before
need_iommu(d) becomes true?

Thanks
Kevin

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel