[PATCH] VTd/dmar: Tweak how the DMAR table is clobbered

[PATCH] VTd/dmar: Tweak how the DMAR table is clobbered

[Andrew Cooper]

Intead of clobbering DMAR -> XMAR and back, clobber to RMAD instead.  This
means that changing the signature does not alter the checksum, which allows
the clobbering/unclobbering to be peformed atomically and idempotently, which
is an advantage on the kexec path which can reenter acpi_dmar_reinstate().

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Yang Zhang <yang.z.zhang@intel.com>
CC: Kevin Tian <kevin.tian@intel.com>
---
 xen/drivers/passthrough/vtd/dmar.c |   20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/xen/drivers/passthrough/vtd/dmar.c b/xen/drivers/passthrough/vtd/dmar.c
index 1152c3a..18d7903 100644
--- a/xen/drivers/passthrough/vtd/dmar.c
+++ b/xen/drivers/passthrough/vtd/dmar.c
@@ -28,6 +28,7 @@
 #include <xen/xmalloc.h>
 #include <xen/pci.h>
 #include <xen/pci_regs.h>
+#include <asm/atomic.h>
 #include <asm/string.h>
 #include "dmar.h"
 #include "iommu.h"
@@ -838,8 +839,7 @@ static int __init acpi_parse_dmar(struct acpi_table_header *table)
 
 out:
     /* Zap ACPI DMAR signature to prevent dom0 using vt-d HW. */
-    dmar->header.signature[0] = 'X';
-    dmar->header.checksum -= 'X'-'D';
+    acpi_dmar_zap();
     return ret;
 }
 
@@ -867,18 +867,18 @@ int __init acpi_dmar_init(void)
 
 void acpi_dmar_reinstate(void)
 {
-    if ( dmar_table == NULL )
-        return;
-    dmar_table->signature[0] = 'D';
-    dmar_table->checksum += 'X'-'D';
+    uint32_t sig = 0x52414d44; /* "DMAR" */
+
+    if ( dmar_table )
+        write_atomic((uint32_t*)&dmar_table->signature[0], sig);
 }
 
 void acpi_dmar_zap(void)
 {
-    if ( dmar_table == NULL )
-        return;
-    dmar_table->signature[0] = 'X';
-    dmar_table->checksum -= 'X'-'D';
+    uint32_t sig = 0x44414d52; /* "RMAD" - doesn't alter table checksum */
+
+    if ( dmar_table )
+        write_atomic((uint32_t*)&dmar_table->signature[0], sig);
 }
 
 int platform_supports_intremap(void)
-- 
1.7.10.4

Re: [PATCH] VTd/dmar: Tweak how the DMAR table is clobbered

[David Vrabel]

On 08/04/15 20:44, Andrew Cooper wrote:
> Intead of clobbering DMAR -> XMAR and back, clobber to RMAD instead.  This
> means that changing the signature does not alter the checksum, which allows
> the clobbering/unclobbering to be peformed atomically and idempotently, which
> is an advantage on the kexec path which can reenter acpi_dmar_reinstate().

Could RMAD be specified as a real table in the future?  Does the
clobbered name have to start with X to avoid future conflicts?

David

Re: [PATCH] VTd/dmar: Tweak how the DMAR table is clobbered

[Andrew Cooper]

On 09/04/15 09:51, David Vrabel wrote:
> On 08/04/15 20:44, Andrew Cooper wrote:
>> Intead of clobbering DMAR -> XMAR and back, clobber to RMAD instead.  This
>> means that changing the signature does not alter the checksum, which allows
>> the clobbering/unclobbering to be peformed atomically and idempotently, which
>> is an advantage on the kexec path which can reenter acpi_dmar_reinstate().
> Could RMAD be specified as a real table in the future?  Does the
> clobbered name have to start with X to avoid future conflicts?
>
> David

I am not aware of any restrictions imposed by the APCI spec.  Any
clobbered signature is potentially a real table in the future.

This DMAR clobbering was introduced by
83904107a33c9badc34ecdd1f8ca0f9271e5e370 which claims that the dom0 VT-d
driver was capable of playing with the IOMMU(s) while Xen was also using
them.  An alternative approach might be to leave the DMAR table alone
and sprinkle some iomem_deny_access() around to forcibly prevent dom0
from playing.

~Andrew

Re: [PATCH] VTd/dmar: Tweak how the DMAR table is clobbered

[Tian, Kevin]

> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: Thursday, April 09, 2015 3:45 AM
> 
> Intead of clobbering DMAR -> XMAR and back, clobber to RMAD instead.
> This
> means that changing the signature does not alter the checksum, which allows
> the clobbering/unclobbering to be peformed atomically and idempotently,
> which
> is an advantage on the kexec path which can reenter acpi_dmar_reinstate().
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Yang Zhang <yang.z.zhang@intel.com>
> CC: Kevin Tian <kevin.tian@intel.com>

Acked-by: Kevin Tian <kevin.tian@intel.com>

and curious do you observe a real atomic issue in kexec or just catch this
potential issue when reading code? :-)

Thanks
Kevin

> ---
>  xen/drivers/passthrough/vtd/dmar.c |   20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/vtd/dmar.c
> b/xen/drivers/passthrough/vtd/dmar.c
> index 1152c3a..18d7903 100644
> --- a/xen/drivers/passthrough/vtd/dmar.c
> +++ b/xen/drivers/passthrough/vtd/dmar.c
> @@ -28,6 +28,7 @@
>  #include <xen/xmalloc.h>
>  #include <xen/pci.h>
>  #include <xen/pci_regs.h>
> +#include <asm/atomic.h>
>  #include <asm/string.h>
>  #include "dmar.h"
>  #include "iommu.h"
> @@ -838,8 +839,7 @@ static int __init acpi_parse_dmar(struct
> acpi_table_header *table)
> 
>  out:
>      /* Zap ACPI DMAR signature to prevent dom0 using vt-d HW. */
> -    dmar->header.signature[0] = 'X';
> -    dmar->header.checksum -= 'X'-'D';
> +    acpi_dmar_zap();
>      return ret;
>  }
> 
> @@ -867,18 +867,18 @@ int __init acpi_dmar_init(void)
> 
>  void acpi_dmar_reinstate(void)
>  {
> -    if ( dmar_table == NULL )
> -        return;
> -    dmar_table->signature[0] = 'D';
> -    dmar_table->checksum += 'X'-'D';
> +    uint32_t sig = 0x52414d44; /* "DMAR" */
> +
> +    if ( dmar_table )
> +        write_atomic((uint32_t*)&dmar_table->signature[0], sig);
>  }
> 
>  void acpi_dmar_zap(void)
>  {
> -    if ( dmar_table == NULL )
> -        return;
> -    dmar_table->signature[0] = 'X';
> -    dmar_table->checksum -= 'X'-'D';
> +    uint32_t sig = 0x44414d52; /* "RMAD" - doesn't alter table checksum */
> +
> +    if ( dmar_table )
> +        write_atomic((uint32_t*)&dmar_table->signature[0], sig);
>  }
> 
>  int platform_supports_intremap(void)
> --
> 1.7.10.4

Re: [PATCH] VTd/dmar: Tweak how the DMAR table is clobbered

[Andrew Cooper]

On 10/04/15 02:23, Tian, Kevin wrote:
>> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
>> Sent: Thursday, April 09, 2015 3:45 AM
>>
>> Intead of clobbering DMAR -> XMAR and back, clobber to RMAD instead.
>> This
>> means that changing the signature does not alter the checksum, which allows
>> the clobbering/unclobbering to be peformed atomically and idempotently,
>> which
>> is an advantage on the kexec path which can reenter acpi_dmar_reinstate().
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> CC: Yang Zhang <yang.z.zhang@intel.com>
>> CC: Kevin Tian <kevin.tian@intel.com>
> Acked-by: Kevin Tian <kevin.tian@intel.com>
>
> and curious do you observe a real atomic issue in kexec or just catch this
> potential issue when reading code? :-)

I have run over it once in the past, but mainly it is one small thing on
a very long list of tweaks to make the crash path for reliable.

As indicated in the other thread, I think the best direction moving
forwards is to see about positively preventing dom0 having access,
rather than simply hiding the table, but that is a job for another time.

~Andrew

Re: [PATCH] VTd/dmar: Tweak how the DMAR table is clobbered

[Jan Beulich]

>>> On 10.04.15 at 11:08, <andrew.cooper3@citrix.com> wrote:
> On 10/04/15 02:23, Tian, Kevin wrote:
>>> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
>>> Sent: Thursday, April 09, 2015 3:45 AM
>>>
>>> Intead of clobbering DMAR -> XMAR and back, clobber to RMAD instead.
>>> This
>>> means that changing the signature does not alter the checksum, which allows
>>> the clobbering/unclobbering to be peformed atomically and idempotently,
>>> which
>>> is an advantage on the kexec path which can reenter acpi_dmar_reinstate().
>>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> CC: Yang Zhang <yang.z.zhang@intel.com>
>>> CC: Kevin Tian <kevin.tian@intel.com>
>> Acked-by: Kevin Tian <kevin.tian@intel.com>
>>
>> and curious do you observe a real atomic issue in kexec or just catch this
>> potential issue when reading code? :-)
> 
> I have run over it once in the past, but mainly it is one small thing on
> a very long list of tweaks to make the crash path for reliable.
> 
> As indicated in the other thread, I think the best direction moving
> forwards is to see about positively preventing dom0 having access,
> rather than simply hiding the table, but that is a job for another time.

And possibly not doable, as this might crash Dom0. What made me
wonder for a very long time though is why similar clobbering isn't
needed for AMD.

In any event, David's point of the now chosen signature perhaps
posing a higher risk of colliding with a real table is an issue that
shouldn't have been discarded before committing. Unless Kevin or
Yang object, I'd therefore suggest reverting the change. Once
we determined why VT-d needs what AMD Vi doesn't need, and
once we settled on the risk of name collision (perhaps using an
underscore prefixed name would further reduce this risk), we could
then do this another way (zap the table from XSDT/RSDT instead?),
or leave it as it was without the change.

(Apart from the above I also don't really see why RMAD was
chosen - this doesn't really resemble anything similar to DMAR
except for using the same letters. If at least it had been the
properly reversed string ...)

Jan

Re: [PATCH] VTd/dmar: Tweak how the DMAR table is clobbered

[Andrew Cooper]

On 14/04/15 08:50, Jan Beulich wrote:
>>>> On 10.04.15 at 11:08, <andrew.cooper3@citrix.com> wrote:
>> On 10/04/15 02:23, Tian, Kevin wrote:
>>>> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
>>>> Sent: Thursday, April 09, 2015 3:45 AM
>>>>
>>>> Intead of clobbering DMAR -> XMAR and back, clobber to RMAD instead.
>>>> This
>>>> means that changing the signature does not alter the checksum, which allows
>>>> the clobbering/unclobbering to be peformed atomically and idempotently,
>>>> which
>>>> is an advantage on the kexec path which can reenter acpi_dmar_reinstate().
>>>>
>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>> CC: Yang Zhang <yang.z.zhang@intel.com>
>>>> CC: Kevin Tian <kevin.tian@intel.com>
>>> Acked-by: Kevin Tian <kevin.tian@intel.com>
>>>
>>> and curious do you observe a real atomic issue in kexec or just catch this
>>> potential issue when reading code? :-)
>> I have run over it once in the past, but mainly it is one small thing on
>> a very long list of tweaks to make the crash path for reliable.
>>
>> As indicated in the other thread, I think the best direction moving
>> forwards is to see about positively preventing dom0 having access,
>> rather than simply hiding the table, but that is a job for another time.
> And possibly not doable, as this might crash Dom0. What made me
> wonder for a very long time though is why similar clobbering isn't
> needed for AMD.

Any dom0 driver will be capable of not crashing if it can't get to
certain pages, or it wouldn't last for any meaningful time on a system
with buggy firmware.  It is the very fact that this hack is only used on
Intel which leads me to suspect that it is the wrong thing to be doing
overall.

>
> In any event, David's point of the now chosen signature perhaps
> posing a higher risk of colliding with a real table is an issue that
> shouldn't have been discarded before committing.

I don't believe the new name is plausibly at a higher risk of colliding.

> Unless Kevin or
> Yang object, I'd therefore suggest reverting the change. Once
> we determined why VT-d needs what AMD Vi doesn't need, and
> once we settled on the risk of name collision (perhaps using an
> underscore prefixed name would further reduce this risk), we could
> then do this another way (zap the table from XSDT/RSDT instead?),
> or leave it as it was without the change.

It is my hope that this can be resolved in the longterm without any
modification to the acpi tables.  Currently, it is not possible to dump
the ACPI tables from dom0 without knowing how to hexedit the XMAR table
back into life.  This is an impediment to debugging.

However, I still believe that the current change is a positive
improvement over what happened previously.

>
> (Apart from the above I also don't really see why RMAD was
> chosen - this doesn't really resemble anything similar to DMAR
> except for using the same letters. If at least it had been the
> properly reversed string ...)

A fully reversed string is RAMD which I felt was slightly more likely to
collide, but I am not too fussed on exactly which string is chosen, so
long as it has the same u8 checksum as "DMAR".

~Andrew

Re: [PATCH] VTd/dmar: Tweak how the DMAR table is clobbered

[Tian, Kevin]

> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: Tuesday, April 14, 2015 5:09 PM
> 
> On 14/04/15 08:50, Jan Beulich wrote:
> >>>> On 10.04.15 at 11:08, <andrew.cooper3@citrix.com> wrote:
> >> On 10/04/15 02:23, Tian, Kevin wrote:
> >>>> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> >>>> Sent: Thursday, April 09, 2015 3:45 AM
> >>>>
> >>>> Intead of clobbering DMAR -> XMAR and back, clobber to RMAD instead.
> >>>> This
> >>>> means that changing the signature does not alter the checksum, which
> allows
> >>>> the clobbering/unclobbering to be peformed atomically and
> idempotently,
> >>>> which
> >>>> is an advantage on the kexec path which can reenter
> acpi_dmar_reinstate().
> >>>>
> >>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> >>>> CC: Yang Zhang <yang.z.zhang@intel.com>
> >>>> CC: Kevin Tian <kevin.tian@intel.com>
> >>> Acked-by: Kevin Tian <kevin.tian@intel.com>
> >>>
> >>> and curious do you observe a real atomic issue in kexec or just catch this
> >>> potential issue when reading code? :-)
> >> I have run over it once in the past, but mainly it is one small thing on
> >> a very long list of tweaks to make the crash path for reliable.
> >>
> >> As indicated in the other thread, I think the best direction moving
> >> forwards is to see about positively preventing dom0 having access,
> >> rather than simply hiding the table, but that is a job for another time.
> > And possibly not doable, as this might crash Dom0. What made me
> > wonder for a very long time though is why similar clobbering isn't
> > needed for AMD.
> 
> Any dom0 driver will be capable of not crashing if it can't get to
> certain pages, or it wouldn't last for any meaningful time on a system
> with buggy firmware.  It is the very fact that this hack is only used on
> Intel which leads me to suspect that it is the wrong thing to be doing
> overall.
> 
> >
> > In any event, David's point of the now chosen signature perhaps
> > posing a higher risk of colliding with a real table is an issue that
> > shouldn't have been discarded before committing.
> 
> I don't believe the new name is plausibly at a higher risk of colliding.
> 
> > Unless Kevin or
> > Yang object, I'd therefore suggest reverting the change. Once
> > we determined why VT-d needs what AMD Vi doesn't need, and
> > once we settled on the risk of name collision (perhaps using an
> > underscore prefixed name would further reduce this risk), we could
> > then do this another way (zap the table from XSDT/RSDT instead?),
> > or leave it as it was without the change.
> 
> It is my hope that this can be resolved in the longterm without any
> modification to the acpi tables.  Currently, it is not possible to dump
> the ACPI tables from dom0 without knowing how to hexedit the XMAR table
> back into life.  This is an impediment to debugging.
> 
> However, I still believe that the current change is a positive
> improvement over what happened previously.
> 

I'm OK with this patch itself as it does improve current situation a bit,
though we do need to figure out the mysterious reason why AMD doesn't
require same hack. My gut-feeling is that hypervisor has to do something
so an unmodified dom0 iommu driver is not activated to use iommu, unless
the dom0 iommu driver has some awareness to give up proactively. 

Add more relevant people to see any input...

Thanks
Kevin

Re: [PATCH] VTd/dmar: Tweak how the DMAR table is clobbered

[Jan Beulich]

>>> On 16.04.15 at 18:44, <kevin.tian@intel.com> wrote:
>>  From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
>> Sent: Tuesday, April 14, 2015 5:09 PM
>> On 14/04/15 08:50, Jan Beulich wrote:
>> > Unless Kevin or
>> > Yang object, I'd therefore suggest reverting the change. Once
>> > we determined why VT-d needs what AMD Vi doesn't need, and
>> > once we settled on the risk of name collision (perhaps using an
>> > underscore prefixed name would further reduce this risk), we could
>> > then do this another way (zap the table from XSDT/RSDT instead?),
>> > or leave it as it was without the change.
>> 
>> It is my hope that this can be resolved in the longterm without any
>> modification to the acpi tables.  Currently, it is not possible to dump
>> the ACPI tables from dom0 without knowing how to hexedit the XMAR table
>> back into life.  This is an impediment to debugging.
>> 
>> However, I still believe that the current change is a positive
>> improvement over what happened previously.
> 
> I'm OK with this patch itself as it does improve current situation a bit,
> though we do need to figure out the mysterious reason why AMD doesn't
> require same hack. My gut-feeling is that hypervisor has to do something
> so an unmodified dom0 iommu driver is not activated to use iommu, unless
> the dom0 iommu driver has some awareness to give up proactively. 

There should be no such thing like an unmodified Dom0 IOMMU driver,
as Dom0 can't be HVM (and if it was HVM, it would necessarily have to
see other than the host's ACPI tables). My impression is that this was
solely added as a workaround by someone too lazy to adjust the Dom0
IOMMU driver back when the functionality was added.

Jan