Re: [Xen-devel] [PATCH v6 1/4] x86/ioreq server: Add HVMOP to map guest ram with p2m_ioreq_server to an ioreq server.

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On Fri, Sep 9, 2016 at 6:51 AM, Yu Zhang <yu.c.zhang@xxxxxxxxxxxxxxx> wrote:
>> On 9/2/2016 6:47 PM, Yu Zhang wrote:
>>> A new HVMOP - HVMOP_map_mem_type_to_ioreq_server, is added to
>>> let one ioreq server claim/disclaim its responsibility for the
>>> handling of guest pages with p2m type p2m_ioreq_server. Users
>>> of this HVMOP can specify which kind of operation is supposed
>>> to be emulated in a parameter named flags. Currently, this HVMOP
>>> only support the emulation of write operations. And it can be
>>> further extended to support the emulation of read ones if an
>>> ioreq server has such requirement in the future.
>>>
>>> For now, we only support one ioreq server for this p2m type, so
>>> once an ioreq server has claimed its ownership, subsequent calls
>>> of the HVMOP_map_mem_type_to_ioreq_server will fail. Users can also
>>> disclaim the ownership of guest ram pages with p2m_ioreq_server, by
>>> triggering this new HVMOP, with ioreq server id set to the current
>>> owner's and flags parameter set to 0.
>>>
>>> Note both HVMOP_map_mem_type_to_ioreq_server and p2m_ioreq_server
>>> are only supported for HVMs with HAP enabled.
>>>
>>> Also note that only after one ioreq server claims its ownership
>>> of p2m_ioreq_server, will the p2m type change to p2m_ioreq_server
>>> be allowed.
>>>
>>> Signed-off-by: Paul Durrant <paul.durrant@xxxxxxxxxx>
>>> Signed-off-by: Yu Zhang <yu.c.zhang@xxxxxxxxxxxxxxx>
>>> Acked-by: Tim Deegan <tim@xxxxxxx>
>>> ---
>>> Cc: Paul Durrant <paul.durrant@xxxxxxxxxx>
>>> Cc: Jan Beulich <jbeulich@xxxxxxxx>
>>> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>>> Cc: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
>>> Cc: Jun Nakajima <jun.nakajima@xxxxxxxxx>
>>> Cc: Kevin Tian <kevin.tian@xxxxxxxxx>
>>> Cc: Tim Deegan <tim@xxxxxxx>
>>>
>>> changes in v6:
>>>    - Clarify logic in hvmemul_do_io().
>>>    - Use recursive lock for ioreq server lock.
>>>    - Remove debug print when mapping ioreq server.
>>>    - Clarify code in ept_p2m_type_to_flags() for consistency.
>>>    - Remove definition of P2M_IOREQ_HANDLE_WRITE_ACCESS.
>>>    - Add comments for HVMMEM_ioreq_server to note only changes
>>>      to/from HVMMEM_ram_rw are permitted.
>>>    - Add domain_pause/unpause() in hvm_map_mem_type_to_ioreq_server()
>>>      to avoid the race condition when a vm exit happens on a write-
>>>      protected page, just to find the ioreq server has been unmapped
>>>      already.
>>>    - Introduce a seperate patch to delay the release of p2m
>>>      lock to avoid the race condition.
>>>    - Introduce a seperate patch to handle the read-modify-write
>>>      operations on a write protected page.
>>>
>>
>> Why do we need to do this?  Won't the default case just DTRT if it finds
>> that the ioreq server has been unmapped?
>
>
> Well, patch 4 will either mark the remaining p2m_ioreq_server entries as
> "recal" or
> reset to p2m_ram_rw directly. So my understanding is that we do not wish to
> see a ept violation due to a p2m_ioreq_server access after the ioreq server
> is unmapped.
> Yet without this domain_pause/unpause() pair, VM accesses may trigger an ept
> violation
> during the hvmop hypercall(hvm_map_mem_type_to_ioreq_server), just to find
> the ioreq
> server is NULL. Then we would have to provide handlers which just do the
> copy to/from
> actions for the VM. This seems awkward to me.

So the race you're worried about is this:

1. Guest fault happens
2. ioreq server calls map_mem_type_to_ioreq_server, unhooking
3. guest  finds no ioreq server present

I think in that case the easiest thing to do would be to simply assume
there was a race and re-execute the instruction.  Is that not possible
for some reason?

 -George

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