| Message ID | 1442893189-5680-2-git-send-email-bharata@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
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,
On Wed, Sep 23, 2015 at 01:28:53PM +1000, David Gibson wrote: > 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. With the change I am doing here, this is no longer an allocation path. Host kernel will just clear the HTAB and return the same htab_shift that we passed here. So do you think it still makes sense to check return value ? Regards, Bharata.
On Thu, Sep 24, 2015 at 09:11:52AM +0530, Bharata B Rao wrote: > On Wed, Sep 23, 2015 at 01:28:53PM +1000, David Gibson wrote: > > 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. > > With the change I am doing here, this is no longer an allocation path. > Host kernel will just clear the HTAB and return the same htab_shift > that we passed here. So do you think it still makes sense to check > return value ? That's the current kernel behaviour, but the interface doesn't guarantee that. So, yes, I still think you have to check the return value.
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); /* 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,
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. Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> --- hw/ppc/spapr.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)