[PATCH for 4.7] Remove HVMMEM_mmio_write_dm from the public interface.

[PATCH for 4.7] Remove HVMMEM_mmio_write_dm from the public interface.

[Yu Zhang]

HVMMEM_mmio_write_dm is removed for new xen interface versions, and
is replaced with type HVMMEM_unused. Attempts to set a page to this
type will return -EINVAL in xen after 4.7.0. And there will be no
pages with type p2m_mmio_write_dm, therefore HVMOP_get_mem_type will
never get the old type - HVMMEM_mmio_write_dm.

New approaches to write protect guest ram pages will be provided in
future patches.

Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <george.dunlap@eu.citrix.com>
Cc: Paul Durrant <paul.durrant@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 xen/arch/x86/hvm/hvm.c          | 7 +++----
 xen/include/public/hvm/hvm_op.h | 5 +++++
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 8cb6e9e..82e2ed1 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -5497,8 +5497,6 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
             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 ( p2m_is_readonly(t) )
                 a.mem_type =  HVMMEM_ram_ro;
             else if ( p2m_is_ram(t) )
@@ -5529,7 +5527,7 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
             [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_unused] = p2m_invalid
         };
 
         if ( copy_from_guest(&a, arg, 1) )
@@ -5553,7 +5551,8 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
              ((a.first_pfn + a.nr - 1) > domain_get_maximum_gpfn(d)) )
             goto setmemtype_fail;
             
-        if ( a.hvmmem_type >= ARRAY_SIZE(memtype) )
+        if ( a.hvmmem_type >= ARRAY_SIZE(memtype) ||
+             unlikely(a.hvmmem_type == HVMMEM_unused) )
             goto setmemtype_fail;
 
         while ( a.nr > start_iter )
diff --git a/xen/include/public/hvm/hvm_op.h b/xen/include/public/hvm/hvm_op.h
index 1606185..ebb907a 100644
--- a/xen/include/public/hvm/hvm_op.h
+++ b/xen/include/public/hvm/hvm_op.h
@@ -83,7 +83,12 @@ 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              /* Placeholder; setting memory to this type
+                                  will fail for code after 4.7.0 */
+#endif
 } hvmmem_type_t;
 
 /* Following tools-only interfaces may change in future. */
-- 
1.9.1


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

Re: [PATCH for 4.7] Remove HVMMEM_mmio_write_dm from the public interface.

[Paul Durrant]

> -----Original Message-----
> From: Yu Zhang [mailto:yu.c.zhang@linux.intel.com]
> Sent: 28 April 2016 09:30
> To: xen-devel@lists.xen.org
> Cc: zhiyuan.lv@intel.com; Jan Beulich; Andrew Cooper; George Dunlap; Paul
> Durrant; Wei Liu
> Subject: [PATCH for 4.7] Remove HVMMEM_mmio_write_dm from the
> public interface.
> 
> HVMMEM_mmio_write_dm is removed for new xen interface versions, and
> is replaced with type HVMMEM_unused. Attempts to set a page to this
> type will return -EINVAL in xen after 4.7.0. And there will be no
> pages with type p2m_mmio_write_dm, therefore HVMOP_get_mem_type
> will
> never get the old type - HVMMEM_mmio_write_dm.
> 
> New approaches to write protect guest ram pages will be provided in
> future patches.
> 
> Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com>
> ---
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: George Dunlap <george.dunlap@eu.citrix.com>
> Cc: Paul Durrant <paul.durrant@citrix.com>

Reviewed-by: Paul Durrant <paul.durrant@citrix.com>

> Cc: Wei Liu <wei.liu2@citrix.com>
> ---
>  xen/arch/x86/hvm/hvm.c          | 7 +++----
>  xen/include/public/hvm/hvm_op.h | 5 +++++
>  2 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 8cb6e9e..82e2ed1 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -5497,8 +5497,6 @@ long do_hvm_op(unsigned long op,
> XEN_GUEST_HANDLE_PARAM(void) arg)
>              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 ( p2m_is_readonly(t) )
>                  a.mem_type =  HVMMEM_ram_ro;
>              else if ( p2m_is_ram(t) )
> @@ -5529,7 +5527,7 @@ long do_hvm_op(unsigned long op,
> XEN_GUEST_HANDLE_PARAM(void) arg)
>              [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_unused] = p2m_invalid
>          };
> 
>          if ( copy_from_guest(&a, arg, 1) )
> @@ -5553,7 +5551,8 @@ long do_hvm_op(unsigned long op,
> XEN_GUEST_HANDLE_PARAM(void) arg)
>               ((a.first_pfn + a.nr - 1) > domain_get_maximum_gpfn(d)) )
>              goto setmemtype_fail;
> 
> -        if ( a.hvmmem_type >= ARRAY_SIZE(memtype) )
> +        if ( a.hvmmem_type >= ARRAY_SIZE(memtype) ||
> +             unlikely(a.hvmmem_type == HVMMEM_unused) )
>              goto setmemtype_fail;
> 
>          while ( a.nr > start_iter )
> diff --git a/xen/include/public/hvm/hvm_op.h
> b/xen/include/public/hvm/hvm_op.h
> index 1606185..ebb907a 100644
> --- a/xen/include/public/hvm/hvm_op.h
> +++ b/xen/include/public/hvm/hvm_op.h
> @@ -83,7 +83,12 @@ 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              /* Placeholder; setting memory to this type
> +                                  will fail for code after 4.7.0 */
> +#endif
>  } hvmmem_type_t;
> 
>  /* Following tools-only interfaces may change in future. */
> --
> 1.9.1


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

Re: [PATCH for 4.7] Remove HVMMEM_mmio_write_dm from the public interface.

[Jan Beulich]

>>> On 28.04.16 at 10:29, <yu.c.zhang@linux.intel.com> wrote:
> @@ -5529,7 +5527,7 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
>              [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_unused] = p2m_invalid

Why don't you simply delete the old line, without replacement?

Jan


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

Re: [PATCH for 4.7] Remove HVMMEM_mmio_write_dm from the public interface.

[Paul Durrant]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 28 April 2016 11:23
> To: Yu Zhang
> Cc: Andrew Cooper; Paul Durrant; Wei Liu; George Dunlap;
> zhiyuan.lv@intel.com; xen-devel@lists.xen.org
> Subject: Re: [PATCH for 4.7] Remove HVMMEM_mmio_write_dm from the
> public interface.
> 
> >>> On 28.04.16 at 10:29, <yu.c.zhang@linux.intel.com> wrote:
> > @@ -5529,7 +5527,7 @@ long do_hvm_op(unsigned long op,
> XEN_GUEST_HANDLE_PARAM(void) arg)
> >              [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_unused] = p2m_invalid
> 
> Why don't you simply delete the old line, without replacement?
> 

I felt that having the replacement line was worth it to re-enforce that the mem type is really not to be used but it is, as you point out, not necessary.

  Paul

> Jan


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

Re: [PATCH for 4.7] Remove HVMMEM_mmio_write_dm from the public interface.

[George Dunlap]

On 28/04/16 11:22, Jan Beulich wrote:
>>>> On 28.04.16 at 10:29, <yu.c.zhang@linux.intel.com> wrote:
>> @@ -5529,7 +5527,7 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
>>              [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_unused] = p2m_invalid
> 
> Why don't you simply delete the old line, without replacement?

That might have been slightly cleaner; but we're going to have to put it
back as soon as the development window opens anyway, so I don't really
see the point of going through the effort of respinning the patch again.

Would you be willing to ack this version anyway?

Thanks,
 -George


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

Re: [PATCH for 4.7] Remove HVMMEM_mmio_write_dm from the public interface.

[George Dunlap]

On 28/04/16 09:29, Yu Zhang wrote:
> HVMMEM_mmio_write_dm is removed for new xen interface versions, and
> is replaced with type HVMMEM_unused. Attempts to set a page to this
> type will return -EINVAL in xen after 4.7.0. And there will be no
> pages with type p2m_mmio_write_dm, therefore HVMOP_get_mem_type will
> never get the old type - HVMMEM_mmio_write_dm.
> 
> New approaches to write protect guest ram pages will be provided in
> future patches.
> 
> Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com>

Thanks,

Reviewed-by: George Dunlap <george.dunlap@citrix.com>


> ---
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: George Dunlap <george.dunlap@eu.citrix.com>
> Cc: Paul Durrant <paul.durrant@citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> ---
>  xen/arch/x86/hvm/hvm.c          | 7 +++----
>  xen/include/public/hvm/hvm_op.h | 5 +++++
>  2 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 8cb6e9e..82e2ed1 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -5497,8 +5497,6 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
>              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 ( p2m_is_readonly(t) )
>                  a.mem_type =  HVMMEM_ram_ro;
>              else if ( p2m_is_ram(t) )
> @@ -5529,7 +5527,7 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
>              [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_unused] = p2m_invalid
>          };
>  
>          if ( copy_from_guest(&a, arg, 1) )
> @@ -5553,7 +5551,8 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
>               ((a.first_pfn + a.nr - 1) > domain_get_maximum_gpfn(d)) )
>              goto setmemtype_fail;
>              
> -        if ( a.hvmmem_type >= ARRAY_SIZE(memtype) )
> +        if ( a.hvmmem_type >= ARRAY_SIZE(memtype) ||
> +             unlikely(a.hvmmem_type == HVMMEM_unused) )
>              goto setmemtype_fail;
>  
>          while ( a.nr > start_iter )
> diff --git a/xen/include/public/hvm/hvm_op.h b/xen/include/public/hvm/hvm_op.h
> index 1606185..ebb907a 100644
> --- a/xen/include/public/hvm/hvm_op.h
> +++ b/xen/include/public/hvm/hvm_op.h
> @@ -83,7 +83,12 @@ 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              /* Placeholder; setting memory to this type
> +                                  will fail for code after 4.7.0 */
> +#endif
>  } hvmmem_type_t;
>  
>  /* Following tools-only interfaces may change in future. */
> 


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

Re: [PATCH for 4.7] Remove HVMMEM_mmio_write_dm from the public interface.

[Wei Liu]

On Thu, Apr 28, 2016 at 04:29:57PM +0800, Yu Zhang wrote:
> HVMMEM_mmio_write_dm is removed for new xen interface versions, and
> is replaced with type HVMMEM_unused. Attempts to set a page to this
> type will return -EINVAL in xen after 4.7.0. And there will be no
> pages with type p2m_mmio_write_dm, therefore HVMOP_get_mem_type will
> never get the old type - HVMMEM_mmio_write_dm.
> 
> New approaches to write protect guest ram pages will be provided in
> future patches.
> 
> Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com>

With or without the adjustment Jan asked for:

Reviewed-by: Wei Liu <wei.liu2@citrix.com>
Release-acked-by: Wei Liu <wei.liu2@citrix.com>

> ---
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: George Dunlap <george.dunlap@eu.citrix.com>
> Cc: Paul Durrant <paul.durrant@citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> ---
>  xen/arch/x86/hvm/hvm.c          | 7 +++----
>  xen/include/public/hvm/hvm_op.h | 5 +++++
>  2 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 8cb6e9e..82e2ed1 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -5497,8 +5497,6 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
>              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 ( p2m_is_readonly(t) )
>                  a.mem_type =  HVMMEM_ram_ro;
>              else if ( p2m_is_ram(t) )
> @@ -5529,7 +5527,7 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
>              [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_unused] = p2m_invalid
>          };
>  
>          if ( copy_from_guest(&a, arg, 1) )
> @@ -5553,7 +5551,8 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
>               ((a.first_pfn + a.nr - 1) > domain_get_maximum_gpfn(d)) )
>              goto setmemtype_fail;
>              
> -        if ( a.hvmmem_type >= ARRAY_SIZE(memtype) )
> +        if ( a.hvmmem_type >= ARRAY_SIZE(memtype) ||
> +             unlikely(a.hvmmem_type == HVMMEM_unused) )
>              goto setmemtype_fail;
>  
>          while ( a.nr > start_iter )
> diff --git a/xen/include/public/hvm/hvm_op.h b/xen/include/public/hvm/hvm_op.h
> index 1606185..ebb907a 100644
> --- a/xen/include/public/hvm/hvm_op.h
> +++ b/xen/include/public/hvm/hvm_op.h
> @@ -83,7 +83,12 @@ 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              /* Placeholder; setting memory to this type
> +                                  will fail for code after 4.7.0 */
> +#endif
>  } hvmmem_type_t;
>  
>  /* Following tools-only interfaces may change in future. */
> -- 
> 1.9.1
> 

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

Re: [PATCH for 4.7] Remove HVMMEM_mmio_write_dm from the public interface.

[Jan Beulich]

>>> On 28.04.16 at 12:42, <george.dunlap@citrix.com> wrote:
> On 28/04/16 11:22, Jan Beulich wrote:
>>>>> On 28.04.16 at 10:29, <yu.c.zhang@linux.intel.com> wrote:
>>> @@ -5529,7 +5527,7 @@ long do_hvm_op(unsigned long op, 
> XEN_GUEST_HANDLE_PARAM(void) arg)
>>>              [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_unused] = p2m_invalid
>> 
>> Why don't you simply delete the old line, without replacement?
> 
> That might have been slightly cleaner; but we're going to have to put it
> back as soon as the development window opens anyway, so I don't really
> see the point of going through the effort of respinning the patch again.
> 
> Would you be willing to ack this version anyway?

I have no problem doing so (and in fact I have it on my to by
committed list already), it is just looked slightly confusing (and
I had already typed half a reply that this isn't what was discussed
until I properly looked at the next hunk), and hence I wanted to
understand the motivation. And btw., I'm not convinced it would
need to be put there anyway later: I don't view the used
mechanism as a good (read: extensible) one to deal with what
would be holes in the array above. Indeed we can't leave them
uninitialized (as that would mean p2m_ram_rw), but I think we
should better find a way to initialize _all_ unused slots without
requiring an initializer for each of them. Sadly the desire to allow
compilation with clang prohibits the most natural solution:

        static const p2m_type_t memtype[] = {
            [0 ... <upper-bound> - 1] = p2m_invalid,
            [HVMMEM_ram_rw]  = p2m_ram_rw,
            [HVMMEM_ram_ro]  = p2m_ram_ro,
            [HVMMEM_mmio_dm] = p2m_mmio_dm,
        };

Maybe we could do (altering the second hunk of this patch)

@@ -5553,7 +5551,10 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
              ((a.first_pfn + a.nr - 1) > domain_get_maximum_gpfn(d)) )
             goto setmemtype_fail;
             
-        if ( a.hvmmem_type >= ARRAY_SIZE(memtype) )
+        BUILD_BUG_ON(p2m_ram_rw);
+        BUILD_BUG_ON(HVMMEM_ram_rw);
+        if ( a.hvmmem_type >= ARRAY_SIZE(memtype) ||
+             (a.hvmmem_type && !memtype[a.hvmmem_type]) )
             goto setmemtype_fail;
 
         while ( a.nr > start_iter )

Jan


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

Re: [PATCH for 4.7] Remove HVMMEM_mmio_write_dm from the public interface.

[Yu, Zhang]

Thanks Jan. And I admire your rigorous thought. :)

On 4/28/2016 6:57 PM, Jan Beulich wrote:
>>>> On 28.04.16 at 12:42, <george.dunlap@citrix.com> wrote:
>> On 28/04/16 11:22, Jan Beulich wrote:
>>>>>> On 28.04.16 at 10:29, <yu.c.zhang@linux.intel.com> wrote:
>>>> @@ -5529,7 +5527,7 @@ long do_hvm_op(unsigned long op,
>> XEN_GUEST_HANDLE_PARAM(void) arg)
>>>>              [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_unused] = p2m_invalid
>>>
>>> Why don't you simply delete the old line, without replacement?
>>

Well, I did not delete the old line, because in my coming patch(the
p2m renaming code), I'm planning to introduce the HVMMEM_ioreq_server,
which is HVMMEM_unused+1. And I do not want the check of a.hvmmem_type
against HVMMEN_unused later in this routine appear in that patch.

>> That might have been slightly cleaner; but we're going to have to put it
>> back as soon as the development window opens anyway, so I don't really
>> see the point of going through the effort of respinning the patch again.
>>
>> Would you be willing to ack this version anyway?
>
> I have no problem doing so (and in fact I have it on my to by
> committed list already), it is just looked slightly confusing (and
> I had already typed half a reply that this isn't what was discussed
> until I properly looked at the next hunk), and hence I wanted to
> understand the motivation. And btw., I'm not convinced it would
> need to be put there anyway later: I don't view the used
> mechanism as a good (read: extensible) one to deal with what
> would be holes in the array above. Indeed we can't leave them
> uninitialized (as that would mean p2m_ram_rw), but I think we
> should better find a way to initialize _all_ unused slots without
> requiring an initializer for each of them. Sadly the desire to allow
> compilation with clang prohibits the most natural solution:
>
>         static const p2m_type_t memtype[] = {
>             [0 ... <upper-bound> - 1] = p2m_invalid,

Not sure if this will compile? Can have a try. :)

>             [HVMMEM_ram_rw]  = p2m_ram_rw,
>             [HVMMEM_ram_ro]  = p2m_ram_ro,
>             [HVMMEM_mmio_dm] = p2m_mmio_dm,
>         };
>
> Maybe we could do (altering the second hunk of this patch)
>
> @@ -5553,7 +5551,10 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
>               ((a.first_pfn + a.nr - 1) > domain_get_maximum_gpfn(d)) )
>              goto setmemtype_fail;
>
> -        if ( a.hvmmem_type >= ARRAY_SIZE(memtype) )
> +        BUILD_BUG_ON(p2m_ram_rw);
> +        BUILD_BUG_ON(HVMMEM_ram_rw);
> +        if ( a.hvmmem_type >= ARRAY_SIZE(memtype) ||
> +             (a.hvmmem_type && !memtype[a.hvmmem_type]) )

I guess by !memtype[a.hvmmem_type] you are trying to check if it's
p2m_invalid? But p2m_ram_rw is 0, and p2m_invalid is 1. So may be it
should be checked like memtype[a.hvmmem_type] < 0 and initialize the
holes with -1.

But I still wonder is this really necessary? Because we only have one
hole in this array in the forseeable future.

>              goto setmemtype_fail;
>
>          while ( a.nr > start_iter )
>
> Jan
>
>

B.R.
Yu

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

Re: [PATCH for 4.7] Remove HVMMEM_mmio_write_dm from the public interface.

[Jan Beulich]

>>> On 28.04.16 at 13:40, <yu.c.zhang@linux.intel.com> wrote:
> On 4/28/2016 6:57 PM, Jan Beulich wrote:
>>>>> On 28.04.16 at 12:42, <george.dunlap@citrix.com> wrote:
>>> That might have been slightly cleaner; but we're going to have to put it
>>> back as soon as the development window opens anyway, so I don't really
>>> see the point of going through the effort of respinning the patch again.
>>>
>>> Would you be willing to ack this version anyway?
>>
>> I have no problem doing so (and in fact I have it on my to by
>> committed list already), it is just looked slightly confusing (and
>> I had already typed half a reply that this isn't what was discussed
>> until I properly looked at the next hunk), and hence I wanted to
>> understand the motivation. And btw., I'm not convinced it would
>> need to be put there anyway later: I don't view the used
>> mechanism as a good (read: extensible) one to deal with what
>> would be holes in the array above. Indeed we can't leave them
>> uninitialized (as that would mean p2m_ram_rw), but I think we
>> should better find a way to initialize _all_ unused slots without
>> requiring an initializer for each of them. Sadly the desire to allow
>> compilation with clang prohibits the most natural solution:
>>
>>         static const p2m_type_t memtype[] = {
>>             [0 ... <upper-bound> - 1] = p2m_invalid,
> 
> Not sure if this will compile? Can have a try. :)

gcc will like it, but as said clang won't (afair).

>>             [HVMMEM_ram_rw]  = p2m_ram_rw,
>>             [HVMMEM_ram_ro]  = p2m_ram_ro,
>>             [HVMMEM_mmio_dm] = p2m_mmio_dm,
>>         };
>>
>> Maybe we could do (altering the second hunk of this patch)
>>
>> @@ -5553,7 +5551,10 @@ long do_hvm_op(unsigned long op, 
> XEN_GUEST_HANDLE_PARAM(void) arg)
>>               ((a.first_pfn + a.nr - 1) > domain_get_maximum_gpfn(d)) )
>>              goto setmemtype_fail;
>>
>> -        if ( a.hvmmem_type >= ARRAY_SIZE(memtype) )
>> +        BUILD_BUG_ON(p2m_ram_rw);
>> +        BUILD_BUG_ON(HVMMEM_ram_rw);
>> +        if ( a.hvmmem_type >= ARRAY_SIZE(memtype) ||
>> +             (a.hvmmem_type && !memtype[a.hvmmem_type]) )
> 
> I guess by !memtype[a.hvmmem_type] you are trying to check if it's
> p2m_invalid? But p2m_ram_rw is 0, and p2m_invalid is 1. So may be it
> should be checked like memtype[a.hvmmem_type] < 0 and initialize the
> holes with -1.

No. As said, I want to avoid explicit initializers for unused slots,
and hence it has to be zero that gets checked against.

> But I still wonder is this really necessary? Because we only have one
> hole in this array in the forseeable future.

How do you know?

Jan


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

Re: [PATCH for 4.7] Remove HVMMEM_mmio_write_dm from the public interface.

[Wei Liu]

On Thu, Apr 28, 2016 at 07:40:45PM +0800, Yu, Zhang wrote:
> Thanks Jan. And I admire your rigorous thought. :)
> 
> On 4/28/2016 6:57 PM, Jan Beulich wrote:
> >>>>On 28.04.16 at 12:42, <george.dunlap@citrix.com> wrote:
> >>On 28/04/16 11:22, Jan Beulich wrote:
> >>>>>>On 28.04.16 at 10:29, <yu.c.zhang@linux.intel.com> wrote:
> >>>>@@ -5529,7 +5527,7 @@ long do_hvm_op(unsigned long op,
> >>XEN_GUEST_HANDLE_PARAM(void) arg)
> >>>>             [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_unused] = p2m_invalid
> >>>
> >>>Why don't you simply delete the old line, without replacement?
> >>
> 
> Well, I did not delete the old line, because in my coming patch(the
> p2m renaming code), I'm planning to introduce the HVMMEM_ioreq_server,
> which is HVMMEM_unused+1. And I do not want the check of a.hvmmem_type
> against HVMMEN_unused later in this routine appear in that patch.
> 
> >>That might have been slightly cleaner; but we're going to have to put it
> >>back as soon as the development window opens anyway, so I don't really
> >>see the point of going through the effort of respinning the patch again.
> >>
> >>Would you be willing to ack this version anyway?
> >
> >I have no problem doing so (and in fact I have it on my to by
> >committed list already), it is just looked slightly confusing (and
> >I had already typed half a reply that this isn't what was discussed
> >until I properly looked at the next hunk), and hence I wanted to
> >understand the motivation. And btw., I'm not convinced it would
> >need to be put there anyway later: I don't view the used
> >mechanism as a good (read: extensible) one to deal with what
> >would be holes in the array above. Indeed we can't leave them
> >uninitialized (as that would mean p2m_ram_rw), but I think we
> >should better find a way to initialize _all_ unused slots without
> >requiring an initializer for each of them. Sadly the desire to allow
> >compilation with clang prohibits the most natural solution:
> >
> >        static const p2m_type_t memtype[] = {
> >            [0 ... <upper-bound> - 1] = p2m_invalid,
> 
> Not sure if this will compile? Can have a try. :)
> 

To answer your question this can compile with gcc but not probably not
with clang. This syntax is gcc extension.

See: https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html

Wei.

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

Re: [PATCH for 4.7] Remove HVMMEM_mmio_write_dm from the public interface.

[Yu, Zhang]

On 4/28/2016 7:52 PM, Jan Beulich wrote:
>>>> On 28.04.16 at 13:40, <yu.c.zhang@linux.intel.com> wrote:
>> On 4/28/2016 6:57 PM, Jan Beulich wrote:
>>>>>> On 28.04.16 at 12:42, <george.dunlap@citrix.com> wrote:
>>>> That might have been slightly cleaner; but we're going to have to put it
>>>> back as soon as the development window opens anyway, so I don't really
>>>> see the point of going through the effort of respinning the patch again.
>>>>
>>>> Would you be willing to ack this version anyway?
>>>
>>> I have no problem doing so (and in fact I have it on my to by
>>> committed list already), it is just looked slightly confusing (and
>>> I had already typed half a reply that this isn't what was discussed
>>> until I properly looked at the next hunk), and hence I wanted to
>>> understand the motivation. And btw., I'm not convinced it would
>>> need to be put there anyway later: I don't view the used
>>> mechanism as a good (read: extensible) one to deal with what
>>> would be holes in the array above. Indeed we can't leave them
>>> uninitialized (as that would mean p2m_ram_rw), but I think we
>>> should better find a way to initialize _all_ unused slots without
>>> requiring an initializer for each of them. Sadly the desire to allow
>>> compilation with clang prohibits the most natural solution:
>>>
>>>         static const p2m_type_t memtype[] = {
>>>             [0 ... <upper-bound> - 1] = p2m_invalid,
>>
>> Not sure if this will compile? Can have a try. :)
>
> gcc will like it, but as said clang won't (afair).
>
>>>             [HVMMEM_ram_rw]  = p2m_ram_rw,
>>>             [HVMMEM_ram_ro]  = p2m_ram_ro,
>>>             [HVMMEM_mmio_dm] = p2m_mmio_dm,
>>>         };
>>>
>>> Maybe we could do (altering the second hunk of this patch)
>>>
>>> @@ -5553,7 +5551,10 @@ long do_hvm_op(unsigned long op,
>> XEN_GUEST_HANDLE_PARAM(void) arg)
>>>               ((a.first_pfn + a.nr - 1) > domain_get_maximum_gpfn(d)) )
>>>              goto setmemtype_fail;
>>>
>>> -        if ( a.hvmmem_type >= ARRAY_SIZE(memtype) )
>>> +        BUILD_BUG_ON(p2m_ram_rw);
>>> +        BUILD_BUG_ON(HVMMEM_ram_rw);
>>> +        if ( a.hvmmem_type >= ARRAY_SIZE(memtype) ||
>>> +             (a.hvmmem_type && !memtype[a.hvmmem_type]) )
>>
>> I guess by !memtype[a.hvmmem_type] you are trying to check if it's
>> p2m_invalid? But p2m_ram_rw is 0, and p2m_invalid is 1. So may be it
>> should be checked like memtype[a.hvmmem_type] < 0 and initialize the
>> holes with -1.
>
> No. As said, I want to avoid explicit initializers for unused slots,
> and hence it has to be zero that gets checked against.
>

But "!memtype[a.hvmmem_type]" is indicating the p2m type is p2m_ram_rw,
which should be allowed here...

>> But I still wonder is this really necessary? Because we only have one
>> hole in this array in the forseeable future.
>
> How do you know?
>

I had thought I'm the only one proposing to add another HVMMEM type.
Maybe I shall not have this speculation.

> Jan
>
>

Yu

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

Re: [PATCH for 4.7] Remove HVMMEM_mmio_write_dm from the public interface.

[Andrew Cooper]

On 28/04/16 12:59, Wei Liu wrote:
> On Thu, Apr 28, 2016 at 07:40:45PM +0800, Yu, Zhang wrote:
>> Thanks Jan. And I admire your rigorous thought. :)
>>
>> On 4/28/2016 6:57 PM, Jan Beulich wrote:
>>>>>> On 28.04.16 at 12:42, <george.dunlap@citrix.com> wrote:
>>>> On 28/04/16 11:22, Jan Beulich wrote:
>>>>>>>> On 28.04.16 at 10:29, <yu.c.zhang@linux.intel.com> wrote:
>>>>>> @@ -5529,7 +5527,7 @@ long do_hvm_op(unsigned long op,
>>>> XEN_GUEST_HANDLE_PARAM(void) arg)
>>>>>>             [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_unused] = p2m_invalid
>>>>> Why don't you simply delete the old line, without replacement?
>> Well, I did not delete the old line, because in my coming patch(the
>> p2m renaming code), I'm planning to introduce the HVMMEM_ioreq_server,
>> which is HVMMEM_unused+1. And I do not want the check of a.hvmmem_type
>> against HVMMEN_unused later in this routine appear in that patch.
>>
>>>> That might have been slightly cleaner; but we're going to have to put it
>>>> back as soon as the development window opens anyway, so I don't really
>>>> see the point of going through the effort of respinning the patch again.
>>>>
>>>> Would you be willing to ack this version anyway?
>>> I have no problem doing so (and in fact I have it on my to by
>>> committed list already), it is just looked slightly confusing (and
>>> I had already typed half a reply that this isn't what was discussed
>>> until I properly looked at the next hunk), and hence I wanted to
>>> understand the motivation. And btw., I'm not convinced it would
>>> need to be put there anyway later: I don't view the used
>>> mechanism as a good (read: extensible) one to deal with what
>>> would be holes in the array above. Indeed we can't leave them
>>> uninitialized (as that would mean p2m_ram_rw), but I think we
>>> should better find a way to initialize _all_ unused slots without
>>> requiring an initializer for each of them. Sadly the desire to allow
>>> compilation with clang prohibits the most natural solution:
>>>
>>>        static const p2m_type_t memtype[] = {
>>>            [0 ... <upper-bound> - 1] = p2m_invalid,
>> Not sure if this will compile? Can have a try. :)
>>
> To answer your question this can compile with gcc but not probably not
> with clang. This syntax is gcc extension.
>
> See: https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html

That syntax works in Clang, but will subsequent entries in the list will
suffer a -Werror,-Winitializer-overrides and fail to compile.

I already had to fix two examples of this to get clang support working
in the past.

(It is a real shame that p2m_invalid doesn't have the value 0)

~Andrew

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

Re: [PATCH for 4.7] Remove HVMMEM_mmio_write_dm from the public interface.

[Wei Liu]

On Thu, Apr 28, 2016 at 01:00:57PM +0100, Andrew Cooper wrote:
> On 28/04/16 12:59, Wei Liu wrote:
> > On Thu, Apr 28, 2016 at 07:40:45PM +0800, Yu, Zhang wrote:
> >> Thanks Jan. And I admire your rigorous thought. :)
> >>
> >> On 4/28/2016 6:57 PM, Jan Beulich wrote:
> >>>>>> On 28.04.16 at 12:42, <george.dunlap@citrix.com> wrote:
> >>>> On 28/04/16 11:22, Jan Beulich wrote:
> >>>>>>>> On 28.04.16 at 10:29, <yu.c.zhang@linux.intel.com> wrote:
> >>>>>> @@ -5529,7 +5527,7 @@ long do_hvm_op(unsigned long op,
> >>>> XEN_GUEST_HANDLE_PARAM(void) arg)
> >>>>>>             [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_unused] = p2m_invalid
> >>>>> Why don't you simply delete the old line, without replacement?
> >> Well, I did not delete the old line, because in my coming patch(the
> >> p2m renaming code), I'm planning to introduce the HVMMEM_ioreq_server,
> >> which is HVMMEM_unused+1. And I do not want the check of a.hvmmem_type
> >> against HVMMEN_unused later in this routine appear in that patch.
> >>
> >>>> That might have been slightly cleaner; but we're going to have to put it
> >>>> back as soon as the development window opens anyway, so I don't really
> >>>> see the point of going through the effort of respinning the patch again.
> >>>>
> >>>> Would you be willing to ack this version anyway?
> >>> I have no problem doing so (and in fact I have it on my to by
> >>> committed list already), it is just looked slightly confusing (and
> >>> I had already typed half a reply that this isn't what was discussed
> >>> until I properly looked at the next hunk), and hence I wanted to
> >>> understand the motivation. And btw., I'm not convinced it would
> >>> need to be put there anyway later: I don't view the used
> >>> mechanism as a good (read: extensible) one to deal with what
> >>> would be holes in the array above. Indeed we can't leave them
> >>> uninitialized (as that would mean p2m_ram_rw), but I think we
> >>> should better find a way to initialize _all_ unused slots without
> >>> requiring an initializer for each of them. Sadly the desire to allow
> >>> compilation with clang prohibits the most natural solution:
> >>>
> >>>        static const p2m_type_t memtype[] = {
> >>>            [0 ... <upper-bound> - 1] = p2m_invalid,
> >> Not sure if this will compile? Can have a try. :)
> >>
> > To answer your question this can compile with gcc but not probably not
> > with clang. This syntax is gcc extension.
> >
> > See: https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html
> 
> That syntax works in Clang, but will subsequent entries in the list will
> suffer a -Werror,-Winitializer-overrides and fail to compile.
> 

This can easily be fixed :-)

 [ 0 ... <first-upper-bound> ] = p2m_inavlid;
 [ <second-lower-bound> ...  <second-upper-bound> ] = p2m_invalid;

But I'm not sure whether you guys think this is pretty or ugly.


Wei.

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

Re: [PATCH for 4.7] Remove HVMMEM_mmio_write_dm from the public interface.

[Yu, Zhang]

On 4/28/2016 8:06 PM, Wei Liu wrote:
> On Thu, Apr 28, 2016 at 01:00:57PM +0100, Andrew Cooper wrote:
>> On 28/04/16 12:59, Wei Liu wrote:
>>> On Thu, Apr 28, 2016 at 07:40:45PM +0800, Yu, Zhang wrote:
>>>> Thanks Jan. And I admire your rigorous thought. :)
>>>>
>>>> On 4/28/2016 6:57 PM, Jan Beulich wrote:
>>>>>>>> On 28.04.16 at 12:42, <george.dunlap@citrix.com> wrote:
>>>>>> On 28/04/16 11:22, Jan Beulich wrote:
>>>>>>>>>> On 28.04.16 at 10:29, <yu.c.zhang@linux.intel.com> wrote:
>>>>>>>> @@ -5529,7 +5527,7 @@ long do_hvm_op(unsigned long op,
>>>>>> XEN_GUEST_HANDLE_PARAM(void) arg)
>>>>>>>>             [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_unused] = p2m_invalid
>>>>>>> Why don't you simply delete the old line, without replacement?
>>>> Well, I did not delete the old line, because in my coming patch(the
>>>> p2m renaming code), I'm planning to introduce the HVMMEM_ioreq_server,
>>>> which is HVMMEM_unused+1. And I do not want the check of a.hvmmem_type
>>>> against HVMMEN_unused later in this routine appear in that patch.
>>>>
>>>>>> That might have been slightly cleaner; but we're going to have to put it
>>>>>> back as soon as the development window opens anyway, so I don't really
>>>>>> see the point of going through the effort of respinning the patch again.
>>>>>>
>>>>>> Would you be willing to ack this version anyway?
>>>>> I have no problem doing so (and in fact I have it on my to by
>>>>> committed list already), it is just looked slightly confusing (and
>>>>> I had already typed half a reply that this isn't what was discussed
>>>>> until I properly looked at the next hunk), and hence I wanted to
>>>>> understand the motivation. And btw., I'm not convinced it would
>>>>> need to be put there anyway later: I don't view the used
>>>>> mechanism as a good (read: extensible) one to deal with what
>>>>> would be holes in the array above. Indeed we can't leave them
>>>>> uninitialized (as that would mean p2m_ram_rw), but I think we
>>>>> should better find a way to initialize _all_ unused slots without
>>>>> requiring an initializer for each of them. Sadly the desire to allow
>>>>> compilation with clang prohibits the most natural solution:
>>>>>
>>>>>        static const p2m_type_t memtype[] = {
>>>>>            [0 ... <upper-bound> - 1] = p2m_invalid,
>>>> Not sure if this will compile? Can have a try. :)
>>>>
>>> To answer your question this can compile with gcc but not probably not
>>> with clang. This syntax is gcc extension.
>>>
>>> See: https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html
>>
>> That syntax works in Clang, but will subsequent entries in the list will
>> suffer a -Werror,-Winitializer-overrides and fail to compile.
>>
>
> This can easily be fixed :-)
>
>  [ 0 ... <first-upper-bound> ] = p2m_inavlid;
>  [ <second-lower-bound> ...  <second-upper-bound> ] = p2m_invalid;
>
> But I'm not sure whether you guys think this is pretty or ugly.
>

Thanks for your information, Wei. :)
But <first-upper-bound> and <second-lower-bound> ... <second-upper
bound> seems to be holes in this array.

I'm still confused why do we need this, especially at such critical
moment. IIUC HVMMEM type is used to get/set mem type, why would someone
define a HVMMEM type but not use it here?

I know HVMMEM_mmio_write_dm is unused now, but I do not think this
should be a common case in the future.

Frankly, I had thought to remove the HVMMEM_unused in the set_mem_type
code, I choose not to do so, because I do not wanna  the check of
a.hvmmem_type against HVMMEN_unused to pop in my next patch, and I do
not think keeping this will harm any functionality. :)

>
> Wei.
>

Yu

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

Re: [PATCH for 4.7] Remove HVMMEM_mmio_write_dm from the public interface.

[Jan Beulich]

>>> On 28.04.16 at 14:00, <yu.c.zhang@linux.intel.com> wrote:

> 
> On 4/28/2016 7:52 PM, Jan Beulich wrote:
>>>>> On 28.04.16 at 13:40, <yu.c.zhang@linux.intel.com> wrote:
>>> On 4/28/2016 6:57 PM, Jan Beulich wrote:
>>>>>>> On 28.04.16 at 12:42, <george.dunlap@citrix.com> wrote:
>>>>> That might have been slightly cleaner; but we're going to have to put it
>>>>> back as soon as the development window opens anyway, so I don't really
>>>>> see the point of going through the effort of respinning the patch again.
>>>>>
>>>>> Would you be willing to ack this version anyway?
>>>>
>>>> I have no problem doing so (and in fact I have it on my to by
>>>> committed list already), it is just looked slightly confusing (and
>>>> I had already typed half a reply that this isn't what was discussed
>>>> until I properly looked at the next hunk), and hence I wanted to
>>>> understand the motivation. And btw., I'm not convinced it would
>>>> need to be put there anyway later: I don't view the used
>>>> mechanism as a good (read: extensible) one to deal with what
>>>> would be holes in the array above. Indeed we can't leave them
>>>> uninitialized (as that would mean p2m_ram_rw), but I think we
>>>> should better find a way to initialize _all_ unused slots without
>>>> requiring an initializer for each of them. Sadly the desire to allow
>>>> compilation with clang prohibits the most natural solution:
>>>>
>>>>         static const p2m_type_t memtype[] = {
>>>>             [0 ... <upper-bound> - 1] = p2m_invalid,
>>>
>>> Not sure if this will compile? Can have a try. :)
>>
>> gcc will like it, but as said clang won't (afair).
>>
>>>>             [HVMMEM_ram_rw]  = p2m_ram_rw,
>>>>             [HVMMEM_ram_ro]  = p2m_ram_ro,
>>>>             [HVMMEM_mmio_dm] = p2m_mmio_dm,
>>>>         };
>>>>
>>>> Maybe we could do (altering the second hunk of this patch)
>>>>
>>>> @@ -5553,7 +5551,10 @@ long do_hvm_op(unsigned long op,
>>> XEN_GUEST_HANDLE_PARAM(void) arg)
>>>>               ((a.first_pfn + a.nr - 1) > domain_get_maximum_gpfn(d)) )
>>>>              goto setmemtype_fail;
>>>>
>>>> -        if ( a.hvmmem_type >= ARRAY_SIZE(memtype) )
>>>> +        BUILD_BUG_ON(p2m_ram_rw);
>>>> +        BUILD_BUG_ON(HVMMEM_ram_rw);
>>>> +        if ( a.hvmmem_type >= ARRAY_SIZE(memtype) ||
>>>> +             (a.hvmmem_type && !memtype[a.hvmmem_type]) )
>>>
>>> I guess by !memtype[a.hvmmem_type] you are trying to check if it's
>>> p2m_invalid? But p2m_ram_rw is 0, and p2m_invalid is 1. So may be it
>>> should be checked like memtype[a.hvmmem_type] < 0 and initialize the
>>> holes with -1.
>>
>> No. As said, I want to avoid explicit initializers for unused slots,
>> and hence it has to be zero that gets checked against.
>>
> 
> But "!memtype[a.hvmmem_type]" is indicating the p2m type is p2m_ram_rw,
> which should be allowed here...

Hence the additional check for a.hvmmem_type to not be zero
(that's the only thing mapping to p2m_ram_rw).

Jan


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

Re: [PATCH for 4.7] Remove HVMMEM_mmio_write_dm from the public interface.

[Jan Beulich]

>>> On 28.04.16 at 14:06, <wei.liu2@citrix.com> wrote:
> On Thu, Apr 28, 2016 at 01:00:57PM +0100, Andrew Cooper wrote:
>> On 28/04/16 12:59, Wei Liu wrote:
>> > On Thu, Apr 28, 2016 at 07:40:45PM +0800, Yu, Zhang wrote:
>> >> Thanks Jan. And I admire your rigorous thought. :)
>> >>
>> >> On 4/28/2016 6:57 PM, Jan Beulich wrote:
>> >>>>>> On 28.04.16 at 12:42, <george.dunlap@citrix.com> wrote:
>> >>>> On 28/04/16 11:22, Jan Beulich wrote:
>> >>>>>>>> On 28.04.16 at 10:29, <yu.c.zhang@linux.intel.com> wrote:
>> >>>>>> @@ -5529,7 +5527,7 @@ long do_hvm_op(unsigned long op,
>> >>>> XEN_GUEST_HANDLE_PARAM(void) arg)
>> >>>>>>             [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_unused] = p2m_invalid
>> >>>>> Why don't you simply delete the old line, without replacement?
>> >> Well, I did not delete the old line, because in my coming patch(the
>> >> p2m renaming code), I'm planning to introduce the HVMMEM_ioreq_server,
>> >> which is HVMMEM_unused+1. And I do not want the check of a.hvmmem_type
>> >> against HVMMEN_unused later in this routine appear in that patch.
>> >>
>> >>>> That might have been slightly cleaner; but we're going to have to put it
>> >>>> back as soon as the development window opens anyway, so I don't really
>> >>>> see the point of going through the effort of respinning the patch again.
>> >>>>
>> >>>> Would you be willing to ack this version anyway?
>> >>> I have no problem doing so (and in fact I have it on my to by
>> >>> committed list already), it is just looked slightly confusing (and
>> >>> I had already typed half a reply that this isn't what was discussed
>> >>> until I properly looked at the next hunk), and hence I wanted to
>> >>> understand the motivation. And btw., I'm not convinced it would
>> >>> need to be put there anyway later: I don't view the used
>> >>> mechanism as a good (read: extensible) one to deal with what
>> >>> would be holes in the array above. Indeed we can't leave them
>> >>> uninitialized (as that would mean p2m_ram_rw), but I think we
>> >>> should better find a way to initialize _all_ unused slots without
>> >>> requiring an initializer for each of them. Sadly the desire to allow
>> >>> compilation with clang prohibits the most natural solution:
>> >>>
>> >>>        static const p2m_type_t memtype[] = {
>> >>>            [0 ... <upper-bound> - 1] = p2m_invalid,
>> >> Not sure if this will compile? Can have a try. :)
>> >>
>> > To answer your question this can compile with gcc but not probably not
>> > with clang. This syntax is gcc extension.
>> >
>> > See: https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html 
>> 
>> That syntax works in Clang, but will subsequent entries in the list will
>> suffer a -Werror,-Winitializer-overrides and fail to compile.
>> 
> 
> This can easily be fixed :-)
> 
>  [ 0 ... <first-upper-bound> ] = p2m_inavlid;
>  [ <second-lower-bound> ...  <second-upper-bound> ] = p2m_invalid;
> 
> But I'm not sure whether you guys think this is pretty or ugly.

What if multiple holes show up in the future? The goal really is to
deal with all holes in one line, once and for all.

Jan


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

Re: [PATCH for 4.7] Remove HVMMEM_mmio_write_dm from the public interface.

[Jan Beulich]

>>> On 28.04.16 at 14:12, <yu.c.zhang@linux.intel.com> wrote:
> I'm still confused why do we need this, especially at such critical
> moment. IIUC HVMMEM type is used to get/set mem type, why would someone
> define a HVMMEM type but not use it here?

Who knows. And as said - the patch can go in as is, I just inquired
because I like to avoid future code churn whenever possible, i.e.
when a certain way of coding makes it less likely for the code
needing touching again compared to some other variant, I'd
generally like that to be used (as long as it's not meaningfully worse
in other respects).

Jan


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

Re: [PATCH for 4.7] Remove HVMMEM_mmio_write_dm from the public interface.

[Wei Liu]

On Thu, Apr 28, 2016 at 06:34:48AM -0600, Jan Beulich wrote:
> >>> On 28.04.16 at 14:06, <wei.liu2@citrix.com> wrote:
> > On Thu, Apr 28, 2016 at 01:00:57PM +0100, Andrew Cooper wrote:
> >> On 28/04/16 12:59, Wei Liu wrote:
> >> > On Thu, Apr 28, 2016 at 07:40:45PM +0800, Yu, Zhang wrote:
> >> >> Thanks Jan. And I admire your rigorous thought. :)
> >> >>
> >> >> On 4/28/2016 6:57 PM, Jan Beulich wrote:
> >> >>>>>> On 28.04.16 at 12:42, <george.dunlap@citrix.com> wrote:
> >> >>>> On 28/04/16 11:22, Jan Beulich wrote:
> >> >>>>>>>> On 28.04.16 at 10:29, <yu.c.zhang@linux.intel.com> wrote:
> >> >>>>>> @@ -5529,7 +5527,7 @@ long do_hvm_op(unsigned long op,
> >> >>>> XEN_GUEST_HANDLE_PARAM(void) arg)
> >> >>>>>>             [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_unused] = p2m_invalid
> >> >>>>> Why don't you simply delete the old line, without replacement?
> >> >> Well, I did not delete the old line, because in my coming patch(the
> >> >> p2m renaming code), I'm planning to introduce the HVMMEM_ioreq_server,
> >> >> which is HVMMEM_unused+1. And I do not want the check of a.hvmmem_type
> >> >> against HVMMEN_unused later in this routine appear in that patch.
> >> >>
> >> >>>> That might have been slightly cleaner; but we're going to have to put it
> >> >>>> back as soon as the development window opens anyway, so I don't really
> >> >>>> see the point of going through the effort of respinning the patch again.
> >> >>>>
> >> >>>> Would you be willing to ack this version anyway?
> >> >>> I have no problem doing so (and in fact I have it on my to by
> >> >>> committed list already), it is just looked slightly confusing (and
> >> >>> I had already typed half a reply that this isn't what was discussed
> >> >>> until I properly looked at the next hunk), and hence I wanted to
> >> >>> understand the motivation. And btw., I'm not convinced it would
> >> >>> need to be put there anyway later: I don't view the used
> >> >>> mechanism as a good (read: extensible) one to deal with what
> >> >>> would be holes in the array above. Indeed we can't leave them
> >> >>> uninitialized (as that would mean p2m_ram_rw), but I think we
> >> >>> should better find a way to initialize _all_ unused slots without
> >> >>> requiring an initializer for each of them. Sadly the desire to allow
> >> >>> compilation with clang prohibits the most natural solution:
> >> >>>
> >> >>>        static const p2m_type_t memtype[] = {
> >> >>>            [0 ... <upper-bound> - 1] = p2m_invalid,
> >> >> Not sure if this will compile? Can have a try. :)
> >> >>
> >> > To answer your question this can compile with gcc but not probably not
> >> > with clang. This syntax is gcc extension.
> >> >
> >> > See: https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html 
> >> 
> >> That syntax works in Clang, but will subsequent entries in the list will
> >> suffer a -Werror,-Winitializer-overrides and fail to compile.
> >> 
> > 
> > This can easily be fixed :-)
> > 
> >  [ 0 ... <first-upper-bound> ] = p2m_inavlid;
> >  [ <second-lower-bound> ...  <second-upper-bound> ] = p2m_invalid;
> > 
> > But I'm not sure whether you guys think this is pretty or ugly.
> 
> What if multiple holes show up in the future? The goal really is to
> deal with all holes in one line, once and for all.
> 

It's up to you to decide what to do. I don't have further suggestions
really.

Wei.

> Jan
> 

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

Re: [PATCH for 4.7] Remove HVMMEM_mmio_write_dm from the public interface.

[Yu, Zhang]

On 4/28/2016 8:39 PM, Jan Beulich wrote:
>>>> On 28.04.16 at 14:12, <yu.c.zhang@linux.intel.com> wrote:
>> I'm still confused why do we need this, especially at such critical
>> moment. IIUC HVMMEM type is used to get/set mem type, why would someone
>> define a HVMMEM type but not use it here?
>
> Who knows. And as said - the patch can go in as is, I just inquired
> because I like to avoid future code churn whenever possible, i.e.
> when a certain way of coding makes it less likely for the code
> needing touching again compared to some other variant, I'd
> generally like that to be used (as long as it's not meaningfully worse
> in other respects).
>

Thanks Jan.
So my understanding is that this patch does not need any change any
more.

As to your concern, I still do not have any better thought.
And this hole is a problem because of the old mistake I have made in
previous version. Could we be careful in the future review to avoid
another hole(besides the HVMMEM_unused one which is unavoidable with
HVMMEM_ioreq_server), and if this can not be avoided, we try to find a
more graceful solution by then?  :)

> Jan
>
>

Yu

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