Re: [Qemu-devel] [PATCH 3/5] Implement fw_cfg DMA interface

[Marc Marí]

On Thu, 6 Aug 2015 23:32:50 +0200
Laszlo Ersek <address@hidden> wrote:

> On 08/06/15 23:11, Marc Marí wrote:
> > On Thu, 6 Aug 2015 22:49:12 +0200
> > Laszlo Ersek <address@hidden> wrote:
> > 
> > [...]
> > 
> >>> +static void fw_cfg_dma_mem_write(void *opaque, hwaddr addr,
> >>> +                                 uint64_t value, unsigned size)
> >>> +{
> >>> +    FWCfgState *s = opaque;
> >>> +
> >>> +    s->dma_addr = be64_to_cpu(value);
> >>> +    fw_cfg_dma_transfer(s);
> >>> +}
> >>
> >> So, this is similar to the ioport size limitation I described at
> >> the top. Namely,  I think that an Aarch32 guest won't be able to
> >> transfer a 64-bit value with a single MMIO access. (I believe
> >> double-width store instructions do exist, but they cannot be
> >> virtualized well. They trap, but the instruction syndrome register
> >> won't give enough info to the hypervisor.)
> >>
> >> Therefore, the address of the dma control structure should be
> >> passed in two 32-bit wide accesses, both for the ioport mapping
> >> and the mmio mapping. This can be done in two ways:
> >> - write the two halves to the same register, and use a latch to
> >>   identify each 2nd access
> >> - use different addresses.
> >>
> >> The latch sucks, because the guest has no way to bring the register
> >> to a known good state. Therefore:
> >>
> >> In the ioport mapped case, the port range should go up to 0x519,
> >> and two outl's are going to be necessary in the guest. The
> >> documentation should spell out which outl (@ 0x512 or @ 0x516)
> >> will trigger the actual transfer.
> >>
> >> (I vaguely recall that someone already described this, but I can't
> >> find the message!)
> > 
> > Previous answer to this patch, by Kevin O'Connor:
> > 
> > "So, I think this code needs to be able to handle a 32bit write to a
> > high bits address and then store those bits until the 32bit write to
> > the low bits address occurs.  (I'd also recommend that after every
> > dma transfer the stored high bits are reset to zero so that the
> > common case of a 32bit address can be performed with a single 32bit
> > write to the low bits field.)"
> > 
> > It's easier to do it this way.
> 
> Thank you for the pointer. :) I don't remember this answer, at least
> not consciously.
> 
> But, "this way" is not different from my suggestion. It just has more
> details filled in (and therefore it is admittedly a smarter, more
> elaborate suggestion than mine).
> 
> - I asked for two separate addresses, and Kevin had asked for two
>   separate addresses too. (The idea that one half is stored until the
>   other half is written was implicit in my suggestion.)
> 
> - I requested that the docs spell out which address would trigger the
>   dma. Kevin had specifically suggested that the low bits address
>   trigger it.
> 
> - The clearing of the stored high bits is extra smartness in Kevin's
>   suggestion (I'm jealous for not thinking of that :)), but that will
>   require more code (one line, probably), which is not "easier". :)
> 
> We're in violent agreement.
> 
> (If you wanted to poke fun at me, you could say that I just repeated
> what Kevin had said, only worse. Thing is, I really don't recall
> seeing his message. Let me search my mailbox for a substring from
> your above quote... Yep, I don't have that message. My email has been
> acting up today. I only have parts of that message *within* one of
> your emails. ... Which I mostly missed too. Doing too many things at
> once, sorry.)
> 

No fun intended, just complement the answer, because you said "I
vaguely recall that someone already described this, but I can't
find the message!".

And although I did not elaborate my answer (my fault), I wanted to say:
Starting on your idea (which is similar to Kevin's), it is easier for
the guest to have the upper address as 0 and trigger on the lower
address, because this lets 32 bit guests to trigger with just one
access. Of course, this has to be documented, even if not explicitly
stated.

Marc