[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 1/2] x86/shadow: sanitize iommu_snoop usage
On 1/13/23 16:22, Jan Beulich wrote:
On 13.01.2023 14:53, Xenia Ragiadakou wrote:
On 1/13/23 15:24, Jan Beulich wrote:
On 13.01.2023 14:07, Xenia Ragiadakou wrote:
On 1/13/23 14:12, Jan Beulich wrote:
On 13.01.2023 12:55, Xenia Ragiadakou wrote:
On 1/13/23 11:53, Jan Beulich wrote:
On 13.01.2023 10:34, Xenia Ragiadakou wrote:
On 1/13/23 10:47, Jan Beulich wrote:
--- a/xen/drivers/passthrough/x86/iommu.c
+++ b/xen/drivers/passthrough/x86/iommu.c
@@ -56,6 +56,13 @@ void __init acpi_iommu_init(void)
if ( !acpi_disabled )
{
ret = acpi_dmar_init();
+
+#ifndef iommu_snoop
+ /* A command line override for snoop control affects VT-d only. */
+ if ( ret )
+ iommu_snoop = true;
+#endif
+
Why here iommu_snoop is forced when iommu is not enabled?
This change is confusing because later on, in iommu_setup, iommu_snoop
will be set to false in the case of !iommu_enabled.
Counter question: Why is it being set to false there? I see no reason at
all. On the same basis as here, I'd actually expect it to be set back to
true in such a case.Which, however, would be a benign change now that
all uses of the variable are properly qualified. Which in turn is why I
thought I'd leave that other place alone.
I think I got confused... AFAIU with disabled iommu snooping cannot be
enforced i.e iommu_snoop=true translates to snooping is enforced by the
iommu (that's why we need to check that the iommu is enabled for the
guest). So if the iommu is disabled how can iommu_snoop be true?
For a non-existent (or disabled) IOMMU the value of this boolean simply
is irrelevant. Or to put it in other words, when there's no active
IOMMU, it doesn't matter whether it would actually snoop.
The variable iommu_snoop is initialized to true.
Also, the above change does not prevent it from being overwritten
through the cmdline iommu param in iommu_setup().
Command line parsing happens earlier (and in parse_iommu_param(), not in
iommu_setup()). iommu_setup() can further overwrite it on its error path,
but as said that's benign then.
You are right. I misunderstood.
I m afraid I still cannot understand why the change above is needed.
When using an AMD IOMMU, with how things work right now the variable ought
to always be true (hence why I've suggested that when !INTEL_IOMMU, this
simply become a #define to true). See also Andrew's comments here and/or
on your patch.
Ok I see, so this change is specific to AMD iommu and when iommu_snoop
becomes a #define, this change won't be needed anymore, right?
Well the (source) code change will still be needed; it'll simply be
compiled out for the case where iommu_snoop is a #define (which it
looks like we agree it will be when !INTEL_IOMMU).
Yes. Actually, I was thinking to move the setup of iommu_snoop out of
the X86 #ifdef and to make it depend only on INTEL_IOMMU since for !X86
only matters to be defined.
--
Xenia
|
