| Message ID | 1463469353-25642-8-git-send-email-peterx@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Tue, May 17, 2016 at 10:15 AM, Peter Xu <peterx@redhat.com> wrote: > Several data structs are defined to better support the rest of the > patches: IRTE to parse remapping table entries, and IOAPIC/MSI related > structure bits to parse interrupt entries to be filled in by guest > kernel. > > Signed-off-by: Peter Xu <peterx@redhat.com> > --- > include/hw/i386/intel_iommu.h | 60 +++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 60 insertions(+) > > diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h > index cc49839..4914fe6 100644 > --- a/include/hw/i386/intel_iommu.h > +++ b/include/hw/i386/intel_iommu.h > @@ -52,6 +52,9 @@ typedef struct IntelIOMMUState IntelIOMMUState; > typedef struct VTDAddressSpace VTDAddressSpace; > typedef struct VTDIOTLBEntry VTDIOTLBEntry; > typedef struct VTDBus VTDBus; > +typedef union VTD_IRTE VTD_IRTE; > +typedef union VTD_IR_IOAPICEntry VTD_IR_IOAPICEntry; > +typedef union VTD_IR_MSIAddress VTD_IR_MSIAddress; > > /* Context-Entry */ > struct VTDContextEntry { > @@ -90,6 +93,63 @@ struct VTDIOTLBEntry { > bool write_flags; > }; > > +/* Interrupt Remapping Table Entry Definition */ > +union VTD_IRTE { > + struct { > + uint8_t present:1; /* Whether entry present/available */ > + uint8_t fault_disable:1; /* Fault Processing Disable */ > + uint8_t dest_mode:1; /* Destination Mode */ > + uint8_t redir_hint:1; /* Redirection Hint */ > + uint8_t trigger_mode:1; /* Trigger Mode */ > + uint8_t delivery_mode:3; /* Delivery Mode */ > + uint8_t __avail:4; /* Available spaces for software */ > + uint8_t __reserved_0:3; /* Reserved 0 */ > + uint8_t irte_mode:1; /* IRTE Mode */ > + uint8_t vector:8; /* Interrupt Vector */ > + uint8_t __reserved_1:8; /* Reserved 1 */ > + uint32_t dest_id:32; /* Destination ID */ > + uint16_t source_id:16; /* Source-ID */ > + uint8_t sid_q:2; /* Source-ID Qualifier */ > + uint8_t sid_vtype:2; /* Source-ID Validation Type */ > + uint64_t __reserved_2:44; /* Reserved 2 */ > + } QEMU_PACKED; > + uint64_t data[2]; > +}; > + > +/* Programming format for IOAPIC table entries */ > +union VTD_IR_IOAPICEntry { > + struct { > + uint8_t vector:8; /* Vector */ > + uint8_t __zeros:3; /* Reserved (all zero) */ > + uint8_t index_h:1; /* Interrupt Index bit 15 */ > + uint8_t status:1; /* Deliver Status */ > + uint8_t polarity:1; /* Interrupt Polarity */ > + uint8_t remote_irr:1; /* Remote IRR */ > + uint8_t trigger_mode:1; /* Trigger Mode */ > + uint8_t mask:1; /* Mask */ > + uint32_t __reserved:31; /* Reserved (should all zero) */ > + uint8_t int_mode:1; /* Interrupt Format */ > + uint16_t index_l:15; /* Interrupt Index bits 14-0 */ > + } QEMU_PACKED; > + uint64_t data; > +}; > + > +/* Programming format for MSI/MSI-X addresses */ > +union VTD_IR_MSIAddress { > + struct { > + uint8_t __not_care:2; > + uint8_t index_h:1; /* Interrupt index bit 15 */ > + uint8_t sub_valid:1; /* SHV: Sub-Handle Valid bit */ > + uint8_t int_mode:1; /* Interrupt format */ > + uint16_t index_l:15; /* Interrupt index bit 14-0 */ > + uint16_t __head:12; /* Should always be: 0x0fee */ > + } QEMU_PACKED; > + uint32_t data; > +}; In a recent discussion, it was brought to my attention that you might have a problem with bitfields when the host cpu is not x86. Have you considered this ? > + > +/* When IR is enabled, all MSI/MSI-X data bits should be zero */ > +#define VTD_IR_MSI_DATA (0) > + > /* The iommu (DMAR) device state struct */ > struct IntelIOMMUState { > SysBusDevice busdev; > -- > 2.4.11 >
On Sun, May 29, 2016 at 11:20 AM, David Kiarie <davidkiarie4@gmail.com> wrote: > On Tue, May 17, 2016 at 10:15 AM, Peter Xu <peterx@redhat.com> wrote: >> Several data structs are defined to better support the rest of the >> patches: IRTE to parse remapping table entries, and IOAPIC/MSI related >> structure bits to parse interrupt entries to be filled in by guest >> kernel. >> >> Signed-off-by: Peter Xu <peterx@redhat.com> >> --- >> include/hw/i386/intel_iommu.h | 60 +++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 60 insertions(+) >> >> diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h >> index cc49839..4914fe6 100644 >> --- a/include/hw/i386/intel_iommu.h >> +++ b/include/hw/i386/intel_iommu.h >> @@ -52,6 +52,9 @@ typedef struct IntelIOMMUState IntelIOMMUState; >> typedef struct VTDAddressSpace VTDAddressSpace; >> typedef struct VTDIOTLBEntry VTDIOTLBEntry; >> typedef struct VTDBus VTDBus; >> +typedef union VTD_IRTE VTD_IRTE; >> +typedef union VTD_IR_IOAPICEntry VTD_IR_IOAPICEntry; >> +typedef union VTD_IR_MSIAddress VTD_IR_MSIAddress; >> >> /* Context-Entry */ >> struct VTDContextEntry { >> @@ -90,6 +93,63 @@ struct VTDIOTLBEntry { >> bool write_flags; >> }; >> >> +/* Interrupt Remapping Table Entry Definition */ >> +union VTD_IRTE { >> + struct { >> + uint8_t present:1; /* Whether entry present/available */ >> + uint8_t fault_disable:1; /* Fault Processing Disable */ >> + uint8_t dest_mode:1; /* Destination Mode */ >> + uint8_t redir_hint:1; /* Redirection Hint */ >> + uint8_t trigger_mode:1; /* Trigger Mode */ >> + uint8_t delivery_mode:3; /* Delivery Mode */ >> + uint8_t __avail:4; /* Available spaces for software */ >> + uint8_t __reserved_0:3; /* Reserved 0 */ >> + uint8_t irte_mode:1; /* IRTE Mode */ >> + uint8_t vector:8; /* Interrupt Vector */ >> + uint8_t __reserved_1:8; /* Reserved 1 */ >> + uint32_t dest_id:32; /* Destination ID */ >> + uint16_t source_id:16; /* Source-ID */ >> + uint8_t sid_q:2; /* Source-ID Qualifier */ >> + uint8_t sid_vtype:2; /* Source-ID Validation Type */ >> + uint64_t __reserved_2:44; /* Reserved 2 */ >> + } QEMU_PACKED; >> + uint64_t data[2]; >> +}; >> + >> +/* Programming format for IOAPIC table entries */ >> +union VTD_IR_IOAPICEntry { >> + struct { >> + uint8_t vector:8; /* Vector */ >> + uint8_t __zeros:3; /* Reserved (all zero) */ >> + uint8_t index_h:1; /* Interrupt Index bit 15 */ >> + uint8_t status:1; /* Deliver Status */ >> + uint8_t polarity:1; /* Interrupt Polarity */ >> + uint8_t remote_irr:1; /* Remote IRR */ >> + uint8_t trigger_mode:1; /* Trigger Mode */ >> + uint8_t mask:1; /* Mask */ >> + uint32_t __reserved:31; /* Reserved (should all zero) */ >> + uint8_t int_mode:1; /* Interrupt Format */ >> + uint16_t index_l:15; /* Interrupt Index bits 14-0 */ >> + } QEMU_PACKED; >> + uint64_t data; >> +}; >> + >> +/* Programming format for MSI/MSI-X addresses */ >> +union VTD_IR_MSIAddress { >> + struct { >> + uint8_t __not_care:2; >> + uint8_t index_h:1; /* Interrupt index bit 15 */ >> + uint8_t sub_valid:1; /* SHV: Sub-Handle Valid bit */ >> + uint8_t int_mode:1; /* Interrupt format */ >> + uint16_t index_l:15; /* Interrupt index bit 14-0 */ >> + uint16_t __head:12; /* Should always be: 0x0fee */ >> + } QEMU_PACKED; >> + uint32_t data; >> +}; > > In a recent discussion, it was brought to my attention that you might > have a problem with bitfields when the host cpu is not x86. Have you > considered this ? In a case when say the host cpu is little endian. > >> + >> +/* When IR is enabled, all MSI/MSI-X data bits should be zero */ >> +#define VTD_IR_MSI_DATA (0) >> + >> /* The iommu (DMAR) device state struct */ >> struct IntelIOMMUState { >> SysBusDevice busdev; >> -- >> 2.4.11 >>
On Sun, May 29, 2016 at 11:21:35AM +0300, David Kiarie wrote: [...] > >> + > >> +/* Programming format for MSI/MSI-X addresses */ > >> +union VTD_IR_MSIAddress { > >> + struct { > >> + uint8_t __not_care:2; > >> + uint8_t index_h:1; /* Interrupt index bit 15 */ > >> + uint8_t sub_valid:1; /* SHV: Sub-Handle Valid bit */ > >> + uint8_t int_mode:1; /* Interrupt format */ > >> + uint16_t index_l:15; /* Interrupt index bit 14-0 */ > >> + uint16_t __head:12; /* Should always be: 0x0fee */ > >> + } QEMU_PACKED; > >> + uint32_t data; > >> +}; > > > > In a recent discussion, it was brought to my attention that you might > > have a problem with bitfields when the host cpu is not x86. Have you > > considered this ? > > In a case when say the host cpu is little endian. I assume you mean when host cpu is big endian. x86 was little endian, and I was testing on x86. I think you are right. I should do conditional byte swap for all uint{16/32/64} cases within the fields. For example, index_l field in above VTD_IR_MSIAddress. And there are several other cases that need special treatment in the patchset. Will go over and fix corresponding issues in next version. Thanks! -- peterx
On 2016-05-30 07:45, Peter Xu wrote: > On Sun, May 29, 2016 at 11:21:35AM +0300, David Kiarie wrote: > [...] >>>> + >>>> +/* Programming format for MSI/MSI-X addresses */ >>>> +union VTD_IR_MSIAddress { >>>> + struct { >>>> + uint8_t __not_care:2; >>>> + uint8_t index_h:1; /* Interrupt index bit 15 */ >>>> + uint8_t sub_valid:1; /* SHV: Sub-Handle Valid bit */ >>>> + uint8_t int_mode:1; /* Interrupt format */ >>>> + uint16_t index_l:15; /* Interrupt index bit 14-0 */ >>>> + uint16_t __head:12; /* Should always be: 0x0fee */ >>>> + } QEMU_PACKED; >>>> + uint32_t data; >>>> +}; >>> >>> In a recent discussion, it was brought to my attention that you might >>> have a problem with bitfields when the host cpu is not x86. Have you >>> considered this ? >> >> In a case when say the host cpu is little endian. > > I assume you mean when host cpu is big endian. x86 was little endian, > and I was testing on x86. > > I think you are right. I should do conditional byte swap for all > uint{16/32/64} cases within the fields. For example, index_l field in > above VTD_IR_MSIAddress. And there are several other cases that need > special treatment in the patchset. Will go over and fix corresponding > issues in next version. You actually need bit-swap with bit fields, see e.g. hw/net/vmxnet3.h. Jan
On Mon, May 30, 2016 at 07:56:16AM +0200, Jan Kiszka wrote: > On 2016-05-30 07:45, Peter Xu wrote: > > On Sun, May 29, 2016 at 11:21:35AM +0300, David Kiarie wrote: > > [...] > >>>> + > >>>> +/* Programming format for MSI/MSI-X addresses */ > >>>> +union VTD_IR_MSIAddress { > >>>> + struct { > >>>> + uint8_t __not_care:2; > >>>> + uint8_t index_h:1; /* Interrupt index bit 15 */ > >>>> + uint8_t sub_valid:1; /* SHV: Sub-Handle Valid bit */ > >>>> + uint8_t int_mode:1; /* Interrupt format */ > >>>> + uint16_t index_l:15; /* Interrupt index bit 14-0 */ > >>>> + uint16_t __head:12; /* Should always be: 0x0fee */ > >>>> + } QEMU_PACKED; > >>>> + uint32_t data; > >>>> +}; > >>> > >>> In a recent discussion, it was brought to my attention that you might > >>> have a problem with bitfields when the host cpu is not x86. Have you > >>> considered this ? > >> > >> In a case when say the host cpu is little endian. > > > > I assume you mean when host cpu is big endian. x86 was little endian, > > and I was testing on x86. > > > > I think you are right. I should do conditional byte swap for all > > uint{16/32/64} cases within the fields. For example, index_l field in > > above VTD_IR_MSIAddress. And there are several other cases that need > > special treatment in the patchset. Will go over and fix corresponding > > issues in next version. > > You actually need bit-swap with bit fields, see e.g. hw/net/vmxnet3.h. Not noticed about bit-field ordering before... So maybe I need both? Thanks, -- peterx
On Mon, May 30, 2016 at 11:14 AM, Peter Xu <peterx@redhat.com> wrote: > On Mon, May 30, 2016 at 07:56:16AM +0200, Jan Kiszka wrote: >> On 2016-05-30 07:45, Peter Xu wrote: >> > On Sun, May 29, 2016 at 11:21:35AM +0300, David Kiarie wrote: >> > [...] >> >>>> + >> >>>> +/* Programming format for MSI/MSI-X addresses */ >> >>>> +union VTD_IR_MSIAddress { >> >>>> + struct { >> >>>> + uint8_t __not_care:2; >> >>>> + uint8_t index_h:1; /* Interrupt index bit 15 */ >> >>>> + uint8_t sub_valid:1; /* SHV: Sub-Handle Valid bit */ >> >>>> + uint8_t int_mode:1; /* Interrupt format */ >> >>>> + uint16_t index_l:15; /* Interrupt index bit 14-0 */ >> >>>> + uint16_t __head:12; /* Should always be: 0x0fee */ >> >>>> + } QEMU_PACKED; >> >>>> + uint32_t data; >> >>>> +}; >> >>> >> >>> In a recent discussion, it was brought to my attention that you might >> >>> have a problem with bitfields when the host cpu is not x86. Have you >> >>> considered this ? >> >> >> >> In a case when say the host cpu is little endian. >> > >> > I assume you mean when host cpu is big endian. x86 was little endian, >> > and I was testing on x86. >> > >> > I think you are right. I should do conditional byte swap for all >> > uint{16/32/64} cases within the fields. For example, index_l field in >> > above VTD_IR_MSIAddress. And there are several other cases that need >> > special treatment in the patchset. Will go over and fix corresponding >> > issues in next version. >> >> You actually need bit-swap with bit fields, see e.g. hw/net/vmxnet3.h. > > Not noticed about bit-field ordering before... So maybe I need both? Yes, I think we will need both though, I think, byte swapping the whole struct will break the code but swapping individual fields is what we need. Myself, I'm defining bitfields as below: struct CMDCompletionWait { #ifdef __BIG_ENDIAN_BITFIELD uint32_t type:4; /* command type */ uint32_t reserved:8; uint64_t store_addr:49; /* addr to write */ uint32_t completion_flush:1; /* allow more executions */ uint32_t completion_int:1; /* set MMIOWAITINT */ uint32_t completion_store:1; /* write data to address */ #else uint32_t completion_store:1; uint32_t completion_int:1; uint32_t completion_flush:1; uint64_t store_addr:49; uint32_t reserved:8; uint32_t type:4; #endif /* __BIG_ENDIAN_BITFIELD */ uint64_t store_data; /* data to write */ if } QEMU_PACKED; So, the bitfields are basically aligned to a {1,2,4,8}-byte boundary. I will have to swap store_addr,type, store_data, e.t.c. > > Thanks, > > -- peterx
On Mon, May 30, 2016 at 11:54:52AM +0300, David Kiarie wrote: > On Mon, May 30, 2016 at 11:14 AM, Peter Xu <peterx@redhat.com> wrote: > > On Mon, May 30, 2016 at 07:56:16AM +0200, Jan Kiszka wrote: > >> On 2016-05-30 07:45, Peter Xu wrote: [...] > >> > > >> > I assume you mean when host cpu is big endian. x86 was little endian, > >> > and I was testing on x86. > >> > > >> > I think you are right. I should do conditional byte swap for all > >> > uint{16/32/64} cases within the fields. For example, index_l field in > >> > above VTD_IR_MSIAddress. And there are several other cases that need > >> > special treatment in the patchset. Will go over and fix corresponding > >> > issues in next version. > >> > >> You actually need bit-swap with bit fields, see e.g. hw/net/vmxnet3.h. > > > > Not noticed about bit-field ordering before... So maybe I need both? > > Yes, I think we will need both though, I think, byte swapping the > whole struct will break the code but swapping individual fields is > what we need. > > Myself, I'm defining bitfields as below: > > struct CMDCompletionWait { > > #ifdef __BIG_ENDIAN_BITFIELD > uint32_t type:4; /* command type */ > uint32_t reserved:8; > uint64_t store_addr:49; /* addr to write */ > uint32_t completion_flush:1; /* allow more executions */ > uint32_t completion_int:1; /* set MMIOWAITINT */ > uint32_t completion_store:1; /* write data to address */ I guess what we need might be this one: uint64_t type:4; /* command type */ uint64_t reserved:8; uint64_t store_addr:49; /* addr to write */ uint64_t completion_flush:1; /* allow more executions */ uint64_t completion_int:1; /* set MMIOWAITINT */ uint64_t completion_store:1; /* write data to address */ IIUC, if we define type:4 as uint32_t rather than uint64_t, it should be bits [29:32] of the struct on big endian machines, not bits [61:64]. Thanks, -- peterx
On Mon, May 30, 2016 at 12:16 PM, Peter Xu <peterx@redhat.com> wrote: > On Mon, May 30, 2016 at 11:54:52AM +0300, David Kiarie wrote: >> On Mon, May 30, 2016 at 11:14 AM, Peter Xu <peterx@redhat.com> wrote: >> > On Mon, May 30, 2016 at 07:56:16AM +0200, Jan Kiszka wrote: >> >> On 2016-05-30 07:45, Peter Xu wrote: > [...] >> >> > >> >> > I assume you mean when host cpu is big endian. x86 was little endian, >> >> > and I was testing on x86. >> >> > >> >> > I think you are right. I should do conditional byte swap for all >> >> > uint{16/32/64} cases within the fields. For example, index_l field in >> >> > above VTD_IR_MSIAddress. And there are several other cases that need >> >> > special treatment in the patchset. Will go over and fix corresponding >> >> > issues in next version. >> >> >> >> You actually need bit-swap with bit fields, see e.g. hw/net/vmxnet3.h. >> > >> > Not noticed about bit-field ordering before... So maybe I need both? >> >> Yes, I think we will need both though, I think, byte swapping the >> whole struct will break the code but swapping individual fields is >> what we need. >> >> Myself, I'm defining bitfields as below: >> >> struct CMDCompletionWait { >> >> #ifdef __BIG_ENDIAN_BITFIELD >> uint32_t type:4; /* command type */ >> uint32_t reserved:8; >> uint64_t store_addr:49; /* addr to write */ >> uint32_t completion_flush:1; /* allow more executions */ >> uint32_t completion_int:1; /* set MMIOWAITINT */ >> uint32_t completion_store:1; /* write data to address */ > > I guess what we need might be this one: > > uint64_t type:4; /* command type */ > uint64_t reserved:8; > uint64_t store_addr:49; /* addr to write */ > uint64_t completion_flush:1; /* allow more executions */ > uint64_t completion_int:1; /* set MMIOWAITINT */ > uint64_t completion_store:1; /* write data to address */ > > IIUC, if we define type:4 as uint32_t rather than uint64_t, it should > be bits [29:32] of the struct on big endian machines, not bits > [61:64]. Yes, you're right. > > Thanks, > > -- peterx
diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h index cc49839..4914fe6 100644 --- a/include/hw/i386/intel_iommu.h +++ b/include/hw/i386/intel_iommu.h @@ -52,6 +52,9 @@ typedef struct IntelIOMMUState IntelIOMMUState; typedef struct VTDAddressSpace VTDAddressSpace; typedef struct VTDIOTLBEntry VTDIOTLBEntry; typedef struct VTDBus VTDBus; +typedef union VTD_IRTE VTD_IRTE; +typedef union VTD_IR_IOAPICEntry VTD_IR_IOAPICEntry; +typedef union VTD_IR_MSIAddress VTD_IR_MSIAddress; /* Context-Entry */ struct VTDContextEntry { @@ -90,6 +93,63 @@ struct VTDIOTLBEntry { bool write_flags; }; +/* Interrupt Remapping Table Entry Definition */ +union VTD_IRTE { + struct { + uint8_t present:1; /* Whether entry present/available */ + uint8_t fault_disable:1; /* Fault Processing Disable */ + uint8_t dest_mode:1; /* Destination Mode */ + uint8_t redir_hint:1; /* Redirection Hint */ + uint8_t trigger_mode:1; /* Trigger Mode */ + uint8_t delivery_mode:3; /* Delivery Mode */ + uint8_t __avail:4; /* Available spaces for software */ + uint8_t __reserved_0:3; /* Reserved 0 */ + uint8_t irte_mode:1; /* IRTE Mode */ + uint8_t vector:8; /* Interrupt Vector */ + uint8_t __reserved_1:8; /* Reserved 1 */ + uint32_t dest_id:32; /* Destination ID */ + uint16_t source_id:16; /* Source-ID */ + uint8_t sid_q:2; /* Source-ID Qualifier */ + uint8_t sid_vtype:2; /* Source-ID Validation Type */ + uint64_t __reserved_2:44; /* Reserved 2 */ + } QEMU_PACKED; + uint64_t data[2]; +}; + +/* Programming format for IOAPIC table entries */ +union VTD_IR_IOAPICEntry { + struct { + uint8_t vector:8; /* Vector */ + uint8_t __zeros:3; /* Reserved (all zero) */ + uint8_t index_h:1; /* Interrupt Index bit 15 */ + uint8_t status:1; /* Deliver Status */ + uint8_t polarity:1; /* Interrupt Polarity */ + uint8_t remote_irr:1; /* Remote IRR */ + uint8_t trigger_mode:1; /* Trigger Mode */ + uint8_t mask:1; /* Mask */ + uint32_t __reserved:31; /* Reserved (should all zero) */ + uint8_t int_mode:1; /* Interrupt Format */ + uint16_t index_l:15; /* Interrupt Index bits 14-0 */ + } QEMU_PACKED; + uint64_t data; +}; + +/* Programming format for MSI/MSI-X addresses */ +union VTD_IR_MSIAddress { + struct { + uint8_t __not_care:2; + uint8_t index_h:1; /* Interrupt index bit 15 */ + uint8_t sub_valid:1; /* SHV: Sub-Handle Valid bit */ + uint8_t int_mode:1; /* Interrupt format */ + uint16_t index_l:15; /* Interrupt index bit 14-0 */ + uint16_t __head:12; /* Should always be: 0x0fee */ + } QEMU_PACKED; + uint32_t data; +}; + +/* When IR is enabled, all MSI/MSI-X data bits should be zero */ +#define VTD_IR_MSI_DATA (0) + /* The iommu (DMAR) device state struct */ struct IntelIOMMUState { SysBusDevice busdev;
Several data structs are defined to better support the rest of the patches: IRTE to parse remapping table entries, and IOAPIC/MSI related structure bits to parse interrupt entries to be filled in by guest kernel. Signed-off-by: Peter Xu <peterx@redhat.com> --- include/hw/i386/intel_iommu.h | 60 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+)