| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 08 May 2012 09:18:22 -0600
From: Alex Williamson <alex.williamson@...hat.com>
To: "Hao, Xudong" <xudong.hao@...el.com>
Cc: Avi Kivity <avi@...hat.com>,
Xudong Hao <xudong.hao@...ux.intel.com>,
"mtosatti@...hat.com" <mtosatti@...hat.com>,
"kvm@...r.kernel.org" <kvm@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"Zhang, Xiantao" <xiantao.zhang@...el.com>
Subject: RE: [PATCH] kvm: Enable device LTR/OBFF capibility before doing
guest device assignment
On Tue, 2012-05-08 at 09:16 +0000, Hao, Xudong wrote:
> > -----Original Message-----
> > From: Alex Williamson [mailto:alex.williamson@...hat.com]
> > Sent: Tuesday, May 08, 2012 12:16 AM
> > To: Hao, Xudong
> > Cc: Avi Kivity; Xudong Hao; mtosatti@...hat.com; kvm@...r.kernel.org;
> > linux-kernel@...r.kernel.org; Zhang, Xiantao
> > Subject: RE: [PATCH] kvm: Enable device LTR/OBFF capibility before doing guest
> > device assignment
> >
> > On Mon, 2012-05-07 at 07:58 +0000, Hao, Xudong wrote:
> > > > -----Original Message-----
> > > > From: Avi Kivity [mailto:avi@...hat.com]
> > > > Sent: Sunday, May 06, 2012 11:34 PM
> > > > To: Xudong Hao
> > > > Cc: mtosatti@...hat.com; kvm@...r.kernel.org;
> > linux-kernel@...r.kernel.org;
> > > > Zhang, Xiantao; Hao, Xudong; Alex Williamson
> > > > Subject: Re: [PATCH] kvm: Enable device LTR/OBFF capibility before doing
> > guest
> > > > device assignment
> > > >
> > > > On 05/06/2012 06:24 PM, Xudong Hao wrote:
> > > > > Enable device LTR/OBFF capibility before do device assignment, so that
> > guest
> > > > can benefit from them.
> > > >
> > > > cc += Alex
> > > >
> > > > > @@ -166,6 +166,10 @@ int kvm_assign_device(struct kvm *kvm,
> > > > > if (pdev == NULL)
> > > > > return -ENODEV;
> > > > >
> > > > > + /* Enable some device capibility before do device assignment,
> > > > > + * so that guest can benefit from them.
> > > > > + */
> > > > > + kvm_iommu_enable_dev_caps(pdev);
> > > > > r = iommu_attach_device(domain, &pdev->dev);
> > > >
> > > > Suppose we fail here. Do we need to disable_dev_caps()?
> > > >
> >
> > If kvm_assign_device() fails we'll try to restore the state we saved in
> > kvm_vm_ioctl_assign_device(), so ltr/obff should be brought back to
> > initial state.
> >
> Right, more clear.
>
> > > I don't think so. When a device will be assigned to guest, it's be
> > > owned by a pci-stub driver, attach_device fail here do not affect
> > > everything. If host want to use it, host device driver has its own
> > > enable/disable dev_caps.
> >
> > Why is device assignment unique here? If there's a default value that's
> > known to be safe, why doesn't pci_enable_device set it for everyone?
> > Host drivers can fine tune the value later if they want.
> >
> > > > > if (r) {
> > > > > printk(KERN_ERR "assign device %x:%x:%x.%x failed",
> > > > > @@ -228,6 +232,7 @@ int kvm_deassign_device(struct kvm *kvm,
> > > > > PCI_SLOT(assigned_dev->host_devfn),
> > > > > PCI_FUNC(assigned_dev->host_devfn));
> > > > >
> > > > > + kvm_iommu_disable_dev_caps(pdev);
> > > > > return 0;
> > > > > }
> > > > >
> > > > > @@ -351,3 +356,30 @@ int kvm_iommu_unmap_guest(struct kvm *kvm)
> > > > > iommu_domain_free(domain);
> > > > > return 0;
> > > > > }
> > > > > +
> > > > > +static void kvm_iommu_enable_dev_caps(struct pci_dev *pdev)
> > > > > +{
> > > > > + /* set default value */
> > > > > + unsigned long type = PCI_EXP_OBFF_SIGNAL_ALWAYS;
> > > > > + int snoop_lat_ns = 1024, nosnoop_lat_ns = 1024;
> > > >
> > > > Where does this magic number come from?
> > > >
> > > The number is the max value that register support, set it as default
> > > here, we did not have any device here, and we do not know what's the
> > > proper value, so it set a default value firstly.
> >
> > The register is composed of latency scale and latency value fields.
> > 1024 is simply the largest value the latency value can hold (+1). The
> > scale field allows latencies up to 34,326,183,936ns to be specified, so
> > please explain how 1024 is a universal default.
> >
>
> Since each platform will have its own max supported latency, I think
> the best way is setting the value to 0 because we have such a device
> now.
What's the benefit to that device vs the risk to other devices? Again,
if there's a safe default value for both LTR and OBFF, why isn't PCI
core setting it for everyone? I'm inclined to wait for qemu express
support and expose LTR/OBFF control to the guest if and only if we can
enable it on the root complex and intermediate switches. Thanks,
Alex
> > > > > +
> > > > > + /* LTR(Latency tolerance reporting) allows devices to send
> > > > > + * messages to the root complex indicating their latency
> > > > > + * tolerance for snooped & unsnooped memory transactions.
> > > > > + */
> > > > > + pci_enable_ltr(pdev);
> > > > > + pci_set_ltr(pdev, snoop_lat_ns, nosnoop_lat_ns);
> > > > > +
> > > > > + /* OBFF (optimized buffer flush/fill), where supported,
> > > > > + * can help improve energy efficiency by giving devices
> > > > > + * information about when interrupts and other activity
> > > > > + * will have a reduced power impact.
> > > > > + */
> > > > > + pci_enable_obff(pdev, type);
> > > > > +}
> > > > > +
> > > > > +static void kvm_iommu_disable_dev_caps(struct pci_dev *pdev)
> > > > > +{
> > > > > + pci_disble_obff(pdev);
> > > > > + pci_disble_ltr(pdev);
> > > > > +}
> > > >
> > > > Do we need to communicate something about these capabilities to the
> > guest?
> > > >
> > >
> > > I guess you means that here host don't know if guest want to enable them,
> > right?
> > > The ltr/obff new feature are supposed to enabled by guest if platform and
> > device supported.
> >
> > It looks like ltr is a two part mechanism, the capability and enable
> > lives in the pci express capability, but the tuning registers live in
> > extended capability space. The guest doesn't yet have access to the
> > latter since we don't have an express chipset. The capability and
> > enable are read-only to the guest currently, same for obff. Thanks,
> >
> > Alex
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists