| Message ID | 1427224544-29955-1-git-send-email-ard.biesheuvel@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 24 March 2015 at 23:00, Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote: > On Tue, 24 Mar 2015, Ard Biesheuvel wrote: >> This library may be used with the caches and MMU off, while the >> hypervisor has the caches enabled. So add explicit cache >> maintenance to deal with the potential incoherency. >> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > Given that it is expensive, wouldn't it be better to do it only if > caches are not enabled? > Yes it is very costly, and noticeably slow on the console GUI So as I mentioned, I am trying to avoid doing any cache maintenance at all by deferring everything related to hypercalls and the Xen console to after the MMU and caches are up. However, for some reason, this works for the release build but not for the debug build (which is a lot noisier), and it is quite difficult to diagnose. > >> .../XenConsoleSerialPortLib.c | 29 ++++++++++++++++++++++ >> .../XenConsoleSerialPortLib.inf | 1 + >> 2 files changed, 30 insertions(+) >> >> diff --git a/OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.c b/OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.c >> index 467cb27a30c4..17d5a5a38d30 100644 >> --- a/OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.c >> +++ b/OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.c >> @@ -19,6 +19,7 @@ >> #include <Library/BaseLib.h> >> #include <Library/SerialPortLib.h> >> #include <Library/XenHypercallLib.h> >> +#include <Library/CacheMaintenanceLib.h> >> >> #include <IndustryStandard/Xen/io/console.h> >> #include <IndustryStandard/Xen/hvm/params.h> >> @@ -80,6 +81,13 @@ SerialPortWrite ( >> return 0; >> } >> >> + // >> + // We may be accessing DRAM directly, while the hypervisor has the caches >> + // enabled. This means we have to force a writeback to ensure that we have >> + // the latest data. >> + // >> + WriteBackDataCacheRange (mXenConsoleInterface, sizeof(*mXenConsoleInterface)); >> >> Consumer = mXenConsoleInterface->out_cons; >> Producer = mXenConsoleInterface->out_prod; >> >> @@ -93,6 +101,13 @@ SerialPortWrite ( >> >> mXenConsoleInterface->out_prod = Producer; >> >> + // >> + // Ensure that the data cache is invalidated after we have written our data. >> + // This ensures that the hypervisor will have to fetch the value from DRAM, >> + // which may be where the data is if we are running with the MMU off. >> + // >> + InvalidateDataCacheRange (mXenConsoleInterface, sizeof(*mXenConsoleInterface)); > > To ensure that the lockless protocol keeps working as expected, I think > we need one InvalidateDataCacheRange call before setting out_prod, to > invalidate the data in the ring, and another one after setting out_prod, to > invalidate the out_prod pointer. Otherwise the reader could read an > updated value of out_prod but stale data in the unlikely scenario where > they are on different cachelines and one of them is already invalidated. > > What do you think? > I think you're right. The data needs to be observable by the reader before updating the out_prod pointer, and that is not guaranteed until the invalidate happens. > >> if (Sent > 0) { >> XenHypercallEventChannelOp (EVTCHNOP_send, &mXenConsoleEventChain); >> } >> @@ -124,6 +139,13 @@ SerialPortRead ( >> return 0; >> } >> >> + // >> + // We may be accessing DRAM directly, while the hypervisor has the caches >> + // enabled. This means we have to force a writeback to ensure that we have >> + // the latest data. >> + // >> + WriteBackDataCacheRange (mXenConsoleInterface, sizeof(*mXenConsoleInterface)); >> + >> Consumer = mXenConsoleInterface->in_cons; >> Producer = mXenConsoleInterface->in_prod; >> >> @@ -137,6 +159,13 @@ SerialPortRead ( >> >> mXenConsoleInterface->in_cons = Consumer; >> >> + // >> + // Ensure that the data cache is invalidated after we have written our data. >> + // This ensures that the hypervisor will have to fetch the value from DRAM, >> + // which may be where the data is if we are running with the MMU off. >> + // >> + InvalidateDataCacheRange (mXenConsoleInterface, sizeof(*mXenConsoleInterface)); >> >> XenHypercallEventChannelOp (EVTCHNOP_send, &mXenConsoleEventChain); >> >> return Received; >> diff --git a/OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.inf b/OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.inf >> index 8a7411558fa3..2fd1c654be78 100644 >> --- a/OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.inf >> +++ b/OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.inf >> @@ -29,6 +29,7 @@ >> [LibraryClasses] >> BaseLib >> XenHypercallLib >> + CacheMaintenanceLib >> >> [Packages] >> MdePkg/MdePkg.dec >> -- >> 1.8.3.2 >> ------------------------------------------------------------------------------ Dive into the World of Parallel Programming The Go Parallel Website, sponsored by Intel and developed in partnership with Slashdot Media, is your hub for all things parallel software development, from weekly thought leadership blogs to news, videos, case studies, tutorials and more. Take a look and join the conversation now. http://goparallel.sourceforge.net/
On 25 March 2015 at 09:42, Laszlo Ersek <lersek@redhat.com> wrote: > On 03/24/15 20:15, Ard Biesheuvel wrote: >> This library may be used with the caches and MMU off, while the >> hypervisor has the caches enabled. So add explicit cache >> maintenance to deal with the potential incoherency. >> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> --- >> .../XenConsoleSerialPortLib.c | 29 ++++++++++++++++++++++ >> .../XenConsoleSerialPortLib.inf | 1 + >> 2 files changed, 30 insertions(+) >> >> diff --git a/OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.c b/OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.c >> index 467cb27a30c4..17d5a5a38d30 100644 >> --- a/OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.c >> +++ b/OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.c >> @@ -19,6 +19,7 @@ >> #include <Library/BaseLib.h> >> #include <Library/SerialPortLib.h> >> #include <Library/XenHypercallLib.h> >> +#include <Library/CacheMaintenanceLib.h> >> >> #include <IndustryStandard/Xen/io/console.h> >> #include <IndustryStandard/Xen/hvm/params.h> >> @@ -80,6 +81,13 @@ SerialPortWrite ( >> return 0; >> } >> >> + // >> + // We may be accessing DRAM directly, while the hypervisor has the caches >> + // enabled. This means we have to force a writeback to ensure that we have >> + // the latest data. >> + // >> + WriteBackDataCacheRange (mXenConsoleInterface, sizeof(*mXenConsoleInterface)); >> + >> Consumer = mXenConsoleInterface->out_cons; >> Producer = mXenConsoleInterface->out_prod; >> >> @@ -93,6 +101,13 @@ SerialPortWrite ( >> >> mXenConsoleInterface->out_prod = Producer; >> >> + // >> + // Ensure that the data cache is invalidated after we have written our data. >> + // This ensures that the hypervisor will have to fetch the value from DRAM, >> + // which may be where the data is if we are running with the MMU off. >> + // >> + InvalidateDataCacheRange (mXenConsoleInterface, sizeof(*mXenConsoleInterface)); >> + >> if (Sent > 0) { >> XenHypercallEventChannelOp (EVTCHNOP_send, &mXenConsoleEventChain); >> } >> @@ -124,6 +139,13 @@ SerialPortRead ( >> return 0; >> } >> >> + // >> + // We may be accessing DRAM directly, while the hypervisor has the caches >> + // enabled. This means we have to force a writeback to ensure that we have >> + // the latest data. >> + // >> + WriteBackDataCacheRange (mXenConsoleInterface, sizeof(*mXenConsoleInterface)); >> + >> Consumer = mXenConsoleInterface->in_cons; >> Producer = mXenConsoleInterface->in_prod; >> >> @@ -137,6 +159,13 @@ SerialPortRead ( >> >> mXenConsoleInterface->in_cons = Consumer; >> >> + // >> + // Ensure that the data cache is invalidated after we have written our data. >> + // This ensures that the hypervisor will have to fetch the value from DRAM, >> + // which may be where the data is if we are running with the MMU off. >> + // >> + InvalidateDataCacheRange (mXenConsoleInterface, sizeof(*mXenConsoleInterface)); >> + >> XenHypercallEventChannelOp (EVTCHNOP_send, &mXenConsoleEventChain); >> >> return Received; >> diff --git a/OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.inf b/OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.inf >> index 8a7411558fa3..2fd1c654be78 100644 >> --- a/OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.inf >> +++ b/OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.inf >> @@ -29,6 +29,7 @@ >> [LibraryClasses] >> BaseLib >> XenHypercallLib >> + CacheMaintenanceLib >> >> [Packages] >> MdePkg/MdePkg.dec >> > > Looks good to me; but how about, in addition: > > ================== > diff --git a/OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.c b/OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.c > index 467cb27..a7bf015 100644 > --- a/OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.c > +++ b/OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.c > @@ -31,8 +31,8 @@ > // in general, it is actually fine for the Xen domU (guest) environment that > // this module is intended for, as UEFI always executes from DRAM in that case. > // > -STATIC evtchn_send_t mXenConsoleEventChain; > -STATIC struct xencons_interface *mXenConsoleInterface; > +STATIC evtchn_send_t mXenConsoleEventChain; > +STATIC struct xencons_interface volatile *mXenConsoleInterface; > > RETURN_STATUS > EFIAPI > @@ -155,6 +155,9 @@ SerialPortPoll ( > VOID > ) > { > - return mXenConsoleInterface && > - mXenConsoleInterface->in_cons != mXenConsoleInterface->in_prod; > + if (!mXenConsoleInterface) { > + return FALSE; > + } > + WriteBackDataCacheRange (mXenConsoleInterface, sizeof(*mXenConsoleInterface)); > + return mXenConsoleInterface->in_cons != mXenConsoleInterface->in_prod; > } > ================== > Ah yes, I missed that one. But let me first figure out if we can do away with this completely. If I defer the SerialPortLib init until after the MMU is up, we should not need any of this, but I am seeing lockups when I remove the cache clean before the console write. Of course, any debug I add *anywhere* makes the problem go away :-( ------------------------------------------------------------------------------ Dive into the World of Parallel Programming The Go Parallel Website, sponsored by Intel and developed in partnership with Slashdot Media, is your hub for all things parallel software development, from weekly thought leadership blogs to news, videos, case studies, tutorials and more. Take a look and join the conversation now. http://goparallel.sourceforge.net/
diff --git a/OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.c b/OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.c index 467cb27a30c4..17d5a5a38d30 100644 --- a/OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.c +++ b/OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.c @@ -19,6 +19,7 @@ #include <Library/BaseLib.h> #include <Library/SerialPortLib.h> #include <Library/XenHypercallLib.h> +#include <Library/CacheMaintenanceLib.h> #include <IndustryStandard/Xen/io/console.h> #include <IndustryStandard/Xen/hvm/params.h> @@ -80,6 +81,13 @@ SerialPortWrite ( return 0; } + // + // We may be accessing DRAM directly, while the hypervisor has the caches + // enabled. This means we have to force a writeback to ensure that we have + // the latest data. + // + WriteBackDataCacheRange (mXenConsoleInterface, sizeof(*mXenConsoleInterface)); + Consumer = mXenConsoleInterface->out_cons; Producer = mXenConsoleInterface->out_prod; @@ -93,6 +101,13 @@ SerialPortWrite ( mXenConsoleInterface->out_prod = Producer; + // + // Ensure that the data cache is invalidated after we have written our data. + // This ensures that the hypervisor will have to fetch the value from DRAM, + // which may be where the data is if we are running with the MMU off. + // + InvalidateDataCacheRange (mXenConsoleInterface, sizeof(*mXenConsoleInterface)); + if (Sent > 0) { XenHypercallEventChannelOp (EVTCHNOP_send, &mXenConsoleEventChain); } @@ -124,6 +139,13 @@ SerialPortRead ( return 0; } + // + // We may be accessing DRAM directly, while the hypervisor has the caches + // enabled. This means we have to force a writeback to ensure that we have + // the latest data. + // + WriteBackDataCacheRange (mXenConsoleInterface, sizeof(*mXenConsoleInterface)); + Consumer = mXenConsoleInterface->in_cons; Producer = mXenConsoleInterface->in_prod; @@ -137,6 +159,13 @@ SerialPortRead ( mXenConsoleInterface->in_cons = Consumer; + // + // Ensure that the data cache is invalidated after we have written our data. + // This ensures that the hypervisor will have to fetch the value from DRAM, + // which may be where the data is if we are running with the MMU off. + // + InvalidateDataCacheRange (mXenConsoleInterface, sizeof(*mXenConsoleInterface)); + XenHypercallEventChannelOp (EVTCHNOP_send, &mXenConsoleEventChain); return Received; diff --git a/OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.inf b/OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.inf index 8a7411558fa3..2fd1c654be78 100644 --- a/OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.inf +++ b/OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.inf @@ -29,6 +29,7 @@ [LibraryClasses] BaseLib XenHypercallLib + CacheMaintenanceLib [Packages] MdePkg/MdePkg.dec
This library may be used with the caches and MMU off, while the hypervisor has the caches enabled. So add explicit cache maintenance to deal with the potential incoherency. Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> --- .../XenConsoleSerialPortLib.c | 29 ++++++++++++++++++++++ .../XenConsoleSerialPortLib.inf | 1 + 2 files changed, 30 insertions(+)