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 <address@hidden>
>> 
>> 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