Re: [PATCH] spapr: nested: Add support for DPDES SPR in GSB for TCG L0

[Harsh Prateek Bora]

Hi Amit,

On 10/18/24 10:47, Harsh Prateek Bora wrote:
> Hi Amit,
>
> On 10/17/24 16:30, Amit Machhiwal wrote:
> > The DPDES support for doorbell emulation and handling for KVM on PAPR
> > guests was added in Linux via [1]. Subsequently, a new GSB element for
> > DPDES was added in Linux; the same has been missing in QEMU L0. Add
>
> s/QEMU L0/ TCG L0 implementation?
>
> > support for DPDES register's APIv2 GSB element and required handling in
> > `spapr_nested.c`.
> >
> > Currently, booting a KVM guest inside a QEMU TCG guest fails with the
> > below crash. The crash is encountered when GUEST_RUN_VCPU hcall made
> > into QEMU TCG L0 fails because H_INVALID_ELEMENT_VALUE is returned as
> > the mapping of the element ID corresponding to DPDES (unknown to QEMU
> > TCG L0) in GSR (Guest State Request) of TCG guest's KVM to the GSB
> > (Guest State Buffer) elements of QEMU TCG L0 fails.
>
> GSB full form would be more appropriate along with first mention of GSB 
> in above text.
>
> With that:
> Reviewed-by: Harsh Prateek Bora <harshpb@linux.ibm.com>
>
> >

<snip>

> > diff --git a/hw/ppc/spapr_nested.c b/hw/ppc/spapr_nested.c
> > index c02785756c1e..b696ad537a77 100644
> > --- a/hw/ppc/spapr_nested.c
> > +++ b/hw/ppc/spapr_nested.c
> > @@ -194,6 +194,7 @@ static void nested_save_state(struct 
> > nested_ppc_state *save, PowerPCCPU *cpu)
> >           save->fscr = env->spr[SPR_FSCR];
> >           save->pspb = env->spr[SPR_PSPB];
> >           save->ctrl = env->spr[SPR_CTRL];
> > +        save->dpdes = env->spr[SPR_DPDES];
> >           save->vrsave = env->spr[SPR_VRSAVE];
> >           save->dar = env->spr[SPR_DAR];
> >           save->dsisr = env->spr[SPR_DSISR];
> > @@ -293,6 +294,7 @@ static void nested_load_state(PowerPCCPU *cpu, 
> > struct nested_ppc_state *load)
> >           env->spr[SPR_FSCR] = load->fscr;
> >           env->spr[SPR_PSPB] = load->pspb;
> >           env->spr[SPR_CTRL] = load->ctrl;
> > +        env->spr[SPR_DPDES] = load->dpdes;
> >           env->spr[SPR_VRSAVE] = load->vrsave;
> >           env->spr[SPR_DAR] = load->dar;
> >           env->spr[SPR_DSISR] = load->dsisr;

Looks like we overlooked above inits already present in existing code.
I just checked above inits in nested_{save,load}_state are already
present as common code between apiv1 & apiv2. So, we only need the inits
below for initializing the lookup table.

regards,
Harsh
> > @@ -982,6 +984,7 @@ struct guest_state_element_type 
> > guest_state_element_types[] = {
> >       GUEST_STATE_ELEMENT_ENV_DW(GSB_VCPU_SPR_FSCR,  fscr),
> >       GUEST_STATE_ELEMENT_ENV_W(GSB_VCPU_SPR_PSPB,   pspb),
> >       GUEST_STATE_ELEMENT_ENV_DW(GSB_VCPU_SPR_CTRL,  ctrl),
> > +    GUEST_STATE_ELEMENT_ENV_DW(GSB_VCPU_SPR_DPDES, dpdes),
> >       GUEST_STATE_ELEMENT_ENV_W(GSB_VCPU_SPR_VRSAVE, vrsave),
> >       GUEST_STATE_ELEMENT_ENV_DW(GSB_VCPU_SPR_DAR,   dar),
> >       GUEST_STATE_ELEMENT_ENV_W(GSB_VCPU_SPR_DSISR,  dsisr),
> > diff --git a/include/hw/ppc/spapr_nested.h 
> > b/include/hw/ppc/spapr_nested.h
> > index 93ef14adcc5e..3b5cd993c256 100644
> > --- a/include/hw/ppc/spapr_nested.h
> > +++ b/include/hw/ppc/spapr_nested.h
> > @@ -99,7 +99,8 @@
> >   #define GSB_VCPU_SPR_HASHKEYR   0x1050
> >   #define GSB_VCPU_SPR_HASHPKEYR  0x1051
> >   #define GSB_VCPU_SPR_CTRL       0x1052
> > -                    /* RESERVED 0x1053 - 0x1FFF */
> > +#define GSB_VCPU_SPR_DPDES      0x1053
> > +                    /* RESERVED 0x1054 - 0x1FFF */
> >   #define GSB_VCPU_SPR_CR         0x2000
> >   #define GSB_VCPU_SPR_PIDR       0x2001
> >   #define GSB_VCPU_SPR_DSISR      0x2002
> >
> > base-commit: aa54f5be44be786636a5d51cc1612ad208a24849