Re: [Xen-devel] [PATCH v9 08/13] Add IOREQ_TYPE_VMWARE_PORT

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> 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@xxxxxxxxxxxxx; 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@xxxxxxxxxxx> wrote:
> > On 02/24/15 10:34, Jan Beulich wrote:
> >>>>> On 17.02.15 at 00:05, <dslutz@xxxxxxxxxxx> 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.

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

  Paul

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