Re: [Xen-devel] [PATCH 3/6] x86/vmx: Introduce and use struct vmx_msr_bitmap

["Tian, Kevin" <kevin.tian@xxxxxxxxx>]

> From: Andrew Cooper [mailto:amc96@xxxxxxxxxxxxxxxx] On Behalf Of
> Andrew Cooper
> 
> On 27/07/2017 07:02, Tian, Kevin wrote:
> >> From: Andrew Cooper [mailto:andrew.cooper3@xxxxxxxxxx]
> >> Sent: Wednesday, July 19, 2017 7:58 PM
> >>
> >> This avoids opencoding the bitmap bases in accessor functions.  Introduce
> a
> >> build_assertions() function to check the structure layout against the
> manual
> >> definiton.  In addition, drop some stale comments and ASSERT() that
> callers
> >> pass an in-range MSR.
> >>
> >> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> > Acked-by: Kevin Tian <kevin.tian@xxxxxxxxx>, with a small comment:
> > [...]
> >
> >> diff --git a/xen/include/asm-x86/hvm/vmx/vmcs.h b/xen/include/asm-
> >> x86/hvm/vmx/vmcs.h
> >> index e318dc2..926e792 100644
> >> --- a/xen/include/asm-x86/hvm/vmx/vmcs.h
> >> +++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
> >> @@ -64,6 +64,14 @@ struct vmx_domain {
> >>      unsigned int status;
> >>  };
> >>
> >> +/* Layout of the MSR bitmap, as interpreted by hardware. */
> >> +struct vmx_msr_bitmap {
> >> +    unsigned long read_low  [0x2000 / BITS_PER_LONG];
> >> +    unsigned long read_high [0x2000 / BITS_PER_LONG];
> >> +    unsigned long write_low [0x2000 / BITS_PER_LONG];
> >> +    unsigned long write_high[0x2000 / BITS_PER_LONG];
> >> +};
> >> +
> > what about taking this chance to define 0x2000 into a macro
> > for better readability?
> 
> What would you suggest to make this more readable?
> 
> The current way it is expressed by the manuals is that the bitmaps
> covers MSRs 0 -> 0x1fff and 0xc0000000 -> 0xc0001fff which is where the
> 0x2000 is derived from.
> 
> Would this be better?
> 
> /* Layout of the MSR bitmap, as interpreted by hardware. */
> struct vmx_msr_bitmap {
>     /* Covers MSRs 0 -> 0x1fff. */
>     unsigned long read_low  [0x2000 / BITS_PER_LONG];
>     unsigned long read_high [0x2000 / BITS_PER_LONG];
>     /* Covers MSRs 0xc0000000 -> 0xc0001fff. */
>     unsigned long write_low [0x2000 / BITS_PER_LONG];
>     unsigned long write_high[0x2000 / BITS_PER_LONG];
> };
> 

yes, this way is better.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel