Re: [PATCH 1/2] xen/arm: Convert runstate address during hypcall

[Julien Grall <julien.grall.oss@xxxxxxxxx>]

On Mon, 15 Jun 2020 at 21:30, Stefano Stabellini <sstabellini@xxxxxxxxxx> wrote:
>
> On Mon, 15 Jun 2020, Bertrand Marquis wrote:
> > > On 13 Jun 2020, at 01:24, Stefano Stabellini <sstabellini@xxxxxxxxxx> 
> > > wrote:
> > >
> > > On Fri, 12 Jun 2020, Bertrand Marquis wrote:
> > >>> On 12 Jun 2020, at 02:09, Stefano Stabellini <sstabellini@xxxxxxxxxx> 
> > >>> wrote:
> > >>>
> > >>> On Thu, 11 Jun 2020, Julien Grall wrote:
> > >>>> Hi Stefano,
> > >>>>
> > >>>> On 11/06/2020 19:50, Stefano Stabellini wrote:
> > >>>>> On Thu, 11 Jun 2020, Julien Grall wrote:
> > >>>>>>>> +        return -EINVAL;
> > >>>>>>>>     }
> > >>>>>>>>
> > >>>>>>>> -    __copy_to_guest(runstate_guest(v), &runstate, 1);
> > >>>>>>>> +    v->arch.runstate_guest.page = page;
> > >>>>>>>> +    v->arch.runstate_guest.offset = offset;
> > >>>>>>>> +
> > >>>>>>>> +    spin_unlock(&v->arch.runstate_guest.lock);
> > >>>>>>>> +
> > >>>>>>>> +    return 0;
> > >>>>>>>> +}
> > >>>>>>>> +
> > >>>>>>>> +
> > >>>>>>>> +/* Update per-VCPU guest runstate shared memory area (if 
> > >>>>>>>> registered).
> > >>>>>>>> */
> > >>>>>>>> +static void update_runstate_area(struct vcpu *v)
> > >>>>>>>> +{
> > >>>>>>>> +    struct vcpu_runstate_info *guest_runstate;
> > >>>>>>>> +    void *p;
> > >>>>>>>> +
> > >>>>>>>> +    spin_lock(&v->arch.runstate_guest.lock);
> > >>>>>>>>
> > >>>>>>>> -    if ( guest_handle )
> > >>>>>>>> +    if ( v->arch.runstate_guest.page )
> > >>>>>>>>     {
> > >>>>>>>> -        runstate.state_entry_time &= ~XEN_RUNSTATE_UPDATE;
> > >>>>>>>> +        p = __map_domain_page(v->arch.runstate_guest.page);
> > >>>>>>>> +        guest_runstate = p + v->arch.runstate_guest.offset;
> > >>>>>>>> +
> > >>>>>>>> +        if ( VM_ASSIST(v->domain, runstate_update_flag) )
> > >>>>>>>> +        {
> > >>>>>>>> +            v->runstate.state_entry_time |= XEN_RUNSTATE_UPDATE;
> > >>>>>>>> +            guest_runstate->state_entry_time |= 
> > >>>>>>>> XEN_RUNSTATE_UPDATE;
> > >>>>>>>
> > >>>>>>> I think that this write to guest_runstate should use write_atomic or
> > >>>>>>> another atomic write operation.
> > >>>>>>
> > >>>>>> I thought about suggesting the same, but  guest_copy_* helpers may 
> > >>>>>> not
> > >>>>>> do a single memory write to state_entry_time.
> > >>>>>> What are you trying to prevent with the write_atomic()?
> > >>>>>
> > >>>>> I am thinking that without using an atomic write, it would be (at 
> > >>>>> least
> > >>>>> theoretically) possible for a guest to see a partial write to
> > >>>>> state_entry_time, which is not good.
> > >>>>
> > >>>> It is already the case with existing implementation as Xen may write 
> > >>>> byte by
> > >>>> byte. So are you suggesting the existing code is also buggy?
> > >>>
> > >>> Writing byte by byte is a different case. That is OK. In that case, the
> > >>> guest could see the state after 3 bytes written and it would be fine and
> > >>> consistent. If this hadn't been the case, then yes, the existing code
> > >>> would also be buggy.
> > >>>
> > >>> So if we did the write with a memcpy, it would be fine, no need for
> > >>> atomics:
> > >>>
> > >>> memcpy(&guest_runstate->state_entry_time,
> > >>>        &v->runstate.state_entry_time,
> > >>>        XXX);
> > >>>
> > >>>
> > >>> The |= case is different: GCC could implement it in any way it likes,
> > >>> including going through a zero-write to any of the bytes in the word, or
> > >>> doing an addition then a subtraction. GCC doesn't make any guarantees.
> > >>> If we want guarantees we need to use atomics.
> > >>
> > >> Wouldn’t that require all accesses to state_entry_time to use also 
> > >> atomic operations ?
> > >> In this case we could not propagate the changes to a guest without 
> > >> changing the interface itself.
> > >>
> > >> As the copy time needs to be protected, the write barriers are there to 
> > >> make sure that during the copy the bit is set and that when we unset it, 
> > >> the copy is done.
> > >> I added for this purpose a barrier after the memcpy to make sure that 
> > >> when/if we unset the bit the copy has already been done.
> > >
> > > As you say, we have a flag to mark a transitiong period, the flag is
> > > XEN_RUNSTATE_UPDATE. So, I think it is OK if we don't use atomics during
> > > the transitioning period. But we need to make sure to use atomics for the
> > > update of the XEN_RUNSTATE_UPDATE flag itself.
> > >
> > > Does it make sense? Or maybe I misunderstood some of the things you
> > > wrote?
> >
> > To achieve this you would do an atomic operation on state_entry_time to 
> > set/unset the XEN_RUNSTATE_UPDATE bit.
> > This field is holding a flag in the upper bit but also a value, so all 
> > operations on state_entry_time would need to be changed to use atomic 
> > operations.
>
> I don't think that all operations on state_entry_time need to be atomic.
> Only the bit write to XEN_RUNSTATE_UPDATE. More on this below.
>
>
> > Also this state_entry_time might also be accessed by the guest on other 
> > cores at the same time (to retrieve the time part).
>
> Yes but they are all just readers, right?

This is only one part of the equation. The second part is the readers
*must not* process the rest of the content while XEN_RUNSTATE_UDPATE
is set.

>
> I was not suggesting to use an atomic_t type. I was only suggesting to
> use an atomic operation, i.e. calling write_u32_atomic directly (or
> something like that.) I would not change the type of state_entry_time.
> Also using memcpy would be acceptable due to the fact that we only need
> to update one byte.

Please don't use memcpy. This is an abuse to think it can make atomic
copy (even if it may be correct for byte).
At the same time, lets avoid to introduce more atomic bit operation on
guest memory that can be read-write (see XSA-295).

In this case a write_atomic() is sufficient as it would do a single
write for size smaller than 64-bit.

Cheers,