| Message ID | 20220314142544.150555-2-dwmw2@infradead.org |
|---|---|
| State | New |
| Headers | show |
| Series | [1/4] target/i386: Fix sanity check on max APIC ID / X2APIC enablement | expand |
On Mon, Mar 14, 2022 at 02:25:42PM +0000, David Woodhouse wrote: > From: David Woodhouse <dwmw@amazon.co.uk> > > By setting none of the SAGAW bits we can indicate to a guest that DMA > translation isn't supported. Tested by booting Windows 10, as well as > Linux guests with the fix at https://git.kernel.org/torvalds/c/c40aaaac10 > > Signed-off-by: David Woodhouse <dwmw@amazon.co.uk> > Reviewed-by: Peter Xu <peterx@redhat.com> > Acked-by: Jason Wang <jasowang@redhat.com> this is borderline like a feature, but ... > --- > hw/i386/intel_iommu.c | 14 ++++++++++---- > include/hw/i386/intel_iommu.h | 1 + > 2 files changed, 11 insertions(+), 4 deletions(-) > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c > index 32471a44cb..948c653e74 100644 > --- a/hw/i386/intel_iommu.c > +++ b/hw/i386/intel_iommu.c > @@ -2214,7 +2214,7 @@ static void vtd_handle_gcmd_write(IntelIOMMUState *s) > uint32_t changed = status ^ val; > > trace_vtd_reg_write_gcmd(status, val); > - if (changed & VTD_GCMD_TE) { > + if ((changed & VTD_GCMD_TE) && s->dma_translation) { > /* Translation enable/disable */ > vtd_handle_gcmd_te(s, val & VTD_GCMD_TE); > } > @@ -3122,6 +3122,7 @@ static Property vtd_properties[] = { > DEFINE_PROP_BOOL("x-scalable-mode", IntelIOMMUState, scalable_mode, FALSE), > DEFINE_PROP_BOOL("snoop-control", IntelIOMMUState, snoop_control, false), > DEFINE_PROP_BOOL("dma-drain", IntelIOMMUState, dma_drain, true), > + DEFINE_PROP_BOOL("dma-translation", IntelIOMMUState, dma_translation, true), > DEFINE_PROP_END_OF_LIST(), > }; > > @@ -3627,12 +3628,17 @@ static void vtd_init(IntelIOMMUState *s) > s->next_frcd_reg = 0; > s->cap = VTD_CAP_FRO | VTD_CAP_NFR | VTD_CAP_ND | > VTD_CAP_MAMV | VTD_CAP_PSI | VTD_CAP_SLLPS | > - VTD_CAP_SAGAW_39bit | VTD_CAP_MGAW(s->aw_bits); > + VTD_CAP_MGAW(s->aw_bits); > if (s->dma_drain) { > s->cap |= VTD_CAP_DRAIN; > } > - if (s->aw_bits == VTD_HOST_AW_48BIT) { > - s->cap |= VTD_CAP_SAGAW_48bit; > + if (s->dma_translation) { > + if (s->aw_bits >= VTD_HOST_AW_39BIT) { > + s->cap |= VTD_CAP_SAGAW_39bit; > + } > + if (s->aw_bits >= VTD_HOST_AW_48BIT) { > + s->cap |= VTD_CAP_SAGAW_48bit; > + } > } > s->ecap = VTD_ECAP_QI | VTD_ECAP_IRO; > ... this looks like you are actually fixing aw_bits < VTD_HOST_AW_39BIT, right? So maybe this patch is ok like this since it also fixes a bug. Pls add this to commit log though. > diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h > index 3b5ac869db..d898be85ce 100644 > --- a/include/hw/i386/intel_iommu.h > +++ b/include/hw/i386/intel_iommu.h > @@ -267,6 +267,7 @@ struct IntelIOMMUState { > bool buggy_eim; /* Force buggy EIM unless eim=off */ > uint8_t aw_bits; /* Host/IOVA address width (in bits) */ > bool dma_drain; /* Whether DMA r/w draining enabled */ > + bool dma_translation; /* Whether DMA translation supported */ > > /* > * Protects IOMMU states in general. Currently it protects the > -- > 2.33.1
On Mon, 2022-03-14 at 11:24 -0400, Michael S. Tsirkin wrote: > On Mon, Mar 14, 2022 at 02:25:42PM +0000, David Woodhouse wrote: > > From: David Woodhouse <dwmw@amazon.co.uk> > > > > By setting none of the SAGAW bits we can indicate to a guest that DMA > > translation isn't supported. Tested by booting Windows 10, as well as > > Linux guests with the fix at https://git.kernel.org/torvalds/c/c40aaaac10 > > > > > > Signed-off-by: David Woodhouse <dwmw@amazon.co.uk> > > Reviewed-by: Peter Xu <peterx@redhat.com> > > Acked-by: Jason Wang <jasowang@redhat.com> > > this is borderline like a feature, but ... It's the opposite of a feature — it's turning the feature *off* ;) > > > > @@ -3627,12 +3628,17 @@ static void vtd_init(IntelIOMMUState *s) > > s->next_frcd_reg = 0; > > s->cap = VTD_CAP_FRO | VTD_CAP_NFR | VTD_CAP_ND | > > VTD_CAP_MAMV | VTD_CAP_PSI | VTD_CAP_SLLPS | > > - VTD_CAP_SAGAW_39bit | VTD_CAP_MGAW(s->aw_bits); > > + VTD_CAP_MGAW(s->aw_bits); > > if (s->dma_drain) { > > s->cap |= VTD_CAP_DRAIN; > > } > > - if (s->aw_bits == VTD_HOST_AW_48BIT) { > > - s->cap |= VTD_CAP_SAGAW_48bit; > > + if (s->dma_translation) { > > + if (s->aw_bits >= VTD_HOST_AW_39BIT) { > > + s->cap |= VTD_CAP_SAGAW_39bit; > > + } > > + if (s->aw_bits >= VTD_HOST_AW_48BIT) { > > + s->cap |= VTD_CAP_SAGAW_48bit; > > + } > > } > > s->ecap = VTD_ECAP_QI | VTD_ECAP_IRO; > > > > > ... this looks like you are actually fixing aw_bits < VTD_HOST_AW_39BIT, > right? So maybe this patch is ok like this since it also fixes a > bug. Pls add this to commit log though. Nah, aw_bits cannot be < VTD_HOST_AW_39BIT. We explicitly check in vtd_decide_config(), and only 39 or 48 bits are supported, corresponding to 3-level or 4-level page tables. The only time we'd want to *not* advertise 39-bit support is if we aren't advertising DMA translation at all. Which is the (anti-)feature introduced by this patch.
On Mon, 2022-03-14 at 14:25 +0000, David Woodhouse wrote: > From: David Woodhouse <dwmw@amazon.co.uk> > > By setting none of the SAGAW bits we can indicate to a guest that DMA > translation isn't supported. Tested by booting Windows 10, as well as > Linux guests with the fix at https://git.kernel.org/torvalds/c/c40aaaac10 I suppose we could update that commit message a little. Since I first posted this and the IRE bugfix in October 2020, the Linux commit it references was released in the v5.10 kernel.
On Mon, Mar 14, 2022 at 03:45:47PM +0000, David Woodhouse wrote: > On Mon, 2022-03-14 at 11:24 -0400, Michael S. Tsirkin wrote: > > On Mon, Mar 14, 2022 at 02:25:42PM +0000, David Woodhouse wrote: > > > From: David Woodhouse <dwmw@amazon.co.uk> > > > > > > By setting none of the SAGAW bits we can indicate to a guest that DMA > > > translation isn't supported. Tested by booting Windows 10, as well as > > > Linux guests with the fix at https://git.kernel.org/torvalds/c/c40aaaac10 > > > > > > > > > Signed-off-by: David Woodhouse <dwmw@amazon.co.uk> > > > Reviewed-by: Peter Xu <peterx@redhat.com> > > > Acked-by: Jason Wang <jasowang@redhat.com> > > > > this is borderline like a feature, but ... > > It's the opposite of a feature — it's turning the feature *off* ;) Right. Still - do you believe it's appropriate in soft freeze and if yes why? > > > > > > @@ -3627,12 +3628,17 @@ static void vtd_init(IntelIOMMUState *s) > > > s->next_frcd_reg = 0; > > > s->cap = VTD_CAP_FRO | VTD_CAP_NFR | VTD_CAP_ND | > > > VTD_CAP_MAMV | VTD_CAP_PSI | VTD_CAP_SLLPS | > > > - VTD_CAP_SAGAW_39bit | VTD_CAP_MGAW(s->aw_bits); > > > + VTD_CAP_MGAW(s->aw_bits); > > > if (s->dma_drain) { > > > s->cap |= VTD_CAP_DRAIN; > > > } > > > - if (s->aw_bits == VTD_HOST_AW_48BIT) { > > > - s->cap |= VTD_CAP_SAGAW_48bit; > > > + if (s->dma_translation) { > > > + if (s->aw_bits >= VTD_HOST_AW_39BIT) { > > > + s->cap |= VTD_CAP_SAGAW_39bit; > > > + } > > > + if (s->aw_bits >= VTD_HOST_AW_48BIT) { > > > + s->cap |= VTD_CAP_SAGAW_48bit; > > > + } > > > } > > > s->ecap = VTD_ECAP_QI | VTD_ECAP_IRO; > > > > > > > > > ... this looks like you are actually fixing aw_bits < VTD_HOST_AW_39BIT, > > right? So maybe this patch is ok like this since it also fixes a > > bug. Pls add this to commit log though. > > Nah, aw_bits cannot be < VTD_HOST_AW_39BIT. We explicitly check in > vtd_decide_config(), and only 39 or 48 bits are supported, > corresponding to 3-level or 4-level page tables. Oh right. So not a bugfix then. > The only time we'd want to *not* advertise 39-bit support is if we > aren't advertising DMA translation at all. Which is the (anti-)feature > introduced by this patch. >
On Mon, 2022-03-14 at 18:27 -0400, Michael S. Tsirkin wrote: > On Mon, Mar 14, 2022 at 03:45:47PM +0000, David Woodhouse wrote: > > It's the opposite of a feature — it's turning the feature *off* ;) > > Right. Still - do you believe it's appropriate in soft freeze > and if yes why? > Not sure I care very much. I've reposted it a couple of times, with the bug fixes that go along with it, since it was first posted in October 2020. I confess I don't keep track very much of the freeze status; I'm only posting the series again this time because Igor posted a related patch. But all it's doing is giving us a way to *disable* functionality; it's not adding new functionality. I suppose someone who cares might make an argument that that's not so egregious a 'feature' to slip in the middle of a set of bug fixes that have been outstanding for so long.
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c index 32471a44cb..948c653e74 100644 --- a/hw/i386/intel_iommu.c +++ b/hw/i386/intel_iommu.c @@ -2214,7 +2214,7 @@ static void vtd_handle_gcmd_write(IntelIOMMUState *s) uint32_t changed = status ^ val; trace_vtd_reg_write_gcmd(status, val); - if (changed & VTD_GCMD_TE) { + if ((changed & VTD_GCMD_TE) && s->dma_translation) { /* Translation enable/disable */ vtd_handle_gcmd_te(s, val & VTD_GCMD_TE); } @@ -3122,6 +3122,7 @@ static Property vtd_properties[] = { DEFINE_PROP_BOOL("x-scalable-mode", IntelIOMMUState, scalable_mode, FALSE), DEFINE_PROP_BOOL("snoop-control", IntelIOMMUState, snoop_control, false), DEFINE_PROP_BOOL("dma-drain", IntelIOMMUState, dma_drain, true), + DEFINE_PROP_BOOL("dma-translation", IntelIOMMUState, dma_translation, true), DEFINE_PROP_END_OF_LIST(), }; @@ -3627,12 +3628,17 @@ static void vtd_init(IntelIOMMUState *s) s->next_frcd_reg = 0; s->cap = VTD_CAP_FRO | VTD_CAP_NFR | VTD_CAP_ND | VTD_CAP_MAMV | VTD_CAP_PSI | VTD_CAP_SLLPS | - VTD_CAP_SAGAW_39bit | VTD_CAP_MGAW(s->aw_bits); + VTD_CAP_MGAW(s->aw_bits); if (s->dma_drain) { s->cap |= VTD_CAP_DRAIN; } - if (s->aw_bits == VTD_HOST_AW_48BIT) { - s->cap |= VTD_CAP_SAGAW_48bit; + if (s->dma_translation) { + if (s->aw_bits >= VTD_HOST_AW_39BIT) { + s->cap |= VTD_CAP_SAGAW_39bit; + } + if (s->aw_bits >= VTD_HOST_AW_48BIT) { + s->cap |= VTD_CAP_SAGAW_48bit; + } } s->ecap = VTD_ECAP_QI | VTD_ECAP_IRO; diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h index 3b5ac869db..d898be85ce 100644 --- a/include/hw/i386/intel_iommu.h +++ b/include/hw/i386/intel_iommu.h @@ -267,6 +267,7 @@ struct IntelIOMMUState { bool buggy_eim; /* Force buggy EIM unless eim=off */ uint8_t aw_bits; /* Host/IOVA address width (in bits) */ bool dma_drain; /* Whether DMA r/w draining enabled */ + bool dma_translation; /* Whether DMA translation supported */ /* * Protects IOMMU states in general. Currently it protects the