On Tuesday, April 8th, 2025 at 12:12 AM, Jan Beulich <jbeul...@suse.com> wrote:
>
>
> On 08.04.2025 03:15, dm...@proton.me wrote:
>
> > From: Denis Mukhin dmuk...@ford.com
> >
> > Use `asm goto()` in vmread_safe() to simplify the error handling logic.
>
>
> This can't be quite right, considering we need to avoid outputs there.
I should have experiment a bit more :-/
Thanks for reviewing this.
>
> > Update __vmread() to return `unsigned long` as per suggestion in [1].
> > Rename __vmread() to vmread_unsafe() to match the behavior.
>
>
> I disagree with this renaming: See e.g. rdmsr() and rdmsr_safe() that we have.
> The common case function wants to not have unnecessary verbosity in its name.
> And there's nothing unsafe about it in the general case. Plus if there was
> anything unsafe, many of the call sites would require some form of error
> handling.
I will revert all the naming changes and limit the scope only to __vmread()
signature change.
>
> > @@ -1957,38 +1955,44 @@ void cf_check vmx_do_resume(void)
> > hvm_do_resume(v);
> >
> > /* Sync host CR4 in case its value has changed. */
> > - __vmread(HOST_CR4, &host_cr4);
> > - if ( host_cr4 != read_cr4() )
> > + if ( vmread_unsafe(HOST_CR4) != read_cr4() )
> > __vmwrite(HOST_CR4, read_cr4());
> >
> > reset_stack_and_jump(vmx_asm_do_vmentry);
> > }
> >
> > -static inline unsigned long vmr(unsigned long field)
> > +static inline unsigned long vmread(unsigned long field)
> > {
> > - unsigned long val;
> > + unsigned long value = 0;
> >
> > - return vmread_safe(field, &val) ? 0 : val;
> > + asm goto ( "vmread %[field], %[value]\n\t"
> > + "jmp %l[out]"
>
>
> Why's the JMP needed here? With it dropped, why's open-coding of
> vmread_unsafe()
> necessary here? And why's the "safe" variant being replaced by the "unsafe"
> one?
>
> > + :
> > + : [field] "r" (field), [value] "m" (value)
>
>
> "value" is an output and hence cannot be just "m" (and hence be an input").
> The only option to make such work correctly would be to ...
>
> > + :
>
>
> ... add a "memory" clobber here. Which may have other unwanted side effects.
>
> > + : out );
> > +out:
>
>
> Nit (here and elsewhere): Labels indented by at least one blank please. See
> ./CODING_STYLE.
Overlooked formatting change.
>
> > + return value;
> > }
> >
> > -#define vmr16(fld) ({ \
> > +#define vmread16(fld) ({ \
> > BUILD_BUG_ON((fld) & 0x6001); \
> > - (uint16_t)vmr(fld); \
> > + (uint16_t)vmread(fld); \
> > })
> >
> > -#define vmr32(fld) ({ \
> > +#define vmread32(fld) ({ \
> > BUILD_BUG_ON(((fld) & 0x6001) != 0x4000); \
> > - (uint32_t)vmr(fld); \
> > + (uint32_t)vmread(fld); \
> > })
> >
> > static void vmx_dump_sel(const char *name, uint32_t selector)
> > {
> > uint32_t sel, attr, limit;
> > uint64_t base;
> > - sel = vmr(selector);
> > - attr = vmr(selector + (GUEST_ES_AR_BYTES - GUEST_ES_SELECTOR));
> > - limit = vmr(selector + (GUEST_ES_LIMIT - GUEST_ES_SELECTOR));
> > - base = vmr(selector + (GUEST_ES_BASE - GUEST_ES_SELECTOR));
> > + sel = vmread(selector);
> > + attr = vmread(selector + (GUEST_ES_AR_BYTES - GUEST_ES_SELECTOR));
> > + limit = vmread(selector + (GUEST_ES_LIMIT - GUEST_ES_SELECTOR));
> > + base = vmread(selector + (GUEST_ES_BASE - GUEST_ES_SELECTOR));
>
>
> The renaming causes entirely unnecessary extra churn here (and of course
> elsewhere). The patch is already big enough without this.
>
> > --- a/xen/arch/x86/include/asm/domain.h
> > +++ b/xen/arch/x86/include/asm/domain.h
> > @@ -595,7 +595,7 @@ struct arch_vcpu
> >
> > /* Debug registers. /
> > unsigned long dr[4];
> > - unsigned long dr7; / Ideally int, but __vmread() needs long. /
> > + unsigned long dr7; / Ideally int, but vmread_unsafe() needs unsigned
> > long. */
> > unsigned int dr6;
>
>
> If you left this comment alone, all would be (largely) fine - this particular
> aspect could then be tidied in a follow-on path. But vmread_unsafe()
> specifically
> does not need "unsigned long" anymore. The issue was with __vmread() taking a
> pointer argument.
>
> > --- a/xen/arch/x86/include/asm/hvm/vmx/vmx.h
> > +++ b/xen/arch/x86/include/asm/hvm/vmx/vmx.h
> > @@ -320,16 +320,40 @@ static always_inline void __vmpclear(u64 addr)
> > BUG();
> > }
> >
> > -static always_inline void __vmread(unsigned long field, unsigned long
> > value)
> > +static always_inline unsigned long vmread_unsafe(unsigned long field)
> > {
> > - asm volatile ( "vmread %1, %0\n\t"
> > - / CF==1 or ZF==1 --> BUG() */
> > - UNLIKELY_START(be, vmread)
> > - _ASM_BUGFRAME_TEXT(0)
> > - UNLIKELY_END_SECTION
> > - : "=rm" (*value)
> > - : "r" (field),
> > - _ASM_BUGFRAME_INFO(BUGFRAME_bug, LINE, FILE, 0) );
> > + unsigned long value;
> > +
> > + asm volatile ( "vmread %[field], %[value]\n\t"
> > + "jc 1f\n\t"
> > + "jz 1f\n\t"
>
>
> Why not JBE as it was before?
>
> > + "jmp 2f\n\t"
> > + "1:\n\t"
> > + " ud2\n\t"
> > + "2:"
>
>
> This is specifically why we used UNLIKELY_*() before. There's no justification
> whatsoever in the description for the dropping of its use here.
>
> Plus - where did _ASM_BUGFRAME_TEXT(0) go? A bare UD2 isn't acceptable, as it
> won't be possible to associate it back with the respective source line.
>
> > + : [value] "=rm" (value)
> > + : [field] "r" (field) );
> > +
> > + return value;
> > +}
> > +
> > +static inline enum vmx_insn_errno vmread_safe(unsigned long field,
> > + unsigned long *value)
> > +{
> > + asm goto ( "vmread %[field], %[value]\n\t"
> > + "jc %l[vmfail_invalid]\n\t"
> > + "jz %l[vmfail_error]"
> > + :
> > + : [field] "r" (field), [value] "m" (*value)
>
>
> See comments on the vmr() adjustments you're making.
>
> Jan
