Re: [Xen-devel] [PATCH v3 07/32] xen/x86: fix arch_set_info_guest for HVM guests

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

On Fri, Jul 24, 2015 at 06:54:09PM +0200, Roger Pau Monné wrote:
> El 24/07/15 a les 17.49, Jan Beulich ha escrit:
> >>>> On 24.07.15 at 17:26, <roger.pau@xxxxxxxxxx> wrote:
> >> El 24/07/15 a les 14.44, Jan Beulich ha escrit:
> >>>>>> On 24.07.15 at 14:11, <roger.pau@xxxxxxxxxx> wrote:
> >>>> Or your idea was to put all the bitness specific registers inside of
> >>>> another separate structure and then unionize them? AFAICT the 16 and
> >>>> 32bit structures are going to be empty.
> >>>
> >>> How that? 64-bit mode e.g. doesn't need full descriptor data for
> >>> many of the segment registers.
> >>
> >> Please bear with me, but AFAIK cs, ds, es and ss still need to point to 
> >> a GDT entry with a 0 base and a limit of 2^64.
> > 
> > No, in 64-bit mode bases are implicitly zero (with the exception of
> > fs and gs) and limits are implicitly 2^64-1 (which you can't even
> > express in a descriptor table entry, and with the exception of AMD's
> > Long Mode Segment Limit extension).
> > 
> > But looking at the code below I see that you're thinking of selector
> > values only, which would imply that you expect the hypervisor
> > to actually read descriptors from the descriptor tables. That's
> > doable, but cumbersome.
> 
> See the comment in the bottom of the email.
> 
> >> What about the following layout:
> >>
> >> #if defined(__GNUC__) && !defined(__STRICT_ANSI__)
> >> /* Anonymous union includes 16-, 32- and 64-bit names (e.g., bp/ebp/rbp). 
> >> */
> >> # define __DECL_REG(n64, n32, n16) union {     \
> >>         uint64_t n64;                          \
> >>         uint32_t n32;                          \
> >>         uint16_t n16;                          \
> >>     }
> >> #else
> >> /* Non-gcc sources must always use the proper 64-bit name (e.g., rbp). */
> >> #define __DECL_REG(n64, n32, n16) uint64_t n64
> >> #endif
> > 
> > Since you keep repeating this, I finally have to comment on it: This
> > is a suitable model for where we use it currently. It's imo not
> > suitable at all here - not the least because even non-gcc users
> > should be allowed to use just a 32-bit field when they mean one.
> > But the problem is also that for 16- and 32-bit mode a 64-bit
> > register is meaningless: Equally well you could expose r8-r15 to
> > them.
> 
> Sorry, I didn't get that feeling by reading your previous comments. So
> you would prefer to pack everything inside of the cpu_x86_<bitness>
> structures?
> 
> >> #define __DECL_GP_REG(n) __DECL_REG(r##n, e##n, n)
> >>
> >> struct cpu_x86_16 {
> >>     /* Control registers. */
> >>     uint32_t cr[8];
> >>     /* Debug registers. */
> >>     uint32_t dr[8];
> >>     /* GDT descriptor address. */
> >>     uint16_t gdtr;
> > 
> > Even in 16-bit mode this is a 32-bit base address. And you're
> > omitting the limit here an below (which is a required part namely
> > when you expect the hypervisor to access this table).
> 
> Well, I had in mind that this would be the memory address where the GDTR
> base and limit is stored, just like the one you would use with the lgdt
> instruction, but please read below.
> 
> >> };
> >>
> >> struct cpu_x86_32 {
> >>     /* Control registers. */
> >>     uint32_t cr[8];
> >>     /* Debug registers. */
> >>     uint32_t dr[8];
> >>     /* GDT descriptor address. */
> >>     uint32_t gdtr;
> >> };
> >>
> >> struct cpu_x86_64 {
> >>     /* Additional amd64 general purpose registers. */
> >>     uint64_t r8, r9, r10, r11, r12, r13, r14, r15;
> >>     /* Control registers. */
> >>     uint64_t cr[9];
> > 
> > [16]
> 
> Why 16? Intel SDM section 2.5 only mentions cr8 as newly added in amd64.
> TBH, I'm not sure it makes much sense to be able to set cr8 from this
> interface, it can always be set by the guest OS and should not be
> required for booting.
> 
> >>     /* Debug registers. */
> >>     uint64_t dr[8];
> >>     /* Extended Feature Enable Register. */
> >>     uint64_t efer;
> >>     /* GDT descriptor address. */
> >>     uint64_t gdtr;
> >> };
> >>
> >> struct cpu_hvm_regs {
> >>     /* General purpose registers. */
> >>     __DECL_GP_REG(ax);
> >>     __DECL_GP_REG(bx);
> >>     __DECL_GP_REG(cx);
> >>     __DECL_GP_REG(dx);
> >>
> >>     /* Index registers. */
> >>     __DECL_GP_REG(di);
> >>     __DECL_GP_REG(si);
> >>
> >>     /* Pointer registers. */
> >>     __DECL_GP_REG(bp);
> >>     __DECL_GP_REG(sp);
> > 
> > Now didn't you agree to use the _natural_ order (i.e. according to
> > the numbers used to encode registers e.g. in instructions)?
> 
> OK, sorry, I didn't understand it that way. I though you only wanted
> them properly sorted (so a, b, c...) and labelled.
> 
> >> struct vcpu_hvm_context {
> >> /* 16bit fields of the strucutre will be used. */
> >> #define _VCPUHVM_MODE_16B              0
> >> #define VCPUHVM_MODE_16B               (1<<_VCPUHVM_MODE_16B)
> >> /* 32bit fields of the structure will be used. */
> >> #define _VCPUHVM_MODE_32B              1
> >> #define VCPUHVM_MODE_32B               (1<<_VCPUHVM_MODE_32B)
> >> /* 64bit fields of the structure will be used. */
> >> #define _VCPUHVM_MODE_64B              2
> >> #define VCPUHVM_MODE_64B               (1<<_VCPUHVM_MODE_64B)
> > 
> > As soon as we have three values here, a boolean flag approach
> > doesn't make sense anymore.
> 
> Yes, taking into account that only _one_ of them can be used at the same
> time.
> 
> I have the feeling that we are over engineering this interface. IMHO we
> should only allow the user to set the control registers, efer (on amd64)
> and the GP registers. Segment selectors would be set by Xen to point to
> a flat segment suitable for the mode the guest has selected. I think
> that should be enough to get the guest OS into it's entry point, and
> then it can do whatever it wants.

If you are doing that why not use the old interface?

> 
> If that's not suitable, then my second option would be to allow the
> guest to set the base, limit and AR bytes of the selectors, but not load
> a GDT.

They should be able to do any of those operations as a normal HVM
guest I would think?

> 
> Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel