Re: [PATCH] make vfio and DAX cache work together

Re: [PATCH] make vfio and DAX cache work together

[Dev Audsin]

Hi Alex and David

@Alex:

Justification on why this region cannot be a DMA target for the device,

virtio-fs with DAX is currently not compatible with NIC Pass through.
When a SR-IOV VF attaches to a qemu process, vfio will try to pin the
entire DAX Window but it is empty when the guest boots and will fail.
A method to make VFIO and DAX to work together is to make vfio skip
DAX cache.

Currently DAX cache need to be set to 0, for the SR-IOV VF to be
attached to Kata containers. Enabling both SR-IOV VF and DAX work
together will potentially improve performance for workloads which are
I/O and network intensive.

@David

1. If DAX mode of virtiofs isn't yet in qemu, what is the best project
that this could be discussed?
2a. Regarding your comment on hard coding the name, I am referring to
the device by its name, which has been initialised in
hw/virtio/vhost-user-fs.c. I downloaded the source code of qemu with
virtiofs support which I obtained in reference to the Kata container
project and analysed it.  I see the following  code snippet in
hw/virtio/vhost-user-fs
  if (fs->conf.cache_size) {
        /* Anonymous, private memory is not counted as overcommit */
        cache_ptr = mmap(NULL, fs->conf.cache_size, DAX_WINDOW_PROT,
                         MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
        if (cache_ptr == MAP_FAILED) {
            error_setg(errp, "Unable to mmap blank cache");
            return;
        }

        memory_region_init_ram_ptr(&fs->cache, OBJECT(vdev),
                                   "virtio-fs-cache",
                                   fs->conf.cache_size, cache_ptr);
    }

In the above code snippet, the memory region is initialised with
device name  "virtio-fs-cache",which I am referring to in my source
code.

2b. Regarding, needing a way for the cache to declare it wants to be
omitted, I am not sure thats what is needed. Currently virtio-fs with
DAX is currently not compatible with vfio. I want to overcome this
problem by making vfio not using the cache.
What I want is cache to be used for purposes other than the VFIO
device. For example, in my deployment scenario, I want DAX cache to be
not used by SR-IOV VF (which is a VFIO device) but used by all other
system.

3. Moved to  vfio_listener_skip_section () and patch resubmitted.

Best
Dev

Re: [PATCH] make vfio and DAX cache work together

[Alex Williamson]

On Mon, 26 Apr 2021 21:50:38 +0100
Dev Audsin <dev.devaqemu@gmail.com> wrote:

> Hi Alex and David
> 
> @Alex:
> 
> Justification on why this region cannot be a DMA target for the device,
> 
> virtio-fs with DAX is currently not compatible with NIC Pass through.
> When a SR-IOV VF attaches to a qemu process, vfio will try to pin the
> entire DAX Window but it is empty when the guest boots and will fail.
> A method to make VFIO and DAX to work together is to make vfio skip
> DAX cache.
> 
> Currently DAX cache need to be set to 0, for the SR-IOV VF to be
> attached to Kata containers. Enabling both SR-IOV VF and DAX work
> together will potentially improve performance for workloads which are
> I/O and network intensive.

Sorry, there's no actual justification described here.  You're enabling
a VM with both features, virtio-fs DAX and VFIO, but there's no
evidence that they "work together" or that your use case is simply
avoiding a scenario where the device might attempt to DMA into the area
with this designation.  With this change, if the device were to attempt
to DMA into this region, it would be blocked by the IOMMU, which might
result in a data loss within the VM.  Justification of this change
needs to prove that this region can never be a DMA target for the
device, not simply that both features can be enabled and we hope that
they don't interact.  Thanks,

Alex

Re: [PATCH] make vfio and DAX cache work together

[Dev Audsin]

Hi Alex

Based on your comments and thinking a bit, wonder if it makes sense to
allow DMA map for the DAX cache but make unexpected mappings to be not
fatal. Please let me know your thoughts.

Dev

On Mon, Apr 26, 2021 at 10:22 PM Alex Williamson
<alex.williamson@redhat.com> wrote:
>
> On Mon, 26 Apr 2021 21:50:38 +0100
> Dev Audsin <dev.devaqemu@gmail.com> wrote:
>
> > Hi Alex and David
> >
> > @Alex:
> >
> > Justification on why this region cannot be a DMA target for the device,
> >
> > virtio-fs with DAX is currently not compatible with NIC Pass through.
> > When a SR-IOV VF attaches to a qemu process, vfio will try to pin the
> > entire DAX Window but it is empty when the guest boots and will fail.
> > A method to make VFIO and DAX to work together is to make vfio skip
> > DAX cache.
> >
> > Currently DAX cache need to be set to 0, for the SR-IOV VF to be
> > attached to Kata containers. Enabling both SR-IOV VF and DAX work
> > together will potentially improve performance for workloads which are
> > I/O and network intensive.
>
> Sorry, there's no actual justification described here.  You're enabling
> a VM with both features, virtio-fs DAX and VFIO, but there's no
> evidence that they "work together" or that your use case is simply
> avoiding a scenario where the device might attempt to DMA into the area
> with this designation.  With this change, if the device were to attempt
> to DMA into this region, it would be blocked by the IOMMU, which might
> result in a data loss within the VM.  Justification of this change
> needs to prove that this region can never be a DMA target for the
> device, not simply that both features can be enabled and we hope that
> they don't interact.  Thanks,
>
> Alex
>

Re: [PATCH] make vfio and DAX cache work together

[Alex Williamson]

On Tue, 27 Apr 2021 17:29:37 +0100
Dev Audsin <dev.devaqemu@gmail.com> wrote:

> Hi Alex
> 
> Based on your comments and thinking a bit, wonder if it makes sense to
> allow DMA map for the DAX cache but make unexpected mappings to be not
> fatal. Please let me know your thoughts.

I think you're still working on the assumption that simply making the
VM boot is an improvement, it's not.  If there's a risk that a possible
DMA target for the device cannot be mapped, it's better that the VM
fail to boot than to expose that risk.  Performance cannot compromise
correctness.

We do allow DMA mappings to other device memory regions to fail
non-fatally with the logic that peer-to-peer DMA is often not trusted
to work by drivers and therefore support would be probed before
assuming that it works.  I don't think that same logic applies here.

Is there something about the definition of this particular region that
precludes it from being a DMA target for an assigned devices?

Otherwise if it's initially unpopulated, maybe something like the
RamDiscardManager could be used to insert DMA mappings as the region
becomes populated.

Simply disabling mapping to boot with both features together, without
analyzing how that missing mapping affects their interaction is not
acceptable.  Thanks,

Alex

> On Mon, Apr 26, 2021 at 10:22 PM Alex Williamson
> <alex.williamson@redhat.com> wrote:
> >
> > On Mon, 26 Apr 2021 21:50:38 +0100
> > Dev Audsin <dev.devaqemu@gmail.com> wrote:
> >  
> > > Hi Alex and David
> > >
> > > @Alex:
> > >
> > > Justification on why this region cannot be a DMA target for the device,
> > >
> > > virtio-fs with DAX is currently not compatible with NIC Pass through.
> > > When a SR-IOV VF attaches to a qemu process, vfio will try to pin the
> > > entire DAX Window but it is empty when the guest boots and will fail.
> > > A method to make VFIO and DAX to work together is to make vfio skip
> > > DAX cache.
> > >
> > > Currently DAX cache need to be set to 0, for the SR-IOV VF to be
> > > attached to Kata containers. Enabling both SR-IOV VF and DAX work
> > > together will potentially improve performance for workloads which are
> > > I/O and network intensive.  
> >
> > Sorry, there's no actual justification described here.  You're enabling
> > a VM with both features, virtio-fs DAX and VFIO, but there's no
> > evidence that they "work together" or that your use case is simply
> > avoiding a scenario where the device might attempt to DMA into the area
> > with this designation.  With this change, if the device were to attempt
> > to DMA into this region, it would be blocked by the IOMMU, which might
> > result in a data loss within the VM.  Justification of this change
> > needs to prove that this region can never be a DMA target for the
> > device, not simply that both features can be enabled and we hope that
> > they don't interact.  Thanks,
> >
> > Alex
> >  
> 

Re: [PATCH] make vfio and DAX cache work together

[Dr. David Alan Gilbert]

* Alex Williamson (alex.williamson@redhat.com) wrote:
> On Tue, 27 Apr 2021 17:29:37 +0100
> Dev Audsin <dev.devaqemu@gmail.com> wrote:
> 
> > Hi Alex
> > 
> > Based on your comments and thinking a bit, wonder if it makes sense to
> > allow DMA map for the DAX cache but make unexpected mappings to be not
> > fatal. Please let me know your thoughts.
> 
> I think you're still working on the assumption that simply making the
> VM boot is an improvement, it's not.  If there's a risk that a possible
> DMA target for the device cannot be mapped, it's better that the VM
> fail to boot than to expose that risk.  Performance cannot compromise
> correctness.
> 
> We do allow DMA mappings to other device memory regions to fail
> non-fatally with the logic that peer-to-peer DMA is often not trusted
> to work by drivers and therefore support would be probed before
> assuming that it works.  I don't think that same logic applies here.
> 
> Is there something about the definition of this particular region that
> precludes it from being a DMA target for an assigned devices?

It's never really the ram that's used.
This area is really a chunk of VMA that's mmap'd over by (chunks of)
normal files in the underlying exported filesystem.  The actual RAM
block itself is just a placeholder for the VMA, and is normally mapped
PROT_NONE until an actual file is mapped on top of it.
That cache bar is a mapping containing multiple separate file chunk
mappings.

So I guess the problems for VFIO are:
  a) At the start it's unmapped, unaccessible, unallocated ram.
  b) Later it's arbitrary chunks of ondisk files.

[on a bad day, and it's bad even without vfio, someone truncates the
file mapping]

Dave

> Otherwise if it's initially unpopulated, maybe something like the
> RamDiscardManager could be used to insert DMA mappings as the region
> becomes populated.
> 
> Simply disabling mapping to boot with both features together, without
> analyzing how that missing mapping affects their interaction is not
> acceptable.  Thanks,
> 
> Alex
> 
> > On Mon, Apr 26, 2021 at 10:22 PM Alex Williamson
> > <alex.williamson@redhat.com> wrote:
> > >
> > > On Mon, 26 Apr 2021 21:50:38 +0100
> > > Dev Audsin <dev.devaqemu@gmail.com> wrote:
> > >  
> > > > Hi Alex and David
> > > >
> > > > @Alex:
> > > >
> > > > Justification on why this region cannot be a DMA target for the device,
> > > >
> > > > virtio-fs with DAX is currently not compatible with NIC Pass through.
> > > > When a SR-IOV VF attaches to a qemu process, vfio will try to pin the
> > > > entire DAX Window but it is empty when the guest boots and will fail.
> > > > A method to make VFIO and DAX to work together is to make vfio skip
> > > > DAX cache.
> > > >
> > > > Currently DAX cache need to be set to 0, for the SR-IOV VF to be
> > > > attached to Kata containers. Enabling both SR-IOV VF and DAX work
> > > > together will potentially improve performance for workloads which are
> > > > I/O and network intensive.  
> > >
> > > Sorry, there's no actual justification described here.  You're enabling
> > > a VM with both features, virtio-fs DAX and VFIO, but there's no
> > > evidence that they "work together" or that your use case is simply
> > > avoiding a scenario where the device might attempt to DMA into the area
> > > with this designation.  With this change, if the device were to attempt
> > > to DMA into this region, it would be blocked by the IOMMU, which might
> > > result in a data loss within the VM.  Justification of this change
> > > needs to prove that this region can never be a DMA target for the
> > > device, not simply that both features can be enabled and we hope that
> > > they don't interact.  Thanks,
> > >
> > > Alex
> > >  
> > 
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH] make vfio and DAX cache work together

[Dev Audsin]

Thanks Dave for your explanation.
Any suggestions on how to make VFIO not attempt to map into the
unaccessible and unallocated RAM.

Best
Dev

On Tue, Apr 27, 2021 at 8:00 PM Dr. David Alan Gilbert
<dgilbert@redhat.com> wrote:
>
> * Alex Williamson (alex.williamson@redhat.com) wrote:
> > On Tue, 27 Apr 2021 17:29:37 +0100
> > Dev Audsin <dev.devaqemu@gmail.com> wrote:
> >
> > > Hi Alex
> > >
> > > Based on your comments and thinking a bit, wonder if it makes sense to
> > > allow DMA map for the DAX cache but make unexpected mappings to be not
> > > fatal. Please let me know your thoughts.
> >
> > I think you're still working on the assumption that simply making the
> > VM boot is an improvement, it's not.  If there's a risk that a possible
> > DMA target for the device cannot be mapped, it's better that the VM
> > fail to boot than to expose that risk.  Performance cannot compromise
> > correctness.
> >
> > We do allow DMA mappings to other device memory regions to fail
> > non-fatally with the logic that peer-to-peer DMA is often not trusted
> > to work by drivers and therefore support would be probed before
> > assuming that it works.  I don't think that same logic applies here.
> >
> > Is there something about the definition of this particular region that
> > precludes it from being a DMA target for an assigned devices?
>
> It's never really the ram that's used.
> This area is really a chunk of VMA that's mmap'd over by (chunks of)
> normal files in the underlying exported filesystem.  The actual RAM
> block itself is just a placeholder for the VMA, and is normally mapped
> PROT_NONE until an actual file is mapped on top of it.
> That cache bar is a mapping containing multiple separate file chunk
> mappings.
>
> So I guess the problems for VFIO are:
>   a) At the start it's unmapped, unaccessible, unallocated ram.
>   b) Later it's arbitrary chunks of ondisk files.
>
> [on a bad day, and it's bad even without vfio, someone truncates the
> file mapping]
>
> Dave
>
> > Otherwise if it's initially unpopulated, maybe something like the
> > RamDiscardManager could be used to insert DMA mappings as the region
> > becomes populated.
> >
> > Simply disabling mapping to boot with both features together, without
> > analyzing how that missing mapping affects their interaction is not
> > acceptable.  Thanks,
> >
> > Alex
> >
> > > On Mon, Apr 26, 2021 at 10:22 PM Alex Williamson
> > > <alex.williamson@redhat.com> wrote:
> > > >
> > > > On Mon, 26 Apr 2021 21:50:38 +0100
> > > > Dev Audsin <dev.devaqemu@gmail.com> wrote:
> > > >
> > > > > Hi Alex and David
> > > > >
> > > > > @Alex:
> > > > >
> > > > > Justification on why this region cannot be a DMA target for the device,
> > > > >
> > > > > virtio-fs with DAX is currently not compatible with NIC Pass through.
> > > > > When a SR-IOV VF attaches to a qemu process, vfio will try to pin the
> > > > > entire DAX Window but it is empty when the guest boots and will fail.
> > > > > A method to make VFIO and DAX to work together is to make vfio skip
> > > > > DAX cache.
> > > > >
> > > > > Currently DAX cache need to be set to 0, for the SR-IOV VF to be
> > > > > attached to Kata containers. Enabling both SR-IOV VF and DAX work
> > > > > together will potentially improve performance for workloads which are
> > > > > I/O and network intensive.
> > > >
> > > > Sorry, there's no actual justification described here.  You're enabling
> > > > a VM with both features, virtio-fs DAX and VFIO, but there's no
> > > > evidence that they "work together" or that your use case is simply
> > > > avoiding a scenario where the device might attempt to DMA into the area
> > > > with this designation.  With this change, if the device were to attempt
> > > > to DMA into this region, it would be blocked by the IOMMU, which might
> > > > result in a data loss within the VM.  Justification of this change
> > > > needs to prove that this region can never be a DMA target for the
> > > > device, not simply that both features can be enabled and we hope that
> > > > they don't interact.  Thanks,
> > > >
> > > > Alex
> > > >
> > >
> >
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>

Re: [PATCH] make vfio and DAX cache work together

[Dr. David Alan Gilbert]

* Dev Audsin (dev.devaqemu@gmail.com) wrote:
> Thanks Dave for your explanation.
> Any suggestions on how to make VFIO not attempt to map into the
> unaccessible and unallocated RAM.

I'm not sure;:

static bool vfio_listener_skipped_section(MemoryRegionSection *section)
{
    return (!memory_region_is_ram(section->mr) &&
            !memory_region_is_iommu(section->mr)) ||
           section->offset_within_address_space & (1ULL << 63);
}

I'm declaring that region with memory_region_init_ram_ptr;  should I be?
it's not quite like RAM.
But then I *do* want a kvm slot for it, and I do want it to be accessed
by mapping rather htan calling IO functions; that makes me think mr->ram
has to be true.
But then do we need to add another flag to memory-regions; if we do,
what is it;
   a) We don't want an 'is_virtio_fs' - it needs to be more generic
   b) 'no_vfio' also feels wrong

Is perhaps 'not_lockable' the right thing to call it?

Dave


> Best
> Dev
> 
> On Tue, Apr 27, 2021 at 8:00 PM Dr. David Alan Gilbert
> <dgilbert@redhat.com> wrote:
> >
> > * Alex Williamson (alex.williamson@redhat.com) wrote:
> > > On Tue, 27 Apr 2021 17:29:37 +0100
> > > Dev Audsin <dev.devaqemu@gmail.com> wrote:
> > >
> > > > Hi Alex
> > > >
> > > > Based on your comments and thinking a bit, wonder if it makes sense to
> > > > allow DMA map for the DAX cache but make unexpected mappings to be not
> > > > fatal. Please let me know your thoughts.
> > >
> > > I think you're still working on the assumption that simply making the
> > > VM boot is an improvement, it's not.  If there's a risk that a possible
> > > DMA target for the device cannot be mapped, it's better that the VM
> > > fail to boot than to expose that risk.  Performance cannot compromise
> > > correctness.
> > >
> > > We do allow DMA mappings to other device memory regions to fail
> > > non-fatally with the logic that peer-to-peer DMA is often not trusted
> > > to work by drivers and therefore support would be probed before
> > > assuming that it works.  I don't think that same logic applies here.
> > >
> > > Is there something about the definition of this particular region that
> > > precludes it from being a DMA target for an assigned devices?
> >
> > It's never really the ram that's used.
> > This area is really a chunk of VMA that's mmap'd over by (chunks of)
> > normal files in the underlying exported filesystem.  The actual RAM
> > block itself is just a placeholder for the VMA, and is normally mapped
> > PROT_NONE until an actual file is mapped on top of it.
> > That cache bar is a mapping containing multiple separate file chunk
> > mappings.
> >
> > So I guess the problems for VFIO are:
> >   a) At the start it's unmapped, unaccessible, unallocated ram.
> >   b) Later it's arbitrary chunks of ondisk files.
> >
> > [on a bad day, and it's bad even without vfio, someone truncates the
> > file mapping]
> >
> > Dave
> >
> > > Otherwise if it's initially unpopulated, maybe something like the
> > > RamDiscardManager could be used to insert DMA mappings as the region
> > > becomes populated.
> > >
> > > Simply disabling mapping to boot with both features together, without
> > > analyzing how that missing mapping affects their interaction is not
> > > acceptable.  Thanks,
> > >
> > > Alex
> > >
> > > > On Mon, Apr 26, 2021 at 10:22 PM Alex Williamson
> > > > <alex.williamson@redhat.com> wrote:
> > > > >
> > > > > On Mon, 26 Apr 2021 21:50:38 +0100
> > > > > Dev Audsin <dev.devaqemu@gmail.com> wrote:
> > > > >
> > > > > > Hi Alex and David
> > > > > >
> > > > > > @Alex:
> > > > > >
> > > > > > Justification on why this region cannot be a DMA target for the device,
> > > > > >
> > > > > > virtio-fs with DAX is currently not compatible with NIC Pass through.
> > > > > > When a SR-IOV VF attaches to a qemu process, vfio will try to pin the
> > > > > > entire DAX Window but it is empty when the guest boots and will fail.
> > > > > > A method to make VFIO and DAX to work together is to make vfio skip
> > > > > > DAX cache.
> > > > > >
> > > > > > Currently DAX cache need to be set to 0, for the SR-IOV VF to be
> > > > > > attached to Kata containers. Enabling both SR-IOV VF and DAX work
> > > > > > together will potentially improve performance for workloads which are
> > > > > > I/O and network intensive.
> > > > >
> > > > > Sorry, there's no actual justification described here.  You're enabling
> > > > > a VM with both features, virtio-fs DAX and VFIO, but there's no
> > > > > evidence that they "work together" or that your use case is simply
> > > > > avoiding a scenario where the device might attempt to DMA into the area
> > > > > with this designation.  With this change, if the device were to attempt
> > > > > to DMA into this region, it would be blocked by the IOMMU, which might
> > > > > result in a data loss within the VM.  Justification of this change
> > > > > needs to prove that this region can never be a DMA target for the
> > > > > device, not simply that both features can be enabled and we hope that
> > > > > they don't interact.  Thanks,
> > > > >
> > > > > Alex
> > > > >
> > > >
> > >
> > --
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> >
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH] make vfio and DAX cache work together

[Alex Williamson]

On Wed, 28 Apr 2021 20:17:23 +0100
"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:

> * Dev Audsin (dev.devaqemu@gmail.com) wrote:
> > Thanks Dave for your explanation.
> > Any suggestions on how to make VFIO not attempt to map into the
> > unaccessible and unallocated RAM.  
> 
> I'm not sure;:
> 
> static bool vfio_listener_skipped_section(MemoryRegionSection *section)
> {
>     return (!memory_region_is_ram(section->mr) &&
>             !memory_region_is_iommu(section->mr)) ||
>            section->offset_within_address_space & (1ULL << 63);
> }
> 
> I'm declaring that region with memory_region_init_ram_ptr;  should I be?
> it's not quite like RAM.
> But then I *do* want a kvm slot for it, and I do want it to be accessed
> by mapping rather htan calling IO functions; that makes me think mr->ram
> has to be true.
> But then do we need to add another flag to memory-regions; if we do,
> what is it;
>    a) We don't want an 'is_virtio_fs' - it needs to be more generic
>    b) 'no_vfio' also feels wrong
> 
> Is perhaps 'not_lockable' the right thing to call it?

This reasoning just seems to lead back to "it doesn't work, therefore
don't do it" rather than identifying the property of the region that
makes it safe not to map it for device DMA (assuming that's actually
the case).  It's clearly "RAM" as far as QEMU is concerned given how
it's created, but does it actually appear in the VM as generic physical
RAM that the guest OS could program to the device as a DMA target?  If
not, what property makes that so, create a flag for that.  Thanks,

Alex

Re: [PATCH] make vfio and DAX cache work together

[Dr. David Alan Gilbert]

* Alex Williamson (alex.williamson@redhat.com) wrote:
> On Wed, 28 Apr 2021 20:17:23 +0100
> "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> 
> > * Dev Audsin (dev.devaqemu@gmail.com) wrote:
> > > Thanks Dave for your explanation.
> > > Any suggestions on how to make VFIO not attempt to map into the
> > > unaccessible and unallocated RAM.  
> > 
> > I'm not sure;:
> > 
> > static bool vfio_listener_skipped_section(MemoryRegionSection *section)
> > {
> >     return (!memory_region_is_ram(section->mr) &&
> >             !memory_region_is_iommu(section->mr)) ||
> >            section->offset_within_address_space & (1ULL << 63);
> > }
> > 
> > I'm declaring that region with memory_region_init_ram_ptr;  should I be?
> > it's not quite like RAM.
> > But then I *do* want a kvm slot for it, and I do want it to be accessed
> > by mapping rather htan calling IO functions; that makes me think mr->ram
> > has to be true.
> > But then do we need to add another flag to memory-regions; if we do,
> > what is it;
> >    a) We don't want an 'is_virtio_fs' - it needs to be more generic
> >    b) 'no_vfio' also feels wrong
> > 
> > Is perhaps 'not_lockable' the right thing to call it?
> 
> This reasoning just seems to lead back to "it doesn't work, therefore
> don't do it" rather than identifying the property of the region that
> makes it safe not to map it for device DMA (assuming that's actually
> the case). 

Yes, I'm struggling to get to what that generic form of that property
is, possibly because I've not got an example of another case to compare
it with.

> It's clearly "RAM" as far as QEMU is concerned given how
> it's created, but does it actually appear in the VM as generic physical
> RAM that the guest OS could program to the device as a DMA target?  If
> not, what property makes that so, create a flag for that.  Thanks,

The guest sees it as a PCI-bar; so it knows it's not 'generic physical
RAM' - but can a guest set other BARs (like frame buffers or pmem) as
DMA targets?  If so, how do I distinguish our bar?

Dave

> Alex
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH] make vfio and DAX cache work together

[Alex Williamson]

On Thu, 29 Apr 2021 09:44:51 +0100
"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:

> * Alex Williamson (alex.williamson@redhat.com) wrote:
> > On Wed, 28 Apr 2021 20:17:23 +0100
> > "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> >   
> > > * Dev Audsin (dev.devaqemu@gmail.com) wrote:  
> > > > Thanks Dave for your explanation.
> > > > Any suggestions on how to make VFIO not attempt to map into the
> > > > unaccessible and unallocated RAM.    
> > > 
> > > I'm not sure;:
> > > 
> > > static bool vfio_listener_skipped_section(MemoryRegionSection *section)
> > > {
> > >     return (!memory_region_is_ram(section->mr) &&
> > >             !memory_region_is_iommu(section->mr)) ||
> > >            section->offset_within_address_space & (1ULL << 63);
> > > }
> > > 
> > > I'm declaring that region with memory_region_init_ram_ptr;  should I be?
> > > it's not quite like RAM.
> > > But then I *do* want a kvm slot for it, and I do want it to be accessed
> > > by mapping rather htan calling IO functions; that makes me think mr->ram
> > > has to be true.
> > > But then do we need to add another flag to memory-regions; if we do,
> > > what is it;
> > >    a) We don't want an 'is_virtio_fs' - it needs to be more generic
> > >    b) 'no_vfio' also feels wrong
> > > 
> > > Is perhaps 'not_lockable' the right thing to call it?  
> > 
> > This reasoning just seems to lead back to "it doesn't work, therefore
> > don't do it" rather than identifying the property of the region that
> > makes it safe not to map it for device DMA (assuming that's actually
> > the case).   
> 
> Yes, I'm struggling to get to what that generic form of that property
> is, possibly because I've not got an example of another case to compare
> it with.
> 
> > It's clearly "RAM" as far as QEMU is concerned given how
> > it's created, but does it actually appear in the VM as generic physical
> > RAM that the guest OS could program to the device as a DMA target?  If
> > not, what property makes that so, create a flag for that.  Thanks,  
> 
> The guest sees it as a PCI-bar; so it knows it's not 'generic physical
> RAM' - but can a guest set other BARs (like frame buffers or pmem) as
> DMA targets?  If so, how do I distinguish our bar?

They can, this is how peer-to-peer DMA between devices works.  However,
we can perhaps take advantage that drivers are generally a bit more
cautious in probing that this type of DMA works before relying on it,
and declare it with memory_region_init_ram_device_ptr() which vfio
would not consider fatal if it fails to map it.  The other semantic
difference is that ram_device_mem_ops are used for read/write access to
avoid some of the opcodes that are not meant to be used for physical
device memory with the default memcpy ops.  If you expect this region
to be mapped as a kvm memory slot, presumably these would never get
used anyway.  Thanks,

Alex

Re: [PATCH] make vfio and DAX cache work together

[Dr. David Alan Gilbert]

* Alex Williamson (alex.williamson@redhat.com) wrote:
> On Thu, 29 Apr 2021 09:44:51 +0100
> "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> 
> > * Alex Williamson (alex.williamson@redhat.com) wrote:
> > > On Wed, 28 Apr 2021 20:17:23 +0100
> > > "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> > >   
> > > > * Dev Audsin (dev.devaqemu@gmail.com) wrote:  
> > > > > Thanks Dave for your explanation.
> > > > > Any suggestions on how to make VFIO not attempt to map into the
> > > > > unaccessible and unallocated RAM.    
> > > > 
> > > > I'm not sure;:
> > > > 
> > > > static bool vfio_listener_skipped_section(MemoryRegionSection *section)
> > > > {
> > > >     return (!memory_region_is_ram(section->mr) &&
> > > >             !memory_region_is_iommu(section->mr)) ||
> > > >            section->offset_within_address_space & (1ULL << 63);
> > > > }
> > > > 
> > > > I'm declaring that region with memory_region_init_ram_ptr;  should I be?
> > > > it's not quite like RAM.
> > > > But then I *do* want a kvm slot for it, and I do want it to be accessed
> > > > by mapping rather htan calling IO functions; that makes me think mr->ram
> > > > has to be true.
> > > > But then do we need to add another flag to memory-regions; if we do,
> > > > what is it;
> > > >    a) We don't want an 'is_virtio_fs' - it needs to be more generic
> > > >    b) 'no_vfio' also feels wrong
> > > > 
> > > > Is perhaps 'not_lockable' the right thing to call it?  
> > > 
> > > This reasoning just seems to lead back to "it doesn't work, therefore
> > > don't do it" rather than identifying the property of the region that
> > > makes it safe not to map it for device DMA (assuming that's actually
> > > the case).   
> > 
> > Yes, I'm struggling to get to what that generic form of that property
> > is, possibly because I've not got an example of another case to compare
> > it with.
> > 
> > > It's clearly "RAM" as far as QEMU is concerned given how
> > > it's created, but does it actually appear in the VM as generic physical
> > > RAM that the guest OS could program to the device as a DMA target?  If
> > > not, what property makes that so, create a flag for that.  Thanks,  
> > 
> > The guest sees it as a PCI-bar; so it knows it's not 'generic physical
> > RAM' - but can a guest set other BARs (like frame buffers or pmem) as
> > DMA targets?  If so, how do I distinguish our bar?
> 
> They can, this is how peer-to-peer DMA between devices works.  However,
> we can perhaps take advantage that drivers are generally a bit more
> cautious in probing that this type of DMA works before relying on it,
> and declare it with memory_region_init_ram_device_ptr() which vfio
> would not consider fatal if it fails to map it.  The other semantic
> difference is that ram_device_mem_ops are used for read/write access to
> avoid some of the opcodes that are not meant to be used for physical
> device memory with the default memcpy ops.  If you expect this region
> to be mapped as a kvm memory slot, presumably these would never get
> used anyway.  Thanks,

Oh, nice, I hadn't spotted memory_region_init_ram_device_ptr();

diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c
index 7afd9495c9..11fb9b5979 100644
--- a/hw/virtio/vhost-user-fs.c
+++ b/hw/virtio/vhost-user-fs.c
@@ -604,7 +604,7 @@ static void vuf_device_realize(DeviceState *dev, Error **errp)
             return;
         }
 
-        memory_region_init_ram_ptr(&fs->cache, OBJECT(vdev),
+        memory_region_init_ram_device_ptr(&fs->cache, OBJECT(vdev),
                                    "virtio-fs-cache",
                                    fs->conf.cache_size, cache_ptr);
     }

apparently still works for us; Dev does that fix it for you?

Dave

> Alex
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH] make vfio and DAX cache work together

[Dev Audsin]

Thanks David. I did a quick test with the above patch and it seems to
work for me. With this patch, apparently I can  create a VM with
SR-IOV VF and DAX cache ( virtio_fs_cache_size = 1024).

Thanks
Dev

On Thu, Apr 29, 2021 at 6:55 PM Dr. David Alan Gilbert
<dgilbert@redhat.com> wrote:
>
> * Alex Williamson (alex.williamson@redhat.com) wrote:
> > On Thu, 29 Apr 2021 09:44:51 +0100
> > "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> >
> > > * Alex Williamson (alex.williamson@redhat.com) wrote:
> > > > On Wed, 28 Apr 2021 20:17:23 +0100
> > > > "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> > > >
> > > > > * Dev Audsin (dev.devaqemu@gmail.com) wrote:
> > > > > > Thanks Dave for your explanation.
> > > > > > Any suggestions on how to make VFIO not attempt to map into the
> > > > > > unaccessible and unallocated RAM.
> > > > >
> > > > > I'm not sure;:
> > > > >
> > > > > static bool vfio_listener_skipped_section(MemoryRegionSection *section)
> > > > > {
> > > > >     return (!memory_region_is_ram(section->mr) &&
> > > > >             !memory_region_is_iommu(section->mr)) ||
> > > > >            section->offset_within_address_space & (1ULL << 63);
> > > > > }
> > > > >
> > > > > I'm declaring that region with memory_region_init_ram_ptr;  should I be?
> > > > > it's not quite like RAM.
> > > > > But then I *do* want a kvm slot for it, and I do want it to be accessed
> > > > > by mapping rather htan calling IO functions; that makes me think mr->ram
> > > > > has to be true.
> > > > > But then do we need to add another flag to memory-regions; if we do,
> > > > > what is it;
> > > > >    a) We don't want an 'is_virtio_fs' - it needs to be more generic
> > > > >    b) 'no_vfio' also feels wrong
> > > > >
> > > > > Is perhaps 'not_lockable' the right thing to call it?
> > > >
> > > > This reasoning just seems to lead back to "it doesn't work, therefore
> > > > don't do it" rather than identifying the property of the region that
> > > > makes it safe not to map it for device DMA (assuming that's actually
> > > > the case).
> > >
> > > Yes, I'm struggling to get to what that generic form of that property
> > > is, possibly because I've not got an example of another case to compare
> > > it with.
> > >
> > > > It's clearly "RAM" as far as QEMU is concerned given how
> > > > it's created, but does it actually appear in the VM as generic physical
> > > > RAM that the guest OS could program to the device as a DMA target?  If
> > > > not, what property makes that so, create a flag for that.  Thanks,
> > >
> > > The guest sees it as a PCI-bar; so it knows it's not 'generic physical
> > > RAM' - but can a guest set other BARs (like frame buffers or pmem) as
> > > DMA targets?  If so, how do I distinguish our bar?
> >
> > They can, this is how peer-to-peer DMA between devices works.  However,
> > we can perhaps take advantage that drivers are generally a bit more
> > cautious in probing that this type of DMA works before relying on it,
> > and declare it with memory_region_init_ram_device_ptr() which vfio
> > would not consider fatal if it fails to map it.  The other semantic
> > difference is that ram_device_mem_ops are used for read/write access to
> > avoid some of the opcodes that are not meant to be used for physical
> > device memory with the default memcpy ops.  If you expect this region
> > to be mapped as a kvm memory slot, presumably these would never get
> > used anyway.  Thanks,
>
> Oh, nice, I hadn't spotted memory_region_init_ram_device_ptr();
>
> diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c
> index 7afd9495c9..11fb9b5979 100644
> --- a/hw/virtio/vhost-user-fs.c
> +++ b/hw/virtio/vhost-user-fs.c
> @@ -604,7 +604,7 @@ static void vuf_device_realize(DeviceState *dev, Error **errp)
>              return;
>          }
>
> -        memory_region_init_ram_ptr(&fs->cache, OBJECT(vdev),
> +        memory_region_init_ram_device_ptr(&fs->cache, OBJECT(vdev),
>                                     "virtio-fs-cache",
>                                     fs->conf.cache_size, cache_ptr);
>      }
>
> apparently still works for us; Dev does that fix it for you?
>
> Dave
>
> > Alex
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>

Re: [PATCH] make vfio and DAX cache work together

[Dr. David Alan Gilbert]

* Dev Audsin (dev.devaqemu@gmail.com) wrote:
> Thanks David. I did a quick test with the above patch and it seems to
> work for me. With this patch, apparently I can  create a VM with
> SR-IOV VF and DAX cache ( virtio_fs_cache_size = 1024).

Great! I'll put it in the next set of DAX patches I send.

Dave

> Thanks
> Dev
> 
> On Thu, Apr 29, 2021 at 6:55 PM Dr. David Alan Gilbert
> <dgilbert@redhat.com> wrote:
> >
> > * Alex Williamson (alex.williamson@redhat.com) wrote:
> > > On Thu, 29 Apr 2021 09:44:51 +0100
> > > "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> > >
> > > > * Alex Williamson (alex.williamson@redhat.com) wrote:
> > > > > On Wed, 28 Apr 2021 20:17:23 +0100
> > > > > "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> > > > >
> > > > > > * Dev Audsin (dev.devaqemu@gmail.com) wrote:
> > > > > > > Thanks Dave for your explanation.
> > > > > > > Any suggestions on how to make VFIO not attempt to map into the
> > > > > > > unaccessible and unallocated RAM.
> > > > > >
> > > > > > I'm not sure;:
> > > > > >
> > > > > > static bool vfio_listener_skipped_section(MemoryRegionSection *section)
> > > > > > {
> > > > > >     return (!memory_region_is_ram(section->mr) &&
> > > > > >             !memory_region_is_iommu(section->mr)) ||
> > > > > >            section->offset_within_address_space & (1ULL << 63);
> > > > > > }
> > > > > >
> > > > > > I'm declaring that region with memory_region_init_ram_ptr;  should I be?
> > > > > > it's not quite like RAM.
> > > > > > But then I *do* want a kvm slot for it, and I do want it to be accessed
> > > > > > by mapping rather htan calling IO functions; that makes me think mr->ram
> > > > > > has to be true.
> > > > > > But then do we need to add another flag to memory-regions; if we do,
> > > > > > what is it;
> > > > > >    a) We don't want an 'is_virtio_fs' - it needs to be more generic
> > > > > >    b) 'no_vfio' also feels wrong
> > > > > >
> > > > > > Is perhaps 'not_lockable' the right thing to call it?
> > > > >
> > > > > This reasoning just seems to lead back to "it doesn't work, therefore
> > > > > don't do it" rather than identifying the property of the region that
> > > > > makes it safe not to map it for device DMA (assuming that's actually
> > > > > the case).
> > > >
> > > > Yes, I'm struggling to get to what that generic form of that property
> > > > is, possibly because I've not got an example of another case to compare
> > > > it with.
> > > >
> > > > > It's clearly "RAM" as far as QEMU is concerned given how
> > > > > it's created, but does it actually appear in the VM as generic physical
> > > > > RAM that the guest OS could program to the device as a DMA target?  If
> > > > > not, what property makes that so, create a flag for that.  Thanks,
> > > >
> > > > The guest sees it as a PCI-bar; so it knows it's not 'generic physical
> > > > RAM' - but can a guest set other BARs (like frame buffers or pmem) as
> > > > DMA targets?  If so, how do I distinguish our bar?
> > >
> > > They can, this is how peer-to-peer DMA between devices works.  However,
> > > we can perhaps take advantage that drivers are generally a bit more
> > > cautious in probing that this type of DMA works before relying on it,
> > > and declare it with memory_region_init_ram_device_ptr() which vfio
> > > would not consider fatal if it fails to map it.  The other semantic
> > > difference is that ram_device_mem_ops are used for read/write access to
> > > avoid some of the opcodes that are not meant to be used for physical
> > > device memory with the default memcpy ops.  If you expect this region
> > > to be mapped as a kvm memory slot, presumably these would never get
> > > used anyway.  Thanks,
> >
> > Oh, nice, I hadn't spotted memory_region_init_ram_device_ptr();
> >
> > diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c
> > index 7afd9495c9..11fb9b5979 100644
> > --- a/hw/virtio/vhost-user-fs.c
> > +++ b/hw/virtio/vhost-user-fs.c
> > @@ -604,7 +604,7 @@ static void vuf_device_realize(DeviceState *dev, Error **errp)
> >              return;
> >          }
> >
> > -        memory_region_init_ram_ptr(&fs->cache, OBJECT(vdev),
> > +        memory_region_init_ram_device_ptr(&fs->cache, OBJECT(vdev),
> >                                     "virtio-fs-cache",
> >                                     fs->conf.cache_size, cache_ptr);
> >      }
> >
> > apparently still works for us; Dev does that fix it for you?
> >
> > Dave
> >
> > > Alex
> > --
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> >
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH] make vfio and DAX cache work together

[Alex Williamson]

On Mon, 26 Apr 2021 13:19:05 +0100
"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:

> * Edge NFV (edgenfv@gmail.com) wrote:
> >  Signed-off-by: Edge NFV <edgenfv@gmail.com>  
> 
> Hi,
>   I take it that 'Edge NFV' isn't your real name; apologies if it is.
> It's unusual not to use a real name; I would be interested to know
> why you feel uncomfortable not doing.

The documentation noted by Laurent even goes so far as to request a
real name for the Sign-off.  Intentionally masking your identity will
only serve to raise suspicion and increase the chances that the patch
is ignored.  I think perhaps aside from one or two legacy contributors,
all QEMU contributions are signed-off by real names these days.

I also require a commit log describing the change.

> > ---
> >  hw/vfio/common.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> > index ae5654fcdb..83e15bf7a3 100644
> > --- a/hw/vfio/common.c
> > +++ b/hw/vfio/common.c
> > @@ -668,6 +668,15 @@ static void vfio_listener_region_add(MemoryListener
> > *listener,
> >                  int128_get64(int128_sub(section->size, int128_one())));
> >          return;
> >      }
> > +
> > +    /* Do not add virtio fs cache section */
> > +    if (!strcmp(memory_region_name(section->mr), "virtio-fs-cache")) {  
> 
> So first, this is a patch that fixes something that isn't yet in qemu;
> the DAX mode of virtiofs.
> Secondly, hard coding the name like this is probably the wrong thing to
> do; we need a way for the cache to declare it wants to be omitted.
> Thirdly, shouldn't this actually be a change to
> vfio_listener_skip_section to add this test?

Agree on all points, there needs to be justification on why this region
cannot be a DMA target for the device, not simply wishing to skip it to
workaround a boot failure.  Thanks,

Alex

> > +        trace_vfio_listener_region_add_skip(
> > +                section->offset_within_address_space,
> > +                section->offset_within_address_space +
> > +                int128_get64(int128_sub(section->size, int128_one())));
> > +        return;
> > +    }
> > 
> >      if (unlikely((section->offset_within_address_space &
> >                    ~qemu_real_host_page_mask) !=
> > -- 
> > 2.25.1  

Re: [PATCH] make vfio and DAX cache work together

[Dr. David Alan Gilbert]

* Edge NFV (edgenfv@gmail.com) wrote:
>  Signed-off-by: Edge NFV <edgenfv@gmail.com>

Hi,
  I take it that 'Edge NFV' isn't your real name; apologies if it is.
It's unusual not to use a real name; I would be interested to know
why you feel uncomfortable not doing.

> ---
>  hw/vfio/common.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index ae5654fcdb..83e15bf7a3 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -668,6 +668,15 @@ static void vfio_listener_region_add(MemoryListener
> *listener,
>                  int128_get64(int128_sub(section->size, int128_one())));
>          return;
>      }
> +
> +    /* Do not add virtio fs cache section */
> +    if (!strcmp(memory_region_name(section->mr), "virtio-fs-cache")) {

So first, this is a patch that fixes something that isn't yet in qemu;
the DAX mode of virtiofs.
Secondly, hard coding the name like this is probably the wrong thing to
do; we need a way for the cache to declare it wants to be omitted.
Thirdly, shouldn't this actually be a change to
vfio_listener_skip_section to add this test?

Dave

> +        trace_vfio_listener_region_add_skip(
> +                section->offset_within_address_space,
> +                section->offset_within_address_space +
> +                int128_get64(int128_sub(section->size, int128_one())));
> +        return;
> +    }
> 
>      if (unlikely((section->offset_within_address_space &
>                    ~qemu_real_host_page_mask) !=
> -- 
> 2.25.1
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[PATCH] make vfio and DAX cache work together

[Edge NFV]

[-- Attachment #1: Type: text/plain, Size: 920 bytes --]

 Signed-off-by: Edge NFV <edgenfv@gmail.com>
---
 hw/vfio/common.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index ae5654fcdb..83e15bf7a3 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -668,6 +668,15 @@ static void vfio_listener_region_add(MemoryListener
*listener,
                 int128_get64(int128_sub(section->size, int128_one())));
         return;
     }
+
+    /* Do not add virtio fs cache section */
+    if (!strcmp(memory_region_name(section->mr), "virtio-fs-cache")) {
+        trace_vfio_listener_region_add_skip(
+                section->offset_within_address_space,
+                section->offset_within_address_space +
+                int128_get64(int128_sub(section->size, int128_one())));
+        return;
+    }

     if (unlikely((section->offset_within_address_space &
                   ~qemu_real_host_page_mask) !=
-- 
2.25.1

[-- Attachment #2: Type: text/html, Size: 1305 bytes --]

[PATCH] make vfio and DAX cache work together

[Edge NFV]

I am using Kata containers and create containers inside the virtual machine. The hypervisor used is QEMU.
My workload is I/O and network intensive. So, I prefer SR-IOV VF (which provides better network isolation) and DAX caching to work together.
However, I was unable to create a QEMU based virual machine when the above mentioned featires are enabled together.

Currently DAX cache need to be set to 0, to boot the VM successfully with the SR-IOV VF to be attached.
Enabling both SR-IOV VF and DAX work together will potentially improve performance for workloads which are I/O and network intensive.
This patch enables SR-IOV VF and DAX to work together.