Re: [Xen-devel] [RESEND][PATCH 0/5]MTRR/PAT virtualization

[Keir Fraser <Keir.Fraser@xxxxxxxxxxxx>]

Some comments:

 * Do not steal the private mtrr structures out of arch/x86/cpu/mtrr/*.
Instead define your own new ones in include/asm-x86/hvm/mtrr.h. Yes, there
will be some overlap with the existing definitions, but the structures are
small, and also there are fields that are applicable to one case and not the
other (e.g., the new fields you add are not really needed by arch/x86/mtrr
code; and some of the existing fields (e.g., has_fixed) are not relevant to
the new HVM virtual-mtrr code). It's cleaner just to totally separate the
two imo.

 * Do not change the existing arch/x86/cpu/mtrr/* code. Some of that
naturally disappears since you will no longer be modifying the mtrr state
structures for that code. Also the shadow_blow_tables() call is imo overly
conservative and is in any case a bug since you will likely be calling it
from irq context, which is invalid. IMO, best to assume that host MTRRs are
set up *before* relevant HVM passthru guests are created. At least in this
initial patchset.

 * Clean up coding style. All new files should follow Xen coding style. The
new hvm/mtrr.c file is a bit of a stylistic mess. Also I think it is being
too clever for its own good. E.g., the logic for overlapping variable MTRRs
in get_mtrr_type() is freaking complicated, when the spec allows us to say
that any overlap implies UC. How hard should that be to implement, really?

 * The new hypercall should be a domctl(), not a memory_op(). It's only to
be used by dom0 for control of other guests.

That's it for now!

 -- Keir

On 17/10/07 09:45, "Su, Disheng" <disheng.su@xxxxxxxxx> wrote:

> Hi,
> The following 5 patches add the MTRR/PAT support into
> hypervisor. 
> 
> Signed-off-by: Disheng Su <disheng.su@xxxxxxxxx>
> 
> Best Regards,
> Disheng, Su
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxxxxxxxx
> http://lists.xensource.com/xen-devel



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel