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

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.


Thanks,

C. 

>>>> Note that there is also another regression that has recently landed in
>>>> git master so you'll also need to revert
>>>> e7c9136977cb99c6eb52c9139f7b8d8b5fa87db9 in order to get back to a
>>>> functioning OpenBIOS.
>>>
>>> I'd preter to see it fixed rather than just reverted..
>>
>> Looks like the original author has found the bug, so there should be a
>> fix coming up for this soon (I only included it here in case you needed
>> an explicit test case).
> 
> Ok.
> 
> So, yeah, I'm not really set up to test Mac machines which means I
> don't easily catch regressions like this.
> 
> Mark,
> 
> Could you look into adding a testcase to "make check" that will at
> least catch these unsubtle breaks boot type regressions?
>