Re: [Qemu-devel] Re: [PATCHv2] qemu: target library, use it in msix

["Michael S. Tsirkin" <mst@redhat.com>]

On Tue, Sep 29, 2009 at 06:15:21PM +0300, Blue Swirl wrote:
> On Tue, Sep 29, 2009 at 5:50 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Sun, Sep 27, 2009 at 06:19:05PM +0300, Blue Swirl wrote:
> >> On Sun, Sep 27, 2009 at 5:24 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> >> > On Sun, Sep 27, 2009 at 04:21:29PM +0200, Michael S. Tsirkin wrote:
> >> >> On Sun, Sep 27, 2009 at 04:14:49PM +0200, Avi Kivity wrote:
> >> >> > On 09/27/2009 04:08 PM, Michael S. Tsirkin wrote:
> >> >> >>
> >> >> >>
> >> >> >>>> In practice, the only user is now msix and it does not.  It has 0x1000
> >> >> >>>> as a constant parameter.  For target_phys_addr_t users if we ever have
> >> >> >>>> them, we'll just add target_phys_page_align. Generally it's unusual for
> >> >> >>>> devices to care about size of target physical page.
> >> >> >>>>
> >> >> >>>>
> >> >> >>> I'd fill better with uint64_t, at least that won't truncate.
> >> >> >>>
> >> >> >> Doesn't naming it target_page_align32 address this concern?
> >> >> >>
> >> >> >
> >> >> > How can the caller (except in your special case) know if it has a
> >> >> > quantity that will fit in 32 bits?
> >> >>
> >> >> It's actually not unusual for devices to limit addressing to 32 bit, whatever
> >> >> the bus supports.
> >> >
> >> > I would say that devices normally have a specific addressing, and should
> >> > not be using target specific types at all.  This alignment to target
> >> > page size is actually an unusual thing.
> >>
> >> Actually, AFAICT MSI-X spec (6.8.2, from the MSI entry in Wikipedia)
> >> only requires a QWORD alignment. There is some blurb about 4k
> >> alignment, but I think it only describes how software should use the
> >> structure.
> >> If this is the case, we could drop the whole target page
> >> stuff.
> >
> > The variable MSIX_PAGE_SIZE actually specifies the size of the space
> > allocated for MSIX in the memory region.  Spec requires locating MSI-X
> > tables in a 4K region separate from any other device register, so from
> > that point of view we could just have had
> > #define MSIX_PAGE_SIZE 0x1000
> 
> Can you cite the spec, I only found the QWORD stuff.

In spec revision 3.0, see this text:

	6.8.2.  MSI-X Capability and Table Structures

	...

	If a Base Address register that maps address space for the MSI-X Table or MSI-X PBA also
	maps other usable address space that is not associated with MSI-X structures, locations (e.g.,
	for CSRs) used in the other address space must not share any naturally aligned 4-KB address
	range with one where either MSI-X structure resides. This allows system software where
	applicable to use different processor attributes for MSI-X structures and the other address



> > The main reason I round the space for MSI-X tables up to cpu page size,
> > is because I have to call cpu_register_physical_memory on it, which
> > requires that size is a multiple of target page size.
> 
> There is no such requirement anymore (since r2868 in 2007), devices
> can register regions of arbitrary size and multiple devices can share
> a page.

Why does the comment on top of cpu_register_physical_memory_offset say
otherwise then? Also, this function seems to call kvm_set_phys_mem,
which also, apparently, assumes target page alignment.

-- 
MST