[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
RE: [PATCH V3 13/13] HV/Storvsc: Add Isolation VM support for storvsc driver
- To: "hch@xxxxxx" <hch@xxxxxx>
- From: Michael Kelley <mikelley@xxxxxxxxxxxxx>
- Date: Fri, 20 Aug 2021 15:40:08 +0000
- Accept-language: en-US
- Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=microsoft.com; dmarc=pass action=none header.from=microsoft.com; dkim=pass header.d=microsoft.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; bh=+/NK5CpwxxqKlpEdtVzC/K4J3I9XlbMl3qZJvfxbzUY=; b=L1hyilwLzh3NcgO4SY21fD8vP3NRvSpHhNinbWLIqXm/dKmwcXh8JZl0ZWBDXO2JrsrHZZE054WmXqGo87S45d46ZP088vi4vF7/34qWqz0XCw8HelKGBF4tmtEWGMkaSJwLqZ5gd5h7zsomiuFNL9BPqG/vdTE/1IjAe3GzNrId+TqY2g0U3LPiZDclw5izVFiaUPhIPqPb1hpxUGbSdGHUN+kY1+AhnX+HLGKMfUgvYv0TTTh53PGso89Onl7sehtIOF9YBySltQn2pvHgihiQ2puMQiRH3e347LMx749xK/rzSvVn8zH6z8zZGqOlgHeu4C51Wk9gaw8GHa2Cqw==
- Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=Twk6JxRCCorgtqG9tUHN26l3kqiniajhANolR2SN+vuVrmuzLs8jxbMOGkiZlb7wdhrGPY+WSWynp23O6LkKw/6ja+ZOcbNaCABlVD2INdsVGWxzYhRMqyP73xikDY6a3t4I5YlZ4JEnQbFLHj4ov1a+9IY7KxfzMvz2L4A8T7CwG5QHsmVZVz7E/cNqIhsBKG/7Wqts5Enwjczk9ViyxZqhg/ehfaPcFgIzvXj96B7GNlk+j8wyYRrqZuzHYGesBEyVbrYbs7KOzbqWR8/cHFhWemqQzi3frQkk189dgW5UrOWSgNZA/WfbEyDf7GBZCOhcLOeRxxIdSGzL1Gt7Nw==
- Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=microsoft.com;
- Cc: Tianyu Lan <ltykernel@xxxxxxxxx>, KY Srinivasan <kys@xxxxxxxxxxxxx>, Haiyang Zhang <haiyangz@xxxxxxxxxxxxx>, Stephen Hemminger <sthemmin@xxxxxxxxxxxxx>, "wei.liu@xxxxxxxxxx" <wei.liu@xxxxxxxxxx>, Dexuan Cui <decui@xxxxxxxxxxxxx>, "tglx@xxxxxxxxxxxxx" <tglx@xxxxxxxxxxxxx>, "mingo@xxxxxxxxxx" <mingo@xxxxxxxxxx>, "bp@xxxxxxxxx" <bp@xxxxxxxxx>, "x86@xxxxxxxxxx" <x86@xxxxxxxxxx>, "hpa@xxxxxxxxx" <hpa@xxxxxxxxx>, "dave.hansen@xxxxxxxxxxxxxxx" <dave.hansen@xxxxxxxxxxxxxxx>, "luto@xxxxxxxxxx" <luto@xxxxxxxxxx>, "peterz@xxxxxxxxxxxxx" <peterz@xxxxxxxxxxxxx>, "konrad.wilk@xxxxxxxxxx" <konrad.wilk@xxxxxxxxxx>, "boris.ostrovsky@xxxxxxxxxx" <boris.ostrovsky@xxxxxxxxxx>, "jgross@xxxxxxxx" <jgross@xxxxxxxx>, "sstabellini@xxxxxxxxxx" <sstabellini@xxxxxxxxxx>, "joro@xxxxxxxxxx" <joro@xxxxxxxxxx>, "will@xxxxxxxxxx" <will@xxxxxxxxxx>, "davem@xxxxxxxxxxxxx" <davem@xxxxxxxxxxxxx>, "kuba@xxxxxxxxxx" <kuba@xxxxxxxxxx>, "jejb@xxxxxxxxxxxxx" <jejb@xxxxxxxxxxxxx>, "martin.petersen@xxxxxxxxxx" <martin.petersen@xxxxxxxxxx>, "arnd@xxxxxxxx" <arnd@xxxxxxxx>, "m.szyprowski@xxxxxxxxxxx" <m.szyprowski@xxxxxxxxxxx>, "robin.murphy@xxxxxxx" <robin.murphy@xxxxxxx>, "thomas.lendacky@xxxxxxx" <thomas.lendacky@xxxxxxx>, "brijesh.singh@xxxxxxx" <brijesh.singh@xxxxxxx>, "ardb@xxxxxxxxxx" <ardb@xxxxxxxxxx>, Tianyu Lan <Tianyu.Lan@xxxxxxxxxxxxx>, "pgonda@xxxxxxxxxx" <pgonda@xxxxxxxxxx>, "martin.b.radev@xxxxxxxxx" <martin.b.radev@xxxxxxxxx>, "akpm@xxxxxxxxxxxxxxxxxxxx" <akpm@xxxxxxxxxxxxxxxxxxxx>, "kirill.shutemov@xxxxxxxxxxxxxxx" <kirill.shutemov@xxxxxxxxxxxxxxx>, "rppt@xxxxxxxxxx" <rppt@xxxxxxxxxx>, "sfr@xxxxxxxxxxxxxxxx" <sfr@xxxxxxxxxxxxxxxx>, "saravanand@xxxxxx" <saravanand@xxxxxx>, "krish.sadhukhan@xxxxxxxxxx" <krish.sadhukhan@xxxxxxxxxx>, "aneesh.kumar@xxxxxxxxxxxxx" <aneesh.kumar@xxxxxxxxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>, "rientjes@xxxxxxxxxx" <rientjes@xxxxxxxxxx>, "hannes@xxxxxxxxxxx" <hannes@xxxxxxxxxxx>, "tj@xxxxxxxxxx" <tj@xxxxxxxxxx>, "iommu@xxxxxxxxxxxxxxxxxxxxxxxxxx" <iommu@xxxxxxxxxxxxxxxxxxxxxxxxxx>, "linux-arch@xxxxxxxxxxxxxxx" <linux-arch@xxxxxxxxxxxxxxx>, "linux-hyperv@xxxxxxxxxxxxxxx" <linux-hyperv@xxxxxxxxxxxxxxx>, "linux-kernel@xxxxxxxxxxxxxxx" <linux-kernel@xxxxxxxxxxxxxxx>, "linux-scsi@xxxxxxxxxxxxxxx" <linux-scsi@xxxxxxxxxxxxxxx>, "netdev@xxxxxxxxxxxxxxx" <netdev@xxxxxxxxxxxxxxx>, vkuznets <vkuznets@xxxxxxxxxx>, "parri.andrea@xxxxxxxxx" <parri.andrea@xxxxxxxxx>, "dave.hansen@xxxxxxxxx" <dave.hansen@xxxxxxxxx>
- Delivery-date: Fri, 20 Aug 2021 15:40:25 +0000
- List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
- Msip_labels: MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_ActionId=ec22af1e-6e43-4509-a7bc-50a42915e01c;MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_ContentBits=0;MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_Enabled=true;MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_Method=Standard;MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_Name=Internal;MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_SetDate=2021-08-20T15:01:05Z;MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_SiteId=72f988bf-86f1-41af-91ab-2d7cd011db47;
- Thread-index: AQHXjUf4MnryDC/QY0a+heAsfHsxAqt3v7VggAQeiYCAAK+YMA==
- Thread-topic: [PATCH V3 13/13] HV/Storvsc: Add Isolation VM support for storvsc driver
From: hch@xxxxxx <hch@xxxxxx> Sent: Thursday, August 19, 2021 9:33 PM
>
> On Thu, Aug 19, 2021 at 06:17:40PM +0000, Michael Kelley wrote:
> > >
> > > @@ -1824,6 +1848,13 @@ static int storvsc_queuecommand(struct Scsi_Host
> > > *host, struct scsi_cmnd *scmnd)
> > > payload->range.len = length;
> > > payload->range.offset = offset_in_hvpg;
> > >
> > > + cmd_request->dma_range = kcalloc(hvpg_count,
> > > + sizeof(*cmd_request->dma_range),
> > > + GFP_ATOMIC);
> >
> > With this patch, it appears that storvsc_queuecommand() is always
> > doing bounce buffering, even when running in a non-isolated VM.
> > The dma_range is always allocated, and the inner loop below does
> > the dma mapping for every I/O page. The corresponding code in
> > storvsc_on_channel_callback() that does the dma unmap allows for
> > the dma_range to be NULL, but that never happens.
>
> Maybe I'm missing something in the hyperv code, but I don't think
> dma_map_page would bounce buffer for the non-isolated case. It
> will just return the physical address.
OK, right. In the isolated VM case, the swiotlb is in force mode
and will do bounce buffering. In the non-isolated case,
dma_map_page_attrs() -> dma_direct_map_page() does a lot of
checking but eventually just returns the physical address. As this
patch is currently coded, it adds a fair amount of overhead
here in storvsc_queuecommand(), plus the overhead of the dma
mapping function deciding to use the identity mapping. But if
dma_map_sg() is used and the code is simplified a bit, the overhead
will be less in general and will be per sgl entry instead of per page.
>
> > > + if (offset_in_hvpg) {
> > > + payload->range.offset = dma &
> > > ~HV_HYP_PAGE_MASK;
> > > + offset_in_hvpg = 0;
> > > + }
> >
> > I'm not clear on why payload->range.offset needs to be set again.
> > Even after the dma mapping is done, doesn't the offset in the first
> > page have to be the same? If it wasn't the same, Hyper-V wouldn't
> > be able to process the PFN list correctly. In fact, couldn't the above
> > code just always set offset_in_hvpg = 0?
>
> Careful. DMA mapping is supposed to keep the offset in the page, but
> for that the DMA mapping code needs to know what the device considers a
> "page". For that the driver needs to set the min_align_mask field in
> struct device_dma_parameters.
>
I see that the swiotlb code gets and uses the min_align_mask field. But
the NVME driver is the only driver that ever sets it, so the value is zero
in all other cases. Does swiotlb just use PAGE_SIZE in that that case? I
couldn't tell from a quick glance at the swiotlb code.
> >
> > The whole approach here is to do dma remapping on each individual page
> > of the I/O buffer. But wouldn't it be possible to use dma_map_sg() to map
> > each scatterlist entry as a unit? Each scatterlist entry describes a range
> > of
> > physically contiguous memory. After dma_map_sg(), the resulting dma
> > address must also refer to a physically contiguous range in the swiotlb
> > bounce buffer memory. So at the top of the "for" loop over the scatterlist
> > entries, do dma_map_sg() if we're in an isolated VM. Then compute the
> > hvpfn value based on the dma address instead of sg_page(). But everything
> > else is the same, and the inner loop for populating the pfn_arry is
> > unmodified.
> > Furthermore, the dma_range array that you've added is not needed, since
> > scatterlist entries already have a dma_address field for saving the mapped
> > address, and dma_unmap_sg() uses that field.
>
> Yes, I think dma_map_sg is the right thing to use here, probably even
> for the non-isolated case so that we can get the hv drivers out of their
> little corner and into being more like a normal kernel driver. That
> is, use the scsi_dma_map/scsi_dma_unmap helpers, and then iterate over
> the dma addresses one page at a time using for_each_sg_dma_page.
>
Doing some broader revisions to the Hyper-V storvsc driver is up next on
my to-do list. Rather than significantly modifying the non-isolated case in
this patch set, I'd suggest factoring it into my broader revisions.
Michael
|