| Message ID | 20211012072428.2569-3-dongli.zhang@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix the Xen HVM kdump/kexec boot panic issue | expand |
On 12.10.2021 09:24, Dongli Zhang wrote: > The guest may access the pv vcpu_time_info immediately after > VCPUOP_register_vcpu_info. This is to borrow the idea of > VCPUOP_register_vcpu_time_memory_area, where the > force_update_vcpu_system_time() is called immediately when the new memory > area is registered. > > Otherwise, we may observe clock drift at the VM side if the VM accesses > the clocksource immediately after VCPUOP_register_vcpu_info(). > > Cc: Joe Jin <joe.jin@oracle.com> > Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com> While I agree with the change in principle, ... > --- a/xen/common/domain.c > +++ b/xen/common/domain.c > @@ -1695,6 +1695,8 @@ long do_vcpu_op(int cmd, unsigned int vcpuid, XEN_GUEST_HANDLE_PARAM(void) arg) > rc = map_vcpu_info(v, info.mfn, info.offset); > domain_unlock(d); > > + force_update_vcpu_system_time(v); ... I'm afraid you're breaking the Arm build here. Arm will first need to gain this function. Jan
Hi Jan, On 10/12/21 1:40 AM, Jan Beulich wrote: > On 12.10.2021 09:24, Dongli Zhang wrote: >> The guest may access the pv vcpu_time_info immediately after >> VCPUOP_register_vcpu_info. This is to borrow the idea of >> VCPUOP_register_vcpu_time_memory_area, where the >> force_update_vcpu_system_time() is called immediately when the new memory >> area is registered. >> >> Otherwise, we may observe clock drift at the VM side if the VM accesses >> the clocksource immediately after VCPUOP_register_vcpu_info(). >> >> Cc: Joe Jin <joe.jin@oracle.com> >> Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com> > > While I agree with the change in principle, ... > >> --- a/xen/common/domain.c >> +++ b/xen/common/domain.c >> @@ -1695,6 +1695,8 @@ long do_vcpu_op(int cmd, unsigned int vcpuid, XEN_GUEST_HANDLE_PARAM(void) arg) >> rc = map_vcpu_info(v, info.mfn, info.offset); >> domain_unlock(d); >> >> + force_update_vcpu_system_time(v); > > ... I'm afraid you're breaking the Arm build here. Arm will first need > to gain this function. > Since I am not familiar with the Xen ARM, would you please let me your suggestion if I just leave ARM as TODO to the ARM developers to verify and implement force_update_vcpu_system_time()? I have tested that the below can build with arm64/aarch64. diff --git a/xen/common/domain.c b/xen/common/domain.c index 40d67ec342..644c65ecd3 100644 --- a/xen/common/domain.c +++ b/xen/common/domain.c @@ -1695,6 +1695,13 @@ long do_vcpu_op(int cmd, unsigned int vcpuid, XEN_GUEST_HANDLE_PARAM(void) arg) rc = map_vcpu_info(v, info.mfn, info.offset); domain_unlock(d); +#ifdef CONFIG_X86 + /* + * TODO: ARM does not have force_update_vcpu_system_time(). + */ + force_update_vcpu_system_time(v); +#endif + break; } Thank you very much! Dongli Zhang
On 12.10.2021 17:43, Dongli Zhang wrote: > Hi Jan, > > On 10/12/21 1:40 AM, Jan Beulich wrote: >> On 12.10.2021 09:24, Dongli Zhang wrote: >>> The guest may access the pv vcpu_time_info immediately after >>> VCPUOP_register_vcpu_info. This is to borrow the idea of >>> VCPUOP_register_vcpu_time_memory_area, where the >>> force_update_vcpu_system_time() is called immediately when the new memory >>> area is registered. >>> >>> Otherwise, we may observe clock drift at the VM side if the VM accesses >>> the clocksource immediately after VCPUOP_register_vcpu_info(). >>> >>> Cc: Joe Jin <joe.jin@oracle.com> >>> Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com> >> >> While I agree with the change in principle, ... >> >>> --- a/xen/common/domain.c >>> +++ b/xen/common/domain.c >>> @@ -1695,6 +1695,8 @@ long do_vcpu_op(int cmd, unsigned int vcpuid, XEN_GUEST_HANDLE_PARAM(void) arg) >>> rc = map_vcpu_info(v, info.mfn, info.offset); >>> domain_unlock(d); >>> >>> + force_update_vcpu_system_time(v); >> >> ... I'm afraid you're breaking the Arm build here. Arm will first need >> to gain this function. >> > > Since I am not familiar with the Xen ARM, would you please let me your > suggestion if I just leave ARM as TODO to the ARM developers to verify > and implement force_update_vcpu_system_time()? I'd much prefer to avoid this. I don't think the function can be that difficult to introduce. And I'm sure the Arm maintainers will apply extra care during review if you point out that you weren't able to actually test this. Jan > I have tested that the below can build with arm64/aarch64. > > diff --git a/xen/common/domain.c b/xen/common/domain.c > index 40d67ec342..644c65ecd3 100644 > --- a/xen/common/domain.c > +++ b/xen/common/domain.c > @@ -1695,6 +1695,13 @@ long do_vcpu_op(int cmd, unsigned int vcpuid, > XEN_GUEST_HANDLE_PARAM(void) arg) > rc = map_vcpu_info(v, info.mfn, info.offset); > domain_unlock(d); > > +#ifdef CONFIG_X86 > + /* > + * TODO: ARM does not have force_update_vcpu_system_time(). > + */ > + force_update_vcpu_system_time(v); > +#endif > + > break; > } > > > > Thank you very much! > > Dongli Zhang >
Hi Jan, On 10/12/21 8:49 AM, Jan Beulich wrote: > On 12.10.2021 17:43, Dongli Zhang wrote: >> Hi Jan, >> >> On 10/12/21 1:40 AM, Jan Beulich wrote: >>> On 12.10.2021 09:24, Dongli Zhang wrote: >>>> The guest may access the pv vcpu_time_info immediately after >>>> VCPUOP_register_vcpu_info. This is to borrow the idea of >>>> VCPUOP_register_vcpu_time_memory_area, where the >>>> force_update_vcpu_system_time() is called immediately when the new memory >>>> area is registered. >>>> >>>> Otherwise, we may observe clock drift at the VM side if the VM accesses >>>> the clocksource immediately after VCPUOP_register_vcpu_info(). >>>> >>>> Cc: Joe Jin <joe.jin@oracle.com> >>>> Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com> >>> >>> While I agree with the change in principle, ... >>> >>>> --- a/xen/common/domain.c >>>> +++ b/xen/common/domain.c >>>> @@ -1695,6 +1695,8 @@ long do_vcpu_op(int cmd, unsigned int vcpuid, XEN_GUEST_HANDLE_PARAM(void) arg) >>>> rc = map_vcpu_info(v, info.mfn, info.offset); >>>> domain_unlock(d); >>>> >>>> + force_update_vcpu_system_time(v); >>> >>> ... I'm afraid you're breaking the Arm build here. Arm will first need >>> to gain this function. >>> >> >> Since I am not familiar with the Xen ARM, would you please let me your >> suggestion if I just leave ARM as TODO to the ARM developers to verify >> and implement force_update_vcpu_system_time()? > > I'd much prefer to avoid this. I don't think the function can be that > difficult to introduce. And I'm sure the Arm maintainers will apply > extra care during review if you point out that you weren't able to > actually test this. > I do not see pvclock used by arm/arm64 in linux kernel for xen. In addition, the implementation at xen hypervisor side is empty. 348 /* VCPU PV clock. */ 349 void update_vcpu_system_time(struct vcpu *v) 350 { 351 /* XXX update shared_info->wc_* */ 352 } I will add a wrapper for it. The bad thing is I see riscv is supported by xen and we may need to add the function for riscv as well. Thank you very much! Dongli Zhang
On 12.10.2021 18:15, Dongli Zhang wrote: > Hi Jan, > > On 10/12/21 8:49 AM, Jan Beulich wrote: >> On 12.10.2021 17:43, Dongli Zhang wrote: >>> Hi Jan, >>> >>> On 10/12/21 1:40 AM, Jan Beulich wrote: >>>> On 12.10.2021 09:24, Dongli Zhang wrote: >>>>> The guest may access the pv vcpu_time_info immediately after >>>>> VCPUOP_register_vcpu_info. This is to borrow the idea of >>>>> VCPUOP_register_vcpu_time_memory_area, where the >>>>> force_update_vcpu_system_time() is called immediately when the new memory >>>>> area is registered. >>>>> >>>>> Otherwise, we may observe clock drift at the VM side if the VM accesses >>>>> the clocksource immediately after VCPUOP_register_vcpu_info(). >>>>> >>>>> Cc: Joe Jin <joe.jin@oracle.com> >>>>> Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com> >>>> >>>> While I agree with the change in principle, ... >>>> >>>>> --- a/xen/common/domain.c >>>>> +++ b/xen/common/domain.c >>>>> @@ -1695,6 +1695,8 @@ long do_vcpu_op(int cmd, unsigned int vcpuid, XEN_GUEST_HANDLE_PARAM(void) arg) >>>>> rc = map_vcpu_info(v, info.mfn, info.offset); >>>>> domain_unlock(d); >>>>> >>>>> + force_update_vcpu_system_time(v); >>>> >>>> ... I'm afraid you're breaking the Arm build here. Arm will first need >>>> to gain this function. >>>> >>> >>> Since I am not familiar with the Xen ARM, would you please let me your >>> suggestion if I just leave ARM as TODO to the ARM developers to verify >>> and implement force_update_vcpu_system_time()? >> >> I'd much prefer to avoid this. I don't think the function can be that >> difficult to introduce. And I'm sure the Arm maintainers will apply >> extra care during review if you point out that you weren't able to >> actually test this. >> > > I do not see pvclock used by arm/arm64 in linux kernel for xen. > > In addition, the implementation at xen hypervisor side is empty. > > 348 /* VCPU PV clock. */ > 349 void update_vcpu_system_time(struct vcpu *v) > 350 { > 351 /* XXX update shared_info->wc_* */ > 352 } > > I will add a wrapper for it. > > The bad thing is I see riscv is supported by xen and we may need to add the > function for riscv as well. There's not really any code for RISC-V yet, so that'll be of concern to those who are working on adding actual code later on. I'm actually wondering about the status of that effort - after the initial bits were added over 3 months ago, no further activity has been visible. Jan
diff --git a/xen/common/domain.c b/xen/common/domain.c index 40d67ec342..c879f6723b 100644 --- a/xen/common/domain.c +++ b/xen/common/domain.c @@ -1695,6 +1695,8 @@ long do_vcpu_op(int cmd, unsigned int vcpuid, XEN_GUEST_HANDLE_PARAM(void) arg) rc = map_vcpu_info(v, info.mfn, info.offset); domain_unlock(d); + force_update_vcpu_system_time(v); + break; }
The guest may access the pv vcpu_time_info immediately after VCPUOP_register_vcpu_info. This is to borrow the idea of VCPUOP_register_vcpu_time_memory_area, where the force_update_vcpu_system_time() is called immediately when the new memory area is registered. Otherwise, we may observe clock drift at the VM side if the VM accesses the clocksource immediately after VCPUOP_register_vcpu_info(). Cc: Joe Jin <joe.jin@oracle.com> Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com> --- xen/common/domain.c | 2 ++ 1 file changed, 2 insertions(+)