On 24 March 2015 at 23:00, Stefano Stabellini
<[email protected]> 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 <[email protected]>
>
> 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/
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/edk2-devel
