[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 1/2] intel_iommu: Support IR-only mode without DMA translatio
From: |
Peter Xu |
Subject: |
Re: [PATCH 1/2] intel_iommu: Support IR-only mode without DMA translation |
Date: |
Fri, 3 Dec 2021 20:14:20 +0800 |
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