Re: [Qemu-devel] [PATCH 1/2] apb_pci: convert PCI space to memory API

[Avi Kivity]

On 09/04/2011 12:18 AM, Blue Swirl wrote:

Add a new memory space for PCI instead of using system memory.

This also fixes a bug where VGA region vga.chain4 is
accidentally mapped to 0xa instead of 0x1ff000a.




Looks good.  I assume you'll commit this directly, since it fixes a 
regression?


--
error compiling committee.c: too many arguments to function

[Qemu-devel] Unsubscription Confirmation

[RealEstateMalaysian.com]

Thank you for subscribing. You have now unsubscribed and no more messages will 
be sent.

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Avi Kivity]

On 09/02/2011 05:56 AM, Wen Congyang wrote:

>
>  You could use something like kvm-unit-tests.git to write a simple test
>  that sets up a BAR (say from hw/ivshmem.c), writes and reads to see that
>  it is visible, programs the bridge to filter part of the BAR out, then
>  writes and reads again to verify that the correct part is filtered out.

I am testing ivshmem now. But I do not know how to access the memory
specified in the BAR.




Use the uio driver - 
http://docs.blackfin.uclinux.org/kernel/generated/uio-howto/.  You just 
mmap() the BAR from userspace and play with it.


--
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [Spice-devel] [PATCH] server: don't call reds_stream_free from worker thread context

[Yonit Halperin]

On 09/02/2011 06:19 PM, Gerd Hoffmann wrote:

reds_stream_free() may call the channel_event callback which is not
supposed to be callsed from worker thread context.  This patch moves
the reds_stream_free call for the display channel from the worker to
the dispatcher to fix this issue.

[ Note: not tested yet, against 0.8 branch, sending out for review&
 comments nevertheless ]

Signed-off-by: Gerd Hoffmann
---
  server/red_dispatcher.c |5 +
  server/red_worker.c |3 +--
  2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/server/red_dispatcher.c b/server/red_dispatcher.c
index f74b13e..801a575 100644
--- a/server/red_dispatcher.c
+++ b/server/red_dispatcher.c
@@ -51,6 +51,7 @@ struct RedDispatcher {
  int y_res;
  int use_hardware_cursor;
  RedDispatcher *next;
+RedsStream *stream;
  RedWorkerMessage async_message;
  pthread_mutex_t  async_lock;
  QXLDevSurfaceCreate surface_create;
@@ -81,6 +82,7 @@ static void red_dispatcher_set_peer(Channel *channel, 
RedsStream *stream, int mi

  red_printf("");
  dispatcher = (RedDispatcher *)channel->data;
+dispatcher->stream = stream;
  RedWorkerMessage message = RED_WORKER_MESSAGE_DISPLAY_CONNECT;
  write_message(dispatcher->channel,&message);
  send_data(dispatcher->channel,&stream, sizeof(RedsStream *));
@@ -93,6 +95,9 @@ static void red_dispatcher_shutdown_peer(Channel *channel)
  red_printf("");
  RedWorkerMessage message = RED_WORKER_MESSAGE_DISPLAY_DISCONNECT;
  write_message(dispatcher->channel,&message);
+read_message(dispatcher->channel,&message);
+ASSERT(message == RED_WORKER_MESSAGE_READY);
+reds_stream_free(dispatcher->stream);


Hi,
RED_WORKER_MESSAGE_DISPLAY_DISCONNECT is not the only place that 
triggers red_disconnect_channel (and as a result, 
reds_stream_free(dispatcher->stream)). red_disconnect_channel is called 
also when there is an error upon receive/send and also when timeouts 
related to the client occur (e.g., in flush_display_commands).


We probably better make the dispatcher bi-directional, i.e., not only 
push messages to the worker, but also listen.


Cheers,
Yonit.


  }

  static void red_dispatcher_migrate(Channel *channel)
diff --git a/server/red_worker.c b/server/red_worker.c
index 5f07803..f77b0f2 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -8486,8 +8486,6 @@ static void red_disconnect_channel(RedChannel *channel)
  {
  channel_release_res(channel);
  red_pipe_clear(channel);
-reds_stream_free(channel->stream);
-channel->stream = NULL;
  channel->send_data.blocked = FALSE;
  channel->send_data.size = channel->send_data.pos = 0;
  spice_marshaller_reset(channel->send_data.marshaller);
@@ -10060,6 +10058,7 @@ static void handle_dev_input(EventListener *listener, 
uint32_t events)
  case RED_WORKER_MESSAGE_CURSOR_DISCONNECT:
  red_printf("cursor disconnect");
  red_disconnect_cursor((RedChannel *)worker->cursor_channel);
+write_ready = 1;
  break;
  case RED_WORKER_MESSAGE_CURSOR_MIGRATE:
  red_printf("cursor migrate");

Re: [Qemu-devel] [PATCH] virtio: Make memory barriers be memory barriers

[Michael S. Tsirkin]

On Sat, Sep 03, 2011 at 06:19:23PM +0200, Paolo Bonzini wrote:
> On 09/02/2011 05:45 PM, Michael S. Tsirkin wrote:
> >Well, can you describe an issue in virtio that lfence/sfence help solve
> >in terms of a memory model please?
> >Pls note that guest uses smp_ variants for barriers.
> 
> /* Make sure buffer is written before we update index. */
> wmb();
> 
> Without it, a guest could see a partially updated buffer, because
> the buffer and index writes are unlocked stores to different
> locations.

Sorry, I still don't understand. Yes, they are unlocked stores to different
locations. How does it follow that a guest could see a partially updated
buffer? Just to make sure - we are talking about x86 here, not ppc,
right?

> Even if the guest uses barriers, with ioeventfd it will only order
> the CPU that is running the guest, not the one that is running the
> iothread.  In fact I'm surprised that it works at all under x86 with
> ioeventfd.
> 
> Paolo

I can try to explain if I understand the problem you see.

-- 
MST

Re: [Qemu-devel] [PATCH] virtio: Make memory barriers be memory barriers

[Michael S. Tsirkin]

On Sun, Sep 04, 2011 at 12:46:35AM +1000, David Gibson wrote:
> On Fri, Sep 02, 2011 at 06:45:50PM +0300, Michael S. Tsirkin wrote:
> > On Thu, Sep 01, 2011 at 04:31:09PM -0400, Paolo Bonzini wrote:
> > > > > > Why not limit the change to ppc then?
> > > > >
> > > > > Because the bug is masked by the x86 memory model, but it is still
> > > > > there even there conceptually. It is not really true that x86 does
> > > > > not need memory barriers, though it doesn't in this case:
> > > > >
> > > > > http://bartoszmilewski.wordpress.com/2008/11/05/who-ordered-memory-fences-on-an-x86/
> > > > >
> > > > > Paolo
> > > > 
> > > > Right.
> > > > To summarize, on x86 we probably want wmb and rmb to be compiler
> > > > barrier only. Only mb might in theory need to be an mfence.
> > > 
> > > No, wmb needs to be sfence and rmb needs to be lfence.  GCC does
> > > not provide those, so they should become __sync_synchronize() too,
> > > or you should use inline assembly.
> > > 
> > > > But there might be reasons why that is not an issue either
> > > > if we look closely enough.
> > > 
> > > Since the ring buffers are not using locked instructions (no xchg
> > > or cmpxchg) the barriers simply must be there, even on x86.  Whether
> > > it works in practice is not interesting, only the formal model is
> > > interesting.
> > > 
> > > Paolo
> > 
> > Well, can you describe an issue in virtio that lfence/sfence help solve
> > in terms of a memory model please?
> > Pls note that guest uses smp_ variants for barriers.
> 
> Ok, so, I'm having a bit of trouble with the fact that I'm having to
> argue the case that things the protocol requiress to be memory
> barriers actually *be* memory barriers on all platforms.
> 
> I mean argue for a richer set of barriers, with per-arch minimal
> implementations instead of the large but portable hammer of
> sync_synchronize, if you will.

That's what I'm saying really. On x86 the richer set of barriers
need not insert code at all for both wmb and rmb macros. All we
might need is an 'optimization barrier'- e.g. linux does
 __asm__ __volatile__("": : :"memory")
ppc needs something like sync_synchronize there.


> But just leaving them out on x86!?
> Seriously, wtf?  Do you enjoy having software that works chiefly by
> accident?

I'm surprised at the controversy too. People seem to argue that
x86 cpu does not reorder stores and that we need an sfence between
stores to prevent the guest from seeing them out of order, at
the same time.


> -- 
> David Gibson  | I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au| minimalist, thank you.  NOT _the_ 
> _other_
>   | _way_ _around_!
> http://www.ozlabs.org/~dgibson

Re: [Qemu-devel] [PATCH] pc: Clean up PIC-to-APIC IRQ path

[Blue Swirl]

On Sat, Sep 3, 2011 at 9:41 PM, Anthony Liguori  wrote:
> On 09/03/2011 04:10 PM, Blue Swirl wrote:
>>
>> On Sat, Sep 3, 2011 at 8:07 PM, Anthony Liguori
>>  wrote:
>>>
>>> On 09/01/2011 12:58 AM, Avi Kivity wrote:

 On 08/31/2011 07:59 PM, Blue Swirl wrote:
>
>>
>> That makes it impossible to migrate level-triggered irq lines. Or at
>
> least,
>>
>> the receiver has to remember the state, instead of (or in addition
>
> to) the
>>
>> sender.
>
> Both ends probably need to remember the state. That should work
> without any multiphase restores and transient suppressors.

 State should always correspond to real hardware state - a flip flop or
 capacitor. Input is not state, it is input.

> It might be also possible to introduce stateful signal lines which
> save and restore their state, then the receiving end could check what
> is the current level. However, if you consider that the devices may be
> restored in random order, if the IRQ line device happens to be
> restored later, the receiver would still get wrong information. Adding
> priorities could solve this, but I think stateless IRQs are the only
> sane way.

 I agree that irqs should be stateless, since they don't have any memory
 associated.
>>>
>>> In Real Life, you can tie a single bit multiple registers together with
>>> boolean logic to form an output pin.  This is essentially computed state.
>>>  If we wanted to model a stateless pin, we would need to do something
>>> like:
>>>
>>> struct Serial {
>>>   uint8_t thr;
>>>   uint8_t lsr;
>>> };
>>>
>>> static bool serial_get_irq(Serial *s) {
>>>   return (s->thr&  THRE) | (s->lsr&  LSRE);
>>> }
>>>
>>> static void serial_write(Serial *s, uint64_t addr, uint8_t value)
>>> {
>>>   switch (addr) {
>>>   case THR:
>>>      bool old_irq = serial_get_irq(s);
>>>      s->thr = value;
>>>      if (!old_irq&&  serial_get_irq(s)) {
>>>          notify_edge_change(s);
>>>      }
>>>   ...
>>> }
>>>
>>> static void serial_init(Serial *s)
>>> {
>>>    register_pin(s, serial_get_irq);
>>> }
>>>
>>> Obviously, this is pretty sucky.  This is what we do today but we don't
>>> have
>>> a way to query irq value which is wrong.  You could fix that by adding
>>> the
>>> get function but that's not terribly fun.  A better way:
>>>
>>> struct Serial {
>>>    Pin irq;
>>>    uint8_t thr;
>>>    uint8_t lsr;
>>> };
>>>
>>> static void serial_update_irq(Serial *s)
>>> {
>>>   pin_set_level(&s->irq, (s->thr&  THRE) | (s->lsr&  LSRE));
>>> }
>>>
>>> static void serial_write(Serial *s) {
>>>   switch (addr) {
>>>   case THR:
>>>      s->thr = value;
>>>      serial_update_irq(s);
>>>   ...
>>> }
>>>
>>> This results in much nicer code.  The edge handling can be done in
>>> generic
>>> code which will make things more robust overall.
>>
>> I'm sorry but I don't see a huge difference, could you elaborate?
>
> The main difference is whether the Pin is capable of determine if there was
> a level change on its own.  It can only do this is if knows the current
> level which implies that its holding state.
>
>>
>> Maybe if the internal state of Pin is magically shared between the
>> endpoint devices (like typedef bool *Pin) and the devices somehow
>> still save Pin state as part of their own despite the duplication,
>
> I'm somewhat confused by what you mean here.

Pretty similar to what you propose below except the magical sharing.

> If you have two devices that have a connection, one has an output pin and
> one has an input pin.  This would look like this:
>
> struct Serial {
>   Pin irq; // output pin
> };
>
> struct PIIX3 {
>   Pin *in[16]; // input pins
> };
>
> As part of connecting devices, you'd basically do:
>
> PIIX3 piix3;
> Serial serial;
>
> piix3.in[4] = &serial.irq;
>
> serial.irq setting it pin level doesn't do anything to piix3.  piix3 has to
> explicitly read the pin state for its behavior to influence anything.
>
> Here's the flow with taking migration into account:
>
> 1) PIIX3 maintains some type of state, performs action (A) whenever in[3]
> changes its state based on an edge change notifier.
>
> 2) During migration, PIIX3 has its state saved as does Serial.  Pin is part
> of Serial so it also has its state saved.
>
> 3) During restore, PIIX3 has its state restored, as does Serial, and Pin.
>  Action (A) is not invoked because notifiers are not fired when a device is
> not realized.  Restore happens before a device is realized.
>
> So the scenario you're concerned about doesn't happen and it doesn't require
> anything funky.

OK, this should work.

>> this could work. Restoring Pins first and then devices is a sort of
>> priority scheme.
>
> There is no priority.  But devices have an explicit realize event and in
> general, shouldn't react to other devices until realize happens.  You need
> this behavior to support construction properly.
>
> Regards,
>
> Anthony Liguori
>

Re: [Qemu-devel] [PATCH 1/2] apb_pci: convert PCI space to memory API

[Blue Swirl]

On Sun, Sep 4, 2011 at 8:10 AM, Avi Kivity  wrote:
> On 09/04/2011 12:18 AM, Blue Swirl wrote:
>>
>> Add a new memory space for PCI instead of using system memory.
>>
>> This also fixes a bug where VGA region vga.chain4 is
>> accidentally mapped to 0xa instead of 0x1ff000a.
>>
>>
>
> Looks good.  I assume you'll commit this directly, since it fixes a
> regression?

OK, pushed.

Re: [Qemu-devel] [PATCH] pc: Clean up PIC-to-APIC IRQ path

[Blue Swirl]

On Wed, Aug 31, 2011 at 7:44 PM, Blue Swirl  wrote:
> On Wed, Aug 31, 2011 at 6:17 PM, Jan Kiszka  wrote:
>> On 2011-08-31 19:41, Blue Swirl wrote:
>>> On Wed, Aug 31, 2011 at 10:53 AM, Jan Kiszka  wrote:
 On 2011-08-31 10:25, Peter Maydell wrote:
> On 30 August 2011 20:28, Jan Kiszka  wrote:
>> Yes, that's the current state. Once we have bidirectional IRQ links in
>> place (pushing downward, querying upward - required to skip IRQ routers
>> for fast, lockless deliveries), that should change again.
>
> Can you elaborate a bit more on this? I don't think anybody has
> proposed links with their own internal state before in the qdev/qom
> discussions...

 That basic idea is to allow

 a) a discovery of the currently active IRQ path from source to sink
   (that would be possible via QOM just using forward links)
>>>
>>> Why, only for b)? This is not possible with real hardware.
>>>
 b) skip updating the states of IRQ routers in the common case, just
   signaling directly the sink from the source (to allow in-kernel IRQ
   delivery or to skip taking some device locks). Whenever some router
   is queried for its current IRQ line state, it would have to ask the
   preceding IRQ source for its state. So we need a backward link.
>>>
>>> I think this would need pretty heavy changes everywhere. At board
>>> level the full path needs to be identified and special versions of
>>> IRQs installed along the way. The routers would need to use callbacks
>>> to inform other parties about routing changes.
>>
>> It already works in practice (based on a hack and minus IRQ router state
>> updates) for x86 PCI device pass-through. At least I don't want this
>> upstream but instead a generic solution. The ability to skip IRQ routers
>> also in pure user space device model scenarios is a useful by-product.
>>
>>>
 We haven't thought about how this could be implemented in details yet
 though. Among other things, it heavily depends on the final QOM design.
>>>
>>> Perhaps a global IRQ manager could help. It would keep track of the
>>> whole IRQ matrix, what are input (x axis) and output (y axis) states
>>> and what each matrix node (router state) looks like (or able to
>>> compute) if asked. I don't think backward links would be needed with
>>> this approach.
>>
>> Well, the backward links would then be moved to that global IRQ manager.
>> It's just moving the data management, but if it turns out to allow a
>> cleaner device design, I would surely not vote against it. But that
>> manager must support lazy updates as well because we cannot call it from
>> kernel space for each and every event.
>
> The global IRQ switch matrix would take over all routing for the
> devices in question. As an example, let's consider a PCI card
> (source), PCI host bridge, IO-APIC, LAPIC and CPU (final destination).
> The matrix would get input from the PCI card and output directly to
> LAPIC since the CPU does not use a qemu_irq yet. These leaf devices
> actually need no changes, just the qemu_irq is routed elsewhere. The
> matrix would also have access to intermediate devices but the qemu_irq
> inputs of those would not be updated in the lazy mode and the outputs
> need not go anywhere.
>
> So for a matrix where x0, x1...  is used for inputs and y0, y1... for
> outputs, x0 would be connected in the example to PCI card, x1 to PCI
> host bridge output and x2 to IO-APIC (ignoring multiple lines). Then
> y0 would be the calculated output from PCI host bridge, y1 IO-APIC and
> only y2 would be connected to LAPIC.
>
> If the intermediate IRQ state between the nodes is queried (for
> example at IO-APIC), the matrix must be able to tell what would be the
> true state at that point and then this value would be supplied to the
> device. So a method to query device IRQ input state from matrix on
> demand is needed. This could be as unspecific as "refresh all my
> inputs". A backward link may not be able to tell the state if
> information about next backward link is lost (logic OR of several
> signals) whereas the matrix should be able to do that even in that
> case.
>
> If routes in IO-APIC change, the matrix needs to be recalculated and
> states in the affected devices should be updated. This update cycle
> could be triggered via normal qemu_irq device input lines or direct
> callback (needs more changes). Maybe even this could be handled lazily
> so that no updates would be needed until the final output changes or
> the state of an intermediate device is queried.
>
> The global manager should handle saving and restoring states, the
> device states would not matter. If it pushed the true state to
> devices, we're back to the ordering problem.

I was thinking that the manager would be at board level, but actually
the wiring at the board level can be considered as a second switch
matrix (mostly constant). Then with these two matrices, all signal
routing for the whole machine could be

Re: [Qemu-devel] [PATCH v2] rbd: fix leak in qemu_rbd_open failure paths

[Stefan Hajnoczi]

On Sat, Sep 3, 2011 at 11:04 PM, Sage Weil  wrote:
> +failed_shutdown:
>     rados_shutdown(s->cluster);
> +    qemu_free(s->snap);

Sorry for being a pain here.  This patch is against an old qemu.git
tree.  All memory allocation is now using glib's g_malloc()/g_free().

Stefan

Re: [Qemu-devel] [PATCH] hw/qxl: Fix format string errors

[Alon Levy]

On Sat, Sep 03, 2011 at 02:48:25PM +0100, Peter Maydell wrote:
> Fix format string errors causing compile failure on 32 bit hosts
> when spice is enabled.
> 

Patch looks good.

Reviewed-by: Alon Levy 

> Signed-off-by: Peter Maydell 
> ---
> This fixes the easy parts of the 32 bit compile failures with
> spice enabled. It leaves the following two warnings:
>  hw/qxl.c: In function 'interface_release_resource':
>  hw/qxl.c:627:46: error: cast to pointer from integer of different size
>  hw/qxl.c: In function 'qxl_phys2virt':
>  hw/qxl.c:1003:16: error: cast to pointer from integer of different size
> 
> which appear to be making deeper "pointers are 64 bits" assumptions
> that I don't know enough about the spice code to suggest fixes for.
> (Throwing in (intptr_t) casts suppresses the warning but seems a
> bit icky...)
> 
> Could somebody else take a look at those, please?
> 

Spice server doesn't work for 32 bit machines. So regardless of the
fix it should not be possible to enable it for 32 bit builds. I think
if it were to work then the casts you suggest would be the simplest
assumption, keeping the qxl device structs the same (64 bit address
fields).


>  hw/qxl-logger.c |2 +-
>  hw/qxl.c|8 
>  2 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/qxl-logger.c b/hw/qxl-logger.c
> index 74cadba..367aad1 100644
> --- a/hw/qxl-logger.c
> +++ b/hw/qxl-logger.c
> @@ -224,7 +224,7 @@ void qxl_log_command(PCIQXLDevice *qxl, const char *ring, 
> QXLCommandExt *ext)
>  if (!qxl->cmdlog) {
>  return;
>  }
> -fprintf(stderr, "%ld qxl-%d/%s:", qemu_get_clock_ns(vm_clock),
> +fprintf(stderr, "%" PRId64 " qxl-%d/%s:", qemu_get_clock_ns(vm_clock),
>  qxl->id, ring);
>  fprintf(stderr, " cmd @ 0x%" PRIx64 " %s%s", ext->cmd.data,
>  qxl_name(qxl_type, ext->cmd.type),
> diff --git a/hw/qxl.c b/hw/qxl.c
> index 45e2401..1fe0b53 100644
> --- a/hw/qxl.c
> +++ b/hw/qxl.c
> @@ -959,7 +959,7 @@ static void qxl_add_memslot(PCIQXLDevice *d, uint32_t 
> slot_id, uint64_t delta,
>  memslot.generation = d->rom->slot_generation = 0;
>  qxl_rom_set_dirty(d);
>  
> -dprint(d, 1, "%s: slot %d: host virt 0x%" PRIx64 " - 0x%" PRIx64 "\n",
> +dprint(d, 1, "%s: slot %d: host virt 0x%lx - 0x%lx\n",
> __FUNCTION__, memslot.slot_id,
> memslot.virt_start, memslot.virt_end);
>  
> @@ -1090,8 +1090,8 @@ static void qxl_set_mode(PCIQXLDevice *d, int modenr, 
> int loadvm)
>  .mem= devmem + d->shadow_rom.draw_area_offset,
>  };
>  
> -dprint(d, 1, "%s: mode %d  [ %d x %d @ %d bpp devmem 0x%lx ]\n", 
> __FUNCTION__,
> -   modenr, mode->x_res, mode->y_res, mode->bits, devmem);
> +dprint(d, 1, "%s: mode %d  [ %d x %d @ %d bpp devmem 0x%" PRIx64 " ]\n",
> +   __func__, modenr, mode->x_res, mode->y_res, mode->bits, devmem);
>  if (!loadvm) {
>  qxl_hard_reset(d, 0);
>  }
> @@ -1229,7 +1229,7 @@ async_common:
>  break;
>  case QXL_IO_LOG:
>  if (d->guestdebug) {
> -fprintf(stderr, "qxl/guest-%d: %ld: %s", d->id,
> +fprintf(stderr, "qxl/guest-%d: %" PRId64 ": %s", d->id,
>  qemu_get_clock_ns(vm_clock), d->ram->log_buf);
>  }
>  break;
> -- 
> 1.7.4.1
> 
> 

Re: [Qemu-devel] [PATCH] pc: Clean up PIC-to-APIC IRQ path

[Jan Kiszka]

On 2011-09-03 21:54, Anthony Liguori wrote:
> On 08/31/2011 05:53 AM, Jan Kiszka wrote:
>> On 2011-08-31 10:25, Peter Maydell wrote:
>>> On 30 August 2011 20:28, Jan Kiszka  wrote:
 Yes, that's the current state. Once we have bidirectional IRQ links in
 place (pushing downward, querying upward - required to skip IRQ routers
 for fast, lockless deliveries), that should change again.
>>>
>>> Can you elaborate a bit more on this? I don't think anybody has
>>> proposed links with their own internal state before in the qdev/qom
>>> discussions...
>>
>> That basic idea is to allow
>>
>> a) a discovery of the currently active IRQ path from source to sink
>> (that would be possible via QOM just using forward links)
>>
>> b) skip updating the states of IRQ routers in the common case, just
>> signaling directly the sink from the source (to allow in-kernel IRQ
>> delivery or to skip taking some device locks). Whenever some router
>> is queried for its current IRQ line state, it would have to ask the
>> preceding IRQ source for its state. So we need a backward link.
> 
> Can you provide some concrete use-cases of this?  I'm not convinced this 
> is really all that important and it seems like tremendous amounts of 
> ugliness would be needed to support it.

INTx support for device assignment, vhost, or any other future in-kernel
IRQ sources. And optimized user space IRQ delivery, particularly under
real-time constraints.

When designing those requirements into a new IRQ/pin management model
right from the beginning, that shouldn't be as ugly as you may think. At
least it will be less ugly and more correct than what we already have in
qemu-kvm today.

Jan



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH] pc: Clean up PIC-to-APIC IRQ path

[Avi Kivity]

On 08/31/2011 01:53 PM, Jan Kiszka wrote:

On 2011-08-31 10:25, Peter Maydell wrote:
>  On 30 August 2011 20:28, Jan Kiszka  wrote:
>>  Yes, that's the current state. Once we have bidirectional IRQ links in
>>  place (pushing downward, querying upward - required to skip IRQ routers
>>  for fast, lockless deliveries), that should change again.
>
>  Can you elaborate a bit more on this? I don't think anybody has
>  proposed links with their own internal state before in the qdev/qom
>  discussions...

That basic idea is to allow

a) a discovery of the currently active IRQ path from source to sink
(that would be possible via QOM just using forward links)

b) skip updating the states of IRQ routers in the common case, just
signaling directly the sink from the source (to allow in-kernel IRQ
delivery or to skip taking some device locks). Whenever some router
is queried for its current IRQ line state, it would have to ask the
preceding IRQ source for its state. So we need a backward link.

We haven't thought about how this could be implemented in details yet
though. Among other things, it heavily depends on the final QOM design.



Looks like a similar path to the memory API.  A declarative description 
of the interrupt hierarchy allows routes to be precalculated and flattened.


(here it's strictly an optimization; with the memory API it's a 
requirement since kvm requires a flattened representation, and tcg is 
greatly simplified by it).


--
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] pc: Clean up PIC-to-APIC IRQ path

[Gleb Natapov]

> On Wed, Aug 31, 2011 at 6:17 PM, Jan Kiszka  wrote:
>> On 2011-08-31 19:41, Blue Swirl wrote:
>>> On Wed, Aug 31, 2011 at 10:53 AM, Jan Kiszka  wrote:
 On 2011-08-31 10:25, Peter Maydell wrote:
> On 30 August 2011 20:28, Jan Kiszka  wrote:
>> Yes, that's the current state. Once we have bidirectional IRQ links in
>> place (pushing downward, querying upward - required to skip IRQ routers
>> for fast, lockless deliveries), that should change again.
>
> Can you elaborate a bit more on this? I don't think anybody has
> proposed links with their own internal state before in the qdev/qom
> discussions...

 That basic idea is to allow

 a) a discovery of the currently active IRQ path from source to sink
   (that would be possible via QOM just using forward links)
>>>
>>> Why, only for b)? This is not possible with real hardware.
>>>
 b) skip updating the states of IRQ routers in the common case, just
   signaling directly the sink from the source (to allow in-kernel IRQ
   delivery or to skip taking some device locks). Whenever some router
   is queried for its current IRQ line state, it would have to ask the
   preceding IRQ source for its state. So we need a backward link.
>>>
>>> I think this would need pretty heavy changes everywhere. At board
>>> level the full path needs to be identified and special versions of
>>> IRQs installed along the way. The routers would need to use callbacks
>>> to inform other parties about routing changes.
>>
>> It already works in practice (based on a hack and minus IRQ router state
>> updates) for x86 PCI device pass-through. At least I don't want this
>> upstream but instead a generic solution. The ability to skip IRQ routers
>> also in pure user space device model scenarios is a useful by-product.
>>
>>>
 We haven't thought about how this could be implemented in details yet
 though. Among other things, it heavily depends on the final QOM design.
>>>
>>> Perhaps a global IRQ manager could help. It would keep track of the
>>> whole IRQ matrix, what are input (x axis) and output (y axis) states
>>> and what each matrix node (router state) looks like (or able to
>>> compute) if asked. I don't think backward links would be needed with
>>> this approach.
>>
>> Well, the backward links would then be moved to that global IRQ manager.
>> It's just moving the data management, but if it turns out to allow a
>> cleaner device design, I would surely not vote against it. But that
>> manager must support lazy updates as well because we cannot call it from
>> kernel space for each and every event.
>
> The global IRQ switch matrix would take over all routing for the
> devices in question. As an example, let's consider a PCI card
> (source), PCI host bridge, IO-APIC, LAPIC and CPU (final destination).
--^ LAPICs and CPUs

--
Gleb.

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Michael S. Tsirkin]

On Sun, Aug 28, 2011 at 04:53:33PM +0300, Avi Kivity wrote:
> On 08/28/2011 04:42 PM, Michael S. Tsirkin wrote:
> >On Sun, Aug 28, 2011 at 04:10:14PM +0300, Avi Kivity wrote:
> >>  On 08/28/2011 02:41 PM, Michael S. Tsirkin wrote:
> >>  >>
> >>  >>   If it really matters, you can add a prefetchability attribute to
> >>  >>   MemoryRegions.  Does it though?
> >>  >
> >>  >Well, its another one of these things that
> >>  >guests *probably* won't in practice use.
> >>  >But I don't see a way to be sure.
> >>  >
> >>  >If the guest puts a prefetcheable memory BAR behind
> >>  >a non-prefetcheable range in the bridge, it won't
> >>  >be able to access that BAR, and it should.
> >>
> >>  Not sure I understand - on real hardware, does it see the BAR or not?
> >
> >It does.
> 
> Ok, was different from what I thought.  So anything that matches the
> two windows is exposed (after clipping).  If the guest enables the
> legacy vga range, then there are three regions, with equal
> treatment, yes?
> 
> >ATM we have each BAR as a memory region, which is
> >in turn within io or memory address space region.
> >With bridges, each bridge has a single range
> >covering legal io addresses below it, and two ranges for memory.
> >
> >Example from a real system:
> > Memory behind bridge: 9820-982f
> > Prefetchable memory behind bridge: 9700-977f
> >
> >And a device can have:
> >
> > Region 0: Memory at 9820 (64-bit, non-prefetchable) [size=1M]
> > Region 2: Memory at 9700 (64-bit, prefetchable) [size=8M]
> >
> >
> >guest can in theory reprogram this:
> >
> > Memory behind bridge: 9820-98af
> > Prefetchable memory behind bridge: 9700-977f
> >
> >and
> > Region 0: Memory at 9820 (64-bit, non-prefetchable) [size=1M]
> > Region 2: Memory at 9830 (64-bit, prefetchable) [size=8M]
> >
> >and the device will work (presumably, guests will try to avoid this
> >as they assume prefetchable ranges are faster).
> 
> >Thus, which range the device BAR is behind depends on the
> >programmed values. How do we model that?
> 
> Create a memory region for the bridge's address space.  This region
> is not directly added to system_memory or its descendants.

I do this for each bridge in the hierarchy, right?

> Devices
> under the bridge see this region as its pci_address_space().  The
> region is as large as the entire address space - it does not take
> into account any windows.
> 
> For each of the three windows (pref, non-pref, vga), create an alias
> with the appropriate start and size.  Map the alias into the
> bridge's parent's pci_address_space(), as subregions.
> 
> fx440 does exactly this, with the following cosmetic changes:
> 
> - the windows are different (vga, pci hole, 64-bit pci area, PAMx, SMRAM)
> - instead of mapping them to the parent bridge's
> pci_address_space(), we map them to get_system_memory()
> 
> >A side note on bus filtering:
> >In cases of bus range partially hinding the BAR from the guest, one can
> >even have multiple non-contigious bits of the BAR visible and the rest
> >hidden.
> 
> The memory API will deal with this perfectly.
> 
> >I'm not saying it's very important to model this,
> >I'm guessing the only important cases are all of the
> >BAR visible and all of the BAR hidden.
> 
> It should all work.  Including a sub-bridge's windows being
> fragmented by the parent bridge.  Too bad it doesn't matter in
> practice, because it's a really neat solution to this non-problem.

Hmm, what ties the windows of a child bridge
to be within the windows of a parent?



> -- 
> error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] pc: Clean up PIC-to-APIC IRQ path

[Jan Kiszka]

On 2011-09-04 14:17, Avi Kivity wrote:
> On 08/31/2011 01:53 PM, Jan Kiszka wrote:
>> On 2011-08-31 10:25, Peter Maydell wrote:
>> >  On 30 August 2011 20:28, Jan Kiszka  wrote:
>> >>  Yes, that's the current state. Once we have bidirectional IRQ
>> links in
>> >>  place (pushing downward, querying upward - required to skip IRQ
>> routers
>> >>  for fast, lockless deliveries), that should change again.
>> >
>> >  Can you elaborate a bit more on this? I don't think anybody has
>> >  proposed links with their own internal state before in the qdev/qom
>> >  discussions...
>>
>> That basic idea is to allow
>>
>> a) a discovery of the currently active IRQ path from source to sink
>> (that would be possible via QOM just using forward links)
>>
>> b) skip updating the states of IRQ routers in the common case, just
>> signaling directly the sink from the source (to allow in-kernel IRQ
>> delivery or to skip taking some device locks). Whenever some router
>> is queried for its current IRQ line state, it would have to ask the
>> preceding IRQ source for its state. So we need a backward link.
>>
>> We haven't thought about how this could be implemented in details yet
>> though. Among other things, it heavily depends on the final QOM design.
>>
> 
> Looks like a similar path to the memory API.  A declarative description
> of the interrupt hierarchy allows routes to be precalculated and flattened.
> 
> (here it's strictly an optimization; with the memory API it's a
> requirement since kvm requires a flattened representation, and tcg is
> greatly simplified by it).

With current kvm device assignment it's mandatory as it only support
kernel/kernel IRQ delivery. Only vfio's eventfds will make it optional
(but still highly desirable).

Jan



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Avi Kivity]

On 09/04/2011 03:30 PM, Michael S. Tsirkin wrote:

>
>  Create a memory region for the bridge's address space.  This region
>  is not directly added to system_memory or its descendants.

I do this for each bridge in the hierarchy, right?


Each bridge does this independently (so yes).


>  fx440 does exactly this, with the following cosmetic changes:
>
>  - the windows are different (vga, pci hole, 64-bit pci area, PAMx, SMRAM)
>  - instead of mapping them to the parent bridge's
>  pci_address_space(), we map them to get_system_memory()

Hmm, what ties the windows of a child bridge
to be within the windows of a parent?



system_memory
  |
  +--- pci0_alias0 (aliases part of pci0)

pci0
  |
  +--- pci1_alias0 (a bridge)

pci1
  |
  +--- pci2_alias0 (another bridge)

pci2
  |
  +--- BAR

When rendering the memory hierarchy, the only parts of BAR which are 
visible are those that fit the clipping regions pci0_alias0 ^ 
pci1_alias0 ^ pci2_alias0.  If there are multiple aliases (like the low 
and high PCI holes, and PAM, it becomes (pci0_alias0 v pci0_alias1) ^ 
(pci1_alias0v pci1_alias1) ^ (pci2_alias0 v pci2_alias1). ( "^" == 
intersection, "v" == union )


--
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] pc: Clean up PIC-to-APIC IRQ path

[Avi Kivity]

On 09/04/2011 03:37 PM, Jan Kiszka wrote:

>
>  (here it's strictly an optimization; with the memory API it's a
>  requirement since kvm requires a flattened representation, and tcg is
>  greatly simplified by it).

With current kvm device assignment it's mandatory as it only support
kernel/kernel IRQ delivery. Only vfio's eventfds will make it optional
(but still highly desirable).



Right.

It's nice to have a single model for everything - understand once, 
confuse everywhere.


--
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Michael S. Tsirkin]

On Sun, Sep 04, 2011 at 03:40:58PM +0300, Avi Kivity wrote:
> On 09/04/2011 03:30 PM, Michael S. Tsirkin wrote:
> >>
> >>  Create a memory region for the bridge's address space.  This region
> >>  is not directly added to system_memory or its descendants.
> >
> >I do this for each bridge in the hierarchy, right?
> 
> Each bridge does this independently (so yes).
> 
> >>  fx440 does exactly this, with the following cosmetic changes:
> >>
> >>  - the windows are different (vga, pci hole, 64-bit pci area, PAMx, SMRAM)
> >>  - instead of mapping them to the parent bridge's
> >>  pci_address_space(), we map them to get_system_memory()
> >
> >Hmm, what ties the windows of a child bridge
> >to be within the windows of a parent?
> >
> 
> system_memory
>   |
>   +--- pci0_alias0 (aliases part of pci0)
> 
> pci0
>   |
>   +--- pci1_alias0 (a bridge)
> 
> pci1
>   |
>   +--- pci2_alias0 (another bridge)
> 
> pci2
>   |
>   +--- BAR
> 
> When rendering the memory hierarchy, the only parts of BAR which are
> visible are those that fit the clipping regions pci0_alias0 ^
> pci1_alias0 ^ pci2_alias0.  If there are multiple aliases (like the
> low and high PCI holes, and PAM, it becomes (pci0_alias0 v
> pci0_alias1) ^ (pci1_alias0v pci1_alias1) ^ (pci2_alias0 v
> pci2_alias1). ( "^" == intersection, "v" == union )

What about BAR directly behind pci0?
That should be unaffected by bridges pci1 and pci2
since the device is not behind that.


> -- 
> error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Avi Kivity]

On 09/04/2011 04:01 PM, Michael S. Tsirkin wrote:

On Sun, Sep 04, 2011 at 03:40:58PM +0300, Avi Kivity wrote:
>  On 09/04/2011 03:30 PM, Michael S. Tsirkin wrote:
>  >>
>  >>   Create a memory region for the bridge's address space.  This region
>  >>   is not directly added to system_memory or its descendants.
>  >
>  >I do this for each bridge in the hierarchy, right?
>
>  Each bridge does this independently (so yes).
>
>  >>   fx440 does exactly this, with the following cosmetic changes:
>  >>
>  >>   - the windows are different (vga, pci hole, 64-bit pci area, PAMx, 
SMRAM)
>  >>   - instead of mapping them to the parent bridge's
>  >>   pci_address_space(), we map them to get_system_memory()
>  >
>  >Hmm, what ties the windows of a child bridge
>  >to be within the windows of a parent?
>  >
>
>  system_memory
>|
>+--- pci0_alias0 (aliases part of pci0)
>
>  pci0
>|
>+--- pci1_alias0 (a bridge)
>
>  pci1
>|
>+--- pci2_alias0 (another bridge)
>
>  pci2
>|
>+--- BAR
>
>  When rendering the memory hierarchy, the only parts of BAR which are
>  visible are those that fit the clipping regions pci0_alias0 ^
>  pci1_alias0 ^ pci2_alias0.  If there are multiple aliases (like the
>  low and high PCI holes, and PAM, it becomes (pci0_alias0 v
>  pci0_alias1) ^ (pci1_alias0v pci1_alias1) ^ (pci2_alias0 v
>  pci2_alias1). ( "^" == intersection, "v" == union )

What about BAR directly behind pci0?
That should be unaffected by bridges pci1 and pci2
since the device is not behind that.



It follows naturally:

system_memory
  |
  +--- pci0_alias0 (aliases part of pci0)
   |
pci0 <-+
  |
  +--- pci1_alias0 (a bridge)
  ||
  +--- BAR0.0  |
   |
pci1 <-+
  |
  +--- pci2_alias0 (another bridge)
  ||
  +--- BAR1.0  |
   |
pci2 <-+
  |
  +--- BAR2.0


BAR0.0 is only filtered by pci0_alias*, BAR1.0 is filtered by 
pci[01]_alias*.  Since pci_register_bar() adds the BAR as a subregion of 
the bridge's pci_address_space(), it works without any global knowledge, 
with this topology, or if pci1 and pci2 are siblings instead of parent 
and child.


--
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Avi Kivity]

On 09/04/2011 04:05 PM, Avi Kivity wrote:


It follows naturally:

system_memory
  |
  +--- pci0_alias0 (aliases part of pci0)
   |
pci0 <-+


The pointer from pci0_alias to pci0 looked a lot better when I drew it.

--
error compiling committee.c: too many arguments to function

[Qemu-devel] [PATCH] Fix comment (install patch -> install path)

[Stefan Weil]

Signed-off-by: Stefan Weil 
---
 vl.c |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/vl.c b/vl.c
index 5ba9b35..c9e1975 100644
--- a/vl.c
+++ b/vl.c
@@ -3061,7 +3061,7 @@ int main(int argc, char **argv, char **envp)
 if (!data_dir) {
 data_dir = os_find_datadir(argv[0]);
 }
-/* If all else fails use the install patch specified when building.  */
+/* If all else fails use the install path specified when building. */
 if (!data_dir) {
 data_dir = CONFIG_QEMU_DATADIR;
 }
-- 
1.7.2.5

[Qemu-devel] [PATCH] configure: Remove relicts from --enable-io-thread

[Stefan Weil]

Commit 12d4536f7d911b6d87a766ad7300482ea663cea2 removed
configure option --enable-io-thread.

Remove help message which is now no longer valid.

Cc: Anthony Liguori 
Signed-off-by: Stefan Weil 
---
 configure |1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/configure b/configure
index e242ece..048ab4b 100755
--- a/configure
+++ b/configure
@@ -1025,7 +1025,6 @@ echo "  --disable-linux-aio  disable Linux AIO 
support"
 echo "  --enable-linux-aio   enable Linux AIO support"
 echo "  --disable-attr   disables attr and xattr support"
 echo "  --enable-attrenable attr and xattr support"
-echo "  --enable-io-thread   enable IO thread"
 echo "  --disable-blobs  disable installing provided firmware blobs"
 echo "  --enable-docsenable documentation build"
 echo "  --disable-docs   disable documentation build"
-- 
1.7.2.5

Re: [Qemu-devel] [PATCH] Rename qemu -> qemu-system-i386

[Andreas Färber]

Am 03.09.2011 um 22:09 schrieb Anthony Liguori:


On 09/03/2011 08:44 AM, Andreas Färber wrote:

Am 02.09.2011 um 17:40 schrieb Anthony Liguori:


On 08/29/2011 09:55 AM, Anthony Liguori wrote:

diff --git a/Makefile.target b/Makefile.target
index 07af4d4..29287ed 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -27,12 +27,8 @@ ifdef CONFIG_USER_ONLY
QEMU_PROG=qemu-$(TARGET_ARCH2)
else
# system emulator name
-ifeq ($(TARGET_ARCH), i386)
-QEMU_PROG=qemu$(EXESUF)
-else
QEMU_PROG=qemu-system-$(TARGET_ARCH2)$(EXESUF)
endif
-endif

PROGS=$(QEMU_PROG)
STPFILES=


This will leave an old qemu executable from a previous `make install`
behind.


You're not supposed to do a make install on top of another install.  
You're supposed to first do a make uninstall in the old tree than a  
make install in the new tree.


In practice though, I do git pull, make, make install and so will  
others.


We should check for it and, unless it's a symlink to qemu-system- 
i386,

remove it in the install target.


Once we're no longer generating an executable, we should be removing  
it from the system.


It's up to the user to remove old files from the system.


Yeah, Paolo is right there, install shouldn't remove.

What I had in mind was Git's switch from lots of git-* scripts to the  
single git. `make install` checked for such artefacts. My point was, I  
believe we should do the same here and *detect* it within the install  
target.
I'd be fine with a prominent textual notice. And I'd contribute a  
patch myself but have more urgent worries first, so if someone with  
more shell foo wants to take a stab, please do.


We should also make sure `make clean` still removes an old qemu from  
the build dir.


Andreas

Re: [Qemu-devel] [PATCH] pc: Clean up PIC-to-APIC IRQ path

[Anthony Liguori]

On 09/04/2011 07:13 AM, Jan Kiszka wrote:

On 2011-09-03 21:54, Anthony Liguori wrote:

On 08/31/2011 05:53 AM, Jan Kiszka wrote:

On 2011-08-31 10:25, Peter Maydell wrote:

On 30 August 2011 20:28, Jan Kiszka   wrote:

Yes, that's the current state. Once we have bidirectional IRQ links in
place (pushing downward, querying upward - required to skip IRQ routers
for fast, lockless deliveries), that should change again.


Can you elaborate a bit more on this? I don't think anybody has
proposed links with their own internal state before in the qdev/qom
discussions...


That basic idea is to allow

a) a discovery of the currently active IRQ path from source to sink
 (that would be possible via QOM just using forward links)

b) skip updating the states of IRQ routers in the common case, just
 signaling directly the sink from the source (to allow in-kernel IRQ
 delivery or to skip taking some device locks). Whenever some router
 is queried for its current IRQ line state, it would have to ask the
 preceding IRQ source for its state. So we need a backward link.


Can you provide some concrete use-cases of this?  I'm not convinced this
is really all that important and it seems like tremendous amounts of
ugliness would be needed to support it.


INTx support for device assignment, vhost, or any other future in-kernel
IRQ sources.


I prefer to not think of IRQs as special things.  They're just single 
bits of information that flow through the device model.  Having a higher 
level representation that understands something like paths seems wrong 
to me.


I'd prefer to treat things like device assignment as a hack.  You just 
need code that can walk the device tree to figure out the path (which is 
not generic code, it's very machine specific).  Then you tell the kernel 
the resolution of the path and are otherwise completely oblivious in 
userspace.


Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH] pc: Clean up PIC-to-APIC IRQ path

[Anthony Liguori]

On 09/04/2011 07:17 AM, Avi Kivity wrote:

On 08/31/2011 01:53 PM, Jan Kiszka wrote:

On 2011-08-31 10:25, Peter Maydell wrote:
> On 30 August 2011 20:28, Jan Kiszka wrote:
>> Yes, that's the current state. Once we have bidirectional IRQ links in
>> place (pushing downward, querying upward - required to skip IRQ
routers
>> for fast, lockless deliveries), that should change again.
>
> Can you elaborate a bit more on this? I don't think anybody has
> proposed links with their own internal state before in the qdev/qom
> discussions...

That basic idea is to allow

a) a discovery of the currently active IRQ path from source to sink
(that would be possible via QOM just using forward links)

b) skip updating the states of IRQ routers in the common case, just
signaling directly the sink from the source (to allow in-kernel IRQ
delivery or to skip taking some device locks). Whenever some router
is queried for its current IRQ line state, it would have to ask the
preceding IRQ source for its state. So we need a backward link.

We haven't thought about how this could be implemented in details yet
though. Among other things, it heavily depends on the final QOM design.



Looks like a similar path to the memory API. A declarative description
of the interrupt hierarchy allows routes to be precalculated and flattened.

(here it's strictly an optimization; with the memory API it's a
requirement since kvm requires a flattened representation, and tcg is
greatly simplified by it).


Let's not be careful to over generalize here.  Memory access is a fairly 
complicated thing involving multiple different types of busses, various 
properties (endianness, alignment, access constraints).  IRQs are 
extremely simple.  They're just single bits of information.


Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH] pc: Clean up PIC-to-APIC IRQ path

[Jan Kiszka]

On 2011-09-04 15:32, Anthony Liguori wrote:
> On 09/04/2011 07:13 AM, Jan Kiszka wrote:
>> On 2011-09-03 21:54, Anthony Liguori wrote:
>>> On 08/31/2011 05:53 AM, Jan Kiszka wrote:
 On 2011-08-31 10:25, Peter Maydell wrote:
> On 30 August 2011 20:28, Jan Kiszka   wrote:
>> Yes, that's the current state. Once we have bidirectional IRQ
>> links in
>> place (pushing downward, querying upward - required to skip IRQ
>> routers
>> for fast, lockless deliveries), that should change again.
>
> Can you elaborate a bit more on this? I don't think anybody has
> proposed links with their own internal state before in the qdev/qom
> discussions...

 That basic idea is to allow

 a) a discovery of the currently active IRQ path from source to sink
  (that would be possible via QOM just using forward links)

 b) skip updating the states of IRQ routers in the common case, just
  signaling directly the sink from the source (to allow in-kernel
 IRQ
  delivery or to skip taking some device locks). Whenever some
 router
  is queried for its current IRQ line state, it would have to ask
 the
  preceding IRQ source for its state. So we need a backward link.
>>>
>>> Can you provide some concrete use-cases of this?  I'm not convinced this
>>> is really all that important and it seems like tremendous amounts of
>>> ugliness would be needed to support it.
>>
>> INTx support for device assignment, vhost, or any other future in-kernel
>> IRQ sources.
> 
> I prefer to not think of IRQs as special things.  They're just single
> bits of information that flow through the device model.  Having a higher
> level representation that understands something like paths seems wrong
> to me.
> 
> I'd prefer to treat things like device assignment as a hack.  You just
> need code that can walk the device tree to figure out the path (which is
> not generic code, it's very machine specific).  Then you tell the kernel
> the resolution of the path and are otherwise completely oblivious in
> userspace.

See it as you like, but we need the support, not only for device
assigment. And I do not see any gain it hacking this instead of
designing it.

Jan



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH] pc: Clean up PIC-to-APIC IRQ path

[Anthony Liguori]

On 09/04/2011 07:37 AM, Jan Kiszka wrote:

On 2011-09-04 14:17, Avi Kivity wrote:

On 08/31/2011 01:53 PM, Jan Kiszka wrote:

On 2011-08-31 10:25, Peter Maydell wrote:

  On 30 August 2011 20:28, Jan Kiszka   wrote:

  Yes, that's the current state. Once we have bidirectional IRQ

links in

  place (pushing downward, querying upward - required to skip IRQ

routers

  for fast, lockless deliveries), that should change again.


  Can you elaborate a bit more on this? I don't think anybody has
  proposed links with their own internal state before in the qdev/qom
  discussions...


That basic idea is to allow

a) a discovery of the currently active IRQ path from source to sink
 (that would be possible via QOM just using forward links)

b) skip updating the states of IRQ routers in the common case, just
 signaling directly the sink from the source (to allow in-kernel IRQ
 delivery or to skip taking some device locks). Whenever some router
 is queried for its current IRQ line state, it would have to ask the
 preceding IRQ source for its state. So we need a backward link.

We haven't thought about how this could be implemented in details yet
though. Among other things, it heavily depends on the final QOM design.



Looks like a similar path to the memory API.  A declarative description
of the interrupt hierarchy allows routes to be precalculated and flattened.

(here it's strictly an optimization; with the memory API it's a
requirement since kvm requires a flattened representation, and tcg is
greatly simplified by it).


With current kvm device assignment it's mandatory as it only support
kernel/kernel IRQ delivery. Only vfio's eventfds will make it optional
(but still highly desirable).


It's not mandatory.  All you need to be able to do is calculate the APIC 
IRQ for a given PCI device interrupt.  That doesn't mean we need to be 
able to do arbitrary interrupt resolution in generic code.


There is potentially tremendous complexity here because you'll have to 
bake all interrupt rerouting logic into a declarative API and/or call 
into generic code to update routing tables.  Given the fact that we 
can't even generically refer to a device reliably today, this is would 
be a daunting task.


We're making this all more complicated than it needs to be.

Regards,

Anthony Liguori


Jan

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Michael S. Tsirkin]

On Sun, Sep 04, 2011 at 04:05:14PM +0300, Avi Kivity wrote:
> It follows naturally:

OK, so it seems the following is more or less what you suggest?
I'm not sure I create/destroy subregions properly.
Both the alias and the subregion get the same start value?
Is the region name for debugging only?
When does priority matter? In case of overlap?

diff --git a/hw/pci.c b/hw/pci.c
index 57ff7b1..56dfa18 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -889,7 +889,6 @@ void pci_register_bar(PCIDevice *pci_dev, int region_num,
 r = &pci_dev->io_regions[region_num];
 r->addr = PCI_BAR_UNMAPPED;
 r->size = size;
-r->filtered_size = size;
 r->type = type;
 r->memory = NULL;
 
@@ -920,41 +919,6 @@ pcibus_t pci_get_bar_addr(PCIDevice *pci_dev, int 
region_num)
 return pci_dev->io_regions[region_num].addr;
 }
 
-static void pci_bridge_filter(PCIDevice *d, pcibus_t *addr, pcibus_t *size,
-  uint8_t type)
-{
-pcibus_t base = *addr;
-pcibus_t limit = *addr + *size - 1;
-PCIDevice *br;
-
-for (br = d->bus->parent_dev; br; br = br->bus->parent_dev) {
-uint16_t cmd = pci_get_word(d->config + PCI_COMMAND);
-
-if (type & PCI_BASE_ADDRESS_SPACE_IO) {
-if (!(cmd & PCI_COMMAND_IO)) {
-goto no_map;
-}
-} else {
-if (!(cmd & PCI_COMMAND_MEMORY)) {
-goto no_map;
-}
-}
-
-base = MAX(base, pci_bridge_get_base(br, type));
-limit = MIN(limit, pci_bridge_get_limit(br, type));
-}
-
-if (base > limit) {
-goto no_map;
-}
-*addr = base;
-*size = limit - base + 1;
-return;
-no_map:
-*addr = PCI_BAR_UNMAPPED;
-*size = 0;
-}
-
 static pcibus_t pci_bar_address(PCIDevice *d,
int reg, uint8_t type, pcibus_t size)
 {
@@ -1024,7 +988,7 @@ static void pci_update_mappings(PCIDevice *d)
 {
 PCIIORegion *r;
 int i;
-pcibus_t new_addr, filtered_size;
+pcibus_t new_addr;
 
 for(i = 0; i < PCI_NUM_REGIONS; i++) {
 r = &d->io_regions[i];
@@ -1035,14 +999,8 @@ static void pci_update_mappings(PCIDevice *d)
 
 new_addr = pci_bar_address(d, i, r->type, r->size);
 
-/* bridge filtering */
-filtered_size = r->size;
-if (new_addr != PCI_BAR_UNMAPPED) {
-pci_bridge_filter(d, &new_addr, &filtered_size, r->type);
-}
-
 /* This bar isn't changed */
-if (new_addr == r->addr && filtered_size == r->filtered_size)
+if (new_addr == r->addr)
 continue;
 
 /* now do the real mapping */
@@ -1050,15 +1008,7 @@ static void pci_update_mappings(PCIDevice *d)
 memory_region_del_subregion(r->address_space, r->memory);
 }
 r->addr = new_addr;
-r->filtered_size = filtered_size;
 if (r->addr != PCI_BAR_UNMAPPED) {
-/*
- * TODO: currently almost all the map funcions assumes
- * filtered_size == size and addr & ~(size - 1) == addr.
- * However with bridge filtering, they aren't always true.
- * Teach them such cases, such that filtered_size < size and
- * addr & (size - 1) != 0.
- */
 if (r->type & PCI_BASE_ADDRESS_SPACE_IO) {
 memory_region_add_subregion_overlap(r->address_space,
 r->addr,
@@ -1576,22 +1526,6 @@ PCIDevice *pci_nic_init_nofail(NICInfo *nd, const char 
*default_model,
 return res;
 }
 
-static void pci_bridge_update_mappings_fn(PCIBus *b, PCIDevice *d)
-{
-pci_update_mappings(d);
-}
-
-void pci_bridge_update_mappings(PCIBus *b)
-{
-PCIBus *child;
-
-pci_for_each_device_under_bus(b, pci_bridge_update_mappings_fn);
-
-QLIST_FOREACH(child, &b->child, sibling) {
-pci_bridge_update_mappings(child);
-}
-}
-
 /* Whether a given bus number is in range of the secondary
  * bus of the given bridge device. */
 static bool pci_secondary_bus_in_range(PCIDevice *dev, int bus_num)
diff --git a/hw/pci.h b/hw/pci.h
index 391217e..65e1568 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -90,7 +90,6 @@ typedef struct PCIIORegion {
 pcibus_t addr; /* current PCI mapping address. -1 means not mapped */
 #define PCI_BAR_UNMAPPED (~(pcibus_t)0)
 pcibus_t size;
-pcibus_t filtered_size;
 uint8_t type;
 MemoryRegion *memory;
 MemoryRegion *address_space;
@@ -277,7 +276,6 @@ int pci_read_devaddr(Monitor *mon, const char *addr, int 
*domp, int *busp,
 
 void do_pci_info_print(Monitor *mon, const QObject *data);
 void do_pci_info(Monitor *mon, QObject **ret_data);
-void pci_bridge_update_mappings(PCIBus *b);
 
 void pci_device_deassert_intx(PCIDevice *dev);
 
diff --git a/hw/pci_bridge.c b/hw/pci_bridge.c
index e0b339e..469dfe8 100644
--- a/hw/pci_bridge.c
+++ b/hw/pci_bridge.c
@@ -135,6 +135,75 @@ pcibus_t pci_bridge_get_limit(const PCIDevice *bridge, 
uint8_t type)
  

Re: [Qemu-devel] [PATCH] pc: Clean up PIC-to-APIC IRQ path

[Anthony Liguori]

On 09/04/2011 08:36 AM, Jan Kiszka wrote:

On 2011-09-04 15:32, Anthony Liguori wrote:

I prefer to not think of IRQs as special things.  They're just single
bits of information that flow through the device model.  Having a higher
level representation that understands something like paths seems wrong
to me.

I'd prefer to treat things like device assignment as a hack.  You just
need code that can walk the device tree to figure out the path (which is
not generic code, it's very machine specific).  Then you tell the kernel
the resolution of the path and are otherwise completely oblivious in
userspace.


See it as you like, but we need the support, not only for device
assigment. And I do not see any gain it hacking this instead of
designing it.


You can design a hack but it's still a hack.

Device state belongs in devices.  Trying to extract device state 
(interrupt routing paths) and externalizing it is by definition a hack.


Having some sort of global interrupt routing table is just going to add 
a layer of complexity for very little obvious gain.


The PCI bus *is* the I/O APIC for all intents and purposes.  Being able 
to ask the i440fx for the interrupt corresponding to a device is a very 
simple task that involves no generic code.  That's more than enough to 
support device assignment.


Why overcomplicate things when the problem can be solved robustly with 
most likely a 10 line function?


Regards,

Anthony Liguori



Jan

Re: [Qemu-devel] [PATCH] pc: Clean up PIC-to-APIC IRQ path

[Jan Kiszka]

On 2011-09-04 15:38, Anthony Liguori wrote:
> On 09/04/2011 07:37 AM, Jan Kiszka wrote:
>> On 2011-09-04 14:17, Avi Kivity wrote:
>>> On 08/31/2011 01:53 PM, Jan Kiszka wrote:
 On 2011-08-31 10:25, Peter Maydell wrote:
>   On 30 August 2011 20:28, Jan Kiszka   wrote:
>>   Yes, that's the current state. Once we have bidirectional IRQ
 links in
>>   place (pushing downward, querying upward - required to skip IRQ
 routers
>>   for fast, lockless deliveries), that should change again.
>
>   Can you elaborate a bit more on this? I don't think anybody has
>   proposed links with their own internal state before in the qdev/qom
>   discussions...

 That basic idea is to allow

 a) a discovery of the currently active IRQ path from source to sink
  (that would be possible via QOM just using forward links)

 b) skip updating the states of IRQ routers in the common case, just
  signaling directly the sink from the source (to allow in-kernel
 IRQ
  delivery or to skip taking some device locks). Whenever some
 router
  is queried for its current IRQ line state, it would have to ask
 the
  preceding IRQ source for its state. So we need a backward link.

 We haven't thought about how this could be implemented in details yet
 though. Among other things, it heavily depends on the final QOM design.

>>>
>>> Looks like a similar path to the memory API.  A declarative description
>>> of the interrupt hierarchy allows routes to be precalculated and
>>> flattened.
>>>
>>> (here it's strictly an optimization; with the memory API it's a
>>> requirement since kvm requires a flattened representation, and tcg is
>>> greatly simplified by it).
>>
>> With current kvm device assignment it's mandatory as it only support
>> kernel/kernel IRQ delivery. Only vfio's eventfds will make it optional
>> (but still highly desirable).
> 
> It's not mandatory.  All you need to be able to do is calculate the APIC
> IRQ for a given PCI device interrupt.

...and establish notifies for changes along this line. And allow to
update intermediate states on access.

>  That doesn't mean we need to be
> able to do arbitrary interrupt resolution in generic code.

We will likely have to solve the same problem on none x86 as well.

> 
> There is potentially tremendous complexity here because you'll have to
> bake all interrupt rerouting logic into a declarative API and/or call
> into generic code to update routing tables.  Given the fact that we
> can't even generically refer to a device reliably today, this is would
> be a daunting task.
> 
> We're making this all more complicated than it needs to be.

We can't discuss the problem away, sorry.

Jan



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH] pc: Clean up PIC-to-APIC IRQ path

[Jan Kiszka]

On 2011-09-04 15:41, Anthony Liguori wrote:
> On 09/04/2011 08:36 AM, Jan Kiszka wrote:
>> On 2011-09-04 15:32, Anthony Liguori wrote:
>>> I prefer to not think of IRQs as special things.  They're just single
>>> bits of information that flow through the device model.  Having a higher
>>> level representation that understands something like paths seems wrong
>>> to me.
>>>
>>> I'd prefer to treat things like device assignment as a hack.  You just
>>> need code that can walk the device tree to figure out the path (which is
>>> not generic code, it's very machine specific).  Then you tell the kernel
>>> the resolution of the path and are otherwise completely oblivious in
>>> userspace.
>>
>> See it as you like, but we need the support, not only for device
>> assigment. And I do not see any gain it hacking this instead of
>> designing it.
> 
> You can design a hack but it's still a hack.
> 
> Device state belongs in devices.  Trying to extract device state
> (interrupt routing paths) and externalizing it is by definition a hack.
> 
> Having some sort of global interrupt routing table is just going to add
> a layer of complexity for very little obvious gain.

It's not yet decided how the problem is solved. A global interrupt
matrix is just one proposal, another option is to extend the pin model
in a way that supports routing change notifiers and backward polling.

> 
> The PCI bus *is* the I/O APIC for all intents and purposes.  Being able
> to ask the i440fx for the interrupt corresponding to a device is a very
> simple task that involves no generic code.  That's more than enough to
> support device assignment.
> 
> Why overcomplicate things when the problem can be solved robustly with
> most likely a 10 line function?

Fine, then INTx is solved. And we will hate us for hacking things for
this single purpose when we later on want to support, say, MSI routing
though an emulated interrupt remapper. Or routing of the Q35 chipset.

It is not only INTx for device assignment, and it is not only x86.

Jan



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH] pc: Clean up PIC-to-APIC IRQ path

[Anthony Liguori]

On 09/04/2011 08:42 AM, Jan Kiszka wrote:

On 2011-09-04 15:38, Anthony Liguori wrote:

With current kvm device assignment it's mandatory as it only support
kernel/kernel IRQ delivery. Only vfio's eventfds will make it optional
(but still highly desirable).


It's not mandatory.  All you need to be able to do is calculate the APIC
IRQ for a given PCI device interrupt.


...and establish notifies for changes along this line. And allow to
update intermediate states on access.


Again, this is all in a single layer.




  That doesn't mean we need to be
able to do arbitrary interrupt resolution in generic code.


We will likely have to solve the same problem on none x86 as well.


But we're already seeing the that device assignment problem is very 
different on other platforms.


Given multiple well known architectures, yes, it makes sense to write 
generic code to handle both.


But we shouldn't just assume that just because we have one use-case that 
it generalizes to arbitrarily complex use-cases.


An IRQ routing table sounds good in theory but in practice it's just not 
that simple.  Image a simple device with an output pin that is high when 
there is data to read.


It looks and smells like an IRQ but there's nothing to say that it's 
ever going to be routed to the CPU as a level interrupt.  It may be 
handled entirely within another device for a certain platform.


But in another platform, it may actually be routed as a level triggered 
interrupt to the CPU.  In fact, as we do more pin level modelling, we'll 
run into a lot of scenarios like this.


Over generalizing is just going to box us into this narrow view of 
anything that's a single bit of information being directly routable to a 
CPU.



There is potentially tremendous complexity here because you'll have to
bake all interrupt rerouting logic into a declarative API and/or call
into generic code to update routing tables.  Given the fact that we
can't even generically refer to a device reliably today, this is would
be a daunting task.

We're making this all more complicated than it needs to be.


We can't discuss the problem away, sorry.


"We may need at some point in the future" is not a sufficient 
justification either.


If we want to talk about concrete scenarios, fine.  I'm all for doing 
what we need to do to support things.  But unless there's an absolutely 
compelling need to introduce a global interrupt routing table, I think 
we're much better off handling it as special cases.


Regards,

Anthony Liguori


Jan

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Avi Kivity]

On 09/04/2011 04:41 PM, Michael S. Tsirkin wrote:

On Sun, Sep 04, 2011 at 04:05:14PM +0300, Avi Kivity wrote:
>  It follows naturally:

OK, so it seems the following is more or less what you suggest?
I'm not sure I create/destroy subregions properly.
Both the alias and the subregion get the same start value?


Yes (so addresses are not shifted).


Is the region name for debugging only?


For everything except RAM regions (there, the name is also used for 
save/restore).



When does priority matter? In case of overlap?


Yes.  In this case, since overlap resolution is not defined by the spec, 
the actual priority does not matter.



@@ -135,6 +135,75 @@ pcibus_t pci_bridge_get_limit(const PCIDevice *bridge, 
uint8_t type)
  return limit;
  }

+static pcibus_t pci_bridge_get_size(const PCIDevice *bridge, uint8_t type)
+{
+return pci_bridge_get_limit(bridge, type)>=
+pci_bridge_get_base(bridge, type) ?
+pci_bridge_get_limit(bridge, type) -
+pci_bridge_get_base(bridge, type)  + 1 : 0;
+}


Correct but unreadable.  Doesn't work for limit == 2^64-1, is this a 
possible value?



+
+static void pci_bridge_region_init(PCIBridge *br)
+{
+PCIBus *sec_bus =&br->sec_bus;
+PCIBus *parent = br->dev.bus;
+memory_region_init_alias(sec_bus->alias_pref_mem, "pci_bridge_pref_mem",
+sec_bus->address_space_mem,
+pci_bridge_get_base(&br->dev, PCI_BASE_ADDRESS_MEM_PREFETCH),
+pci_bridge_get_size(&br->dev, PCI_BASE_ADDRESS_MEM_PREFETCH));
+memory_region_add_subregion_overlap(parent->address_space_mem,
+pci_bridge_get_base(&br->dev, PCI_BASE_ADDRESS_MEM_PREFETCH),
+ sec_bus->alias_pref_mem, 1);
+memory_region_init_alias(sec_bus->alias_mem, "pci_bridge_memory",
+sec_bus->address_space_mem,
+pci_bridge_get_base(&br->dev, PCI_BASE_ADDRESS_SPACE_MEMORY),
+pci_bridge_get_size(&br->dev, PCI_BASE_ADDRESS_SPACE_MEMORY));
+memory_region_add_subregion_overlap(parent->address_space_mem,
+pci_bridge_get_base(&br->dev, PCI_BASE_ADDRESS_SPACE_MEMORY),
+ sec_bus->alias_mem, 1);
+memory_region_init_alias(sec_bus->alias_io, "pci_bridge_io",
+sec_bus->address_space_io,
+pci_bridge_get_base(&br->dev, PCI_BASE_ADDRESS_SPACE_IO),
+pci_bridge_get_size(&br->dev, PCI_BASE_ADDRESS_SPACE_IO));
+memory_region_add_subregion_overlap(parent->address_space_io,
+pci_bridge_get_base(&br->dev, PCI_BASE_ADDRESS_SPACE_IO),
+ sec_bus->alias_io, 1);
+}


This looks right.  Might want to use pci_address_space() instead of 
->address_space_mem.


Don't you have to do something similar for the vga window?


+
+static void pci_bridge_region_cleanup(PCIBridge *br)
+{
+PCIBus *sec_bus =&br->sec_bus;
+PCIBus *parent = br->dev.bus;
+memory_region_del_subregion(parent->address_space_mem,
+sec_bus->alias_pref_mem);
+memory_region_destroy(sec_bus->alias_pref_mem);
+memory_region_del_subregion(parent->address_space_mem,
+sec_bus->alias_mem);
+memory_region_destroy(sec_bus->alias_mem);
+memory_region_del_subregion(parent->address_space_io,
+sec_bus->alias_io);
+memory_region_destroy(sec_bus->alias_io);
+}


This is fine too.


+
+static void pci_bridge_update_mappings(PCIBridge *br)
+{
+/* TODO: this doesn't handle the case of one VCPU
+ * updating the bridge while another accesses an unaffected
+ * region. To fix we'll need new memory region APIs. */
+pci_bridge_region_cleanup(br);
+pci_bridge_region_init(br);


memory_region_transaction_{begin,commit}()

(isn't 100% implemented, but at least the API is in place)

+
+#if 0
+TODO: do we need to propagate updates to child buses?
+
+pci_for_each_device_under_bus(b, pci_bridge_update_mappings_fn);
+
+QLIST_FOREACH(child,&b->child, sibling) {
+pci_bridge_update_mappings(child);
+}
+#endif
+}


Don't need this.

--
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] pc: Clean up PIC-to-APIC IRQ path

[Anthony Liguori]

On 09/04/2011 08:49 AM, Jan Kiszka wrote:

On 2011-09-04 15:41, Anthony Liguori wrote:

On 09/04/2011 08:36 AM, Jan Kiszka wrote:

On 2011-09-04 15:32, Anthony Liguori wrote:

I prefer to not think of IRQs as special things.  They're just single
bits of information that flow through the device model.  Having a higher
level representation that understands something like paths seems wrong
to me.

I'd prefer to treat things like device assignment as a hack.  You just
need code that can walk the device tree to figure out the path (which is
not generic code, it's very machine specific).  Then you tell the kernel
the resolution of the path and are otherwise completely oblivious in
userspace.


See it as you like, but we need the support, not only for device
assigment. And I do not see any gain it hacking this instead of
designing it.


You can design a hack but it's still a hack.

Device state belongs in devices.  Trying to extract device state
(interrupt routing paths) and externalizing it is by definition a hack.

Having some sort of global interrupt routing table is just going to add
a layer of complexity for very little obvious gain.


It's not yet decided how the problem is solved. A global interrupt
matrix is just one proposal, another option is to extend the pin model
in a way that supports routing change notifiers and backward polling.


If that's all you need, then you really just want notification on socket 
changes.  Backwards polling can be achieved by just adding state to the 
Pin (which I full heartedly support).


If that's all you're proposing, than I'm entirely happy with it :-)

I'm happy with that because all of the route detection logic can live 
outside of the devices which at least contains the complexity.


Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 2/2] main: switch qemu_set_fd_handler to g_io_add_watch

[Avi Kivity]

On 08/22/2011 04:12 PM, Anthony Liguori wrote:

This patch changes qemu_set_fd_handler to be implemented in terms of
g_io_add_watch().  The semantics are a bit different so some glue is required.

qemu_set_fd_handler2 is much harder to convert because of its use of polling.

The glib main loop has the major of advantage of having a proven thread safe
architecture.  By using the glib main loop instead of our own, it will allow us
to eventually introduce multiple I/O threads.

I'm pretty sure that this will work on Win32, but I would appreciate some help
testing.  I think the semantics of g_io_channel_unix_new() are really just tied
to the notion of a "unix fd" and not necessarily unix itself.


'git bisect' fingered this as responsible for breaking 
qcow2+cache=unsafe.  I think there's an off-by-one here and the guilty 
patch is the one that switches the main loop, but that's just a guess.


The symptoms are that a guest that is restarted (new qemu process) after 
install doesn't make it through grub - some image data didn't make it do 
disk.  With qcow2 and cache=unsafe that can easily happen through exit 
notifiers not being run and the entire qcow2 metadata being thrown out 
the window.  Running with raw+cache=unsafe works.





diff --git a/iohandler.c b/iohandler.c
index 4deae1e..5ef66fb 100644
--- a/iohandler.c
+++ b/iohandler.c
@@ -80,12 +80,67 @@ int qemu_set_fd_handler2(int fd,
  return 0;
  }

+typedef struct IOTrampoline
+{
+GIOChannel *chan;
+IOHandler *fd_read;
+IOHandler *fd_write;
+void *opaque;
+guint tag;
+} IOTrampoline;
+
+static gboolean fd_trampoline(GIOChannel *chan, GIOCondition cond, gpointer 
opaque)
+{
+IOTrampoline *tramp = opaque;
+
+if (tramp->opaque == NULL) {
+return FALSE;
+}
+
+if ((cond&  G_IO_IN)&&  tramp->fd_read) {
+tramp->fd_read(tramp->opaque);
+}
+
+if ((cond&  G_IO_OUT)&&  tramp->fd_write) {
+tramp->fd_write(tramp->opaque);
+}
+
+return TRUE;
+}
+
  int qemu_set_fd_handler(int fd,
  IOHandler *fd_read,
  IOHandler *fd_write,
  void *opaque)
  {
-return qemu_set_fd_handler2(fd, NULL, fd_read, fd_write, opaque);
+static IOTrampoline fd_trampolines[FD_SETSIZE];
+IOTrampoline *tramp =&fd_trampolines[fd];
+
+if (tramp->tag != 0) {
+g_io_channel_unref(tramp->chan);
+g_source_remove(tramp->tag);
+}
+
+if (opaque) {
+GIOCondition cond = 0;
+
+tramp->fd_read = fd_read;
+tramp->fd_write = fd_write;
+tramp->opaque = opaque;
+
+if (fd_read) {
+cond |= G_IO_IN | G_IO_ERR;
+}
+
+if (fd_write) {
+cond |= G_IO_OUT | G_IO_ERR;
+}
+
+tramp->chan = g_io_channel_unix_new(fd);
+tramp->tag = g_io_add_watch(tramp->chan, cond, fd_trampoline, tramp);
+}
+
+return 0;
  }

  void qemu_iohandler_fill(int *pnfds, fd_set *readfds, fd_set *writefds, 
fd_set *xfds)



--
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] pc: Clean up PIC-to-APIC IRQ path

[Avi Kivity]

On 09/04/2011 04:41 PM, Anthony Liguori wrote:

See it as you like, but we need the support, not only for device
assigment. And I do not see any gain it hacking this instead of
designing it.



You can design a hack but it's still a hack.

Device state belongs in devices.  Trying to extract device state 
(interrupt routing paths) and externalizing it is by definition a hack.


Pet peeve - saying something is "by definition" a hack is just rhetoric 
unless the definition of device state is "something that cannot be 
extracted and externalized".  Let's avoid this.


In fact it's exactly what we do with the memory API.  Memory routing is 
part of device state, yet we expose it to the memory API and let it do 
its thing instead of going through the hierarchy on every single memory 
access.


Whether it's worthwhile to do this for interrupts as well depends on how 
common the situation is and what we gain from it.  We can only do this 
if the routing is semi-static (depends only on other state but not the 
actual interrupt) - probably the only exception to this is the "least 
priority" mode which means the last leg cannot be computed statically.


I agree the gain is low if we limit ourselves to 440fx, but if we add 
interrupt remapping it becomes considerably more complicated to do this 
backward path computation.


--
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Michael S. Tsirkin]

On Sun, Sep 04, 2011 at 04:55:33PM +0300, Avi Kivity wrote:
> On 09/04/2011 04:41 PM, Michael S. Tsirkin wrote:
> >On Sun, Sep 04, 2011 at 04:05:14PM +0300, Avi Kivity wrote:
> >>  It follows naturally:
> >
> >OK, so it seems the following is more or less what you suggest?
> >I'm not sure I create/destroy subregions properly.
> >Both the alias and the subregion get the same start value?
> 
> Yes (so addresses are not shifted).
> 
> >Is the region name for debugging only?
> 
> For everything except RAM regions (there, the name is also used for
> save/restore).
> 
> >When does priority matter? In case of overlap?
> 
> Yes.  In this case, since overlap resolution is not defined by the
> spec, the actual priority does not matter.
> 
> >@@ -135,6 +135,75 @@ pcibus_t pci_bridge_get_limit(const PCIDevice *bridge, 
> >uint8_t type)
> >  return limit;
> >  }
> >
> >+static pcibus_t pci_bridge_get_size(const PCIDevice *bridge, uint8_t type)
> >+{
> >+return pci_bridge_get_limit(bridge, type)>=
> >+pci_bridge_get_base(bridge, type) ?
> >+pci_bridge_get_limit(bridge, type) -
> >+pci_bridge_get_base(bridge, type)  + 1 : 0;
> >+}
> 
> Correct

To make sure: 0 size is ok?

> but unreadable.

variables will make it clearer.

base = pci_bridge_get_base(bridge, type);
limit = pci_bridge_get_limit(bridge, type);
return limit >= bridge ? limit - base + 1 : 0;


>  Doesn't work for limit == 2^64-1, is this a
> possible value?

You mean when base == 0?  Yes, but what can I do?
This seems to be an API limitation of the memory API:
memory_region_init(sec_bus->address_space_mem, "pci_pridge_pci", INT64_MAX);
and the same for aliases: max size seems to be INT64_MAX,
the last byte isn't covered.

The only way to fix this would be to pass first/last instead
of offset/size. Too late for such an API change?

> >+
> >+static void pci_bridge_region_init(PCIBridge *br)
> >+{
> >+PCIBus *sec_bus =&br->sec_bus;
> >+PCIBus *parent = br->dev.bus;
> >+memory_region_init_alias(sec_bus->alias_pref_mem, "pci_bridge_pref_mem",
> >+ sec_bus->address_space_mem,
> >+ pci_bridge_get_base(&br->dev, PCI_BASE_ADDRESS_MEM_PREFETCH),
> >+ pci_bridge_get_size(&br->dev, PCI_BASE_ADDRESS_MEM_PREFETCH));
> >+memory_region_add_subregion_overlap(parent->address_space_mem,
> >+ pci_bridge_get_base(&br->dev, PCI_BASE_ADDRESS_MEM_PREFETCH),
> >+ sec_bus->alias_pref_mem, 1);
> >+memory_region_init_alias(sec_bus->alias_mem, "pci_bridge_memory",
> >+ sec_bus->address_space_mem,
> >+ pci_bridge_get_base(&br->dev, PCI_BASE_ADDRESS_SPACE_MEMORY),
> >+ pci_bridge_get_size(&br->dev, PCI_BASE_ADDRESS_SPACE_MEMORY));
> >+memory_region_add_subregion_overlap(parent->address_space_mem,
> >+ pci_bridge_get_base(&br->dev, PCI_BASE_ADDRESS_SPACE_MEMORY),
> >+ sec_bus->alias_mem, 1);
> >+memory_region_init_alias(sec_bus->alias_io, "pci_bridge_io",
> >+ sec_bus->address_space_io,
> >+ pci_bridge_get_base(&br->dev, PCI_BASE_ADDRESS_SPACE_IO),
> >+ pci_bridge_get_size(&br->dev, PCI_BASE_ADDRESS_SPACE_IO));
> >+memory_region_add_subregion_overlap(parent->address_space_io,
> >+ pci_bridge_get_base(&br->dev, PCI_BASE_ADDRESS_SPACE_IO),
> >+ sec_bus->alias_io, 1);
> >+}
> 
> This looks right.  Might want to use pci_address_space() instead of
> ->address_space_mem.
> 
> Don't you have to do something similar for the vga window?

Yes.
These are controlled by VGA Enable and VGA palette snooping enable
bits in the bridge, which we don't yet implement. It's on the TODO,
at the moment vga devices behind a bridge don't work.
the relevant bits from the spec:

--

The VGA Enable bit in the Bridge Control register (see Section 3.2.5.18)
is used to control response by the bridge to both the VGA frame buffer
addresses and to the VGA register addresses. When a VGA compatible
device is located downstream of a PCI-to-PCI bridge, the VGA Enable bit
must be set. When set, the bridge will positively decode and forward
memory accesses to VGA frame buffer addresses and I/O accesses to VGA
registers from the primary to secondary interface and block forwarding
of these same accesses from the secondary to primary interface (see
Section 4.5.1).

VGA memory addresses:
0A h through 0B h
VGA I/O addresses (including ISA aliases address - AD[15::10] are not
decoded):
AD[9::0] = 3B0h through 3BBh and 3C0h through 3DFh

We also may need to foward palette snooping accesses:

behaviors of each device type are described below.
The VGA palette addresses are as follows (inclusive of ISA aliases -
AD[15::10] are not decoded):
AD[9::0] = 3C6h, 3C8h, and 3C9h

--


> >+
> >+static void pci_bridge_region_cleanup(PCIBridge *br)
> >+{
> >+PCIBus *sec_bus =&br->sec_bus;
> >+PCIBus *parent = br->dev.bus;
> >+memory_region_del_subregion(parent->address_space_mem,
> >+ 

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Avi Kivity]

On 09/04/2011 05:21 PM, Michael S. Tsirkin wrote:

>  >
>  >+static pcibus_t pci_bridge_get_size(const PCIDevice *bridge, uint8_t type)
>  >+{
>  >+return pci_bridge_get_limit(bridge, type)>=
>  >+pci_bridge_get_base(bridge, type) ?
>  >+pci_bridge_get_limit(bridge, type) -
>  >+pci_bridge_get_base(bridge, type)  + 1 : 0;
>  >+}
>
>  Correct

To make sure: 0 size is ok?


Yes.



>  but unreadable.

variables will make it clearer.

base = pci_bridge_get_base(bridge, type);
limit = pci_bridge_get_limit(bridge, type);
return limit>= bridge ? limit - base + 1 : 0;


So long as we're nitpicking, limit + 1 - base.




>   Doesn't work for limit == 2^64-1, is this a
>  possible value?

You mean when base == 0?  Yes, but what can I do?
This seems to be an API limitation of the memory API:
memory_region_init(sec_bus->address_space_mem, "pci_pridge_pci", INT64_MAX);
and the same for aliases: max size seems to be INT64_MAX,
the last byte isn't covered.


Right.


The only way to fix this would be to pass first/last instead
of offset/size. Too late for such an API change?


Way too late.  And also won't work, since often the offset is determined 
by one party and the size by another.



VGA I/O addresses (including ISA aliases address - AD[15::10] are not
decoded):
AD[9::0] = 3B0h through 3BBh and 3C0h through 3DFh


These "not decoded" bits mean you need to instantiate tons of aliases to 
implement correctly.


I plan to add core support for that eventually.


--
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] pc: Clean up PIC-to-APIC IRQ path

[Anthony Liguori]

On 09/04/2011 08:57 AM, Anthony Liguori wrote:

On 09/04/2011 08:49 AM, Jan Kiszka wrote:

On 2011-09-04 15:41, Anthony Liguori wrote:

On 09/04/2011 08:36 AM, Jan Kiszka wrote:
Having some sort of global interrupt routing table is just going to add
a layer of complexity for very little obvious gain.


It's not yet decided how the problem is solved. A global interrupt
matrix is just one proposal, another option is to extend the pin model
in a way that supports routing change notifiers and backward polling.


If that's all you need, then you really just want notification on socket
changes. Backwards polling can be achieved by just adding state to the
Pin (which I full heartedly support).

If that's all you're proposing, than I'm entirely happy with it :-)


It's not that simple.

Routing paths can change because of device changes, not just socket changes.

I think you would need an interface for irq routing.  Something like:

struct IrqRouter {
Interface parent;

void (*foreach_output)(IrqRouter *obj,
   void (*fn)(const char *out, void *opaque),
   void *opaque);

void (*foreach_input)(IrqRouter *obj,
  void (*fn)(const char *in, void *opaque),
  void *opaque);

const char *(*get_mapping)(IrqRouter *obj, const char *in);
};

You could then implement this interface in I440FX or any other 
controller where we want to be able to support device passthrough.


Representing endpoints as strings means that you can correlate inputs to 
outputs throughout the chain provided that you understand how 
inputs/outputs relate to plugs/sockets.


I think this has the property of letting you write reasonably generic 
code to discover routing while only having to add complexity to the bare 
minimum set of devices to enable device passthrough.


It's basically what I suggested for PCI INTx mapping but a little more 
generic as an interface so that it can extend to other types of devices.


Regards,

Anthony Liguori


I'm happy with that because all of the route detection logic can live
outside of the devices which at least contains the complexity.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH] pc: Clean up PIC-to-APIC IRQ path

[Anthony Liguori]

On 09/04/2011 09:12 AM, Avi Kivity wrote:

On 09/04/2011 04:41 PM, Anthony Liguori wrote:

See it as you like, but we need the support, not only for device
assigment. And I do not see any gain it hacking this instead of
designing it.



You can design a hack but it's still a hack.

Device state belongs in devices. Trying to extract device state
(interrupt routing paths) and externalizing it is by definition a hack.


Pet peeve - saying something is "by definition" a hack is just rhetoric
unless the definition of device state is "something that cannot be
extracted and externalized". Let's avoid this.


Likewise, I would prefer to avoid stating that something is a hard 
requirement in the absence of concrete use-cases.


I do think the definition of device state is more or less something that 
is opaque and owned by the device.  The more we externalize the device 
state, the more we have to deal with compatibility (even if it's just 
internal compatibility).  This makes modularity hard because there's too 
many things with complex dependencies.



In fact it's exactly what we do with the memory API. Memory routing is
part of device state, yet we expose it to the memory API and let it do
its thing instead of going through the hierarchy on every single memory
access.


Yes, and the memory API is complicated and invasive :-)  But it's worth 
it at the end of the day (although I think it could be simplified at the 
expensive of not allowing as much flattening).



Whether it's worthwhile to do this for interrupts as well depends on how
common the situation is and what we gain from it. We can only do this if
the routing is semi-static (depends only on other state but not the
actual interrupt) - probably the only exception to this is the "least
priority" mode which means the last leg cannot be computed statically.

I agree the gain is low if we limit ourselves to 440fx, but if we add
interrupt remapping it becomes considerably more complicated to do this
backward path computation.


If we start with a simple interface that works for the i440fx and then 
expand it as we support more layers, I think we'll be okay.


What I'm concerned about is an attempt to globally track IRQ routing.  I 
can imagine constructing a board where you have two simple devices that 
have level triggered interrupts and wanting to tie them to a single 
input pin.  A simple OR gate would be sufficient to do this.  Having to 
make an OR gate an IRQ router seems like far too much complexity to me.


I think we need to strive to keep simple things simple.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH] pc: Clean up PIC-to-APIC IRQ path

[Anthony Liguori]

On 09/03/2011 04:01 PM, Blue Swirl wrote:

On Sat, Sep 3, 2011 at 7:53 PM, Anthony Liguori  wrote:

On 08/31/2011 11:59 AM, Blue Swirl wrote:


On Wed, Aug 31, 2011 at 8:28 AM, Avi Kivitywrote:


On 08/30/2011 10:19 PM, Blue Swirl wrote:




  We need some kind of two phase restore. In the first phase all state
is
  restored; since some of that state drivers outputs that are input to
other
  devices, they may experience an edge, and we need to supress that.  In
the
  second phase edge detection is unsupressed and the device goes live.


No. Devices may not perform any externally visible activities (like
toggle a qemu_irq) during or after load because 1) qemu_irq is
stateless and 2) since the receiving end is also freshly loaded, both
states are already in synch without any calls or toggling.


That makes it impossible to migrate level-triggered irq lines.  Or at
least,
the receiver has to remember the state, instead of (or in addition to)
the
sender.


Both ends probably need to remember the state. That should work
without any multiphase restores and transient suppressors.

It might be also possible to introduce stateful signal lines which
save and restore their state, then the receiving end could check what
is the current level. However, if you consider that the devices may be
restored in random order, if the IRQ line device happens to be
restored later, the receiver would still get wrong information. Adding
priorities could solve this, but I think stateless IRQs are the only
sane way.


We shouldn't really use the term IRQ as it's confusing.  I like the term
"pin" better because that describes what we're really talking about.

qemu_irq is designed oddly today because is represents something that is
intrinsically state (whether a pin is high or low) with an edge notification
with the assumption that the state is held somewhere else (which is usually
true).

Modelling stateful pins is useful though for doing something like
introspecting pin levels, supporting live migration, etc.

The way this works in QOM right now is that the Pin object has a level state
that can be queried but it also has the ability to register for
notifications on level change.

The edge change signal isn't registered until realize.  This means that you
can connect all of the device models, restore all of the pin states, and
then realize the device model all at once.  At the point of realize, all of
the devices have the right pin levels so each device can add their edge
change signals and read the incoming pins and respond accordingly.


Even if the devices read the input pins on restore, they shouldn't
make any changes to their output pins because that would propagate to
other devices. To handle this in non-chaotic way would need hacks to
each device, multiphase stuff, priorities or transient suppressors.
 From the device point of view, restoring is not a state change and no
edges should be seen at that moment.


A device wouldn't get a signal about an irq edge at realize.  Restoring 
an arbitrarily complex device model wouldn't result in any irq edge 
notifications because all of the devices are created and their state is 
set before any device is realized.  Since an edge event only occurs when 
the state changes of an IRQ after realize, no edge events will happen.


Whether a device reads the level of an IRQ during realize though depends 
on the device.  I don't think there's any strong reason to but I also 
don't think it's fundamentally wrong for a device to read an IRQ level 
at realize.


Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 2/2] main: switch qemu_set_fd_handler to g_io_add_watch

[Anthony Liguori]

On 09/04/2011 09:03 AM, Avi Kivity wrote:

On 08/22/2011 04:12 PM, Anthony Liguori wrote:

This patch changes qemu_set_fd_handler to be implemented in terms of
g_io_add_watch(). The semantics are a bit different so some glue is
required.

qemu_set_fd_handler2 is much harder to convert because of its use of
polling.

The glib main loop has the major of advantage of having a proven
thread safe
architecture. By using the glib main loop instead of our own, it will
allow us
to eventually introduce multiple I/O threads.

I'm pretty sure that this will work on Win32, but I would appreciate
some help
testing. I think the semantics of g_io_channel_unix_new() are really
just tied
to the notion of a "unix fd" and not necessarily unix itself.


'git bisect' fingered this as responsible for breaking
qcow2+cache=unsafe. I think there's an off-by-one here and the guilty
patch is the one that switches the main loop, but that's just a guess.


Semantically, this patch changes when a file descriptor callback is issued.

Does the following make a difference:

diff --git a/vl.c b/vl.c
index 5ba9b35..02f694d 100644
--- a/vl.c
+++ b/vl.c
@@ -1432,9 +1432,9 @@ int main_loop_wait(int nonblocking)
 qemu_mutex_lock_iothread();
 }

+glib_select_poll(&rfds, &wfds, &xfds, (ret < 0));
 qemu_iohandler_poll(&rfds, &wfds, &xfds, ret);
 slirp_select_poll(&rfds, &wfds, &xfds, (ret < 0));
-glib_select_poll(&rfds, &wfds, &xfds, (ret < 0));

 qemu_run_all_timers();

What type of guest install is it that fails for you and is this 
qemu-kvm.git or qemu.git?


Regards,

Anthony Liguori



The symptoms are that a guest that is restarted (new qemu process) after
install doesn't make it through grub - some image data didn't make it do
disk. With qcow2 and cache=unsafe that can easily happen through exit
notifiers not being run and the entire qcow2 metadata being thrown out
the window. Running with raw+cache=unsafe works.




diff --git a/iohandler.c b/iohandler.c
index 4deae1e..5ef66fb 100644
--- a/iohandler.c
+++ b/iohandler.c
@@ -80,12 +80,67 @@ int qemu_set_fd_handler2(int fd,
return 0;
}

+typedef struct IOTrampoline
+{
+ GIOChannel *chan;
+ IOHandler *fd_read;
+ IOHandler *fd_write;
+ void *opaque;
+ guint tag;
+} IOTrampoline;
+
+static gboolean fd_trampoline(GIOChannel *chan, GIOCondition cond,
gpointer opaque)
+{
+ IOTrampoline *tramp = opaque;
+
+ if (tramp->opaque == NULL) {
+ return FALSE;
+ }
+
+ if ((cond& G_IO_IN)&& tramp->fd_read) {
+ tramp->fd_read(tramp->opaque);
+ }
+
+ if ((cond& G_IO_OUT)&& tramp->fd_write) {
+ tramp->fd_write(tramp->opaque);
+ }
+
+ return TRUE;
+}
+
int qemu_set_fd_handler(int fd,
IOHandler *fd_read,
IOHandler *fd_write,
void *opaque)
{
- return qemu_set_fd_handler2(fd, NULL, fd_read, fd_write, opaque);
+ static IOTrampoline fd_trampolines[FD_SETSIZE];
+ IOTrampoline *tramp =&fd_trampolines[fd];
+
+ if (tramp->tag != 0) {
+ g_io_channel_unref(tramp->chan);
+ g_source_remove(tramp->tag);
+ }
+
+ if (opaque) {
+ GIOCondition cond = 0;
+
+ tramp->fd_read = fd_read;
+ tramp->fd_write = fd_write;
+ tramp->opaque = opaque;
+
+ if (fd_read) {
+ cond |= G_IO_IN | G_IO_ERR;
+ }
+
+ if (fd_write) {
+ cond |= G_IO_OUT | G_IO_ERR;
+ }
+
+ tramp->chan = g_io_channel_unix_new(fd);
+ tramp->tag = g_io_add_watch(tramp->chan, cond, fd_trampoline, tramp);
+ }
+
+ return 0;
}

void qemu_iohandler_fill(int *pnfds, fd_set *readfds, fd_set
*writefds, fd_set *xfds)

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Michael S. Tsirkin]

On Sun, Sep 04, 2011 at 05:36:56PM +0300, Avi Kivity wrote:
> On 09/04/2011 05:21 PM, Michael S. Tsirkin wrote:
> >>  >
> >>  >+static pcibus_t pci_bridge_get_size(const PCIDevice *bridge, uint8_t 
> >> type)
> >>  >+{
> >>  >+return pci_bridge_get_limit(bridge, type)>=
> >>  >+pci_bridge_get_base(bridge, type) ?
> >>  >+pci_bridge_get_limit(bridge, type) -
> >>  >+pci_bridge_get_base(bridge, type)  + 1 : 0;
> >>  >+}
> >>
> >>  Correct
> >
> >To make sure: 0 size is ok?
> 
> Yes.
> 
> >
> >>  but unreadable.
> >
> >variables will make it clearer.
> >
> >base = pci_bridge_get_base(bridge, type);
> >limit = pci_bridge_get_limit(bridge, type);
> >return limit>= bridge ? limit - base + 1 : 0;
> 
> So long as we're nitpicking, limit + 1 - base.

If this makes you happier, OK :)

> >
> >
> >>   Doesn't work for limit == 2^64-1, is this a
> >>  possible value?
> >
> >You mean when base == 0?  Yes, but what can I do?
> >This seems to be an API limitation of the memory API:
> >memory_region_init(sec_bus->address_space_mem, "pci_pridge_pci", INT64_MAX);
> >and the same for aliases: max size seems to be INT64_MAX,
> >the last byte isn't covered.
> 
> Right.
> 
> >The only way to fix this would be to pass first/last instead
> >of offset/size. Too late for such an API change?
> 
> Way too late.  And also won't work, since often the offset is
> determined by one party and the size by another.

For things like BARs, yes - but these don't need to be
that big normally. We could add an additinal API
that gets first/last parameters. last < first means 0 size.
Feasible?

> >VGA I/O addresses (including ISA aliases address - AD[15::10] are not
> >decoded):
> >AD[9::0] = 3B0h through 3BBh and 3C0h through 3DFh
> 
> These "not decoded" bits mean you need to instantiate tons of
> aliases to implement correctly.
> I plan to add core support for that eventually.

There's a flag to enable 16-bit decode actually:
bit 4 in bridge control register.
How does VGA work at the moment without a bridge? Ignores the ISA aliases?
then we can do that too I think.

> -- 
> error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 2/2] main: switch qemu_set_fd_handler to g_io_add_watch

[Anthony Liguori]

On 09/04/2011 09:03 AM, Avi Kivity wrote:

On 08/22/2011 04:12 PM, Anthony Liguori wrote:

This patch changes qemu_set_fd_handler to be implemented in terms of
g_io_add_watch(). The semantics are a bit different so some glue is
required.

qemu_set_fd_handler2 is much harder to convert because of its use of
polling.

The glib main loop has the major of advantage of having a proven
thread safe
architecture. By using the glib main loop instead of our own, it will
allow us
to eventually introduce multiple I/O threads.

I'm pretty sure that this will work on Win32, but I would appreciate
some help
testing. I think the semantics of g_io_channel_unix_new() are really
just tied
to the notion of a "unix fd" and not necessarily unix itself.


'git bisect' fingered this as responsible for breaking
qcow2+cache=unsafe. I think there's an off-by-one here and the guilty
patch is the one that switches the main loop, but that's just a guess.

The symptoms are that a guest that is restarted (new qemu process) after
install doesn't make it through grub - some image data didn't make it do
disk. With qcow2 and cache=unsafe that can easily happen through exit
notifiers not being run and the entire qcow2 metadata being thrown out
the window. Running with raw+cache=unsafe works.


Can you share your full command line?

Nothing that would be in the obvious path actually uses 
qemu_set_fd_handler...


Regards,

Anthony Liguori






diff --git a/iohandler.c b/iohandler.c
index 4deae1e..5ef66fb 100644
--- a/iohandler.c
+++ b/iohandler.c
@@ -80,12 +80,67 @@ int qemu_set_fd_handler2(int fd,
return 0;
}

+typedef struct IOTrampoline
+{
+ GIOChannel *chan;
+ IOHandler *fd_read;
+ IOHandler *fd_write;
+ void *opaque;
+ guint tag;
+} IOTrampoline;
+
+static gboolean fd_trampoline(GIOChannel *chan, GIOCondition cond,
gpointer opaque)
+{
+ IOTrampoline *tramp = opaque;
+
+ if (tramp->opaque == NULL) {
+ return FALSE;
+ }
+
+ if ((cond& G_IO_IN)&& tramp->fd_read) {
+ tramp->fd_read(tramp->opaque);
+ }
+
+ if ((cond& G_IO_OUT)&& tramp->fd_write) {
+ tramp->fd_write(tramp->opaque);
+ }
+
+ return TRUE;
+}
+
int qemu_set_fd_handler(int fd,
IOHandler *fd_read,
IOHandler *fd_write,
void *opaque)
{
- return qemu_set_fd_handler2(fd, NULL, fd_read, fd_write, opaque);
+ static IOTrampoline fd_trampolines[FD_SETSIZE];
+ IOTrampoline *tramp =&fd_trampolines[fd];
+
+ if (tramp->tag != 0) {
+ g_io_channel_unref(tramp->chan);
+ g_source_remove(tramp->tag);
+ }
+
+ if (opaque) {
+ GIOCondition cond = 0;
+
+ tramp->fd_read = fd_read;
+ tramp->fd_write = fd_write;
+ tramp->opaque = opaque;
+
+ if (fd_read) {
+ cond |= G_IO_IN | G_IO_ERR;
+ }
+
+ if (fd_write) {
+ cond |= G_IO_OUT | G_IO_ERR;
+ }
+
+ tramp->chan = g_io_channel_unix_new(fd);
+ tramp->tag = g_io_add_watch(tramp->chan, cond, fd_trampoline, tramp);
+ }
+
+ return 0;
}

void qemu_iohandler_fill(int *pnfds, fd_set *readfds, fd_set
*writefds, fd_set *xfds)

Re: [Qemu-devel] [PATCH] pc: Clean up PIC-to-APIC IRQ path

[Avi Kivity]

On 09/04/2011 05:43 PM, Anthony Liguori wrote:

Pet peeve - saying something is "by definition" a hack is just rhetoric
unless the definition of device state is "something that cannot be
extracted and externalized". Let's avoid this.



Likewise, I would prefer to avoid stating that something is a hard 
requirement in the absence of concrete use-cases.


Agree.  But let's not fight rhetoric with rhetoric.

I also agree that if it were just the 440fx, then we could have a 
private channel between device assignment and the ioapic.  But if there 
are more use cases, it calls for a generic solution.


I do think the definition of device state is more or less something 
that is opaque and owned by the device.  The more we externalize the 
device state, the more we have to deal with compatibility (even if 
it's just internal compatibility).  This makes modularity hard because 
there's too many things with complex dependencies.


That depends on the number of special cases we'll see.  I really have no 
insight here.





In fact it's exactly what we do with the memory API. Memory routing is
part of device state, yet we expose it to the memory API and let it do
its thing instead of going through the hierarchy on every single memory
access.


Yes, and the memory API is complicated and invasive :-)  But it's 
worth it at the end of the day (although I think it could be 
simplified at the expensive of not allowing as much flattening).


(we should have spent a few hours at kf2011 to convince you that this is 
impossible)




What I'm concerned about is an attempt to globally track IRQ routing.  
I can imagine constructing a board where you have two simple devices 
that have level triggered interrupts and wanting to tie them to a 
single input pin.  A simple OR gate would be sufficient to do this.  
Having to make an OR gate an IRQ router seems like far too much 
complexity to me.


Depends on the API.  If Pin has a method pin_add_tristate_input(), then 
it becomes trivial.



I think we need to strive to keep simple things simple.


Too late.

--
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Avi Kivity]

On 09/04/2011 05:54 PM, Michael S. Tsirkin wrote:

>  Way too late.  And also won't work, since often the offset is
>  determined by one party and the size by another.

For things like BARs, yes - but these don't need to be
that big normally. We could add an additinal API
that gets first/last parameters. last<  first means 0 size.
Feasible?


Let's defer this for now.

Does PCI actually have 64-bit addresses?  Note that most/all cpus don't.

We may need 65-bit arithmetic for that.



>  >VGA I/O addresses (including ISA aliases address - AD[15::10] are not
>  >decoded):
>  >AD[9::0] = 3B0h through 3BBh and 3C0h through 3DFh
>
>  These "not decoded" bits mean you need to instantiate tons of
>  aliases to implement correctly.
>  I plan to add core support for that eventually.

There's a flag to enable 16-bit decode actually:
bit 4 in bridge control register.
How does VGA work at the moment without a bridge? Ignores the ISA aliases?
then we can do that too I think.



Of course it doesn't ignore it.  See the 440fx implementation, if you 
disable VGA access (via the SMRAM register), vga goes away.


(vga registers its legacy space as a subregion of pci_address_space(dev))

--
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] pc: Clean up PIC-to-APIC IRQ path

[Anthony Liguori]

On 09/04/2011 10:03 AM, Avi Kivity wrote:

On 09/04/2011 05:43 PM, Anthony Liguori wrote:

In fact it's exactly what we do with the memory API. Memory routing is
part of device state, yet we expose it to the memory API and let it do
its thing instead of going through the hierarchy on every single memory
access.


Yes, and the memory API is complicated and invasive :-) But it's worth
it at the end of the day (although I think it could be simplified at
the expensive of not allowing as much flattening).


(we should have spent a few hours at kf2011 to convince you that this is
impossible)


I don't mean for RAM, but for device I/O.

Instead of implementing it_shift in the core API, you could implement 
it_shift by having a device that takes an input MemoryRegion and an 
output MemoryRegion and implements the it_shift logic.


I think endianness could also be handled this way too.

For stuff like coalesced I/O and eventfds, I understand the difficulties.





What I'm concerned about is an attempt to globally track IRQ routing.
I can imagine constructing a board where you have two simple devices
that have level triggered interrupts and wanting to tie them to a
single input pin. A simple OR gate would be sufficient to do this.
Having to make an OR gate an IRQ router seems like far too much
complexity to me.


Depends on the API. If Pin has a method pin_add_tristate_input(), then
it becomes trivial.


See my self-reply to Jan.  I think there's a reasonable middle ground 
using an IrqRouter interface.


Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH] pc: Clean up PIC-to-APIC IRQ path

[Blue Swirl]

On Sun, Sep 4, 2011 at 2:37 PM, Anthony Liguori  wrote:
> On 09/04/2011 08:57 AM, Anthony Liguori wrote:
>>
>> On 09/04/2011 08:49 AM, Jan Kiszka wrote:
>>>
>>> On 2011-09-04 15:41, Anthony Liguori wrote:

 On 09/04/2011 08:36 AM, Jan Kiszka wrote:
 Having some sort of global interrupt routing table is just going to add
 a layer of complexity for very little obvious gain.
>>>
>>> It's not yet decided how the problem is solved. A global interrupt
>>> matrix is just one proposal, another option is to extend the pin model
>>> in a way that supports routing change notifiers and backward polling.
>>
>> If that's all you need, then you really just want notification on socket
>> changes. Backwards polling can be achieved by just adding state to the
>> Pin (which I full heartedly support).
>>
>> If that's all you're proposing, than I'm entirely happy with it :-)
>
> It's not that simple.
>
> Routing paths can change because of device changes, not just socket changes.

Yes, that's why callbacks are needed to let the device inform global matrix.

> I think you would need an interface for irq routing.  Something like:
>
> struct IrqRouter {
>    Interface parent;
>
>    void (*foreach_output)(IrqRouter *obj,
>                           void (*fn)(const char *out, void *opaque),
>                           void *opaque);
>
>    void (*foreach_input)(IrqRouter *obj,
>                          void (*fn)(const char *in, void *opaque),
>                          void *opaque);

Are the above a way for a parent to tell this device that its inputs
are changed?

>    const char *(*get_mapping)(IrqRouter *obj, const char *in);

This would be useful for the matrix too, the matrix would use this to
query for initial routing, maybe also for later changes if the change
callback didn't tell the mapping directly.

> };
>
> You could then implement this interface in I440FX or any other controller
> where we want to be able to support device passthrough.
>
> Representing endpoints as strings means that you can correlate inputs to
> outputs throughout the chain provided that you understand how inputs/outputs
> relate to plugs/sockets.

The string format assumes that there is a reliable way to convert Pin
to string and vice versa. Why can't they be array of pointers to Pins?

String representation (e.g. "/i440fx/pci-bridge@2,0/serial@3fc:2")
could be useful for debugging and 'info qtree' ('info irqtree'?)
though.

> I think this has the property of letting you write reasonably generic code
> to discover routing while only having to add complexity to the bare minimum
> set of devices to enable device passthrough.
>
> It's basically what I suggested for PCI INTx mapping but a little more
> generic as an interface so that it can extend to other types of devices.
>
> Regards,
>
> Anthony Liguori
>
>> I'm happy with that because all of the route detection logic can live
>> outside of the devices which at least contains the complexity.
>>
>> Regards,
>>
>> Anthony Liguori
>
>

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Michael S. Tsirkin]

On Sun, Sep 04, 2011 at 06:14:22PM +0300, Avi Kivity wrote:
> On 09/04/2011 05:54 PM, Michael S. Tsirkin wrote:
> >>  Way too late.  And also won't work, since often the offset is
> >>  determined by one party and the size by another.
> >
> >For things like BARs, yes - but these don't need to be
> >that big normally. We could add an additinal API
> >that gets first/last parameters. last<  first means 0 size.
> >Feasible?
> 
> Let's defer this for now.
> 
> Does PCI actually have 64-bit addresses?

Yes.

>  Note that most/all cpus don't.
> We may need 65-bit arithmetic for that.

Jut using start/limit carefully should be enough ...


> >
> >>  >VGA I/O addresses (including ISA aliases address - AD[15::10] are not
> >>  >decoded):
> >>  >AD[9::0] = 3B0h through 3BBh and 3C0h through 3DFh
> >>
> >>  These "not decoded" bits mean you need to instantiate tons of
> >>  aliases to implement correctly.
> >>  I plan to add core support for that eventually.
> >
> >There's a flag to enable 16-bit decode actually:
> >bit 4 in bridge control register.
> >How does VGA work at the moment without a bridge? Ignores the ISA aliases?
> >then we can do that too I think.
> >
> 
> Of course it doesn't ignore it.  See the 440fx implementation, if
> you disable VGA access (via the SMRAM register), vga goes away.

Yes but that's for VGA RAM, right? I'm talking about the IO addresses:
are tons of aliases created as you suggest?

> (vga registers its legacy space as a subregion of pci_address_space(dev))
> 
> -- 
> error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Michael S. Tsirkin]

On Sun, Sep 04, 2011 at 05:36:56PM +0300, Avi Kivity wrote:
> So long as we're nitpicking ...

OK, this should do it then.


diff --git a/hw/pci.c b/hw/pci.c
index 57ff7b1..56dfa18 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -889,7 +889,6 @@ void pci_register_bar(PCIDevice *pci_dev, int region_num,
 r = &pci_dev->io_regions[region_num];
 r->addr = PCI_BAR_UNMAPPED;
 r->size = size;
-r->filtered_size = size;
 r->type = type;
 r->memory = NULL;
 
@@ -920,41 +919,6 @@ pcibus_t pci_get_bar_addr(PCIDevice *pci_dev, int 
region_num)
 return pci_dev->io_regions[region_num].addr;
 }
 
-static void pci_bridge_filter(PCIDevice *d, pcibus_t *addr, pcibus_t *size,
-  uint8_t type)
-{
-pcibus_t base = *addr;
-pcibus_t limit = *addr + *size - 1;
-PCIDevice *br;
-
-for (br = d->bus->parent_dev; br; br = br->bus->parent_dev) {
-uint16_t cmd = pci_get_word(d->config + PCI_COMMAND);
-
-if (type & PCI_BASE_ADDRESS_SPACE_IO) {
-if (!(cmd & PCI_COMMAND_IO)) {
-goto no_map;
-}
-} else {
-if (!(cmd & PCI_COMMAND_MEMORY)) {
-goto no_map;
-}
-}
-
-base = MAX(base, pci_bridge_get_base(br, type));
-limit = MIN(limit, pci_bridge_get_limit(br, type));
-}
-
-if (base > limit) {
-goto no_map;
-}
-*addr = base;
-*size = limit - base + 1;
-return;
-no_map:
-*addr = PCI_BAR_UNMAPPED;
-*size = 0;
-}
-
 static pcibus_t pci_bar_address(PCIDevice *d,
int reg, uint8_t type, pcibus_t size)
 {
@@ -1024,7 +988,7 @@ static void pci_update_mappings(PCIDevice *d)
 {
 PCIIORegion *r;
 int i;
-pcibus_t new_addr, filtered_size;
+pcibus_t new_addr;
 
 for(i = 0; i < PCI_NUM_REGIONS; i++) {
 r = &d->io_regions[i];
@@ -1035,14 +999,8 @@ static void pci_update_mappings(PCIDevice *d)
 
 new_addr = pci_bar_address(d, i, r->type, r->size);
 
-/* bridge filtering */
-filtered_size = r->size;
-if (new_addr != PCI_BAR_UNMAPPED) {
-pci_bridge_filter(d, &new_addr, &filtered_size, r->type);
-}
-
 /* This bar isn't changed */
-if (new_addr == r->addr && filtered_size == r->filtered_size)
+if (new_addr == r->addr)
 continue;
 
 /* now do the real mapping */
@@ -1050,15 +1008,7 @@ static void pci_update_mappings(PCIDevice *d)
 memory_region_del_subregion(r->address_space, r->memory);
 }
 r->addr = new_addr;
-r->filtered_size = filtered_size;
 if (r->addr != PCI_BAR_UNMAPPED) {
-/*
- * TODO: currently almost all the map funcions assumes
- * filtered_size == size and addr & ~(size - 1) == addr.
- * However with bridge filtering, they aren't always true.
- * Teach them such cases, such that filtered_size < size and
- * addr & (size - 1) != 0.
- */
 if (r->type & PCI_BASE_ADDRESS_SPACE_IO) {
 memory_region_add_subregion_overlap(r->address_space,
 r->addr,
@@ -1576,22 +1526,6 @@ PCIDevice *pci_nic_init_nofail(NICInfo *nd, const char 
*default_model,
 return res;
 }
 
-static void pci_bridge_update_mappings_fn(PCIBus *b, PCIDevice *d)
-{
-pci_update_mappings(d);
-}
-
-void pci_bridge_update_mappings(PCIBus *b)
-{
-PCIBus *child;
-
-pci_for_each_device_under_bus(b, pci_bridge_update_mappings_fn);
-
-QLIST_FOREACH(child, &b->child, sibling) {
-pci_bridge_update_mappings(child);
-}
-}
-
 /* Whether a given bus number is in range of the secondary
  * bus of the given bridge device. */
 static bool pci_secondary_bus_in_range(PCIDevice *dev, int bus_num)
diff --git a/hw/pci.h b/hw/pci.h
index 391217e..65e1568 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -90,7 +90,6 @@ typedef struct PCIIORegion {
 pcibus_t addr; /* current PCI mapping address. -1 means not mapped */
 #define PCI_BAR_UNMAPPED (~(pcibus_t)0)
 pcibus_t size;
-pcibus_t filtered_size;
 uint8_t type;
 MemoryRegion *memory;
 MemoryRegion *address_space;
@@ -277,7 +276,6 @@ int pci_read_devaddr(Monitor *mon, const char *addr, int 
*domp, int *busp,
 
 void do_pci_info_print(Monitor *mon, const QObject *data);
 void do_pci_info(Monitor *mon, QObject **ret_data);
-void pci_bridge_update_mappings(PCIBus *b);
 
 void pci_device_deassert_intx(PCIDevice *dev);
 
diff --git a/hw/pci_bridge.c b/hw/pci_bridge.c
index e0b339e..bedd615 100644
--- a/hw/pci_bridge.c
+++ b/hw/pci_bridge.c
@@ -135,6 +135,72 @@ pcibus_t pci_bridge_get_limit(const PCIDevice *bridge, 
uint8_t type)
 return limit;
 }
 
+static void pci_bridge_init_alias(PCIBridge *bridge, MemoryRegion *alias,
+  uint8_t type, const char *name,
+  MemoryRegion *space,
+

Re: [Qemu-devel] [PATCH] pc: Clean up PIC-to-APIC IRQ path

[Blue Swirl]

On Sun, Sep 4, 2011 at 2:43 PM, Anthony Liguori  wrote:
> On 09/04/2011 09:12 AM, Avi Kivity wrote:
>>
>> On 09/04/2011 04:41 PM, Anthony Liguori wrote:

 See it as you like, but we need the support, not only for device
 assigment. And I do not see any gain it hacking this instead of
 designing it.
>>>
>>>
>>> You can design a hack but it's still a hack.
>>>
>>> Device state belongs in devices. Trying to extract device state
>>> (interrupt routing paths) and externalizing it is by definition a hack.
>>
>> Pet peeve - saying something is "by definition" a hack is just rhetoric
>> unless the definition of device state is "something that cannot be
>> extracted and externalized". Let's avoid this.
>
> Likewise, I would prefer to avoid stating that something is a hard
> requirement in the absence of concrete use-cases.
>
> I do think the definition of device state is more or less something that is
> opaque and owned by the device.  The more we externalize the device state,
> the more we have to deal with compatibility (even if it's just internal
> compatibility).  This makes modularity hard because there's too many things
> with complex dependencies.
>
>> In fact it's exactly what we do with the memory API. Memory routing is
>> part of device state, yet we expose it to the memory API and let it do
>> its thing instead of going through the hierarchy on every single memory
>> access.
>
> Yes, and the memory API is complicated and invasive :-)  But it's worth it
> at the end of the day (although I think it could be simplified at the
> expensive of not allowing as much flattening).
>
>> Whether it's worthwhile to do this for interrupts as well depends on how
>> common the situation is and what we gain from it. We can only do this if
>> the routing is semi-static (depends only on other state but not the
>> actual interrupt) - probably the only exception to this is the "least
>> priority" mode which means the last leg cannot be computed statically.
>>
>> I agree the gain is low if we limit ourselves to 440fx, but if we add
>> interrupt remapping it becomes considerably more complicated to do this
>> backward path computation.
>
> If we start with a simple interface that works for the i440fx and then
> expand it as we support more layers, I think we'll be okay.
>
> What I'm concerned about is an attempt to globally track IRQ routing.  I can
> imagine constructing a board where you have two simple devices that have
> level triggered interrupts and wanting to tie them to a single input pin.  A
> simple OR gate would be sufficient to do this.  Having to make an OR gate an
> IRQ router seems like far too much complexity to me.

For the simple OR gate this is a downside of course, but I don't see a
way to avoid it, same with the memory API or other APIs that need to
cater for wide variety of needs.

> I think we need to strive to keep simple things simple.

If the APIs need only minor changes, the changes to the devices would
not be so big. At this point I think your router API would only need
the change callback to support the matrix and Pin should be
compatible.

> Regards,
>
> Anthony Liguori
>
>

Re: [Qemu-devel] [PATCH] Fix comment (install patch -> install path)

[Stefan Hajnoczi]

On Sun, Sep 04, 2011 at 03:17:46PM +0200, Stefan Weil wrote:
> Signed-off-by: Stefan Weil 
> ---
>  vl.c |2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)

Thanks, applied to the trivial patches -next tree:
http://repo.or.cz/w/qemu/stefanha.git/shortlog/refs/heads/trivial-patches-next

Stefan

Re: [Qemu-devel] [PATCH] configure: Remove relicts from --enable-io-thread

[Stefan Hajnoczi]

On Sun, Sep 04, 2011 at 03:29:32PM +0200, Stefan Weil wrote:
> Commit 12d4536f7d911b6d87a766ad7300482ea663cea2 removed
> configure option --enable-io-thread.
> 
> Remove help message which is now no longer valid.
> 
> Cc: Anthony Liguori 
> Signed-off-by: Stefan Weil 
> ---
>  configure |1 -
>  1 files changed, 0 insertions(+), 1 deletions(-)

Thanks, applied to the trivial patches -next tree:
http://repo.or.cz/w/qemu/stefanha.git/shortlog/refs/heads/trivial-patches-next

Stefan

[Qemu-devel] [PULL] Memory API batch 5, v2

[Avi Kivity]

Please pull from

  git://github.com/avikivity/qemu.git memory/batch

v2: just a rebase to make sure bisects see the rom_device fix.

Avi Kivity (22):
  mips_fulong2e: convert to memory API
  stellaris_enet: convert to memory API
  sysbus: add helpers to add and delete memory regions to the 
system bus

  pci_host: convert conf index and data ports to memory API
  ReadWriteHandler: remove
  an5206: convert to memory API
  armv7m: convert to memory API
  axis_dev88: convert to memory API (RAM only)
  sysbus: add sysbus_add_memory_overlap()
  integratorcp: convert to memory API (RAM/flash only)
  leon3: convert to memory API
  cirrus: wrap memory update in a transaction
  piix_pci: wrap memory update in a transaction
  Makefile.hw: allow hw/ files to include glib headers
  pflash_cfi01/pflash_cfi02: convert to memory API
  dummy_m68k: convert to memory API
  lm32_boards: convert to memory API
  mainstone: convert to memory API
  mcf5208: convert to memory API
  milkymist-minimac2: convert to memory API
  milkymist-softusb: convert to memory API
  milkymist: convert to memory API

 Makefile.hw   |1 +
 Makefile.target   |1 -
 hw/an5206.c   |   12 +++--
 hw/arm-misc.h |5 ++-
 hw/armv7m.c   |   22 +
 hw/axis_dev88.c   |   16 +++---
 hw/cirrus_vga.c   |2 +
 hw/collie.c   |7 +--
 hw/dec_pci.c  |   13 +++---
 hw/dummy_m68k.c   |7 ++-
 hw/flash.h|   13 +-
 hw/grackle_pci.c  |   13 +++---
 hw/gumstix.c  |6 +--
 hw/integratorcp.c |   28 +---
 hw/leon3.c|   15 ---
 hw/lm32_boards.c  |   23 +-
 hw/mainstone.c|   20 +
 hw/mcf5208.c  |   72 ++-
 hw/milkymist-minimac2.c   |   43 +-
 hw/milkymist-softusb.c|   48 ++--
 hw/milkymist.c|   13 +++---
 hw/mips_fulong2e.c|   17 ---
 hw/mips_malta.c   |   54 +++
 hw/mips_r4k.c |   13 +++---
 hw/musicpal.c |8 ++--
 hw/omap_sx1.c |8 ++--
 hw/pci_host.c |   86 -
 hw/pci_host.h |   16 +++
 hw/petalogix_ml605_mmu.c  |5 +-
 hw/petalogix_s3adsp1800_mmu.c |5 +-
 hw/pflash_cfi01.c |   78 ++---
 hw/pflash_cfi02.c |   95 
+

 hw/piix_pci.c |   13 +-
 hw/ppc405_boards.c|   49 -
 hw/ppc4xx_pci.c   |   10 +++--
 hw/ppce500_pci.c  |   21 -
 hw/prep_pci.c |   12 -
 hw/r2d.c  |2 +-
 hw/stellaris.c|5 ++-
 hw/stellaris_enet.c   |   29 +---
 hw/sysbus.c   |   29 
 hw/sysbus.h   |8 +++
 hw/unin_pci.c |   82 ++--
 hw/virtex_ml507.c |4 +-
 hw/z2.c   |2 +-
 rwhandler.c   |   87 -
 rwhandler.h   |   27 
 47 files changed, 551 insertions(+), 594 deletions(-)
 delete mode 100644 rwhandler.c
 delete mode 100644 rwhandler.h

--
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] pc: Clean up PIC-to-APIC IRQ path

[Anthony Liguori]

On 09/04/2011 10:20 AM, Blue Swirl wrote:

On Sun, Sep 4, 2011 at 2:37 PM, Anthony Liguori  wrote:

On 09/04/2011 08:57 AM, Anthony Liguori wrote:


On 09/04/2011 08:49 AM, Jan Kiszka wrote:


On 2011-09-04 15:41, Anthony Liguori wrote:


On 09/04/2011 08:36 AM, Jan Kiszka wrote:
Having some sort of global interrupt routing table is just going to add
a layer of complexity for very little obvious gain.


It's not yet decided how the problem is solved. A global interrupt
matrix is just one proposal, another option is to extend the pin model
in a way that supports routing change notifiers and backward polling.


If that's all you need, then you really just want notification on socket
changes. Backwards polling can be achieved by just adding state to the
Pin (which I full heartedly support).

If that's all you're proposing, than I'm entirely happy with it :-)


It's not that simple.

Routing paths can change because of device changes, not just socket changes.


Yes, that's why callbacks are needed to let the device inform global matrix.


I think you would need an interface for irq routing.  Something like:

struct IrqRouter {
Interface parent;

void (*foreach_output)(IrqRouter *obj,
   void (*fn)(const char *out, void *opaque),
   void *opaque);

void (*foreach_input)(IrqRouter *obj,
  void (*fn)(const char *in, void *opaque),
  void *opaque);


Are the above a way for a parent to tell this device that its inputs
are changed?


No... there really isn't a notion of parent/children.

If something is interested in the routing path, it needs to listen for 
mapping changes at every IrqRouter point.  I neglected to add an 
interface for registering for mapping changes so there would also need 
to be a notification registration interface here.





const char *(*get_mapping)(IrqRouter *obj, const char *in);


This would be useful for the matrix too, the matrix would use this to
query for initial routing, maybe also for later changes if the change
callback didn't tell the mapping directly.


I think the main difference is, do you try to solve the problem 
everywhere, or do you just allow for certain paths to be introspected. 
And then who is reasonable for maintaining the full path.





};

You could then implement this interface in I440FX or any other controller
where we want to be able to support device passthrough.

Representing endpoints as strings means that you can correlate inputs to
outputs throughout the chain provided that you understand how inputs/outputs
relate to plugs/sockets.


The string format assumes that there is a reliable way to convert Pin
to string and vice versa. Why can't they be array of pointers to Pins?


Yes, all Pins are devices and all devices have unique names.  You could 
just as easily use Pin *s.


It's not clear to me whether you want to make the strings device names 
or just property names though.




String representation (e.g. "/i440fx/pci-bridge@2,0/serial@3fc:2")
could be useful for debugging and 'info qtree' ('info irqtree'?)
though.


IIUC, then this would be (in QOM)

i440fx::piix3::serial[0]::irq

This is the unique name the device would get.  That's because the PIIX3 
and Serial devices are created via composition.  You could indirectly 
reference it multiple ways:


/i440fx/slot[2]/serial[0]/irq
/i440fx/piix3/serial[0]/irq

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH] pc: Clean up PIC-to-APIC IRQ path

[Avi Kivity]

On 09/04/2011 06:19 PM, Anthony Liguori wrote:

Yes, and the memory API is complicated and invasive :-) But it's worth
it at the end of the day (although I think it could be simplified at
the expensive of not allowing as much flattening).


(we should have spent a few hours at kf2011 to convince you that this is
impossible)



I don't mean for RAM, but for device I/O.


It's impossible to make the distinction.  A piece of RAM can overlay an 
mmio region and split it in two, or an mmio region can split a RAM 
region.  This means the machinery cannot consider the region type anyway.




Instead of implementing it_shift in the core API, you could implement 
it_shift by having a device that takes an input MemoryRegion and an 
output MemoryRegion and implements the it_shift logic.


We could, but that imposes a burden on users to find that device and 
glue it to the memory API.  I'm not after a mean and lean API, I'm after 
something that is easy enough to use that people manage to get device 
models that work.




I think endianness could also be handled this way too.


On some archs, endianness can find itself in RAM, so we need complete 
control.  And again, I don't want users gluing stuff together the wrong 
way, I want them using a simple interface.


--
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Avi Kivity]

On 09/04/2011 06:24 PM, Michael S. Tsirkin wrote:

>
>  Of course it doesn't ignore it.  See the 440fx implementation, if
>  you disable VGA access (via the SMRAM register), vga goes away.

Yes but that's for VGA RAM, right? I'm talking about the IO addresses:
are tons of aliases created as you suggest?


No.

--
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Avi Kivity]

On 09/04/2011 06:26 PM, Michael S. Tsirkin wrote:

On Sun, Sep 04, 2011 at 05:36:56PM +0300, Avi Kivity wrote:
>  So long as we're nitpicking ...

+static void pci_bridge_init_alias(PCIBridge *bridge, MemoryRegion *alias,
+  uint8_t type, const char *name,
+  MemoryRegion *space,
+  MemoryRegion *parent_space)
+{
+pcibus_t base = pci_bridge_get_base(&bridge->dev, type);
+pcibus_t limit = pci_bridge_get_limit(&bridge->dev, type);
+/* TODO: this doesn't handle base = 0 limit = 2^64 - 1 correctly.
+ * Apparent_spacely no way to do this with existing memory APIs. */


Spelling out spaces is a new fashion?


+pcibus_t size = limit>= base ? limit + 1 - base : 0;
+
+memory_region_init_alias(alias, name, space, base, size);
+memory_region_add_subregion_overlap(parent_space, base, alias, 1);
+}
+

@@ -246,10 +312,14 @@ int pci_bridge_initfn(PCIDevice *dev)
  br->bus_name);
  sec_bus->parent_dev = dev;
  sec_bus->map_irq = br->map_irq;
-/* TODO: use memory API to perform memory filtering. */
-sec_bus->address_space_mem = parent->address_space_mem;
-sec_bus->address_space_io = parent->address_space_io;
-
+sec_bus->address_space_mem = g_new(MemoryRegion, 1);
+memory_region_init(sec_bus->address_space_mem, "pci_pridge_pci", 
INT64_MAX);
+sec_bus->address_space_io = g_new(MemoryRegion, 1);
+memory_region_init(sec_bus->address_space_io, "pci_bridge_io", 65536);
+sec_bus->alias_pref_mem = g_new(MemoryRegion, 1);
+sec_bus->alias_mem = g_new(MemoryRegion, 1);
+sec_bus->alias_io = g_new(MemoryRegion, 1);


Why pointers?  Regular fields require less upkeep.


--
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Michael S. Tsirkin]

On Sun, Sep 04, 2011 at 06:37:08PM +0300, Avi Kivity wrote:
> On 09/04/2011 06:24 PM, Michael S. Tsirkin wrote:
> >>
> >>  Of course it doesn't ignore it.  See the 440fx implementation, if
> >>  you disable VGA access (via the SMRAM register), vga goes away.
> >
> >Yes but that's for VGA RAM, right? I'm talking about the IO addresses:
> >are tons of aliases created as you suggest?
> 
> No.

So a full 16 bit decode is done for now?

> -- 
> error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] pc: Clean up PIC-to-APIC IRQ path

[Blue Swirl]

On Sun, Sep 4, 2011 at 3:31 PM, Anthony Liguori  wrote:
> On 09/04/2011 10:20 AM, Blue Swirl wrote:
>>
>> On Sun, Sep 4, 2011 at 2:37 PM, Anthony Liguori
>>  wrote:
>>>
>>> On 09/04/2011 08:57 AM, Anthony Liguori wrote:

 On 09/04/2011 08:49 AM, Jan Kiszka wrote:
>
> On 2011-09-04 15:41, Anthony Liguori wrote:
>>
>> On 09/04/2011 08:36 AM, Jan Kiszka wrote:
>> Having some sort of global interrupt routing table is just going to
>> add
>> a layer of complexity for very little obvious gain.
>
> It's not yet decided how the problem is solved. A global interrupt
> matrix is just one proposal, another option is to extend the pin model
> in a way that supports routing change notifiers and backward polling.

 If that's all you need, then you really just want notification on socket
 changes. Backwards polling can be achieved by just adding state to the
 Pin (which I full heartedly support).

 If that's all you're proposing, than I'm entirely happy with it :-)
>>>
>>> It's not that simple.
>>>
>>> Routing paths can change because of device changes, not just socket
>>> changes.
>>
>> Yes, that's why callbacks are needed to let the device inform global
>> matrix.
>>
>>> I think you would need an interface for irq routing.  Something like:
>>>
>>> struct IrqRouter {
>>>    Interface parent;
>>>
>>>    void (*foreach_output)(IrqRouter *obj,
>>>                           void (*fn)(const char *out, void *opaque),
>>>                           void *opaque);
>>>
>>>    void (*foreach_input)(IrqRouter *obj,
>>>                          void (*fn)(const char *in, void *opaque),
>>>                          void *opaque);
>>
>> Are the above a way for a parent to tell this device that its inputs
>> are changed?
>
> No... there really isn't a notion of parent/children.
>
> If something is interested in the routing path, it needs to listen for
> mapping changes at every IrqRouter point.  I neglected to add an interface
> for registering for mapping changes so there would also need to be a
> notification registration interface here.

With that change I think we're 100% in agreement.

>>
>>>    const char *(*get_mapping)(IrqRouter *obj, const char *in);
>>
>> This would be useful for the matrix too, the matrix would use this to
>> query for initial routing, maybe also for later changes if the change
>> callback didn't tell the mapping directly.
>
> I think the main difference is, do you try to solve the problem everywhere,
> or do you just allow for certain paths to be introspected. And then who is
> reasonable for maintaining the full path.

The matrix should take full responsibility for the devices (and the
connections of those devices) that are under its control. Other
devices can continue to do as before, they can be converted to matrix
if needed.

The benefit from the matrix would be performance and that would come
from lazy update of the devices in addition to the fast direct path
from source to final destination. If all the intermediate devices are
probed by the OS (if there are no vectored interrupts etc. to
determine the IRQ from PC), the lazy updates will not help and benefit
will be lower.

>>
>>> };
>>>
>>> You could then implement this interface in I440FX or any other controller
>>> where we want to be able to support device passthrough.
>>>
>>> Representing endpoints as strings means that you can correlate inputs to
>>> outputs throughout the chain provided that you understand how
>>> inputs/outputs
>>> relate to plugs/sockets.
>>
>> The string format assumes that there is a reliable way to convert Pin
>> to string and vice versa. Why can't they be array of pointers to Pins?
>
> Yes, all Pins are devices and all devices have unique names.  You could just
> as easily use Pin *s.
>
> It's not clear to me whether you want to make the strings device names or
> just property names though.

I think either would work if the string->Pin conversion works.

>>
>> String representation (e.g. "/i440fx/pci-bridge@2,0/serial@3fc:2")
>> could be useful for debugging and 'info qtree' ('info irqtree'?)
>> though.
>
> IIUC, then this would be (in QOM)
>
> i440fx::piix3::serial[0]::irq
>
> This is the unique name the device would get.  That's because the PIIX3 and
> Serial devices are created via composition.  You could indirectly reference
> it multiple ways:
>
> /i440fx/slot[2]/serial[0]/irq
> /i440fx/piix3/serial[0]/irq
>
> Regards,
>
> Anthony Liguori
>

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Michael S. Tsirkin]

On Sun, Sep 04, 2011 at 06:42:42PM +0300, Avi Kivity wrote:
> On 09/04/2011 06:26 PM, Michael S. Tsirkin wrote:
> >On Sun, Sep 04, 2011 at 05:36:56PM +0300, Avi Kivity wrote:
> >>  So long as we're nitpicking ...
> >
> >+static void pci_bridge_init_alias(PCIBridge *bridge, MemoryRegion *alias,
> >+  uint8_t type, const char *name,
> >+  MemoryRegion *space,
> >+  MemoryRegion *parent_space)
> >+{
> >+pcibus_t base = pci_bridge_get_base(&bridge->dev, type);
> >+pcibus_t limit = pci_bridge_get_limit(&bridge->dev, type);
> >+/* TODO: this doesn't handle base = 0 limit = 2^64 - 1 correctly.
> >+ * Apparent_spacely no way to do this with existing memory APIs. */
> 
> Spelling out spaces is a new fashion?

Will fix.

> >+pcibus_t size = limit>= base ? limit + 1 - base : 0;
> >+
> >+memory_region_init_alias(alias, name, space, base, size);
> >+memory_region_add_subregion_overlap(parent_space, base, alias, 1);
> >+}
> >+
> >
> >@@ -246,10 +312,14 @@ int pci_bridge_initfn(PCIDevice *dev)
> >  br->bus_name);
> >  sec_bus->parent_dev = dev;
> >  sec_bus->map_irq = br->map_irq;
> >-/* TODO: use memory API to perform memory filtering. */
> >-sec_bus->address_space_mem = parent->address_space_mem;
> >-sec_bus->address_space_io = parent->address_space_io;
> >-
> >+sec_bus->address_space_mem = g_new(MemoryRegion, 1);
> >+memory_region_init(sec_bus->address_space_mem, "pci_pridge_pci", 
> >INT64_MAX);
> >+sec_bus->address_space_io = g_new(MemoryRegion, 1);
> >+memory_region_init(sec_bus->address_space_io, "pci_bridge_io", 65536);
> >+sec_bus->alias_pref_mem = g_new(MemoryRegion, 1);
> >+sec_bus->alias_mem = g_new(MemoryRegion, 1);
> >+sec_bus->alias_io = g_new(MemoryRegion, 1);
> 
> Why pointers?  Regular fields require less upkeep.

Good point. Why does PIIX use pointers? I just copied that ...

> -- 
> error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Avi Kivity]

On 09/04/2011 06:45 PM, Michael S. Tsirkin wrote:

On Sun, Sep 04, 2011 at 06:37:08PM +0300, Avi Kivity wrote:
>  On 09/04/2011 06:24 PM, Michael S. Tsirkin wrote:
>  >>
>  >>   Of course it doesn't ignore it.  See the 440fx implementation, if
>  >>   you disable VGA access (via the SMRAM register), vga goes away.
>  >
>  >Yes but that's for VGA RAM, right? I'm talking about the IO addresses:
>  >are tons of aliases created as you suggest?
>
>  No.

So a full 16 bit decode is done for now?



Correct.  But isn't it needed?  Otherwise why don't vga accesses alias 
with a virtio device at 0xc3c0?


--
error compiling committee.c: too many arguments to function

[Qemu-devel] [PATCH 0/9] clang analyzer patches

[Blue Swirl]

I run the sources through clang analyzer again and made patches for
some of the issues.

Blue Swirl (9):
  win32: improve version.o dependency
  qemu-io: remove unnecessary assignment
  scsi-bus: remove duplicate table entries
  hid: fix misassignment
  kvm: remove unnecessary assignments
  cpu-exec: remove unnecessary assignment
  openpic: avoid a warning from clang analyzer
  lsi53c895a: avoid a warning from clang analyzer
  Sparc64: remove useless variable

 Makefile |2 +-
 cpu-exec.c   |5 +++--
 hw/hid.c |2 +-
 hw/lsi53c895a.c  |   14 --
 hw/openpic.c |1 +
 hw/scsi-bus.c|4 +---
 qemu-io.c|1 -
 target-i386/kvm.c|2 +-
 target-sparc/op_helper.c |6 ++
 9 files changed, 18 insertions(+), 19 deletions(-)

[Qemu-devel] [PATCH 1/9] win32: improve version.o dependency

[Blue Swirl]

Actually, version.rc doesn't need config-host.mak but config-host.h, fix it.

Signed-off-by: Blue Swirl 
---
 Makefile |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/Makefile b/Makefile
index e0cf51a..7e9382f 100644
--- a/Makefile
+++ b/Makefile
@@ -116,7 +116,7 @@ ui/vnc.o: QEMU_CFLAGS += $(VNC_TLS_CFLAGS)

 bt-host.o: QEMU_CFLAGS += $(BLUEZ_CFLAGS)

-version.o: $(SRC_PATH)/version.rc config-host.mak
+version.o: $(SRC_PATH)/version.rc config-host.h
$(call quiet-command,$(WINDRES) -I. -o $@ $<,"  RC$(TARGET_DIR)$@")

 version-obj-$(CONFIG_WIN32) += version.o
-- 
1.6.2.4
From 70f99a25b7732d4c9ea54f74c089ccb9bb323ea6 Mon Sep 17 00:00:00 2001
Message-Id: <70f99a25b7732d4c9ea54f74c089ccb9bb323ea6.1315150286.git.blauwir...@gmail.com>
From: Blue Swirl 
Date: Sun, 4 Sep 2011 09:32:55 +
Subject: [PATCH 1/9] win32: improve version.o dependency

Actually, version.rc doesn't need config-host.mak but config-host.h, fix it.

Signed-off-by: Blue Swirl 
---
 Makefile |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/Makefile b/Makefile
index e0cf51a..7e9382f 100644
--- a/Makefile
+++ b/Makefile
@@ -116,7 +116,7 @@ ui/vnc.o: QEMU_CFLAGS += $(VNC_TLS_CFLAGS)
 
 bt-host.o: QEMU_CFLAGS += $(BLUEZ_CFLAGS)
 
-version.o: $(SRC_PATH)/version.rc config-host.mak
+version.o: $(SRC_PATH)/version.rc config-host.h
 	$(call quiet-command,$(WINDRES) -I. -o $@ $<,"  RC$(TARGET_DIR)$@")
 
 version-obj-$(CONFIG_WIN32) += version.o
-- 
1.7.2.5

[Qemu-devel] [PATCH 2/9] qemu-io: remove unnecessary assignment

[Blue Swirl]

Remove an unnecessary assignment, spotted by clang analyzer:
/src/qemu/qemu-io.c:995:9: warning: Value stored to 'offset' is never read
offset += reqs[i].qiov->size;

Signed-off-by: Blue Swirl 
---
 qemu-io.c |1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/qemu-io.c b/qemu-io.c
index 7e40c48..5e46213 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -992,7 +992,6 @@ static int multiwrite_f(int argc, char **argv)

 optind = j + 1;

-offset += reqs[i].qiov->size;
 pattern++;
 }

-- 
1.6.2.4
From f092cf2775a0398a3267100ff0c8cd3caac5d5d6 Mon Sep 17 00:00:00 2001
Message-Id: 
In-Reply-To: <70f99a25b7732d4c9ea54f74c089ccb9bb323ea6.1315150286.git.blauwir...@gmail.com>
References: <70f99a25b7732d4c9ea54f74c089ccb9bb323ea6.1315150286.git.blauwir...@gmail.com>
From: Blue Swirl 
Date: Sun, 4 Sep 2011 10:38:01 +
Subject: [PATCH 2/9] qemu-io: remove unnecessary assignment

Remove an unnecessary assignment, spotted by clang analyzer:
/src/qemu/qemu-io.c:995:9: warning: Value stored to 'offset' is never read
offset += reqs[i].qiov->size;

Signed-off-by: Blue Swirl 
---
 qemu-io.c |1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/qemu-io.c b/qemu-io.c
index 7e40c48..5e46213 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -992,7 +992,6 @@ static int multiwrite_f(int argc, char **argv)
 
 optind = j + 1;
 
-offset += reqs[i].qiov->size;
 pattern++;
 }
 
-- 
1.7.2.5

[Qemu-devel] [PATCH 3/9] scsi-bus: remove duplicate table entries

[Blue Swirl]

Remove duplicate entries from SCSI command table, spotted by
clang analyzer:
/src/qemu/hw/scsi-bus.c:979:40: warning: initializer overrides prior
initialization of this subobject
[ ERASE_16 ] = "ERASE_16",
/src/qemu/hw/scsi-bus.c:978:40: note: previous initialization is here
[ WRITE_SAME_16] = "WRITE_SAME_16",
/src/qemu/hw/scsi-bus.c:984:40: warning: initializer overrides prior
initialization of this subobject
[ MAINTENANCE_IN   ] = "MAINTENANCE_IN",
/src/qemu/hw/scsi-bus.c:917:40: note: previous initialization is here
[ MAINTENANCE_IN   ] = "MAINTENANCE_IN",
/src/qemu/hw/scsi-bus.c:985:40: warning: initializer overrides prior
initialization of this subobject
[ MAINTENANCE_OUT  ] = "MAINTENANCE_OUT",
/src/qemu/hw/scsi-bus.c:918:40: note: previous initialization is here
[ MAINTENANCE_OUT  ] = "MAINTENANCE_OUT",

Signed-off-by: Blue Swirl 
---
 hw/scsi-bus.c |4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c
index 6f0d039..2703309 100644
--- a/hw/scsi-bus.c
+++ b/hw/scsi-bus.c
@@ -976,13 +976,11 @@ static const char *scsi_command_name(uint8_t cmd)
 [ SYNCHRONIZE_CACHE_16 ] = "SYNCHRONIZE_CACHE_16",
 [ LOCATE_16] = "LOCATE_16",
 [ WRITE_SAME_16] = "WRITE_SAME_16",
-[ ERASE_16 ] = "ERASE_16",
+/* ERASE_16 and WRITE_SAME_16 use the same operation code */
 [ SERVICE_ACTION_IN] = "SERVICE_ACTION_IN",
 [ WRITE_LONG_16] = "WRITE_LONG_16",
 [ REPORT_LUNS  ] = "REPORT_LUNS",
 [ BLANK] = "BLANK",
-[ MAINTENANCE_IN   ] = "MAINTENANCE_IN",
-[ MAINTENANCE_OUT  ] = "MAINTENANCE_OUT",
 [ MOVE_MEDIUM  ] = "MOVE_MEDIUM",
 [ LOAD_UNLOAD  ] = "LOAD_UNLOAD",
 [ READ_12  ] = "READ_12",
-- 
1.6.2.4
From 09ddd2115cdfb70dffcf10761c6aba466ec1d224 Mon Sep 17 00:00:00 2001
Message-Id: <09ddd2115cdfb70dffcf10761c6aba466ec1d224.1315150286.git.blauwir...@gmail.com>
In-Reply-To: <70f99a25b7732d4c9ea54f74c089ccb9bb323ea6.1315150286.git.blauwir...@gmail.com>
References: <70f99a25b7732d4c9ea54f74c089ccb9bb323ea6.1315150286.git.blauwir...@gmail.com>
From: Blue Swirl 
Date: Sun, 4 Sep 2011 10:56:51 +
Subject: [PATCH 3/9] scsi-bus: remove duplicate table entries

Remove duplicate entries from SCSI command table, spotted by
clang analyzer:
/src/qemu/hw/scsi-bus.c:979:40: warning: initializer overrides prior initialization of this subobject
[ ERASE_16 ] = "ERASE_16",
/src/qemu/hw/scsi-bus.c:978:40: note: previous initialization is here
[ WRITE_SAME_16] = "WRITE_SAME_16",
/src/qemu/hw/scsi-bus.c:984:40: warning: initializer overrides prior initialization of this subobject
[ MAINTENANCE_IN   ] = "MAINTENANCE_IN",
/src/qemu/hw/scsi-bus.c:917:40: note: previous initialization is here
[ MAINTENANCE_IN   ] = "MAINTENANCE_IN",
/src/qemu/hw/scsi-bus.c:985:40: warning: initializer overrides prior initialization of this subobject
[ MAINTENANCE_OUT  ] = "MAINTENANCE_OUT",
/src/qemu/hw/scsi-bus.c:918:40: note: previous initialization is here
[ MAINTENANCE_OUT  ] = "MAINTENANCE_OUT",

Signed-off-by: Blue Swirl 
---
 hw/scsi-bus.c |4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c
index 6f0d039..2703309 100644
--- a/hw/scsi-bus.c
+++ b/hw/scsi-bus.c
@@ -976,13 +976,11 @@ static const char *scsi_command_name(uint8_t cmd)
 [ SYNCHRONIZE_CACHE_16 ] = "SYNCHRONIZE_CACHE_16",
 [ LOCATE_16] = "LOCATE_16",
 [ WRITE_SAME_16] = "WRITE_SAME_16",
-[ ERASE_16 ] = "ERASE_16",
+/* ERASE_16 and WRITE_SAME_16 use the same operation code */
 [ SERVICE_ACTION_IN] = "SERVICE_ACTION_IN",
 [ WRITE_LONG_16] = "WRITE_LONG_16",
 [ REPORT_LUNS  ] = "REPORT_LUNS",
 [ BLANK] = "BLANK",
-[ MAINTENANCE_IN   ] = "MAINTENANCE_IN",
-[ MAINTENANCE_OUT  ] = "MAINTENANCE_OUT",
 [ MOVE_MEDIUM  ] = "MOVE_MEDIUM",
 [ LOAD_UNLOAD  ] = "LOAD_UNLOAD",
 [ READ_12  ] = "READ_12",
-- 
1.7.2.5

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Avi Kivity]

On 09/04/2011 06:46 PM, Michael S. Tsirkin wrote:

>
>  Why pointers?  Regular fields require less upkeep.

Good point. Why does PIIX use pointers? I just copied that ...



It doesn't, at least not completely:


struct PCII440FXState {
PCIDevice dev;
MemoryRegion *system_memory;
MemoryRegion *pci_address_space;
MemoryRegion *ram_memory;
MemoryRegion pci_hole;
MemoryRegion pci_hole_64bit;
PAMMemoryRegion pam_regions[13];
MemoryRegion smram_region;
uint8_t smm_enabled;
bool smram_enabled;
PIIX3State *piix3;
};

Arguably, ram_memory and pci_address_space should not be pointers, since 
this address space is supplied by 440fx.


(pci_hole, pci_hole64, pam_regions[], and smram_regions are all aliases 
analogous to the bridge windows)


--
error compiling committee.c: too many arguments to function

[Qemu-devel] [PATCH 4/9] hid: fix misassignment

[Blue Swirl]

The code does not have any effect as is, fix it.

Spotted by clang analyzer:
/src/qemu/hw/hid.c:99:13: warning: Value stored to 'x1' is never read
x1 = 1;

Signed-off-by: Blue Swirl 
---
 hw/hid.c |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/hw/hid.c b/hw/hid.c
index ec066cf..03761ab 100644
--- a/hw/hid.c
+++ b/hw/hid.c
@@ -96,7 +96,7 @@ static void
hid_pointer_event_combine(HIDPointerEvent *e, int xyrel,
 /* Windows drivers do not like the 0/0 position and ignore such
  * events. */
 if (!(x1 | y1)) {
-x1 = 1;
+e->xdx = 1;
 }
 }
 e->dz += z1;
-- 
1.6.2.4
From 15e3e8c915b6e89a0f9aad2dfec0a2e6c499fb0f Mon Sep 17 00:00:00 2001
Message-Id: <15e3e8c915b6e89a0f9aad2dfec0a2e6c499fb0f.1315150286.git.blauwir...@gmail.com>
In-Reply-To: <70f99a25b7732d4c9ea54f74c089ccb9bb323ea6.1315150286.git.blauwir...@gmail.com>
References: <70f99a25b7732d4c9ea54f74c089ccb9bb323ea6.1315150286.git.blauwir...@gmail.com>
From: Blue Swirl 
Date: Sun, 4 Sep 2011 10:59:52 +
Subject: [PATCH 4/9] hid: fix misassignment

The code does not have any effect as is, fix it.

Spotted by clang analyzer:
/src/qemu/hw/hid.c:99:13: warning: Value stored to 'x1' is never read
x1 = 1;

Signed-off-by: Blue Swirl 
---
 hw/hid.c |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/hw/hid.c b/hw/hid.c
index ec066cf..03761ab 100644
--- a/hw/hid.c
+++ b/hw/hid.c
@@ -96,7 +96,7 @@ static void hid_pointer_event_combine(HIDPointerEvent *e, int xyrel,
 /* Windows drivers do not like the 0/0 position and ignore such
  * events. */
 if (!(x1 | y1)) {
-x1 = 1;
+e->xdx = 1;
 }
 }
 e->dz += z1;
-- 
1.7.2.5

[Qemu-devel] [PATCH 5/9] kvm: remove unnecessary assignments

[Blue Swirl]

Avoid these warnings from clang analyzer:
/src/qemu/target-i386/kvm.c:772:5: warning: Value stored to 'cwd' is never read
cwd = swd = twd = 0;
/src/qemu/target-i386/kvm.c:772:11: warning: Although the value stored
to 'swd' is used in the enclosing expression, the value is never
actually read from 'swd'
cwd = swd = twd = 0;

Signed-off-by: Blue Swirl 
---
 target-i386/kvm.c |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 70ef74b..22b1dd0 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -769,7 +769,7 @@ static int kvm_put_xsave(CPUState *env)

 xsave = qemu_memalign(4096, sizeof(struct kvm_xsave));
 memset(xsave, 0, sizeof(struct kvm_xsave));
-cwd = swd = twd = 0;
+twd = 0;
 swd = env->fpus & ~(7 << 11);
 swd |= (env->fpstt & 7) << 11;
 cwd = env->fpuc;
-- 
1.6.2.4
From 93a750bfabac20ec5da32c4ee6ff1d33c464cd1e Mon Sep 17 00:00:00 2001
Message-Id: <93a750bfabac20ec5da32c4ee6ff1d33c464cd1e.1315150286.git.blauwir...@gmail.com>
In-Reply-To: <70f99a25b7732d4c9ea54f74c089ccb9bb323ea6.1315150286.git.blauwir...@gmail.com>
References: <70f99a25b7732d4c9ea54f74c089ccb9bb323ea6.1315150286.git.blauwir...@gmail.com>
From: Blue Swirl 
Date: Sun, 4 Sep 2011 11:03:52 +
Subject: [PATCH 5/9] kvm: remove unnecessary assignments

Avoid these warnings from clang analyzer:
/src/qemu/target-i386/kvm.c:772:5: warning: Value stored to 'cwd' is never read
cwd = swd = twd = 0;
/src/qemu/target-i386/kvm.c:772:11: warning: Although the value stored to 'swd' is used in the enclosing expression, the value is never actually read from 'swd'
cwd = swd = twd = 0;

Signed-off-by: Blue Swirl 
---
 target-i386/kvm.c |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 70ef74b..22b1dd0 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -769,7 +769,7 @@ static int kvm_put_xsave(CPUState *env)
 
 xsave = qemu_memalign(4096, sizeof(struct kvm_xsave));
 memset(xsave, 0, sizeof(struct kvm_xsave));
-cwd = swd = twd = 0;
+twd = 0;
 swd = env->fpus & ~(7 << 11);
 swd |= (env->fpstt & 7) << 11;
 cwd = env->fpuc;
-- 
1.7.2.5

[Qemu-devel] [PATCH 6/9] cpu-exec: remove unnecessary assignment

[Blue Swirl]

Avoid this warning from clang analyzer:
/src/qemu/cpu-exec.c:97:5: warning: Value stored to 'phys_page2' is never read
phys_page2 = -1;

Adjust the scope of the variable while at it.

Signed-off-by: Blue Swirl 
---
 cpu-exec.c |5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/cpu-exec.c b/cpu-exec.c
index de0d716..b00753c 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -86,7 +86,7 @@ static TranslationBlock *tb_find_slow(CPUState *env,
 {
 TranslationBlock *tb, **ptb1;
 unsigned int h;
-tb_page_addr_t phys_pc, phys_page1, phys_page2;
+tb_page_addr_t phys_pc, phys_page1;
 target_ulong virt_page2;

 tb_invalidated_flag = 0;
@@ -94,7 +94,6 @@ static TranslationBlock *tb_find_slow(CPUState *env,
 /* find translated block using physical mappings */
 phys_pc = get_page_addr_code(env, pc);
 phys_page1 = phys_pc & TARGET_PAGE_MASK;
-phys_page2 = -1;
 h = tb_phys_hash_func(phys_pc);
 ptb1 = &tb_phys_hash[h];
 for(;;) {
@@ -107,6 +106,8 @@ static TranslationBlock *tb_find_slow(CPUState *env,
 tb->flags == flags) {
 /* check next page if needed */
 if (tb->page_addr[1] != -1) {
+tb_page_addr_t phys_page2;
+
 virt_page2 = (pc & TARGET_PAGE_MASK) +
 TARGET_PAGE_SIZE;
 phys_page2 = get_page_addr_code(env, virt_page2);
-- 
1.6.2.4
From c125f7140947ee081fb860b311e29cea34bb6773 Mon Sep 17 00:00:00 2001
Message-Id: 
In-Reply-To: <70f99a25b7732d4c9ea54f74c089ccb9bb323ea6.1315150286.git.blauwir...@gmail.com>
References: <70f99a25b7732d4c9ea54f74c089ccb9bb323ea6.1315150286.git.blauwir...@gmail.com>
From: Blue Swirl 
Date: Sun, 4 Sep 2011 11:06:22 +
Subject: [PATCH 6/9] cpu-exec: remove unnecessary assignment

Avoid this warning from clang analyzer:
/src/qemu/cpu-exec.c:97:5: warning: Value stored to 'phys_page2' is never read
phys_page2 = -1;

Adjust the scope of the variable while at it.

Signed-off-by: Blue Swirl 
---
 cpu-exec.c |5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/cpu-exec.c b/cpu-exec.c
index de0d716..b00753c 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -86,7 +86,7 @@ static TranslationBlock *tb_find_slow(CPUState *env,
 {
 TranslationBlock *tb, **ptb1;
 unsigned int h;
-tb_page_addr_t phys_pc, phys_page1, phys_page2;
+tb_page_addr_t phys_pc, phys_page1;
 target_ulong virt_page2;
 
 tb_invalidated_flag = 0;
@@ -94,7 +94,6 @@ static TranslationBlock *tb_find_slow(CPUState *env,
 /* find translated block using physical mappings */
 phys_pc = get_page_addr_code(env, pc);
 phys_page1 = phys_pc & TARGET_PAGE_MASK;
-phys_page2 = -1;
 h = tb_phys_hash_func(phys_pc);
 ptb1 = &tb_phys_hash[h];
 for(;;) {
@@ -107,6 +106,8 @@ static TranslationBlock *tb_find_slow(CPUState *env,
 tb->flags == flags) {
 /* check next page if needed */
 if (tb->page_addr[1] != -1) {
+tb_page_addr_t phys_page2;
+
 virt_page2 = (pc & TARGET_PAGE_MASK) +
 TARGET_PAGE_SIZE;
 phys_page2 = get_page_addr_code(env, virt_page2);
-- 
1.7.2.5

[Qemu-devel] [PATCH 7/9] openpic: avoid a warning from clang analyzer

[Blue Swirl]

Avoid this warning by clang analyzer by defining a default case:
/src/qemu/hw/openpic.c:477:5: warning: Undefined or garbage value
returned to caller
return retval;

Signed-off-by: Blue Swirl 
---
 hw/openpic.c |1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/hw/openpic.c b/hw/openpic.c
index 26c96e2..4b883ac 100644
--- a/hw/openpic.c
+++ b/hw/openpic.c
@@ -469,6 +469,7 @@ static inline uint32_t read_IRQreg (openpic_t
*opp, int n_IRQ, uint32_t reg)
 case IRQ_IPVP:
 retval = opp->src[n_IRQ].ipvp;
 break;
+default:
 case IRQ_IDE:
 retval = opp->src[n_IRQ].ide;
 break;
-- 
1.6.2.4
From 3c4f48ac6edafd3fae07d081e66b40e8144b1818 Mon Sep 17 00:00:00 2001
Message-Id: <3c4f48ac6edafd3fae07d081e66b40e8144b1818.1315150286.git.blauwir...@gmail.com>
In-Reply-To: <70f99a25b7732d4c9ea54f74c089ccb9bb323ea6.1315150286.git.blauwir...@gmail.com>
References: <70f99a25b7732d4c9ea54f74c089ccb9bb323ea6.1315150286.git.blauwir...@gmail.com>
From: Blue Swirl 
Date: Sun, 4 Sep 2011 11:19:33 +
Subject: [PATCH 7/9] openpic: avoid a warning from clang analyzer

Avoid this warning by clang analyzer by defining a default case:
/src/qemu/hw/openpic.c:477:5: warning: Undefined or garbage value returned to caller
return retval;

Signed-off-by: Blue Swirl 
---
 hw/openpic.c |1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/hw/openpic.c b/hw/openpic.c
index 26c96e2..4b883ac 100644
--- a/hw/openpic.c
+++ b/hw/openpic.c
@@ -469,6 +469,7 @@ static inline uint32_t read_IRQreg (openpic_t *opp, int n_IRQ, uint32_t reg)
 case IRQ_IPVP:
 retval = opp->src[n_IRQ].ipvp;
 break;
+default:
 case IRQ_IDE:
 retval = opp->src[n_IRQ].ide;
 break;
-- 
1.7.2.5

[Qemu-devel] [PATCH 8/9] lsi53c895a: avoid a warning from clang analyzer

[Blue Swirl]

Avoid this warning from clang analyzer by adjusting the scope
of the variable:
/src/qemu/hw/lsi53c895a.c:895:5: warning: Value stored to 'id' is never read
id = (current_tag >> 8) & 0xf;

Signed-off-by: Blue Swirl 
---
 hw/lsi53c895a.c |   14 --
 1 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/hw/lsi53c895a.c b/hw/lsi53c895a.c
index 1643a63..9d900d0 100644
--- a/hw/lsi53c895a.c
+++ b/hw/lsi53c895a.c
@@ -883,7 +883,6 @@ static void lsi_do_msgout(LSIState *s)
 int len;
 uint32_t current_tag;
 lsi_request *current_req, *p, *p_next;
-int id;

 if (s->current) {
 current_tag = s->current->tag;
@@ -892,7 +891,6 @@ static void lsi_do_msgout(LSIState *s)
 current_tag = s->select_tag;
 current_req = lsi_find_by_tag(s, current_tag);
 }
-id = (current_tag >> 8) & 0xf;

 DPRINTF("MSG out len=%d\n", s->dbc);
 while (s->dbc) {
@@ -977,10 +975,14 @@ static void lsi_do_msgout(LSIState *s)
device, but this is currently not implemented (and seems not
to be really necessary). So let's simply clear all queued
commands for the current device: */
-id = current_tag & 0xff00;
-QTAILQ_FOREACH_SAFE(p, &s->queue, next, p_next) {
-if ((p->tag & 0xff00) == id) {
-scsi_req_cancel(p->req);
+{
+int id;
+
+id = current_tag & 0xff00;
+QTAILQ_FOREACH_SAFE(p, &s->queue, next, p_next) {
+if ((p->tag & 0xff00) == id) {
+scsi_req_cancel(p->req);
+}
 }
 }

-- 
1.6.2.4
From f787e79fa55affb8999ffc1765ea486c8edd46ce Mon Sep 17 00:00:00 2001
Message-Id: 
In-Reply-To: <70f99a25b7732d4c9ea54f74c089ccb9bb323ea6.1315150286.git.blauwir...@gmail.com>
References: <70f99a25b7732d4c9ea54f74c089ccb9bb323ea6.1315150286.git.blauwir...@gmail.com>
From: Blue Swirl 
Date: Sun, 4 Sep 2011 11:22:31 +
Subject: [PATCH 8/9] lsi53c895a: avoid a warning from clang analyzer

Avoid this warning from clang analyzer by adjusting the scope
of the variable:
/src/qemu/hw/lsi53c895a.c:895:5: warning: Value stored to 'id' is never read
id = (current_tag >> 8) & 0xf;

Signed-off-by: Blue Swirl 
---
 hw/lsi53c895a.c |   14 --
 1 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/hw/lsi53c895a.c b/hw/lsi53c895a.c
index 1643a63..9d900d0 100644
--- a/hw/lsi53c895a.c
+++ b/hw/lsi53c895a.c
@@ -883,7 +883,6 @@ static void lsi_do_msgout(LSIState *s)
 int len;
 uint32_t current_tag;
 lsi_request *current_req, *p, *p_next;
-int id;
 
 if (s->current) {
 current_tag = s->current->tag;
@@ -892,7 +891,6 @@ static void lsi_do_msgout(LSIState *s)
 current_tag = s->select_tag;
 current_req = lsi_find_by_tag(s, current_tag);
 }
-id = (current_tag >> 8) & 0xf;
 
 DPRINTF("MSG out len=%d\n", s->dbc);
 while (s->dbc) {
@@ -977,10 +975,14 @@ static void lsi_do_msgout(LSIState *s)
device, but this is currently not implemented (and seems not
to be really necessary). So let's simply clear all queued
commands for the current device: */
-id = current_tag & 0xff00;
-QTAILQ_FOREACH_SAFE(p, &s->queue, next, p_next) {
-if ((p->tag & 0xff00) == id) {
-scsi_req_cancel(p->req);
+{
+int id;
+
+id = current_tag & 0xff00;
+QTAILQ_FOREACH_SAFE(p, &s->queue, next, p_next) {
+if ((p->tag & 0xff00) == id) {
+scsi_req_cancel(p->req);
+}
 }
 }
 
-- 
1.7.2.5

[Qemu-devel] [PATCH 9/9] Sparc64: remove useless variable

[Blue Swirl]

Remove a useless variable, spotted by clang analyzer:
/src/qemu/target-sparc/op_helper.c:3904:18: warning: unused variable
'tmp' [-Wunused-variable]
target_ulong tmp = val;
The error message is actually incorrect since the variable is used.

Signed-off-by: Blue Swirl 
---
 target-sparc/op_helper.c |6 ++
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/target-sparc/op_helper.c b/target-sparc/op_helper.c
index d1a8dd9..48e1db8 100644
--- a/target-sparc/op_helper.c
+++ b/target-sparc/op_helper.c
@@ -3901,10 +3901,8 @@ target_ulong cpu_get_ccr(CPUState *env1)

 static void put_ccr(target_ulong val)
 {
-target_ulong tmp = val;
-
-env->xcc = (tmp >> 4) << 20;
-env->psr = (tmp & 0xf) << 20;
+env->xcc = (val >> 4) << 20;
+env->psr = (val & 0xf) << 20;
 CC_OP = CC_OP_FLAGS;
 }

-- 
1.6.2.4
From c532ec1bf51f11d7a57dfb8999a4fd9edd7c56cb Mon Sep 17 00:00:00 2001
Message-Id: 
In-Reply-To: <70f99a25b7732d4c9ea54f74c089ccb9bb323ea6.1315150286.git.blauwir...@gmail.com>
References: <70f99a25b7732d4c9ea54f74c089ccb9bb323ea6.1315150286.git.blauwir...@gmail.com>
From: Blue Swirl 
Date: Sun, 4 Sep 2011 11:32:23 +
Subject: [PATCH 9/9] Sparc64: remove useless variable

Remove a useless variable, spotted by clang analyzer:
/src/qemu/target-sparc/op_helper.c:3904:18: warning: unused variable 'tmp' [-Wunused-variable]
target_ulong tmp = val;
The error message is actually incorrect since the variable is used.

Signed-off-by: Blue Swirl 
---
 target-sparc/op_helper.c |6 ++
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/target-sparc/op_helper.c b/target-sparc/op_helper.c
index d1a8dd9..48e1db8 100644
--- a/target-sparc/op_helper.c
+++ b/target-sparc/op_helper.c
@@ -3901,10 +3901,8 @@ target_ulong cpu_get_ccr(CPUState *env1)
 
 static void put_ccr(target_ulong val)
 {
-target_ulong tmp = val;
-
-env->xcc = (tmp >> 4) << 20;
-env->psr = (tmp & 0xf) << 20;
+env->xcc = (val >> 4) << 20;
+env->psr = (val & 0xf) << 20;
 CC_OP = CC_OP_FLAGS;
 }
 
-- 
1.7.2.5

Re: [Qemu-devel] [PATCH v2] rbd: fix leak in qemu_rbd_open failure paths

[Sage Weil]

On Sun, 4 Sep 2011, Stefan Hajnoczi wrote:
> On Sat, Sep 3, 2011 at 11:04 PM, Sage Weil  wrote:
> > +failed_shutdown:
> >     rados_shutdown(s->cluster);
> > +    qemu_free(s->snap);
> 
> Sorry for being a pain here.  This patch is against an old qemu.git
> tree.  All memory allocation is now using glib's g_malloc()/g_free().

Heh, no problem.  We've been working off the last release to avoid 
worrying about transient instability in master.  Resending!

sage

[Qemu-devel] [PATCH v3] rbd: fix leak in qemu_rbd_open failure paths

[Sage Weil]

Fix leak of s->snap in failure path.  Simplify error paths for the whole
function.

Reported-by: Stefan Hajnoczi 
Signed-off-by: Sage Weil 
---
 block/rbd.c |   28 +---
 1 files changed, 13 insertions(+), 15 deletions(-)

diff --git a/block/rbd.c b/block/rbd.c
index 2763092..9f32211 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -427,10 +427,6 @@ static int qemu_rbd_open(BlockDriverState *bs, const char 
*filename, int flags)
conf, sizeof(conf)) < 0) {
 return -EINVAL;
 }
-s->snap = NULL;
-if (snap_buf[0] != '\0') {
-s->snap = g_strdup(snap_buf);
-}
 
 clientname = qemu_rbd_parse_clientname(conf, clientname_buf);
 r = rados_create(&s->cluster, clientname);
@@ -439,12 +435,16 @@ static int qemu_rbd_open(BlockDriverState *bs, const char 
*filename, int flags)
 return r;
 }
 
+s->snap = NULL;
+if (snap_buf[0] != '\0') {
+s->snap = g_strdup(snap_buf);
+}
+
 if (strstr(conf, "conf=") == NULL) {
 r = rados_conf_read_file(s->cluster, NULL);
 if (r < 0) {
 error_report("error reading config file");
-rados_shutdown(s->cluster);
-return r;
+   goto failed_shutdown;
 }
 }
 
@@ -452,31 +452,26 @@ static int qemu_rbd_open(BlockDriverState *bs, const char 
*filename, int flags)
 r = qemu_rbd_set_conf(s->cluster, conf);
 if (r < 0) {
 error_report("error setting config options");
-rados_shutdown(s->cluster);
-return r;
+   goto failed_shutdown;
 }
 }
 
 r = rados_connect(s->cluster);
 if (r < 0) {
 error_report("error connecting");
-rados_shutdown(s->cluster);
-return r;
+   goto failed_shutdown;
 }
 
 r = rados_ioctx_create(s->cluster, pool, &s->io_ctx);
 if (r < 0) {
 error_report("error opening pool %s", pool);
-rados_shutdown(s->cluster);
-return r;
+   goto failed_shutdown;
 }
 
 r = rbd_open(s->io_ctx, s->name, &s->image, s->snap);
 if (r < 0) {
 error_report("error reading header from %s", s->name);
-rados_ioctx_destroy(s->io_ctx);
-rados_shutdown(s->cluster);
-return r;
+   goto failed_open;
 }
 
 bs->read_only = (s->snap != NULL);
@@ -497,8 +492,11 @@ static int qemu_rbd_open(BlockDriverState *bs, const char 
*filename, int flags)
 
 failed:
 rbd_close(s->image);
+failed_open:
 rados_ioctx_destroy(s->io_ctx);
+failed_shutdown:
 rados_shutdown(s->cluster);
+g_free(s->snap);
 return r;
 }
 
-- 
1.7.2.5

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Michael S. Tsirkin]

On Sun, Sep 04, 2011 at 06:46:46PM +0300, Avi Kivity wrote:
> On 09/04/2011 06:45 PM, Michael S. Tsirkin wrote:
> >On Sun, Sep 04, 2011 at 06:37:08PM +0300, Avi Kivity wrote:
> >>  On 09/04/2011 06:24 PM, Michael S. Tsirkin wrote:
> >>  >>
> >>  >>   Of course it doesn't ignore it.  See the 440fx implementation, if
> >>  >>   you disable VGA access (via the SMRAM register), vga goes away.
> >>  >
> >>  >Yes but that's for VGA RAM, right? I'm talking about the IO addresses:
> >>  >are tons of aliases created as you suggest?
> >>
> >>  No.
> >
> >So a full 16 bit decode is done for now?
> >
> 
> Correct.

Then we probably can ignore the issue and do 16 bit decode all over
for now.

> But isn't it needed?  Otherwise why don't vga accesses
> alias with a virtio device at 0xc3c0?

It really depends on the device I think.

> -- 
> error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Michael S. Tsirkin]

On Sun, Sep 04, 2011 at 06:49:34PM +0300, Avi Kivity wrote:
> On 09/04/2011 06:46 PM, Michael S. Tsirkin wrote:
> >>
> >>  Why pointers?  Regular fields require less upkeep.
> >
> >Good point. Why does PIIX use pointers? I just copied that ...
> >
> 
> It doesn't, at least not completely:
> 
> 
> struct PCII440FXState {
> PCIDevice dev;
> MemoryRegion *system_memory;
> MemoryRegion *pci_address_space;
> MemoryRegion *ram_memory;
> MemoryRegion pci_hole;
> MemoryRegion pci_hole_64bit;
> PAMMemoryRegion pam_regions[13];
> MemoryRegion smram_region;
> uint8_t smm_enabled;
> bool smram_enabled;
> PIIX3State *piix3;
> };
> 
> Arguably, ram_memory and pci_address_space should not be pointers,
> since this address space is supplied by 440fx.
> 
> (pci_hole, pci_hole64, pam_regions[], and smram_regions are all
> aliases analogous to the bridge windows)


I'll switch my code to that, you take care of piix.

> -- 
> error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 8/9] lsi53c895a: avoid a warning from clang analyzer

[Avi Kivity]

On 09/04/2011 06:53 PM, Blue Swirl wrote:

Avoid this warning from clang analyzer by adjusting the scope
of the variable:
/src/qemu/hw/lsi53c895a.c:895:5: warning: Value stored to 'id' is never read
 id = (current_tag>>  8)&  0xf;

Signed-off-by: Blue Swirl
---
  hw/lsi53c895a.c |   14 --
  1 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/hw/lsi53c895a.c b/hw/lsi53c895a.c
index 1643a63..9d900d0 100644
--- a/hw/lsi53c895a.c
+++ b/hw/lsi53c895a.c
@@ -883,7 +883,6 @@ static void lsi_do_msgout(LSIState *s)
  int len;
  uint32_t current_tag;
  lsi_request *current_req, *p, *p_next;
-int id;

  if (s->current) {
  current_tag = s->current->tag;
@@ -892,7 +891,6 @@ static void lsi_do_msgout(LSIState *s)
  current_tag = s->select_tag;
  current_req = lsi_find_by_tag(s, current_tag);
  }
-id = (current_tag>>  8)&  0xf;

  DPRINTF("MSG out len=%d\n", s->dbc);
  while (s->dbc) {
@@ -977,10 +975,14 @@ static void lsi_do_msgout(LSIState *s)
 device, but this is currently not implemented (and seems not
 to be really necessary). So let's simply clear all queued
 commands for the current device: */
-id = current_tag&  0xff00;
-QTAILQ_FOREACH_SAFE(p,&s->queue, next, p_next) {
-if ((p->tag&  0xff00) == id) {
-scsi_req_cancel(p->req);
+{
+int id;
+
+id = current_tag&  0xff00;
+QTAILQ_FOREACH_SAFE(p,&s->queue, next, p_next) {
+if ((p->tag&  0xff00) == id) {
+scsi_req_cancel(p->req);
+}
  }
  }



Why not keep id declared in the outer scope?  This extra indentation is 
annoying.



--
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Avi Kivity]

On 09/04/2011 07:19 PM, Michael S. Tsirkin wrote:

>  But isn't it needed?  Otherwise why don't vga accesses
>  alias with a virtio device at 0xc3c0?

It really depends on the device I think.



It's probably the bus.  ISA may not decode A10-15, but if the pci/isa 
bridge does, then it doesn't matter.


--
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH V8 07/14] Implementation of the libtpms-based backend

[Michael S. Tsirkin]

On Thu, Sep 01, 2011 at 09:24:26PM -0400, Stefan Berger wrote:
> In patch 6 I am adding a skeleton backend driver that I am
> transforming into the libtpms-based backend in patch 7. I didn't
> name the file tpm_skeleton.c but already tpm_builtin.c and all
> functions already start with the prefix tpm_builtin. This presumably
> makes it easier to review since the 'meat' is added in part 7 and
> unnecessary function name changes are avoided.

It is a good idea to split your code to patches.
But you don't need your code to actually work
in an intermediate step - or even compile
if it is not added to the Makefile.

That will help reviewers by sending small patches
and not waste reviewer's time on reading code
removed in a later patch.

-- 
MST

Re: [Qemu-devel] [PATCH 8/9] lsi53c895a: avoid a warning from clang analyzer

[Blue Swirl]

On Sun, Sep 4, 2011 at 4:20 PM, Avi Kivity  wrote:
> On 09/04/2011 06:53 PM, Blue Swirl wrote:
>>
>> Avoid this warning from clang analyzer by adjusting the scope
>> of the variable:
>> /src/qemu/hw/lsi53c895a.c:895:5: warning: Value stored to 'id' is never
>> read
>>     id = (current_tag>>  8)&  0xf;
>>
>> Signed-off-by: Blue Swirl
>> ---
>>  hw/lsi53c895a.c |   14 --
>>  1 files changed, 8 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/lsi53c895a.c b/hw/lsi53c895a.c
>> index 1643a63..9d900d0 100644
>> --- a/hw/lsi53c895a.c
>> +++ b/hw/lsi53c895a.c
>> @@ -883,7 +883,6 @@ static void lsi_do_msgout(LSIState *s)
>>      int len;
>>      uint32_t current_tag;
>>      lsi_request *current_req, *p, *p_next;
>> -    int id;
>>
>>      if (s->current) {
>>          current_tag = s->current->tag;
>> @@ -892,7 +891,6 @@ static void lsi_do_msgout(LSIState *s)
>>          current_tag = s->select_tag;
>>          current_req = lsi_find_by_tag(s, current_tag);
>>      }
>> -    id = (current_tag>>  8)&  0xf;
>>
>>      DPRINTF("MSG out len=%d\n", s->dbc);
>>      while (s->dbc) {
>> @@ -977,10 +975,14 @@ static void lsi_do_msgout(LSIState *s)
>>                 device, but this is currently not implemented (and seems
>> not
>>                 to be really necessary). So let's simply clear all queued
>>                 commands for the current device: */
>> -            id = current_tag&  0xff00;
>> -            QTAILQ_FOREACH_SAFE(p,&s->queue, next, p_next) {
>> -                if ((p->tag&  0xff00) == id) {
>> -                    scsi_req_cancel(p->req);
>> +            {
>> +                int id;
>> +
>> +                id = current_tag&  0xff00;
>> +                QTAILQ_FOREACH_SAFE(p,&s->queue, next, p_next) {
>> +                    if ((p->tag&  0xff00) == id) {
>> +                        scsi_req_cancel(p->req);
>> +                    }
>>                  }
>>              }
>>
>
> Why not keep id declared in the outer scope?  This extra indentation is
> annoying.

Also the variable could be dropped altogether simply with:
if ((p->tag & 0xff00) == (current_tag & 0xff00)) {

I have no preference.

Re: [Qemu-devel] [PATCH V8 01/14] Support for TPM command line options

[Michael S. Tsirkin]

On Thu, Sep 01, 2011 at 09:01:32PM -0400, Stefan Berger wrote:
> On 09/01/2011 01:14 PM, Michael S. Tsirkin wrote:
> >On Wed, Aug 31, 2011 at 10:35:52AM -0400, Stefan Berger wrote:
> >>This patch adds support for TPM command line options.
> >>The command line supported here (considering the libtpms based
> >>backend) are
> >>
> >>./qemu-... -tpm builtin,path=
> >>
> >>and
> >>
> >>./qemu-... -tpmdev builtin,path=,id=
> >>-device tpm-tis,tpmdev=
> >do we really need both?
> I had chatted with Anthony about this. I am following the existing
> pattern is use for example for -netdev / -net.

Must be some kind of misunderstanding.
Please do not add multiple ways to do one thing.
We only support -net for legacy reasons.

-- 
MST

[Qemu-devel] [PATCH] configure: Avoid duplicate flags when calling compile_prog

[Stefan Weil]

This patch removes redundant shell code and cleans it a little bit.

Shell macro compile_prog takes two arguments:
local_cflags and local ldflags.

$QEMU_CFLAGS is added automatically to the cflags, so there is no need
to pass it as an argument.

It is also unnecessary to pass -Werror twice.

$flag is a compiler warning option, so it should be in local_cflags
instead of local_ldflags.

Signed-off-by: Stefan Weil 
---
 configure |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/configure b/configure
index c3044c7..d261fdd 100755
--- a/configure
+++ b/configure
@@ -1071,7 +1071,7 @@ cat > $TMPC << EOF
 int main(void) { return 0; }
 EOF
 for flag in $gcc_flags; do
-if compile_prog "-Werror $QEMU_CFLAGS" "-Werror $flag" ; then
+if compile_prog "$flag -Werror" "" ; then
QEMU_CFLAGS="$QEMU_CFLAGS $flag"
 fi
 done
-- 
1.7.0.4

Re: [Qemu-devel] [PATCH V8 14/14] Allow to provide inital TPM state

[Michael S. Tsirkin]

On Thu, Sep 01, 2011 at 11:00:56PM -0400, Stefan Berger wrote:
> >Do we really have to hand-craft file reading?
> >How large is TPMPROP_TPM_MAX_NV_SPACE?
> >If not too large, we can just allocate that
> >and do a single fread call?
> Yes, I could do that, too.
> >Or, we rely on glib now - can we use
> >g_io_channel_read_to_end () or something like that?
> >
> >
> GIOStatus   g_io_channel_read_to_end(GIOChannel *channel,
>  gchar **str_return,
>  gsize *length,
>  GError **error);
> 
> I'd rather not use it. Presumably it uses g_malloc() to internally
> allocate the str_return. However, the buffer we are allocating in
> this function is given to the libtpms, which in turn will call plain
> free() on it. I don't want this to cause a problem if not g_free()
> is called on this buffer.

Well, copy the data and malloc, or do something else,
must be easier than this manual loop.

> >>+
> >Separate options for fd and for file mode would be better.
> >
> initstate_fd=base64:
> initstate_fd=bin:
> initstate=base64:
> initstate=bin:
> 
> Along these lines?

No

initstate_fd=
initstate_base64=on/off (or base64/bin if you really expect
more formats in the future)

and use qemu routines to get the fd so they can be
passed through the monitor later ...


> Yes, that's a problem. Above would require bin:file:
> to be understood.
> 
>Stefan

So avoid it. Give each option it's own flag.

-- 
MST

Re: [Qemu-devel] [PATCH V8 13/14] Add a TPM backend null driver implementation

[Michael S. Tsirkin]

On Thu, Sep 01, 2011 at 10:41:04PM -0400, Stefan Berger wrote:
> On 09/01/2011 01:40 PM, Michael S. Tsirkin wrote:
> >On Wed, Aug 31, 2011 at 10:36:04AM -0400, Stefan Berger wrote:
> >>This patch adds a TPM null driver implementation acting as a backend for
> >>the TIS hardware emulation. The NULL driver responds to all commands with
> >>a TPM fault response.
> >>
> >>To use this null driver, use either
> >>
> >>-tpm null
> >>
> >>or
> >>
> >>-tpmdev null,id=tpm0 -device tpm-tis,tpmdev=tpm0
> >>
> >>as parameters on the command line.
> >>
> >>If TPM support is chosen via './configure --enable-tpm ...' TPM support is 
> >>now
> >>always compiled into Qemu and at least the null driver will be available on
> >>emulators for x86_64 and i386.
> >Why limit this to intel platforms then?
> >
> Same as with my previous comment: only hw/pc.c handles the TPM
> device for x86_64 and i386 emulators.
> >>v8:
> >>  - initializing 'in' variable
> >>
> >>Signed-off-by: Stefan Berger
> >>
> >>---
> >>  Makefile.target |2
> >>  configure   |8 -
> >>  hw/tpm_null.c   |  327 
> >> 
> >>  qemu-options.hx |   13 +-
> >>  tpm.c   |1
> >>  tpm.h   |1
> >>  6 files changed, 341 insertions(+), 11 deletions(-)
> >>
> >>Index: qemu-git/hw/tpm_null.c
> >>===
> >>--- /dev/null
> >>+++ qemu-git/hw/tpm_null.c
> >>@@ -0,0 +1,327 @@
> >>+/*
> >>+ *  builtin 'null' TPM driver
> >Is this the same one an earlier patch removed?
> It's similar, yes, but previously it served to cut the 'builtin'
> backend's code size into two smaller patches.
> >>+ *
> >>+ *  Copyright (c) 2010, 2011 IBM Corporation
> >>+ *  Copyright (c) 2010, 2011 Stefan Berger
> >Why dual copyright btw?
> >
> Not a specific reason...
> >>+ *
> >>+ * This library is free software; you can redistribute it and/or
> >>+ * modify it under the terms of the GNU Lesser General Public
> >>+ * License as published by the Free Software Foundation; either
> >>+ * version 2 of the License, or (at your option) any later version.
> >>+ *
> >>+ * This library is distributed in the hope that it will be useful,
> >>+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
> >>+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> >>+ * Lesser General Public License for more details.
> >>+ *
> >>+ * You should have received a copy of the GNU Lesser General Public
> >>+ * License along with this library; if not, 
> >>see
> >>+ */
> >>+
> >>+#include "qemu-common.h"
> >>+#include "tpm.h"
> >>+#include "hw/hw.h"
> >>+#include "hw/tpm_tis.h"
> >>+#include "hw/pc.h"
> >>+
> >>+
> >>+//#define DEBUG_TPM
> >>+//#define DEBUG_TPM_SR /* suspend - resume */
> >don't use C++ comments please.
> I ran the perl script over the patches and it pointed that out to
> me. I only kept it because I found similar comments for DEBUG
> #defines in other files.
> >>+
> >>+
> >>+/* data structures */
> >>+
> >>+typedef struct ThreadParams {
> >>+TPMState *tpm_state;
> >>+
> >>+TPMRecvDataCB *recv_data_callback;
> >>+} ThreadParams;
> >>+
> >>+
> >>+/* local variables */
> >>+
> >>+static QemuThread thread;
> >>+
> >>+static bool thread_terminate;
> >>+static bool thread_running;
> >>+
> >>+static ThreadParams tpm_thread_params;
> >this lacks consistency in naming.
> >prefix everything with tpm_null?
> >
> Ok.
> 
> >>+
> >>+static const unsigned char tpm_std_fatal_error_response[10] = {
> >>+0x00, 0xc4, 0x00, 0x00, 0x00, 0x0A, 0x00, 0x00, 0x00, 0x09 /* TPM_FAIL 
> >>*/
> >>+};
> >>+
> >>+static char dev_description[80];
> >Use symbolic constants above?
> >
> In this case dev_description could be a constant.
> 
> static char tpm_null_dev_description[] = "Null TPM backend driver";

Right. Same for the tpm_std_fatal_error_response magic numbers ...

> >>+
> >>+
> >>+static void *tpm_null_main_loop(void *d)
> >>+{
> >>+ThreadParams *thr_parms = d;
> >>+uint32_t in_len;
> >>+uint8_t *in, *out;
> >>+uint8_t locty;
> >>+
> >>+#ifdef DEBUG_TPM
> >>+fprintf(stderr, "tpm: THREAD IS STARTING\n");
> >>+#endif
> >>+
> >>+/* start command processing */
> >>+while (!thread_terminate) {
> >>+/* receive and handle commands */
> >>+in_len = 0;
> >>+do {
> >>+#ifdef DEBUG_TPM
> >>+fprintf(stderr, "tpm: waiting for commands...\n");
> >>+#endif
> >>+
> >>+if (thread_terminate) {
> >>+break;
> >>+}
> >>+
> >>+qemu_mutex_lock(&thr_parms->tpm_state->state_lock);
> >>+
> >>+/* in case we were to slow and missed the signal, the
> >>+   to_tpm_execute boolean tells us about a pending command */
> >>+if (!thr_parms->tpm_state->to_tpm_execute) {
> >>+qemu_cond_wait(&thr_parms->tpm_state->to_tpm_cond,
> >>+&thr_parms->tpm_state->state_lock);
> >>+}
> >>+

Re: [Qemu-devel] [PATCH V8 01/14] Support for TPM command line options

[Michael S. Tsirkin]

On Thu, Sep 01, 2011 at 09:01:32PM -0400, Stefan Berger wrote:
> >>Monitor support for 'info tpm' has been added. It for example prints the
> >>following:
> >>
> >>TPM devices:
> >>   builtin: model=tpm-tis,id=tpm0
> >This mixes frontend and backend properties.
> >
> There's currently only one frontend 'model' and that's the
> 'tpm-tis'. In case someone would want to write a virtio equivalent
> it would show the that the 'builtin' backend is connected to the
> 'virtio' frontend model. If above is not correct, how should it look
> like?

E.g. for net: we list backends and frontends each with its
properties.

  virtio-net-pci.0: type=nic,model=virtio-net-pci,macaddr=52:54:00:12:34:56
   \ foo: type=tap,ifname=msttap0,script=/home/mst/ifup,downscript=no



> >>+
> >>+value = qemu_opt_get(opts, "type");
> >>+if (!value) {
> >>+qerror_report(QERR_MISSING_PARAMETER, "type");
> >>+tpm_display_backend_drivers(stderr);
> >>+return 1;
> >>+}
> >>+
> >>+be = tpm_get_backend_driver(value);
> >>+if (be == NULL) {
> >>+qerror_report(QERR_INVALID_PARAMETER_VALUE, "type",
> >>+  "a tpm backend type");
> >>+tpm_display_backend_drivers(stderr);
> >>+return 1;
> >>+}
> >>+
> >>+assert((is_tpmdev&&  model == NULL) || (!is_tpmdev&&  model != NULL));
> >Why isn't this using qdev for parameter passing?
> >
> Can you point me to a device that is using qdev for parameter
> passing.


virtio-pci devices: block, net - have a huge number of these.

I'm talking about frontend primarily.

> Also this part is very similar to how the networking works
> (net.c).
> 
>Stefan

A large part of that is legacy processing.

-- 
MST

Re: [Qemu-devel] [PATCH V8 10/14] Encrypt state blobs using AES CBC encryption

[Michael S. Tsirkin]

On Thu, Sep 01, 2011 at 10:23:51PM -0400, Stefan Berger wrote:
> >>Checks are added that test
> >>- whether encryption is supported follwing the revision of the directory
> >>   structure (rev>= 2)
> >You never generate rev 1 code, right?
> I did this in the previous patch that implemented rev 1 that knew
> nothing about the encryption added in rev 2.
> >So why keep that support around in code?
> >The first version merged into qemu should be revision 0 (or 1, as you like).
> I chose '1'. See patch 9:
> 
> +#define BS_DIR_REV1 1
> +
> +#define BS_DIR_REV_CURRENT  BS_DIR_REV1
> +
> 
> So I think it's the proper thing to do to increase the revision
> number from 1 to 2 since it's in two separate patches (even if they
> were to be applied immediately).

Look, the code is not merged yet and you already have
legacy with revision history to support? Why create work?

> >Don't support legacy with old version of your patch.
> >
> >>- whether a key has been provided although all data are stored in clear-text
> >>- whether a key is missing for decryption.
> >>
> >>In either one of the cases the backend reports an error message to the user
> >>and Qemu terminates.
> >>
> >>-v7:
> >>   - cleaned up function parsing key
> >>
> >>-v6:
> >>   - changed the format of the key= to take the type of encryption into
> >> account: key=aes-cbc:0x12345... and reworked code for encryption and
> >> decryption of blobs;
> >separate type and data:
> >keytype=aes-cbc,key=0x123  to avoid introducing more option parsing.
> >Also, are people likely to have the key in a file?
> >If yes maybe read a key from there and skip parsing completely?
> >
> I think both choices should probably exist. Now what's a good file
> format? Would we expect to find a hex number in there or should it
> always be assumed to be a binary file?

binary

> >>   - modified directory entry to hold a uint_8 describing the encryption
> >> type (none, aes-cbc) being used for the blobs.
> >>   - incrementing revision of the directory to '2' indicating encryption
> >> support
> >>
> >>-v5:
> >>   - -tpmdev now also gets a key parameter
> >>   - add documentation about key parameter
> >>
> >>Signed-off-by: Stefan Berger
> >>
> >>---
> >>  hw/tpm_builtin.c |  285 
> >> +--
> >>  qemu-config.c|   10 +
> >>  qemu-options.hx  |   22 +++-
> >>  tpm.c|   10 +
> >>  4 files changed, 318 insertions(+), 9 deletions(-)
> >>
> >>Index: qemu-git/hw/tpm_builtin.c
> >>===
> >>--- qemu-git.orig/hw/tpm_builtin.c
> >>+++ qemu-git/hw/tpm_builtin.c
> >>@@ -27,6 +27,7 @@
> >>  #include "hw/pc.h"
> >>  #include "migration.h"
> >>  #include "sysemu.h"
> >>+#include "aes.h"
> >>
> >>  #include
> >>  #include
> >>@@ -110,14 +111,27 @@ typedef struct BSDir {
> >>  uint16_t  rev;
> >>  uint32_t  checksum;
> >>  uint32_t  num_entries;
> >>-uint32_t  reserved[10];
> >>+uint8_t   enctype;
> >>+uint8_t   reserved1[3];
> >>+uint32_t  reserved[8];
> >>  BSEntry   entries[BS_DIR_MAX_NUM_ENTRIES];
> >>  } __attribute__((packed)) BSDir;
> >>
> >>
> >>  #define BS_DIR_REV1 1
> >>+/* rev 2 added encryption */
> >>+#define BS_DIR_REV2 2
> >>
> >>-#define BS_DIR_REV_CURRENT  BS_DIR_REV1
> >>+
> >>+#define BS_DIR_REV_CURRENT  BS_DIR_REV2
> >>+
> >>+/* above enctype */
> >>+enum BSEnctype {
> >>+BS_DIR_ENCTYPE_NONE = 0,
> >>+BS_DIR_ENCTYPE_AES_CBC,
> >>+
> >>+BS_DIR_ENCTYPE_LAST,
> >>+};
> >>
> >>  /* local variables */
> >>
> >>@@ -150,6 +164,11 @@ static const unsigned char tpm_std_fatal
> >>
> >>  static char dev_description[80];
> >>
> >>+static struct enckey {
> >>+uint8_t enctype;
> >>+AES_KEY tpm_enc_key;
> >>+AES_KEY tpm_dec_key;
> >>+} enckey;
> >>
> >>  static int tpm_builtin_load_sized_data_from_bs(BlockDriverState *bs,
> >> enum BSEntryType be,
> >>@@ -264,7 +283,7 @@ static uint32_t tpm_builtin_calc_dir_che
> >>
> >>  static bool tpm_builtin_is_valid_bsdir(BSDir *dir)
> >>  {
> >>-if (dir->rev != BS_DIR_REV_CURRENT ||
> >>+if (dir->rev>  BS_DIR_REV_CURRENT ||
> >>  dir->num_entries>  BS_DIR_MAX_NUM_ENTRIES) {
> >>  return false;
> >>  }
> >>@@ -295,6 +314,33 @@ static bool tpm_builtin_has_valid_conten
> >>  return rc;
> >>  }
> >>
> >>+static bool tpm_builtin_supports_encryption(const BSDir *dir)
> >>+{
> >>+return (dir->rev>= BS_DIR_REV2);
> >>+}
> >>+
> >>+
> >>+static bool tpm_builtin_has_missing_key(const BSDir *dir)
> >>+{
> >>+return ((dir->enctype   != BS_DIR_ENCTYPE_NONE)&&
> >>+(enckey.enctype == BS_DIR_ENCTYPE_NONE));
> >>+}
> >>+
> >>+
> >>+static bool tpm_builtin_has_unnecessary_key(const BSDir *dir)
> >>+{
> >>+return (((dir->enctype   == BS_DIR_ENCTYPE_NONE)&&
> >>+ (enckey.enctype != BS_DIR_ENCTYPE_NONE)) ||
> >>+((!tpm_builtin_support

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Michael S. Tsirkin]

On Sun, Sep 04, 2011 at 07:22:54PM +0300, Avi Kivity wrote:
> On 09/04/2011 07:19 PM, Michael S. Tsirkin wrote:
> >>  But isn't it needed?  Otherwise why don't vga accesses
> >>  alias with a virtio device at 0xc3c0?
> >
> >It really depends on the device I think.
> >
> 
> It's probably the bus.  ISA may not decode A10-15, but if the
> pci/isa bridge does, then it doesn't matter.

The bridge has a 16 bit decode bit. I'm guessing we should
make BIOS enable that.

> -- 
> error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 8/9] lsi53c895a: avoid a warning from clang analyzer

[Peter Maydell]

On 4 September 2011 17:20, Avi Kivity  wrote:
> Why not keep id declared in the outer scope?  This extra indentation is
> annoying.

Personally I find that in a 125 line long function, declaring
a variable at function scope when it's actually used only in
a very small section of the code makes for worse readability:
you have to search through to confirm that it really is only
used in that small section. It gets worse if you have one
variable reused for several unrelated things.

So I think either the patch as submitted or the other approach
Blue Swirl suggests of just dropping the variable are better
than leaving id at function scope.

-- PMM

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Michael S. Tsirkin]

On Mon, Jul 04, 2011 at 12:43:58PM +0300, Michael S. Tsirkin wrote:
> This adds support for a standard pci to pci bridge,
> enabling support for more than 32 PCI devices in the system.
> To use, specify the device id as a 'bus' option.
> Example:
>   -device pci-bridge,id=bridge1 \
>   -netdev user,id=u \
>   -device ne2k_pci,id=net2,bus=bridge1,netdev=u

BTW, I just noticed that -device ne2k_pci,? does
not list the bus option. Any idea why?

-- 
MST

Re: [Qemu-devel] emulated ARM performance vs real processor ?

[Antti P Miettinen]

Julien Heyman  writes:
> Hi,
>
> I was wondering if anyone had some data regarding the relative performance of
> any given ARM board emulated in QEMU versus the real thing. Yes, I do know
> this depends a lot on the host PC running qemu, but some ballpark/example
> figures would help. Say, I emulate a 400 Mhz ARM9 processor on a Core2Duo
> laptop @ 2 Ghz, what kind of performance/timing ratio should I expect, one way
> or the other ? For example, for boot time.
> I have no idea whether the overhead of emulation is over-compensated by the
> huge processing power of the host compared to the real HW target, and by which
> factor.
>
> Regards,
> Julien
>

Taking a look at:

http://adt.cs.upb.de/quf/quf2011_proceedings.pdf

page 20 (24th page in the PDF), figure 1b, the noprof bars, I'd expect
2GHz host to be on average faster than native target. The emulation
speed depends on how core intensive vs memory intensive your workload
is. Workloads that are memory bound in the target (e.g. gzip ASCII
compression) can me emulated much faster (e.g. factor of two) than core
bound workloads (e.g. mcrypt encryption).

--
http://www.iki.fi/~ananaza/

[Qemu-devel] [PATCH] pci: implement bridge filtering

[Michael S. Tsirkin]

Support bridge filtering on top of the memory
API as suggested by Avi Kivity:

Create a memory region for the bridge's address space.  This region is
not directly added to system_memory or its descendants.  Devices under
the bridge see this region as its pci_address_space().  The region is
as large as the entire address space - it does not take into account
any windows.

For each of the three windows (pref, non-pref, vga), create an alias
with the appropriate start and size.  Map the alias into the bridge's
parent's pci_address_space(), as subregions.

Signed-off-by: Michael S. Tsirkin 
---

The below seems to work fine for me so I applied this.
Still need to test bridge filtering, any help with this
appreciated.

 hw/pci.c   |   70 +---
 hw/pci.h   |2 -
 hw/pci_bridge.c|   89 +---
 hw/pci_internals.h |3 ++
 4 files changed, 89 insertions(+), 75 deletions(-)

diff --git a/hw/pci.c b/hw/pci.c
index 57ff7b1..56dfa18 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -889,7 +889,6 @@ void pci_register_bar(PCIDevice *pci_dev, int region_num,
 r = &pci_dev->io_regions[region_num];
 r->addr = PCI_BAR_UNMAPPED;
 r->size = size;
-r->filtered_size = size;
 r->type = type;
 r->memory = NULL;
 
@@ -920,41 +919,6 @@ pcibus_t pci_get_bar_addr(PCIDevice *pci_dev, int 
region_num)
 return pci_dev->io_regions[region_num].addr;
 }
 
-static void pci_bridge_filter(PCIDevice *d, pcibus_t *addr, pcibus_t *size,
-  uint8_t type)
-{
-pcibus_t base = *addr;
-pcibus_t limit = *addr + *size - 1;
-PCIDevice *br;
-
-for (br = d->bus->parent_dev; br; br = br->bus->parent_dev) {
-uint16_t cmd = pci_get_word(d->config + PCI_COMMAND);
-
-if (type & PCI_BASE_ADDRESS_SPACE_IO) {
-if (!(cmd & PCI_COMMAND_IO)) {
-goto no_map;
-}
-} else {
-if (!(cmd & PCI_COMMAND_MEMORY)) {
-goto no_map;
-}
-}
-
-base = MAX(base, pci_bridge_get_base(br, type));
-limit = MIN(limit, pci_bridge_get_limit(br, type));
-}
-
-if (base > limit) {
-goto no_map;
-}
-*addr = base;
-*size = limit - base + 1;
-return;
-no_map:
-*addr = PCI_BAR_UNMAPPED;
-*size = 0;
-}
-
 static pcibus_t pci_bar_address(PCIDevice *d,
int reg, uint8_t type, pcibus_t size)
 {
@@ -1024,7 +988,7 @@ static void pci_update_mappings(PCIDevice *d)
 {
 PCIIORegion *r;
 int i;
-pcibus_t new_addr, filtered_size;
+pcibus_t new_addr;
 
 for(i = 0; i < PCI_NUM_REGIONS; i++) {
 r = &d->io_regions[i];
@@ -1035,14 +999,8 @@ static void pci_update_mappings(PCIDevice *d)
 
 new_addr = pci_bar_address(d, i, r->type, r->size);
 
-/* bridge filtering */
-filtered_size = r->size;
-if (new_addr != PCI_BAR_UNMAPPED) {
-pci_bridge_filter(d, &new_addr, &filtered_size, r->type);
-}
-
 /* This bar isn't changed */
-if (new_addr == r->addr && filtered_size == r->filtered_size)
+if (new_addr == r->addr)
 continue;
 
 /* now do the real mapping */
@@ -1050,15 +1008,7 @@ static void pci_update_mappings(PCIDevice *d)
 memory_region_del_subregion(r->address_space, r->memory);
 }
 r->addr = new_addr;
-r->filtered_size = filtered_size;
 if (r->addr != PCI_BAR_UNMAPPED) {
-/*
- * TODO: currently almost all the map funcions assumes
- * filtered_size == size and addr & ~(size - 1) == addr.
- * However with bridge filtering, they aren't always true.
- * Teach them such cases, such that filtered_size < size and
- * addr & (size - 1) != 0.
- */
 if (r->type & PCI_BASE_ADDRESS_SPACE_IO) {
 memory_region_add_subregion_overlap(r->address_space,
 r->addr,
@@ -1576,22 +1526,6 @@ PCIDevice *pci_nic_init_nofail(NICInfo *nd, const char 
*default_model,
 return res;
 }
 
-static void pci_bridge_update_mappings_fn(PCIBus *b, PCIDevice *d)
-{
-pci_update_mappings(d);
-}
-
-void pci_bridge_update_mappings(PCIBus *b)
-{
-PCIBus *child;
-
-pci_for_each_device_under_bus(b, pci_bridge_update_mappings_fn);
-
-QLIST_FOREACH(child, &b->child, sibling) {
-pci_bridge_update_mappings(child);
-}
-}
-
 /* Whether a given bus number is in range of the secondary
  * bus of the given bridge device. */
 static bool pci_secondary_bus_in_range(PCIDevice *dev, int bus_num)
diff --git a/hw/pci.h b/hw/pci.h
index 391217e..65e1568 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -90,7 +90,6 @@ typedef struct PCIIORegion {
 pcibus_t addr; /* current PCI mapping address. -1 means not mapped */
 #define PCI_BAR_UNMAPPED (~(pcibus_t)0)
 pcibus_t

Re: [Qemu-devel] [PATCH v4 01/32] target-xtensa: add target stubs

[Blue Swirl]

On Thu, Sep 1, 2011 at 8:45 PM, Max Filippov  wrote:
> Signed-off-by: Max Filippov 
> ---
>  Makefile.target           |    2 +
>  arch_init.c               |    2 +
>  arch_init.h               |    1 +
>  cpu-exec.c                |    2 +
>  elf.h                     |    2 +
>  hw/xtensa_pic.c           |   38 ++
>  target-xtensa/cpu.h       |   95 
> +
>  target-xtensa/helper.c    |   73 ++
>  target-xtensa/machine.c   |   38 ++
>  target-xtensa/op_helper.c |   52 
>  target-xtensa/translate.c |   67 +++
>  11 files changed, 372 insertions(+), 0 deletions(-)
>  create mode 100644 hw/xtensa_pic.c
>  create mode 100644 target-xtensa/cpu.h
>  create mode 100644 target-xtensa/helper.c
>  create mode 100644 target-xtensa/machine.c
>  create mode 100644 target-xtensa/op_helper.c
>  create mode 100644 target-xtensa/translate.c
>
> diff --git a/Makefile.target b/Makefile.target
> index 07af4d4..b833c10 100644
> --- a/Makefile.target
> +++ b/Makefile.target
> @@ -373,6 +373,8 @@ obj-s390x-y = s390-virtio-bus.o s390-virtio.o
>  obj-alpha-y = i8259.o mc146818rtc.o
>  obj-alpha-y += vga.o cirrus_vga.o
>
> +obj-xtensa-y += xtensa_pic.o
> +
>  main.o: QEMU_CFLAGS+=$(GPROF_CFLAGS)
>
>  monitor.o: hmp-commands.h qmp-commands.h
> diff --git a/arch_init.c b/arch_init.c
> index 567ab32..9a5a0e3 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -78,6 +78,8 @@ const char arch_config_name[] = CONFIG_QEMU_CONFDIR 
> "/target-" TARGET_ARCH ".con
>  #define QEMU_ARCH QEMU_ARCH_SH4
>  #elif defined(TARGET_SPARC)
>  #define QEMU_ARCH QEMU_ARCH_SPARC
> +#elif defined(TARGET_XTENSA)
> +#define QEMU_ARCH QEMU_ARCH_XTENSA
>  #endif
>
>  const uint32_t arch_type = QEMU_ARCH;
> diff --git a/arch_init.h b/arch_init.h
> index 2de9f08..a74187a 100644
> --- a/arch_init.h
> +++ b/arch_init.h
> @@ -17,6 +17,7 @@ enum {
>     QEMU_ARCH_S390X = 512,
>     QEMU_ARCH_SH4 = 1024,
>     QEMU_ARCH_SPARC = 2048,
> +    QEMU_ARCH_XTENSA = 4096,
>  };
>
>  extern const uint32_t arch_type;
> diff --git a/cpu-exec.c b/cpu-exec.c
> index de0d716..3fce033 100644
> --- a/cpu-exec.c
> +++ b/cpu-exec.c
> @@ -222,6 +222,7 @@ int cpu_exec(CPUState *env)
>  #elif defined(TARGET_SH4)
>  #elif defined(TARGET_CRIS)
>  #elif defined(TARGET_S390X)
> +#elif defined(TARGET_XTENSA)
>     /* X */
>  #else
>  #error unsupported target CPU
> @@ -616,6 +617,7 @@ int cpu_exec(CPUState *env)
>  #elif defined(TARGET_ALPHA)
>  #elif defined(TARGET_CRIS)
>  #elif defined(TARGET_S390X)
> +#elif defined(TARGET_XTENSA)
>     /* X */
>  #else
>  #error unsupported target CPU
> diff --git a/elf.h b/elf.h
> index ffcac7e..2e05d34 100644
> --- a/elf.h
> +++ b/elf.h
> @@ -125,6 +125,8 @@ typedef int64_t  Elf64_Sxword;
>  #define EM_MICROBLAZE      189
>  #define EM_MICROBLAZE_OLD  0xBAAB
>
> +#define EM_XTENSA   94      /* Tensilica Xtensa */
> +
>  /* This is the info that is needed to parse the dynamic section of the file 
> */
>  #define DT_NULL                0
>  #define DT_NEEDED      1
> diff --git a/hw/xtensa_pic.c b/hw/xtensa_pic.c
> new file mode 100644
> index 000..91a5445
> --- /dev/null
> +++ b/hw/xtensa_pic.c
> @@ -0,0 +1,38 @@
> +/*
> + * Copyright (c) 2011, Max Filippov, Open Source and Linux Lab.
> + * All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions are 
> met:
> + *     * Redistributions of source code must retain the above copyright
> + *       notice, this list of conditions and the following disclaimer.
> + *     * Redistributions in binary form must reproduce the above copyright
> + *       notice, this list of conditions and the following disclaimer in the
> + *       documentation and/or other materials provided with the distribution.
> + *     * Neither the name of the Open Source and Linux Lab nor the
> + *       names of its contributors may be used to endorse or promote products
> + *       derived from this software without specific prior written 
> permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS 
> IS"
> + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
> + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
> + * ARE DISCLAIMED. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY
> + * DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
> + * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR 
> SERVICES;
> + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED 
> AND
> + * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF 
> THIS
> + * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
>

Re: [Qemu-devel] [PATCH v4 06/32] target-xtensa: add sample board

[Blue Swirl]

On Thu, Sep 1, 2011 at 8:45 PM, Max Filippov  wrote:
> Sample board and sample CPU core are used for debug and may be used for
> development of custom SoC emulators.
>
> This board has two fixed size memory regions for DTCM and ITCM and
> variable length SRAM region.
>
> Signed-off-by: Max Filippov 
> ---
>  Makefile.target    |    1 +
>  hw/xtensa_sample.c |  105 
> 
>  2 files changed, 106 insertions(+), 0 deletions(-)
>  create mode 100644 hw/xtensa_sample.c
>
> diff --git a/Makefile.target b/Makefile.target
> index b833c10..98455e3 100644
> --- a/Makefile.target
> +++ b/Makefile.target
> @@ -374,6 +374,7 @@ obj-alpha-y = i8259.o mc146818rtc.o
>  obj-alpha-y += vga.o cirrus_vga.o
>
>  obj-xtensa-y += xtensa_pic.o
> +obj-xtensa-y += xtensa_sample.o
>
>  main.o: QEMU_CFLAGS+=$(GPROF_CFLAGS)
>
> diff --git a/hw/xtensa_sample.c b/hw/xtensa_sample.c
> new file mode 100644
> index 000..9f7733b
> --- /dev/null
> +++ b/hw/xtensa_sample.c
> @@ -0,0 +1,105 @@
> +/*
> + * Copyright (c) 2011, Max Filippov, Open Source and Linux Lab.
> + * All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions are 
> met:
> + *     * Redistributions of source code must retain the above copyright
> + *       notice, this list of conditions and the following disclaimer.
> + *     * Redistributions in binary form must reproduce the above copyright
> + *       notice, this list of conditions and the following disclaimer in the
> + *       documentation and/or other materials provided with the distribution.
> + *     * Neither the name of the Open Source and Linux Lab nor the
> + *       names of its contributors may be used to endorse or promote products
> + *       derived from this software without specific prior written 
> permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS 
> IS"
> + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
> + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
> + * ARE DISCLAIMED. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY
> + * DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
> + * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR 
> SERVICES;
> + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED 
> AND
> + * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF 
> THIS
> + * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +#include "sysemu.h"
> +#include "boards.h"
> +#include "loader.h"
> +#include "elf.h"
> +
> +static void xtensa_sample_reset(void *env)
> +{
> +    cpu_reset(env);
> +}
> +
> +static void xtensa_init(ram_addr_t ram_size,
> +        const char *boot_device,
> +        const char *kernel_filename, const char *kernel_cmdline,
> +        const char *initrd_filename, const char *cpu_model)
> +{
> +    CPUState *env = NULL;
> +    ram_addr_t ram_offset;
> +    int n;
> +
> +    for (n = 0; n < smp_cpus; n++) {
> +        env = cpu_init(cpu_model);
> +        if (!env) {
> +            fprintf(stderr, "Unable to find CPU definition\n");
> +            exit(1);
> +        }
> +        qemu_register_reset(xtensa_sample_reset, env);
> +    }
> +
> +    ram_offset = qemu_ram_alloc(NULL, "xtensa.dram", 0x1);
> +    cpu_register_physical_memory(0x5ffd, 0x1, ram_offset);
> +
> +    ram_offset = qemu_ram_alloc(NULL, "xtensa.iram", 0x2);
> +    cpu_register_physical_memory(0x5ffe, 0x2, ram_offset);
> +
> +    ram_offset = qemu_ram_alloc(NULL, "xtensa.sram", ram_size);
> +    cpu_register_physical_memory(0x6000, ram_size, ram_offset);

These should be converted to the new memory API. Please see pc.c for reference.

> +
> +    if (kernel_filename) {
> +        uint64_t elf_entry;
> +        uint64_t elf_lowaddr;
> +#ifdef TARGET_WORDS_BIGENDIAN
> +        int success = load_elf(kernel_filename, NULL, NULL, &elf_entry,
> +                &elf_lowaddr, NULL, 1, ELF_MACHINE, 0);
> +#else
> +        int success = load_elf(kernel_filename, NULL, NULL, &elf_entry,
> +                &elf_lowaddr, NULL, 0, ELF_MACHINE, 0);
> +#endif
> +        if (success > 0) {
> +            env->pc = elf_entry;
> +        }
> +    }
> +}
> +
> +static void xtensa_sample_init(ram_addr_t ram_size,
> +                     const char *boot_device,
> +                     const char *kernel_filename, const char *kernel_cmdline,
> +                     const char *initrd_filename, const char *cpu_model)
> +{
> +    if (!cpu_model) {
> +        cpu_model = "sample-xtensa-core";
> +    }
> +    xtensa_init(ram_size, boot_device, kernel_filename, kernel_cmdline,
> +                  initrd_filename, cpu_model);
> +}
> +
> +static QEMUMachine xten

Re: [Qemu-devel] [PATCH v4 09/32] target-xtensa: add special and user registers

[Blue Swirl]

On Thu, Sep 1, 2011 at 8:45 PM, Max Filippov  wrote:
> Special Registers hold the majority of the state added to the processor
> by the options. See ISA, 5.3 for details.
>
> User Registers hold state added in support of designer's TIE and in some
> cases of options that Tensilica provides. See ISA, 5.4 for details.
>
> Only registers mapped in sregnames or uregnames are considered valid.
>
> Signed-off-by: Max Filippov 
> ---
>  target-xtensa/cpu.h       |    7 ++
>  target-xtensa/translate.c |   47 +++-
>  2 files changed, 52 insertions(+), 2 deletions(-)
>
> diff --git a/target-xtensa/cpu.h b/target-xtensa/cpu.h
> index c323891..8c3fe2e 100644
> --- a/target-xtensa/cpu.h
> +++ b/target-xtensa/cpu.h
> @@ -99,6 +99,12 @@ enum {
>     XTENSA_OPTION_TRACE_PORT,
>  };
>
> +enum {
> +    THREADPTR = 231,
> +    FCR = 232,
> +    FSR = 233,
> +};
> +
>  typedef struct XtensaConfig {
>     const char *name;
>     uint64_t options;
> @@ -109,6 +115,7 @@ typedef struct CPUXtensaState {
>     uint32_t regs[16];
>     uint32_t pc;
>     uint32_t sregs[256];
> +    uint32_t uregs[256];
>
>     CPU_COMMON
>  } CPUXtensaState;
> diff --git a/target-xtensa/translate.c b/target-xtensa/translate.c
> index 26cce83..3c8d877 100644
> --- a/target-xtensa/translate.c
> +++ b/target-xtensa/translate.c
> @@ -52,9 +52,20 @@ typedef struct DisasContext {
>  static TCGv_ptr cpu_env;
>  static TCGv_i32 cpu_pc;
>  static TCGv_i32 cpu_R[16];
> +static TCGv_i32 cpu_SR[256];
> +static TCGv_i32 cpu_UR[256];
>
>  #include "gen-icount.h"
>
> +static const char * const sregnames[256] = {
> +};
> +
> +static const char * const uregnames[256] = {
> +    [THREADPTR] = "THREADPTR",
> +    [FCR] = "FCR",
> +    [FSR] = "FSR",
> +};
> +
>  void xtensa_translate_init(void)
>  {
>     static const char * const regnames[] = {
> @@ -74,6 +85,22 @@ void xtensa_translate_init(void)
>                 offsetof(CPUState, regs[i]),
>                 regnames[i]);
>     }
> +
> +    for (i = 0; i < 256; ++i) {
> +        if (sregnames[i]) {
> +            cpu_SR[i] = tcg_global_mem_new_i32(TCG_AREG0,
> +                    offsetof(CPUState, sregs[i]),
> +                    sregnames[i]);
> +        }
> +    }
> +
> +    for (i = 0; i < 256; ++i) {
> +        if (uregnames[i]) {
> +            cpu_UR[i] = tcg_global_mem_new_i32(TCG_AREG0,
> +                    offsetof(CPUState, uregs[i]),
> +                    uregnames[i]);
> +        }
> +    }
>  #define GEN_HELPER 2
>  #include "helpers.h"
>  }
> @@ -784,9 +811,25 @@ void gen_intermediate_code_pc(CPUState *env, 
> TranslationBlock *tb)
>  void cpu_dump_state(CPUState *env, FILE *f, fprintf_function cpu_fprintf,
>         int flags)
>  {
> -    int i;
> +    int i, j;
> +
> +    cpu_fprintf(f, "PC=%08x\n\n", env->pc);
> +
> +    for (i = j = 0; i < 256; ++i)

Braces, please.

> +        if (sregnames[i]) {
> +            cpu_fprintf(f, "%s=%08x%c", sregnames[i], env->sregs[i],
> +                    (j++ % 4) == 3 ? '\n' : ' ');
> +        }
> +
> +    cpu_fprintf(f, (j % 4) == 0 ? "\n" : "\n\n");
> +
> +    for (i = j = 0; i < 256; ++i)

Also here.

> +        if (uregnames[i]) {
> +            cpu_fprintf(f, "%s=%08x%c", uregnames[i], env->uregs[i],
> +                    (j++ % 4) == 3 ? '\n' : ' ');
> +        }
>
> -    cpu_fprintf(f, "PC=%08x\n", env->pc);
> +    cpu_fprintf(f, (j % 4) == 0 ? "\n" : "\n\n");
>
>     for (i = 0; i < 16; ++i)
>         cpu_fprintf(f, "A%02d=%08x%c", i, env->regs[i],
> --
> 1.7.6
>
>
>

Re: [Qemu-devel] [PATCH v3] rbd: fix leak in qemu_rbd_open failure paths

[Stefan Hajnoczi]

On Sun, Sep 4, 2011 at 5:19 PM, Sage Weil  wrote:
> Fix leak of s->snap in failure path.  Simplify error paths for the whole
> function.
>
> Reported-by: Stefan Hajnoczi 
> Signed-off-by: Sage Weil 
> ---
>  block/rbd.c |   28 +---
>  1 files changed, 13 insertions(+), 15 deletions(-)

Reviewed-by: Stefan Hajnoczi 

Re: [Qemu-devel] [PATCH v4 17/32] target-xtensa: implement exceptions

[Blue Swirl]

On Thu, Sep 1, 2011 at 8:45 PM, Max Filippov  wrote:
> - mark privileged opcodes with ring check;
> - make debug exception on exception handler entry.
>
> Signed-off-by: Max Filippov 
> ---
>  cpu-exec.c                |    6 +++
>  target-xtensa/cpu.h       |   67 
>  target-xtensa/helper.c    |   37 +++-
>  target-xtensa/helpers.h   |    2 +
>  target-xtensa/op_helper.c |   29 
>  target-xtensa/translate.c |  107 ++--
>  6 files changed, 242 insertions(+), 6 deletions(-)
>
> diff --git a/cpu-exec.c b/cpu-exec.c
> index 3fce033..2fc37d8 100644
> --- a/cpu-exec.c
> +++ b/cpu-exec.c
> @@ -488,6 +488,12 @@ int cpu_exec(CPUState *env)
>                         do_interrupt(env);
>                         next_tb = 0;
>                     }
> +#elif defined(TARGET_XTENSA)
> +                    if (interrupt_request & CPU_INTERRUPT_HARD) {
> +                        env->exception_index = EXC_IRQ;
> +                        do_interrupt(env);
> +                        next_tb = 0;
> +                    }
>  #endif
>                    /* Don't use the cached interrupt_request value,
>                       do_interrupt may have updated the EXITTB flag. */
> diff --git a/target-xtensa/cpu.h b/target-xtensa/cpu.h
> index 939222c..cae6637 100644
> --- a/target-xtensa/cpu.h
> +++ b/target-xtensa/cpu.h
> @@ -108,7 +108,12 @@ enum {
>  enum {
>     SAR = 3,
>     SCOMPARE1 = 12,
> +    EPC1 = 177,
> +    DEPC = 192,
> +    EXCSAVE1 = 209,
>     PS = 230,
> +    EXCCAUSE = 232,
> +    EXCVADDR = 238,
>  };
>
>  #define PS_INTLEVEL 0xf
> @@ -129,9 +134,60 @@ enum {
>
>  #define PS_WOE 0x4
>
> +enum {
> +    /* Static vectors */
> +    EXC_RESET,
> +    EXC_MEMORY_ERROR,
> +
> +    /* Dynamic vectors */
> +    EXC_WINDOW_OVERFLOW4,
> +    EXC_WINDOW_UNDERFLOW4,
> +    EXC_WINDOW_OVERFLOW8,
> +    EXC_WINDOW_UNDERFLOW8,
> +    EXC_WINDOW_OVERFLOW12,
> +    EXC_WINDOW_UNDERFLOW12,
> +    EXC_IRQ,
> +    EXC_KERNEL,
> +    EXC_USER,
> +    EXC_DOUBLE,
> +    EXC_MAX
> +};
> +
> +enum {
> +    ILLEGAL_INSTRUCTION_CAUSE = 0,
> +    SYSCALL_CAUSE,
> +    INSTRUCTION_FETCH_ERROR_CAUSE,
> +    LOAD_STORE_ERROR_CAUSE,
> +    LEVEL1_INTERRUPT_CAUSE,
> +    ALLOCA_CAUSE,
> +    INTEGER_DIVIDE_BY_ZERO_CAUSE,
> +    PRIVILEGED_CAUSE = 8,
> +    LOAD_STORE_ALIGNMENT_CAUSE,
> +
> +    INSTR_PIF_DATA_ERROR_CAUSE = 12,
> +    LOAD_STORE_PIF_DATA_ERROR_CAUSE,
> +    INSTR_PIF_ADDR_ERROR_CAUSE,
> +    LOAD_STORE_PIF_ADDR_ERROR_CAUSE,
> +
> +    INST_TLB_MISS_CAUSE,
> +    INST_TLB_MULTI_HIT_CAUSE,
> +    INST_FETCH_PRIVILEGE_CAUSE,
> +    INST_FETCH_PROHIBITED_CAUSE = 20,
> +    LOAD_STORE_TLB_MISS_CAUSE = 24,
> +    LOAD_STORE_TLB_MULTI_HIT_CAUSE,
> +    LOAD_STORE_PRIVILEGE_CAUSE,
> +    LOAD_PROHIBITED_CAUSE = 28,
> +    STORE_PROHIBITED_CAUSE,
> +
> +    COPROCESSOR0_DISABLED = 32,
> +};
> +
>  typedef struct XtensaConfig {
>     const char *name;
>     uint64_t options;
> +    int excm_level;
> +    int ndepc;
> +    uint32_t exception_vector[EXC_MAX];
>  } XtensaConfig;
>
>  typedef struct CPUXtensaState {
> @@ -141,6 +197,8 @@ typedef struct CPUXtensaState {
>     uint32_t sregs[256];
>     uint32_t uregs[256];
>
> +    int exception_taken;
> +
>     CPU_COMMON
>  } CPUXtensaState;
>
> @@ -164,6 +222,15 @@ static inline bool xtensa_option_enabled(const 
> XtensaConfig *config, int opt)
>     return (config->options & XTENSA_OPTION_BIT(opt)) != 0;
>  }
>
> +static inline int xtensa_get_cintlevel(const CPUState *env)
> +{
> +    int level = (env->sregs[PS] & PS_INTLEVEL) >> PS_INTLEVEL_SHIFT;
> +    if ((env->sregs[PS] & PS_EXCM) && env->config->excm_level > level) {
> +        level = env->config->excm_level;
> +    }
> +    return level;
> +}
> +
>  static inline int xtensa_get_ring(const CPUState *env)
>  {
>     if (xtensa_option_enabled(env->config, XTENSA_OPTION_MMU)) {
> diff --git a/target-xtensa/helper.c b/target-xtensa/helper.c
> index 83b8a04..44ebb9f 100644
> --- a/target-xtensa/helper.c
> +++ b/target-xtensa/helper.c
> @@ -36,7 +36,8 @@
>
>  void cpu_reset(CPUXtensaState *env)
>  {
> -    env->pc = 0;
> +    env->exception_taken = 0;
> +    env->pc = env->config->exception_vector[EXC_RESET];
>     env->sregs[PS] = 0x1f;
>  }
>
> @@ -44,6 +45,20 @@ static const XtensaConfig core_config[] = {
>     {
>         .name = "sample-xtensa-core",
>         .options = -1,
> +        .ndepc = 1,
> +        .excm_level = 16,
> +        .exception_vector = {
> +            [EXC_RESET] = 0x5fff8000,
> +            [EXC_WINDOW_OVERFLOW4] = 0x5fff8400,
> +            [EXC_WINDOW_UNDERFLOW4] = 0x5fff8440,
> +            [EXC_WINDOW_OVERFLOW8] = 0x5fff8480,
> +            [EXC_WINDOW_UNDERFLOW8] = 0x5fff84c0,
> +            [EXC_WINDOW_OVERFLOW12] = 0x5fff8500,
> +            [EXC_WINDOW_UNDERFLOW12] = 0x5fff8540,
> +            [EXC_KERNEL] = 0x5fff861c,
> +            [EXC_USER] = 0x5fff863c,
> +            [EXC_DOUBLE] = 0x5fff865c,
> + 

Re: [Qemu-devel] [PATCH v4 19/32] target-xtensa: implement windowed registers

[Blue Swirl]

On Thu, Sep 1, 2011 at 8:45 PM, Max Filippov  wrote:
> See ISA, 4.7.1 for details.
>
> Physical registers and currently visible window are separate fields in
> CPUEnv. Only current window is accessible to TCG. On operations that
> change window base helpers copy current window to and from physical
> registers.
>
> Window overflow check described in 4.7.1.3 is in separate patch.
>
> Signed-off-by: Max Filippov 
> ---
>  target-xtensa/cpu.h       |    8 ++
>  target-xtensa/helper.c    |    1 +
>  target-xtensa/helpers.h   |    8 ++
>  target-xtensa/op_helper.c |  185 
> +
>  target-xtensa/translate.c |  144 +--
>  5 files changed, 337 insertions(+), 9 deletions(-)
>
> diff --git a/target-xtensa/cpu.h b/target-xtensa/cpu.h
> index cae6637..7e662f5 100644
> --- a/target-xtensa/cpu.h
> +++ b/target-xtensa/cpu.h
> @@ -108,6 +108,8 @@ enum {
>  enum {
>     SAR = 3,
>     SCOMPARE1 = 12,
> +    WINDOW_BASE = 72,
> +    WINDOW_START = 73,
>     EPC1 = 177,
>     DEPC = 192,
>     EXCSAVE1 = 209,
> @@ -134,6 +136,8 @@ enum {
>
>  #define PS_WOE 0x4
>
> +#define MAX_NAREG 64
> +
>  enum {
>     /* Static vectors */
>     EXC_RESET,
> @@ -185,6 +189,7 @@ enum {
>  typedef struct XtensaConfig {
>     const char *name;
>     uint64_t options;
> +    unsigned nareg;
>     int excm_level;
>     int ndepc;
>     uint32_t exception_vector[EXC_MAX];
> @@ -196,6 +201,7 @@ typedef struct CPUXtensaState {
>     uint32_t pc;
>     uint32_t sregs[256];
>     uint32_t uregs[256];
> +    uint32_t phys_regs[MAX_NAREG];
>
>     int exception_taken;
>
> @@ -214,6 +220,8 @@ int cpu_xtensa_exec(CPUXtensaState *s);
>  void do_interrupt(CPUXtensaState *s);
>  int cpu_xtensa_signal_handler(int host_signum, void *pinfo, void *puc);
>  void xtensa_cpu_list(FILE *f, fprintf_function cpu_fprintf);
> +void xtensa_sync_window_from_phys(CPUState *env);
> +void xtensa_sync_phys_from_window(CPUState *env);
>
>  #define XTENSA_OPTION_BIT(opt) (((uint64_t)1) << (opt))
>
> diff --git a/target-xtensa/helper.c b/target-xtensa/helper.c
> index 44ebb9f..4f86934 100644
> --- a/target-xtensa/helper.c
> +++ b/target-xtensa/helper.c
> @@ -45,6 +45,7 @@ static const XtensaConfig core_config[] = {
>     {
>         .name = "sample-xtensa-core",
>         .options = -1,
> +        .nareg = 64,
>         .ndepc = 1,
>         .excm_level = 16,
>         .exception_vector = {
> diff --git a/target-xtensa/helpers.h b/target-xtensa/helpers.h
> index 6329c43..0971fde 100644
> --- a/target-xtensa/helpers.h
> +++ b/target-xtensa/helpers.h
> @@ -5,5 +5,13 @@ DEF_HELPER_2(exception_cause, void, i32, i32)
>  DEF_HELPER_3(exception_cause_vaddr, void, i32, i32, i32)
>  DEF_HELPER_1(nsa, i32, i32)
>  DEF_HELPER_1(nsau, i32, i32)
> +DEF_HELPER_1(wsr_windowbase, void, i32)
> +DEF_HELPER_3(entry, void, i32, i32, i32)
> +DEF_HELPER_1(retw, i32, i32)
> +DEF_HELPER_1(rotw, void, i32)
> +DEF_HELPER_2(window_check, void, i32, i32)
> +DEF_HELPER_0(restore_owb, void)
> +DEF_HELPER_1(movsp, void, i32)
> +DEF_HELPER_0(dump_state, void)
>
>  #include "def-helper.h"
> diff --git a/target-xtensa/op_helper.c b/target-xtensa/op_helper.c
> index 794a834..7f75422 100644
> --- a/target-xtensa/op_helper.c
> +++ b/target-xtensa/op_helper.c
> @@ -100,3 +100,188 @@ uint32_t HELPER(nsau)(uint32_t v)
>  {
>     return v ? clz32(v) : 32;
>  }
> +
> +static void copy_window_from_phys(CPUState *env,
> +        uint32_t window, uint32_t phys, uint32_t n)
> +{
> +    assert(phys < env->config->nareg);
> +    if (phys + n <= env->config->nareg) {
> +        memcpy(env->regs + window, env->phys_regs + phys,
> +                n * sizeof(uint32_t));
> +    } else {
> +        uint32_t n1 = env->config->nareg - phys;
> +        memcpy(env->regs + window, env->phys_regs + phys,
> +                n1 * sizeof(uint32_t));
> +        memcpy(env->regs + window + n1, env->phys_regs,
> +                (n - n1) * sizeof(uint32_t));
> +    }
> +}
> +
> +static void copy_phys_from_window(CPUState *env,
> +        uint32_t phys, uint32_t window, uint32_t n)
> +{
> +    assert(phys < env->config->nareg);
> +    if (phys + n <= env->config->nareg) {
> +        memcpy(env->phys_regs + phys, env->regs + window,
> +                n * sizeof(uint32_t));
> +    } else {
> +        uint32_t n1 = env->config->nareg - phys;
> +        memcpy(env->phys_regs + phys, env->regs + window,
> +                n1 * sizeof(uint32_t));
> +        memcpy(env->phys_regs, env->regs + window + n1,
> +                (n - n1) * sizeof(uint32_t));
> +    }
> +}
> +
> +
> +#define WINDOWBASE_BOUND(a) ((a) & (env->config->nareg / 4 - 1))
> +#define WINDOW_BOUND(a) ((a) & (env->config->nareg - 1))

'env' could be a parameter to the macro, though inline functions are
preferred over macros these days.

> +#define WINDOWSTART_BIT(a) (1 << WINDOWBASE_BOUND(a))
> +
> +void xtensa_sync_window_from_phys(CPUState *env)
> +{
> +    copy_window_from_phys(env, 0, env->sregs[WIN

Re: [Qemu-devel] [PATCH v4 22/32] target-xtensa: implement unaligned exception option

[Blue Swirl]

On Thu, Sep 1, 2011 at 8:45 PM, Max Filippov  wrote:
> See ISA, 4.4.4 for details.
>
> Correct (aligned as per ISA) address for unaligned access is generated
> in case this option is not enabled.
>
> Signed-off-by: Max Filippov 
> ---
>  target-xtensa/helper.c    |    4 ++-
>  target-xtensa/op_helper.c |   26 
>  target-xtensa/translate.c |   47 ++--
>  3 files changed, 73 insertions(+), 4 deletions(-)
>
> diff --git a/target-xtensa/helper.c b/target-xtensa/helper.c
> index 074207f..bcd6c77 100644
> --- a/target-xtensa/helper.c
> +++ b/target-xtensa/helper.c
> @@ -45,7 +45,9 @@ void cpu_reset(CPUXtensaState *env)
>  static const XtensaConfig core_config[] = {
>     {
>         .name = "sample-xtensa-core",
> -        .options = -1,
> +        .options = -1 ^
> +            (XTENSA_OPTION_BIT(XTENSA_OPTION_HW_ALIGNMENT) |
> +             XTENSA_OPTION_BIT(XTENSA_OPTION_MMU)),
>         .nareg = 64,
>         .ndepc = 1,
>         .excm_level = 16,
> diff --git a/target-xtensa/op_helper.c b/target-xtensa/op_helper.c
> index 26d2d2e..8577f5d 100644
> --- a/target-xtensa/op_helper.c
> +++ b/target-xtensa/op_helper.c
> @@ -30,6 +30,10 @@
>  #include "helpers.h"
>  #include "host-utils.h"
>
> +static void do_unaligned_access(target_ulong addr, int is_write, int is_user,
> +        void *retaddr);
> +
> +#define ALIGNED_ONLY
>  #define MMUSUFFIX _mmu
>
>  #define SHIFT 0
> @@ -44,6 +48,28 @@
>  #define SHIFT 3
>  #include "softmmu_template.h"
>
> +static void do_restore_state(void *pc_ptr)
> +{
> +    TranslationBlock *tb;
> +    uint32_t pc = (uint32_t)(intptr_t)pc_ptr;
> +
> +    tb = tb_find_pc(pc);
> +    if (tb) {
> +        cpu_restore_state(tb, env, pc);
> +    }
> +}
> +
> +static void do_unaligned_access(target_ulong addr, int is_write, int is_user,
> +        void *retaddr)
> +{
> +    if (xtensa_option_enabled(env->config, 
> XTENSA_OPTION_UNALIGNED_EXCEPTION) &&
> +            !xtensa_option_enabled(env->config, XTENSA_OPTION_HW_ALIGNMENT)) 
> {
> +        do_restore_state(retaddr);
> +        HELPER(exception_cause_vaddr)(
> +                env->pc, LOAD_STORE_ALIGNMENT_CAUSE, addr);
> +    }
> +}
> +
>  void tlb_fill(target_ulong addr, int is_write, int mmu_idx, void *retaddr)
>  {
>     tlb_set_page(cpu_single_env,
> diff --git a/target-xtensa/translate.c b/target-xtensa/translate.c
> index 7bcbe4f..ad2b699 100644
> --- a/target-xtensa/translate.c
> +++ b/target-xtensa/translate.c
> @@ -203,6 +203,16 @@ static void gen_exception_cause(DisasContext *dc, 
> uint32_t cause)
>     tcg_temp_free(_cause);
>  }
>
> +static void gen_exception_cause_vaddr(DisasContext *dc, uint32_t cause,
> +        TCGv_i32 vaddr)
> +{
> +    TCGv_i32 _pc = tcg_const_i32(dc->pc);
> +    TCGv_i32 _cause = tcg_const_i32(cause);

Underscores.

> +    gen_helper_exception_cause_vaddr(_pc, _cause, vaddr);
> +    tcg_temp_free(_pc);
> +    tcg_temp_free(_cause);
> +}
> +
>  static void gen_check_privilege(DisasContext *dc)
>  {
>     if (dc->cring) {
> @@ -397,6 +407,23 @@ static void gen_wsr(DisasContext *dc, uint32_t sr, 
> TCGv_i32 s)
>     }
>  }
>
> +static void gen_load_store_alignment(DisasContext *dc, int shift,
> +        TCGv_i32 addr, bool no_hw_alignment)
> +{
> +    if (!option_enabled(dc, XTENSA_OPTION_UNALIGNED_EXCEPTION)) {
> +        tcg_gen_andi_i32(addr, addr, ~0 << shift);
> +    } else if (option_enabled(dc, XTENSA_OPTION_HW_ALIGNMENT) &&
> +            no_hw_alignment) {
> +        int label = gen_new_label();
> +        TCGv_i32 tmp = tcg_temp_new_i32();
> +        tcg_gen_andi_i32(tmp, addr, ~(~0 << shift));
> +        tcg_gen_brcondi_i32(TCG_COND_EQ, tmp, 0, label);
> +        gen_exception_cause_vaddr(dc, LOAD_STORE_ALIGNMENT_CAUSE, addr);
> +        gen_set_label(label);
> +        tcg_temp_free(tmp);
> +    }
> +}
> +
>  static void disas_xtensa_insn(DisasContext *dc)
>  {
>  #define HAS_OPTION(opt) do { \
> @@ -1339,6 +1366,9 @@ static void disas_xtensa_insn(DisasContext *dc)
>  #define gen_load_store(type, shift) do { \
>             TCGv_i32 addr = tcg_temp_new_i32(); \
>             tcg_gen_addi_i32(addr, cpu_R[RRI8_S], RRI8_IMM8 << shift); \
> +            if (shift) { \
> +                gen_load_store_alignment(dc, shift, addr, false); \
> +            } \
>             tcg_gen_qemu_##type(cpu_R[RRI8_T], addr, dc->cring); \
>             tcg_temp_free(addr); \
>         } while (0)
> @@ -1468,6 +1498,7 @@ static void disas_xtensa_insn(DisasContext *dc)
>         case 9: /*L16SI*/
>             gen_load_store(ld16s, 1);
>             break;
> +#undef gen_load_store
>
>         case 10: /*MOVI*/
>             tcg_gen_movi_i32(cpu_R[RRI8_T],
> @@ -1475,9 +1506,17 @@ static void disas_xtensa_insn(DisasContext *dc)
>                     ((RRI8_S & 0x8) ? 0xf000 : 0));
>             break;
>
> +#define gen_load_store_no_hw_align(type) do { \
> +            TCGv_i32 addr = tcg_temp_local_new_i32(); \
> +            tcg_gen_addi_i32(