Re: [Xen-devel] [PATCH] VTd/dmar: Tweak how the DMAR table is clobbered
>>> On 16.04.15 at 18:44, wrote: >> From: Andrew Cooper [mailto:andrew.coop...@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 ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] VTd/dmar: Tweak how the DMAR table is clobbered
> From: Andrew Cooper [mailto:andrew.coop...@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, wrote: > >> On 10/04/15 02:23, Tian, Kevin wrote: > From: Andrew Cooper [mailto:andrew.coop...@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 > CC: Yang Zhang > CC: Kevin Tian > >>> Acked-by: Kevin Tian > >>> > >>> 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 ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] VTd/dmar: Tweak how the DMAR table is clobbered
On 14/04/15 08:50, Jan Beulich wrote: On 10.04.15 at 11:08, wrote: >> On 10/04/15 02:23, Tian, Kevin wrote: From: Andrew Cooper [mailto:andrew.coop...@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 CC: Yang Zhang CC: Kevin Tian >>> Acked-by: Kevin Tian >>> >>> 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 ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] VTd/dmar: Tweak how the DMAR table is clobbered
>>> On 10.04.15 at 11:08, wrote: > On 10/04/15 02:23, Tian, Kevin wrote: >>> From: Andrew Cooper [mailto:andrew.coop...@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 >>> CC: Yang Zhang >>> CC: Kevin Tian >> Acked-by: Kevin Tian >> >> 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 ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] VTd/dmar: Tweak how the DMAR table is clobbered
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 ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] VTd/dmar: Tweak how the DMAR table is clobbered
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 ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
[Xen-devel] [PATCH] VTd/dmar: Tweak how the DMAR table is clobbered
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
CC: Yang Zhang
CC: Kevin Tian
---
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
#include
#include
+#include
#include
#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
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
