Re: [Qemu-ppc] [PATCH v5 22/36] spapr/xive: add models for KVM support

[David Gibson]

On Wed, Nov 28, 2018 at 11:45:46PM +0100, Cédric Le Goater wrote:
> On 11/28/18 6:52 AM, David Gibson wrote:
> > On Fri, Nov 16, 2018 at 11:57:15AM +0100, Cédric Le Goater wrote:
> >> This introduces a set of XIVE models specific to KVM which derive from
> >> the XIVE base models. The interfaces with KVM are a new capability and
> >> a new KVM device for the XIVE native exploitation interrupt mode.
> >>
> >> They handle the initialization of the TIMA and the source ESB memory
> >> regions which have a different type under KVM. These are 'ram device'
> >> memory mappings, similarly to VFIO, exposed to the guest and the
> >> associated VMAs on the host are populated dynamically with the
> >> appropriate pages using a fault handler.
> >>
> >> Signed-off-by: Cédric Le Goater <address@hidden>
> > 
> > The logic here looks fine, but I think it would be better to activate
> > it with explicit if (kvm) type logic rather than using a subclass.
> 
> ok. ARM has taken a different path, the one proposed below, but it should 
> be possible to use a "if (kvm)" type logic. There should be less noise 
> in the object design.

Yeah, it seemed like a good path when I wrote the XICS code, but
experience with that has led me to decide it wasn't a good idea.  I'm
not sure if the ARM people copied that or came up with it on their
own.

[snip]
> >> +/*
> >> + * XIVE Thread Interrupt Management context (KVM)
> >> + */
> >> +
> >> +static void xive_tctx_kvm_init(XiveTCTX *tctx, Error **errp)
> >> +{
> >> +    sPAPRXive *xive;
> >> +    unsigned long vcpu_id;
> >> +    int ret;
> >> +
> >> +    /* Check if CPU was hot unplugged and replugged. */
> >> +    if (kvm_cpu_is_enabled(tctx->cs)) {
> >> +        return;
> >> +    }
> >> +
> >> +    vcpu_id = kvm_arch_vcpu_id(tctx->cs);
> >> +    xive = SPAPR_XIVE_KVM(tctx->xrtr);
> > 
> > Is this the first use of tctx->xrtr?
> 
> No, the second. the first is the reset_tctx() ops doing the CAM reset.
> But we said that we could remove it.

And I think we can remove it here too.  We know this is PAPR specific
so we can go qdev_get_machine() -> PAPR xive object.

I normally don't like using qdev_get_machine(), but I think it's a
little less ugly than including this backlink (and it is sometimes
necessary to deal with the different abstraction boundaries between
qemu and KVM).

-- 
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

signature.asc
Description: PGP signature