[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 0/2] VT-d: correct / extend workaround(s) leaving an IOMMU disabled [and 1 more messages]


  • To: Ian Jackson <iwj@xxxxxxxxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Mon, 11 Oct 2021 13:09:38 +0200
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=suse.com; dmarc=pass action=none header.from=suse.com; dkim=pass header.d=suse.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=cj2s4Ep6h3MsZ/i8kx3tvEoapr0vwNyWOGWcj7wJ3Xw=; b=SkMKOKRVSC5PMqZTe12+p2iRf3A2AYhJeNPx8UAQu1sKodTo8+CzUJQiwpxQKfBb2u0Wad2c7Xr6nVxqhIQS/RhhNdgcrdQqRlGsbt6OkWvMNFcxqMvqXkghIbeZRNWXo3UsRdGinEEhNXCzVTkqsPGLElapZ2EW3avbrHbZGHX0Rg88MIkgAQrw+weyWD73wiCnrwR0tQ4FCCyHX1jT3AA+p5i96MVq48FIVxRZqN7YfpcP8v1M3riw/tS3RDjs+buvnIu+EIsQUumbkp8jSCJs0WcH+bPgx9UWiJKUav80A/ezLE2z0xI+1j7AeZ2a7jHkU85q4C1RLjFwvlkAIw==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=bOVec5BsMoydb4pc83shMElX6A8u608ZQN3Wy0RTds8Q5DJ+uniXXXeiMkjo39wkzNR/6lkbWwA1gMARHEOIlVDs2IxWQN5/bUHcVZPqE5SCTv3aikxQetMW+epuBRWG5MYC6R5wCnq6ZT19ek6UvAicpOcflXgw0XjcVHVv6qCC+oVky/LsM/uCjx87tUy1q4vm5lSlD9/8ytKuZmu6sjgetGzhXYoz8J76Y0amf5Oc/ajY36ZNBSGVUSsKbnFMUVFdcHtLxhbh77gPURqtRYj34+fiPJ6R27u2VesvaC1v2ieYgwMJ7QboLP6CbX9xlF97FyaYCJtSYDYadtOHkA==
  • Authentication-results: intel.com; dkim=none (message not signed) header.d=none;intel.com; dmarc=none action=none header.from=suse.com;
  • Cc: "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Kevin Tian <kevin.tian@xxxxxxxxx>
  • Delivery-date: Mon, 11 Oct 2021 11:09:49 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 11.10.2021 12:56, Ian Jackson wrote:
> Jan Beulich writes ("[PATCH 0/2] VT-d: correct / extend workaround(s) leaving 
> an IOMMU disabled"):
>> Ian - I'm also Cc-ing you since this feels like being on the edge
>> between a new feature and a bug fix.
> 
> Thanks.
> 
> I think 2/ is a new quirk (or, new behaviour for an existing quirk).
> I think I am happy to treat that as a bugfix, assuming we are
> reasonably confident that most systems (including in particular all
> systems without the quirk) will take unchanged codepaths.  Is that
> right ?

Yes. According to Linux there's exactly one BIOS flavor known to
exhibit the issue.

> I don't understand 1/.  It looks bugfixish to me but I am really not
> qualified.  I am inclined to defer to your judgement, but it would
> help me if you explicitly addressed the overall risks/benefits.

Right now our documentation claims similarity to a Linux workaround
without the similarity actually existing in the general case. A
common case (a single integrated graphics device) is handled, but the
perhaps yet more common case of a single add-in graphics devices is
not. Plus the criteria by which a device is determined to be a
graphics one was completely flawed. Hence people in need of the
workaround may find it non-functional. However, since our doc tells
people to report if they have a need to use the option engaging the
workaround, and since we didn't have any such reports in a number
of years, I guess both benefits and possible risks here are of
purely theoretical nature. Note that I've specifically said "possible"
because I can't really see any beyond me not having properly matched
Linux'es equivalent workaround - that workaround has been in place
unchanged for very many years.

> But when reading the patch I did notice one thing that struck me as
> undesriable:
> 
>> --- a/xen/drivers/passthrough/vtd/iommu.c
>> +++ b/xen/drivers/passthrough/vtd/iommu.c
>> @@ -750,27 +750,43 @@ static void iommu_enable_translation(str
>>              if ( force_iommu )
>> -                panic("BIOS did not enable IGD for VT properly, crash Xen 
>> for security purpose\n");
>> +                panic(crash_fmt, msg);
> ...
>> +        if ( force_iommu )
>> +            panic(crash_fmt, msg);
> 
> Does this really mean that every exit path from
> iommu_enable_translation that doesn't enable the iommu has to have a
> check for force_iommu ?
> 
> That seems like a recipe for missing one.  And I think a missed one
> would be an XSA.  Could we not structure the code some way to avoid
> this foreseeable human error ?

I'm afraid I don't see a good way to do so, as imo it's desirable to
have separate log messages. IOW something like

    if ( ... )
    {
        msg = "...";
        goto dead;
    }

doesn't look any better to me. Also leaving individual IOMMUs disabled
should really be the exception anyway.

Jan




 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.