Re: [Xen-devel] [PATCH v2 07/11] xen/hvmlite: Initialize context for secondary VCPUs
On 02/04/2016 07:58 AM, Doug Goldstein wrote: On 2/2/16 10:58 AM, Boris Ostrovsky wrote: On 02/02/2016 11:21 AM, David Vrabel wrote: This needs some more description in the commit message. --- a/arch/x86/xen/smp.c +++ b/arch/x86/xen/smp.c [...] +hctxt->cpu_regs.x86_32.cs_base = 0; +hctxt->cpu_regs.x86_32.cs_limit = ~0u; +hctxt->cpu_regs.x86_32.cs_ar = 0xc9b; +hctxt->cpu_regs.x86_32.ds_base = 0; +hctxt->cpu_regs.x86_32.ds_limit = ~0u; +hctxt->cpu_regs.x86_32.ds_ar = 0xc93; +hctxt->cpu_regs.x86_32.es_base = 0; +hctxt->cpu_regs.x86_32.es_limit = ~0u; +hctxt->cpu_regs.x86_32.es_ar = 0xc93; +hctxt->cpu_regs.x86_32.ss_base = 0; +hctxt->cpu_regs.x86_32.ss_limit = ~0u; +hctxt->cpu_regs.x86_32.ss_ar = 0xc93; +hctxt->cpu_regs.x86_32.tr_base = 0; +hctxt->cpu_regs.x86_32.tr_limit = 0xff; +hctxt->cpu_regs.x86_32.tr_ar = 0x8b; Lots of hard-coded values here. Should this be #defined somewhere? We also don't need to set bases to zero since hctxt is kzalloc'd. I'll remove that and add a comment. As for macros --- I couldn't find the bits defined symbolically anywhere and since this is the only place this is used the macros would be local here. -boris It could be useful to have them defined locally if only to give them some more meaning by having a name rather than 0x8b. Just a thought. Yes, I'll do that (or at least have a comment explaining the bits). Looks like this we should wait with this series though until we figure out APIC emulation status. -boris
Re: [Xen-devel] [PATCH v2 07/11] xen/hvmlite: Initialize context for secondary VCPUs
On 2/2/16 10:58 AM, Boris Ostrovsky wrote: > On 02/02/2016 11:21 AM, David Vrabel wrote: >> This needs some more description in the commit message. >> >>> --- a/arch/x86/xen/smp.c >>> +++ b/arch/x86/xen/smp.c >> [...] >>> +hctxt->cpu_regs.x86_32.cs_base = 0; >>> +hctxt->cpu_regs.x86_32.cs_limit = ~0u; >>> +hctxt->cpu_regs.x86_32.cs_ar = 0xc9b; >>> +hctxt->cpu_regs.x86_32.ds_base = 0; >>> +hctxt->cpu_regs.x86_32.ds_limit = ~0u; >>> +hctxt->cpu_regs.x86_32.ds_ar = 0xc93; >>> +hctxt->cpu_regs.x86_32.es_base = 0; >>> +hctxt->cpu_regs.x86_32.es_limit = ~0u; >>> +hctxt->cpu_regs.x86_32.es_ar = 0xc93; >>> +hctxt->cpu_regs.x86_32.ss_base = 0; >>> +hctxt->cpu_regs.x86_32.ss_limit = ~0u; >>> +hctxt->cpu_regs.x86_32.ss_ar = 0xc93; >>> +hctxt->cpu_regs.x86_32.tr_base = 0; >>> +hctxt->cpu_regs.x86_32.tr_limit = 0xff; >>> +hctxt->cpu_regs.x86_32.tr_ar = 0x8b; >> Lots of hard-coded values here. Should this be #defined somewhere? > > We also don't need to set bases to zero since hctxt is kzalloc'd. I'll > remove that and add a comment. > > As for macros --- I couldn't find the bits defined symbolically anywhere > and since this is the only place this is used the macros would be local > here. > > -boris > It could be useful to have them defined locally if only to give them some more meaning by having a name rather than 0x8b. Just a thought. -- Doug Goldstein signature.asc Description: OpenPGP digital signature
Re: [Xen-devel] [PATCH v2 07/11] xen/hvmlite: Initialize context for secondary VCPUs
On 02/02/16 16:58, Boris Ostrovsky wrote: > On 02/02/2016 11:21 AM, David Vrabel wrote: >> This needs some more description in the commit message. >> >>> --- a/arch/x86/xen/smp.c >>> +++ b/arch/x86/xen/smp.c >> [...] >>> +hctxt->cpu_regs.x86_32.cs_base = 0; >>> +hctxt->cpu_regs.x86_32.cs_limit = ~0u; >>> +hctxt->cpu_regs.x86_32.cs_ar = 0xc9b; >>> +hctxt->cpu_regs.x86_32.ds_base = 0; >>> +hctxt->cpu_regs.x86_32.ds_limit = ~0u; >>> +hctxt->cpu_regs.x86_32.ds_ar = 0xc93; >>> +hctxt->cpu_regs.x86_32.es_base = 0; >>> +hctxt->cpu_regs.x86_32.es_limit = ~0u; >>> +hctxt->cpu_regs.x86_32.es_ar = 0xc93; >>> +hctxt->cpu_regs.x86_32.ss_base = 0; >>> +hctxt->cpu_regs.x86_32.ss_limit = ~0u; >>> +hctxt->cpu_regs.x86_32.ss_ar = 0xc93; >>> +hctxt->cpu_regs.x86_32.tr_base = 0; >>> +hctxt->cpu_regs.x86_32.tr_limit = 0xff; >>> +hctxt->cpu_regs.x86_32.tr_ar = 0x8b; >> Lots of hard-coded values here. Should this be #defined somewhere? > > We also don't need to set bases to zero since hctxt is kzalloc'd. I'll > remove that and add a comment. > > As for macros --- I couldn't find the bits defined symbolically anywhere > and since this is the only place this is used the macros would be local > here. Ok. David
Re: [Xen-devel] [PATCH v2 07/11] xen/hvmlite: Initialize context for secondary VCPUs
On 02/02/16 16:58, Boris Ostrovsky wrote: > On 02/02/2016 11:21 AM, David Vrabel wrote: >> This needs some more description in the commit message. >> >>> --- a/arch/x86/xen/smp.c >>> +++ b/arch/x86/xen/smp.c >> [...] >>> +hctxt->cpu_regs.x86_32.cs_base = 0; >>> +hctxt->cpu_regs.x86_32.cs_limit = ~0u; >>> +hctxt->cpu_regs.x86_32.cs_ar = 0xc9b; >>> +hctxt->cpu_regs.x86_32.ds_base = 0; >>> +hctxt->cpu_regs.x86_32.ds_limit = ~0u; >>> +hctxt->cpu_regs.x86_32.ds_ar = 0xc93; >>> +hctxt->cpu_regs.x86_32.es_base = 0; >>> +hctxt->cpu_regs.x86_32.es_limit = ~0u; >>> +hctxt->cpu_regs.x86_32.es_ar = 0xc93; >>> +hctxt->cpu_regs.x86_32.ss_base = 0; >>> +hctxt->cpu_regs.x86_32.ss_limit = ~0u; >>> +hctxt->cpu_regs.x86_32.ss_ar = 0xc93; >>> +hctxt->cpu_regs.x86_32.tr_base = 0; >>> +hctxt->cpu_regs.x86_32.tr_limit = 0xff; >>> +hctxt->cpu_regs.x86_32.tr_ar = 0x8b; >> Lots of hard-coded values here. Should this be #defined somewhere? > > We also don't need to set bases to zero since hctxt is kzalloc'd. I'll > remove that and add a comment. > > As for macros --- I couldn't find the bits defined symbolically anywhere > and since this is the only place this is used the macros would be local > here. Ok. David
Re: [Xen-devel] [PATCH v2 07/11] xen/hvmlite: Initialize context for secondary VCPUs
On 2/2/16 10:58 AM, Boris Ostrovsky wrote: > On 02/02/2016 11:21 AM, David Vrabel wrote: >> This needs some more description in the commit message. >> >>> --- a/arch/x86/xen/smp.c >>> +++ b/arch/x86/xen/smp.c >> [...] >>> +hctxt->cpu_regs.x86_32.cs_base = 0; >>> +hctxt->cpu_regs.x86_32.cs_limit = ~0u; >>> +hctxt->cpu_regs.x86_32.cs_ar = 0xc9b; >>> +hctxt->cpu_regs.x86_32.ds_base = 0; >>> +hctxt->cpu_regs.x86_32.ds_limit = ~0u; >>> +hctxt->cpu_regs.x86_32.ds_ar = 0xc93; >>> +hctxt->cpu_regs.x86_32.es_base = 0; >>> +hctxt->cpu_regs.x86_32.es_limit = ~0u; >>> +hctxt->cpu_regs.x86_32.es_ar = 0xc93; >>> +hctxt->cpu_regs.x86_32.ss_base = 0; >>> +hctxt->cpu_regs.x86_32.ss_limit = ~0u; >>> +hctxt->cpu_regs.x86_32.ss_ar = 0xc93; >>> +hctxt->cpu_regs.x86_32.tr_base = 0; >>> +hctxt->cpu_regs.x86_32.tr_limit = 0xff; >>> +hctxt->cpu_regs.x86_32.tr_ar = 0x8b; >> Lots of hard-coded values here. Should this be #defined somewhere? > > We also don't need to set bases to zero since hctxt is kzalloc'd. I'll > remove that and add a comment. > > As for macros --- I couldn't find the bits defined symbolically anywhere > and since this is the only place this is used the macros would be local > here. > > -boris > It could be useful to have them defined locally if only to give them some more meaning by having a name rather than 0x8b. Just a thought. -- Doug Goldstein signature.asc Description: OpenPGP digital signature
Re: [Xen-devel] [PATCH v2 07/11] xen/hvmlite: Initialize context for secondary VCPUs
On 02/04/2016 07:58 AM, Doug Goldstein wrote: On 2/2/16 10:58 AM, Boris Ostrovsky wrote: On 02/02/2016 11:21 AM, David Vrabel wrote: This needs some more description in the commit message. --- a/arch/x86/xen/smp.c +++ b/arch/x86/xen/smp.c [...] +hctxt->cpu_regs.x86_32.cs_base = 0; +hctxt->cpu_regs.x86_32.cs_limit = ~0u; +hctxt->cpu_regs.x86_32.cs_ar = 0xc9b; +hctxt->cpu_regs.x86_32.ds_base = 0; +hctxt->cpu_regs.x86_32.ds_limit = ~0u; +hctxt->cpu_regs.x86_32.ds_ar = 0xc93; +hctxt->cpu_regs.x86_32.es_base = 0; +hctxt->cpu_regs.x86_32.es_limit = ~0u; +hctxt->cpu_regs.x86_32.es_ar = 0xc93; +hctxt->cpu_regs.x86_32.ss_base = 0; +hctxt->cpu_regs.x86_32.ss_limit = ~0u; +hctxt->cpu_regs.x86_32.ss_ar = 0xc93; +hctxt->cpu_regs.x86_32.tr_base = 0; +hctxt->cpu_regs.x86_32.tr_limit = 0xff; +hctxt->cpu_regs.x86_32.tr_ar = 0x8b; Lots of hard-coded values here. Should this be #defined somewhere? We also don't need to set bases to zero since hctxt is kzalloc'd. I'll remove that and add a comment. As for macros --- I couldn't find the bits defined symbolically anywhere and since this is the only place this is used the macros would be local here. -boris It could be useful to have them defined locally if only to give them some more meaning by having a name rather than 0x8b. Just a thought. Yes, I'll do that (or at least have a comment explaining the bits). Looks like this we should wait with this series though until we figure out APIC emulation status. -boris
Re: [Xen-devel] [PATCH v2 07/11] xen/hvmlite: Initialize context for secondary VCPUs
On 02/02/2016 11:21 AM, David Vrabel wrote: This needs some more description in the commit message. --- a/arch/x86/xen/smp.c +++ b/arch/x86/xen/smp.c [...] + hctxt->cpu_regs.x86_32.cs_base = 0; + hctxt->cpu_regs.x86_32.cs_limit = ~0u; + hctxt->cpu_regs.x86_32.cs_ar = 0xc9b; + hctxt->cpu_regs.x86_32.ds_base = 0; + hctxt->cpu_regs.x86_32.ds_limit = ~0u; + hctxt->cpu_regs.x86_32.ds_ar = 0xc93; + hctxt->cpu_regs.x86_32.es_base = 0; + hctxt->cpu_regs.x86_32.es_limit = ~0u; + hctxt->cpu_regs.x86_32.es_ar = 0xc93; + hctxt->cpu_regs.x86_32.ss_base = 0; + hctxt->cpu_regs.x86_32.ss_limit = ~0u; + hctxt->cpu_regs.x86_32.ss_ar = 0xc93; + hctxt->cpu_regs.x86_32.tr_base = 0; + hctxt->cpu_regs.x86_32.tr_limit = 0xff; + hctxt->cpu_regs.x86_32.tr_ar = 0x8b; Lots of hard-coded values here. Should this be #defined somewhere? We also don't need to set bases to zero since hctxt is kzalloc'd. I'll remove that and add a comment. As for macros --- I couldn't find the bits defined symbolically anywhere and since this is the only place this is used the macros would be local here. -boris
Re: [Xen-devel] [PATCH v2 07/11] xen/hvmlite: Initialize context for secondary VCPUs
This needs some more description in the commit message. > --- a/arch/x86/xen/smp.c > +++ b/arch/x86/xen/smp.c [...] > + hctxt->cpu_regs.x86_32.cs_base = 0; > + hctxt->cpu_regs.x86_32.cs_limit = ~0u; > + hctxt->cpu_regs.x86_32.cs_ar = 0xc9b; > + hctxt->cpu_regs.x86_32.ds_base = 0; > + hctxt->cpu_regs.x86_32.ds_limit = ~0u; > + hctxt->cpu_regs.x86_32.ds_ar = 0xc93; > + hctxt->cpu_regs.x86_32.es_base = 0; > + hctxt->cpu_regs.x86_32.es_limit = ~0u; > + hctxt->cpu_regs.x86_32.es_ar = 0xc93; > + hctxt->cpu_regs.x86_32.ss_base = 0; > + hctxt->cpu_regs.x86_32.ss_limit = ~0u; > + hctxt->cpu_regs.x86_32.ss_ar = 0xc93; > + hctxt->cpu_regs.x86_32.tr_base = 0; > + hctxt->cpu_regs.x86_32.tr_limit = 0xff; > + hctxt->cpu_regs.x86_32.tr_ar = 0x8b; Lots of hard-coded values here. Should this be #defined somewhere? David
Re: [Xen-devel] [PATCH v2 07/11] xen/hvmlite: Initialize context for secondary VCPUs
On 02/02/2016 11:21 AM, David Vrabel wrote: This needs some more description in the commit message. --- a/arch/x86/xen/smp.c +++ b/arch/x86/xen/smp.c [...] + hctxt->cpu_regs.x86_32.cs_base = 0; + hctxt->cpu_regs.x86_32.cs_limit = ~0u; + hctxt->cpu_regs.x86_32.cs_ar = 0xc9b; + hctxt->cpu_regs.x86_32.ds_base = 0; + hctxt->cpu_regs.x86_32.ds_limit = ~0u; + hctxt->cpu_regs.x86_32.ds_ar = 0xc93; + hctxt->cpu_regs.x86_32.es_base = 0; + hctxt->cpu_regs.x86_32.es_limit = ~0u; + hctxt->cpu_regs.x86_32.es_ar = 0xc93; + hctxt->cpu_regs.x86_32.ss_base = 0; + hctxt->cpu_regs.x86_32.ss_limit = ~0u; + hctxt->cpu_regs.x86_32.ss_ar = 0xc93; + hctxt->cpu_regs.x86_32.tr_base = 0; + hctxt->cpu_regs.x86_32.tr_limit = 0xff; + hctxt->cpu_regs.x86_32.tr_ar = 0x8b; Lots of hard-coded values here. Should this be #defined somewhere? We also don't need to set bases to zero since hctxt is kzalloc'd. I'll remove that and add a comment. As for macros --- I couldn't find the bits defined symbolically anywhere and since this is the only place this is used the macros would be local here. -boris
Re: [Xen-devel] [PATCH v2 07/11] xen/hvmlite: Initialize context for secondary VCPUs
This needs some more description in the commit message. > --- a/arch/x86/xen/smp.c > +++ b/arch/x86/xen/smp.c [...] > + hctxt->cpu_regs.x86_32.cs_base = 0; > + hctxt->cpu_regs.x86_32.cs_limit = ~0u; > + hctxt->cpu_regs.x86_32.cs_ar = 0xc9b; > + hctxt->cpu_regs.x86_32.ds_base = 0; > + hctxt->cpu_regs.x86_32.ds_limit = ~0u; > + hctxt->cpu_regs.x86_32.ds_ar = 0xc93; > + hctxt->cpu_regs.x86_32.es_base = 0; > + hctxt->cpu_regs.x86_32.es_limit = ~0u; > + hctxt->cpu_regs.x86_32.es_ar = 0xc93; > + hctxt->cpu_regs.x86_32.ss_base = 0; > + hctxt->cpu_regs.x86_32.ss_limit = ~0u; > + hctxt->cpu_regs.x86_32.ss_ar = 0xc93; > + hctxt->cpu_regs.x86_32.tr_base = 0; > + hctxt->cpu_regs.x86_32.tr_limit = 0xff; > + hctxt->cpu_regs.x86_32.tr_ar = 0x8b; Lots of hard-coded values here. Should this be #defined somewhere? David
