* [PATCH 1/2] intel_iommu: Support IR-only mode without DMA translation
@ 2021-12-01 20:51 David Woodhouse
2021-12-01 20:51 ` [PATCH 2/2] intel_iommu: Only allow interrupt remapping to be enabled if it's supported David Woodhouse
2021-12-02 3:49 ` [PATCH 1/2] intel_iommu: Support IR-only mode without DMA translation Jason Wang
0 siblings, 2 replies; 10+ messages in thread
From: David Woodhouse @ 2021-12-01 20:51 UTC (permalink / raw)
To: qemu-devel
Cc: Eduardo Habkost, Michael S. Tsirkin, Jason Wang,
Richard Henderson, Peter Xu, Paolo Bonzini
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>
---
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 294499ee20..ffc852d110 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -2202,7 +2202,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);
}
@@ -3100,6 +3100,7 @@ static Property vtd_properties[] = {
DEFINE_PROP_BOOL("caching-mode", IntelIOMMUState, caching_mode, FALSE),
DEFINE_PROP_BOOL("x-scalable-mode", IntelIOMMUState, scalable_mode, FALSE),
DEFINE_PROP_BOOL("dma-drain", IntelIOMMUState, dma_drain, true),
+ DEFINE_PROP_BOOL("dma-translation", IntelIOMMUState, dma_translation, true),
DEFINE_PROP_END_OF_LIST(),
};
@@ -3605,12 +3606,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 41783ee46d..42d6a6a636 100644
--- a/include/hw/i386/intel_iommu.h
+++ b/include/hw/i386/intel_iommu.h
@@ -266,6 +266,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.31.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH 2/2] intel_iommu: Only allow interrupt remapping to be enabled if it's supported
2021-12-01 20:51 [PATCH 1/2] intel_iommu: Support IR-only mode without DMA translation David Woodhouse
@ 2021-12-01 20:51 ` David Woodhouse
2021-12-02 3:53 ` Jason Wang
2021-12-02 3:49 ` [PATCH 1/2] intel_iommu: Support IR-only mode without DMA translation Jason Wang
1 sibling, 1 reply; 10+ messages in thread
From: David Woodhouse @ 2021-12-01 20:51 UTC (permalink / raw)
To: qemu-devel
Cc: Eduardo Habkost, Michael S. Tsirkin, Jason Wang,
Richard Henderson, Peter Xu, Paolo Bonzini
From: David Woodhouse <dwmw@amazon.co.uk>
We should probably check if we were meant to be exposing IR, before
letting the guest turn the IRE bit on.
Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
hw/i386/intel_iommu.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index ffc852d110..297e1972f8 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -2197,6 +2197,7 @@ static void vtd_handle_gcmd_ire(IntelIOMMUState *s, bool en)
/* Handle write to Global Command Register */
static void vtd_handle_gcmd_write(IntelIOMMUState *s)
{
+ X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(s);
uint32_t status = vtd_get_long_raw(s, DMAR_GSTS_REG);
uint32_t val = vtd_get_long_raw(s, DMAR_GCMD_REG);
uint32_t changed = status ^ val;
@@ -2218,7 +2219,8 @@ static void vtd_handle_gcmd_write(IntelIOMMUState *s)
/* Set/update the interrupt remapping root-table pointer */
vtd_handle_gcmd_sirtp(s);
}
- if (changed & VTD_GCMD_IRE) {
+ if ((changed & VTD_GCMD_IRE) &&
+ x86_iommu_ir_supported(x86_iommu)) {
/* Interrupt remap enable/disable */
vtd_handle_gcmd_ire(s, val & VTD_GCMD_IRE);
}
--
2.31.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH 2/2] intel_iommu: Only allow interrupt remapping to be enabled if it's supported
2021-12-01 20:51 ` [PATCH 2/2] intel_iommu: Only allow interrupt remapping to be enabled if it's supported David Woodhouse
@ 2021-12-02 3:53 ` Jason Wang
2021-12-03 7:42 ` Peter Xu
0 siblings, 1 reply; 10+ messages in thread
From: Jason Wang @ 2021-12-02 3:53 UTC (permalink / raw)
To: David Woodhouse
Cc: Eduardo Habkost, Michael S. Tsirkin, Richard Henderson,
qemu-devel, Peter Xu, Paolo Bonzini
On Thu, Dec 2, 2021 at 4:58 AM David Woodhouse <dwmw2@infradead.org> wrote:
>
> From: David Woodhouse <dwmw@amazon.co.uk>
>
> We should probably check if we were meant to be exposing IR, before
> letting the guest turn the IRE bit on.
This looks correct, but it's a change of guest noticeable behaviour.
It's probably fine since we don't expect a guest that enable IR
without checking ecap.
So
Acked-by: Jason Wang <jasowang@redhat.com>
>
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> ---
> hw/i386/intel_iommu.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index ffc852d110..297e1972f8 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -2197,6 +2197,7 @@ static void vtd_handle_gcmd_ire(IntelIOMMUState *s, bool en)
> /* Handle write to Global Command Register */
> static void vtd_handle_gcmd_write(IntelIOMMUState *s)
> {
> + X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(s);
> uint32_t status = vtd_get_long_raw(s, DMAR_GSTS_REG);
> uint32_t val = vtd_get_long_raw(s, DMAR_GCMD_REG);
> uint32_t changed = status ^ val;
> @@ -2218,7 +2219,8 @@ static void vtd_handle_gcmd_write(IntelIOMMUState *s)
> /* Set/update the interrupt remapping root-table pointer */
> vtd_handle_gcmd_sirtp(s);
> }
> - if (changed & VTD_GCMD_IRE) {
> + if ((changed & VTD_GCMD_IRE) &&
> + x86_iommu_ir_supported(x86_iommu)) {
> /* Interrupt remap enable/disable */
> vtd_handle_gcmd_ire(s, val & VTD_GCMD_IRE);
> }
> --
> 2.31.1
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 2/2] intel_iommu: Only allow interrupt remapping to be enabled if it's supported
2021-12-02 3:53 ` Jason Wang
@ 2021-12-03 7:42 ` Peter Xu
0 siblings, 0 replies; 10+ messages in thread
From: Peter Xu @ 2021-12-03 7:42 UTC (permalink / raw)
To: Jason Wang
Cc: Eduardo Habkost, Michael S. Tsirkin, Richard Henderson,
qemu-devel, Paolo Bonzini, David Woodhouse
On Thu, Dec 02, 2021 at 11:53:25AM +0800, Jason Wang wrote:
> On Thu, Dec 2, 2021 at 4:58 AM David Woodhouse <dwmw2@infradead.org> wrote:
> >
> > From: David Woodhouse <dwmw@amazon.co.uk>
> >
> > We should probably check if we were meant to be exposing IR, before
> > letting the guest turn the IRE bit on.
>
> This looks correct, but it's a change of guest noticeable behaviour.
> It's probably fine since we don't expect a guest that enable IR
> without checking ecap.
Agreed.
>
> So
>
> Acked-by: Jason Wang <jasowang@redhat.com>
Reviewed-by: Peter Xu <peterx@redhat.com>
--
Peter Xu
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] intel_iommu: Support IR-only mode without DMA translation
2021-12-01 20:51 [PATCH 1/2] intel_iommu: Support IR-only mode without DMA translation David Woodhouse
2021-12-01 20:51 ` [PATCH 2/2] intel_iommu: Only allow interrupt remapping to be enabled if it's supported David Woodhouse
@ 2021-12-02 3:49 ` Jason Wang
2021-12-03 7:38 ` Peter Xu
1 sibling, 1 reply; 10+ messages in thread
From: Jason Wang @ 2021-12-02 3:49 UTC (permalink / raw)
To: David Woodhouse
Cc: Eduardo Habkost, Michael S. Tsirkin, Richard Henderson,
qemu-devel, Peter Xu, Paolo Bonzini
On Thu, Dec 2, 2021 at 4:55 AM David Woodhouse <dwmw2@infradead.org> 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>
> ---
> 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 294499ee20..ffc852d110 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -2202,7 +2202,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);
> }
> @@ -3100,6 +3100,7 @@ static Property vtd_properties[] = {
> DEFINE_PROP_BOOL("caching-mode", IntelIOMMUState, caching_mode, FALSE),
> DEFINE_PROP_BOOL("x-scalable-mode", IntelIOMMUState, scalable_mode, FALSE),
> DEFINE_PROP_BOOL("dma-drain", IntelIOMMUState, dma_drain, true),
> + DEFINE_PROP_BOOL("dma-translation", IntelIOMMUState, dma_translation, true),
> DEFINE_PROP_END_OF_LIST(),
> };
>
> @@ -3605,12 +3606,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;
> + }
> }
Just wonder if this is the hardware behaviour as I see 0 is reserved
for SAGAW in vtd 3.3 spec.
Thanks
> 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 41783ee46d..42d6a6a636 100644
> --- a/include/hw/i386/intel_iommu.h
> +++ b/include/hw/i386/intel_iommu.h
> @@ -266,6 +266,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.31.1
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 1/2] intel_iommu: Support IR-only mode without DMA translation
2021-12-02 3:49 ` [PATCH 1/2] intel_iommu: Support IR-only mode without DMA translation Jason Wang
@ 2021-12-03 7:38 ` Peter Xu
2021-12-03 10:46 ` David Woodhouse
0 siblings, 1 reply; 10+ messages in thread
From: Peter Xu @ 2021-12-03 7:38 UTC (permalink / raw)
To: Jason Wang
Cc: Eduardo Habkost, Michael S. Tsirkin, Richard Henderson,
qemu-devel, Paolo Bonzini, David Woodhouse
On Thu, Dec 02, 2021 at 11:49:25AM +0800, Jason Wang wrote:
> On Thu, Dec 2, 2021 at 4:55 AM David Woodhouse <dwmw2@infradead.org> 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>
> > ---
> > 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 294499ee20..ffc852d110 100644
> > --- a/hw/i386/intel_iommu.c
> > +++ b/hw/i386/intel_iommu.c
> > @@ -2202,7 +2202,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);
> > }
> > @@ -3100,6 +3100,7 @@ static Property vtd_properties[] = {
> > DEFINE_PROP_BOOL("caching-mode", IntelIOMMUState, caching_mode, FALSE),
> > DEFINE_PROP_BOOL("x-scalable-mode", IntelIOMMUState, scalable_mode, FALSE),
> > DEFINE_PROP_BOOL("dma-drain", IntelIOMMUState, dma_drain, true),
> > + DEFINE_PROP_BOOL("dma-translation", IntelIOMMUState, dma_translation, true),
> > DEFINE_PROP_END_OF_LIST(),
> > };
> >
> > @@ -3605,12 +3606,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;
> > + }
> > }
>
> Just wonder if this is the hardware behaviour as I see 0 is reserved
> for SAGAW in vtd 3.3 spec.
Yes I have the same question. But if latest Linux & Windows work fine then it
seems ok if we have explicit use scenario with enabling IR only.
In that case commenting with some details would be more than welcomed, e.g.
mentioning Linux commit c40aaaac1018 ("iommu/vt-d: Gracefully handle DMAR units
with no supported address widths", 2020-10-07) as comments above the code.
One more question: SAGAW is a bitmask, is it intended to only set 1 bit out of
the mask? I'm afraid it could break guest OS that only support 39 bits when
the qemu cmdline has aw-bits=48 specified.
Thanks,
--
Peter Xu
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 1/2] intel_iommu: Support IR-only mode without DMA translation
2021-12-03 7:38 ` Peter Xu
@ 2021-12-03 10:46 ` David Woodhouse
2021-12-03 12:14 ` Peter Xu
0 siblings, 1 reply; 10+ messages in thread
From: David Woodhouse @ 2021-12-03 10:46 UTC (permalink / raw)
To: Peter Xu, Jason Wang
Cc: Paolo Bonzini, Michael S. Tsirkin, Richard Henderson, qemu-devel,
Eduardo Habkost
[-- Attachment #1: Type: text/plain, Size: 4412 bytes --]
On Fri, 2021-12-03 at 15:38 +0800, Peter Xu wrote:
> On Thu, Dec 02, 2021 at 11:49:25AM +0800, Jason Wang wrote:
> > On Thu, Dec 2, 2021 at 4:55 AM David Woodhouse <dwmw2@infradead.org> 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>
> > > ---
> > > 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 294499ee20..ffc852d110 100644
> > > --- a/hw/i386/intel_iommu.c
> > > +++ b/hw/i386/intel_iommu.c
> > > @@ -2202,7 +2202,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);
> > > }
> > > @@ -3100,6 +3100,7 @@ static Property vtd_properties[] = {
> > > DEFINE_PROP_BOOL("caching-mode", IntelIOMMUState, caching_mode, FALSE),
> > > DEFINE_PROP_BOOL("x-scalable-mode", IntelIOMMUState, scalable_mode, FALSE),
> > > DEFINE_PROP_BOOL("dma-drain", IntelIOMMUState, dma_drain, true),
> > > + DEFINE_PROP_BOOL("dma-translation", IntelIOMMUState, dma_translation, true),
> > > DEFINE_PROP_END_OF_LIST(),
> > > };
> > >
> > > @@ -3605,12 +3606,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;
> > > + }
> > > }
> >
> > Just wonder if this is the hardware behaviour as I see 0 is reserved
> > for SAGAW in vtd 3.3 spec.
>
> Yes I have the same question. But if latest Linux & Windows work fine then it
> seems ok if we have explicit use scenario with enabling IR only.
Bit zero is reserved. The *value* zero is just what you get when none
of the bits are set.
"A value of 1 in any of these bits indicates the corresponding
adjusted guest address width is supported.The adjusted guest
address widths corresponding to various bit positions within
this field are:
• 0: Reserved
• 1: 39-bit AGAW (3-level page-table)
• 2: 48-bit AGAW (4-level page-table)
• 3: 57-bit AGAW (5-level page-table)
• 4: Reserved
Software must ensure that the adjusted guest address width used
to set up the page tables is one of the supported guest address
widths reported in this field.
Hardware implementations reporting second-level translation
support (SLTS) field as Clear also report this field as 0.
> In that case commenting with some details would be more than welcomed, e.g.
> mentioning Linux commit c40aaaac1018 ("iommu/vt-d: Gracefully handle DMAR units
> with no supported address widths", 2020-10-07) as comments above the code.
>
> One more question: SAGAW is a bitmask, is it intended to only set 1 bit out of
> the mask? I'm afraid it could break guest OS that only support 39 bits when
> the qemu cmdline has aw-bits=48 specified.
No, we should set all bits that correspond to supported page table
depths. And I believe we still do; there's no 'else' in the patch above
so it will set *both* the 39-bit and 48-bit support bits if s->aw_bits
is high enough to support both.
[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5174 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 1/2] intel_iommu: Support IR-only mode without DMA translation
2021-12-03 10:46 ` David Woodhouse
@ 2021-12-03 12:14 ` Peter Xu
2021-12-06 6:49 ` Jason Wang
0 siblings, 1 reply; 10+ messages in thread
From: Peter Xu @ 2021-12-03 12:14 UTC (permalink / raw)
To: David Woodhouse
Cc: Eduardo Habkost, Michael S. Tsirkin, Jason Wang,
Richard Henderson, qemu-devel, Paolo Bonzini
On Fri, Dec 03, 2021 at 10:46:46AM +0000, David Woodhouse wrote:
> On Fri, 2021-12-03 at 15:38 +0800, Peter Xu wrote:
> > On Thu, Dec 02, 2021 at 11:49:25AM +0800, Jason Wang wrote:
> > > On Thu, Dec 2, 2021 at 4:55 AM David Woodhouse <dwmw2@infradead.org> 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>
> > > > ---
> > > > 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 294499ee20..ffc852d110 100644
> > > > --- a/hw/i386/intel_iommu.c
> > > > +++ b/hw/i386/intel_iommu.c
> > > > @@ -2202,7 +2202,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);
> > > > }
> > > > @@ -3100,6 +3100,7 @@ static Property vtd_properties[] = {
> > > > DEFINE_PROP_BOOL("caching-mode", IntelIOMMUState, caching_mode, FALSE),
> > > > DEFINE_PROP_BOOL("x-scalable-mode", IntelIOMMUState, scalable_mode, FALSE),
> > > > DEFINE_PROP_BOOL("dma-drain", IntelIOMMUState, dma_drain, true),
> > > > + DEFINE_PROP_BOOL("dma-translation", IntelIOMMUState, dma_translation, true),
> > > > DEFINE_PROP_END_OF_LIST(),
> > > > };
> > > >
> > > > @@ -3605,12 +3606,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;
> > > > + }
> > > > }
> > >
> > > Just wonder if this is the hardware behaviour as I see 0 is reserved
> > > for SAGAW in vtd 3.3 spec.
> >
> > Yes I have the same question. But if latest Linux & Windows work fine then it
> > seems ok if we have explicit use scenario with enabling IR only.
>
> Bit zero is reserved. The *value* zero is just what you get when none
> of the bits are set.
>
> "A value of 1 in any of these bits indicates the corresponding
> adjusted guest address width is supported.The adjusted guest
> address widths corresponding to various bit positions within
> this field are:
>
> • 0: Reserved
> • 1: 39-bit AGAW (3-level page-table)
> • 2: 48-bit AGAW (4-level page-table)
> • 3: 57-bit AGAW (5-level page-table)
> • 4: Reserved
>
> Software must ensure that the adjusted guest address width used
> to set up the page tables is one of the supported guest address
> widths reported in this field.
>
> Hardware implementations reporting second-level translation
> support (SLTS) field as Clear also report this field as 0.
I see.
>
> > In that case commenting with some details would be more than welcomed, e.g.
> > mentioning Linux commit c40aaaac1018 ("iommu/vt-d: Gracefully handle DMAR units
> > with no supported address widths", 2020-10-07) as comments above the code.
> >
> > One more question: SAGAW is a bitmask, is it intended to only set 1 bit out of
> > the mask? I'm afraid it could break guest OS that only support 39 bits when
> > the qemu cmdline has aw-bits=48 specified.
>
> No, we should set all bits that correspond to supported page table
> depths. And I believe we still do; there's no 'else' in the patch above
> so it will set *both* the 39-bit and 48-bit support bits if s->aw_bits
> is high enough to support both.
Yeah I missed that, sorry.
Reviewed-by: Peter Xu <peterx@redhat.com>
Thanks,
--
Peter Xu
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 1/2] intel_iommu: Support IR-only mode without DMA translation
2021-12-03 12:14 ` Peter Xu
@ 2021-12-06 6:49 ` Jason Wang
0 siblings, 0 replies; 10+ messages in thread
From: Jason Wang @ 2021-12-06 6:49 UTC (permalink / raw)
To: Peter Xu
Cc: Eduardo Habkost, Michael S. Tsirkin, Richard Henderson,
qemu-devel, Paolo Bonzini, David Woodhouse
On Fri, Dec 3, 2021 at 8:14 PM Peter Xu <peterx@redhat.com> wrote:
>
> On Fri, Dec 03, 2021 at 10:46:46AM +0000, David Woodhouse wrote:
> > On Fri, 2021-12-03 at 15:38 +0800, Peter Xu wrote:
> > > On Thu, Dec 02, 2021 at 11:49:25AM +0800, Jason Wang wrote:
> > > > On Thu, Dec 2, 2021 at 4:55 AM David Woodhouse <dwmw2@infradead.org> 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>
> > > > > ---
> > > > > 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 294499ee20..ffc852d110 100644
> > > > > --- a/hw/i386/intel_iommu.c
> > > > > +++ b/hw/i386/intel_iommu.c
> > > > > @@ -2202,7 +2202,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);
> > > > > }
> > > > > @@ -3100,6 +3100,7 @@ static Property vtd_properties[] = {
> > > > > DEFINE_PROP_BOOL("caching-mode", IntelIOMMUState, caching_mode, FALSE),
> > > > > DEFINE_PROP_BOOL("x-scalable-mode", IntelIOMMUState, scalable_mode, FALSE),
> > > > > DEFINE_PROP_BOOL("dma-drain", IntelIOMMUState, dma_drain, true),
> > > > > + DEFINE_PROP_BOOL("dma-translation", IntelIOMMUState, dma_translation, true),
> > > > > DEFINE_PROP_END_OF_LIST(),
> > > > > };
> > > > >
> > > > > @@ -3605,12 +3606,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;
> > > > > + }
> > > > > }
> > > >
> > > > Just wonder if this is the hardware behaviour as I see 0 is reserved
> > > > for SAGAW in vtd 3.3 spec.
> > >
> > > Yes I have the same question. But if latest Linux & Windows work fine then it
> > > seems ok if we have explicit use scenario with enabling IR only.
> >
> > Bit zero is reserved. The *value* zero is just what you get when none
> > of the bits are set.
> >
> > "A value of 1 in any of these bits indicates the corresponding
> > adjusted guest address width is supported.The adjusted guest
> > address widths corresponding to various bit positions within
> > this field are:
> >
> > • 0: Reserved
> > • 1: 39-bit AGAW (3-level page-table)
> > • 2: 48-bit AGAW (4-level page-table)
> > • 3: 57-bit AGAW (5-level page-table)
> > • 4: Reserved
> >
> > Software must ensure that the adjusted guest address width used
> > to set up the page tables is one of the supported guest address
> > widths reported in this field.
> >
> > Hardware implementations reporting second-level translation
> > support (SLTS) field as Clear also report this field as 0.
>
> I see.
Right.
Acked-by: Jason Wang <jasowang@redhat.com>
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/2] intel_iommu: Support IR-only mode without DMA translation
@ 2020-10-15 18:34 David Woodhouse
2020-10-15 18:34 ` [PATCH 2/2] intel_iommu: Only allow interrupt remapping to be enabled if it's supported David Woodhouse
0 siblings, 1 reply; 10+ messages in thread
From: David Woodhouse @ 2020-10-15 18:34 UTC (permalink / raw)
To: qemu-devel
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>
---
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 749eb6ad63..f49b4af931 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -2189,7 +2189,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);
}
@@ -3086,6 +3086,7 @@ static Property vtd_properties[] = {
DEFINE_PROP_BOOL("caching-mode", IntelIOMMUState, caching_mode, FALSE),
DEFINE_PROP_BOOL("x-scalable-mode", IntelIOMMUState, scalable_mode, FALSE),
DEFINE_PROP_BOOL("dma-drain", IntelIOMMUState, dma_drain, true),
+ DEFINE_PROP_BOOL("dma-translation", IntelIOMMUState, dma_translation, true),
DEFINE_PROP_END_OF_LIST(),
};
@@ -3607,12 +3608,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 41783ee46d..42d6a6a636 100644
--- a/include/hw/i386/intel_iommu.h
+++ b/include/hw/i386/intel_iommu.h
@@ -266,6 +266,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.26.2
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH 2/2] intel_iommu: Only allow interrupt remapping to be enabled if it's supported
2020-10-15 18:34 David Woodhouse
@ 2020-10-15 18:34 ` David Woodhouse
0 siblings, 0 replies; 10+ messages in thread
From: David Woodhouse @ 2020-10-15 18:34 UTC (permalink / raw)
To: qemu-devel
From: David Woodhouse <dwmw@amazon.co.uk>
We should probably check if we were meant to be exposing IR, before
letting the guest turn the IRE bit on.
Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
hw/i386/intel_iommu.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index f49b4af931..7e3570e875 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -2184,6 +2184,7 @@ static void vtd_handle_gcmd_ire(IntelIOMMUState *s, bool en)
/* Handle write to Global Command Register */
static void vtd_handle_gcmd_write(IntelIOMMUState *s)
{
+ X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(s);
uint32_t status = vtd_get_long_raw(s, DMAR_GSTS_REG);
uint32_t val = vtd_get_long_raw(s, DMAR_GCMD_REG);
uint32_t changed = status ^ val;
@@ -2205,7 +2206,8 @@ static void vtd_handle_gcmd_write(IntelIOMMUState *s)
/* Set/update the interrupt remapping root-table pointer */
vtd_handle_gcmd_sirtp(s);
}
- if (changed & VTD_GCMD_IRE) {
+ if ((changed & VTD_GCMD_IRE) &&
+ x86_iommu_ir_supported(x86_iommu)) {
/* Interrupt remap enable/disable */
vtd_handle_gcmd_ire(s, val & VTD_GCMD_IRE);
}
--
2.26.2
^ permalink raw reply related [flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-12-06 6:51 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-01 20:51 [PATCH 1/2] intel_iommu: Support IR-only mode without DMA translation David Woodhouse
2021-12-01 20:51 ` [PATCH 2/2] intel_iommu: Only allow interrupt remapping to be enabled if it's supported David Woodhouse
2021-12-02 3:53 ` Jason Wang
2021-12-03 7:42 ` Peter Xu
2021-12-02 3:49 ` [PATCH 1/2] intel_iommu: Support IR-only mode without DMA translation Jason Wang
2021-12-03 7:38 ` Peter Xu
2021-12-03 10:46 ` David Woodhouse
2021-12-03 12:14 ` Peter Xu
2021-12-06 6:49 ` Jason Wang
-- strict thread matches above, loose matches on Subject: below --
2020-10-15 18:34 David Woodhouse
2020-10-15 18:34 ` [PATCH 2/2] intel_iommu: Only allow interrupt remapping to be enabled if it's supported David Woodhouse
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.