[Qemu-devel] [PATCH] xen: fix xen-mapcache build on non-Xen capable targets

[Qemu-devel] [PATCH] xen: fix xen-mapcache build on non-Xen capable targets

[Avi Kivity]

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 xen-mapcache.h |   32 ++++++++++++++++++++++++++++++++
 1 files changed, 32 insertions(+), 0 deletions(-)

diff --git a/xen-mapcache.h b/xen-mapcache.h
index 606b8af..da874ca 100644
--- a/xen-mapcache.h
+++ b/xen-mapcache.h
@@ -9,6 +9,10 @@
 #ifndef XEN_MAPCACHE_H
 #define XEN_MAPCACHE_H
 
+#include <stdlib.h>
+
+#ifdef CONFIG_XEN
+
 void xen_map_cache_init(void);
 uint8_t *xen_map_cache(target_phys_addr_t phys_addr, target_phys_addr_t size,
                        uint8_t lock);
@@ -16,4 +20,32 @@ ram_addr_t xen_ram_addr_from_mapcache(void *ptr);
 void xen_invalidate_map_cache_entry(uint8_t *buffer);
 void xen_invalidate_map_cache(void);
 
+#else
+
+static inline void xen_map_cache_init(void)
+{
+}
+
+static inline uint8_t *xen_map_cache(target_phys_addr_t phys_addr,
+                                     target_phys_addr_t size,
+                                     uint8_t lock)
+{
+    abort();
+}
+
+static inline ram_addr_t xen_ram_addr_from_mapcache(void *ptr)
+{
+    abort();
+}
+
+static inline void xen_invalidate_map_cache_entry(uint8_t *buffer)
+{
+}
+
+static inline void xen_invalidate_map_cache(void)
+{
+}
+
+#endif
+
 #endif /* !XEN_MAPCACHE_H */
-- 
1.7.5.3

Re: [Qemu-devel] [PATCH] xen: fix xen-mapcache build on non-Xen capable targets

[Alexander Graf]

Oh, did the code that break this make it upstream already? I'll send a new pull request with my patches to fix all the regressions I've encountered ASAP.

Alex


On 20.07.2011, at 11:14, Avi Kivity wrote:

> Signed-off-by: Avi Kivity <avi@redhat.com>
> ---
> xen-mapcache.h |   32 ++++++++++++++++++++++++++++++++
> 1 files changed, 32 insertions(+), 0 deletions(-)
> 
> diff --git a/xen-mapcache.h b/xen-mapcache.h
> index 606b8af..da874ca 100644
> --- a/xen-mapcache.h
> +++ b/xen-mapcache.h
> @@ -9,6 +9,10 @@
> #ifndef XEN_MAPCACHE_H
> #define XEN_MAPCACHE_H
> 
> +#include <stdlib.h>
> +
> +#ifdef CONFIG_XEN
> +
> void xen_map_cache_init(void);
> uint8_t *xen_map_cache(target_phys_addr_t phys_addr, target_phys_addr_t size,
>                        uint8_t lock);
> @@ -16,4 +20,32 @@ ram_addr_t xen_ram_addr_from_mapcache(void *ptr);
> void xen_invalidate_map_cache_entry(uint8_t *buffer);
> void xen_invalidate_map_cache(void);
> 
> +#else
> +
> +static inline void xen_map_cache_init(void)
> +{
> +}
> +
> +static inline uint8_t *xen_map_cache(target_phys_addr_t phys_addr,
> +                                     target_phys_addr_t size,
> +                                     uint8_t lock)
> +{
> +    abort();
> +}
> +
> +static inline ram_addr_t xen_ram_addr_from_mapcache(void *ptr)
> +{
> +    abort();
> +}
> +
> +static inline void xen_invalidate_map_cache_entry(uint8_t *buffer)
> +{
> +}
> +
> +static inline void xen_invalidate_map_cache(void)
> +{
> +}
> +
> +#endif
> +
> #endif /* !XEN_MAPCACHE_H */
> -- 
> 1.7.5.3
> 

Re: [Qemu-devel] [PATCH] xen: fix xen-mapcache build on non-Xen capable targets

[Stefano Stabellini]

On Wed, 20 Jul 2011, Avi Kivity wrote:
> Signed-off-by: Avi Kivity <avi@redhat.com>

Alex sent another patch few days ago to do the same thing:

http://marc.info/?l=qemu-devel&m=131099500331906&w=2

it hasn't been merged yet but it is in Alex's xen-next branch.

Re: [Qemu-devel] [PATCH] xen: fix xen-mapcache build on non-Xen capable targets

[Stefan Weil]

Am 20.07.2011 13:01, schrieb Stefano Stabellini:
> On Wed, 20 Jul 2011, Avi Kivity wrote:
>> Signed-off-by: Avi Kivity <avi@redhat.com>
>
> Alex sent another patch few days ago to do the same thing:
>
> http://marc.info/?l=qemu-devel&m=131099500331906&w=2
>
> it hasn't been merged yet but it is in Alex's xen-next branch.

So there are now at least 3 solutions (because I also
had prepared a patch which is still unpublished).

The last time I had sent a patch for the same problem with
conditionalcompilation (so no stubs were needed):
http://patchwork.ozlabs.org/patch/96204/.
Jan then proposed using static inline stub functions.

IMHO, static inline stub functions (Avi's patch) are
better than stub functions (Alex' patch) because they
avoid dead code for the stub functions. As far as I
know most linkers don't remove dead code.

In Avi's patch, there remain some questions:

* CONFIG_XEN needs config.h. Is it also included
   (indirectly) in xen-all.c? I don't know that up to now,
   that's the reason why I did not publish my patch.

* I only needed three inline stub functions:

static inline uint8_t *xen_map_cache(target_phys_addr_t phys_addr,
                                      target_phys_addr_t size, uint8_t lock)
{
     return NULL;
}

static inline ram_addr_t xen_ram_addr_from_mapcache(void *ptr)
{
     return 0;
}

static void xen_invalidate_map_cache_entry(uint8_t *buffer)
{
}

* Do we need abort() and the related include statement?
   Maybe assert(!"unexpected function call") would be a better
   replacement for abort(), or simply do nothing.

No matter which solution is applied, I'd appreciate that
it is applied soon because the current builds are broken
for compilations without optimization.

Cheers,
Stefan Weil

Re: [Qemu-devel] [PATCH] xen: fix xen-mapcache build on non-Xen capable targets

[Alexander Graf]

On 20.07.2011, at 18:31, Stefan Weil wrote:

> Am 20.07.2011 13:01, schrieb Stefano Stabellini:
>> On Wed, 20 Jul 2011, Avi Kivity wrote:
>>> Signed-off-by: Avi Kivity <avi@redhat.com>
>> 
>> Alex sent another patch few days ago to do the same thing:
>> 
>> http://marc.info/?l=qemu-devel&m=131099500331906&w=2
>> 
>> it hasn't been merged yet but it is in Alex's xen-next branch.
> 
> So there are now at least 3 solutions (because I also
> had prepared a patch which is still unpublished).
> 
> The last time I had sent a patch for the same problem with
> conditionalcompilation (so no stubs were needed):
> http://patchwork.ozlabs.org/patch/96204/.
> Jan then proposed using static inline stub functions.
> 
> IMHO, static inline stub functions (Avi's patch) are
> better than stub functions (Alex' patch) because they
> avoid dead code for the stub functions. As far as I
> know most linkers don't remove dead code.

While I agree in general, in this case it's not quite as bad. Yes, the code is dead, but the more pressing question is wether it gets called at all. And that shouldn't happen. The only references we have are in if (xen_enabled()) blocks which again are optimized out normally on non-x86/xen.

> 
> In Avi's patch, there remain some questions:
> 
> * CONFIG_XEN needs config.h. Is it also included
>  (indirectly) in xen-all.c? I don't know that up to now,
>  that's the reason why I did not publish my patch.

Worst case, just include it. xen memslots functions should only be used in target specific code (exec.c and the likes), so I don't see how including config.h would break anything there.

> 
> * I only needed three inline stub functions:
> 
> static inline uint8_t *xen_map_cache(target_phys_addr_t phys_addr,
>                                     target_phys_addr_t size, uint8_t lock)
> {
>    return NULL;
> }
> 
> static inline ram_addr_t xen_ram_addr_from_mapcache(void *ptr)
> {
>    return 0;
> }
> 
> static void xen_invalidate_map_cache_entry(uint8_t *buffer)
> {
> }
> 
> * Do we need abort() and the related include statement?
>  Maybe assert(!"unexpected function call") would be a better
>  replacement for abort(), or simply do nothing.

Yes. Anything really.

> No matter which solution is applied, I'd appreciate that
> it is applied soon because the current builds are broken
> for compilations without optimization.

Yup - already sent out a pull request. Let's hope this time around it goes through faster and we can close this ugly chapter :). I'll try to keep a brown paper bad handy on kvm forum.


Alex

Re: [Qemu-devel] [PATCH] xen: fix xen-mapcache build on non-Xen capable targets

[Blue Swirl]

Thanks, applied.

On Wed, Jul 20, 2011 at 12:14 PM, Avi Kivity <avi@redhat.com> wrote:
> Signed-off-by: Avi Kivity <avi@redhat.com>
> ---
>  xen-mapcache.h |   32 ++++++++++++++++++++++++++++++++
>  1 files changed, 32 insertions(+), 0 deletions(-)
>
> diff --git a/xen-mapcache.h b/xen-mapcache.h
> index 606b8af..da874ca 100644
> --- a/xen-mapcache.h
> +++ b/xen-mapcache.h
> @@ -9,6 +9,10 @@
>  #ifndef XEN_MAPCACHE_H
>  #define XEN_MAPCACHE_H
>
> +#include <stdlib.h>
> +
> +#ifdef CONFIG_XEN
> +
>  void xen_map_cache_init(void);
>  uint8_t *xen_map_cache(target_phys_addr_t phys_addr, target_phys_addr_t size,
>                        uint8_t lock);
> @@ -16,4 +20,32 @@ ram_addr_t xen_ram_addr_from_mapcache(void *ptr);
>  void xen_invalidate_map_cache_entry(uint8_t *buffer);
>  void xen_invalidate_map_cache(void);
>
> +#else
> +
> +static inline void xen_map_cache_init(void)
> +{
> +}
> +
> +static inline uint8_t *xen_map_cache(target_phys_addr_t phys_addr,
> +                                     target_phys_addr_t size,
> +                                     uint8_t lock)
> +{
> +    abort();
> +}
> +
> +static inline ram_addr_t xen_ram_addr_from_mapcache(void *ptr)
> +{
> +    abort();
> +}
> +
> +static inline void xen_invalidate_map_cache_entry(uint8_t *buffer)
> +{
> +}
> +
> +static inline void xen_invalidate_map_cache(void)
> +{
> +}
> +
> +#endif
> +
>  #endif /* !XEN_MAPCACHE_H */
> --
> 1.7.5.3
>
>
>

Re: [Qemu-devel] [PATCH] xen: fix xen-mapcache build on non-Xen capable targets

[Alexander Graf]

Great. Thanks. Instead of just pulling my xen-next tree which had an outstanding pull request for days now you apply Avi's patch which obviously conflicts with my current queue, so I have to rebase it again.

Not everyone is as privileged as you. Some of us have to wait for Anthony or someone else to pull their trees. We can't just commit :(.

Either way - leave the patch in. I'll rebase my tree.


Alex

Am 22.07.2011 um 20:54 schrieb Blue Swirl <blauwirbel@gmail.com>:

> Thanks, applied.
> 
> On Wed, Jul 20, 2011 at 12:14 PM, Avi Kivity <avi@redhat.com> wrote:
>> Signed-off-by: Avi Kivity <avi@redhat.com>
>> ---
>>  xen-mapcache.h |   32 ++++++++++++++++++++++++++++++++
>>  1 files changed, 32 insertions(+), 0 deletions(-)
>> 
>> diff --git a/xen-mapcache.h b/xen-mapcache.h
>> index 606b8af..da874ca 100644
>> --- a/xen-mapcache.h
>> +++ b/xen-mapcache.h
>> @@ -9,6 +9,10 @@
>>  #ifndef XEN_MAPCACHE_H
>>  #define XEN_MAPCACHE_H
>> 
>> +#include <stdlib.h>
>> +
>> +#ifdef CONFIG_XEN
>> +
>>  void xen_map_cache_init(void);
>>  uint8_t *xen_map_cache(target_phys_addr_t phys_addr, target_phys_addr_t size,
>>                        uint8_t lock);
>> @@ -16,4 +20,32 @@ ram_addr_t xen_ram_addr_from_mapcache(void *ptr);
>>  void xen_invalidate_map_cache_entry(uint8_t *buffer);
>>  void xen_invalidate_map_cache(void);
>> 
>> +#else
>> +
>> +static inline void xen_map_cache_init(void)
>> +{
>> +}
>> +
>> +static inline uint8_t *xen_map_cache(target_phys_addr_t phys_addr,
>> +                                     target_phys_addr_t size,
>> +                                     uint8_t lock)
>> +{
>> +    abort();
>> +}
>> +
>> +static inline ram_addr_t xen_ram_addr_from_mapcache(void *ptr)
>> +{
>> +    abort();
>> +}
>> +
>> +static inline void xen_invalidate_map_cache_entry(uint8_t *buffer)
>> +{
>> +}
>> +
>> +static inline void xen_invalidate_map_cache(void)
>> +{
>> +}
>> +
>> +#endif
>> +
>>  #endif /* !XEN_MAPCACHE_H */
>> --
>> 1.7.5.3
>> 
>> 
>> 

Re: [Qemu-devel] [PATCH] xen: fix xen-mapcache build on non-Xen capable targets

[Blue Swirl]

On Sat, Jul 23, 2011 at 3:07 AM, Alexander Graf <agraf@suse.de> wrote:
> Great. Thanks. Instead of just pulling my xen-next tree which had an outstanding pull request for days now you apply Avi's patch which obviously conflicts with my current queue, so I have to rebase it again.

Your patch carried the tree with it, while Avi's was only a single
patch fixing specifically the broken build. Given the release
schedule, I didn't want to pull large patch sets anymore.

> Not everyone is as privileged as you. Some of us have to wait for Anthony or someone else to pull their trees. We can't just commit :(.

I don't think rebasing could be avoided even if there were 2000
developers with commit rights.

> Either way - leave the patch in. I'll rebase my tree.

Thank you for your work. If you think Avi's patch has some huge
problems, you could also revert it as the first step.

>
>
> Alex
>
> Am 22.07.2011 um 20:54 schrieb Blue Swirl <blauwirbel@gmail.com>:
>
>> Thanks, applied.
>>
>> On Wed, Jul 20, 2011 at 12:14 PM, Avi Kivity <avi@redhat.com> wrote:
>>> Signed-off-by: Avi Kivity <avi@redhat.com>
>>> ---
>>>  xen-mapcache.h |   32 ++++++++++++++++++++++++++++++++
>>>  1 files changed, 32 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/xen-mapcache.h b/xen-mapcache.h
>>> index 606b8af..da874ca 100644
>>> --- a/xen-mapcache.h
>>> +++ b/xen-mapcache.h
>>> @@ -9,6 +9,10 @@
>>>  #ifndef XEN_MAPCACHE_H
>>>  #define XEN_MAPCACHE_H
>>>
>>> +#include <stdlib.h>
>>> +
>>> +#ifdef CONFIG_XEN
>>> +
>>>  void xen_map_cache_init(void);
>>>  uint8_t *xen_map_cache(target_phys_addr_t phys_addr, target_phys_addr_t size,
>>>                        uint8_t lock);
>>> @@ -16,4 +20,32 @@ ram_addr_t xen_ram_addr_from_mapcache(void *ptr);
>>>  void xen_invalidate_map_cache_entry(uint8_t *buffer);
>>>  void xen_invalidate_map_cache(void);
>>>
>>> +#else
>>> +
>>> +static inline void xen_map_cache_init(void)
>>> +{
>>> +}
>>> +
>>> +static inline uint8_t *xen_map_cache(target_phys_addr_t phys_addr,
>>> +                                     target_phys_addr_t size,
>>> +                                     uint8_t lock)
>>> +{
>>> +    abort();
>>> +}
>>> +
>>> +static inline ram_addr_t xen_ram_addr_from_mapcache(void *ptr)
>>> +{
>>> +    abort();
>>> +}
>>> +
>>> +static inline void xen_invalidate_map_cache_entry(uint8_t *buffer)
>>> +{
>>> +}
>>> +
>>> +static inline void xen_invalidate_map_cache(void)
>>> +{
>>> +}
>>> +
>>> +#endif
>>> +
>>>  #endif /* !XEN_MAPCACHE_H */
>>> --
>>> 1.7.5.3
>>>
>>>
>>>
>

Re: [Qemu-devel] [PATCH] xen: fix xen-mapcache build on non-Xen capable targets

[Alexander Graf]

On 23.07.2011, at 08:57, Blue Swirl wrote:

> On Sat, Jul 23, 2011 at 3:07 AM, Alexander Graf <agraf@suse.de> wrote:
>> Great. Thanks. Instead of just pulling my xen-next tree which had an outstanding pull request for days now you apply Avi's patch which obviously conflicts with my current queue, so I have to rebase it again.
> 
> Your patch carried the tree with it, while Avi's was only a single
> patch fixing specifically the broken build. Given the release
> schedule, I didn't want to pull large patch sets anymore.

I see :). Anthony sounded quite contrary to that, specifically stating that pull requests that are issued until Friday get in. Oh well - deadline is past now and it's not like the other patches were important to have in 0.15 either.

> 
>> Not everyone is as privileged as you. Some of us have to wait for Anthony or someone else to pull their trees. We can't just commit :(.
> 
> I don't think rebasing could be avoided even if there were 2000
> developers with commit rights.
> 
>> Either way - leave the patch in. I'll rebase my tree.
> 
> Thank you for your work. If you think Avi's patch has some huge
> problems, you could also revert it as the first step.

Avi's patch is fine. In fact, it might even be better than the one I had in my tree. It was a mere thing of timing. Thanks a lot for caring about broken builds btw :).


Alex