[PATCH] AMD/guest_iommu: Disable guest iommu support

[PATCH] AMD/guest_iommu: Disable guest iommu support

[Andrew Cooper]

AMD Guest IOMMU support was added to allow correct use of PASID and PRI
hardware support with an ATS-aware guest driver.

However, support cannot possibly function as guest_iommu_set_base() has no
callers.  This means that its MMIO region's P2M pages are not set to
p2m_mmio_dm, preventing any invocation of the MMIO read/write handlers.

c/s fd186384 "x86/HVM: extend LAPIC shortcuts around P2M lookups" introduces a
path (via hvm_mmio_internal()) where iommu_mmio_handler claims its MMIO range,
and causes __hvm_copy() to fail with HVMCOPY_bad_gfn_to_mfn.

iommu->mmio_base defaults to 0, with a range of 8 pages, and is unilaterally
enabled in any HVM guests when the host IOMMU(s) supports any extended
features.

Unfortunately, HVMLoader's AP boot trampoline executes an `lmsw` instruction
at linear address 0x100c which unconditionally requires emulation.  The
instruction fetch in turn fails as __hvm_copy() fails with
HVMCOPY_bad_gfn_to_mfn.

The result is that multi-vcpu HVM guests do not work on newer AMD hardware, if
IOMMU support is enabled in the BIOS.

Change the default mmio_base address to ~0ULL.  This prevents
guest_iommu_mmio_range() from actually claiming any physical range
whatsoever, which allows the emulation of `lmsw` to succeed.

Reported-by: Roberto Luongo <rluongo@ready.it>
Suggested-by: Jan Beulich <JBeulich@suse.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Tested-by: Roberto Luongo <rluongo@ready.it>
CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
CC: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
---
 xen/drivers/passthrough/amd/iommu_guest.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/xen/drivers/passthrough/amd/iommu_guest.c b/xen/drivers/passthrough/amd/iommu_guest.c
index 5660020..98e7b38 100644
--- a/xen/drivers/passthrough/amd/iommu_guest.c
+++ b/xen/drivers/passthrough/amd/iommu_guest.c
@@ -885,6 +885,7 @@ int guest_iommu_init(struct domain* d)
     }
 
     guest_iommu_reg_init(iommu);
+    iommu->mmio_base = ~0ULL;
     iommu->domain = d;
     hd->arch.g_iommu = iommu;
 
-- 
1.7.10.4

Re: [PATCH] AMD/guest_iommu: Disable guest iommu support

[Andrew Cooper]

On 02/10/14 15:02, Andrew Cooper wrote:
> AMD Guest IOMMU support was added to allow correct use of PASID and PRI
> hardware support with an ATS-aware guest driver.
>
> However, support cannot possibly function as guest_iommu_set_base() has no
> callers.  This means that its MMIO region's P2M pages are not set to
> p2m_mmio_dm, preventing any invocation of the MMIO read/write handlers.
>
> c/s fd186384 "x86/HVM: extend LAPIC shortcuts around P2M lookups" introduces a
> path (via hvm_mmio_internal()) where iommu_mmio_handler claims its MMIO range,
> and causes __hvm_copy() to fail with HVMCOPY_bad_gfn_to_mfn.
>
> iommu->mmio_base defaults to 0, with a range of 8 pages, and is unilaterally
> enabled in any HVM guests when the host IOMMU(s) supports any extended
> features.
>
> Unfortunately, HVMLoader's AP boot trampoline executes an `lmsw` instruction
> at linear address 0x100c which unconditionally requires emulation.  The
> instruction fetch in turn fails as __hvm_copy() fails with
> HVMCOPY_bad_gfn_to_mfn.
>
> The result is that multi-vcpu HVM guests do not work on newer AMD hardware, if
> IOMMU support is enabled in the BIOS.
>
> Change the default mmio_base address to ~0ULL.  This prevents
> guest_iommu_mmio_range() from actually claiming any physical range
> whatsoever, which allows the emulation of `lmsw` to succeed.
>
> Reported-by: Roberto Luongo <rluongo@ready.it>
> Suggested-by: Jan Beulich <JBeulich@suse.com>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Tested-by: Roberto Luongo <rluongo@ready.it>
> CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> CC: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
> ---

I forgot to explicitly indicate, but this is a bugfix and is therefore
intended for inclusion in 4.5

~Andrew

Re: [PATCH] AMD/guest_iommu: Disable guest iommu support

[Jan Beulich]

>>> On 02.10.14 at 16:08, <andrew.cooper3@citrix.com> wrote:
> I forgot to explicitly indicate, but this is a bugfix and is therefore
> intended for inclusion in 4.5

And I continue to think that this also isn't necessary until pretty
late in the release process. We really need this indication for late
features coming in.

Jan

Re: [PATCH] AMD/guest_iommu: Disable guest iommu support

[Konrad Rzeszutek Wilk]

On Thu, Oct 02, 2014 at 03:50:14PM +0100, Jan Beulich wrote:
> >>> On 02.10.14 at 16:08, <andrew.cooper3@citrix.com> wrote:
> > I forgot to explicitly indicate, but this is a bugfix and is therefore
> > intended for inclusion in 4.5
> 
> And I continue to think that this also isn't necessary until pretty
> late in the release process. We really need this indication for late
> features coming in.

I am OK with bugfixes being added in before RC0 without having to
rubber-stamp them (as surely the maintainers know best whether the
bug-fix should go in or not).

But after RC0 I believe it makes sense to take a step to figure out the
regression factor (if any).
> 
> Jan
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH] AMD/guest_iommu: Disable guest iommu support

[Suravee Suthikulpanit]

On 10/02/2014 09:02 AM, Andrew Cooper wrote:
> AMD Guest IOMMU support was added to allow correct use of PASID and PRI
> hardware support with an ATS-aware guest driver.
>
> However, support cannot possibly function as guest_iommu_set_base() has no
> callers.  This means that its MMIO region's P2M pages are not set to
> p2m_mmio_dm, preventing any invocation of the MMIO read/write handlers.
>
> c/s fd186384 "x86/HVM: extend LAPIC shortcuts around P2M lookups" introduces a
> path (via hvm_mmio_internal()) where iommu_mmio_handler claims its MMIO range,
> and causes __hvm_copy() to fail with HVMCOPY_bad_gfn_to_mfn.
>
> iommu->mmio_base defaults to 0, with a range of 8 pages, and is unilaterally
> enabled in any HVM guests when the host IOMMU(s) supports any extended
> features.
>
> Unfortunately, HVMLoader's AP boot trampoline executes an `lmsw` instruction
> at linear address 0x100c which unconditionally requires emulation.  The
> instruction fetch in turn fails as __hvm_copy() fails with
> HVMCOPY_bad_gfn_to_mfn.
>
> The result is that multi-vcpu HVM guests do not work on newer AMD hardware, if
> IOMMU support is enabled in the BIOS.
>
> Change the default mmio_base address to ~0ULL.  This prevents
> guest_iommu_mmio_range() from actually claiming any physical range
> whatsoever, which allows the emulation of `lmsw` to succeed.
>
> Reported-by: Roberto Luongo <rluongo@ready.it>
> Suggested-by: Jan Beulich <JBeulich@suse.com>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Tested-by: Roberto Luongo <rluongo@ready.it>
> CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> CC: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
> ---
>   xen/drivers/passthrough/amd/iommu_guest.c |    1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/xen/drivers/passthrough/amd/iommu_guest.c b/xen/drivers/passthrough/amd/iommu_guest.c
> index 5660020..98e7b38 100644
> --- a/xen/drivers/passthrough/amd/iommu_guest.c
> +++ b/xen/drivers/passthrough/amd/iommu_guest.c
> @@ -885,6 +885,7 @@ int guest_iommu_init(struct domain* d)
>       }
>
>       guest_iommu_reg_init(iommu);
> +    iommu->mmio_base = ~0ULL;
>       iommu->domain = d;
>       hd->arch.g_iommu = iommu;
>
>
Thank you, Andrew.

Acked-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>

Jan,

The function guest_iommu_set_base() was added by Wei before my time. I 
checked the log and didn't see the evidence that this code has ever been 
used. Do you remember why he added this code to begin with, and how this 
was planned to be used?

Suravee

Re: [PATCH] AMD/guest_iommu: Disable guest iommu support

[Jan Beulich]

>>> On 06.10.14 at 08:46, <Suravee.Suthikulpanit@amd.com> wrote:
> The function guest_iommu_set_base() was added by Wei before my time. I 
> checked the log and didn't see the evidence that this code has ever been 
> used. Do you remember why he added this code to begin with, and how this 
> was planned to be used?

The plan here clearly was to allow guests to see a virtual IOMMU,
and the setting of the respective base address would then be
a requirement for tool stack and/or virtual firmware to set up the
environment for the guest.

Jan

Re: [PATCH] AMD/guest_iommu: Disable guest iommu support

[Andrew Cooper]

On 06/10/14 08:59, Jan Beulich wrote:
>>>> On 06.10.14 at 08:46, <Suravee.Suthikulpanit@amd.com> wrote:
>> The function guest_iommu_set_base() was added by Wei before my time. I 
>> checked the log and didn't see the evidence that this code has ever been 
>> used. Do you remember why he added this code to begin with, and how this 
>> was planned to be used?
> The plan here clearly was to allow guests to see a virtual IOMMU,
> and the setting of the respective base address would then be
> a requirement for tool stack and/or virtual firmware to set up the
> environment for the guest.
>
> Jan
>

The commit which introduces the virtual IOMMU (c/s 4547137c3) indicates
that a virtual IOMMU is necessary for correct functioning of ATS-capable
guest device drivers using PASID or PRI, in passthrough.

~Andrew