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

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

[Mark Cave-Ayland]

In order to handle a race condition in MacOS 9, 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 that the previous reliability issues are no
longer present, and this value can be reduced down to 20us with no apparent
ill effects in my local tests. This has the benefit of considerably improving
the responsiveness of the ADB keyboard and mouse with the guest.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/misc/macio/cuda.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/hw/misc/macio/cuda.c b/hw/misc/macio/cuda.c
index c4f7a2f39b..3febacdd1e 100644
--- a/hw/misc/macio/cuda.c
+++ b/hw/misc/macio/cuda.c
@@ -97,17 +97,8 @@ static void cuda_set_sr_int(void *opaque)
 
 static void cuda_delay_set_sr_int(CUDAState *s)
 {
-    MOS6522CUDAState *mcs = &s->mos6522_cuda;
-    MOS6522State *ms = MOS6522(mcs);
-    MOS6522DeviceClass *mdc = MOS6522_DEVICE_GET_CLASS(ms);
     int64_t expire;
 
-    if (ms->dirb == 0xff || s->sr_delay_ns == 0) {
-        /* Disabled or not in Mac OS, fire the IRQ directly */
-        mdc->set_sr_int(ms);
-        return;
-    }
-
     trace_cuda_delay_set_sr_int();
 
     expire = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + s->sr_delay_ns;
@@ -542,7 +533,7 @@ static void cuda_realize(DeviceState *dev, Error **errp)
     s->tick_offset = (uint32_t)mktimegm(&tm) + RTC_OFFSET;
 
     s->sr_delay_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, cuda_set_sr_int, s);
-    s->sr_delay_ns = 300 * SCALE_US;
+    s->sr_delay_ns = 20 * SCALE_US;
 
     s->adb_poll_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, cuda_adb_poll, s);
     s->adb_poll_mask = 0xffff;
-- 
2.11.0

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2348 bytes --]

On Sun, Feb 10, 2019 at 05:44:21PM +0000, Mark Cave-Ayland wrote:
> In order to handle a race condition in MacOS 9, 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 that the previous reliability issues are no
> longer present, and this value can be reduced down to 20us with no apparent
> ill effects in my local tests. This has the benefit of considerably improving
> the responsiveness of the ADB keyboard and mouse with the guest.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Applied to ppc-for-4.0, thanks.

> ---
>  hw/misc/macio/cuda.c | 11 +----------
>  1 file changed, 1 insertion(+), 10 deletions(-)
> 
> diff --git a/hw/misc/macio/cuda.c b/hw/misc/macio/cuda.c
> index c4f7a2f39b..3febacdd1e 100644
> --- a/hw/misc/macio/cuda.c
> +++ b/hw/misc/macio/cuda.c
> @@ -97,17 +97,8 @@ static void cuda_set_sr_int(void *opaque)
>  
>  static void cuda_delay_set_sr_int(CUDAState *s)
>  {
> -    MOS6522CUDAState *mcs = &s->mos6522_cuda;
> -    MOS6522State *ms = MOS6522(mcs);
> -    MOS6522DeviceClass *mdc = MOS6522_DEVICE_GET_CLASS(ms);
>      int64_t expire;
>  
> -    if (ms->dirb == 0xff || s->sr_delay_ns == 0) {
> -        /* Disabled or not in Mac OS, fire the IRQ directly */
> -        mdc->set_sr_int(ms);
> -        return;
> -    }
> -
>      trace_cuda_delay_set_sr_int();
>  
>      expire = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + s->sr_delay_ns;
> @@ -542,7 +533,7 @@ static void cuda_realize(DeviceState *dev, Error **errp)
>      s->tick_offset = (uint32_t)mktimegm(&tm) + RTC_OFFSET;
>  
>      s->sr_delay_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, cuda_set_sr_int, s);
> -    s->sr_delay_ns = 300 * SCALE_US;
> +    s->sr_delay_ns = 20 * SCALE_US;
>  
>      s->adb_poll_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, cuda_adb_poll, s);
>      s->adb_poll_mask = 0xffff;

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Philippe Mathieu-Daudé]

Hi Mark,

On 2/10/19 6:44 PM, Mark Cave-Ayland wrote:
> In order to handle a race condition in MacOS 9, 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 that the previous reliability issues are no
> longer present, and this value can be reduced down to 20us with no apparent
> ill effects in my local tests. This has the benefit of considerably improving
> the responsiveness of the ADB keyboard and mouse with the guest.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/misc/macio/cuda.c | 11 +----------
>  1 file changed, 1 insertion(+), 10 deletions(-)
> 
> diff --git a/hw/misc/macio/cuda.c b/hw/misc/macio/cuda.c
> index c4f7a2f39b..3febacdd1e 100644
> --- a/hw/misc/macio/cuda.c
> +++ b/hw/misc/macio/cuda.c
> @@ -97,17 +97,8 @@ static void cuda_set_sr_int(void *opaque)
>  
>  static void cuda_delay_set_sr_int(CUDAState *s)
>  {
> -    MOS6522CUDAState *mcs = &s->mos6522_cuda;
> -    MOS6522State *ms = MOS6522(mcs);
> -    MOS6522DeviceClass *mdc = MOS6522_DEVICE_GET_CLASS(ms);
>      int64_t expire;
>  
> -    if (ms->dirb == 0xff || s->sr_delay_ns == 0) {
> -        /* Disabled or not in Mac OS, fire the IRQ directly */
> -        mdc->set_sr_int(ms);
> -        return;
> -    }

The change of sr_delay_ns below is well explained, but I don't
understand why you remove the previous if().

> -
>      trace_cuda_delay_set_sr_int();
>  
>      expire = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + s->sr_delay_ns;
> @@ -542,7 +533,7 @@ static void cuda_realize(DeviceState *dev, Error **errp)
>      s->tick_offset = (uint32_t)mktimegm(&tm) + RTC_OFFSET;
>  
>      s->sr_delay_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, cuda_set_sr_int, s);
> -    s->sr_delay_ns = 300 * SCALE_US;
> +    s->sr_delay_ns = 20 * SCALE_US;
>  
>      s->adb_poll_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, cuda_adb_poll, s);
>      s->adb_poll_mask = 0xffff;
> 

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

[Mark Cave-Ayland]

On 11/02/2019 23:35, Philippe Mathieu-Daudé wrote:

> Hi Mark,
> 
> On 2/10/19 6:44 PM, Mark Cave-Ayland wrote:
>> In order to handle a race condition in MacOS 9, 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 that the previous reliability issues are no
>> longer present, and this value can be reduced down to 20us with no apparent
>> ill effects in my local tests. This has the benefit of considerably improving
>> the responsiveness of the ADB keyboard and mouse with the guest.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>  hw/misc/macio/cuda.c | 11 +----------
>>  1 file changed, 1 insertion(+), 10 deletions(-)
>>
>> diff --git a/hw/misc/macio/cuda.c b/hw/misc/macio/cuda.c
>> index c4f7a2f39b..3febacdd1e 100644
>> --- a/hw/misc/macio/cuda.c
>> +++ b/hw/misc/macio/cuda.c
>> @@ -97,17 +97,8 @@ static void cuda_set_sr_int(void *opaque)
>>  
>>  static void cuda_delay_set_sr_int(CUDAState *s)
>>  {
>> -    MOS6522CUDAState *mcs = &s->mos6522_cuda;
>> -    MOS6522State *ms = MOS6522(mcs);
>> -    MOS6522DeviceClass *mdc = MOS6522_DEVICE_GET_CLASS(ms);
>>      int64_t expire;
>>  
>> -    if (ms->dirb == 0xff || s->sr_delay_ns == 0) {
>> -        /* Disabled or not in Mac OS, fire the IRQ directly */
>> -        mdc->set_sr_int(ms);
>> -        return;
>> -    }
> 
> The change of sr_delay_ns below is well explained, but I don't
> understand why you remove the previous if().

IIRC it was a hack by Alex to try and restrict the delay on the interrupt just to
MacOS instead of Linux, but with the reduced value it doesn't really matter any more.
As a plus it also prevents a guest OS from accidentally triggering the hack whilst
programming the VIA port.


ATB,

Mark.

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

[BALATON Zoltan]

On Tue, 12 Feb 2019, Mark Cave-Ayland wrote:
> On 11/02/2019 23:35, Philippe Mathieu-Daudé wrote:
>
>> Hi Mark,
>>
>> On 2/10/19 6:44 PM, Mark Cave-Ayland wrote:
>>> In order to handle a race condition in MacOS 9, 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 that the previous reliability issues are no
>>> longer present, and this value can be reduced down to 20us with no apparent
>>> ill effects in my local tests. This has the benefit of considerably improving
>>> the responsiveness of the ADB keyboard and mouse with the guest.
>>>
>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>> ---
>>>  hw/misc/macio/cuda.c | 11 +----------
>>>  1 file changed, 1 insertion(+), 10 deletions(-)
>>>
>>> diff --git a/hw/misc/macio/cuda.c b/hw/misc/macio/cuda.c
>>> index c4f7a2f39b..3febacdd1e 100644
>>> --- a/hw/misc/macio/cuda.c
>>> +++ b/hw/misc/macio/cuda.c
>>> @@ -97,17 +97,8 @@ static void cuda_set_sr_int(void *opaque)
>>>
>>>  static void cuda_delay_set_sr_int(CUDAState *s)
>>>  {
>>> -    MOS6522CUDAState *mcs = &s->mos6522_cuda;
>>> -    MOS6522State *ms = MOS6522(mcs);
>>> -    MOS6522DeviceClass *mdc = MOS6522_DEVICE_GET_CLASS(ms);
>>>      int64_t expire;
>>>
>>> -    if (ms->dirb == 0xff || s->sr_delay_ns == 0) {
>>> -        /* Disabled or not in Mac OS, fire the IRQ directly */
>>> -        mdc->set_sr_int(ms);
>>> -        return;
>>> -    }
>>
>> The change of sr_delay_ns below is well explained, but I don't
>> understand why you remove the previous if().
>
> IIRC it was a hack by Alex to try and restrict the delay on the interrupt just to
> MacOS instead of Linux, but with the reduced value it doesn't really matter any more.

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.)

> 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?

Regards,
BALATON Zoltan

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

[Mark Cave-Ayland]

On 12/02/2019 11:03, BALATON Zoltan wrote:

> On Tue, 12 Feb 2019, Mark Cave-Ayland wrote:
>> On 11/02/2019 23:35, Philippe Mathieu-Daudé wrote:
>>
>>> Hi Mark,
>>>
>>> On 2/10/19 6:44 PM, Mark Cave-Ayland wrote:
>>>> In order to handle a race condition in MacOS 9, 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 that the previous reliability issues are no
>>>> longer present, and this value can be reduced down to 20us with no apparent
>>>> ill effects in my local tests. This has the benefit of considerably improving
>>>> the responsiveness of the ADB keyboard and mouse with the guest.
>>>>
>>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>>> ---
>>>>  hw/misc/macio/cuda.c | 11 +----------
>>>>  1 file changed, 1 insertion(+), 10 deletions(-)
>>>>
>>>> diff --git a/hw/misc/macio/cuda.c b/hw/misc/macio/cuda.c
>>>> index c4f7a2f39b..3febacdd1e 100644
>>>> --- a/hw/misc/macio/cuda.c
>>>> +++ b/hw/misc/macio/cuda.c
>>>> @@ -97,17 +97,8 @@ static void cuda_set_sr_int(void *opaque)
>>>>
>>>>  static void cuda_delay_set_sr_int(CUDAState *s)
>>>>  {
>>>> -    MOS6522CUDAState *mcs = &s->mos6522_cuda;
>>>> -    MOS6522State *ms = MOS6522(mcs);
>>>> -    MOS6522DeviceClass *mdc = MOS6522_DEVICE_GET_CLASS(ms);
>>>>      int64_t expire;
>>>>
>>>> -    if (ms->dirb == 0xff || s->sr_delay_ns == 0) {
>>>> -        /* Disabled or not in Mac OS, fire the IRQ directly */
>>>> -        mdc->set_sr_int(ms);
>>>> -        return;
>>>> -    }
>>>
>>> The change of sr_delay_ns below is well explained, but I don't
>>> understand why you remove the previous if().
>>
>> IIRC it was a hack by Alex to try and restrict the delay on the interrupt just to
>> MacOS instead of Linux, but with the reduced value it doesn't really matter any more.
> 
> 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.


ATB,

Mark.

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

[Philippe Mathieu-Daudé]

On 2/12/19 5:51 PM, Mark Cave-Ayland wrote:
> On 12/02/2019 11:03, BALATON Zoltan wrote:
> 
>> On Tue, 12 Feb 2019, Mark Cave-Ayland wrote:
>>> On 11/02/2019 23:35, Philippe Mathieu-Daudé wrote:
>>>
>>>> Hi Mark,
>>>>
>>>> On 2/10/19 6:44 PM, Mark Cave-Ayland wrote:
>>>>> In order to handle a race condition in MacOS 9, 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 that the previous reliability issues are no
>>>>> longer present, and this value can be reduced down to 20us with no apparent
>>>>> ill effects in my local tests. This has the benefit of considerably improving
>>>>> the responsiveness of the ADB keyboard and mouse with the guest.
>>>>>
>>>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>>>> ---
>>>>>  hw/misc/macio/cuda.c | 11 +----------
>>>>>  1 file changed, 1 insertion(+), 10 deletions(-)
>>>>>
>>>>> diff --git a/hw/misc/macio/cuda.c b/hw/misc/macio/cuda.c
>>>>> index c4f7a2f39b..3febacdd1e 100644
>>>>> --- a/hw/misc/macio/cuda.c
>>>>> +++ b/hw/misc/macio/cuda.c
>>>>> @@ -97,17 +97,8 @@ static void cuda_set_sr_int(void *opaque)
>>>>>
>>>>>  static void cuda_delay_set_sr_int(CUDAState *s)
>>>>>  {
>>>>> -    MOS6522CUDAState *mcs = &s->mos6522_cuda;
>>>>> -    MOS6522State *ms = MOS6522(mcs);
>>>>> -    MOS6522DeviceClass *mdc = MOS6522_DEVICE_GET_CLASS(ms);
>>>>>      int64_t expire;
>>>>>
>>>>> -    if (ms->dirb == 0xff || s->sr_delay_ns == 0) {
>>>>> -        /* Disabled or not in Mac OS, fire the IRQ directly */
>>>>> -        mdc->set_sr_int(ms);
>>>>> -        return;
>>>>> -    }
>>>>
>>>> The change of sr_delay_ns below is well explained, but I don't
>>>> understand why you remove the previous if().
>>>
>>> IIRC it was a hack by Alex to try and restrict the delay on the interrupt just to
>>> MacOS instead of Linux, but with the reduced value it doesn't really matter any more.
>>
>> 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...

Thanks!

Phil.

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

[Mark Cave-Ayland]

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 <mark.cave-ayland@ilande.co.uk>



ATB,

Mark.

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

[Philippe Mathieu-Daudé]

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 <mark.cave-ayland@ilande.co.uk>
> 

Thanks!

Phil.

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

[Mark Cave-Ayland]

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 <mark.cave-ayland@ilande.co.uk>
>>
> 
> Thanks!
> 
> Phil.

No worries. David, are you able to update the commit message in your ppc-for-4.0
branch accordingly?


ATB,

Mark.

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3837 bytes --]

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 <mark.cave-ayland@ilande.co.uk>
> >>
> > 
> > Thanks!
> > 
> > Phil.
> 
> No worries. David, are you able to update the commit message in your ppc-for-4.0
> branch accordingly?

Done.

> 
> 
> ATB,
> 
> Mark.
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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 <mark.cave-ayland@ilande.co.uk>
>>>>
>>>
>>> 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.