| Message ID | 20230114161557.499685-2-ackerleytng@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix kvm_setup_gdt and reuse in vcpu_init_descriptor_tables | expand |
On Sat, Jan 14, 2023, Ackerley Tng wrote: > Subtract 1 to initialize GDT limit according to spec. Nit, make changelogs standalone, i.e. don't make me read the code just to understand the changelog. "Subtract 1" is meaningless without seeing the existing code. The changelog doesn't need to be a play-by-play, e.g. describing the change as "inclusive vs. exclusive" is also fine, the important thing is that readers can gain a basic understanding of the change without needing to read code. > Signed-off-by: Ackerley Tng <ackerleytng@google.com> > --- > tools/testing/selftests/kvm/lib/x86_64/processor.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c > index acfa1d01e7df..33ca7f5232a4 100644 > --- a/tools/testing/selftests/kvm/lib/x86_64/processor.c > +++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c > @@ -502,7 +502,12 @@ static void kvm_setup_gdt(struct kvm_vm *vm, struct kvm_dtable *dt) > vm->gdt = __vm_vaddr_alloc_page(vm, MEM_REGION_DATA); > > dt->base = vm->gdt; > - dt->limit = getpagesize(); > + > + /* > + * Intel SDM Volume 3, 3.5.1: As a general rule, especially in code comments, never reference manual sections by their index/numbers, there's a 99% chance the comment will be stale within a few years. Quoting manuals is ok, because if the quote because stale then that in and of itself is an interesting datapoint. If referencing a specific section is the easiest way to convey something, then use then name of the section, as that's less likely to be arbitrarily change. In this case, I'd just omit the comment entirely. We have to assume readers have a minimum level of knowledge, and IMO this is firmly below (above?) the threshold. > "the GDT limit should always be one less > + * than an integral multiple of eight" > + */ > + dt->limit = getpagesize() - 1; > } > > static void kvm_setup_tss_64bit(struct kvm_vm *vm, struct kvm_segment *segp, > -- > 2.39.0.314.g84b9a713c41-goog >
On Wed, Jan 18, 2023, Sean Christopherson wrote: > On Sat, Jan 14, 2023, Ackerley Tng wrote: > > Subtract 1 to initialize GDT limit according to spec. > > Nit, make changelogs standalone, i.e. don't make me read the code just to > understand the changelog. "Subtract 1" is meaningless without seeing the > existing code. The changelog doesn't need to be a play-by-play, e.g. describing > the change as "inclusive vs. exclusive" is also fine, the important thing is that > readers can gain a basic understanding of the change without needing to read code. > > > Signed-off-by: Ackerley Tng <ackerleytng@google.com> > > --- > > tools/testing/selftests/kvm/lib/x86_64/processor.c | 7 ++++++- > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c > > index acfa1d01e7df..33ca7f5232a4 100644 > > --- a/tools/testing/selftests/kvm/lib/x86_64/processor.c > > +++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c > > @@ -502,7 +502,12 @@ static void kvm_setup_gdt(struct kvm_vm *vm, struct kvm_dtable *dt) > > vm->gdt = __vm_vaddr_alloc_page(vm, MEM_REGION_DATA); > > > > dt->base = vm->gdt; > > - dt->limit = getpagesize(); > > + > > + /* > > + * Intel SDM Volume 3, 3.5.1: > > As a general rule, especially in code comments, never reference manual sections > by their index/numbers, there's a 99% chance the comment will be stale within a > few years. > > Quoting manuals is ok, because if the quote because stale then that in and of > itself is an interesting datapoint. If referencing a specific section is the > easiest way to convey something, then use then name of the section, as that's less > likely to be arbitrarily change. > > In this case, I'd just omit the comment entirely. We have to assume readers have > a minimum level of knowledge, and IMO this is firmly below (above?) the threshold. No need to post a v2, I'll fold this patch into a larger cleanup of the descriptor table code. Thanks!
diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c index acfa1d01e7df..33ca7f5232a4 100644 --- a/tools/testing/selftests/kvm/lib/x86_64/processor.c +++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c @@ -502,7 +502,12 @@ static void kvm_setup_gdt(struct kvm_vm *vm, struct kvm_dtable *dt) vm->gdt = __vm_vaddr_alloc_page(vm, MEM_REGION_DATA); dt->base = vm->gdt; - dt->limit = getpagesize(); + + /* + * Intel SDM Volume 3, 3.5.1: "the GDT limit should always be one less + * than an integral multiple of eight" + */ + dt->limit = getpagesize() - 1; } static void kvm_setup_tss_64bit(struct kvm_vm *vm, struct kvm_segment *segp,
Subtract 1 to initialize GDT limit according to spec. Signed-off-by: Ackerley Tng <ackerleytng@google.com> --- tools/testing/selftests/kvm/lib/x86_64/processor.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)