Re: [PATCH] svm: support VMCB cleanbits

[Keir Fraser <keir@xen.org>]

On 15/12/2010 12:36, "Christoph Egger" <Christoph.Egger@amd.com> wrote:

> On Wednesday 15 December 2010 12:27:51 Tim Deegan wrote:
>> This seems to change the logic so it doesn't clear the intercepts if
>> debug_state == 0.  Is that OK?
> 
> No, that's not ok. I fixed that in the new patch.
> 
>> More generally, I'm not sure I like having all the VMCB accessor
>> functions in files called "cleanbits" -- wouldn't it make sense to have
>> all that in the vmcb files so people will see them and know to use them?
>> You could rename the actual vmcb fields as well to catch anyone writing
>> them directly, e.g. in forward-ported patches.
> 
> I renamed the 'svmcleanbits.[ch]' files to 'vmc_funcs.[ch]'
> 
> Thanks for your review.

I went through this patch quite brutally when I applied it (c/s 22546). In
particular I made the VMCB field accessor functions more consistent in name
and semantics, and pulled out their implementations into a common macro to
make the code clearer.

There should be no significant changes compared with your patch *EXCEPT*:
 1. Updates to the MSR and I/O bitmaps do not affect clear bits
 2. Updates to lbr_control.enable do not affect clear bits
 3. Updates to debugctlmsr *do* affect clear bits

In the above I am following what is described in AMD Volume 2 Section
15.15.3 "VMCB Clean Field".

I note that the MSRPM_BASE and IOPM_BASE fields are listed as cacheable, but
*no* mention is made of caching the bitmap contents.

Also, bit 10 (LBR) has debugctlmsr listed as cacheable, but again *no*
mention is made of the lbr_control.enable bit flag.

If any of the above is wrong, then: (a) the reference manual should be
fixed; (b) I would accept a fixup patch, with a patch description
explaininbg why behaviour is deviating from cleanbits behaviour describved
in the latest version of the AMD reference manuals.

 -- Keir

> Signed-off-by: Wei Huang <Wei.Huang2@amd.com>
> Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>
> 
> Christoph
>