Re: [PATCH 3/3] Add HVMOP to map guest ram with p2m_ioreq_server to an ioreq server

[George Dunlap <george.dunlap@citrix.com>]

On 22/03/16 17:51, Paul Durrant wrote:
>> -----Original Message-----
>> From: George Dunlap [mailto:george.dunlap@citrix.com]
>> Sent: 22 March 2016 17:27
>> To: Yu Zhang; xen-devel@lists.xen.org
>> Cc: Keir (Xen.org); jbeulich@suse.com; Andrew Cooper; George Dunlap;
>> jun.nakajima@intel.com; Kevin Tian; Tim (Xen.org); Paul Durrant;
>> zhiyuan.lv@intel.com
>> Subject: Re: [PATCH 3/3] Add HVMOP to map guest ram with
>> p2m_ioreq_server to an ioreq server
>>
>> On 16/03/16 12:22, Yu Zhang wrote:
>>> A new HVMOP - HVMOP_map_mem_type_to_ioreq_server, is added to
>>> let one ioreq server claim its responsibility for the handling
>>> of guest pages with p2m type p2m_ioreq_server. Users of this
>>> HVMOP can specify whether the p2m_ioreq_server is supposed to
>>> handle write accesses or read ones in a flag. By now, we only
>>> support one ioreq server for this p2m type, so once an ioreq
>>> server has claimed its ownership, following calls of the
>>> HVMOP_map_mem_type_to_ioreq_server will fail.
>>>
>>> Note: this HVMOP does not change the p2m type of any guest ram
>>> page, until the HVMOP_set_mem_type is triggered. So normally
>>> the steps would be the backend driver first claims its ownership
>>> of guest ram pages with p2m_ioreq_server type. At then sets the
>>> memory type to p2m_ioreq_server for specified guest ram pages.
>>
>> Yu, thanks for this work.  I think it's heading in the right direction.
>>
>> A couple of comments:
>>
>> There's not much documentation in the code about how this is expected to
>> be used.
>>
>> For instance, having separate flags seems to imply that you can
>> independently select either read intercept, write intercept, or both;
>> but [ept_]p2m_type_to_flags() seems to assume that READ_ACCESS implies
>> WRITE_ACCESS.  Do you plan to implement them separately in the future?
>> If not, would it be better to make the interface an enum instead?
>>
>> At very least it should be documented that READ_ACCESS implies
>> WRITE_ACCESS.
> 
> That's not true. If WRITE_ACCESS has not been requested then writes are handled directly in Xen rather than being forwarded to the ioreq server. If h/w were to allow pages to be marked write-only then we wouldn't need to do that.

Right -- well this at least needs to be in the commit message, and it
would be good if it were in a comment somewhere as well.

It's probably also useful to note in the interface that if you're only
intercepting writes, reads will happen at full speed; but that if you're
only intercepting reads, writes must be emulated (and so there will have
a significant performance impact).

>>> @@ -168,13 +226,65 @@ static int hvmemul_do_io(
>>>          break;
>>>      case X86EMUL_UNHANDLEABLE:
>>>      {
>>> -        struct hvm_ioreq_server *s =
>>> -            hvm_select_ioreq_server(curr->domain, &p);
>>> +        struct hvm_ioreq_server *s;
>>> +        p2m_type_t p2mt;
>>> +
>>> +        if ( is_mmio )
>>> +        {
>>> +            unsigned long gmfn = paddr_to_pfn(addr);
>>> +
>>> +            (void) get_gfn_query_unlocked(currd, gmfn, &p2mt);
>>> +
>>> +            switch ( p2mt )
>>> +            {
>>> +                case p2m_ioreq_server:
>>> +                {
>>> +                    unsigned long flags;
>>> +
>>> +                    p2m_get_ioreq_server(currd, p2mt, &flags, &s);
>>> +
>>> +                    if ( !s )
>>> +                        break;
>>> +
>>> +                    if ( (dir == IOREQ_READ &&
>>> +                          !(flags & P2M_IOREQ_HANDLE_READ_ACCESS)) ||
>>> +                         (dir == IOREQ_WRITE &&
>>> +                          !(flags & P2M_IOREQ_HANDLE_WRITE_ACCESS)) )
>>> +                        s = NULL;
>>> +
>>> +                    break;
>>> +                }
>>> +                case p2m_ram_rw:
>>> +                    s = NULL;
>>> +                    break;
>>> +
>>> +                default:
>>> +                    s = hvm_select_ioreq_server(currd, &p);
>>> +                    break;
>>> +            }
>>> +        }
>>> +        else
>>> +        {
>>> +            p2mt = p2m_invalid;
>>> +
>>> +            s = hvm_select_ioreq_server(currd, &p);
>>> +        }
>>>
>>>          /* If there is no suitable backing DM, just ignore accesses */
>>>          if ( !s )
>>>          {
>>> -            rc = hvm_process_io_intercept(&null_handler, &p);
>>> +            switch ( p2mt )
>>> +            {
>>> +            case p2m_ioreq_server:
>>> +            case p2m_ram_rw:
>>> +                rc = hvm_process_io_intercept(&mem_handler, &p);
>>> +                break;
>>> +
>>> +            default:
>>> +                rc = hvm_process_io_intercept(&null_handler, &p);
>>> +                break;
>>> +            }
>>
>> Is it actually possible to get here with "is_mmio" true and p2mt ==
>> p2m_ram_rw?
> 
> I think that's possible if the type change races with an access.

OK -- a brief comment about that would be helpful.

Thanks,
 -George


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