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

["Cédric Le Goater" <clg@kaod.org>]

On 06/02/2016 08:09 PM, Mark Cave-Ayland wrote:
> On 02/06/16 09:47, Mark Cave-Ayland wrote:
> 
>> 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.
> 
> Just to add in case it wasn't clear from my previous reply - this is
> actually the correct and expected behaviour. If you add a hard disk
> device with -hda then it will appear on-screen below the prompt at which
> point you can proceed with the installation as normal.

yes. I tried that and it did.

C.