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

[Peter Maydell]

On 15 September 2016 at 21:38, Michael S. Tsirkin <address@hidden> 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 <address@hidden>
>
> ----------------------------------------------------------------

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