[PATCH] Avoid needless flush cache when guest change MTRR

[PATCH] Avoid needless flush cache when guest change MTRR

[Liang Li]

Flushing cache is needed only when guest has IOMMU device, using
need_iommu(d) can minimize the impact to guest with device assigned,
since a guest may be hot plugged with a device thus there may be dirty
cache lines before need_iommu(d) becoming true, force the flush_all
when the first device is assigned to guest to amend this issue.

Signed-off-by: Liang Li <liang.z.li@intel.com>
Signed-off-by: Yang Zhang <yang.z.zhang@intel.com>
---
 xen/arch/x86/hvm/mtrr.c       | 2 +-
 xen/drivers/passthrough/pci.c | 5 +++++
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/hvm/mtrr.c b/xen/arch/x86/hvm/mtrr.c
index ee18553..858fb7e 100644
--- a/xen/arch/x86/hvm/mtrr.c
+++ b/xen/arch/x86/hvm/mtrr.c
@@ -791,7 +791,7 @@ HVM_REGISTER_SAVE_RESTORE(MTRR, hvm_save_mtrr_msr, hvm_load_mtrr_msr,
 
 void memory_type_changed(struct domain *d)
 {
-    if ( iommu_enabled && d->vcpu && d->vcpu[0] )
+    if ( need_iommu(d) && d->vcpu && d->vcpu[0] )
     {
         p2m_memory_type_changed(d);
         flush_all(FLUSH_CACHE);
diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index 78c6977..a023d1d 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -1365,6 +1365,11 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn)
             }
         }
         d->need_iommu = 1;
+        /* There may be dirty cache lines when a device is assigned
+         * and before need_iommu(d) becoming true, this will cause
+         * memory_type_changed lose effect if memory type changes.
+         * Call memory_type_changed here to amend this. */
+        memory_type_changed(d);
     }
 
     pdev = pci_get_pdev_by_domain(hardware_domain, seg, bus, devfn);
-- 
1.9.1

Re: [PATCH] Avoid needless flush cache when guest change MTRR

[Tian, Kevin]

> From: Li, Liang Z
> Sent: Wednesday, February 04, 2015 11:28 AM
> 
> Flushing cache is needed only when guest has IOMMU device, using
> need_iommu(d) can minimize the impact to guest with device assigned,
> since a guest may be hot plugged with a device thus there may be dirty
> cache lines before need_iommu(d) becoming true, force the flush_all
> when the first device is assigned to guest to amend this issue.
> 
> Signed-off-by: Liang Li <liang.z.li@intel.com>
> Signed-off-by: Yang Zhang <yang.z.zhang@intel.com>
> ---
>  xen/arch/x86/hvm/mtrr.c       | 2 +-
>  xen/drivers/passthrough/pci.c | 5 +++++
>  2 files changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/x86/hvm/mtrr.c b/xen/arch/x86/hvm/mtrr.c
> index ee18553..858fb7e 100644
> --- a/xen/arch/x86/hvm/mtrr.c
> +++ b/xen/arch/x86/hvm/mtrr.c
> @@ -791,7 +791,7 @@ HVM_REGISTER_SAVE_RESTORE(MTRR,
> hvm_save_mtrr_msr, hvm_load_mtrr_msr,
> 
>  void memory_type_changed(struct domain *d)
>  {
> -    if ( iommu_enabled && d->vcpu && d->vcpu[0] )
> +    if ( need_iommu(d) && d->vcpu && d->vcpu[0] )
>      {
>          p2m_memory_type_changed(d);
>          flush_all(FLUSH_CACHE);
> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
> index 78c6977..a023d1d 100644
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -1365,6 +1365,11 @@ static int assign_device(struct domain *d, u16 seg,
> u8 bus, u8 devfn)
>              }
>          }
>          d->need_iommu = 1;
> +        /* There may be dirty cache lines when a device is assigned
> +         * and before need_iommu(d) becoming true, this will cause
> +         * memory_type_changed lose effect if memory type changes.
> +         * Call memory_type_changed here to amend this. */
> +        memory_type_changed(d);

could we relax this force flush only on the 1st assigned device which actually
changes need_iommu(d) from false to true?

>      }
> 
>      pdev = pci_get_pdev_by_domain(hardware_domain, seg, bus, devfn);
> --
> 1.9.1

Re: [PATCH] Avoid needless flush cache when guest change MTRR

[Zhang, Yang Z]

Tian, Kevin wrote on 2015-02-04:
>> From: Li, Liang Z
>> Sent: Wednesday, February 04, 2015 11:28 AM
>> 
>> Flushing cache is needed only when guest has IOMMU device, using
>> need_iommu(d) can minimize the impact to guest with device assigned,
>> since a guest may be hot plugged with a device thus there may be
>> dirty cache lines before need_iommu(d) becoming true, force the
>> flush_all when the first device is assigned to guest to amend this issue.
>> 
>> Signed-off-by: Liang Li <liang.z.li@intel.com>
>> Signed-off-by: Yang Zhang <yang.z.zhang@intel.com>
>> ---
>>  xen/arch/x86/hvm/mtrr.c       | 2 +-
>>  xen/drivers/passthrough/pci.c | 5 +++++
>>  2 files changed, 6 insertions(+), 1 deletion(-)
>> diff --git a/xen/arch/x86/hvm/mtrr.c b/xen/arch/x86/hvm/mtrr.c index
>> ee18553..858fb7e 100644 --- a/xen/arch/x86/hvm/mtrr.c +++
>> b/xen/arch/x86/hvm/mtrr.c @@ -791,7 +791,7 @@
>> HVM_REGISTER_SAVE_RESTORE(MTRR, hvm_save_mtrr_msr, hvm_load_mtrr_msr,
>> 
>>  void memory_type_changed(struct domain *d)  {
>> -    if ( iommu_enabled && d->vcpu && d->vcpu[0] )
>> +    if ( need_iommu(d) && d->vcpu && d->vcpu[0] )
>>      {
>>          p2m_memory_type_changed(d);
>>          flush_all(FLUSH_CACHE);
>> diff --git a/xen/drivers/passthrough/pci.c
>> b/xen/drivers/passthrough/pci.c index 78c6977..a023d1d 100644
>> --- a/xen/drivers/passthrough/pci.c
>> +++ b/xen/drivers/passthrough/pci.c
>> @@ -1365,6 +1365,11 @@ static int assign_device(struct domain *d,
>> u16 seg,
>> u8 bus, u8 devfn)
>>              }
>>          }
>>          d->need_iommu = 1;
>> +        /* There may be dirty cache lines when a device is assigned
>> +         * and before need_iommu(d) becoming true, this will cause
>> +         * memory_type_changed lose effect if memory type changes.
>> +         * Call memory_type_changed here to amend this. */
>> +        memory_type_changed(d);
> 
> could we relax this force flush only on the 1st assigned device which
> actually changes need_iommu(d) from false to true?

It is already covered by current code when updating need_iommu, but the patch doesn't show it:

if ( need_iommu(d) <= 0 )
{    
    .....
    d->need_iommu = 1;
    memory_type_changed(d);
}

> 
>>      }
>>      
>>      pdev = pci_get_pdev_by_domain(hardware_domain, seg, bus,
>> devfn);
>> --
>> 1.9.1


Best regards,
Yang

Re: [PATCH] Avoid needless flush cache when guest change MTRR

[Tian, Kevin]

> From: Zhang, Yang Z
> Sent: Wednesday, February 04, 2015 1:54 PM
> 
> Tian, Kevin wrote on 2015-02-04:
> >> From: Li, Liang Z
> >> Sent: Wednesday, February 04, 2015 11:28 AM
> >>
> >> Flushing cache is needed only when guest has IOMMU device, using
> >> need_iommu(d) can minimize the impact to guest with device assigned,
> >> since a guest may be hot plugged with a device thus there may be
> >> dirty cache lines before need_iommu(d) becoming true, force the
> >> flush_all when the first device is assigned to guest to amend this issue.
> >>
> >> Signed-off-by: Liang Li <liang.z.li@intel.com>
> >> Signed-off-by: Yang Zhang <yang.z.zhang@intel.com>
> >> ---
> >>  xen/arch/x86/hvm/mtrr.c       | 2 +-
> >>  xen/drivers/passthrough/pci.c | 5 +++++
> >>  2 files changed, 6 insertions(+), 1 deletion(-)
> >> diff --git a/xen/arch/x86/hvm/mtrr.c b/xen/arch/x86/hvm/mtrr.c index
> >> ee18553..858fb7e 100644 --- a/xen/arch/x86/hvm/mtrr.c +++
> >> b/xen/arch/x86/hvm/mtrr.c @@ -791,7 +791,7 @@
> >> HVM_REGISTER_SAVE_RESTORE(MTRR, hvm_save_mtrr_msr,
> hvm_load_mtrr_msr,
> >>
> >>  void memory_type_changed(struct domain *d)  {
> >> -    if ( iommu_enabled && d->vcpu && d->vcpu[0] )
> >> +    if ( need_iommu(d) && d->vcpu && d->vcpu[0] )
> >>      {
> >>          p2m_memory_type_changed(d);
> >>          flush_all(FLUSH_CACHE);
> >> diff --git a/xen/drivers/passthrough/pci.c
> >> b/xen/drivers/passthrough/pci.c index 78c6977..a023d1d 100644
> >> --- a/xen/drivers/passthrough/pci.c
> >> +++ b/xen/drivers/passthrough/pci.c
> >> @@ -1365,6 +1365,11 @@ static int assign_device(struct domain *d,
> >> u16 seg,
> >> u8 bus, u8 devfn)
> >>              }
> >>          }
> >>          d->need_iommu = 1;
> >> +        /* There may be dirty cache lines when a device is assigned
> >> +         * and before need_iommu(d) becoming true, this will cause
> >> +         * memory_type_changed lose effect if memory type changes.
> >> +         * Call memory_type_changed here to amend this. */
> >> +        memory_type_changed(d);
> >
> > could we relax this force flush only on the 1st assigned device which
> > actually changes need_iommu(d) from false to true?
> 
> It is already covered by current code when updating need_iommu, but the
> patch doesn't show it:
> 
> if ( need_iommu(d) <= 0 )
> {
>     .....
>     d->need_iommu = 1;
>     memory_type_changed(d);
> }
> 

You're right. :-)

Acked-by: Kevin Tian <kevin.tian@intel.com>

Thanks
Kevin

Re: [PATCH] Avoid needless flush cache when guest change MTRR

[Jan Beulich]

>>> On 04.02.15 at 04:28, <liang.z.li@intel.com> wrote:
> --- a/xen/arch/x86/hvm/mtrr.c
> +++ b/xen/arch/x86/hvm/mtrr.c
> @@ -791,7 +791,7 @@ HVM_REGISTER_SAVE_RESTORE(MTRR, hvm_save_mtrr_msr, hvm_load_mtrr_msr,
>  
>  void memory_type_changed(struct domain *d)
>  {
> -    if ( iommu_enabled && d->vcpu && d->vcpu[0] )
> +    if ( need_iommu(d) && d->vcpu && d->vcpu[0] )
>      {
>          p2m_memory_type_changed(d);
>          flush_all(FLUSH_CACHE);

This doesn't do what title and description say: You not only suppress
the flush, but also the EPT table adjustments. Either adjust at least
the description accordingly, or make the code do what the description
says.

> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -1365,6 +1365,11 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn)
>              }
>          }
>          d->need_iommu = 1;
> +        /* There may be dirty cache lines when a device is assigned
> +         * and before need_iommu(d) becoming true, this will cause
> +         * memory_type_changed lose effect if memory type changes.
> +         * Call memory_type_changed here to amend this. */
> +        memory_type_changed(d);

The comment formatting does not adhere to ./CODING_STYLE.

Jan