On Wed, 2021-10-20 at 17:10 +0200, Paolo Bonzini wrote:
> > +#define FIELD_BIT_CHANGE(name, bitmap) change_bit(f_pos(name),
> > bitmap)
> > +#define FIELD64_BIT_CHANGE(name, bitmap) \
> > + do {change_bit(f_pos(name), bitmap); \
> > + change_bit(f_pos(name) + (sizeof(u32) / sizeof(u16)),
> > bitmap);\
> > + } while (0)
> > +
> > +/*
>
> Hi Robert,
>
> I'd rather not have FIELD_BIT_CHANGE, and instead have something like
>
> #define FIELD_BIT_ASSIGN(name, bitmap, value) \
> if (value) \
> FIELD_BIT_SET(name, bitmap); \
> else
> FIELD_BIT_CLEAR(name, bitmap);
>
Sure. I had also hesitated if it could be assert that no
accident/unknown flipping on those bits by some other routine.
> Also, these set_bit/clear_bit can use the non-atomic variants
> __set_bit
> and __clear_bit, because the bitmaps are protected by the vCPU mutex.
OK, thanks.
>
> > + FIELD64_BIT_CHANGE(posted_intr_desc_addr, bitmap);
>
> Many of the fields you mark as 64-bit are actually natural sized.
Could you point me some example? I just compared with those natural
width fields in struct vmcs12{}, I don't find mistakes.
e.g. above line, "posted_intr_desc_addr" is indeed 64 bit field per SDM
vol.3 appendix B.
>
> > + if ((old_val ^ new_val) &
> > + CPU_BASED_ACTIVATE_SECONDARY_CONTROLS) {
> > + FIELD_BIT_CHANGE(secondary_vm_exec_control, bitmap);
> > + }
> > +}
>
> If secondary controls are not available, you should treat the
> corresponding MSR as if it was all zeroes. Likewise if VMFUNC is
> disabled.
OK. So you mean, if secondary_vm_exec_control bit cleared, I shall set
vmx->nested.msrs.secondary_ctls_{low,high} = 0, right?
>
> > + if ((old_val ^ new_val) & SECONDARY_EXEC_PAUSE_LOOP_EXITING) {
> > + FIELD64_BIT_CHANGE(vmread_bitmap, bitmap);
> > + FIELD64_BIT_CHANGE(vmwrite_bitmap, bitmap);
>
> This seems wrong.
You're right. Here shall be bit changes of PLE_{GAP,WINDOW}, but since
they're not defined in vmcs12{} yet, I'm going to remove these lines.
>
> Paolo
>
