| Message ID | 20090923200635.GA21246@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
On Wed, Sep 23, 2009 at 11:06 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > This creates target.c, which builds per-target, and makes it possible > for devices to become target-independent. Use it in msix, reverting > part of 5e520a7d500ec2569d22d80f9ef4272a34cb3c80, as we no longer have > to pass target page around. > +unsigned target_page_align(unsigned value) > +{ > + return TARGET_PAGE_ALIGN(value); > +} This must be: target_phys_addr_t target_page_align(target_phys_addr_t value) As this is not a clean revert anyway, please don't revert the part changing if (x) y; else z; to if (x) { y; } else { z; }
On Thu, Sep 24, 2009 at 08:50:11PM +0300, Blue Swirl wrote: > On Wed, Sep 23, 2009 at 11:06 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > > This creates target.c, which builds per-target, and makes it possible > > for devices to become target-independent. Use it in msix, reverting > > part of 5e520a7d500ec2569d22d80f9ef4272a34cb3c80, as we no longer have > > to pass target page around. > > > +unsigned target_page_align(unsigned value) > > +{ > > + return TARGET_PAGE_ALIGN(value); > > +} > > This must be: > target_phys_addr_t target_page_align(target_phys_addr_t value) what's the point then? It has to be target independent. Let's make it unsigned long long, should be good enough. > As this is not a clean revert anyway, please don't revert the part changing > if (x) > y; > else > z; > > to > > if (x) { > y; > } else { > z; > } Did I mention I hate this style? But okay :).
On Thu, Sep 24, 2009 at 10:11 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > On Thu, Sep 24, 2009 at 08:50:11PM +0300, Blue Swirl wrote: >> On Wed, Sep 23, 2009 at 11:06 PM, Michael S. Tsirkin <mst@redhat.com> wrote: >> > This creates target.c, which builds per-target, and makes it possible >> > for devices to become target-independent. Use it in msix, reverting >> > part of 5e520a7d500ec2569d22d80f9ef4272a34cb3c80, as we no longer have >> > to pass target page around. >> >> > +unsigned target_page_align(unsigned value) >> > +{ >> > + return TARGET_PAGE_ALIGN(value); >> > +} >> >> This must be: >> target_phys_addr_t target_page_align(target_phys_addr_t value) > > what's the point then? It has to be target independent. > Let's make it unsigned long long, should be good enough. target_phys_addr_t is handled by hwlib definitions, so it can be used for hwlib devices. If you can remove all target_phys_addr_t uses, then the compilation of the file could be moved from Makefile.hw to Makefile. One day the bus addresses should be decoupled from the target CPU addresses, but for now we are stick with target_phys_addr_t. >> As this is not a clean revert anyway, please don't revert the part changing >> if (x) >> y; >> else >> z; >> >> to >> >> if (x) { >> y; >> } else { >> z; >> } > > Did I mention I hate this style? But okay :). I'm not a great fan of this either, but this is the CODING_STYLE.
On Thu, Sep 24, 2009 at 08:50:11PM +0300, Blue Swirl wrote: > On Wed, Sep 23, 2009 at 11:06 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > > This creates target.c, which builds per-target, and makes it possible > > for devices to become target-independent. Use it in msix, reverting > > part of 5e520a7d500ec2569d22d80f9ef4272a34cb3c80, as we no longer have > > to pass target page around. > > > +unsigned target_page_align(unsigned value) > > +{ > > + return TARGET_PAGE_ALIGN(value); > > +} > > This must be: > target_phys_addr_t target_page_align(target_phys_addr_t value) Thinking about this some more, this function just says "align a value to page size". The value might not be a bus address at all, and indeed with msix use, it is not. Makes sense?
On 09/27/2009 10:20 AM, Michael S. Tsirkin wrote: > On Thu, Sep 24, 2009 at 08:50:11PM +0300, Blue Swirl wrote: > >> On Wed, Sep 23, 2009 at 11:06 PM, Michael S. Tsirkin<mst@redhat.com> wrote: >> >>> This creates target.c, which builds per-target, and makes it possible >>> for devices to become target-independent. Use it in msix, reverting >>> part of 5e520a7d500ec2569d22d80f9ef4272a34cb3c80, as we no longer have >>> to pass target page around. >>> >> >>> +unsigned target_page_align(unsigned value) >>> +{ >>> + return TARGET_PAGE_ALIGN(value); >>> +} >>> >> This must be: >> target_phys_addr_t target_page_align(target_phys_addr_t value) >> > Thinking about this some more, this function just says "align a value to > page size". The value might not be a bus address at all, and indeed with > msix use, it is not. Makes sense? > In any case restricting it to unsigned invites truncation. We can have target_phys_addr_t target_phys_page_align(); target_ulong_t target_virt_page_align(); or just uint64_t target_page_align(); (which is wasteful on 32/32 guests and 32 hosts).
On Sun, Sep 27, 2009 at 12:40:16PM +0200, Avi Kivity wrote: > On 09/27/2009 10:20 AM, Michael S. Tsirkin wrote: >> On Thu, Sep 24, 2009 at 08:50:11PM +0300, Blue Swirl wrote: >> >>> On Wed, Sep 23, 2009 at 11:06 PM, Michael S. Tsirkin<mst@redhat.com> wrote: >>> >>>> This creates target.c, which builds per-target, and makes it possible >>>> for devices to become target-independent. Use it in msix, reverting >>>> part of 5e520a7d500ec2569d22d80f9ef4272a34cb3c80, as we no longer have >>>> to pass target page around. >>>> >>> >>>> +unsigned target_page_align(unsigned value) >>>> +{ >>>> + return TARGET_PAGE_ALIGN(value); >>>> +} >>>> >>> This must be: >>> target_phys_addr_t target_page_align(target_phys_addr_t value) >>> >> Thinking about this some more, this function just says "align a value to >> page size". The value might not be a bus address at all, and indeed with >> msix use, it is not. Makes sense? >> > > In any case restricting it to unsigned invites truncation. If the value we are aligning fits in 32 bit, so does the aligned value. > We can have > > target_phys_addr_t target_phys_page_align(); > target_ulong_t target_virt_page_align(); Right. And also target_page_align32(); target_page_align64(); We only use a 32 bit version now (passed value is a constant), so I think I'll only implement it, and we'll add more when they are used. > or just > > uint64_t target_page_align(); > (which is wasteful on 32/32 guests and 32 hosts). > -- > Do not meddle in the internals of kernels, for they are subtle and quick to panic.
On 09/27/2009 01:45 PM, Michael S. Tsirkin wrote: > > >>> Thinking about this some more, this function just says "align a value to >>> page size". The value might not be a bus address at all, and indeed with >>> msix use, it is not. Makes sense? >>> >>> >> In any case restricting it to unsigned invites truncation. >> > If the value we are aligning fits in 32 bit, so does the aligned value. > It's perfectly reasonable to call a such a function with a target_ulong_t or target_phys_addr_t input and expect it to work. >> We can have >> >> target_phys_addr_t target_phys_page_align(); >> target_ulong_t target_virt_page_align(); >> > Right. And also > target_page_align32(); > target_page_align64(); > > We only use a 32 bit version now (passed value is a constant), > so I think I'll only implement it, and we'll add more when > they are used. > How would the caller know what size argument they have? They usually have a target_phys_addr_t or a target_ulong_t.
On Sun, Sep 27, 2009 at 01:55:17PM +0200, Avi Kivity wrote: > On 09/27/2009 01:45 PM, Michael S. Tsirkin wrote: >> >> >>>> Thinking about this some more, this function just says "align a value to >>>> page size". The value might not be a bus address at all, and indeed with >>>> msix use, it is not. Makes sense? >>>> >>>> >>> In any case restricting it to unsigned invites truncation. >>> >> If the value we are aligning fits in 32 bit, so does the aligned value. >> > > It's perfectly reasonable to call a such a function with a > target_ulong_t [mst@tuck qemu]$ grep -rIi target_ulong_t . [mst@tuck qemu]$ ? > or target_phys_addr_t input and expect it to work. Not if it's called target_page_align32 :) >>> We can have >>> >>> target_phys_addr_t target_phys_page_align(); >>> target_ulong_t target_virt_page_align(); >>> >> Right. And also >> target_page_align32(); >> target_page_align64(); >> >> We only use a 32 bit version now (passed value is a constant), >> so I think I'll only implement it, and we'll add more when >> they are used. >> > > How would the caller know what size argument they have? They usually > have a target_phys_addr_t or a target_ulong_t. 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. > -- > Do not meddle in the internals of kernels, for they are subtle and quick to panic.
On 09/27/2009 02:00 PM, Michael S. Tsirkin wrote: > On Sun, Sep 27, 2009 at 01:55:17PM +0200, Avi Kivity wrote: > >> On 09/27/2009 01:45 PM, Michael S. Tsirkin wrote: >> >>> >>> >>>>> Thinking about this some more, this function just says "align a value to >>>>> page size". The value might not be a bus address at all, and indeed with >>>>> msix use, it is not. Makes sense? >>>>> >>>>> >>>>> >>>> In any case restricting it to unsigned invites truncation. >>>> >>>> >>> If the value we are aligning fits in 32 bit, so does the aligned value. >>> >>> >> It's perfectly reasonable to call a such a function with a >> target_ulong_t >> > [mst@tuck qemu]$ grep -rIi target_ulong_t . > [mst@tuck qemu]$ > > ? > > target_ulong. >> or target_phys_addr_t input and expect it to work. >> > Not if it's called target_page_align32 :) > You generally don't know the size of quantities you align. >> How would the caller know what size argument they have? They usually >> have a target_phys_addr_t or a target_ulong_t. >> > 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.
On Sun, Sep 27, 2009 at 02:19:41PM +0200, Avi Kivity wrote: > On 09/27/2009 02:00 PM, Michael S. Tsirkin wrote: >> On Sun, Sep 27, 2009 at 01:55:17PM +0200, Avi Kivity wrote: >> >>> On 09/27/2009 01:45 PM, Michael S. Tsirkin wrote: >>> >>>> >>>> >>>>>> Thinking about this some more, this function just says "align a value to >>>>>> page size". The value might not be a bus address at all, and indeed with >>>>>> msix use, it is not. Makes sense? >>>>>> >>>>>> >>>>>> >>>>> In any case restricting it to unsigned invites truncation. >>>>> >>>>> >>>> If the value we are aligning fits in 32 bit, so does the aligned value. >>>> >>>> >>> It's perfectly reasonable to call a such a function with a >>> target_ulong_t >>> >> [mst@tuck qemu]$ grep -rIi target_ulong_t . >> [mst@tuck qemu]$ >> >> ? >> >> > > target_ulong. > >>> or target_phys_addr_t input and expect it to work. >>> >> Not if it's called target_page_align32 :) >> > > You generally don't know the size of quantities you align. > >>> How would the caller know what size argument they have? They usually >>> have a target_phys_addr_t or a target_ulong_t. >>> >> 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? > -- > Do not meddle in the internals of kernels, for they are subtle and quick to panic.
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?
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. For example, the value might come from a 32 bit pci bar, even on a 64 bit system this will get values 0 to 4G. > > -- > Do not meddle in the internals of kernels, for they are subtle and quick to panic.
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. > For example, the value might come from a 32 bit pci > bar, even on a 64 bit system this will get values 0 to 4G. > > > > > -- > > Do not meddle in the internals of kernels, for they are subtle and quick to panic.
On 09/27/2009 04:21 PM, Michael S. Tsirkin wrote: > >> 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. For example, the value might come from a 32 bit pci > bar, even on a 64 bit system this will get values 0 to 4G. > What if the caller is a device that doesn't intentionally cripple itself?
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.
On Sun, Sep 27, 2009 at 04:26:33PM +0200, Avi Kivity wrote: > On 09/27/2009 04:21 PM, Michael S. Tsirkin wrote: >> >>> 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. For example, the value might come from a 32 bit pci >> bar, even on a 64 bit system this will get values 0 to 4G. >> > > What if the caller is a device that doesn't intentionally cripple itself? That will need a 64 bit version, even if target is 32 bit. > -- > Do not meddle in the internals of kernels, for they are subtle and quick to panic.
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 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.
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. > 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.
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.
On Thu, Sep 24, 2009 at 11:13:38PM +0300, Blue Swirl wrote: > >> As this is not a clean revert anyway, please don't revert the part changing > >> if (x) > >> y; > >> else > >> z; > >> > >> to > >> > >> if (x) { > >> y; > >> } else { > >> z; > >> } > > > > Did I mention I hate this style? But okay :). > > I'm not a great fan of this either, but this is the CODING_STYLE. Maybe we can all vote to change this?
On 09/29/2009 05:57 PM, Michael S. Tsirkin wrote: > >> 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. > That's only for RAM, which the msix area isn't.
On Tue, Sep 29, 2009 at 06:26:14PM +0200, Avi Kivity wrote: > On 09/29/2009 05:57 PM, Michael S. Tsirkin wrote: >> >>> 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. >> > > That's only for RAM, which the msix area isn't. OK, looks like things should just work defining MSIX_PAGE_SIZE to 0x1000 then. Unsurprisingly, this works fine on intel here. > -- > error compiling committee.c: too many arguments to function
On Tue, Sep 29, 2009 at 7:11 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > On Thu, Sep 24, 2009 at 11:13:38PM +0300, Blue Swirl wrote: >> >> As this is not a clean revert anyway, please don't revert the part changing >> >> if (x) >> >> y; >> >> else >> >> z; >> >> >> >> to >> >> >> >> if (x) { >> >> y; >> >> } else { >> >> z; >> >> } >> > >> > Did I mention I hate this style? But okay :). >> >> I'm not a great fan of this either, but this is the CODING_STYLE. > > Maybe we can all vote to change this? Actually, I tried to tweak 'indent' to match QEMU style without much success. We could also select a new style, like Linux kernel one. I'm also not a great fan of that either. But at least 'indent' could be used very easily and I think there would be other benefits, like reuse of Git hooks, patch checking scripts and Emacs configs. With full 'indent' support all sources could be massaged mechanically to make the switch less painful. Then there are K&R and GNU styles, but I like them even less.
On Tue, Sep 29, 2009 at 6:57 PM, Michael S. Tsirkin <mst@redhat.com> 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 <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 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.
On Tue, Sep 29, 2009 at 10:34:53PM +0300, Blue Swirl wrote: > On Tue, Sep 29, 2009 at 6:57 PM, Michael S. Tsirkin <mst@redhat.com> 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 <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 > > 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. Yes. All I do is make sure tables are page aligned.
On Tue, Sep 29, 2009 at 09:15:04PM +0300, Blue Swirl wrote: > On Tue, Sep 29, 2009 at 7:11 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > > On Thu, Sep 24, 2009 at 11:13:38PM +0300, Blue Swirl wrote: > >> >> As this is not a clean revert anyway, please don't revert the part changing > >> >> if (x) > >> >> y; > >> >> else > >> >> z; > >> >> > >> >> to > >> >> > >> >> if (x) { > >> >> y; > >> >> } else { > >> >> z; > >> >> } > >> > > >> > Did I mention I hate this style? But okay :). > >> > >> I'm not a great fan of this either, but this is the CODING_STYLE. > > > > Maybe we can all vote to change this? > > Actually, I tried to tweak 'indent' to match QEMU style without much success. > > We could also select a new style, like Linux kernel one. I'm also not > a great fan of that either. But at least 'indent' could be used very > easily and I think there would be other benefits, like reuse of Git > hooks, patch checking scripts and Emacs configs. With full 'indent' > support all sources could be massaged mechanically to make the switch > less painful. > > Then there are K&R and GNU styles, but I like them even less. So ... Linux kernel style wins? What do others think?
On Tue, Sep 29, 2009 at 09:15:04PM +0300, Blue Swirl wrote: > We could also select a new style, like Linux kernel one. I'm also not > a great fan of that either. But at least 'indent' could be used very > easily and I think there would be other benefits, like reuse of Git > hooks, patch checking scripts and Emacs configs. With full 'indent' > support all sources could be massaged mechanically to make the switch > less painful. > > Then there are K&R and GNU styles, but I like them even less. Everything but GNU obsfucation :) Seriously, something resembling Linux CodingStyle or style(9) in the various BSDs would probably help a lot of people avoiding to context switch too much.
On Wed, Sep 30, 2009 at 06:06:17PM +0200, Christoph Hellwig wrote: > On Tue, Sep 29, 2009 at 09:15:04PM +0300, Blue Swirl wrote: > > We could also select a new style, like Linux kernel one. I'm also not > > a great fan of that either. But at least 'indent' could be used very > > easily and I think there would be other benefits, like reuse of Git > > hooks, patch checking scripts and Emacs configs. With full 'indent' > > support all sources could be massaged mechanically to make the switch > > less painful. > > > > Then there are K&R and GNU styles, but I like them even less. > > Everything but GNU obsfucation :) Seriously, something resembling Linux > CodingStyle or style(9) in the various BSDs would probably help a lot > of people avoiding to context switch too much. OK, thats 3 people in favor of switching to linux CodingStyle so far.
On Wed, 30 Sep 2009, Michael S. Tsirkin wrote: > On Tue, Sep 29, 2009 at 09:15:04PM +0300, Blue Swirl wrote: > > On Tue, Sep 29, 2009 at 7:11 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > > > On Thu, Sep 24, 2009 at 11:13:38PM +0300, Blue Swirl wrote: > > >> >> As this is not a clean revert anyway, please don't revert the part changing > > >> >> if (x) > > >> >> y; > > >> >> else > > >> >> z; > > >> >> > > >> >> to > > >> >> > > >> >> if (x) { > > >> >> y; > > >> >> } else { > > >> >> z; > > >> >> } > > >> > > > >> > Did I mention I hate this style? But okay :). > > >> > > >> I'm not a great fan of this either, but this is the CODING_STYLE. > > > > > > Maybe we can all vote to change this? > > > > Actually, I tried to tweak 'indent' to match QEMU style without much success. > > > > We could also select a new style, like Linux kernel one. I'm also not > > a great fan of that either. But at least 'indent' could be used very > > easily and I think there would be other benefits, like reuse of Git > > hooks, patch checking scripts and Emacs configs. With full 'indent' > > support all sources could be massaged mechanically to make the switch > > less painful. > > > > Then there are K&R and GNU styles, but I like them even less. > > So ... Linux kernel style wins? What do others think? I don't think so.
On 09/30/2009 03:51 PM, Michael S. Tsirkin wrote: > So ... Linux kernel style wins? What do others think? > Any change is going to cause a large amount of pain if implemented.
"Michael S. Tsirkin" <mst@redhat.com> wrote: .... >> Actually, I tried to tweak 'indent' to match QEMU style without much success. >> >> We could also select a new style, like Linux kernel one. I'm also not >> a great fan of that either. But at least 'indent' could be used very >> easily and I think there would be other benefits, like reuse of Git >> hooks, patch checking scripts and Emacs configs. With full 'indent' >> support all sources could be massaged mechanically to make the switch >> less painful. >> >> Then there are K&R and GNU styles, but I like them even less. > > So ... Linux kernel style wins? What do others think? Where do I have to sign? I very much prefer Linux kernel style, but that is a preference. Having an style that indent understands is a great idea, as it makes trivial to make sure that you are not going againsnt CodingStyle. Bigger advantage of Linux Kernel one is the infrastructure/hooks already again. Later, Juan. PD. I am also in the team that dislikes K&R and GNU (speciall GNU). From linux-2.6/Documentation/CodingStyle Linux kernel coding style ..... First off, I'd suggest printing out a copy of the GNU coding standards, and NOT read it. Burn them, it's a great symbolic gesture.
"Michael S. Tsirkin" <mst@redhat.com> writes: > On Tue, Sep 29, 2009 at 09:15:04PM +0300, Blue Swirl wrote: >> On Tue, Sep 29, 2009 at 7:11 PM, Michael S. Tsirkin <mst@redhat.com> wrote: >> > On Thu, Sep 24, 2009 at 11:13:38PM +0300, Blue Swirl wrote: >> >> >> As this is not a clean revert anyway, please don't revert the part changing >> >> >> if (x) >> >> >> y; >> >> >> else >> >> >> z; >> >> >> >> >> >> to >> >> >> >> >> >> if (x) { >> >> >> y; >> >> >> } else { >> >> >> z; >> >> >> } >> >> > >> >> > Did I mention I hate this style? But okay :). >> >> >> >> I'm not a great fan of this either, but this is the CODING_STYLE. >> > >> > Maybe we can all vote to change this? >> >> Actually, I tried to tweak 'indent' to match QEMU style without much success. >> >> We could also select a new style, like Linux kernel one. I'm also not >> a great fan of that either. But at least 'indent' could be used very >> easily and I think there would be other benefits, like reuse of Git >> hooks, patch checking scripts and Emacs configs. With full 'indent' >> support all sources could be massaged mechanically to make the switch >> less painful. >> >> Then there are K&R and GNU styles, but I like them even less. > > So ... Linux kernel style wins? What do others think? I find certain aspects of QEMU style rather distasteful, too. Linux kernel style is okay. It can push deeply indented code far to the right and over the cliff, though. Some claim the only correct way to format such code is to indent six feet downward and cover with dirt, but that's not always practical. Nevertheless, I'd favor adopting Linux kernel style. The world doesn't need more indentation styles.
On Wed, Sep 30, 2009 at 07:00:20PM +0200, Avi Kivity wrote: > On 09/30/2009 03:51 PM, Michael S. Tsirkin wrote: >> So ... Linux kernel style wins? What do others think? >> > > > Any change is going to cause a large amount of pain if implemented. First thing would be to start running checkpatch on submissions. This way, we gradually convert. Current style does not have automated checker, so new code constantly deviates from it. > -- > error compiling committee.c: too many arguments to function
On Wed, Sep 30, 2009 at 8:00 PM, Avi Kivity <avi@redhat.com> wrote: > On 09/30/2009 03:51 PM, Michael S. Tsirkin wrote: >> >> So ... Linux kernel style wins? What do others think? >> > > > Any change is going to cause a large amount of pain if implemented. True, but with 'indent' support the pain would be brief.
>> We could also select a new style, like Linux kernel one. I'm also not >> a great fan of that either. But at least 'indent' could be used very >> easily and I think there would be other benefits, like reuse of Git >> hooks, patch checking scripts and Emacs configs. With full 'indent' >> support all sources could be massaged mechanically to make the switch >> less painful. >> >> Then there are K&R and GNU styles, but I like them even less. > > So ... Linux kernel style wins? What do others think? Linux kernel with 4-char indent? That would avoid the need to run indent (indent requires knowledge of all the names of the types, so a mechanical pass through the sources is more easily said than done). Paolo
On Wed, Sep 30, 2009 at 07:31:36PM +0200, Paolo Bonzini wrote: > >>> We could also select a new style, like Linux kernel one. I'm also not >>> a great fan of that either. But at least 'indent' could be used very >>> easily and I think there would be other benefits, like reuse of Git >>> hooks, patch checking scripts and Emacs configs. With full 'indent' >>> support all sources could be massaged mechanically to make the switch >>> less painful. >>> >>> Then there are K&R and GNU styles, but I like them even less. >> >> So ... Linux kernel style wins? What do others think? > > Linux kernel with 4-char indent? That would avoid the need to run > indent That loses the benefit of automatic style checkers/formatters developed for kernel. > (indent requires knowledge of all the names of the types, so a > mechanical pass through the sources is more easily said than done). > > Paolo vim does a decent job typically.
On Wed, Sep 30, 2009 at 07:31:36PM +0200, Paolo Bonzini wrote: > (indent requires knowledge of all the names of the types, so a > mechanical pass through the sources is more easily said than done). How do you tell it the names of the types?
On 09/30/2009 07:48 PM, Michael S. Tsirkin wrote: > On Wed, Sep 30, 2009 at 07:31:36PM +0200, Paolo Bonzini wrote: >> (indent requires knowledge of all the names of the types, so a >> mechanical pass through the sources is more easily said than done). > > How do you tell it the names of the types? You use the `-T' option. `-T' can be specified more than once, and all names specified are used. For example, if your program contains typedef unsigned long CODE_ADDR; typedef enum {red, blue, green} COLOR; you would use the options `-T CODE_ADDR -T COLOR'. Actually, you only need that for types that are used in arguments and declared in .h files (as opposed to the same .c file). Example: typedef struct color color; void f(color* p) works, but if you remove the typedef you get void f (color * p) Paolo
"Michael S. Tsirkin" <mst@redhat.com> writes: > On Wed, Sep 30, 2009 at 07:31:36PM +0200, Paolo Bonzini wrote: >> (indent requires knowledge of all the names of the types, so a >> mechanical pass through the sources is more easily said than done). > > How do you tell it the names of the types? -T
Paolo Bonzini wrote: > >>> We could also select a new style, like Linux kernel one. I'm also not >>> a great fan of that either. But at least 'indent' could be used very >>> easily and I think there would be other benefits, like reuse of Git >>> hooks, patch checking scripts and Emacs configs. With full 'indent' >>> support all sources could be massaged mechanically to make the switch >>> less painful. >>> >>> Then there are K&R and GNU styles, but I like them even less. >> >> So ... Linux kernel style wins? What do others think? > > Linux kernel with 4-char indent? That would avoid the need to run > indent (indent requires knowledge of all the names of the types, so a > mechanical pass through the sources is more easily said than done). I strongly disagree with running indent against the source tree. Indentation is purely cosmetic and honestly is something people get too upset about. Doing something like a flag day indent run would result in git annotate becoming more or less useless. I prefer having tools that we can use to debug real code instead of worry about indentation. Regards, Anthony Liguori
Blue Swirl wrote: > On Wed, Sep 30, 2009 at 8:00 PM, Avi Kivity <avi@redhat.com> wrote: > >> On 09/30/2009 03:51 PM, Michael S. Tsirkin wrote: >> >>> So ... Linux kernel style wins? What do others think? >>> >>> >> Any change is going to cause a large amount of pain if implemented. >> > > True, but with 'indent' support the pain would be brief. > It's equivalent to losing a massive amount of revision history which hurts debugging. Regards, Anthony Liguori
Michael S. Tsirkin wrote: > On Wed, Sep 30, 2009 at 06:06:17PM +0200, Christoph Hellwig wrote: > >> On Tue, Sep 29, 2009 at 09:15:04PM +0300, Blue Swirl wrote: >> >>> We could also select a new style, like Linux kernel one. I'm also not >>> a great fan of that either. But at least 'indent' could be used very >>> easily and I think there would be other benefits, like reuse of Git >>> hooks, patch checking scripts and Emacs configs. With full 'indent' >>> support all sources could be massaged mechanically to make the switch >>> less painful. >>> >>> Then there are K&R and GNU styles, but I like them even less. >>> >> Everything but GNU obsfucation :) Seriously, something resembling Linux >> CodingStyle or style(9) in the various BSDs would probably help a lot >> of people avoiding to context switch too much. >> > > OK, thats 3 people in favor of switching to linux CodingStyle so far. > I don't think switching coding style is an option on the table. We're long, long past the point where something like that would make sense at all. Regards, Anthony Liguori
Anthony Liguori <anthony@codemonkey.ws> writes: > I strongly disagree with running indent against the source tree. > Indentation is purely cosmetic and honestly is something people get > too upset about. Doing something like a flag day indent run would > result in git annotate becoming more or less useless. FWIW, git-blame -w exists. [...]
Markus Armbruster wrote: > Anthony Liguori <anthony@codemonkey.ws> writes: > > >> I strongly disagree with running indent against the source tree. >> Indentation is purely cosmetic and honestly is something people get >> too upset about. Doing something like a flag day indent run would >> result in git annotate becoming more or less useless. >> > > FWIW, git-blame -w exists. > That's pretty useful. It's still a merge nightmare though. Anyone carrying patches or downstream is going to have a massive headache due to conflicts. Regards, Anthony Liguori > [...] >
On Wed, Sep 30, 2009 at 07:00:21PM +0200, Juan Quintela wrote: > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > .... > > >> Actually, I tried to tweak 'indent' to match QEMU style without much success. > >> > >> We could also select a new style, like Linux kernel one. I'm also not > >> a great fan of that either. But at least 'indent' could be used very > >> easily and I think there would be other benefits, like reuse of Git > >> hooks, patch checking scripts and Emacs configs. With full 'indent' > >> support all sources could be massaged mechanically to make the switch > >> less painful. > >> > >> Then there are K&R and GNU styles, but I like them even less. > > > > So ... Linux kernel style wins? What do others think? > > Where do I have to sign? Yeah. I really don't care very much. I've changed my CRIS and MicroBlaze code from style to style a couple of times but the only style regardless of style I find offensive is the: if (x) { x=1; } else { x=0; } TBH, the extra curley braceys are just pure nonsense. Remove those and I'm happy with tab or 2 or 4 or 8 or whatever amount of indentation spaces. Cheers
On Wed, Sep 30, 2009 at 08:32:32PM +0200, Paolo Bonzini wrote: > On 09/30/2009 07:48 PM, Michael S. Tsirkin wrote: >> On Wed, Sep 30, 2009 at 07:31:36PM +0200, Paolo Bonzini wrote: >>> (indent requires knowledge of all the names of the types, so a >>> mechanical pass through the sources is more easily said than done). >> >> How do you tell it the names of the types? > > You use the `-T' option. `-T' can be specified more than once, and all > names specified are used. For example, if your program contains > > typedef unsigned long CODE_ADDR; > typedef enum {red, blue, green} COLOR; > > you would use the options `-T CODE_ADDR -T COLOR'. > > Actually, you only need that for types that are used in arguments and > declared in .h files (as opposed to the same .c file). Example: > > typedef struct color color; > void f(color* p) > > works, but if you remove the typedef you get > > void f (color * p) > > Paolo I see. Well, as we switch to linux CodingStyle we'll cut the number of typedefs dramatically, making this less of an issue :)
On Wed, Sep 30, 2009 at 04:01:00PM -0500, Anthony Liguori wrote: > Blue Swirl wrote: >> On Wed, Sep 30, 2009 at 8:00 PM, Avi Kivity <avi@redhat.com> wrote: >> >>> On 09/30/2009 03:51 PM, Michael S. Tsirkin wrote: >>> >>>> So ... Linux kernel style wins? What do others think? >>>> >>>> >>> Any change is going to cause a large amount of pain if implemented. >>> >> >> True, but with 'indent' support the pain would be brief. >> > > It's equivalent to losing a massive amount of revision history which > hurts debugging. It is? Why is it? git bisect still works. what else do you use for debugging that gets broken? > Regards, > > Anthony Liguori >
On 09/30/2009 07:29 PM, Blue Swirl wrote: > On Wed, Sep 30, 2009 at 8:00 PM, Avi Kivity<avi@redhat.com> wrote: > >> On 09/30/2009 03:51 PM, Michael S. Tsirkin wrote: >> >>> So ... Linux kernel style wins? What do others think? >>> >>> >> >> Any change is going to cause a large amount of pain if implemented. >> > True, but with 'indent' support the pain would be brief. > External trees (qemu-kvm, the Xen tree, distros which carry patches) will all have to adapt, just so that we can reach 80 columns twice as quickly.
On (Wed) Sep 30 2009 [19:02:09], Markus Armbruster wrote: > > So ... Linux kernel style wins? What do others think? > > I find certain aspects of QEMU style rather distasteful, too. Linux > kernel style is okay. It can push deeply indented code far to the right > and over the cliff, though. The point it is that way is that if your code is indented so much that it overflows, things are better rearranged in separate functions. There are many such examples in qemu where splitting things off in separate functions would make the code a lot more readable (and maintainable). Amit
On Thu, Oct 01, 2009 at 03:25:48AM +0200, Edgar E. Iglesias wrote: > On Wed, Sep 30, 2009 at 07:00:21PM +0200, Juan Quintela wrote: > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > .... > > > > >> Actually, I tried to tweak 'indent' to match QEMU style without much success. > > >> > > >> We could also select a new style, like Linux kernel one. I'm also not > > >> a great fan of that either. But at least 'indent' could be used very > > >> easily and I think there would be other benefits, like reuse of Git > > >> hooks, patch checking scripts and Emacs configs. With full 'indent' > > >> support all sources could be massaged mechanically to make the switch > > >> less painful. > > >> > > >> Then there are K&R and GNU styles, but I like them even less. > > > > > > So ... Linux kernel style wins? What do others think? > > > > Where do I have to sign? > > Yeah. > > I really don't care very much. I've changed my CRIS and MicroBlaze code from > style to style a couple of times but the only style regardless of style I find > offensive is the: > > if (x) { > x=1; > } else { > x=0; > } > > TBH, the extra curley braceys are just pure nonsense. > > Remove those and I'm happy with tab or 2 or 4 or 8 or whatever amount of > indentation spaces. > > Cheers The advantage of following linux kernel is you get automatic code checking/correction tools. Most importantly scripts/checkpatch.pl Deviate from it and you have to maintain your own.
On Thu, Oct 01, 2009 at 08:31:16AM +0200, Avi Kivity wrote: > On 09/30/2009 07:29 PM, Blue Swirl wrote: >> On Wed, Sep 30, 2009 at 8:00 PM, Avi Kivity<avi@redhat.com> wrote: >> >>> On 09/30/2009 03:51 PM, Michael S. Tsirkin wrote: >>> >>>> So ... Linux kernel style wins? What do others think? >>>> >>>> >>> >>> Any change is going to cause a large amount of pain if implemented. >>> >> True, but with 'indent' support the pain would be brief. >> > > External trees (qemu-kvm, the Xen tree, distros which carry patches) > will all have to adapt, just so that we can reach 80 columns twice as > quickly. Can external trees just run indent themselves before merging upstream? > -- > Do not meddle in the internals of kernels, for they are subtle and quick to panic.
>>> True, but with 'indent' support the pain would be brief. >>> >> >> External trees (qemu-kvm, the Xen tree, distros which carry patches) >> will all have to adapt, just so that we can reach 80 columns twice as >> quickly. > > Can external trees just run indent themselves before merging upstream? That does not help merging. You have "git blame -w" but not "git merge -w". You can run a huge "git merge -s ours" (or add a jumbo patch to the distribution), but you still have to make sure you do not introduce spacing differences WRT upstream, or _future_ conflicts will be even worse. Personally I believe it is not worth it. I think that incremental updates to the coding style are enough, and not too bad if we go on with 4-space soft tabs; whipping up a version of checkpatch that supports 4-space indent should not be hard. Which means, you do not need to do much besides deciding how you want the braces. Paolo
Am 01.10.2009 08:17, schrieb Michael S. Tsirkin: > On Wed, Sep 30, 2009 at 04:01:00PM -0500, Anthony Liguori wrote: >> Blue Swirl wrote: >>> On Wed, Sep 30, 2009 at 8:00 PM, Avi Kivity <avi@redhat.com> wrote: >>> >>>> On 09/30/2009 03:51 PM, Michael S. Tsirkin wrote: >>>> >>>>> So ... Linux kernel style wins? What do others think? >>>>> >>>>> >>>> Any change is going to cause a large amount of pain if implemented. >>>> >>> >>> True, but with 'indent' support the pain would be brief. >>> >> >> It's equivalent to losing a massive amount of revision history which >> hurts debugging. > > It is? Why is it? git bisect still works. what else do you use > for debugging that gets broken? git blame. Kevin
Am 01.10.2009 03:25, schrieb Edgar E. Iglesias: > On Wed, Sep 30, 2009 at 07:00:21PM +0200, Juan Quintela wrote: >> "Michael S. Tsirkin" <mst@redhat.com> wrote: >> >> .... >> >>>> Actually, I tried to tweak 'indent' to match QEMU style without much success. >>>> >>>> We could also select a new style, like Linux kernel one. I'm also not >>>> a great fan of that either. But at least 'indent' could be used very >>>> easily and I think there would be other benefits, like reuse of Git >>>> hooks, patch checking scripts and Emacs configs. With full 'indent' >>>> support all sources could be massaged mechanically to make the switch >>>> less painful. >>>> >>>> Then there are K&R and GNU styles, but I like them even less. >>> >>> So ... Linux kernel style wins? What do others think? >> >> Where do I have to sign? > > Yeah. > > I really don't care very much. I've changed my CRIS and MicroBlaze code from > style to style a couple of times but the only style regardless of style I find > offensive is the: > > if (x) { > x=1; > } else { > x=0; > } > > TBH, the extra curley braceys are just pure nonsense. Except that when adding another line after x=1; the patch doesn't contain a change to the if line (with a possibly long condition and the possibility of introducing typos that nobody will see because everybody assumes it's just the new brace). But then, you could say the ugliness of braces is more important and I really don't care too much about the braces. However, I don't think we should change the coding style radically like switching to the Linux kernel style. Kevin
On Thu, Oct 01, 2009 at 10:43:16AM +0200, Kevin Wolf wrote: > Am 01.10.2009 08:17, schrieb Michael S. Tsirkin: > > On Wed, Sep 30, 2009 at 04:01:00PM -0500, Anthony Liguori wrote: > >> Blue Swirl wrote: > >>> On Wed, Sep 30, 2009 at 8:00 PM, Avi Kivity <avi@redhat.com> wrote: > >>> > >>>> On 09/30/2009 03:51 PM, Michael S. Tsirkin wrote: > >>>> > >>>>> So ... Linux kernel style wins? What do others think? > >>>>> > >>>>> > >>>> Any change is going to cause a large amount of pain if implemented. > >>>> > >>> > >>> True, but with 'indent' support the pain would be brief. > >>> > >> > >> It's equivalent to losing a massive amount of revision history which > >> hurts debugging. > > > > It is? Why is it? git bisect still works. what else do you use > > for debugging that gets broken? > > git blame. > > Kevin I think git blame has "-w" flag to ignore whitespace changes.
On Thu, Oct 01, 2009 at 08:41:29AM +0200, Michael S. Tsirkin wrote: > On Thu, Oct 01, 2009 at 03:25:48AM +0200, Edgar E. Iglesias wrote: > > On Wed, Sep 30, 2009 at 07:00:21PM +0200, Juan Quintela wrote: > > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > > .... > > > > > > >> Actually, I tried to tweak 'indent' to match QEMU style without much success. > > > >> > > > >> We could also select a new style, like Linux kernel one. I'm also not > > > >> a great fan of that either. But at least 'indent' could be used very > > > >> easily and I think there would be other benefits, like reuse of Git > > > >> hooks, patch checking scripts and Emacs configs. With full 'indent' > > > >> support all sources could be massaged mechanically to make the switch > > > >> less painful. > > > >> > > > >> Then there are K&R and GNU styles, but I like them even less. > > > > > > > > So ... Linux kernel style wins? What do others think? > > > > > > Where do I have to sign? > > > > Yeah. > > > > I really don't care very much. I've changed my CRIS and MicroBlaze code from > > style to style a couple of times but the only style regardless of style I find > > offensive is the: > > > > if (x) { > > x=1; > > } else { > > x=0; > > } > > > > TBH, the extra curley braceys are just pure nonsense. > > > > Remove those and I'm happy with tab or 2 or 4 or 8 or whatever amount of > > indentation spaces. > > > > Cheers > > The advantage of following linux kernel is you get automatic > code checking/correction tools. > Most importantly scripts/checkpatch.pl > > Deviate from it and you have to maintain your own. Add flag to checkpatch.pl to use different indentation an submit it upstream. -- Gleb.
On Thu, Oct 01, 2009 at 03:25:48AM +0200, Edgar E. Iglesias wrote: > On Wed, Sep 30, 2009 at 07:00:21PM +0200, Juan Quintela wrote: > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > .... > > > > >> Actually, I tried to tweak 'indent' to match QEMU style without much success. > > >> > > >> We could also select a new style, like Linux kernel one. I'm also not > > >> a great fan of that either. But at least 'indent' could be used very > > >> easily and I think there would be other benefits, like reuse of Git > > >> hooks, patch checking scripts and Emacs configs. With full 'indent' > > >> support all sources could be massaged mechanically to make the switch > > >> less painful. > > >> > > >> Then there are K&R and GNU styles, but I like them even less. > > > > > > So ... Linux kernel style wins? What do others think? > > > > Where do I have to sign? > > Yeah. > > I really don't care very much. I've changed my CRIS and MicroBlaze code from > style to style a couple of times but the only style regardless of style I find > offensive is the: > > if (x) { > x=1; > } else { > x=0; > } > > TBH, the extra curley braceys are just pure nonsense. > > > Remove those and I'm happy with tab or 2 or 4 or 8 or whatever amount of > indentation spaces. This is me to thread, so: +1 -- Gleb.
On Thu, Oct 01, 2009 at 10:56:15AM +0200, Kevin Wolf wrote: > Am 01.10.2009 03:25, schrieb Edgar E. Iglesias: > > On Wed, Sep 30, 2009 at 07:00:21PM +0200, Juan Quintela wrote: > >> "Michael S. Tsirkin" <mst@redhat.com> wrote: > >> > >> .... > >> > >>>> Actually, I tried to tweak 'indent' to match QEMU style without much success. > >>>> > >>>> We could also select a new style, like Linux kernel one. I'm also not > >>>> a great fan of that either. But at least 'indent' could be used very > >>>> easily and I think there would be other benefits, like reuse of Git > >>>> hooks, patch checking scripts and Emacs configs. With full 'indent' > >>>> support all sources could be massaged mechanically to make the switch > >>>> less painful. > >>>> > >>>> Then there are K&R and GNU styles, but I like them even less. > >>> > >>> So ... Linux kernel style wins? What do others think? > >> > >> Where do I have to sign? > > > > Yeah. > > > > I really don't care very much. I've changed my CRIS and MicroBlaze code from > > style to style a couple of times but the only style regardless of style I find > > offensive is the: > > > > if (x) { > > x=1; > > } else { > > x=0; > > } > > > > TBH, the extra curley braceys are just pure nonsense. > > Except that when adding another line after x=1; the patch doesn't > contain a change to the if line (with a possibly long condition and the > possibility of introducing typos that nobody will see because everybody > assumes it's just the new brace). I don't think I saw a bug introduced this way in ages. The absense of braces is easy enough to spot. > But then, you could say the ugliness of braces is more important and I > really don't care too much about the braces. However, I don't think we > should change the coding style radically like switching to the Linux > kernel style. > > Kevin I think this is the only way to get a consistent style, for 3 reasons: - a lot of people hack at both projects, they just keep mixing styles - linux has automatic checkers that are maintained and improved over time - linux style is just better ;)
Am 01.10.2009 10:58, schrieb Michael S. Tsirkin: > On Thu, Oct 01, 2009 at 10:43:16AM +0200, Kevin Wolf wrote: >> Am 01.10.2009 08:17, schrieb Michael S. Tsirkin: >>> On Wed, Sep 30, 2009 at 04:01:00PM -0500, Anthony Liguori wrote: >>>> Blue Swirl wrote: >>>>> On Wed, Sep 30, 2009 at 8:00 PM, Avi Kivity <avi@redhat.com> wrote: >>>>> >>>>>> On 09/30/2009 03:51 PM, Michael S. Tsirkin wrote: >>>>>> >>>>>>> So ... Linux kernel style wins? What do others think? >>>>>>> >>>>>>> >>>>>> Any change is going to cause a large amount of pain if implemented. >>>>>> >>>>> >>>>> True, but with 'indent' support the pain would be brief. >>>>> >>>> >>>> It's equivalent to losing a massive amount of revision history which >>>> hurts debugging. >>> >>> It is? Why is it? git bisect still works. what else do you use >>> for debugging that gets broken? >> >> git blame. >> >> Kevin > > I think git blame has "-w" flag to ignore whitespace changes. Are we only talking about changing the indentation? Switching from 4 spaces to tabs of 8 characters also means additional line breaks and so on (or does it handle these, too?). And I thought I read in this thread about braces, typedefs and probably some more things. Can -w really handle all of them? Kevin
On Thu, Oct 01, 2009 at 11:10:12AM +0200, Kevin Wolf wrote: > Am 01.10.2009 10:58, schrieb Michael S. Tsirkin: > > On Thu, Oct 01, 2009 at 10:43:16AM +0200, Kevin Wolf wrote: > >> Am 01.10.2009 08:17, schrieb Michael S. Tsirkin: > >>> On Wed, Sep 30, 2009 at 04:01:00PM -0500, Anthony Liguori wrote: > >>>> Blue Swirl wrote: > >>>>> On Wed, Sep 30, 2009 at 8:00 PM, Avi Kivity <avi@redhat.com> wrote: > >>>>> > >>>>>> On 09/30/2009 03:51 PM, Michael S. Tsirkin wrote: > >>>>>> > >>>>>>> So ... Linux kernel style wins? What do others think? > >>>>>>> > >>>>>>> > >>>>>> Any change is going to cause a large amount of pain if implemented. > >>>>>> > >>>>> > >>>>> True, but with 'indent' support the pain would be brief. > >>>>> > >>>> > >>>> It's equivalent to losing a massive amount of revision history which > >>>> hurts debugging. > >>> > >>> It is? Why is it? git bisect still works. what else do you use > >>> for debugging that gets broken? > >> > >> git blame. > >> > >> Kevin > > > > I think git blame has "-w" flag to ignore whitespace changes. > > Are we only talking about changing the indentation? Switching from 4 > spaces to tabs of 8 characters also means additional line breaks and so > on (or does it handle these, too?). And I thought I read in this thread > about braces, typedefs and probably some more things. Can -w really > handle all of them? > > Kevin No. But if a line is very long, any code refactoring will create the same effect. Most lines arent that long.
On Thu, Oct 01, 2009 at 11:02:59AM +0200, Michael S. Tsirkin wrote: > On Thu, Oct 01, 2009 at 10:56:15AM +0200, Kevin Wolf wrote: > > Am 01.10.2009 03:25, schrieb Edgar E. Iglesias: > > > On Wed, Sep 30, 2009 at 07:00:21PM +0200, Juan Quintela wrote: > > >> "Michael S. Tsirkin" <mst@redhat.com> wrote: > > >> > > >> .... > > >> > > >>>> Actually, I tried to tweak 'indent' to match QEMU style without much success. > > >>>> > > >>>> We could also select a new style, like Linux kernel one. I'm also not > > >>>> a great fan of that either. But at least 'indent' could be used very > > >>>> easily and I think there would be other benefits, like reuse of Git > > >>>> hooks, patch checking scripts and Emacs configs. With full 'indent' > > >>>> support all sources could be massaged mechanically to make the switch > > >>>> less painful. > > >>>> > > >>>> Then there are K&R and GNU styles, but I like them even less. > > >>> > > >>> So ... Linux kernel style wins? What do others think? > > >> > > >> Where do I have to sign? > > > > > > Yeah. > > > > > > I really don't care very much. I've changed my CRIS and MicroBlaze code from > > > style to style a couple of times but the only style regardless of style I find > > > offensive is the: > > > > > > if (x) { > > > x=1; > > > } else { > > > x=0; > > > } > > > > > > TBH, the extra curley braceys are just pure nonsense. > > > > Except that when adding another line after x=1; the patch doesn't > > contain a change to the if line (with a possibly long condition and the > > possibility of introducing typos that nobody will see because everybody > > assumes it's just the new brace). > > I don't think I saw a bug introduced this way in ages. > The absense of braces is easy enough to spot. > With modern editors that indent automatically it's impossible not to spot. > > But then, you could say the ugliness of braces is more important and I > > really don't care too much about the braces. However, I don't think we > > should change the coding style radically like switching to the Linux > > kernel style. > > > > Kevin > > I think this is the only way to get a consistent style, for 3 reasons: > - a lot of people hack at both projects, they just keep mixing styles > - linux has automatic checkers that are maintained and improved over time > - linux style is just better ;) > Real men's editor (TM) is able to switch style depending on what you edit. -- Gleb.
On Thu, Oct 01, 2009 at 11:46:16AM +0200, Gleb Natapov wrote: > On Thu, Oct 01, 2009 at 11:02:59AM +0200, Michael S. Tsirkin wrote: > > On Thu, Oct 01, 2009 at 10:56:15AM +0200, Kevin Wolf wrote: > > > Am 01.10.2009 03:25, schrieb Edgar E. Iglesias: > > > > On Wed, Sep 30, 2009 at 07:00:21PM +0200, Juan Quintela wrote: > > > >> "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > >> > > > >> .... > > > >> > > > >>>> Actually, I tried to tweak 'indent' to match QEMU style without much success. > > > >>>> > > > >>>> We could also select a new style, like Linux kernel one. I'm also not > > > >>>> a great fan of that either. But at least 'indent' could be used very > > > >>>> easily and I think there would be other benefits, like reuse of Git > > > >>>> hooks, patch checking scripts and Emacs configs. With full 'indent' > > > >>>> support all sources could be massaged mechanically to make the switch > > > >>>> less painful. > > > >>>> > > > >>>> Then there are K&R and GNU styles, but I like them even less. > > > >>> > > > >>> So ... Linux kernel style wins? What do others think? > > > >> > > > >> Where do I have to sign? > > > > > > > > Yeah. > > > > > > > > I really don't care very much. I've changed my CRIS and MicroBlaze code from > > > > style to style a couple of times but the only style regardless of style I find > > > > offensive is the: > > > > > > > > if (x) { > > > > x=1; > > > > } else { > > > > x=0; > > > > } > > > > > > > > TBH, the extra curley braceys are just pure nonsense. > > > > > > Except that when adding another line after x=1; the patch doesn't > > > contain a change to the if line (with a possibly long condition and the > > > possibility of introducing typos that nobody will see because everybody > > > assumes it's just the new brace). > > > > I don't think I saw a bug introduced this way in ages. > > The absense of braces is easy enough to spot. > > > With modern editors that indent automatically it's impossible not to > spot. > > > > But then, you could say the ugliness of braces is more important and I > > > really don't care too much about the braces. However, I don't think we > > > should change the coding style radically like switching to the Linux > > > kernel style. > > > > > > Kevin > > > > I think this is the only way to get a consistent style, for 3 reasons: > > - a lot of people hack at both projects, they just keep mixing styles > > - linux has automatic checkers that are maintained and improved over time > > - linux style is just better ;) > > > Real men's editor (TM) is able to switch style depending on what you > edit. > -- > Gleb.
diff --git a/Makefile.target b/Makefile.target index 0ebef17..c0f143a 100644 --- a/Makefile.target +++ b/Makefile.target @@ -55,6 +55,9 @@ libobj-$(CONFIG_S390_DIS) += s390-dis.o libobj-$(CONFIG_SH4_DIS) += sh4-dis.o libobj-$(CONFIG_SPARC_DIS) += sparc-dis.o +# Target library: makes devices target-independent +libobj-y += target.o + # libqemu libqemu.a: $(libobj-y) diff --git a/hw/msix.c b/hw/msix.c index 3782994..af820ec 100644 --- a/hw/msix.c +++ b/hw/msix.c @@ -14,6 +14,7 @@ #include "hw.h" #include "msix.h" #include "pci.h" +#include "target.h" /* Declaration from linux/pci_regs.h */ #define PCI_CAP_ID_MSIX 0x11 /* MSI-X */ @@ -38,6 +39,15 @@ #define MSIX_VECTOR_CTRL 12 #define MSIX_ENTRY_SIZE 16 #define MSIX_VECTOR_MASK 0x1 + +/* How much space does an MSIX table need. */ +/* The spec requires giving the table structure + * a 4K aligned region all by itself. Align it to + * target pages so that drivers can do passthrough + * on the rest of the region. */ +#define MSIX_PAGE_SIZE target_page_align(0x1000) +/* Reserve second half of the page for pending bits */ +#define MSIX_PAGE_PENDING (MSIX_PAGE_SIZE / 2) #define MSIX_MAX_ENTRIES 32 @@ -53,12 +63,6 @@ /* Flag for interrupt controller to declare MSI-X support */ int msix_supported; -/* Reserve second half of the page for pending bits */ -static int msix_page_pending(PCIDevice *d) -{ - return (d->msix_page_size / 2); -} - /* Add MSI-X capability to the config space for the device. */ /* Given a bar and its size, add MSI-X table on top of it * and fill MSI-X capability in the config space. @@ -77,11 +81,11 @@ static int msix_add_config(struct PCIDevice *pdev, unsigned short nentries, return -ENOSPC; /* Add space for MSI-X structures */ - if (!bar_size) { - new_size = pdev->msix_page_size; - } else if (bar_size < pdev->msix_page_size) { - bar_size = pdev->msix_page_size; - new_size = pdev->msix_page_size * 2; + if (!bar_size) + new_size = MSIX_PAGE_SIZE; + else if (bar_size < MSIX_PAGE_SIZE) { + bar_size = MSIX_PAGE_SIZE; + new_size = MSIX_PAGE_SIZE * 2; } else new_size = bar_size * 2; @@ -95,8 +99,8 @@ static int msix_add_config(struct PCIDevice *pdev, unsigned short nentries, /* Table on top of BAR */ pci_set_long(config + MSIX_TABLE_OFFSET, bar_size | bar_nr); /* Pending bits on top of that */ - pci_set_long(config + MSIX_PBA_OFFSET, (bar_size + msix_page_pending(pdev)) - | bar_nr); + pci_set_long(config + MSIX_PBA_OFFSET, (bar_size + MSIX_PAGE_PENDING) | + bar_nr); pdev->msix_cap = config_offset; /* Make flags bit writeable. */ pdev->wmask[config_offset + MSIX_ENABLE_OFFSET] |= MSIX_ENABLE_MASK; @@ -126,7 +130,7 @@ void msix_write_config(PCIDevice *dev, uint32_t addr, static uint32_t msix_mmio_readl(void *opaque, target_phys_addr_t addr) { PCIDevice *dev = opaque; - unsigned int offset = addr & (dev->msix_page_size - 1); + unsigned int offset = addr & (MSIX_PAGE_SIZE - 1); void *page = dev->msix_table_page; uint32_t val = 0; @@ -148,7 +152,7 @@ static uint8_t msix_pending_mask(int vector) static uint8_t *msix_pending_byte(PCIDevice *dev, int vector) { - return dev->msix_table_page + msix_page_pending(dev) + vector / 8; + return dev->msix_table_page + MSIX_PAGE_PENDING + vector / 8; } static int msix_is_pending(PCIDevice *dev, int vector) @@ -176,7 +180,7 @@ static void msix_mmio_writel(void *opaque, target_phys_addr_t addr, uint32_t val) { PCIDevice *dev = opaque; - unsigned int offset = addr & (dev->msix_page_size - 1); + unsigned int offset = addr & (MSIX_PAGE_SIZE - 1); int vector = offset / MSIX_ENTRY_SIZE; memcpy(dev->msix_table_page + offset, &val, 4); if (!msix_is_masked(dev, vector) && msix_is_pending(dev, vector)) { @@ -205,7 +209,7 @@ void msix_mmio_map(PCIDevice *d, int region_num, { uint8_t *config = d->config + d->msix_cap; uint32_t table = pci_get_long(config + MSIX_TABLE_OFFSET); - uint32_t offset = table & ~(d->msix_page_size - 1); + uint32_t offset = table & ~(MSIX_PAGE_SIZE - 1); /* TODO: for assigned devices, we'll want to make it possible to map * pending bits separately in case they are in a separate bar. */ int table_bir = table & PCI_MSIX_FLAGS_BIRMASK; @@ -221,7 +225,7 @@ void msix_mmio_map(PCIDevice *d, int region_num, /* Initialize the MSI-X structures. Note: if MSI-X is supported, BAR size is * modified, it should be retrieved with msix_bar_size. */ int msix_init(struct PCIDevice *dev, unsigned short nentries, - unsigned bar_nr, unsigned bar_size, target_phys_addr_t page_size) + unsigned bar_nr, unsigned bar_size) { int ret; /* Nothing to do if MSI is not supported by interrupt controller */ @@ -234,8 +238,7 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries, dev->msix_entry_used = qemu_mallocz(MSIX_MAX_ENTRIES * sizeof *dev->msix_entry_used); - dev->msix_page_size = page_size; - dev->msix_table_page = qemu_mallocz(dev->msix_page_size); + dev->msix_table_page = qemu_mallocz(MSIX_PAGE_SIZE); dev->msix_mmio_index = cpu_register_io_memory(msix_mmio_read, msix_mmio_write, dev); @@ -290,8 +293,7 @@ void msix_save(PCIDevice *dev, QEMUFile *f) } qemu_put_buffer(f, dev->msix_table_page, n * MSIX_ENTRY_SIZE); - qemu_put_buffer(f, dev->msix_table_page + msix_page_pending(dev), - (n + 7) / 8); + qemu_put_buffer(f, dev->msix_table_page + MSIX_PAGE_PENDING, (n + 7) / 8); } /* Should be called after restoring the config space. */ @@ -305,8 +307,7 @@ void msix_load(PCIDevice *dev, QEMUFile *f) msix_free_irq_entries(dev); qemu_get_buffer(f, dev->msix_table_page, n * MSIX_ENTRY_SIZE); - qemu_get_buffer(f, dev->msix_table_page + msix_page_pending(dev), - (n + 7) / 8); + qemu_get_buffer(f, dev->msix_table_page + MSIX_PAGE_PENDING, (n + 7) / 8); } /* Does device support MSI-X? */ @@ -356,7 +357,7 @@ void msix_reset(PCIDevice *dev) return; msix_free_irq_entries(dev); dev->config[dev->msix_cap + MSIX_ENABLE_OFFSET] &= MSIX_ENABLE_MASK; - memset(dev->msix_table_page, 0, dev->msix_page_size); + memset(dev->msix_table_page, 0, MSIX_PAGE_SIZE); } /* PCI spec suggests that devices make it possible for software to configure diff --git a/hw/msix.h b/hw/msix.h index 9367ba3..3427778 100644 --- a/hw/msix.h +++ b/hw/msix.h @@ -3,9 +3,8 @@ #include "qemu-common.h" -int msix_init(struct PCIDevice *dev, unsigned short nentries, - unsigned bar_nr, unsigned bar_size, - target_phys_addr_t page_size); +int msix_init(PCIDevice *pdev, unsigned short nentries, + unsigned bar_nr, unsigned bar_size); void msix_write_config(PCIDevice *pci_dev, uint32_t address, uint32_t val, int len); diff --git a/hw/pci.h b/hw/pci.h index dddd599..8cfc38d 100644 --- a/hw/pci.h +++ b/hw/pci.h @@ -214,12 +214,6 @@ struct PCIDevice { uint32_t msix_bar_size; /* Version id needed for VMState */ int32_t version_id; - /* How much space does an MSIX table need. */ - /* The spec requires giving the table structure - * a 4K aligned region all by itself. Align it to - * target pages so that drivers can do passthrough - * on the rest of the region. */ - target_phys_addr_t msix_page_size; }; PCIDevice *pci_register_device(PCIBus *bus, const char *name, diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c index 1f14c5e..3a0231e 100644 --- a/hw/virtio-pci.c +++ b/hw/virtio-pci.c @@ -410,8 +410,7 @@ static void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev, config[0x3d] = 1; - if (vdev->nvectors && !msix_init(&proxy->pci_dev, vdev->nvectors, 1, 0, - TARGET_PAGE_SIZE)) { + if (vdev->nvectors && !msix_init(&proxy->pci_dev, vdev->nvectors, 1, 0)) { pci_register_bar(&proxy->pci_dev, 1, msix_bar_size(&proxy->pci_dev), PCI_ADDRESS_SPACE_MEM, diff --git a/target.c b/target.c new file mode 100644 index 0000000..3287eea --- /dev/null +++ b/target.c @@ -0,0 +1,17 @@ +/* + * Target definitions for use by devices. + * + * Author: Michael S. Tsirkin <mst@redhat.com> + * + * Copyright (c) 2009, Red Hat Inc, Michael S. Tsirkin (mst@redhat.com) + * + * This work is licensed under the terms of the GNU GPL, version 2. See + * the COPYING file in the top-level directory. + */ +#include "qemu-common.h" +#include "target.h" + +unsigned target_page_align(unsigned value) +{ + return TARGET_PAGE_ALIGN(value); +} diff --git a/target.h b/target.h new file mode 100644 index 0000000..0e01e47 --- /dev/null +++ b/target.h @@ -0,0 +1,6 @@ +#ifndef QEMU_TARGET_H +#define QEMU_TARGET_H + +unsigned target_page_align(unsigned value); + +#endif
This creates target.c, which builds per-target, and makes it possible for devices to become target-independent. Use it in msix, reverting part of 5e520a7d500ec2569d22d80f9ef4272a34cb3c80, as we no longer have to pass target page around. Signed-off-by: Michael S. Tsirkin <mst@redhat.com> --- Makefile.target | 3 +++ hw/msix.c | 51 ++++++++++++++++++++++++++------------------------- hw/msix.h | 5 ++--- hw/pci.h | 6 ------ hw/virtio-pci.c | 3 +-- target.c | 17 +++++++++++++++++ target.h | 6 ++++++ 7 files changed, 55 insertions(+), 36 deletions(-) create mode 100644 target.c create mode 100644 target.h