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

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


ATB,

Mark.