Re: [Qemu-devel] [PATCH] ahci: map memory via device's address space instead of address_space_memory

[Michael S. Tsirkin]

On Thu, Jul 03, 2014 at 10:43:57AM +0200, Jan Kiszka wrote:
> On 2014-07-03 10:26, Le Tan wrote:
> > In map_page() in hw/ide/ahci.c, replace cpu_physical_memory_map() and
> > cpu_physical_memory_unmap() with dma_memory_map() and dma_memory_unmap(),
> > because ahci devices should not access memory directly but via their address
> > space. Add an AddressSpace parameter to map_page(). In order to call
> > map_page(), we should pass the AHCIState.as as the AddressSpace argument.
> 
> BTW, when doing "git grep cpu_physical_memory_map hw", there are some
> more cases that should be checked (for x86). I suppose vhost is
> incompatible with an IOMMU,

vhost can be made to work: you just need to
update its memory tables as appropriate.
But see below

> but plain virtio should work,

It doesn't: all guests pass in physical addresses at the moment.
We discussed requiring this for virtio 1.0, but in the end,
most people thought that passing through virtio devices
isn't worthwhile.
We can certainly add that as an option, with a feature bit.

If you feel otherwise, you can comment on the latest spec draft.

> same for vmxnet.
> 
> Jan
> 
> > 
> > Signed-off-by: Le Tan <address@hidden>
> > ---
> >  hw/ide/ahci.c |   21 +++++++++++----------
> >  1 file changed, 11 insertions(+), 10 deletions(-)
> > 
> > diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> > index 9bae22e..7bb0a03 100644
> > --- a/hw/ide/ahci.c
> > +++ b/hw/ide/ahci.c
> > @@ -175,17 +175,18 @@ static void ahci_trigger_irq(AHCIState *s, AHCIDevice 
> > *d,
> >      ahci_check_irq(s);
> >  }
> >  
> > -static void map_page(uint8_t **ptr, uint64_t addr, uint32_t wanted)
> > +static void map_page(AddressSpace *as, uint8_t **ptr, uint64_t addr,
> > +                     uint32_t wanted)
> >  {
> >      hwaddr len = wanted;
> >  
> >      if (*ptr) {
> > -        cpu_physical_memory_unmap(*ptr, len, 1, len);
> > +        dma_memory_unmap(as, *ptr, len, DMA_DIRECTION_FROM_DEVICE, len);
> >      }
> >  
> > -    *ptr = cpu_physical_memory_map(addr, &len, 1);
> > +    *ptr = dma_memory_map(as, addr, &len, DMA_DIRECTION_FROM_DEVICE);
> >      if (len < wanted) {
> > -        cpu_physical_memory_unmap(*ptr, len, 1, len);
> > +        dma_memory_unmap(as, *ptr, len, DMA_DIRECTION_FROM_DEVICE, len);
> >          *ptr = NULL;
> >      }
> >  }
> > @@ -198,24 +199,24 @@ static void  ahci_port_write(AHCIState *s, int port, 
> > int offset, uint32_t val)
> >      switch (offset) {
> >          case PORT_LST_ADDR:
> >              pr->lst_addr = val;
> > -            map_page(&s->dev[port].lst,
> > +            map_page(s->as, &s->dev[port].lst,
> >                       ((uint64_t)pr->lst_addr_hi << 32) | pr->lst_addr, 
> > 1024);
> >              s->dev[port].cur_cmd = NULL;
> >              break;
> >          case PORT_LST_ADDR_HI:
> >              pr->lst_addr_hi = val;
> > -            map_page(&s->dev[port].lst,
> > +            map_page(s->as, &s->dev[port].lst,
> >                       ((uint64_t)pr->lst_addr_hi << 32) | pr->lst_addr, 
> > 1024);
> >              s->dev[port].cur_cmd = NULL;
> >              break;
> >          case PORT_FIS_ADDR:
> >              pr->fis_addr = val;
> > -            map_page(&s->dev[port].res_fis,
> > +            map_page(s->as, &s->dev[port].res_fis,
> >                       ((uint64_t)pr->fis_addr_hi << 32) | pr->fis_addr, 
> > 256);
> >              break;
> >          case PORT_FIS_ADDR_HI:
> >              pr->fis_addr_hi = val;
> > -            map_page(&s->dev[port].res_fis,
> > +            map_page(s->as, &s->dev[port].res_fis,
> >                       ((uint64_t)pr->fis_addr_hi << 32) | pr->fis_addr, 
> > 256);
> >              break;
> >          case PORT_IRQ_STAT:
> > @@ -1260,9 +1261,9 @@ static int ahci_state_post_load(void *opaque, int 
> > version_id)
> >          ad = &s->dev[i];
> >          AHCIPortRegs *pr = &ad->port_regs;
> >  
> > -        map_page(&ad->lst,
> > +        map_page(s->as, &ad->lst,
> >                   ((uint64_t)pr->lst_addr_hi << 32) | pr->lst_addr, 1024);
> > -        map_page(&ad->res_fis,
> > +        map_page(s->as, &ad->res_fis,
> >                   ((uint64_t)pr->fis_addr_hi << 32) | pr->fis_addr, 256);
> >          /*
> >           * All pending i/o should be flushed out on a migrate. However,
> > 
> 
>