Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/exec/ram_addr.h | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
index 5e59a3d8d7..dd8713179e 100644
--- a/include/exec/ram_addr.h
+++ b/include/exec/ram_addr.h
@@ -330,7 +330,9 @@ static inline void cpu_physical_memory_set_dirty_range(ram_addr_t start,
         }
     }
 
-    xen_hvm_modified_memory(start, length);
+    if (xen_enabled()) {
+        xen_hvm_modified_memory(start, length);
+    }
 }
 
 #if !defined(_WIN32)
@@ -388,7 +390,9 @@ static inline void cpu_physical_memory_set_dirty_lebitmap(unsigned long *bitmap,
             }
         }
 
-        xen_hvm_modified_memory(start, pages << TARGET_PAGE_BITS);
+        if (xen_enabled()) {
+            xen_hvm_modified_memory(start, pages << TARGET_PAGE_BITS);
+        }
     } else {
         uint8_t clients = tcg_enabled() ? DIRTY_CLIENTS_ALL : DIRTY_CLIENTS_NOCODE;
 
-- 
2.21.3

Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  include/exec/ram_addr.h | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
> index 5e59a3d8d7..dd8713179e 100644
> --- a/include/exec/ram_addr.h
> +++ b/include/exec/ram_addr.h
> @@ -330,7 +330,9 @@ static inline void cpu_physical_memory_set_dirty_range(ram_addr_t start,
>          }
>      }
>  
> -    xen_hvm_modified_memory(start, length);
> +    if (xen_enabled()) {
> +        xen_hvm_modified_memory(start, length);
> +    }
>  }
>  
>  #if !defined(_WIN32)
> @@ -388,7 +390,9 @@ static inline void cpu_physical_memory_set_dirty_lebitmap(unsigned long *bitmap,
>              }
>          }
>  
> -        xen_hvm_modified_memory(start, pages << TARGET_PAGE_BITS);
> +        if (xen_enabled()) {
> +            xen_hvm_modified_memory(start, pages << TARGET_PAGE_BITS);
> +        }
>      } else {
>          uint8_t clients = tcg_enabled() ? DIRTY_CLIENTS_ALL : DIRTY_CLIENTS_NOCODE;

I don't object moving the xen code to accell.  But I think that this
change is bad.

On the following patch:
- You export xen_allowed
  (ok, it was already exported, but I think it shouldn't)

(master)$ find . -type f | xargs grep xen_allowed
./hw/xen/xen-common.c:    ac->allowed = &xen_allowed;
./include/hw/xen/xen.h:extern bool xen_allowed;
./include/hw/xen/xen.h:    return xen_allowed;
./softmmu/vl.c:bool xen_allowed;

This are all the users that I can find.

And xen_havm_modified_memory() is an empty function if xen is not
compiled in.  And in the case that xen is compiled in, the 1st thing
that it checks is:

   if (unlikely(xen_in_migration)) {

That is way more restrictive that xen_enabled().

So, I think that it is better to drop this patch, maintain next one, but
just un-exporting xen_allowed.

What do you think?

Later, Juan.

Hi Juan,

On 5/8/20 10:39 AM, Juan Quintela wrote:
> Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>   include/exec/ram_addr.h | 8 ++++++--
>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
>> index 5e59a3d8d7..dd8713179e 100644
>> --- a/include/exec/ram_addr.h
>> +++ b/include/exec/ram_addr.h
>> @@ -330,7 +330,9 @@ static inline void cpu_physical_memory_set_dirty_range(ram_addr_t start,
>>           }
>>       }
>>   
>> -    xen_hvm_modified_memory(start, length);
>> +    if (xen_enabled()) {
>> +        xen_hvm_modified_memory(start, length);
>> +    }
>>   }
>>   
>>   #if !defined(_WIN32)
>> @@ -388,7 +390,9 @@ static inline void cpu_physical_memory_set_dirty_lebitmap(unsigned long *bitmap,
>>               }
>>           }
>>   
>> -        xen_hvm_modified_memory(start, pages << TARGET_PAGE_BITS);
>> +        if (xen_enabled()) {
>> +            xen_hvm_modified_memory(start, pages << TARGET_PAGE_BITS);
>> +        }
>>       } else {
>>           uint8_t clients = tcg_enabled() ? DIRTY_CLIENTS_ALL : DIRTY_CLIENTS_NOCODE;
> 
> I don't object moving the xen code to accell.  But I think that this
> change is bad.
> 
> On the following patch:
> - You export xen_allowed
>    (ok, it was already exported, but I think it shouldn't)
> 
> (master)$ find . -type f | xargs grep xen_allowed
> ./hw/xen/xen-common.c:    ac->allowed = &xen_allowed;
> ./include/hw/xen/xen.h:extern bool xen_allowed;
> ./include/hw/xen/xen.h:    return xen_allowed;
> ./softmmu/vl.c:bool xen_allowed;
> 
> This are all the users that I can find.
> 
> And xen_havm_modified_memory() is an empty function if xen is not
> compiled in.  And in the case that xen is compiled in, the 1st thing
> that it checks is:
> 
>     if (unlikely(xen_in_migration)) {
> 
> That is way more restrictive that xen_enabled().
> 
> So, I think that it is better to drop this patch, maintain next one, but
> just un-exporting xen_allowed.
> 
> What do you think?

I blindly trust your judgement on this :) I'd rather not touch this code 
but as it happens to be in "exec/ram_addr.h" I had to modify it.

Thanks for your reviews!

> 
> Later, Juan.
> 
>