| Message ID | 20160801171028.11615-4-proskurin@sec.in.tum.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hello Sergej, Title: s/altp2m/p2m/ On 01/08/16 18:10, Sergej Proskurin wrote: > The struct vttbr introduces a simple way to precisely access the > individual fields of the vttbr. I am not sure whether this is really helpful. You don't seem to take often advantage of those fields and the actual accesses don't seem necessary (I will comment on the usage). > --- > xen/arch/arm/p2m.c | 8 ++++---- > xen/arch/arm/traps.c | 2 +- > xen/include/asm-arm/p2m.h | 2 +- > xen/include/asm-arm/processor.h | 16 ++++++++++++++++ > 4 files changed, 22 insertions(+), 6 deletions(-) > > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c > index 40a0b80..cbc64a1 100644 > --- a/xen/arch/arm/p2m.c > +++ b/xen/arch/arm/p2m.c > @@ -122,7 +122,7 @@ void p2m_restore_state(struct vcpu *n) > WRITE_SYSREG(hcr & ~HCR_VM, HCR_EL2); > isb(); > > - WRITE_SYSREG64(p2m->vttbr, VTTBR_EL2); > + WRITE_SYSREG64(p2m->vttbr.vttbr, VTTBR_EL2); > isb(); > > if ( is_32bit_domain(n->domain) ) > @@ -147,10 +147,10 @@ static void p2m_flush_tlb(struct p2m_domain *p2m) > * VMID. So switch to the VTTBR of a given P2M if different. > */ > ovttbr = READ_SYSREG64(VTTBR_EL2); > - if ( ovttbr != p2m->vttbr ) > + if ( ovttbr != p2m->vttbr.vttbr ) > { > local_irq_save(flags); > - WRITE_SYSREG64(p2m->vttbr, VTTBR_EL2); > + WRITE_SYSREG64(p2m->vttbr.vttbr, VTTBR_EL2); > isb(); > } > > @@ -1293,7 +1293,7 @@ static int p2m_alloc_table(struct domain *d) > > p2m->root = page; > > - p2m->vttbr = page_to_maddr(p2m->root) | ((uint64_t)p2m->vmid & 0xff) << 48; > + p2m->vttbr.vttbr = page_to_maddr(p2m->root) | ((uint64_t)p2m->vmid & 0xff) << 48; > > /* > * Make sure that all TLBs corresponding to the new VMID are flushed > diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c > index 06f06e3..12be7c9 100644 > --- a/xen/arch/arm/traps.c > +++ b/xen/arch/arm/traps.c > @@ -881,7 +881,7 @@ void vcpu_show_registers(const struct vcpu *v) > ctxt.ifsr32_el2 = v->arch.ifsr; > #endif > > - ctxt.vttbr_el2 = v->domain->arch.p2m.vttbr; > + ctxt.vttbr_el2 = v->domain->arch.p2m.vttbr.vttbr; > > _show_registers(&v->arch.cpu_info->guest_cpu_user_regs, &ctxt, 1, v); > } > diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h > index 53c4d78..5c7cd1a 100644 > --- a/xen/include/asm-arm/p2m.h > +++ b/xen/include/asm-arm/p2m.h > @@ -33,7 +33,7 @@ struct p2m_domain { > uint8_t vmid; > > /* Current Translation Table Base Register for the p2m */ > - uint64_t vttbr; > + struct vttbr vttbr; > > /* > * Highest guest frame that's ever been mapped in the p2m > diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h > index 15bf890..f8ca18c 100644 > --- a/xen/include/asm-arm/processor.h > +++ b/xen/include/asm-arm/processor.h > @@ -529,6 +529,22 @@ union hsr { > > > }; > + > +/* VTTBR: Virtualization Translation Table Base Register */ > +struct vttbr { > + union { > + struct { > + u64 baddr :40, /* variable res0: from 0-(x-1) bit */ As mentioned on the previous series, this field is 48 bits for ARMv8 (see ARM D7.2.102 in DDI 0487A.j). > + res1 :8, > + vmid :8, > + res2 :8; > + }; > + u64 vttbr; > + }; > +}; > + > +#define INVALID_VTTBR (0UL) > + > #endif > > /* HSR.EC == HSR_CP{15,14,10}_32 */ > Regards,
(CC Stefano) On 03/08/16 18:04, Julien Grall wrote: > Hello Sergej, > > Title: s/altp2m/p2m/ > > On 01/08/16 18:10, Sergej Proskurin wrote: >> The struct vttbr introduces a simple way to precisely access the >> individual fields of the vttbr. > > I am not sure whether this is really helpful. You don't seem to take > often advantage of those fields and the actual accesses don't seem > necessary (I will comment on the usage). > >> --- >> xen/arch/arm/p2m.c | 8 ++++---- >> xen/arch/arm/traps.c | 2 +- >> xen/include/asm-arm/p2m.h | 2 +- >> xen/include/asm-arm/processor.h | 16 ++++++++++++++++ >> 4 files changed, 22 insertions(+), 6 deletions(-) >> >> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c >> index 40a0b80..cbc64a1 100644 >> --- a/xen/arch/arm/p2m.c >> +++ b/xen/arch/arm/p2m.c >> @@ -122,7 +122,7 @@ void p2m_restore_state(struct vcpu *n) >> WRITE_SYSREG(hcr & ~HCR_VM, HCR_EL2); >> isb(); >> >> - WRITE_SYSREG64(p2m->vttbr, VTTBR_EL2); >> + WRITE_SYSREG64(p2m->vttbr.vttbr, VTTBR_EL2); >> isb(); >> >> if ( is_32bit_domain(n->domain) ) >> @@ -147,10 +147,10 @@ static void p2m_flush_tlb(struct p2m_domain *p2m) >> * VMID. So switch to the VTTBR of a given P2M if different. >> */ >> ovttbr = READ_SYSREG64(VTTBR_EL2); >> - if ( ovttbr != p2m->vttbr ) >> + if ( ovttbr != p2m->vttbr.vttbr ) >> { >> local_irq_save(flags); >> - WRITE_SYSREG64(p2m->vttbr, VTTBR_EL2); >> + WRITE_SYSREG64(p2m->vttbr.vttbr, VTTBR_EL2); >> isb(); >> } >> >> @@ -1293,7 +1293,7 @@ static int p2m_alloc_table(struct domain *d) >> >> p2m->root = page; >> >> - p2m->vttbr = page_to_maddr(p2m->root) | ((uint64_t)p2m->vmid & >> 0xff) << 48; >> + p2m->vttbr.vttbr = page_to_maddr(p2m->root) | >> ((uint64_t)p2m->vmid & 0xff) << 48; >> >> /* >> * Make sure that all TLBs corresponding to the new VMID are flushed >> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c >> index 06f06e3..12be7c9 100644 >> --- a/xen/arch/arm/traps.c >> +++ b/xen/arch/arm/traps.c >> @@ -881,7 +881,7 @@ void vcpu_show_registers(const struct vcpu *v) >> ctxt.ifsr32_el2 = v->arch.ifsr; >> #endif >> >> - ctxt.vttbr_el2 = v->domain->arch.p2m.vttbr; >> + ctxt.vttbr_el2 = v->domain->arch.p2m.vttbr.vttbr; >> >> _show_registers(&v->arch.cpu_info->guest_cpu_user_regs, &ctxt, 1, >> v); >> } >> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h >> index 53c4d78..5c7cd1a 100644 >> --- a/xen/include/asm-arm/p2m.h >> +++ b/xen/include/asm-arm/p2m.h >> @@ -33,7 +33,7 @@ struct p2m_domain { >> uint8_t vmid; >> >> /* Current Translation Table Base Register for the p2m */ >> - uint64_t vttbr; >> + struct vttbr vttbr; >> >> /* >> * Highest guest frame that's ever been mapped in the p2m >> diff --git a/xen/include/asm-arm/processor.h >> b/xen/include/asm-arm/processor.h >> index 15bf890..f8ca18c 100644 >> --- a/xen/include/asm-arm/processor.h >> +++ b/xen/include/asm-arm/processor.h >> @@ -529,6 +529,22 @@ union hsr { >> >> >> }; >> + >> +/* VTTBR: Virtualization Translation Table Base Register */ >> +struct vttbr { >> + union { >> + struct { >> + u64 baddr :40, /* variable res0: from 0-(x-1) bit */ > > As mentioned on the previous series, this field is 48 bits for ARMv8 > (see ARM D7.2.102 in DDI 0487A.j). > >> + res1 :8, >> + vmid :8, >> + res2 :8; >> + }; >> + u64 vttbr; >> + }; >> +}; >> + >> +#define INVALID_VTTBR (0UL) >> + >> #endif >> >> /* HSR.EC == HSR_CP{15,14,10}_32 */ >> > > Regards, >
Hi Julien, >> Hello Sergej, >> >> Title: s/altp2m/p2m/ I will adapt the titles of all patches, thank you. >> >> On 01/08/16 18:10, Sergej Proskurin wrote: >>> The struct vttbr introduces a simple way to precisely access the >>> individual fields of the vttbr. >> >> I am not sure whether this is really helpful. You don't seem to take >> often advantage of those fields and the actual accesses don't seem >> necessary (I will comment on the usage). >> >>> --- >>> xen/arch/arm/p2m.c | 8 ++++---- >>> xen/arch/arm/traps.c | 2 +- >>> xen/include/asm-arm/p2m.h | 2 +- >>> xen/include/asm-arm/processor.h | 16 ++++++++++++++++ >>> 4 files changed, 22 insertions(+), 6 deletions(-) >>> >>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c >>> index 40a0b80..cbc64a1 100644 >>> --- a/xen/arch/arm/p2m.c >>> +++ b/xen/arch/arm/p2m.c >>> @@ -122,7 +122,7 @@ void p2m_restore_state(struct vcpu *n) >>> WRITE_SYSREG(hcr & ~HCR_VM, HCR_EL2); >>> isb(); >>> >>> - WRITE_SYSREG64(p2m->vttbr, VTTBR_EL2); >>> + WRITE_SYSREG64(p2m->vttbr.vttbr, VTTBR_EL2); >>> isb(); >>> >>> if ( is_32bit_domain(n->domain) ) >>> @@ -147,10 +147,10 @@ static void p2m_flush_tlb(struct p2m_domain *p2m) >>> * VMID. So switch to the VTTBR of a given P2M if different. >>> */ >>> ovttbr = READ_SYSREG64(VTTBR_EL2); >>> - if ( ovttbr != p2m->vttbr ) >>> + if ( ovttbr != p2m->vttbr.vttbr ) >>> { >>> local_irq_save(flags); >>> - WRITE_SYSREG64(p2m->vttbr, VTTBR_EL2); >>> + WRITE_SYSREG64(p2m->vttbr.vttbr, VTTBR_EL2); >>> isb(); >>> } >>> >>> @@ -1293,7 +1293,7 @@ static int p2m_alloc_table(struct domain *d) >>> >>> p2m->root = page; >>> >>> - p2m->vttbr = page_to_maddr(p2m->root) | ((uint64_t)p2m->vmid & >>> 0xff) << 48; >>> + p2m->vttbr.vttbr = page_to_maddr(p2m->root) | >>> ((uint64_t)p2m->vmid & 0xff) << 48; >>> >>> /* >>> * Make sure that all TLBs corresponding to the new VMID are >>> flushed >>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c >>> index 06f06e3..12be7c9 100644 >>> --- a/xen/arch/arm/traps.c >>> +++ b/xen/arch/arm/traps.c >>> @@ -881,7 +881,7 @@ void vcpu_show_registers(const struct vcpu *v) >>> ctxt.ifsr32_el2 = v->arch.ifsr; >>> #endif >>> >>> - ctxt.vttbr_el2 = v->domain->arch.p2m.vttbr; >>> + ctxt.vttbr_el2 = v->domain->arch.p2m.vttbr.vttbr; >>> >>> _show_registers(&v->arch.cpu_info->guest_cpu_user_regs, &ctxt, 1, >>> v); >>> } >>> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h >>> index 53c4d78..5c7cd1a 100644 >>> --- a/xen/include/asm-arm/p2m.h >>> +++ b/xen/include/asm-arm/p2m.h >>> @@ -33,7 +33,7 @@ struct p2m_domain { >>> uint8_t vmid; >>> >>> /* Current Translation Table Base Register for the p2m */ >>> - uint64_t vttbr; >>> + struct vttbr vttbr; >>> >>> /* >>> * Highest guest frame that's ever been mapped in the p2m >>> diff --git a/xen/include/asm-arm/processor.h >>> b/xen/include/asm-arm/processor.h >>> index 15bf890..f8ca18c 100644 >>> --- a/xen/include/asm-arm/processor.h >>> +++ b/xen/include/asm-arm/processor.h >>> @@ -529,6 +529,22 @@ union hsr { >>> >>> >>> }; >>> + >>> +/* VTTBR: Virtualization Translation Table Base Register */ >>> +struct vttbr { >>> + union { >>> + struct { >>> + u64 baddr :40, /* variable res0: from 0-(x-1) bit */ >> >> As mentioned on the previous series, this field is 48 bits for ARMv8 >> (see ARM D7.2.102 in DDI 0487A.j). >> I must have missed it during refactoring. At this point, I will distinguish between __arm__ and __aarch64__, thank you. >>> + res1 :8, >>> + vmid :8, >>> + res2 :8; >>> + }; >>> + u64 vttbr; >>> + }; >>> +}; >>> + >>> +#define INVALID_VTTBR (0UL) >>> + >>> #endif >>> >>> /* HSR.EC == HSR_CP{15,14,10}_32 */ >>> >> >> Regards, >> > Best regards, ~Sergej
On 04/08/16 17:11, Sergej Proskurin wrote: >>>> diff --git a/xen/include/asm-arm/processor.h >>>> b/xen/include/asm-arm/processor.h >>>> index 15bf890..f8ca18c 100644 >>>> --- a/xen/include/asm-arm/processor.h >>>> +++ b/xen/include/asm-arm/processor.h >>>> @@ -529,6 +529,22 @@ union hsr { >>>> >>>> >>>> }; >>>> + >>>> +/* VTTBR: Virtualization Translation Table Base Register */ >>>> +struct vttbr { >>>> + union { >>>> + struct { >>>> + u64 baddr :40, /* variable res0: from 0-(x-1) bit */ >>> >>> As mentioned on the previous series, this field is 48 bits for ARMv8 >>> (see ARM D7.2.102 in DDI 0487A.j). >>> > > I must have missed it during refactoring. At this point, I will > distinguish between __arm__ and __aarch64__, thank you. After reading this series I see no point having this union. So I would much prefer to see this patch dropped. Regards,
Hi Julien, On 08/04/2016 06:15 PM, Julien Grall wrote: > > > On 04/08/16 17:11, Sergej Proskurin wrote: >>>>> diff --git a/xen/include/asm-arm/processor.h >>>>> b/xen/include/asm-arm/processor.h >>>>> index 15bf890..f8ca18c 100644 >>>>> --- a/xen/include/asm-arm/processor.h >>>>> +++ b/xen/include/asm-arm/processor.h >>>>> @@ -529,6 +529,22 @@ union hsr { >>>>> >>>>> >>>>> }; >>>>> + >>>>> +/* VTTBR: Virtualization Translation Table Base Register */ >>>>> +struct vttbr { >>>>> + union { >>>>> + struct { >>>>> + u64 baddr :40, /* variable res0: from 0-(x-1) bit */ >>>> >>>> As mentioned on the previous series, this field is 48 bits for ARMv8 >>>> (see ARM D7.2.102 in DDI 0487A.j). >>>> >> >> I must have missed it during refactoring. At this point, I will >> distinguish between __arm__ and __aarch64__, thank you. > > After reading this series I see no point having this union. So I would > much prefer to see this patch dropped. > I can do that. However, I do not understand why we would prefer using error prone bit operations for VTTBR initialization instead of having a unified and simple way of initializing and using the VTTBR including the VMID and the root table address. Best regards, ~Sergej
On 06/08/2016 09:54, Sergej Proskurin wrote: > Hi Julien, Hello Sergej, > On 08/04/2016 06:15 PM, Julien Grall wrote: >> >> >> On 04/08/16 17:11, Sergej Proskurin wrote: >>>>>> diff --git a/xen/include/asm-arm/processor.h >>>>>> b/xen/include/asm-arm/processor.h >>>>>> index 15bf890..f8ca18c 100644 >>>>>> --- a/xen/include/asm-arm/processor.h >>>>>> +++ b/xen/include/asm-arm/processor.h >>>>>> @@ -529,6 +529,22 @@ union hsr { >>>>>> >>>>>> >>>>>> }; >>>>>> + >>>>>> +/* VTTBR: Virtualization Translation Table Base Register */ >>>>>> +struct vttbr { >>>>>> + union { >>>>>> + struct { >>>>>> + u64 baddr :40, /* variable res0: from 0-(x-1) bit */ >>>>> >>>>> As mentioned on the previous series, this field is 48 bits for ARMv8 >>>>> (see ARM D7.2.102 in DDI 0487A.j). >>>>> >>> >>> I must have missed it during refactoring. At this point, I will >>> distinguish between __arm__ and __aarch64__, thank you. >> >> After reading this series I see no point having this union. So I would >> much prefer to see this patch dropped. >> > > I can do that. However, I do not understand why we would prefer using > error prone bit operations for VTTBR initialization instead of having a > unified and simple way of initializing and using the VTTBR including the > VMID and the root table address. The VTTBR only needs to be initialized in one place and we don't care accessing the fields. So I don't see the benefit to introduce a structure for that. Regards,
On 08/06/2016 03:20 PM, Julien Grall wrote: > > > On 06/08/2016 09:54, Sergej Proskurin wrote: >> Hi Julien, > > Hello Sergej, > >> On 08/04/2016 06:15 PM, Julien Grall wrote: >>> >>> >>> On 04/08/16 17:11, Sergej Proskurin wrote: >>>>>>> diff --git a/xen/include/asm-arm/processor.h >>>>>>> b/xen/include/asm-arm/processor.h >>>>>>> index 15bf890..f8ca18c 100644 >>>>>>> --- a/xen/include/asm-arm/processor.h >>>>>>> +++ b/xen/include/asm-arm/processor.h >>>>>>> @@ -529,6 +529,22 @@ union hsr { >>>>>>> >>>>>>> >>>>>>> }; >>>>>>> + >>>>>>> +/* VTTBR: Virtualization Translation Table Base Register */ >>>>>>> +struct vttbr { >>>>>>> + union { >>>>>>> + struct { >>>>>>> + u64 baddr :40, /* variable res0: from 0-(x-1) bit */ >>>>>> >>>>>> As mentioned on the previous series, this field is 48 bits for ARMv8 >>>>>> (see ARM D7.2.102 in DDI 0487A.j). >>>>>> >>>> >>>> I must have missed it during refactoring. At this point, I will >>>> distinguish between __arm__ and __aarch64__, thank you. >>> >>> After reading this series I see no point having this union. So I would >>> much prefer to see this patch dropped. >>> >> >> I can do that. However, I do not understand why we would prefer using >> error prone bit operations for VTTBR initialization instead of having a >> unified and simple way of initializing and using the VTTBR including the >> VMID and the root table address. > > The VTTBR only needs to be initialized in one place and we don't care > accessing the fields. So I don't see the benefit to introduce a > structure for that. > Ok. I will drop this patch. Best regards, ~Sergej
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c index 40a0b80..cbc64a1 100644 --- a/xen/arch/arm/p2m.c +++ b/xen/arch/arm/p2m.c @@ -122,7 +122,7 @@ void p2m_restore_state(struct vcpu *n) WRITE_SYSREG(hcr & ~HCR_VM, HCR_EL2); isb(); - WRITE_SYSREG64(p2m->vttbr, VTTBR_EL2); + WRITE_SYSREG64(p2m->vttbr.vttbr, VTTBR_EL2); isb(); if ( is_32bit_domain(n->domain) ) @@ -147,10 +147,10 @@ static void p2m_flush_tlb(struct p2m_domain *p2m) * VMID. So switch to the VTTBR of a given P2M if different. */ ovttbr = READ_SYSREG64(VTTBR_EL2); - if ( ovttbr != p2m->vttbr ) + if ( ovttbr != p2m->vttbr.vttbr ) { local_irq_save(flags); - WRITE_SYSREG64(p2m->vttbr, VTTBR_EL2); + WRITE_SYSREG64(p2m->vttbr.vttbr, VTTBR_EL2); isb(); } @@ -1293,7 +1293,7 @@ static int p2m_alloc_table(struct domain *d) p2m->root = page; - p2m->vttbr = page_to_maddr(p2m->root) | ((uint64_t)p2m->vmid & 0xff) << 48; + p2m->vttbr.vttbr = page_to_maddr(p2m->root) | ((uint64_t)p2m->vmid & 0xff) << 48; /* * Make sure that all TLBs corresponding to the new VMID are flushed diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c index 06f06e3..12be7c9 100644 --- a/xen/arch/arm/traps.c +++ b/xen/arch/arm/traps.c @@ -881,7 +881,7 @@ void vcpu_show_registers(const struct vcpu *v) ctxt.ifsr32_el2 = v->arch.ifsr; #endif - ctxt.vttbr_el2 = v->domain->arch.p2m.vttbr; + ctxt.vttbr_el2 = v->domain->arch.p2m.vttbr.vttbr; _show_registers(&v->arch.cpu_info->guest_cpu_user_regs, &ctxt, 1, v); } diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h index 53c4d78..5c7cd1a 100644 --- a/xen/include/asm-arm/p2m.h +++ b/xen/include/asm-arm/p2m.h @@ -33,7 +33,7 @@ struct p2m_domain { uint8_t vmid; /* Current Translation Table Base Register for the p2m */ - uint64_t vttbr; + struct vttbr vttbr; /* * Highest guest frame that's ever been mapped in the p2m diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h index 15bf890..f8ca18c 100644 --- a/xen/include/asm-arm/processor.h +++ b/xen/include/asm-arm/processor.h @@ -529,6 +529,22 @@ union hsr { }; + +/* VTTBR: Virtualization Translation Table Base Register */ +struct vttbr { + union { + struct { + u64 baddr :40, /* variable res0: from 0-(x-1) bit */ + res1 :8, + vmid :8, + res2 :8; + }; + u64 vttbr; + }; +}; + +#define INVALID_VTTBR (0UL) + #endif /* HSR.EC == HSR_CP{15,14,10}_32 */