| Message ID | 1461832197-26772-1-git-send-email-yu.c.zhang@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> -----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
>>> 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
> -----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
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
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. */ >
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 >
>>> 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
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
>>> 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
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.
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
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
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.
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
>>> 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
>>> 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
>>> 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
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 >
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
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. */
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(-)