Re: [Qemu-devel] [Qemu-ppc] [PATCH] cuda: decrease time delay before raising VIA SR interrupt

[Mark Cave-Ayland]

On 13/02/2019 00:21, David Gibson wrote:

> On Tue, Feb 12, 2019 at 08:01:22PM +0000, Mark Cave-Ayland wrote:
>> On 12/02/2019 18:21, Philippe Mathieu-Daudé wrote:
>>
>>> On 2/12/19 6:50 PM, Mark Cave-Ayland wrote:
>>>> On 12/02/2019 17:21, Philippe Mathieu-Daudé wrote:
>>>>
>>>>>>> If this delay is to prevent a bug which only happens in MacOS then 
>>>>>>> that's the hack
>>>>>>> not the normal code path to run without the delay that you've just 
>>>>>>> removed. So maybe
>>>>>>> this should be kept if possible to avoid unecessary delays for other 
>>>>>>> guests.
>>>>>>> (Although if this only affects mac99,via=cuda but not mac99,via=pmu 
>>>>>>> then I don't care
>>>>>>> much as long as pmu works.)
>>>>>>
>>>>>> Well the reality is that the detection above doesn't actually seem to 
>>>>>> work anyway -
>>>>>> at least a quick boot test with Linux, MacOS X and MacOS 9 with a 
>>>>>> printf() added into
>>>>>> the if() shows nothing firing once the kernel takes over. So the slow 
>>>>>> path with the
>>>>>> delay included was always being taken within the OS anyway.
>>>>>>
>>>>>> And indeed, the code doesn't affect pmu so you won't see any difference 
>>>>>> there.
>>>>>>
>>>>>>>> As a plus it also prevents a guest OS from accidentally triggering the 
>>>>>>>> hack whilst
>>>>>>>> programming the VIA port.
>>>>>>>
>>>>>>> That may be a problem though. What's the issue exactly? Why is the 
>>>>>>> delay needed in
>>>>>>> the first place?
>>>>>>
>>>>>> It's some kind of racy polling with OS 9 (I wasn't involved in the 
>>>>>> technical details,
>>>>>> sorry) which causes OS 9 to hang on boot if the delay isn't present. And 
>>>>>> even better
>>>>>> the slow path that was previously always being taken has now been 
>>>>>> reduced from 300us
>>>>>> to 30us so whichever way you look at it, having this patch applied is a 
>>>>>> win.
>>>>>
>>>>> Can you write a paragraph about this, that David can amend to your
>>>>> patch? That would stop worrying me about looking at this patch in
>>>>> various months...
>>>>
>>>> Hmmmm well the existing description already describes the interrupt race 
>>>> in OS 9 so I
>>>> guess the only part missing is the bit about the fast path. How about the 
>>>> revised
>>>> text below for the patch description?
>>>>
>>>>
>>>>     cuda: decrease time delay before raising VIA SR interrupt and remove 
>>>> fast path
>>>>
>>>>     In order to handle a race condition in the MacOS 9 CUDA driver, a 
>>>> delay was
>>>>     introduced when raising the VIA SR interrupt inspired by similar code 
>>>> in
>>>>     MacOnLinux.
>>>>
>>>>     During original testing of the MacOS 9 patches it was found that the 
>>>> 30us
>>>>     delay used in MacOnLinux did not work reliably within QEMU, and a 
>>>> value of
>>>>     300us was required to function correctly.
>>>>
>>>>     Recent experiments have shown two things: firstly when booting Linux, 
>>>> MacOS
>>>>     9 and MacOS X the fast path which bypasses the delay is never 
>>>> triggered once the
>>>>     OS kernel is loaded making it effectively useless. Rather than leave 
>>>> this code
>>>>     in place where a guest could potentially enable it by accident and 
>>>> break itself,
>>>>     we might as well just remove it.
>>>>
>>>>     Secondly the previous reliability issues are no longer present, and 
>>>> this value
>>>>     can be reduced down to 20us with no apparent ill effects. This has the 
>>>> benefit of
>>>>     considerably improving the responsiveness of the ADB keyboard and 
>>>> mouse within
>>>>     the guest.
>>>>
>>>>     Signed-off-by: Mark Cave-Ayland <address@hidden>
>>>>
>>>
>>> Thanks!
>>>
>>> Phil.
>>
>> No worries. David, are you able to update the commit message in your 
>> ppc-for-4.0
>> branch accordingly?
> 
> Done.

Great, thanks!


ATB,

Mark.