Re: [Xen-devel] [PATCH RFC v1 57/74] x86/pv-shim: shadow PV console's page for L2 DomU

[Sergey Dyasli <sergey.dyasli@xxxxxxxxxx>]

On Tue, 2018-01-09 at 09:28 -0700, Jan Beulich wrote:
> > > > On 09.01.18 at 16:43, <sergey.dyasli@xxxxxxxxxx> wrote:
> > 
> > On Tue, 2018-01-09 at 02:13 -0700, Jan Beulich wrote:
> > > > > > On 04.01.18 at 14:06, <wei.liu2@xxxxxxxxxx> wrote:
> > > > 
> > > > +size_t consoled_guest_rx(void)
> > > > +{
> > > > +    size_t recv = 0, idx = 0;
> > > > +    XENCONS_RING_IDX cons, prod;
> > > > +
> > > > +    if ( !cons_ring )
> > > > +        return 0;
> > > > +
> > > > +    spin_lock(&rx_lock);
> > > > +
> > > > +    cons = cons_ring->out_cons;
> > > > +    prod = ACCESS_ONCE(cons_ring->out_prod);
> > > > +    ASSERT((prod - cons) <= sizeof(cons_ring->out));
> > > > +
> > > > +    /* Is the ring empty? */
> > > > +    if ( cons == prod )
> > > > +        goto out;
> > > > +
> > > > +    /* Update pointers before accessing the ring */
> > > > +    smp_rmb();
> > > 
> > > I think this need to move up ahead of the if(). In the comment
> > > perhaps s/Update/Latch/?
> > 
> > The read/write memory barriers here are between read/write accesses to
> > ring->out_prod and ring->out array. So there is no need to move them.
> > (the same goes for the input ring)
> 
> And there is no multiple-read issue here?

As Andrew has kindly explained to me, there is an issue indeed.
So I moved smp_rmb() to be right after cons and prod read, and updated
the comment to say:

"Latch pointers before accessing the ring. Included compiler barrier also
ensures that pointers are really read only once into local variables."

-- 
Thanks,
Sergey
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel