| Message ID | 20240708100816.1916346-1-pykfirst@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] hw/i386/intel_iommu: Block CFI when necessary | expand |
Hi On 08/07/2024 12:08, Yuke Peng wrote: > Caution: External email. Do not open attachments or click links, unless this email comes from a known sender and you know the content is safe. > > > According to Intel VT-d specification 5.1.4, CFI must be blocked when > Extended Interrupt Mode is enabled or Compatibility format interrupts > are disabled. > > Signed-off-by: Yuke Peng <pykfirst@gmail.com> > --- > Changes in v2: > - Use subsections for the cfi_enabled field. > - Link to v1: https://lore.kernel.org/qemu-devel/20240625112819.862282-1-pykfirst@gmail.com/ > > --- > hw/i386/intel_iommu.c | 53 +++++++++++++++++++++++++++++++++-- > hw/i386/trace-events | 1 + > include/hw/i386/intel_iommu.h | 1 + > 3 files changed, 53 insertions(+), 2 deletions(-) > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c > index 5085a6fee3..af9c864bde 100644 > --- a/hw/i386/intel_iommu.c > +++ b/hw/i386/intel_iommu.c > @@ -2410,6 +2410,22 @@ static void vtd_handle_gcmd_ire(IntelIOMMUState *s, bool en) > } > } > > +/* Handle Compatibility Format Interrupts Enable/Disable */ > +static void vtd_handle_gcmd_cfi(IntelIOMMUState *s, bool en) > +{ > + trace_vtd_cfi_enable(en); > + > + if (en) { > + s->cfi_enabled = true; > + /* Ok - report back to driver */ > + vtd_set_clear_mask_long(s, DMAR_GSTS_REG, 0, VTD_GSTS_CFIS); > + } else { > + s->cfi_enabled = false; > + /* Ok - report back to driver */ > + vtd_set_clear_mask_long(s, DMAR_GSTS_REG, VTD_GSTS_CFIS, 0); > + } > +} > + > /* Handle write to Global Command Register */ > static void vtd_handle_gcmd_write(IntelIOMMUState *s) > { > @@ -2440,6 +2456,10 @@ static void vtd_handle_gcmd_write(IntelIOMMUState *s) > /* Interrupt remap enable/disable */ > vtd_handle_gcmd_ire(s, val & VTD_GCMD_IRE); > } > + if (changed & VTD_GCMD_CFI) { > + /* Compatibility format interrupts enable/disable */ > + vtd_handle_gcmd_cfi(s, val & VTD_GCMD_CFI); > + } > } > > /* Handle write to Context Command Register */ > @@ -3283,7 +3303,25 @@ static int vtd_post_load(void *opaque, int version_id) > return 0; > } > > -static const VMStateDescription vtd_vmstate = { > +static bool vtd_cfi_needed(void *opaque) > +{ > + IntelIOMMUState *iommu = opaque; > + > + return iommu->intr_enabled && !iommu->intr_eime; > +} > + > +static const VMStateDescription vmstate_vtd_cfi = { > + .name = "iommu-intel/cfi", > + .version_id = 1, > + .minimum_version_id = 1, > + .needed = vtd_cfi_needed, > + .fields = (VMStateField[]) { > + VMSTATE_BOOL(cfi_enabled, IntelIOMMUState), > + VMSTATE_END_OF_LIST() > + } > +}; > + > +static const VMStateDescription vmstate_vtd = { Why is vtd_vmstate renamed to vmstate_vtd? is it necessary? > .name = "iommu-intel", > .version_id = 1, > .minimum_version_id = 1, > @@ -3306,6 +3344,10 @@ static const VMStateDescription vtd_vmstate = { > VMSTATE_BOOL(intr_enabled, IntelIOMMUState), > VMSTATE_BOOL(intr_eime, IntelIOMMUState), > VMSTATE_END_OF_LIST() > + }, > + .subsections = (const VMStateDescription * []) { > + &vmstate_vtd_cfi, > + NULL > } > }; > > @@ -3525,6 +3567,12 @@ static int vtd_interrupt_remap_msi(IntelIOMMUState *iommu, > > /* This is compatible mode. */ > if (addr.addr.int_mode != VTD_IR_INT_FORMAT_REMAP) { > + if (iommu->intr_eime || !iommu->cfi_enabled) { > + if (do_fault) { > + vtd_report_ir_fault(iommu, sid, VTD_FR_IR_REQ_COMPAT, 0); > + } > + return -EINVAL; > + } > memcpy(translated, origin, sizeof(*origin)); > goto out; > } > @@ -3950,6 +3998,7 @@ static void vtd_init(IntelIOMMUState *s) > s->root_scalable = false; > s->dmar_enabled = false; > s->intr_enabled = false; > + s->cfi_enabled = false; > s->iq_head = 0; > s->iq_tail = 0; > s->iq = 0; > @@ -4243,7 +4292,7 @@ static void vtd_class_init(ObjectClass *klass, void *data) > X86IOMMUClass *x86_class = X86_IOMMU_DEVICE_CLASS(klass); > > dc->reset = vtd_reset; > - dc->vmsd = &vtd_vmstate; > + dc->vmsd = &vmstate_vtd; > device_class_set_props(dc, vtd_properties); > dc->hotpluggable = false; > x86_class->realize = vtd_realize; > diff --git a/hw/i386/trace-events b/hw/i386/trace-events > index 53c02d7ac8..ffd87db65f 100644 > --- a/hw/i386/trace-events > +++ b/hw/i386/trace-events > @@ -57,6 +57,7 @@ vtd_dmar_translate(uint8_t bus, uint8_t slot, uint8_t func, uint64_t iova, uint6 > vtd_dmar_enable(bool en) "enable %d" > vtd_dmar_fault(uint16_t sid, int fault, uint64_t addr, bool is_write) "sid 0x%"PRIx16" fault %d addr 0x%"PRIx64" write %d" > vtd_ir_enable(bool en) "enable %d" > +vtd_cfi_enable(bool en) "enable %d" > vtd_ir_irte_get(int index, uint64_t lo, uint64_t hi) "index %d low 0x%"PRIx64" high 0x%"PRIx64 > vtd_ir_remap(int index, int tri, int vec, int deliver, uint32_t dest, int dest_mode) "index %d trigger %d vector %d deliver %d dest 0x%"PRIx32" mode %d" > vtd_ir_remap_type(const char *type) "%s" > diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h > index 7fa0a695c8..38e20d0f2c 100644 > --- a/include/hw/i386/intel_iommu.h > +++ b/include/hw/i386/intel_iommu.h > @@ -294,6 +294,7 @@ struct IntelIOMMUState { > > /* interrupt remapping */ > bool intr_enabled; /* Whether guest enabled IR */ > + bool cfi_enabled; /* Whether CFI is enabled */ > dma_addr_t intr_root; /* Interrupt remapping table pointer */ > uint32_t intr_size; /* Number of IR table entries */ > bool intr_eime; /* Extended interrupt mode enabled */ > -- > 2.34.1 > >
On Tue, Jul 09, 2024 at 04:43:25AM +0000, CLEMENT MATHIEU--DRIF wrote: > Hi > > On 08/07/2024 12:08, Yuke Peng wrote: > > Caution: External email. Do not open attachments or click links, unless this email comes from a known sender and you know the content is safe. > > > > > > According to Intel VT-d specification 5.1.4, CFI must be blocked when > > Extended Interrupt Mode is enabled or Compatibility format interrupts > > are disabled. > > > > Signed-off-by: Yuke Peng <pykfirst@gmail.com> > > --- > > Changes in v2: > > - Use subsections for the cfi_enabled field. > > - Link to v1: https://lore.kernel.org/qemu-devel/20240625112819.862282-1-pykfirst@gmail.com/ > > > > --- > > hw/i386/intel_iommu.c | 53 +++++++++++++++++++++++++++++++++-- > > hw/i386/trace-events | 1 + > > include/hw/i386/intel_iommu.h | 1 + > > 3 files changed, 53 insertions(+), 2 deletions(-) > > > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c > > index 5085a6fee3..af9c864bde 100644 > > --- a/hw/i386/intel_iommu.c > > +++ b/hw/i386/intel_iommu.c > > @@ -2410,6 +2410,22 @@ static void vtd_handle_gcmd_ire(IntelIOMMUState *s, bool en) > > } > > } > > > > +/* Handle Compatibility Format Interrupts Enable/Disable */ > > +static void vtd_handle_gcmd_cfi(IntelIOMMUState *s, bool en) > > +{ > > + trace_vtd_cfi_enable(en); > > + > > + if (en) { > > + s->cfi_enabled = true; > > + /* Ok - report back to driver */ > > + vtd_set_clear_mask_long(s, DMAR_GSTS_REG, 0, VTD_GSTS_CFIS); > > + } else { > > + s->cfi_enabled = false; > > + /* Ok - report back to driver */ > > + vtd_set_clear_mask_long(s, DMAR_GSTS_REG, VTD_GSTS_CFIS, 0); > > + } > > +} > > + > > /* Handle write to Global Command Register */ > > static void vtd_handle_gcmd_write(IntelIOMMUState *s) > > { > > @@ -2440,6 +2456,10 @@ static void vtd_handle_gcmd_write(IntelIOMMUState *s) > > /* Interrupt remap enable/disable */ > > vtd_handle_gcmd_ire(s, val & VTD_GCMD_IRE); > > } > > + if (changed & VTD_GCMD_CFI) { > > + /* Compatibility format interrupts enable/disable */ > > + vtd_handle_gcmd_cfi(s, val & VTD_GCMD_CFI); > > + } > > } > > > > /* Handle write to Context Command Register */ > > @@ -3283,7 +3303,25 @@ static int vtd_post_load(void *opaque, int version_id) > > return 0; > > } > > > > -static const VMStateDescription vtd_vmstate = { > > +static bool vtd_cfi_needed(void *opaque) > > +{ > > + IntelIOMMUState *iommu = opaque; > > + > > + return iommu->intr_enabled && !iommu->intr_eime; > > +} > > + > > +static const VMStateDescription vmstate_vtd_cfi = { > > + .name = "iommu-intel/cfi", > > + .version_id = 1, > > + .minimum_version_id = 1, > > + .needed = vtd_cfi_needed, > > + .fields = (VMStateField[]) { > > + VMSTATE_BOOL(cfi_enabled, IntelIOMMUState), > > + VMSTATE_END_OF_LIST() > > + } > > +}; > > + > > +static const VMStateDescription vmstate_vtd = { > Why is vtd_vmstate renamed to vmstate_vtd? is it necessary? Yuke Peng, do you plan to answer this? > > .name = "iommu-intel", > > .version_id = 1, > > .minimum_version_id = 1, > > @@ -3306,6 +3344,10 @@ static const VMStateDescription vtd_vmstate = { > > VMSTATE_BOOL(intr_enabled, IntelIOMMUState), > > VMSTATE_BOOL(intr_eime, IntelIOMMUState), > > VMSTATE_END_OF_LIST() > > + }, > > + .subsections = (const VMStateDescription * []) { > > + &vmstate_vtd_cfi, > > + NULL > > } > > }; > > > > @@ -3525,6 +3567,12 @@ static int vtd_interrupt_remap_msi(IntelIOMMUState *iommu, > > > > /* This is compatible mode. */ > > if (addr.addr.int_mode != VTD_IR_INT_FORMAT_REMAP) { > > + if (iommu->intr_eime || !iommu->cfi_enabled) { > > + if (do_fault) { > > + vtd_report_ir_fault(iommu, sid, VTD_FR_IR_REQ_COMPAT, 0); > > + } > > + return -EINVAL; > > + } > > memcpy(translated, origin, sizeof(*origin)); > > goto out; > > } > > @@ -3950,6 +3998,7 @@ static void vtd_init(IntelIOMMUState *s) > > s->root_scalable = false; > > s->dmar_enabled = false; > > s->intr_enabled = false; > > + s->cfi_enabled = false; > > s->iq_head = 0; > > s->iq_tail = 0; > > s->iq = 0; > > @@ -4243,7 +4292,7 @@ static void vtd_class_init(ObjectClass *klass, void *data) > > X86IOMMUClass *x86_class = X86_IOMMU_DEVICE_CLASS(klass); > > > > dc->reset = vtd_reset; > > - dc->vmsd = &vtd_vmstate; > > + dc->vmsd = &vmstate_vtd; > > device_class_set_props(dc, vtd_properties); > > dc->hotpluggable = false; > > x86_class->realize = vtd_realize; > > diff --git a/hw/i386/trace-events b/hw/i386/trace-events > > index 53c02d7ac8..ffd87db65f 100644 > > --- a/hw/i386/trace-events > > +++ b/hw/i386/trace-events > > @@ -57,6 +57,7 @@ vtd_dmar_translate(uint8_t bus, uint8_t slot, uint8_t func, uint64_t iova, uint6 > > vtd_dmar_enable(bool en) "enable %d" > > vtd_dmar_fault(uint16_t sid, int fault, uint64_t addr, bool is_write) "sid 0x%"PRIx16" fault %d addr 0x%"PRIx64" write %d" > > vtd_ir_enable(bool en) "enable %d" > > +vtd_cfi_enable(bool en) "enable %d" > > vtd_ir_irte_get(int index, uint64_t lo, uint64_t hi) "index %d low 0x%"PRIx64" high 0x%"PRIx64 > > vtd_ir_remap(int index, int tri, int vec, int deliver, uint32_t dest, int dest_mode) "index %d trigger %d vector %d deliver %d dest 0x%"PRIx32" mode %d" > > vtd_ir_remap_type(const char *type) "%s" > > diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h > > index 7fa0a695c8..38e20d0f2c 100644 > > --- a/include/hw/i386/intel_iommu.h > > +++ b/include/hw/i386/intel_iommu.h > > @@ -294,6 +294,7 @@ struct IntelIOMMUState { > > > > /* interrupt remapping */ > > bool intr_enabled; /* Whether guest enabled IR */ > > + bool cfi_enabled; /* Whether CFI is enabled */ > > dma_addr_t intr_root; /* Interrupt remapping table pointer */ > > uint32_t intr_size; /* Number of IR table entries */ > > bool intr_eime; /* Extended interrupt mode enabled */ > > -- > > 2.34.1 > > > >
Hi On Sun, Jul 21, 2024 at 3:05 AM Michael S. Tsirkin <mst@redhat.com> wrote: > > On Tue, Jul 09, 2024 at 04:43:25AM +0000, CLEMENT MATHIEU--DRIF wrote: > > Hi > > > > On 08/07/2024 12:08, Yuke Peng wrote: > > > Caution: External email. Do not open attachments or click links, unless this email comes from a known sender and you know the content is safe. > > > > > > > > > According to Intel VT-d specification 5.1.4, CFI must be blocked when > > > Extended Interrupt Mode is enabled or Compatibility format interrupts > > > are disabled. > > > > > > Signed-off-by: Yuke Peng <pykfirst@gmail.com> > > > --- > > > Changes in v2: > > > - Use subsections for the cfi_enabled field. > > > - Link to v1: https://lore.kernel.org/qemu-devel/20240625112819.862282-1-pykfirst@gmail.com/ > > > > > > --- > > > hw/i386/intel_iommu.c | 53 +++++++++++++++++++++++++++++++++-- > > > hw/i386/trace-events | 1 + > > > include/hw/i386/intel_iommu.h | 1 + > > > 3 files changed, 53 insertions(+), 2 deletions(-) > > > > > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c > > > index 5085a6fee3..af9c864bde 100644 > > > --- a/hw/i386/intel_iommu.c > > > +++ b/hw/i386/intel_iommu.c > > > @@ -2410,6 +2410,22 @@ static void vtd_handle_gcmd_ire(IntelIOMMUState *s, bool en) > > > } > > > } > > > > > > +/* Handle Compatibility Format Interrupts Enable/Disable */ > > > +static void vtd_handle_gcmd_cfi(IntelIOMMUState *s, bool en) > > > +{ > > > + trace_vtd_cfi_enable(en); > > > + > > > + if (en) { > > > + s->cfi_enabled = true; > > > + /* Ok - report back to driver */ > > > + vtd_set_clear_mask_long(s, DMAR_GSTS_REG, 0, VTD_GSTS_CFIS); > > > + } else { > > > + s->cfi_enabled = false; > > > + /* Ok - report back to driver */ > > > + vtd_set_clear_mask_long(s, DMAR_GSTS_REG, VTD_GSTS_CFIS, 0); > > > + } > > > +} > > > + > > > /* Handle write to Global Command Register */ > > > static void vtd_handle_gcmd_write(IntelIOMMUState *s) > > > { > > > @@ -2440,6 +2456,10 @@ static void vtd_handle_gcmd_write(IntelIOMMUState *s) > > > /* Interrupt remap enable/disable */ > > > vtd_handle_gcmd_ire(s, val & VTD_GCMD_IRE); > > > } > > > + if (changed & VTD_GCMD_CFI) { > > > + /* Compatibility format interrupts enable/disable */ > > > + vtd_handle_gcmd_cfi(s, val & VTD_GCMD_CFI); > > > + } > > > } > > > > > > /* Handle write to Context Command Register */ > > > @@ -3283,7 +3303,25 @@ static int vtd_post_load(void *opaque, int version_id) > > > return 0; > > > } > > > > > > -static const VMStateDescription vtd_vmstate = { > > > +static bool vtd_cfi_needed(void *opaque) > > > +{ > > > + IntelIOMMUState *iommu = opaque; > > > + > > > + return iommu->intr_enabled && !iommu->intr_eime; > > > +} > > > + > > > +static const VMStateDescription vmstate_vtd_cfi = { > > > + .name = "iommu-intel/cfi", > > > + .version_id = 1, > > > + .minimum_version_id = 1, > > > + .needed = vtd_cfi_needed, > > > + .fields = (VMStateField[]) { > > > + VMSTATE_BOOL(cfi_enabled, IntelIOMMUState), > > > + VMSTATE_END_OF_LIST() > > > + } > > > +}; > > > + > > > +static const VMStateDescription vmstate_vtd = { > > Why is vtd_vmstate renamed to vmstate_vtd? is it necessary? > > > > Yuke Peng, do you plan to answer this? Sorry for the late reply and for not using the "reply to all" option. I will update the reply email: On Fri, Jul 12, 2024 at 2:56 AM CLEMENT MATHIEU--DRIF <clement.mathieu--drif@eviden.com> wrote: > > On 11/07/2024 10:19, Yuke Peng wrote: > > > > On Tue, Jul 9, 2024 at 12:43 PM CLEMENT MATHIEU--DRIF <clement.mathieu--drif@eviden.com> wrote: > > > > > > Hi > > > > > > On 08/07/2024 12:08, Yuke Peng wrote: > > > > +static const VMStateDescription vmstate_vtd_cfi = { > > > > + .name = "iommu-intel/cfi", > > > > + .version_id = 1, > > > > + .minimum_version_id = 1, > > > > + .needed = vtd_cfi_needed, > > > > + .fields = (VMStateField[]) { > > > > + VMSTATE_BOOL(cfi_enabled, IntelIOMMUState), > > > > + VMSTATE_END_OF_LIST() > > > > + } > > > > +}; > > > > + > > > > +static const VMStateDescription vmstate_vtd = { > > > Why is vtd_vmstate renamed to vmstate_vtd? is it necessary? > > > > > > > When I attempted to use vtd_vmstate as a reference to name the > > cfi subsection. I found the proposed name was a bit odd for me, like > > "vtd_cfi_vmstate" or "vtd_vmstate_cfi". > > > > I checked the other examples using subsections and I found they use > > vmstate_xxx like "vmstate_serial" and "vmstate_serial_timeout_ipending". > > So here I rename vtd_vmstate to vmstate_vtd. > > Hi, > > I can't say if this will be approved or not. > But if you really think renaming is important, you should make a patch for that specific change instead of using the same patch for all changes. > > Maybe you should consider using "reply all" so that people on the list can keep track of the conversation. I had intended to split this patch into two parts, but I've been too busy to work on it. I'm sorry about that. > > > > > .name = "iommu-intel", > > > .version_id = 1, > > > .minimum_version_id = 1, > > > @@ -3306,6 +3344,10 @@ static const VMStateDescription vtd_vmstate = { > > > VMSTATE_BOOL(intr_enabled, IntelIOMMUState), > > > VMSTATE_BOOL(intr_eime, IntelIOMMUState), > > > VMSTATE_END_OF_LIST() > > > + }, > > > + .subsections = (const VMStateDescription * []) { > > > + &vmstate_vtd_cfi, > > > + NULL > > > } > > > }; > > > > > > @@ -3525,6 +3567,12 @@ static int vtd_interrupt_remap_msi(IntelIOMMUState *iommu, > > > > > > /* This is compatible mode. */ > > > if (addr.addr.int_mode != VTD_IR_INT_FORMAT_REMAP) { > > > + if (iommu->intr_eime || !iommu->cfi_enabled) { > > > + if (do_fault) { > > > + vtd_report_ir_fault(iommu, sid, VTD_FR_IR_REQ_COMPAT, 0); > > > + } > > > + return -EINVAL; > > > + } > > > memcpy(translated, origin, sizeof(*origin)); > > > goto out; > > > } > > > @@ -3950,6 +3998,7 @@ static void vtd_init(IntelIOMMUState *s) > > > s->root_scalable = false; > > > s->dmar_enabled = false; > > > s->intr_enabled = false; > > > + s->cfi_enabled = false; > > > s->iq_head = 0; > > > s->iq_tail = 0; > > > s->iq = 0; > > > @@ -4243,7 +4292,7 @@ static void vtd_class_init(ObjectClass *klass, void *data) > > > X86IOMMUClass *x86_class = X86_IOMMU_DEVICE_CLASS(klass); > > > > > > dc->reset = vtd_reset; > > > - dc->vmsd = &vtd_vmstate; > > > + dc->vmsd = &vmstate_vtd; > > > device_class_set_props(dc, vtd_properties); > > > dc->hotpluggable = false; > > > x86_class->realize = vtd_realize; > > > diff --git a/hw/i386/trace-events b/hw/i386/trace-events > > > index 53c02d7ac8..ffd87db65f 100644 > > > --- a/hw/i386/trace-events > > > +++ b/hw/i386/trace-events > > > @@ -57,6 +57,7 @@ vtd_dmar_translate(uint8_t bus, uint8_t slot, uint8_t func, uint64_t iova, uint6 > > > vtd_dmar_enable(bool en) "enable %d" > > > vtd_dmar_fault(uint16_t sid, int fault, uint64_t addr, bool is_write) "sid 0x%"PRIx16" fault %d addr 0x%"PRIx64" write %d" > > > vtd_ir_enable(bool en) "enable %d" > > > +vtd_cfi_enable(bool en) "enable %d" > > > vtd_ir_irte_get(int index, uint64_t lo, uint64_t hi) "index %d low 0x%"PRIx64" high 0x%"PRIx64 > > > vtd_ir_remap(int index, int tri, int vec, int deliver, uint32_t dest, int dest_mode) "index %d trigger %d vector %d deliver %d dest 0x%"PRIx32" mode %d" > > > vtd_ir_remap_type(const char *type) "%s" > > > diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h > > > index 7fa0a695c8..38e20d0f2c 100644 > > > --- a/include/hw/i386/intel_iommu.h > > > +++ b/include/hw/i386/intel_iommu.h > > > @@ -294,6 +294,7 @@ struct IntelIOMMUState { > > > > > > /* interrupt remapping */ > > > bool intr_enabled; /* Whether guest enabled IR */ > > > + bool cfi_enabled; /* Whether CFI is enabled */ > > > dma_addr_t intr_root; /* Interrupt remapping table pointer */ > > > uint32_t intr_size; /* Number of IR table entries */ > > > bool intr_eime; /* Extended interrupt mode enabled */ > > > -- > > > 2.34.1 > > > > > > > Yuke Peng
On Mon, Jul 08, 2024 at 06:08:16PM +0800, Yuke Peng wrote: > According to Intel VT-d specification 5.1.4, CFI must be blocked when > Extended Interrupt Mode is enabled or Compatibility format interrupts > are disabled. > > Signed-off-by: Yuke Peng <pykfirst@gmail.com> The rename is fine. The issue with the patch is the extra section. We need to avoid adding it for compat machine types. Do you have the time to address this? > --- > Changes in v2: > - Use subsections for the cfi_enabled field. > - Link to v1: https://lore.kernel.org/qemu-devel/20240625112819.862282-1-pykfirst@gmail.com/ > > --- > hw/i386/intel_iommu.c | 53 +++++++++++++++++++++++++++++++++-- > hw/i386/trace-events | 1 + > include/hw/i386/intel_iommu.h | 1 + > 3 files changed, 53 insertions(+), 2 deletions(-) > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c > index 5085a6fee3..af9c864bde 100644 > --- a/hw/i386/intel_iommu.c > +++ b/hw/i386/intel_iommu.c > @@ -2410,6 +2410,22 @@ static void vtd_handle_gcmd_ire(IntelIOMMUState *s, bool en) > } > } > > +/* Handle Compatibility Format Interrupts Enable/Disable */ > +static void vtd_handle_gcmd_cfi(IntelIOMMUState *s, bool en) > +{ > + trace_vtd_cfi_enable(en); > + > + if (en) { > + s->cfi_enabled = true; > + /* Ok - report back to driver */ > + vtd_set_clear_mask_long(s, DMAR_GSTS_REG, 0, VTD_GSTS_CFIS); > + } else { > + s->cfi_enabled = false; > + /* Ok - report back to driver */ > + vtd_set_clear_mask_long(s, DMAR_GSTS_REG, VTD_GSTS_CFIS, 0); > + } > +} > + > /* Handle write to Global Command Register */ > static void vtd_handle_gcmd_write(IntelIOMMUState *s) > { > @@ -2440,6 +2456,10 @@ static void vtd_handle_gcmd_write(IntelIOMMUState *s) > /* Interrupt remap enable/disable */ > vtd_handle_gcmd_ire(s, val & VTD_GCMD_IRE); > } > + if (changed & VTD_GCMD_CFI) { > + /* Compatibility format interrupts enable/disable */ > + vtd_handle_gcmd_cfi(s, val & VTD_GCMD_CFI); > + } > } > > /* Handle write to Context Command Register */ > @@ -3283,7 +3303,25 @@ static int vtd_post_load(void *opaque, int version_id) > return 0; > } > > -static const VMStateDescription vtd_vmstate = { > +static bool vtd_cfi_needed(void *opaque) > +{ > + IntelIOMMUState *iommu = opaque; > + > + return iommu->intr_enabled && !iommu->intr_eime; > +} > + > +static const VMStateDescription vmstate_vtd_cfi = { > + .name = "iommu-intel/cfi", > + .version_id = 1, > + .minimum_version_id = 1, > + .needed = vtd_cfi_needed, > + .fields = (VMStateField[]) { > + VMSTATE_BOOL(cfi_enabled, IntelIOMMUState), > + VMSTATE_END_OF_LIST() > + } > +}; > + > +static const VMStateDescription vmstate_vtd = { > .name = "iommu-intel", > .version_id = 1, > .minimum_version_id = 1, > @@ -3306,6 +3344,10 @@ static const VMStateDescription vtd_vmstate = { > VMSTATE_BOOL(intr_enabled, IntelIOMMUState), > VMSTATE_BOOL(intr_eime, IntelIOMMUState), > VMSTATE_END_OF_LIST() > + }, > + .subsections = (const VMStateDescription * []) { > + &vmstate_vtd_cfi, > + NULL > } > }; > > @@ -3525,6 +3567,12 @@ static int vtd_interrupt_remap_msi(IntelIOMMUState *iommu, > > /* This is compatible mode. */ > if (addr.addr.int_mode != VTD_IR_INT_FORMAT_REMAP) { > + if (iommu->intr_eime || !iommu->cfi_enabled) { > + if (do_fault) { > + vtd_report_ir_fault(iommu, sid, VTD_FR_IR_REQ_COMPAT, 0); > + } > + return -EINVAL; > + } > memcpy(translated, origin, sizeof(*origin)); > goto out; > } > @@ -3950,6 +3998,7 @@ static void vtd_init(IntelIOMMUState *s) > s->root_scalable = false; > s->dmar_enabled = false; > s->intr_enabled = false; > + s->cfi_enabled = false; > s->iq_head = 0; > s->iq_tail = 0; > s->iq = 0; > @@ -4243,7 +4292,7 @@ static void vtd_class_init(ObjectClass *klass, void *data) > X86IOMMUClass *x86_class = X86_IOMMU_DEVICE_CLASS(klass); > > dc->reset = vtd_reset; > - dc->vmsd = &vtd_vmstate; > + dc->vmsd = &vmstate_vtd; > device_class_set_props(dc, vtd_properties); > dc->hotpluggable = false; > x86_class->realize = vtd_realize; > diff --git a/hw/i386/trace-events b/hw/i386/trace-events > index 53c02d7ac8..ffd87db65f 100644 > --- a/hw/i386/trace-events > +++ b/hw/i386/trace-events > @@ -57,6 +57,7 @@ vtd_dmar_translate(uint8_t bus, uint8_t slot, uint8_t func, uint64_t iova, uint6 > vtd_dmar_enable(bool en) "enable %d" > vtd_dmar_fault(uint16_t sid, int fault, uint64_t addr, bool is_write) "sid 0x%"PRIx16" fault %d addr 0x%"PRIx64" write %d" > vtd_ir_enable(bool en) "enable %d" > +vtd_cfi_enable(bool en) "enable %d" > vtd_ir_irte_get(int index, uint64_t lo, uint64_t hi) "index %d low 0x%"PRIx64" high 0x%"PRIx64 > vtd_ir_remap(int index, int tri, int vec, int deliver, uint32_t dest, int dest_mode) "index %d trigger %d vector %d deliver %d dest 0x%"PRIx32" mode %d" > vtd_ir_remap_type(const char *type) "%s" > diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h > index 7fa0a695c8..38e20d0f2c 100644 > --- a/include/hw/i386/intel_iommu.h > +++ b/include/hw/i386/intel_iommu.h > @@ -294,6 +294,7 @@ struct IntelIOMMUState { > > /* interrupt remapping */ > bool intr_enabled; /* Whether guest enabled IR */ > + bool cfi_enabled; /* Whether CFI is enabled */ > dma_addr_t intr_root; /* Interrupt remapping table pointer */ > uint32_t intr_size; /* Number of IR table entries */ > bool intr_eime; /* Extended interrupt mode enabled */ > -- > 2.34.1
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c index 5085a6fee3..af9c864bde 100644 --- a/hw/i386/intel_iommu.c +++ b/hw/i386/intel_iommu.c @@ -2410,6 +2410,22 @@ static void vtd_handle_gcmd_ire(IntelIOMMUState *s, bool en) } } +/* Handle Compatibility Format Interrupts Enable/Disable */ +static void vtd_handle_gcmd_cfi(IntelIOMMUState *s, bool en) +{ + trace_vtd_cfi_enable(en); + + if (en) { + s->cfi_enabled = true; + /* Ok - report back to driver */ + vtd_set_clear_mask_long(s, DMAR_GSTS_REG, 0, VTD_GSTS_CFIS); + } else { + s->cfi_enabled = false; + /* Ok - report back to driver */ + vtd_set_clear_mask_long(s, DMAR_GSTS_REG, VTD_GSTS_CFIS, 0); + } +} + /* Handle write to Global Command Register */ static void vtd_handle_gcmd_write(IntelIOMMUState *s) { @@ -2440,6 +2456,10 @@ static void vtd_handle_gcmd_write(IntelIOMMUState *s) /* Interrupt remap enable/disable */ vtd_handle_gcmd_ire(s, val & VTD_GCMD_IRE); } + if (changed & VTD_GCMD_CFI) { + /* Compatibility format interrupts enable/disable */ + vtd_handle_gcmd_cfi(s, val & VTD_GCMD_CFI); + } } /* Handle write to Context Command Register */ @@ -3283,7 +3303,25 @@ static int vtd_post_load(void *opaque, int version_id) return 0; } -static const VMStateDescription vtd_vmstate = { +static bool vtd_cfi_needed(void *opaque) +{ + IntelIOMMUState *iommu = opaque; + + return iommu->intr_enabled && !iommu->intr_eime; +} + +static const VMStateDescription vmstate_vtd_cfi = { + .name = "iommu-intel/cfi", + .version_id = 1, + .minimum_version_id = 1, + .needed = vtd_cfi_needed, + .fields = (VMStateField[]) { + VMSTATE_BOOL(cfi_enabled, IntelIOMMUState), + VMSTATE_END_OF_LIST() + } +}; + +static const VMStateDescription vmstate_vtd = { .name = "iommu-intel", .version_id = 1, .minimum_version_id = 1, @@ -3306,6 +3344,10 @@ static const VMStateDescription vtd_vmstate = { VMSTATE_BOOL(intr_enabled, IntelIOMMUState), VMSTATE_BOOL(intr_eime, IntelIOMMUState), VMSTATE_END_OF_LIST() + }, + .subsections = (const VMStateDescription * []) { + &vmstate_vtd_cfi, + NULL } }; @@ -3525,6 +3567,12 @@ static int vtd_interrupt_remap_msi(IntelIOMMUState *iommu, /* This is compatible mode. */ if (addr.addr.int_mode != VTD_IR_INT_FORMAT_REMAP) { + if (iommu->intr_eime || !iommu->cfi_enabled) { + if (do_fault) { + vtd_report_ir_fault(iommu, sid, VTD_FR_IR_REQ_COMPAT, 0); + } + return -EINVAL; + } memcpy(translated, origin, sizeof(*origin)); goto out; } @@ -3950,6 +3998,7 @@ static void vtd_init(IntelIOMMUState *s) s->root_scalable = false; s->dmar_enabled = false; s->intr_enabled = false; + s->cfi_enabled = false; s->iq_head = 0; s->iq_tail = 0; s->iq = 0; @@ -4243,7 +4292,7 @@ static void vtd_class_init(ObjectClass *klass, void *data) X86IOMMUClass *x86_class = X86_IOMMU_DEVICE_CLASS(klass); dc->reset = vtd_reset; - dc->vmsd = &vtd_vmstate; + dc->vmsd = &vmstate_vtd; device_class_set_props(dc, vtd_properties); dc->hotpluggable = false; x86_class->realize = vtd_realize; diff --git a/hw/i386/trace-events b/hw/i386/trace-events index 53c02d7ac8..ffd87db65f 100644 --- a/hw/i386/trace-events +++ b/hw/i386/trace-events @@ -57,6 +57,7 @@ vtd_dmar_translate(uint8_t bus, uint8_t slot, uint8_t func, uint64_t iova, uint6 vtd_dmar_enable(bool en) "enable %d" vtd_dmar_fault(uint16_t sid, int fault, uint64_t addr, bool is_write) "sid 0x%"PRIx16" fault %d addr 0x%"PRIx64" write %d" vtd_ir_enable(bool en) "enable %d" +vtd_cfi_enable(bool en) "enable %d" vtd_ir_irte_get(int index, uint64_t lo, uint64_t hi) "index %d low 0x%"PRIx64" high 0x%"PRIx64 vtd_ir_remap(int index, int tri, int vec, int deliver, uint32_t dest, int dest_mode) "index %d trigger %d vector %d deliver %d dest 0x%"PRIx32" mode %d" vtd_ir_remap_type(const char *type) "%s" diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h index 7fa0a695c8..38e20d0f2c 100644 --- a/include/hw/i386/intel_iommu.h +++ b/include/hw/i386/intel_iommu.h @@ -294,6 +294,7 @@ struct IntelIOMMUState { /* interrupt remapping */ bool intr_enabled; /* Whether guest enabled IR */ + bool cfi_enabled; /* Whether CFI is enabled */ dma_addr_t intr_root; /* Interrupt remapping table pointer */ uint32_t intr_size; /* Number of IR table entries */ bool intr_eime; /* Extended interrupt mode enabled */
According to Intel VT-d specification 5.1.4, CFI must be blocked when Extended Interrupt Mode is enabled or Compatibility format interrupts are disabled. Signed-off-by: Yuke Peng <pykfirst@gmail.com> --- Changes in v2: - Use subsections for the cfi_enabled field. - Link to v1: https://lore.kernel.org/qemu-devel/20240625112819.862282-1-pykfirst@gmail.com/ --- hw/i386/intel_iommu.c | 53 +++++++++++++++++++++++++++++++++-- hw/i386/trace-events | 1 + include/hw/i386/intel_iommu.h | 1 + 3 files changed, 53 insertions(+), 2 deletions(-)