Re: [Qemu-devel] [PULL v3 0/6] virtio,pci: fixes and updates

["Michael S. Tsirkin" <mst@redhat.com>]

On Fri, Sep 16, 2016 at 11:57:54AM +0100, Peter Maydell wrote:
> On 15 September 2016 at 21:38, Michael S. Tsirkin <mst@redhat.com> wrote:
> > The following changes since commit d1eb8f2acba579830cf3798c3c15ce51be852c56:
> >
> >   fpu: add mechanism to check for invalid long double formats (2016-09-15 12:43:18 +0100)
> >
> > are available in the git repository at:
> >
> >   git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_upstream
> >
> > for you to fetch changes up to a5a43875b352810d29dc27e7b0fb602eb7ef2d31:
> >
> >   MAINTAINERS: add virtio-* tests (2016-09-15 23:37:16 +0300)
> >
> > ----------------------------------------------------------------
> > virtio,pci: fixes and updates
> >
> > AMD IOMMU emulation
> > virtio feature negotiation rework
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >
> > ----------------------------------------------------------------
> 
> Fails to build on ppc64be, still:
> /home/pm215/qemu/hw/i386/amd_iommu.c:245:5: error: expected ‘,’, ‘;’
> or ‘}’ before ‘uint32_t’
>      uint32_t reserved_3:29;
>      ^
> /home/pm215/qemu/hw/i386/amd_iommu.c: In function ‘amdvi_complete_ppr’:
> /home/pm215/qemu/hw/i386/amd_iommu.c:588:62: error: ‘CMDCompletePPR’
> has no member named ‘reserved_3’
>      if (pprcomp->reserved_1 || pprcomp->reserved_2 || pprcomp->reserved_3 ||
>                                                               ^
> /home/pm215/qemu/hw/i386/amd_iommu.c:589:16: error: ‘CMDCompletePPR’
> has no member named ‘reserved_4’
>          pprcomp->reserved_4 || pprcomp->reserved_5) {
>                 ^
> /home/pm215/qemu/hw/i386/amd_iommu.c:589:39: error: ‘CMDCompletePPR’
> has no member named ‘reserved_5’
>          pprcomp->reserved_4 || pprcomp->reserved_5) {
>                                        ^
> 
> Missing semicolon, again, line 237.
> 
> In fact looking at this code it just looks broken. Structures
> like this:
> 
> typedef struct QEMU_PACKED {
> #ifdef HOST_WORDS_BIGENDIAN
>     uint64_t type:4;          /* command type        */
>     uint64_t reserved_1:44;
>     uint64_t devid:16;        /* related devid       */
> #else
>     uint64_t devid:16;
>     uint64_t reserved_1:44;
>     uint64_t type:4;
> #endif /* __BIG_ENDIAN_BITFIELD */
>     uint64_t reserved_2;
> } CMDInvalIntrTable;
> 
> seem to be trying to represent bit layouts in memory using
> bitfields, but this is just not portable. It's not sufficient
> to have a "bigendian vs littleendian" set of ifdefs.
> 
> The portable way to do this is to write the code to use
> bitwise logical operations (and functions like extract64
> and deposit64) to manipulate things. As a bonus you get rid
> of all these host-specific #ifdefs that are tripping you
> up now.
> 
> It would be nice if C bitfields worked the way this code
> wants them to, but they don't, alas.
> 
> thanks
> -- PMM

I agree, I wanted to rework this in tree but maybe best to fix it first.
I dropped all this code from tree for now.

-- 
MST