Xen devel - Aug 2006 - [PATCH]Fix the read error from IRR,ISR and TMR

This patch fixes the error when read from APIC registers like IRR, ISR
and TMR, guest cannot get correct value.

Since from SDM3 spec, for APIC registers, all 32-bit registers should be
accessed using 128-bit aligned 32bit loads or stores. 

And wider registers (64-bit or 256-bit) must be accessed using multiple
32-bit loads or stores.

 

In old APIC virtualization code, we use IRR, ISR and TMR which are
256-bit registers as contiguous bit maps other than multiple 32-bit.

So guest always fetch error values.

 

Signed-off-by: Xiaohui Xin <xiaohui.xin@intel.com>

Signed-off-by: Yunhong Jiang <yunhong.jiang@intel.com>

Signed-off-by: Eddie Dong <eddie.dong@intel.com>




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

On 31/8/06 8:47 am, "Xin, Xiaohui" <xiaohui.xin@intel.com>
wrote:
> Since from SDM3 spec, for APIC registers, all 32-bit registers should be
> accessed using 128-bit aligned 32bit loads or stores.
> 
> And wider registers (64-bit or 256-bit) must be accessed using multiple
> 32-bit loads or stores.
> 
> In old APIC virtualization code, we use IRR, ISR and TMR which are
> 256-bit registers as contiguous bit maps other than multiple 32-bit.
SDM3 is not at all clear that these bitmaps are 8 32-bit registers rather
than a single 256-bit register. In fact, the manual makes it look very much
like they are the latter (e.g., by saying that some registers are 256 bits,
and that access to secondary 32-bit subwords happen at non-128-bit aligned
addresses), which is very misleading!

Yet another example where the manuals provide heaps of obvious and redundant
info (like explaining that APIC registers are not MSRs -- duh!) but being
ambiguous or misleading on subtle but essential details like this.

Since it turns out that *all* registers are 128-bit aligned and 32-bits, I
think the regs block should be changed to be an array of 32-bit registers,
skipping the unused parts of the APIC page (i.e., so that register x can be
found by regs[x>>4]). Then the bitmaps would be conveniently contiguous
again! And since the array would be only 1kB, it would mean we can get rid
of the alloc_domheap_page() and map_domain_page_global().

 -- Keir



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

Keir Fraser wrote:
> Since it turns out that *all* registers are 128-bit aligned and
> 32-bits, I think the regs block should be changed to be an array of
> 32-bit registers, skipping the unused parts of the APIC page (i.e.,
> so that register x can be found by regs[x>>4]). Then the bitmaps
> would be conveniently contiguous again! And since the array would be
> only 1kB, it would mean we can get rid of the alloc_domheap_page()
> and map_domain_page_global(). 
> 
Keir:
	Yes, skipping the unused parts and compact them together will
benefit us now :-) While then we may not be able to do CR8 acceleration.
The later one will need an APIC page to back for guest APIC states,
currently it is only TPR (CR8) base on VT spec. So 4K memory per
processor for each APIC page is unavoidable. 
	Meanwhile, we are thinking about another important enhancement
to accelerate some proprietary OS which will frequently access TPR. In
that patch, the APIC page is directly mapped to guest as "RO" that
greatly increase UP OS performance in our test. But the patch today is
still hold on due to SMP support issue, as today''s shadow page table is
still global. I believe eventually shadow page table will be per VP, and
the APIC acceleration patch will be out too. Then eventually we need to
organize the APIC data in hardware format like Xiaohui''s patch show.
thx,eddie

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

Ok, I hadn''t considered this. I''ll reconsider the patch!

 -- Keir

On 1/9/06 8:44 am, "Dong, Eddie" <eddie.dong@intel.com> wrote:
> Yes, skipping the unused parts and compact them together will
> benefit us now :-) While then we may not be able to do CR8 acceleration.
> The later one will need an APIC page to back for guest APIC states,
> currently it is only TPR (CR8) base on VT spec. So 4K memory per
> processor for each APIC page is unavoidable.
> Meanwhile, we are thinking about another important enhancement
> to accelerate some proprietary OS which will frequently access TPR. In
> that patch, the APIC page is directly mapped to guest as "RO"
that
> greatly increase UP OS performance in our test. But the patch today is
> still hold on due to SMP support issue, as today''s shadow page
table is
> still global. I believe eventually shadow page table will be per VP, and
> the APIC acceleration patch will be out too. Then eventually we need to
> organize the APIC data in hardware format like Xiaohui''s patch
show.


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