Re: [Qemu-devel] [Qemu-ppc] [PULL 04/12] ppc: tlbie, tlbia and tlbisync are HV only

[Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>]

On 02/06/16 09:23, Cédric Le Goater wrote:

> On 06/02/2016 09:45 AM, Mark Cave-Ayland wrote:
>> On 02/06/16 08:37, Cédric Le Goater wrote:
>>> On 06/02/2016 05:17 AM, David Gibson wrote:
>>>> On Wed, Jun 01, 2016 at 08:03:08AM +0100, Mark Cave-Ayland wrote:
>>>>> On 01/06/16 03:15, David Gibson wrote:
>>>>>
>>>>>> On Tue, May 31, 2016 at 11:28:49PM +0100, Mark Cave-Ayland wrote:
>>>>>>> On 31/05/16 01:41, David Gibson wrote:
>>>>>>>
>>>>>>>> From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>>>>>>>>
>>>>>>>> Not that anything remotely recent supports tlbia but ...
>>>>>>>>
>>>>>>>> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>>>>>>>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>>>>>>>> ---
>>>>>>>>  target-ppc/translate.c | 6 +++---
>>>>>>>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/target-ppc/translate.c b/target-ppc/translate.c
>>>>>>>> index dfd3010..690ffd2 100644
>>>>>>>> --- a/target-ppc/translate.c
>>>>>>>> +++ b/target-ppc/translate.c
>>>>>>>> @@ -4858,7 +4858,7 @@ static void gen_tlbie(DisasContext *ctx)
>>>>>>>>  #if defined(CONFIG_USER_ONLY)
>>>>>>>>      gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
>>>>>>>>  #else
>>>>>>>> -    if (unlikely(ctx->pr)) {
>>>>>>>> +    if (unlikely(ctx->pr || !ctx->hv)) {
>>>>>>>>          gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
>>>>>>>>          return;
>>>>>>>>      }
>>>>>>>> @@ -4879,7 +4879,7 @@ static void gen_tlbsync(DisasContext *ctx)
>>>>>>>>  #if defined(CONFIG_USER_ONLY)
>>>>>>>>      gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
>>>>>>>>  #else
>>>>>>>> -    if (unlikely(ctx->pr)) {
>>>>>>>> +    if (unlikely(ctx->pr || !ctx->hv)) {
>>>>>>>>          gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
>>>>>>>>          return;
>>>>>>>>      }
>>>>>>>> @@ -4898,7 +4898,7 @@ static void gen_slbia(DisasContext *ctx)
>>>>>>>>  #if defined(CONFIG_USER_ONLY)
>>>>>>>>      gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
>>>>>>>>  #else
>>>>>>>> -    if (unlikely(ctx->pr)) {
>>>>>>>> +    if (unlikely(ctx->pr || !ctx->hv)) {
>>>>>>>>          gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
>>>>>>>>          return;
>>>>>>>>      }
>>>>>>>
>>>>>>> Unfortunately this patch breaks qemu-system-ppc for both g3beige and
>>>>>>> mac99 under TCG causing a freeze in OpenBIOS when starting
>>>>>>> qemu-system-ppc with no parameters.
>>>>>>
>>>>>> Bother, sorry.
>>>>>>
>>>>>> I think this is because I applied this without the patch that treats
>>>>>> machines with no hypervisor mode (e.g. Apples) as always being in
>>>>>> hypervisor mode.
>>>>>
>>>>> No problem, I can cope for a couple of days or so.
>>>>
>>>> Cédric,
>>>>
>>>> Not sure if you've seen this thread, but one of the HV-mode patches
>>>> caused a regression on Mac.  I think it's because I didn't include the
>>>> other patch which treats Apple-mode PPCs as always having HV=1.
>>>
>>> I missed that as I didn't put myself in Cc :/ 
>>>  
>>>> Can you make sending your updated version of that patch a priority,
>>>> even if the rest of the batch of HV patches isn't ready yet.
>>>
>>> sure. I will/should today or tomorrow. I suppose we want these patches :
>>>
>>> 	[05/12] ppc: Fix hreg_store_msr() so that non-HV mode cannot alter MSR:HV
>>> 		http://patchwork.ozlabs.org/patch/618083/
>>>
>>> 	[07/12] ppc: Better figure out if processor has HV mode	
>>> 		http://patchwork.ozlabs.org/patch/618089/
>>>
>>>
>>> Mark,
>>>
>>> I tried to boot a darwinppc-602.iso with :
>>>
>>> 	qemu-system-ppc -M g3beige -cdrom darwinx86-602.iso -boot d
>>>
>>> but I get a :
>>>
>>> 	"No valid state has been set by load or ..."
>>>
>>> or we don't need to go further ? may be I need a newer FW.
>>
>> Hmmm that looks like you've got an x86 ISO there which is why
>> OpenBIOS/PPC fails to execute the bootloader. The image I use for
>> testing can be found here:
>> https://opensource.apple.com/static/iso/darwinppc-602.cdr.gz (simply
>> gunzip and then rename to .iso).
> 
> Got it. much better with ppc :) ppc is not that omnipotent.

:)

>>> Could you try the two patches above please ? They apply on top of Dave's
>>> ppc-for-2.7-20160531 and seem to have a good behavior with the small test
>>> I could do.
>>
>> I'll try and take a look tomorrow, however in the meantime see if the
>> above image enables you to replicate the issue locally.
> 
> 
> so, on top of ppc-for-2.7-20160531, with your fix for :
> 
> 	ppc: Use split I/D mmu modes to avoid flushes on interrupts

Unfortunately this isn't really a fix: the whole point of splitting the
MMU modes is to be able to avoid these expensive cache flushes in the
first place. Then again it could be that this is exposing an existing
bug elsewhere...

> and these two patches :
> 
>  	[05/12] ppc: Fix hreg_store_msr() so that non-HV mode cannot alter MSR:HV
>  		http://patchwork.ozlabs.org/patch/618083/
> 
>  	[07/12] ppc: Better figure out if processor has HV mode	
>  		http://patchwork.ozlabs.org/patch/618089/
> 
> The darwin cd boots correctly up to :
> 
> 	...
> 	The following devices are available for installation :
> 
> and then loops on something. But I don't get a kernel panic anymore.

Yes, that effectively matches what I see here - glad that you are now
able to reproduce this.


ATB,

Mark.