[OpenBIOS] [PATCH] ppc: use rfid when running under a CPU from the 970 family.

Alexander Graf agraf at suse.de
Mon Jun 20 18:36:33 CEST 2016



> Am 20.06.2016 um 17:42 schrieb Cédric Le Goater <clg at kaod.org>:
> 
>> On 06/20/2016 03:26 PM, Alexander Graf wrote:
>>> On 06/20/2016 02:59 PM, Cédric Le Goater wrote:
>>>> On 06/20/2016 11:49 AM, Mark Cave-Ayland wrote:
>>>>> On 20/06/16 10:33, Alexander Graf wrote:
>>>>> 
>>>>>> On 06/20/2016 11:32 AM, Cédric Le Goater wrote:
>>>>>>> On 06/20/2016 09:39 AM, Alexander Graf wrote:
>>>>>>>> On 20.06.16 09:34, Cédric Le Goater wrote:
>>>>>>>> A recent attempt to restrict the use of rfi to 64bit cpus in qemu
>>>>>>>> broke 32bit OpenBIOS when run under a 970.
>>>>>>>> 
>>>>>>>> This is adding a dynamic check on the pvr to choose between rfi and
>>>>>>>> rfid. Only the 970 family is supported.
>>>>>>>> 
>>>>>>>> Signed-off-by: Cédric Le Goater <clg at kaod.org>
>>>>>>>> ---
>>>>>>>> 
>>>>>>>>   Tested on qemu.
>>>>>>>> 
>>>>>>>>   arch/ppc/qemu/start.S |   25 +++++++++++++++++++++++--
>>>>>>>>   arch/ppc/start.S      |   25 +++++++++++++++++++++++--
>>>>>>>>   2 files changed, 46 insertions(+), 4 deletions(-)
>>>>>>>> 
>>>>>>>> Index: openbios.git/arch/ppc/qemu/start.S
>>>>>>>> ===================================================================
>>>>>>>> --- openbios.git.orig/arch/ppc/qemu/start.S
>>>>>>>> +++ openbios.git/arch/ppc/qemu/start.S
>>>>>>>> @@ -27,6 +27,27 @@
>>>>>>>>     #ifdef CONFIG_PPC_64BITSUPPORT
>>>>>>>>   +/* When running on ppc64, we cannot use rfi anymore. Let's try to
>>>>>>>> + * catch which cpu we are running on and act accordingly. This is
>>>>>>>> + * for 970s only.
>>>>>>> Do you think you could move the check into the cpu init function and use
>>>>>>> a global variable / live patching instead? I don't remember if we need
>>>>>>> to handle any faults before we reach that path.
>>>>>> So we get an ISI really early, when returning from setup_mmu(), which is
>>>>>> the first call in _entry.
>>>>> Ok, can we add the check there?
>>>> Bear in mind that the code for entry/context switching has been reworked
>>>> pending commit whilst we switch the OpenBIOS repository over from SVN to
>>>> git. The relevant commits are here:
>>>> https://www.coreboot.org/pipermail/openbios/2016-May/009395.html.
>>>> 
>>>> So please consider OpenBIOS git master plus the patchset above to be the
>>>> latest code.
>>> OK. The first version applies fine. I pushed everything here :
>>> 
>>>    https://github.com/legoater/openbios/commits/master
>>> 
>>> I am still looking at what Alex is asking for : a global variable doing
>>> live patching for rfi.
>> 
>> It's not both - it's either or :).
> 
> ah :) I need to grow my live patching skills.
> 
>> So either you define a global variable that we can just read from to determine 
>> whether we do rfi or rfid
> 
> That is what I started doing and that was much bigger then v1.
> 
>> or we live patch. Looking at the code, it's probably
>> easiest to live patch even. Just call a function from setup_mmu() that checks 
>> PVR and if it's 64bit, loops through (0x0 .. 0x1000) in 32bit steps and checks 
>> if *p == INST_RFI. If so, replace with INST_RFID and flush the cache.
> 
> Indeed, the hunk below seems to do the trick :
> 
>    @@ -524,6 +524,17 @@ setup_mmu(unsigned long ramsize)
>             asm volatile("mtsrin %0,%1" :: "r" (sr_base + i), "r" (j));
>         }
>     
>    +    if (is_ppc64()) {
>    +#define RFI    0x4c000064
>    +#define RFID    0x4c000024
>    +        unsigned long addr;
>    +
>    +        for (addr = 0x2000; addr < 0x3000; addr += 4) {
>    +            unsigned long *ptr = (unsigned long *) addr;
>    +            if (*ptr == RFI) *ptr = RFID;
>    +        }
>    +    }
>    +
>     #endif
> 
> and that's permitted ? :) 

I don't see why not. But please make sure you run after the code got copied to phys 0 ;).

> 
> flushing the cache does not seem necessary. I suppose because we did not go 
> through this range of instructions yet, I am not sure how qemu handle 
> the cache instructions. Need to check.

It's not strictly necessary for tcg because that doesn't expose caching semantics and it's not really needed for pr kvm either because you patch a privileged inst with another privileged one. But it's "the correct thing to do" :).

Alex

> 
> Thanks, I will bake a patch for it. 
> 
> C. 
> 
> 




More information about the OpenBIOS mailing list