| Message ID | 20200508080738.2646-2-philmd@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | accel: Move Xen accelerator code under accel/xen/ | expand |
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. > >
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;
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- include/exec/ram_addr.h | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)