Re: [Qemu-devel] [RFC PATCH v0 1/2] spapr: Allocate HTAB from machine init

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 2576 bytes --]

On Tue, Sep 22, 2015 at 09:09:48AM +0530, Bharata B Rao wrote:
> Allocate HTAB from ppc_spapr_init() so that we can abort the guest
> if requested HTAB size is't allocated by the host. However retain the
> htab reset call in spapr_reset_htab() so that HTAB gets reset (and
> not allocated) during machine reset.

I was briefly worried about this, because I recall there as a reason
htab allocation got moved to the reset handler in the first place.
Looking at the git history, however, I've convinced myself this is
basically ok (because you preserve the call during reset to wipe clean
the htab).

> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> ---
>  hw/ppc/spapr.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 7f4f196..4692122 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -979,7 +979,7 @@ static void emulate_spapr_hypercall(PowerPCCPU *cpu)
>  #define CLEAN_HPTE(_hpte)  ((*(uint64_t *)(_hpte)) &= tswap64(~HPTE64_V_HPTE_DIRTY))
>  #define DIRTY_HPTE(_hpte)  ((*(uint64_t *)(_hpte)) |= tswap64(HPTE64_V_HPTE_DIRTY))
>  
> -static void spapr_reset_htab(sPAPRMachineState *spapr)
> +static void spapr_alloc_htab(sPAPRMachineState *spapr)
>  {
>      long shift;
>      int index;
> @@ -1012,6 +1012,16 @@ static void spapr_reset_htab(sPAPRMachineState *spapr)
>              DIRTY_HPTE(HPTE(spapr->htab, index));
>          }
>      }
> +}
> +
> +static void spapr_reset_htab(sPAPRMachineState *spapr)
> +{
> +    /*
> +     * We have already allocated the hash page table, this call will
> +     * not again allocate but only result in clearing of hash page
> +     * table entries.
> +     */
> +    kvmppc_reset_htab(spapr->htab_shift);

It's unlikely the kernel will give us less htab than we already have,
but we really should at least check for that.  Probably not much we
can do except abort() but at least we can give a useful error message.

>      /* Update the RMA size if necessary */
>      if (spapr->vrma_adjust) {
> @@ -1709,6 +1719,7 @@ static void ppc_spapr_init(MachineState *machine)
>          }
>          spapr->htab_shift++;
>      }
> +    spapr_alloc_htab(spapr);
>  
>      /* Set up Interrupt Controller before we create the VCPUs */
>      spapr->icp = xics_system_init(machine,

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]