Re: [PATCH v13 7/8] Add IOREQ_TYPE_VMWARE_PORT

[Paul Durrant <Paul.Durrant@citrix.com>]

> -----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?

> 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 somesuch and then use bit 3 to indicate whether or not the vmport ioreq page should be allocated.

> 
> > 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.

  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
> >>>