From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jan Kiszka Subject: Re: [PATCH 12/13] pci-assign: Generic config space access management Date: Tue, 28 Jun 2011 11:20:06 +0200 Message-ID: <4E099CC6.8010506@web.de> References: <007a0322c5a4af8a621375c4d2951eee01f2aa05.1309198794.git.jan.kiszka@web.de> <20110628081016.GG10881@redhat.com> <4E098E4C.9080901@web.de> <20110628083017.GK10881@redhat.com> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="------------enigB0D78A727EF5C67235E312A3" Cc: Avi Kivity , Marcelo Tosatti , kvm@vger.kernel.org, Alex Williamson To: "Michael S. Tsirkin" Return-path: Received: from fmmailgate03.web.de ([217.72.192.234]:58328 "EHLO fmmailgate03.web.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756776Ab1F1JUJ (ORCPT ); Tue, 28 Jun 2011 05:20:09 -0400 In-Reply-To: <20110628083017.GK10881@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: This is an OpenPGP/MIME signed message (RFC 2440 and 3156) --------------enigB0D78A727EF5C67235E312A3 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable On 2011-06-28 10:30, Michael S. Tsirkin wrote: > On Tue, Jun 28, 2011 at 10:18:20AM +0200, Jan Kiszka wrote: >>> Is this an optimization? What harm does it do to virtualize always? >> >> Just replicating the old behavior, >=20 > Oh, I absolutely agree with that. > It's better to do more cleanups in separate patches, > this one is huge so it's better if bisect can lead us to > a specific change. >=20 >> but I can virtualize it unconditionally. >=20 >=20 > Just pointing out follow-up > cleanups that become possible. >=20 >>> >>>> =20 >>>> return 0; >>>> @@ -1694,6 +1571,26 @@ static int assigned_initfn(struct PCIDevice *= pci_dev) >>>> return -1; >>>> } >>>> =20 >>>> + /* >>>> + * Set up basic config space access control. Will be further re= fined during >>>> + * device initialization. >>>> + * >>>> + * Direct read/write access is grangted for: >>> >>> typo >>> >>>> + * - status & command registers, revision ID & class code, cac= heline size, >>>> + * latency timer, header type, BIST >>>> + * - cardbus CIS pointer, subsystem vendor & ID >>>> + * - reserved fields >>>> + * - Min_Gnt & Max_Lat >>>> + */ >>>> + memset(dev->direct_config_read + PCI_COMMAND, 0xff, 12); >>>> + memset(dev->direct_config_read + PCI_CARDBUS_CIS, 0xff, 8); >>>> + memset(dev->direct_config_read + PCI_CAPABILITY_LIST + 1, 0xff,= 7); >>>> + memset(dev->direct_config_read + PCI_MIN_GNT, 0xff, 2); >>>> + memset(dev->direct_config_write + PCI_COMMAND, 0xff, 12); >>>> + memset(dev->direct_config_write + PCI_CARDBUS_CIS, 0xff, 8); >>>> + memset(dev->direct_config_write + PCI_CAPABILITY_LIST + 1, 0xff= , 7); >>>> + memset(dev->direct_config_write + PCI_MIN_GNT, 0xff, 2); >>>> + >>>> if (get_real_device(dev, dev->host.seg, dev->host.bus, >>>> dev->host.dev, dev->host.func)) { >>>> error_report("pci-assign: Error: Couldn't get real device (= %s)!", >>>> diff --git a/hw/device-assignment.h b/hw/device-assignment.h >>>> index 3d20207..ed5defb 100644 >>>> --- a/hw/device-assignment.h >>>> +++ b/hw/device-assignment.h >>>> @@ -104,12 +104,13 @@ typedef struct AssignedDevice { >>>> #define ASSIGNED_DEVICE_MSIX_MASKED (1 << 2) >>>> uint32_t state; >>>> } cap; >>>> + uint8_t direct_config_read[PCI_CONFIG_SPACE_SIZE]; >>>> + uint8_t direct_config_write[PCI_CONFIG_SPACE_SIZE]; >>> >>> What direct means isn't obvious. Add a comment? >> >> Ok. >> >>> >>> We could revert the logic, have bits for >>> emulate_config_read/emulate_config_write. I think that most of the >>> registers can safely be read/written directly, so protecting relevant= >>> bits will be less work. Is that true? >> >> I intentionally chose whitelisting to avoid accidentally granting acce= ss >> to forgotten fields. I don't think it's a good idea to switch to >> blacklisting. >> >> Jan >> >=20 > At least in theory iommu protects us from most harm device can do > (besides downstream transactions, which is why we must protect the BAR)= =2E > Even if we change things, it probably should be a separate patch since > whitelisting is what existing code had. > However, I think emulate_xx is just a better name for the field. > You can preset them to all ones if you think it's safer. OK, that sounds reasonable. Jan --------------enigB0D78A727EF5C67235E312A3 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.15 (GNU/Linux) Comment: Using GnuPG with SUSE - http://enigmail.mozdev.org/ iEYEARECAAYFAk4JnMYACgkQitSsb3rl5xSrXgCfYsvBJm9gnmDtNxudVFBm4oY1 Z3oAoOskY937vEzVM+1/4FIBD9DHJLCE =Fhnm -----END PGP SIGNATURE----- --------------enigB0D78A727EF5C67235E312A3--