Re: [PATCH v5 11/28] xsplice: Implement support for applying/reverting/replacing patches.

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

> >> So you check for there being one such section. Is having multiple
> >> of them okay / meaningful?
> > 
> > /me blinks. You can have multiple ELF sections with the same name?
> > I will double-check the spec over the weekend to see.
> 
> Of course you can. Remember me telling you that using section
> names for identification is a bad idea (even if widely done that
> way)? That's precisely because section names in the spec serve
> no meaning other than making sections identifiable to the human
> eye. For certain sections there's a more or less strict convention
> on what their names should be, but that's all there is to names.
> Section types and attributes really are what describe their
> purposes.

This is going to take a bit of time to get right I am afraid.
(The checks are easy - but make sure the payload files that are generated
are doing the right thing).
> 
> And even if that really was forbidden by the spec, you'd still need
> to make sure the binary meets the spec.

Yes.
> >> > +void check_for_xsplice_work(void)
> >> > +{
> >> > +    /* Set at -1, so will go up to num_online_cpus - 1. */
> >> > +    if ( atomic_inc_and_test(&xsplice_work.semaphore) )
> >> > +    {
> >> > +        struct payload *p;
> >> > +        unsigned int total_cpus;
> >> > +
> >> > +        p = xsplice_work.data;
> >> > +        if ( !get_cpu_maps() )
> >> > +        {
> >> > +            printk(XENLOG_ERR "%s%s: CPU%u - unable to get cpu_maps lock!\n",
> >> > +                   XSPLICE, p->name, cpu);
> >> > +            per_cpu(work_to_do, cpu) = 0;
> >> > +            xsplice_work.data->rc = -EBUSY;
> >> > +            xsplice_work.do_work = 0;
> >> 
> >> On x86 such possibly simultaneous accesses may be okay, but is
> >> that universally the case? Wouldn't it better be only the monarch
> >> which updates shared data?
> > 
> > Monarch? Oh you mean arch specific code path?
> 
> Oh, I think you call it "master" elsewhere. IA64 terminology which
> I came to like.
> 

The master CPU is the one updating it.
> >> > +        /* All CPUs are waiting, now signal to disable IRQs. */
> >> > +        xsplice_work.ready = 1;
> >> > +        smp_wmb();
> >> > +
> >> > +        atomic_inc(&xsplice_work.irq_semaphore);
> >> > +        if ( !xsplice_do_wait(&xsplice_work.irq_semaphore, timeout, total_cpus,
> >> > +                              "Timed out on IRQ semaphore") )
> >> 
> >> I'd prefer if the common parts of that message moved into
> >> xsplice_do_wait() - no reason to have more string literal space
> >> occupied than really needed. Also, looking back, the respective
> >> function parameter could do with a more descriptive name.
> >> 
> >> And then - does it make sense to wait the perhaps full 30ms
> >> on this second step? Rendezvoused CPUs should react rather
> >> quickly to the signal to disable interrupts.
> > 
> > We don't reset the timeout - the timeout is for both calls
> > to xsplice_do_wait.
> 
> I understand that's the way it's right now, but that's what I'm putting
> under question. Rendezvousing CPUs is quite a bit more at risk of
> taking some time compared to having already rendezvoused CPUs
> disable their IRQs.

Yes. I could expand the timeout, but maybe have some reset (add more
timeout) once CPUs  come along?
> >> >  static int __init xsplice_init(void)
> >> >  {
> >> > +    BUILD_BUG_ON( sizeof(struct xsplice_patch_func) != 64 );
> >> > +    BUILD_BUG_ON( offsetof(struct xsplice_patch_func, new_addr) != 8 );
> >> > +    BUILD_BUG_ON( offsetof(struct xsplice_patch_func, new_size) != 24 );
> >> 
> >> What assumptions get broken if these sizes change?
> > 
> > I think you mean the offsets? They can change. I added them to make sure
> > that on 32-bit hypervisor (ARM) the offsets would be the same as on 64-bit
> > hypervisor (x86).
> > 
> > The size however MUST remain the same - otherwise the toolstack won't 
> > produce proper payloads anymore.
> 
> Well, that's a very bad thing then imo. You'd better version the
> structure instead.

Andrew reminded me that we also will have build-ids. Those by nature imply
a version. That is the payload won't ever apply against a different type
hypervisor.

But I still added a version field in the struct so when we get the
'checking the old code' we have a mechanism to select -build-id or
the newer way.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel