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

[Sarah Newman <srn@xxxxxxxxx>]

On 01/10/2018 08:56 AM, Sergey Dyasli wrote:
> 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."
> 

There is an incompatibility in this (also vixen's) serial output. The NetBSD 
installer includes NUL characters in its output and does not display
properly due to the call to puts. putc demonstrably works, though it would be 
better to add a length oriented function to serial.c.

--Sarah

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