[v2,2/3] x86/ioreq server: Rename p2m_mmio_write_dm to p2m_ioreq_server

Commit Message

Yu Zhang March 31, 2016, 10:53 a.m. UTC

Previously p2m type p2m_mmio_write_dm was introduced for write-
protected memory pages whose write operations are supposed to be
forwarded to and emulated by an ioreq server. Yet limitations of
rangeset restrict the number of guest pages to be write-protected.

This patch replaces the p2m type p2m_mmio_write_dm with a new name:
p2m_ioreq_server, which means this p2m type can be claimed by one
ioreq server, instead of being tracked inside the rangeset of ioreq
server. Patches following up will add the related hvmop handling
code which map/unmap type p2m_ioreq_server to/from an ioreq server.

Signed-off-by: Paul Durrant <[e-mail]>
Signed-off-by: Yu Zhang <[e-mail]>
Cc: Keir Fraser <[e-mail]>
Cc: Jan Beulich <[e-mail]>
Cc: Andrew Cooper <[e-mail]>
Cc: Jun Nakajima <[e-mail]>
Cc: Kevin Tian <[e-mail]>
Cc: George Dunlap <[e-mail]>
Cc: Tim Deegan <[e-mail]>
---
 xen/arch/x86/hvm/hvm.c          | 10 +++++-----
 xen/arch/x86/mm/p2m-ept.c       |  2 +-
 xen/arch/x86/mm/p2m-pt.c        |  2 +-
 xen/arch/x86/mm/shadow/multi.c  |  2 +-
 xen/include/asm-x86/p2m.h       |  4 ++--
 xen/include/public/hvm/hvm_op.h |  2 +-
 6 files changed, 11 insertions(+), 11 deletions(-)

Comments

George Dunlap April 5, 2016, 2:38 p.m. UTC | #1

On Thu, Mar 31, 2016 at 11:53 AM, Yu Zhang <[e-mail]> wrote:
> Previously p2m type p2m_mmio_write_dm was introduced for write-
> protected memory pages whose write operations are supposed to be
> forwarded to and emulated by an ioreq server. Yet limitations of
> rangeset restrict the number of guest pages to be write-protected.
>
> This patch replaces the p2m type p2m_mmio_write_dm with a new name:
> p2m_ioreq_server, which means this p2m type can be claimed by one
> ioreq server, instead of being tracked inside the rangeset of ioreq
> server. Patches following up will add the related hvmop handling
> code which map/unmap type p2m_ioreq_server to/from an ioreq server.
>
> Signed-off-by: Paul Durrant <[e-mail]>
> Signed-off-by: Yu Zhang <[e-mail]>

Again, this would have been a lot easier to review if you'd said that
the only difference was to make it a straight rename, and move the
type change to the next patch.

Reviewed-by: George Dunlap <[e-mail]>

Andrew Cooper April 8, 2016, 1:26 p.m. UTC | #2

On 31/03/16 11:53, Yu Zhang wrote:
> Previously p2m type p2m_mmio_write_dm was introduced for write-
> protected memory pages whose write operations are supposed to be
> forwarded to and emulated by an ioreq server. Yet limitations of
> rangeset restrict the number of guest pages to be write-protected.
>
> This patch replaces the p2m type p2m_mmio_write_dm with a new name:
> p2m_ioreq_server, which means this p2m type can be claimed by one
> ioreq server, instead of being tracked inside the rangeset of ioreq
> server. Patches following up will add the related hvmop handling
> code which map/unmap type p2m_ioreq_server to/from an ioreq server.
>
> Signed-off-by: Paul Durrant <[e-mail]>
> Signed-off-by: Yu Zhang <[e-mail]>

Acked-by: Andrew Cooper <[e-mail]>

Jan Beulich April 8, 2016, 9:48 p.m. UTC | #3

>>> On 31.03.16 at 12:53, <[e-mail]> wrote:
> --- a/xen/include/public/hvm/hvm_op.h
> +++ b/xen/include/public/hvm/hvm_op.h
> @@ -83,7 +83,7 @@ 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 */
> -    HVMMEM_mmio_write_dm       /* Read-only; writes go to the device model */
> +    HVMMEM_ioreq_server,
>  } hvmmem_type_t;
>  
>  /* Following tools-only interfaces may change in future. */

So there's one problem here, which the comment at the bottom
of the context already hints at: This enum is part of the not
tools restricted interface (as HVMOP_get_mem_type is usable
by guests themselves), which we cannot change like this. Since
the meaning of the enumerator value doesn't change, I guess
we can get away with simply retaining its old name for non-up-
to-date __XEN_INTERFACE_VERSION__.

Jan

Paul Durrant April 18, 2016, 8:41 a.m. UTC | #4

> -----Original Message-----
> From: Jan Beulich [mailto:[e-mail]]
> Sent: 08 April 2016 22:48
> To: [e-mail]
> Cc: Andrew Cooper; Paul Durrant; George Dunlap; Jun Nakajima; Kevin Tian;
> [e-mail]; Yu Zhang; Keir (Xen.org); Tim (Xen.org)
> Subject: Re: [PATCH v2 2/3] x86/ioreq server: Rename p2m_mmio_write_dm
> to p2m_ioreq_server
> 
> >>> On 31.03.16 at 12:53, <[e-mail]> wrote:
> > --- a/xen/include/public/hvm/hvm_op.h
> > +++ b/xen/include/public/hvm/hvm_op.h
> > @@ -83,7 +83,7 @@ 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
> */
> > -    HVMMEM_mmio_write_dm       /* Read-only; writes go to the device
> model */
> > +    HVMMEM_ioreq_server,
> >  } hvmmem_type_t;
> >
> >  /* Following tools-only interfaces may change in future. */
> 
> So there's one problem here, which the comment at the bottom
> of the context already hints at: This enum is part of the not
> tools restricted interface (as HVMOP_get_mem_type is usable
> by guests themselves), which we cannot change like this. Since
> the meaning of the enumerator value doesn't change, I guess
> we can get away with simply retaining its old name for non-up-
> to-date __XEN_INTERFACE_VERSION__.
>

Has the type made it into a release yet. I was assuming we could make the change without any need to play with the version since it's only ever been present in  xen-unstable so far.

  Paul

 
> Jan

George Dunlap April 18, 2016, 9:10 a.m. UTC | #5

On Mon, Apr 18, 2016 at 9:41 AM, Paul Durrant <[e-mail]> wrote:
>> -----Original Message-----
>> From: Jan Beulich [mailto:[e-mail]]
>> Sent: 08 April 2016 22:48
>> To: [e-mail]
>> Cc: Andrew Cooper; Paul Durrant; George Dunlap; Jun Nakajima; Kevin Tian;
>> [e-mail]; Yu Zhang; Keir (Xen.org); Tim (Xen.org)
>> Subject: Re: [PATCH v2 2/3] x86/ioreq server: Rename p2m_mmio_write_dm
>> to p2m_ioreq_server
>>
>> >>> On 31.03.16 at 12:53, <[e-mail]> wrote:
>> > --- a/xen/include/public/hvm/hvm_op.h
>> > +++ b/xen/include/public/hvm/hvm_op.h
>> > @@ -83,7 +83,7 @@ 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
>> */
>> > -    HVMMEM_mmio_write_dm       /* Read-only; writes go to the device
>> model */
>> > +    HVMMEM_ioreq_server,
>> >  } hvmmem_type_t;
>> >
>> >  /* Following tools-only interfaces may change in future. */
>>
>> So there's one problem here, which the comment at the bottom
>> of the context already hints at: This enum is part of the not
>> tools restricted interface (as HVMOP_get_mem_type is usable
>> by guests themselves), which we cannot change like this. Since
>> the meaning of the enumerator value doesn't change, I guess
>> we can get away with simply retaining its old name for non-up-
>> to-date __XEN_INTERFACE_VERSION__.
>>
>
> Has the type made it into a release yet. I was assuming we could make the change without any need to play with the version since it's only ever been present in  xen-unstable so far.

I think you need a Release Manager ack for that; but if it's the case
that the type has never been seen in public, then I think it should be
able to be renamed (in fact, we should rename it so that we don't have
to deal with backwards compatibility later).

 -George

Wei Liu April 18, 2016, 9:14 a.m. UTC | #6

On Mon, Apr 18, 2016 at 10:10:00AM +0100, George Dunlap wrote:
> On Mon, Apr 18, 2016 at 9:41 AM, Paul Durrant <[e-mail]> wrote:
> >> -----Original Message-----
> >> From: Jan Beulich [mailto:[e-mail]]
> >> Sent: 08 April 2016 22:48
> >> To: [e-mail]
> >> Cc: Andrew Cooper; Paul Durrant; George Dunlap; Jun Nakajima; Kevin Tian;
> >> [e-mail]; Yu Zhang; Keir (Xen.org); Tim (Xen.org)
> >> Subject: Re: [PATCH v2 2/3] x86/ioreq server: Rename p2m_mmio_write_dm
> >> to p2m_ioreq_server
> >>
> >> >>> On 31.03.16 at 12:53, <[e-mail]> wrote:
> >> > --- a/xen/include/public/hvm/hvm_op.h
> >> > +++ b/xen/include/public/hvm/hvm_op.h
> >> > @@ -83,7 +83,7 @@ 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
> >> */
> >> > -    HVMMEM_mmio_write_dm       /* Read-only; writes go to the device
> >> model */
> >> > +    HVMMEM_ioreq_server,
> >> >  } hvmmem_type_t;
> >> >
> >> >  /* Following tools-only interfaces may change in future. */
> >>
> >> So there's one problem here, which the comment at the bottom
> >> of the context already hints at: This enum is part of the not
> >> tools restricted interface (as HVMOP_get_mem_type is usable
> >> by guests themselves), which we cannot change like this. Since
> >> the meaning of the enumerator value doesn't change, I guess
> >> we can get away with simply retaining its old name for non-up-
> >> to-date __XEN_INTERFACE_VERSION__.
> >>
> >
> > Has the type made it into a release yet. I was assuming we could make the change without any need to play with the version since it's only ever been present in  xen-unstable so far.
> 
> I think you need a Release Manager ack for that; but if it's the case
> that the type has never been seen in public, then I think it should be
> able to be renamed (in fact, we should rename it so that we don't have
> to deal with backwards compatibility later).
> 

If it is not released yet, feel free to change it -- but at this point
you also need to present argument on why it should be changed. I don't
expect the seddery too hard to review.

Wei.

>  -George

Paul Durrant April 18, 2016, 9:45 a.m. UTC | #7

> -----Original Message-----
> From: Wei Liu [mailto:[e-mail]]
> Sent: 18 April 2016 10:15
> To: George Dunlap
> Cc: Paul Durrant; Jan Beulich; [e-mail]; Kevin Tian; Keir
> (Xen.org); Andrew Cooper; Tim (Xen.org); Yu Zhang; [e-mail];
> Jun Nakajima; Wei Liu
> Subject: Re: [Xen-devel] [PATCH v2 2/3] x86/ioreq server: Rename
> p2m_mmio_write_dm to p2m_ioreq_server
> 
> On Mon, Apr 18, 2016 at 10:10:00AM +0100, George Dunlap wrote:
> > On Mon, Apr 18, 2016 at 9:41 AM, Paul Durrant <[e-mail]>
> wrote:
> > >> -----Original Message-----
> > >> From: Jan Beulich [mailto:[e-mail]]
> > >> Sent: 08 April 2016 22:48
> > >> To: [e-mail]
> > >> Cc: Andrew Cooper; Paul Durrant; George Dunlap; Jun Nakajima; Kevin
> Tian;
> > >> [e-mail]; Yu Zhang; Keir (Xen.org); Tim (Xen.org)
> > >> Subject: Re: [PATCH v2 2/3] x86/ioreq server: Rename
> p2m_mmio_write_dm
> > >> to p2m_ioreq_server
> > >>
> > >> >>> On 31.03.16 at 12:53, <[e-mail]> wrote:
> > >> > --- a/xen/include/public/hvm/hvm_op.h
> > >> > +++ b/xen/include/public/hvm/hvm_op.h
> > >> > @@ -83,7 +83,7 @@ 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
> > >> */
> > >> > -    HVMMEM_mmio_write_dm       /* Read-only; writes go to the
> device
> > >> model */
> > >> > +    HVMMEM_ioreq_server,
> > >> >  } hvmmem_type_t;
> > >> >
> > >> >  /* Following tools-only interfaces may change in future. */
> > >>
> > >> So there's one problem here, which the comment at the bottom
> > >> of the context already hints at: This enum is part of the not
> > >> tools restricted interface (as HVMOP_get_mem_type is usable
> > >> by guests themselves), which we cannot change like this. Since
> > >> the meaning of the enumerator value doesn't change, I guess
> > >> we can get away with simply retaining its old name for non-up-
> > >> to-date __XEN_INTERFACE_VERSION__.
> > >>
> > >
> > > Has the type made it into a release yet. I was assuming we could make
> the change without any need to play with the version since it's only ever
> been present in  xen-unstable so far.
> >
> > I think you need a Release Manager ack for that; but if it's the case
> > that the type has never been seen in public, then I think it should be
> > able to be renamed (in fact, we should rename it so that we don't have
> > to deal with backwards compatibility later).
> >
> 
> If it is not released yet, feel free to change it -- but at this point
> you also need to present argument on why it should be changed. I don't
> expect the seddery too hard to review.
> 

There was a design doc posted to the list a couple of months back which has the justification. See http://lists.xen.org/archives/html/xen-devel/2016-02/msg03770.html

  Paul

> Wei.
> 
> >  -George

Jan Beulich April 18, 2016, 4:40 p.m. UTC | #8

>>> Paul Durrant <[e-mail]> 04/18/16 10:41 AM >>>
> From: Jan Beulich [mailto:[e-mail]]
> Sent: 08 April 2016 22:48
>> >>> On 31.03.16 at 12:53, <[e-mail]> wrote:
>> > --- a/xen/include/public/hvm/hvm_op.h
>> > +++ b/xen/include/public/hvm/hvm_op.h
>> > @@ -83,7 +83,7 @@ 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
>> */
>> > -    HVMMEM_mmio_write_dm       /* Read-only; writes go to the device
>> model */
>> > +    HVMMEM_ioreq_server,
>> >  } hvmmem_type_t;
>> >
>> >  /* Following tools-only interfaces may change in future. */
>> 
>> So there's one problem here, which the comment at the bottom
>> of the context already hints at: This enum is part of the not
>> tools restricted interface (as HVMOP_get_mem_type is usable
>> by guests themselves), which we cannot change like this. Since
>> the meaning of the enumerator value doesn't change, I guess
>> we can get away with simply retaining its old name for non-up-
>> to-date __XEN_INTERFACE_VERSION__.
>
>Has the type made it into a release yet. I was assuming we could make the change without any need to play with the version since it's only ever been present in  >xen-unstable so far.

  Oh, I didn't realize this got added only after 4.6. If that was the case, then the
change of course could be done without any conditional. Checking ... No, 4.6.1
has it.

As for any of this going in now - I suppose this would need a freeze exception,
which I think we've meant to avoid as much as possible this time round.

Jan

Paul Durrant April 18, 2016, 4:45 p.m. UTC | #9

> -----Original Message-----
> From: Jan Beulich [mailto:[e-mail]]
> Sent: 18 April 2016 17:40
> To: Paul Durrant
> Cc: Andrew Cooper; George Dunlap; [e-mail]; Kevin Tian;
> [e-mail]; [e-mail]; [e-mail];
> Keir (Xen.org); Tim (Xen.org)
> Subject: Re: RE: [PATCH v2 2/3] x86/ioreq server: Rename
> p2m_mmio_write_dm to p2m_ioreq_server
> 
> >>> Paul Durrant <[e-mail]> 04/18/16 10:41 AM >>>
> > From: Jan Beulich [mailto:[e-mail]]
> > Sent: 08 April 2016 22:48
> >> >>> On 31.03.16 at 12:53, <[e-mail]> wrote:
> >> > --- a/xen/include/public/hvm/hvm_op.h
> >> > +++ b/xen/include/public/hvm/hvm_op.h
> >> > @@ -83,7 +83,7 @@ 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
> >> */
> >> > -    HVMMEM_mmio_write_dm       /* Read-only; writes go to the device
> >> model */
> >> > +    HVMMEM_ioreq_server,
> >> >  } hvmmem_type_t;
> >> >
> >> >  /* Following tools-only interfaces may change in future. */
> >>
> >> So there's one problem here, which the comment at the bottom
> >> of the context already hints at: This enum is part of the not
> >> tools restricted interface (as HVMOP_get_mem_type is usable
> >> by guests themselves), which we cannot change like this. Since
> >> the meaning of the enumerator value doesn't change, I guess
> >> we can get away with simply retaining its old name for non-up-
> >> to-date __XEN_INTERFACE_VERSION__.
> >
> >Has the type made it into a release yet. I was assuming we could make the
> change without any need to play with the version since it's only ever been
> present in  >xen-unstable so far.
> 
>   Oh, I didn't realize this got added only after 4.6. If that was the case, then
> the
> change of course could be done without any conditional. Checking ... No,
> 4.6.1
> has it.
> 

Damn. It needs to be ifdef-ed then :-(

> As for any of this going in now - I suppose this would need a freeze
> exception,
> which I think we've meant to avoid as much as possible this time round.
> 

The original patch was posted before the cut-off IIRC so I'm not sure of the policy regarding freeze-exceptions.

  Paul

> Jan

Jan Beulich April 18, 2016, 4:47 p.m. UTC | #10

>>> Paul Durrant <[e-mail]> 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.

Jan

Paul Durrant April 18, 2016, 4:58 p.m. UTC | #11

> -----Original Message-----

> From: Xen-devel [mailto:[e-mail]] 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; [e-mail]; [e-mail];

> [e-mail]; [e-mail]

> Subject: Re: [Xen-devel] [PATCH v2 2/3] x86/ioreq server: Rename

> p2m_mmio_write_dm to p2m_ioreq_server

> 

> >>> Paul Durrant <[e-mail]> 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.

  Paul

> Jan

> 

> 

> _______________________________________________

> Xen-devel mailing list

> [e-mail]

> http://lists.xen.org/xen-devel

Yu Zhang April 19, 2016, 11:02 a.m. UTC | #12

On 4/19/2016 12:58 AM, Paul Durrant wrote:
>> -----Original Message-----
>> From: Xen-devel [mailto:[e-mail]] 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; [e-mail]; [e-mail];
>> [e-mail]; [e-mail]
>> Subject: Re: [Xen-devel] [PATCH v2 2/3] x86/ioreq server: Rename
>> p2m_mmio_write_dm to p2m_ioreq_server
>>
>>>>> Paul Durrant <[e-mail]> 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.

Thanks
Yu

>    Paul
>
>> Jan
>>
>>
>> _______________________________________________
>> Xen-devel mailing list
>> [e-mail]
>> http://lists.xen.org/xen-devel
> _______________________________________________
> Xen-devel mailing list
> [e-mail]
> http://lists.xen.org/xen-devel
>

Paul Durrant April 19, 2016, 11:15 a.m. UTC | #13

> -----Original Message-----

> From: Yu, Zhang [mailto:[e-mail]]

> Sent: 19 April 2016 12:03

> To: Paul Durrant; Jan Beulich; Wei Liu

> Cc: Kevin Tian; Keir (Xen.org); Andrew Cooper; Tim (Xen.org); George

> Dunlap; [e-mail]; [e-mail];

> [e-mail]

> Subject: Re: [Xen-devel] [PATCH v2 2/3] x86/ioreq server: Rename

> p2m_mmio_write_dm to p2m_ioreq_server

> 

> 

> 

> On 4/19/2016 12:58 AM, Paul Durrant wrote:

> >> -----Original Message-----

> >> From: Xen-devel [mailto:[e-mail]] 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; [e-mail]; [e-mail];

> >> [e-mail]; [e-mail]

> >> Subject: Re: [Xen-devel] [PATCH v2 2/3] x86/ioreq server: Rename

> >> p2m_mmio_write_dm to p2m_ioreq_server

> >>

> >>>>> Paul Durrant <[e-mail]> 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.


I talked to Wei earlier and he is happy to give a freeze exception to this change.

  Paul

> 

> Thanks

> Yu

> 

> >    Paul

> >

> >> Jan

> >>

> >>

> >> _______________________________________________

> >> Xen-devel mailing list

> >> [e-mail]

> >> http://lists.xen.org/xen-devel

> > _______________________________________________

> > Xen-devel mailing list

> > [e-mail]

> > http://lists.xen.org/xen-devel

> >

Yu Zhang April 19, 2016, 11:38 a.m. UTC | #14

On 4/19/2016 7:15 PM, Paul Durrant wrote:
>> -----Original Message-----
>> From: Yu, Zhang [mailto:[e-mail]]
>> Sent: 19 April 2016 12:03
>> To: Paul Durrant; Jan Beulich; Wei Liu
>> Cc: Kevin Tian; Keir (Xen.org); Andrew Cooper; Tim (Xen.org); George
>> Dunlap; [e-mail]; [e-mail];
>> [e-mail]
>> Subject: Re: [Xen-devel] [PATCH v2 2/3] x86/ioreq server: Rename
>> p2m_mmio_write_dm to p2m_ioreq_server
>>
>>
>>
>> On 4/19/2016 12:58 AM, Paul Durrant wrote:
>>>> -----Original Message-----
>>>> From: Xen-devel [mailto:[e-mail]] 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; [e-mail]; [e-mail];
>>>> [e-mail]; [e-mail]
>>>> Subject: Re: [Xen-devel] [PATCH v2 2/3] x86/ioreq server: Rename
>>>> p2m_mmio_write_dm to p2m_ioreq_server
>>>>
>>>>>>> Paul Durrant <[e-mail]> 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.
>
> I talked to Wei earlier and he is happy to give a freeze exception to this change.
>

Great! I really obliged. :)
BTW, Does some form of application need to be submitted? I'm not
familiar with the procedure.

Yu
>    Paul
>
>>
>> Thanks
>> Yu
>>
>>>     Paul
>>>
>>>> Jan
>>>>
>>>>
>>>> _______________________________________________
>>>> Xen-devel mailing list
>>>> [e-mail]
>>>> http://lists.xen.org/xen-devel
>>> _______________________________________________
>>> Xen-devel mailing list
>>> [e-mail]
>>> http://lists.xen.org/xen-devel
>>>

Paul Durrant April 19, 2016, 11:50 a.m. UTC | #15

> -----Original Message-----

> From: Yu, Zhang [mailto:[e-mail]]

> Sent: 19 April 2016 12:39

> To: Paul Durrant; Jan Beulich; Wei Liu

> Cc: Kevin Tian; Keir (Xen.org); Andrew Cooper; Tim (Xen.org); George

> Dunlap; [e-mail]; [e-mail];

> [e-mail]

> Subject: Re: [Xen-devel] [PATCH v2 2/3] x86/ioreq server: Rename

> p2m_mmio_write_dm to p2m_ioreq_server

> 

> 

> 

> On 4/19/2016 7:15 PM, Paul Durrant wrote:

> >> -----Original Message-----

> >> From: Yu, Zhang [mailto:[e-mail]]

> >> Sent: 19 April 2016 12:03

> >> To: Paul Durrant; Jan Beulich; Wei Liu

> >> Cc: Kevin Tian; Keir (Xen.org); Andrew Cooper; Tim (Xen.org); George

> >> Dunlap; [e-mail]; [e-mail];

> >> [e-mail]

> >> Subject: Re: [Xen-devel] [PATCH v2 2/3] x86/ioreq server: Rename

> >> p2m_mmio_write_dm to p2m_ioreq_server

> >>

> >>

> >>

> >> On 4/19/2016 12:58 AM, Paul Durrant wrote:

> >>>> -----Original Message-----

> >>>> From: Xen-devel [mailto:[e-mail]] 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; [e-mail]; [e-mail];

> >>>> [e-mail]; [e-mail]

> >>>> Subject: Re: [Xen-devel] [PATCH v2 2/3] x86/ioreq server: Rename

> >>>> p2m_mmio_write_dm to p2m_ioreq_server

> >>>>

> >>>>>>> Paul Durrant <[e-mail]> 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.

> >

> > I talked to Wei earlier and he is happy to give a freeze exception to this

> change.

> >

> 

> Great! I really obliged. :)

> BTW, Does some form of application need to be submitted? I'm not

> familiar with the procedure.


Nor am I. I would expect that you should submit the patch with something like "PATCH for 4.7" in the subject.

  Paul

> 

> Yu

> >    Paul

> >

> >>

> >> Thanks

> >> Yu

> >>

> >>>     Paul

> >>>

> >>>> Jan

> >>>>

> >>>>

> >>>> _______________________________________________

> >>>> Xen-devel mailing list

> >>>> [e-mail]

> >>>> http://lists.xen.org/xen-devel

> >>> _______________________________________________

> >>> Xen-devel mailing list

> >>> [e-mail]

> >>> http://lists.xen.org/xen-devel

> >>>

Jan Beulich April 19, 2016, 4:51 p.m. UTC | #16

>>> "Yu, Zhang" <[e-mail]> 04/19/16 1:46 PM >>>
>On 4/19/2016 7:15 PM, Paul Durrant wrote:
>> I talked to Wei earlier and he is happy to give a freeze exception to this change.
>
>Great! I really obliged. :)
>BTW, Does some form of application need to be submitted? I'm not
>familiar with the procedure.

To add some clarification (hopefully; Wei, please correct me if I'm wrong): The
exception so far was granted only for this one patch, not the entire series.
Hence it would help if in the re-submission (with the I think single pending
change needed to this particular patch) you'd put this first in the series (or
submit it on its own). And make sure you Cc Wei in addition to any of the
people you're normally expected to Cc.

Jan

Wei Liu April 20, 2016, 2:59 p.m. UTC | #17

On Tue, Apr 19, 2016 at 10:51:14AM -0600, Jan Beulich wrote:
> >>> "Yu, Zhang" <[e-mail]> 04/19/16 1:46 PM >>>
> >On 4/19/2016 7:15 PM, Paul Durrant wrote:
> >> I talked to Wei earlier and he is happy to give a freeze exception to this change.
> >
> >Great! I really obliged. :)
> >BTW, Does some form of application need to be submitted? I'm not
> >familiar with the procedure.
> 
> To add some clarification (hopefully; Wei, please correct me if I'm wrong): The
> exception so far was granted only for this one patch, not the entire series.
> Hence it would help if in the re-submission (with the I think single pending
> change needed to this particular patch) you'd put this first in the series (or
> submit it on its own). And make sure you Cc Wei in addition to any of the
> people you're normally expected to Cc.

Hmm... I had the impression that this is the only patch needed and the
enum wasn't released, but I was wrong. I will go over this series in
detailed and let you know.

Wei.

> 
> Jan
>

George Dunlap April 20, 2016, 3:02 p.m. UTC | #18

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:[e-mail]] 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; [e-mail]; [e-mail];
>>> [e-mail]; [e-mail]
>>> Subject: Re: [Xen-devel] [PATCH v2 2/3] x86/ioreq server: Rename
>>> p2m_mmio_write_dm to p2m_ioreq_server
>>>
>>>>>> Paul Durrant <[e-mail]> 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.

 -George

George Dunlap April 20, 2016, 4:30 p.m. UTC | #19

On Wed, Apr 20, 2016 at 4:02 PM, George Dunlap <[e-mail]> 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:[e-mail]] 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; [e-mail]; [e-mail];
>>>> [e-mail]; [e-mail]
>>>> Subject: Re: [Xen-devel] [PATCH v2 2/3] x86/ioreq server: Rename
>>>> p2m_mmio_write_dm to p2m_ioreq_server
>>>>
>>>>>>> Paul Durrant <[e-mail]> 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

Jan Beulich April 20, 2016, 4:52 p.m. UTC | #20

>>> George Dunlap <[e-mail]> 04/20/16 6:30 PM >>>
>On Wed, Apr 20, 2016 at 4:02 PM, George Dunlap <[e-mail]> wrote:
>> On 19/04/16 12:02, Yu, Zhang wrote:
>>> 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.

We don't normally do it like that - we aim at keeping things compatible
right away. I don't know of a case where we would have knowingly broken
compatibility for users of the public headers (leaving aside tool stack only
stuff of course).

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

I had the impression that the renaming patch was what it is - a renaming
patch, without altering behavior.

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

The only thing it can do is ask for the Xen version. The ABI version is not
being returned by anything (but perhaps should be).

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

For the full series I'm not sure I can really tell.But as said, for the rename
patch alone I thought it is just a rename. And that's what we want to get
in (see Paul's earlier reply - he wants to see the old name gone, so it won't
be used any further).

>2. What's the best thing to do for this release?

If the entire series (no matter whether to go in now or later) is changing
behavior, then the only choice is to consider the currently used enum
value burnt, and use a fresh one for the new semantics.

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

How would we know whether there are other users?

Jan

Paul Durrant April 20, 2016, 4:58 p.m. UTC | #21

> -----Original Message-----

> From: Xen-devel [mailto:[e-mail]] On Behalf Of Jan

> Beulich

> Sent: 20 April 2016 17:53

> To: George Dunlap; Paul Durrant; Wei Liu; [e-mail]

> Cc: Kevin Tian; Keir (Xen.org); Andrew Cooper; Tim (Xen.org); xen-

> [e-mail]; [e-mail]; [e-mail]

> Subject: Re: [Xen-devel] [PATCH v2 2/3] x86/ioreq server: Rename

> p2m_mmio_write_dm to p2m_ioreq_server

> 

> >>> George Dunlap <[e-mail]> 04/20/16 6:30 PM >>>

> >On Wed, Apr 20, 2016 at 4:02 PM, George Dunlap

> <[e-mail]> wrote:

> >> On 19/04/16 12:02, Yu, Zhang wrote:

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

> 

> We don't normally do it like that - we aim at keeping things compatible

> right away. I don't know of a case where we would have knowingly broken

> compatibility for users of the public headers (leaving aside tool stack only

> stuff of course).

> 

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

> 

> I had the impression that the renaming patch was what it is - a renaming

> patch, without altering behavior.

> 

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

> 

> The only thing it can do is ask for the Xen version. The ABI version is not

> being returned by anything (but perhaps should be).

> 

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

> 

> For the full series I'm not sure I can really tell.But as said, for the rename

> patch alone I thought it is just a rename. And that's what we want to get

> in (see Paul's earlier reply - he wants to see the old name gone, so it won't

> be used any further).

> 

> >2. What's the best thing to do for this release?

> 

> If the entire series (no matter whether to go in now or later) is changing

> behavior, then the only choice is to consider the currently used enum

> value burnt, and use a fresh one for the new semantics.


It sounds like that would be best way. If we don't so that then we have to maintain the write-dm semantics for pages of that type unless the type is claimed (by using the new hypercall) and that's bit icky. I much prefer that pages of the new type are treated as RAM until claimed.

  Paul

> 

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

> 

> How would we know whether there are other users?

> 

> Jan

> 

> 

> _______________________________________________

> Xen-devel mailing list

> [e-mail]

> http://lists.xen.org/xen-devel

George Dunlap April 20, 2016, 5:06 p.m. UTC | #22

On 20/04/16 17:58, Paul Durrant wrote:
>> -----Original Message-----
>> From: Xen-devel [mailto:[e-mail]] On Behalf Of Jan
>> Beulich
>> Sent: 20 April 2016 17:53
>> To: George Dunlap; Paul Durrant; Wei Liu; [e-mail]
>> Cc: Kevin Tian; Keir (Xen.org); Andrew Cooper; Tim (Xen.org); xen-
>> [e-mail]; [e-mail]; [e-mail]
>> Subject: Re: [Xen-devel] [PATCH v2 2/3] x86/ioreq server: Rename
>> p2m_mmio_write_dm to p2m_ioreq_server
>>
>>>>> George Dunlap <[e-mail]> 04/20/16 6:30 PM >>>
>>> On Wed, Apr 20, 2016 at 4:02 PM, George Dunlap
>> <[e-mail]> wrote:
>>>> On 19/04/16 12:02, Yu, Zhang wrote:
>>>>> 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.
>>
>> We don't normally do it like that - we aim at keeping things compatible
>> right away. I don't know of a case where we would have knowingly broken
>> compatibility for users of the public headers (leaving aside tool stack only
>> stuff of course).
>>
>>> 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.
>>
>> I had the impression that the renaming patch was what it is - a renaming
>> patch, without altering behavior.
>>
>>> 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.
>>
>> The only thing it can do is ask for the Xen version. The ABI version is not
>> being returned by anything (but perhaps should be).
>>
>>> 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?
>>
>> For the full series I'm not sure I can really tell.But as said, for the rename
>> patch alone I thought it is just a rename. And that's what we want to get
>> in (see Paul's earlier reply - he wants to see the old name gone, so it won't
>> be used any further).
>>
>>> 2. What's the best thing to do for this release?
>>
>> If the entire series (no matter whether to go in now or later) is changing
>> behavior, then the only choice is to consider the currently used enum
>> value burnt, and use a fresh one for the new semantics.
> 
> It sounds like that would be best way. If we don't so that then we have to maintain the write-dm semantics for pages of that type unless the type is claimed (by using the new hypercall) and that's bit icky. I much prefer that pages of the new type are treated as RAM until claimed.

I think the only sensible way to keep the enum is to also keep the
functionality, which would mean using *another* p2m type for ioreq_server.

Given that the functionality isn't going away for 4.7, I don't see an
urgent need to remove the enum; but if Paul does, then a patch renaming
it to HVMMEM_unused would be the way forward then I guess.  Once the
underlying p2m type goes away, you'll want to return -EINVAL for this
enum value.

 -George

George Dunlap April 20, 2016, 5:08 p.m. UTC | #23

On Wed, Apr 20, 2016 at 5:52 PM, Jan Beulich <[e-mail]> wrote:
>>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".
>
> How would we know whether there are other users?

Well we'd wait for them to complain... at which point we would have
had a release with an *incompatible* ABI and no way to change it.

So yes, upon consideration, burning the enum value is really the only
option. :-)

 -George

Paul Durrant April 20, 2016, 5:09 p.m. UTC | #24

> -----Original Message-----

> From: George Dunlap [mailto:[e-mail]]

> Sent: 20 April 2016 18:07

> To: Paul Durrant; Jan Beulich; Wei Liu; [e-mail]

> Cc: Kevin Tian; Keir (Xen.org); Andrew Cooper; Tim (Xen.org); xen-

> [e-mail]; [e-mail]; [e-mail]

> Subject: Re: [Xen-devel] [PATCH v2 2/3] x86/ioreq server: Rename

> p2m_mmio_write_dm to p2m_ioreq_server

> 

> On 20/04/16 17:58, Paul Durrant wrote:

> >> -----Original Message-----

> >> From: Xen-devel [mailto:[e-mail]] On Behalf Of

> Jan

> >> Beulich

> >> Sent: 20 April 2016 17:53

> >> To: George Dunlap; Paul Durrant; Wei Liu; [e-mail]

> >> Cc: Kevin Tian; Keir (Xen.org); Andrew Cooper; Tim (Xen.org); xen-

> >> [e-mail]; [e-mail]; [e-mail]

> >> Subject: Re: [Xen-devel] [PATCH v2 2/3] x86/ioreq server: Rename

> >> p2m_mmio_write_dm to p2m_ioreq_server

> >>

> >>>>> George Dunlap <[e-mail]> 04/20/16 6:30 PM >>>

> >>> On Wed, Apr 20, 2016 at 4:02 PM, George Dunlap

> >> <[e-mail]> wrote:

> >>>> On 19/04/16 12:02, Yu, Zhang wrote:

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

> >>

> >> We don't normally do it like that - we aim at keeping things compatible

> >> right away. I don't know of a case where we would have knowingly

> broken

> >> compatibility for users of the public headers (leaving aside tool stack only

> >> stuff of course).

> >>

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

> >>

> >> I had the impression that the renaming patch was what it is - a renaming

> >> patch, without altering behavior.

> >>

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

> >>

> >> The only thing it can do is ask for the Xen version. The ABI version is not

> >> being returned by anything (but perhaps should be).

> >>

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

> >>

> >> For the full series I'm not sure I can really tell.But as said, for the rename

> >> patch alone I thought it is just a rename. And that's what we want to get

> >> in (see Paul's earlier reply - he wants to see the old name gone, so it won't

> >> be used any further).

> >>

> >>> 2. What's the best thing to do for this release?

> >>

> >> If the entire series (no matter whether to go in now or later) is changing

> >> behavior, then the only choice is to consider the currently used enum

> >> value burnt, and use a fresh one for the new semantics.

> >

> > It sounds like that would be best way. If we don't so that then we have to

> maintain the write-dm semantics for pages of that type unless the type is

> claimed (by using the new hypercall) and that's bit icky. I much prefer that

> pages of the new type are treated as RAM until claimed.

> 

> I think the only sensible way to keep the enum is to also keep the

> functionality, which would mean using *another* p2m type for ioreq_server.

> 

> Given that the functionality isn't going away for 4.7, I don't see an

> urgent need to remove the enum; but if Paul does, then a patch renaming

> it to HVMMEM_unused would be the way forward then I guess.  Once the

> underlying p2m type goes away, you'll want to return -EINVAL for this

> enum value.

> 


Since the old semantics made it into the wild in 4.6.1 (which I was unaware of until a couple of days ago) then I guess we are going to need some form of deprecation like this.

  Paul

>  -George

Yu Zhang April 21, 2016, 12:04 p.m. UTC | #25

maintainers,

  Thanks for the discussion and sorry for my delayed reply...

On 4/21/2016 12:52 AM, Jan Beulich wrote:
>>>> George Dunlap <[e-mail]> 04/20/16 6:30 PM >>>
>> On Wed, Apr 20, 2016 at 4:02 PM, George Dunlap <[e-mail]> wrote:
>>> On 19/04/16 12:02, Yu, Zhang wrote:
>>>> 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.
>
> We don't normally do it like that - we aim at keeping things compatible
> right away. I don't know of a case where we would have knowingly broken
> compatibility for users of the public headers (leaving aside tool stack only
> stuff of course).
>
>> 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.
>
> I had the impression that the renaming patch was what it is - a renaming
> patch, without altering behavior.
>
>> 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.
>
> The only thing it can do is ask for the Xen version. The ABI version is not
> being returned by anything (but perhaps should be).
>
>> 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?
>
> For the full series I'm not sure I can really tell.But as said, for the rename
> patch alone I thought it is just a rename. And that's what we want to get
> in (see Paul's earlier reply - he wants to see the old name gone, so it won't
> be used any further).
>
>> 2. What's the best thing to do for this release?
>
> If the entire series (no matter whether to go in now or later) is changing
> behavior, then the only choice is to consider the currently used enum
> value burnt, and use a fresh one for the new semantics.
>

The 3rd patch in this V2 series do change the semantics of this type.

But IIUC, we have reached an agreement in the discussion of patch 3
that for now, we only support the write emulation, and it shall be no
p2m_ioreq_server entries before an ioreq server claimed its ownership.
So in my V3 series(nearly finished, not sent out yet), this type shall
have the same semantics with the old one.

Besides, I'm sorry, and I do not quite understand what "consider the
currently used enum value burnt" means. Any example? :)

>> 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".
>
> How would we know whether there are other users?
>

B.R.
Yu

Yu Zhang April 21, 2016, 12:24 p.m. UTC | #26

On 4/21/2016 1:06 AM, George Dunlap wrote:
> On 20/04/16 17:58, Paul Durrant wrote:
>>> -----Original Message-----
>>> From: Xen-devel [mailto:[e-mail]] On Behalf Of Jan
>>> Beulich
>>> Sent: 20 April 2016 17:53
>>> To: George Dunlap; Paul Durrant; Wei Liu; [e-mail]
>>> Cc: Kevin Tian; Keir (Xen.org); Andrew Cooper; Tim (Xen.org); xen-
>>> [e-mail]; [e-mail]; [e-mail]
>>> Subject: Re: [Xen-devel] [PATCH v2 2/3] x86/ioreq server: Rename
>>> p2m_mmio_write_dm to p2m_ioreq_server
>>>
>>>>>> George Dunlap <[e-mail]> 04/20/16 6:30 PM >>>
>>>> On Wed, Apr 20, 2016 at 4:02 PM, George Dunlap
>>> <[e-mail]> wrote:
>>>>> On 19/04/16 12:02, Yu, Zhang wrote:
>>>>>> 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.
>>>
>>> We don't normally do it like that - we aim at keeping things compatible
>>> right away. I don't know of a case where we would have knowingly broken
>>> compatibility for users of the public headers (leaving aside tool stack only
>>> stuff of course).
>>>
>>>> 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.
>>>
>>> I had the impression that the renaming patch was what it is - a renaming
>>> patch, without altering behavior.
>>>
>>>> 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.
>>>
>>> The only thing it can do is ask for the Xen version. The ABI version is not
>>> being returned by anything (but perhaps should be).
>>>
>>>> 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?
>>>
>>> For the full series I'm not sure I can really tell.But as said, for the rename
>>> patch alone I thought it is just a rename. And that's what we want to get
>>> in (see Paul's earlier reply - he wants to see the old name gone, so it won't
>>> be used any further).
>>>
>>>> 2. What's the best thing to do for this release?
>>>
>>> If the entire series (no matter whether to go in now or later) is changing
>>> behavior, then the only choice is to consider the currently used enum
>>> value burnt, and use a fresh one for the new semantics.
>>
>> It sounds like that would be best way. If we don't so that then we have to maintain the write-dm semantics for pages of that type unless the type is claimed (by using the new hypercall) and that's bit icky. I much prefer that pages of the new type are treated as RAM until claimed.
>
> I think the only sensible way to keep the enum is to also keep the
> functionality, which would mean using *another* p2m type for ioreq_server.
>
> Given that the functionality isn't going away for 4.7, I don't see an
> urgent need to remove the enum; but if Paul does, then a patch renaming
> it to HVMMEM_unused would be the way forward then I guess.  Once the
> underlying p2m type goes away, you'll want to return -EINVAL for this
> enum value.
>

So the enum would be sth. 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_mmio_write_dm, /* Read-only; writes go to the device model */
#else
     HVMMEM_unused,
#endif
     HVMMEM_ioreq_server
} hvmmem_type_t;

Thanks
Yu

Paul Durrant April 21, 2016, 1:31 p.m. UTC | #27

> -----Original Message-----

> From: Yu, Zhang [mailto:[e-mail]]

> Sent: 21 April 2016 13:25

> To: George Dunlap; Paul Durrant; Jan Beulich; Wei Liu

> Cc: Kevin Tian; Keir (Xen.org); Andrew Cooper; Tim (Xen.org); xen-

> [e-mail]; [e-mail]; [e-mail]

> Subject: Re: [Xen-devel] [PATCH v2 2/3] x86/ioreq server: Rename

> p2m_mmio_write_dm to p2m_ioreq_server

> 

> 

> 

> On 4/21/2016 1:06 AM, George Dunlap wrote:

> > On 20/04/16 17:58, Paul Durrant wrote:

> >>> -----Original Message-----

> >>> From: Xen-devel [mailto:[e-mail]] On Behalf

> Of Jan

> >>> Beulich

> >>> Sent: 20 April 2016 17:53

> >>> To: George Dunlap; Paul Durrant; Wei Liu; [e-mail]

> >>> Cc: Kevin Tian; Keir (Xen.org); Andrew Cooper; Tim (Xen.org); xen-

> >>> [e-mail]; [e-mail]; [e-mail]

> >>> Subject: Re: [Xen-devel] [PATCH v2 2/3] x86/ioreq server: Rename

> >>> p2m_mmio_write_dm to p2m_ioreq_server

> >>>

> >>>>>> George Dunlap <[e-mail]> 04/20/16 6:30 PM >>>

> >>>> On Wed, Apr 20, 2016 at 4:02 PM, George Dunlap

> >>> <[e-mail]> wrote:

> >>>>> On 19/04/16 12:02, Yu, Zhang wrote:

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

> >>>

> >>> We don't normally do it like that - we aim at keeping things compatible

> >>> right away. I don't know of a case where we would have knowingly

> broken

> >>> compatibility for users of the public headers (leaving aside tool stack only

> >>> stuff of course).

> >>>

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

> >>>

> >>> I had the impression that the renaming patch was what it is - a renaming

> >>> patch, without altering behavior.

> >>>

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

> >>>

> >>> The only thing it can do is ask for the Xen version. The ABI version is not

> >>> being returned by anything (but perhaps should be).

> >>>

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

> >>>

> >>> For the full series I'm not sure I can really tell.But as said, for the rename

> >>> patch alone I thought it is just a rename. And that's what we want to get

> >>> in (see Paul's earlier reply - he wants to see the old name gone, so it

> won't

> >>> be used any further).

> >>>

> >>>> 2. What's the best thing to do for this release?

> >>>

> >>> If the entire series (no matter whether to go in now or later) is changing

> >>> behavior, then the only choice is to consider the currently used enum

> >>> value burnt, and use a fresh one for the new semantics.

> >>

> >> It sounds like that would be best way. If we don't so that then we have to

> maintain the write-dm semantics for pages of that type unless the type is

> claimed (by using the new hypercall) and that's bit icky. I much prefer that

> pages of the new type are treated as RAM until claimed.

> >

> > I think the only sensible way to keep the enum is to also keep the

> > functionality, which would mean using *another* p2m type for

> ioreq_server.

> >

> > Given that the functionality isn't going away for 4.7, I don't see an

> > urgent need to remove the enum; but if Paul does, then a patch renaming

> > it to HVMMEM_unused would be the way forward then I guess.  Once the

> > underlying p2m type goes away, you'll want to return -EINVAL for this

> > enum value.

> >

> 

> So the enum would be sth. 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_mmio_write_dm, /* Read-only; writes go to the device model

> */

> #else

>      HVMMEM_unused,

> #endif

>      HVMMEM_ioreq_server

> } hvmmem_type_t;

> 


I believe that's correct, but presumably there's need to be a change to the hypervisor since any reference there to HVMMEM_mmio_write_dm (which I think is limited to the get and set mem type code in hvm.c) will now need to map HVMMEM_unused to the old p2m_mmio_write_dm type.

  Paul

> Thanks

> Yu

Yu Zhang April 21, 2016, 1:48 p.m. UTC | #28

On 4/21/2016 9:31 PM, Paul Durrant wrote:
>> -----Original Message-----
>> From: Yu, Zhang [mailto:[e-mail]]
>> Sent: 21 April 2016 13:25
>> To: George Dunlap; Paul Durrant; Jan Beulich; Wei Liu
>> Cc: Kevin Tian; Keir (Xen.org); Andrew Cooper; Tim (Xen.org); xen-
>> [e-mail]; [e-mail]; [e-mail]
>> Subject: Re: [Xen-devel] [PATCH v2 2/3] x86/ioreq server: Rename
>> p2m_mmio_write_dm to p2m_ioreq_server
>>
>>
>>
>> On 4/21/2016 1:06 AM, George Dunlap wrote:
>>> On 20/04/16 17:58, Paul Durrant wrote:
>>>>> -----Original Message-----
>>>>> From: Xen-devel [mailto:[e-mail]] On Behalf
>> Of Jan
>>>>> Beulich
>>>>> Sent: 20 April 2016 17:53
>>>>> To: George Dunlap; Paul Durrant; Wei Liu; [e-mail]
>>>>> Cc: Kevin Tian; Keir (Xen.org); Andrew Cooper; Tim (Xen.org); xen-
>>>>> [e-mail]; [e-mail]; [e-mail]
>>>>> Subject: Re: [Xen-devel] [PATCH v2 2/3] x86/ioreq server: Rename
>>>>> p2m_mmio_write_dm to p2m_ioreq_server
>>>>>
>>>>>>>> George Dunlap <[e-mail]> 04/20/16 6:30 PM >>>
>>>>>> On Wed, Apr 20, 2016 at 4:02 PM, George Dunlap
>>>>> <[e-mail]> wrote:
>>>>>>> On 19/04/16 12:02, Yu, Zhang wrote:
>>>>>>>> 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.
>>>>>
>>>>> We don't normally do it like that - we aim at keeping things compatible
>>>>> right away. I don't know of a case where we would have knowingly
>> broken
>>>>> compatibility for users of the public headers (leaving aside tool stack only
>>>>> stuff of course).
>>>>>
>>>>>> 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.
>>>>>
>>>>> I had the impression that the renaming patch was what it is - a renaming
>>>>> patch, without altering behavior.
>>>>>
>>>>>> 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.
>>>>>
>>>>> The only thing it can do is ask for the Xen version. The ABI version is not
>>>>> being returned by anything (but perhaps should be).
>>>>>
>>>>>> 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?
>>>>>
>>>>> For the full series I'm not sure I can really tell.But as said, for the rename
>>>>> patch alone I thought it is just a rename. And that's what we want to get
>>>>> in (see Paul's earlier reply - he wants to see the old name gone, so it
>> won't
>>>>> be used any further).
>>>>>
>>>>>> 2. What's the best thing to do for this release?
>>>>>
>>>>> If the entire series (no matter whether to go in now or later) is changing
>>>>> behavior, then the only choice is to consider the currently used enum
>>>>> value burnt, and use a fresh one for the new semantics.
>>>>
>>>> It sounds like that would be best way. If we don't so that then we have to
>> maintain the write-dm semantics for pages of that type unless the type is
>> claimed (by using the new hypercall) and that's bit icky. I much prefer that
>> pages of the new type are treated as RAM until claimed.
>>>
>>> I think the only sensible way to keep the enum is to also keep the
>>> functionality, which would mean using *another* p2m type for
>> ioreq_server.
>>>
>>> Given that the functionality isn't going away for 4.7, I don't see an
>>> urgent need to remove the enum; but if Paul does, then a patch renaming
>>> it to HVMMEM_unused would be the way forward then I guess.  Once the
>>> underlying p2m type goes away, you'll want to return -EINVAL for this
>>> enum value.
>>>
>>
>> So the enum would be sth. 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_mmio_write_dm, /* Read-only; writes go to the device model
>> */
>> #else
>>      HVMMEM_unused,
>> #endif
>>      HVMMEM_ioreq_server
>> } hvmmem_type_t;
>>
>
> I believe that's correct, but presumably there's need to be a change to the hypervisor since any reference there to HVMMEM_mmio_write_dm (which I think is limited to the get and set mem type code in hvm.c) will now need to map HVMMEM_unused to the old p2m_mmio_write_dm type.
>
Thank you, Paul.

But p2m_mmio_write_dm will not exist any more...
E.g. if in hvmop_get_mem_type(), if type 0xf(p2m_ioreq_server) is
returned, we could just return HVMMEM_ioreq_server. No need to
worry about the HVMMEM_mmio_write_dm.

Maybe we only need to change the beginning of hvmop_set_mem_type()
to sth. like this:

/* Interface types to internal p2m types */
static const p2m_type_t memtype[] = {
     [HVMMEM_ram_rw]  = p2m_ram_rw,
     [HVMMEM_ram_ro]  = p2m_ram_ro,
     [HVMMEM_mmio_dm] = p2m_mmio_dm,
     [HVMMEM_unused] = p2m_invalid,  /* this will be rejected later */
     [HVMMEM_ioreq_server] = p2m_ioreq_server
};
and later in the same routine, just reject the HVMMEM_unused type, in
an if(with unlikely) statement.

>   Paul

B.R.
Yu

Paul Durrant April 21, 2016, 1:56 p.m. UTC | #29

> -----Original Message-----

> From: Yu, Zhang [mailto:[e-mail]]

> Sent: 21 April 2016 14:49

> To: Paul Durrant; George Dunlap; Jan Beulich; Wei Liu

> Cc: Kevin Tian; Keir (Xen.org); Andrew Cooper; Tim (Xen.org); xen-

> [e-mail]; [e-mail]; [e-mail]

> Subject: Re: [Xen-devel] [PATCH v2 2/3] x86/ioreq server: Rename

> p2m_mmio_write_dm to p2m_ioreq_server

> 

> 

> 

> On 4/21/2016 9:31 PM, Paul Durrant wrote:

> >> -----Original Message-----

> >> From: Yu, Zhang [mailto:[e-mail]]

> >> Sent: 21 April 2016 13:25

> >> To: George Dunlap; Paul Durrant; Jan Beulich; Wei Liu

> >> Cc: Kevin Tian; Keir (Xen.org); Andrew Cooper; Tim (Xen.org); xen-

> >> [e-mail]; [e-mail]; [e-mail]

> >> Subject: Re: [Xen-devel] [PATCH v2 2/3] x86/ioreq server: Rename

> >> p2m_mmio_write_dm to p2m_ioreq_server

> >>

> >>

> >>

> >> On 4/21/2016 1:06 AM, George Dunlap wrote:

> >>> On 20/04/16 17:58, Paul Durrant wrote:

> >>>>> -----Original Message-----

> >>>>> From: Xen-devel [mailto:[e-mail]] On Behalf

> >> Of Jan

> >>>>> Beulich

> >>>>> Sent: 20 April 2016 17:53

> >>>>> To: George Dunlap; Paul Durrant; Wei Liu; [e-mail]

> >>>>> Cc: Kevin Tian; Keir (Xen.org); Andrew Cooper; Tim (Xen.org); xen-

> >>>>> [e-mail]; [e-mail]; [e-mail]

> >>>>> Subject: Re: [Xen-devel] [PATCH v2 2/3] x86/ioreq server: Rename

> >>>>> p2m_mmio_write_dm to p2m_ioreq_server

> >>>>>

> >>>>>>>> George Dunlap <[e-mail]> 04/20/16 6:30 PM

> >>>

> >>>>>> On Wed, Apr 20, 2016 at 4:02 PM, George Dunlap

> >>>>> <[e-mail]> wrote:

> >>>>>>> On 19/04/16 12:02, Yu, Zhang wrote:

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

> >>>>>

> >>>>> We don't normally do it like that - we aim at keeping things compatible

> >>>>> right away. I don't know of a case where we would have knowingly

> >> broken

> >>>>> compatibility for users of the public headers (leaving aside tool stack

> only

> >>>>> stuff of course).

> >>>>>

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

> >>>>>

> >>>>> I had the impression that the renaming patch was what it is - a

> renaming

> >>>>> patch, without altering behavior.

> >>>>>

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

> >>>>>

> >>>>> The only thing it can do is ask for the Xen version. The ABI version is

> not

> >>>>> being returned by anything (but perhaps should be).

> >>>>>

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

> >>>>>

> >>>>> For the full series I'm not sure I can really tell.But as said, for the

> rename

> >>>>> patch alone I thought it is just a rename. And that's what we want to

> get

> >>>>> in (see Paul's earlier reply - he wants to see the old name gone, so it

> >> won't

> >>>>> be used any further).

> >>>>>

> >>>>>> 2. What's the best thing to do for this release?

> >>>>>

> >>>>> If the entire series (no matter whether to go in now or later) is

> changing

> >>>>> behavior, then the only choice is to consider the currently used enum

> >>>>> value burnt, and use a fresh one for the new semantics.

> >>>>

> >>>> It sounds like that would be best way. If we don't so that then we have

> to

> >> maintain the write-dm semantics for pages of that type unless the type is

> >> claimed (by using the new hypercall) and that's bit icky. I much prefer that

> >> pages of the new type are treated as RAM until claimed.

> >>>

> >>> I think the only sensible way to keep the enum is to also keep the

> >>> functionality, which would mean using *another* p2m type for

> >> ioreq_server.

> >>>

> >>> Given that the functionality isn't going away for 4.7, I don't see an

> >>> urgent need to remove the enum; but if Paul does, then a patch

> renaming

> >>> it to HVMMEM_unused would be the way forward then I guess.  Once

> the

> >>> underlying p2m type goes away, you'll want to return -EINVAL for this

> >>> enum value.

> >>>

> >>

> >> So the enum would be sth. 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_mmio_write_dm, /* Read-only; writes go to the device

> model

> >> */

> >> #else

> >>      HVMMEM_unused,

> >> #endif

> >>      HVMMEM_ioreq_server

> >> } hvmmem_type_t;

> >>

> >

> > I believe that's correct, but presumably there's need to be a change to the

> hypervisor since any reference there to HVMMEM_mmio_write_dm (which I

> think is limited to the get and set mem type code in hvm.c) will now need to

> map HVMMEM_unused to the old p2m_mmio_write_dm type.

> >

> Thank you, Paul.

> 

> But p2m_mmio_write_dm will not exist any more...

> E.g. if in hvmop_get_mem_type(), if type 0xf(p2m_ioreq_server) is

> returned, we could just return HVMMEM_ioreq_server. No need to

> worry about the HVMMEM_mmio_write_dm.

> 

> Maybe we only need to change the beginning of hvmop_set_mem_type()

> to sth. like this:

> 

> /* Interface types to internal p2m types */

> static const p2m_type_t memtype[] = {

>      [HVMMEM_ram_rw]  = p2m_ram_rw,

>      [HVMMEM_ram_ro]  = p2m_ram_ro,

>      [HVMMEM_mmio_dm] = p2m_mmio_dm,

>      [HVMMEM_unused] = p2m_invalid,  /* this will be rejected later */

>      [HVMMEM_ioreq_server] = p2m_ioreq_server

> };

> and later in the same routine, just reject the HVMMEM_unused type, in

> an if(with unlikely) statement.

> 


As long as everyone is in agreement then we can break the functionality that exists in 4.6.1 (and presumably 4.7 now) then that’s ok.

  Paul

> >   Paul

> 

> B.R.

> Yu

George Dunlap April 21, 2016, 2:09 p.m. UTC | #30

On 21/04/16 14:56, Paul Durrant wrote:
>> -----Original Message-----
>> From: Yu, Zhang [mailto:[e-mail]]
>> Sent: 21 April 2016 14:49
>> To: Paul Durrant; George Dunlap; Jan Beulich; Wei Liu
>> Cc: Kevin Tian; Keir (Xen.org); Andrew Cooper; Tim (Xen.org); xen-
>> [e-mail]; [e-mail]; [e-mail]
>> Subject: Re: [Xen-devel] [PATCH v2 2/3] x86/ioreq server: Rename
>> p2m_mmio_write_dm to p2m_ioreq_server
>>
>>
>>
>> On 4/21/2016 9:31 PM, Paul Durrant wrote:
>>>> -----Original Message-----
>>>> From: Yu, Zhang [mailto:[e-mail]]
>>>> Sent: 21 April 2016 13:25
>>>> To: George Dunlap; Paul Durrant; Jan Beulich; Wei Liu
>>>> Cc: Kevin Tian; Keir (Xen.org); Andrew Cooper; Tim (Xen.org); xen-
>>>> [e-mail]; [e-mail]; [e-mail]
>>>> Subject: Re: [Xen-devel] [PATCH v2 2/3] x86/ioreq server: Rename
>>>> p2m_mmio_write_dm to p2m_ioreq_server
>>>>
>>>>
>>>>
>>>> On 4/21/2016 1:06 AM, George Dunlap wrote:
>>>>> On 20/04/16 17:58, Paul Durrant wrote:
>>>>>>> -----Original Message-----
>>>>>>> From: Xen-devel [mailto:[e-mail]] On Behalf
>>>> Of Jan
>>>>>>> Beulich
>>>>>>> Sent: 20 April 2016 17:53
>>>>>>> To: George Dunlap; Paul Durrant; Wei Liu; [e-mail]
>>>>>>> Cc: Kevin Tian; Keir (Xen.org); Andrew Cooper; Tim (Xen.org); xen-
>>>>>>> [e-mail]; [e-mail]; [e-mail]
>>>>>>> Subject: Re: [Xen-devel] [PATCH v2 2/3] x86/ioreq server: Rename
>>>>>>> p2m_mmio_write_dm to p2m_ioreq_server
>>>>>>>
>>>>>>>>>> George Dunlap <[e-mail]> 04/20/16 6:30 PM
>>>>>
>>>>>>>> On Wed, Apr 20, 2016 at 4:02 PM, George Dunlap
>>>>>>> <[e-mail]> wrote:
>>>>>>>>> On 19/04/16 12:02, Yu, Zhang wrote:
>>>>>>>>>> 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.
>>>>>>>
>>>>>>> We don't normally do it like that - we aim at keeping things compatible
>>>>>>> right away. I don't know of a case where we would have knowingly
>>>> broken
>>>>>>> compatibility for users of the public headers (leaving aside tool stack
>> only
>>>>>>> stuff of course).
>>>>>>>
>>>>>>>> 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.
>>>>>>>
>>>>>>> I had the impression that the renaming patch was what it is - a
>> renaming
>>>>>>> patch, without altering behavior.
>>>>>>>
>>>>>>>> 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.
>>>>>>>
>>>>>>> The only thing it can do is ask for the Xen version. The ABI version is
>> not
>>>>>>> being returned by anything (but perhaps should be).
>>>>>>>
>>>>>>>> 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?
>>>>>>>
>>>>>>> For the full series I'm not sure I can really tell.But as said, for the
>> rename
>>>>>>> patch alone I thought it is just a rename. And that's what we want to
>> get
>>>>>>> in (see Paul's earlier reply - he wants to see the old name gone, so it
>>>> won't
>>>>>>> be used any further).
>>>>>>>
>>>>>>>> 2. What's the best thing to do for this release?
>>>>>>>
>>>>>>> If the entire series (no matter whether to go in now or later) is
>> changing
>>>>>>> behavior, then the only choice is to consider the currently used enum
>>>>>>> value burnt, and use a fresh one for the new semantics.
>>>>>>
>>>>>> It sounds like that would be best way. If we don't so that then we have
>> to
>>>> maintain the write-dm semantics for pages of that type unless the type is
>>>> claimed (by using the new hypercall) and that's bit icky. I much prefer that
>>>> pages of the new type are treated as RAM until claimed.
>>>>>
>>>>> I think the only sensible way to keep the enum is to also keep the
>>>>> functionality, which would mean using *another* p2m type for
>>>> ioreq_server.
>>>>>
>>>>> Given that the functionality isn't going away for 4.7, I don't see an
>>>>> urgent need to remove the enum; but if Paul does, then a patch
>> renaming
>>>>> it to HVMMEM_unused would be the way forward then I guess.  Once
>> the
>>>>> underlying p2m type goes away, you'll want to return -EINVAL for this
>>>>> enum value.
>>>>>
>>>>
>>>> So the enum would be sth. 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_mmio_write_dm, /* Read-only; writes go to the device
>> model
>>>> */
>>>> #else
>>>>      HVMMEM_unused,
>>>> #endif
>>>>      HVMMEM_ioreq_server
>>>> } hvmmem_type_t;
>>>>
>>>
>>> I believe that's correct, but presumably there's need to be a change to the
>> hypervisor since any reference there to HVMMEM_mmio_write_dm (which I
>> think is limited to the get and set mem type code in hvm.c) will now need to
>> map HVMMEM_unused to the old p2m_mmio_write_dm type.
>>>
>> Thank you, Paul.
>>
>> But p2m_mmio_write_dm will not exist any more...
>> E.g. if in hvmop_get_mem_type(), if type 0xf(p2m_ioreq_server) is
>> returned, we could just return HVMMEM_ioreq_server. No need to
>> worry about the HVMMEM_mmio_write_dm.
>>
>> Maybe we only need to change the beginning of hvmop_set_mem_type()
>> to sth. like this:
>>
>> /* Interface types to internal p2m types */
>> static const p2m_type_t memtype[] = {
>>      [HVMMEM_ram_rw]  = p2m_ram_rw,
>>      [HVMMEM_ram_ro]  = p2m_ram_ro,
>>      [HVMMEM_mmio_dm] = p2m_mmio_dm,
>>      [HVMMEM_unused] = p2m_invalid,  /* this will be rejected later */
>>      [HVMMEM_ioreq_server] = p2m_ioreq_server
>> };
>> and later in the same routine, just reject the HVMMEM_unused type, in
>> an if(with unlikely) statement.
>>
> 
> As long as everyone is in agreement then we can break the functionality that exists in 4.6.1 (and presumably 4.7 now) then that’s ok.

I think we're all in agreement that we can *remove* functionality as
long as (from Xen's perspective) it should fail gracefully.  Which means
that if anyone uses the hvm_mem_type enum  value previously allocated do
HVMMEM_mmio_write_dm, return an error (one way or another).

The exact method for doing so is up for review.

 -George

Patch

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index f700923..bec6a8a 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -3166,7 +3166,7 @@  int hvm_hap_nested_page_fault(paddr_t gpa, unsigned long gla,
      */
     if ( (p2mt == p2m_mmio_dm) || 
          (npfec.write_access &&
-          (p2m_is_discard_write(p2mt) || (p2mt == p2m_mmio_write_dm))) )
+          (p2m_is_discard_write(p2mt) || (p2mt == p2m_ioreq_server))) )
     {
         __put_gfn(p2m, gfn);
         if ( ap2m_active )
@@ -6578,8 +6578,8 @@  static int hvmop_get_mem_type(
     get_gfn_query_unlocked(d, a.pfn, &t);
     if ( p2m_is_mmio(t) )
         a.mem_type =  HVMMEM_mmio_dm;
-    else if ( t == p2m_mmio_write_dm )
-        a.mem_type = HVMMEM_mmio_write_dm;
+    else if ( t == p2m_ioreq_server )
+        a.mem_type = HVMMEM_ioreq_server;
     else if ( p2m_is_readonly(t) )
         a.mem_type =  HVMMEM_ram_ro;
     else if ( p2m_is_ram(t) )
@@ -6614,7 +6614,7 @@  static bool_t hvm_allow_p2m_type_change(p2m_type_t old, p2m_type_t new)
 {
     if ( p2m_is_ram(old) ||
          (p2m_is_hole(old) && new == p2m_mmio_dm) ||
-         (old == p2m_mmio_write_dm && new == p2m_ram_rw) )
+         (old == p2m_ioreq_server && new == p2m_ram_rw) )
         return 1;
 
     return 0;
@@ -6634,7 +6634,7 @@  static int hvmop_set_mem_type(
         [HVMMEM_ram_rw]  = p2m_ram_rw,
         [HVMMEM_ram_ro]  = p2m_ram_ro,
         [HVMMEM_mmio_dm] = p2m_mmio_dm,
-        [HVMMEM_mmio_write_dm] = p2m_mmio_write_dm
+        [HVMMEM_ioreq_server] = p2m_ioreq_server
     };
 
     if ( copy_from_guest(&a, arg, 1) )
diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
index 3cb6868..380ec25 100644
--- a/xen/arch/x86/mm/p2m-ept.c
+++ b/xen/arch/x86/mm/p2m-ept.c
@@ -171,7 +171,7 @@  static void ept_p2m_type_to_flags(struct p2m_domain *p2m, ept_entry_t *entry,
             entry->a = entry->d = !!cpu_has_vmx_ept_ad;
             break;
         case p2m_grant_map_ro:
-        case p2m_mmio_write_dm:
+        case p2m_ioreq_server:
             entry->r = 1;
             entry->w = entry->x = 0;
             entry->a = !!cpu_has_vmx_ept_ad;
diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c
index 3d80612..eabd2e3 100644
--- a/xen/arch/x86/mm/p2m-pt.c
+++ b/xen/arch/x86/mm/p2m-pt.c
@@ -94,7 +94,7 @@  static unsigned long p2m_type_to_flags(p2m_type_t t, mfn_t mfn,
     default:
         return flags | _PAGE_NX_BIT;
     case p2m_grant_map_ro:
-    case p2m_mmio_write_dm:
+    case p2m_ioreq_server:
         return flags | P2M_BASE_FLAGS | _PAGE_NX_BIT;
     case p2m_ram_ro:
     case p2m_ram_logdirty:
diff --git a/xen/arch/x86/mm/shadow/multi.c b/xen/arch/x86/mm/shadow/multi.c
index e5c8499..c81302a 100644
--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -3225,7 +3225,7 @@  static int sh_page_fault(struct vcpu *v,
 
     /* Need to hand off device-model MMIO to the device model */
     if ( p2mt == p2m_mmio_dm
-         || (p2mt == p2m_mmio_write_dm && ft == ft_demand_write) )
+         || (p2mt == p2m_ioreq_server && ft == ft_demand_write) )
     {
         gpa = guest_walk_to_gpa(&gw);
         goto mmio;
diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index 5392eb0..ee2ea9c 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -71,7 +71,7 @@  typedef enum {
     p2m_ram_shared = 12,          /* Shared or sharable memory */
     p2m_ram_broken = 13,          /* Broken page, access cause domain crash */
     p2m_map_foreign  = 14,        /* ram pages from foreign domain */
-    p2m_mmio_write_dm = 15,       /* Read-only; writes go to the device model */
+    p2m_ioreq_server = 15,
 } p2m_type_t;
 
 /* Modifiers to the query */
@@ -112,7 +112,7 @@  typedef unsigned int p2m_query_t;
                       | p2m_to_mask(p2m_ram_ro)         \
                       | p2m_to_mask(p2m_grant_map_ro)   \
                       | p2m_to_mask(p2m_ram_shared)     \
-                      | p2m_to_mask(p2m_mmio_write_dm))
+                      | p2m_to_mask(p2m_ioreq_server))
 
 /* Write-discard types, which should discard the write operations */
 #define P2M_DISCARD_WRITE_TYPES (p2m_to_mask(p2m_ram_ro)     \
diff --git a/xen/include/public/hvm/hvm_op.h b/xen/include/public/hvm/hvm_op.h
index 1606185..a1eae52 100644
--- a/xen/include/public/hvm/hvm_op.h
+++ b/xen/include/public/hvm/hvm_op.h
@@ -83,7 +83,7 @@  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 */
-    HVMMEM_mmio_write_dm       /* Read-only; writes go to the device model */
+    HVMMEM_ioreq_server,
 } hvmmem_type_t;
 
 /* Following tools-only interfaces may change in future. */