Re: [Qemu-devel] [PATCH v2 02/13] spapr/xive: add hcall support when under KVM

[David Gibson]

On Thu, Mar 14, 2019 at 10:24:49PM +0100, Cédric Le Goater wrote:
> On 3/14/19 3:11 AM, David Gibson wrote:
> > On Wed, Mar 13, 2019 at 11:43:54AM +0100, Cédric Le Goater wrote:
> >> On 3/12/19 11:26 AM, David Gibson wrote:
> >>> On Mon, Mar 11, 2019 at 06:32:05PM +0100, Cédric Le Goater wrote:
> >>>> On 2/26/19 12:22 AM, David Gibson wrote:
> >>>>> On Fri, Feb 22, 2019 at 02:13:11PM +0100, Cédric Le Goater wrote:
> >>> [snip]
> >>>>>> +void kvmppc_xive_set_source_config(sPAPRXive *xive, uint32_t lisn, 
> >>>>>> XiveEAS *eas,
> >>>>>> +                                   Error **errp)
> >>>>>> +{
> >>>>>> +    uint32_t end_idx;
> >>>>>> +    uint32_t end_blk;
> >>>>>> +    uint32_t eisn;
> >>>>>> +    uint8_t priority;
> >>>>>> +    uint32_t server;
> >>>>>> +    uint64_t kvm_src;
> >>>>>> +    Error *local_err = NULL;
> >>>>>> +
> >>>>>> +    /*
> >>>>>> +     * No need to set a MASKED source, this is the default state after
> >>>>>> +     * reset.
> >>>>>
> >>>>> I don't quite follow this comment, why is there no need to call a
> >>>>> MASKED source?
> >>>>
> >>>> because MASKED is the default state in which KVM initializes the IRQ. I 
> >>>> will
> >>>> clarify.
> >>>
> >>> I believe it's possible - though rare - to process an incoming
> >>> migration on an established VM which isn't in fresh reset state.  So
> >>> it's best not to rely on that.
> >>>
> >>>>>> +static void xive_esb_trigger(XiveSource *xsrc, int srcno)
> >>>>>> +{
> >>>>>> +    unsigned long addr = (unsigned long) xsrc->esb_mmap +
> >>>>>> +        xive_source_esb_page(xsrc, srcno);
> >>>>>> +
> >>>>>> +    *((uint64_t *) addr) = 0x0;
> >>>>>> +}
> >>>>>
> >>>>> Also.. aren't some of these register accesses likely to need memory
> >>>>> barriers?
> >>>>
> >>>> AIUI, these are CI pages. So we shouldn't need barriers.
> >>>
> >>> CI doesn't negate the need for barriers, althugh it might change the
> >>> type you need.  At the very least you need a compiler barrier to stop
> >>> it re-ordering the access, but you can also have in-cpu store and load
> >>> queues.
> >>>
> >>
> >> ok. So I will need to add some smp_r/wmb() 
> > 
> > No, smp_[rw]mb() is for cases where it's strictly about cpu vs. cpu
> > ordering.  Here it's cpu vs. IO ordering so you need plain [rw]mb().
> 
> I don't see any in QEMU ?

Ah, my mistake.  I was mixing up the kernel atomics and the qemu
atomics.

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