| Message ID | 20230728142748.305341-5-thuth@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Use Clang for compiling in the 64-bit MSYS2 job | expand |
On Fri, Jul 28, 2023 at 04:27:46PM +0200, Thomas Huth wrote: > We might want to compile QEMU with Clang on Windows - but it > does not support the __attribute__((gcc_struct)) yet. So we > have to make sure that the structs will stay the same when > the compiler uses the "ms_struct" layout. The VTD_IR_TableEntry > struct is affected - rewrite it a little bit so that it works > fine with both struct layouts. > > Reported-by: Daniel P. Berrangé <berrange@redhat.com> > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > include/hw/i386/intel_iommu.h | 14 ++++++++------ > hw/i386/intel_iommu.c | 2 +- > 2 files changed, 9 insertions(+), 7 deletions(-) > > diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h > index 89dcbc5e1e..08bf220393 100644 > --- a/include/hw/i386/intel_iommu.h > +++ b/include/hw/i386/intel_iommu.h > @@ -204,18 +204,20 @@ union VTD_IR_TableEntry { > #endif > uint32_t dest_id; /* Destination ID */ > uint16_t source_id; /* Source-ID */ > + uint16_t __reserved_2; /* Reserved 2 */ > #if HOST_BIG_ENDIAN > - uint64_t __reserved_2:44; /* Reserved 2 */ > - uint64_t sid_vtype:2; /* Source-ID Validation Type */ > - uint64_t sid_q:2; /* Source-ID Qualifier */ > + uint32_t __reserved_3:28; /* Reserved 3 */ > + uint32_t sid_vtype:2; /* Source-ID Validation Type */ > + uint32_t sid_q:2; /* Source-ID Qualifier */ > #else > - uint64_t sid_q:2; /* Source-ID Qualifier */ > - uint64_t sid_vtype:2; /* Source-ID Validation Type */ > - uint64_t __reserved_2:44; /* Reserved 2 */ > + uint32_t sid_q:2; /* Source-ID Qualifier */ > + uint32_t sid_vtype:2; /* Source-ID Validation Type */ > + uint32_t __reserved_3:28; /* Reserved 3 */ Hasn't this has changed the struct layout in the else clause Old layout: source_id : 16 sid_q : 2 sid_vtype : 2 reserved_2 : 44 New layout source_id : 16 reserved_2 : 16 sid_q : 2 sid_vtype : 2 reserved_3 : 28 Was there something wrong with the change I suggested to just make source_id be a bitfield too: uint64_t source_id: 16; /* Source-ID */ which could make ms_struct layout avoid padding to the following bitfields. > #endif > } QEMU_PACKED irte; > uint64_t data[2]; > }; With regards, Daniel
On 7/28/23 07:37, Daniel P. Berrangé wrote: > On Fri, Jul 28, 2023 at 04:27:46PM +0200, Thomas Huth wrote: >> We might want to compile QEMU with Clang on Windows - but it >> does not support the __attribute__((gcc_struct)) yet. So we >> have to make sure that the structs will stay the same when >> the compiler uses the "ms_struct" layout. The VTD_IR_TableEntry >> struct is affected - rewrite it a little bit so that it works >> fine with both struct layouts. >> >> Reported-by: Daniel P. Berrangé <berrange@redhat.com> >> Signed-off-by: Thomas Huth <thuth@redhat.com> >> --- >> include/hw/i386/intel_iommu.h | 14 ++++++++------ >> hw/i386/intel_iommu.c | 2 +- >> 2 files changed, 9 insertions(+), 7 deletions(-) >> >> diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h >> index 89dcbc5e1e..08bf220393 100644 >> --- a/include/hw/i386/intel_iommu.h >> +++ b/include/hw/i386/intel_iommu.h >> @@ -204,18 +204,20 @@ union VTD_IR_TableEntry { >> #endif >> uint32_t dest_id; /* Destination ID */ >> uint16_t source_id; /* Source-ID */ >> + uint16_t __reserved_2; /* Reserved 2 */ >> #if HOST_BIG_ENDIAN >> - uint64_t __reserved_2:44; /* Reserved 2 */ >> - uint64_t sid_vtype:2; /* Source-ID Validation Type */ >> - uint64_t sid_q:2; /* Source-ID Qualifier */ >> + uint32_t __reserved_3:28; /* Reserved 3 */ >> + uint32_t sid_vtype:2; /* Source-ID Validation Type */ >> + uint32_t sid_q:2; /* Source-ID Qualifier */ >> #else >> - uint64_t sid_q:2; /* Source-ID Qualifier */ >> - uint64_t sid_vtype:2; /* Source-ID Validation Type */ >> - uint64_t __reserved_2:44; /* Reserved 2 */ >> + uint32_t sid_q:2; /* Source-ID Qualifier */ >> + uint32_t sid_vtype:2; /* Source-ID Validation Type */ >> + uint32_t __reserved_3:28; /* Reserved 3 */ > > Hasn't this has changed the struct layout in the else clause > > Old layout: > > source_id : 16 > sid_q : 2 > sid_vtype : 2 > reserved_2 : 44 > > New layout > > source_id : 16 > reserved_2 : 16 > sid_q : 2 > sid_vtype : 2 > reserved_3 : 28 > > Was there something wrong with the change I suggested to > just make source_id be a bitfield too: > > uint64_t source_id: 16; /* Source-ID */ > > which could make ms_struct layout avoid padding to the following > bitfields. > >> #endif >> } QEMU_PACKED irte; >> uint64_t data[2]; >> }; Making the point that we should never use bitfields to match hardware. This should be converted to <hw/registerfields.h>, ARRAY_FIELD_EX64. r~
On 28/07/2023 16.37, Daniel P. Berrangé wrote: > On Fri, Jul 28, 2023 at 04:27:46PM +0200, Thomas Huth wrote: >> We might want to compile QEMU with Clang on Windows - but it >> does not support the __attribute__((gcc_struct)) yet. So we >> have to make sure that the structs will stay the same when >> the compiler uses the "ms_struct" layout. The VTD_IR_TableEntry >> struct is affected - rewrite it a little bit so that it works >> fine with both struct layouts. >> >> Reported-by: Daniel P. Berrangé <berrange@redhat.com> >> Signed-off-by: Thomas Huth <thuth@redhat.com> >> --- >> include/hw/i386/intel_iommu.h | 14 ++++++++------ >> hw/i386/intel_iommu.c | 2 +- >> 2 files changed, 9 insertions(+), 7 deletions(-) >> >> diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h >> index 89dcbc5e1e..08bf220393 100644 >> --- a/include/hw/i386/intel_iommu.h >> +++ b/include/hw/i386/intel_iommu.h >> @@ -204,18 +204,20 @@ union VTD_IR_TableEntry { >> #endif >> uint32_t dest_id; /* Destination ID */ >> uint16_t source_id; /* Source-ID */ >> + uint16_t __reserved_2; /* Reserved 2 */ >> #if HOST_BIG_ENDIAN >> - uint64_t __reserved_2:44; /* Reserved 2 */ >> - uint64_t sid_vtype:2; /* Source-ID Validation Type */ >> - uint64_t sid_q:2; /* Source-ID Qualifier */ >> + uint32_t __reserved_3:28; /* Reserved 3 */ >> + uint32_t sid_vtype:2; /* Source-ID Validation Type */ >> + uint32_t sid_q:2; /* Source-ID Qualifier */ >> #else >> - uint64_t sid_q:2; /* Source-ID Qualifier */ >> - uint64_t sid_vtype:2; /* Source-ID Validation Type */ >> - uint64_t __reserved_2:44; /* Reserved 2 */ >> + uint32_t sid_q:2; /* Source-ID Qualifier */ >> + uint32_t sid_vtype:2; /* Source-ID Validation Type */ >> + uint32_t __reserved_3:28; /* Reserved 3 */ > > Hasn't this has changed the struct layout in the else clause > > Old layout: > > source_id : 16 > sid_q : 2 > sid_vtype : 2 > reserved_2 : 44 > > New layout > > source_id : 16 > reserved_2 : 16 > sid_q : 2 > sid_vtype : 2 > reserved_3 : 28 Drat, you're right, I missed the fact that the whole stuff is read and written via the uint64_t data[2] part from the union in the code ... :-( > Was there something wrong with the change I suggested to > just make source_id be a bitfield too: > > uint64_t source_id: 16; /* Source-ID */ > > which could make ms_struct layout avoid padding to the following > bitfields. That likely works, but I think we then need to add it then twice, one time in the HOST_BIG_ENDIAN at the end, and one time in the #else part? Anyway, that whole code looks like it's completely wrong on big endian machines. The struct is read via dma_memory_read() from guest memory, but then the values are never byte-swapped, except for the error_report and trace functions, e.g. entry->irte.present is used without calling le64_to_cpu() first. entry->irte.source_id is swapped with le32_to_cpu() which looks also wrong since this is a 16 bit field. Sigh. This is another good example why we shouldn't use bitfields at all in structures that exchange data. As Richard suggested in his reply, this really should be rewritten, e.g. with the stuff from hw/registerfields.h. Thomas
On Mon, Jul 31, 2023 at 10:20:40AM +0200, Thomas Huth wrote: > On 28/07/2023 16.37, Daniel P. Berrangé wrote: > > On Fri, Jul 28, 2023 at 04:27:46PM +0200, Thomas Huth wrote: > > > We might want to compile QEMU with Clang on Windows - but it > > > does not support the __attribute__((gcc_struct)) yet. So we > > > have to make sure that the structs will stay the same when > > > the compiler uses the "ms_struct" layout. The VTD_IR_TableEntry > > > struct is affected - rewrite it a little bit so that it works > > > fine with both struct layouts. > > > > > > Reported-by: Daniel P. Berrangé <berrange@redhat.com> > > > Signed-off-by: Thomas Huth <thuth@redhat.com> > > > --- > > > include/hw/i386/intel_iommu.h | 14 ++++++++------ > > > hw/i386/intel_iommu.c | 2 +- > > > 2 files changed, 9 insertions(+), 7 deletions(-) > > > > > > diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h > > > index 89dcbc5e1e..08bf220393 100644 > > > --- a/include/hw/i386/intel_iommu.h > > > +++ b/include/hw/i386/intel_iommu.h > > > @@ -204,18 +204,20 @@ union VTD_IR_TableEntry { > > > #endif > > > uint32_t dest_id; /* Destination ID */ > > > uint16_t source_id; /* Source-ID */ > > > + uint16_t __reserved_2; /* Reserved 2 */ > > > #if HOST_BIG_ENDIAN > > > - uint64_t __reserved_2:44; /* Reserved 2 */ > > > - uint64_t sid_vtype:2; /* Source-ID Validation Type */ > > > - uint64_t sid_q:2; /* Source-ID Qualifier */ > > > + uint32_t __reserved_3:28; /* Reserved 3 */ > > > + uint32_t sid_vtype:2; /* Source-ID Validation Type */ > > > + uint32_t sid_q:2; /* Source-ID Qualifier */ > > > #else > > > - uint64_t sid_q:2; /* Source-ID Qualifier */ > > > - uint64_t sid_vtype:2; /* Source-ID Validation Type */ > > > - uint64_t __reserved_2:44; /* Reserved 2 */ > > > + uint32_t sid_q:2; /* Source-ID Qualifier */ > > > + uint32_t sid_vtype:2; /* Source-ID Validation Type */ > > > + uint32_t __reserved_3:28; /* Reserved 3 */ > > > > Hasn't this has changed the struct layout in the else clause > > > > Old layout: > > > > source_id : 16 > > sid_q : 2 > > sid_vtype : 2 > > reserved_2 : 44 > > > > New layout > > > > source_id : 16 > > reserved_2 : 16 > > sid_q : 2 > > sid_vtype : 2 > > reserved_3 : 28 > > Drat, you're right, I missed the fact that the whole stuff is read and > written via the uint64_t data[2] part from the union in the code ... :-( Yes, that's actually part of the VT-d spec. > > > Was there something wrong with the change I suggested to > > just make source_id be a bitfield too: > > > > uint64_t source_id: 16; /* Source-ID */ > > > > which could make ms_struct layout avoid padding to the following > > bitfields. > > That likely works, but I think we then need to add it then twice, one time > in the HOST_BIG_ENDIAN at the end, and one time in the #else part? > > Anyway, that whole code looks like it's completely wrong on big endian > machines. The struct is read via dma_memory_read() from guest memory, but > then the values are never byte-swapped, except for the error_report and > trace functions, e.g. entry->irte.present is used without calling > le64_to_cpu() first. > entry->irte.source_id is swapped with le32_to_cpu() which looks also wrong > since this is a 16 bit field. > > Sigh. This is another good example why we shouldn't use bitfields at all in > structures that exchange data. As Richard suggested in his reply, this > really should be rewritten, e.g. with the stuff from hw/registerfields.h. I can definitely review the iommu-side changes if someone would like to finally enable that for either clang or whatever purpose. Sorry if it never worked.. But then if it's broken for 7 years since the start, it probably also means no one ever used it on big endian hosts, either, as a functionality.. so another approach is we can opt-out VT-d as a whole for big endian, if that's easier. Thanks,
On Mon, Jul 31, 2023 at 04:26:44PM -0400, Peter Xu wrote: > On Mon, Jul 31, 2023 at 10:20:40AM +0200, Thomas Huth wrote: > > On 28/07/2023 16.37, Daniel P. Berrangé wrote: > > > On Fri, Jul 28, 2023 at 04:27:46PM +0200, Thomas Huth wrote: > > > > We might want to compile QEMU with Clang on Windows - but it > > > > does not support the __attribute__((gcc_struct)) yet. So we > > > > have to make sure that the structs will stay the same when > > > > the compiler uses the "ms_struct" layout. The VTD_IR_TableEntry > > > > struct is affected - rewrite it a little bit so that it works > > > > fine with both struct layouts. > > > > > > > > Reported-by: Daniel P. Berrangé <berrange@redhat.com> > > > > Signed-off-by: Thomas Huth <thuth@redhat.com> > > > > --- > > > > include/hw/i386/intel_iommu.h | 14 ++++++++------ > > > > hw/i386/intel_iommu.c | 2 +- > > > > 2 files changed, 9 insertions(+), 7 deletions(-) > > > > > > > > diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h > > > > index 89dcbc5e1e..08bf220393 100644 > > > > --- a/include/hw/i386/intel_iommu.h > > > > +++ b/include/hw/i386/intel_iommu.h > > > > @@ -204,18 +204,20 @@ union VTD_IR_TableEntry { > > > > #endif > > > > uint32_t dest_id; /* Destination ID */ > > > > uint16_t source_id; /* Source-ID */ > > > > + uint16_t __reserved_2; /* Reserved 2 */ > > > > #if HOST_BIG_ENDIAN > > > > - uint64_t __reserved_2:44; /* Reserved 2 */ > > > > - uint64_t sid_vtype:2; /* Source-ID Validation Type */ > > > > - uint64_t sid_q:2; /* Source-ID Qualifier */ > > > > + uint32_t __reserved_3:28; /* Reserved 3 */ > > > > + uint32_t sid_vtype:2; /* Source-ID Validation Type */ > > > > + uint32_t sid_q:2; /* Source-ID Qualifier */ > > > > #else > > > > - uint64_t sid_q:2; /* Source-ID Qualifier */ > > > > - uint64_t sid_vtype:2; /* Source-ID Validation Type */ > > > > - uint64_t __reserved_2:44; /* Reserved 2 */ > > > > + uint32_t sid_q:2; /* Source-ID Qualifier */ > > > > + uint32_t sid_vtype:2; /* Source-ID Validation Type */ > > > > + uint32_t __reserved_3:28; /* Reserved 3 */ > > > > > > Hasn't this has changed the struct layout in the else clause > > > > > > Old layout: > > > > > > source_id : 16 > > > sid_q : 2 > > > sid_vtype : 2 > > > reserved_2 : 44 > > > > > > New layout > > > > > > source_id : 16 > > > reserved_2 : 16 > > > sid_q : 2 > > > sid_vtype : 2 > > > reserved_3 : 28 > > > > Drat, you're right, I missed the fact that the whole stuff is read and > > written via the uint64_t data[2] part from the union in the code ... :-( > > Yes, that's actually part of the VT-d spec. > > > > > > Was there something wrong with the change I suggested to > > > just make source_id be a bitfield too: > > > > > > uint64_t source_id: 16; /* Source-ID */ > > > > > > which could make ms_struct layout avoid padding to the following > > > bitfields. > > > > That likely works, but I think we then need to add it then twice, one time > > in the HOST_BIG_ENDIAN at the end, and one time in the #else part? > > > > Anyway, that whole code looks like it's completely wrong on big endian > > machines. The struct is read via dma_memory_read() from guest memory, but > > then the values are never byte-swapped, except for the error_report and > > trace functions, e.g. entry->irte.present is used without calling > > le64_to_cpu() first. > > entry->irte.source_id is swapped with le32_to_cpu() which looks also wrong > > since this is a 16 bit field. > > > > Sigh. This is another good example why we shouldn't use bitfields at all in > > structures that exchange data. As Richard suggested in his reply, this > > really should be rewritten, e.g. with the stuff from hw/registerfields.h. > > I can definitely review the iommu-side changes if someone would like to > finally enable that for either clang or whatever purpose. Sorry if it > never worked.. > > But then if it's broken for 7 years since the start, it probably also means > no one ever used it on big endian hosts, either, as a functionality.. so > another approach is we can opt-out VT-d as a whole for big endian, if > that's easier. > > Thanks, Let's just fix it properly please. Bad code proliferates.
diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h index 89dcbc5e1e..08bf220393 100644 --- a/include/hw/i386/intel_iommu.h +++ b/include/hw/i386/intel_iommu.h @@ -204,18 +204,20 @@ union VTD_IR_TableEntry { #endif uint32_t dest_id; /* Destination ID */ uint16_t source_id; /* Source-ID */ + uint16_t __reserved_2; /* Reserved 2 */ #if HOST_BIG_ENDIAN - uint64_t __reserved_2:44; /* Reserved 2 */ - uint64_t sid_vtype:2; /* Source-ID Validation Type */ - uint64_t sid_q:2; /* Source-ID Qualifier */ + uint32_t __reserved_3:28; /* Reserved 3 */ + uint32_t sid_vtype:2; /* Source-ID Validation Type */ + uint32_t sid_q:2; /* Source-ID Qualifier */ #else - uint64_t sid_q:2; /* Source-ID Qualifier */ - uint64_t sid_vtype:2; /* Source-ID Validation Type */ - uint64_t __reserved_2:44; /* Reserved 2 */ + uint32_t sid_q:2; /* Source-ID Qualifier */ + uint32_t sid_vtype:2; /* Source-ID Validation Type */ + uint32_t __reserved_3:28; /* Reserved 3 */ #endif } QEMU_PACKED irte; uint64_t data[2]; }; +QEMU_BUILD_BUG_ON(sizeof(union VTD_IR_TableEntry) != 16); #define VTD_IR_INT_FORMAT_COMPAT (0) /* Compatible Interrupt */ #define VTD_IR_INT_FORMAT_REMAP (1) /* Remappable Interrupt */ diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c index dcc334060c..d5c5ee0751 100644 --- a/hw/i386/intel_iommu.c +++ b/hw/i386/intel_iommu.c @@ -3335,7 +3335,7 @@ static int vtd_irte_get(IntelIOMMUState *iommu, uint16_t index, } if (entry->irte.__reserved_0 || entry->irte.__reserved_1 || - entry->irte.__reserved_2) { + entry->irte.__reserved_2 || entry->irte.__reserved_3) { error_report_once("%s: detected non-zero reserved IRTE " "(index=%u, high=0x%" PRIx64 ", low=0x%" PRIx64 ")", __func__, index, le64_to_cpu(entry->data[1]),
We might want to compile QEMU with Clang on Windows - but it does not support the __attribute__((gcc_struct)) yet. So we have to make sure that the structs will stay the same when the compiler uses the "ms_struct" layout. The VTD_IR_TableEntry struct is affected - rewrite it a little bit so that it works fine with both struct layouts. Reported-by: Daniel P. Berrangé <berrange@redhat.com> Signed-off-by: Thomas Huth <thuth@redhat.com> --- include/hw/i386/intel_iommu.h | 14 ++++++++------ hw/i386/intel_iommu.c | 2 +- 2 files changed, 9 insertions(+), 7 deletions(-)