Keir Fraser wrote:
> 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.
>
The reason we reuse the mtrr strutures is we want to reuse the private
copy of host MTRR(mtrr_state).host MTRR state is needed to calculate
guest pte caching attribute, and host MTRR will be changed by dom0, e.g.
by dom0 graphic driver during "startx". So if adding a new mtrr
structure, there are two options here:
1. Also recording the dynamic change of host mtrr by dom0, e.g. adding
code in mtrr_wrmsr(again it will change existing arch/x86/cpu/mtrr/*).
2. Don't care about this change by dom0. In most cases, the overlapped
range between dom0 and domU is RAM. domU will not use the same MMIO
address as dom0 does. And dom0 usually only changes caching attribute
for MMIO range. Can we take this assumption here for simplicity? If
that, shadow_blow_tables() is also can be removed. Indeed, I also want
to create a new mtrr data struture, the private struture is already
changed a few days ago.
> * 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?
>
Sorry, I don't catch your idea here "any overlap implies UC". Checking
overlapping variable MTRRs is complicated, but it is as spec said. And
we add the things, such as three lookup tables, one variable to record
the overlapped status in MTRR, just in order to accerlate the procedure
to find the guest caching attribute. Get_pat_flags() is called heavily.
> * 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
Best Regards,
Disheng, Su
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home