Re: [PATCH V2 06/12] virtio_pci: harden MSI-X interrupts
On Sat, Oct 16, 2021 at 1:27 AM Michael S. Tsirkin wrote:
>
> On Fri, Oct 15, 2021 at 05:09:38AM -0700, Dongli Zhang wrote:
> > Hi Jason,
> >
> > On 10/11/21 11:52 PM, Jason Wang wrote:
> > > We used to synchronize pending MSI-X irq handlers via
> > > synchronize_irq(), this may not work for the untrusted device which
> > > may keep sending interrupts after reset which may lead unexpected
> > > results. Similarly, we should not enable MSI-X interrupt until the
> >
> > About "unexpected results", while you mentioned below in v1 ...
> >
> > "My understanding is that e.g in the case of SEV/TDX we don't trust the
> > hypervisor. So the hypervisor can keep sending interrupts even if the
> > device is reset. The guest can only trust its own software interrupt
> > management logic to avoid call virtio callback in this case."
> >
> > .. and you also mentioned to avoid the panic (due to untrusted device) in as
> > many scenarios as possible.
> >
> >
> > Here is my understanding.
> >
> > The reason we do not trust hypervisor is to protect (1) data/code privacy
> > for
> > most of times and sometimes (2) program execution integrity.
> >
> > The bad thing is: the hypervisor is able to panic/destroy the VM in the
> > worst case.
> >
> > It is reasonable to re-configure/recover if we assume there is BUG at
> > hypervisor/device side. That is, the hypervisor/device is buggy, but not
> > malicious.
> >
> > However, how about to just detect and report the hypervisor/device is
> > malicious
> > and shutdown/panic the VM immediately? If we have detected the malicious
> > hypervisor, we should avoid running VMs on the malicious hypervisor
> > further. At
> > least how about to print error message to reminder users that the
> > hypervisor is
> > malicious?
I understand your point, but several questions needs to be answered:
1) how can we easily differentiate "malicious" from "buggy"?
2) If we want to detect malicious hypervisor, virtio is not the only
place that we want to do this
3) Is there a way that "malicious" hypervisor can prevent guest from
shutting down itself?
> >
> >
> > About "unexpected results", it should not be hang/panic. I suggest ...
> >
It's just the phenomenon not the logic behind that. It could be e.g
OOB which may lead the unexpected kernel codes to be executed in
unexpected ways (e.g mark the page as shared or go with IOTLB etc).
Sometimes we can see panic finally but it's not always.
> > Assuming SEV/TDX is involved, the hypervisor should never be able to derive
> > the
> > VM privacy (at least secure memory data) by providing malicious
> > configuration,
> > e.g., num_queues=0.
Yes.
> > If we detect hypervisor is malicious, the VM is
> > panic/shutdown immediately.
This seems to enforce the policy into the kernel, we need to leave
this to userspace to decide.
> > At least how about to print error message to
> > reminder users.
This is fine.
> >
> >
> > BTW, while I always do care about the loss of interrupt issue, the malicious
> > device is able to hang a VM by dropping a single interrupt on purpose for
> > virtio-scsi :)
> >
Right.
> >
> > Thank you very much!
>
>
> Can't say I understand what it's about. TDX does not protect against
> hypervisor DoS attacks.
Yes, I think what Dongli wants to discuss is how to behave if we
detect a malicious hypervisor. He suggested a shutdown instead of
failing the probe.
Thanks
>
> > Dongli Zhang
> >
> > > device is ready. So this patch fixes those two issues by:
> > >
> > > 1) switching to use disable_irq() to prevent the virtio interrupt
> > >handlers to be called after the device is reset.
> > > 2) using IRQF_NO_AUTOEN and enable the MSI-X irq during .ready()
> > >
> > > This can make sure the virtio interrupt handler won't be called before
> > > virtio_device_ready() and after reset.
> > >
> > > Cc: Thomas Gleixner
> > > Cc: Peter Zijlstra
> > > Cc: Paul E. McKenney
> > > Signed-off-by: Jason Wang
> > > ---
> > > drivers/virtio/virtio_pci_common.c | 27 +--
> > > drivers/virtio/virtio_pci_common.h | 6 --
> > > drivers/virtio/virtio_pci_legacy.c | 5 +++--
> > > drivers/virtio/virtio_pci_modern.c | 6 --
> > > 4 files changed, 32 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/drivers/virtio/virtio_pci_common.c
> > > b/drivers/virtio/virtio_pci_common.c
> > > index b35bb2d57f62..0b9523e6dd39 100644
> > > --- a/drivers/virtio/virtio_pci_common.c
> > > +++ b/drivers/virtio/virtio_pci_common.c
> > > @@ -24,8 +24,8 @@ MODULE_PARM_DESC(force_legacy,
> > > "Force legacy mode for transitional virtio 1 devices");
> > > #endif
> > >
> > > -/* wait for pending irq handlers */
> > > -void vp_synchronize_vectors(struct virtio_device *vdev)
> > > +/* disable irq handlers */
> > > +void vp_disable_vectors(struct virtio_device *vdev)
> > > {
> > > struct virtio_pci_device *vp_dev = to_vp_device(vdev);
> > > int i;
> > > @@ -34,7 +34,20 @@ void vp_synchronize_vectors(struct
Re: [PATCH 0/5] iommu/virtio: Add identity domains
On Mon, Oct 18, 2021 at 11:35 PM Michael S. Tsirkin wrote:
>
> On Mon, Oct 18, 2021 at 04:23:41PM +0100, Jean-Philippe Brucker wrote:
> > On Thu, Oct 14, 2021 at 03:00:38AM +, Tian, Kevin wrote:
> > > > From: Jean-Philippe Brucker
> > > > Sent: Wednesday, October 13, 2021 8:11 PM
> > > >
> > > > Support identity domains, allowing to only enable IOMMU protection for a
> > > > subset of endpoints (those assigned to userspace, for example). Users
> > > > may enable identity domains at compile time
> > > > (CONFIG_IOMMU_DEFAULT_PASSTHROUGH), boot time
> > > > (iommu.passthrough=1) or
> > > > runtime (/sys/kernel/iommu_groups/*/type = identity).
> > >
> > > Do we want to use consistent terms between spec (bypass domain)
> > > and code (identity domain)?
> >
> > I don't think we have to. Linux uses "identity" domains and "passthrough"
> > IOMMU. The old virtio-iommu feature was "bypass" so we should keep that
> > for the new one, to be consistent. And then I've used "bypass" for domains
> > as well, in the spec, because it would look strange to use a different
> > term for the same concept. I find that it sort of falls into place: Linux'
> > identity domains can be implemented either with bypass or identity-mapped
> > virtio-iommu domains.
> >
> > > >
> > > > Patches 1-2 support identity domains using the optional
> > > > VIRTIO_IOMMU_F_BYPASS_CONFIG feature. The feature bit is not yet in the
> > > > spec, see [1] for the latest proposal.
> > > >
> > > > Patches 3-5 add a fallback to identity mappings, when the feature is not
> > > > supported.
> > > >
> > > > Note that this series doesn't touch the global bypass bit added by
> > > > VIRTIO_IOMMU_F_BYPASS_CONFIG. All endpoints managed by the IOMMU
> > > > should
> > > > be attached to a domain, so global bypass isn't in use after endpoints
> > >
> > > I saw a concept of deferred attach in iommu core. See iommu_is_
> > > attach_deferred(). Currently this is vendor specific and I haven't
> > > looked into the exact reason why some vendor sets it now. Just
> > > be curious whether the same reason might be applied to virtio-iommu.
> > >
> > > > are probed. Before that, the global bypass policy is decided by the
> > > > hypervisor and firmware. So I don't think Linux needs to touch the
> > >
> > > This reminds me one thing. The spec says that the global bypass
> > > bit is sticky and not affected by reset.
> >
> > The spec talks about *device* reset, triggered by software writing 0 to
> > the status register, but it doesn't mention system reset. Would be good to
> > clarify that. Something like:
> >
> > If the device offers the VIRTIO_IOMMU_F_BYPASS_CONFIG feature, it MAY
> > initialize the \field{bypass} field to 1. Field \field{bypass} SHOULD
> > NOT change on device reset, but SHOULD be restored to its initial
> > value on system reset.
> >
> > > This implies that in the case
> > > of rebooting the VM into a different OS, the previous OS actually
> > > has the right to override this setting for the next OS. Is it a right
> > > design? Even the firmware itself is unable to identify the original
> > > setting enforced by the hypervisor after reboot. I feel the hypervisor
> > > setting should be recovered after reset since it reflects the
> > > security measure enforced by the virtual platform?
> >
> > So I think clarifying system reset should address your questions.
> > I believe we should leave bypass sticky across device reset, so a FW->OS
> > transition, where the OS resets the device, does not open a vulnerability
> > (if bypass was enabled at boot and then left disabled by FW.)
> >
> > It's still a good idea for the OS to restore on shutdown the bypass value
> > it was booted with. So it can kexec into an OS that doesn't support
> > virtio-iommu, for example.
> >
> > Thanks,
> > Jean
>
> Is this stickiness really important? Can't this be addressed just by
> hypervisor disabling bypass at boot?
And I'm not sure if sticky can survive transport reset.
Thanks
>
> --
> MST
>
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/5] iova: Move fast alloc size roundup into alloc_iova_fast()
On Fri, Sep 24, 2021 at 06:01:53PM +0800, John Garry wrote: > It really is a property of the IOVA rcache code that we need to alloc a > power-of-2 size, so relocate the functionality to resize into > alloc_iova_fast(), rather than the callsites. > > Signed-off-by: John Garry for vdpa code: Acked-by: Michael S. Tsirkin > --- > drivers/iommu/dma-iommu.c| 8 > drivers/iommu/iova.c | 9 + > drivers/vdpa/vdpa_user/iova_domain.c | 8 > 3 files changed, 9 insertions(+), 16 deletions(-) > > diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c > index 896bea04c347..a99b3445fef8 100644 > --- a/drivers/iommu/dma-iommu.c > +++ b/drivers/iommu/dma-iommu.c > @@ -444,14 +444,6 @@ static dma_addr_t iommu_dma_alloc_iova(struct > iommu_domain *domain, > > shift = iova_shift(iovad); > iova_len = size >> shift; > - /* > - * Freeing non-power-of-two-sized allocations back into the IOVA caches > - * will come back to bite us badly, so we have to waste a bit of space > - * rounding up anything cacheable to make sure that can't happen. The > - * order of the unadjusted size will still match upon freeing. > - */ > - if (iova_len < (1 << (IOVA_RANGE_CACHE_MAX_SIZE - 1))) > - iova_len = roundup_pow_of_two(iova_len); > > dma_limit = min_not_zero(dma_limit, dev->bus_dma_limit); > > diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c > index 9e8bc802ac05..ff567cbc42f7 100644 > --- a/drivers/iommu/iova.c > +++ b/drivers/iommu/iova.c > @@ -497,6 +497,15 @@ alloc_iova_fast(struct iova_domain *iovad, unsigned long > size, > unsigned long iova_pfn; > struct iova *new_iova; > > + /* > + * Freeing non-power-of-two-sized allocations back into the IOVA caches > + * will come back to bite us badly, so we have to waste a bit of space > + * rounding up anything cacheable to make sure that can't happen. The > + * order of the unadjusted size will still match upon freeing. > + */ > + if (size < (1 << (IOVA_RANGE_CACHE_MAX_SIZE - 1))) > + size = roundup_pow_of_two(size); > + > iova_pfn = iova_rcache_get(iovad, size, limit_pfn + 1); > if (iova_pfn) > return iova_pfn; > diff --git a/drivers/vdpa/vdpa_user/iova_domain.c > b/drivers/vdpa/vdpa_user/iova_domain.c > index 1daae2608860..2b1143f11d8f 100644 > --- a/drivers/vdpa/vdpa_user/iova_domain.c > +++ b/drivers/vdpa/vdpa_user/iova_domain.c > @@ -292,14 +292,6 @@ vduse_domain_alloc_iova(struct iova_domain *iovad, > unsigned long iova_len = iova_align(iovad, size) >> shift; > unsigned long iova_pfn; > > - /* > - * Freeing non-power-of-two-sized allocations back into the IOVA caches > - * will come back to bite us badly, so we have to waste a bit of space > - * rounding up anything cacheable to make sure that can't happen. The > - * order of the unadjusted size will still match upon freeing. > - */ > - if (iova_len < (1 << (IOVA_RANGE_CACHE_MAX_SIZE - 1))) > - iova_len = roundup_pow_of_two(iova_len); > iova_pfn = alloc_iova_fast(iovad, iova_len, limit >> shift, true); > > return iova_pfn << shift; > -- > 2.26.2 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 0/5] iommu/virtio: Add identity domains
On Mon, Oct 18, 2021 at 04:23:41PM +0100, Jean-Philippe Brucker wrote:
> On Thu, Oct 14, 2021 at 03:00:38AM +, Tian, Kevin wrote:
> > > From: Jean-Philippe Brucker
> > > Sent: Wednesday, October 13, 2021 8:11 PM
> > >
> > > Support identity domains, allowing to only enable IOMMU protection for a
> > > subset of endpoints (those assigned to userspace, for example). Users
> > > may enable identity domains at compile time
> > > (CONFIG_IOMMU_DEFAULT_PASSTHROUGH), boot time
> > > (iommu.passthrough=1) or
> > > runtime (/sys/kernel/iommu_groups/*/type = identity).
> >
> > Do we want to use consistent terms between spec (bypass domain)
> > and code (identity domain)?
>
> I don't think we have to. Linux uses "identity" domains and "passthrough"
> IOMMU. The old virtio-iommu feature was "bypass" so we should keep that
> for the new one, to be consistent. And then I've used "bypass" for domains
> as well, in the spec, because it would look strange to use a different
> term for the same concept. I find that it sort of falls into place: Linux'
> identity domains can be implemented either with bypass or identity-mapped
> virtio-iommu domains.
>
> > >
> > > Patches 1-2 support identity domains using the optional
> > > VIRTIO_IOMMU_F_BYPASS_CONFIG feature. The feature bit is not yet in the
> > > spec, see [1] for the latest proposal.
> > >
> > > Patches 3-5 add a fallback to identity mappings, when the feature is not
> > > supported.
> > >
> > > Note that this series doesn't touch the global bypass bit added by
> > > VIRTIO_IOMMU_F_BYPASS_CONFIG. All endpoints managed by the IOMMU
> > > should
> > > be attached to a domain, so global bypass isn't in use after endpoints
> >
> > I saw a concept of deferred attach in iommu core. See iommu_is_
> > attach_deferred(). Currently this is vendor specific and I haven't
> > looked into the exact reason why some vendor sets it now. Just
> > be curious whether the same reason might be applied to virtio-iommu.
> >
> > > are probed. Before that, the global bypass policy is decided by the
> > > hypervisor and firmware. So I don't think Linux needs to touch the
> >
> > This reminds me one thing. The spec says that the global bypass
> > bit is sticky and not affected by reset.
>
> The spec talks about *device* reset, triggered by software writing 0 to
> the status register, but it doesn't mention system reset. Would be good to
> clarify that. Something like:
>
> If the device offers the VIRTIO_IOMMU_F_BYPASS_CONFIG feature, it MAY
> initialize the \field{bypass} field to 1. Field \field{bypass} SHOULD
> NOT change on device reset, but SHOULD be restored to its initial
> value on system reset.
>
> > This implies that in the case
> > of rebooting the VM into a different OS, the previous OS actually
> > has the right to override this setting for the next OS. Is it a right
> > design? Even the firmware itself is unable to identify the original
> > setting enforced by the hypervisor after reboot. I feel the hypervisor
> > setting should be recovered after reset since it reflects the
> > security measure enforced by the virtual platform?
>
> So I think clarifying system reset should address your questions.
> I believe we should leave bypass sticky across device reset, so a FW->OS
> transition, where the OS resets the device, does not open a vulnerability
> (if bypass was enabled at boot and then left disabled by FW.)
>
> It's still a good idea for the OS to restore on shutdown the bypass value
> it was booted with. So it can kexec into an OS that doesn't support
> virtio-iommu, for example.
>
> Thanks,
> Jean
Is this stickiness really important? Can't this be addressed just by
hypervisor disabling bypass at boot?
--
MST
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 0/5] iommu/virtio: Add identity domains
On Thu, Oct 14, 2021 at 03:00:38AM +, Tian, Kevin wrote:
> > From: Jean-Philippe Brucker
> > Sent: Wednesday, October 13, 2021 8:11 PM
> >
> > Support identity domains, allowing to only enable IOMMU protection for a
> > subset of endpoints (those assigned to userspace, for example). Users
> > may enable identity domains at compile time
> > (CONFIG_IOMMU_DEFAULT_PASSTHROUGH), boot time
> > (iommu.passthrough=1) or
> > runtime (/sys/kernel/iommu_groups/*/type = identity).
>
> Do we want to use consistent terms between spec (bypass domain)
> and code (identity domain)?
I don't think we have to. Linux uses "identity" domains and "passthrough"
IOMMU. The old virtio-iommu feature was "bypass" so we should keep that
for the new one, to be consistent. And then I've used "bypass" for domains
as well, in the spec, because it would look strange to use a different
term for the same concept. I find that it sort of falls into place: Linux'
identity domains can be implemented either with bypass or identity-mapped
virtio-iommu domains.
> >
> > Patches 1-2 support identity domains using the optional
> > VIRTIO_IOMMU_F_BYPASS_CONFIG feature. The feature bit is not yet in the
> > spec, see [1] for the latest proposal.
> >
> > Patches 3-5 add a fallback to identity mappings, when the feature is not
> > supported.
> >
> > Note that this series doesn't touch the global bypass bit added by
> > VIRTIO_IOMMU_F_BYPASS_CONFIG. All endpoints managed by the IOMMU
> > should
> > be attached to a domain, so global bypass isn't in use after endpoints
>
> I saw a concept of deferred attach in iommu core. See iommu_is_
> attach_deferred(). Currently this is vendor specific and I haven't
> looked into the exact reason why some vendor sets it now. Just
> be curious whether the same reason might be applied to virtio-iommu.
>
> > are probed. Before that, the global bypass policy is decided by the
> > hypervisor and firmware. So I don't think Linux needs to touch the
>
> This reminds me one thing. The spec says that the global bypass
> bit is sticky and not affected by reset.
The spec talks about *device* reset, triggered by software writing 0 to
the status register, but it doesn't mention system reset. Would be good to
clarify that. Something like:
If the device offers the VIRTIO_IOMMU_F_BYPASS_CONFIG feature, it MAY
initialize the \field{bypass} field to 1. Field \field{bypass} SHOULD
NOT change on device reset, but SHOULD be restored to its initial
value on system reset.
> This implies that in the case
> of rebooting the VM into a different OS, the previous OS actually
> has the right to override this setting for the next OS. Is it a right
> design? Even the firmware itself is unable to identify the original
> setting enforced by the hypervisor after reboot. I feel the hypervisor
> setting should be recovered after reset since it reflects the
> security measure enforced by the virtual platform?
So I think clarifying system reset should address your questions.
I believe we should leave bypass sticky across device reset, so a FW->OS
transition, where the OS resets the device, does not open a vulnerability
(if bypass was enabled at boot and then left disabled by FW.)
It's still a good idea for the OS to restore on shutdown the bypass value
it was booted with. So it can kexec into an OS that doesn't support
virtio-iommu, for example.
Thanks,
Jean
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v5 12/16] PCI: Add pci_iomap_host_shared(), pci_iomap_host_shared_range()
On Mon, Oct 18, 2021 at 02:15:47PM +0200, Greg KH wrote:
> On Mon, Oct 11, 2021 at 07:59:17AM -0400, Michael S. Tsirkin wrote:
> > On Sun, Oct 10, 2021 at 03:22:39PM -0700, Andi Kleen wrote:
> > >
> > > > To which Andi replied
> > > > One problem with removing the ioremap opt-in is that
> > > > it's still possible for drivers to get at devices without going
> > > > through probe.
> > > >
> > > > To which Greg replied:
> > > > https://lore.kernel.org/all/yvxbnj431yiww...@kroah.com/
> > > > If there are in-kernel PCI drivers that do not do this, they
> > > > need to be
> > > > fixed today.
> > > >
> > > > Can you guys resolve the differences here?
> > >
> > >
> > > I addressed this in my other mail, but we may need more discussion.
> >
> > Hopefully Greg will reply to that one.
>
> Note, when wanting Greg to reply, someone should at the very least cc:
> him.
"that one" being "Andi's other mail". Which I don't remember what it was,
by now. Sorry.
> {sigh}
>
> greg k-h
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v5 12/16] PCI: Add pci_iomap_host_shared(), pci_iomap_host_shared_range()
On Mon, Oct 11, 2021 at 07:59:17AM -0400, Michael S. Tsirkin wrote:
> On Sun, Oct 10, 2021 at 03:22:39PM -0700, Andi Kleen wrote:
> >
> > > To which Andi replied
> > > One problem with removing the ioremap opt-in is that
> > > it's still possible for drivers to get at devices without going through
> > > probe.
> > >
> > > To which Greg replied:
> > > https://lore.kernel.org/all/yvxbnj431yiww...@kroah.com/
> > > If there are in-kernel PCI drivers that do not do this, they need to be
> > > fixed today.
> > >
> > > Can you guys resolve the differences here?
> >
> >
> > I addressed this in my other mail, but we may need more discussion.
>
> Hopefully Greg will reply to that one.
Note, when wanting Greg to reply, someone should at the very least cc:
him.
{sigh}
greg k-h
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v5 12/16] PCI: Add pci_iomap_host_shared(), pci_iomap_host_shared_range()
On Tue, Oct 12, 2021 at 11:35:04AM -0700, Andi Kleen wrote: > > I'd rather see more concerted efforts focused/limited core changes > > rather than leaf driver changes until there is a clearer definition of > > hardened. > > A hardened driver is a driver that Ah, you do define this, thank you! > - Had similar security (not API) oriented review of its IO operations > (mainly MMIO access, but also PCI config space) as a non privileged user > interface (like a ioctl). That review should be focused on memory safety. Where is this review done? Where is is documented? Who is responsible for keeping it up to date with every code change to the driver, and to the code that the driver calls and the code that calls the driver? > - Had some fuzzing on these IO interfaces using to be released tools. "some"? What tools? What is the input, and where is that defined? How much fuzzing do you claim is "good enough"? > Right now it's only three virtio drivers (console, net, block) Where was this work done and published? And why only 3? > Really it's no different than what we do for every new unprivileged user > interface. Really? I have seen loads of new drivers from Intel submitted in the past months that would fail any of the above things just based on obvious code reviews that I end up having to do... If you want to start a "hardened driver" effort, there's a lot of real work that needs to be done here and documented, and explained why it can not just be done for the whole kernel... greg k-h ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v5 12/16] PCI: Add pci_iomap_host_shared(), pci_iomap_host_shared_range()
On Sun, Oct 10, 2021 at 03:11:23PM -0700, Andi Kleen wrote: > > On 10/9/2021 1:39 PM, Dan Williams wrote: > > On Sat, Oct 9, 2021 at 2:53 AM Michael S. Tsirkin wrote: > > > On Fri, Oct 08, 2021 at 05:37:07PM -0700, Kuppuswamy Sathyanarayanan > > > wrote: > > > > From: Andi Kleen > > > > > > > > For Confidential VM guests like TDX, the host is untrusted and hence > > > > the devices emulated by the host or any data coming from the host > > > > cannot be trusted. So the drivers that interact with the outside world > > > > have to be hardened by sharing memory with host on need basis > > > > with proper hardening fixes. > > > > > > > > For the PCI driver case, to share the memory with the host add > > > > pci_iomap_host_shared() and pci_iomap_host_shared_range() APIs. > > > > > > > > Signed-off-by: Andi Kleen > > > > Signed-off-by: Kuppuswamy Sathyanarayanan > > > > > > > So I proposed to make all pci mappings shared, eliminating the need > > > to patch drivers. > > > > > > To which Andi replied > > > One problem with removing the ioremap opt-in is that > > > it's still possible for drivers to get at devices without going > > > through probe. > > > > > > To which Greg replied: > > > https://lore.kernel.org/all/yvxbnj431yiww...@kroah.com/ > > > If there are in-kernel PCI drivers that do not do this, they > > > need to be > > > fixed today. > > > > > > Can you guys resolve the differences here? > > I agree with you and Greg here. If a driver is accessing hardware > > resources outside of the bind lifetime of one of the devices it > > supports, and in a way that neither modrobe-policy nor > > device-authorization -policy infrastructure can block, that sounds > > like a bug report. > > The 5.15 tree has something like ~2.4k IO accesses (including MMIO and > others) in init functions that also register drivers (thanks Elena for the > number) > > Some are probably old drivers that could be fixed, but it's quite a few > legitimate cases. For example for platform or ISA drivers that's the only > way they can be implemented because they often have no other enumeration > mechanism. For PCI drivers it's rarer, but also still can happen. One > example that comes to mind here is the x86 Intel uncore drivers, which > support a mix of MSR, ioremap and PCI config space accesses all from the > same driver. This particular example can (and should be) fixed in other > ways, but similar things also happen in other drivers, and they're not all > broken. Even for the broken ones they're usually for some crufty old devices > that has very few users, so it's likely untestable in practice. > > My point is just that the ecosystem of devices that Linux supports is messy > enough that there are legitimate exceptions from the "First IO only in probe > call only" rule. No, there should not be for PCI drivers. If there is, that is a bug that you can, and should, fix. > And we can't just fix them all. Even if we could it would be hard to > maintain. Not true at all, you can fix them, and write a simple coccinelle rule to prevent them from ever coming back in. > Using a "firewall model" hooking into a few strategic points like we're > proposing here is much saner for everyone. No it is not. It is "easier" for you because you all do not want to fix up all of the drivers and want to add additional code complexity on top of the current mess that we have and then you can claim that you have "hardened" the drivers you care about. Despite no one ever explaining exactly what "hardened" means to me. Again, fix the existing drivers, you have the whole source, if this is something that you all care about, it should not be hard to do. Stop making excuses. greg k-h ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 0/5] iommu/virtio: Add identity domains
On Thu, Oct 14, 2021 at 03:00:38AM +, Tian, Kevin wrote: > I saw a concept of deferred attach in iommu core. See iommu_is_ > attach_deferred(). Currently this is vendor specific and I haven't > looked into the exact reason why some vendor sets it now. Just > be curious whether the same reason might be applied to virtio-iommu. The reason for attach_deferred is kdump support, where the IOMMU driver needs to keep the mappings from the old kernel until the device driver of the new kernel takes over. Regards, Joerg ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH RFC] virtio: wrap config->reset calls
On Wed, Oct 13, 2021 at 06:55:31AM -0400, Michael S. Tsirkin wrote: This will enable cleanups down the road. The idea is to disable cbs, then add "flush_queued_cbs" callback as a parameter, this way drivers can flush any work queued after callbacks have been disabled. Signed-off-by: Michael S. Tsirkin --- arch/um/drivers/virt-pci.c | 2 +- drivers/block/virtio_blk.c | 4 ++-- drivers/bluetooth/virtio_bt.c | 2 +- drivers/char/hw_random/virtio-rng.c| 2 +- drivers/char/virtio_console.c | 4 ++-- drivers/crypto/virtio/virtio_crypto_core.c | 8 drivers/firmware/arm_scmi/virtio.c | 2 +- drivers/gpio/gpio-virtio.c | 2 +- drivers/gpu/drm/virtio/virtgpu_kms.c | 2 +- drivers/i2c/busses/i2c-virtio.c| 2 +- drivers/iommu/virtio-iommu.c | 2 +- drivers/net/caif/caif_virtio.c | 2 +- drivers/net/virtio_net.c | 4 ++-- drivers/net/wireless/mac80211_hwsim.c | 2 +- drivers/nvdimm/virtio_pmem.c | 2 +- drivers/rpmsg/virtio_rpmsg_bus.c | 2 +- drivers/scsi/virtio_scsi.c | 2 +- drivers/virtio/virtio.c| 5 + drivers/virtio/virtio_balloon.c| 2 +- drivers/virtio/virtio_input.c | 2 +- drivers/virtio/virtio_mem.c| 2 +- fs/fuse/virtio_fs.c| 4 ++-- include/linux/virtio.h | 1 + net/9p/trans_virtio.c | 2 +- net/vmw_vsock/virtio_transport.c | 4 ++-- Reviewed-by: Stefano Garzarella ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v4 3/3] vdpa: Check for iova range at mappings changes
On Thu, Oct 14, 2021 at 04:12:36PM +0200, Eugenio Pérez wrote: Check vdpa device range before updating memory regions so we don't add any outside of it, and report the invalid change if any. Signed-off-by: Eugenio Pérez --- include/hw/virtio/vhost-vdpa.h | 2 ++ hw/virtio/vhost-vdpa.c | 62 +- hw/virtio/trace-events | 1 + 3 files changed, 49 insertions(+), 16 deletions(-) Reviewed-by: Stefano Garzarella ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v4 2/3] vdpa: Add vhost_vdpa_section_end
On Thu, Oct 14, 2021 at 04:12:35PM +0200, Eugenio Pérez wrote: Abstract this operation, that will be reused when validating the region against the iova range that the device supports. Signed-off-by: Eugenio Pérez Acked-by: Jason Wang --- hw/virtio/vhost-vdpa.c | 22 +++--- 1 file changed, 15 insertions(+), 7 deletions(-) Reviewed-by: Stefano Garzarella ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v4 1/3] vdpa: Skip protected ram IOMMU mappings
On Thu, Oct 14, 2021 at 04:12:34PM +0200, Eugenio Pérez wrote:
Following the logic of commit 56918a126ae ("memory: Add RAM_PROTECTED
flag to skip IOMMU mappings") with VFIO, skip memory sections
inaccessible via normal mechanisms, including DMA.
Signed-off-by: Eugenio Pérez
Acked-by: Jason Wang
---
hw/virtio/vhost-vdpa.c | 1 +
1 file changed, 1 insertion(+)
Reviewed-by: Stefano Garzarella
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH RFC] virtio: wrap config->reset calls
On Wed, Oct 13, 2021 at 06:55:31AM -0400, Michael S. Tsirkin wrote: > This will enable cleanups down the road. > The idea is to disable cbs, then add "flush_queued_cbs" callback > as a parameter, this way drivers can flush any work > queued after callbacks have been disabled. > > Signed-off-by: Michael S. Tsirkin > --- > arch/um/drivers/virt-pci.c | 2 +- > drivers/block/virtio_blk.c | 4 ++-- > drivers/bluetooth/virtio_bt.c | 2 +- > drivers/char/hw_random/virtio-rng.c| 2 +- > drivers/char/virtio_console.c | 4 ++-- > drivers/crypto/virtio/virtio_crypto_core.c | 8 > drivers/firmware/arm_scmi/virtio.c | 2 +- > drivers/gpio/gpio-virtio.c | 2 +- > drivers/gpu/drm/virtio/virtgpu_kms.c | 2 +- > drivers/i2c/busses/i2c-virtio.c| 2 +- > drivers/iommu/virtio-iommu.c | 2 +- > drivers/net/caif/caif_virtio.c | 2 +- > drivers/net/virtio_net.c | 4 ++-- > drivers/net/wireless/mac80211_hwsim.c | 2 +- > drivers/nvdimm/virtio_pmem.c | 2 +- > drivers/rpmsg/virtio_rpmsg_bus.c | 2 +- > drivers/scsi/virtio_scsi.c | 2 +- > drivers/virtio/virtio.c| 5 + > drivers/virtio/virtio_balloon.c| 2 +- > drivers/virtio/virtio_input.c | 2 +- > drivers/virtio/virtio_mem.c| 2 +- > fs/fuse/virtio_fs.c| 4 ++-- > include/linux/virtio.h | 1 + > net/9p/trans_virtio.c | 2 +- > net/vmw_vsock/virtio_transport.c | 4 ++-- > sound/virtio/virtio_card.c | 4 ++-- > 26 files changed, 39 insertions(+), 33 deletions(-) Reviewed-by: Stefan Hajnoczi signature.asc Description: PGP signature ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [GIT PULL] virtio,vdpa: fixes
The pull request you sent on Sun, 17 Oct 2021 10:49:00 -0400: > https://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git tags/for_linus has been merged into torvalds/linux.git: https://git.kernel.org/torvalds/c/3bb50f8530c9cb5ec69c0744b7fd32d0ca404254 Thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/prtracker.html ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
