Re: [Xen-devel] [PATCH v13 7/8] Add IOREQ_TYPE_VMWARE_PORT

[Don Slutz <don.slutz@xxxxxxxxx>]

On 12/07/15 08:36, Paul Durrant wrote:
>> -----Original Message-----
> [snip]
>>>>>>
>>>>>>      if ( rc )
>>>>>> -        hvm_unmap_ioreq_page(s, 0);
>>>>>> +    {
>>>>>> +        hvm_unmap_ioreq_page(s, IOREQ_PAGE_TYPE_IOREQ);
>>>>>> +        return rc;
>>>>>> +    }
>>>>>> +
>>>>>> +    rc = hvm_map_ioreq_page(s, IOREQ_PAGE_TYPE_VMPORT,
>>>>>> vmport_ioreq_pfn);
>>>>>
>>>>> Is every ioreq server going to have one of these? It doesn't look
>>>>> like it, so should you not have validity check on the pfn?
>>>>>
>>>>
>>>>
>>>> Currently the default is that all ioreq servers get the mapping:
>>>>
>>>
>>> That's probably a bit wasteful. It should probably be
>>> selectable in the create HVM op.
>>
>> Since the most common case is QEMU and it can use it since upstream
>> version 2.2.0, the waste is real small.  If a non-QEMU ioreq server does
>> not want it, it add 0 overhead. 
> 
> It's not 0 overhead if an extra magic page is used for each ioreq server is 
> it?
>

My understanding is that the Xen overhead is 1 entry in p2m for each
ioreq server.


>> The only case I know of (which is PVH
>> related to PVH) is if QEMU is not running and you add a non-QEMU ioreq
>> server that does not use it, you get 1 page + page table entries.
>>
>> While the following exists:
>>
>> #define HVM_IOREQSRV_BUFIOREQ_OFF    0
>> #define HVM_IOREQSRV_BUFIOREQ_LEGACY 1
>> /*
>>  * Use this when read_pointer gets updated atomically and
>>  * the pointer pair gets read atomically:
>>  */
>> #define HVM_IOREQSRV_BUFIOREQ_ATOMIC 2
>>     uint8_t handle_bufioreq; /* IN - should server handle buffered ioreqs */
>>
>> I see no tests that use these.  Also adding vmport enable/disable to
>> handle_bufioreq is much more of a hack then I expect to pass a code
>> review.
>>
>> Does not look simple to add a new additional argument, but that could
>> just be my lack of understanding in the area.
> 
> It doesn't look that bad. The bufioreq flag has now expanded
> from 1 bit to 2 bits, but you could rename 'handle_bufioreq' to
> 'flags' or some such and then use bit 3 to indicate whether or
> not the vmport ioreq page should be allocated.
> 

Ok, I will code this up and plan to go with it.  Since old QEMU need to
be supported, bit 3 will be a negative flag, when set no page will be
mapped.

>>
>>> I don't know whether you'd
>>> even need it there in the default server since I guess the QEMU
>>> end of things post-dates the use of the HVM op (rather than the
>>> old param).
>>>
>>
>> Not sure how to parse "post-dates".  Here is some tables about this that
>> I know about:
>>
>>
>> xen     Supports                handle_bufioreq
>>          create_ioreq_server
>> 4.5     yes                     0 or !0
>> 4.6     yes                     0 or !0
>> 4.7     yes                     0 or !0
>>
>> Upstream vmport         is_default atomic
>>  QEMU     support
>>
>> 2.2.x    yes            yes        n/a
>> 2.3.x    yes            no         no
>> 2.4.x    yes            no         no
>> 2.5.x    yes            no         yes
>>
>> Xen      vmport         is_default atomic
>>  QEMU     support
>>
>> 4.5.x    no             yes        n/a
>> 4.6.x    yes            no         yes
>> 4.7.x    yes            no         yes
>>
>> With just a "xen only" build, you will not get a QEMU that is a default
>> ioreq server.  However it looks possible to mix Xen and QEMU and get
>> this case.
>>
> 
> What I meant was that I believe that the vmport ioreq page will
> only be used by a qemu that also make use of the
> create_ioreq_server hmvop, so you don't have to care about
> making it work with older QEMU. It looks like 2.2.x may prove
> me wrong though in which case it should be present in the
> default ioreq server but still optional for all others.
> 

Yes, default ioreq server will get the mapping when enabled,
optional for all others.

   -Don Slutz

>   Paul
> 
>> So unless I hear otherwise, I will not be making a change here.
>>
>>>> +        /* VMware port */
>>>> +        if ( i == HVMOP_IO_RANGE_VMWARE_PORT &&
>>>> +            s->domain->arch.hvm_domain.is_vmware_port_enabled )
>>>> +            rc = rangeset_add_range(s->range[i], 1, 1);
>>>>
>>>>
>>>>
>>>> but you are right that a check on is_vmware_port_enabled should be
>>>> added.  Will do.
>>>
>>> Cool. Thanks,
>>>
>>>   Paul
>>>
>>>>
>>>>    -Don Slutz
>>>>
>>>>>   Paul
>>>>>

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