| Message ID | 20230116040405.260935-2-gshan@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Improve dirty ring warning report | expand |
Hi Gavin, On Mon, Jan 16, 2023 at 12:04:02PM +0800, Gavin Shan wrote: > When dirty ring is enabled, the dirty page information is pushed to > the dirty ring if there is a running VCPU context. Otherwise, the > dirty page information is still tracked by the backup dirty bitmap. > In order to detect if there is a running VCPU context when a guest > page becomes dirty, kvm_arch_allow_write_without_running_vcpu() was > introduced to warn when no running VCPU context exists on unknown > cases. > > Other than the site of saving ITS tables, it's possible to save vgic3 > LPI pending status in no running vcpu context because it can happen when > ITS ITE is restored through the command KVM_DEV_ARM_ITS_RESTORE_TABLES > on 'kvm-arm-vgic-its' device. > > Fix it by allowing to save vgic3 LPI pending status in no running > vcpu context. > > Signed-off-by: Gavin Shan <gshan@redhat.com> > --- > Documentation/virt/kvm/api.rst | 5 +++-- > arch/arm64/kvm/vgic/vgic-its.c | 3 ++- > arch/arm64/kvm/vgic/vgic-v3.c | 3 +++ > include/kvm/arm_vgic.h | 1 + > 4 files changed, 9 insertions(+), 3 deletions(-) > > diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst > index 9807b05a1b57..18b245a0ba02 100644 > --- a/Documentation/virt/kvm/api.rst > +++ b/Documentation/virt/kvm/api.rst > @@ -8071,8 +8071,9 @@ state is final and avoid missing dirty pages from another ioctl ordered > after the bitmap collection. > > NOTE: One example of using the backup bitmap is saving arm64 vgic/its > -tables through KVM_DEV_ARM_{VGIC_GRP_CTRL, ITS_SAVE_TABLES} command on > -KVM device "kvm-arm-vgic-its" when dirty ring is enabled. > +tables and vgic3 LPI pending status through KVM_DEV_ARM_{VGIC_GRP_CTRL, > +ITS_SAVE_TABLES} and KVM_DEV_ARM_{VGIC_GRP_CTRL, ITS_RESTORE_TABLES} > +command on KVM device "kvm-arm-vgic-its" when dirty ring is enabled. > > 8.30 KVM_CAP_XEN_HVM > -------------------- > diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c > index 94a666dd1443..119a9c7a0a52 100644 > --- a/arch/arm64/kvm/vgic/vgic-its.c > +++ b/arch/arm64/kvm/vgic/vgic-its.c > @@ -2792,7 +2792,8 @@ bool kvm_arch_allow_write_without_running_vcpu(struct kvm *kvm) > { > struct vgic_dist *dist = &kvm->arch.vgic; > > - return dist->save_its_tables_in_progress; > + return dist->save_vgic_v3_tables_in_progress || > + dist->save_its_tables_in_progress; I'd much prefer using a single bool to keep track of this, i.e: return dist->save_tables_in_progress; > } > > static int vgic_its_set_attr(struct kvm_device *dev, > diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c > index 2074521d4a8c..32998c8587a8 100644 > --- a/arch/arm64/kvm/vgic/vgic-v3.c > +++ b/arch/arm64/kvm/vgic/vgic-v3.c > @@ -304,6 +304,7 @@ void vgic_v3_enable(struct kvm_vcpu *vcpu) > int vgic_v3_lpi_sync_pending_status(struct kvm *kvm, struct vgic_irq *irq) > { > struct kvm_vcpu *vcpu; > + struct vgic_dist *dist = &kvm->arch.vgic; > int byte_offset, bit_nr; > gpa_t pendbase, ptr; > bool status; > @@ -339,7 +340,9 @@ int vgic_v3_lpi_sync_pending_status(struct kvm *kvm, struct vgic_irq *irq) > if (status) { > /* clear consumed data */ > val &= ~(1 << bit_nr); > + dist->save_vgic_v3_tables_in_progress = true; > ret = kvm_write_guest_lock(kvm, ptr, &val, 1); > + dist->save_vgic_v3_tables_in_progress = false; With the above suggestion of using a bool, this should become a helper used at all the affected callsites: static int vgic_write_guest_lock(struct kvm *kvm, gpa_t gpa, const void *data, unsigned long len) { struct vgic_dist *dist = &kvm->arch.vgic; int ret; dist->save_tables_in_progress = true; ret = kvm_write_guest_lock(kvm, gpa, data, len); dist->save_tables_in_progress = false; return ret; } -- Thanks, Oliver
Hi Oliver, On 1/18/23 7:51 AM, Oliver Upton wrote: > On Mon, Jan 16, 2023 at 12:04:02PM +0800, Gavin Shan wrote: >> When dirty ring is enabled, the dirty page information is pushed to >> the dirty ring if there is a running VCPU context. Otherwise, the >> dirty page information is still tracked by the backup dirty bitmap. >> In order to detect if there is a running VCPU context when a guest >> page becomes dirty, kvm_arch_allow_write_without_running_vcpu() was >> introduced to warn when no running VCPU context exists on unknown >> cases. >> >> Other than the site of saving ITS tables, it's possible to save vgic3 >> LPI pending status in no running vcpu context because it can happen when >> ITS ITE is restored through the command KVM_DEV_ARM_ITS_RESTORE_TABLES >> on 'kvm-arm-vgic-its' device. >> >> Fix it by allowing to save vgic3 LPI pending status in no running >> vcpu context. >> >> Signed-off-by: Gavin Shan <gshan@redhat.com> >> --- >> Documentation/virt/kvm/api.rst | 5 +++-- >> arch/arm64/kvm/vgic/vgic-its.c | 3 ++- >> arch/arm64/kvm/vgic/vgic-v3.c | 3 +++ >> include/kvm/arm_vgic.h | 1 + >> 4 files changed, 9 insertions(+), 3 deletions(-) >> >> diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst >> index 9807b05a1b57..18b245a0ba02 100644 >> --- a/Documentation/virt/kvm/api.rst >> +++ b/Documentation/virt/kvm/api.rst >> @@ -8071,8 +8071,9 @@ state is final and avoid missing dirty pages from another ioctl ordered >> after the bitmap collection. >> >> NOTE: One example of using the backup bitmap is saving arm64 vgic/its >> -tables through KVM_DEV_ARM_{VGIC_GRP_CTRL, ITS_SAVE_TABLES} command on >> -KVM device "kvm-arm-vgic-its" when dirty ring is enabled. >> +tables and vgic3 LPI pending status through KVM_DEV_ARM_{VGIC_GRP_CTRL, >> +ITS_SAVE_TABLES} and KVM_DEV_ARM_{VGIC_GRP_CTRL, ITS_RESTORE_TABLES} >> +command on KVM device "kvm-arm-vgic-its" when dirty ring is enabled. >> >> 8.30 KVM_CAP_XEN_HVM >> -------------------- >> diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c >> index 94a666dd1443..119a9c7a0a52 100644 >> --- a/arch/arm64/kvm/vgic/vgic-its.c >> +++ b/arch/arm64/kvm/vgic/vgic-its.c >> @@ -2792,7 +2792,8 @@ bool kvm_arch_allow_write_without_running_vcpu(struct kvm *kvm) >> { >> struct vgic_dist *dist = &kvm->arch.vgic; >> >> - return dist->save_its_tables_in_progress; >> + return dist->save_vgic_v3_tables_in_progress || >> + dist->save_its_tables_in_progress; > > I'd much prefer using a single bool to keep track of this, i.e: > Yes, it's clean to have 'dist->save_tables_in_progress' for all 3 cases. One more concern like below. > return dist->save_tables_in_progress; > >> } >> >> static int vgic_its_set_attr(struct kvm_device *dev, >> diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c >> index 2074521d4a8c..32998c8587a8 100644 >> --- a/arch/arm64/kvm/vgic/vgic-v3.c >> +++ b/arch/arm64/kvm/vgic/vgic-v3.c >> @@ -304,6 +304,7 @@ void vgic_v3_enable(struct kvm_vcpu *vcpu) >> int vgic_v3_lpi_sync_pending_status(struct kvm *kvm, struct vgic_irq *irq) >> { >> struct kvm_vcpu *vcpu; >> + struct vgic_dist *dist = &kvm->arch.vgic; >> int byte_offset, bit_nr; >> gpa_t pendbase, ptr; >> bool status; >> @@ -339,7 +340,9 @@ int vgic_v3_lpi_sync_pending_status(struct kvm *kvm, struct vgic_irq *irq) >> if (status) { >> /* clear consumed data */ >> val &= ~(1 << bit_nr); >> + dist->save_vgic_v3_tables_in_progress = true; >> ret = kvm_write_guest_lock(kvm, ptr, &val, 1); >> + dist->save_vgic_v3_tables_in_progress = false; > > With the above suggestion of using a bool, this should become a helper > used at all the affected callsites: > > static int vgic_write_guest_lock(struct kvm *kvm, gpa_t gpa, > const void *data, unsigned long len) > { > struct vgic_dist *dist = &kvm->arch.vgic; > int ret; > > dist->save_tables_in_progress = true; > ret = kvm_write_guest_lock(kvm, gpa, data, len); > dist->save_tables_in_progress = false; > > return ret; > } > I will have vgic_write_guest_lock() in v2. Note that those 3 paths can't be running in parallel since one switch is shared by them. Alternatively, we extend struct vgic_dist::save_tables_in_progress from 'bool' to 'unsigned long'. Several bit is defined for each site as below. In this way, the 3 paths can be running in parallel: unsigned long struct vgic_dist::save_tables_in_progress #define VGIC_DIST_SAVE_ITS_ITE 0 /* ITS Translation Entry */ #define VGIC_DIST_SAVE_ITS_DTE 1 /* ITS Device Table Entry */ #define VGIC_DIST_SAVE_ITS_CTE 2 /* ITS Collection Table Entry */ #define VGIC_DIST_SAVE_ITS_CT 3 /* ITS Collection Table */ #define VGIC_DIST_SAVE_VGIC3_LPI 4 /* VGIC3 LPI Pending Status */ #define VGIC_DIST_SAVE_VGIC3_PENDING_TABLE 5 /* VGIC3 Pending Table */ The drawback is the calls are limited to 64. If those 3 paths can't be running in parallel, we needn't the extension at all. Thanks, Gavin
On Thu, 19 Jan 2023 01:11:44 +0000, Gavin Shan <gshan@redhat.com> wrote: > > I will have vgic_write_guest_lock() in v2. Note that those 3 paths can't be > running in parallel since one switch is shared by them. Alternatively, we > extend struct vgic_dist::save_tables_in_progress from 'bool' to 'unsigned long'. > Several bit is defined for each site as below. In this way, the 3 paths can be > running in parallel: > > unsigned long struct vgic_dist::save_tables_in_progress > > #define VGIC_DIST_SAVE_ITS_ITE 0 /* ITS Translation Entry */ > #define VGIC_DIST_SAVE_ITS_DTE 1 /* ITS Device Table Entry */ > #define VGIC_DIST_SAVE_ITS_CTE 2 /* ITS Collection Table Entry */ > #define VGIC_DIST_SAVE_ITS_CT 3 /* ITS Collection Table */ > #define VGIC_DIST_SAVE_VGIC3_LPI 4 /* VGIC3 LPI Pending Status */ > #define VGIC_DIST_SAVE_VGIC3_PENDING_TABLE 5 /* VGIC3 Pending Table */ > > The drawback is the calls are limited to 64. If those 3 paths can't be running > in parallel, we needn't the extension at all. It should all be completely sequential. KVM_DEV_ARM_ITS_SAVE_TABLES runs in a context where everything is locked, and so is VGIC_DIST_SAVE_VGIC3_PENDING_TABLE. M.
Hi Marc, On 1/20/23 2:47 AM, Marc Zyngier wrote: > On Thu, 19 Jan 2023 01:11:44 +0000, > Gavin Shan <gshan@redhat.com> wrote: >> >> I will have vgic_write_guest_lock() in v2. Note that those 3 paths can't be >> running in parallel since one switch is shared by them. Alternatively, we >> extend struct vgic_dist::save_tables_in_progress from 'bool' to 'unsigned long'. >> Several bit is defined for each site as below. In this way, the 3 paths can be >> running in parallel: >> >> unsigned long struct vgic_dist::save_tables_in_progress >> >> #define VGIC_DIST_SAVE_ITS_ITE 0 /* ITS Translation Entry */ >> #define VGIC_DIST_SAVE_ITS_DTE 1 /* ITS Device Table Entry */ >> #define VGIC_DIST_SAVE_ITS_CTE 2 /* ITS Collection Table Entry */ >> #define VGIC_DIST_SAVE_ITS_CT 3 /* ITS Collection Table */ >> #define VGIC_DIST_SAVE_VGIC3_LPI 4 /* VGIC3 LPI Pending Status */ >> #define VGIC_DIST_SAVE_VGIC3_PENDING_TABLE 5 /* VGIC3 Pending Table */ >> >> The drawback is the calls are limited to 64. If those 3 paths can't be running >> in parallel, we needn't the extension at all. > > It should all be completely sequential. KVM_DEV_ARM_ITS_SAVE_TABLES > runs in a context where everything is locked, and so is > VGIC_DIST_SAVE_VGIC3_PENDING_TABLE. > Thanks for your confirm. Yeah, it's sequential because 'kvm->lock' is hold on KVM_DEV_ARM_ITS_SAVE_TABLES and VGIC_DIST_SAVE_VGIC3_PENDING_TABLE. So all good to have one shared switch. v2 will be posted pretty soon. Thanks, Gavin
diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst index 9807b05a1b57..18b245a0ba02 100644 --- a/Documentation/virt/kvm/api.rst +++ b/Documentation/virt/kvm/api.rst @@ -8071,8 +8071,9 @@ state is final and avoid missing dirty pages from another ioctl ordered after the bitmap collection. NOTE: One example of using the backup bitmap is saving arm64 vgic/its -tables through KVM_DEV_ARM_{VGIC_GRP_CTRL, ITS_SAVE_TABLES} command on -KVM device "kvm-arm-vgic-its" when dirty ring is enabled. +tables and vgic3 LPI pending status through KVM_DEV_ARM_{VGIC_GRP_CTRL, +ITS_SAVE_TABLES} and KVM_DEV_ARM_{VGIC_GRP_CTRL, ITS_RESTORE_TABLES} +command on KVM device "kvm-arm-vgic-its" when dirty ring is enabled. 8.30 KVM_CAP_XEN_HVM -------------------- diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c index 94a666dd1443..119a9c7a0a52 100644 --- a/arch/arm64/kvm/vgic/vgic-its.c +++ b/arch/arm64/kvm/vgic/vgic-its.c @@ -2792,7 +2792,8 @@ bool kvm_arch_allow_write_without_running_vcpu(struct kvm *kvm) { struct vgic_dist *dist = &kvm->arch.vgic; - return dist->save_its_tables_in_progress; + return dist->save_vgic_v3_tables_in_progress || + dist->save_its_tables_in_progress; } static int vgic_its_set_attr(struct kvm_device *dev, diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c index 2074521d4a8c..32998c8587a8 100644 --- a/arch/arm64/kvm/vgic/vgic-v3.c +++ b/arch/arm64/kvm/vgic/vgic-v3.c @@ -304,6 +304,7 @@ void vgic_v3_enable(struct kvm_vcpu *vcpu) int vgic_v3_lpi_sync_pending_status(struct kvm *kvm, struct vgic_irq *irq) { struct kvm_vcpu *vcpu; + struct vgic_dist *dist = &kvm->arch.vgic; int byte_offset, bit_nr; gpa_t pendbase, ptr; bool status; @@ -339,7 +340,9 @@ int vgic_v3_lpi_sync_pending_status(struct kvm *kvm, struct vgic_irq *irq) if (status) { /* clear consumed data */ val &= ~(1 << bit_nr); + dist->save_vgic_v3_tables_in_progress = true; ret = kvm_write_guest_lock(kvm, ptr, &val, 1); + dist->save_vgic_v3_tables_in_progress = false; if (ret) return ret; } diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h index 9270cd87da3f..0485b4e82b00 100644 --- a/include/kvm/arm_vgic.h +++ b/include/kvm/arm_vgic.h @@ -264,6 +264,7 @@ struct vgic_dist { bool has_its; bool save_its_tables_in_progress; + bool save_vgic_v3_tables_in_progress; /* * Contains the attributes and gpa of the LPI configuration table.
When dirty ring is enabled, the dirty page information is pushed to the dirty ring if there is a running VCPU context. Otherwise, the dirty page information is still tracked by the backup dirty bitmap. In order to detect if there is a running VCPU context when a guest page becomes dirty, kvm_arch_allow_write_without_running_vcpu() was introduced to warn when no running VCPU context exists on unknown cases. Other than the site of saving ITS tables, it's possible to save vgic3 LPI pending status in no running vcpu context because it can happen when ITS ITE is restored through the command KVM_DEV_ARM_ITS_RESTORE_TABLES on 'kvm-arm-vgic-its' device. Fix it by allowing to save vgic3 LPI pending status in no running vcpu context. Signed-off-by: Gavin Shan <gshan@redhat.com> --- Documentation/virt/kvm/api.rst | 5 +++-- arch/arm64/kvm/vgic/vgic-its.c | 3 ++- arch/arm64/kvm/vgic/vgic-v3.c | 3 +++ include/kvm/arm_vgic.h | 1 + 4 files changed, 9 insertions(+), 3 deletions(-)