On Mon, 11 Sep 2017 22:50:45 +1000 David Gibson <da...@gibson.dropbear.id.au> wrote:
> On Mon, Sep 11, 2017 at 02:04:37PM +0200, Greg Kurz wrote:
> > On Sun, 10 Sep 2017 13:41:41 +1000
> > David Gibson <da...@gibson.dropbear.id.au> wrote:
> >
> > > On Mon, Sep 04, 2017 at 11:47:05PM +0200, Greg Kurz wrote:
> > > > The formula used to compute the address of the HPT allocated by QEMU is
> > > > open-coded in several places. This patch moves the magic to a dedicated
> > > > helper. While here, we also patch the callers to only pass the address
> > > > to KVM if we indeed have a userland HPT (ie, KVM PR).
> > >
> > > The helper function seems reasonable, though I'm not sure about the
> > > name (a. it's not just a pointer, since it includes the encoded size
> > > and b. the name doesn't indicate this is basically KVM PR specific).
> > >
> >
> > Sure, I'll come up with a better name.
> >
> > > THe "only pass the address to KVM if we indeed have a userland HPT
> > > (ie, KVM PR)" bit doesn't really work. You're doing it by testing for
> > > (sdr1 != 0), but that can only be true if the HPT is minimum size,
> > > which doesn't have much to do with anything meaningful.
> > >
> >
> > Hmmm... if QEMU has allocated an HPT in userspace then the helper
> > necessarily returns a non-null value, no matter the HPT size. Am
> > I missing something ?
>
> Yes, but the reverse is not true. Even if qemu *hasn't* allocated an
> HPT in userspace, it will usually return a non-zero value - the only
> case it won't is when the HPT is minimum size. That makes the test
> pretty pointless.
>
The helper does return 0 if QEMU hasn't allocated an HPT in userspace.
[...]
> > > > +target_ulong spapr_get_hpt_pointer(sPAPRMachineState *spapr)
> > > > +{
> > > > + if (!spapr->htab) {
> > > > + return 0;
> > > > + }
> > > > +
> > > > + return (target_ulong)(uintptr_t)spapr->htab | (spapr->htab_shift -
> > > > 18);
> > > > +}
> > > > +
but I agree the patch isn't good... for the comprehension at least. I'll
rework the patchset.
Also this causes a behavior change: we won't update SDR1 anymore with KVM HV,
which already handles it BTW.
void kvmppc_set_hpt(struct kvm *kvm, struct kvm_hpt_info *info)
{
atomic64_set(&kvm->arch.mmio_update, 0);
kvm->arch.hpt = *info;
kvm->arch.sdr1 = __pa(info->virt) | (info->order - 18);
Maybe I should push this behavior change to a separate patch anyway...
pgpUrtWXZrvsT.pgp
Description: OpenPGP digital signature
