| Message ID | 1412244158-12124-2-git-send-email-stefano.stabellini@eu.citrix.com |
|---|---|
| State | New |
| Headers | show |
Hi, At 11:02 +0100 on 02 Oct (1412244156), Stefano Stabellini wrote: > Check whether an mfn has been granted to a given domain on a target > grant table. The implementation looks good but I'm concerned about the need for it, since linear scans of busy grant tables could get fairly expensive. I see in patch 3/4 you add a hypercall to flush caches, taking an mfn range. Should that not take a gfn range? That would be the idiomatic interface, and the p2m lookup would tell you whether the guest had appropiate rights. I suspect, from reading 0/4, that I'm missing something about the tangle of non-IOMMU dom0 operations on ARM. :) Cheers, Tim
On Thu, 2 Oct 2014, Tim Deegan wrote: > Hi, > > At 11:02 +0100 on 02 Oct (1412244156), Stefano Stabellini wrote: > > Check whether an mfn has been granted to a given domain on a target > > grant table. > > The implementation looks good but I'm concerned about the need for it, > since linear scans of busy grant tables could get fairly expensive. > > I see in patch 3/4 you add a hypercall to flush caches, taking an mfn > range. Should that not take a gfn range? That would be the idiomatic > interface, and the p2m lookup would tell you whether the guest had > appropiate rights. > > I suspect, from reading 0/4, that I'm missing something about the > tangle of non-IOMMU dom0 operations on ARM. :) The hypercall is going to be used to flush foreign pages grant mapped in Dom0. Unfortunately dom0 doesn't know the gfn, only the dma address (==mfn).
>>> On 02.10.14 at 12:42, <stefano.stabellini@eu.citrix.com> wrote: > On Thu, 2 Oct 2014, Tim Deegan wrote: >> At 11:02 +0100 on 02 Oct (1412244156), Stefano Stabellini wrote: >> > Check whether an mfn has been granted to a given domain on a target >> > grant table. >> >> The implementation looks good but I'm concerned about the need for it, >> since linear scans of busy grant tables could get fairly expensive. >> >> I see in patch 3/4 you add a hypercall to flush caches, taking an mfn >> range. Should that not take a gfn range? That would be the idiomatic >> interface, and the p2m lookup would tell you whether the guest had >> appropiate rights. >> >> I suspect, from reading 0/4, that I'm missing something about the >> tangle of non-IOMMU dom0 operations on ARM. :) > > The hypercall is going to be used to flush foreign pages grant mapped in > Dom0. Unfortunately dom0 doesn't know the gfn, only the dma address > (==mfn). Don't grant maps get entered into the P2M as they get established? Jan
On Thu, 2 Oct 2014, Jan Beulich wrote: > >>> On 02.10.14 at 12:42, <stefano.stabellini@eu.citrix.com> wrote: > > On Thu, 2 Oct 2014, Tim Deegan wrote: > >> At 11:02 +0100 on 02 Oct (1412244156), Stefano Stabellini wrote: > >> > Check whether an mfn has been granted to a given domain on a target > >> > grant table. > >> > >> The implementation looks good but I'm concerned about the need for it, > >> since linear scans of busy grant tables could get fairly expensive. > >> > >> I see in patch 3/4 you add a hypercall to flush caches, taking an mfn > >> range. Should that not take a gfn range? That would be the idiomatic > >> interface, and the p2m lookup would tell you whether the guest had > >> appropiate rights. > >> > >> I suspect, from reading 0/4, that I'm missing something about the > >> tangle of non-IOMMU dom0 operations on ARM. :) > > > > The hypercall is going to be used to flush foreign pages grant mapped in > > Dom0. Unfortunately dom0 doesn't know the gfn, only the dma address > > (==mfn). > > Don't grant maps get entered into the P2M as they get established? Yes, they do. Still dom0 only has an mfn in its hands. Tracking the mfn to pfn relationship in Linux has been tried unsuccessfully before. Not only it is slow but it is also difficult as the same page can be granted multiple times simultaneously.
At 11:42 +0100 on 02 Oct (1412246555), Stefano Stabellini wrote: > On Thu, 2 Oct 2014, Tim Deegan wrote: > > Hi, > > > > At 11:02 +0100 on 02 Oct (1412244156), Stefano Stabellini wrote: > > > Check whether an mfn has been granted to a given domain on a target > > > grant table. > > > > The implementation looks good but I'm concerned about the need for it, > > since linear scans of busy grant tables could get fairly expensive. > > > > I see in patch 3/4 you add a hypercall to flush caches, taking an mfn > > range. Should that not take a gfn range? That would be the idiomatic > > interface, and the p2m lookup would tell you whether the guest had > > appropiate rights. > > > > I suspect, from reading 0/4, that I'm missing something about the > > tangle of non-IOMMU dom0 operations on ARM. :) > > The hypercall is going to be used to flush foreign pages grant mapped in > Dom0. Unfortunately dom0 doesn't know the gfn, only the dma address > (==mfn). Blargh. Silly linux, forgetting its grant handles. :) So after some IRL discussion with Stefano and IanC, I'm convinced that the alternatives (linux maintaining per-map lookup tables to get back from DMA address to grant handle) aren't going to work. But we can make this API a bit neater. I think the design we came up with is: - the cache-flush hypercall becomes a grant-table op instead of a memop. - the argument becomes a 'dev_bus_addr' instead of an MFN. That is, it must be the address returned in the dev_bus_addr field of a grant map operation. If not, it will return EINVAL. - the interals are shuffled so that the grant code calls out to arch-specific code to do the cache flush, which solves the locking issue. Unlocking before the flush is probably the right thing to do anyway, but we can avoid exposing this new grant_map_exists() function that might encourage people to write racy code. Cheers, Tim.
>>> On 02.10.14 at 13:37, <stefano.stabellini@eu.citrix.com> wrote: > On Thu, 2 Oct 2014, Jan Beulich wrote: >> >>> On 02.10.14 at 12:42, <stefano.stabellini@eu.citrix.com> wrote: >> > On Thu, 2 Oct 2014, Tim Deegan wrote: >> >> At 11:02 +0100 on 02 Oct (1412244156), Stefano Stabellini wrote: >> >> > Check whether an mfn has been granted to a given domain on a target >> >> > grant table. >> >> >> >> The implementation looks good but I'm concerned about the need for it, >> >> since linear scans of busy grant tables could get fairly expensive. >> >> >> >> I see in patch 3/4 you add a hypercall to flush caches, taking an mfn >> >> range. Should that not take a gfn range? That would be the idiomatic >> >> interface, and the p2m lookup would tell you whether the guest had >> >> appropiate rights. >> >> >> >> I suspect, from reading 0/4, that I'm missing something about the >> >> tangle of non-IOMMU dom0 operations on ARM. :) >> > >> > The hypercall is going to be used to flush foreign pages grant mapped in >> > Dom0. Unfortunately dom0 doesn't know the gfn, only the dma address >> > (==mfn). >> >> Don't grant maps get entered into the P2M as they get established? > > Yes, they do. Still dom0 only has an mfn in its hands. Tracking the mfn > to pfn relationship in Linux has been tried unsuccessfully before. Not > only it is slow but it is also difficult as the same page can be granted > multiple times simultaneously. It can't be that difficult for Dom0 to track at which PFN is maps a particular grant. And as long as Dom0 serializes properly wrt unmaps, which GFN of the perhaps multiple ones it picks for doing the cache flush doesn't even matter. Jan
>>> On 02.10.14 at 13:41, <tim@xen.org> wrote: > At 11:42 +0100 on 02 Oct (1412246555), Stefano Stabellini wrote: >> On Thu, 2 Oct 2014, Tim Deegan wrote: >> > Hi, >> > >> > At 11:02 +0100 on 02 Oct (1412244156), Stefano Stabellini wrote: >> > > Check whether an mfn has been granted to a given domain on a target >> > > grant table. >> > >> > The implementation looks good but I'm concerned about the need for it, >> > since linear scans of busy grant tables could get fairly expensive. >> > >> > I see in patch 3/4 you add a hypercall to flush caches, taking an mfn >> > range. Should that not take a gfn range? That would be the idiomatic >> > interface, and the p2m lookup would tell you whether the guest had >> > appropiate rights. >> > >> > I suspect, from reading 0/4, that I'm missing something about the >> > tangle of non-IOMMU dom0 operations on ARM. :) >> >> The hypercall is going to be used to flush foreign pages grant mapped in >> Dom0. Unfortunately dom0 doesn't know the gfn, only the dma address >> (==mfn). > > Blargh. Silly linux, forgetting its grant handles. :) > > So after some IRL discussion with Stefano and IanC, I'm convinced that > the alternatives (linux maintaining per-map lookup tables to get back > from DMA address to grant handle) aren't going to work. But we can > make this API a bit neater. I think the design we came up with is: > > - the cache-flush hypercall becomes a grant-table op instead of a > memop. > - the argument becomes a 'dev_bus_addr' instead of an MFN. That is, > it must be the address returned in the dev_bus_addr field of a > grant map operation. If not, it will return EINVAL. Then at the very least let's allow for either a dev_bus_addr or a grant-ref (even if for the moment the latter may yield -EOPNOTSUPP in order to not spend more time on this than immediately necessary). That way guests remembering what they did a few microseconds back can do this in a more well behaved fashion. Jan > - the interals are shuffled so that the grant code calls out to > arch-specific code to do the cache flush, which solves the locking > issue. Unlocking before the flush is probably the right thing to > do anyway, but we can avoid exposing this new grant_map_exists() > function that might encourage people to write racy code. > > Cheers, > > Tim.
At 12:59 +0100 on 02 Oct (1412251189), Jan Beulich wrote: > >>> On 02.10.14 at 13:41, <tim@xen.org> wrote: > > Blargh. Silly linux, forgetting its grant handles. :) > > > > So after some IRL discussion with Stefano and IanC, I'm convinced that > > the alternatives (linux maintaining per-map lookup tables to get back > > from DMA address to grant handle) aren't going to work. But we can > > make this API a bit neater. I think the design we came up with is: > > > > - the cache-flush hypercall becomes a grant-table op instead of a > > memop. > > - the argument becomes a 'dev_bus_addr' instead of an MFN. That is, > > it must be the address returned in the dev_bus_addr field of a > > grant map operation. If not, it will return EINVAL. > > Then at the very least let's allow for either a dev_bus_addr or > a grant-ref (even if for the moment the latter may yield > -EOPNOTSUPP in order to not spend more time on this than > immediately necessary). That way guests remembering what > they did a few microseconds back can do this in a more well > behaved fashion. Good idea. Another thing that occurs to me: this should handle transitive grants as well if possible. Cheers, Tim.
On Thu, 2 Oct 2014, Jan Beulich wrote: > >>> On 02.10.14 at 13:41, <tim@xen.org> wrote: > > At 11:42 +0100 on 02 Oct (1412246555), Stefano Stabellini wrote: > >> On Thu, 2 Oct 2014, Tim Deegan wrote: > >> > Hi, > >> > > >> > At 11:02 +0100 on 02 Oct (1412244156), Stefano Stabellini wrote: > >> > > Check whether an mfn has been granted to a given domain on a target > >> > > grant table. > >> > > >> > The implementation looks good but I'm concerned about the need for it, > >> > since linear scans of busy grant tables could get fairly expensive. > >> > > >> > I see in patch 3/4 you add a hypercall to flush caches, taking an mfn > >> > range. Should that not take a gfn range? That would be the idiomatic > >> > interface, and the p2m lookup would tell you whether the guest had > >> > appropiate rights. > >> > > >> > I suspect, from reading 0/4, that I'm missing something about the > >> > tangle of non-IOMMU dom0 operations on ARM. :) > >> > >> The hypercall is going to be used to flush foreign pages grant mapped in > >> Dom0. Unfortunately dom0 doesn't know the gfn, only the dma address > >> (==mfn). > > > > Blargh. Silly linux, forgetting its grant handles. :) > > > > So after some IRL discussion with Stefano and IanC, I'm convinced that > > the alternatives (linux maintaining per-map lookup tables to get back > > from DMA address to grant handle) aren't going to work. But we can > > make this API a bit neater. I think the design we came up with is: > > > > - the cache-flush hypercall becomes a grant-table op instead of a > > memop. > > - the argument becomes a 'dev_bus_addr' instead of an MFN. That is, > > it must be the address returned in the dev_bus_addr field of a > > grant map operation. If not, it will return EINVAL. > > Then at the very least let's allow for either a dev_bus_addr or > a grant-ref (even if for the moment the latter may yield > -EOPNOTSUPP in order to not spend more time on this than > immediately necessary). That way guests remembering what > they did a few microseconds back can do this in a more well > behaved fashion. I can add the gref as a possible parameter in a union, but I would like to keep the interface multi-page capable. So the gref case is going to work implicitly only on a single page.
On Fri, 3 Oct 2014, Stefano Stabellini wrote: > On Thu, 2 Oct 2014, Jan Beulich wrote: > > >>> On 02.10.14 at 13:41, <tim@xen.org> wrote: > > > At 11:42 +0100 on 02 Oct (1412246555), Stefano Stabellini wrote: > > >> On Thu, 2 Oct 2014, Tim Deegan wrote: > > >> > Hi, > > >> > > > >> > At 11:02 +0100 on 02 Oct (1412244156), Stefano Stabellini wrote: > > >> > > Check whether an mfn has been granted to a given domain on a target > > >> > > grant table. > > >> > > > >> > The implementation looks good but I'm concerned about the need for it, > > >> > since linear scans of busy grant tables could get fairly expensive. > > >> > > > >> > I see in patch 3/4 you add a hypercall to flush caches, taking an mfn > > >> > range. Should that not take a gfn range? That would be the idiomatic > > >> > interface, and the p2m lookup would tell you whether the guest had > > >> > appropiate rights. > > >> > > > >> > I suspect, from reading 0/4, that I'm missing something about the > > >> > tangle of non-IOMMU dom0 operations on ARM. :) > > >> > > >> The hypercall is going to be used to flush foreign pages grant mapped in > > >> Dom0. Unfortunately dom0 doesn't know the gfn, only the dma address > > >> (==mfn). > > > > > > Blargh. Silly linux, forgetting its grant handles. :) > > > > > > So after some IRL discussion with Stefano and IanC, I'm convinced that > > > the alternatives (linux maintaining per-map lookup tables to get back > > > from DMA address to grant handle) aren't going to work. But we can > > > make this API a bit neater. I think the design we came up with is: > > > > > > - the cache-flush hypercall becomes a grant-table op instead of a > > > memop. > > > - the argument becomes a 'dev_bus_addr' instead of an MFN. That is, > > > it must be the address returned in the dev_bus_addr field of a > > > grant map operation. If not, it will return EINVAL. > > > > Then at the very least let's allow for either a dev_bus_addr or > > a grant-ref (even if for the moment the latter may yield > > -EOPNOTSUPP in order to not spend more time on this than > > immediately necessary). That way guests remembering what > > they did a few microseconds back can do this in a more well > > behaved fashion. > > I can add the gref as a possible parameter in a union, but I would like > to keep the interface multi-page capable. So the gref case is going to > work implicitly only on a single page. I take it back, the hypercall working on a single grant is OK as otherwise pages wouldn't be contiguous in mfn space and couldn't be used in a single dma_op.
>>> Stefano Stabellini <stefano.stabellini@eu.citrix.com> 10/03/14 3:50 PM >>> >On Thu, 2 Oct 2014, Jan Beulich wrote: >> Then at the very least let's allow for either a dev_bus_addr or >> a grant-ref (even if for the moment the latter may yield >> -EOPNOTSUPP in order to not spend more time on this than >> immediately necessary). That way guests remembering what >> they did a few microseconds back can do this in a more well >> behaved fashion. > >I can add the gref as a possible parameter in a union, but I would like >to keep the interface multi-page capable. So the gref case is going to >work implicitly only on a single page. Imo that's not a reasonable design choice when this is to become a gnttab op - this should instead use the usual built-in batching most gnttab ops have to deal with multi-page extents. Jan
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c index 23266c3..e1dc330 100644 --- a/xen/common/grant_table.c +++ b/xen/common/grant_table.c @@ -484,6 +484,38 @@ static int _set_status(unsigned gt_version, return _set_status_v2(domid, readonly, mapflag, shah, act, status); } +bool_t grant_map_exists(struct domain *ld, + struct grant_table *rgt, + unsigned long mfn) +{ + struct active_grant_entry *act; + grant_ref_t ref; + bool_t ret = 0; + + spin_lock(&rgt->lock); + + for ( ref = 0; ref != nr_grant_entries(rgt); ref++ ) + { + act = &active_entry(rgt, ref); + + if ( !act->pin ) + continue; + + if ( act->domid != ld->domain_id ) + continue; + + if ( act->frame != mfn ) + continue; + + ret = 1; + break; + } + + spin_unlock(&rgt->lock); + + return ret; +} + static void mapcount( struct grant_table *lgt, struct domain *rd, unsigned long mfn, unsigned int *wrc, unsigned int *rdc) diff --git a/xen/include/xen/grant_table.h b/xen/include/xen/grant_table.h index 5941191..2df390f 100644 --- a/xen/include/xen/grant_table.h +++ b/xen/include/xen/grant_table.h @@ -97,6 +97,10 @@ int grant_table_create( void grant_table_destroy( struct domain *d); +bool_t grant_map_exists(struct domain *ld, + struct grant_table *rgt, + unsigned long mfn); + /* Domain death release of granted mappings of other domains' memory. */ void gnttab_release_mappings(
Check whether an mfn has been granted to a given domain on a target grant table. Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> CC: jbeulich@suse.com CC: tim@xen.org --- xen/common/grant_table.c | 32 ++++++++++++++++++++++++++++++++ xen/include/xen/grant_table.h | 4 ++++ 2 files changed, 36 insertions(+)