| Message ID | 0987641ced136706961cf419eb5ed83d1302357b.1576697796.git.tamas.lengyel@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | VM forking | expand |
On 18/12/2019 19:40, Tamas K Lengyel wrote: > diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c > index 614ed60fe4..5a3a962fbb 100644 > --- a/xen/arch/x86/hvm/hvm.c > +++ b/xen/arch/x86/hvm/hvm.c > @@ -4072,16 +4072,17 @@ static int hvmop_set_evtchn_upcall_vector( > } > > static int hvm_allow_set_param(struct domain *d, > - const struct xen_hvm_param *a) > + uint32_t index, > + uint64_t new_value) These two lines can be folded together. > @@ -4134,13 +4135,11 @@ static int hvm_allow_set_param(struct domain *d, > return rc; > } > > -static int hvmop_set_param( > +int hvmop_set_param( > XEN_GUEST_HANDLE_PARAM(xen_hvm_param_t) arg) and here. > @@ -4160,23 +4159,42 @@ static int hvmop_set_param( > if ( !is_hvm_domain(d) ) > goto out; > > - rc = hvm_allow_set_param(d, &a); > + rc = hvm_set_param(d, a.index, a.value); > + > + out: > + rcu_unlock_domain(d); > + return rc; > +} > + > +int hvm_set_param( > + struct domain *d, > + uint32_t index, > + uint64_t value) and again. > @@ -4340,34 +4358,33 @@ static int hvmop_set_param( > * 256 bits interrupt redirection bitmap + 64k bits I/O bitmap > * plus one padding byte). > */ > - if ( (a.value >> 32) > sizeof(struct tss32) + > + if ( (value >> 32) > sizeof(struct tss32) + > (0x100 / 8) + (0x10000 / 8) + 1 ) > - a.value = (uint32_t)a.value | > + value = (uint32_t)value | > ((sizeof(struct tss32) + (0x100 / 8) + > (0x10000 / 8) + 1) << 32); Can you reindent the surrounding lines so they line up again? > @@ -4429,42 +4446,60 @@ static int hvmop_get_param( > if ( !is_hvm_domain(d) ) > goto out; > > - rc = hvm_allow_get_param(d, &a); > + rc = hvm_get_param(d, a.index, &a.value); > if ( rc ) > goto out; > > - switch ( a.index ) > + rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0; > + > + HVM_DBG_LOG(DBG_LEVEL_HCALL, "get param %u = %"PRIx64, > + a.index, a.value); > + > + out: > + rcu_unlock_domain(d); > + return rc; > +} > + > +int hvm_get_param( > + struct domain *d, > + uint32_t index, > + uint64_t *value) Fold. > +{ > + int rc; > + > + if ( index >= HVM_NR_PARAMS || !value ) No need for !value. It had better only ever point onto the hypervisor stack now that this is an internal function. > diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h > index 1d7b66f927..a6f4ae76a1 100644 > --- a/xen/include/asm-x86/hvm/hvm.h > +++ b/xen/include/asm-x86/hvm/hvm.h > @@ -335,6 +335,10 @@ unsigned long hvm_cr4_guest_valid_bits(const struct domain *d, bool restore); > bool hvm_flush_vcpu_tlb(bool (*flush_vcpu)(void *ctxt, struct vcpu *v), > void *ctxt); > > +/* Caller must hold domain locks */ How about asserting so? No major problems so Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
> > --- a/xen/include/asm-x86/hvm/hvm.h > > +++ b/xen/include/asm-x86/hvm/hvm.h > > @@ -335,6 +335,10 @@ unsigned long hvm_cr4_guest_valid_bits(const struct domain *d, bool restore); > > bool hvm_flush_vcpu_tlb(bool (*flush_vcpu)(void *ctxt, struct vcpu *v), > > void *ctxt); > > > > +/* Caller must hold domain locks */ > > How about asserting so? AFAICT there is no "domain_locked_by_me" function, only paging/p2m/gfn_locked_by_me. So any suggestion on how to achieve that? Thanks! Tamas
On 19/12/2019 19:38, Tamas K Lengyel wrote: >>> --- a/xen/include/asm-x86/hvm/hvm.h >>> +++ b/xen/include/asm-x86/hvm/hvm.h >>> @@ -335,6 +335,10 @@ unsigned long hvm_cr4_guest_valid_bits(const struct domain *d, bool restore); >>> bool hvm_flush_vcpu_tlb(bool (*flush_vcpu)(void *ctxt, struct vcpu *v), >>> void *ctxt); >>> >>> +/* Caller must hold domain locks */ >> How about asserting so? > AFAICT there is no "domain_locked_by_me" function, only > paging/p2m/gfn_locked_by_me. So any suggestion on how to achieve that? spin_is_locked() gets you most of the way, and would be a start. But yes - now you say this, I remember that we don't currently have suitable infrastructure. ~Andrew
On Thu, Dec 19, 2019 at 12:41 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote: > > On 19/12/2019 19:38, Tamas K Lengyel wrote: > >>> --- a/xen/include/asm-x86/hvm/hvm.h > >>> +++ b/xen/include/asm-x86/hvm/hvm.h > >>> @@ -335,6 +335,10 @@ unsigned long hvm_cr4_guest_valid_bits(const struct domain *d, bool restore); > >>> bool hvm_flush_vcpu_tlb(bool (*flush_vcpu)(void *ctxt, struct vcpu *v), > >>> void *ctxt); > >>> > >>> +/* Caller must hold domain locks */ > >> How about asserting so? > > AFAICT there is no "domain_locked_by_me" function, only > > paging/p2m/gfn_locked_by_me. So any suggestion on how to achieve that? > > spin_is_locked() gets you most of the way, and would be a start. > > But yes - now you say this, I remember that we don't currently have > suitable infrastructure. Is the domain lock even a spin lock (the on you use by rcu_lock_live_remote_domain_by_id)? Looks to me it just goes down to "rcu_read_lock" which only does a preempt_disable() call o.O Tamas
On 19/12/2019 19:49, Tamas K Lengyel wrote: > On Thu, Dec 19, 2019 at 12:41 PM Andrew Cooper > <andrew.cooper3@citrix.com> wrote: >> On 19/12/2019 19:38, Tamas K Lengyel wrote: >>>>> --- a/xen/include/asm-x86/hvm/hvm.h >>>>> +++ b/xen/include/asm-x86/hvm/hvm.h >>>>> @@ -335,6 +335,10 @@ unsigned long hvm_cr4_guest_valid_bits(const struct domain *d, bool restore); >>>>> bool hvm_flush_vcpu_tlb(bool (*flush_vcpu)(void *ctxt, struct vcpu *v), >>>>> void *ctxt); >>>>> >>>>> +/* Caller must hold domain locks */ >>>> How about asserting so? >>> AFAICT there is no "domain_locked_by_me" function, only >>> paging/p2m/gfn_locked_by_me. So any suggestion on how to achieve that? >> spin_is_locked() gets you most of the way, and would be a start. >> >> But yes - now you say this, I remember that we don't currently have >> suitable infrastructure. > Is the domain lock even a spin lock (the on you use by > rcu_lock_live_remote_domain_by_id)? Looks to me it just goes down to > "rcu_read_lock" which only does a preempt_disable() call o.O /sigh - of course we have two things called the domain lock. The RCU one is to ensure that the struct domain can't get freed behind our back, and shouldn't be interesting in this context (obtaining the d pointer in the first place causes it to be safe). If that is the only one which matters, I would drop the comment. ~Andrew
On Thu, Dec 19, 2019 at 12:57 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote: > > On 19/12/2019 19:49, Tamas K Lengyel wrote: > > On Thu, Dec 19, 2019 at 12:41 PM Andrew Cooper > > <andrew.cooper3@citrix.com> wrote: > >> On 19/12/2019 19:38, Tamas K Lengyel wrote: > >>>>> --- a/xen/include/asm-x86/hvm/hvm.h > >>>>> +++ b/xen/include/asm-x86/hvm/hvm.h > >>>>> @@ -335,6 +335,10 @@ unsigned long hvm_cr4_guest_valid_bits(const struct domain *d, bool restore); > >>>>> bool hvm_flush_vcpu_tlb(bool (*flush_vcpu)(void *ctxt, struct vcpu *v), > >>>>> void *ctxt); > >>>>> > >>>>> +/* Caller must hold domain locks */ > >>>> How about asserting so? > >>> AFAICT there is no "domain_locked_by_me" function, only > >>> paging/p2m/gfn_locked_by_me. So any suggestion on how to achieve that? > >> spin_is_locked() gets you most of the way, and would be a start. > >> > >> But yes - now you say this, I remember that we don't currently have > >> suitable infrastructure. > > Is the domain lock even a spin lock (the on you use by > > rcu_lock_live_remote_domain_by_id)? Looks to me it just goes down to > > "rcu_read_lock" which only does a preempt_disable() call o.O > > /sigh - of course we have two things called the domain lock. > > The RCU one is to ensure that the struct domain can't get freed behind > our back, and shouldn't be interesting in this context (obtaining the d > pointer in the first place causes it to be safe). If that is the only > one which matters, I would drop the comment. Yes, only the RCU lock gets taken right now by all callers, so I'll drop the comment. Thanks, Tamas
On 18.12.2019 20:40, Tamas K Lengyel wrote: > Currently the hvm parameters are only accessible via the HVMOP hypercalls. By > exposing hvm_{get/set}_param it will be possible for VM forking to copy the > parameters directly into the clone domain. Having peeked ahead at patch 17, where this gets used, I wonder why you want a pair of one-by-one functions, rather than a copy-all one. This then wouldn't require exposure of the functions you touch here. > @@ -4429,42 +4446,60 @@ static int hvmop_get_param( > if ( !is_hvm_domain(d) ) > goto out; > > - rc = hvm_allow_get_param(d, &a); > + rc = hvm_get_param(d, a.index, &a.value); > if ( rc ) > goto out; > > - switch ( a.index ) > + rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0; > + > + HVM_DBG_LOG(DBG_LEVEL_HCALL, "get param %u = %"PRIx64, > + a.index, a.value); > + > + out: > + rcu_unlock_domain(d); > + return rc; > +} > + > +int hvm_get_param( > + struct domain *d, If this is to be non-static, I think it would be quite nice if this parameter was const. This will take a prereq patch to constify the XSM path involved, but other than this I can't see anything getting in the way. Jan
On Fri, Dec 20, 2019 at 9:47 AM Jan Beulich <jbeulich@suse.com> wrote: > > On 18.12.2019 20:40, Tamas K Lengyel wrote: > > Currently the hvm parameters are only accessible via the HVMOP hypercalls. By > > exposing hvm_{get/set}_param it will be possible for VM forking to copy the > > parameters directly into the clone domain. > > Having peeked ahead at patch 17, where this gets used, I wonder why > you want a pair of one-by-one functions, rather than a copy-all one. > This then wouldn't require exposure of the functions you touch here. Well, provided there is no such function in existence today it was just easier to use what's already available. I still wouldn't want to implement a one-shot function like that because this same code-path is shared by the save-restore operations on the toolstack side, so at least I have a reasonable assumption that it won't break on me in the future. > > @@ -4429,42 +4446,60 @@ static int hvmop_get_param( > > if ( !is_hvm_domain(d) ) > > goto out; > > > > - rc = hvm_allow_get_param(d, &a); > > + rc = hvm_get_param(d, a.index, &a.value); > > if ( rc ) > > goto out; > > > > - switch ( a.index ) > > + rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0; > > + > > + HVM_DBG_LOG(DBG_LEVEL_HCALL, "get param %u = %"PRIx64, > > + a.index, a.value); > > + > > + out: > > + rcu_unlock_domain(d); > > + return rc; > > +} > > + > > +int hvm_get_param( > > + struct domain *d, > > If this is to be non-static, I think it would be quite nice if > this parameter was const. This will take a prereq patch to > constify the XSM path involved, but other than this I can't > see anything getting in the way. Sure.
On 20/12/2019 17:27, Tamas K Lengyel wrote: > On Fri, Dec 20, 2019 at 9:47 AM Jan Beulich <jbeulich@suse.com> wrote: >> On 18.12.2019 20:40, Tamas K Lengyel wrote: >>> Currently the hvm parameters are only accessible via the HVMOP hypercalls. By >>> exposing hvm_{get/set}_param it will be possible for VM forking to copy the >>> parameters directly into the clone domain. >> Having peeked ahead at patch 17, where this gets used, I wonder why >> you want a pair of one-by-one functions, rather than a copy-all one. >> This then wouldn't require exposure of the functions you touch here. > Well, provided there is no such function in existence today it was > just easier to use what's already available. I still wouldn't want to > implement a one-shot function like that because this same code-path is > shared by the save-restore operations on the toolstack side, so at > least I have a reasonable assumption that it won't break on me in the > future. In particular, a number of the set operations are distinctly non-trivial. (OTOH, those are not long for this world, and should be creation X86_EMU_* constants instead). ~Andrew
On Fri, Dec 20, 2019 at 10:32 AM Andrew Cooper <andrew.cooper3@citrix.com> wrote: > > On 20/12/2019 17:27, Tamas K Lengyel wrote: > > On Fri, Dec 20, 2019 at 9:47 AM Jan Beulich <jbeulich@suse.com> wrote: > >> On 18.12.2019 20:40, Tamas K Lengyel wrote: > >>> Currently the hvm parameters are only accessible via the HVMOP hypercalls. By > >>> exposing hvm_{get/set}_param it will be possible for VM forking to copy the > >>> parameters directly into the clone domain. > >> Having peeked ahead at patch 17, where this gets used, I wonder why > >> you want a pair of one-by-one functions, rather than a copy-all one. > >> This then wouldn't require exposure of the functions you touch here. > > Well, provided there is no such function in existence today it was > > just easier to use what's already available. I still wouldn't want to > > implement a one-shot function like that because this same code-path is > > shared by the save-restore operations on the toolstack side, so at > > least I have a reasonable assumption that it won't break on me in the > > future. > > In particular, a number of the set operations are distinctly > non-trivial. (OTOH, those are not long for this world, and should be > creation X86_EMU_* constants instead). > I actually wanted to ask about that. In https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=tools/libxc/xc_sr_save_x86_hvm.c;h=97a8c49807f192c47209525f51e4d79a50c66cec;hb=HEAD#l61 the toolstack only selects certain HVM params to be saved (and restored later). I originally followed the same logic in the fork code-path but after a lot of experiments it looks like it's actually OK to grab all params but only call set_param on the ones that have a non-zero value. So setting some params with a zero value has certainly lead to crashes, but otherwise it seems to "just work" to copy all the rest. Tamas
On 20/12/2019 17:36, Tamas K Lengyel wrote: > On Fri, Dec 20, 2019 at 10:32 AM Andrew Cooper > <andrew.cooper3@citrix.com> wrote: >> On 20/12/2019 17:27, Tamas K Lengyel wrote: >>> On Fri, Dec 20, 2019 at 9:47 AM Jan Beulich <jbeulich@suse.com> wrote: >>>> On 18.12.2019 20:40, Tamas K Lengyel wrote: >>>>> Currently the hvm parameters are only accessible via the HVMOP hypercalls. By >>>>> exposing hvm_{get/set}_param it will be possible for VM forking to copy the >>>>> parameters directly into the clone domain. >>>> Having peeked ahead at patch 17, where this gets used, I wonder why >>>> you want a pair of one-by-one functions, rather than a copy-all one. >>>> This then wouldn't require exposure of the functions you touch here. >>> Well, provided there is no such function in existence today it was >>> just easier to use what's already available. I still wouldn't want to >>> implement a one-shot function like that because this same code-path is >>> shared by the save-restore operations on the toolstack side, so at >>> least I have a reasonable assumption that it won't break on me in the >>> future. >> In particular, a number of the set operations are distinctly >> non-trivial. (OTOH, those are not long for this world, and should be >> creation X86_EMU_* constants instead). >> > I actually wanted to ask about that. In > https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=tools/libxc/xc_sr_save_x86_hvm.c;h=97a8c49807f192c47209525f51e4d79a50c66cec;hb=HEAD#l61 > the toolstack only selects certain HVM params to be saved (and > restored later). I originally followed the same logic in the fork > code-path but after a lot of experiments it looks like it's actually > OK to grab all params but only call set_param on the ones that have a > non-zero value. So setting some params with a zero value has certainly > lead to crashes, but otherwise it seems to "just work" to copy all the > rest. I think you're trying to ascribe any form of design/plan to a system which had none. :) The code you quote was like that because that is how legacy migration worked. That said, eliding empty records was an effort-saving exercise (avoid redundant hypercalls on destination side), not because there was any suggestion that attempting to explicitly set 0 would crash. Do you have any idea which param was causing problems? ~Andrew
On Fri, Dec 20, 2019 at 10:47 AM Andrew Cooper <andrew.cooper3@citrix.com> wrote: > > On 20/12/2019 17:36, Tamas K Lengyel wrote: > > On Fri, Dec 20, 2019 at 10:32 AM Andrew Cooper > > <andrew.cooper3@citrix.com> wrote: > >> On 20/12/2019 17:27, Tamas K Lengyel wrote: > >>> On Fri, Dec 20, 2019 at 9:47 AM Jan Beulich <jbeulich@suse.com> wrote: > >>>> On 18.12.2019 20:40, Tamas K Lengyel wrote: > >>>>> Currently the hvm parameters are only accessible via the HVMOP hypercalls. By > >>>>> exposing hvm_{get/set}_param it will be possible for VM forking to copy the > >>>>> parameters directly into the clone domain. > >>>> Having peeked ahead at patch 17, where this gets used, I wonder why > >>>> you want a pair of one-by-one functions, rather than a copy-all one. > >>>> This then wouldn't require exposure of the functions you touch here. > >>> Well, provided there is no such function in existence today it was > >>> just easier to use what's already available. I still wouldn't want to > >>> implement a one-shot function like that because this same code-path is > >>> shared by the save-restore operations on the toolstack side, so at > >>> least I have a reasonable assumption that it won't break on me in the > >>> future. > >> In particular, a number of the set operations are distinctly > >> non-trivial. (OTOH, those are not long for this world, and should be > >> creation X86_EMU_* constants instead). > >> > > I actually wanted to ask about that. In > > https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=tools/libxc/xc_sr_save_x86_hvm.c;h=97a8c49807f192c47209525f51e4d79a50c66cec;hb=HEAD#l61 > > the toolstack only selects certain HVM params to be saved (and > > restored later). I originally followed the same logic in the fork > > code-path but after a lot of experiments it looks like it's actually > > OK to grab all params but only call set_param on the ones that have a > > non-zero value. So setting some params with a zero value has certainly > > lead to crashes, but otherwise it seems to "just work" to copy all the > > rest. > > I think you're trying to ascribe any form of design/plan to a system > which had none. :) > > The code you quote was like that because that is how legacy migration > worked. That said, eliding empty records was an effort-saving exercise > (avoid redundant hypercalls on destination side), not because there was > any suggestion that attempting to explicitly set 0 would crash. > > Do you have any idea which param was causing problems? Yes, HVM_PARAM_IDENT_PT was one sure. There may have been others (I don't recall now) but simply checking for non-zero value before calling set_param resolved everything. Tamas
On 20/12/2019 17:50, Tamas K Lengyel wrote: > On Fri, Dec 20, 2019 at 10:47 AM Andrew Cooper > <andrew.cooper3@citrix.com> wrote: >> On 20/12/2019 17:36, Tamas K Lengyel wrote: >>> On Fri, Dec 20, 2019 at 10:32 AM Andrew Cooper >>> <andrew.cooper3@citrix.com> wrote: >>>> On 20/12/2019 17:27, Tamas K Lengyel wrote: >>>>> On Fri, Dec 20, 2019 at 9:47 AM Jan Beulich <jbeulich@suse.com> wrote: >>>>>> On 18.12.2019 20:40, Tamas K Lengyel wrote: >>>>>>> Currently the hvm parameters are only accessible via the HVMOP hypercalls. By >>>>>>> exposing hvm_{get/set}_param it will be possible for VM forking to copy the >>>>>>> parameters directly into the clone domain. >>>>>> Having peeked ahead at patch 17, where this gets used, I wonder why >>>>>> you want a pair of one-by-one functions, rather than a copy-all one. >>>>>> This then wouldn't require exposure of the functions you touch here. >>>>> Well, provided there is no such function in existence today it was >>>>> just easier to use what's already available. I still wouldn't want to >>>>> implement a one-shot function like that because this same code-path is >>>>> shared by the save-restore operations on the toolstack side, so at >>>>> least I have a reasonable assumption that it won't break on me in the >>>>> future. >>>> In particular, a number of the set operations are distinctly >>>> non-trivial. (OTOH, those are not long for this world, and should be >>>> creation X86_EMU_* constants instead). >>>> >>> I actually wanted to ask about that. In >>> https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=tools/libxc/xc_sr_save_x86_hvm.c;h=97a8c49807f192c47209525f51e4d79a50c66cec;hb=HEAD#l61 >>> the toolstack only selects certain HVM params to be saved (and >>> restored later). I originally followed the same logic in the fork >>> code-path but after a lot of experiments it looks like it's actually >>> OK to grab all params but only call set_param on the ones that have a >>> non-zero value. So setting some params with a zero value has certainly >>> lead to crashes, but otherwise it seems to "just work" to copy all the >>> rest. >> I think you're trying to ascribe any form of design/plan to a system >> which had none. :) >> >> The code you quote was like that because that is how legacy migration >> worked. That said, eliding empty records was an effort-saving exercise >> (avoid redundant hypercalls on destination side), not because there was >> any suggestion that attempting to explicitly set 0 would crash. >> >> Do you have any idea which param was causing problems? > Yes, HVM_PARAM_IDENT_PT was one sure. There may have been others (I > don't recall now) but simply checking for non-zero value before > calling set_param resolved everything. IDENT_PT is an Westmere(?) wrinkle. There was one processor back in those days which supported EPT, but didn't support VT-x running in unpaged mode. Therefore, we had to fake up unpaged mode by pointing vCR3 at an identity pagetable inside the guests physical address space. The crash won't be from the IDENT_PT itself, but the paging_update_cr3() side effect. Was it a host crash, or guest crash? ~Andrew
On Fri, Dec 20, 2019 at 11:00 AM Andrew Cooper <andrew.cooper3@citrix.com> wrote: > > On 20/12/2019 17:50, Tamas K Lengyel wrote: > > On Fri, Dec 20, 2019 at 10:47 AM Andrew Cooper > > <andrew.cooper3@citrix.com> wrote: > >> On 20/12/2019 17:36, Tamas K Lengyel wrote: > >>> On Fri, Dec 20, 2019 at 10:32 AM Andrew Cooper > >>> <andrew.cooper3@citrix.com> wrote: > >>>> On 20/12/2019 17:27, Tamas K Lengyel wrote: > >>>>> On Fri, Dec 20, 2019 at 9:47 AM Jan Beulich <jbeulich@suse.com> wrote: > >>>>>> On 18.12.2019 20:40, Tamas K Lengyel wrote: > >>>>>>> Currently the hvm parameters are only accessible via the HVMOP hypercalls. By > >>>>>>> exposing hvm_{get/set}_param it will be possible for VM forking to copy the > >>>>>>> parameters directly into the clone domain. > >>>>>> Having peeked ahead at patch 17, where this gets used, I wonder why > >>>>>> you want a pair of one-by-one functions, rather than a copy-all one. > >>>>>> This then wouldn't require exposure of the functions you touch here. > >>>>> Well, provided there is no such function in existence today it was > >>>>> just easier to use what's already available. I still wouldn't want to > >>>>> implement a one-shot function like that because this same code-path is > >>>>> shared by the save-restore operations on the toolstack side, so at > >>>>> least I have a reasonable assumption that it won't break on me in the > >>>>> future. > >>>> In particular, a number of the set operations are distinctly > >>>> non-trivial. (OTOH, those are not long for this world, and should be > >>>> creation X86_EMU_* constants instead). > >>>> > >>> I actually wanted to ask about that. In > >>> https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=tools/libxc/xc_sr_save_x86_hvm.c;h=97a8c49807f192c47209525f51e4d79a50c66cec;hb=HEAD#l61 > >>> the toolstack only selects certain HVM params to be saved (and > >>> restored later). I originally followed the same logic in the fork > >>> code-path but after a lot of experiments it looks like it's actually > >>> OK to grab all params but only call set_param on the ones that have a > >>> non-zero value. So setting some params with a zero value has certainly > >>> lead to crashes, but otherwise it seems to "just work" to copy all the > >>> rest. > >> I think you're trying to ascribe any form of design/plan to a system > >> which had none. :) > >> > >> The code you quote was like that because that is how legacy migration > >> worked. That said, eliding empty records was an effort-saving exercise > >> (avoid redundant hypercalls on destination side), not because there was > >> any suggestion that attempting to explicitly set 0 would crash. > >> > >> Do you have any idea which param was causing problems? > > Yes, HVM_PARAM_IDENT_PT was one sure. There may have been others (I > > don't recall now) but simply checking for non-zero value before > > calling set_param resolved everything. > > IDENT_PT is an Westmere(?) wrinkle. > > There was one processor back in those days which supported EPT, but > didn't support VT-x running in unpaged mode. Therefore, we had to fake > up unpaged mode by pointing vCR3 at an identity pagetable inside the > guests physical address space. Eh, yikes. > > The crash won't be from the IDENT_PT itself, but the paging_update_cr3() > side effect. Was it a host crash, or guest crash? > Yes, that's what I recall after I looked into it. It was a guest a crash as I remember. Tamas
On 20.12.2019 18:32, Andrew Cooper wrote: > On 20/12/2019 17:27, Tamas K Lengyel wrote: >> On Fri, Dec 20, 2019 at 9:47 AM Jan Beulich <jbeulich@suse.com> wrote: >>> On 18.12.2019 20:40, Tamas K Lengyel wrote: >>>> Currently the hvm parameters are only accessible via the HVMOP hypercalls. By >>>> exposing hvm_{get/set}_param it will be possible for VM forking to copy the >>>> parameters directly into the clone domain. >>> Having peeked ahead at patch 17, where this gets used, I wonder why >>> you want a pair of one-by-one functions, rather than a copy-all one. >>> This then wouldn't require exposure of the functions you touch here. >> Well, provided there is no such function in existence today it was >> just easier to use what's already available. I still wouldn't want to >> implement a one-shot function like that because this same code-path is >> shared by the save-restore operations on the toolstack side, so at >> least I have a reasonable assumption that it won't break on me in the >> future. > > In particular, a number of the set operations are distinctly > non-trivial. How is trivial or not related to there being one function doing the looping wanted here vs the looping being done by the caller around the two per-entity calls? Jan
On Mon, Dec 23, 2019 at 2:37 AM Jan Beulich <jbeulich@suse.com> wrote: > > On 20.12.2019 18:32, Andrew Cooper wrote: > > On 20/12/2019 17:27, Tamas K Lengyel wrote: > >> On Fri, Dec 20, 2019 at 9:47 AM Jan Beulich <jbeulich@suse.com> wrote: > >>> On 18.12.2019 20:40, Tamas K Lengyel wrote: > >>>> Currently the hvm parameters are only accessible via the HVMOP hypercalls. By > >>>> exposing hvm_{get/set}_param it will be possible for VM forking to copy the > >>>> parameters directly into the clone domain. > >>> Having peeked ahead at patch 17, where this gets used, I wonder why > >>> you want a pair of one-by-one functions, rather than a copy-all one. > >>> This then wouldn't require exposure of the functions you touch here. > >> Well, provided there is no such function in existence today it was > >> just easier to use what's already available. I still wouldn't want to > >> implement a one-shot function like that because this same code-path is > >> shared by the save-restore operations on the toolstack side, so at > >> least I have a reasonable assumption that it won't break on me in the > >> future. > > > > In particular, a number of the set operations are distinctly > > non-trivial. > > How is trivial or not related to there being one function doing > the looping wanted here vs the looping being done by the caller > around the two per-entity calls? I don't really get why would it matter where the looping is being done? Even if I were to add a single function to do this, it would do the same looping and just call the now internally kept get/set params functions. Tamas
(re-sending, as I still don't see the mail having appeared on the list) On 23.12.2019 15:55, Tamas K Lengyel wrote: > On Mon, Dec 23, 2019 at 2:37 AM Jan Beulich <jbeulich@suse.com> wrote: >> >> On 20.12.2019 18:32, Andrew Cooper wrote: >>> On 20/12/2019 17:27, Tamas K Lengyel wrote: >>>> On Fri, Dec 20, 2019 at 9:47 AM Jan Beulich <jbeulich@suse.com> wrote: >>>>> On 18.12.2019 20:40, Tamas K Lengyel wrote: >>>>>> Currently the hvm parameters are only accessible via the HVMOP hypercalls. By >>>>>> exposing hvm_{get/set}_param it will be possible for VM forking to copy the >>>>>> parameters directly into the clone domain. >>>>> Having peeked ahead at patch 17, where this gets used, I wonder why >>>>> you want a pair of one-by-one functions, rather than a copy-all one. >>>>> This then wouldn't require exposure of the functions you touch here. >>>> Well, provided there is no such function in existence today it was >>>> just easier to use what's already available. I still wouldn't want to >>>> implement a one-shot function like that because this same code-path is >>>> shared by the save-restore operations on the toolstack side, so at >>>> least I have a reasonable assumption that it won't break on me in the >>>> future. >>> >>> In particular, a number of the set operations are distinctly >>> non-trivial. >> >> How is trivial or not related to there being one function doing >> the looping wanted here vs the looping being done by the caller >> around the two per-entity calls? > > I don't really get why would it matter where the looping is being > done? Even if I were to add a single function to do this, it would do > the same looping and just call the now internally kept get/set params > functions. The difference (to me) is what level of control gets exposed outside of the file. For example I also dislike external code doing this somewhat odd (but necessary as per your communication with Andrew) checking against zero values. Such are implementation details which would better not be scatter around. Jan
On Fri, Dec 27, 2019 at 1:04 AM Jan Beulich <JBeulich@suse.com> wrote: > > (re-sending, as I still don't see the mail having appeared on the list) > > On 23.12.2019 15:55, Tamas K Lengyel wrote: > > On Mon, Dec 23, 2019 at 2:37 AM Jan Beulich <jbeulich@suse.com> wrote: > >> > >> On 20.12.2019 18:32, Andrew Cooper wrote: > >>> On 20/12/2019 17:27, Tamas K Lengyel wrote: > >>>> On Fri, Dec 20, 2019 at 9:47 AM Jan Beulich <jbeulich@suse.com> wrote: > >>>>> On 18.12.2019 20:40, Tamas K Lengyel wrote: > >>>>>> Currently the hvm parameters are only accessible via the HVMOP hypercalls. By > >>>>>> exposing hvm_{get/set}_param it will be possible for VM forking to copy the > >>>>>> parameters directly into the clone domain. > >>>>> Having peeked ahead at patch 17, where this gets used, I wonder why > >>>>> you want a pair of one-by-one functions, rather than a copy-all one. > >>>>> This then wouldn't require exposure of the functions you touch here. > >>>> Well, provided there is no such function in existence today it was > >>>> just easier to use what's already available. I still wouldn't want to > >>>> implement a one-shot function like that because this same code-path is > >>>> shared by the save-restore operations on the toolstack side, so at > >>>> least I have a reasonable assumption that it won't break on me in the > >>>> future. > >>> > >>> In particular, a number of the set operations are distinctly > >>> non-trivial. > >> > >> How is trivial or not related to there being one function doing > >> the looping wanted here vs the looping being done by the caller > >> around the two per-entity calls? > > > > I don't really get why would it matter where the looping is being > > done? Even if I were to add a single function to do this, it would do > > the same looping and just call the now internally kept get/set params > > functions. > > The difference (to me) is what level of control gets exposed outside > of the file. For example I also dislike external code doing this > somewhat odd (but necessary as per your communication with Andrew) > checking against zero values. Such are implementation details which > would better not be scatter around. But you do realize that both of these functions are already exposed via hypercalls? So it's OK to call them from the toolstack but not from other parts of Xen itself? I don't see much reason there. But to me it makes 0 difference where the loop that copies the params is done, as long as it's within Xen. So if you really want it to be in hvm.c I can do that, I just find it silly. Tamas
On 27.12.2019 14:10, Tamas K Lengyel wrote: > On Fri, Dec 27, 2019 at 1:04 AM Jan Beulich <JBeulich@suse.com> wrote: >> >> (re-sending, as I still don't see the mail having appeared on the list) >> >> On 23.12.2019 15:55, Tamas K Lengyel wrote: >>> On Mon, Dec 23, 2019 at 2:37 AM Jan Beulich <jbeulich@suse.com> wrote: >>>> >>>> On 20.12.2019 18:32, Andrew Cooper wrote: >>>>> On 20/12/2019 17:27, Tamas K Lengyel wrote: >>>>>> On Fri, Dec 20, 2019 at 9:47 AM Jan Beulich <jbeulich@suse.com> wrote: >>>>>>> On 18.12.2019 20:40, Tamas K Lengyel wrote: >>>>>>>> Currently the hvm parameters are only accessible via the HVMOP hypercalls. By >>>>>>>> exposing hvm_{get/set}_param it will be possible for VM forking to copy the >>>>>>>> parameters directly into the clone domain. >>>>>>> Having peeked ahead at patch 17, where this gets used, I wonder why >>>>>>> you want a pair of one-by-one functions, rather than a copy-all one. >>>>>>> This then wouldn't require exposure of the functions you touch here. >>>>>> Well, provided there is no such function in existence today it was >>>>>> just easier to use what's already available. I still wouldn't want to >>>>>> implement a one-shot function like that because this same code-path is >>>>>> shared by the save-restore operations on the toolstack side, so at >>>>>> least I have a reasonable assumption that it won't break on me in the >>>>>> future. >>>>> >>>>> In particular, a number of the set operations are distinctly >>>>> non-trivial. >>>> >>>> How is trivial or not related to there being one function doing >>>> the looping wanted here vs the looping being done by the caller >>>> around the two per-entity calls? >>> >>> I don't really get why would it matter where the looping is being >>> done? Even if I were to add a single function to do this, it would do >>> the same looping and just call the now internally kept get/set params >>> functions. >> >> The difference (to me) is what level of control gets exposed outside >> of the file. For example I also dislike external code doing this >> somewhat odd (but necessary as per your communication with Andrew) >> checking against zero values. Such are implementation details which >> would better not be scatter around. > > But you do realize that both of these functions are already exposed > via hypercalls? So it's OK to call them from the toolstack but not > from other parts of Xen itself? I don't see much reason there. Right now we have exactly one path each allowing this get/set. Adding a 2nd (from outside of hvm.c) opens the door for possible races between the various (for now just two) possible call sites. Noticing a possible problem when adding new code is imo quite a bit more likely if everything lives centralized in one place. IOW "exposure" here isn't meant so much in the sense of what entity in the system gets to drive the data, but which entities within Xen may play with it. Jan
On Fri, Dec 27, 2019 at 6:44 AM Jan Beulich <jbeulich@suse.com> wrote: > > On 27.12.2019 14:10, Tamas K Lengyel wrote: > > On Fri, Dec 27, 2019 at 1:04 AM Jan Beulich <JBeulich@suse.com> wrote: > >> > >> (re-sending, as I still don't see the mail having appeared on the list) > >> > >> On 23.12.2019 15:55, Tamas K Lengyel wrote: > >>> On Mon, Dec 23, 2019 at 2:37 AM Jan Beulich <jbeulich@suse.com> wrote: > >>>> > >>>> On 20.12.2019 18:32, Andrew Cooper wrote: > >>>>> On 20/12/2019 17:27, Tamas K Lengyel wrote: > >>>>>> On Fri, Dec 20, 2019 at 9:47 AM Jan Beulich <jbeulich@suse.com> wrote: > >>>>>>> On 18.12.2019 20:40, Tamas K Lengyel wrote: > >>>>>>>> Currently the hvm parameters are only accessible via the HVMOP hypercalls. By > >>>>>>>> exposing hvm_{get/set}_param it will be possible for VM forking to copy the > >>>>>>>> parameters directly into the clone domain. > >>>>>>> Having peeked ahead at patch 17, where this gets used, I wonder why > >>>>>>> you want a pair of one-by-one functions, rather than a copy-all one. > >>>>>>> This then wouldn't require exposure of the functions you touch here. > >>>>>> Well, provided there is no such function in existence today it was > >>>>>> just easier to use what's already available. I still wouldn't want to > >>>>>> implement a one-shot function like that because this same code-path is > >>>>>> shared by the save-restore operations on the toolstack side, so at > >>>>>> least I have a reasonable assumption that it won't break on me in the > >>>>>> future. > >>>>> > >>>>> In particular, a number of the set operations are distinctly > >>>>> non-trivial. > >>>> > >>>> How is trivial or not related to there being one function doing > >>>> the looping wanted here vs the looping being done by the caller > >>>> around the two per-entity calls? > >>> > >>> I don't really get why would it matter where the looping is being > >>> done? Even if I were to add a single function to do this, it would do > >>> the same looping and just call the now internally kept get/set params > >>> functions. > >> > >> The difference (to me) is what level of control gets exposed outside > >> of the file. For example I also dislike external code doing this > >> somewhat odd (but necessary as per your communication with Andrew) > >> checking against zero values. Such are implementation details which > >> would better not be scatter around. > > > > But you do realize that both of these functions are already exposed > > via hypercalls? So it's OK to call them from the toolstack but not > > from other parts of Xen itself? I don't see much reason there. > > Right now we have exactly one path each allowing this get/set. Adding > a 2nd (from outside of hvm.c) opens the door for possible races > between the various (for now just two) possible call sites. Noticing > a possible problem when adding new code is imo quite a bit more > likely if everything lives centralized in one place. IOW "exposure" > here isn't meant so much in the sense of what entity in the system > gets to drive the data, but which entities within Xen may play with > it. Sure, I'll move the loop to hvm.c then. Thanks, Tamas
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index 614ed60fe4..5a3a962fbb 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -4072,16 +4072,17 @@ static int hvmop_set_evtchn_upcall_vector( } static int hvm_allow_set_param(struct domain *d, - const struct xen_hvm_param *a) + uint32_t index, + uint64_t new_value) { - uint64_t value = d->arch.hvm.params[a->index]; + uint64_t value = d->arch.hvm.params[index]; int rc; rc = xsm_hvm_param(XSM_TARGET, d, HVMOP_set_param); if ( rc ) return rc; - switch ( a->index ) + switch ( index ) { /* The following parameters can be set by the guest. */ case HVM_PARAM_CALLBACK_IRQ: @@ -4114,7 +4115,7 @@ static int hvm_allow_set_param(struct domain *d, if ( rc ) return rc; - switch ( a->index ) + switch ( index ) { /* The following parameters should only be changed once. */ case HVM_PARAM_VIRIDIAN: @@ -4124,7 +4125,7 @@ static int hvm_allow_set_param(struct domain *d, case HVM_PARAM_NR_IOREQ_SERVER_PAGES: case HVM_PARAM_ALTP2M: case HVM_PARAM_MCA_CAP: - if ( value != 0 && a->value != value ) + if ( value != 0 && new_value != value ) rc = -EEXIST; break; default: @@ -4134,13 +4135,11 @@ static int hvm_allow_set_param(struct domain *d, return rc; } -static int hvmop_set_param( +int hvmop_set_param( XEN_GUEST_HANDLE_PARAM(xen_hvm_param_t) arg) { - struct domain *curr_d = current->domain; struct xen_hvm_param a; struct domain *d; - struct vcpu *v; int rc; if ( copy_from_guest(&a, arg, 1) ) @@ -4160,23 +4159,42 @@ static int hvmop_set_param( if ( !is_hvm_domain(d) ) goto out; - rc = hvm_allow_set_param(d, &a); + rc = hvm_set_param(d, a.index, a.value); + + out: + rcu_unlock_domain(d); + return rc; +} + +int hvm_set_param( + struct domain *d, + uint32_t index, + uint64_t value) +{ + struct domain *curr_d = current->domain; + int rc; + struct vcpu *v; + + if ( index >= HVM_NR_PARAMS ) + return -EINVAL; + + rc = hvm_allow_set_param(d, index, value); if ( rc ) goto out; - switch ( a.index ) + switch ( index ) { case HVM_PARAM_CALLBACK_IRQ: - hvm_set_callback_via(d, a.value); + hvm_set_callback_via(d, value); hvm_latch_shinfo_size(d); break; case HVM_PARAM_TIMER_MODE: - if ( a.value > HVMPTM_one_missed_tick_pending ) + if ( value > HVMPTM_one_missed_tick_pending ) rc = -EINVAL; break; case HVM_PARAM_VIRIDIAN: - if ( (a.value & ~HVMPV_feature_mask) || - !(a.value & HVMPV_base_freq) ) + if ( (value & ~HVMPV_feature_mask) || + !(value & HVMPV_base_freq) ) rc = -EINVAL; break; case HVM_PARAM_IDENT_PT: @@ -4186,7 +4204,7 @@ static int hvmop_set_param( */ if ( !paging_mode_hap(d) || !cpu_has_vmx ) { - d->arch.hvm.params[a.index] = a.value; + d->arch.hvm.params[index] = value; break; } @@ -4201,7 +4219,7 @@ static int hvmop_set_param( rc = 0; domain_pause(d); - d->arch.hvm.params[a.index] = a.value; + d->arch.hvm.params[index] = value; for_each_vcpu ( d, v ) paging_update_cr3(v, false); domain_unpause(d); @@ -4210,23 +4228,23 @@ static int hvmop_set_param( break; case HVM_PARAM_DM_DOMAIN: /* The only value this should ever be set to is DOMID_SELF */ - if ( a.value != DOMID_SELF ) + if ( value != DOMID_SELF ) rc = -EINVAL; - a.value = curr_d->domain_id; + value = curr_d->domain_id; break; case HVM_PARAM_ACPI_S_STATE: rc = 0; - if ( a.value == 3 ) + if ( value == 3 ) hvm_s3_suspend(d); - else if ( a.value == 0 ) + else if ( value == 0 ) hvm_s3_resume(d); else rc = -EINVAL; break; case HVM_PARAM_ACPI_IOPORTS_LOCATION: - rc = pmtimer_change_ioport(d, a.value); + rc = pmtimer_change_ioport(d, value); break; case HVM_PARAM_MEMORY_EVENT_CR0: case HVM_PARAM_MEMORY_EVENT_CR3: @@ -4241,24 +4259,24 @@ static int hvmop_set_param( rc = xsm_hvm_param_nested(XSM_PRIV, d); if ( rc ) break; - if ( a.value > 1 ) + if ( value > 1 ) rc = -EINVAL; /* * Remove the check below once we have * shadow-on-shadow. */ - if ( !paging_mode_hap(d) && a.value ) + if ( !paging_mode_hap(d) && value ) rc = -EINVAL; - if ( a.value && + if ( value && d->arch.hvm.params[HVM_PARAM_ALTP2M] ) rc = -EINVAL; /* Set up NHVM state for any vcpus that are already up. */ - if ( a.value && + if ( value && !d->arch.hvm.params[HVM_PARAM_NESTEDHVM] ) for_each_vcpu(d, v) if ( rc == 0 ) rc = nestedhvm_vcpu_initialise(v); - if ( !a.value || rc ) + if ( !value || rc ) for_each_vcpu(d, v) nestedhvm_vcpu_destroy(v); break; @@ -4266,30 +4284,30 @@ static int hvmop_set_param( rc = xsm_hvm_param_altp2mhvm(XSM_PRIV, d); if ( rc ) break; - if ( a.value > XEN_ALTP2M_limited ) + if ( value > XEN_ALTP2M_limited ) rc = -EINVAL; - if ( a.value && + if ( value && d->arch.hvm.params[HVM_PARAM_NESTEDHVM] ) rc = -EINVAL; break; case HVM_PARAM_TRIPLE_FAULT_REASON: - if ( a.value > SHUTDOWN_MAX ) + if ( value > SHUTDOWN_MAX ) rc = -EINVAL; break; case HVM_PARAM_IOREQ_SERVER_PFN: - d->arch.hvm.ioreq_gfn.base = a.value; + d->arch.hvm.ioreq_gfn.base = value; break; case HVM_PARAM_NR_IOREQ_SERVER_PAGES: { unsigned int i; - if ( a.value == 0 || - a.value > sizeof(d->arch.hvm.ioreq_gfn.mask) * 8 ) + if ( value == 0 || + value > sizeof(d->arch.hvm.ioreq_gfn.mask) * 8 ) { rc = -EINVAL; break; } - for ( i = 0; i < a.value; i++ ) + for ( i = 0; i < value; i++ ) set_bit(i, &d->arch.hvm.ioreq_gfn.mask); break; @@ -4301,35 +4319,35 @@ static int hvmop_set_param( sizeof(d->arch.hvm.ioreq_gfn.legacy_mask) * 8); BUILD_BUG_ON(HVM_PARAM_BUFIOREQ_PFN > sizeof(d->arch.hvm.ioreq_gfn.legacy_mask) * 8); - if ( a.value ) - set_bit(a.index, &d->arch.hvm.ioreq_gfn.legacy_mask); + if ( value ) + set_bit(index, &d->arch.hvm.ioreq_gfn.legacy_mask); break; case HVM_PARAM_X87_FIP_WIDTH: - if ( a.value != 0 && a.value != 4 && a.value != 8 ) + if ( value != 0 && value != 4 && value != 8 ) { rc = -EINVAL; break; } - d->arch.x87_fip_width = a.value; + d->arch.x87_fip_width = value; break; case HVM_PARAM_VM86_TSS: /* Hardware would silently truncate high bits. */ - if ( a.value != (uint32_t)a.value ) + if ( value != (uint32_t)value ) { if ( d == curr_d ) domain_crash(d); rc = -EINVAL; } /* Old hvmloader binaries hardcode the size to 128 bytes. */ - if ( a.value ) - a.value |= (128ULL << 32) | VM86_TSS_UPDATED; - a.index = HVM_PARAM_VM86_TSS_SIZED; + if ( value ) + value |= (128ULL << 32) | VM86_TSS_UPDATED; + index = HVM_PARAM_VM86_TSS_SIZED; break; case HVM_PARAM_VM86_TSS_SIZED: - if ( (a.value >> 32) < sizeof(struct tss32) ) + if ( (value >> 32) < sizeof(struct tss32) ) { if ( d == curr_d ) domain_crash(d); @@ -4340,34 +4358,33 @@ static int hvmop_set_param( * 256 bits interrupt redirection bitmap + 64k bits I/O bitmap * plus one padding byte). */ - if ( (a.value >> 32) > sizeof(struct tss32) + + if ( (value >> 32) > sizeof(struct tss32) + (0x100 / 8) + (0x10000 / 8) + 1 ) - a.value = (uint32_t)a.value | + value = (uint32_t)value | ((sizeof(struct tss32) + (0x100 / 8) + (0x10000 / 8) + 1) << 32); - a.value |= VM86_TSS_UPDATED; + value |= VM86_TSS_UPDATED; break; case HVM_PARAM_MCA_CAP: - rc = vmce_enable_mca_cap(d, a.value); + rc = vmce_enable_mca_cap(d, value); break; } if ( rc != 0 ) goto out; - d->arch.hvm.params[a.index] = a.value; + d->arch.hvm.params[index] = value; HVM_DBG_LOG(DBG_LEVEL_HCALL, "set param %u = %"PRIx64, - a.index, a.value); + index, value); out: - rcu_unlock_domain(d); return rc; } static int hvm_allow_get_param(struct domain *d, - const struct xen_hvm_param *a) + uint32_t index) { int rc; @@ -4375,7 +4392,7 @@ static int hvm_allow_get_param(struct domain *d, if ( rc ) return rc; - switch ( a->index ) + switch ( index ) { /* The following parameters can be read by the guest. */ case HVM_PARAM_CALLBACK_IRQ: @@ -4429,42 +4446,60 @@ static int hvmop_get_param( if ( !is_hvm_domain(d) ) goto out; - rc = hvm_allow_get_param(d, &a); + rc = hvm_get_param(d, a.index, &a.value); if ( rc ) goto out; - switch ( a.index ) + rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0; + + HVM_DBG_LOG(DBG_LEVEL_HCALL, "get param %u = %"PRIx64, + a.index, a.value); + + out: + rcu_unlock_domain(d); + return rc; +} + +int hvm_get_param( + struct domain *d, + uint32_t index, + uint64_t *value) +{ + int rc; + + if ( index >= HVM_NR_PARAMS || !value ) + return -EINVAL; + + rc = hvm_allow_get_param(d, index); + if ( rc ) + return rc; + + switch ( index ) { case HVM_PARAM_ACPI_S_STATE: - a.value = d->arch.hvm.is_s3_suspended ? 3 : 0; + *value = d->arch.hvm.is_s3_suspended ? 3 : 0; break; case HVM_PARAM_VM86_TSS: - a.value = (uint32_t)d->arch.hvm.params[HVM_PARAM_VM86_TSS_SIZED]; + *value = (uint32_t)d->arch.hvm.params[HVM_PARAM_VM86_TSS_SIZED]; break; case HVM_PARAM_VM86_TSS_SIZED: - a.value = d->arch.hvm.params[HVM_PARAM_VM86_TSS_SIZED] & - ~VM86_TSS_UPDATED; + *value = d->arch.hvm.params[HVM_PARAM_VM86_TSS_SIZED] & + ~VM86_TSS_UPDATED; break; case HVM_PARAM_X87_FIP_WIDTH: - a.value = d->arch.x87_fip_width; + *value = d->arch.x87_fip_width; break; default: - a.value = d->arch.hvm.params[a.index]; + *value = d->arch.hvm.params[index]; break; } - rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0; - - HVM_DBG_LOG(DBG_LEVEL_HCALL, "get param %u = %"PRIx64, - a.index, a.value); + return 0; +}; - out: - rcu_unlock_domain(d); - return rc; -} /* * altp2m operations are envisioned as being used in several different diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h index 1d7b66f927..a6f4ae76a1 100644 --- a/xen/include/asm-x86/hvm/hvm.h +++ b/xen/include/asm-x86/hvm/hvm.h @@ -335,6 +335,10 @@ unsigned long hvm_cr4_guest_valid_bits(const struct domain *d, bool restore); bool hvm_flush_vcpu_tlb(bool (*flush_vcpu)(void *ctxt, struct vcpu *v), void *ctxt); +/* Caller must hold domain locks */ +int hvm_get_param(struct domain *d, uint32_t index, uint64_t *value); +int hvm_set_param(struct domain *d, uint32_t index, uint64_t value); + #ifdef CONFIG_HVM #define hvm_get_guest_tsc(v) hvm_get_guest_tsc_fixed(v, 0)
Currently the hvm parameters are only accessible via the HVMOP hypercalls. By exposing hvm_{get/set}_param it will be possible for VM forking to copy the parameters directly into the clone domain. Signed-off-by: Tamas K Lengyel <tamas.lengyel@intel.com> --- xen/arch/x86/hvm/hvm.c | 169 ++++++++++++++++++++-------------- xen/include/asm-x86/hvm/hvm.h | 4 + 2 files changed, 106 insertions(+), 67 deletions(-)