Hi, The following 5 patches add the MTRR/PAT support into hypervisor. Signed-off-by: Disheng Su <disheng.su@intel.com> Best Regards, Disheng, Su _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2007-Oct-22 09:35 UTC
Re: [Xen-devel] [RESEND][PATCH 0/5]MTRR/PAT virtualization
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@intel.com> wrote:> Hi, > The following 5 patches add the MTRR/PAT support into > hypervisor. > > Signed-off-by: Disheng Su <disheng.su@intel.com> > > Best Regards, > Disheng, Su > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Su, Disheng
2007-Oct-22 11:38 UTC
RE: [Xen-devel] [RESEND][PATCH 0/5]MTRR/PAT virtualization
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@intel.com> wrote: > >> Hi, >> The following 5 patches add the MTRR/PAT support into hypervisor. >> >> Signed-off-by: Disheng Su <disheng.su@intel.com> >> >> Best Regards, >> Disheng, Su >> >> _______________________________________________ >> Xen-devel mailing list >> Xen-devel@lists.xensource.com >> http://lists.xensource.com/xen-develBest Regards, Disheng, Su _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2007-Oct-22 13:19 UTC
Re: [Xen-devel] [RESEND][PATCH 0/5]MTRR/PAT virtualization
All applied except for number 4 (pinning hypercall). The hypercall should be a domctl rather than a memory_op. K. On 17/10/07 09:45, "Su, Disheng" <disheng.su@intel.com> wrote:> Hi, > The following 5 patches add the MTRR/PAT support into > hypervisor. > > Signed-off-by: Disheng Su <disheng.su@intel.com> > > Best Regards, > Disheng, Su > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Su, Disheng
2007-Oct-23 10:46 UTC
RE: [Xen-devel] [RESEND][PATCH 0/5]MTRR/PAT virtualization
Keir Fraser wrote:> All applied except for number 4 (pinning hypercall). The hypercall > should be a domctl rather than a memory_op.Thanks! Attached is the new pinning hypercall per your suggestion.> > K. > > > On 17/10/07 09:45, "Su, Disheng" <disheng.su@intel.com> wrote: > >> Hi, >> The following 5 patches add the MTRR/PAT support into hypervisor. >> >> Signed-off-by: Disheng Su <disheng.su@intel.com> >> >> Best Regards, >> Disheng, Su >> >> _______________________________________________ >> Xen-devel mailing list >> Xen-devel@lists.xensource.com >> http://lists.xensource.com/xen-develBest Regards, Disheng, Su _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel