about the funtion call memory_type_changed()

about the funtion call memory_type_changed()

[Li, Liang Z]

Hi Jan,

I found the restore process of the live migration is quit long, so I try to find out what's going on.
By debugging, I found the most time consuming process is restore the VM's MTRR MSR,
The process is done in the function hvm_load_mtrr_msr(), it will call the
memory_type_changed(), which eventually call the time consuming function
flush_all().

All this is caused by adding the memory_type_changed in your patch, here is the link
http://lists.xen.org/archives/html/xen-devel/2014-03/msg03792.html,

I am not sure if it's necessary to call flush_all, even it's necessary,  call the function
 hvm_load_mtrr_msr one time will cause dozens call of flush_all, and each call of the
 flush_all function will consume about 8 milliseconds, in my test environment, the VM
 has 4 VCPUs, the hvm_load_mtrr_msr() will be called four times, and totally consumes
 about 500 milliseconds. Obviously, there are too many flush_all calls.

 I think something should be done to solve this issue, do you think so?

Liang

Re: about the funtion call memory_type_changed()

[Andrew Cooper]

On 16/01/15 10:29, Li, Liang Z wrote:
> Hi Jan,
>
> I found the restore process of the live migration is quit long, so I try to find out what's going on.
> By debugging, I found the most time consuming process is restore the VM's MTRR MSR,
> The process is done in the function hvm_load_mtrr_msr(), it will call the
> memory_type_changed(), which eventually call the time consuming function
> flush_all().
>
> All this is caused by adding the memory_type_changed in your patch, here is the link
> http://lists.xen.org/archives/html/xen-devel/2014-03/msg03792.html,
>
> I am not sure if it's necessary to call flush_all, even it's necessary,  call the function
>  hvm_load_mtrr_msr one time will cause dozens call of flush_all, and each call of the
>  flush_all function will consume about 8 milliseconds, in my test environment, the VM
>  has 4 VCPUs, the hvm_load_mtrr_msr() will be called four times, and totally consumes
>  about 500 milliseconds. Obviously, there are too many flush_all calls.
>
>  I think something should be done to solve this issue, do you think so?

The flush_all() cant be avoided completely, as it is permitted to use
sethvmcontext on an already-running VM.  In this case, the flush
certainly does need to happen if altering the MTRRs has had a real
effect on dirty cache lines.

However, having a batching mechanism across hvm_load_mtrr_msr() with a
single flush at the end seems like a wise move.

~Andrew

Re: about the funtion call memory_type_changed()

[Jan Beulich]

>>> On 16.01.15 at 11:46, <andrew.cooper3@citrix.com> wrote:
> On 16/01/15 10:29, Li, Liang Z wrote:
>> I found the restore process of the live migration is quit long, so I try to 
> find out what's going on.
>> By debugging, I found the most time consuming process is restore the VM's 
> MTRR MSR,
>> The process is done in the function hvm_load_mtrr_msr(), it will call the
>> memory_type_changed(), which eventually call the time consuming function
>> flush_all().
>>
>> All this is caused by adding the memory_type_changed in your patch, here is 
> the link
>> http://lists.xen.org/archives/html/xen-devel/2014-03/msg03792.html,
>>
>> I am not sure if it's necessary to call flush_all, even it's necessary,  
> call the function
>>  hvm_load_mtrr_msr one time will cause dozens call of flush_all, and each 
> call of the
>>  flush_all function will consume about 8 milliseconds, in my test 
> environment, the VM
>>  has 4 VCPUs, the hvm_load_mtrr_msr() will be called four times, and totally 
> consumes
>>  about 500 milliseconds. Obviously, there are too many flush_all calls.
>>
>>  I think something should be done to solve this issue, do you think so?
> 
> The flush_all() cant be avoided completely, as it is permitted to use
> sethvmcontext on an already-running VM.  In this case, the flush
> certainly does need to happen if altering the MTRRs has had a real
> effect on dirty cache lines.

Plus the actual functions calling memory_type_changed() in mtrr.c
can also be called while the VM is already running.

> However, having a batching mechanism across hvm_load_mtrr_msr() with a
> single flush at the end seems like a wise move.

And that shouldn't be very difficult to achieve. Furthermore perhaps
it would be possible to check whether the VM did run at all already,
and if it didn't we could avoid the flush altogether in the context load
case?

Jan

Re: about the funtion call memory_type_changed()

[Andrew Cooper]

On 16/01/15 11:54, Jan Beulich wrote:
>>>> On 16.01.15 at 11:46, <andrew.cooper3@citrix.com> wrote:
>> On 16/01/15 10:29, Li, Liang Z wrote:
>>> I found the restore process of the live migration is quit long, so I try to 
>> find out what's going on.
>>> By debugging, I found the most time consuming process is restore the VM's 
>> MTRR MSR,
>>> The process is done in the function hvm_load_mtrr_msr(), it will call the
>>> memory_type_changed(), which eventually call the time consuming function
>>> flush_all().
>>>
>>> All this is caused by adding the memory_type_changed in your patch, here is 
>> the link
>>> http://lists.xen.org/archives/html/xen-devel/2014-03/msg03792.html,
>>>
>>> I am not sure if it's necessary to call flush_all, even it's necessary,  
>> call the function
>>>  hvm_load_mtrr_msr one time will cause dozens call of flush_all, and each 
>> call of the
>>>  flush_all function will consume about 8 milliseconds, in my test 
>> environment, the VM
>>>  has 4 VCPUs, the hvm_load_mtrr_msr() will be called four times, and totally 
>> consumes
>>>  about 500 milliseconds. Obviously, there are too many flush_all calls.
>>>
>>>  I think something should be done to solve this issue, do you think so?
>> The flush_all() cant be avoided completely, as it is permitted to use
>> sethvmcontext on an already-running VM.  In this case, the flush
>> certainly does need to happen if altering the MTRRs has had a real
>> effect on dirty cache lines.
> Plus the actual functions calling memory_type_changed() in mtrr.c
> can also be called while the VM is already running.
>
>> However, having a batching mechanism across hvm_load_mtrr_msr() with a
>> single flush at the end seems like a wise move.
> And that shouldn't be very difficult to achieve. Furthermore perhaps
> it would be possible to check whether the VM did run at all already,
> and if it didn't we could avoid the flush altogether in the context load
> case?

I am not sure whether we currently have that information available. 
Guests are currently created with a single ref in the systemcontroller
pause count, and require an unpause hypercall to get going.

That said, the overwhelmingly common case is that the only hvmsetcontext
hypercall made on a domain will be during construction, so there are
probably many improvements to be had by knowing there is no dirty state
to flush.

~Andrew

Re: about the funtion call memory_type_changed()

[Li, Liang Z]

> >> I found the restore process of the live migration is quit long, so I
> >> try to
> > find out what's going on.
> >> By debugging, I found the most time consuming process is restore the
> >> VM's
> > MTRR MSR,
> >> The process is done in the function hvm_load_mtrr_msr(), it will call
> >> the memory_type_changed(), which eventually call the time consuming
> >> function flush_all().
> >>
> >> All this is caused by adding the memory_type_changed in your patch,
> >> here is
> > the link
> >> http://lists.xen.org/archives/html/xen-devel/2014-03/msg03792.html,
> >>
> >> I am not sure if it's necessary to call flush_all, even it's
> >> necessary,
> > call the function
> >>  hvm_load_mtrr_msr one time will cause dozens call of flush_all, and
> >> each
> > call of the
> >>  flush_all function will consume about 8 milliseconds, in my test
> > environment, the VM
> >>  has 4 VCPUs, the hvm_load_mtrr_msr() will be called four times, and
> >> totally
> > consumes
> >>  about 500 milliseconds. Obviously, there are too many flush_all calls.
> >>
> >>  I think something should be done to solve this issue, do you think so?
> >
> > The flush_all() cant be avoided completely, as it is permitted to use
> > sethvmcontext on an already-running VM.  In this case, the flush
> > certainly does need to happen if altering the MTRRs has had a real
> > effect on dirty cache lines.

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 ?

> Plus the actual functions calling memory_type_changed() in mtrr.c can also
> be called while the VM is already running.
> 
> > However, having a batching mechanism across hvm_load_mtrr_msr() with
> a
> > single flush at the end seems like a wise move.
> 
> And that shouldn't be very difficult to achieve. Furthermore perhaps it would
> be possible to check whether the VM did run at all already, and if it didn't we
> could avoid the flush altogether in the context load case?
> 

I have write a patch according to your suggestions. But there is still a lot of flush_all 
when the guest booting, and this prolong the guest booting time about 600ms.

Re: about the funtion call memory_type_changed()

[Jan Beulich]

>>> On 21.01.15 at 11:15, <liang.z.li@intel.com> wrote:
>> >> I found the restore process of the live migration is quit long, so I
>> >> try to
>> > find out what's going on.
>> >> By debugging, I found the most time consuming process is restore the
>> >> VM's
>> > MTRR MSR,
>> >> The process is done in the function hvm_load_mtrr_msr(), it will call
>> >> the memory_type_changed(), which eventually call the time consuming
>> >> function flush_all().
>> >>
>> >> All this is caused by adding the memory_type_changed in your patch,
>> >> here is
>> > the link
>> >> http://lists.xen.org/archives/html/xen-devel/2014-03/msg03792.html,
>> >>
>> >> I am not sure if it's necessary to call flush_all, even it's
>> >> necessary,
>> > call the function
>> >>  hvm_load_mtrr_msr one time will cause dozens call of flush_all, and
>> >> each
>> > call of the
>> >>  flush_all function will consume about 8 milliseconds, in my test
>> > environment, the VM
>> >>  has 4 VCPUs, the hvm_load_mtrr_msr() will be called four times, and
>> >> totally
>> > consumes
>> >>  about 500 milliseconds. Obviously, there are too many flush_all calls.
>> >>
>> >>  I think something should be done to solve this issue, do you think so?
>> >
>> > The flush_all() cant be avoided completely, as it is permitted to use
>> > sethvmcontext on an already-running VM.  In this case, the flush
>> > certainly does need to happen if altering the MTRRs has had a real
>> > effect on dirty cache lines.
> 
> 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.

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.
Possibly one could instead use need_iommu(d), but I didn't
properly think through eventual consequences of doing so.

>> Plus the actual functions calling memory_type_changed() in mtrr.c can also
>> be called while the VM is already running.
>> 
>> > However, having a batching mechanism across hvm_load_mtrr_msr() with
>> a
>> > single flush at the end seems like a wise move.
>> 
>> And that shouldn't be very difficult to achieve. Furthermore perhaps it 
> would
>> be possible to check whether the VM did run at all already, and if it didn't 
> we
>> could avoid the flush altogether in the context load case?
>> 
> 
> I have write a patch according to your suggestions. But there is still a lot 
> of flush_all 
> when the guest booting, and this prolong the guest booting time about 600ms.

Without you telling us where those remaining ones come from, I don't
think we can easily help you.

Jan

Re: about the funtion call memory_type_changed()

[Li, Liang Z]

> >> >>  flush_all function will consume about 8 milliseconds, in my test
> >> > environment, the VM
> >> >>  has 4 VCPUs, the hvm_load_mtrr_msr() will be called four times,
> >> >> and totally
> >> > consumes
> >> >>  about 500 milliseconds. Obviously, there are too many flush_all calls.
> >> >>
> >> >>  I think something should be done to solve this issue, do you think so?
> >> >
> >> > The flush_all() cant be avoided completely, as it is permitted to
> >> > use sethvmcontext on an already-running VM.  In this case, the
> >> > flush certainly does need to happen if altering the MTRRs has had a
> >> > real effect on dirty cache lines.
> >
> > 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.
> 
> 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.
> Possibly one could instead use need_iommu(d), but I didn't properly think
> through eventual consequences of doing so.

Jan, thanks for your explanation.

> >> Plus the actual functions calling memory_type_changed() in mtrr.c can
> >> also be called while the VM is already running.
> >>
> >> > However, having a batching mechanism across hvm_load_mtrr_msr()
> >> > with
> >> a
> >> > single flush at the end seems like a wise move.
> >>
> >> And that shouldn't be very difficult to achieve. Furthermore perhaps
> >> it
> > would
> >> be possible to check whether the VM did run at all already, and if it
> >> didn't
> > we
> >> could avoid the flush altogether in the context load case?
> >>
> >
> > I have write a patch according to your suggestions. But there is still
> > a lot of flush_all when the guest booting, and this prolong the guest
> > booting time about 600ms
> 
> Without you telling us where those remaining ones come from, I don't think
> we can easily help you.

When the guest booting, it will initialize the MTRR, and the MSR write will intercepted by Xen.
Because there are dozens of MSR MTRR operation in the guest, so the 'mtrr_fix_range_msr_set',
'mtrr_def_type_msr_set'  and 'mtrr_var_range_msr_set ' will be called many times. And all of 
them will eventually call the flush_all, it's similar to ' hvm_load_mtrr_msr '.  It seems not easy to
use a batching mechanism in this case.

Each Flush_all will consume  5~8 ms,  it is a big problem if a  malicious guest frequently change
 the MTRR.

Re: about the funtion call memory_type_changed()

[Jan Beulich]

>>> On 21.01.15 at 12:14, <liang.z.li@intel.com> wrote:
>> > I have write a patch according to your suggestions. But there is still
>> > a lot of flush_all when the guest booting, and this prolong the guest
>> > booting time about 600ms
>> 
>> Without you telling us where those remaining ones come from, I don't think
>> we can easily help you.
> 
> When the guest booting, it will initialize the MTRR, and the MSR write will 
> intercepted by Xen.
> Because there are dozens of MSR MTRR operation in the guest, so the 
> 'mtrr_fix_range_msr_set',
> 'mtrr_def_type_msr_set'  and 'mtrr_var_range_msr_set ' will be called many 
> times. And all of 
> them will eventually call the flush_all, it's similar to ' hvm_load_mtrr_msr 
> '.  It seems not easy to
> use a batching mechanism in this case.

Indeed - I don't think we can avoid the flushes in that case, except
maybe detect cases where the values actually don't change. One
question though: Iirc Linux updates the MTRRs only when it finds
some problems with them - is it Linux that you're seeing this with (in
which case investigating _why_ the updates are happening might be
worthwhile).

> Each Flush_all will consume  5~8 ms,  it is a big problem if a  malicious 
> guest frequently change
>  the MTRR.

A guest doing so can only harm itself, i.e. the word "malicious" is a
little odd to be used here.

Jan

Re: about the funtion call memory_type_changed()

[Li, Liang Z]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Wednesday, January 21, 2015 7:22 PM
> To: Li, Liang Z
> Cc: Andrew Cooper; Dong, Eddie; Tian, Kevin; Zhang, Yang Z; xen-
> devel@lists.xen.org; keir@xen.org; Tim Deegan
> Subject: RE: [Xen-devel] about the funtion call memory_type_changed()
> 
> >>> On 21.01.15 at 12:14, <liang.z.li@intel.com> wrote:
> >> > I have write a patch according to your suggestions. But there is
> >> > still a lot of flush_all when the guest booting, and this prolong
> >> > the guest booting time about 600ms
> >>
> >> Without you telling us where those remaining ones come from, I don't
> >> think we can easily help you.
> >
> > When the guest booting, it will initialize the MTRR, and the MSR write
> > will intercepted by Xen.
> > Because there are dozens of MSR MTRR operation in the guest, so the
> > 'mtrr_fix_range_msr_set', 'mtrr_def_type_msr_set'  and
> > 'mtrr_var_range_msr_set ' will be called many times. And all of them
> > will eventually call the flush_all, it's similar to '
> > hvm_load_mtrr_msr '.  It seems not easy to use a batching mechanism in
> > this case.
> 
> Indeed - I don't think we can avoid the flushes in that case, except maybe
> detect cases where the values actually don't change. One question though:
> Iirc Linux updates the MTRRs only when it finds some problems with them - is
> it Linux that you're seeing this with (in which case investigating _why_ the
> updates are happening might be worthwhile).
> 
> > Each Flush_all will consume  5~8 ms,  it is a big problem if a
> > malicious guest frequently change  the MTRR.
> 
> A guest doing so can only harm itself, i.e. the word "malicious" is a little odd
> to be used here.
> 

But the flush_all function will exec wbinvd instruction to invalidate the cache, this will 
heavily  impact  the system's performance, is that right?

Re: about the funtion call memory_type_changed()

[Tian, Kevin]

> From: Li, Liang Z
> Sent: Thursday, January 22, 2015 12:29 PM
> 
> > -----Original Message-----
> > From: Jan Beulich [mailto:JBeulich@suse.com]
> > Sent: Wednesday, January 21, 2015 7:22 PM
> > To: Li, Liang Z
> > Cc: Andrew Cooper; Dong, Eddie; Tian, Kevin; Zhang, Yang Z; xen-
> > devel@lists.xen.org; keir@xen.org; Tim Deegan
> > Subject: RE: [Xen-devel] about the funtion call memory_type_changed()
> >
> > >>> On 21.01.15 at 12:14, <liang.z.li@intel.com> wrote:
> > >> > I have write a patch according to your suggestions. But there is
> > >> > still a lot of flush_all when the guest booting, and this prolong
> > >> > the guest booting time about 600ms
> > >>
> > >> Without you telling us where those remaining ones come from, I don't
> > >> think we can easily help you.
> > >
> > > When the guest booting, it will initialize the MTRR, and the MSR write
> > > will intercepted by Xen.
> > > Because there are dozens of MSR MTRR operation in the guest, so the
> > > 'mtrr_fix_range_msr_set', 'mtrr_def_type_msr_set'  and
> > > 'mtrr_var_range_msr_set ' will be called many times. And all of them
> > > will eventually call the flush_all, it's similar to '
> > > hvm_load_mtrr_msr '.  It seems not easy to use a batching mechanism in
> > > this case.
> >
> > Indeed - I don't think we can avoid the flushes in that case, except maybe
> > detect cases where the values actually don't change. One question though:
> > Iirc Linux updates the MTRRs only when it finds some problems with them -
> is
> > it Linux that you're seeing this with (in which case investigating _why_ the
> > updates are happening might be worthwhile).
> >
> > > Each Flush_all will consume  5~8 ms,  it is a big problem if a
> > > malicious guest frequently change  the MTRR.
> >
> > A guest doing so can only harm itself, i.e. the word "malicious" is a little odd
> > to be used here.
> >
> 
> But the flush_all function will exec wbinvd instruction to invalidate the cache,
> this will
> heavily  impact  the system's performance, is that right?
> 

guest can invalidate cache anyway. You can't prevent it (though mitigated by
cache qos feature).

Thanks
Kevin

Re: about the funtion call memory_type_changed()

[Tian, Kevin]

> From: Jan Beulich [mailto:JBeulich@suse.com]
> 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.
> 
> 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.
> Possibly one could instead use need_iommu(d), but I didn't
> properly think through eventual consequences of doing so.
> 

using need_iommu(d) can minimize the impact to only guests
with device assigned. of course just check need_iommu(d) is
not enough, since a guest may be hotplugged with a device
thus there may be dirty cache lines before need_iommu(d)
becoming true. this can be amended by forcing a flush_all
in assign_device if noting one or more memory type changes
happened before.

Thanks
Kevin

Re: about the funtion call memory_type_changed()

[Zhang, Yang Z]

Tian, Kevin wrote on 2015-01-22:
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> 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?

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

Best regards,
Yang

Re: about the funtion call memory_type_changed()

[Jan Beulich]

>>> On 22.01.15 at 08:44, <yang.z.zhang@intel.com> wrote:
> Tian, Kevin wrote on 2015-01-22:
>>> From: Jan Beulich [mailto:JBeulich@suse.com]
>>> 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).

Jan

Re: about the funtion call memory_type_changed()

[Li, Liang Z]

> >>> On 22.01.15 at 08:44, <yang.z.zhang@intel.com> wrote:
> > Tian, Kevin wrote on 2015-01-22:
> >>> From: Jan Beulich [mailto:JBeulich@suse.com]
> >>> 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?

Liang

Re: about the funtion call memory_type_changed()

[Tian, Kevin]

> From: Li, Liang Z
> Sent: Friday, January 23, 2015 1:55 PM
> 
> > >>> On 22.01.15 at 08:44, <yang.z.zhang@intel.com> wrote:
> > > Tian, Kevin wrote on 2015-01-22:
> > >>> From: Jan Beulich [mailto:JBeulich@suse.com]
> > >>> 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