Re: [Qemu-devel] [PATCH 08/12] spapr-rtas: use hypercall interface and remove special vty interfaces

[Alexander Graf]

On 19.06.2013, at 23:45, Anthony Liguori wrote:

> Alexander Graf <address@hidden> writes:
> 
>> On 19.06.2013, at 22:40, Anthony Liguori wrote:
>> 
>>> Signed-off-by: Anthony Liguori <address@hidden>
>>> ---
>>> hw/char/spapr_vty.c        | 36 ++++++++++++++++++------------------
>>> hw/ppc/spapr_rtas.c        | 18 ++++++++++--------
>>> include/hw/ppc/spapr_vio.h |  2 --
>>> 3 files changed, 28 insertions(+), 28 deletions(-)
>>> 
>>> diff --git a/hw/char/spapr_vty.c b/hw/char/spapr_vty.c
>>> index ecc2bb5..4bac79e 100644
>>> --- a/hw/char/spapr_vty.c
>>> +++ b/hw/char/spapr_vty.c
>>> @@ -63,7 +63,7 @@ static int vty_getchars(VIOsPAPRDevice *sdev, uint8_t 
>>> *buf, int max)
>>>    return n;
>>> }
>>> 
>>> -void vty_putchars(VIOsPAPRDevice *sdev, uint8_t *buf, int len)
>>> +static void vty_putchars(VIOsPAPRDevice *sdev, uint8_t *buf, int len)
>>> {
>>>    VIOsPAPRVTYDevice *dev = VIO_SPAPR_VTY_DEVICE(sdev);
>>> 
>>> @@ -86,6 +86,23 @@ static int spapr_vty_init(VIOsPAPRDevice *sdev)
>>>    return 0;
>>> }
>>> 
>>> +static VIOsPAPRDevice *vty_lookup(sPAPREnvironment *spapr, target_ulong 
>>> reg)
>>> +{
>>> +    VIOsPAPRDevice *sdev;
>>> +
>>> +    sdev = spapr_vio_find_by_reg(spapr->vio_bus, reg);
>>> +    if (!sdev && reg == 0) {
>>> +        /* Hack for kernel early debug, which always specifies reg==0.
>>> +         * We search all VIO devices, and grab the vty with the lowest
>>> +         * reg.  This attempts to mimic existing PowerVM behaviour
>>> +         * (early debug does work there, despite having no vty with
>>> +         * reg==0. */
>>> +        return spapr_vty_get_default(spapr->vio_bus);
>>> +    }
>>> +
>>> +    return sdev;
>>> +}
>>> +
>>> /* Forward declaration */
>>> static target_ulong h_put_term_char(PowerPCCPU *cpu, sPAPREnvironment 
>>> *spapr,
>>>                                    target_ulong opcode, target_ulong *args)
>>> @@ -211,23 +228,6 @@ VIOsPAPRDevice *spapr_vty_get_default(VIOsPAPRBus *bus)
>>>    return selected;
>>> }
>>> 
>>> -VIOsPAPRDevice *vty_lookup(sPAPREnvironment *spapr, target_ulong reg)
>>> -{
>>> -    VIOsPAPRDevice *sdev;
>>> -
>>> -    sdev = spapr_vio_find_by_reg(spapr->vio_bus, reg);
>>> -    if (!sdev && reg == 0) {
>>> -        /* Hack for kernel early debug, which always specifies reg==0.
>>> -         * We search all VIO devices, and grab the vty with the lowest
>>> -         * reg.  This attempts to mimic existing PowerVM behaviour
>>> -         * (early debug does work there, despite having no vty with
>>> -         * reg==0. */
>>> -        return spapr_vty_get_default(spapr->vio_bus);
>>> -    }
>>> -
>>> -    return sdev;
>>> -}
>>> -
>>> static void spapr_vty_register_types(void)
>>> {
>>>    spapr_register_hypercall(H_PUT_TERM_CHAR, h_put_term_char);
>>> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
>>> index 5887e04..019aed5 100644
>>> --- a/hw/ppc/spapr_rtas.c
>>> +++ b/hw/ppc/spapr_rtas.c
>>> @@ -44,14 +44,16 @@ static void rtas_display_character(PowerPCCPU *cpu, 
>>> sPAPREnvironment *spapr,
>>>                                   uint32_t nret, target_ulong rets)
>>> {
>>>    uint8_t c = rtas_ld(args, 0);
>>> -    VIOsPAPRDevice *sdev = vty_lookup(spapr, 0);
>>> -
>>> -    if (!sdev) {
>>> -        rtas_st(rets, 0, -1);
>>> -    } else {
>>> -        vty_putchars(sdev, &c, sizeof(c));
>>> -        rtas_st(rets, 0, 0);
>>> -    }
>>> +    target_ulong hargs[4] = {
>> 
>> This is too small. I believe it works with today's code, but the
>> hypercall ABI allows for more registers to be accessed,
> 
> Not for this hypercall, but I can introduce spapr_hypercall[0-9] calls
> if it makes you feel better about it :-)

I think it's a lot easier to merely always pass an array of 9 args into the 
helper function. It's what the guest facing hypercall code does too. It just 
passes &env->gprs[4].


Alex