Re: [Qemu-devel] [PULL 00/20] s390x/kvm: Features and fixes for 2.3

[Thomas Huth]

On Wed, 11 Mar 2015 09:09:21 +0100
Christian Borntraeger <address@hidden> wrote:

> Am 11.03.2015 um 08:57 schrieb Markus Armbruster:
> > Christian Borntraeger <address@hidden> writes:
> > 
> >> Am 10.03.2015 um 19:01 schrieb Peter Maydell:
> >>> On 10 March 2015 at 08:37, Christian Borntraeger
> >>> <address@hidden> wrote:
> >>>> Peter,
> >>>>
> >>>> this pull reuqest contains a header sync to 4.0-rc3. This should cause
> >>>> no merge conflicts with other header syncs against 4.0-rc2. So here
> >>>> is the s390 pull request.
> >>>>
> >>>> The following changes since commit 
> >>>> 277263e1b320d759a760ba6c5ea75ec268f929e5:
> >>>>
> >>>>   Merge remote-tracking branch
> >>>> remotes/agraf/tags/signed-ppc-for-upstream' into staging
> >>>> (2015-03-09 14:04:14 +0000)
> >>>>
> >>>> are available in the git repository at:
> >>>>
> >>>>   git://github.com/borntraeger/qemu.git tags/s390x-20150310
> >>>>
> >>>> for you to fetch changes up to 6342cb1443b37d249a0e2005128ade07a80a97f5:
> >>>>
> >>>>   s390-ccw: rebuild BIOS (2015-03-10 09:26:33 +0100)
> >>>
> >>> Unfortunately I see build failures on 32 bit hosts:
> >>>
> >>> /root/qemu/hw/s390x/ipl.c: In function ‘bios_translate_addr’:
> >>> /root/qemu/hw/s390x/ipl.c:101:24: error: cast from pointer to integer
> >>> of different size [-Werror=pointer-to-int-cast]
> >>> /root/qemu/hw/s390x/ipl.c: In function ‘s390_ipl_init’:
> >>> /root/qemu/hw/s390x/ipl.c:135:30: error: cast to pointer from integer
> >>> of different size [-Werror=int-to-pointer-cast]
> >>>
> >>
> >> O dear. Seems that every pull request after your vacation is somewhat 
> >> broken.
> >> I fixed up patch 19 (" s390/bios: Make the s390-ccw.img relocatable") as
> >> such
> >>
> >> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> >> index ab7fd8a..6ed18d3 100644
> >> --- a/hw/s390x/ipl.c
> >> +++ b/hw/s390x/ipl.c
> >> @@ -98,7 +98,7 @@ static const VMStateDescription vmstate_ipl = {
> >>  
> >>  static uint64_t bios_translate_addr(void *opaque, uint64_t srcaddr)
> >>  {
> >> -    uint64_t dstaddr = (uint64_t)opaque;
> >> +    uint64_t dstaddr = (uint64_t) (unsigned long) opaque;
> >>      /*
> >>       * Assuming that our s390-ccw.img was linked for starting at address 
> >> 0,
> >>       * we can simply add the destination address for the final location
> > 
> > Won't this shift the warning from hosts where sizeof(void *) !=
> > sizeof(uint64_t) to hosts where it's != sizeof(unsigned long)?
> 
> Indeed. I was only thinking about Linux, where sizeof(unsigned long) == 
> sizeof(void*).
> But Windows has a different model. We have to cover all kind of lp64, llp64 
> and friends.
> 
> > What about using uintptr_t instead?
> 
> Should work. Will have a try on Linux 32bit/64bit..no way to test on windows, 
> though :-(

Thinking about this again, I think the really proper solution is rather
this:

diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
index 437c3ea..2eb55ca 100644
--- a/hw/s390x/ipl.c
+++ b/hw/s390x/ipl.c
@@ -98,7 +98,7 @@ static const VMStateDescription vmstate_ipl = {
 
 static uint64_t bios_translate_addr(void *opaque, uint64_t srcaddr)
 {
-    uint64_t dstaddr = (uint64_t)opaque;
+    uint64_t dstaddr = *(uint64_t*)opaque;
     /*
      * Assuming that our s390-ccw.img was linked for starting at address 0,
      * we can simply add the destination address for the final location
@@ -132,7 +132,7 @@ static int s390_ipl_init(SysBusDevice *dev)
         }
 
         bios_size = load_elf(bios_filename, bios_translate_addr,
-                             (void *)fwbase, &ipl->bios_start_addr,
+                             &fwbase, &ipl->bios_start_addr,
                              NULL, NULL, 1, ELF_MACHINE, 0);
 
         if (bios_size > 0) {

Instead of passing the uint64 value in the pointer, let's rather pass a
pointer to that value instead. Then we can also be sure to get the full
64-bit value in bios_translate_addr().

 Thomas