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

[Blue Swirl]

On Tue, Sep 29, 2009 at 6:57 PM, Michael S. Tsirkin <address@hidden> wrote:
> 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 <address@hidden> 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 <address@hidden> 
>> >> 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

I think these are instructions for writing system software, not
description on how MSI-X hardware needs the tables laid out. That
means, the tables should use page alignment (in order to support some
CPU attributes), but the hardware only cares that the data is QWORD
aligned.