Re: [PATCH v9 08/13] Add IOREQ_TYPE_VMWARE_PORT

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

> -----Original Message-----
> From: Don Slutz [mailto:dslutz@verizon.com]
> Sent: 26 February 2015 14:55
> To: Paul Durrant; Jan Beulich; Don Slutz
> Cc: Aravind Gopalakrishnan; Suravee Suthikulpanit; Andrew Cooper; Ian
> Campbell; George Dunlap; Ian Jackson; Stefano Stabellini; Eddie Dong; Jun
> Nakajima; Kevin Tian; xen-devel@lists.xen.org; Boris Ostrovsky; Konrad
> Rzeszutek Wilk; Keir (Xen.org); Tim (Xen.org)
> Subject: Re: [PATCH v9 08/13] Add IOREQ_TYPE_VMWARE_PORT
> 
> On 02/26/15 06:49, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: Jan Beulich [mailto:JBeulich@suse.com]
> >> Sent: 26 February 2015 08:08
> >> To: Don Slutz
> >> Cc: Aravind Gopalakrishnan; Suravee Suthikulpanit; Andrew Cooper; Ian
> >> Campbell; Paul Durrant; George Dunlap; Ian Jackson; Stefano Stabellini;
> Eddie
> >> Dong; Jun Nakajima; Kevin Tian; xen-devel@lists.xen.org; Boris Ostrovsky;
> >> Konrad Rzeszutek Wilk; Keir (Xen.org); Tim (Xen.org)
> >> Subject: Re: [PATCH v9 08/13] Add IOREQ_TYPE_VMWARE_PORT
> >>
> >>>>> On 25.02.15 at 21:20, <dslutz@verizon.com> wrote:
> >>> On 02/24/15 10:34, Jan Beulich wrote:
> >>>>>>> On 17.02.15 at 00:05, <dslutz@verizon.com> wrote:
> >>>>> @@ -501,22 +542,50 @@ static void hvm_free_ioreq_gmfn(struct
> >> domain *d, unsigned long gmfn)
> >>>>>      clear_bit(i, &d->arch.hvm_domain.ioreq_gmfn.mask);
> >>>>>  }
> >>>>>
> >>>>> -static void hvm_unmap_ioreq_page(struct hvm_ioreq_server *s,
> >> bool_t buf)
> >>>>> +static void hvm_unmap_ioreq_page(struct hvm_ioreq_server *s, int
> >> buf)
> >>>>>  {
> >>>>> -    struct hvm_ioreq_page *iorp = buf ? &s->bufioreq : &s->ioreq;
> >>>>> +    struct hvm_ioreq_page *iorp = NULL;
> >>>>> +
> >>>>> +    switch ( buf )
> >>>>> +    {
> >>>>> +    case 0:
> >>>>> +        iorp = &s->ioreq;
> >>>>> +        break;
> >>>>> +    case 1:
> >>>>> +        iorp = &s->bufioreq;
> >>>>> +        break;
> >>>>> +    case 2:
> >>>>> +        iorp = &s->vmport_ioreq;
> >>>>> +        break;
> >>>>> +    }
> >>>>
> >>>> These literals should become an enum.
> >>>>
> >>>
> >>> Paul Durrant asked for #defined values.  So it is not clear which way to
> >>> go. Will wait for response.
> >>
> >> Obviously either would be fine. An enum has the potential advantage
> >> of the compiler being able to check switch statements for completeness
> >> (albeit there are cases where this ends up being a disadvantage).
> >>
> >
> > I'm fine with an enum... just not with (repeated) magic numbers in the
> code.
> >
> 
> Ok, will use enum.
> 
> 
> > [snip]
> >>> Some background.  When Julien Grall added 2, this was said:
> >>>
> >>> Keir Fraser
> >>> [...]
> >>> They were almost certainly used for representing R-M-W ALU
> operations
> >> back
> >>> in the days of the old IO emulator, very long ago. Still, there''s no
> >>> harm in
> >>> leaving them unused.
> >>> [...]
> >>> I did find the old defn:
> >>>
> >>> dcs-xen-54:~/xen>git show 4ff8936 | grep IOREQ_TYPE_
> >>> #define IOREQ_TYPE_PIO          0 /* pio */
> >>> #define IOREQ_TYPE_COPY         1 /* mmio ops */
> >>> #define IOREQ_TYPE_AND          2
> >>> #define IOREQ_TYPE_OR           3
> >>> #define IOREQ_TYPE_XOR          4
> >>> #define IOREQ_TYPE_XCHG         5
> >>> #define IOREQ_TYPE_ADD          6
> >>> [...]
> >>> Which matches what Keir Fraser said.
> >>>
> >>> I did not find why Paul Durrant did not use 9.  I can only find it as 2
> >>> in all Paul's patch sets.  Which is closely connected to:
> >>>
> >>>          BUILD_BUG_ON(IOREQ_TYPE_PIO != HVMOP_IO_RANGE_PORT);
> >>>          BUILD_BUG_ON(IOREQ_TYPE_COPY !=
> >> HVMOP_IO_RANGE_MEMORY);
> >>>          BUILD_BUG_ON(IOREQ_TYPE_PCI_CONFIG !=
> >> HVMOP_IO_RANGE_PCI);
> >>>
> >>> (a new hyper call arg).  This also did not add a hole in "range" so
> >>> Paul Durrant did not have to check for a "range" hole.
> >>>
> >>> So I did just like Paul Durrant did.  He also approved the patch with
> >>> this number in QEMU.  Since this is now in upstream QEMU, changing it
> at
> >>> this time is a slower process.  Since the only time QEMU uses its
> >>> version is when Xen header files are missing, I do not see how a QEMU
> >>> built with its version would be usable as a QEMU for Xen.  So I am
> >>> happy to change to a new number like 9.
> >>
> >> Yes please. I'm not saying we absolutely have to correct the one
> >> Paul added (unless we learn it causes problems), but I think we
> >> should avoid making the same (even if only potential) mistake twice.
> >> Of course it would help to get insight from Paul (now Cc-ed) here.
> >>
> >
> > I used the hole because I really did not think anyone would
> > ever expect to use an ancient emulator against a recent
> > Xen. Given there has been no fallout I don't see why we can't
> > use the hole.
> 
> 
> Well, this is a little confusing (I read this as Paul is fine with 3).
> Since both Jan Beulich and Keir Fraser want to skip the hole, I will
> switch to 9.

I read Keir's comment as meaning he didn't care either way. If Jan wants to use 9 then I have no objection.

  Paul

> 
>    -Don Slutz
> 
> >   Paul
> >