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

[Don Slutz <dslutz@xxxxxxxxxxx>]

On 02/26/15 06:49, Paul Durrant wrote:
>> -----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.
> 

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.

   -Don Slutz

>   Paul
> 

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