Re: [Xen-devel] [PATCH v2 2/3] x86/ioreq server: Rename p2m_mmio_write_dm to p2m_ioreq_server

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On Wed, Apr 20, 2016 at 4:02 PM, George Dunlap <george.dunlap@xxxxxxxxxx> wrote:
> On 19/04/16 12:02, Yu, Zhang wrote:
>>
>>
>> On 4/19/2016 12:58 AM, Paul Durrant wrote:
>>>> -----Original Message-----
>>>> From: Xen-devel [mailto:xen-devel-bounces@xxxxxxxxxxxxx] On Behalf Of
>>>> Jan
>>>> Beulich
>>>> Sent: 18 April 2016 17:47
>>>> To: Paul Durrant
>>>> Cc: Kevin Tian; Keir (Xen.org); Andrew Cooper; Tim (Xen.org); George
>>>> Dunlap; xen-devel@xxxxxxxxxxxxx; yu.c.zhang@xxxxxxxxxxxxxxx;
>>>> zhiyuan.lv@xxxxxxxxx; jun.nakajima@xxxxxxxxx
>>>> Subject: Re: [Xen-devel] [PATCH v2 2/3] x86/ioreq server: Rename
>>>> p2m_mmio_write_dm to p2m_ioreq_server
>>>>
>>>>>>> Paul Durrant <Paul.Durrant@xxxxxxxxxx> 04/18/16 6:45 PM >>>
>>>>> The original patch was posted before the cut-off IIRC so I'm not sure
>>>>> of the policy regarding freeze-exceptions.
>>>>
>>>>    It was submitted before the feature freeze, yes, but didn't make
>>>> it in by
>>>> code freeze. So it's my understanding that an exception would be needed.
>>>>
>>>
>>> Ok. Thanks for the clarification. IMO getting this in is worth the
>>> freeze exception... it's a shame p2m_mmio_write_dm made it into 4.6.1.
>>> It needs to go before it proliferates any further.
>>>
>>
>> Thanks, Paul.
>>
>> So I suppose the only place we need change for this patch is
>> for hvmmem_type_t, which should be defined like this?
>>
>> typedef enum {
>>     HVMMEM_ram_rw,             /* Normal read/write guest RAM */
>>     HVMMEM_ram_ro,             /* Read-only; writes are discarded */
>>     HVMMEM_mmio_dm,            /* Reads and write go to the device model */
>> #if __XEN_INTERFACE_VERSION__ >= 0x00040700
>>     HVMMEM_ioreq_server
>> #else
>>     HVMMEM_mmio_write_dm
>> #endif
>> } hvmmem_type_t;
>>
>> Besides, does 4.7 still accept freeze exception? It would be great
>> if we can get an approval for this.
>
> Wait, do we *actually* need this?  Is anyone actually using this?
>
> I'd say remove it, and if anyone complains, *then* do the #ifdef'ery as
> a bug-fix.  I'm pretty sure that's Linux's policy -- You Must Keep
> Userspace Working, but you can break it to see if anyone complains first.

Going further than this:

The proposed patch series not only changes the name, it changes the
functionality.  We do not want code to *compile* against 4.7 and then
not *work* against 4.7; and the worst of all is to compile and sort of
work but do it incorrectly.

Does the ioreq server have a way of asking Xen what version of the ABI
it's providing?  I'm assuming the answer is "no"; in which case code
that is compiled against the 4.6 interface but run on a 4.8 interface
that looks like this will fail in a somewhat unpredictable way.

Given that:

1. When we do check the ioreq server functionality in, what's the
correct way to deal with code that wants to use the old interface, and
what do we do with code compiled against the old interface but running
on the new one?
2. What's the best thing to do for this release?

If it's the case that the only code that uses this is in XenServer,
then I'd say the answer to #1 can be simply, "Don't compile" and
"Don't do that" respectively; and the answer to #2 can be either
"Leave it be" or "Remove the enum from the public interface".

If there are other projects that have started to use this interface,
then we need a better answer to #1 than "Compile but fail in
unpredicatble ways".

 -George

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